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

[<Victor.Duicu@microchip.com>]

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@microchip.com>
> wrote:
> > From: Victor Duicu <victor.duicu@microchip.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@gmail.com>
> > Signed-off-by: Victor Duicu <victor.duicu@microchip.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?
Also, the use of mixed case was suggested by Andy to increase
readability.

...

> 
> 
> Best regards,
> Matteo Martelli

With Best Regards,
Duicu Victor