[PATCH 00/10] spi: rzv2m-csi: Code refactoring

[PATCH 00/10] spi: rzv2m-csi: Code refactoring

[Fabrizio Castro]

Dear All,

this series is to follow up on Geert and Andy feedback:
https://patchwork.kernel.org/project/linux-renesas-soc/patch/20230622113341.657842-4-fabrizio.castro.jz@renesas.com/

Thanks,
Fab

Fabrizio Castro (10):
  spi: rzv2m-csi: Add missing include
  spi: rzv2m-csi: Adopt HZ_PER_MHZ for max spi clock
  spi: rzv2m-csi: Rework CSI_CKS_MAX definition
  spi: rzv2m-csi: Leave readl_poll_timeout calls for last
  spi: rzv2m-csi: Replace unnecessary ternary operators
  spi: rzv2m-csi: Squash timing settings into one statement
  spi: rzv2m-csi: Switch to using {read,write}s{b,w}
  spi: rzv2m-csi: Improve data types and alignment
  spi: rzv2m-csi: Get rid of the x_trg{_words} tables
  spi: rzv2m-csi: Make use of device_set_node

 drivers/spi/spi-rzv2m-csi.c | 139 +++++++++++++++---------------------
 1 file changed, 58 insertions(+), 81 deletions(-)

-- 
2.34.1

[PATCH 01/10] spi: rzv2m-csi: Add missing include

[Fabrizio Castro]

Add missing include of bits.h file.

Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
---
 drivers/spi/spi-rzv2m-csi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/spi/spi-rzv2m-csi.c b/drivers/spi/spi-rzv2m-csi.c
index c0d9a776770f..b595f3b7294d 100644
--- a/drivers/spi/spi-rzv2m-csi.c
+++ b/drivers/spi/spi-rzv2m-csi.c
@@ -5,6 +5,7 @@
  * Copyright (C) 2023 Renesas Electronics Corporation
  */
 
+#include <linux/bits.h>
 #include <linux/clk.h>
 #include <linux/count_zeros.h>
 #include <linux/interrupt.h>
-- 
2.34.1

[PATCH 02/10] spi: rzv2m-csi: Adopt HZ_PER_MHZ for max spi clock

[Fabrizio Castro]

Make use of HZ_PER_MHZ for CSI_MAX_SPI_SCKO to make it clear
what its value means.

Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
---
 drivers/spi/spi-rzv2m-csi.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-rzv2m-csi.c b/drivers/spi/spi-rzv2m-csi.c
index b595f3b7294d..3931045a85eb 100644
--- a/drivers/spi/spi-rzv2m-csi.c
+++ b/drivers/spi/spi-rzv2m-csi.c
@@ -13,6 +13,7 @@
 #include <linux/platform_device.h>
 #include <linux/reset.h>
 #include <linux/spi/spi.h>
+#include <linux/units.h>
 
 /* Registers */
 #define CSI_MODE		0x00	/* CSI mode control */
@@ -71,7 +72,7 @@
 #define TX_TIMEOUT_ERROR	BIT(2)
 #define RX_TIMEOUT_ERROR	BIT(3)
 
-#define CSI_MAX_SPI_SCKO	8000000
+#define CSI_MAX_SPI_SCKO	(8 * HZ_PER_MHZ)
 
 struct rzv2m_csi_priv {
 	void __iomem *base;
-- 
2.34.1

Re: [PATCH 02/10] spi: rzv2m-csi: Adopt HZ_PER_MHZ for max spi clock

[Geert Uytterhoeven]

On Sat, Jul 15, 2023 at 3:04 AM Fabrizio Castro
<fabrizio.castro.jz@renesas.com> wrote:
> Make use of HZ_PER_MHZ for CSI_MAX_SPI_SCKO to make it clear
> what its value means.
>
> Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH 03/10] spi: rzv2m-csi: Rework CSI_CKS_MAX definition

[Fabrizio Castro]

Clock "csiclk" gets divided by 2 * CSI_CLKSEL_CKS in order to generate
the serial clock (output from master), with CSI_CLKSEL_CKS ranging from
0x1 (that means "csiclk" is divided by 2) to 0x3FFF ("csiclk" is divided
by 32766). CSI_CKS_MAX is used for referring to the setting
corresponding to the maximum frequency divider.
Value 0x3FFF for CSI_CKS_MAX doesn't really means much to the reader
without an explanation and a more readable definition.

Add a comment with a meaningful description and also replace value
0x3FFF with the corresponding GENMASK, to make it very clear what the
macro means.

Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
---
 drivers/spi/spi-rzv2m-csi.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-rzv2m-csi.c b/drivers/spi/spi-rzv2m-csi.c
index 3931045a85eb..621774949bde 100644
--- a/drivers/spi/spi-rzv2m-csi.c
+++ b/drivers/spi/spi-rzv2m-csi.c
@@ -65,7 +65,12 @@
 #define CSI_FIFO_SIZE_BYTES	32
 #define CSI_FIFO_HALF_SIZE	16
 #define CSI_EN_DIS_TIMEOUT_US	100
-#define CSI_CKS_MAX		0x3FFF
+/*
+ * Clock "csiclk" gets divided by 2 * CSI_CLKSEL_CKS in order to generate the
+ * serial clock (output from master), with CSI_CLKSEL_CKS ranging from 0x1 (that
+ * means "csiclk" is divided by 2) to 0x3FFF ("csiclk" is divided by 32766).
+ */
+#define CSI_CKS_MAX		GENMASK(13, 0)
 
 #define UNDERRUN_ERROR		BIT(0)
 #define OVERFLOW_ERROR		BIT(1)
-- 
2.34.1

Re: [PATCH 03/10] spi: rzv2m-csi: Rework CSI_CKS_MAX definition

[Geert Uytterhoeven]

On Sat, Jul 15, 2023 at 3:04 AM Fabrizio Castro
<fabrizio.castro.jz@renesas.com> wrote:
> Clock "csiclk" gets divided by 2 * CSI_CLKSEL_CKS in order to generate
> the serial clock (output from master), with CSI_CLKSEL_CKS ranging from
> 0x1 (that means "csiclk" is divided by 2) to 0x3FFF ("csiclk" is divided
> by 32766). CSI_CKS_MAX is used for referring to the setting
> corresponding to the maximum frequency divider.
> Value 0x3FFF for CSI_CKS_MAX doesn't really means much to the reader
> without an explanation and a more readable definition.
>
> Add a comment with a meaningful description and also replace value
> 0x3FFF with the corresponding GENMASK, to make it very clear what the
> macro means.
>
> Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH 04/10] spi: rzv2m-csi: Leave readl_poll_timeout calls for last

