[PATCH] iio: sx9310: Support ACPI property

[PATCH] iio: sx9310: Support ACPI property

[Gwendal Grignou]

Use device_property_read_... to support both device tree and ACPI
bindings.

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

Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
---
 drivers/iio/proximity/sx9310.c | 36 ++++++++++++++++++++--------------
 1 file changed, 21 insertions(+), 15 deletions(-)

diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c
index 37fd0b65a0140..1a8a441c9774d 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>
@@ -1213,31 +1214,36 @@ 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;
+	int ret, count;
 	u32 combined[SX9310_NUM_CHANNELS] = { 4, 4, 4, 4 };
 	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++) {
+		count = device_property_read_u32_array(dev,
+				"semtech,combined-sensors", NULL, 0);
+		if (count > 0 && count <= ARRAY_SIZE(combined))
+			ret = device_property_read_u32_array(dev,
+					"semtech,combined-sensors", combined,
+					count);
+		else
+			ret = -EINVAL;
+		if (ret)
+			count = ARRAY_SIZE(combined);
+
+		for (i = 0; i < count; i++) {
 			if (combined[i] <= SX9310_NUM_CHANNELS)
 				comb_mask |= BIT(combined[i]);
 		}
@@ -1254,7 +1260,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;
 
@@ -1278,7 +1284,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);
@@ -1288,7 +1294,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);
@@ -1301,7 +1307,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;
 
@@ -1337,7 +1343,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;
-- 
2.30.0.365.g02bc693789-goog

Re: [PATCH] iio: sx9310: Support ACPI property

[Stephen Boyd]

