[PATCH v2 0/2] hwmon: iio: Add labels

[PATCH v2 0/2] hwmon: iio: Add labels

[Sean Anderson]

Add support for using IIO channel labels for HWMON labels.

Changes in v2:
- Check if the label exists before creating the attribute

Sean Anderson (2):
  iio: Add iio_read_channel_label to inkern API
  hwmon: iio: Add labels from IIO channels

 drivers/hwmon/iio_hwmon.c       | 45 +++++++++++++++++++++++++++++----
 drivers/iio/iio_core.h          |  4 +++
 drivers/iio/industrialio-core.c | 23 ++++++++++-------
 drivers/iio/inkern.c            |  6 +++++
 include/linux/iio/consumer.h    | 10 ++++++++
 5 files changed, 74 insertions(+), 14 deletions(-)

-- 
2.35.1.1320.gc452695387.dirty

[PATCH v2 1/2] iio: Add iio_read_channel_label to inkern API

[Sean Anderson]

It can be convenient for other in-kernel drivers to reuse IIO channel
labels. Export the iio_read_channel_label function to allow this. The
signature is different depending on where we are calling it from, so
the meat is moved to do_iio_read_channel_label.

Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---

(no changes since v1)

 drivers/iio/iio_core.h          |  4 ++++
 drivers/iio/industrialio-core.c | 23 ++++++++++++++---------
 drivers/iio/inkern.c            |  6 ++++++
 include/linux/iio/consumer.h    | 10 ++++++++++
 4 files changed, 34 insertions(+), 9 deletions(-)

diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
index 1a38b1915e7a..b7d5f4f0fada 100644
--- a/drivers/iio/iio_core.h
+++ b/drivers/iio/iio_core.h
@@ -34,6 +34,10 @@ void iio_device_ioctl_handler_register(struct iio_dev *indio_dev,
 				       struct iio_ioctl_handler *h);
 void iio_device_ioctl_handler_unregister(struct iio_ioctl_handler *h);
 
+ssize_t do_iio_read_channel_label(struct iio_dev *indio_dev,
+				  const struct iio_chan_spec *c,
+				  char *buf);
+
 int __iio_add_chan_devattr(const char *postfix,
 			   struct iio_chan_spec const *chan,
 			   ssize_t (*func)(struct device *dev,
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 2f185b386949..0f6cda7ffe45 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -727,22 +727,27 @@ ssize_t iio_format_value(char *buf, unsigned int type, int size, int *vals)
 }
 EXPORT_SYMBOL_GPL(iio_format_value);
 
-static ssize_t iio_read_channel_label(struct device *dev,
-				      struct device_attribute *attr,
-				      char *buf)
+ssize_t do_iio_read_channel_label(struct iio_dev *indio_dev,
+				  const struct iio_chan_spec *c,
+				  char *buf)
 {
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
-
 	if (indio_dev->info->read_label)
-		return indio_dev->info->read_label(indio_dev, this_attr->c, buf);
+		return indio_dev->info->read_label(indio_dev, c, buf);
 
-	if (this_attr->c->extend_name)
-		return sysfs_emit(buf, "%s\n", this_attr->c->extend_name);
+	if (c->extend_name)
+		return sysfs_emit(buf, "%s\n", c->extend_name);
 
 	return -EINVAL;
 }
 
+static ssize_t iio_read_channel_label(struct device *dev,
+				      struct device_attribute *attr,
+				      char *buf)
+{
+	return do_iio_read_channel_label(dev_to_iio_dev(dev),
+					 to_iio_dev_attr(attr)->c, buf);
+}
+
 static ssize_t iio_read_channel_info(struct device *dev,
 				     struct device_attribute *attr,
 				     char *buf)
diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
index 39cf26d69d17..9f484c94bc6e 100644
--- a/drivers/iio/inkern.c
+++ b/drivers/iio/inkern.c
@@ -1010,3 +1010,9 @@ ssize_t iio_write_channel_ext_info(struct iio_channel *chan, const char *attr,
 			       chan->channel, buf, len);
 }
 EXPORT_SYMBOL_GPL(iio_write_channel_ext_info);
