RE: [PATCH v5 3/7] iio: Add support for DA9150 GPADC

[Opensource [Adam Thomson]]

On January 20, 2015 20:49, Jonathan Cameron wrote:

> >>>>> +#define DA9150_GPADC_CHANNEL_PROCESSED(_id, _hw_id, _type,
> >> _ext_name)
> >>>> 	\
> >>>>> +	DA9150_GPADC_CHANNEL(_id, _hw_id, _type,			\
> >>>>> +			     BIT(IIO_CHAN_INFO_PROCESSED), _ext_name)
> >>>>> +
> >>>>> +/* Supported channels */
> >>>>> +static const struct iio_chan_spec da9150_gpadc_channels[] = {
> >>>>> +	DA9150_GPADC_CHANNEL_PROCESSED(GPIOA, GPIOA_6V, IIO_VOLTAGE,
> >>>> "GPIOA"),
> >>>>> +	DA9150_GPADC_CHANNEL_PROCESSED(GPIOB, GPIOB_6V, IIO_VOLTAGE,
> >>>> "GPIOB"),
> >>>>> +	DA9150_GPADC_CHANNEL_PROCESSED(GPIOC, GPIOC_6V, IIO_VOLTAGE,
> >>>> "GPIOC"),
> >>>>> +	DA9150_GPADC_CHANNEL_PROCESSED(GPIOD, GPIOD_6V, IIO_VOLTAGE,
> >>>> "GPIOD"),
> >>>> I'm not sure some of these really deserve extended names.  Those are usually
> >>>> reserved for naming strange internal adc channels etc.  These first 4 are
> >>>> presumably just for input pins?  Those should just be channels 0..3
> >>>> On another note, unless you want really weird sysfs attribute names, the
> >>>> extended names want to be lowercase.
> >>>>
> >>>
> >>> I'd prefer to keep the names because the input pins are muxed with GPIOs of the
> >>> chip, so thought it sensible to show that this is the case. Am happy to change
> >>> to lower-case to follow convention.
> >> Hmm.  It's a bit of an oddity as the point of the naming
> >> is about the uses, not which pins they are on.  If we exposed the
> >> 'datasheet_name' parameter directly rather than just using it internally
> >> I'd suggest relying on that - but clearly you want it to be apparent
> >> in the interface.  Whether that is useful is the question I'd raise
> >> here (and is the reason datasheet_name is not exposed.
> >>
> >> The obvious question is does userspace care?  Answer is probably not.
> >>
> >> It cares what is being measured but this is about what pins it is
> >> on and doesn't provide any information on what is connected to them.
> >>
> >
> > Surely it helps when using sysfs to access whatever is connected to one of
> > those pins if we label the pin with something meaningful? If say you have a
> > device connected to GPIC of the charger IC, it's easier to work out which ADC
> > channel you need to access through sysfs if the naming is as I have now, rather
> > than some arbitrary number which doesn't necessarily tally to the channel in the
> > datasheet. You'd then need to look at the code and work out which channel
> number
> > GPIOC actually was. Or am I just missing something here? :)
> Not really for the vast majority of users.  They tend not to have a detailed
> board layout in front of them.  It's more interesting if you know 'what' they
> are measuring (hence we do use these names when that is true - such as
> internal voltage measurements).
> 
> The numbers almost never tally with the datasheet, but then datasheet numbering
> has a habit of being inconsistent as well!

Can't say I agree that this won't be useful/informative to many users, but don't
want to make this a sticking point so will update.