lkml.org 
[lkml]   [2022]   [Dec]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC PATCH] iio: adc: qcom-spmi-vadc: Propagate fw node name/label to extend_name
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 <marijn.suijten@somainline.org> wrote:
>
> > On 2022-11-12 16:27:19, Jonathan Cameron wrote:
> > > On Sun, 6 Nov 2022 21:24:45 +0100
> > > Marijn Suijten <marijn.suijten@somainline.org> 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

\
 
 \ /
  Last update: 2023-03-26 23:17    [W:0.064 / U:1.524 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site