Re: [PATCH] iio: sx9310: Support ACPI property

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.



[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux