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

[Victor.Duicu]

On Mon, 2024-10-14 at 19:46 +0100, Jonathan Cameron wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> > 
> > > > > +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.
> 
> I'll ask the silly question. How big a shunt resistor do you have?
> If it's necessary to change them over that is fine but if that means
> existing dt is wrong, then you'd need to maintain compatibility.
> So test with a 32 bit dt value and 64 bit. Probably need to try
> 64 bit and if it fails try 32 bits.

The maximum resistor we use is 4K. I agree now that it is unnecessary
to change the devicetree to read 64b values. I will undo those changes
and read only 32b values.

> 
> > 
> > > > > +
> > > > > +       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

Thank you,
Victor Duicu