| Message ID | 20210202064541.2335915-1-gwendal@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio: sx9310: Support ACPI property | expand |
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>
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>
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?
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.
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.
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.
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.
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.
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)))
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;
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(-)