diff mbox series

[v7] iio: adc: pac1921: Add ACPI support to Microchip pac1921

Message ID 20241031065205.50154-1-victor.duicu@microchip.com (mailing list archive)
State Accepted
Headers show
Series [v7] iio: adc: pac1921: Add ACPI support to Microchip pac1921 | expand

Commit Message

victor.duicu@microchip.com Oct. 31, 2024, 6:52 a.m. UTC 
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.

Signed-off-by: Victor Duicu <victor.duicu@microchip.com>
---

The patch was tested on minnowboard and sama5.

Differences related to previous versions:
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 | 95 +++++++++++++++++++++++++++++++++++----
 1 file changed, 86 insertions(+), 9 deletions(-)


base-commit: e2687d0723257db5025a4cf8cefbd80bed1e2681

Comments

Jonathan Cameron Nov. 2, 2024, 3:45 p.m. UTC | #1 
On Thu, 31 Oct 2024 08:52:05 +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 shunt resistor value and label from ACPI table.
> 
> Signed-off-by: Victor Duicu <victor.duicu@microchip.com>
Hi Victor.

I made a few minor tweaks whilst applying (see inline)

Applied to the togreg branch of iio.git and pushed out as testing for now
so that the bots can take a look and see if we missed anything.

Jonathan

> +/*
> + * 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 *rez;
> +	guid_t guid;
> +	char *label;
> +	struct pac1921_priv *priv = iio_priv(indio_dev);
> +	struct device *dev = &priv->client->dev;
> +
> +	guid_parse(PAC1921_DSM_UUID, &guid);
> +	handle = ACPI_HANDLE(dev);
> +
> +	rez = acpi_evaluate_dsm(handle, &guid, 1, PAC1921_ACPI_GET_UOHMS_VALS, NULL);
> +	if (!rez)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "Could not read shunt from ACPI table\n");
> +
> +	priv->rshunt_uohm = rez->package.elements[0].integer.value;
> +	ACPI_FREE(rez);
> +
> +	rez = acpi_evaluate_dsm(handle, &guid, 1, PAC1921_ACPI_GET_LABEL, NULL);
> +	if (!rez)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "Could not read label from ACPI table\n");
> +
> +	label = devm_kstrdup(dev, rez->package.elements->string.pointer, GFP_KERNEL);
> +	if (!label)
> +		return dev_err_probe(dev, -EINVAL, "Label is NULL\n");
ENOMEM appropriate I think. 

Also, a package is an array of elements, and this is the first one so
maybe res->package.elements[0].string.pointer is more appropriate?
(similar to above).


> +
> +	indio_dev->label = label;
> +	ACPI_FREE(rez);
> +
> +	return 0;
> +}
Andy Shevchenko Nov. 2, 2024, 7:48 p.m. UTC | #2 
Thu, Oct 31, 2024 at 08:52:05AM +0200, victor.duicu@microchip.com kirjoitti:
> 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.

This ID might be okay, but can we please have:
1) the list of the models (or a model) of the device on the market that has this;
2) ACPI DSDT excerpt of the respective Device object?

(I mean a laptop, tablet, phone or other device that has this sensor described
in the ACPI)

...

> +/*
> + * documentation related to the ACPI device definition

Documentation

> + * https://ww1.microchip.com/downloads/aemDocuments/documents/OTH/ApplicationNotes/ApplicationNotes/PAC193X-Integration-Notes-for-Microsoft-Windows-10-and-Windows-11-Driver-Support-DS00002534.pdf
> + */

...

> +	if (ACPI_HANDLE(dev))

Hmm... Want this be really needed? You can try to call DSM. and if it fails try
DT (or actually other way around as we usually do).

> +		ret = pac1921_match_acpi_device(indio_dev);
> +	else
> +		ret = pac1921_parse_of_fw(indio_dev);

...

