diff mbox series

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

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

Commit Message

victor.duicu@microchip.com Nov. 8, 2024, 8:50 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:
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 | 98 +++++++++++++++++++++++++++++++++++----
 1 file changed, 89 insertions(+), 9 deletions(-)


base-commit: 6415477ba63dea58b5cbf9ddcae75f18f33c71a4

Comments

Andy Shevchenko Nov. 8, 2024, 9:23 a.m. UTC | #1
On Fri, Nov 8, 2024 at 10:52 AM <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.

read the shunt
from the ACPI

...

>  #include <linux/iio/triggered_buffer.h>
>  #include <linux/regmap.h>
>  #include <linux/units.h>
> +#include <linux/limits.h>

Please, keep them sorted.

...

> +#define PAC1921_DSM_UUID                        "f7bb9932-86ee-4516-a236-7a7a742e55cb"

Besides the need to include uuid.h and call guid_parse(), just put
this to the comment and use
GUID_INIT() instead. It will save a few CPU cycles each time we want
to read that information from the ACPI.

...

> +static inline bool pac1921_shunt_is_valid(u32 shunt_val)
> +{
> +       return shunt_val > 0 && shunt_val <= INT_MAX;
> +}

This basically is the (shunt_val - 1) & BIT(31) which can be used
inline in the caller. Hence, drop this function and use the check
inline. See also below.

...

> +       /* This check is to ensure rshunt_uohm is non-zero positive */

Please, describe better the range of the values, because currently
it's quite confusing, taking into account the unsigned type of the
variable.

> +       if (!pac1921_shunt_is_valid(rshunt_uohm))
>                 return -EINVAL;

...

> +       guid_parse(PAC1921_DSM_UUID, &guid);

No need after use of GUID_INIT().
Matteo Martelli Nov. 8, 2024, 9:56 a.m. UTC | #2
On Fri, 8 Nov 2024 11:23:18 +0200, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Fri, Nov 8, 2024 at 10:52 AM <victor.duicu@microchip.com> wrote:
> >
> > From: Victor Duicu <victor.duicu@microchip.com>
> >

...

> > +static inline bool pac1921_shunt_is_valid(u32 shunt_val)
> > +{
> > +       return shunt_val > 0 && shunt_val <= INT_MAX;
> > +}
> 
> This basically is the (shunt_val - 1) & BIT(31) which can be used
> inline in the caller. Hence, drop this function and use the check
> inline. See also below.
> 

I think the current comparison check is more clear. Also my suggestion
to move the check in a seperate function was to keep it consistent in
different places since such check can change in future and one might
change it only in one place, as it was happening during the first
iterations of this series. However I am fine to remove the function and
move the check back inline in the caller as the check is now only in two
places and it shouldn't be a big deal.

...

> -- 
> With Best Regards,
> Andy Shevchenko

Best regards,
Matteo Martelli
Andy Shevchenko Nov. 8, 2024, 1:33 p.m. UTC | #3
On Fri, Nov 8, 2024 at 11:56 AM Matteo Martelli
<matteomartelli3@gmail.com> wrote:
> On Fri, 8 Nov 2024 11:23:18 +0200, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Fri, Nov 8, 2024 at 10:52 AM <victor.duicu@microchip.com> wrote:

...

> > > +static inline bool pac1921_shunt_is_valid(u32 shunt_val)
> > > +{
> > > +       return shunt_val > 0 && shunt_val <= INT_MAX;
> > > +}
> >
> > This basically is the (shunt_val - 1) & BIT(31) which can be used
> > inline in the caller. Hence, drop this function and use the check
> > inline. See also below.
>
> I think the current comparison check is more clear. Also my suggestion
> to move the check in a seperate function was to keep it consistent in
> different places since such check can change in future and one might
> change it only in one place, as it was happening during the first
> iterations of this series. However I am fine to remove the function and
> move the check back inline in the caller as the check is now only in two
> places and it shouldn't be a big deal.

Up to you. But then drop the comment (which kinda useless) in the
caller and add in the callee, i.e. in this helper to explain the range
of valid values more clearly, ideally with reference to datasheet text
or so.
diff mbox series

Patch

diff --git a/drivers/iio/adc/pac1921.c b/drivers/iio/adc/pac1921.c
index b0f6727cfe38..e24015390503 100644
--- a/drivers/iio/adc/pac1921.c
+++ b/drivers/iio/adc/pac1921.c
@@ -14,6 +14,7 @@ 
 #include <linux/iio/triggered_buffer.h>
 #include <linux/regmap.h>
 #include <linux/units.h>
+#include <linux/limits.h>
 
 /* pac1921 registers */
 #define PAC1921_REG_GAIN_CFG		0x00
@@ -67,6 +68,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		(INT_MAX / MICRO)
+
 /*
  * 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_valid(u32 shunt_val)
+{
+	return shunt_val > 0 && shunt_val <= INT_MAX;
+}
+
 /*
  * Check if first integration after configuration update has completed.
  *
@@ -782,7 +794,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,8 +805,13 @@  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 > PAC1921_MAX_SHUNT_VALUE_OHMS)
+		return -EINVAL;
+
 	rshunt_uohm = val * MICRO + val_fract;
-	if (rshunt_uohm == 0 || rshunt_uohm > INT_MAX)
+	/* This check is to ensure rshunt_uohm is non-zero positive */
+	if (!pac1921_shunt_is_valid(rshunt_uohm))
 		return -EINVAL;
 
 	guard(mutex)(&priv->lock);
@@ -1151,6 +1168,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 *status;
+	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);
+
+	status = acpi_evaluate_dsm(handle, &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, &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 (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_valid(priv->rshunt_uohm))
+		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,