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@xxxxxxxxxxxxxxxx>
> > Sent: Tuesday, January 28, 2025 6:32 PM
> > To: Hennerich, Michael <Michael.Hennerich@xxxxxxxxxx>
> > Cc: lgirdwood@xxxxxxxxx; broonie@xxxxxxxxxx;
> > linux-kernel@xxxxxxxxxxxxxxx;
> > Isaac Scott <isaac.scott@xxxxxxxxxxxxxxxx>
> > 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@xxxxxxxxxxxxxxxx>
> > ---
> >  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