Add support for minimum/maximum attributes. Like the _input attribute,
we just need to call into the IIO API.
Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
---
drivers/hwmon/iio_hwmon.c | 94 ++++++++++++++++++++++++++++++++++++++-
1 file changed, 93 insertions(+), 1 deletion(-)
diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c
index 7dc156d2aea4..3db4d4b30022 100644
--- a/drivers/hwmon/iio_hwmon.c
+++ b/drivers/hwmon/iio_hwmon.c
@@ -95,6 +95,54 @@ static ssize_t iio_hwmon_read_val(struct device *dev,
return sprintf(buf, "%d\n", result);
}
+static ssize_t iio_hwmon_read_event(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
+ struct iio_hwmon_state *state = dev_get_drvdata(dev);
+ struct iio_channel *chan = &state->channels[sattr->index];
+ int ret, result, scale;
+
+ scale = iio_hwmon_scale(chan);
+ if (scale < 0)
+ return scale;
+
+ ret = iio_read_event_processed_scale(chan, IIO_EV_TYPE_THRESH,
+ sattr->nr, IIO_EV_INFO_VALUE,
+ &result, scale);
+ if (ret < 0)
+ return ret;
+
+ return sprintf(buf, "%d\n", result);
+}
+
+static ssize_t iio_hwmon_write_event(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
+ struct iio_hwmon_state *state = dev_get_drvdata(dev);
+ struct iio_channel *chan = &state->channels[sattr->index];
+ int ret, scale, val;
+
+ ret = kstrtoint(buf, 0, &val);
+ if (ret)
+ return ret;
+
+ scale = iio_hwmon_scale(chan);
+ if (scale < 0)
+ return scale;
+
+ ret = iio_write_event_processed_scale(chan, IIO_EV_TYPE_THRESH,
+ sattr->nr, IIO_EV_INFO_VALUE,
+ val, scale);
+ if (ret < 0)
+ return ret;
+
+ return count;
+}
+
static int add_device_attr(struct device *dev, struct iio_hwmon_state *st,
ssize_t (*show)(struct device *dev,
struct device_attribute *attr,
@@ -123,6 +171,40 @@ static int add_device_attr(struct device *dev, struct iio_hwmon_state *st,
return 0;
}
+static int add_event_attr(struct device *dev, struct iio_hwmon_state *st,
+ int i, enum iio_event_direction dir,
+ const char *fmt, ...)
+{
+ struct sensor_device_attribute_2 *a;
+ umode_t mode;
+ va_list ap;
+
+ mode = iio_event_mode(&st->channels[i], IIO_EV_TYPE_THRESH, dir,
+ IIO_EV_INFO_VALUE);
+ if (!mode)
+ return 0;
+
+ 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 = iio_hwmon_read_event;
+ a->dev_attr.store = iio_hwmon_write_event;
+ a->dev_attr.attr.mode = mode;
+ a->index = i;
+ a->nr = dir;
+
+ 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;
@@ -156,7 +238,7 @@ static int iio_hwmon_probe(struct platform_device *pdev)
st->num_channels++;
st->attrs = devm_kcalloc(dev,
- 2 * st->num_channels + 1, sizeof(*st->attrs),
+ 4 * st->num_channels + 1, sizeof(*st->attrs),
GFP_KERNEL);
if (st->attrs == NULL)
return -ENOMEM;
@@ -206,6 +288,16 @@ static int iio_hwmon_probe(struct platform_device *pdev)
if (ret)
return ret;
}
+
+ ret = add_event_attr(dev, st, i, IIO_EV_DIR_FALLING,
+ "%s%d_min", prefix, n);
+ if (ret)
+ return ret;
+
+ ret = add_event_attr(dev, st, i, IIO_EV_DIR_RISING,
+ "%s%d_max", 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:22 -0400
Sean Anderson <sean.anderson@linux.dev> wrote:
> Add support for minimum/maximum attributes. Like the _input attribute,
> we just need to call into the IIO API.
>
> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
Similar comments to previous. I'm not keen on the blend of allocation of
attributes and registration. If we can break that link I think it will give
cleaner code.
> static int add_device_attr(struct device *dev, struct iio_hwmon_state *st,
> ssize_t (*show)(struct device *dev,
> struct device_attribute *attr,
> @@ -123,6 +171,40 @@ static int add_device_attr(struct device *dev, struct iio_hwmon_state *st,
> return 0;
> }
>
> +static int add_event_attr(struct device *dev, struct iio_hwmon_state *st,
> + int i, enum iio_event_direction dir,
> + const char *fmt, ...)
> +{
> + struct sensor_device_attribute_2 *a;
> + umode_t mode;
> + va_list ap;
> +
> + mode = iio_event_mode(&st->channels[i], IIO_EV_TYPE_THRESH, dir,
> + IIO_EV_INFO_VALUE);
> + if (!mode)
> + return 0;
> +
> + 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 = iio_hwmon_read_event;
> + a->dev_attr.store = iio_hwmon_write_event;
> + a->dev_attr.attr.mode = mode;
> + a->index = i;
> + a->nr = dir;
> +
> + st->attrs[st->num_attrs++] = &a->dev_attr.attr;
similar comment to the previous, though here I think we'd
need to pass in the channel to an iio_hwmon_alloc_event_attr() as ideally we'd
not be messing with st at all in here. So maybe it doesn't work out.
> + return 0;
> +}
On 7/27/25 12:35, Jonathan Cameron wrote:
> On Mon, 14 Jul 2025 21:20:22 -0400
> Sean Anderson <sean.anderson@linux.dev> wrote:
>
>> Add support for minimum/maximum attributes. Like the _input attribute,
>> we just need to call into the IIO API.
>>
>> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
>
> Similar comments to previous. I'm not keen on the blend of allocation of
> attributes and registration. If we can break that link I think it will give
> cleaner code.
>
>> static int add_device_attr(struct device *dev, struct iio_hwmon_state *st,
>> ssize_t (*show)(struct device *dev,
>> struct device_attribute *attr,
>> @@ -123,6 +171,40 @@ static int add_device_attr(struct device *dev, struct iio_hwmon_state *st,
>> return 0;
>> }
>>
>> +static int add_event_attr(struct device *dev, struct iio_hwmon_state *st,
>> + int i, enum iio_event_direction dir,
>> + const char *fmt, ...)
>> +{
>> + struct sensor_device_attribute_2 *a;
>> + umode_t mode;
>> + va_list ap;
>> +
>> + mode = iio_event_mode(&st->channels[i], IIO_EV_TYPE_THRESH, dir,
>> + IIO_EV_INFO_VALUE);
>> + if (!mode)
>> + return 0;
>> +
>> + 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 = iio_hwmon_read_event;
>> + a->dev_attr.store = iio_hwmon_write_event;
>> + a->dev_attr.attr.mode = mode;
>> + a->index = i;
>> + a->nr = dir;
>> +
>> + st->attrs[st->num_attrs++] = &a->dev_attr.attr;
> similar comment to the previous, though here I think we'd
> need to pass in the channel to an iio_hwmon_alloc_event_attr() as ideally we'd
> not be messing with st at all in here. So maybe it doesn't work out.
Well, I used to have
+ if (iio_read_channel_label(&st->channels[i], buf) >= 0) {
+ st->attrs[attr] = create_attr(dev, iio_hwmon_read_label,
+ NULL, 0444, i, 0, 0, 0,
+ "%s%d_label", prefix, n);
+ if (!st->attrs[attr++])
+ return -ENOMEM;
+ }
but even with a shorter function name, all the parameters are starting
to get bunched up on the right side. And if we make it longer as you
propose it starts looking like
+ if (iio_read_channel_label(&st->channels[i], buf) >= 0) {
+ st->attrs[attr] =
+ iio_hwmon_create_device_attr(dev,
+ iio_hwmon_read_label,
+ NULL, 0444, i, 0, 0,
+ 0, "%s%d_label",
+ prefix, n);
+ if (!st->attrs[attr++])
+ return -ENOMEM;
+ }
which is IMO really terrible-looking.
Maybe we should just stick everything in an xarray and linearize it at
the end of probe...
--Sean
On Mon, 28 Jul 2025 18:32:43 -0400
Sean Anderson <sean.anderson@linux.dev> wrote:
> On 7/27/25 12:35, Jonathan Cameron wrote:
> > On Mon, 14 Jul 2025 21:20:22 -0400
> > Sean Anderson <sean.anderson@linux.dev> wrote:
> >
> >> Add support for minimum/maximum attributes. Like the _input attribute,
> >> we just need to call into the IIO API.
> >>
> >> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
> >
> > Similar comments to previous. I'm not keen on the blend of allocation of
> > attributes and registration. If we can break that link I think it will give
> > cleaner code.
> >
> >> static int add_device_attr(struct device *dev, struct iio_hwmon_state *st,
> >> ssize_t (*show)(struct device *dev,
> >> struct device_attribute *attr,
> >> @@ -123,6 +171,40 @@ static int add_device_attr(struct device *dev, struct iio_hwmon_state *st,
> >> return 0;
> >> }
> >>
> >> +static int add_event_attr(struct device *dev, struct iio_hwmon_state *st,
> >> + int i, enum iio_event_direction dir,
> >> + const char *fmt, ...)
> >> +{
> >> + struct sensor_device_attribute_2 *a;
> >> + umode_t mode;
> >> + va_list ap;
> >> +
> >> + mode = iio_event_mode(&st->channels[i], IIO_EV_TYPE_THRESH, dir,
> >> + IIO_EV_INFO_VALUE);
> >> + if (!mode)
> >> + return 0;
> >> +
> >> + 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 = iio_hwmon_read_event;
> >> + a->dev_attr.store = iio_hwmon_write_event;
> >> + a->dev_attr.attr.mode = mode;
> >> + a->index = i;
> >> + a->nr = dir;
> >> +
> >> + st->attrs[st->num_attrs++] = &a->dev_attr.attr;
> > similar comment to the previous, though here I think we'd
> > need to pass in the channel to an iio_hwmon_alloc_event_attr() as ideally we'd
> > not be messing with st at all in here. So maybe it doesn't work out.
>
> Well, I used to have
>
> + if (iio_read_channel_label(&st->channels[i], buf) >= 0) {
> + st->attrs[attr] = create_attr(dev, iio_hwmon_read_label,
> + NULL, 0444, i, 0, 0, 0,
> + "%s%d_label", prefix, n);
> + if (!st->attrs[attr++])
pushing attr off the end is not a good idea even if we know we don't use it
any more.
> + return -ENOMEM;
> + }
>
> but even with a shorter function name, all the parameters are starting
> to get bunched up on the right side. And if we make it longer as you
> propose it starts looking like
Using a local variable
struct attribute *att;
att = create_attr(dev, iio_hwmon_read_lanel,
...
if (!att)
return -ENOMEM;
st->attrs[attr++] = att;
helps but still ugly.
>
>
> + if (iio_read_channel_label(&st->channels[i], buf) >= 0) {
> + st->attrs[attr] =
> + iio_hwmon_create_device_attr(dev,
> + iio_hwmon_read_label,
> + NULL, 0444, i, 0, 0,
> + 0, "%s%d_label",
> + prefix, n);
> + if (!st->attrs[attr++])
> + return -ENOMEM;
> + }
>
> which is IMO really terrible-looking.
Fair enough. let's leave it as is.
>
> Maybe we should just stick everything in an xarray and linearize it at
> the end of probe...
If it looks nicer - feel free!
J
>
> --Sean
On Mon, Jul 14, 2025 at 09:20:22PM -0400, Sean Anderson wrote:
> Add support for minimum/maximum attributes. Like the _input attribute,
> we just need to call into the IIO API.
...
> +static ssize_t iio_hwmon_read_event(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
> + struct iio_hwmon_state *state = dev_get_drvdata(dev);
> + struct iio_channel *chan = &state->channels[sattr->index];
> + int ret, result, scale;
> +
> + scale = iio_hwmon_scale(chan);
> + if (scale < 0)
This part is definitely missed in the respective description.
> + return scale;
> +
> + ret = iio_read_event_processed_scale(chan, IIO_EV_TYPE_THRESH,
> + sattr->nr, IIO_EV_INFO_VALUE,
> + &result, scale);
> + if (ret < 0)
Why ' < 0' here?
> + return ret;
> +
> + return sprintf(buf, "%d\n", result);
Mustn't be sysfs_emit() ?
> +}
...
> + ret = iio_write_event_processed_scale(chan, IIO_EV_TYPE_THRESH,
> + sattr->nr, IIO_EV_INFO_VALUE,
> + val, scale);
> + if (ret < 0)
< 0 ?
> + return ret;
...
> +static int add_event_attr(struct device *dev, struct iio_hwmon_state *st,
> + int i, enum iio_event_direction dir,
> + const char *fmt, ...)
Same comments as per previous patch adding another attribute API.
...
> + va_start(ap, fmt);
> + a->dev_attr.attr.name = devm_kvasprintf(dev, GFP_KERNEL, fmt, ap);
> + va_end(ap);
Can't %pV be used?
--
With Best Regards,
Andy Shevchenko
On 7/15/25 04:41, Andy Shevchenko wrote:
> On Mon, Jul 14, 2025 at 09:20:22PM -0400, Sean Anderson wrote:
>> Add support for minimum/maximum attributes. Like the _input attribute,
>> we just need to call into the IIO API.
>
> ...
>
>> +static ssize_t iio_hwmon_read_event(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>> + struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
>> + struct iio_hwmon_state *state = dev_get_drvdata(dev);
>> + struct iio_channel *chan = &state->channels[sattr->index];
>> + int ret, result, scale;
>> +
>> + scale = iio_hwmon_scale(chan);
>
>> + if (scale < 0)
>
> This part is definitely missed in the respective description.
OK
>> + return scale;
>> +
>> + ret = iio_read_event_processed_scale(chan, IIO_EV_TYPE_THRESH,
>> + sattr->nr, IIO_EV_INFO_VALUE,
>> + &result, scale);
>> + if (ret < 0)
>
> Why ' < 0' here?
This originally returned IIO_VAL_INT on success.
>> + return ret;
>> +
>> + return sprintf(buf, "%d\n", result);
>
> Mustn't be sysfs_emit() ?
It doesn't matter in this case (as %d can never emit more
than 20ish characters), but that works too.
>> +}
>
> ...
>
>> + ret = iio_write_event_processed_scale(chan, IIO_EV_TYPE_THRESH,
>> + sattr->nr, IIO_EV_INFO_VALUE,
>> + val, scale);
>> + if (ret < 0)
>
> < 0 ?
>
>> + return ret;
>
> ...
>
>> +static int add_event_attr(struct device *dev, struct iio_hwmon_state *st,
>> + int i, enum iio_event_direction dir,
>> + const char *fmt, ...)
>
> Same comments as per previous patch adding another attribute API.
>
> ...
>
>> + va_start(ap, fmt);
>> + a->dev_attr.attr.name = devm_kvasprintf(dev, GFP_KERNEL, fmt, ap);
>> + va_end(ap);
>
> Can't %pV be used?
%pV is for when we have additional info to add. e.g. if we were doing
devm_kasprintf(dev, GFP_KERNEL, "my_extra_info_%d_%pV", i, &vaf);
but we aren't so there's no point adding a level of indirection.
--Sean
On Tue, Jul 15, 2025 at 12:05:15PM -0400, Sean Anderson wrote: > On 7/15/25 04:41, Andy Shevchenko wrote: > > On Mon, Jul 14, 2025 at 09:20:22PM -0400, Sean Anderson wrote: ... > >> + return sprintf(buf, "%d\n", result); > > > > Mustn't be sysfs_emit() ? > > It doesn't matter in this case (as %d can never emit more > than 20ish characters), but that works too. Have you read the documentation? It uses word 'must'... -- With Best Regards, Andy Shevchenko
On 7/16/25 06:01, Andy Shevchenko wrote: > On Tue, Jul 15, 2025 at 12:05:15PM -0400, Sean Anderson wrote: >> On 7/15/25 04:41, Andy Shevchenko wrote: >> > On Mon, Jul 14, 2025 at 09:20:22PM -0400, Sean Anderson wrote: > > ... > >> >> + return sprintf(buf, "%d\n", result); >> > >> > Mustn't be sysfs_emit() ? >> >> It doesn't matter in this case (as %d can never emit more >> than 20ish characters), but that works too. > > Have you read the documentation? It uses word 'must'... > Documentation for what? sysfs_emit just says it's aware of the PAGE_SIZE buffer. Nothing about that it "must" be used. --Sean
© 2016 - 2025 Red Hat, Inc.