diff mbox series

[4.19.y-cip,5/7] spi: spi-mem: Split spi_mem_exec_op() code

Message ID 20201123120354.26413-6-prabhakar.mahadev-lad.rj@bp.renesas.com (mailing list archive)
State Changes Requested
Headers show
Series Add RPC-IF driver for RZ/G2x SoC's | expand

Commit Message

Lad Prabhakar Nov. 23, 2020, 12:03 p.m. UTC 
From: Boris Brezillon <boris.brezillon@bootlin.com>

commit f86c24f4795303e4024bc113196de32782f6ccb5 upstream.

The logic surrounding the ->exec_op() call applies to direct mapping
accessors. Move this code to separate functions to avoid duplicating
code.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/spi/spi-mem.c | 63 ++++++++++++++++++++++++++++---------------
 1 file changed, 42 insertions(+), 21 deletions(-)

Comments

Pavel Machek Nov. 23, 2020, 7:43 p.m. UTC | #1 
Hi!

> From: Boris Brezillon <boris.brezillon@bootlin.com>
> 
> commit f86c24f4795303e4024bc113196de32782f6ccb5 upstream.
> 
> The logic surrounding the ->exec_op() call applies to direct mapping
> accessors. Move this code to separate functions to avoid duplicating
> code.
> 

> +++ b/drivers/spi/spi-mem.c
> @@ -213,6 +213,44 @@ bool spi_mem_supports_op(struct spi_mem *mem, const struct spi_mem_op *op)
>  }
>  EXPORT_SYMBOL_GPL(spi_mem_supports_op);
>  
> +static int spi_mem_access_start(struct spi_mem *mem)
> +{
> +	struct spi_controller *ctlr = mem->spi->controller;
...
> +	if (ctlr->auto_runtime_pm) {
> +		int ret;
> +
> +		ret = pm_runtime_get_sync(ctlr->dev.parent);
> +		if (ret < 0) {
> +			dev_err(&ctlr->dev, "Failed to power device: %d\n",
> +				ret);
> +			return ret;
> +		}

This misses pm_runtime_put() in the error case.

> +	mutex_lock(&ctlr->bus_lock_mutex);
> +	mutex_lock(&ctlr->io_mutex);

Plus hiding locking into function like this ... is quite
"interesting".

Best regards,
								Pavel
Lad Prabhakar Nov. 23, 2020, 11:05 p.m. UTC | #2 
Hi Pavel,

Thank you for the review.

> -----Original Message-----
> From: Pavel Machek <pavel@denx.de>
> Sent: 23 November 2020 19:44
> To: Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Cc: cip-dev@lists.cip-project.org; Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp>; Pavel Machek
> <pavel@denx.de>; Biju Das <biju.das.jz@bp.renesas.com>
> Subject: Re: [PATCH 4.19.y-cip 5/7] spi: spi-mem: Split spi_mem_exec_op() code
> 
> Hi!
> 
> > From: Boris Brezillon <boris.brezillon@bootlin.com>
> >
> > commit f86c24f4795303e4024bc113196de32782f6ccb5 upstream.
> >
> > The logic surrounding the ->exec_op() call applies to direct mapping
> > accessors. Move this code to separate functions to avoid duplicating
> > code.
> >
> 
> > +++ b/drivers/spi/spi-mem.c
> > @@ -213,6 +213,44 @@ bool spi_mem_supports_op(struct spi_mem *mem, const struct spi_mem_op *op)
> >  }
> >  EXPORT_SYMBOL_GPL(spi_mem_supports_op);
> >
> > +static int spi_mem_access_start(struct spi_mem *mem)
> > +{
> > +	struct spi_controller *ctlr = mem->spi->controller;
> ...
> > +	if (ctlr->auto_runtime_pm) {
> > +		int ret;
> > +
> > +		ret = pm_runtime_get_sync(ctlr->dev.parent);
> > +		if (ret < 0) {
> > +			dev_err(&ctlr->dev, "Failed to power device: %d\n",
> > +				ret);
> > +			return ret;
> > +		}
> 
> This misses pm_runtime_put() in the error case.
> 
Agreed (pm_runtime_put_noidle()).

> > +	mutex_lock(&ctlr->bus_lock_mutex);
> > +	mutex_lock(&ctlr->io_mutex);
> 
> Plus hiding locking into function like this ... is quite
> "interesting".
>
diff mbox series

Patch

diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
index b319a9f0138c..0908f979f6a8 100644
--- a/drivers/spi/spi-mem.c
+++ b/drivers/spi/spi-mem.c
@@ -213,6 +213,44 @@  bool spi_mem_supports_op(struct spi_mem *mem, const struct spi_mem_op *op)
 }
 EXPORT_SYMBOL_GPL(spi_mem_supports_op);
 
+static int spi_mem_access_start(struct spi_mem *mem)
+{
+	struct spi_controller *ctlr = mem->spi->controller;
+
+	/*
+	 * Flush the message queue before executing our SPI memory
+	 * operation to prevent preemption of regular SPI transfers.
+	 */
+	spi_flush_queue(ctlr);
+
+	if (ctlr->auto_runtime_pm) {
+		int ret;
+
+		ret = pm_runtime_get_sync(ctlr->dev.parent);
+		if (ret < 0) {
+			dev_err(&ctlr->dev, "Failed to power device: %d\n",
+				ret);
+			return ret;
+		}
+	}
+
+	mutex_lock(&ctlr->bus_lock_mutex);
+	mutex_lock(&ctlr->io_mutex);
+
+	return 0;
+}
+
+static void spi_mem_access_end(struct spi_mem *mem)
+{
+	struct spi_controller *ctlr = mem->spi->controller;
+
+	mutex_unlock(&ctlr->io_mutex);
+	mutex_unlock(&ctlr->bus_lock_mutex);
+
+	if (ctlr->auto_runtime_pm)
+		pm_runtime_put(ctlr->dev.parent);
+}
+
 /**
  * spi_mem_exec_op() - Execute a memory operation
  * @mem: the SPI memory
@@ -242,30 +280,13 @@  int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
 		return -ENOTSUPP;
 
 	if (ctlr->mem_ops) {
-		/*
-		 * Flush the message queue before executing our SPI memory
-		 * operation to prevent preemption of regular SPI transfers.
-		 */
-		spi_flush_queue(ctlr);
-
-		if (ctlr->auto_runtime_pm) {
-			ret = pm_runtime_get_sync(ctlr->dev.parent);
-			if (ret < 0) {
-				dev_err(&ctlr->dev,
-					"Failed to power device: %d\n",
-					ret);
-				return ret;
-			}
-		}
+		ret = spi_mem_access_start(mem);
+		if (ret)
+			return ret;
 
-		mutex_lock(&ctlr->bus_lock_mutex);
-		mutex_lock(&ctlr->io_mutex);
 		ret = ctlr->mem_ops->exec_op(mem, op);
-		mutex_unlock(&ctlr->io_mutex);
-		mutex_unlock(&ctlr->bus_lock_mutex);
 
-		if (ctlr->auto_runtime_pm)
-			pm_runtime_put(ctlr->dev.parent);
+		spi_mem_access_end(mem);
 
 		/*
 		 * Some controllers only optimize specific paths (typically the