On Tue, Jun 6, 2023 at 4:54 PM Herve Codina <herve.codina@xxxxxxxxxxx> wrote:
> On Sat, 3 Jun 2023 21:26:19 +0300
> andy.shevchenko@xxxxxxxxx wrote:
> > Tue, May 23, 2023 at 05:12:21PM +0200, Herve Codina kirjoitti:
...
> > > + int max;
> > > + int min;
> >
> > Wondering if there is already a data type for the ranges (like linear_range.h,
> > but not sure it's applicable here).
>
> Seems not applicable here.
> - IIO does not use linear_range or something similar. It just uses simple int.
> - ASoC does not use linear_range or something similar. It just uses simple long.
>
> So, I keep the simple int min and max.
Sure.
...
> > > + return 1; /* The value changed */
> >
> > Perhaps this 1 needs a definition?
>
> Yes but to be coherent, in ASoC code, many places need to be changed too
> in order to use the newly defined value.
> I don't think these modifications should be part of this series.
Yes, we are all for consistency.
...
> > > + for (i = 0; i < iio_aux->num_chans; i++) {
> > > + iio_aux_chan = iio_aux->chans + i;
> > > +
> > > + ret = of_property_read_string_index(np, "io-channel-names", i,
> > > + &iio_aux_chan->name);
> > > + if (ret < 0) {
> > > + dev_err(iio_aux->dev, "%pOF: failed to read io-channel-names[%d]\n", np, i);
> > > + return ret;
> >
> > Ditto.
> Will be changed in next iteration.
> >
> > > + }
> >
> > > + tmp = 0;
> > > + of_property_read_u32_index(np, "snd-control-invert-range", i, &tmp);
> >
> > > + iio_aux_chan->is_invert_range = tmp;
> >
> > You can use this variable directly.
>
> Not sure, is_invert_range is a bool and tmp is a u32.
Ah, I see.
> In previous iteration, I wrote
> iio_aux_chan->is_invert_range = !!tmp;
>
> > > + }
> >
> > Btw, can you avoid using OF APIs? It's better to have device property/fwnode
> > API to be used from day 1.
>
> Hum, this comment was raised in the previous iteration
> https://lore.kernel.org/linux-kernel/20230501162456.3448c494@jic23-huawei/
>
> I didn't find any equivalent to of_property_read_u32_index() in the
> device_property_read_*() function family.
> I mean I did find anything available to get a value from an array using an index.
This is done by reading the entire array at once and then parsing as
you wish in the code, device_property_read_u32_array() is for that.
> In the previous iteration it was concluded that keeping OF APIs in this series
> seemed "reasonable".
Maybe, but consider the above.
--
With Best Regards,
Andy Shevchenko
