[RFC] iio: adc: qcom-spmi-vadc: Propagate fw node name/label to extend_name

Commit Message

Marijn Suijten Nov. 6, 2022, 7:30 p.m. UTC

Much like the ADC5 driver iio_chan_spec::extend_name has to be set for
friendly/useful names to show up in sysfs, allowing users to correlate
readout values with the corresponding probe.  This name is read from
firmware, taking both the node name and - if set - node label into
account.  This is particularly useful for custom thermistors being
attached to otherwise-generically-named GPIOs.

Signed-off-by: Marijn Suijten <[e-mail]>

---

This RFC may seem a bit controversial as there are multiple patches
going around in DT-land changing how nodes are labeled [1] (or
introducing new ones: [2]), seemingly to appease binding conventions
without considering how the driver propagates them to IIO (and in turn
what userspace sees in sysfs).  I hope we can put together the right
conventions with this RFC.

Before getting started, note that ADC5 provides this DT/FW node
name/label in *both* extend_name *and* datasheet_name;
adc5_channels::datasheet_name provided by the macros remains *unread*
(except for a non-null check).
Since the names hardcoded in the driver seem to be somewhat
"datasheet"-y, and the names in DT typically take the form of a more
friendly "<device>-therm" indicating where the thermistor (or voltage
probe) is located on the board or attached to, I have opted to persist
the original use of vadc_channels::datasheet_name in
iio_chan_spec::datasheet_name, and only propagate the data from DT/FW
into extend_name.
(We should likely rename vadc_channel_prop::datasheet_name to
extend_name to this end.)

Back when I submitted patches for pm6125 [3] (utilizing ADC5)
4f47a236a23d ("iio: adc: qcom-spmi-adc5: convert to device properties")
didn't yet land, and these patches use the node name to convey a
useful/friendly name (again, the string literals in ADC5 are unused).
fwnode_get_name() however includes the `@xx` reg suffix, making for an
unpleasant reading experience in sysfs.

With all that context in mind, I feel like we should answer the
following questions:

1. Should we propagate names from DT/FW at all?
2. If so, how should a node be represented in DT?  Should it use generic
   node names (which we might not want to use anyway considering the
   `@xx` suffix highlighted above) or labels exclusively?
3. If only labels are going to be used in conjunction with generic node
   names, should ADC5 be changed to ignore the node name?
4. If a label (or node name) is not set, do we fall back to
   datasheet_name hardcoded in the driver?
5. What do we use for datasheet_name vs extend_name?
6. Any other vadc drivers that need the same treatment, when we come to
   a resolution?

[1]: https://lore.kernel.org/linux-arm-msm/[e-mail]/T/#u
[2]: https://lore.kernel.org/linux-arm-msm/[e-mail]/
[3]: https://lore.kernel.org/linux-arm-msm/[e-mail]/T/#u

Thanks for considering this!
- Marijn

 drivers/iio/adc/qcom-spmi-vadc.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

--
2.38.1

Comments

Marijn Suijten Nov. 6, 2022, 8:24 p.m. UTC | #1

Adding Krzysztof to CC for the DT bindings discussion.

