drivers/iio/adc/pac1921.c | 106 +++++++++++++++++++++++++++++++++----- 1 file changed, 93 insertions(+), 13 deletions(-)
From: Victor Duicu <victor.duicu@microchip.com>
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@microchip.com>
---
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);
+}
+
/*
* 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;
+ if (pac1921_shunt_is_valid(rshunt_uohm))
return -EINVAL;
guard(mutex)(&priv->lock);
- priv->rshunt_uohm = rshunt_uohm;
+ priv->rshunt_uohm = (u32)rshunt_uohm;
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");
+
+ 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);
+
+ 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);
+
+ priv->rshunt_uohm = (u32)temp;
+ pac1921_calc_current_scales(priv);
+
+ return 0;
+}
+
static int pac1921_probe(struct i2c_client *client)
{
struct device *dev = &client->dev;
@@ -1176,17 +1253,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))
@@ -1243,11 +1317,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: c4f9679c92dc8f5a16cd3ad1c9a4a23c6d3f52d7
--
2.43.0On Fri, 11 Oct 2024 16:44:54 +0300
<victor.duicu@microchip.com> wrote:
> From: Victor Duicu <victor.duicu@microchip.com>
>
> 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".
>
Change log should be below the --- as it doesn't want to end up in the git history.
> 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@microchip.com>
A few comments in line.
J
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);
> +
No line break here. Want to associate that error check with the line above
as clearly as possible.
> + 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);
> +
> + priv->rshunt_uohm = (u32)temp;
> + pac1921_calc_current_scales(priv);
> +
> + return 0;
> +}
Quoting victor.duicu@microchip.com (2024-10-11 15:44:54)
> From: Victor Duicu <victor.duicu@microchip.com>
>
> 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@microchip.com>
> ---
> 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
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@microchip.com (2024-10-11 15:44:54)
> > From: Victor Duicu <victor.duicu@microchip.com>
> >
> > 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@microchip.com>
> > ---
> > 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
Quoting Victor.Duicu@microchip.com (2024-10-14 12:08:05)
> 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@microchip.com (2024-10-11 15:44:54)
> > > From: Victor Duicu <victor.duicu@microchip.com>
> > >
> > > 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@microchip.com>
...
> > > /*
> > > * 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.
>
You are right, the (u64) explicit cast is necessary and I think the
issue is relevant even when val is lesser than INT_MAX: on 32bit
architectures, val * MICRO is implicitly casted to u32, thus a resulting
value of that multiplication that is bigger than INT_MAX could pass as
valid even if it's not. For example if val is 0x40000000, val * MICRO
would be casted to 0 even if way bigger than INT_MAX.
...
> > > +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.
> > > +
> > > + 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
>
> > > > +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
On Mon, 2024-10-14 at 19:46 +0100, Jonathan Cameron wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
>
> >
> > > > > +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.
The maximum resistor we use is 4K. I agree now that it is unnecessary
to change the devicetree to read 64b values. I will undo those changes
and read only 32b values.
>
> >
> > > > > +
> > > > > + 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
Thank you,
Victor Duicu
© 2016 - 2024 Red Hat, Inc.