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@xxxxxxxxxxxxxx>
>
> ---
>
> 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@xxxxxxxxx/T/#u
> [2]: https://lore.kernel.org/linux-arm-msm/20221101161801.1058969-2-luca@xxxxxxxxx/
> [3]: https://lore.kernel.org/linux-arm-msm/20220926190148.283805-1-marijn.suijten@xxxxxxxxxxxxxx/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
>
