[PATCH v1 0/2] iio: adc: ti-adc128s052: add support for TI's ADC121s021

[PATCH v1 0/2] iio: adc: ti-adc128s052: add support for TI's ADC121s021

[Lothar Rubusch]

- Add support for ADC121s021 and related (single channel).

This adds support for the 1-channel variants ADC121s021/051/101.

I'm not certain whether this patch is truly necessary, or if there's a
more general solution for single-channel ADCs that would make this patch
obsolete. If such a solution exists, please let me know. Hence, this is
perhaps more of a question than a formal request to accept the patch.

Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
Lothar Rubusch (2):
  iio: adc: ti-adc128s052: add support for adc121s021
  iio: adc: ti-adc128s052: replace literal by unit expression

 drivers/iio/adc/ti-adc128s052.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)


base-commit: b57cb7c47e31244bef6612f271c5dc390f761e17
-- 
2.39.5

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

[Lothar Rubusch]

Add support for the single channel variant(s) of this ADC.

Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
 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",
 	},
 };
 
@@ -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);
-- 
2.39.5

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

[Andy Shevchenko]

On Wed, Jun 25, 2025 at 05:02:17PM +0000, Lothar Rubusch wrote:
> Add support for the single channel variant(s) of this ADC.

...

> -	{ }

> +	{ },

Stray change.

-- 
With Best Regards,
Andy Shevchenko

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

[Matti Vaittinen]

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@gmail.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?

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.

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);

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

[Jonathan Cameron]

On Thu, 26 Jun 2025 08:24:41 +0300
Matti Vaittinen <mazziesaccount@gmail.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@gmail.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.

> 
> 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);  
> 

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

[Lothar Rubusch]

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@kernel.org> wrote:
>
> On Thu, 26 Jun 2025 08:24:41 +0300
> Matti Vaittinen <mazziesaccount@gmail.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@gmail.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);
> >
>

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

[David Lechner]

On 6/26/25 4:33 PM, Lothar Rubusch wrote:
> Hi guys,
> 

...

> 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?
> 

I don't think there is a naming convention for supplies other than making
it match the pin name from the datasheet.

If we were to try to come up with some standard naming convention though,
I would not include the voltage value in the name. Rather, the properties
should be named after the function that it does, like vref-supply for an
external reference voltage, vio-supply for I/O pin voltage supply,
power-supply for a whole-chip or main supply, analog-supply and digital-supply
for chips that don't have a whole-chip supply but rather split the
analog and digital circuitry. These are the most common ones that I have
seen on ADCs.

The fact that the TI chips in this driver use "vref-supply" doesn't really
make sense in the DT bindings. V_REF is an internal signal in the ADC.
In other words, it's kind of abusing the binding to specify the reference
voltage without actually saying that the chip also has power supplies.

Chips like adc128s052 should really have va-supply for the power supply
connected to the V_A pin that also serves as the reference voltage and
vd-supply for the supply connected to the V_D pin for the digital I/O
supply. And adc121s021 would only have va-supply because there is no
separate V_D pin for a separate I/O supply.

But there are lot's of ADCs already incorrectly using vref-supply like
this, so not sure if it is worth trying to fix them or not. But if we
wanted to fix it for these TI chips, I would suggest to deprecate the
vref-supply and add the actual supplies to the DT bindings and implement
a fallback in the driver to check for vref-supply if the other supplies
are not given so that we don't break existing dtbs.

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

[Jonathan Cameron]

On Fri, 27 Jun 2025 11:27:22 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On 6/26/25 4:33 PM, Lothar Rubusch wrote:
> > Hi guys,
> >   
> 
> ...
> 
> > 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?
> >   
> 
> I don't think there is a naming convention for supplies other than making
> it match the pin name from the datasheet.
> 
> If we were to try to come up with some standard naming convention though,
> I would not include the voltage value in the name. Rather, the properties
> should be named after the function that it does, like vref-supply for an
> external reference voltage, vio-supply for I/O pin voltage supply,
> power-supply for a whole-chip or main supply, analog-supply and digital-supply
> for chips that don't have a whole-chip supply but rather split the
> analog and digital circuitry. These are the most common ones that I have
> seen on ADCs.
> 
> The fact that the TI chips in this driver use "vref-supply" doesn't really
> make sense in the DT bindings. V_REF is an internal signal in the ADC.
> In other words, it's kind of abusing the binding to specify the reference
> voltage without actually saying that the chip also has power supplies.
> 
> Chips like adc128s052 should really have va-supply for the power supply
> connected to the V_A pin that also serves as the reference voltage and
> vd-supply for the supply connected to the V_D pin for the digital I/O
> supply. And adc121s021 would only have va-supply because there is no
> separate V_D pin for a separate I/O supply.
> 
> But there are lot's of ADCs already incorrectly using vref-supply like
> this, so not sure if it is worth trying to fix them or not. But if we
> wanted to fix it for these TI chips, I would suggest to deprecate the
> vref-supply and add the actual supplies to the DT bindings and implement
> a fallback in the driver to check for vref-supply if the other supplies
> are not given so that we don't break existing dtbs.

