[PATCH RESEND v3 0/5] iio: adc: qcom-spmi-vadc: Propagate fw node label to userspace

[PATCH RESEND v3 0/5] iio: adc: qcom-spmi-vadc: Propagate fw node label to userspace

[Marijn Suijten]

Implement read_label in qcom-spmi-vadc to see DT-specified label names
in userspace.  At the same time clear up some documentation around
extend_name to promote read_label, and normalize similar code in
qcom-spmi-adc5.

v3 resend: added missing to/cc addresses via b4 prep --auto-to-cc.

Changes since v2:
- Dropped RFC tag;
- Reworded @extend_name deprecation comment.

v2: https://lore.kernel.org/r/20230116220909.196926-1-marijn.suijten@somainline.org

Changes since v1:
- qcom-spmi-vadc: Use read_label instead of extend_name.

New since v1:
- core: Point users of extend_name field to read_label callback
- qcom-spmi-adc5: Use datasheet_name string literal for
  iio_chan_spec::datasheet_name;
- qcom-spmi-adc5: Fall back to datasheet_name instead of
  fwnode_get_name() for iio_chan_spec::extend_name (gets rid of @xx in
  sysfs filenames and labels);
- qcom-spmi-adc5: Remove unnecessary datasheet_name NULL check.

v1: https://lore.kernel.org/linux-arm-msm/20221106193018.270106-1-marijn.suijten@somainline.org/
---
Marijn Suijten (5):
      iio: core: Point users of extend_name field to read_label callback
      iio: adc: qcom-spmi-adc5: Use driver datasheet_name instead of DT label
      iio: adc: qcom-spmi-adc5: Fall back to datasheet_name instead of fwnode name
      iio: adc: qcom-spmi-adc5: Remove unnecessary datasheet_name NULL check
      iio: adc: qcom-spmi-vadc: Propagate fw node label to userspace

 drivers/iio/adc/qcom-spmi-adc5.c | 15 +++++++--------
 drivers/iio/adc/qcom-spmi-vadc.c | 19 ++++++++++++++++++-
 include/linux/iio/iio.h          |  3 +++
 3 files changed, 28 insertions(+), 9 deletions(-)
---
base-commit: 92e815cf07ed24ee1c51b122f24ffcf2964b4b13
change-id: 20230502-iio-adc-propagate-fw-node-label-b1fff2e63ae8

[PATCH RESEND v3 1/5] iio: core: Point users of extend_name field to read_label callback

[Marijn Suijten]

As mentioned and discussed in [1] extend_name should not be used for
full channel labels (and most drivers seem to only use it to express a
short type of a channel) as this affects sysfs filenames, while the
label name is supposed to be extracted from the *_label sysfs file
instead.  This appears to have been unclear to some drivers as
extend_name is also used when read_label is unset, achieving an initial
goal of providing sensible names in *_label sysfs files without noticing
that sysfs filenames are (negatively and likely unintentionally)
affected as well.

Point readers of iio_chan_spec::extend_name to iio_info::read_label by
mentioning deprecation and side-effects of this field.

[1]: https://lore.kernel.org/linux-arm-msm/20221221223432.si2aasbleiicayfl@SoMainline.org/