+
+ssize_t iio_read_channel_label(struct iio_channel *chan, char *buf)
+{
+	return do_iio_read_channel_label(chan->indio_dev, chan->channel, buf);
+}
+EXPORT_SYMBOL_GPL(iio_read_channel_label);
diff --git a/include/linux/iio/consumer.h b/include/linux/iio/consumer.h
index e9910b41d48e..333d1d8ccb37 100644
--- a/include/linux/iio/consumer.h
+++ b/include/linux/iio/consumer.h
@@ -441,4 +441,14 @@ ssize_t iio_read_channel_ext_info(struct iio_channel *chan,
 ssize_t iio_write_channel_ext_info(struct iio_channel *chan, const char *attr,
 				   const char *buf, size_t len);
 
+/**
+ * iio_read_channel_label() - read label for a given channel
+ * @chan:		The channel being queried.
+ * @buf:		Where to store the attribute value. Assumed to hold
+ *			at least PAGE_SIZE bytes.
+ *
+ * Returns the number of bytes written to buf, or an error code.
+ */
+ssize_t iio_read_channel_label(struct iio_channel *chan, char *buf);
+
 #endif
-- 
2.35.1.1320.gc452695387.dirty

[PATCH v2 2/2] hwmon: iio: Add labels from IIO channels

[Sean Anderson]

Add labels from IIO channels to our channels. This allows userspace to
display more meaningful names instead of "in0" or "temp5".

Although lm-sensors gracefully handles errors when reading channel
labels, the ABI says the label attribute

> Should only be created if the driver has hints about what this voltage
> channel is being used for, and user-space doesn't.

Therefore, we test to see if the channel has a label before
creating the attribute.

Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
---

Changes in v2:
- Check if the label exists before creating the attribute

 drivers/hwmon/iio_hwmon.c | 45 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 40 insertions(+), 5 deletions(-)

diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c
index 4c8a80847891..5722cb9d81f9 100644
--- a/drivers/hwmon/iio_hwmon.c
+++ b/drivers/hwmon/iio_hwmon.c
@@ -33,6 +33,17 @@ struct iio_hwmon_state {
 	struct attribute **attrs;
 };
 
+static ssize_t iio_hwmon_read_label(struct device *dev,
+				  struct device_attribute *attr,
+				  char *buf)
+{
+	struct sensor_device_attribute *sattr = to_sensor_dev_attr(attr);
+	struct iio_hwmon_state *state = dev_get_drvdata(dev);
+	struct iio_channel *chan = &state->channels[sattr->index];
+
+	return iio_read_channel_label(chan, buf);
+}
+
 /*
  * Assumes that IIO and hwmon operate in the same base units.
  * This is supposed to be true, but needs verification for
@@ -68,12 +79,13 @@ static int iio_hwmon_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct iio_hwmon_state *st;
 	struct sensor_device_attribute *a;
-	int ret, i;
+	int ret, i, attr = 0;
 	int in_i = 1, temp_i = 1, curr_i = 1, humidity_i = 1, power_i = 1;
 	enum iio_chan_type type;
 	struct iio_channel *channels;
 	struct device *hwmon_dev;
 	char *sname;
+	void *buf;
 
 	channels = devm_iio_channel_get_all(dev);
 	if (IS_ERR(channels)) {
@@ -85,17 +97,18 @@ static int iio_hwmon_probe(struct platform_device *pdev)
 	}
 
 	st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
-	if (st == NULL)
+	buf = (void *)devm_get_free_pages(dev, GFP_KERNEL, 0);
+	if (!st || !buf)
 		return -ENOMEM;
 
 	st->channels = channels;
 
-	/* count how many attributes we have */
+	/* count how many channels we have */
 	while (st->channels[st->num_channels].indio_dev)
 		st->num_channels++;
 
 	st->attrs = devm_kcalloc(dev,
-				 st->num_channels + 1, sizeof(*st->attrs),
+				 2 * st->num_channels + 1, sizeof(*st->attrs),
 				 GFP_KERNEL);
 	if (st->attrs == NULL)
 		return -ENOMEM;
@@ -147,9 +160,31 @@ static int iio_hwmon_probe(struct platform_device *pdev)
 		a->dev_attr.show = iio_hwmon_read_val;
 		a->dev_attr.attr.mode = 0444;
 		a->index = i;
-		st->attrs[i] = &a->dev_attr.attr;
+		st->attrs[attr++] = &a->dev_attr.attr;
+
+		/* Let's see if we have a label... */
+		if (iio_read_channel_label(&st->channels[i], buf) < 0)
+			continue;
+
+		a = devm_kzalloc(dev, sizeof(*a), GFP_KERNEL);
+		if (a == NULL)
+			return -ENOMEM;
+
+		sysfs_attr_init(&a->dev_attr.attr);
+		a->dev_attr.attr.name = devm_kasprintf(dev, GFP_KERNEL,
+						       "%s%d_label",
+						       prefix, n);
+		if (!a->dev_attr.attr.name)
+			return -ENOMEM;
+
+		a->dev_attr.show = iio_hwmon_read_label;
+		a->dev_attr.attr.mode = 0444;
+		a->index = i;
+		st->attrs[attr++] = &a->dev_attr.attr;
 	}
 
