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.dirtyOn 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);
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
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
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
© 2016 - 2025 Red Hat, Inc.