[PATCH v2 0/3] regulator: Add device tree support to AD5398

[PATCH v2 0/3] regulator: Add device tree support to AD5398

[Isaac Scott]

The AD5398 is a DAC that can be used to control current flow in circuits
in a wide variety of applications such as motor control, or in my case,
LED control. I found when working with the current driver that it did
not work for my use case. It transpired that it only had support for
platform_data, and didn't appear to be correctly implemented according
to the datasheet, which can be found here:

https://www.analog.com/media/en/technical-documentation/data-sheets/ad5398.pdf

One example of this is the "soft power-down" bit being referred to in
the driver as simply "enable", which gives the impression that the
setting that bit will allow current through the regulator, which it does
not. 

This series allows the regulator to be given its constraints via the
device tree, and makes the function of the enable register much more
obvious.

Best wishes,

Isaac

Tested on v6.13 (origin/master)

Isaac Scott (3):
  regulator: ad5398: change enable bit name to improve readibility
  RFC: regulator: ad5398: Change selector division calculation
  regulator: ad5398: Add device tree support

 drivers/regulator/ad5398.c | 33 +++++++++++++++++++--------------
 1 file changed, 19 insertions(+), 14 deletions(-)

-- 
2.43.0

[PATCH v2 1/3] regulator: ad5398: change enable bit name to improve readibility

[Isaac Scott]

The mask name AD5398_CURRENT_EN_MASK is misleading, as it implies that
setting bit 16 of the AD5398 enables current flow. In fact, setting this
bit prevents current flow, due to this bit being a software power down
control. This bit is referred to as "soft power down" in the datasheet.
As such, change the name of the bit and modify its use in the driver to
make the regulator more intuitively usable.

(When calling ad5398_enable, current will start flowing, and vice
versa).

Signed-off-by: Isaac Scott <isaac.scott@ideasonboard.com>
---
 drivers/regulator/ad5398.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/regulator/ad5398.c b/drivers/regulator/ad5398.c
index 40f7dba42b5a..e6f45c6e750c 100644
--- a/drivers/regulator/ad5398.c
+++ b/drivers/regulator/ad5398.c
@@ -15,7 +15,7 @@
 #include <linux/regulator/driver.h>
 #include <linux/regulator/machine.h>
 