+	devm_free_pages(dev, (unsigned long)buf);
+
 	st->attr_group.attrs = st->attrs;
 	st->groups[0] = &st->attr_group;
 
-- 
2.35.1.1320.gc452695387.dirty

Re: [PATCH v2 2/2] hwmon: iio: Add labels from IIO channels

[Guenter Roeck]

On 6/24/24 10:46, Sean Anderson wrote:
> Add labels from IIO channels to our channels. This allows userspace to
> display more meaningful names instead of "in0" or "temp5".
> 
> Although lm-sensors gracefully handles errors when reading channel
> labels, the ABI says the label attribute
> 
>> Should only be created if the driver has hints about what this voltage
>> channel is being used for, and user-space doesn't.
> 
> Therefore, we test to see if the channel has a label before
> creating the attribute.
> 

FWIW, complaining about an ABI really does not belong into a commit
message. Maybe you and lm-sensors don't care about error returns when
reading a label, but there are other userspace applications which may
expect drivers to follow the ABI. Last time I checked, the basic rule
was still "Don't break userspace", and that doesn't mean "it's ok to
violate / break an ABI as long as no one notices".

> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
> ---
> 
> Changes in v2:
> - Check if the label exists before creating the attribute
> 
>   drivers/hwmon/iio_hwmon.c | 45 ++++++++++++++++++++++++++++++++++-----
>   1 file changed, 40 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c
> index 4c8a80847891..5722cb9d81f9 100644
> --- a/drivers/hwmon/iio_hwmon.c
> +++ b/drivers/hwmon/iio_hwmon.c
> @@ -33,6 +33,17 @@ struct iio_hwmon_state {
>   	struct attribute **attrs;
>   };
>   
> +static ssize_t iio_hwmon_read_label(struct device *dev,
> +				  struct device_attribute *attr,
> +				  char *buf)
> +{
> +	struct sensor_device_attribute *sattr = to_sensor_dev_attr(attr);
> +	struct iio_hwmon_state *state = dev_get_drvdata(dev);
> +	struct iio_channel *chan = &state->channels[sattr->index];
> +
> +	return iio_read_channel_label(chan, buf);
> +}
> +

I personally find it a bit kludgy that an in-kernel API would do a
sysfs write like this and expect a page-aligned buffer as parameter,
but since Jonathan is fine with it:

Acked-by: Guenter Roeck <linux@roeck-us.net>

