iio: Simplify IRQ and trigger management in RPR0521

[PATCH RFC v1 1/2] iio: light: rpr0521: Use generic iio_pollfunc_store_time()

Posted by Vasileios Amoiridis 1 year, 4 months ago

The custom rpr0521_trigger_consumer_store_time() is registered as trigger
handler in the devm_iio_triggered_buffer_setup() function. This function
is called from the calling of the iio_trigger_poll() used in the
sysfs/hrt triggers and it is not used anywhere else in this driver.

The irq handler of the driver is the rpr0521_drdy_irq_handler() which
saves the timestamp and then wakes the irq thread. The irq thread is
the rpr0521_drdy_irq_thread() function which checks if the irq came
from the sensor and wakes up the trigger threaded handler through
iio_trigger_poll_nested() or returns IRQ_NONE in case the irq didn't
come from this sensor.

This means that in the current driver, you can't reach the
rpr0521_trigger_consumer_store_time() when the device's irq is
triggered. This means that the extra check of iio_trigger_using_own()
is redundant since it will always be false so the general
iio_pollfunc_store_time() can be used.

Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
---
 drivers/iio/light/rpr0521.c | 14 +-------------
 1 file changed, 1 insertion(+), 13 deletions(-)

diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
index 78c08e0bd077..56f5fbbf79ac 100644
--- a/drivers/iio/light/rpr0521.c
+++ b/drivers/iio/light/rpr0521.c
@@ -438,18 +438,6 @@ static irqreturn_t rpr0521_drdy_irq_thread(int irq, void *private)
 	return IRQ_NONE;
 }
 
