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@xxxxxxxxxxxx>
> ---
> 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)))