On 2022-11-06 20:30:18, Marijn Suijten wrote:
> Much like the ADC5 driver iio_chan_spec::extend_name has to be set for
> friendly/useful names to show up in sysfs, allowing users to correlate
> readout values with the corresponding probe.  This name is read from
> firmware, taking both the node name and - if set - node label into
> account.  This is particularly useful for custom thermistors being
> attached to otherwise-generically-named GPIOs.
> 
> Signed-off-by: Marijn Suijten <[e-mail]>
> 
> ---
> 
> This RFC may seem a bit controversial as there are multiple patches
> going around in DT-land changing how nodes are labeled [1] (or
> introducing new ones: [2]), seemingly to appease binding conventions
> without considering how the driver propagates them to IIO (and in turn
> what userspace sees in sysfs).  I hope we can put together the right
> conventions with this RFC.
> 
> Before getting started, note that ADC5 provides this DT/FW node
> name/label in *both* extend_name *and* datasheet_name;
> adc5_channels::datasheet_name provided by the macros remains *unread*
> (except for a non-null check).
> Since the names hardcoded in the driver seem to be somewhat
> "datasheet"-y, and the names in DT typically take the form of a more
> friendly "<device>-therm" indicating where the thermistor (or voltage
> probe) is located on the board or attached to, I have opted to persist
> the original use of vadc_channels::datasheet_name in
> iio_chan_spec::datasheet_name, and only propagate the data from DT/FW
> into extend_name.
> (We should likely rename vadc_channel_prop::datasheet_name to
> extend_name to this end.)
> 
> Back when I submitted patches for pm6125 [3] (utilizing ADC5)
> 4f47a236a23d ("iio: adc: qcom-spmi-adc5: convert to device properties")
> didn't yet land, and these patches use the node name to convey a
> useful/friendly name (again, the string literals in ADC5 are unused).
> fwnode_get_name() however includes the `@xx` reg suffix, making for an
> unpleasant reading experience in sysfs.
> 
> With all that context in mind, I feel like we should answer the
> following questions:
> 
> 1. Should we propagate names from DT/FW at all?
> 2. If so, how should a node be represented in DT?  Should it use generic
>    node names (which we might not want to use anyway considering the
>    `@xx` suffix highlighted above) or labels exclusively?
> 3. If only labels are going to be used in conjunction with generic node
>    names, should ADC5 be changed to ignore the node name?
> 4. If a label (or node name) is not set, do we fall back to
>    datasheet_name hardcoded in the driver?
> 5. What do we use for datasheet_name vs extend_name?
> 6. Any other vadc drivers that need the same treatment, when we come to
>    a resolution?
> 
> [1]: https://lore.kernel.org/linux-arm-msm/[e-mail]/T/#u
> [2]: https://lore.kernel.org/linux-arm-msm/[e-mail]/
> [3]: https://lore.kernel.org/linux-arm-msm/[e-mail]/T/#u
> 
> Thanks for considering this!
> - Marijn
> 
>  drivers/iio/adc/qcom-spmi-vadc.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/qcom-spmi-vadc.c b/drivers/iio/adc/qcom-spmi-vadc.c
> index bcff0f62b70e..8c6c7fa13cfe 100644
> --- a/drivers/iio/adc/qcom-spmi-vadc.c
> +++ b/drivers/iio/adc/qcom-spmi-vadc.c
> @@ -84,6 +84,7 @@
>   *	that is an average of multiple measurements.
>   * @scale_fn_type: Represents the scaling function to convert voltage
>   *	physical units desired by the client for the channel.
> + * @datasheet_name: Channel name used in device tree.
>   */
>  struct vadc_channel_prop {
>  	unsigned int channel;
> @@ -93,6 +94,7 @@ struct vadc_channel_prop {
>  	unsigned int hw_settle_time;
>  	unsigned int avg_samples;
>  	enum vadc_scale_fn_type scale_fn_type;
> +	const char *datasheet_name;
>  };
> 
>  /**
> @@ -652,7 +654,7 @@ static int vadc_get_fw_channel_data(struct device *dev,
>  				    struct vadc_channel_prop *prop,
>  				    struct fwnode_handle *fwnode)
>  {
> -	const char *name = fwnode_get_name(fwnode);
> +	const char *name = fwnode_get_name(fwnode), *channel_name;
>  	u32 chan, value, varr[2];
>  	int ret;
> 
> @@ -670,6 +672,12 @@ static int vadc_get_fw_channel_data(struct device *dev,
>  	/* the channel has DT description */
>  	prop->channel = chan;
> 
> +	ret = fwnode_property_read_string(fwnode, "label", &channel_name);
> +	if (ret)
> +		channel_name = name;
> +
> +	prop->datasheet_name = channel_name;
> +
>  	ret = fwnode_property_read_u32(fwnode, "qcom,decimation", &value);
>  	if (!ret) {
>  		ret = qcom_vadc_decimation_from_dt(value);
> @@ -771,6 +779,7 @@ static int vadc_get_fw_data(struct vadc_priv *vadc)
> 
>  		iio_chan->channel = prop.channel;
>  		iio_chan->datasheet_name = vadc_chan->datasheet_name;
> +		iio_chan->extend_name = prop.datasheet_name;
>  		iio_chan->info_mask_separate = vadc_chan->info_mask;
>  		iio_chan->type = vadc_chan->type;
>  		iio_chan->indexed = 1;
> --
> 2.38.1
>

Jonathan Cameron Nov. 12, 2022, 4:27 p.m. UTC | #2

On Sun, 6 Nov 2022 21:24:45 +0100
Marijn Suijten <[e-mail]> wrote:

> Adding Krzysztof to CC for the DT bindings discussion.
> 
> On 2022-11-06 20:30:18, Marijn Suijten wrote:
> > Much like the ADC5 driver iio_chan_spec::extend_name has to be set for
> > friendly/useful names to show up in sysfs, allowing users to correlate
> > readout values with the corresponding probe. This name is read from
> > firmware, taking both the node name and - if set - node label into
> > account.  This is particularly useful for custom thermistors being
> > attached to otherwise-generically-named GPIOs.
> > 

If you are attaching thermistors to an ADC channel, then you should have
a driver for that thermistor.  It will be a consumer of the ADC channel
in question and any labels etc should apply there (along with scaling
/ non linear transforms to get to a temperature), not at the ADC
level.

> > Signed-off-by: Marijn Suijten <[e-mail]>
> > 
> > ---
> > 
> > This RFC may seem a bit controversial as there are multiple patches
> > going around in DT-land changing how nodes are labeled [1] (or
> > introducing new ones: [2]), seemingly to appease binding conventions
> > without considering how the driver propagates them to IIO (and in turn
> > what userspace sees in sysfs).  I hope we can put together the right
> > conventions with this RFC.

> > 
> > Before getting started, note that ADC5 provides this DT/FW node
> > name/label in *both* extend_name *and* datasheet_name;
> > adc5_channels::datasheet_name provided by the macros remains *unread*
> > (except for a non-null check).

There was some history here if I recall correctly.  Until recently(ish) we didn't
have the "label" attribute for channels so the only route was to use
extended_name. That makes a mess for userspace developers however because
it is harder to write a parser that is happy with free form sections
of an attribute name.  So extended_name is more or less deprecated with the
exception of a few legacy cases that we might carry forwards into very similar
drivers.

datasheet_name was introduced to allow binding the channels to consumers
in a human readable form. Note that this dates back to predevice tree
days - so mostly you'll see it used when an mfd registers its own
consumers.  They weren't at the time intended to be used directly by the
drivers at all.


> > Since the names hardcoded in the driver seem to be somewhat
> > "datasheet"-y, and the names in DT typically take the form of a more
> > friendly "<device>-therm" indicating where the thermistor (or voltage
> > probe) is located on the board or attached to, I have opted to persist
> > the original use of vadc_channels::datasheet_name in
> > iio_chan_spec::datasheet_name, and only propagate the data from DT/FW
> > into extend_name.

To clarify datasheet_name is the name on the datasheet of the provider part
not the naming on the board datasheet - basically it's meant to be the pin name.

If you modify extend_name at all you break userspace ABI.
So that's pretty much a non starter (and one reason why we added the label
attribute).

Also, if the ADC channel is labelled with what it is consumed by that feels
backwards.  The thermistor could be connected to any channel.  Any nice
naming should be at the thermistor driver end.  So say I put a thermistor
on input 8.  It should just bind to input 8. The bit of the binding for
the ADC just provides the consumer services for that input 8.

> > (We should likely rename vadc_channel_prop::datasheet_name to
> > extend_name to this end.)
> > 
> > Back when I submitted patches for pm6125 [3] (utilizing ADC5)
> > 4f47a236a23d ("iio: adc: qcom-spmi-adc5: convert to device properties")
> > didn't yet land, and these patches use the node name to convey a
> > useful/friendly name (again, the string literals in ADC5 are unused).
> > fwnode_get_name() however includes the `@xx` reg suffix, making for an
> > unpleasant reading experience in sysfs.
> > 
> > With all that context in mind, I feel like we should answer the
> > following questions:
> > 
> > 1. Should we propagate names from DT/FW at all?

This question needs to make it clear - which name?  Propagating channel
labels to sysfs is often useful via the in_voltageX_label type attributes.
Whether it is useful in this specific driver depends on whether we have
information to convey that isn't provided by channel numbers alone.

> > 2. If so, how should a node be represented in DT?  Should it use generic
> >    node names (which we might not want to use anyway considering the
> >    `@xx` suffix highlighted above) or labels exclusively?

I would suggest only labels.  Though in the case you give of a thermistor attached
this handling is wrong anyway.

> > 3. If only labels are going to be used in conjunction with generic node
> >    names, should ADC5 be changed to ignore the node name?
From a quick search, I'm only seeing the node name used in debug prints currently.
That feels fine to me as it's telling us where the binding parsing went wrong...
Am I missing some use outside of vadc_get_fw_channel_data()?

> > 4. If a label (or node name) is not set, do we fall back to
> >    datasheet_name hardcoded in the driver?

Hmm. Probably not.

> > 5. What do we use for datasheet_name vs extend_name?
Expand that to include label.
datasheet_name : When you want to have human readable pin names from the ADC
  datasheet, used as part of provide services to consumer drivers. Doesn't
  work with DT though as it wasn't part of the binding for consumers.
  So largely irrelevant unless you have an MFD where the ADC consumers are
  also part of the MFD children and so the map is set up in the way we used
  to do it for board files.
extended_name: Short answer is don't use it today.  It was a bad design decision
  a long time back.
label: This is the one you should info from DT through to today.  As it is freeform
  and comes from the bindings - we don't encode this in the const iio_chan_spec array
  but rather use the iio_info->read_label() callback.  It is provided to userspace
  as a per channel _label attribute.

> > 6. Any other vadc drivers that need the same treatment, when we come to
> >    a resolution?
Any resolution can only 'add' ABI to userspace.  So adding labels is fine.
extend_name never is.

Hope that helps.

Jonathan

> > 
> > [1]: https://lore.kernel.org/linux-arm-msm/[e-mail]/T/#u
> > [2]: https://lore.kernel.org/linux-arm-msm/[e-mail]/
> > [3]: https://lore.kernel.org/linux-arm-msm/[e-mail]/T/#u
> > 
> > Thanks for considering this!
> > - Marijn
> > 
> >  drivers/iio/adc/qcom-spmi-vadc.c | 11 ++++++++++-
> >  1 file changed, 10 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/iio/adc/qcom-spmi-vadc.c b/drivers/iio/adc/qcom-spmi-vadc.c
> > index bcff0f62b70e..8c6c7fa13cfe 100644
> > --- a/drivers/iio/adc/qcom-spmi-vadc.c
> > +++ b/drivers/iio/adc/qcom-spmi-vadc.c
> > @@ -84,6 +84,7 @@
> >   *	that is an average of multiple measurements.
> >   * @scale_fn_type: Represents the scaling function to convert voltage
> >   *	physical units desired by the client for the channel.
> > + * @datasheet_name: Channel name used in device tree.
> >   */
> >  struct vadc_channel_prop {
> >  	unsigned int channel;
> > @@ -93,6 +94,7 @@ struct vadc_channel_prop {
> >  	unsigned int hw_settle_time;
> >  	unsigned int avg_samples;
> >  	enum vadc_scale_fn_type scale_fn_type;
> > +	const char *datasheet_name;
> >  };
> > 
> >  /**
> > @@ -652,7 +654,7 @@ static int vadc_get_fw_channel_data(struct device *dev,
> >  				    struct vadc_channel_prop *prop,
> >  				    struct fwnode_handle *fwnode)
> >  {
> > -	const char *name = fwnode_get_name(fwnode);
> > +	const char *name = fwnode_get_name(fwnode), *channel_name;
> >  	u32 chan, value, varr[2];
> >  	int ret;
> > 
> > @@ -670,6 +672,12 @@ static int vadc_get_fw_channel_data(struct device *dev,
> >  	/* the channel has DT description */
> >  	prop->channel = chan;
> > 
> > +	ret = fwnode_property_read_string(fwnode, "label", &channel_name);
> > +	if (ret)
> > +		channel_name = name;
> > +
> > +	prop->datasheet_name = channel_name;
> > +
> >  	ret = fwnode_property_read_u32(fwnode, "qcom,decimation", &value);
> >  	if (!ret) {
> >  		ret = qcom_vadc_decimation_from_dt(value);
> > @@ -771,6 +779,7 @@ static int vadc_get_fw_data(struct vadc_priv *vadc)
> > 
> >  		iio_chan->channel = prop.channel;
> >  		iio_chan->datasheet_name = vadc_chan->datasheet_name;
> > +		iio_chan->extend_name = prop.datasheet_name;
> >  		iio_chan->info_mask_separate = vadc_chan->info_mask;
> >  		iio_chan->type = vadc_chan->type;
> >  		iio_chan->indexed = 1;
> > --
> > 2.38.1
> >

Marijn Suijten Nov. 30, 2022, 8:54 p.m. UTC | #3

On 2022-11-12 16:27:19, Jonathan Cameron wrote:
> On Sun, 6 Nov 2022 21:24:45 +0100
> Marijn Suijten <[e-mail]> wrote:
> 
> > Adding Krzysztof to CC for the DT bindings discussion.
> > 
> > On 2022-11-06 20:30:18, Marijn Suijten wrote:
> > > Much like the ADC5 driver iio_chan_spec::extend_name has to be set for
> > > friendly/useful names to show up in sysfs, allowing users to correlate
> > > readout values with the corresponding probe. This name is read from
> > > firmware, taking both the node name and - if set - node label into
> > > account.  This is particularly useful for custom thermistors being
> > > attached to otherwise-generically-named GPIOs.
> > > 
> 
> If you are attaching thermistors to an ADC channel, then you should have
> a driver for that thermistor.  It will be a consumer of the ADC channel
> in question and any labels etc should apply there (along with scaling
> / non linear transforms to get to a temperature), not at the ADC
> level.

This is what happens in the ADC5 driver, though.  In /sys/bus/iio names
show up for ADC channels that aren't otherwise consumed by (thermistor)
drivers.  There are also voltage readings.  The IIO driver seems to be
aware of both the unit and (linear iirc) scaling.

> > > Signed-off-by: Marijn Suijten <[e-mail]>
> > > 
> > > ---
> > > 
> > > This RFC may seem a bit controversial as there are multiple patches
> > > going around in DT-land changing how nodes are labeled [1] (or
> > > introducing new ones: [2]), seemingly to appease binding conventions
> > > without considering how the driver propagates them to IIO (and in turn
> > > what userspace sees in sysfs).  I hope we can put together the right
> > > conventions with this RFC.
> 
> > > 
> > > Before getting started, note that ADC5 provides this DT/FW node
> > > name/label in *both* extend_name *and* datasheet_name;
> > > adc5_channels::datasheet_name provided by the macros remains *unread*
> > > (except for a non-null check).
> 
> There was some history here if I recall correctly.  Until recently(ish) we didn't
> have the "label" attribute for channels so the only route was to use
> extended_name. That makes a mess for userspace developers however because
> it is harder to write a parser that is happy with free form sections
> of an attribute name.  So extended_name is more or less deprecated with the
> exception of a few legacy cases that we might carry forwards into very similar
> drivers.

Making sure we're talking about the same thing: it's extend_name, not
extendED_name.

> datasheet_name was introduced to allow binding the channels to consumers
> in a human readable form. Note that this dates back to predevice tree
> days - so mostly you'll see it used when an mfd registers its own
> consumers.  They weren't at the time intended to be used directly by the
> drivers at all.

It is unfortunate that I don't see these in sysfs then; vadc only
assigns datasheet_name but not extend_name.

> > > Since the names hardcoded in the driver seem to be somewhat
> > > "datasheet"-y, and the names in DT typically take the form of a more
> > > friendly "<device>-therm" indicating where the thermistor (or voltage
> > > probe) is located on the board or attached to, I have opted to persist
> > > the original use of vadc_channels::datasheet_name in
> > > iio_chan_spec::datasheet_name, and only propagate the data from DT/FW
> > > into extend_name.
> 
> To clarify datasheet_name is the name on the datasheet of the provider part
> not the naming on the board datasheet - basically it's meant to be the pin name.

Right; it may not have come across but that is what I assumed (datsheet
name of the part, which would be the names hardcoded in the adc5/vadc
driver), and then have the labels - assigned in /board/ dts specialize
that where it is not a hardwired reading within the part.

> If you modify extend_name at all you break userspace ABI.
> So that's pretty much a non starter (and one reason why we added the label
> attribute).

The sysfs filenames will change, but they currently don't carry an
in_{voltage,temp}X_label attribute.  Only when I set extend_name to
something sensible.  But then X changes from an index to that same name
too.

Note that this is already the case for ADC5.

> Also, if the ADC channel is labelled with what it is consumed by that feels
> backwards.  The thermistor could be connected to any channel.  Any nice
> naming should be at the thermistor driver end.  So say I put a thermistor
> on input 8.  It should just bind to input 8. The bit of the binding for
> the ADC just provides the consumer services for that input 8.

This is how these drivers are describing their channels though, except
for a few freely assignable GPIO channels?

> > > (We should likely rename vadc_channel_prop::datasheet_name to
> > > extend_name to this end.)
> > > 
> > > Back when I submitted patches for pm6125 [3] (utilizing ADC5)
> > > 4f47a236a23d ("iio: adc: qcom-spmi-adc5: convert to device properties")
> > > didn't yet land, and these patches use the node name to convey a
> > > useful/friendly name (again, the string literals in ADC5 are unused).
> > > fwnode_get_name() however includes the `@xx` reg suffix, making for an
> > > unpleasant reading experience in sysfs.
> > > 
> > > With all that context in mind, I feel like we should answer the
> > > following questions:
> > > 
> > > 1. Should we propagate names from DT/FW at all?
> 
> This question needs to make it clear - which name?  Propagating channel
> labels to sysfs is often useful via the in_voltageX_label type attributes.

Note that X here gets replaced by the value of extend_name, it seems.

> Whether it is useful in this specific driver depends on whether we have
> information to convey that isn't provided by channel numbers alone.

The driver contains channel names for the purpose of clarifying what the
channel is, which isn't easily deducible (nor very user-friendly) when
only having access to channel indices/numbers.

> > > 2. If so, how should a node be represented in DT?  Should it use generic
> > >    node names (which we might not want to use anyway considering the
> > >    `@xx` suffix highlighted above) or labels exclusively?
> 
> I would suggest only labels.

Ack, the node name is a mess nowadays.  That means ADC5 shouldn't use it
as fallback either when a DT label is not set (and instead use the
currently-unused adc5_channels::datasheet_name field).

Can I remove it (use of fwnode_get_name() as datasheet_name)?

> Though in the case you give of a thermistor attached
> this handling is wrong anyway.

Not sure I follow you here.  The driver defines when a channel is a
thermistor or a voltage, and even gives it a name/label.  The values are
readable through /sys/bus/iio.  Not sure if they're all correct
readings, and some (but not all) are later routed into a "thermal
manager", but having at least a _label for these would be useful.

> > > 3. If only labels are going to be used in conjunction with generic node
> > >    names, should ADC5 be changed to ignore the node name?
>
> From a quick search, I'm only seeing the node name used in debug prints currently.
> That feels fine to me as it's telling us where the binding parsing went wrong...
> Am I missing some use outside of vadc_get_fw_channel_data()?

That's the VADC driver.  Look at adc5_get_fw_channel_data, specifically
where it calls fwnode_property_read_string() to overwrite
prop->datasheet_name.

> > > 4. If a label (or node name) is not set, do we fall back to
> > >    datasheet_name hardcoded in the driver?
> 
> Hmm. Probably not.

Then we might as well remove this useless data from the kernel driver
altogether...

> > > 5. What do we use for datasheet_name vs extend_name?
> Expand that to include label.
> datasheet_name : When you want to have human readable pin names from the ADC
>   datasheet, used as part of provide services to consumer drivers. Doesn't
>   work with DT though as it wasn't part of the binding for consumers.
>   So largely irrelevant unless you have an MFD where the ADC consumers are
>   also part of the MFD children and so the map is set up in the way we used
>   to do it for board files.

... or this could remain to feed into datasheet_name?

> extended_name: Short answer is don't use it today.  It was a bad design decision
>   a long time back.
> label: This is the one you should info from DT through to today.  As it is freeform
>   and comes from the bindings - we don't encode this in the const iio_chan_spec array
>   but rather use the iio_info->read_label() callback.  It is provided to userspace
>   as a per channel _label attribute.

Thanks, I have been looking for this and scanning through
iio_read_channel_label() now.  It'll use ->read_label() and only defer
to extend_name if the getter isn't available.

I'll insert a getter here in the vadc driver that returns the DT label
if set, otherwise the hardcoded driver name (which'll still feed into
iio_chan_spec::datasheet_name).

Do we then remove extend_name from qcom-spmi-adc5 and give it the same
treatment, since it would now use DT node names as filenames unless a
label is set?  I can only imagine it having been set because the ADC5
author(s) didn't see a name nor label in sysfs either, without knowing
about the existence of read_label.

> > > 6. Any other vadc drivers that need the same treatment, when we come to
> > >    a resolution?
> Any resolution can only 'add' ABI to userspace.  So adding labels is fine.
> extend_name never is.

Saying the above in a different way: would removing extend_name
assignment from qcom-spmi-adc5 be fine?

> Hope that helps.

A lot, now knowing that read_label is the part of the puzzle I
previously missed.  Thanks!

- Marijn

Jonathan Cameron Dec. 3, 2022, 5:06 p.m. UTC | #4

On Wed, 30 Nov 2022 21:54:14 +0100
Marijn Suijten <[e-mail]> wrote:

> On 2022-11-12 16:27:19, Jonathan Cameron wrote:
> > On Sun, 6 Nov 2022 21:24:45 +0100
> > Marijn Suijten <[e-mail]> wrote:
> >   
> > > Adding Krzysztof to CC for the DT bindings discussion.
> > > 
> > > On 2022-11-06 20:30:18, Marijn Suijten wrote:  
> > > > Much like the ADC5 driver iio_chan_spec::extend_name has to be set for
> > > > friendly/useful names to show up in sysfs, allowing users to correlate
> > > > readout values with the corresponding probe. This name is read from
> > > > firmware, taking both the node name and - if set - node label into
> > > > account.  This is particularly useful for custom thermistors being
> > > > attached to otherwise-generically-named GPIOs.
> > > >   
> > 
> > If you are attaching thermistors to an ADC channel, then you should have
> > a driver for that thermistor.  It will be a consumer of the ADC channel
> > in question and any labels etc should apply there (along with scaling
> > / non linear transforms to get to a temperature), not at the ADC
> > level.  
> 
> This is what happens in the ADC5 driver, though.  In /sys/bus/iio names
> show up for ADC channels that aren't otherwise consumed by (thermistor)
> drivers.  There are also voltage readings.  The IIO driver seems to be
> aware of both the unit and (linear iirc) scaling.

There were cases where we did that but my understanding of what was going
on at the time may have been wrong. I was assuming there was specific
hardware on the SOC side of things that did 'special' stuff for the
thermistor rather than just being an ADC channel.

We have plenty of other cases with explicit analog sensors drivers.
see drivers/thermal/thermal-generic-adc.c or hwmon/ntc_thermistor.c
for example.

Looking at the example you have in adc5 that you mention below
which is rather odd I can see where confusion came from!

> 
> > > > Signed-off-by: Marijn Suijten <[e-mail]>
> > > > 
> > > > ---
> > > > 
> > > > This RFC may seem a bit controversial as there are multiple patches
> > > > going around in DT-land changing how nodes are labeled [1] (or
> > > > introducing new ones: [2]), seemingly to appease binding conventions
> > > > without considering how the driver propagates them to IIO (and in turn
> > > > what userspace sees in sysfs).  I hope we can put together the right
> > > > conventions with this RFC.  
> >   
> > > > 
> > > > Before getting started, note that ADC5 provides this DT/FW node
> > > > name/label in *both* extend_name *and* datasheet_name;
> > > > adc5_channels::datasheet_name provided by the macros remains *unread*
> > > > (except for a non-null check).  
> > 
> > There was some history here if I recall correctly.  Until recently(ish) we didn't
> > have the "label" attribute for channels so the only route was to use
> > extended_name. That makes a mess for userspace developers however because
> > it is harder to write a parser that is happy with free form sections
> > of an attribute name.  So extended_name is more or less deprecated with the
> > exception of a few legacy cases that we might carry forwards into very similar
> > drivers.  
> 
> Making sure we're talking about the same thing: it's extend_name, not
> extendED_name.

Sure.  I remembered it wrong.

> 
> > datasheet_name was introduced to allow binding the channels to consumers
> > in a human readable form. Note that this dates back to predevice tree
> > days - so mostly you'll see it used when an mfd registers its own
> > consumers.  They weren't at the time intended to be used directly by the
> > drivers at all.  
> 
> It is unfortunate that I don't see these in sysfs then; vadc only
> assigns datasheet_name but not extend_name.

You can provide a read_label callback to expose that if it makes sense
for a particular device to provide those as the labels.  Cleverer things like
using those as a default that can be over ridden by a board specific naming
are fine as well.

> 
> > > > Since the names hardcoded in the driver seem to be somewhat
> > > > "datasheet"-y, and the names in DT typically take the form of a more
> > > > friendly "<device>-therm" indicating where the thermistor (or voltage
> > > > probe) is located on the board or attached to, I have opted to persist
> > > > the original use of vadc_channels::datasheet_name in
> > > > iio_chan_spec::datasheet_name, and only propagate the data from DT/FW
> > > > into extend_name.  
> > 
> > To clarify datasheet_name is the name on the datasheet of the provider part
> > not the naming on the board datasheet - basically it's meant to be the pin name.  
> 
> Right; it may not have come across but that is what I assumed (datsheet
> name of the part, which would be the names hardcoded in the adc5/vadc
> driver), and then have the labels - assigned in /board/ dts specialize
> that where it is not a hardwired reading within the part.

yes.

> 
> > If you modify extend_name at all you break userspace ABI.
> > So that's pretty much a non starter (and one reason why we added the label
> > attribute).  
> 
> The sysfs filenames will change, but they currently don't carry an
> in_{voltage,temp}X_label attribute.  Only when I set extend_name to
> something sensible.  But then X changes from an index to that same name
> too.

Add the label in that case by providing the read_label callback.
There is a fallback related to extend_name that is intended to help with
problem of writing userspace code against the legacy use of extend_name.
As a result, when no read_label callback is provided the label attribute
provides the extend_name.  Intent is that provides the information that
allows userspace code to know how to parse the naming.

That path is there as a fallback, not as how new code should do it.

> 
> Note that this is already the case for ADC5.

Best practice changes over time.  Because of the feedback we have had
from userspace developers we now very very rarely use extend_name.
Obviously we can't 'fix' old drivers thought without breaking the ABI.

> 
> > Also, if the ADC channel is labelled with what it is consumed by that feels
> > backwards.  The thermistor could be connected to any channel.  Any nice
> > naming should be at the thermistor driver end.  So say I put a thermistor
> > on input 8.  It should just bind to input 8. The bit of the binding for
> > the ADC just provides the consumer services for that input 8.  
> 
> This is how these drivers are describing their channels though, except
> for a few freely assignable GPIO channels?

My assumption was that the inputs were not general purpose.  With the exception
of external temperature sensors, many SoC ADCs have some channels wired
to internal voltage lines and temperature sensors, so seemed reasonable
to label them as such.  If that's wrong then it was my misunderstanding when
reading the original code.

Lack of easy availability of suitable datasheets means we have to rely
on submitters distinguishing internally wired, from board wiring based
associations.

> 
> > > > (We should likely rename vadc_channel_prop::datasheet_name to
> > > > extend_name to this end.)
> > > > 
> > > > Back when I submitted patches for pm6125 [3] (utilizing ADC5)
> > > > 4f47a236a23d ("iio: adc: qcom-spmi-adc5: convert to device properties")
> > > > didn't yet land, and these patches use the node name to convey a
> > > > useful/friendly name (again, the string literals in ADC5 are unused).
> > > > fwnode_get_name() however includes the `@xx` reg suffix, making for an
> > > > unpleasant reading experience in sysfs.
> > > > 
> > > > With all that context in mind, I feel like we should answer the
> > > > following questions:
> > > > 
> > > > 1. Should we propagate names from DT/FW at all?  
> > 
> > This question needs to make it clear - which name?  Propagating channel
> > labels to sysfs is often useful via the in_voltageX_label type attributes.  
> 
> Note that X here gets replaced by the value of extend_name, it seems.

You talk about this below, so I've removed what I wrote that said the
same thing

> 
> > Whether it is useful in this specific driver depends on whether we have
> > information to convey that isn't provided by channel numbers alone.  
> 
> The driver contains channel names for the purpose of clarifying what the
> channel is, which isn't easily deducible (nor very user-friendly) when
> only having access to channel indices/numbers.

In that case sounds like you should provide the read_label() callback.

> 
> > > > 2. If so, how should a node be represented in DT?  Should it use generic
> > > >    node names (which we might not want to use anyway considering the
> > > >    `@xx` suffix highlighted above) or labels exclusively?  
> > 
> > I would suggest only labels.  
> 
> Ack, the node name is a mess nowadays.  That means ADC5 shouldn't use it
> as fallback either when a DT label is not set (and instead use the
> currently-unused adc5_channels::datasheet_name field).
> 
> Can I remove it (use of fwnode_get_name() as datasheet_name)?

Ah. That's indeed a mess. From an ABI point of view you can indeed break the
connection between datasheet_name and the "label", but you can't
change the use for extend_name (ABI breakage) unless you are very very sure
it won't break existing userspace code.

Now from a potential consumers point of view, it's possible someone is relying
on the datasheet name to get the right channel. Given those are only
used if a driver is directly registering an iio_map, should be easy enough
to fix..

> 
> > Though in the case you give of a thermistor attached
> > this handling is wrong anyway.  
> 
> Not sure I follow you here.  The driver defines when a channel is a
> thermistor or a voltage, and even gives it a name/label.  The values are
> readable through /sys/bus/iio.  Not sure if they're all correct
> readings, and some (but not all) are later routed into a "thermal
> manager", but having at least a _label for these would be useful.

Fine to have a label as those are intended to provide useful info to
userspace.
Assuming there is nothing magic set in hardware dependent on thermistor
vs voltage then... If we were doing this today, we'd have them all as ADC
channels.  Board specific labels are fine, but used as part of the consumer
side of things.
Each thermistor would be a separate 'device' consuming an ADC channel.
Anything that needed that thermal info would then consumer that thermistor
device (which would have done relevant scaling etc depending on the analog
component). Advantage of this approach is that it allows for much wider
variety of external temperature sensors without having to code up the
temperature to voltage functions in every ADC driver.

In some drivers we have older code that squashes the thermistor handling
into the driver.  That can be necessary if there is handling to do on the
ADC side of things. From a quick glance, I'm not sure there is any to do
here (an example where this gets complex is the more sophisticated
touchscreen controllers, where there is a lot of sequencing involved
alongside reading particular ADC channels).

> 
> > > > 3. If only labels are going to be used in conjunction with generic node
> > > >    names, should ADC5 be changed to ignore the node name?  
> >
> > From a quick search, I'm only seeing the node name used in debug prints currently.
> > That feels fine to me as it's telling us where the binding parsing went wrong...
> > Am I missing some use outside of vadc_get_fw_channel_data()?  
> 
> That's the VADC driver.  Look at adc5_get_fw_channel_data, specifically
> where it calls fwnode_property_read_string() to overwrite
> prop->datasheet_name.

Ah. Thanks for the pointer, though I'm still confused.

	ret = fwnode_property_read_string(fwnode, "label", &channel_name);
	if (ret)
		channel_name = name;

	prop->datasheet_name = channel_name;

That's reading the label property, not the node name.

> 
> > > > 4. If a label (or node name) is not set, do we fall back to
> > > >    datasheet_name hardcoded in the driver?  
> > 
> > Hmm. Probably not.  
> 
> Then we might as well remove this useless data from the kernel driver
> altogether...

Ok. May make sense to use the datasheet name if noting better provided
for the label.  Assuming the datasheet names are them selves somewhat
useful information for a user.

> 
> > > > 5. What do we use for datasheet_name vs extend_name?  
> > Expand that to include label.
> > datasheet_name : When you want to have human readable pin names from the ADC
> >   datasheet, used as part of provide services to consumer drivers. Doesn't
> >   work with DT though as it wasn't part of the binding for consumers.
> >   So largely irrelevant unless you have an MFD where the ADC consumers are
> >   also part of the MFD children and so the map is set up in the way we used
> >   to do it for board files.  
> 
> ... or this could remain to feed into datasheet_name?
Now I'm confused.  Feed into label perhaps?

> 
> > extended_name: Short answer is don't use it today.  It was a bad design decision
> >   a long time back.
> > label: This is the one you should info from DT through to today.  As it is freeform
> >   and comes from the bindings - we don't encode this in the const iio_chan_spec array
> >   but rather use the iio_info->read_label() callback.  It is provided to userspace
> >   as a per channel _label attribute.  
> 
> Thanks, I have been looking for this and scanning through
> iio_read_channel_label() now.  It'll use ->read_label() and only defer
> to extend_name if the getter isn't available.
yup.  The extend_name fallback was added to solve complex parsing for
legacy drivers.  I guess that added confusion here.

> 
> I'll insert a getter here in the vadc driver that returns the DT label
> if set, otherwise the hardcoded driver name (which'll still feed into
> iio_chan_spec::datasheet_name).

I guess that's safe enough as long as no one registers an iio_map in
one of these drivers. grep suggests no one does :)