Quoting Gwendal Grignou (2021-02-01 22:45:41)
> Use device_property_read_... to support both device tree and ACPI
> bindings.
> 
> Add support for variable array per documentation
> ("iio/proximity/semtech,sx9310.yaml").
> 
> Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
> ---
>  drivers/iio/proximity/sx9310.c | 36 ++++++++++++++++++++--------------
>  1 file changed, 21 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c
> index 37fd0b65a0140..1a8a441c9774d 100644
> --- a/drivers/iio/proximity/sx9310.c
> +++ b/drivers/iio/proximity/sx9310.c
> @@ -1213,31 +1214,36 @@ 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;
> +       int ret, count;
>         u32 combined[SX9310_NUM_CHANNELS] = { 4, 4, 4, 4 };
>         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++) {
> +               count = device_property_read_u32_array(dev,
> +                               "semtech,combined-sensors", NULL, 0);
> +               if (count > 0 && count <= ARRAY_SIZE(combined))
> +                       ret = device_property_read_u32_array(dev,
> +                                       "semtech,combined-sensors", combined,
> +                                       count);
> +               else
> +                       ret = -EINVAL;
> +               if (ret)
> +                       count = ARRAY_SIZE(combined);

I wish this could be written simpler. Unfortunately there isn't any sort
of for_each_device_property() iterator macro like we have with
of_property_for_each_u32(). Or device_property_read_u32_array() can be
OK if the length of the property doesn't exceed the size of the
'combined' array?

> +
> +               for (i = 0; i < count; i++) {
>                         if (combined[i] <= SX9310_NUM_CHANNELS)
>                                 comb_mask |= BIT(combined[i]);
>                 }

Otherwise

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

Re: [PATCH] iio: sx9310: Support ACPI property

[Gwendal Grignou]

On Tue, Feb 2, 2021 at 11:17 AM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Gwendal Grignou (2021-02-01 22:45:41)
> > Use device_property_read_... to support both device tree and ACPI
> > bindings.
> >
> > Add support for variable array per documentation
> > ("iio/proximity/semtech,sx9310.yaml").
> >
> > Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
> > ---
> >  drivers/iio/proximity/sx9310.c | 36 ++++++++++++++++++++--------------
> >  1 file changed, 21 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c
> > index 37fd0b65a0140..1a8a441c9774d 100644
> > --- a/drivers/iio/proximity/sx9310.c
> > +++ b/drivers/iio/proximity/sx9310.c
> > @@ -1213,31 +1214,36 @@ 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;
> > +       int ret, count;
> >         u32 combined[SX9310_NUM_CHANNELS] = { 4, 4, 4, 4 };
> >         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++) {
> > +               count = device_property_read_u32_array(dev,
> > +                               "semtech,combined-sensors", NULL, 0);
> > +               if (count > 0 && count <= ARRAY_SIZE(combined))
> > +                       ret = device_property_read_u32_array(dev,
> > +                                       "semtech,combined-sensors", combined,
> > +                                       count);
> > +               else
> > +                       ret = -EINVAL;
> > +               if (ret)
> > +                       count = ARRAY_SIZE(combined);
>
> I wish this could be written simpler. Unfortunately there isn't any sort
> of for_each_device_property() iterator macro like we have with
> of_property_for_each_u32(). Or device_property_read_u32_array() can be
> OK if the length of the property doesn't exceed the size of the
> 'combined' array?
device_property_read_u32_array(...,nval) calls
acpi_data_get_property(..., nval) in ACPI case.
If nval > obj->package.count, then -EOVERFLOW is returned.
Therefore count can not be to SX9310_NUM_CHANNELS, in case
combined-sensors is only 3 entries or less.

This method of asking first for the number of element and a second
time for the values is already used at different places in the kernel:
drivers/power/supply/gpio-charger.c : see init_charge_current_limit()
or  madera_get_variable_u32_array insound/soc/codecs/madera.c.

However, it could use device_property_count_u32(...), which is more
readable than device_property_count_u32(..., NULL,0).

Gwendal.


>
> > +
> > +               for (i = 0; i < count; i++) {
> >                         if (combined[i] <= SX9310_NUM_CHANNELS)
> >                                 comb_mask |= BIT(combined[i]);
> >                 }
>
> Otherwise
>
> Reviewed-by: Stephen Boyd <swboyd@chromium.org>

Re: [PATCH] iio: sx9310: Support ACPI property

[Stephen Boyd]

Quoting Gwendal Grignou (2021-02-05 13:49:12)
> On Tue, Feb 2, 2021 at 11:17 AM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Quoting Gwendal Grignou (2021-02-01 22:45:41)
> > > diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c
> > > index 37fd0b65a0140..1a8a441c9774d 100644
> > > --- a/drivers/iio/proximity/sx9310.c
> > > +++ b/drivers/iio/proximity/sx9310.c
> > > @@ -1213,31 +1214,36 @@ 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;
> > > +       int ret, count;
> > >         u32 combined[SX9310_NUM_CHANNELS] = { 4, 4, 4, 4 };
> > >         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++) {
> > > +               count = device_property_read_u32_array(dev,
> > > +                               "semtech,combined-sensors", NULL, 0);
> > > +               if (count > 0 && count <= ARRAY_SIZE(combined))
> > > +                       ret = device_property_read_u32_array(dev,
> > > +                                       "semtech,combined-sensors", combined,
> > > +                                       count);
> > > +               else
> > > +                       ret = -EINVAL;
> > > +               if (ret)
> > > +                       count = ARRAY_SIZE(combined);
> >
> > I wish this could be written simpler. Unfortunately there isn't any sort
> > of for_each_device_property() iterator macro like we have with
> > of_property_for_each_u32(). Or device_property_read_u32_array() can be
> > OK if the length of the property doesn't exceed the size of the
> > 'combined' array?
>
> device_property_read_u32_array(...,nval) calls
> acpi_data_get_property(..., nval) in ACPI case.
> If nval > obj->package.count, then -EOVERFLOW is returned.
> Therefore count can not be to SX9310_NUM_CHANNELS, in case
> combined-sensors is only 3 entries or less.
> 
> This method of asking first for the number of element and a second
> time for the values is already used at different places in the kernel:
> drivers/power/supply/gpio-charger.c : see init_charge_current_limit()
> or  madera_get_variable_u32_array insound/soc/codecs/madera.c.

Sure it's used but that doesn't really mean it's a good pattern to copy.
If the number is more than 4 I'd say we should ignore it and move on,
but if it's less than 4 is an error returned?

> 
> However, it could use device_property_count_u32(...), which is more
> readable than device_property_count_u32(..., NULL,0).
> 

How about

  ret = device_property_read_u32_array(dev, "semtech,combined-sensors", combined, ARRAY_SIZE(combined));
  if (ret)
  	break; /* Must have overflowed or not existed; ignore */

  for (i = 0; i < ARRAY_SIZE(combined); i++)

and then it should work as before?

Re: [PATCH] iio: sx9310: Support ACPI property

[Gwendal Grignou]

On Fri, Feb 5, 2021 at 5:41 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Gwendal Grignou (2021-02-05 13:49:12)
> > On Tue, Feb 2, 2021 at 11:17 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > >
> > > Quoting Gwendal Grignou (2021-02-01 22:45:41)
> > > > diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c
> > > > index 37fd0b65a0140..1a8a441c9774d 100644
> > > > --- a/drivers/iio/proximity/sx9310.c
> > > > +++ b/drivers/iio/proximity/sx9310.c
> > > > @@ -1213,31 +1214,36 @@ 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;
> > > > +       int ret, count;
> > > >         u32 combined[SX9310_NUM_CHANNELS] = { 4, 4, 4, 4 };
> > > >         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++) {
> > > > +               count = device_property_read_u32_array(dev,
> > > > +                               "semtech,combined-sensors", NULL, 0);
> > > > +               if (count > 0 && count <= ARRAY_SIZE(combined))
> > > > +                       ret = device_property_read_u32_array(dev,
> > > > +                                       "semtech,combined-sensors", combined,
> > > > +                                       count);
> > > > +               else
> > > > +                       ret = -EINVAL;
> > > > +               if (ret)
> > > > +                       count = ARRAY_SIZE(combined);
> > >
> > > I wish this could be written simpler. Unfortunately there isn't any sort
> > > of for_each_device_property() iterator macro like we have with
> > > of_property_for_each_u32(). Or device_property_read_u32_array() can be
> > > OK if the length of the property doesn't exceed the size of the
> > > 'combined' array?
> >
> > device_property_read_u32_array(...,nval) calls
> > acpi_data_get_property(..., nval) in ACPI case.
> > If nval > obj->package.count, then -EOVERFLOW is returned.
> > Therefore count can not be to SX9310_NUM_CHANNELS, in case
> > combined-sensors is only 3 entries or less.
> >
> > This method of asking first for the number of element and a second
> > time for the values is already used at different places in the kernel:
> > drivers/power/supply/gpio-charger.c : see init_charge_current_limit()
> > or  madera_get_variable_u32_array insound/soc/codecs/madera.c.
>
> Sure it's used but that doesn't really mean it's a good pattern to copy.
> If the number is more than 4 I'd say we should ignore it and move on,
> but if it's less than 4 is an error returned?
>
> >
> > However, it could use device_property_count_u32(...), which is more
> > readable than device_property_count_u32(..., NULL,0).
> >
>
> How about
>
>   ret = device_property_read_u32_array(dev, "semtech,combined-sensors", combined, ARRAY_SIZE(combined));
>   if (ret)
>         break; /* Must have overflowed or not existed; ignore */
>
>   for (i = 0; i < ARRAY_SIZE(combined); i++)
>
> and then it should work as before?
It will not work:
If the DTD has a valid array of only 3 elements (for instance  [CS0,
CS1, CS2] as in
         Package (0x02)
         {
             "semtech,combined-sensors",
             Package (0x03)
             {
                 Zero,
                 One,
                 0x02
             }
         },
)

device_property_read_u32_array(...., 4) will return -EOVERFLOW and we
will use the default in the driver, which we do not want.

Gwendal.

Re: [PATCH] iio: sx9310: Support ACPI property

[Stephen Boyd]

Quoting Gwendal Grignou (2021-02-08 18:36:19)
> On Fri, Feb 5, 2021 at 5:41 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Quoting Gwendal Grignou (2021-02-05 13:49:12)
> > > On Tue, Feb 2, 2021 at 11:17 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > > >
> > > > Quoting Gwendal Grignou (2021-02-01 22:45:41)
> > > > > diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c
> > > > > index 37fd0b65a0140..1a8a441c9774d 100644
> > > > > --- a/drivers/iio/proximity/sx9310.c
> > > > > +++ b/drivers/iio/proximity/sx9310.c
> > > This method of asking first for the number of element and a second
> > > time for the values is already used at different places in the kernel:
> > > drivers/power/supply/gpio-charger.c : see init_charge_current_limit()
> > > or  madera_get_variable_u32_array insound/soc/codecs/madera.c.
> >
> > Sure it's used but that doesn't really mean it's a good pattern to copy.
> > If the number is more than 4 I'd say we should ignore it and move on,
> > but if it's less than 4 is an error returned?
> >
> > >
> > > However, it could use device_property_count_u32(...), which is more
> > > readable than device_property_count_u32(..., NULL,0).
> > >
> >
> > How about
> >
> >   ret = device_property_read_u32_array(dev, "semtech,combined-sensors", combined, ARRAY_SIZE(combined));
> >   if (ret)
> >         break; /* Must have overflowed or not existed; ignore */
> >
> >   for (i = 0; i < ARRAY_SIZE(combined); i++)
> >
> > and then it should work as before?
> It will not work:
> If the DTD has a valid array of only 3 elements (for instance  [CS0,
> CS1, CS2] as in
>          Package (0x02)
>          {
>              "semtech,combined-sensors",
>              Package (0x03)
>              {
>                  Zero,
>                  One,
>                  0x02
>              }
>          },
> )
> 
> device_property_read_u32_array(...., 4) will return -EOVERFLOW and we
> will use the default in the driver, which we do not want.
> 

Isn't that a bug in the ACPI property reading code? 3 doesn't overflow 4
so I'm lost why it would return an error code to indicate it can't fit
the whole property into an array that is one size larger.

Re: [PATCH] iio: sx9310: Support ACPI property

[Stephen Boyd]

Quoting Stephen Boyd (2021-02-08 18:38:20)
> Quoting Gwendal Grignou (2021-02-08 18:36:19)
> > On Fri, Feb 5, 2021 at 5:41 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > >
> > > Quoting Gwendal Grignou (2021-02-05 13:49:12)
> > > > On Tue, Feb 2, 2021 at 11:17 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > > > >
> > > > > Quoting Gwendal Grignou (2021-02-01 22:45:41)
> > > > > > diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c
> > > > > > index 37fd0b65a0140..1a8a441c9774d 100644
> > > > > > --- a/drivers/iio/proximity/sx9310.c
> > > > > > +++ b/drivers/iio/proximity/sx9310.c
> > > > This method of asking first for the number of element and a second
> > > > time for the values is already used at different places in the kernel:
> > > > drivers/power/supply/gpio-charger.c : see init_charge_current_limit()
> > > > or  madera_get_variable_u32_array insound/soc/codecs/madera.c.
> > >
> > > Sure it's used but that doesn't really mean it's a good pattern to copy.
> > > If the number is more than 4 I'd say we should ignore it and move on,
> > > but if it's less than 4 is an error returned?
> > >
> > > >
> > > > However, it could use device_property_count_u32(...), which is more
> > > > readable than device_property_count_u32(..., NULL,0).
> > > >
> > >
> > > How about
> > >
> > >   ret = device_property_read_u32_array(dev, "semtech,combined-sensors", combined, ARRAY_SIZE(combined));
> > >   if (ret)
> > >         break; /* Must have overflowed or not existed; ignore */
> > >
> > >   for (i = 0; i < ARRAY_SIZE(combined); i++)
> > >
> > > and then it should work as before?
> > It will not work:
> > If the DTD has a valid array of only 3 elements (for instance  [CS0,
> > CS1, CS2] as in
> >          Package (0x02)
> >          {
> >              "semtech,combined-sensors",
> >              Package (0x03)
> >              {
> >                  Zero,
> >                  One,
> >                  0x02
> >              }
> >          },
> > )
> > 
> > device_property_read_u32_array(...., 4) will return -EOVERFLOW and we
> > will use the default in the driver, which we do not want.
> > 
> 
> Isn't that a bug in the ACPI property reading code? 3 doesn't overflow 4
> so I'm lost why it would return an error code to indicate it can't fit
> the whole property into an array that is one size larger.

Or it's a bug in the driver because it's assuming that it can read the
DT property out even when it is only length 1 when the property read
should be variable length. Can you split this into two and fix the
underlying problem with the read of the array not matching the length of
the property? I think it needs to be
of_property_read_variable_u32_array() with 1 and 4 for min and max.

Re: [PATCH] iio: sx9310: Support ACPI property

[Gwendal Grignou]

On Mon, Feb 8, 2021 at 6:38 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Gwendal Grignou (2021-02-08 18:36:19)
> > On Fri, Feb 5, 2021 at 5:41 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > >
> > > Quoting Gwendal Grignou (2021-02-05 13:49:12)
> > > > On Tue, Feb 2, 2021 at 11:17 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > > > >
> > > > > Quoting Gwendal Grignou (2021-02-01 22:45:41)
> > > > > > diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c
> > > > > > index 37fd0b65a0140..1a8a441c9774d 100644
> > > > > > --- a/drivers/iio/proximity/sx9310.c
> > > > > > +++ b/drivers/iio/proximity/sx9310.c
> > > > This method of asking first for the number of element and a second
> > > > time for the values is already used at different places in the kernel:
> > > > drivers/power/supply/gpio-charger.c : see init_charge_current_limit()
> > > > or  madera_get_variable_u32_array insound/soc/codecs/madera.c.
> > >
> > > Sure it's used but that doesn't really mean it's a good pattern to copy.
> > > If the number is more than 4 I'd say we should ignore it and move on,
> > > but if it's less than 4 is an error returned?
> > >
> > > >
> > > > However, it could use device_property_count_u32(...), which is more
> > > > readable than device_property_count_u32(..., NULL,0).
> > > >
> > >
> > > How about
> > >
> > >   ret = device_property_read_u32_array(dev, "semtech,combined-sensors", combined, ARRAY_SIZE(combined));
> > >   if (ret)
> > >         break; /* Must have overflowed or not existed; ignore */
> > >
> > >   for (i = 0; i < ARRAY_SIZE(combined); i++)
> > >
> > > and then it should work as before?
> > It will not work:
> > If the DTD has a valid array of only 3 elements (for instance  [CS0,
> > CS1, CS2] as in
> >          Package (0x02)
> >          {
> >              "semtech,combined-sensors",
> >              Package (0x03)
> >              {
> >                  Zero,
> >                  One,
> >                  0x02
> >              }
> >          },
> > )
> >
> > device_property_read_u32_array(...., 4) will return -EOVERFLOW and we
> > will use the default in the driver, which we do not want.
> >
>
> Isn't that a bug in the ACPI property reading code? 3 doesn't overflow 4
> so I'm lost why it would return an error code to indicate it can't fit
> the whole property into an array that is one size larger.

This is how the code has been written: from the documentation (see
commit b31384fa5de37a):
+ * of_property_read_u64_array - Find and read an array of 64 bit
integers [same for 32 bit arrays]
+ * from a property.
+ *
[...]
+ * @sz:                number of array elements to read
+ *
+ * Search for a property in a device node and read 64-bit value(s) from
+ * it. Returns 0 on success,[...], and -EOVERFLOW if the
+ * property data isn't large enough.

If the given array is too big, an error is returned. This makes sense
since the elements that can not be read will have undefined values.
Once again, the method of using count=device_property_count_u32() then
device_property_read_u32_array(..., count) is used throughout the
kernel.

Gwendal.

Re: [PATCH] iio: sx9310: Support ACPI property

[Gwendal Grignou]

On Mon, Feb 8, 2021 at 6:53 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Stephen Boyd (2021-02-08 18:38:20)
> > Quoting Gwendal Grignou (2021-02-08 18:36:19)
> > > On Fri, Feb 5, 2021 at 5:41 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > > >
> > > > Quoting Gwendal Grignou (2021-02-05 13:49:12)
> > > > > On Tue, Feb 2, 2021 at 11:17 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > > > > >
> > > > > > Quoting Gwendal Grignou (2021-02-01 22:45:41)
> > > > > > > diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c
> > > > > > > index 37fd0b65a0140..1a8a441c9774d 100644
> > > > > > > --- a/drivers/iio/proximity/sx9310.c
> > > > > > > +++ b/drivers/iio/proximity/sx9310.c
> > > > > This method of asking first for the number of element and a second
> > > > > time for the values is already used at different places in the kernel:
> > > > > drivers/power/supply/gpio-charger.c : see init_charge_current_limit()
> > > > > or  madera_get_variable_u32_array insound/soc/codecs/madera.c.
> > > >
> > > > Sure it's used but that doesn't really mean it's a good pattern to copy.
> > > > If the number is more than 4 I'd say we should ignore it and move on,
> > > > but if it's less than 4 is an error returned?
> > > >
> > > > >
> > > > > However, it could use device_property_count_u32(...), which is more
> > > > > readable than device_property_count_u32(..., NULL,0).
> > > > >
> > > >
> > > > How about
> > > >
> > > >   ret = device_property_read_u32_array(dev, "semtech,combined-sensors", combined, ARRAY_SIZE(combined));
> > > >   if (ret)
> > > >         break; /* Must have overflowed or not existed; ignore */
> > > >
> > > >   for (i = 0; i < ARRAY_SIZE(combined); i++)
> > > >
> > > > and then it should work as before?
> > > It will not work:
> > > If the DTD has a valid array of only 3 elements (for instance  [CS0,
> > > CS1, CS2] as in
> > >          Package (0x02)
> > >          {
> > >              "semtech,combined-sensors",
> > >              Package (0x03)
> > >              {
> > >                  Zero,
> > >                  One,
> > >                  0x02
> > >              }
> > >          },
> > > )
> > >
> > > device_property_read_u32_array(...., 4) will return -EOVERFLOW and we
> > > will use the default in the driver, which we do not want.
> > >
> >
> > Isn't that a bug in the ACPI property reading code? 3 doesn't overflow 4
> > so I'm lost why it would return an error code to indicate it can't fit
> > the whole property into an array that is one size larger.
>
> Or it's a bug in the driver because it's assuming that it can read the
> DT property out even when it is only length 1 when the property read
> should be variable length. Can you split this into two and fix the
> underlying problem with the read of the array not matching the length of
> the property? I think it needs to be
> of_property_read_variable_u32_array() with 1 and 4 for min and max.
Splitting the patch in 2 does not make sense given
of_property_read_variable_u32_array has no equivalent in the
device_property_read_ realm.
That function introduced in the first patch would be removed in the
second patch.
I will update the commit message to indicate this patch takes care of
the "semtech,combined-sensors" variable array for both DT and ACPI.

Gwendal.

Re: [PATCH] iio: sx9310: Support ACPI property

[Stephen Boyd]

Quoting Gwendal Grignou (2021-02-08 20:07:19)
> On Mon, Feb 8, 2021 at 6:53 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Quoting Stephen Boyd (2021-02-08 18:38:20)
> > >
> > > Isn't that a bug in the ACPI property reading code? 3 doesn't overflow 4
> > > so I'm lost why it would return an error code to indicate it can't fit
> > > the whole property into an array that is one size larger.
> >
> > Or it's a bug in the driver because it's assuming that it can read the
> > DT property out even when it is only length 1 when the property read
> > should be variable length. Can you split this into two and fix the
> > underlying problem with the read of the array not matching the length of
> > the property? I think it needs to be
> > of_property_read_variable_u32_array() with 1 and 4 for min and max.
> Splitting the patch in 2 does not make sense given
> of_property_read_variable_u32_array has no equivalent in the
> device_property_read_ realm.
> That function introduced in the first patch would be removed in the
> second patch.
> I will update the commit message to indicate this patch takes care of
> the "semtech,combined-sensors" variable array for both DT and ACPI.
> 

I suggest we apply this patch and send it back to stable trees. I didn't
notice because I was setting combined-sensors to 3, and that is the
default value and it was getting removed from the default register
values all the time, even though the read of the DT property was
failing. With this change we'll always read the array and if the length
is less than zero or more than the size of the array we'll break early
and get out of there. Otherwise we'll continue on and build a bitmap out
of the values in the array to compare and set the reg_def->def bits to.
Importantly we don't mask out the settings from the default register
values until we successfully read the property.

After this change you can introduce the device_property_read_*() APIs to
add the new feature. But this is a fix that needs to be backported.

----8<-----
diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c
index 6a04959df35e..6ba063e51af5 100644
--- a/drivers/iio/proximity/sx9310.c
+++ b/drivers/iio/proximity/sx9310.c
@@ -1236,15 +1236,17 @@ sx9310_get_default_reg(struct sx9310_data *data, int i,
 			reg_def->def |= SX9310_REG_PROX_CTRL2_SHIELDEN_GROUND;
 		}
 
+		ret = of_property_read_variable_u32_array(np, "semtech,combined-sensors",
+					   combined, 0, ARRAY_SIZE(combined));
+		if (ret < 0)
+			break;
+
 		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)
+			if (combined[i] < SX9310_NUM_CHANNELS)
 				comb_mask |= BIT(combined[i]);
 		}
 
-		comb_mask &= 0xf;
 		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)))