[Fabrizio Castro]

Both rzv2m_csi_sw_reset and rzv2m_csi_start_stop_operation
call into readl_poll_timeout upon a certain condition, and
return 0 otherwise.
Flip the logic to improve readability.

Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
---
 drivers/spi/spi-rzv2m-csi.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/spi/spi-rzv2m-csi.c b/drivers/spi/spi-rzv2m-csi.c
index 621774949bde..1c65cd5f2039 100644
--- a/drivers/spi/spi-rzv2m-csi.c
+++ b/drivers/spi/spi-rzv2m-csi.c
@@ -131,13 +131,12 @@ static int rzv2m_csi_sw_reset(struct rzv2m_csi_priv *csi, int assert)
 
 	rzv2m_csi_reg_write_bit(csi, CSI_CNT, CSI_CNT_CSIRST, assert);
 
-	if (assert) {
-		return readl_poll_timeout(csi->base + CSI_MODE, reg,
-					  !(reg & CSI_MODE_CSOT), 0,
-					  CSI_EN_DIS_TIMEOUT_US);
-	}
+	if (!assert)
+		return 0;
 
-	return 0;
+	return readl_poll_timeout(csi->base + CSI_MODE, reg,
+				  !(reg & CSI_MODE_CSOT), 0,
+				  CSI_EN_DIS_TIMEOUT_US);
 }
 
 static int rzv2m_csi_start_stop_operation(const struct rzv2m_csi_priv *csi,
@@ -147,12 +146,12 @@ static int rzv2m_csi_start_stop_operation(const struct rzv2m_csi_priv *csi,
 
 	rzv2m_csi_reg_write_bit(csi, CSI_MODE, CSI_MODE_CSIE, enable);
 
-	if (!enable && wait)
-		return readl_poll_timeout(csi->base + CSI_MODE, reg,
-					  !(reg & CSI_MODE_CSOT), 0,
-					  CSI_EN_DIS_TIMEOUT_US);
+	if (enable || !wait)
+		return 0;
 
-	return 0;
+	return readl_poll_timeout(csi->base + CSI_MODE, reg,
+				  !(reg & CSI_MODE_CSOT), 0,
+				  CSI_EN_DIS_TIMEOUT_US);
 }
 
 static int rzv2m_csi_fill_txfifo(struct rzv2m_csi_priv *csi)
-- 
2.34.1

Re: [PATCH 04/10] spi: rzv2m-csi: Leave readl_poll_timeout calls for last

[Geert Uytterhoeven]

On Sat, Jul 15, 2023 at 3:04 AM Fabrizio Castro
<fabrizio.castro.jz@renesas.com> wrote:
> Both rzv2m_csi_sw_reset and rzv2m_csi_start_stop_operation
> call into readl_poll_timeout upon a certain condition, and
> return 0 otherwise.
> Flip the logic to improve readability.
>
> Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH 05/10] spi: rzv2m-csi: Replace unnecessary ternary operators

[Fabrizio Castro]

The ternary operators used to initialize tx_completed and rx_completed
are not necessary, replace them with a better implementation.

Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
---
 drivers/spi/spi-rzv2m-csi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-rzv2m-csi.c b/drivers/spi/spi-rzv2m-csi.c
index 1c65cd5f2039..038f1486b7d7 100644
--- a/drivers/spi/spi-rzv2m-csi.c
+++ b/drivers/spi/spi-rzv2m-csi.c
@@ -439,8 +439,8 @@ static int rzv2m_csi_setup(struct spi_device *spi)
 
 static int rzv2m_csi_pio_transfer(struct rzv2m_csi_priv *csi)
 {
-	bool tx_completed = csi->txbuf ? false : true;
-	bool rx_completed = csi->rxbuf ? false : true;
+	bool tx_completed = !csi->txbuf;
+	bool rx_completed = !csi->rxbuf;
 	int ret = 0;
 
 	/* Make sure the TX FIFO is empty */
-- 
2.34.1

Re: [PATCH 05/10] spi: rzv2m-csi: Replace unnecessary ternary operators

[Geert Uytterhoeven]

On Sat, Jul 15, 2023 at 3:04 AM Fabrizio Castro
<fabrizio.castro.jz@renesas.com> wrote:
> The ternary operators used to initialize tx_completed and rx_completed
> are not necessary, replace them with a better implementation.
>
> Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH 06/10] spi: rzv2m-csi: Squash timing settings into one statement

[Fabrizio Castro]

Register CLKSEL hosts the configuration for both clock polarity
and data phase, and both values can be set in one write operation.

Squash the clock polarity and data phase register writes into
one statement, for efficiency.

Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
---
 drivers/spi/spi-rzv2m-csi.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi-rzv2m-csi.c b/drivers/spi/spi-rzv2m-csi.c
index 038f1486b7d7..faf5898bc3e0 100644
--- a/drivers/spi/spi-rzv2m-csi.c
+++ b/drivers/spi/spi-rzv2m-csi.c
@@ -38,6 +38,7 @@
 /* CSI_CLKSEL */
 #define CSI_CLKSEL_CKP		BIT(17)
 #define CSI_CLKSEL_DAP		BIT(16)
+#define CSI_CLKSEL_MODE		(CSI_CLKSEL_CKP|CSI_CLKSEL_DAP)
 #define CSI_CLKSEL_SLAVE	BIT(15)
 #define CSI_CLKSEL_CKS		GENMASK(14, 1)
 
@@ -408,10 +409,8 @@ static int rzv2m_csi_setup(struct spi_device *spi)
 	writel(CSI_MODE_SETUP, csi->base + CSI_MODE);
 
 	/* Setup clock polarity and phase timing */
