Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230523151223.109551-1-herve.codina@bootlin.com>
 <20230523151223.109551-8-herve.codina@bootlin.com> <ZHuFywIrTnEFpX6e@surfacebook>
 <20230606155404.28ada064@bootlin.com>
In-Reply-To: <20230606155404.28ada064@bootlin.com>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Tue, 6 Jun 2023 17:34:22 +0300
Message-ID: <CAHp75Vd00N8z7kgTb=WTZHJW3XhsKbLfhTTKPjnCvKUSfL+xDQ@mail.gmail.com>
Subject: Re: [PATCH v2 7/9] ASoC: codecs: Add support for the generic IIO
 auxiliary devices
To:     Herve Codina <herve.codina@bootlin.com>
Cc:     Liam Girdwood <lgirdwood@gmail.com>,
        Mark Brown <broonie@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>,
        Jonathan Cameron <jic23@kernel.org>,
        Lars-Peter Clausen <lars@metafoo.de>,
        Jaroslav Kysela <perex@perex.cz>,
        Takashi Iwai <tiwai@suse.com>,
        Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>,
        alsa-devel@alsa-project.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-iio@vger.kernel.org,
        Christophe Leroy <christophe.leroy@csgroup.eu>,
        Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Jun 6, 2023 at 4:54=E2=80=AFPM Herve Codina <herve.codina@bootlin.c=
om> wrote:
> On Sat, 3 Jun 2023 21:26:19 +0300
> andy.shevchenko@gmail.com 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_r=
ange.h,
> > but not sure it's applicable here).
>
> Seems not applicable here.
>  - IIO does not use linear_range or something similar. It just uses simpl=
e int.
>  - ASoC does not use linear_range or something similar. It just uses simp=
le 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 =3D 0; i < iio_aux->num_chans; i++) {
> > > +           iio_aux_chan =3D iio_aux->chans + i;
> > > +
> > > +           ret =3D of_property_read_string_index(np, "io-channel-nam=
es", i,
> > > +                                               &iio_aux_chan->name);
> > > +           if (ret < 0) {
> > > +                   dev_err(iio_aux->dev, "%pOF: failed to read io-ch=
annel-names[%d]\n", np, i);
> > > +                   return ret;
> >
> > Ditto.
> Will be changed in next iteration.
> >
> > > +           }
> >
> > > +           tmp =3D 0;
> > > +           of_property_read_u32_index(np, "snd-control-invert-range"=
, i, &tmp);
> >
> > > +           iio_aux_chan->is_invert_range =3D 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 =3D !!tmp;
>
> > > +   }
> >
> > Btw, can you avoid using OF APIs? It's better to have device property/f=
wnode
> > 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-huaw=
ei/
>
> 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 a=
n 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 s=
eries
> seemed "reasonable".

Maybe, but consider the above.

--
With Best Regards,
Andy Shevchenko