| Message ID | 20240901120839.9274-1-gye976@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v2] iio: imu: inv_mpu6050: Remove duplicate code between labels | expand |
On Sun, 1 Sep 2024 21:08:39 +0900 Gyeyoung Baek <gye976@gmail.com> wrote: > 'flush_fifo' label performs same task as 'endsession' label > immediately after calling 'env_reset_fifo' function. > So i remove that duplication. > > Signed-off-by: Gyeyoung Baek <gye976@gmail.com> Ok. This doesn't greatly affect readability and the code ends up a bit shorter. Applied Jonathan > --- > I worked on the wrong branch in v1, and reworked on the latest branch. > > drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c > index 3d3b27f28c9d..4753016d6ee0 100644 > --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c > +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c > @@ -121,15 +121,12 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p) > iio_push_to_buffers_with_timestamp(indio_dev, data, timestamp); > } > > -end_session: > - mutex_unlock(&st->lock); > - iio_trigger_notify_done(indio_dev->trig); > - > - return IRQ_HANDLED; > > flush_fifo: > /* Flush HW and SW FIFOs. */ > inv_reset_fifo(indio_dev); > + > +end_session: > mutex_unlock(&st->lock); > iio_trigger_notify_done(indio_dev->trig); >
On Sun, Sep 1, 2024 at 11:11 PM Jonathan Cameron <jic23@kernel.org> wrote: > > On Sun, 1 Sep 2024 21:08:39 +0900 > Gyeyoung Baek <gye976@gmail.com> wrote: > > > 'flush_fifo' label performs same task as 'endsession' label > > immediately after calling 'env_reset_fifo' function. > > So i remove that duplication. > > > > Signed-off-by: Gyeyoung Baek <gye976@gmail.com> > > Ok. This doesn't greatly affect readability and the code > ends up a bit shorter. > > Applied > > Jonathan > Thank you for approving it. -Gyeyoung
Hello, thanks for the patch, but beware that this modification is breaking the code! You are deleting the normal function path return IRQ_HANDLED (without the goto end_session). With this patch, the reset fifo function code inv_reset_fifo() will be called even if the IRQ handler is functioning correctly. This is a no go for me, I don't think we can easily change these 2 labels anyway. Best regards, JB
> > 'flush_fifo' label performs same task as 'endsession' label > > immediately after calling 'env_reset_fifo' function. > > So i remove that duplication. … > Ok. This doesn't greatly affect readability and the code > ends up a bit shorter. How can you find such a change description acceptable? > Applied Please reconsider this positive feedback once more. It temporarily looked too promising to apply a goto chain. But the original control flow needs other development considerations if you would like to reduce the explicit repetition of common actions. Regards, Markus
Sun, Sep 01, 2024 at 09:08:39PM +0900, Gyeyoung Baek kirjoitti: > 'flush_fifo' label performs same task as 'endsession' label > immediately after calling 'env_reset_fifo' function. > So i remove that duplication. ... > -end_session: > - mutex_unlock(&st->lock); > - iio_trigger_notify_done(indio_dev->trig); > - > - return IRQ_HANDLED; You missed goto end_session; here. > flush_fifo: > /* Flush HW and SW FIFOs. */ > inv_reset_fifo(indio_dev); > + > +end_session: > mutex_unlock(&st->lock); > iio_trigger_notify_done(indio_dev->trig);
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c index 3d3b27f28c9d..4753016d6ee0 100644 --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c @@ -121,15 +121,12 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p) iio_push_to_buffers_with_timestamp(indio_dev, data, timestamp); } -end_session: - mutex_unlock(&st->lock); - iio_trigger_notify_done(indio_dev->trig); - - return IRQ_HANDLED; flush_fifo: /* Flush HW and SW FIFOs. */ inv_reset_fifo(indio_dev); + +end_session: mutex_unlock(&st->lock); iio_trigger_notify_done(indio_dev->trig);
'flush_fifo' label performs same task as 'endsession' label immediately after calling 'env_reset_fifo' function. So i remove that duplication. Signed-off-by: Gyeyoung Baek <gye976@gmail.com> --- I worked on the wrong branch in v1, and reworked on the latest branch. drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)