-	rzv2m_csi_reg_write_bit(csi, CSI_CLKSEL, CSI_CLKSEL_CKP,
-				!(spi->mode & SPI_CPOL));
-	rzv2m_csi_reg_write_bit(csi, CSI_CLKSEL, CSI_CLKSEL_DAP,
-				!(spi->mode & SPI_CPHA));
+	rzv2m_csi_reg_write_bit(csi, CSI_CLKSEL, CSI_CLKSEL_MODE,
+				~spi->mode & SPI_MODE_X_MASK);
 
 	/* Setup serial data order */
 	rzv2m_csi_reg_write_bit(csi, CSI_MODE, CSI_MODE_DIR,
-- 
2.34.1

Re: [PATCH 06/10] spi: rzv2m-csi: Squash timing settings into one statement

[Andy Shevchenko]

On Sat, Jul 15, 2023 at 4:04 AM Fabrizio Castro
<fabrizio.castro.jz@renesas.com> wrote:
>
> Register CLKSEL hosts the configuration for both clock polarity
> and data phase, and both values can be set in one write operation.
>
> Squash the clock polarity and data phase register writes into
> one statement, for efficiency.

...

>         /* Setup clock polarity and phase timing */
> -       rzv2m_csi_reg_write_bit(csi, CSI_CLKSEL, CSI_CLKSEL_CKP,
> -                               !(spi->mode & SPI_CPOL));
> -       rzv2m_csi_reg_write_bit(csi, CSI_CLKSEL, CSI_CLKSEL_DAP,
> -                               !(spi->mode & SPI_CPHA));
> +       rzv2m_csi_reg_write_bit(csi, CSI_CLKSEL, CSI_CLKSEL_MODE,
> +                               ~spi->mode & SPI_MODE_X_MASK);

I think this now regresses due to the absence of parentheses.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH 06/10] spi: rzv2m-csi: Squash timing settings into one statement

[Fabrizio Castro]

Hi Andy,

Thanks for your reply.

> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Subject: Re: [PATCH 06/10] spi: rzv2m-csi: Squash timing settings into
> one statement
> 
> On Sat, Jul 15, 2023 at 4:04 AM Fabrizio Castro
> <fabrizio.castro.jz@renesas.com> wrote:
> >
> > Register CLKSEL hosts the configuration for both clock polarity
> > and data phase, and both values can be set in one write operation.
> >
> > Squash the clock polarity and data phase register writes into
> > one statement, for efficiency.
> 
> ...
> 
> >         /* Setup clock polarity and phase timing */
> > -       rzv2m_csi_reg_write_bit(csi, CSI_CLKSEL, CSI_CLKSEL_CKP,
> > -                               !(spi->mode & SPI_CPOL));
> > -       rzv2m_csi_reg_write_bit(csi, CSI_CLKSEL, CSI_CLKSEL_DAP,
> > -                               !(spi->mode & SPI_CPHA));
> > +       rzv2m_csi_reg_write_bit(csi, CSI_CLKSEL, CSI_CLKSEL_MODE,
> > +                               ~spi->mode & SPI_MODE_X_MASK);
> 
> I think this now regresses due to the absence of parentheses.

This looks okay to me. CSI_CLKSEL_CKP needs to contain the inverted value
of SPI_CPOL, and CSI_CLKSEL_DAP needs to contain the inverted value of
SPI_CPHA, and that happens with both use cases.

Thanks,
Fab

> 
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH 06/10] spi: rzv2m-csi: Squash timing settings into one statement

[Andy Shevchenko]

On Mon, Jul 17, 2023 at 1:44 PM Fabrizio Castro
<fabrizio.castro.jz@renesas.com> wrote:
> > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > On Sat, Jul 15, 2023 at 4:04 AM Fabrizio Castro
> > <fabrizio.castro.jz@renesas.com> wrote:

...

> > >         /* Setup clock polarity and phase timing */
> > > -       rzv2m_csi_reg_write_bit(csi, CSI_CLKSEL, CSI_CLKSEL_CKP,
> > > -                               !(spi->mode & SPI_CPOL));
> > > -       rzv2m_csi_reg_write_bit(csi, CSI_CLKSEL, CSI_CLKSEL_DAP,
> > > -                               !(spi->mode & SPI_CPHA));
> > > +       rzv2m_csi_reg_write_bit(csi, CSI_CLKSEL, CSI_CLKSEL_MODE,
> > > +                               ~spi->mode & SPI_MODE_X_MASK);
> >
> > I think this now regresses due to the absence of parentheses.
>
> This looks okay to me. CSI_CLKSEL_CKP needs to contain the inverted value
> of SPI_CPOL, and CSI_CLKSEL_DAP needs to contain the inverted value of
> SPI_CPHA, and that happens with both use cases.

Ah, this is interchangeable since we will get the same bits in the end, indeed.

-- 
With Best Regards,
Andy Shevchenko

[PATCH 07/10] spi: rzv2m-csi: Switch to using {read,write}s{b,w}

[Fabrizio Castro]

The RX/TX FIFOs implemented by the CSI IP are accessed by
repeatedly reading/writing the same memory address, and
therefore they are the ideal candidate for {read,write}s{b,w}.
The RZ/V2M CSI driver currently implements loops to fill up
the TX FIFO and empty the RX FIFO, differentiating between
8-bit and 16-bit word size.
Switch to using {read,write}s{b,w} to get rid of the bespoke
loops.

Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
---
 drivers/spi/spi-rzv2m-csi.c | 42 +++++++++++++------------------------
 1 file changed, 14 insertions(+), 28 deletions(-)

