diff mbox series

[v4] iio: sx9310: Support ACPI property

Message ID 20210217061829.1199408-1-gwendal@chromium.org (mailing list archive)
State New
Headers show
Series [v4] iio: sx9310: Support ACPI property | expand

Commit Message

Gwendal Grignou Feb. 17, 2021, 6:18 a.m. UTC
Use device_property_read_... to support both device tree and ACPI
bindings.
Given |semtech,combined-sensors| is a variable array, use
device_property_count to get the length of the array first before
reading it to avoid underflow errors.

Add support for variable array per documentation
("iio/proximity/semtech,sx9310.yaml").

Fixes: 5b19ca2c78a0 ("iio: sx9310: Set various settings from DT")

Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
---
 Changes since v3:
   Add "Fixes" comment in commit message
   Fix the logic set COMBMODE register: when we know the DT property is
   missing or incorrect, exit as soon as possible.
 Changes since v2:
   Add comment how the default array is used.
   Add comment in commit message to indicate this CL fix an issue with
     existing use of of_property_read_u32_array() when reading a variale
     length array.
 Changes since v1:
   Use device_property_count_u32(...) instead of device_property_read_u32_array(..., NULL, 0)

 drivers/iio/proximity/sx9310.c | 54 +++++++++++++++++++++-------------
 1 file changed, 34 insertions(+), 20 deletions(-)

Comments

Stephen Boyd Feb. 25, 2021, 7:57 p.m. UTC | #1
Quoting Gwendal Grignou (2021-02-16 22:18:29)
> Use device_property_read_... to support both device tree and ACPI
> bindings.
> Given |semtech,combined-sensors| is a variable array, use
> device_property_count to get the length of the array first before
> reading it to avoid underflow errors.
> 
> Add support for variable array per documentation
> ("iio/proximity/semtech,sx9310.yaml").
> 
> Fixes: 5b19ca2c78a0 ("iio: sx9310: Set various settings from DT")
> 

Usually there isn't a newline here and Fixes is attached to the SoB.

> Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
> ---
>  Changes since v3:
>    Add "Fixes" comment in commit message
>    Fix the logic set COMBMODE register: when we know the DT property is
>    missing or incorrect, exit as soon as possible.

I'd still prefer it to be two patches, one for the fix of array property
parsing to send back to stable trees and one to add the ACPI parsing
support, but I'm not the maintainer here so this isn't really up to me.

