[PATCH RFC v1 0/2] iio: Simplify IRQ and trigger management in RPR0521

[PATCH RFC v1 0/2] iio: Simplify IRQ and trigger management in RPR0521

[Vasileios Amoiridis]

This is an RFC series, because I have no way to test those changes apart from
compiling them. It was just a bit unfamiliar the way the trigger and irq was
working in this driver compared to the other ones so I did a bit of digging to
see if we it is actually needed and if we can simplify it somehow. Plese let me
know how this looks like.

Vasileios Amoiridis (2):
  iio: light: rpr0521: Use generic iio_pollfunc_store_time()
  iio: light: rpr0521: Drop unnecessary checks for timestamp value

 drivers/iio/light/rpr0521.c | 22 ++--------------------
 1 file changed, 2 insertions(+), 20 deletions(-)


base-commit: 8bea3878a1511bceadc2fbf284b00bcc5a2ef28d
-- 
2.25.1

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

[Vasileios Amoiridis]

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

[PATCH RFC v1 2/2] iio: light: rpr0521: Drop unnecessary checks for timestamp value

[Vasileios Amoiridis]

The rpr0521_trigger_consumer_handler() is registered as the trigger
threaded handler in the devm_iio_triggered_buffer_setup() function.
This function is being called in 2 ways:

a) when there is a registered trigger being trigger like sysfs or hrt.
The call of the trigger handler (which is the iio_pollfunc_store_time())
follows which saves the timestamp and then, wakes up the trigger
threaded handler.

b) The irq handler is using the iio_trigger_poll_nested() which wakes
up the trigger threaded handler.

In both cases, the pf->timestamp has already been assigned a value
so there is no need to check if it is 0, neither to 0 it after
the push to the buffer.

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

diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
index 56f5fbbf79ac..ae6a22b91b8d 100644
--- a/drivers/iio/light/rpr0521.c
+++ b/drivers/iio/light/rpr0521.c
@@ -446,13 +446,8 @@ static irqreturn_t rpr0521_trigger_consumer_handler(int irq, void *p)
 	int err;
 
 	/* Use irq timestamp when reasonable. */
-	if (iio_trigger_using_own(indio_dev) && data->irq_timestamp) {
+	if (iio_trigger_using_own(indio_dev))
 		pf->timestamp = data->irq_timestamp;
-		data->irq_timestamp = 0;
-	}
-	/* Other chained trigger polls get timestamp only here. */
-	if (!pf->timestamp)
-		pf->timestamp = iio_get_time_ns(indio_dev);
 
 	err = regmap_bulk_read(data->regmap, RPR0521_REG_PXS_DATA,
 		data->scan.channels,
@@ -463,7 +458,6 @@ static irqreturn_t rpr0521_trigger_consumer_handler(int irq, void *p)
 	else
 		dev_err(&data->client->dev,
 			"Trigger consumer can't read from sensor.\n");
-	pf->timestamp = 0;
 
 	iio_trigger_notify_done(indio_dev->trig);
 
-- 
2.25.1

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

[Jonathan Cameron]

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 2/2] iio: light: rpr0521: Drop unnecessary checks for timestamp value

[Jonathan Cameron]

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

> The rpr0521_trigger_consumer_handler() is registered as the trigger
> threaded handler in the devm_iio_triggered_buffer_setup() function.
> This function is being called in 2 ways:
> 
> a) when there is a registered trigger being trigger like sysfs or hrt.
> The call of the trigger handler (which is the iio_pollfunc_store_time())
> follows which saves the timestamp and then, wakes up the trigger
> threaded handler.
> 
> b) The irq handler is using the iio_trigger_poll_nested() which wakes
> up the trigger threaded handler.
> 
> In both cases, the pf->timestamp has already been assigned a value
> so there is no need to check if it is 0, neither to 0 it after
> the push to the buffer.

Not quite right (I think).  The caller of iio_trigger_poll_nested() might not
be this driver.  There are other potential triggers that are nested
because of need to check some status register, but can still be used
for other devices.   In theory you could drive light capture of
the as3935 for when you want to know how bright it was just after
a lightening strike :)

We don't have a general solution for timestamps when that
happens, so this driver is papering over that with this code.


Jonathan



