| Message ID | 20210422101911.135630-6-nuno.sa@analog.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Adis IRQ fixes and minor improvements | expand |
On Thu, Apr 22, 2021 at 1:17 PM Nuno Sa <nuno.sa@analog.com> wrote: > > There's no point in pushing data to IIO buffers in case 'spi_sync()' > fails. > Overall, this feels like it's adding some duplication. However, short-term I'm not seeing a considerably better way to do this. Maybe, this would require some refactoring of the adis_trigger_handler() to handle the paging logic a bit more elegantly. But that's a broader change. Reviewed-by: Alexandru Ardelean <ardeleanalex@gmail.com> > Signed-off-by: Nuno Sa <nuno.sa@analog.com> > --- > drivers/iio/imu/adis_buffer.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/iio/imu/adis_buffer.c b/drivers/iio/imu/adis_buffer.c > index 0ae551a748eb..a29d22f657ce 100644 > --- a/drivers/iio/imu/adis_buffer.c > +++ b/drivers/iio/imu/adis_buffer.c > @@ -144,9 +144,12 @@ static irqreturn_t adis_trigger_handler(int irq, void *p) > } > > ret = spi_sync(adis->spi, &adis->msg); > - if (ret) > + if (ret) { > dev_err(&adis->spi->dev, "Failed to read data: %d", ret); > - > + if (adis->data->has_paging) > + mutex_unlock(&adis->state_lock); > + goto irq_done; > + } > > if (adis->data->has_paging) { > adis->current_page = 0; > -- > 2.31.1 >
On Fri, 23 Apr 2021 10:28:08 +0300 Alexandru Ardelean <ardeleanalex@gmail.com> wrote: > On Thu, Apr 22, 2021 at 1:17 PM Nuno Sa <nuno.sa@analog.com> wrote: > > > > There's no point in pushing data to IIO buffers in case 'spi_sync()' > > fails. > > > > Overall, this feels like it's adding some duplication. > However, short-term I'm not seeing a considerably better way to do this. > Maybe, this would require some refactoring of the > adis_trigger_handler() to handle the paging logic a bit more > elegantly. > But that's a broader change. > > Reviewed-by: Alexandru Ardelean <ardeleanalex@gmail.com> In here the read failed, but I think the switch to current_page = 0 succeeded (as was before this spi_sync). So should we not be setting current_page = 0 even int his error path? With that in mind can we just move the if (ret) check past the existing unlock? + does it make sense to just move the setting of current_page = 0 up to where it's actually set? > > > Signed-off-by: Nuno Sa <nuno.sa@analog.com> > > --- > > drivers/iio/imu/adis_buffer.c | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/iio/imu/adis_buffer.c b/drivers/iio/imu/adis_buffer.c > > index 0ae551a748eb..a29d22f657ce 100644 > > --- a/drivers/iio/imu/adis_buffer.c > > +++ b/drivers/iio/imu/adis_buffer.c > > @@ -144,9 +144,12 @@ static irqreturn_t adis_trigger_handler(int irq, void *p) > > } > > > > ret = spi_sync(adis->spi, &adis->msg); > > - if (ret) > > + if (ret) { > > dev_err(&adis->spi->dev, "Failed to read data: %d", ret); > > - > > + if (adis->data->has_paging) > > + mutex_unlock(&adis->state_lock); > > + goto irq_done; > > + } > > > > if (adis->data->has_paging) { > > adis->current_page = 0; > > -- > > 2.31.1 > >
diff --git a/drivers/iio/imu/adis_buffer.c b/drivers/iio/imu/adis_buffer.c index 0ae551a748eb..a29d22f657ce 100644 --- a/drivers/iio/imu/adis_buffer.c +++ b/drivers/iio/imu/adis_buffer.c @@ -144,9 +144,12 @@ static irqreturn_t adis_trigger_handler(int irq, void *p) } ret = spi_sync(adis->spi, &adis->msg); - if (ret) + if (ret) { dev_err(&adis->spi->dev, "Failed to read data: %d", ret); - + if (adis->data->has_paging) + mutex_unlock(&adis->state_lock); + goto irq_done; + } if (adis->data->has_paging) { adis->current_page = 0;
There's no point in pushing data to IIO buffers in case 'spi_sync()' fails. Signed-off-by: Nuno Sa <nuno.sa@analog.com> --- drivers/iio/imu/adis_buffer.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)