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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <dd50b01ea752ef0a6c028cd7eebf927c5b2f877b.camel@ideasonboard.com>
Date: Wed, 29 Jan 2025 18:21:18 +0000
From: Isaac Scott <isaac.scott@...asonboard.com>
To: "Hennerich, Michael" <Michael.Hennerich@...log.com>
Cc: "lgirdwood@...il.com" <lgirdwood@...il.com>, "broonie@...nel.org"
	 <broonie@...nel.org>, "linux-kernel@...r.kernel.org"
	 <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 2/3] RFC: regulator: ad5398: Change selector division
 calculation

Hi,

On Wed, 2025-01-29 at 08:34 +0000, Hennerich, Michael wrote:
> 
> 
> > -----Original Message-----
> > From: Isaac Scott <isaac.scott@...asonboard.com>
> > Sent: Tuesday, January 28, 2025 6:32 PM
> > To: Hennerich, Michael <Michael.Hennerich@...log.com>
> > Cc: lgirdwood@...il.com; broonie@...nel.org;
> > linux-kernel@...r.kernel.org;
> > Isaac Scott <isaac.scott@...asonboard.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@...asonboard.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