[PATCH] spi: s3c64xx: Extract FIFO depth calculation to a dedicated macro

[PATCH] spi: s3c64xx: Extract FIFO depth calculation to a dedicated macro

[Sam Protsenko]

Simplify the code by extracting all cases of FIFO depth calculation into
a dedicated macro. No functional change.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
 drivers/spi/spi-s3c64xx.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index f7d623ad6ac3..7f7eb8f742e4 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -109,6 +109,7 @@
 #define TX_FIFO_LVL(v, i) (((v) >> 6) & FIFO_LVL_MASK(i))
 #define RX_FIFO_LVL(v, i) (((v) >> (i)->port_conf->rx_lvl_offset) & \
 					FIFO_LVL_MASK(i))
+#define FIFO_DEPTH(i) ((FIFO_LVL_MASK(i) >> 1) + 1)
 
 #define S3C64XX_SPI_MAX_TRAILCNT	0x3ff
 #define S3C64XX_SPI_TRAILCNT_OFF	19
@@ -406,7 +407,7 @@ static bool s3c64xx_spi_can_dma(struct spi_controller *host,
 	struct s3c64xx_spi_driver_data *sdd = spi_controller_get_devdata(host);
 
 	if (sdd->rx_dma.ch && sdd->tx_dma.ch) {
-		return xfer->len > (FIFO_LVL_MASK(sdd) >> 1) + 1;
+		return xfer->len > FIFO_DEPTH(sdd);
 	} else {
 		return false;
 	}
@@ -495,9 +496,7 @@ static u32 s3c64xx_spi_wait_for_timeout(struct s3c64xx_spi_driver_data *sdd,
 	void __iomem *regs = sdd->regs;
 	unsigned long val = 1;
 	u32 status;
-
-	/* max fifo depth available */
-	u32 max_fifo = (FIFO_LVL_MASK(sdd) >> 1) + 1;
+	u32 max_fifo = FIFO_DEPTH(sdd);
 
 	if (timeout_ms)
 		val = msecs_to_loops(timeout_ms);
@@ -604,7 +603,7 @@ static int s3c64xx_wait_for_pio(struct s3c64xx_spi_driver_data *sdd,
 	 * For any size less than the fifo size the below code is
 	 * executed atleast once.
 	 */
-	loops = xfer->len / ((FIFO_LVL_MASK(sdd) >> 1) + 1);
+	loops = xfer->len / FIFO_DEPTH(sdd);
 	buf = xfer->rx_buf;
 	do {
 		/* wait for data to be received in the fifo */
@@ -741,7 +740,7 @@ static int s3c64xx_spi_transfer_one(struct spi_controller *host,
 				    struct spi_transfer *xfer)
 {
 	struct s3c64xx_spi_driver_data *sdd = spi_controller_get_devdata(host);
-	const unsigned int fifo_len = (FIFO_LVL_MASK(sdd) >> 1) + 1;
+	const unsigned int fifo_len = FIFO_DEPTH(sdd);
 	const void *tx_buf = NULL;
 	void *rx_buf = NULL;
 	int target_len = 0, origin_len = 0;
@@ -1280,7 +1279,7 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
 	dev_dbg(&pdev->dev, "Samsung SoC SPI Driver loaded for Bus SPI-%d with %d Targets attached\n",
 					sdd->port_id, host->num_chipselect);
 	dev_dbg(&pdev->dev, "\tIOmem=[%pR]\tFIFO %dbytes\n",
-					mem_res, (FIFO_LVL_MASK(sdd) >> 1) + 1);
+					mem_res, FIFO_DEPTH(sdd));
 
 	pm_runtime_mark_last_busy(&pdev->dev);
 	pm_runtime_put_autosuspend(&pdev->dev);
-- 
2.39.2

Re: [PATCH] spi: s3c64xx: Extract FIFO depth calculation to a dedicated macro

[Andi Shyti]

Hi Sam,

>  	void __iomem *regs = sdd->regs;
>  	unsigned long val = 1;
>  	u32 status;
> -
> -	/* max fifo depth available */
> -	u32 max_fifo = (FIFO_LVL_MASK(sdd) >> 1) + 1;
> +	u32 max_fifo = FIFO_DEPTH(sdd);

Why have you removed the comment? Perhaps you could place it on
the side in order to remove that awful space.

Not a biding comment, though:

Reviewed-by: Andi Shyti <andi.shyti@kernel.org>

Andi

Re: [PATCH] spi: s3c64xx: Extract FIFO depth calculation to a dedicated macro

[Sam Protsenko]

On Sun, Jan 21, 2024 at 2:24 PM Andi Shyti <andi.shyti@kernel.org> wrote:
>
> Hi Sam,
>
> >       void __iomem *regs = sdd->regs;
> >       unsigned long val = 1;
> >       u32 status;
> > -
> > -     /* max fifo depth available */
> > -     u32 max_fifo = (FIFO_LVL_MASK(sdd) >> 1) + 1;
> > +     u32 max_fifo = FIFO_DEPTH(sdd);
>
> Why have you removed the comment? Perhaps you could place it on
> the side in order to remove that awful space.
>

The fact that `max_fifo' contains max FIFO depth is already coded in
the variable name itself. And with that new FIFO_DEPTH() macro, it
would be basically stating the same thing the third time on the same
string. Thought the removal of that comment only made the code easier
to read. If you think I should bring the comment back, please let me
know and I'll send v2.

> Not a biding comment, though:
>
> Reviewed-by: Andi Shyti <andi.shyti@kernel.org>
>
> Andi

Re: [PATCH] spi: s3c64xx: Extract FIFO depth calculation to a dedicated macro

[Andi Shyti]

Hi Sam,

On Sun, Jan 21, 2024 at 04:11:21PM -0600, Sam Protsenko wrote:
> On Sun, Jan 21, 2024 at 2:24 PM Andi Shyti <andi.shyti@kernel.org> wrote:
> >
> > Hi Sam,
> >
> > >       void __iomem *regs = sdd->regs;
> > >       unsigned long val = 1;
> > >       u32 status;
> > > -
> > > -     /* max fifo depth available */
> > > -     u32 max_fifo = (FIFO_LVL_MASK(sdd) >> 1) + 1;
> > > +     u32 max_fifo = FIFO_DEPTH(sdd);
> >
> > Why have you removed the comment? Perhaps you could place it on
> > the side in order to remove that awful space.
> >
> 
> The fact that `max_fifo' contains max FIFO depth is already coded in
> the variable name itself. And with that new FIFO_DEPTH() macro, it
> would be basically stating the same thing the third time on the same
> string. Thought the removal of that comment only made the code easier
> to read. If you think I should bring the comment back, please let me
> know and I'll send v2.

No, that's fine... you have a point here :-)

Andi

Re: [PATCH] spi: s3c64xx: Extract FIFO depth calculation to a dedicated macro

[Sam Protsenko]

On Sun, Jan 21, 2024 at 5:27 PM Andi Shyti <andi.shyti@kernel.org> wrote:
>
> Hi Sam,
>
> On Sun, Jan 21, 2024 at 04:11:21PM -0600, Sam Protsenko wrote:
> > On Sun, Jan 21, 2024 at 2:24 PM Andi Shyti <andi.shyti@kernel.org> wrote:
> > >
> > > Hi Sam,
> > >
> > > >       void __iomem *regs = sdd->regs;
> > > >       unsigned long val = 1;
> > > >       u32 status;
> > > > -
> > > > -     /* max fifo depth available */
> > > > -     u32 max_fifo = (FIFO_LVL_MASK(sdd) >> 1) + 1;
> > > > +     u32 max_fifo = FIFO_DEPTH(sdd);
> > >
> > > Why have you removed the comment? Perhaps you could place it on
> > > the side in order to remove that awful space.
> > >
> >
> > The fact that `max_fifo' contains max FIFO depth is already coded in
> > the variable name itself. And with that new FIFO_DEPTH() macro, it
> > would be basically stating the same thing the third time on the same
> > string. Thought the removal of that comment only made the code easier
> > to read. If you think I should bring the comment back, please let me
> > know and I'll send v2.
>
> No, that's fine... you have a point here :-)
>

Thanks for the review! :)

> Andi

Re: [PATCH] spi: s3c64xx: Extract FIFO depth calculation to a dedicated macro

[Mark Brown]

On Sat, 20 Jan 2024 11:00:01 -0600, Sam Protsenko wrote:
> Simplify the code by extracting all cases of FIFO depth calculation into
> a dedicated macro. No functional change.
> 
> 

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[1/1] spi: s3c64xx: Extract FIFO depth calculation to a dedicated macro
      commit: 460efee706c2b6a4daba62ec143fea29c2e7b358

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark