| Message ID | 20241018120624.15409-1-victor.duicu@microchip.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v5] iio: adc: pac1921: Add ACPI support to Microchip pac1921 | expand |
Quoting victor.duicu@microchip.com (2024-10-18 14:06:24) > From: Victor Duicu <victor.duicu@microchip.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@microchip.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
On Fri, 2024-10-18 at 17:47 +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-18 14:06:24) > > From: Victor Duicu <victor.duicu@microchip.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@microchip.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. Will set maximum acceptable value to INT_MAX. > > - 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. If the ACPI table has revision set to 1, all values higher than 32b will be truncated to 32b. > 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. Yes, in v4 I changed pac1921_parse_of_fw and in v5 I changed pac1921_match_acpi_device. I changed both back to 32b in order to be consistent. > > > > 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. > > > > > > 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) */ > > > > > > > > @@ -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. The cast to 64b was removed in order to be consistent with dt and ACPI that will read only 32b values. > > 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. Will remove the temp var in the next version. > > + 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; > > +} > > > > -- > > 2.43.0 > > > > Best regards, > Matteo Martelli
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; 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; + 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,