> 
> Do we then remove extend_name from qcom-spmi-adc5 and give it the same
> treatment, since it would now use DT node names as filenames unless a
> label is set?  I can only imagine it having been set because the ADC5
> author(s) didn't see a name nor label in sysfs either, without knowing
> about the existence of read_label.

Sadly we can't remove it because of the ABI change that would result and
potential userspace breakage.

Not sure how age of this driver matches up with the 'change of plan'
but we 'used' to use extend_name for this purpose before I got moaned
at a lot by the userspace folk. They were right of course, that encoding
free form data in filenames isn't the best idea I ever had.  


> 
> > > > 6. Any other vadc drivers that need the same treatment, when we come to
> > > >    a resolution?  
> > Any resolution can only 'add' ABI to userspace.  So adding labels is fine.
> > extend_name never is.  
> 
> Saying the above in a different way: would removing extend_name
> assignment from qcom-spmi-adc5 be fine?

Sadly not.  We are stuck with that.

> 
> > Hope that helps.  
> 
> A lot, now knowing that read_label is the part of the puzzle I
> previously missed.  Thanks!

When I let the extend_name fallback in for the labels
it didn't occur to me that it would make it more confusing for
people looking at older code.  Long shot, but would a comment
in iio.h for extend_name to say something to this effect be likely
to have been something you'd have seen?  If it would, let's add
one to potentially make this less confusing for the next person!