diff --git a/drivers/spi/spi-rzv2m-csi.c b/drivers/spi/spi-rzv2m-csi.c
index faf5898bc3e0..d0d6b183ffaf 100644
--- a/drivers/spi/spi-rzv2m-csi.c
+++ b/drivers/spi/spi-rzv2m-csi.c
@@ -86,8 +86,8 @@ struct rzv2m_csi_priv {
 	struct clk *pclk;
 	struct device *dev;
 	struct spi_controller *controller;
-	const u8 *txbuf;
-	u8 *rxbuf;
+	const void *txbuf;
+	void *rxbuf;
 	int buffer_len;
 	int bytes_sent;
 	int bytes_received;
@@ -157,22 +157,15 @@ static int rzv2m_csi_start_stop_operation(const struct rzv2m_csi_priv *csi,
 
 static int rzv2m_csi_fill_txfifo(struct rzv2m_csi_priv *csi)
 {
-	int i;
-
 	if (readl(csi->base + CSI_OFIFOL))
 		return -EIO;
 
-	if (csi->bytes_per_word == 2) {
-		u16 *buf = (u16 *)csi->txbuf;
-
-		for (i = 0; i < csi->words_to_transfer; i++)
-			writel(buf[i], csi->base + CSI_OFIFO);
-	} else {
-		u8 *buf = (u8 *)csi->txbuf;
-
-		for (i = 0; i < csi->words_to_transfer; i++)
-			writel(buf[i], csi->base + CSI_OFIFO);
-	}
+	if (csi->bytes_per_word == 2)
+		writesw(csi->base + CSI_OFIFO, csi->txbuf,
+			csi->words_to_transfer);
+	else
+		writesb(csi->base + CSI_OFIFO, csi->txbuf,
+			csi->words_to_transfer);
 
 	csi->txbuf += csi->bytes_to_transfer;
 	csi->bytes_sent += csi->bytes_to_transfer;
@@ -182,22 +175,15 @@ static int rzv2m_csi_fill_txfifo(struct rzv2m_csi_priv *csi)
 
 static int rzv2m_csi_read_rxfifo(struct rzv2m_csi_priv *csi)
 {
-	int i;
-
 	if (readl(csi->base + CSI_IFIFOL) != csi->bytes_to_transfer)
 		return -EIO;
 
-	if (csi->bytes_per_word == 2) {
-		u16 *buf = (u16 *)csi->rxbuf;
-
-		for (i = 0; i < csi->words_to_transfer; i++)
-			buf[i] = (u16)readl(csi->base + CSI_IFIFO);
-	} else {
-		u8 *buf = (u8 *)csi->rxbuf;
-
-		for (i = 0; i < csi->words_to_transfer; i++)
-			buf[i] = (u8)readl(csi->base + CSI_IFIFO);
-	}
+	if (csi->bytes_per_word == 2)
+		readsw(csi->base + CSI_IFIFO, csi->rxbuf,
+		       csi->words_to_transfer);
+	else
+		readsb(csi->base + CSI_IFIFO, csi->rxbuf,
+		       csi->words_to_transfer);
 
 	csi->rxbuf += csi->bytes_to_transfer;
 	csi->bytes_received += csi->bytes_to_transfer;
-- 
2.34.1

Re: [PATCH 07/10] spi: rzv2m-csi: Switch to using {read,write}s{b,w}

[Geert Uytterhoeven]

Hi Fabrizio,

On Sat, Jul 15, 2023 at 3:04 AM Fabrizio Castro
<fabrizio.castro.jz@renesas.com> wrote:
> The RX/TX FIFOs implemented by the CSI IP are accessed by
> repeatedly reading/writing the same memory address, and
> therefore they are the ideal candidate for {read,write}s{b,w}.
> The RZ/V2M CSI driver currently implements loops to fill up
> the TX FIFO and empty the RX FIFO, differentiating between
> 8-bit and 16-bit word size.
> Switch to using {read,write}s{b,w} to get rid of the bespoke
> loops.
>
> Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>

Thanks for your patch!

> --- a/drivers/spi/spi-rzv2m-csi.c
> +++ b/drivers/spi/spi-rzv2m-csi.c

> @@ -157,22 +157,15 @@ static int rzv2m_csi_start_stop_operation(const struct rzv2m_csi_priv *csi,
>
>  static int rzv2m_csi_fill_txfifo(struct rzv2m_csi_priv *csi)
>  {
> -       int i;
> -
>         if (readl(csi->base + CSI_OFIFOL))
>                 return -EIO;
>
> -       if (csi->bytes_per_word == 2) {
> -               u16 *buf = (u16 *)csi->txbuf;
> -
> -               for (i = 0; i < csi->words_to_transfer; i++)
> -                       writel(buf[i], csi->base + CSI_OFIFO);
> -       } else {
> -               u8 *buf = (u8 *)csi->txbuf;
> -
> -               for (i = 0; i < csi->words_to_transfer; i++)
> -                       writel(buf[i], csi->base + CSI_OFIFO);
> -       }
> +       if (csi->bytes_per_word == 2)
> +               writesw(csi->base + CSI_OFIFO, csi->txbuf,
> +                       csi->words_to_transfer);
> +       else
> +               writesb(csi->base + CSI_OFIFO, csi->txbuf,
> +                       csi->words_to_transfer);

According to the hardware documentation[1], the access size for both the
CSI_OFIFO and CSI_IFIFO registers is 32 bits, so you must use writel()
resp. readl().  So please check with the hardware people first.

[1] RZ/V2M User's Manual Hardware, Rev. 1.30.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH 07/10] spi: rzv2m-csi: Switch to using {read,write}s{b,w}

[Fabrizio Castro]

Hi Geert,

Thanks for your reply!

> From: Geert Uytterhoeven <geert@linux-m68k.org>
> Subject: Re: [PATCH 07/10] spi: rzv2m-csi: Switch to using
> {read,write}s{b,w}
> 
> Hi Fabrizio,
> 
> On Sat, Jul 15, 2023 at 3:04 AM Fabrizio Castro
> <fabrizio.castro.jz@renesas.com> wrote:
> > The RX/TX FIFOs implemented by the CSI IP are accessed by
> > repeatedly reading/writing the same memory address, and
> > therefore they are the ideal candidate for {read,write}s{b,w}.
> > The RZ/V2M CSI driver currently implements loops to fill up
> > the TX FIFO and empty the RX FIFO, differentiating between
> > 8-bit and 16-bit word size.
> > Switch to using {read,write}s{b,w} to get rid of the bespoke
> > loops.
> >
> > Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> 
> Thanks for your patch!
> 
> > --- a/drivers/spi/spi-rzv2m-csi.c
> > +++ b/drivers/spi/spi-rzv2m-csi.c
> 
> > @@ -157,22 +157,15 @@ static int
> rzv2m_csi_start_stop_operation(const struct rzv2m_csi_priv *csi,
> >
> >  static int rzv2m_csi_fill_txfifo(struct rzv2m_csi_priv *csi)
> >  {
> > -       int i;
> > -
> >         if (readl(csi->base + CSI_OFIFOL))
> >                 return -EIO;
> >
> > -       if (csi->bytes_per_word == 2) {
> > -               u16 *buf = (u16 *)csi->txbuf;
> > -
> > -               for (i = 0; i < csi->words_to_transfer; i++)
> > -                       writel(buf[i], csi->base + CSI_OFIFO);
> > -       } else {
> > -               u8 *buf = (u8 *)csi->txbuf;
> > -
> > -               for (i = 0; i < csi->words_to_transfer; i++)
> > -                       writel(buf[i], csi->base + CSI_OFIFO);
> > -       }
> > +       if (csi->bytes_per_word == 2)
> > +               writesw(csi->base + CSI_OFIFO, csi->txbuf,
> > +                       csi->words_to_transfer);
> > +       else
> > +               writesb(csi->base + CSI_OFIFO, csi->txbuf,
> > +                       csi->words_to_transfer);
> 
> According to the hardware documentation[1], the access size for both
> the
> CSI_OFIFO and CSI_IFIFO registers is 32 bits, so you must use writel()
> resp. readl().  So please check with the hardware people first.
> 
> [1] RZ/V2M User's Manual Hardware, Rev. 1.30.

You are right, access is 32 bits (and although this patch works fine,
we should avoid accessing those regs any other way). Now I remember
why I decided to go for the bespoke loops in the first place, writesl
and readsl provide the right register access, but the wrong pointer
arithmetic for this use case.
For this patch I ended up selecting writesw/writesb/readsw/readsb to
get the right pointer arithmetic, but the register access is not as
per manual.

I can either fallback to using the bespoke loops (I can still
remove the unnecessary u8* and u16* casting ;-) ), or I can add
new APIs for this sort of access to io.h (e.g. writesbl, writeswl,
readsbl, readswl, in order to get the pointer arithmetic right for
the type of array handled, while keeping memory access as expected).

What are your thoughts on that?

Thanks,
Fab

> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 --
> geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a
> hacker. But
> when I'm talking to journalists I just say "programmer" or something
> like that.
>                                 -- Linus Torvalds

Re: [PATCH 07/10] spi: rzv2m-csi: Switch to using {read,write}s{b,w}

[Andy Shevchenko]

On Mon, Jul 17, 2023 at 1:36 PM Fabrizio Castro
<fabrizio.castro.jz@renesas.com> wrote:
> > From: Geert Uytterhoeven <geert@linux-m68k.org>
> > Subject: Re: [PATCH 07/10] spi: rzv2m-csi: Switch to using
> > {read,write}s{b,w}
> > On Sat, Jul 15, 2023 at 3:04 AM Fabrizio Castro
> > <fabrizio.castro.jz@renesas.com> wrote:

...

> > According to the hardware documentation[1], the access size for both
> > the
> > CSI_OFIFO and CSI_IFIFO registers is 32 bits, so you must use writel()
> > resp. readl().  So please check with the hardware people first.
> >
> > [1] RZ/V2M User's Manual Hardware, Rev. 1.30.
>
> You are right, access is 32 bits (and although this patch works fine,
> we should avoid accessing those regs any other way). Now I remember
> why I decided to go for the bespoke loops in the first place, writesl
> and readsl provide the right register access, but the wrong pointer
> arithmetic for this use case.
> For this patch I ended up selecting writesw/writesb/readsw/readsb to
> get the right pointer arithmetic, but the register access is not as
> per manual.
>
> I can either fallback to using the bespoke loops (I can still
> remove the unnecessary u8* and u16* casting ;-) ), or I can add
> new APIs for this sort of access to io.h (e.g. writesbl, writeswl,
> readsbl, readswl, in order to get the pointer arithmetic right for
> the type of array handled, while keeping memory access as expected).
>
> What are your thoughts on that?

I think that you need to use readsl() / writesl() on the custom buffer
with something like

*_sparse() / *_condence() APIs added (perhaps locally to this driver)
as they may be reused by others in the future.
Having all flavours of read*()/write*() does not scale in my opinion.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH 07/10] spi: rzv2m-csi: Switch to using {read,write}s{b,w}

[Fabrizio Castro]

Hi Andy,

Thanks for your reply.

> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Subject: Re: [PATCH 07/10] spi: rzv2m-csi: Switch to using
> {read,write}s{b,w}
> 
> On Mon, Jul 17, 2023 at 1:36 PM Fabrizio Castro
> <fabrizio.castro.jz@renesas.com> wrote:
> > > From: Geert Uytterhoeven <geert@linux-m68k.org>
> > > Subject: Re: [PATCH 07/10] spi: rzv2m-csi: Switch to using
> > > {read,write}s{b,w}
> > > On Sat, Jul 15, 2023 at 3:04 AM Fabrizio Castro
> > > <fabrizio.castro.jz@renesas.com> wrote:
> 
> ...
> 
> > > According to the hardware documentation[1], the access size for
> both
> > > the
> > > CSI_OFIFO and CSI_IFIFO registers is 32 bits, so you must use
> writel()
> > > resp. readl().  So please check with the hardware people first.
> > >
> > > [1] RZ/V2M User's Manual Hardware, Rev. 1.30.
> >
> > You are right, access is 32 bits (and although this patch works
> fine,
> > we should avoid accessing those regs any other way). Now I remember
> > why I decided to go for the bespoke loops in the first place,
> writesl
> > and readsl provide the right register access, but the wrong pointer
> > arithmetic for this use case.
> > For this patch I ended up selecting writesw/writesb/readsw/readsb to
> > get the right pointer arithmetic, but the register access is not as
> > per manual.
> >
> > I can either fallback to using the bespoke loops (I can still
> > remove the unnecessary u8* and u16* casting ;-) ), or I can add
> > new APIs for this sort of access to io.h (e.g. writesbl, writeswl,
> > readsbl, readswl, in order to get the pointer arithmetic right for
> > the type of array handled, while keeping memory access as expected).
> >
> > What are your thoughts on that?
> 
> I think that you need to use readsl() / writesl() on the custom buffer
> with something like
> 
> *_sparse() / *_condence() APIs added (perhaps locally to this driver)
> as they may be reused by others in the future.
> Having all flavours of read*()/write*() does not scale in my opinion.

Maybe a "generic" macro then (one for reading and one for writing)?
So that we can pass the data type (to get the pointer arithmetic
right) and the function name to use for the read/write operations
(to get the register operations right)?
Maybe that would scale and cater for most needs (including the one
from this patch)?

Cheers,
Fab

> 
> --
> With Best Regards,
> Andy Shevchenko

RE: [PATCH 07/10] spi: rzv2m-csi: Switch to using {read,write}s{b,w}

[Fabrizio Castro]

> From: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> Subject: RE: [PATCH 07/10] spi: rzv2m-csi: Switch to using
> {read,write}s{b,w}
> 
> Hi Andy,
> 
> Thanks for your reply.
> 
> > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > Subject: Re: [PATCH 07/10] spi: rzv2m-csi: Switch to using
> > {read,write}s{b,w}
> >
> > On Mon, Jul 17, 2023 at 1:36 PM Fabrizio Castro
> > <fabrizio.castro.jz@renesas.com> wrote:
> > > > From: Geert Uytterhoeven <geert@linux-m68k.org>
> > > > Subject: Re: [PATCH 07/10] spi: rzv2m-csi: Switch to using
> > > > {read,write}s{b,w}
> > > > On Sat, Jul 15, 2023 at 3:04 AM Fabrizio Castro
> > > > <fabrizio.castro.jz@renesas.com> wrote:
> >
> > ...
> >
> > > > According to the hardware documentation[1], the access size for
> > both
> > > > the
> > > > CSI_OFIFO and CSI_IFIFO registers is 32 bits, so you must use
> > writel()
> > > > resp. readl().  So please check with the hardware people first.
> > > >
> > > > [1] RZ/V2M User's Manual Hardware, Rev. 1.30.
> > >
> > > You are right, access is 32 bits (and although this patch works
> > fine,
> > > we should avoid accessing those regs any other way). Now I
> remember
> > > why I decided to go for the bespoke loops in the first place,
> > writesl
> > > and readsl provide the right register access, but the wrong
> pointer
> > > arithmetic for this use case.
> > > For this patch I ended up selecting writesw/writesb/readsw/readsb
> to
> > > get the right pointer arithmetic, but the register access is not
> as
> > > per manual.
> > >
> > > I can either fallback to using the bespoke loops (I can still
> > > remove the unnecessary u8* and u16* casting ;-) ), or I can add
> > > new APIs for this sort of access to io.h (e.g. writesbl, writeswl,
> > > readsbl, readswl, in order to get the pointer arithmetic right for
> > > the type of array handled, while keeping memory access as
> expected).
> > >
> > > What are your thoughts on that?
> >
> > I think that you need to use readsl() / writesl() on the custom
> buffer
> > with something like
> >
> > *_sparse() / *_condence() APIs added (perhaps locally to this
> driver)
> > as they may be reused by others in the future.
> > Having all flavours of read*()/write*() does not scale in my
> opinion.
> 
> Maybe a "generic" macro then (one for reading and one for writing)?
> So that we can pass the data type (to get the pointer arithmetic
> right) and the function name to use for the read/write operations
> (to get the register operations right)?
> Maybe that would scale and cater for most needs (including the one
> from this patch)?

Something like the below perhaps:

#ifndef readsx
#define readsx(atype, readc, addr, buffer, count)	\
	({								\
		if (count) {					\
			unsigned int cnt = count;		\
			atype *buf = buffer;			\
									\
			do {						\
				atype x = readc(addr);		\
				*buf++ = x;				\
			} while (--cnt);				\
		}							\
	})
#endif

#ifndef writesx
#define writesx(atype, writec, addr, buffer, count)	\
	({								\
		if (count) {					\
			unsigned int cnt = count;		\
			const atype *buf = buffer;		\
									\
			do {						\
				writec(*buf++, addr);		\
			} while (--cnt);				\
		}							\

	})

#endif

Cheers,
Fab

> 
> Cheers,
> Fab
> 
> >
> > --
> > With Best Regards,
> > Andy Shevchenko

[PATCH 08/10] spi: rzv2m-csi: Improve data types and alignment

[Fabrizio Castro]

"unsigned int" is more appropriate than "int" for the members
of "struct rzv2m_csi_priv".
Also, members "bytes_per_word" and "errors" introduce gaps
in the structure.
Adjust "struct rzv2m_csi_priv" and its members usage accordingly.
While at it, remove the unnecessary casting of "data" to
"struct rzv2m_csi_priv*" in function "rzv2m_csi_irq_handler".

Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
---
 drivers/spi/spi-rzv2m-csi.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/spi/spi-rzv2m-csi.c b/drivers/spi/spi-rzv2m-csi.c
index d0d6b183ffaf..1e5ed1089f42 100644
--- a/drivers/spi/spi-rzv2m-csi.c
+++ b/drivers/spi/spi-rzv2m-csi.c
@@ -63,8 +63,8 @@
 /* CSI_FIFOTRG */
 #define CSI_FIFOTRG_R_TRG       GENMASK(2, 0)
 
-#define CSI_FIFO_SIZE_BYTES	32
-#define CSI_FIFO_HALF_SIZE	16
+#define CSI_FIFO_SIZE_BYTES	32U
+#define CSI_FIFO_HALF_SIZE	16U
 #define CSI_EN_DIS_TIMEOUT_US	100
 /*
  * Clock "csiclk" gets divided by 2 * CSI_CLKSEL_CKS in order to generate the
@@ -88,14 +88,14 @@ struct rzv2m_csi_priv {
 	struct spi_controller *controller;
 	const void *txbuf;
 	void *rxbuf;
-	int buffer_len;
-	int bytes_sent;
-	int bytes_received;
-	int bytes_to_transfer;
-	int words_to_transfer;
-	unsigned char bytes_per_word;
+	unsigned int buffer_len;
+	unsigned int bytes_sent;
+	unsigned int bytes_received;
+	unsigned int bytes_to_transfer;
+	unsigned int words_to_transfer;
+	unsigned int bytes_per_word;
 	wait_queue_head_t wait;
-	u8 errors;
+	u32 errors;
 	u32 status;
 };
 
@@ -193,9 +193,9 @@ static int rzv2m_csi_read_rxfifo(struct rzv2m_csi_priv *csi)
 
 static inline void rzv2m_csi_calc_current_transfer(struct rzv2m_csi_priv *csi)
 {
-	int bytes_transferred = max_t(int, csi->bytes_received, csi->bytes_sent);
-	int bytes_remaining = csi->buffer_len - bytes_transferred;
-	int to_transfer;
+	unsigned int bytes_transferred = max(csi->bytes_received, csi->bytes_sent);
+	unsigned int bytes_remaining = csi->buffer_len - bytes_transferred;
+	unsigned int to_transfer;
 
 	if (csi->txbuf)
 		/*
@@ -203,9 +203,9 @@ static inline void rzv2m_csi_calc_current_transfer(struct rzv2m_csi_priv *csi)
 		 * hard to raise an overflow error (which is only possible
 		 * when IP transmits and receives at the same time).
 		 */
-		to_transfer = min_t(int, CSI_FIFO_HALF_SIZE, bytes_remaining);
+		to_transfer = min(CSI_FIFO_HALF_SIZE, bytes_remaining);
 	else
-		to_transfer = min_t(int, CSI_FIFO_SIZE_BYTES, bytes_remaining);
+		to_transfer = min(CSI_FIFO_SIZE_BYTES, bytes_remaining);
 
 	if (csi->bytes_per_word == 2)
 		to_transfer >>= 1;
@@ -325,7 +325,7 @@ static inline int rzv2m_csi_wait_for_rx_ready(struct rzv2m_csi_priv *csi)
 
 static irqreturn_t rzv2m_csi_irq_handler(int irq, void *data)
 {
-	struct rzv2m_csi_priv *csi = (struct rzv2m_csi_priv *)data;
+	struct rzv2m_csi_priv *csi = data;
 
 	csi->status = readl(csi->base + CSI_INT);
 	rzv2m_csi_disable_irqs(csi, csi->status);
-- 
2.34.1

Re: [PATCH 08/10] spi: rzv2m-csi: Improve data types and alignment

[Geert Uytterhoeven]

Hi Fabrizio,

Thanks for your patch!

On Sat, Jul 15, 2023 at 3:04 AM Fabrizio Castro
<fabrizio.castro.jz@renesas.com> wrote:
> "unsigned int" is more appropriate than "int" for the members
> of "struct rzv2m_csi_priv".

Agreed.

> Also, members "bytes_per_word" and "errors" introduce gaps
> in the structure.

While enlarging the types does get rid of the gaps, that was not the
intent of my comment ;-)
You can reorder fields to avoid gaps, and reduce the size of the structure.

> Adjust "struct rzv2m_csi_priv" and its members usage accordingly.
> While at it, remove the unnecessary casting of "data" to
> "struct rzv2m_csi_priv*" in function "rzv2m_csi_irq_handler".
>
> Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>

> --- a/drivers/spi/spi-rzv2m-csi.c
> +++ b/drivers/spi/spi-rzv2m-csi.c

> @@ -88,14 +88,14 @@ struct rzv2m_csi_priv {
>         struct spi_controller *controller;
>         const void *txbuf;
>         void *rxbuf;
> -       int buffer_len;
> -       int bytes_sent;
> -       int bytes_received;
> -       int bytes_to_transfer;
> -       int words_to_transfer;
> -       unsigned char bytes_per_word;
> +       unsigned int buffer_len;
> +       unsigned int bytes_sent;
> +       unsigned int bytes_received;
> +       unsigned int bytes_to_transfer;
> +       unsigned int words_to_transfer;
> +       unsigned int bytes_per_word;

bytes_per_word is calculated from spi_transfer.bits_per_word,
so u8 was fine.

>         wait_queue_head_t wait;
> -       u8 errors;
> +       u32 errors;

u8 was sufficiently large to hold all possible values.

>         u32 status;
>  };
>

Anyway, the code should work fine, so
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH 09/10] spi: rzv2m-csi: Get rid of the x_trg{_words} tables

[Fabrizio Castro]

Table x_trg can be replaced with ilog2(), and table x_trg_words
can be replaced with rounddown_pow_of_two().
Replace the tables usage with the corresponding macros.
While at it, remove a couple of unnecessary empty lines.

Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
---
 drivers/spi/spi-rzv2m-csi.c | 21 +++------------------
 1 file changed, 3 insertions(+), 18 deletions(-)

diff --git a/drivers/spi/spi-rzv2m-csi.c b/drivers/spi/spi-rzv2m-csi.c
index 1e5ed1089f42..1874ca1c2747 100644
--- a/drivers/spi/spi-rzv2m-csi.c
+++ b/drivers/spi/spi-rzv2m-csi.c
@@ -10,6 +10,7 @@
 #include <linux/count_zeros.h>
 #include <linux/interrupt.h>
 #include <linux/iopoll.h>
+#include <linux/log2.h>
 #include <linux/platform_device.h>
 #include <linux/reset.h>
 #include <linux/spi/spi.h>
@@ -99,20 +100,6 @@ struct rzv2m_csi_priv {
 	u32 status;
 };
 
-static const unsigned char x_trg[] = {
-	0, 1, 1, 2, 2, 2, 2, 3,
-	3, 3, 3, 3, 3, 3, 3, 4,
-	4, 4, 4, 4, 4, 4, 4, 4,
-	4, 4, 4, 4, 4, 4, 4, 5
-};
-
-static const unsigned char x_trg_words[] = {
-	1,  2,  2,  4,  4,  4,  4,  8,
-	8,  8,  8,  8,  8,  8,  8,  16,
-	16, 16, 16, 16, 16, 16, 16, 16,
-	16, 16, 16, 16, 16, 16, 16, 32
-};
-
 static void rzv2m_csi_reg_write_bit(const struct rzv2m_csi_priv *csi,
 				    int reg_offs, int bit_mask, u32 value)
 {
@@ -216,7 +203,7 @@ static inline void rzv2m_csi_calc_current_transfer(struct rzv2m_csi_priv *csi)
 	 * less than or equal to the number of bytes we need to transfer.
 	 * This may result in multiple smaller transfers.
 	 */
-	csi->words_to_transfer = x_trg_words[to_transfer - 1];
+	csi->words_to_transfer = rounddown_pow_of_two(to_transfer);
 
 	if (csi->bytes_per_word == 2)
 		csi->bytes_to_transfer = csi->words_to_transfer << 1;
@@ -227,7 +214,7 @@ static inline void rzv2m_csi_calc_current_transfer(struct rzv2m_csi_priv *csi)
 static inline void rzv2m_csi_set_rx_fifo_trigger_level(struct rzv2m_csi_priv *csi)
 {
 	rzv2m_csi_reg_write_bit(csi, CSI_FIFOTRG, CSI_FIFOTRG_R_TRG,
-				x_trg[csi->words_to_transfer - 1]);
+				ilog2(csi->words_to_transfer));
 }
 
 static inline void rzv2m_csi_enable_rx_trigger(struct rzv2m_csi_priv *csi,
@@ -300,7 +287,6 @@ static int rzv2m_csi_wait_for_tx_empty(struct rzv2m_csi_priv *csi)
 		return 0;
 
 	ret = rzv2m_csi_wait_for_interrupt(csi, CSI_INT_TREND, CSI_CNT_TREND_E);
-
 	if (ret == -ETIMEDOUT)
 		csi->errors |= TX_TIMEOUT_ERROR;
 
@@ -316,7 +302,6 @@ static inline int rzv2m_csi_wait_for_rx_ready(struct rzv2m_csi_priv *csi)
 
 	ret = rzv2m_csi_wait_for_interrupt(csi, CSI_INT_R_TRGR,
 					   CSI_CNT_R_TRGR_E);
-
 	if (ret == -ETIMEDOUT)
 		csi->errors |= RX_TIMEOUT_ERROR;
 
-- 
2.34.1

Re: [PATCH 09/10] spi: rzv2m-csi: Get rid of the x_trg{_words} tables

[Andy Shevchenko]

On Sat, Jul 15, 2023 at 4:04 AM Fabrizio Castro
<fabrizio.castro.jz@renesas.com> wrote:
>
> Table x_trg can be replaced with ilog2(), and table x_trg_words
> can be replaced with rounddown_pow_of_two().
> Replace the tables usage with the corresponding macros.
> While at it, remove a couple of unnecessary empty lines.

Suggested-by: ?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 09/10] spi: rzv2m-csi: Get rid of the x_trg{_words} tables

[Geert Uytterhoeven]

On Sat, Jul 15, 2023 at 3:04 AM Fabrizio Castro
<fabrizio.castro.jz@renesas.com> wrote:
> Table x_trg can be replaced with ilog2(), and table x_trg_words
> can be replaced with rounddown_pow_of_two().
> Replace the tables usage with the corresponding macros.
> While at it, remove a couple of unnecessary empty lines.
>
> Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH 10/10] spi: rzv2m-csi: Make use of device_set_node

[Fabrizio Castro]

Use device_set_node instead of assigning controller->dev.of_node
directly.

Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
---
 drivers/spi/spi-rzv2m-csi.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-rzv2m-csi.c b/drivers/spi/spi-rzv2m-csi.c
index 1874ca1c2747..ad7ca514eb09 100644
--- a/drivers/spi/spi-rzv2m-csi.c
+++ b/drivers/spi/spi-rzv2m-csi.c
@@ -575,12 +575,13 @@ static int rzv2m_csi_probe(struct platform_device *pdev)
 	init_waitqueue_head(&csi->wait);
 
 	controller->mode_bits = SPI_CPOL | SPI_CPHA | SPI_LSB_FIRST;
-	controller->dev.of_node = pdev->dev.of_node;
 	controller->bits_per_word_mask = SPI_BPW_MASK(16) | SPI_BPW_MASK(8);
 	controller->setup = rzv2m_csi_setup;
 	controller->transfer_one = rzv2m_csi_transfer_one;
 	controller->use_gpio_descriptors = true;
 
+	device_set_node(&controller->dev, dev_fwnode(dev));
+
 	ret = devm_request_irq(dev, irq, rzv2m_csi_irq_handler, 0,
 			       dev_name(dev), csi);
 	if (ret)
-- 
2.34.1

Re: [PATCH 10/10] spi: rzv2m-csi: Make use of device_set_node

[Andy Shevchenko]

On Sat, Jul 15, 2023 at 4:04 AM Fabrizio Castro
<fabrizio.castro.jz@renesas.com> wrote:
>
> Use device_set_node instead of assigning controller->dev.of_node
> directly.

"...because it also sets the firmware node."

You probably need to add property.h, if not added yet.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH 10/10] spi: rzv2m-csi: Make use of device_set_node