Jonathan, please apply through your tree.

Thanks,
Guenter

Re: [PATCH v2 2/2] hwmon: iio: Add labels from IIO channels

[Jonathan Cameron]

On Mon, 24 Jun 2024 11:47:39 -0700
Guenter Roeck <linux@roeck-us.net> wrote:

> On 6/24/24 10:46, Sean Anderson wrote:
> > Add labels from IIO channels to our channels. This allows userspace to
> > display more meaningful names instead of "in0" or "temp5".
> > 
> > Although lm-sensors gracefully handles errors when reading channel
> > labels, the ABI says the label attribute
> >   
> >> Should only be created if the driver has hints about what this voltage
> >> channel is being used for, and user-space doesn't.  
> > 
> > Therefore, we test to see if the channel has a label before
> > creating the attribute.
> >   
> 
> FWIW, complaining about an ABI really does not belong into a commit
> message. Maybe you and lm-sensors don't care about error returns when
> reading a label, but there are other userspace applications which may
> expect drivers to follow the ABI. Last time I checked, the basic rule
> was still "Don't break userspace", and that doesn't mean "it's ok to
> violate / break an ABI as long as no one notices".
> 
> > Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
> > ---
> > 
> > Changes in v2:
> > - Check if the label exists before creating the attribute
> > 
> >   drivers/hwmon/iio_hwmon.c | 45 ++++++++++++++++++++++++++++++++++-----
> >   1 file changed, 40 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c
> > index 4c8a80847891..5722cb9d81f9 100644
> > --- a/drivers/hwmon/iio_hwmon.c
> > +++ b/drivers/hwmon/iio_hwmon.c
> > @@ -33,6 +33,17 @@ struct iio_hwmon_state {
> >   	struct attribute **attrs;
> >   };
> >   
> > +static ssize_t iio_hwmon_read_label(struct device *dev,
> > +				  struct device_attribute *attr,
> > +				  char *buf)
> > +{
> > +	struct sensor_device_attribute *sattr = to_sensor_dev_attr(attr);
> > +	struct iio_hwmon_state *state = dev_get_drvdata(dev);
> > +	struct iio_channel *chan = &state->channels[sattr->index];
> > +
> > +	return iio_read_channel_label(chan, buf);
> > +}
> > +  
> 
> I personally find it a bit kludgy that an in-kernel API would do a
> sysfs write like this and expect a page-aligned buffer as parameter,
> but since Jonathan is fine with it:

That's a good point that I'd not picked up on and it probably makes sense
to address that before it bites us on some other subsystem.

It was more reasonable when the only path was to a light wrapper that went
directly around the sysfs callback. Now we are wrapping these up for more
general use we should avoid that restriction.

Two approaches to that occur to me.
1) Fix up read_label() everywhere to not use sysfs_emit and take a size
   of the buffer to print into. There are only 11 implementations so
   far so this should be straight forward.

2) Add a bounce buffer so we emit into a suitable size for sysfs_emit()
  then reprint from there into a buffer provided via this interface with
  the appropriate size provided.  This one is clunky and given the relatively
  few call sits I think fixing it via option 1 is the better route forwards.
 
Jonathan


> 
> Acked-by: Guenter Roeck <linux@roeck-us.net>
> 
> Jonathan, please apply through your tree.
> 
> Thanks,
> Guenter
> 

Re: [PATCH v2 2/2] hwmon: iio: Add labels from IIO channels

[Sean Anderson]

