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

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 16 Feb 2026 14:49:38 -0800
srinivas pandruvada <srinivas.pandruvada@linux.intel.com> wrote:

> On Sat, 2026-02-14 at 18:16 +0000, Jonathan Cameron wrote:
> > On Mon,  9 Feb 2026 12:42:27 -0800
> > Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> 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@linux.intel.com>
> > > ---
> > > This is RFC, because
> > > The current user space in distro "iio-sensor-proxy" is not working
> > > in
> > > trigerless mode as it expects
> > > /sys/bus/iio/devices/iio:device0/trigger/current_trigger.
> > > So, change needs to be submitted to fix that.  
> > 
> > Sorry I took a while to reply to the previous thread - been off sick
> > and
> > just catching up again.  
> 
> No problem. Hope you are feeling better.
> 
> > 
> > I think we can't make this change on it's own because of the
> > backwards compatibility
> > problem.  Please can you try what you have here without removing the
> > trigger adding
> > chunk (as we still need that to exist) + 
> > 
> > iio_dev->modes = INDIO_DIRECT | INDIO_HARDWARE_TRIGGERED;
> >   
> This is not enough as this will fail when buffer0 enable attribute is
> set to 1.
> 
> https://elixir.bootlin.com/linux/v6.18.6/source/drivers/iio/industrialio-buffer.c#L951

 
> But 
> 
> iio_dev->modes |= INDIO_DIRECT | INDIO_HARDWARE_TRIGGERED;
> 
> works.

I'm lost.  Which other mode is set?   Maybe shift this up before
whatever sets that would be clearer?

J
> 
> 
> > It's been a while but I think that is there basically to hook up
> > current_trigger.
> > That was intended for cases where there are several to choose between
> > but
> > I think it should do the job here of bringing back the interface.  
> > Add a comment
> > though on why it is there.
> > 
> > I've tried to say roughly what to keep and drop inline.
> > 
> > thanks,
> > 
> > Jonathan
> > 
> > 
> >   
> > > 
> > >  .../common/hid-sensors/hid-sensor-trigger.c   | 62 ++++++---------
> > > ----
> > >  1 file changed, 18 insertions(+), 44 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..113fd1361643 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,
> > > +};  
> > I think these changes all help simplify things anyway so probably
> > good to have.  Maybe we could do them in a follow up rather than the
> > fix but I'll leave that up to you  
> 
> This is required as the hid_sensor_trigger_ops.set_trigger_state() is
> not called once iio_triggered_buffer_setup_ext() is removed.
> 
> >   
> > > +
> > >  void hid_sensor_remove_trigger(struct iio_dev *indio_dev,
> > >  			       struct hid_sensor_common *attrb)
> > >  {
> > > @@ -217,59 +227,30 @@ void hid_sensor_remove_trigger(struct iio_dev
> > > *indio_dev,
> > >  	pm_runtime_set_suspended(&attrb->pdev->dev);
> > >  
> > >  	cancel_work_sync(&attrb->work);
> > > -	iio_trigger_unregister(attrb->trigger);
> > > -	iio_trigger_free(attrb->trigger);  
> > Keep the trigger parts here.
> >   
> > > -	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,
> > > -};  
> > and this.  
> This callback is not called without iio_triggered_buffer_setup_ext().
> 
> > > -
> > >  int hid_sensor_setup_trigger(struct iio_dev *indio_dev, const char
> > > *name,
> > >  				struct hid_sensor_common *attrb)
> > >  {
> > >  	const struct iio_dev_attr **fifo_attrs;
> > >  	int ret;
> > > -	struct iio_trigger *trig;
> > >  
> > >  	if (hid_sensor_batch_mode_supported(attrb))
> > >  		fifo_attrs = hid_sensor_fifo_attributes;
> > >  	else
> > >  		fifo_attrs = NULL;
> > >  
> > > -	ret = iio_triggered_buffer_setup_ext(indio_dev,
> > > -					    
> > > &iio_pollfunc_store_time, NULL,
> > > -					    
> > > IIO_BUFFER_DIRECTION_IN,
> > > -					     NULL, fifo_attrs);
> > > +	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;
> > >  	}  
> > Down to here is good but keep the trigger setup.  
> 
> I can keep with additional
> 
> iio_dev->modes |= INDIO_DIRECT | INDIO_HARDWARE_TRIGGERED;
> 
> I will send a patch with the changes.
> 
> Thanks,
> Srinivas
> 
> >   
> > > -
> > > -	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;
> > > -	}
> > > -
> > > -	iio_trigger_set_drvdata(trig, attrb);
> > > -	trig->ops = &hid_sensor_trigger_ops;
> > > -	ret = iio_trigger_register(trig);
> > > -
> > > -	if (ret) {
> > > -		dev_err(&indio_dev->dev, "Trigger Register
> > > Failed\n");
> > > -		goto error_free_trig;
> > > -	}
> > > -	attrb->trigger = trig;
> > > -	indio_dev->trig = iio_trigger_get(trig);
> > > -
> > >  	ret = pm_runtime_set_active(&indio_dev->dev);
> > >  	if (ret)
> > > -		goto error_unreg_trigger;
> > > +		return ret;
> > >  
> > >  	iio_device_set_drvdata(indio_dev, attrb);
> > >  
> > > @@ -280,13 +261,6 @@ int hid_sensor_setup_trigger(struct iio_dev
> > > *indio_dev, const char *name,
> > >  	pm_runtime_set_autosuspend_delay(&attrb->pdev->dev,
> > >  					 3000);
> > >  	return ret;
> > > -error_unreg_trigger:
> > > -	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");
> > >    
>