[Fabrizio Castro]

Hi Andy,

> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Subject: Re: [PATCH 10/10] spi: rzv2m-csi: Make use of device_set_node
> 
> On Sat, Jul 15, 2023 at 4:04 AM Fabrizio Castro
> <fabrizio.castro.jz@renesas.com> wrote:
> >
> > Use device_set_node instead of assigning controller->dev.of_node
> > directly.
> 
> "...because it also sets the firmware node."
> 
> You probably need to add property.h, if not added yet.

You are right, I'll send a v2 for this.
I'll also add the related Suggested-by and Reviewed-by tags in v2.

Cheers,
Fab

> 
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH 00/10] spi: rzv2m-csi: Code refactoring

[Andy Shevchenko]

On Sat, Jul 15, 2023 at 4:04 AM Fabrizio Castro
<fabrizio.castro.jz@renesas.com> wrote:
>
> Dear All,
>
> this series is to follow up on Geert and Andy feedback:
> https://patchwork.kernel.org/project/linux-renesas-soc/patch/20230622113341.657842-4-fabrizio.castro.jz@renesas.com/

Thank you!

Some comments I sent against individual patches (Suggested-by can be
applied to many, btw), if you address them as suggested, feel free to
add
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
to the entire series.

-- 
With Best Regards,
Andy Shevchenko

