Message-ID: <172926643961.69047.11194012379257398256@njaxe.localdomain>
Date: Fri, 18 Oct 2024 17:47:19 +0200
From: Matteo Martelli <matteomartelli3@...il.com>
To: jic23@...nel.org, lars@...afoo.de, victor.duicu@...rochip.com
Cc: marius.cristea@...rochip.com, victor.duicu@...rochip.com, linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5] iio: adc: pac1921: Add ACPI support to Microchip pac1921

Quoting victor.duicu@...rochip.com (2024-10-18 14:06:24)
> From: Victor Duicu <victor.duicu@...rochip.com>
> 
> This driver implements ACPI support to Microchip pac1921.
> The driver can read shunt resistor value and label from ACPI table.
> 
> Signed-off-by: Victor Duicu <victor.duicu@...rochip.com>
> ---
> 
> The patch was tested on minnowboard and sama5.
> 
> Differences related to previous versions:
> v5:

I think v4 was cleaner. Are the following changes necessary?

> - set maximum acceptable value of shunt resistor to 2KOHM in devicetree,
> ACPI table and user input. The chosen value is lesser than INT_MAX,
> which is about 2.1KOHM.

I would personally keep INT_MAX since the limitation is only given by the types
used in the current conversions (see pac1921_calc_scale()) rather than being a
specific PAC1921 hardware limitation. Otherwise I would extend the comment on
top of those two definitions explaining why that's the maximum.

> - in pac1921_match_acpi_device and pac1921_parse_of_fw change to only
>   read 32b values for resistor shunt.
>

I think that in pac1921_match_acpi_device(), the u64 temp var was introduced to
address the possible overflow coming from the u64 rez->package.elements[0].integer.value
and to safely perform a sanity check. I don't think we can trust blindly that
the ACPI table always has a valid 32bit value in that field.

pac1921_parse_of_fw() doesn't look changed compared to v4, you already switched
back to device_property_read_u32, if it's what you are referring to.

> 
> v4:
> - change name of pac1921_shunt_is_valid to pac1921_shunt_is_invalid.
> - fix coding style.
> - in pac1921_parse_of_fw change back to device_property_read_u32.
> 
> 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.
> 
>  drivers/iio/adc/pac1921.c | 109 +++++++++++++++++++++++++++++++++-----
>  1 file changed, 96 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/iio/adc/pac1921.c b/drivers/iio/adc/pac1921.c
> index a96fae546bc1..9622b0da6196 100644
> --- a/drivers/iio/adc/pac1921.c
> +++ b/drivers/iio/adc/pac1921.c
> @@ -67,6 +67,13 @@ 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"
> +/*The maximum accepted value of shunt_resistor is 2Kohms */
> +#define PAC1921_MAX_SHUNT_VALUE_OHMS           2000
> +#define PAC1921_MAX_SHUNT_VALUE_UOHMS          2000000000
> +
>  /*
>   * Pre-computed scale factors for BUS voltage
>   * format: IIO_VAL_INT_PLUS_NANO
> @@ -204,6 +211,11 @@ struct pac1921_priv {
>         } scan;
>  };
>  
> +static inline bool pac1921_shunt_is_invalid(u32 shunt_val)
> +{
> +       return (shunt_val == 0 || shunt_val > PAC1921_MAX_SHUNT_VALUE_UOHMS);
> +}
> +
>  /*
>   * Check if first integration after configuration update has completed.
>   *
> @@ -781,7 +793,7 @@ static ssize_t pac1921_write_shunt_resistor(struct iio_dev *indio_dev,
>                                             const char *buf, size_t len)
>  {
>         struct pac1921_priv *priv = iio_priv(indio_dev);
> -       u64 rshunt_uohm;
> +       u32 rshunt_uohm;
>         int val, val_fract;
>         int ret;
>  
> @@ -791,9 +803,13 @@ static ssize_t pac1921_write_shunt_resistor(struct iio_dev *indio_dev,
>         ret = iio_str_to_fixpoint(buf, 100000, &val, &val_fract);
>         if (ret)
>                 return ret;
> +       
> +       /* This check is to ensure val*MICRO won't overflow*/
> +       if (val < 0 || val > PAC1921_MAX_SHUNT_VALUE_OHMS)
> +               return -EINVAL;
>  
>         rshunt_uohm = val * MICRO + val_fract;
> -       if (rshunt_uohm == 0 || rshunt_uohm > INT_MAX)
> +       if (pac1921_shunt_is_invalid(rshunt_uohm))
>                 return -EINVAL;
>  

