drivers/iio/adc/pac1921.c | 100 ++++++++++++++++++++++++++++++++++---- 1 file changed, 90 insertions(+), 10 deletions(-)
From: Victor Duicu <victor.duicu@microchip.com>
This patch implements ACPI support to Microchip pac1921.
The driver can read the shunt resistor value and label from the ACPI table.
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Victor Duicu <victor.duicu@microchip.com>
---
The patch was tested on minnowboard and sama5.
Differences related to previous versions:
v12:
- in pac1921_match_acpi_device reformat the instructions that
were over 80 characters width.
- remove PAC1921_MAX_SHUNT_VALUE_uOHMS. Now the maximum acceptable value
of shunt resistor is INT_MAX. In pac1921_write_shunt_resistor
change the shunt check to respect the new limit and to avoid
overflow in val * MICRO.
v11:
- credit Andy Shevchenko for code review.
v10:
- fix coding style mistakes.
- add UL to PAC1921_MAX_SHUNT_VALUE_uOHMS.
- edit comment in pac1921_write_shunt_resistor.
- in pac1921_probe use is_acpi_device_node instead of
ACPI_HANDLE.
v9:
- put limits.h in sorted order.
- remove guid_parse and implement GUID_INIT.
- remove pac1921_shunt_is_valid.
- change maximum acceptable value of shunt resistor to 2146.999999.
This change was made so the readings for dt and ACPI share
the same range.
With this value the maximum current that can be read is
0.1V / 2146.999999 = 46.576 uA
If we use INT_MAX the maximum current is
0.1V / 2147.483647 = 46.566 uA
The relative error between the two is 0.0214, so it is
small enough to allow for code simplicity.
A shunt value over a few hundred Ohms is quite unusual.
v8:
- fix multiple coding style errors.
- in pac1921_match_acpi_device change error type to ENOMEM
at label is NULL branch.
- in pac1921_match_acpi_device when reading label,
change accesing method of string.
- change name of PAC1921_ACPI_GET_UOHMS_VALS to
PAC1921_ACPI_GET_uOHMS_VALS.
- add limits.h in include list.
- change integer constant in PAC1921_MAX_SHUNT_VALUE_OHMS
to INT_MAX / MICRO.
- change pac1921_shunt_is_invalid to pac1921_shunt_is_valid.
- in pac1921_match_acpi_device change name of variable rez to status.
v7:
- in pac1921_shunt_is_invalid remove brackets in return.
- in pac1921_match_acpi_device and pac1921_parse_of_fw move checking of
shunt value and scale calculation to pac1921_probe.
- in pac1921_match_acpi_device change devm_kmemdup to devm_kstrdup
and add label check for NULL.
- in pac1921_match_acpi_device and pac1921_parse_of_fw remove unnecessary
entry arguments. Now indio_dev is the only entry argument.
- in pac1921_probe, pac1921_match_acpi_device and pac1921_parse_of_fw
standardised structure accesing.
v6:
- set maximum acceptable value of shunt resistor to INT_MAX UOHMS
in devicetree, ACPI table and user input.
- in pac1921_match_acpi_device remove temp variable.
v5:
- 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.
- in pac1921_match_acpi_device and pac1921_parse_of_fw change to only
read 32b values for resistor shunt.
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 | 100 ++++++++++++++++++++++++++++++++++----
1 file changed, 90 insertions(+), 10 deletions(-)
diff --git a/drivers/iio/adc/pac1921.c b/drivers/iio/adc/pac1921.c
index b0f6727cfe38..a78ae34057a1 100644
--- a/drivers/iio/adc/pac1921.c
+++ b/drivers/iio/adc/pac1921.c
@@ -12,6 +12,7 @@
#include <linux/iio/iio.h>
#include <linux/iio/trigger_consumer.h>
#include <linux/iio/triggered_buffer.h>
+#include <linux/limits.h>
#include <linux/regmap.h>
#include <linux/units.h>
@@ -67,6 +68,14 @@ 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
+
+/* f7bb9932-86ee-4516-a236-7a7a742e55cb */
+static const guid_t pac1921_guid =
+ GUID_INIT(0xf7bb9932, 0x86ee, 0x4516, 0xa2,
+ 0x36, 0x7a, 0x7a, 0x74, 0x2e, 0x55, 0xcb);
+
/*
* Pre-computed scale factors for BUS voltage
* format: IIO_VAL_INT_PLUS_NANO
@@ -782,7 +791,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;
@@ -793,10 +802,17 @@ 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)
+ /*
+ * This check validates the shunt is not zero and does not surpass
+ * INT_MAX. The check is done before calculating in order to avoid
+ * val * MICRO overflowing.
+ */
+ if ((!val && !val_fract) || (val > INT_MAX / MICRO) ||
+ ((val == INT_MAX / MICRO) && (val_fract > INT_MAX % MICRO)))
return -EINVAL;
+ rshunt_uohm = val * MICRO + val_fract;
+
guard(mutex)(&priv->lock);
priv->rshunt_uohm = rshunt_uohm;
@@ -1151,6 +1167,61 @@ 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 iio_dev *indio_dev)
+{
+ acpi_handle handle;
+ union acpi_object *status;
+ char *label;
+ struct pac1921_priv *priv = iio_priv(indio_dev);
+ struct device *dev = &priv->client->dev;
+
+ handle = ACPI_HANDLE(dev);
+
+ status = acpi_evaluate_dsm(handle, &pac1921_guid, 1,
+ PAC1921_ACPI_GET_uOHMS_VALS, NULL);
+ if (!status)
+ return dev_err_probe(dev, -EINVAL,
+ "Could not read shunt from ACPI table\n");
+
+ priv->rshunt_uohm = status->package.elements[0].integer.value;
+ ACPI_FREE(status);
+
+ status = acpi_evaluate_dsm(handle, &pac1921_guid, 1,
+ PAC1921_ACPI_GET_LABEL, NULL);
+ if (!status)
+ return dev_err_probe(dev, -EINVAL,
+ "Could not read label from ACPI table\n");
+
+ label = devm_kstrdup(dev, status->package.elements[0].string.pointer,
+ GFP_KERNEL);
+ if (!label)
+ return -ENOMEM;
+
+ indio_dev->label = label;
+ ACPI_FREE(status);
+
+ return 0;
+}
+
+static int pac1921_parse_of_fw(struct iio_dev *indio_dev)
+{
+ int ret;
+ struct pac1921_priv *priv = iio_priv(indio_dev);
+ struct device *dev = &priv->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");
+
+ return 0;
+}
+
static int pac1921_probe(struct i2c_client *client)
{
struct device *dev = &client->dev;
@@ -1179,14 +1250,16 @@ 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)
+ if (is_acpi_device_node(dev->fwnode))
+ ret = pac1921_match_acpi_device(indio_dev);
+ else
+ ret = pac1921_parse_of_fw(indio_dev);
+ if (ret < 0)
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",
+ "Parameter parsing error\n");
+
+ if (!priv->rshunt_uohm || priv->rshunt_uohm > INT_MAX)
+ return dev_err_probe(dev, -EINVAL, "Invalid shunt resistor: %u\n",
priv->rshunt_uohm);
pac1921_calc_current_scales(priv);
@@ -1246,11 +1319,18 @@ 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: 20fd1383cd616d61b2a79967da1221dc6cfb8430
--
2.43.0Hi Victor,
Below just a few very minor styling points. Sorry I didn't notice them
before. However, I am also fine with this version as it is.
Acked-by: Matteo Martelli <matteomartelli3@gmail.com>
On Fri, 15 Nov 2024 12:07:56 +0200, <victor.duicu@microchip.com> wrote:
> From: Victor Duicu <victor.duicu@microchip.com>
>
> This patch implements ACPI support to Microchip pac1921.
> The driver can read the shunt resistor value and label from the ACPI table.
>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Signed-off-by: Victor Duicu <victor.duicu@microchip.com>
> ---
>
> The patch was tested on minnowboard and sama5.
>
> Differences related to previous versions:
> v12:
> - in pac1921_match_acpi_device reformat the instructions that
> were over 80 characters width.
> - remove PAC1921_MAX_SHUNT_VALUE_uOHMS. Now the maximum acceptable value
> of shunt resistor is INT_MAX. In pac1921_write_shunt_resistor
> change the shunt check to respect the new limit and to avoid
> overflow in val * MICRO.
>
> v11:
> - credit Andy Shevchenko for code review.
>
> v10:
> - fix coding style mistakes.
> - add UL to PAC1921_MAX_SHUNT_VALUE_uOHMS.
> - edit comment in pac1921_write_shunt_resistor.
> - in pac1921_probe use is_acpi_device_node instead of
> ACPI_HANDLE.
>
> v9:
> - put limits.h in sorted order.
> - remove guid_parse and implement GUID_INIT.
> - remove pac1921_shunt_is_valid.
> - change maximum acceptable value of shunt resistor to 2146.999999.
> This change was made so the readings for dt and ACPI share
> the same range.
> With this value the maximum current that can be read is
> 0.1V / 2146.999999 = 46.576 uA
> If we use INT_MAX the maximum current is
> 0.1V / 2147.483647 = 46.566 uA
> The relative error between the two is 0.0214, so it is
> small enough to allow for code simplicity.
> A shunt value over a few hundred Ohms is quite unusual.
>
> v8:
> - fix multiple coding style errors.
> - in pac1921_match_acpi_device change error type to ENOMEM
> at label is NULL branch.
> - in pac1921_match_acpi_device when reading label,
> change accesing method of string.
> - change name of PAC1921_ACPI_GET_UOHMS_VALS to
> PAC1921_ACPI_GET_uOHMS_VALS.
> - add limits.h in include list.
> - change integer constant in PAC1921_MAX_SHUNT_VALUE_OHMS
> to INT_MAX / MICRO.
> - change pac1921_shunt_is_invalid to pac1921_shunt_is_valid.
> - in pac1921_match_acpi_device change name of variable rez to status.
>
> v7:
> - in pac1921_shunt_is_invalid remove brackets in return.
> - in pac1921_match_acpi_device and pac1921_parse_of_fw move checking of
> shunt value and scale calculation to pac1921_probe.
> - in pac1921_match_acpi_device change devm_kmemdup to devm_kstrdup
> and add label check for NULL.
> - in pac1921_match_acpi_device and pac1921_parse_of_fw remove unnecessary
> entry arguments. Now indio_dev is the only entry argument.
> - in pac1921_probe, pac1921_match_acpi_device and pac1921_parse_of_fw
> standardised structure accesing.
>
> v6:
> - set maximum acceptable value of shunt resistor to INT_MAX UOHMS
> in devicetree, ACPI table and user input.
> - in pac1921_match_acpi_device remove temp variable.
>
> v5:
> - 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.
> - in pac1921_match_acpi_device and pac1921_parse_of_fw change to only
> read 32b values for resistor shunt.
>
> 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 | 100 ++++++++++++++++++++++++++++++++++----
> 1 file changed, 90 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/iio/adc/pac1921.c b/drivers/iio/adc/pac1921.c
> index b0f6727cfe38..a78ae34057a1 100644
> --- a/drivers/iio/adc/pac1921.c
> +++ b/drivers/iio/adc/pac1921.c
> @@ -12,6 +12,7 @@
> #include <linux/iio/iio.h>
> #include <linux/iio/trigger_consumer.h>
> #include <linux/iio/triggered_buffer.h>
> +#include <linux/limits.h>
> #include <linux/regmap.h>
> #include <linux/units.h>
>
> @@ -67,6 +68,14 @@ 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
> +
> +/* f7bb9932-86ee-4516-a236-7a7a742e55cb */
> +static const guid_t pac1921_guid =
> + GUID_INIT(0xf7bb9932, 0x86ee, 0x4516, 0xa2,
> + 0x36, 0x7a, 0x7a, 0x74, 0x2e, 0x55, 0xcb);
> +
> /*
> * Pre-computed scale factors for BUS voltage
> * format: IIO_VAL_INT_PLUS_NANO
> @@ -782,7 +791,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;
>
> @@ -793,10 +802,17 @@ 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)
> + /*
> + * This check validates the shunt is not zero and does not surpass
> + * INT_MAX. The check is done before calculating in order to avoid
> + * val * MICRO overflowing.
> + */
> + if ((!val && !val_fract) || (val > INT_MAX / MICRO) ||
> + ((val == INT_MAX / MICRO) && (val_fract > INT_MAX % MICRO)))
> return -EINVAL;
Can the unnecessary parenthesis be removed?
if ((!val && !val_fract) || (val > INT_MAX / MICRO) ||
^.....................^
((val == INT_MAX / MICRO) && (val_fract > INT_MAX % MICRO)))
^......................^ ^...........................^
>
> + rshunt_uohm = val * MICRO + val_fract;
> +
> guard(mutex)(&priv->lock);
>
> priv->rshunt_uohm = rshunt_uohm;
> @@ -1151,6 +1167,61 @@ 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 iio_dev *indio_dev)
> +{
> + acpi_handle handle;
> + union acpi_object *status;
> + char *label;
> + struct pac1921_priv *priv = iio_priv(indio_dev);
> + struct device *dev = &priv->client->dev;
> +
> + handle = ACPI_HANDLE(dev);
> +
> + status = acpi_evaluate_dsm(handle, &pac1921_guid, 1,
> + PAC1921_ACPI_GET_uOHMS_VALS, NULL);
> + if (!status)
> + return dev_err_probe(dev, -EINVAL,
> + "Could not read shunt from ACPI table\n");
> +
> + priv->rshunt_uohm = status->package.elements[0].integer.value;
> + ACPI_FREE(status);
> +
> + status = acpi_evaluate_dsm(handle, &pac1921_guid, 1,
> + PAC1921_ACPI_GET_LABEL, NULL);
> + if (!status)
> + return dev_err_probe(dev, -EINVAL,
> + "Could not read label from ACPI table\n");
> +
> + label = devm_kstrdup(dev, status->package.elements[0].string.pointer,
> + GFP_KERNEL);
> + if (!label)
> + return -ENOMEM;
> +
> + indio_dev->label = label;
> + ACPI_FREE(status);
> +
> + return 0;
> +}
> +
> +static int pac1921_parse_of_fw(struct iio_dev *indio_dev)
> +{
> + int ret;
> + struct pac1921_priv *priv = iio_priv(indio_dev);
> + struct device *dev = &priv->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");
> +
> + return 0;
> +}
> +
> static int pac1921_probe(struct i2c_client *client)
> {
> struct device *dev = &client->dev;
> @@ -1179,14 +1250,16 @@ 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)
> + if (is_acpi_device_node(dev->fwnode))
> + ret = pac1921_match_acpi_device(indio_dev);
> + else
> + ret = pac1921_parse_of_fw(indio_dev);
> + if (ret < 0)
For consistency: if (ret) instead of if (ret < 0)
> 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",
> + "Parameter parsing error\n");
> +
> + if (!priv->rshunt_uohm || priv->rshunt_uohm > INT_MAX)
> + return dev_err_probe(dev, -EINVAL, "Invalid shunt resistor: %u\n",
The error string in a new line to keep it 80 char wide.
> priv->rshunt_uohm);
>
> pac1921_calc_current_scales(priv);
> @@ -1246,11 +1319,18 @@ 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: 20fd1383cd616d61b2a79967da1221dc6cfb8430
> --
> 2.43.0
>
Thanks,
Matteo Martelli
© 2016 - 2024 Red Hat, Inc.