On 6/24/24 15:24, Jonathan Cameron wrote:
> On Mon, 24 Jun 2024 11:47:39 -0700
> Guenter Roeck <linux@roeck-us.net> wrote:
> 
>> On 6/24/24 10:46, Sean Anderson wrote:
>> > Add labels from IIO channels to our channels. This allows userspace to
>> > display more meaningful names instead of "in0" or "temp5".
>> > 
>> > Although lm-sensors gracefully handles errors when reading channel
>> > labels, the ABI says the label attribute
>> >   
>> >> Should only be created if the driver has hints about what this voltage
>> >> channel is being used for, and user-space doesn't.  
>> > 
>> > Therefore, we test to see if the channel has a label before
>> > creating the attribute.
>> >   
>> 
>> FWIW, complaining about an ABI really does not belong into a commit
>> message. Maybe you and lm-sensors don't care about error returns when
>> reading a label, but there are other userspace applications which may
>> expect drivers to follow the ABI. Last time I checked, the basic rule
>> was still "Don't break userspace", and that doesn't mean "it's ok to
>> violate / break an ABI as long as no one notices".
>> 
>> > Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
>> > ---
>> > 
>> > Changes in v2:
>> > - Check if the label exists before creating the attribute
>> > 
>> >   drivers/hwmon/iio_hwmon.c | 45 ++++++++++++++++++++++++++++++++++-----
>> >   1 file changed, 40 insertions(+), 5 deletions(-)
>> > 
>> > diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c
>> > index 4c8a80847891..5722cb9d81f9 100644
>> > --- a/drivers/hwmon/iio_hwmon.c
>> > +++ b/drivers/hwmon/iio_hwmon.c
>> > @@ -33,6 +33,17 @@ struct iio_hwmon_state {
>> >   	struct attribute **attrs;
>> >   };
>> >   
>> > +static ssize_t iio_hwmon_read_label(struct device *dev,
>> > +				  struct device_attribute *attr,
>> > +				  char *buf)
>> > +{
>> > +	struct sensor_device_attribute *sattr = to_sensor_dev_attr(attr);
>> > +	struct iio_hwmon_state *state = dev_get_drvdata(dev);
>> > +	struct iio_channel *chan = &state->channels[sattr->index];
>> > +
>> > +	return iio_read_channel_label(chan, buf);
>> > +}
>> > +  
>> 
>> I personally find it a bit kludgy that an in-kernel API would do a
>> sysfs write like this and expect a page-aligned buffer as parameter,
>> but since Jonathan is fine with it:
> 
> That's a good point that I'd not picked up on and it probably makes sense
> to address that before it bites us on some other subsystem.
> 
> It was more reasonable when the only path was to a light wrapper that went
> directly around the sysfs callback. Now we are wrapping these up for more
> general use we should avoid that restriction.
> 
> Two approaches to that occur to me.
> 1) Fix up read_label() everywhere to not use sysfs_emit and take a size
>    of the buffer to print into. There are only 11 implementations so
>    far so this should be straight forward.

This API is the same as the existing iio_read_channel_ext_info. It is
used for the same purpose: forwarding sysfs reads/writes from one
device to another (see e.g. iio-mux and iio-rescale). ext_info is used
by around 85 drivers, so it is not so trivial to change the API. While I
agree that the current API is unusual, it's not too bad given that we
get the same guarantees from device_attribute.show.

--Sean

> 2) Add a bounce buffer so we emit into a suitable size for sysfs_emit()
>   then reprint from there into a buffer provided via this interface with
>   the appropriate size provided.  This one is clunky and given the relatively
>   few call sits I think fixing it via option 1 is the better route forwards.

Re: [PATCH v2 2/2] hwmon: iio: Add labels from IIO channels

[Jonathan Cameron]

On Thu, 27 Jun 2024 14:37:01 -0400
Sean Anderson <sean.anderson@linux.dev> wrote:

