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

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

[Srinivas Pandruvada]

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.

 .../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,
+};
+
 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);
-	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)
 {
 	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;
 	}
-
-	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");
 
-- 
2.52.0

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

[Jonathan Cameron]

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.

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;

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.

> +
>  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.
> -
>  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.

> -
> -	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");
>  

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

[srinivas pandruvada]

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.


> 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");
> >  

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

[Jonathan Cameron]

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");
> > >    
> 

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

[srinivas pandruvada]

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

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

[Jonathan Cameron]

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");
> > > > >      
> > >