Jonathan


> 
> - Marijn

Marijn Suijten Dec. 21, 2022, 10:34 p.m. UTC | #5

Hi Jonathan,

Apologies for another late reply; we really shouldn't make these
messages this long and I'll try to only reply to the most relevant
points and cull out the rest.

On 2022-12-03 17:06:56, Jonathan Cameron wrote:
> On Wed, 30 Nov 2022 21:54:14 +0100
> Marijn Suijten <[e-mail]> wrote:
> 
> > On 2022-11-12 16:27:19, Jonathan Cameron wrote:
> > > On Sun, 6 Nov 2022 21:24:45 +0100
> > > Marijn Suijten <[e-mail]> wrote:
> > >   
> > > > Adding Krzysztof to CC for the DT bindings discussion.
> > > > 
> > > > On 2022-11-06 20:30:18, Marijn Suijten wrote:  
> > > > > Much like the ADC5 driver iio_chan_spec::extend_name has to be set for
> > > > > friendly/useful names to show up in sysfs, allowing users to correlate
> > > > > readout values with the corresponding probe. This name is read from
> > > > > firmware, taking both the node name and - if set - node label into
> > > > > account.  This is particularly useful for custom thermistors being
> > > > > attached to otherwise-generically-named GPIOs.
> > > > >   
> > > 
> > > If you are attaching thermistors to an ADC channel, then you should have
> > > a driver for that thermistor.  It will be a consumer of the ADC channel
> > > in question and any labels etc should apply there (along with scaling
> > > / non linear transforms to get to a temperature), not at the ADC
> > > level.  
> > 
> > This is what happens in the ADC5 driver, though.  In /sys/bus/iio names
> > show up for ADC channels that aren't otherwise consumed by (thermistor)
> > drivers.  There are also voltage readings.  The IIO driver seems to be
> > aware of both the unit and (linear iirc) scaling.
> 
> There were cases where we did that but my understanding of what was going
> on at the time may have been wrong. I was assuming there was specific
> hardware on the SOC side of things that did 'special' stuff for the
> thermistor rather than just being an ADC channel.