Re: (subset) [PATCH 00/10] spi: rzv2m-csi: Code refactoring

[Mark Brown]

On Sat, 15 Jul 2023 02:03:57 +0100, Fabrizio Castro wrote:
> this series is to follow up on Geert and Andy feedback:
> https://patchwork.kernel.org/project/linux-renesas-soc/patch/20230622113341.657842-4-fabrizio.castro.jz@renesas.com/
> 
> Thanks,
> Fab
> 
> Fabrizio Castro (10):
>   spi: rzv2m-csi: Add missing include
>   spi: rzv2m-csi: Adopt HZ_PER_MHZ for max spi clock
>   spi: rzv2m-csi: Rework CSI_CKS_MAX definition
>   spi: rzv2m-csi: Leave readl_poll_timeout calls for last
>   spi: rzv2m-csi: Replace unnecessary ternary operators
>   spi: rzv2m-csi: Squash timing settings into one statement
>   spi: rzv2m-csi: Switch to using {read,write}s{b,w}
>   spi: rzv2m-csi: Improve data types and alignment
>   spi: rzv2m-csi: Get rid of the x_trg{_words} tables
>   spi: rzv2m-csi: Make use of device_set_node
> 
> [...]

Applied to

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

Thanks!

[01/10] spi: rzv2m-csi: Add missing include
        commit: f572ba797c639c9b1705908d3f5d71ed7c3f53e0
[02/10] spi: rzv2m-csi: Adopt HZ_PER_MHZ for max spi clock
        commit: 74e27ce8d23c3aeb1a9fdcaf6261462506bbbfc3
[03/10] spi: rzv2m-csi: Rework CSI_CKS_MAX definition
        commit: aecf9fbdb7a4dc6d83e8d9984c8d9dc074d8ea2e
[04/10] spi: rzv2m-csi: Leave readl_poll_timeout calls for last
        commit: 2ed2699f58891c72fcd462129345d09424f986c5
[05/10] spi: rzv2m-csi: Replace unnecessary ternary operators
        commit: 9f5ac599801c0f7c0969fa94c638265ed988b9bc

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