> +static const struct acpi_device_id pac1921_acpi_match[] = {
> +	{ "MCHP1921" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(acpi, pac1921_acpi_match);

Missing blank line here.

...

>  	.driver	 = {
>  		.name = "pac1921",
>  		.pm = pm_sleep_ptr(&pac1921_pm_ops),
>  		.of_match_table = pac1921_of_match,
> +		.acpi_match_table = pac1921_acpi_match


Missing trailing comma here.

>  	},
Andy Shevchenko Nov. 2, 2024, 8:01 p.m. UTC | #3 
Thu, Oct 31, 2024 at 08:52:05AM +0200, victor.duicu@microchip.com kirjoitti:
> 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.

More comments below.

...

> +#define PAC1921_ACPI_GET_UOHMS_VALS             0

uOHM ?

#define PAC1921_ACPI_GET_uOHM_VALS             0

...

> +/* The maximum accepted value of shunt_resistor in UOHMS <= INT_MAX */
> +#define PAC1921_MAX_SHUNT_VALUE_OHMS		2147

Instead of the comment do it like this:

#define PAC1921_MAX_SHUNT_VALUE_OHM		(INT_MAX / MICRO)

Need to include limits.h and units.h.

...

> +static inline bool pac1921_shunt_is_invalid(u32 shunt_val)

is_invalid is confusing name, make it rather is_valid

> +{
> +	return shunt_val == 0 || shunt_val > INT_MAX;
> +}

...

> +	/* 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;

With the above check how this can't be a dead code?

...

> +	rez = acpi_evaluate_dsm(handle, &guid, 1, PAC1921_ACPI_GET_UOHMS_VALS, NULL);
> +	if (!rez)

Use status instead of rez for the variable name. This is kinda standard to keep
the return of ACPI APIs.

> +		return dev_err_probe(dev, -EINVAL,
> +				     "Could not read shunt from ACPI table\n");

...

> +	label = devm_kstrdup(dev, rez->package.elements->string.pointer, GFP_KERNEL);
> +	if (!label)
> +		return dev_err_probe(dev, -EINVAL, "Label is NULL\n");

We do not print an error for -ENOMEM, which should be here as the error code
(Jonathan already pointed out on this).

So, just

		return -ENOMEM;
Jonathan Cameron Nov. 3, 2024, 11:01 a.m. UTC | #4 
On Sat, 2 Nov 2024 15:45:09 +0000
Jonathan Cameron <jic23@kernel.org> wrote:

> On Thu, 31 Oct 2024 08:52:05 +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 shunt resistor value and label from ACPI table.
> > 
> > Signed-off-by: Victor Duicu <victor.duicu@microchip.com>  
> Hi Victor.
> 
> I made a few minor tweaks whilst applying (see inline)
> 
> Applied to the togreg branch of iio.git and pushed out as testing for now
> so that the bots can take a look and see if we missed anything.
Backed out again so you can resolve Andy's comments.

Jonathan

> 
> Jonathan
> 
> > +/*
> > + * 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 *rez;
> > +	guid_t guid;
> > +	char *label;
> > +	struct pac1921_priv *priv = iio_priv(indio_dev);
> > +	struct device *dev = &priv->client->dev;
> > +
> > +	guid_parse(PAC1921_DSM_UUID, &guid);
> > +	handle = ACPI_HANDLE(dev);
> > +
> > +	rez = acpi_evaluate_dsm(handle, &guid, 1, PAC1921_ACPI_GET_UOHMS_VALS, NULL);
> > +	if (!rez)
> > +		return dev_err_probe(dev, -EINVAL,
> > +				     "Could not read shunt from ACPI table\n");
> > +
> > +	priv->rshunt_uohm = rez->package.elements[0].integer.value;
> > +	ACPI_FREE(rez);
> > +
> > +	rez = acpi_evaluate_dsm(handle, &guid, 1, PAC1921_ACPI_GET_LABEL, NULL);
> > +	if (!rez)
> > +		return dev_err_probe(dev, -EINVAL,
> > +				     "Could not read label from ACPI table\n");
> > +
> > +	label = devm_kstrdup(dev, rez->package.elements->string.pointer, GFP_KERNEL);
> > +	if (!label)
> > +		return dev_err_probe(dev, -EINVAL, "Label is NULL\n");  
> ENOMEM appropriate I think. 
> 
> Also, a package is an array of elements, and this is the first one so
> maybe res->package.elements[0].string.pointer is more appropriate?
> (similar to above).
> 
> 
> > +
> > +	indio_dev->label = label;
> > +	ACPI_FREE(rez);
> > +
> > +	return 0;
> > +}  
> 
>
Jonathan Cameron Nov. 3, 2024, 11:10 a.m. UTC | #5 
On Sat, 2 Nov 2024 21:48:04 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> Thu, Oct 31, 2024 at 08:52:05AM +0200, victor.duicu@microchip.com kirjoitti:
> > 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.  
> 
> This ID might be okay, but can we please have:
> 1) the list of the models (or a model) of the device on the market that has this;
> 2) ACPI DSDT excerpt of the respective Device object?
> 
> (I mean a laptop, tablet, phone or other device that has this sensor described
> in the ACPI)

For a valid ID used in a patch submitted by an employee of the relevant
company this is a steep ask.  As far as I'm concerned it is definitely not
a requirement (unlike for non compliant IDs!) There is no reason why a
device might be public at this time. 

For a good upstream first company almost all IDs go into the kernel
'before' device announcement or sales.  I very much want to encourage
that not put potential barriers in the way.

So Victor, if you can provide the info Andy requested, then great but if not
feel free to say so. (+ the snippet is useful anyway and is unlikely
to contain anything confidential.) 

Jonathan
victor.duicu@microchip.com Nov. 5, 2024, 8:31 a.m. UTC | #6 
On Sat, 2024-11-02 at 21:48 +0200, Andy Shevchenko wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> Thu, Oct 31, 2024 at 08:52:05AM +0200,
> victor.duicu@microchip.com kirjoitti:
> > From: Victor Duicu <victor.duicu@microchip.com>

Hi Andy,

> > 
> > This patch implements ACPI support to Microchip pac1921.
> > The driver can read shunt resistor value and label from ACPI table.
> 
> This ID might be okay, but can we please have:
> 1) the list of the models (or a model) of the device on the market
> that has this;
> 2) ACPI DSDT excerpt of the respective Device object?
> 
> (I mean a laptop, tablet, phone or other device that has this sensor
> described
> in the ACPI)

We do not have a list of end-use devices for pac1921.

> 
> ...
> 
> > +/*
> > + * documentation related to the ACPI device definition
> 
> Documentation
> 
> > + *
> > https://ww1.microchip.com/downloads/aemDocuments/documents/OTH/ApplicationNotes/ApplicationNotes/PAC193X-Integration-Notes-for-Microsoft-Windows-10-and-Windows-11-Driver-Support-DS00002534.pdf
> > + */
> 
> ...
> 
> > +     if (ACPI_HANDLE(dev))
> 
> Hmm... Want this be really needed? You can try to call DSM. and if it
> fails try
> DT (or actually other way around as we usually do).
> 
> > +             ret = pac1921_match_acpi_device(indio_dev);
> > +     else
> > +             ret = pac1921_parse_of_fw(indio_dev);

My approach is to cleanly separate the code for dt and ACPI and to
allow flexibility for future additions.

> 
> ...
> 
> > +static const struct acpi_device_id pac1921_acpi_match[] = {
> > +     { "MCHP1921" },
> > +     { }
> > +};
> > +MODULE_DEVICE_TABLE(acpi, pac1921_acpi_match);
> 
> Missing blank line here.
> 
> ...
> 
> >       .driver  = {
> >               .name = "pac1921",
> >               .pm = pm_sleep_ptr(&pac1921_pm_ops),
> >               .of_match_table = pac1921_of_match,
> > +             .acpi_match_table = pac1921_acpi_match
> 
> 
> Missing trailing comma here.
> 
> >       },
> 
> --
> With Best Regards,
> Andy Shevchenko
> 
> 

Best Regards,
Victor Duicu
diff mbox series

Patch

diff --git a/drivers/iio/adc/pac1921.c b/drivers/iio/adc/pac1921.c
index a96fae546bc1..c6c8d85e3092 100644
--- a/drivers/iio/adc/pac1921.c
+++ b/drivers/iio/adc/pac1921.c
@@ -67,6 +67,12 @@  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 in UOHMS <= INT_MAX */
+#define PAC1921_MAX_SHUNT_VALUE_OHMS		2147
+
 /*
  * Pre-computed scale factors for BUS voltage
  * format: IIO_VAL_INT_PLUS_NANO
@@ -204,6 +210,11 @@  struct pac1921_priv {
 	} scan;
 };
 
+static inline bool pac1921_shunt_is_invalid(u32 shunt_val)
+{
+	return shunt_val == 0 || shunt_val > INT_MAX;
+}
+
 /*
  * Check if first integration after configuration update has completed.
  *
@@ -781,7 +792,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;
 
@@ -792,8 +803,12 @@  static ssize_t pac1921_write_shunt_resistor(struct iio_dev *indio_dev,
 	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 +1165,60 @@  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 *rez;
+	guid_t guid;
+	char *label;
+	struct pac1921_priv *priv = iio_priv(indio_dev);
+	struct device *dev = &priv->client->dev;
+
+	guid_parse(PAC1921_DSM_UUID, &guid);
+	handle = ACPI_HANDLE(dev);
+
+	rez = acpi_evaluate_dsm(handle, &guid, 1, PAC1921_ACPI_GET_UOHMS_VALS, NULL);
+	if (!rez)
+		return dev_err_probe(dev, -EINVAL,
+				     "Could not read shunt from ACPI table\n");
+
+	priv->rshunt_uohm = rez->package.elements[0].integer.value;
+	ACPI_FREE(rez);
+
+	rez = acpi_evaluate_dsm(handle, &guid, 1, PAC1921_ACPI_GET_LABEL, NULL);
+	if (!rez)
+		return dev_err_probe(dev, -EINVAL,
+				     "Could not read label from ACPI table\n");
+
+	label = devm_kstrdup(dev, rez->package.elements->string.pointer, GFP_KERNEL);
+	if (!label)
+		return dev_err_probe(dev, -EINVAL, "Label is NULL\n");
+
+	indio_dev->label = label;
+	ACPI_FREE(rez);
+
+	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;
@@ -1176,14 +1245,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 (ACPI_HANDLE(dev))
+		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 (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);
@@ -1243,11 +1314,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,