There is, it's the thermal monitor driver but I don't think it binds to
every ADC channel, and the ADC channels provided by VADC/ADC5 provide
useful information on their own in sysfs.

<snip>

> > This is how these drivers are describing their channels though, except
> > for a few freely assignable GPIO channels?
> 
> My assumption was that the inputs were not general purpose.  With the exception
> of external temperature sensors, many SoC ADCs have some channels wired
> to internal voltage lines and temperature sensors, so seemed reasonable
> to label them as such.  If that's wrong then it was my misunderstanding when
> reading the original code.

These drivers seem to support both.  Internal PMIC channels, probably a
couple that go off-chip but are for a "specific" usecase, and a few
"GPIO" channels that appear to be multipurpose (per General Purpose...
IO).

> Lack of easy availability of suitable datasheets means we have to rely
> on submitters distinguishing internally wired, from board wiring based
> associations.

And submitters typically rely on copy-pasting downstream - at least with
the read_label callback contributors should have an easier way of
correlating sysfs file readings back to the mapping they described in
DTS, and ballpark-guesstimate that the reading is correct (e.g. on
pm6125 I found that I was missing some pinctrl biases this way resulting
in extraneous temperature readings).

<snip>

> > Ack, the node name is a mess nowadays.  That means ADC5 shouldn't use it
> > as fallback either when a DT label is not set (and instead use the
> > currently-unused adc5_channels::datasheet_name field).
> > 
> > Can I remove it (use of fwnode_get_name() as datasheet_name)?
> 
> Ah. That's indeed a mess. From an ABI point of view you can indeed break the
> connection between datasheet_name and the "label", but you can't
> change the use for extend_name (ABI breakage) unless you are very very sure
> it won't break existing userspace code.

