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