Suggested-by: Jonathan Cameron <jic23@kernel.org>
Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
---
 include/linux/iio/iio.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index 81413cd3a3e7a..6fc06063505a7 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -221,6 +221,9 @@ struct iio_event_spec {
  * @extend_name:	Allows labeling of channel attributes with an
  *			informative name. Note this has no effect codes etc,
  *			unlike modifiers.
+ *			This field is deprecated in favour of providing
+ *			iio_info->read_label() to override the label, which
+ *			unlike @extend_name does not affect sysfs filenames.
  * @datasheet_name:	A name used in in-kernel mapping of channels. It should
  *			correspond to the first name that the channel is referred
  *			to by in the datasheet (e.g. IND), or the nearest

-- 
2.40.1

[PATCH RESEND v3 2/5] iio: adc: qcom-spmi-adc5: Use driver datasheet_name instead of DT label

[Marijn Suijten]

iio_chan_spec::datasheet_name expects a channel/pin name on the hardware
part, i.e. from its datasheet, instead of a friendly name from DT which
typically describes the use of said channel.  GPIO channels are commonly
specialized in QCOM board DTS based on what a - typically thermistor -
is connected to.

Also rename adc5_channel_prop::datasheet_name to channel_name to that
effect.

Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
---
 drivers/iio/adc/qcom-spmi-adc5.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/adc/qcom-spmi-adc5.c b/drivers/iio/adc/qcom-spmi-adc5.c
index c2d5e06f137a7..6e4e5cb5cbbb7 100644
--- a/drivers/iio/adc/qcom-spmi-adc5.c
+++ b/drivers/iio/adc/qcom-spmi-adc5.c
@@ -114,7 +114,7 @@ enum adc5_cal_val {
  *	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.
+ * @channel_name: Channel name used in device tree.
  */
 struct adc5_channel_prop {
 	unsigned int		channel;
@@ -126,7 +126,7 @@ struct adc5_channel_prop {
 	unsigned int		hw_settle_time;
 	unsigned int		avg_samples;
 	enum vadc_scale_fn_type	scale_fn_type;
-	const char		*datasheet_name;
+	const char		*channel_name;
 };
 
 /**
@@ -671,7 +671,7 @@ static int adc5_get_fw_channel_data(struct adc5_chip *adc,
 	if (ret)
 		channel_name = name;
 
-	prop->datasheet_name = channel_name;
+	prop->channel_name = channel_name;
 
 	ret = fwnode_property_read_u32(fwnode, "qcom,decimation", &value);
 	if (!ret) {
@@ -861,8 +861,8 @@ static int adc5_get_fw_data(struct adc5_chip *adc)
 		adc_chan = &adc->data->adc_chans[prop.channel];
 
 		iio_chan->channel = prop.channel;
-		iio_chan->datasheet_name = prop.datasheet_name;
-		iio_chan->extend_name = prop.datasheet_name;
+		iio_chan->datasheet_name = adc_chan->datasheet_name;
+		iio_chan->extend_name = prop.channel_name;
 		iio_chan->info_mask_separate = adc_chan->info_mask;
 		iio_chan->type = adc_chan->type;
 		iio_chan->address = index;

-- 
2.40.1

[PATCH RESEND v3 3/5] iio: adc: qcom-spmi-adc5: Fall back to datasheet_name instead of fwnode name

[Marijn Suijten]

Since the migration to fwnode_get_name in commit 4f47a236a23d ("iio:
adc: qcom-spmi-adc5: convert to device properties") the resulting
adc5_channel_prop::channel_name (renamed from datasheet_name in the
previous patch) - which is propagated into iio_chan_spec::extend_name -
was containing the DT node name including @xx suffix if a "label"
property is not present, while adc5_channels::datasheet_name was thus
far set by the macros but always remained unread.  Put it to use instead
of using a confusing name containing @xx in sysfs filenames (again, when
"label" is not set).

Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
---
 drivers/iio/adc/qcom-spmi-adc5.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/adc/qcom-spmi-adc5.c b/drivers/iio/adc/qcom-spmi-adc5.c
index 6e4e5cb5cbbb7..fe2000209d97d 100644
--- a/drivers/iio/adc/qcom-spmi-adc5.c
+++ b/drivers/iio/adc/qcom-spmi-adc5.c
@@ -669,7 +669,7 @@ static int adc5_get_fw_channel_data(struct adc5_chip *adc,
 
 	ret = fwnode_property_read_string(fwnode, "label", &channel_name);
 	if (ret)
-		channel_name = name;
+		channel_name = data->adc_chans[chan].datasheet_name;
 
 	prop->channel_name = channel_name;
 

-- 
2.40.1

[PATCH RESEND v3 4/5] iio: adc: qcom-spmi-adc5: Remove unnecessary datasheet_name NULL check

[Marijn Suijten]

datasheet_name is statically filled by a macro for every channel, and is
nor should ever be set to NULL.

Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
---
 drivers/iio/adc/qcom-spmi-adc5.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/iio/adc/qcom-spmi-adc5.c b/drivers/iio/adc/qcom-spmi-adc5.c
index fe2000209d97d..0a4fd3a461138 100644
--- a/drivers/iio/adc/qcom-spmi-adc5.c
+++ b/drivers/iio/adc/qcom-spmi-adc5.c
@@ -657,8 +657,7 @@ static int adc5_get_fw_channel_data(struct adc5_chip *adc,
 		chan = chan & ADC_CHANNEL_MASK;
 	}
 
-	if (chan > ADC5_PARALLEL_ISENSE_VBAT_IDATA ||
-	    !data->adc_chans[chan].datasheet_name) {
+	if (chan > ADC5_PARALLEL_ISENSE_VBAT_IDATA) {
 		dev_err(dev, "%s invalid channel number %d\n", name, chan);
 		return -EINVAL;
 	}

-- 
2.40.1

[PATCH RESEND v3 5/5] iio: adc: qcom-spmi-vadc: Propagate fw node label to userspace

[Marijn Suijten]

Set the read_label() callback to return a friendly name provided in DT
(firmware), in order to make in_{therm,voltage}X_label attributes show
up in sysfs for userspace to consume a channel name.  This is
particularly useful for custom thermistors being attached to otherwise
generically named GPIOs, where the name is known by the board DT.

If the channel name isn't set in DT, use the datasheet_name hardcoded in
the driver instead.

Note that this doesn't fall back to fwnode_get_name() as that provides
suboptimally readable names, with an @xx address suffix from board DT.

Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
---
 drivers/iio/adc/qcom-spmi-vadc.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/adc/qcom-spmi-vadc.c b/drivers/iio/adc/qcom-spmi-vadc.c
index bcff0f62b70e0..f5c6f1f27b2c7 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.
+ * @channel_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 *channel_name;
 };
 
 /**
@@ -495,8 +497,18 @@ static int vadc_fwnode_xlate(struct iio_dev *indio_dev,
 	return -EINVAL;
 }
 
+static int vadc_read_label(struct iio_dev *indio_dev,
+			   struct iio_chan_spec const *chan, char *label)
+{
+	struct vadc_priv *vadc = iio_priv(indio_dev);
+	const char *name = vadc->chan_props[chan->address].channel_name;
+
+	return sysfs_emit(label, "%s\n", name);
+}
+
 static const struct iio_info vadc_info = {
 	.read_raw = vadc_read_raw,
+	.read_label = vadc_read_label,
 	.fwnode_xlate = vadc_fwnode_xlate,
 };
 
@@ -652,7 +664,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), *label;
 	u32 chan, value, varr[2];
 	int ret;
 
@@ -667,6 +679,11 @@ static int vadc_get_fw_channel_data(struct device *dev,
 		return -EINVAL;
 	}
 
+	ret = fwnode_property_read_string(fwnode, "label", &label);
+	if (ret)
+		label = vadc_chans[chan].datasheet_name;
+	prop->channel_name = label;
+
 	/* the channel has DT description */
 	prop->channel = chan;
 

-- 
2.40.1

Re: [PATCH RESEND v3 0/5] iio: adc: qcom-spmi-vadc: Propagate fw node label to userspace

[Jonathan Cameron]

On Tue,  2 May 2023 01:17:32 +0200
Marijn Suijten <marijn.suijten@somainline.org> wrote:

> Implement read_label in qcom-spmi-vadc to see DT-specified label names
> in userspace.  At the same time clear up some documentation around
> extend_name to promote read_label, and normalize similar code in
> qcom-spmi-adc5.

Whole series looks good to me. I'd like some review from QCOM focused
folk though before I take it.

Thanks,

Jonathan

> 
> v3 resend: added missing to/cc addresses via b4 prep --auto-to-cc.
> 
> Changes since v2:
> - Dropped RFC tag;
> - Reworded @extend_name deprecation comment.
> 
> v2: https://lore.kernel.org/r/20230116220909.196926-1-marijn.suijten@somainline.org
> 
> Changes since v1:
> - qcom-spmi-vadc: Use read_label instead of extend_name.
> 
> New since v1:
> - core: Point users of extend_name field to read_label callback
> - qcom-spmi-adc5: Use datasheet_name string literal for
>   iio_chan_spec::datasheet_name;
> - qcom-spmi-adc5: Fall back to datasheet_name instead of
>   fwnode_get_name() for iio_chan_spec::extend_name (gets rid of @xx in
>   sysfs filenames and labels);
> - qcom-spmi-adc5: Remove unnecessary datasheet_name NULL check.
> 
> v1: https://lore.kernel.org/linux-arm-msm/20221106193018.270106-1-marijn.suijten@somainline.org/
> ---
> Marijn Suijten (5):
>       iio: core: Point users of extend_name field to read_label callback
>       iio: adc: qcom-spmi-adc5: Use driver datasheet_name instead of DT label
>       iio: adc: qcom-spmi-adc5: Fall back to datasheet_name instead of fwnode name
>       iio: adc: qcom-spmi-adc5: Remove unnecessary datasheet_name NULL check
>       iio: adc: qcom-spmi-vadc: Propagate fw node label to userspace
> 
>  drivers/iio/adc/qcom-spmi-adc5.c | 15 +++++++--------
>  drivers/iio/adc/qcom-spmi-vadc.c | 19 ++++++++++++++++++-
>  include/linux/iio/iio.h          |  3 +++
>  3 files changed, 28 insertions(+), 9 deletions(-)
> ---
> base-commit: 92e815cf07ed24ee1c51b122f24ffcf2964b4b13
> change-id: 20230502-iio-adc-propagate-fw-node-label-b1fff2e63ae8

Re: [PATCH RESEND v3 5/5] iio: adc: qcom-spmi-vadc: Propagate fw node label to userspace

[Dmitry Baryshkov]

On 02/05/2023 02:17, Marijn Suijten wrote:
> Set the read_label() callback to return a friendly name provided in DT
> (firmware), in order to make in_{therm,voltage}X_label attributes show
> up in sysfs for userspace to consume a channel name.  This is
> particularly useful for custom thermistors being attached to otherwise
> generically named GPIOs, where the name is known by the board DT.
> 
> If the channel name isn't set in DT, use the datasheet_name hardcoded in
> the driver instead.
> 
> Note that this doesn't fall back to fwnode_get_name() as that provides
> suboptimally readable names, with an @xx address suffix from board DT.
> 
> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> ---
>   drivers/iio/adc/qcom-spmi-vadc.c | 19 ++++++++++++++++++-
>   1 file changed, 18 insertions(+), 1 deletion(-)

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

-- 
With best wishes
Dmitry

Re: [PATCH RESEND v3 2/5] iio: adc: qcom-spmi-adc5: Use driver datasheet_name instead of DT label

[Dmitry Baryshkov]

On 02/05/2023 02:17, Marijn Suijten wrote:
> iio_chan_spec::datasheet_name expects a channel/pin name on the hardware
> part, i.e. from its datasheet, instead of a friendly name from DT which
> typically describes the use of said channel.  GPIO channels are commonly
> specialized in QCOM board DTS based on what a - typically thermistor -
> is connected to.
> 
> Also rename adc5_channel_prop::datasheet_name to channel_name to that
> effect.
> 
> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> ---
>   drivers/iio/adc/qcom-spmi-adc5.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

-- 
With best wishes
Dmitry

Re: [PATCH RESEND v3 3/5] iio: adc: qcom-spmi-adc5: Fall back to datasheet_name instead of fwnode name

[Dmitry Baryshkov]

On 02/05/2023 02:17, Marijn Suijten wrote:
> Since the migration to fwnode_get_name in commit 4f47a236a23d ("iio:
> adc: qcom-spmi-adc5: convert to device properties") the resulting
> adc5_channel_prop::channel_name (renamed from datasheet_name in the
> previous patch) - which is propagated into iio_chan_spec::extend_name -
> was containing the DT node name including @xx suffix if a "label"
> property is not present, while adc5_channels::datasheet_name was thus
> far set by the macros but always remained unread.  Put it to use instead
> of using a confusing name containing @xx in sysfs filenames (again, when
> "label" is not set).
> 
> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> ---
>   drivers/iio/adc/qcom-spmi-adc5.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

-- 
With best wishes
Dmitry

Re: [PATCH RESEND v3 4/5] iio: adc: qcom-spmi-adc5: Remove unnecessary datasheet_name NULL check

[Dmitry Baryshkov]

On 02/05/2023 02:17, Marijn Suijten wrote:
> datasheet_name is statically filled by a macro for every channel, and is
> nor should ever be set to NULL.
> 
> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> ---
>   drivers/iio/adc/qcom-spmi-adc5.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

-- 
With best wishes
Dmitry

Re: [PATCH RESEND v3 5/5] iio: adc: qcom-spmi-vadc: Propagate fw node label to userspace

[Jonathan Cameron]

On Sun, 7 May 2023 18:43:16 +0300
Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:

> On 02/05/2023 02:17, Marijn Suijten wrote:
> > Set the read_label() callback to return a friendly name provided in DT
> > (firmware), in order to make in_{therm,voltage}X_label attributes show
> > up in sysfs for userspace to consume a channel name.  This is
> > particularly useful for custom thermistors being attached to otherwise
> > generically named GPIOs, where the name is known by the board DT.
> > 
> > If the channel name isn't set in DT, use the datasheet_name hardcoded in
> > the driver instead.
> > 
> > Note that this doesn't fall back to fwnode_get_name() as that provides
> > suboptimally readable names, with an @xx address suffix from board DT.
> > 
> > Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> > ---
> >   drivers/iio/adc/qcom-spmi-vadc.c | 19 ++++++++++++++++++-
> >   1 file changed, 18 insertions(+), 1 deletion(-)  
> 
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> 

Thanks,

Series applied to the togreg branch of iio.git - initially pushed out
as testing to let 0-day work it's magic before I go potentially making
a mess in linux-next.

Jonathan