linux-kernel - Re: [PATCH v1 1/2] iio: adc: ti-adc128s052: add support for adc121s021

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAFXKEHZ8zDEXLT57BD5Dg1mTN-Gj0Z7uhxX5Xx=XH0wFeAhe6g@mail.gmail.com>
Date: Thu, 26 Jun 2025 23:33:25 +0200
From: Lothar Rubusch <l.rubusch@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Matti Vaittinen <mazziesaccount@...il.com>, dlechner@...libre.com, nuno.sa@...log.com, 
	andy@...nel.org, linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Sukrut Bellary <sbellary@...libre.com>
Subject: Re: [PATCH v1 1/2] iio: adc: ti-adc128s052: add support for adc121s021

Hi guys,

I absolutely agree and won't send further versions of this.

Hi Sukrut, if you find some possibility to use it, great. If not, nevermind.

Thank you all, for the feedback. One small question below.

On Thu, Jun 26, 2025 at 8:28 PM Jonathan Cameron <jic23@...nel.org> wrote:
>
> On Thu, 26 Jun 2025 08:24:41 +0300
> Matti Vaittinen <mazziesaccount@...il.com> wrote:
>
> > Hi Lothar,
> >
> > On 25/06/2025 20:02, Lothar Rubusch wrote:
> > > Add support for the single channel variant(s) of this ADC.
> > >
> > > Signed-off-by: Lothar Rubusch <l.rubusch@...il.com>
> >
> > Thanks for this addition. In principle, this looks good to me but I am
> > afraid there is another colliding series being worked on:
> >
> > https://lore.kernel.org/all/20250614091504.575685-3-sbellary@baylibre.com/
> >
> > Maybe you can align the effort with Sukrut?
> +CC Sukrut.
>

Hi Matti,

Perhaps just one little question here to you. You used the regulator
name "vdd" where others
before used "vref". At the end, this seems to be pretty free,
depending on how it is set in the
DT or how you name it in the DT (in my case it was "5v0", but I wanted
to keep the convention,
if so).

So, my question is, is there a naming convention what to take for a,
say, default
regulator naming or fixed 5V regulator?

Best,
L



> >
> > What I specifically like (and think is the right thing to do) in
> > Sukrut's series is replacing the 'adc122s021_channels' -array with
> > individual structures. In my opinion the array is just unnecessary
> > complexity and individual structures are simpler.
> >
> > Other than that, this looks good to me.
>
>
> Sukrut, perhaps you could add this to the end of your series, rebased
> to those changes?  Would save a synchronization step for your v5 (and
> later if needed)
>
> No problem if not, but I agree with Matti that we should take your
> series first.
>
> Jonathan
>
>
> >
> > Yours,
> >       -- Matti
> >
> >
> > > ---
> > >   drivers/iio/adc/ti-adc128s052.c | 17 ++++++++++++++++-
> > >   1 file changed, 16 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/iio/adc/ti-adc128s052.c b/drivers/iio/adc/ti-adc128s052.c
> > > index 1b46a8155803..cf271c39e663 100644
> > > --- a/drivers/iio/adc/ti-adc128s052.c
> > > +++ b/drivers/iio/adc/ti-adc128s052.c
> > > @@ -7,6 +7,7 @@
> > >    * https://www.ti.com/lit/ds/symlink/adc128s052.pdf
> > >    * https://www.ti.com/lit/ds/symlink/adc122s021.pdf
> > >    * https://www.ti.com/lit/ds/symlink/adc124s021.pdf
> > > + * https://www.ti.com/lit/ds/symlink/adc121s021.pdf
> > >    */
> > >
> > >   #include <linux/cleanup.h>
> > > @@ -110,6 +111,10 @@ static const struct iio_chan_spec adc128s052_channels[] = {
> > >     ADC128_VOLTAGE_CHANNEL(7),
> > >   };
> > >
> > > +static const struct iio_chan_spec adc121s021_channels[] = {
> > > +   ADC128_VOLTAGE_CHANNEL(0),
> > > +};
> > > +
> > >   static const struct iio_chan_spec adc122s021_channels[] = {
> > >     ADC128_VOLTAGE_CHANNEL(0),
> > >     ADC128_VOLTAGE_CHANNEL(1),
> > > @@ -143,6 +148,10 @@ static const struct adc128_configuration adc128_config[] = {
> > >             .refname = "vdd",
> > >             .other_regulators = &bd79104_regulators,
> > >             .num_other_regulators = 1,
> > > +   }, {
> > > +           .channels = adc121s021_channels,
> > > +           .num_channels = ARRAY_SIZE(adc121s021_channels),
> > > +           .refname = "vref",
> > >     },
> > >   };
> >
> > I'd love seeing this array split to individual structs.
> >
> > >
> > > @@ -207,7 +216,10 @@ static const struct of_device_id adc128_of_match[] = {
> > >     { .compatible = "ti,adc124s051", .data = &adc128_config[2] },
> > >     { .compatible = "ti,adc124s101", .data = &adc128_config[2] },
> > >     { .compatible = "rohm,bd79104", .data = &adc128_config[3] },
> > > -   { }
> > > +   { .compatible = "ti,adc121s021", .data = &adc128_config[4] },
> > > +   { .compatible = "ti,adc121s051", .data = &adc128_config[4] },
> > > +   { .compatible = "ti,adc121s101", .data = &adc128_config[4] },
> > > +   { },
> > >   };
> > >   MODULE_DEVICE_TABLE(of, adc128_of_match);
> > >
> > > @@ -220,6 +232,9 @@ static const struct spi_device_id adc128_id[] = {
> > >     { "adc124s051", (kernel_ulong_t)&adc128_config[2] },
> > >     { "adc124s101", (kernel_ulong_t)&adc128_config[2] },
> > >     { "bd79104", (kernel_ulong_t)&adc128_config[3] },
> > > +   { "adc121s021", (kernel_ulong_t)&adc128_config[4] },
> > > +   { "adc121s051", (kernel_ulong_t)&adc128_config[4] },
> > > +   { "adc121s101", (kernel_ulong_t)&adc128_config[4] },
> > >     { }
> > >   };
> > >   MODULE_DEVICE_TABLE(spi, adc128_id);
> >
>