Agreed. vref-supply should only be used if it's an external pin labeled
vref.  (which is fairly common).  Where it's labeled V_A like here we
should name it after that.  Fix would be as you suggest with the fallback
to cover DT bindings in use.

Jonathan

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

[Sukrut Bellary]

On Thu, Jun 26, 2025 at 07:28:02PM +0100, Jonathan Cameron wrote:
> On Thu, 26 Jun 2025 08:24:41 +0300
> Matti Vaittinen <mazziesaccount@gmail.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@gmail.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.
> 
> > 
> > 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
>
Sure, I will add these adc121s0xx to the end of my v5.
Thanks.

> 
> > 
> > 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);  
> > 
> 

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

[Lothar Rubusch]

On Sun, Jun 29, 2025 at 2:00 AM Sukrut Bellary <sbellary@baylibre.com> wrote:
>
> On Thu, Jun 26, 2025 at 07:28:02PM +0100, Jonathan Cameron wrote:
> > On Thu, 26 Jun 2025 08:24:41 +0300
> > Matti Vaittinen <mazziesaccount@gmail.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@gmail.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.
> >
> > >
> > > 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
> >
> Sure, I will add these adc121s0xx to the end of my v5.
> Thanks.
>

Hi Sukrut,

Since David Lechner still asked for ordering the TI ADC vs Rohm
entries a bit, and complained about the missing binding entry: Shall I
fix this rapidly and send in another version?

Best,
L

...

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

[Sukrut Bellary]

On Sun, Jun 29, 2025 at 06:13:54PM +0200, Lothar Rubusch wrote:
> On Sun, Jun 29, 2025 at 2:00 AM Sukrut Bellary <sbellary@baylibre.com> wrote:
> >
> > On Thu, Jun 26, 2025 at 07:28:02PM +0100, Jonathan Cameron wrote:
> > > On Thu, 26 Jun 2025 08:24:41 +0300
> > > Matti Vaittinen <mazziesaccount@gmail.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@gmail.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.
> > >
> > > >
> > > > 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
> > >
> > Sure, I will add these adc121s0xx to the end of my v5.
> > Thanks.
> >
> 
> Hi Sukrut,
> 
> Since David Lechner still asked for ordering the TI ADC vs Rohm
> entries a bit, and complained about the missing binding entry: Shall I
> fix this rapidly and send in another version?
> 
The ordering of TI and Rohm has been addressed in my series v4 [1].
I will arrange ti,adc121xx in order in v5.

[1]. https://lore.kernel.org/all/20250614091504.575685-4-sbellary@baylibre.com/

> Best,
> L
> 
> ...

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

[David Lechner]

On 6/25/25 12:02 PM, Lothar Rubusch wrote:
> Add support for the single channel variant(s) of this ADC.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---
>  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",
>  	},


Let's try to keep these sorted rather than having rohm in the middle
of ti chips.

Same applies to the rest of the changes in this patch as well (other
than adc121s021_channels[] which look to be in a logical order already).


>  };
>  
> @@ -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] },

We'll need another patch to add these compatible strings to
Documentation/devicetree/bindings/iio/adc/ti,adc128s052.yaml

> +	{ },
>  };
>  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);

[PATCH v1 2/2] iio: adc: ti-adc128s052: replace literal by unit expression

[Lothar Rubusch]

Replace the literal number 1000 by MILLI from linux/units.h

Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
 drivers/iio/adc/ti-adc128s052.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/adc/ti-adc128s052.c b/drivers/iio/adc/ti-adc128s052.c
index cf271c39e663..67bc7fbd52bc 100644
--- a/drivers/iio/adc/ti-adc128s052.c
+++ b/drivers/iio/adc/ti-adc128s052.c
@@ -18,6 +18,7 @@
 #include <linux/property.h>
 #include <linux/regulator/consumer.h>
 #include <linux/spi/spi.h>
+#include <linux/units.h>
 
 struct adc128_configuration {
 	const struct iio_chan_spec	*channels;
@@ -189,7 +190,7 @@ static int adc128_probe(struct spi_device *spi)
 				     "failed to read '%s' voltage",
 				     config->refname);
 
