Re: [PATCH v2] iio: hid-sensors: Use software trigger

[Jonathan Cameron]

On Fri, 20 Feb 2026 14:45:14 -0800
Srinivas Pandruvada <srinivas.pandruvada@xxxxxxxxxxxxxxx> wrote:

> Recent changes linux mainline resulted in warning:
> "genirq: Warn about using IRQF_ONESHOT without a threaded handler"
> when HID sensor hub is used.
> 
> When INDIO_BUFFER_TRIGGERED is used, the core attaches a poll function
> when enabling the buffer. This poll function uses request_threaded_irq()
> with both bottom half and top half handlers. But when using HID
> sensor hub, bottom half (thread handler) is not registered.
> 
> In HID sensors, once a sensor is powered on, the hub collects samples
> and pushes data to the host when programmed thresholds are met. When
> this data is received for a sensor, it is pushed using
> iio_push_to_buffers_with_ts().
> 
> The sensor is powered ON or OFF based on the trigger callback
> set_trigger_state() when the poll function is attached. During the call
> to iio_triggered_buffer_setup_ext(), the HID sensor specifies only a
> handler function but provides no thread handler, as there is no data
> to read from the hub in thread context. Internally, this results in
> calling request_threaded_irq(). Recent kernel changes now warn when
> request_threaded_irq() is called without a thread handler.
> 
> To address this issue, fundamental changes are required to avoid using
> iio_triggered_buffer_setup_ext(). HID sensors can use
> INDIO_BUFFER_SOFTWARE instead of INDIO_BUFFER_TRIGGERED, as this can
> work in trigger-less mode.
> 
> In this approach, when user space opens the buffer, the sensor is powered
> on, and when the buffer is closed, the sensor is powered off using
> iio_buffer_setup_ops callbacks.
> 
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@xxxxxxxxxxxxxxx>
I think this is fine, but it almost looks like coincidence that where we
introduce a new devm_ call is at the point where the driver moves
from devm to non devm - the only place we can safely swap one for
the other like this.

It might we be worth exploring if the driver can easily make more
use of devm to reduce what we have to manually cleanup.

Anyhow, applied this to the fixes-togreg branch of iio.git.
I haven't given it a fixes tag though as there wasn't really a bug here
just some misuse of interfaces.