As in, as long as we don't touch extend_name which would affect sysfs
names, changing the label returned by read_label is fine?  And changing
datasheet_name to only ever use the datasheet_name provided by the
driver and never the name provided in DTS is also okay?

> Now from a potential consumers point of view, it's possible someone is relying
> on the datasheet name to get the right channel. Given those are only
> used if a driver is directly registering an iio_map, should be easy enough
> to fix..

I am unfortunately completely unfamiliar with iio_map, and hope it
doesn't distract too much from trying to add label files to QCom's SPMI
VADC driver :)

<snip>

> In some drivers we have older code that squashes the thermistor handling
> into the driver.  That can be necessary if there is handling to do on the
> ADC side of things. From a quick glance, I'm not sure there is any to do
> here (an example where this gets complex is the more sophisticated
> touchscreen controllers, where there is a lot of sequencing involved
> alongside reading particular ADC channels).

Seems this works OOTB already (as in, when reading sysfs I see values
that could be sensibly interpreted as "room temperature" in celsius).
And not something I intend to look into, again, only labels.

> > > > > 3. If only labels are going to be used in conjunction with generic node
> > > > >    names, should ADC5 be changed to ignore the node name?  
> > >
> > > From a quick search, I'm only seeing the node name used in debug prints currently.
> > > That feels fine to me as it's telling us where the binding parsing went wrong...
> > > Am I missing some use outside of vadc_get_fw_channel_data()?  
> > 
> > That's the VADC driver.  Look at adc5_get_fw_channel_data, specifically
> > where it calls fwnode_property_read_string() to overwrite
> > prop->datasheet_name.
> 
> Ah. Thanks for the pointer, though I'm still confused.
> 
> 	ret = fwnode_property_read_string(fwnode, "label", &channel_name);
> 	if (ret)
> 		channel_name = name;
  		             ^ here ^

> 	prop->datasheet_name = channel_name;
> 
> That's reading the label property, not the node name.

The node name sits in `name`, and that's used if there's no "label"
property in wich case ret is non-zero and we end up in `if (ret)
channel_name = name;`.

> > > > > 4. If a label (or node name) is not set, do we fall back to
> > > > >    datasheet_name hardcoded in the driver?  
> > > 
> > > Hmm. Probably not.  
> > 
> > Then we might as well remove this useless data from the kernel driver
> > altogether...
> 
> Ok. May make sense to use the datasheet name if noting better provided
> for the label.  Assuming the datasheet names are them selves somewhat
> useful information for a user.

They're generated from the macro (hence capitalized) in VADC, manually
written in ADC5.  Would it make sense to add handwritten string
literals for this?

> > > > > 5. What do we use for datasheet_name vs extend_name?  
> > > Expand that to include label.
> > > datasheet_name : When you want to have human readable pin names from the ADC
> > >   datasheet, used as part of provide services to consumer drivers. Doesn't
> > >   work with DT though as it wasn't part of the binding for consumers.
> > >   So largely irrelevant unless you have an MFD where the ADC consumers are
> > >   also part of the MFD children and so the map is set up in the way we used
> > >   to do it for board files.  
> > 
> > ... or this could remain to feed into datasheet_name?
> Now I'm confused.  Feed into label perhaps?

Feed into read_label when no label was otherwise provided in DTS, but
always feed into iio_chan_spec::datasheet_name since we discussed that
this should represent the name of the part (e.g. PMIC), not the board
and way in which it consumes the channel.

<snip>

> > Do we then remove extend_name from qcom-spmi-adc5 and give it the same
> > treatment, since it would now use DT node names as filenames unless a
> > label is set?  I can only imagine it having been set because the ADC5
> > author(s) didn't see a name nor label in sysfs either, without knowing
> > about the existence of read_label.
> 
> Sadly we can't remove it because of the ABI change that would result and
> potential userspace breakage.

The change to fwnode_get_name is already breaking this sysfs ABI though,
as discussed in a DTS series where I replaced all node names with labels
to support (the followup of) this iio series.

We could unbreak it by either stripping @xx off of it, or setting
extend_name to a label (if it exists) instead, but the latter is
breaking :(

<snip>

> > > Hope that helps.  
> > 
> > A lot, now knowing that read_label is the part of the puzzle I
> > previously missed.  Thanks!
> 
> When I let the extend_name fallback in for the labels
> it didn't occur to me that it would make it more confusing for
> people looking at older code.  Long shot, but would a comment
> in iio.h for extend_name to say something to this effect be likely
> to have been something you'd have seen?  If it would, let's add
> one to potentially make this less confusing for the next person!

Yes, I think I visited the documentation/definition of extend_name at
some point and would have been been helped if that pointed me right over
to read_label.  Right in ADC5 (and other drivers) would be even better
but may be overly verbose.

Regardless of that VADC/ADC5 do some _really confusing_ things, passing
strings around in various weird ways (or not), and it took some time to
keep the various similar structs apart :)

