Message-ID: <afc6aa6ad4b633f9d834acf933734985f14c5563.camel@gmail.com>
Date: Tue, 15 Jul 2025 12:28:50 +0100
From: Nuno Sá <noname.nuno@...il.com>
To: Sean Anderson <sean.anderson@...ux.dev>, Jonathan Cameron
 <jic23@...nel.org>,  Jean Delvare <jdelvare@...e.com>, Guenter Roeck
 <linux@...ck-us.net>, linux-iio@...r.kernel.org, 
	linux-hwmon@...r.kernel.org
Cc: Andy Shevchenko <andy@...nel.org>, Nuno Sá
	 <nuno.sa@...log.com>, linux-kernel@...r.kernel.org, David Lechner
	 <dlechner@...libre.com>
Subject: Re: [PATCH 7/7] hwmon: iio: Add alarm support

On Mon, 2025-07-14 at 21:20 -0400, Sean Anderson wrote:
> Add alarm support based on IIO threshold events. The alarm is cleared on
> read, but will be set again if the condition is still present. This is
> detected by disabling and re-enabling the event. The same trick is done
> when creating the attribute to detect already-triggered events.
> 
> The alarms are updated by an event listener. To keep the notifier call
> chain short, we create one listener per iio device, shared across all
> hwmon devices.
> 
> To avoid dynamic creation of alarms, alarms for all possible events are
> allocated at creation. Lookup is done by a linear scan, as I expect
> events to occur rarely. If performance becomes an issue, a binary search
> could be done instead (or some kind of hash lookup).
> 
> Signed-off-by: Sean Anderson <sean.anderson@...ux.dev>
> ---
> 
>  drivers/hwmon/iio_hwmon.c | 322 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 321 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c
> index 3db4d4b30022..c963bc5452ba 100644
> --- a/drivers/hwmon/iio_hwmon.c
> +++ b/drivers/hwmon/iio_hwmon.c
> @@ -8,6 +8,7 @@
>  #include <linux/slab.h>
>  #include <linux/mod_devicetable.h>
>  #include <linux/module.h>
> +#include <linux/notifier.h>
>  #include <linux/err.h>
>  #include <linux/platform_device.h>
>  #include <linux/property.h>
> @@ -15,7 +16,192 @@
>  #include <linux/hwmon.h>
>  #include <linux/hwmon-sysfs.h>
>  #include <linux/iio/consumer.h>
> +#include <linux/iio/events.h>
> +#include <linux/iio/iio.h>
>  #include <linux/iio/types.h>
> +#include <uapi/linux/iio/events.h>
> +
> +/* Protects iio_hwmon_listeners and listeners' refcnt */
> +DEFINE_MUTEX(iio_hwmon_listener_lock);
> +LIST_HEAD(iio_hwmon_listeners);
> +
> +/**
> + * struct iio_hwmon_listener - Listener for IIO events
> + * @block: Notifier for events
> + * @ids: Array of IIO event ids, one per alarm
> + * @alarms: Bitmap of alarms
> + * @num_alarms: Length of @ids and @alarms
> + * @indio_dev: Device we are listening to
> + * @list: List of all listeners
> + * @refcnt: Reference count
> + */
> +struct iio_hwmon_listener {
> +	struct notifier_block block;
> +	u64 *ids;
> +	unsigned long *alarms;
> +	size_t num_alarms;
> +
> +	struct iio_dev *indio_dev;
> +	struct list_head list;
> +	unsigned int refcnt;
> +};
> +
> +/**
> + * iio_hwmon_lookup_alarm() - Find an alarm by id
> + * @listener: Event listener
> + * @id: IIO event id
> + *
> + * Return: index of @id in @listener->ids, or -1 if not found
> + */
> +static ssize_t iio_hwmon_lookup_alarm(struct iio_hwmon_listener *listener,
> +				      u64 id)
> +{
> +	ssize_t i;
> +
> +	for (i = 0; i < listener->num_alarms; i++)
> +		if (listener->ids[i] == id)
> +			return i;
> +
> +	return -1;
> +}
> +
> +static int iio_hwmon_listener_callback(struct notifier_block *block,
> +				       unsigned long action, void *data)
> +{
> +	struct iio_hwmon_listener *listener =
> +		container_of(block, struct iio_hwmon_listener, block);
> +	struct iio_event_data *ev = data;
> +	ssize_t i;
> +
> +	if (action != IIO_NOTIFY_EVENT)
> +		return NOTIFY_DONE;
> +
> +	i = iio_hwmon_lookup_alarm(listener, ev->id);
> +	if (i >= 0)
> +		set_bit(i, listener->alarms);
> +	else
> +		dev_warn_once(&listener->indio_dev->dev,
> +			      "unknown event %016llx\n", ev->id);
> +
> +	return NOTIFY_DONE;
> +}
> +
> +/**
> + * iio_event_id() - Calculate an IIO event id
> + * @channel: IIO channel for this event
> + * @type: Event type (theshold, rate-of-change, etc.)
> + * @dir: Event direction (rising, falling, etc.)
> + *
> + * Return: IIO event id corresponding to this event's IIO id
> + */
> +static u64 iio_event_id(struct iio_chan_spec const *chan,
> +			enum iio_event_type type,
> +			enum iio_event_direction dir)
> +{
> +	if (chan->differential)
> +		return IIO_DIFF_EVENT_CODE(chan->type, chan->channel,
> +					   chan->channel2, type, dir);
> +	if (chan->modified)
> +		return IIO_MOD_EVENT_CODE(chan->type, chan->channel,
> +					  chan->channel2, type, dir);
> +	return IIO_UNMOD_EVENT_CODE(chan->type, chan->channel, type, dir);
> +}
> +
> +/**
> + * iio_hwmon_listener_get() - Get a listener for an IIO device
> + * @indio_dev: IIO device to listen to
> + *
> + * Look up or create a new listener for @indio_dev. The returned listener is
> + * registered with @indio_dev, but events still need to be manually enabled.
> + * You must call iio_hwmon_listener_put() when you are done.
> + *
> + * Return: Listener for @indio_dev, or an error pointer
> + */
> +static struct iio_hwmon_listener *iio_hwmon_listener_get(struct iio_dev
> *indio_dev)
> +{
> +	struct iio_hwmon_listener *listener;
> +	int err = -ENOMEM;
> +	size_t i, j;
> +
> +	guard(mutex)(&iio_hwmon_listener_lock);
> +	list_for_each_entry(listener, &iio_hwmon_listeners, list) {
> +		if (listener->indio_dev == indio_dev) {
> +			if (likely(listener->refcnt != UINT_MAX))
> +				listener->refcnt++;

I dunno for the above to ever happen :). And as Andy stated, let's just use
proper refcount APIs.
> +			return listener;
> +		}
> +	}
> +
> +	listener = kzalloc(sizeof(*listener), GFP_KERNEL);
> +	if (!listener)
> +		goto err_unlock;
> +
> +	listener->refcnt = 1;
> +	listener->indio_dev = indio_dev;
> +	listener->block.notifier_call = iio_hwmon_listener_callback;
> +	for (i = 0; i < indio_dev->num_channels; i++)
> +		listener->num_alarms += indio_dev-
> >channels[i].num_event_specs;
> +
> +	listener->ids = kcalloc(listener->num_alarms, sizeof(*listener->ids),
> +				GFP_KERNEL);
> +	listener->alarms = bitmap_zalloc(listener->num_alarms, GFP_KERNEL);
> +	if (!listener->ids || !listener->alarms)
> +		goto err_listener;
> +
> +	i = 0;
> +	for (j = 0; j < indio_dev->num_channels; j++) {
> +		struct iio_chan_spec const *chan = &indio_dev->channels[j];
> +		size_t k;
> +
> +		for (k = 0; k < chan->num_event_specs; k++)
> +			listener->ids[i++] =
> +				iio_event_id(chan, chan->event_spec[k].type,
> +					     chan->event_spec[k].dir);
> +	}
> +
> +	err = iio_event_register(indio_dev, &listener->block);
> +	if (err)
> +		goto err_alarms;
> +
> +	list_add(&listener->list, &iio_hwmon_listeners);
> +	mutex_unlock(&iio_hwmon_listener_lock);
> +	return listener;
> +
> +err_alarms:
> +	kfree(listener->alarms);
> +	kfree(listener->ids);
> +err_listener:
> +	kfree(listener);
> +err_unlock:
> +	mutex_unlock(&iio_hwmon_listener_lock);
> +	return ERR_PTR(err);
> +}
> +
> +/**
> + * iio_hwmon_listener_put() - Release a listener
> + * @data: &struct iio_hwmon_listener to release
> + *
> + * For convenience, @data is void.
> + */
> +static void iio_hwmon_listener_put(void *data)
> +{
> +	struct iio_hwmon_listener *listener = data;
> +
> +	scoped_guard(mutex, &iio_hwmon_listener_lock) {
> +		if (unlikely(listener->refcnt == UINT_MAX))
> +			return;
> +
> +		if (--listener->refcnt)
> +			return;
> +
> +		list_del(&listener->list);
> +		iio_event_unregister(listener->indio_dev, &listener->block);
> +	}
> +
> +	kfree(listener->alarms);
> +	kfree(listener->ids);
> +	kfree(listener);
> +}
>  
>  /**
>   * struct iio_hwmon_state - device instance state
> @@ -143,6 +329,68 @@ static ssize_t iio_hwmon_write_event(struct device *dev,
>  	return count;
>  }
>  
> +/**
> + * struct iio_hwmon_alarm_attribute - IIO HWMON alarm attribute
> + * @dev_attr: Base device attribute
> + * @listener: Listener for this alarm
> + * @index: Index of the channel in the IIO HWMON
> + * @alarm: Index of the alarm within @listener
> + */
> +struct iio_hwmon_alarm_attribute {
> +	struct device_attribute dev_attr;
> +	struct iio_hwmon_listener *listener;
> +	size_t index;
> +	size_t alarm;
> +};
> +#define to_alarm_attr(_dev_attr) \
> +	container_of(_dev_attr, struct iio_hwmon_alarm_attribute, dev_attr)
> +
> +/**
> + * iio_hwmon_alarm_toggle() - Turn an event off and back on again
> + * @chan: Channel of the event
> + * @dir: Event direction (rising, falling, etc.)
> + *
> + * Toggle an event's enable so we get notified if the alarm is already
> + * triggered. We use this to convert IIO's event-triggered events into
> + * level-triggered alarms.
> + *
> + * Return: 0 on success or negative error on failure
> + */
> +static int iio_hwmon_alarm_toggle(struct iio_channel *chan,
> +				  enum iio_event_direction dir)
> +{
> +	int ret;
> +
> +	ret = iio_write_event_processed_scale(chan, IIO_EV_TYPE_THRESH, dir,
> +					      IIO_EV_INFO_ENABLE, 0, 1);
> +	if (ret)
> +		return ret;
> +
> +	return iio_write_event_processed_scale(chan, IIO_EV_TYPE_THRESH, dir,
> +					       IIO_EV_INFO_ENABLE, 1, 1);
> +}
> +
> +static ssize_t iio_hwmon_read_alarm(struct device *dev,
> +				    struct device_attribute *attr,
> +				    char *buf)
> +{
> +	struct iio_hwmon_alarm_attribute *sattr = to_alarm_attr(attr);
> +	struct iio_hwmon_state *state = dev_get_drvdata(dev);
> +	struct iio_channel *chan = &state->channels[sattr->index];
> +
> +	if (test_and_clear_bit(sattr->alarm, sattr->listener->alarms)) {
> +		u64 id = sattr->listener->ids[sattr->alarm];
> +		enum iio_event_direction dir =
> IIO_EVENT_CODE_EXTRACT_DIR(id);
> +
> +		WARN_ON(iio_hwmon_alarm_toggle(chan, dir));

WARN_ON() is highly discouraged... Also do we really need a "scary" splat in
this case?

> +		strcpy(buf, "1\n");
> +		return 2;
> +	}
> +
> +	strcpy(buf, "0\n");
> +	return 2;

As stated, sysfs_emit()

> +}
> +
>  static int add_device_attr(struct device *dev, struct iio_hwmon_state *st,
>  			   ssize_t (*show)(struct device *dev,
>  					   struct device_attribute *attr,
> @@ -205,6 +453,63 @@ static int add_event_attr(struct device *dev, struct
> iio_hwmon_state *st,
>  	return 0;
>  }
>  
> +static int add_alarm_attr(struct device *dev, struct iio_hwmon_state *st,
> +			  int i, enum iio_event_direction dir,
> +			  const char *fmt, ...)
> +{
> +	struct iio_hwmon_alarm_attribute *a;
> +	struct iio_hwmon_listener *listener;
> +	ssize_t alarm;
> +	umode_t mode;
> +	va_list ap;
> +	int ret;
> +
> +	mode = iio_event_mode(&st->channels[i], IIO_EV_TYPE_THRESH, dir,
> +			      IIO_EV_INFO_ENABLE);
> +	if (!(mode & 0200))
> +		return 0;
> +
> +	listener = iio_hwmon_listener_get(st->channels[i].indio_dev);
> +	if (listener == ERR_PTR(-EBUSY))
> +		return 0;

Maybe I missed something, where can we get -EBUSY? And should we ignore it?

> +	if (IS_ERR(listener))
> +		return PTR_ERR(listener);
> +
> +	ret = devm_add_action_or_reset(dev, iio_hwmon_listener_put,
> listener);
> +	if (ret)
> +		return ret;
> +
> +	alarm = iio_hwmon_lookup_alarm(listener,
> +				       iio_event_id(st->channels[i].channel,
> +						    IIO_EV_TYPE_THRESH,
> dir));
> +	if (WARN_ON_ONCE(alarm < 0))
> +		return -ENOENT;
> +

Again, I would drop WARN_ON_ONCE()

- Nuno Sá

Powered by blists - more mailing lists