In v3 you added the (u64)val cast to solve the multiplication overflow. Wasn't
that enough? However, if this is for better clarity I am fine with it.

>         guard(mutex)(&priv->lock);
> @@ -1150,6 +1166,71 @@ 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;
> +       u32 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;

I don't think we can trust rez->package.elements[0].integer.value to always be
in 32bit boundaries. But if that's the case then the temp var looks unnecessary.

> +       ACPI_FREE(rez);
> +
> +       if (pac1921_shunt_is_invalid(temp))
> +               return dev_err_probe(&client->dev, -EINVAL, "Invalid shunt resistor\n");
> +
> +       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)
> +{
> +       int ret;
> +       struct device *dev = &client->dev;
> +
> +       ret = device_property_read_u32(dev, "shunt-resistor-micro-ohms",
> +                                      &priv->rshunt_uohm);
> +       if (ret)
> +               return dev_err_probe(dev, ret,
> +                                    "Cannot read shunt resistor property\n");
> +
> +       if (pac1921_shunt_is_invalid(priv->rshunt_uohm))
> +               return dev_err_probe(dev, -EINVAL, "Invalid shunt resistor: %u\n",
> +                                    priv->rshunt_uohm);
> +
> +       pac1921_calc_current_scales(priv);
> +
> +       return 0;
> +}
> +
>  static int pac1921_probe(struct i2c_client *client)
>  {
>         struct device *dev = &client->dev;
> @@ -1176,17 +1257,13 @@ static int pac1921_probe(struct i2c_client *client)
>         priv->di_gain = PAC1921_DEFAULT_DI_GAIN;
>         priv->n_samples = PAC1921_DEFAULT_NUM_SAMPLES;
>  
> -       ret = device_property_read_u32(dev, "shunt-resistor-micro-ohms",
> -                                      &priv->rshunt_uohm);
> -       if (ret)
> -               return dev_err_probe(dev, ret,
> -                                    "Cannot read shunt resistor property\n");
> -       if (priv->rshunt_uohm == 0 || priv->rshunt_uohm > INT_MAX)
> -               return dev_err_probe(dev, -EINVAL,
> -                                    "Invalid shunt resistor: %u\n",
> -                                    priv->rshunt_uohm);
> -
> -       pac1921_calc_current_scales(priv);
> +       if (ACPI_HANDLE(&client->dev))
> +               ret = pac1921_match_acpi_device(client, priv, indio_dev);
> +       else
> +               ret = pac1921_parse_of_fw(client, priv);
> +       if (ret < 0)
> +               return dev_err_probe(&client->dev, ret,
> +                                    "parameter parsing error\n");
>  
>         priv->vdd = devm_regulator_get(dev, "vdd");
>         if (IS_ERR(priv->vdd))
> @@ -1243,11 +1320,17 @@ static const struct of_device_id pac1921_of_match[] = {
>  };
>  MODULE_DEVICE_TABLE(of, pac1921_of_match);
>  
> +static const struct acpi_device_id pac1921_acpi_match[] = {
> +       { "MCHP1921" },
> +       { }
> +};
> +MODULE_DEVICE_TABLE(acpi, pac1921_acpi_match);
>  static struct i2c_driver pac1921_driver = {
>         .driver  = {
>                 .name = "pac1921",
>                 .pm = pm_sleep_ptr(&pac1921_pm_ops),
>                 .of_match_table = pac1921_of_match,
> +               .acpi_match_table = pac1921_acpi_match
>         },
>         .probe = pac1921_probe,
>         .id_table = pac1921_id,
> 
> base-commit: 57573ace0c1b142433dfe3d63ebf375269c80fc1
> -- 
> 2.43.0
> 

Best regards,
Matteo Martelli

Powered by blists - more mailing lists