> On 6/24/24 15:24, Jonathan Cameron wrote:
> > On Mon, 24 Jun 2024 11:47:39 -0700
> > Guenter Roeck <linux@roeck-us.net> wrote:
> >   
> >> On 6/24/24 10:46, Sean Anderson wrote:  
> >> > Add labels from IIO channels to our channels. This allows userspace to
> >> > display more meaningful names instead of "in0" or "temp5".
> >> > 
> >> > Although lm-sensors gracefully handles errors when reading channel
> >> > labels, the ABI says the label attribute
> >> >     
> >> >> Should only be created if the driver has hints about what this voltage
> >> >> channel is being used for, and user-space doesn't.    
> >> > 
> >> > Therefore, we test to see if the channel has a label before
> >> > creating the attribute.
> >> >     
> >> 
> >> FWIW, complaining about an ABI really does not belong into a commit
> >> message. Maybe you and lm-sensors don't care about error returns when
> >> reading a label, but there are other userspace applications which may
> >> expect drivers to follow the ABI. Last time I checked, the basic rule
> >> was still "Don't break userspace", and that doesn't mean "it's ok to
> >> violate / break an ABI as long as no one notices".
> >>   
> >> > Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
> >> > ---
> >> > 
> >> > Changes in v2:
> >> > - Check if the label exists before creating the attribute
> >> > 
> >> >   drivers/hwmon/iio_hwmon.c | 45 ++++++++++++++++++++++++++++++++++-----
> >> >   1 file changed, 40 insertions(+), 5 deletions(-)
> >> > 
> >> > diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c
> >> > index 4c8a80847891..5722cb9d81f9 100644
> >> > --- a/drivers/hwmon/iio_hwmon.c
> >> > +++ b/drivers/hwmon/iio_hwmon.c
> >> > @@ -33,6 +33,17 @@ struct iio_hwmon_state {
> >> >   	struct attribute **attrs;
> >> >   };
> >> >   
> >> > +static ssize_t iio_hwmon_read_label(struct device *dev,
> >> > +				  struct device_attribute *attr,
> >> > +				  char *buf)
> >> > +{
> >> > +	struct sensor_device_attribute *sattr = to_sensor_dev_attr(attr);
> >> > +	struct iio_hwmon_state *state = dev_get_drvdata(dev);
> >> > +	struct iio_channel *chan = &state->channels[sattr->index];
> >> > +
> >> > +	return iio_read_channel_label(chan, buf);
> >> > +}
> >> > +    
> >> 
> >> I personally find it a bit kludgy that an in-kernel API would do a
> >> sysfs write like this and expect a page-aligned buffer as parameter,
> >> but since Jonathan is fine with it:  
> > 
> > That's a good point that I'd not picked up on and it probably makes sense
> > to address that before it bites us on some other subsystem.
> > 
> > It was more reasonable when the only path was to a light wrapper that went
> > directly around the sysfs callback. Now we are wrapping these up for more
> > general use we should avoid that restriction.
> > 
> > Two approaches to that occur to me.
> > 1) Fix up read_label() everywhere to not use sysfs_emit and take a size
> >    of the buffer to print into. There are only 11 implementations so
> >    far so this should be straight forward.  
> 
> This API is the same as the existing iio_read_channel_ext_info. It is
> used for the same purpose: forwarding sysfs reads/writes from one
> device to another (see e.g. iio-mux and iio-rescale). ext_info is used
> by around 85 drivers, so it is not so trivial to change the API. While I
> agree that the current API is unusual, it's not too bad given that we
> get the same guarantees from device_attribute.show.

Fair enough.  Maybe we can clean this up at somepoint but let's not do 
it today. Applied to the togreg branch of iio.git and pushed out as testing for
0-day to poke at it and maybe find something we missed.

Jonathan

> 
> --Sean
> 
> > 2) Add a bounce buffer so we emit into a suitable size for sysfs_emit()
> >   then reprint from there into a buffer provided via this interface with
> >   the appropriate size provided.  This one is clunky and given the relatively
> >   few call sits I think fixing it via option 1 is the better route forwards.  
> 
> 

Re: [PATCH v2 2/2] hwmon: iio: Add labels from IIO channels

[Sean Anderson]

