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

[Victor.Duicu]

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>
> > ---
> >  drivers/iio/adc/pac1921.c | 106 +++++++++++++++++++++++++++++++++-
> > ----
> >  1 file changed, 93 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/pac1921.c b/drivers/iio/adc/pac1921.c
> > index 567279664e74..01c5eceab0be 100644
> > --- a/drivers/iio/adc/pac1921.c
> > +++ b/drivers/iio/adc/pac1921.c
> > @@ -67,6 +67,10 @@ enum pac1921_mxsl {
> >  #define PAC1921_DEFAULT_DI_GAIN                0 /* 2^(value): 1x
> > gain (HW default) */
> >  #define PAC1921_DEFAULT_NUM_SAMPLES    0 /* 2^(value): 1 sample
> > (HW default) */
> > 
> > +#define PAC1921_ACPI_GET_UOHMS_VALS             0
> > +#define PAC1921_ACPI_GET_LABEL                 1
> > +#define PAC1921_DSM_UUID                        "f7bb9932-86ee-
> > 4516-a236-7a7a742e55cb"
> > +
> >  /*
> >   * Pre-computed scale factors for BUS voltage
> >   * format: IIO_VAL_INT_PLUS_NANO
> > @@ -204,6 +208,11 @@ struct pac1921_priv {
> >         } scan;
> >  };
> > 
> > +static inline bool pac1921_shunt_is_valid(u64 shunt_val)
> > +{
> > +       return (shunt_val == 0 || shunt_val > INT_MAX);
> > +}
> > +
> 
> It's very confusing that this returns true when the shunt is NOT
> valid. I would
> either negate the return value or change the name.
> 
> >  /*
> >   * 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.

> > +       if (pac1921_shunt_is_valid(rshunt_uohm))
> >                 return -EINVAL;
> 
> The error should be returned when the shunt is NOT valid.
> 
> > 
> >         guard(mutex)(&priv->lock);
> > 
> > -       priv->rshunt_uohm = rshunt_uohm;
> > +       priv->rshunt_uohm = (u32)rshunt_uohm;
> 
> I would remove the unnecessary explicit cast for the above reason.
> > 
> >         pac1921_calc_current_scales(priv);
> > 
> > @@ -1150,6 +1159,74 @@ static void pac1921_regulator_disable(void
> > *data)
> >         regulator_disable(regulator);
> >  }
> > 
> > +/*
> > + * documentation related to the ACPI device definition
> > + *
> > https://ww1.microchip.com/downloads/aemDocuments/documents/OTH/ApplicationNotes/ApplicationNotes/PAC193X-Integration-Notes-for-Microsoft-Windows-10-and-Windows-11-Driver-Support-DS00002534.pdf
> > + */
> > +static int pac1921_match_acpi_device(struct i2c_client *client,
> > struct pac1921_priv *priv,
> > +                                    struct iio_dev *indio_dev)
> > +{
> > +       acpi_handle handle;
> > +       union acpi_object *rez;
> > +       guid_t guid;
> > +       char *label;
> > +       u64 temp;
> > +
> > +       guid_parse(PAC1921_DSM_UUID, &guid);
> > +       handle = ACPI_HANDLE(&client->dev);
> > +
> > +       rez = acpi_evaluate_dsm(handle, &guid, 1,
> > PAC1921_ACPI_GET_UOHMS_VALS, NULL);
> > +       if (!rez)
> > +               return dev_err_probe(&client->dev, -EINVAL,
> > +                                    "Could not read shunt from
> > ACPI table\n");
> > +
> > +       temp = rez->package.elements[0].integer.value;
> > +       ACPI_FREE(rez);
> > +
> > +       if (pac1921_shunt_is_valid(temp))
> > +               return dev_err_probe(&client->dev, -EINVAL,
> > "Invalid shunt resistor\n");
> 
> The error should be returned when the shunt is NOT valid.
> 
> > +
> > +       priv->rshunt_uohm = temp;
> > +       pac1921_calc_current_scales(priv);
> > +
> > +       rez = acpi_evaluate_dsm(handle, &guid, 1,
> > PAC1921_ACPI_GET_LABEL, NULL);
> > +       if (!rez)
> > +               return dev_err_probe(&client->dev, -EINVAL,
> > +                                    "Could not read label from
> > ACPI table\n");
> > +
> > +       label = devm_kmemdup(&client->dev, rez->package.elements-
> > >string.pointer,
> > +                            (size_t)rez->package.elements-
> > >string.length + 1,
> > +                            GFP_KERNEL);
> > +       label[rez->package.elements->string.length] = '\0';
> > +       indio_dev->label = label;
> > +       ACPI_FREE(rez);
> > +
> > +       return 0;
> > +}
> > +
> > +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.

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