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

[RFC PATCH v2 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.

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(-)

--
2.39.0

[RFC PATCH v2 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 81413cd3a3e7..36c89f238fb9 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 overriding read_label
+ *			in iio_info, 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.39.0

[RFC PATCH v2 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 e90c299c913a..26144a086fac 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;
 };
 
 /**
@@ -663,7 +663,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) {
@@ -853,8 +853,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.39.0

[RFC PATCH v2 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>
---

Note that for this to make sense a successor to "arm64: dts: qcom: Use
labels with generic node names for ADC channels" [1] will have to land,
otherwise most channels that currently pull datasheet_name/extend_name
from DT fall back to datasheet_name hardcoded in this driver.

[1]: https://lore.kernel.org/linux-arm-msm/20221209215308.1781047-1-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 26144a086fac..01aafd9df6c6 100644
--- a/drivers/iio/adc/qcom-spmi-adc5.c
+++ b/drivers/iio/adc/qcom-spmi-adc5.c
@@ -661,7 +661,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.39.0

[RFC PATCH v2 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 01aafd9df6c6..5def2b21f206 100644
--- a/drivers/iio/adc/qcom-spmi-adc5.c
+++ b/drivers/iio/adc/qcom-spmi-adc5.c
@@ -649,8 +649,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.39.0

[RFC PATCH v2 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 bcff0f62b70e..f5c6f1f27b2c 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.39.0

Re: [RFC PATCH v2 1/5] iio: core: Point users of extend_name field to read_label callback

[Jonathan Cameron]

On Mon, 16 Jan 2023 23:09:05 +0100
Marijn Suijten <marijn.suijten@somainline.org> wrote:

> 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 81413cd3a3e7..36c89f238fb9 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 overriding read_label
> + *			in iio_info, which unlike @extend_name does not affect sysfs
> + *			filenames.
Perhaps reword as

This field is deprecated in favour of overriding the default label
by providing a read_label() callback in iio_info, 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

Re: [RFC PATCH v2 1/5] iio: core: Point users of extend_name field to read_label callback

[Marijn Suijten]

On 2023-01-18 16:19:20, Jonathan Cameron wrote:
> On Mon, 16 Jan 2023 23:09:05 +0100
> Marijn Suijten <marijn.suijten@somainline.org> wrote:
> 
> > 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 81413cd3a3e7..36c89f238fb9 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 overriding read_label
> > + *			in iio_info, which unlike @extend_name does not affect sysfs
> > + *			filenames.
> Perhaps reword as
> 
> This field is deprecated in favour of overriding the default label
> by providing a read_label() callback in iio_info, which unlike
> @extend_name does not affect sysfs filenames.
> ?

Agreed, explicitly stating "the default label by" makes this much more
clear.  Though, maybe swap that around into "in favour of providing
read_label() in iio_info to override the label"?  Otherwise this could
be interpreted as "overriding the default label" is preferred to setting
extend_name... which one would do to override the default label.

I can queue this up for v3 unless you'll fix it up while applying,
presuming no other changes have to be made (aside from dropping patch
3/5).

Will read_label() turn into a link?  And is the @extend_name reference
proper?  Is there something to link to iio_info, perhaps a hashtag - or
maybe fully qualify `#iio_info::read_label()` for linking purposes?
/me jumps over to kerneldoc documentation :)

- Marijn

> >   * @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
> 

Re: [RFC PATCH v2 1/5] iio: core: Point users of extend_name field to read_label callback

[Marijn Suijten]

On 2023-01-18 17:35:27, Marijn Suijten wrote:
> On 2023-01-18 16:19:20, Jonathan Cameron wrote:
> > On Mon, 16 Jan 2023 23:09:05 +0100
> > Marijn Suijten <marijn.suijten@somainline.org> wrote:
> > 
> > > 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 81413cd3a3e7..36c89f238fb9 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 overriding read_label
> > > + *			in iio_info, which unlike @extend_name does not affect sysfs
> > > + *			filenames.
> > Perhaps reword as
> > 
> > This field is deprecated in favour of overriding the default label
> > by providing a read_label() callback in iio_info, which unlike
> > @extend_name does not affect sysfs filenames.
> > ?
> 
> Agreed, explicitly stating "the default label by" makes this much more
> clear.  Though, maybe swap that around into "in favour of providing
> read_label() in iio_info to override the label"?  Otherwise this could
> be interpreted as "overriding the default label" is preferred to setting
> extend_name... which one would do to override the default label.
> 
> I can queue this up for v3 unless you'll fix it up while applying,
> presuming no other changes have to be made (aside from dropping patch
> 3/5).
> 
> Will read_label() turn into a link?  And is the @extend_name reference

Yes, if read_label is a function that exists (but it's a member)... and
@extend_name will indeed reference the member anywhere.

> proper?  Is there something to link to iio_info, perhaps a hashtag - or
> maybe fully qualify `#iio_info::read_label()` for linking purposes?
> /me jumps over to kerneldoc documentation :)

Looks like that would be &iio_info->read_label() (not sure about the
trailing () though):
https://docs.kernel.org/doc-guide/kernel-doc.html#highlights-and-cross-references

- Marijn

Re: [RFC PATCH v2 1/5] iio: core: Point users of extend_name field to read_label callback

[Jonathan Cameron]

On Wed, 18 Jan 2023 17:35:25 +0100
Marijn Suijten <marijn.suijten@somainline.org> wrote:

> On 2023-01-18 16:19:20, Jonathan Cameron wrote:
> > On Mon, 16 Jan 2023 23:09:05 +0100
> > Marijn Suijten <marijn.suijten@somainline.org> wrote:
> >   
> > > 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 81413cd3a3e7..36c89f238fb9 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 overriding read_label
> > > + *			in iio_info, which unlike @extend_name does not affect sysfs
> > > + *			filenames.  
> > Perhaps reword as
> > 
> > This field is deprecated in favour of overriding the default label
> > by providing a read_label() callback in iio_info, which unlike
> > @extend_name does not affect sysfs filenames.
> > ?  
> 
> Agreed, explicitly stating "the default label by" makes this much more
> clear.  Though, maybe swap that around into "in favour of providing
> read_label() in iio_info to override the label"?  Otherwise this could
> be interpreted as "overriding the default label" is preferred to setting
> extend_name... which one would do to override the default label.

Sure that works.
> 
> I can queue this up for v3 unless you'll fix it up while applying,
> presuming no other changes have to be made (aside from dropping patch
> 3/5).
I haven't looked closely enough at the rest, but if this is all that
needs updating I'll change it whilst applying.

> 
> Will read_label() turn into a link?  And is the @extend_name reference
> proper?  Is there something to link to iio_info, perhaps a hashtag - or
> maybe fully qualify `#iio_info::read_label()` for linking purposes?
> /me jumps over to kerneldoc documentation :)
> 
> - Marijn
> 
> > >   * @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  
> >   
> 

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

[Jonathan Cameron]

On Mon, 16 Jan 2023 23:09:04 +0100
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.
> 

I think this is a good route forwards, but it is making changes
to ABI so I definitely want input on this from at least one of
the qualcomm maintainers before I pick it up - particularly
the changes in patch 3.

Don't want to cause anyone nasty surprises.

J

> 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(-)
> 
> --
> 2.39.0
> 

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

[Marijn Suijten]

On 2023-01-22 16:59:47, Jonathan Cameron wrote:
> On Mon, 16 Jan 2023 23:09:04 +0100
> 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.
> > 
> 
> I think this is a good route forwards, but it is making changes
> to ABI so I definitely want input on this from at least one of
> the qualcomm maintainers before I pick it up - particularly
> the changes in patch 3.
> 
> Don't want to cause anyone nasty surprises.

Ack, patch 3 is the odd one of the bunch (as discussed many times
prior).  It is an ABI break and would effectively obsolete "iio: adc:
qcom-spmi-adc5: Fix the channel name" [1] as it then only affects a few
dev_err/dev_dbg.  Let's wait and hear from Qcom maintainers.

[1]: https://lore.kernel.org/linux-arm-msm/20230118100623.42255-1-andriy.shevchenko@linux.intel.com/

- Marijn

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

[Jonathan Cameron]

On Mon, 23 Jan 2023 00:41:37 +0100
Marijn Suijten <marijn.suijten@somainline.org> wrote:

> On 2023-01-22 16:59:47, Jonathan Cameron wrote:
> > On Mon, 16 Jan 2023 23:09:04 +0100
> > 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.
> > >   
> > 
> > I think this is a good route forwards, but it is making changes
> > to ABI so I definitely want input on this from at least one of
> > the qualcomm maintainers before I pick it up - particularly
> > the changes in patch 3.
> > 
> > Don't want to cause anyone nasty surprises.  
> 
> Ack, patch 3 is the odd one of the bunch (as discussed many times
> prior).  It is an ABI break and would effectively obsolete "iio: adc:
> qcom-spmi-adc5: Fix the channel name" [1] as it then only affects a few
> dev_err/dev_dbg.  Let's wait and hear from Qcom maintainers.
> 
> [1]: https://lore.kernel.org/linux-arm-msm/20230118100623.42255-1-andriy.shevchenko@linux.intel.com/

Perhaps time for a resend of this series (drop the RFC marking as that
tends to mean people don't read things if they are low on time).

I'm going to mark this in patchwork as changes requested on basis a
v3 will show up shortly.

Thanks,

Jonathan

> 
> - Marijn

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

[Marijn Suijten]

On 2023-05-01 17:21:51, Jonathan Cameron wrote:
> On Mon, 23 Jan 2023 00:41:37 +0100
> Marijn Suijten <marijn.suijten@somainline.org> wrote:
> 
> > On 2023-01-22 16:59:47, Jonathan Cameron wrote:
> > > On Mon, 16 Jan 2023 23:09:04 +0100
> > > 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.
> > > >   
> > > 
> > > I think this is a good route forwards, but it is making changes
> > > to ABI so I definitely want input on this from at least one of
> > > the qualcomm maintainers before I pick it up - particularly
> > > the changes in patch 3.
> > > 
> > > Don't want to cause anyone nasty surprises.  
> > 
> > Ack, patch 3 is the odd one of the bunch (as discussed many times
> > prior).  It is an ABI break and would effectively obsolete "iio: adc:
> > qcom-spmi-adc5: Fix the channel name" [1] as it then only affects a few
> > dev_err/dev_dbg.  Let's wait and hear from Qcom maintainers.
> > 
> > [1]: https://lore.kernel.org/linux-arm-msm/20230118100623.42255-1-andriy.shevchenko@linux.intel.com/
> 
> Perhaps time for a resend of this series (drop the RFC marking as that
> tends to mean people don't read things if they are low on time).
> 
> I'm going to mark this in patchwork as changes requested on basis a
> v3 will show up shortly.

v3 is out now after once again messing with b4 to import an older
series, with a RESEND to include all the email addresses which b4 only
addes after using prep --auto-to-cc, and there not being any obvious way
to retroactively add the bunch of To:/Cc: commands to a previously sent
version before running b4 send --resend v3.

Fingers crossed it all went right and reaches the right people.

- Marijn