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

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 18 Feb 2026 15:47:09 -0800
srinivas pandruvada <srinivas.pandruvada@linux.intel.com> wrote:

> On Wed, 2026-02-18 at 19:13 +0000, Jonathan Cameron wrote:
> > 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?
> >   
> 
> Driver initialize this to
> iio_dev->modes = INDIO_DIRECT
> 
> Call to 
> https://elixir.bootlin.com/linux/v6.18.6/C/ident/devm_iio_kfifo_buffer_setup_ext
> will set to 
> 
> indio_dev->modes |= INDIO_BUFFER_SOFTWARE;
> 
> After if we do
> iio_dev->modes = INDIO_DIRECT | INDIO_HARDWARE_TRIGGERED;
> 
> INDIO_BUFFER_SOFTWARE will be overwritten.
> 
> I think we can set 
> iio_dev->modes = INDIO_DIRECT | INDIO_HARDWARE_TRIGGERED;
> 
> before calling
> devm_iio_kfifo_buffer_setup_ext(), then there is no need for "|".

Thanks. That makes sense.   At some point we should probably wrap
up seeing modes in a helper to ensure the ordering doesn't matter
for cases like this one.

That will be a big job for fairly small gain though so maybe not
worth the effort.

Jonathan

> 
> Thanks,
> Srinivas
> 
> 
> > 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");
> > > > >      
> > >