Message-ID: <172794015844.2520.11909797050797595912@njaxe.localdomain>
Date: Thu, 03 Oct 2024 09:22:38 +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 v2] iio: adc: pac1921: add ACPI support to pac1921

Quoting victor.duicu@...rochip.com (2024-10-02 14:54:45)
> From: Victor Duicu <victor.duicu@...rochip.com>
> 
> This patch implements ACPI support to Microchip pac1921.
> The driver can read shunt resistor value and device label
> from ACPI table.
> 
> Differences related to previous versions:
> 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.

Thanks Victor for having addressed our previous points. I still have a few
comments, see below.

> 
> Signed-off-by: Victor Duicu <victor.duicu@...rochip.com>
> ---
>  drivers/iio/adc/pac1921.c | 112 ++++++++++++++++++++++++++++++++++----
>  1 file changed, 101 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/iio/adc/pac1921.c b/drivers/iio/adc/pac1921.c
> index 4c2a1c07bc39..95ade1c4d5e8 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_NAME                  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,14 @@ struct pac1921_priv {
>         } scan;
>  };
>  
> +static bool pac1921_shunt_is_valid(u32 shunt_val)
> +{
> +       if (shunt_val == 0 || shunt_val > INT_MAX)
> +               return false;
> +       else
> +               return true;
> +}
> +

I think this can be inline. Also, it looks a more common pattern to just return
the condition expression result.
To see some examples: grep -A 5 -R "inline .*bool .*" .

Moreover, since both the values coming from ACPI and sysfs can exceed 32-bit
boundaries, maybe it would be better to get a u64 argument here. Otherwise a
value like 5KOhm (5_000_000_000) would wrap around in the cast (~7Ohm) but still
considered as valid.

>  /*
>   * Check if first integration after configuration update has completed.
>   *
> @@ -794,7 +806,7 @@ static ssize_t pac1921_write_shunt_resistor(struct iio_dev *indio_dev,
>                 return ret;
>  
>         rshunt_uohm = (u32)val * MICRO + (u32)val_fract;
> -       if (rshunt_uohm == 0 || rshunt_uohm > INT_MAX)
> +       if (!pac1921_shunt_is_valid((u32)rshunt_uohm))

The explicit cast is not necessary, especially if the validity function would
take a u64 argument.

>                 return -EINVAL;
>  
>         guard(mutex)(&priv->lock);
> @@ -1151,6 +1163,81 @@ 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/PAC1934-Integration-Notes-for-Microsoft-Windows-10-and-Windows-11-Driver-Support-DS00002534.pdf

It looks like the link is not working, is the following the new one (PAC1934 -> PAC193X) ?
https://ww1.microchip.com/downloads/aemDocuments/documents/OTH/ApplicationNotes/ApplicationNotes/PAC193X-Integration-Notes-for-Microsoft-Windows-10-and-Windows-11-Driver-Support-DS00002534.pdf

Should it be updated for the pac1934 driver as well?

> + */
> +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;
> +
> +       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");
> +
> +       priv->rshunt_uohm = rez->package.elements[0].integer.value;

Doing this assignment before the validity check may result into accepting a
value out of the 32-bit range which would wrap around in the implicit cast (e.g.
5KOhm -> ~7Ohm). I think you could either move the validity check before or use
a u64 temp var.

> +       ACPI_FREE(rez);
> +
> +       if (!pac1921_shunt_is_valid(priv->rshunt_uohm))
> +               return dev_err_probe(&client->dev, -EINVAL, "Invalid shunt resistor: %u\n",
> +                                    priv->rshunt_uohm);
> +
> +       pac1921_calc_current_scales(priv);
> +
> +       rez = acpi_evaluate_dsm(handle, &guid, 1, PAC1921_ACPI_GET_NAME, NULL);
> +       if (!rez)
> +               return dev_err_probe(&client->dev, -EINVAL,
> +                                    "Could not read name 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;
> +
> +       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_valid(priv->rshunt_uohm))
> +               return dev_err_probe(dev, -EINVAL, "Invalid shunt resistor: %u\n",
> +                                    priv->rshunt_uohm);
> +
> +       pac1921_calc_current_scales(priv);
> +
> +       if (device_property_present(dev, "label")) {
> +               ret = device_property_read_string(dev, "label",
> +                                                 (const char **)&indio_dev->label);
> +               if (ret)
> +                       return dev_err_probe(&client->dev, ret,
> +                                            "Invalid rail-name value\n");
> +       } else {
> +               indio_dev->label = "pac1921";
> +       }

IIO core already checks for the label string property from DT, see
industrialio-core.c:__iio_device_register(). So I think it can be removed from
here, unless you want the driver probe to fail for an invalid label string,
which it would not happen with the IIO core handling. Also, is it necessary to
have a default label? It looks meaningless to me since it would have the same
value of the name attribute.

> +
> +       return 0;
> +}
> +
>  static int pac1921_probe(struct i2c_client *client)
>  {
>         struct device *dev = &client->dev;
> @@ -1177,17 +1264,14 @@ 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);
> +       if (ACPI_HANDLE(&client->dev))
> +               ret = pac1921_match_acpi_device(client, priv, indio_dev);
> +       else
> +               ret = pac1921_parse_of_fw(client, priv,  indio_dev);
>  
> -       pac1921_calc_current_scales(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))
> @@ -1244,11 +1328,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: fec496684388685647652ab4213454fbabdab099
> -- 
> 2.43.0
> 

Thanks,
Matteo Martelli

Powered by blists - more mailing lists