diff mbox series

[1/9] iio: imu: adis_buffer: split trigger handling

Message ID 20240618-dev-iio-adis-cleanup-v1-1-bd93ce7845c7@analog.com (mailing list archive)
State Accepted
Headers show
Series iio: imu: adis: make use the cleanup.h magic | expand

Commit Message

Nuno Sá June 18, 2024, 1:32 p.m. UTC
Split trigger handling for devices that have paging and need to
select the correct page to get the data. Although this actually
introduces more LOC, it makes the code and the locking clear. It will
also make the following move to the cleanup magic cleaner.

Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/imu/adis_buffer.c | 56 ++++++++++++++++++++++++-------------------
 1 file changed, 31 insertions(+), 25 deletions(-)

Comments

Jonathan Cameron June 23, 2024, 4:03 p.m. UTC | #1
On Tue, 18 Jun 2024 15:32:04 +0200
Nuno Sa <nuno.sa@analog.com> wrote:

> Split trigger handling for devices that have paging and need to
> select the correct page to get the data. Although this actually
> introduces more LOC, it makes the code and the locking clear. It will
> also make the following move to the cleanup magic cleaner.
> 
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
Hi Nuno,

Just one thing,

> +		ret = spi_sync(adis->spi, &adis->msg);
> +	if (ret)
>  		dev_err(&adis->spi->dev, "Failed to read data: %d", ret);
> -		goto irq_done;
> -	}
> +	else
> +		iio_push_to_buffers_with_timestamp(indio_dev, adis->buffer,
> +						   pf->timestamp);

Keep the goto as having an indented 'good' path is not great for readability.

>  
> -	iio_push_to_buffers_with_timestamp(indio_dev, adis->buffer,
> -					   pf->timestamp);
> -
> -irq_done:
>  	iio_trigger_notify_done(indio_dev->trig);
>  
>  	return IRQ_HANDLED;
>
Jonathan Cameron June 23, 2024, 4:15 p.m. UTC | #2
On Sun, 23 Jun 2024 17:03:19 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Tue, 18 Jun 2024 15:32:04 +0200
> Nuno Sa <nuno.sa@analog.com> wrote:
> 
> > Split trigger handling for devices that have paging and need to
> > select the correct page to get the data. Although this actually
> > introduces more LOC, it makes the code and the locking clear. It will
> > also make the following move to the cleanup magic cleaner.
> > 
> > Signed-off-by: Nuno Sa <nuno.sa@analog.com>  
> Hi Nuno,
> 
> Just one thing,
> 
> > +		ret = spi_sync(adis->spi, &adis->msg);
> > +	if (ret)
> >  		dev_err(&adis->spi->dev, "Failed to read data: %d", ret);
> > -		goto irq_done;
> > -	}
> > +	else
> > +		iio_push_to_buffers_with_timestamp(indio_dev, adis->buffer,
> > +						   pf->timestamp);  
> 
> Keep the goto as having an indented 'good' path is not great for readability.
> 
Meh. That was (almost) all I found to comment on so I changed it back whilst applying.

Applied to the togreg branch of iio.git and pushed out as testing.

Thanks,

Jonathan

> >  
> > -	iio_push_to_buffers_with_timestamp(indio_dev, adis->buffer,
> > -					   pf->timestamp);
> > -
> > -irq_done:
> >  	iio_trigger_notify_done(indio_dev->trig);
> >  
> >  	return IRQ_HANDLED;
> >   
> 
>
diff mbox series

Patch

diff --git a/drivers/iio/imu/adis_buffer.c b/drivers/iio/imu/adis_buffer.c
index 871b78b225e2..d1d1e4f512b9 100644
--- a/drivers/iio/imu/adis_buffer.c
+++ b/drivers/iio/imu/adis_buffer.c
@@ -126,6 +126,30 @@  int adis_update_scan_mode(struct iio_dev *indio_dev,
 }
 EXPORT_SYMBOL_NS_GPL(adis_update_scan_mode, IIO_ADISLIB);
 
+static int adis_paging_trigger_handler(struct adis *adis)
+{
+	int ret;
+
+	mutex_lock(&adis->state_lock);
+	if (adis->current_page != 0) {
+		adis->tx[0] = ADIS_WRITE_REG(ADIS_REG_PAGE_ID);
+		adis->tx[1] = 0;
+		ret = spi_write(adis->spi, adis->tx, 2);
+		if (ret) {
+			dev_err(&adis->spi->dev, "Failed to change device page: %d\n", ret);
+			mutex_unlock(&adis->state_lock);
+			return ret;
+		}
+
+		adis->current_page = 0;
+	}
+
+	ret = spi_sync(adis->spi, &adis->msg);
+	mutex_unlock(&adis->state_lock);
+
+	return ret;
+}
+
 static irqreturn_t adis_trigger_handler(int irq, void *p)
 {
 	struct iio_poll_func *pf = p;
@@ -133,34 +157,16 @@  static irqreturn_t adis_trigger_handler(int irq, void *p)
 	struct adis *adis = iio_device_get_drvdata(indio_dev);
 	int ret;
 
-	if (adis->data->has_paging) {
-		mutex_lock(&adis->state_lock);
-		if (adis->current_page != 0) {
-			adis->tx[0] = ADIS_WRITE_REG(ADIS_REG_PAGE_ID);
-			adis->tx[1] = 0;
-			ret = spi_write(adis->spi, adis->tx, 2);
-			if (ret) {
-				dev_err(&adis->spi->dev, "Failed to change device page: %d\n", ret);
-				mutex_unlock(&adis->state_lock);
-				goto irq_done;
-			}
-
-			adis->current_page = 0;
-		}
-	}
-
-	ret = spi_sync(adis->spi, &adis->msg);
 	if (adis->data->has_paging)
-		mutex_unlock(&adis->state_lock);
-	if (ret) {
+		ret = adis_paging_trigger_handler(adis);
+	else
+		ret = spi_sync(adis->spi, &adis->msg);
+	if (ret)
 		dev_err(&adis->spi->dev, "Failed to read data: %d", ret);
-		goto irq_done;
-	}
+	else
+		iio_push_to_buffers_with_timestamp(indio_dev, adis->buffer,
+						   pf->timestamp);
 
-	iio_push_to_buffers_with_timestamp(indio_dev, adis->buffer,
-					   pf->timestamp);
-
-irq_done:
 	iio_trigger_notify_done(indio_dev->trig);
 
 	return IRQ_HANDLED;