- Marijn

Jonathan Cameron Dec. 23, 2022, 5:44 p.m. UTC | #6

On Wed, 21 Dec 2022 23:34:32 +0100
Marijn Suijten <[e-mail]> wrote:

> Hi Jonathan,
> 
> Apologies for another late reply; we really shouldn't make these
> messages this long and I'll try to only reply to the most relevant
> points and cull out the rest.
> 
> On 2022-12-03 17:06:56, Jonathan Cameron wrote:
> > On Wed, 30 Nov 2022 21:54:14 +0100
> > Marijn Suijten <[e-mail]> wrote:
> >   
> > > On 2022-11-12 16:27:19, Jonathan Cameron wrote:  
> > > > On Sun, 6 Nov 2022 21:24:45 +0100
> > > > Marijn Suijten <[e-mail]> wrote:
> > > >     
> > > > > Adding Krzysztof to CC for the DT bindings discussion.
> > > > > 
> > > > > On 2022-11-06 20:30:18, Marijn Suijten wrote:    
> > > > > > Much like the ADC5 driver iio_chan_spec::extend_name has to be set for
> > > > > > friendly/useful names to show up in sysfs, allowing users to correlate
> > > > > > readout values with the corresponding probe. This name is read from
> > > > > > firmware, taking both the node name and - if set - node label into
> > > > > > account.  This is particularly useful for custom thermistors being
> > > > > > attached to otherwise-generically-named GPIOs.
> > > > > >     
> > > > 
> > > > If you are attaching thermistors to an ADC channel, then you should have
> > > > a driver for that thermistor.  It will be a consumer of the ADC channel
> > > > in question and any labels etc should apply there (along with scaling
> > > > / non linear transforms to get to a temperature), not at the ADC
> > > > level.    
> > > 
> > > This is what happens in the ADC5 driver, though.  In /sys/bus/iio names
> > > show up for ADC channels that aren't otherwise consumed by (thermistor)
> > > drivers.  There are also voltage readings.  The IIO driver seems to be
> > > aware of both the unit and (linear iirc) scaling.  
> > 
> > There were cases where we did that but my understanding of what was going
> > on at the time may have been wrong. I was assuming there was specific
> > hardware on the SOC side of things that did 'special' stuff for the
> > thermistor rather than just being an ADC channel.  
> 
> There is, it's the thermal monitor driver but I don't think it binds to
> every ADC channel, and the ADC channels provided by VADC/ADC5 provide
> useful information on their own in sysfs.
> 

Ah. That make sense - the specific logic is hidden in the other driver.

> > > Ack, the node name is a mess nowadays.  That means ADC5 shouldn't use it
> > > as fallback either when a DT label is not set (and instead use the
> > > currently-unused adc5_channels::datasheet_name field).
> > > 
> > > Can I remove it (use of fwnode_get_name() as datasheet_name)?  
> > 
> > Ah. That's indeed a mess. From an ABI point of view you can indeed break the
> > connection between datasheet_name and the "label", but you can't
> > change the use for extend_name (ABI breakage) unless you are very very sure
> > it won't break existing userspace code.  
> 
> As in, as long as we don't touch extend_name which would affect sysfs
> names, changing the label returned by read_label is fine? 

Sticky corner, but I think we should be fine doing so on basis of changing
ABI we don't think anyone will notice. In lots of cases the label is effected
by DTS files that may change under the kernel anyway so hopefully no one is
relying on the value too much *crossed fingers*

> And changing
> datasheet_name to only ever use the datasheet_name provided by the
> driver and never the name provided in DTS is also okay?

datasheet_name is internal to kernel only so can definitely change that
as long as we don't have any upstream users (I'm fairly sure there aren't any)

> 
> > Now from a potential consumers point of view, it's possible someone is relying
> > on the datasheet name to get the right channel. Given those are only
> > used if a driver is directly registering an iio_map, should be easy enough
> > to fix..  
> 
> I am unfortunately completely unfamiliar with iio_map, and hope it
> doesn't distract too much from trying to add label files to QCom's SPMI
> VADC driver :)

Just think of it as the board file way of doing equivalent of what we have
to set up IIO consumers in DT. It's also used in driver that hard code
relationships with their consumers - not including this one so we should
be fine.

> 

> 
> > > > > > 3. If only labels are going to be used in conjunction with generic node
> > > > > >    names, should ADC5 be changed to ignore the node name?    
> > > >
> > > > From a quick search, I'm only seeing the node name used in debug prints currently.
> > > > That feels fine to me as it's telling us where the binding parsing went wrong...
> > > > Am I missing some use outside of vadc_get_fw_channel_data()?    
> > > 
> > > That's the VADC driver.  Look at adc5_get_fw_channel_data, specifically
> > > where it calls fwnode_property_read_string() to overwrite
> > > prop->datasheet_name.  
> > 
> > Ah. Thanks for the pointer, though I'm still confused.
> > 
> > 	ret = fwnode_property_read_string(fwnode, "label", &channel_name);
> > 	if (ret)
> > 		channel_name = name;  
>   		             ^ here ^
> 
> > 	prop->datasheet_name = channel_name;
> > 
> > That's reading the label property, not the node name.  
> 
> The node name sits in `name`, and that's used if there's no "label"
> property in wich case ret is non-zero and we end up in `if (ret)
> channel_name = name;`.
Ah. Missed that.  thanks!
> 
> > > > > > 4. If a label (or node name) is not set, do we fall back to
> > > > > >    datasheet_name hardcoded in the driver?    
> > > > 
> > > > Hmm. Probably not.    
> > > 
> > > Then we might as well remove this useless data from the kernel driver
> > > altogether...  
> > 
> > Ok. May make sense to use the datasheet name if noting better provided
> > for the label.  Assuming the datasheet names are them selves somewhat
> > useful information for a user.  
> 
> They're generated from the macro (hence capitalized) in VADC, manually
> written in ADC5.  Would it make sense to add handwritten string
> literals for this?

Not sure. I've rather lost track of where we are on this.
> 
> > > > > > 5. What do we use for datasheet_name vs extend_name?    
> > > > Expand that to include label.
> > > > datasheet_name : When you want to have human readable pin names from the ADC
> > > >   datasheet, used as part of provide services to consumer drivers. Doesn't
> > > >   work with DT though as it wasn't part of the binding for consumers.
> > > >   So largely irrelevant unless you have an MFD where the ADC consumers are
> > > >   also part of the MFD children and so the map is set up in the way we used
> > > >   to do it for board files.    
> > > 
> > > ... or this could remain to feed into datasheet_name?  
> > Now I'm confused.  Feed into label perhaps?  
> 
> Feed into read_label when no label was otherwise provided in DTS, but
> always feed into iio_chan_spec::datasheet_name since we discussed that
> this should represent the name of the part (e.g. PMIC), not the board
> and way in which it consumes the channel.

Should be the name of the pin on the part, but otherwise agreed.

