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