On Fri, Feb 5, 2021 at 5:41 PM Stephen Boyd <swboyd@xxxxxxxxxxxx> wrote: > > Quoting Gwendal Grignou (2021-02-05 13:49:12) > > On Tue, Feb 2, 2021 at 11:17 AM Stephen Boyd <swboyd@xxxxxxxxxxxx> 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.