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

[Aidan MacDonald]

Jonathan Cameron <jic23@xxxxxxxxxx> writes:

> 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

Thanks for the explanation, makes sense that "gpio0_v" is probably not
all that useful. I was thinking mainly of channels like battery charge
current where the channel usage is fixed by hardware. The labels aren't
terribly useful to me so I'll just leave them out, I'd rather not bother
with adding dt labels.

Regards,
Aidan