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.5On 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);
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);
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);
>
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);
> >
>
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 ...
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 > > ...
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);
> >
>
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.
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
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
© 2016 - 2025 Red Hat, Inc.