hwmon: iio: Add alarm support

[PATCH 5/7] hwmon: iio: Add helper function for creating attributes

Posted by Sean Anderson 6 months, 4 weeks ago

Add a helper function to create attributes and initialize their fields.
This reduces repetition when creating several attributes per channel.

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

 drivers/hwmon/iio_hwmon.c | 78 +++++++++++++++++++++------------------
 1 file changed, 42 insertions(+), 36 deletions(-)

diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c
index bba8919377eb..7dc156d2aea4 100644
--- a/drivers/hwmon/iio_hwmon.c
+++ b/drivers/hwmon/iio_hwmon.c
@@ -24,13 +24,15 @@
  * @attr_group:		the group of attributes
  * @groups:		null terminated array of attribute groups
  * @attrs:		null terminated array of attribute pointers.
+ * @num_attrs:          length of @attrs
  */
 struct iio_hwmon_state {
 	struct iio_channel *channels;
-	int num_channels;
 	struct attribute_group attr_group;
 	const struct attribute_group *groups[2];
 	struct attribute **attrs;
+	size_t num_attrs;
+	int num_channels;
 };
 
 static ssize_t iio_hwmon_read_label(struct device *dev,
@@ -93,12 +95,39 @@ static ssize_t iio_hwmon_read_val(struct device *dev,
 	return sprintf(buf, "%d\n", result);
 }
 
+static int add_device_attr(struct device *dev, struct iio_hwmon_state *st,
+			   ssize_t (*show)(struct device *dev,
+					   struct device_attribute *attr,
+					   char *buf),
+			   int i, const char *fmt, ...)
+{
+	struct sensor_device_attribute *a;
+	va_list ap;
+
+	a = devm_kzalloc(dev, sizeof(*a), GFP_KERNEL);
+	if (!a)
+		return -ENOMEM;
+
+	sysfs_attr_init(&a->dev_attr.attr);
+	va_start(ap, fmt);
+	a->dev_attr.attr.name = devm_kvasprintf(dev, GFP_KERNEL, fmt, ap);
+	va_end(ap);
+	if (!a->dev_attr.attr.name)
+		return -ENOMEM;
+
+	a->dev_attr.show = show;
+	a->dev_attr.attr.mode = 0444;
+	a->index = i;
+
+	st->attrs[st->num_attrs++] = &a->dev_attr.attr;
+	return 0;
+}
+
 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, attr = 0;
+	int ret, i;
 	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;
@@ -136,11 +165,6 @@ static int iio_hwmon_probe(struct platform_device *pdev)
 		const char *prefix;
 		int n;
 
-		a = devm_kzalloc(dev, sizeof(*a), GFP_KERNEL);
-		if (a == NULL)
-			return -ENOMEM;
-
-		sysfs_attr_init(&a->dev_attr.attr);
 		ret = iio_get_channel_type(&st->channels[i], &type);
 		if (ret < 0)
 			return ret;
@@ -170,36 +194,18 @@ static int iio_hwmon_probe(struct platform_device *pdev)
 			return -EINVAL;
 		}
 
