| Message ID | 20240922162041.525896-3-vassilisamir@gmail.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | iio: Simplify IRQ and trigger management in RPR0521 | expand |
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); > > >
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);
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(-)