Re: [PATCH v3] iio: adc: pac1921: add ACPI support to Microchip pac1921.

[Matteo Martelli]

Quoting Victor.Duicu@xxxxxxxxxxxxx (2024-10-14 12:08:05)
> On Sat, 2024-10-12 at 12:05 +0200, Matteo Martelli wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > know the content is safe
> > 
> > Quoting victor.duicu@xxxxxxxxxxxxx (2024-10-11 15:44:54)
> > > From: Victor Duicu <victor.duicu@xxxxxxxxxxxxx>
> > > 
> > > This patch implements ACPI support to Microchip pac1921.
> > > The driver can read shunt resistor value and label from ACPI table.
> > > 
> > > The patch was tested on a minnowboard(64b) and sama5(32b).
> > > In order to avoid overflow when reading 64b values from ACPi table
> > > or
> > > devicetree it is necessary:
> > > - the revision of .dsl file must be 2 or greater to enable 64b
> > > arithmetic.
> > > - the shunt resistor variable in devicetree must have the prefix
> > > "/bits/ 64".
> > > 
> > > Differences related to previous versions:
> > > v3:
> > > - simplify and make inline function pac1921_shunt_is_valid. Make
> > > argument u64.
> > > - fix link to DSM documentation.
> > > - in pac1921_match_acpi_device and pac1921_parse_of_fw, the shunt
> > > value is
> > > read as u64.
> > > - in pac1921_parse_of_fw remove code for reading label value from
> > > devicetree.
> > > - in pac1921_write_shunt_resistor cast the multiply result to u64
> > > in order
> > > to fix overflow.
> > > 
> > > v2:
> > > - remove name variable from priv. Driver reads label attribute with
> > > sysfs.
> > > - define pac1921_shunt_is_valid function.
> > > - move default assignments in pac1921_probe to original position.
> > > - roll back coding style changes.
> > > - add documentation for DSM(the linked document was used as
> > > reference).
> > > - remove acpi_match_device in pac1921_match_acpi_device.
> > > - remove unnecessary null assignment and comment.
> > > - change name of function pac1921_match_of_device to
> > > pac1921_parse_of_fw.
> > > 
> > > v1:
> > > - initial version for review.
> > > 
> > > Signed-off-by: Victor Duicu <victor.duicu@xxxxxxxxxxxxx>

...

> > >  /*
> > >   * Check if first integration after configuration update has
> > > completed.
> > >   *
> > > @@ -792,13 +801,13 @@ static ssize_t
> > > pac1921_write_shunt_resistor(struct iio_dev *indio_dev,
> > >         if (ret)
> > >                 return ret;
> > > 
> > > -       rshunt_uohm = val * MICRO + val_fract;
> > > -       if (rshunt_uohm == 0 || rshunt_uohm > INT_MAX)
> > > +       rshunt_uohm = (u64)val * MICRO + val_fract;
> > 
> > In commit a9bb0610b2fa ("iio: pac1921: remove unnecessary explicit
> > casts"),
> > unnecessary explicit casts had been removed since it seems the
> > preferred
> > approach in order to improve readability. This (u64)val cast seems
> > unnecessary
> > as well thus I would keep the expression without it.
> 
> While testing on SamA5 board , the multiplication between val and MICRO
> can overflow when val is greater than INT_MAX. The cast to (u64) is
> necessary to correctly calculate the new shunt value.
> 

You are right, the (u64) explicit cast is necessary and I think the
issue is relevant even when val is lesser than INT_MAX: on 32bit
architectures, val * MICRO is implicitly casted to u32, thus a resulting
value of that multiplication that is bigger than INT_MAX could pass as
valid even if it's not. For example if val is 0x40000000, val * MICRO
would be casted to 0 even if way bigger than INT_MAX.

...

> > > +static int pac1921_parse_of_fw(struct i2c_client *client, struct
> > > pac1921_priv *priv,
> > > +                              struct iio_dev *indio_dev)
> > > +{
> > > +       int ret;
> > > +       struct device *dev = &client->dev;
> > > +       u64 temp;
> > > +
> > > +       ret = device_property_read_u64(dev, "shunt-resistor-micro-
> > > ohms", &temp);
> > 
> > Since the driver would discard a value out of INT boundaries, I don't
> > see the
> > need to read a value larger than u32 that would be discarded anyway.
> > To my
> > understanding, device_property_read_u32() should fail for an
> > overflowing value
> > thus I would keep device_property_read_u32() here, and at that point
> > the temp
> > var would not be necessary as well. I think it would also help to
> > keep the patch
> > diff confined in the ACPI extension context.
> 
> If the value in .dtso is greater than 32b, at compilation it will be
> truncated, and the incorrect value will be accepted by the driver. By
> adding "/bits/ 64" in the devicetree to shunt resistor the value will
> not be truncated. This way values on 32b and 64b can be read correctly.
> 

I see your point but if I understand this correctly with this change the
shunt-resistor-micro-ohms field in the DT should always be specified
with /bits/ 64, even for values in 32bit boundaries. I might be wrong
but this looks like something that should be documented in
Documentation/devicetree/bindings, especially since all the other
shunt-resistor-micro-ohms instances look to be interpreted as u32.
Also, I think that such change would fit better in a different patch as
it is not related to the introduction of ACPI support.

> > > +
> > > +       if (ret)
> > > +               return dev_err_probe(dev, ret,
> > > +                                    "Cannot read shunt resistor
> > > property\n");
> > > +
> > > +       if (pac1921_shunt_is_valid(temp))
> > > +               return dev_err_probe(dev, -EINVAL, "Invalid shunt
> > > resistor: %u\n",
> > > +                                    priv->rshunt_uohm);
> > 
> > The error should be returned when the shunt is NOT valid.
> > 
> > > +
> > > +       priv->rshunt_uohm = (u32)temp;
> > 
> > The temp var should not be necessary if switching back to
> > device_property_read_u32(),
> > otherwise I would remove the unnecessary explicit cast for the above
> > reason.
> > 
> > > +       pac1921_calc_current_scales(priv);
> > > +
> > > +       return 0;
> > > +}
> > > +

Thanks,
Matteo Martelli