-	adc->vref_mv = ret / 1000;
+	adc->vref_mv = ret / MILLI;
 
 	if (config->num_other_regulators) {
 		ret = devm_regulator_bulk_get_enable(&spi->dev,
-- 
2.39.5

Re: [PATCH v1 2/2] iio: adc: ti-adc128s052: replace literal by unit expression

[Andy Shevchenko]

On Wed, Jun 25, 2025 at 05:02:18PM +0000, Lothar Rubusch wrote:
> Replace the literal number 1000 by MILLI from linux/units.h

Yeah, but with this units (mV, uV, mA, uA) it would probably better to have
dedicated definitions, like we have for Hz, s and maybe something else
(bytes?).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 2/2] iio: adc: ti-adc128s052: replace literal by unit expression

[Jonathan Cameron]

On Wed, 25 Jun 2025 20:57:37 +0300
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Wed, Jun 25, 2025 at 05:02:18PM +0000, Lothar Rubusch wrote:
> > Replace the literal number 1000 by MILLI from linux/units.h  
> 
> Yeah, but with this units (mV, uV, mA, uA) it would probably better to have
> dedicated definitions, like we have for Hz, s and maybe something else
> (bytes?).

I'm not that keen on us getting loads unit specific conversion factors.

From a code readability point of view (given my wish to avoid lots
and lots of unit specific defines) I'd favour using
MICRO / MILLI here and let the compiler resolve that to 1000

> 

Re: [PATCH v1 2/2] iio: adc: ti-adc128s052: replace literal by unit expression

[Matti Vaittinen]

On 25/06/2025 20:02, Lothar Rubusch wrote:
> Replace the literal number 1000 by MILLI from linux/units.h
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---
>   drivers/iio/adc/ti-adc128s052.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/ti-adc128s052.c b/drivers/iio/adc/ti-adc128s052.c
> index cf271c39e663..67bc7fbd52bc 100644
> --- a/drivers/iio/adc/ti-adc128s052.c
> +++ b/drivers/iio/adc/ti-adc128s052.c
> @@ -18,6 +18,7 @@
>   #include <linux/property.h>
>   #include <linux/regulator/consumer.h>
>   #include <linux/spi/spi.h>
> +#include <linux/units.h>
>   
>   struct adc128_configuration {
>   	const struct iio_chan_spec	*channels;
> @@ -189,7 +190,7 @@ static int adc128_probe(struct spi_device *spi)
>   				     "failed to read '%s' voltage",
>   				     config->refname);
>   
> -	adc->vref_mv = ret / 1000;
> +	adc->vref_mv = ret / MILLI;

This makes no sense to me. What does it mean we divide the micro volts 
by 'milli'? It is clear when we divide micro volts by 1000 that we get 
milli volts. Also, the mv suffix in variable makes units clear already, 
division by MILLI just obfuscates things. I'd keep it as 1000.

I would just drop this unless Jonathan disagrees.

Yours,
	-- Matti

Re: [PATCH v1 2/2] iio: adc: ti-adc128s052: replace literal by unit expression

[Jonathan Cameron]

On Thu, 26 Jun 2025 08:07:19 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> On 25/06/2025 20:02, Lothar Rubusch wrote:
> > Replace the literal number 1000 by MILLI from linux/units.h
> > 
> > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > ---
> >   drivers/iio/adc/ti-adc128s052.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/iio/adc/ti-adc128s052.c b/drivers/iio/adc/ti-adc128s052.c
> > index cf271c39e663..67bc7fbd52bc 100644
> > --- a/drivers/iio/adc/ti-adc128s052.c
> > +++ b/drivers/iio/adc/ti-adc128s052.c
> > @@ -18,6 +18,7 @@
> >   #include <linux/property.h>
> >   #include <linux/regulator/consumer.h>
> >   #include <linux/spi/spi.h>
> > +#include <linux/units.h>
> >   
> >   struct adc128_configuration {
> >   	const struct iio_chan_spec	*channels;
> > @@ -189,7 +190,7 @@ static int adc128_probe(struct spi_device *spi)
> >   				     "failed to read '%s' voltage",
> >   				     config->refname);
> >   
> > -	adc->vref_mv = ret / 1000;
> > +	adc->vref_mv = ret / MILLI;  
> 
> This makes no sense to me. What does it mean we divide the micro volts 
> by 'milli'? It is clear when we divide micro volts by 1000 that we get 
> milli volts. Also, the mv suffix in variable makes units clear already, 
> division by MILLI just obfuscates things. I'd keep it as 1000.
> 

ret / (MICRO / MILLI) would be more descriptive as indicates
we are converting form microvolts to milivolts.
 
> I would just drop this unless Jonathan disagrees.
> 
> Yours,
> 	-- Matti