-#define AD5398_CURRENT_EN_MASK	0x8000
+#define AD5398_SW_POWER_DOWN	BIT(16)
 
 struct ad5398_chip_info {
 	struct i2c_client *client;
@@ -113,7 +113,7 @@ static int ad5398_set_current_limit(struct regulator_dev *rdev, int min_uA, int
 
 	/* prepare register data */
 	selector = (selector << chip->current_offset) & chip->current_mask;
-	data = (unsigned short)selector | (data & AD5398_CURRENT_EN_MASK);
+	data = (unsigned short)selector | (data & AD5398_SW_POWER_DOWN);
 
 	/* write the new current value back as well as enable bit */
 	ret = ad5398_write_reg(client, data);
@@ -132,10 +132,10 @@ static int ad5398_is_enabled(struct regulator_dev *rdev)
 	if (ret < 0)
 		return ret;
 
-	if (data & AD5398_CURRENT_EN_MASK)
-		return 1;
-	else
+	if (data & AD5398_SW_POWER_DOWN)
 		return 0;
+	else
+		return 1;
 }
 
 static int ad5398_enable(struct regulator_dev *rdev)
@@ -149,10 +149,10 @@ static int ad5398_enable(struct regulator_dev *rdev)
 	if (ret < 0)
 		return ret;
 
-	if (data & AD5398_CURRENT_EN_MASK)
+	if (!(data & AD5398_SW_POWER_DOWN))
 		return 0;
 
-	data |= AD5398_CURRENT_EN_MASK;
+	data &= ~AD5398_SW_POWER_DOWN;
 
 	ret = ad5398_write_reg(client, data);
 
@@ -170,10 +170,10 @@ static int ad5398_disable(struct regulator_dev *rdev)
 	if (ret < 0)
 		return ret;
 
-	if (!(data & AD5398_CURRENT_EN_MASK))
+	if (data & AD5398_SW_POWER_DOWN)
 		return 0;
 
-	data &= ~AD5398_CURRENT_EN_MASK;
+	data |= AD5398_SW_POWER_DOWN;
 
 	ret = ad5398_write_reg(client, data);
 
-- 
2.43.0

[PATCH v2 2/3] RFC: regulator: ad5398: Change selector division calculation

[Isaac Scott]

If the AD5398 is defined to have a current limit with no range, i.e.
when max_Ua and min_Ua are equal, the DIV_ROUND_UP erroneously tries to
set the current to a higher level than the max_Ua, which causes the
driver to fail to set the current. Fix this so the driver slightly
underestimates the current to set.

Signed-off-by: Isaac Scott <isaac.scott@ideasonboard.com>
---
 drivers/regulator/ad5398.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/regulator/ad5398.c b/drivers/regulator/ad5398.c
index e6f45c6e750c..0c60ecd1f0f2 100644
--- a/drivers/regulator/ad5398.c
+++ b/drivers/regulator/ad5398.c
@@ -98,8 +98,7 @@ static int ad5398_set_current_limit(struct regulator_dev *rdev, int min_uA, int
 	if (min_uA > chip->max_uA || max_uA < chip->min_uA)
 		return -EINVAL;
 
-	selector = DIV_ROUND_UP((min_uA - chip->min_uA) * chip->current_level,
-				range_uA);
+	selector = ((min_uA - chip->min_uA) * chip->current_level / range_uA);
 	if (ad5398_calc_current(chip, selector) > max_uA)
 		return -EINVAL;
 
-- 
2.43.0

[PATCH v2 3/3] regulator: ad5398: Add device tree support

[Isaac Scott]

Previously, the ad5398 driver used only platform_data, which is
deprecated in favour of device tree. This caused the AD5398 to fail to
probe as it could not load its init_data. If the AD5398 has a device
tree node, pull the init_data from there using
of_get_regulator_init_data.

Signed-off-by: Isaac Scott <isaac.scott@ideasonboard.com>
---
 drivers/regulator/ad5398.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/regulator/ad5398.c b/drivers/regulator/ad5398.c
index 0c60ecd1f0f2..619c999946f3 100644
--- a/drivers/regulator/ad5398.c
+++ b/drivers/regulator/ad5398.c
@@ -14,6 +14,7 @@
 #include <linux/platform_device.h>
 #include <linux/regulator/driver.h>
 #include <linux/regulator/machine.h>
+#include <linux/regulator/of_regulator.h>
 
 #define AD5398_SW_POWER_DOWN	BIT(16)
 
@@ -220,15 +221,20 @@ static int ad5398_probe(struct i2c_client *client)
 	const struct ad5398_current_data_format *df =
 			(struct ad5398_current_data_format *)id->driver_data;
 
-	if (!init_data)
-		return -EINVAL;
-
 	chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
 	if (!chip)
 		return -ENOMEM;
 
 	config.dev = &client->dev;
+	if (client->dev.of_node)
+		init_data = of_get_regulator_init_data(&client->dev,
+						       client->dev.of_node,
+						       &ad5398_reg);
+	if (!init_data)
+		return -EINVAL;
+
 	config.init_data = init_data;
+	config.of_node = client->dev.of_node;
 	config.driver_data = chip;
 
 	chip->client = client;
-- 
2.43.0

RE: [PATCH v2 1/3] regulator: ad5398: change enable bit name to improve readibility

[Hennerich, Michael]

> -----Original Message-----
> From: Isaac Scott <isaac.scott@ideasonboard.com>
> Sent: Tuesday, January 28, 2025 6:32 PM
> To: Hennerich, Michael <Michael.Hennerich@analog.com>
> Cc: lgirdwood@gmail.com; broonie@kernel.org; linux-kernel@vger.kernel.org;
> Isaac Scott <isaac.scott@ideasonboard.com>
> Subject: [PATCH v2 1/3] regulator: ad5398: change enable bit name to
> improve readibility
> 
> 
> The mask name AD5398_CURRENT_EN_MASK is misleading, as it implies that
> setting bit 16 of the AD5398 enables current flow. In fact, setting this bit
> prevents current flow, due to this bit being a software power down control.
> This bit is referred to as "soft power down" in the datasheet.
> As such, change the name of the bit and modify its use in the driver to make
> the regulator more intuitively usable.
> 
> (When calling ad5398_enable, current will start flowing, and vice versa).
> 
> Signed-off-by: Isaac Scott <isaac.scott@ideasonboard.com>

Acked-by: Michael Hennerich <michael.hennerich@analog.com>

> ---
>  drivers/regulator/ad5398.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/regulator/ad5398.c b/drivers/regulator/ad5398.c index
> 40f7dba42b5a..e6f45c6e750c 100644
> --- a/drivers/regulator/ad5398.c
> +++ b/drivers/regulator/ad5398.c
> @@ -15,7 +15,7 @@
>  #include <linux/regulator/driver.h>
>  #include <linux/regulator/machine.h>
> 
> -#define AD5398_CURRENT_EN_MASK	0x8000
> +#define AD5398_SW_POWER_DOWN	BIT(16)
> 
>  struct ad5398_chip_info {
>  	struct i2c_client *client;
> @@ -113,7 +113,7 @@ static int ad5398_set_current_limit(struct
> regulator_dev *rdev, int min_uA, int
> 
>  	/* prepare register data */
>  	selector = (selector << chip->current_offset) & chip->current_mask;
> -	data = (unsigned short)selector | (data &
> AD5398_CURRENT_EN_MASK);
> +	data = (unsigned short)selector | (data &
> AD5398_SW_POWER_DOWN);
> 
>  	/* write the new current value back as well as enable bit */
>  	ret = ad5398_write_reg(client, data);
> @@ -132,10 +132,10 @@ static int ad5398_is_enabled(struct regulator_dev
> *rdev)
>  	if (ret < 0)
>  		return ret;
> 
> -	if (data & AD5398_CURRENT_EN_MASK)
> -		return 1;
> -	else
> +	if (data & AD5398_SW_POWER_DOWN)
>  		return 0;
> +	else
> +		return 1;
>  }
> 
>  static int ad5398_enable(struct regulator_dev *rdev) @@ -149,10 +149,10
> @@ static int ad5398_enable(struct regulator_dev *rdev)
>  	if (ret < 0)
>  		return ret;
> 
> -	if (data & AD5398_CURRENT_EN_MASK)
> +	if (!(data & AD5398_SW_POWER_DOWN))
>  		return 0;
> 
> -	data |= AD5398_CURRENT_EN_MASK;
> +	data &= ~AD5398_SW_POWER_DOWN;
> 
>  	ret = ad5398_write_reg(client, data);
> 
> @@ -170,10 +170,10 @@ static int ad5398_disable(struct regulator_dev
> *rdev)
>  	if (ret < 0)
>  		return ret;
> 
> -	if (!(data & AD5398_CURRENT_EN_MASK))
> +	if (data & AD5398_SW_POWER_DOWN)
>  		return 0;
> 
> -	data &= ~AD5398_CURRENT_EN_MASK;
> +	data |= AD5398_SW_POWER_DOWN;
> 
>  	ret = ad5398_write_reg(client, data);
> 
> --
> 2.43.0

RE: [PATCH v2 3/3] regulator: ad5398: Add device tree support

[Hennerich, Michael]

> -----Original Message-----
> From: Isaac Scott <isaac.scott@ideasonboard.com>
> Sent: Tuesday, January 28, 2025 6:32 PM
> To: Hennerich, Michael <Michael.Hennerich@analog.com>
> Cc: lgirdwood@gmail.com; broonie@kernel.org; linux-kernel@vger.kernel.org;
> Isaac Scott <isaac.scott@ideasonboard.com>
> Subject: [PATCH v2 3/3] regulator: ad5398: Add device tree support
> 
> 
> Previously, the ad5398 driver used only platform_data, which is deprecated in
> favour of device tree. This caused the AD5398 to fail to probe as it could not
> load its init_data. If the AD5398 has a device tree node, pull the init_data from
> there using of_get_regulator_init_data.
> 
> Signed-off-by: Isaac Scott <isaac.scott@ideasonboard.com>

Acked-by: Michael Hennerich <michael.hennerich@analog.com>

> ---
>  drivers/regulator/ad5398.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/regulator/ad5398.c b/drivers/regulator/ad5398.c index
> 0c60ecd1f0f2..619c999946f3 100644
> --- a/drivers/regulator/ad5398.c
> +++ b/drivers/regulator/ad5398.c
> @@ -14,6 +14,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/regulator/driver.h>
>  #include <linux/regulator/machine.h>
> +#include <linux/regulator/of_regulator.h>
> 
>  #define AD5398_SW_POWER_DOWN	BIT(16)
> 
> @@ -220,15 +221,20 @@ static int ad5398_probe(struct i2c_client *client)
>  	const struct ad5398_current_data_format *df =
>  			(struct ad5398_current_data_format *)id-
> >driver_data;
> 
> -	if (!init_data)
> -		return -EINVAL;
> -
>  	chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
>  	if (!chip)
>  		return -ENOMEM;
> 
>  	config.dev = &client->dev;
> +	if (client->dev.of_node)
> +		init_data = of_get_regulator_init_data(&client->dev,
> +						       client->dev.of_node,
> +						       &ad5398_reg);
> +	if (!init_data)
> +		return -EINVAL;
> +
>  	config.init_data = init_data;
> +	config.of_node = client->dev.of_node;
>  	config.driver_data = chip;
> 
>  	chip->client = client;
> --
> 2.43.0

RE: [PATCH v2 2/3] RFC: regulator: ad5398: Change selector division calculation

[Hennerich, Michael]

> -----Original Message-----
> From: Isaac Scott <isaac.scott@ideasonboard.com>
> Sent: Tuesday, January 28, 2025 6:32 PM
> To: Hennerich, Michael <Michael.Hennerich@analog.com>
> Cc: lgirdwood@gmail.com; broonie@kernel.org; linux-kernel@vger.kernel.org;
> Isaac Scott <isaac.scott@ideasonboard.com>
> Subject: [PATCH v2 2/3] RFC: regulator: ad5398: Change selector division
> calculation
> 
> [External]
> 
> If the AD5398 is defined to have a current limit with no range, i.e.
> when max_Ua and min_Ua are equal, the DIV_ROUND_UP erroneously tries
> to set the current to a higher level than the max_Ua, which causes the driver to
> fail to set the current. Fix this so the driver slightly underestimates the current
> to set.
> 
> Signed-off-by: Isaac Scott <isaac.scott@ideasonboard.com>
> ---
>  drivers/regulator/ad5398.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/regulator/ad5398.c b/drivers/regulator/ad5398.c index
> e6f45c6e750c..0c60ecd1f0f2 100644
> --- a/drivers/regulator/ad5398.c
> +++ b/drivers/regulator/ad5398.c
> @@ -98,8 +98,7 @@ static int ad5398_set_current_limit(struct regulator_dev
> *rdev, int min_uA, int
>  	if (min_uA > chip->max_uA || max_uA < chip->min_uA)
>  		return -EINVAL;
> 
> -	selector = DIV_ROUND_UP((min_uA - chip->min_uA) * chip-
> >current_level,
> -				range_uA);
> +	selector = ((min_uA - chip->min_uA) * chip->current_level /
> range_uA);

Not sure if this is a good idea. The rational was to set the limit slightly higher.
This will do the opposite. The ranges are already checked.
Why not clamp() the calculated value? 

>  	if (ad5398_calc_current(chip, selector) > max_uA)
>  		return -EINVAL;
> 
> --
> 2.43.0

Re: [PATCH v2 2/3] RFC: regulator: ad5398: Change selector division calculation

[Isaac Scott]

Hi,

On Wed, 2025-01-29 at 08:34 +0000, Hennerich, Michael wrote:
> 
> 
> > -----Original Message-----
> > From: Isaac Scott <isaac.scott@ideasonboard.com>
> > Sent: Tuesday, January 28, 2025 6:32 PM
> > To: Hennerich, Michael <Michael.Hennerich@analog.com>
> > Cc: lgirdwood@gmail.com; broonie@kernel.org;
> > linux-kernel@vger.kernel.org;
> > Isaac Scott <isaac.scott@ideasonboard.com>
> > Subject: [PATCH v2 2/3] RFC: regulator: ad5398: Change selector
> > division
> > calculation
> > 
> > [External]
> > 
> > If the AD5398 is defined to have a current limit with no range,
> > i.e.
> > when max_Ua and min_Ua are equal, the DIV_ROUND_UP erroneously
> > tries
> > to set the current to a higher level than the max_Ua, which causes
> > the driver to
> > fail to set the current. Fix this so the driver slightly
> > underestimates the current
> > to set.
> > 
> > Signed-off-by: Isaac Scott <isaac.scott@ideasonboard.com>
> > ---
> >  drivers/regulator/ad5398.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/drivers/regulator/ad5398.c
> > b/drivers/regulator/ad5398.c index
> > e6f45c6e750c..0c60ecd1f0f2 100644
> > --- a/drivers/regulator/ad5398.c
> > +++ b/drivers/regulator/ad5398.c
> > @@ -98,8 +98,7 @@ static int ad5398_set_current_limit(struct
> > regulator_dev
> > *rdev, int min_uA, int
> >  	if (min_uA > chip->max_uA || max_uA < chip->min_uA)
> >  		return -EINVAL;
> > 
> > -	selector = DIV_ROUND_UP((min_uA - chip->min_uA) * chip-
> > > current_level,
> > -				range_uA);
> > +	selector = ((min_uA - chip->min_uA) * chip->current_level
> > /
> > range_uA);
> 
> Not sure if this is a good idea. The rational was to set the limit
> slightly higher.
> This will do the opposite. The ranges are already checked.
> Why not clamp() the calculated value? 

That sounds like a better idea. Thank you! Out of interest, why is it
beneficial for the limit to be higher? 

> 
> >  	if (ad5398_calc_current(chip, selector) > max_uA)
> >  		return -EINVAL;
> > 
> > --
> > 2.43.0
> 

Best wishes,

Isaac

Re: (subset) [PATCH v2 0/3] regulator: Add device tree support to AD5398

[Mark Brown]

On Tue, 28 Jan 2025 17:31:40 +0000, Isaac Scott wrote:
> The AD5398 is a DAC that can be used to control current flow in circuits
> in a wide variety of applications such as motor control, or in my case,
> LED control. I found when working with the current driver that it did
> not work for my use case. It transpired that it only had support for
> platform_data, and didn't appear to be correctly implemented according
> to the datasheet, which can be found here:
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next

Thanks!

[1/3] regulator: ad5398: change enable bit name to improve readibility
      commit: 19d022d67d7353f0e6e9ba255435d3de93862ac4
[3/3] regulator: ad5398: Add device tree support
      commit: 5a6a461079decea452fdcae955bccecf92e07e97

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

Re: [PATCH v2 1/3] regulator: ad5398: change enable bit name to improve readibility

[Guenter Roeck]

On Tue, Jan 28, 2025 at 05:31:41PM +0000, Isaac Scott wrote:
> The mask name AD5398_CURRENT_EN_MASK is misleading, as it implies that
> setting bit 16 of the AD5398 enables current flow. In fact, setting this
> bit prevents current flow, due to this bit being a software power down
> control. This bit is referred to as "soft power down" in the datasheet.
> As such, change the name of the bit and modify its use in the driver to
> make the regulator more intuitively usable.
> 
> (When calling ad5398_enable, current will start flowing, and vice
> versa).
> 
> Signed-off-by: Isaac Scott <isaac.scott@ideasonboard.com>
> ---
[ ... ]
>  
> -#define AD5398_CURRENT_EN_MASK	0x8000
> +#define AD5398_SW_POWER_DOWN	BIT(16)

0x8000 is BIT(15).

Guenter

Re: [PATCH v2 1/3] regulator: ad5398: change enable bit name to improve readibility

[Isaac Scott]

Hi Guenter,

On Wed, 2025-02-05 at 10:45 -0800, Guenter Roeck wrote:
> On Tue, Jan 28, 2025 at 05:31:41PM +0000, Isaac Scott wrote:
> > The mask name AD5398_CURRENT_EN_MASK is misleading, as it implies
> > that
> > setting bit 16 of the AD5398 enables current flow. In fact, setting
> > this
> > bit prevents current flow, due to this bit being a software power
> > down
> > control. This bit is referred to as "soft power down" in the
> > datasheet.
> > As such, change the name of the bit and modify its use in the
> > driver to
> > make the regulator more intuitively usable.
> > 
> > (When calling ad5398_enable, current will start flowing, and vice
> > versa).
> > 
> > Signed-off-by: Isaac Scott <isaac.scott@ideasonboard.com>
> > ---
> [ ... ]
> >  
> > -#define AD5398_CURRENT_EN_MASK	0x8000
> > +#define AD5398_SW_POWER_DOWN	BIT(16)
> 
> 0x8000 is BIT(15).

You're totally right! The reason this got through is because in my use
case, I am powering the regulator on and off in multiple ways. This
should definitely be BIT(15), so I will send a V3 shortly.

Thank you very much!

Best wishes,

Isaac

> 
> Guenter

Re: [PATCH v2 1/3] regulator: ad5398: change enable bit name to improve readibility

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 644 bytes --]

On Thu, Feb 06, 2025 at 09:59:13AM +0000, Isaac Scott wrote:
> On Wed, 2025-02-05 at 10:45 -0800, Guenter Roeck wrote:

> > 0x8000 is BIT(15).

> You're totally right! The reason this got through is because in my use
> case, I am powering the regulator on and off in multiple ways. This
> should definitely be BIT(15), so I will send a V3 shortly.

Please do not submit new versions of already applied patches, please
submit incremental updates to the existing code.  Modifying existing
commits creates problems for other users building on top of those
commits so it's best practice to only change pubished git commits if
absolutely essential.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 2/3] RFC: regulator: ad5398: Change selector division calculation

[Isaac Scott]

On Wed, 2025-01-29 at 08:34 +0000, Hennerich, Michael wrote:
> 
> 
> > -----Original Message-----
> > From: Isaac Scott <isaac.scott@ideasonboard.com>
> > Sent: Tuesday, January 28, 2025 6:32 PM
> > To: Hennerich, Michael <Michael.Hennerich@analog.com>
> > Cc: lgirdwood@gmail.com; broonie@kernel.org;
> > linux-kernel@vger.kernel.org;
> > Isaac Scott <isaac.scott@ideasonboard.com>
> > Subject: [PATCH v2 2/3] RFC: regulator: ad5398: Change selector
> > division
> > calculation
> > 
> > [External]
> > 
> > If the AD5398 is defined to have a current limit with no range,
> > i.e.
> > when max_Ua and min_Ua are equal, the DIV_ROUND_UP erroneously
> > tries
> > to set the current to a higher level than the max_Ua, which causes
> > the driver to
> > fail to set the current. Fix this so the driver slightly
> > underestimates the current
> > to set.
> > 
> > Signed-off-by: Isaac Scott <isaac.scott@ideasonboard.com>
> > ---
> >  drivers/regulator/ad5398.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/drivers/regulator/ad5398.c
> > b/drivers/regulator/ad5398.c index
> > e6f45c6e750c..0c60ecd1f0f2 100644
> > --- a/drivers/regulator/ad5398.c
> > +++ b/drivers/regulator/ad5398.c
> > @@ -98,8 +98,7 @@ static int ad5398_set_current_limit(struct
> > regulator_dev
> > *rdev, int min_uA, int
> >  	if (min_uA > chip->max_uA || max_uA < chip->min_uA)
> >  		return -EINVAL;
> > 
> > -	selector = DIV_ROUND_UP((min_uA - chip->min_uA) * chip-
> > > current_level,
> > -				range_uA);
> > +	selector = ((min_uA - chip->min_uA) * chip->current_level
> > /
> > range_uA);
> 
> Not sure if this is a good idea. The rational was to set the limit
> slightly higher.
> This will do the opposite. The ranges are already checked.
> Why not clamp() the calculated value? 
> 

The documentation for set_current_limit says "the driver should select
the current closest to max_uA". In this case, does DIV_ROUND_UP always
choose the value closest to the limit?

In the use case where you want to set the current to exactly the
maximum, it does not make sense to overestimate the current (for
example, when the regulator is powering an LED, and if the LED gets too
hot from being over current, it could burn someone).

In the case where the user is setting a current that is within but is
not close the configured max current, it makes sense to estimate
slightly over. In the case where you configure the maximum, I think it
should calculate the max current that can be set so that the final
current does not exceed what is defined as the maximum in the device
tree.

What are your thoughts?

> >  	if (ad5398_calc_current(chip, selector) > max_uA)
> >  		return -EINVAL;
> > 
> > --
> > 2.43.0
> 

Best wishes,
Isaac