>  Changes since v2:
>    Add comment how the default array is used.
>    Add comment in commit message to indicate this CL fix an issue with
>      existing use of of_property_read_u32_array() when reading a variale
>      length array.
>  Changes since v1:
>    Use device_property_count_u32(...) instead of device_property_read_u32_array(..., NULL, 0)
> 
>  drivers/iio/proximity/sx9310.c | 54 +++++++++++++++++++++-------------
>  1 file changed, 34 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c
> index 6a04959df35e5..77d2c9e102842 100644
> --- a/drivers/iio/proximity/sx9310.c
> +++ b/drivers/iio/proximity/sx9310.c
> @@ -20,6 +20,7 @@
>  #include <linux/mod_devicetable.h>
>  #include <linux/module.h>
>  #include <linux/pm.h>
> +#include <linux/property.h>
>  #include <linux/regmap.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/slab.h>
> @@ -1215,36 +1216,49 @@ static int sx9310_init_compensation(struct iio_dev *indio_dev)
>  }
>  
>  static const struct sx9310_reg_default *
> -sx9310_get_default_reg(struct sx9310_data *data, int i,
> +sx9310_get_default_reg(struct device *dev, int i,
>                        struct sx9310_reg_default *reg_def)
>  {
> -       int ret;
> -       const struct device_node *np = data->client->dev.of_node;
> -       u32 combined[SX9310_NUM_CHANNELS] = { 4, 4, 4, 4 };
> +       int ret, count;
> +       u32 combined[SX9310_NUM_CHANNELS];
>         unsigned long comb_mask = 0;
>         const char *res;
>         u32 start = 0, raw = 0, pos = 0;
>  
>         memcpy(reg_def, &sx9310_default_regs[i], sizeof(*reg_def));
> -       if (!np)
> -               return reg_def;
> -
>         switch (reg_def->reg) {
>         case SX9310_REG_PROX_CTRL2:
> -               if (of_property_read_bool(np, "semtech,cs0-ground")) {
> +               if (device_property_read_bool(dev, "semtech,cs0-ground")) {
>                         reg_def->def &= ~SX9310_REG_PROX_CTRL2_SHIELDEN_MASK;
>                         reg_def->def |= SX9310_REG_PROX_CTRL2_SHIELDEN_GROUND;
>                 }
>  
> -               reg_def->def &= ~SX9310_REG_PROX_CTRL2_COMBMODE_MASK;
> -               of_property_read_u32_array(np, "semtech,combined-sensors",
> -                                          combined, ARRAY_SIZE(combined));
> -               for (i = 0; i < ARRAY_SIZE(combined); i++) {
> -                       if (combined[i] <= SX9310_NUM_CHANNELS)
> -                               comb_mask |= BIT(combined[i]);
> +               count = device_property_count_u32(dev, "semtech,combined-sensors");
> +               if (count > 0 && count <= ARRAY_SIZE(combined)) {
> +                       ret = device_property_read_u32_array(dev,
> +                                       "semtech,combined-sensors", combined,
> +                                       count);
> +               } else {
> +                       /*
> +                        * Either the property does not exist in the DT, the
> +                        * number of entries is incorrect.
> +                        */
> +                       break;
> +               }
> +               if (ret) {
> +                       /* We could not read the array (invalid DT). */
> +                       break;
>                 }

Wouldn't this be shorter?

		count = device_property_count_u32(dev, "semtech,combined-sensors");
		if (count < 0 || count > ARRAY_SIZE(combined))
			break;
		
		ret = device_property_read_u32_array(dev,
						     "semtech,combined-sensors",
						     combined, count);
		if (ret)
			break;


If the 'break' logic is off-putting then I suggest moving it to another
function and using 'return' to indicate early returns from the code
flow.

>  
> -               comb_mask &= 0xf;
> +               for (i = 0; i < count; i++) {
> +                       if (combined[i] >= SX9310_NUM_CHANNELS) {
> +                               /* Invalid sensor (invalid DT). */
> +                               break;

Don't think we need to do this. We have DT validation for this instead
so that we don't have to carry code in the kernel to validate something
that can be checked at build time. Hopefully ACPI has something similar?

> +                       }
> +                       comb_mask |= BIT(combined[i]);
> +               }
> +
> +               reg_def->def &= ~SX9310_REG_PROX_CTRL2_COMBMODE_MASK;
>                 if (comb_mask == (BIT(3) | BIT(2) | BIT(1) | BIT(0)))
>                         reg_def->def |= SX9310_REG_PROX_CTRL2_COMBMODE_CS0_CS1_CS2_CS3;
>                 else if (comb_mask == (BIT(1) | BIT(2)))
diff mbox series

Patch

diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c
index 6a04959df35e5..77d2c9e102842 100644
--- a/drivers/iio/proximity/sx9310.c
+++ b/drivers/iio/proximity/sx9310.c
@@ -20,6 +20,7 @@ 
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
 #include <linux/pm.h>
+#include <linux/property.h>
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
 #include <linux/slab.h>
@@ -1215,36 +1216,49 @@  static int sx9310_init_compensation(struct iio_dev *indio_dev)
 }
 
 static const struct sx9310_reg_default *
-sx9310_get_default_reg(struct sx9310_data *data, int i,
+sx9310_get_default_reg(struct device *dev, int i,
 		       struct sx9310_reg_default *reg_def)
 {
-	int ret;
-	const struct device_node *np = data->client->dev.of_node;
-	u32 combined[SX9310_NUM_CHANNELS] = { 4, 4, 4, 4 };
+	int ret, count;
+	u32 combined[SX9310_NUM_CHANNELS];
 	unsigned long comb_mask = 0;
 	const char *res;
 	u32 start = 0, raw = 0, pos = 0;
 
 	memcpy(reg_def, &sx9310_default_regs[i], sizeof(*reg_def));
-	if (!np)
-		return reg_def;
-
 	switch (reg_def->reg) {
 	case SX9310_REG_PROX_CTRL2:
-		if (of_property_read_bool(np, "semtech,cs0-ground")) {
+		if (device_property_read_bool(dev, "semtech,cs0-ground")) {
 			reg_def->def &= ~SX9310_REG_PROX_CTRL2_SHIELDEN_MASK;
 			reg_def->def |= SX9310_REG_PROX_CTRL2_SHIELDEN_GROUND;
 		}
 
-		reg_def->def &= ~SX9310_REG_PROX_CTRL2_COMBMODE_MASK;
-		of_property_read_u32_array(np, "semtech,combined-sensors",
-					   combined, ARRAY_SIZE(combined));
-		for (i = 0; i < ARRAY_SIZE(combined); i++) {
-			if (combined[i] <= SX9310_NUM_CHANNELS)
-				comb_mask |= BIT(combined[i]);
+		count = device_property_count_u32(dev, "semtech,combined-sensors");
+		if (count > 0 && count <= ARRAY_SIZE(combined)) {
+			ret = device_property_read_u32_array(dev,
+					"semtech,combined-sensors", combined,
+					count);
+		} else {
+			/*
+			 * Either the property does not exist in the DT, the
+			 * number of entries is incorrect.
+			 */
+			break;
+		}
+		if (ret) {
+			/* We could not read the array (invalid DT). */
+			break;
 		}
 
-		comb_mask &= 0xf;
+		for (i = 0; i < count; i++) {
+			if (combined[i] >= SX9310_NUM_CHANNELS) {
+				/* Invalid sensor (invalid DT). */
+				break;
+			}
+			comb_mask |= BIT(combined[i]);
+		}
+
+		reg_def->def &= ~SX9310_REG_PROX_CTRL2_COMBMODE_MASK;
 		if (comb_mask == (BIT(3) | BIT(2) | BIT(1) | BIT(0)))
 			reg_def->def |= SX9310_REG_PROX_CTRL2_COMBMODE_CS0_CS1_CS2_CS3;
 		else if (comb_mask == (BIT(1) | BIT(2)))
@@ -1256,7 +1270,7 @@  sx9310_get_default_reg(struct sx9310_data *data, int i,
 
 		break;
 	case SX9310_REG_PROX_CTRL4:
-		ret = of_property_read_string(np, "semtech,resolution", &res);
+		ret = device_property_read_string(dev, "semtech,resolution", &res);
 		if (ret)
 			break;
 
@@ -1280,7 +1294,7 @@  sx9310_get_default_reg(struct sx9310_data *data, int i,
 
 		break;
 	case SX9310_REG_PROX_CTRL5:
-		ret = of_property_read_u32(np, "semtech,startup-sensor", &start);
+		ret = device_property_read_u32(dev, "semtech,startup-sensor", &start);
 		if (ret) {
 			start = FIELD_GET(SX9310_REG_PROX_CTRL5_STARTUPSENS_MASK,
 					  reg_def->def);
@@ -1290,7 +1304,7 @@  sx9310_get_default_reg(struct sx9310_data *data, int i,
 		reg_def->def |= FIELD_PREP(SX9310_REG_PROX_CTRL5_STARTUPSENS_MASK,
 					   start);
 
-		ret = of_property_read_u32(np, "semtech,proxraw-strength", &raw);
+		ret = device_property_read_u32(dev, "semtech,proxraw-strength", &raw);
 		if (ret) {
 			raw = FIELD_GET(SX9310_REG_PROX_CTRL5_RAWFILT_MASK,
 					reg_def->def);
@@ -1303,7 +1317,7 @@  sx9310_get_default_reg(struct sx9310_data *data, int i,
 					   raw);
 		break;
 	case SX9310_REG_PROX_CTRL7:
-		ret = of_property_read_u32(np, "semtech,avg-pos-strength", &pos);
+		ret = device_property_read_u32(dev, "semtech,avg-pos-strength", &pos);
 		if (ret)
 			break;
 
@@ -1339,7 +1353,7 @@  static int sx9310_init_device(struct iio_dev *indio_dev)
 
 	/* Program some sane defaults. */
 	for (i = 0; i < ARRAY_SIZE(sx9310_default_regs); i++) {
-		initval = sx9310_get_default_reg(data, i, &tmp);
+		initval = sx9310_get_default_reg(&indio_dev->dev, i, &tmp);
 		ret = regmap_write(data->regmap, initval->reg, initval->def);
 		if (ret)
 			return ret;