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@xxxxxxxxxxxxxx> wrote: > > > On 2022-11-12 16:27:19, Jonathan Cameron wrote: > > > On Sun, 6 Nov 2022 21:24:45 +0100 > > > Marijn Suijten <marijn.suijten@xxxxxxxxxxxxxx> 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