> ---
> Changes:
> v2:
> changed 
> indio_dev->modes |= INDIO_DIRECT_MODE | INDIO_HARDWARE_TRIGGERED;
> to 
> indio_dev->modes = INDIO_DIRECT_MODE | INDIO_HARDWARE_TRIGGERED;
> 
> From RFC
> - The trigger alloc and register is still kept as old code with
> additional
> indio_dev->modes |= INDIO_DIRECT_MODE | INDIO_HARDWARE_TRIGGERED;
> 
>  .../common/hid-sensors/hid-sensor-trigger.c   | 48 ++++++++++++-------
>  1 file changed, 30 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> index 5540e2d28f4a..417c4ab8c1b2 100644
> --- a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> +++ b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> @@ -14,6 +14,7 @@
>  #include <linux/iio/triggered_buffer.h>
>  #include <linux/iio/trigger_consumer.h>
>  #include <linux/iio/sysfs.h>
> +#include <linux/iio/kfifo_buf.h>
>  #include "hid-sensor-trigger.h"
>  
>  static ssize_t _hid_sensor_set_report_latency(struct device *dev,
> @@ -202,12 +203,21 @@ static void hid_sensor_set_power_work(struct work_struct *work)
>  		_hid_sensor_power_state(attrb, true);
>  }
>  
> -static int hid_sensor_data_rdy_trigger_set_state(struct iio_trigger *trig,
> -						bool state)
> +static int buffer_postenable(struct iio_dev *indio_dev)
>  {
> -	return hid_sensor_power_state(iio_trigger_get_drvdata(trig), state);
> +	return hid_sensor_power_state(iio_device_get_drvdata(indio_dev), 1);
>  }
>  
> +static int buffer_predisable(struct iio_dev *indio_dev)
> +{
> +	return hid_sensor_power_state(iio_device_get_drvdata(indio_dev), 0);
> +}
> +
> +static const struct iio_buffer_setup_ops hid_sensor_buffer_ops = {
> +	.postenable = buffer_postenable,
> +	.predisable = buffer_predisable,
> +};
> +
>  void hid_sensor_remove_trigger(struct iio_dev *indio_dev,
>  			       struct hid_sensor_common *attrb)
>  {
> @@ -219,14 +229,9 @@ void hid_sensor_remove_trigger(struct iio_dev *indio_dev,
>  	cancel_work_sync(&attrb->work);
>  	iio_trigger_unregister(attrb->trigger);
>  	iio_trigger_free(attrb->trigger);
> -	iio_triggered_buffer_cleanup(indio_dev);
>  }
>  EXPORT_SYMBOL_NS(hid_sensor_remove_trigger, "IIO_HID");
>  
> -static const struct iio_trigger_ops hid_sensor_trigger_ops = {
> -	.set_trigger_state = &hid_sensor_data_rdy_trigger_set_state,
> -};
> -
>  int hid_sensor_setup_trigger(struct iio_dev *indio_dev, const char *name,
>  				struct hid_sensor_common *attrb)
>  {
> @@ -239,25 +244,34 @@ int hid_sensor_setup_trigger(struct iio_dev *indio_dev, const char *name,
>  	else
>  		fifo_attrs = NULL;
>  
> -	ret = iio_triggered_buffer_setup_ext(indio_dev,
> -					     &iio_pollfunc_store_time, NULL,
> -					     IIO_BUFFER_DIRECTION_IN,
> -					     NULL, fifo_attrs);
> +	indio_dev->modes = INDIO_DIRECT_MODE | INDIO_HARDWARE_TRIGGERED;
> +
> +	ret = devm_iio_kfifo_buffer_setup_ext(&indio_dev->dev, indio_dev,
> +					      &hid_sensor_buffer_ops,
> +					      fifo_attrs);
>  	if (ret) {
> -		dev_err(&indio_dev->dev, "Triggered Buffer Setup Failed\n");
> +		dev_err(&indio_dev->dev, "Kfifo Buffer Setup Failed\n");
>  		return ret;
>  	}
>  
> +	/*
> +	 * The current user space in distro "iio-sensor-proxy" is not working in
> +	 * trigerless mode and it expects
> +	 * /sys/bus/iio/devices/iio:device0/trigger/current_trigger.
> +	 * The change replacing iio_triggered_buffer_setup_ext() with
> +	 * devm_iio_kfifo_buffer_setup_ext() will not create attribute without
> +	 * registering a trigger with INDIO_HARDWARE_TRIGGERED.
> +	 * So the below code fragment is still required.
> +	 */
> +
>  	trig = iio_trigger_alloc(indio_dev->dev.parent,
>  				 "%s-dev%d", name, iio_device_id(indio_dev));
>  	if (trig == NULL) {
>  		dev_err(&indio_dev->dev, "Trigger Allocate Failed\n");
> -		ret = -ENOMEM;
> -		goto error_triggered_buffer_cleanup;
> +		return -ENOMEM;
>  	}
>  
>  	iio_trigger_set_drvdata(trig, attrb);
> -	trig->ops = &hid_sensor_trigger_ops;
>  	ret = iio_trigger_register(trig);
>  
>  	if (ret) {
> @@ -284,8 +298,6 @@ int hid_sensor_setup_trigger(struct iio_dev *indio_dev, const char *name,
>  	iio_trigger_unregister(trig);
>  error_free_trig:
>  	iio_trigger_free(trig);
> -error_triggered_buffer_cleanup:
> -	iio_triggered_buffer_cleanup(indio_dev);
>  	return ret;
>  }
>  EXPORT_SYMBOL_NS(hid_sensor_setup_trigger, "IIO_HID");