> 
> <snip>
> 
> > > Do we then remove extend_name from qcom-spmi-adc5 and give it the same
> > > treatment, since it would now use DT node names as filenames unless a
> > > label is set?  I can only imagine it having been set because the ADC5
> > > author(s) didn't see a name nor label in sysfs either, without knowing
> > > about the existence of read_label.  
> > 
> > Sadly we can't remove it because of the ABI change that would result and
> > potential userspace breakage.  
> 
> The change to fwnode_get_name is already breaking this sysfs ABI though,
> as discussed in a DTS series where I replaced all node names with labels
> to support (the followup of) this iio series.
> 
> We could unbreak it by either stripping @xx off of it, or setting
> extend_name to a label (if it exists) instead, but the latter is
> breaking :(
> 
> <snip>

I hate it when we break ABI and don't notice, precisely because it then
becomes guessing game on which one people might be relying on.

Let's take the view it is the older one without the @xx? 
So strip that off as a fix that we backport.

> 
> > > > Hope that helps.    
> > > 
> > > A lot, now knowing that read_label is the part of the puzzle I
> > > previously missed.  Thanks!  
> > 
> > When I let the extend_name fallback in for the labels
> > it didn't occur to me that it would make it more confusing for
> > people looking at older code.  Long shot, but would a comment
> > in iio.h for extend_name to say something to this effect be likely
> > to have been something you'd have seen?  If it would, let's add
> > one to potentially make this less confusing for the next person!  
> 
> Yes, I think I visited the documentation/definition of extend_name at
> some point and would have been been helped if that pointed me right over
> to read_label.  Right in ADC5 (and other drivers) would be even better
> but may be overly verbose.
I agree that a cross reference for that 'other' use of extend_name
would make sense and that it can be overridden.  Though the override is
kind of the common case, if you are looking at extend_name docs you
are presumably in the case where such docs would help.
Patches for docs welcome :) 
> 
> Regardless of that VADC/ADC5 do some _really confusing_ things, passing
> strings around in various weird ways (or not), and it took some time to
> keep the various similar structs apart :)

I'm feeling a bit guilty I never noticed this insanity at the time.
I blame my younger self.

Have a good break if you are having one.

Jonathan
> 
> - Marijn

Marijn Suijten Jan. 16, 2023, 9:18 p.m. UTC | #7

On 2022-12-23 17:44:45, Jonathan Cameron wrote:

<snip>

> > As in, as long as we don't touch extend_name which would affect sysfs
> > names, changing the label returned by read_label is fine? 
> 
> Sticky corner, but I think we should be fine doing so on basis of changing
> ABI we don't think anyone will notice. In lots of cases the label is effected
> by DTS files that may change under the kernel anyway so hopefully no one is
> relying on the value too much *crossed fingers*

Not sure I'd go as far as implementing read_label in adc5 (just queued
that up locally for vadc though and it's simple) only to get around
extend_name, but we could do so.  That still leaves @xx in the filename
and makes for general inconsistency when read_label (returned label name
from sysfs) and extend_name (filename in sysfs) use different codepaths
to determine their value.

> > And changing
> > datasheet_name to only ever use the datasheet_name provided by the
> > driver and never the name provided in DTS is also okay?
> 
> datasheet_name is internal to kernel only so can definitely change that
> as long as we don't have any upstream users (I'm fairly sure there aren't any)

Ack, queued up for RFC v2.

> > I am unfortunately completely unfamiliar with iio_map, and hope it
> > doesn't distract too much from trying to add label files to QCom's SPMI
> > VADC driver :)
> 
> Just think of it as the board file way of doing equivalent of what we have
> to set up IIO consumers in DT. It's also used in driver that hard code
> relationships with their consumers - not including this one so we should
> be fine.

Guess QCOM is all DT, so this shouldn't matter.

<snip>

> > > Ok. May make sense to use the datasheet name if noting better provided
> > > for the label.  Assuming the datasheet names are them selves somewhat
> > > useful information for a user.  
> > 
> > They're generated from the macro (hence capitalized) in VADC, manually
> > written in ADC5.  Would it make sense to add handwritten string
> > literals for this?
> 
> Not sure. I've rather lost track of where we are on this.

I'll send a v2 RFC shortly with what we've accumulated thus far, and
will make sure to mention this.  In short, in qcom-spmi-vadc.c there is:

    .datasheet_name = __stringify(_dname),

Which gives us a full-caps DIE_TEMP, for example, instead of a more
friendly "die_temp" string literal in qcom-spmi-adc5.c.  Not a
requirement for my patches, this should all go in separate bits.

<snip>

> > Feed into read_label when no label was otherwise provided in DTS, but
> > always feed into iio_chan_spec::datasheet_name since we discussed that
> > this should represent the name of the part (e.g. PMIC), not the board
> > and way in which it consumes the channel.
> 
> Should be the name of the pin on the part, but otherwise agreed.

Ack, I think that's what we have in the qcom-spmi-vadc/adc5 drivers
currently.

<snip>

> I hate it when we break ABI and don't notice, precisely because it then
> becomes guessing game on which one people might be relying on.
> 
> Let's take the view it is the older one without the @xx? 
> So strip that off as a fix that we backport.

I was intending to send a patch that falls back to
adc5_channels::datasheet_name when no label is supplied (and ignoring
fwnode_name for use in extend_name altogether) but that's breaking ABI
in a slightly different way... and depends on my "arm64: dts: qcom: Use
labels with generic node names for ADC channels" [1] RFC being resent
and actually landing.

[1]: https://lore.kernel.org/linux-arm-msm/[e-mail]/

Will probably be shot down but I'll give it a try anyway.

<snip>

> I agree that a cross reference for that 'other' use of extend_name
> would make sense and that it can be overridden.  Though the override is
> kind of the common case, if you are looking at extend_name docs you
> are presumably in the case where such docs would help.
> Patches for docs welcome :) 

I'll see what I can add :)

> > Regardless of that VADC/ADC5 do some _really confusing_ things, passing
> > strings around in various weird ways (or not), and it took some time to
> > keep the various similar structs apart :)
> 
> I'm feeling a bit guilty I never noticed this insanity at the time.
> I blame my younger self.

Don't worry, we live and learn; just ABI (for something I doubt anyone
uses / cares about...) biting us every once in a while.

> Have a good break if you are having one.

Hope you had a good one; mine was filled with hardly any free time for
hobbies like the mainline kernel :)

- Marijn

Patch

diff --git a/drivers/iio/adc/qcom-spmi-vadc.c b/drivers/iio/adc/qcom-spmi-vadc.c
index bcff0f62b70e..8c6c7fa13cfe 100644
--- a/drivers/iio/adc/qcom-spmi-vadc.c
+++ b/drivers/iio/adc/qcom-spmi-vadc.c
@@ -84,6 +84,7 @@ 
  *	that is an average of multiple measurements.
  * @scale_fn_type: Represents the scaling function to convert voltage
  *	physical units desired by the client for the channel.
+ * @datasheet_name: Channel name used in device tree.
  */
 struct vadc_channel_prop {
 	unsigned int channel;
@@ -93,6 +94,7 @@  struct vadc_channel_prop {
 	unsigned int hw_settle_time;
 	unsigned int avg_samples;
 	enum vadc_scale_fn_type scale_fn_type;
+	const char *datasheet_name;
 };

 /**
@@ -652,7 +654,7 @@  static int vadc_get_fw_channel_data(struct device *dev,
 				    struct vadc_channel_prop *prop,
 				    struct fwnode_handle *fwnode)
 {
-	const char *name = fwnode_get_name(fwnode);
+	const char *name = fwnode_get_name(fwnode), *channel_name;
 	u32 chan, value, varr[2];
 	int ret;

@@ -670,6 +672,12 @@  static int vadc_get_fw_channel_data(struct device *dev,
 	/* the channel has DT description */
 	prop->channel = chan;

+	ret = fwnode_property_read_string(fwnode, "label", &channel_name);
+	if (ret)
+		channel_name = name;
+
+	prop->datasheet_name = channel_name;
+
 	ret = fwnode_property_read_u32(fwnode, "qcom,decimation", &value);
 	if (!ret) {
 		ret = qcom_vadc_decimation_from_dt(value);
@@ -771,6 +779,7 @@  static int vadc_get_fw_data(struct vadc_priv *vadc)

 		iio_chan->channel = prop.channel;
 		iio_chan->datasheet_name = vadc_chan->datasheet_name;
+		iio_chan->extend_name = prop.datasheet_name;
 		iio_chan->info_mask_separate = vadc_chan->info_mask;
 		iio_chan->type = vadc_chan->type;
 		iio_chan->indexed = 1;