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

[Jonathan Cameron]

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


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