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.
