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

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

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