Re: [PATCH 3/3] iio: adc: adc128s052: Support ROHM BD7910[0,1,2,3]

[Matti Vaittinen]

On 14/08/2025 18:01, David Lechner wrote:
> On 8/14/25 3:35 AM, Matti Vaittinen wrote:
> > The ROHM BD79100, BD79101, BD79102, BD79103 are very similar ADCs as the
> > ROHM BD79104. The BD79100 has only 1 channel. BD79101 has 2 channels and
> > the BD79102 has 4 channels. Both BD79103 and BD79104 have 4 channels,
> > and, based on the data sheets, they seem identical from the software
> > point-of-view.
> >
> > Signed-off-by: Matti Vaittinen <mazziesaccount@xxxxxxxxx>
> >
> > ---
>
> One small suggestion. With that:
>
> Reviewed-by: David Lechner <dlechner@xxxxxxxxxxxx>
>
> > ---
> >   drivers/iio/adc/ti-adc128s052.c | 36 +++++++++++++++++++++++++++++++++
> >   1 file changed, 36 insertions(+)
> >
> > diff --git a/drivers/iio/adc/ti-adc128s052.c b/drivers/iio/adc/ti-adc128s052.c
> > index 81153253529e..2f2ed438cf4e 100644
> > --- a/drivers/iio/adc/ti-adc128s052.c
> > +++ b/drivers/iio/adc/ti-adc128s052.c
> > @@ -122,6 +122,10 @@ static const struct iio_chan_spec adc124s021_channels[] = {
> >   	ADC128_VOLTAGE_CHANNEL(3),
> >   };
> >   
> > +static const struct iio_chan_spec bd79100_channels[] = {
>
> Even though the driver doesn't support it yet, there is a
> adc121s021 [1] so would be nice to use that instead of bd79100
> to keep the naming consistent.

I have to disagree on this one. For people who don't use the TI ADCs, 
the TI numbering does not bring any clarity. Furthermore, I don't like 
preparing for the support added somewhere in the future - because future 
is uncertain. It could be this TI's variant never gets added here. If 
this series gets merged now, then there is only one IC using this 
channel spec - the bd79100. Naming it after unsupported TI's IC would be 
plain confusing.

In my opinion, structs should get either named based on the IC model 
which is using them first - or based on the functionality. And actually, 
when the design of the IC is not too obscure, I would prefer naming 
based on the functionality, which should help others to re-use the 
driver. Hence, I wouldn't object someone re-naming all these channel 
structs based on functionality though - for example something like:

static const struct iio_chan_spec simple_adc_channels1 {}
static const struct iio_chan_spec simple_adc_channels2 {}
static const struct iio_chan_spec simple_adc_channels4 {}
static const struct iio_chan_spec simple_adc_channels8 {}

This which should be clear(ish) for developer no matter which of the 
supported IC(s) were used. But if we stick with the IC based naming, 
then we should use naming by supported IC.

> [1]: https://www.ti.com/product/ADC121C021
>
> > +	ADC128_VOLTAGE_CHANNEL(0),
> > +};
> > +

Yours,
	-- Matti