> 
> Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> ---
>  drivers/iio/light/rpr0521.c | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
> index 56f5fbbf79ac..ae6a22b91b8d 100644
> --- a/drivers/iio/light/rpr0521.c
> +++ b/drivers/iio/light/rpr0521.c
> @@ -446,13 +446,8 @@ static irqreturn_t rpr0521_trigger_consumer_handler(int irq, void *p)
>  	int err;
>  
>  	/* Use irq timestamp when reasonable. */
> -	if (iio_trigger_using_own(indio_dev) && data->irq_timestamp) {
> +	if (iio_trigger_using_own(indio_dev))
>  		pf->timestamp = data->irq_timestamp;
> -		data->irq_timestamp = 0;
> -	}
> -	/* Other chained trigger polls get timestamp only here. */
> -	if (!pf->timestamp)
> -		pf->timestamp = iio_get_time_ns(indio_dev);
>  
>  	err = regmap_bulk_read(data->regmap, RPR0521_REG_PXS_DATA,
>  		data->scan.channels,
> @@ -463,7 +458,6 @@ static irqreturn_t rpr0521_trigger_consumer_handler(int irq, void *p)
>  	else
>  		dev_err(&data->client->dev,
>  			"Trigger consumer can't read from sensor.\n");
> -	pf->timestamp = 0;
>  
>  	iio_trigger_notify_done(indio_dev->trig);
>  

Re: [PATCH RFC v1 2/2] iio: light: rpr0521: Drop unnecessary checks for timestamp value

[Vasileios Amoiridis]

On Sat, Sep 28, 2024 at 04:15:27PM +0100, Jonathan Cameron wrote:
> On Sun, 22 Sep 2024 18:20:41 +0200
> Vasileios Amoiridis <vassilisamir@gmail.com> wrote:
> 
> > The rpr0521_trigger_consumer_handler() is registered as the trigger
> > threaded handler in the devm_iio_triggered_buffer_setup() function.
> > This function is being called in 2 ways:
> > 
> > a) when there is a registered trigger being trigger like sysfs or hrt.
> > The call of the trigger handler (which is the iio_pollfunc_store_time())
> > follows which saves the timestamp and then, wakes up the trigger
> > threaded handler.
> > 
> > b) The irq handler is using the iio_trigger_poll_nested() which wakes
> > up the trigger threaded handler.
> > 
> > In both cases, the pf->timestamp has already been assigned a value
> > so there is no need to check if it is 0, neither to 0 it after
> > the push to the buffer.
> 
> Not quite right (I think).  The caller of iio_trigger_poll_nested() might not
> be this driver.  There are other potential triggers that are nested
> because of need to check some status register, but can still be used
> for other devices.   In theory you could drive light capture of
> the as3935 for when you want to know how bright it was just after
> a lightening strike :)
> 
> We don't have a general solution for timestamps when that
> happens, so this driver is papering over that with this code.
> 
> 
> Jonathan
> 

Hi Jonathan,

Thank you very much for the reply! I see your point, I also think I 
am wrong after all. I was just interested to see why this driver makes
it so different from the other ones when it is coming to trigger/irq
handling. Thank you very much for the explanation!

Cheers,
Vasilis

> 
> 
> > 
> > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> > ---
> >  drivers/iio/light/rpr0521.c | 8 +-------
> >  1 file changed, 1 insertion(+), 7 deletions(-)
> > 
> > diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
> > index 56f5fbbf79ac..ae6a22b91b8d 100644
> > --- a/drivers/iio/light/rpr0521.c
> > +++ b/drivers/iio/light/rpr0521.c
> > @@ -446,13 +446,8 @@ static irqreturn_t rpr0521_trigger_consumer_handler(int irq, void *p)
> >  	int err;
> >  
> >  	/* Use irq timestamp when reasonable. */
> > -	if (iio_trigger_using_own(indio_dev) && data->irq_timestamp) {
> > +	if (iio_trigger_using_own(indio_dev))
> >  		pf->timestamp = data->irq_timestamp;
> > -		data->irq_timestamp = 0;
> > -	}
> > -	/* Other chained trigger polls get timestamp only here. */
> > -	if (!pf->timestamp)
> > -		pf->timestamp = iio_get_time_ns(indio_dev);
> >  
> >  	err = regmap_bulk_read(data->regmap, RPR0521_REG_PXS_DATA,
> >  		data->scan.channels,
> > @@ -463,7 +458,6 @@ static irqreturn_t rpr0521_trigger_consumer_handler(int irq, void *p)
> >  	else
> >  		dev_err(&data->client->dev,
> >  			"Trigger consumer can't read from sensor.\n");
> > -	pf->timestamp = 0;
> >  
> >  	iio_trigger_notify_done(indio_dev->trig);
> >  
> 

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

[Jonathan Cameron]

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) {  
> 
>