On Mon, Feb 8, 2021 at 6:53 PM Stephen Boyd <swboyd@xxxxxxxxxxxx> 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@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
> > > > > 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.
