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

[Marijn Suijten]

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
>