On 6/24/24 14:47, Guenter Roeck wrote:
> On 6/24/24 10:46, Sean Anderson wrote:
>> Add labels from IIO channels to our channels. This allows userspace to
>> display more meaningful names instead of "in0" or "temp5".
>>
>> Although lm-sensors gracefully handles errors when reading channel
>> labels, the ABI says the label attribute
>>
>>> Should only be created if the driver has hints about what this voltage
>>> channel is being used for, and user-space doesn't.
>>
>> Therefore, we test to see if the channel has a label before
>> creating the attribute.
>>
> 
> FWIW, complaining about an ABI really does not belong into a commit
> message. Maybe you and lm-sensors don't care about error returns when
> reading a label, but there are other userspace applications which may
> expect drivers to follow the ABI. Last time I checked, the basic rule
> was still "Don't break userspace", and that doesn't mean "it's ok to
> violate / break an ABI as long as no one notices".

This isn't complaining about the ABI, just documenting the reason it was
done this way...

>> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
>> ---
>>
>> Changes in v2:
>> - Check if the label exists before creating the attribute
>>
>>   drivers/hwmon/iio_hwmon.c | 45 ++++++++++++++++++++++++++++++++++-----
>>   1 file changed, 40 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c
>> index 4c8a80847891..5722cb9d81f9 100644
>> --- a/drivers/hwmon/iio_hwmon.c
>> +++ b/drivers/hwmon/iio_hwmon.c
>> @@ -33,6 +33,17 @@ struct iio_hwmon_state {
>>       struct attribute **attrs;
>>   };
>>   +static ssize_t iio_hwmon_read_label(struct device *dev,
>> +                  struct device_attribute *attr,
>> +                  char *buf)
>> +{
>> +    struct sensor_device_attribute *sattr = to_sensor_dev_attr(attr);
>> +    struct iio_hwmon_state *state = dev_get_drvdata(dev);
>> +    struct iio_channel *chan = &state->channels[sattr->index];
>> +
>> +    return iio_read_channel_label(chan, buf);
>> +}
>> +
> 
> I personally find it a bit kludgy that an in-kernel API would do a
> sysfs write like this and expect a page-aligned buffer as parameter,
> but since Jonathan is fine with it:

This is also how the in-kernel iio_read_channel_ext_info API works.
I agree that it is a strange API, but I do not want to rewrite all
the implementing drivers.

--Sean

> Acked-by: Guenter Roeck <linux@roeck-us.net>
> 
> Jonathan, please apply through your tree.
> 
> Thanks,
> Guenter
> 

Re: [PATCH v2 2/2] hwmon: iio: Add labels from IIO channels

[Guenter Roeck]

On 6/24/24 12:34, Sean Anderson wrote:
> On 6/24/24 14:47, Guenter Roeck wrote:
>> On 6/24/24 10:46, Sean Anderson wrote:
>>> Add labels from IIO channels to our channels. This allows userspace to
>>> display more meaningful names instead of "in0" or "temp5".
>>>
>>> Although lm-sensors gracefully handles errors when reading channel
>>> labels, the ABI says the label attribute
>>>
>>>> Should only be created if the driver has hints about what this voltage
>>>> channel is being used for, and user-space doesn't.
>>>
>>> Therefore, we test to see if the channel has a label before
>>> creating the attribute.
>>>
>>
>> FWIW, complaining about an ABI really does not belong into a commit
>> message. Maybe you and lm-sensors don't care about error returns when
>> reading a label, but there are other userspace applications which may
>> expect drivers to follow the ABI. Last time I checked, the basic rule
>> was still "Don't break userspace", and that doesn't mean "it's ok to
>> violate / break an ABI as long as no one notices".
> 
> This isn't complaining about the ABI, just documenting the reason it was
> done this way...
> 

That a patch is implemented to follow its ABI is not worth mentioning
in the commit message. You _do_ mention it, and added "Although lm-sensors
gracefully ... ". So, from my perspective it is complaining about the ABI,
unless you think that pretty much all patches should include "this is done
to comply with the ABI, even though <some userspace application> is fine
with violating it".

Never mind though, I gave it my Acked-by:, and consider the issue closed.

Guenter