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

[Isaac Scott <isaac.scott@ideasonboard.com>]

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