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?