-		a->dev_attr.attr.name = devm_kasprintf(dev, GFP_KERNEL,
-						       "%s%d_input",
-						       prefix, n);
-		if (a->dev_attr.attr.name == NULL)
-			return -ENOMEM;
-
-		a->dev_attr.show = iio_hwmon_read_val;
-		a->dev_attr.attr.mode = 0444;
-		a->index = i;
-		st->attrs[attr++] = &a->dev_attr.attr;
+		ret = add_device_attr(dev, st, iio_hwmon_read_val, i,
+				      "%s%d_input", prefix, n);
+		if (ret)
+			return ret;
 
 		/* 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;
+		if (iio_read_channel_label(&st->channels[i], buf) >= 0) {
+			ret = add_device_attr(dev, st, iio_hwmon_read_label,
+					      i, "%s%d_label", prefix, n);
+			if (ret)
+				return ret;
+		}
 	}
 
 	devm_free_pages(dev, (unsigned long)buf);
-- 
2.35.1.1320.gc452695387.dirty

Re: [PATCH 5/7] hwmon: iio: Add helper function for creating attributes

Posted by Jonathan Cameron 6 months, 2 weeks ago

On Mon, 14 Jul 2025 21:20:21 -0400
Sean Anderson <sean.anderson@linux.dev> wrote:

> Add a helper function to create attributes and initialize their fields.
> This reduces repetition when creating several attributes per channel.
> 
> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
> ---
> 
>  drivers/hwmon/iio_hwmon.c | 78 +++++++++++++++++++++------------------
>  1 file changed, 42 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c
> index bba8919377eb..7dc156d2aea4 100644
> --- a/drivers/hwmon/iio_hwmon.c
> +++ b/drivers/hwmon/iio_hwmon.c
> @@ -24,13 +24,15 @@
>   * @attr_group:		the group of attributes
>   * @groups:		null terminated array of attribute groups
>   * @attrs:		null terminated array of attribute pointers.
> + * @num_attrs:          length of @attrs
>   */
>  struct iio_hwmon_state {
>  	struct iio_channel *channels;
> -	int num_channels;
>  	struct attribute_group attr_group;
>  	const struct attribute_group *groups[2];
>  	struct attribute **attrs;
> +	size_t num_attrs;

It's a little unfortunate that we need to keep the state around when it
is only relevant in probe.

> +	int num_channels;
>  };
>  
>  static ssize_t iio_hwmon_read_label(struct device *dev,
> @@ -93,12 +95,39 @@ static ssize_t iio_hwmon_read_val(struct device *dev,
>  	return sprintf(buf, "%d\n", result);
>  }
>  
> +static int add_device_attr(struct device *dev, struct iio_hwmon_state *st,

That's a very generic name. I'd prefix it to avoid potential namespace
issues in future.

> +			   ssize_t (*show)(struct device *dev,
> +					   struct device_attribute *attr,
> +					   char *buf),
> +			   int i, const char *fmt, ...)
> +{
> +	struct sensor_device_attribute *a;
> +	va_list ap;
> +
> +	a = devm_kzalloc(dev, sizeof(*a), GFP_KERNEL);
> +	if (!a)
> +		return -ENOMEM;
> +
> +	sysfs_attr_init(&a->dev_attr.attr);
> +	va_start(ap, fmt);
> +	a->dev_attr.attr.name = devm_kvasprintf(dev, GFP_KERNEL, fmt, ap);
> +	va_end(ap);
> +	if (!a->dev_attr.attr.name)
> +		return -ENOMEM;
> +
> +	a->dev_attr.show = show;
> +	a->dev_attr.attr.mode = 0444;
> +	a->index = i;
> +
> +	st->attrs[st->num_attrs++] = &a->dev_attr.attr

I wonder if we should break this up into
	static attribute *iio_hwmon_alloc_attr();

and setting st->attrs in the caller as then we can make num_attrs a local
variable.

> +	return 0;
> +}
> +
>  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, attr = 0;
> +	int ret, i;
>  	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;
> @@ -136,11 +165,6 @@ static int iio_hwmon_probe(struct platform_device *pdev)
>  		const char *prefix;
>  		int n;
>  
> -		a = devm_kzalloc(dev, sizeof(*a), GFP_KERNEL);
> -		if (a == NULL)
> -			return -ENOMEM;
> -
> -		sysfs_attr_init(&a->dev_attr.attr);
>  		ret = iio_get_channel_type(&st->channels[i], &type);
>  		if (ret < 0)
>  			return ret;
> @@ -170,36 +194,18 @@ static int iio_hwmon_probe(struct platform_device *pdev)
>  			return -EINVAL;
>  		}
>  
> -		a->dev_attr.attr.name = devm_kasprintf(dev, GFP_KERNEL,
> -						       "%s%d_input",
> -						       prefix, n);
> -		if (a->dev_attr.attr.name == NULL)
> -			return -ENOMEM;
> -
> -		a->dev_attr.show = iio_hwmon_read_val;
> -		a->dev_attr.attr.mode = 0444;
> -		a->index = i;
> -		st->attrs[attr++] = &a->dev_attr.attr;
> +		ret = add_device_attr(dev, st, iio_hwmon_read_val, i,
> +				      "%s%d_input", prefix, n);
> +		if (ret)
> +			return ret;
>  
>  		/* 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;
> +		if (iio_read_channel_label(&st->channels[i], buf) >= 0) {
> +			ret = add_device_attr(dev, st, iio_hwmon_read_label,
> +					      i, "%s%d_label", prefix, n);
> +			if (ret)
> +				return ret;
> +		}
>  	}
>  
>  	devm_free_pages(dev, (unsigned long)buf);

Re: [PATCH 5/7] hwmon: iio: Add helper function for creating attributes

Posted by Andy Shevchenko 6 months, 4 weeks ago

On Mon, Jul 14, 2025 at 09:20:21PM -0400, Sean Anderson wrote:
> Add a helper function to create attributes and initialize their fields.
> This reduces repetition when creating several attributes per channel.

...

> + * @num_attrs:          length of @attrs

Other lines use TABs.

...

> +static int add_device_attr(struct device *dev, struct iio_hwmon_state *st,

This should hint that this is managed:

add_device_managed_attr()

?

> +			   ssize_t (*show)(struct device *dev,
> +					   struct device_attribute *attr,
> +					   char *buf),
> +			   int i, const char *fmt, ...)

__printf() attribute is missing.

> +{
> +	struct sensor_device_attribute *a;
> +	va_list ap;
> +
> +	a = devm_kzalloc(dev, sizeof(*a), GFP_KERNEL);
> +	if (!a)
> +		return -ENOMEM;
> +
> +	sysfs_attr_init(&a->dev_attr.attr);
> +	va_start(ap, fmt);
> +	a->dev_attr.attr.name = devm_kvasprintf(dev, GFP_KERNEL, fmt, ap);
> +	va_end(ap);
> +	if (!a->dev_attr.attr.name)
> +		return -ENOMEM;
> +
> +	a->dev_attr.show = show;
> +	a->dev_attr.attr.mode = 0444;
> +	a->index = i;
> +
> +	st->attrs[st->num_attrs++] = &a->dev_attr.attr;
> +	return 0;
> +}

...

>  	struct device *dev = &pdev->dev;
>  	struct iio_hwmon_state *st;
> -	struct sensor_device_attribute *a;
> -	int ret, i, attr = 0;
> +	int ret, i;

Also move it a bit to make it more of a reversed xmas tree ordering?

>  	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;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 5/7] hwmon: iio: Add helper function for creating attributes

Posted by Sean Anderson 6 months, 4 weeks ago

On 7/15/25 04:38, Andy Shevchenko wrote:
> On Mon, Jul 14, 2025 at 09:20:21PM -0400, Sean Anderson wrote:
>> Add a helper function to create attributes and initialize their fields.
>> This reduces repetition when creating several attributes per channel.
> 
> ...
> 
>> + * @num_attrs:          length of @attrs
> 
> Other lines use TABs.
> 
> ...

OK

>> +static int add_device_attr(struct device *dev, struct iio_hwmon_state *st,
> 
> This should hint that this is managed:
> 
> add_device_managed_attr()

That just makes it more difficult to format the calling code within 80 columns...

> 
>> +			   ssize_t (*show)(struct device *dev,
>> +					   struct device_attribute *attr,
>> +					   char *buf),
>> +			   int i, const char *fmt, ...)
> 
> __printf() attribute is missing.

It's static, so I thought the compiler could infer it but I guess not.

>> +{
>> +	struct sensor_device_attribute *a;
>> +	va_list ap;
>> +
>> +	a = devm_kzalloc(dev, sizeof(*a), GFP_KERNEL);
>> +	if (!a)
>> +		return -ENOMEM;
>> +
>> +	sysfs_attr_init(&a->dev_attr.attr);
>> +	va_start(ap, fmt);
>> +	a->dev_attr.attr.name = devm_kvasprintf(dev, GFP_KERNEL, fmt, ap);
>> +	va_end(ap);
>> +	if (!a->dev_attr.attr.name)
>> +		return -ENOMEM;
>> +
>> +	a->dev_attr.show = show;
>> +	a->dev_attr.attr.mode = 0444;
>> +	a->index = i;
>> +
>> +	st->attrs[st->num_attrs++] = &a->dev_attr.attr;
>> +	return 0;
>> +}
> 
> ...
> 
>>  	struct device *dev = &pdev->dev;
>>  	struct iio_hwmon_state *st;
>> -	struct sensor_device_attribute *a;
>> -	int ret, i, attr = 0;
>> +	int ret, i;
> 
> Also move it a bit to make it more of a reversed xmas tree ordering?

It's not ordered as-is, and I don't think this subsystem requires it.

>>  	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;
> 

--Sean

Re: [PATCH 5/7] hwmon: iio: Add helper function for creating attributes

Posted by Andy Shevchenko 6 months, 3 weeks ago

On Tue, Jul 15, 2025 at 11:55:08AM -0400, Sean Anderson wrote:
> On 7/15/25 04:38, Andy Shevchenko wrote:
> > On Mon, Jul 14, 2025 at 09:20:21PM -0400, Sean Anderson wrote:

...

> >> +static int add_device_attr(struct device *dev, struct iio_hwmon_state *st,
> > 
> > This should hint that this is managed:
> > 
> > add_device_managed_attr()
> 
> That just makes it more difficult to format the calling code within 80 columns...

Choose your name, but important that it should hint the caller that it's only
for the ->probe() stages.

...

> >> +			   ssize_t (*show)(struct device *dev,
> >> +					   struct device_attribute *attr,
> >> +					   char *buf),
> >> +			   int i, const char *fmt, ...)
> > 
> > __printf() attribute is missing.
> 
> It's static, so I thought the compiler could infer it but I guess not.

TBH, I haven't checked, but I think it might be the good style to add.

-- 
With Best Regards,
Andy Shevchenko