Re: [PATCH 08/10] iio: adc: axp20x_adc: Add support for AXP192

[Jonathan Cameron]

On Sat, 04 Jun 2022 12:47:38 +0100
Aidan MacDonald <aidanmacdonald.0x0@xxxxxxxxx> wrote:

> Jonathan Cameron <jic23@xxxxxxxxxx> writes:
> 
> > On Fri,  3 Jun 2022 14:57:12 +0100
> > Aidan MacDonald <aidanmacdonald.0x0@xxxxxxxxx> wrote:
> >  
> >> The AXP192 is identical to the AXP20x, except for the addition of
> >> two more GPIO ADC channels.
> >> 
> >> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@xxxxxxxxx>  
> > Hi Aidan,
> >
> > A few minor questions and comments inline.
> >
> > Thanks,
> >
> > Jonathan
> >
> > Unless I missed a previous patch adding labels to the other devices supported,
> > this is the first driver to use these.  Why do they make sense here but not
> > to add to existing supported devices?
> >
> > I don't particularly mind this addition, just looking for an explanation.
> >  
> 
> That'd be because 1d4ef9b39ebecca8 ("iio: core: Add optional symbolic
> label to a device channel") added read_label in 2020, while the AXP
> driver was introduced in 2017. I could add read_label for the other
> chips while I'm here, for consistency.

Thanks, I don't really mind either way on adding support for additional parts.

> 
> One question I have is why read_label exists when the kernel already has
> unique names for IIO channels. Why not just expose the datasheet_name to
> userspace from the IIO core instead of making drivers do it?

In general, datasheet_name refers to the name of the pin on a datasheet for this
device, whereas label can refer to how it is used.
There are dt bindings to allow a per channel label letting a driver (where it
makes sense) provide them for each individual ADC channel.
(e.g. the ad7768-1 driver does this).

On other devices they come from entirely different sources such as the hardcoded
choices in hid-sensor-custom-intel-hinge.

I vaguely recall that we've talked in the past about exposing datasheet name directly
but for many devices it's not that useful (the user doesn't care if a channel is
aux channel 1 or 7, but rather what it is wired up to).

At the moment this driver just exposes all channels rather than having
per channel bindings, so we don't have the option to use labeling in the device
tree to assign the names.   If it's particularly useful to you to have labels
that are datasheet names that's fine.

Jonathan