linux-kernel - Re: [PATCH v11] iio: adc: pac1921: Add ACPI support to Microchip pac1921

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: <20241123162557.02247657@jic23-huawei>
Date: Sat, 23 Nov 2024 16:25:57 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Matteo Martelli <matteomartelli3@...il.com>
Cc: Victor.Duicu@...rochip.com, andy.shevchenko@...il.com, lars@...afoo.de,
 Marius.Cristea@...rochip.com, linux-kernel@...r.kernel.org,
 linux-iio@...r.kernel.org
Subject: Re: [PATCH v11] iio: adc: pac1921: Add ACPI support to Microchip
 pac1921

On Thu, 14 Nov 2024 14:43:11 +0100
Matteo Martelli <matteomartelli3@...il.com> wrote:

> On Thu, 14 Nov 2024 12:52:12 +0000, <Victor.Duicu@...rochip.com> wrote:
> > On Thu, 2024-11-14 at 12:00 +0100, Matteo Martelli wrote:  
> > > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > > know the content is safe
> > >   
> > 
> > Hi Matteo,
> >   
> > > On Thu, 14 Nov 2024 10:47:02 +0200, <victor.duicu@...rochip.com>
> > > wrote:  
> > > > From: Victor Duicu <victor.duicu@...rochip.com>
> > > > 
> > > > This patch implements ACPI support to Microchip pac1921.
> > > > The driver can read the shunt resistor value and label from the
> > > > ACPI table.
> > > > 
> > > > Reviewed-by: Andy Shevchenko <andy.shevchenko@...il.com>
> > > > Signed-off-by: Victor Duicu <victor.duicu@...rochip.com>
> > > > ---
> > > >   
> > 
> > ....
> >   
> > > > 
> > > > 
> > > > +#define PAC1921_ACPI_GET_uOHMS_VALS             0
> > > > +#define PAC1921_ACPI_GET_LABEL                       1
> > > > +/*
> > > > + * The maximum acceptable shunt value is 2146.999999 OHM.
> > > > + * This value, which is below INT_MAX, was chosen in order to
> > > > + * allow the readings from dt and ACPI to share the same range
> > > > + * and to simplify the checks.
> > > > + * With this value the maximum current that can be read is
> > > > + * 0.1V / 2146.999999OHM = 46.576 uA
> > > > + * If we use INT_MAX the maximum current that can be read is
> > > > + * 0.1V / 2147.483647OHM = 46.566 uA
> > > > + * The relative error between the two values is
> > > > + * |(46.566 - 46.576) / 46.566| * 100 = 0.0214
> > > > + */
> > > > +#define PAC1921_MAX_SHUNT_VALUE_uOHMS                2146999999UL
> > > > +  
> > > 
> > > Just a minor point about this: if I understand correctly that value
> > > comes from (INT_MAX / MICRO - 1) * MICRO + MAX_MICRO. This was to
> > > simplify the check in a single statement in
> > > pac1921_write_shunt_resistor()
> > > which is called when the shunt resistor is set from *sysfs* (neither
> > > from DT nor ACPI). I'm fine with this value and the new check but I
> > > find
> > > the explanation comment a bit confusing. If you could come up with a
> > > bit
> > > more clear explanation about the reason of such value I think it
> > > would be
> > > better otherwise I am fine with it as it is. Also, maybe use the full
> > > room
> > > for 80 characters per line and UOHMS instead of uOHMS to avoid mixed
> > > case if
> > > you are going with a new version.  
> > 
> > We could completely remove the need to use a constant below INT_MAX
> > with this check in pac1921_write_shunt_resistor:
> > 
> > if ((!val && !val_fract) || 
> > ((val >= INT_MAX / MICRO) && (val_fract > INT_MAX % MICRO)))
> > 	return -EINVAL;
> > 
> > Do you agree with this approach?  
> 
> Yes, something like this would be clearer to me.
> 
> Anyway, I think you also need to check for val > INT_MAX / MICRO when
> val_fract is < INT_MAX % MICRO, right?
> 
> Also, I think you can remove a couple of parenthesis.
> 
> So something like the following maybe (but please double check it):
> 
> 	if ((!val && !val_fract) || val > INT_MAX / MICRO ||
> 	   (val == INT_MAX / MICRO && val_fract > INT_MAX % MICRO))
> 
> I think that usually it would be better to use pre-computed constants
> instead of run-time divisions for efficiency but since the shunt
> resistor is likely going to be set rarely, I would go for this code for
> better clarity.
FWIW The compiler should be able to squash those into compile time constants anyway.
> 
> 
> > Also, the use of mixed case was suggested by Andy to increase
> > readability.  
> 
> Ah, sorry for missing Andy's comment. I am fine with it if you also find
> it more readable.
> 
> > 
> > ...
> >   
> > > 
> > > 
> > > Best regards,
> > > Matteo Martelli  
> > 
> > With Best Regards,
> > Duicu Victor  
> 
> Thanks,
> Matteo Martelli