| Message ID | 20221106193018.270106-1-marijn.suijten@somainline.org |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] iio: adc: qcom-spmi-vadc: Propagate fw node name/label to extend_name | expand |
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 <marijn.suijten@somainline.org> > > --- > > 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/20221031181022.947412-1-luca@z3ntu.xyz/T/#u > [2]: https://lore.kernel.org/linux-arm-msm/20221101161801.1058969-2-luca@z3ntu.xyz/ > [3]: https://lore.kernel.org/linux-arm-msm/20220926190148.283805-1-marijn.suijten@somainline.org/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 >
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. > > Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org> > > > > --- > > > > 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/20221031181022.947412-1-luca@z3ntu.xyz/T/#u > > [2]: https://lore.kernel.org/linux-arm-msm/20221101161801.1058969-2-luca@z3ntu.xyz/ > > [3]: https://lore.kernel.org/linux-arm-msm/20220926190148.283805-1-marijn.suijten@somainline.org/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 > >
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. > > > Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org> > > > > > > --- > > > > > > 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
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. 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 <marijn.suijten@somainline.org> > > > > > > > > --- > > > > > > > > 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
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
On Wed, 21 Dec 2022 23:34:32 +0100 Marijn Suijten <marijn.suijten@somainline.org> 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 <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. > 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
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/20221209215308.1781047-1-marijn.suijten@somainline.org/ 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
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;
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 <marijn.suijten@somainline.org> --- 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/20221031181022.947412-1-luca@z3ntu.xyz/T/#u [2]: https://lore.kernel.org/linux-arm-msm/20221101161801.1058969-2-luca@z3ntu.xyz/ [3]: https://lore.kernel.org/linux-arm-msm/20220926190148.283805-1-marijn.suijten@somainline.org/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