-static irqreturn_t rpr0521_trigger_consumer_store_time(int irq, void *p)
-{
-	struct iio_poll_func *pf = p;
-	struct iio_dev *indio_dev = pf->indio_dev;
-
-	/* Other trigger polls store time here. */
-	if (!iio_trigger_using_own(indio_dev))
-		pf->timestamp = iio_get_time_ns(indio_dev);
-
-	return IRQ_WAKE_THREAD;
-}
-
 static irqreturn_t rpr0521_trigger_consumer_handler(int irq, void *p)
 {
 	struct iio_poll_func *pf = p;
@@ -1016,7 +1004,7 @@ static int rpr0521_probe(struct i2c_client *client)
 		/* Trigger consumer setup */
 		ret = devm_iio_triggered_buffer_setup(indio_dev->dev.parent,
 			indio_dev,
-			rpr0521_trigger_consumer_store_time,
+			iio_pollfunc_store_time,
 			rpr0521_trigger_consumer_handler,
 			&rpr0521_buffer_setup_ops);
 		if (ret < 0) {
-- 
2.25.1

Re: [PATCH RFC v1 1/2] iio: light: rpr0521: Use generic iio_pollfunc_store_time()

Posted by Jonathan Cameron 1 year, 4 months ago

On Sun, 22 Sep 2024 18:20:40 +0200
Vasileios Amoiridis <vassilisamir@gmail.com> wrote:

> The custom rpr0521_trigger_consumer_store_time() is registered as trigger
> handler in the devm_iio_triggered_buffer_setup() function. This function
> is called from the calling of the iio_trigger_poll() used in the
> sysfs/hrt triggers and it is not used anywhere else in this driver.
It might be any number of other triggers (hardware triggers from other
drivers for example).

Other than that I think your reasoning is correct but would ideally
like some input from someone more familiar with this driver.

If that isn't forthcoming I'll pick this up in a week or two.

Jonathan

> 
> The irq handler of the driver is the rpr0521_drdy_irq_handler() which
> saves the timestamp and then wakes the irq thread. The irq thread is
> the rpr0521_drdy_irq_thread() function which checks if the irq came
> from the sensor and wakes up the trigger threaded handler through
> iio_trigger_poll_nested() or returns IRQ_NONE in case the irq didn't
> come from this sensor.
> 
> This means that in the current driver, you can't reach the
> rpr0521_trigger_consumer_store_time() when the device's irq is
> triggered. This means that the extra check of iio_trigger_using_own()
> is redundant since it will always be false so the general
> iio_pollfunc_store_time() can be used.
> 
> Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> ---
>  drivers/iio/light/rpr0521.c | 14 +-------------
>  1 file changed, 1 insertion(+), 13 deletions(-)
> 
> diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
> index 78c08e0bd077..56f5fbbf79ac 100644
> --- a/drivers/iio/light/rpr0521.c
> +++ b/drivers/iio/light/rpr0521.c
> @@ -438,18 +438,6 @@ static irqreturn_t rpr0521_drdy_irq_thread(int irq, void *private)
>  	return IRQ_NONE;
>  }
>  
> -static irqreturn_t rpr0521_trigger_consumer_store_time(int irq, void *p)
> -{
> -	struct iio_poll_func *pf = p;
> -	struct iio_dev *indio_dev = pf->indio_dev;
> -
> -	/* Other trigger polls store time here. */
> -	if (!iio_trigger_using_own(indio_dev))
> -		pf->timestamp = iio_get_time_ns(indio_dev);
> -
> -	return IRQ_WAKE_THREAD;
> -}
> -
>  static irqreturn_t rpr0521_trigger_consumer_handler(int irq, void *p)
>  {
>  	struct iio_poll_func *pf = p;
> @@ -1016,7 +1004,7 @@ static int rpr0521_probe(struct i2c_client *client)
>  		/* Trigger consumer setup */
>  		ret = devm_iio_triggered_buffer_setup(indio_dev->dev.parent,
>  			indio_dev,
> -			rpr0521_trigger_consumer_store_time,
> +			iio_pollfunc_store_time,
>  			rpr0521_trigger_consumer_handler,
>  			&rpr0521_buffer_setup_ops);
>  		if (ret < 0) {

Re: [PATCH RFC v1 1/2] iio: light: rpr0521: Use generic iio_pollfunc_store_time()

Posted by Jonathan Cameron 1 year, 3 months ago

On Sat, 28 Sep 2024 16:10:17 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Sun, 22 Sep 2024 18:20:40 +0200
> Vasileios Amoiridis <vassilisamir@gmail.com> wrote:
> 
> > The custom rpr0521_trigger_consumer_store_time() is registered as trigger
> > handler in the devm_iio_triggered_buffer_setup() function. This function
> > is called from the calling of the iio_trigger_poll() used in the
> > sysfs/hrt triggers and it is not used anywhere else in this driver.  
> It might be any number of other triggers (hardware triggers from other
> drivers for example).
> 
> Other than that I think your reasoning is correct but would ideally
> like some input from someone more familiar with this driver.
> 
> If that isn't forthcoming I'll pick this up in a week or two.
Two weeks gone. No one surfaced and I think this is fine.

Applied.

Jonathan

> 
> Jonathan
> 
> > 
> > The irq handler of the driver is the rpr0521_drdy_irq_handler() which
> > saves the timestamp and then wakes the irq thread. The irq thread is
> > the rpr0521_drdy_irq_thread() function which checks if the irq came
> > from the sensor and wakes up the trigger threaded handler through
> > iio_trigger_poll_nested() or returns IRQ_NONE in case the irq didn't
> > come from this sensor.
> > 
> > This means that in the current driver, you can't reach the
> > rpr0521_trigger_consumer_store_time() when the device's irq is
> > triggered. This means that the extra check of iio_trigger_using_own()
> > is redundant since it will always be false so the general
> > iio_pollfunc_store_time() can be used.
> > 
> > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> > ---
> >  drivers/iio/light/rpr0521.c | 14 +-------------
> >  1 file changed, 1 insertion(+), 13 deletions(-)
> > 
> > diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
> > index 78c08e0bd077..56f5fbbf79ac 100644
> > --- a/drivers/iio/light/rpr0521.c
> > +++ b/drivers/iio/light/rpr0521.c
> > @@ -438,18 +438,6 @@ static irqreturn_t rpr0521_drdy_irq_thread(int irq, void *private)
> >  	return IRQ_NONE;
> >  }
> >  
> > -static irqreturn_t rpr0521_trigger_consumer_store_time(int irq, void *p)
> > -{
> > -	struct iio_poll_func *pf = p;
> > -	struct iio_dev *indio_dev = pf->indio_dev;
> > -
> > -	/* Other trigger polls store time here. */
> > -	if (!iio_trigger_using_own(indio_dev))
> > -		pf->timestamp = iio_get_time_ns(indio_dev);
> > -
> > -	return IRQ_WAKE_THREAD;
> > -}
> > -
> >  static irqreturn_t rpr0521_trigger_consumer_handler(int irq, void *p)
> >  {
> >  	struct iio_poll_func *pf = p;
> > @@ -1016,7 +1004,7 @@ static int rpr0521_probe(struct i2c_client *client)
> >  		/* Trigger consumer setup */
> >  		ret = devm_iio_triggered_buffer_setup(indio_dev->dev.parent,
> >  			indio_dev,
> > -			rpr0521_trigger_consumer_store_time,
> > +			iio_pollfunc_store_time,
> >  			rpr0521_trigger_consumer_handler,
> >  			&rpr0521_buffer_setup_ops);
> >  		if (ret < 0) {  
> 
>