Re: [PATCH v2 7/9] ASoC: codecs: Add support for the generic IIO auxiliary devices

[Andy Shevchenko]

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