[PATCH v2 0/4] spi: rzv2m-csi: Code refactoring

[PATCH v2 0/4] spi: rzv2m-csi: Code refactoring

[Fabrizio Castro]

Dear All,

I am sending this series to follow up on the comments received
from the first version of this series.
The first 5 patches of the first series have been taken by Mark
already (thanks Mark), this second version only addresses the
remaining patches.

I would like to highlight that I have dropped patch
"spi: rzv2m-csi: Switch to using {read,write}s{b,w}" for now,
and maybe I will send a follow up patch later on.

Thanks,
Fab

Fabrizio Castro (4):
  spi: rzv2m-csi: Squash timing settings into one statement
  spi: rzv2m-csi: Improve data types, casting 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 | 78 +++++++++++++++----------------------
 1 file changed, 32 insertions(+), 46 deletions(-)

-- 
2.34.1

[PATCH v2 1/4] 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>
Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---

v2: No change.

 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

[PATCH v2 2/4] spi: rzv2m-csi: Improve data types, casting and alignment

[Fabrizio Castro]

"unsigned int" is more appropriate than "int" for the members
of "struct rzv2m_csi_priv".
Using void* rather than u8* for txbuf and rxbuf allows for
the removal of some type casting.
Remove the unnecessary casting of "data" to "struct rzv2m_csi_priv*"
in function "rzv2m_csi_irq_handler".
Also, members "bytes_per_word" and "errors" introduce gaps
in the structure.
Adjust "struct rzv2m_csi_priv" and its members usage accordingly.

Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
Suggested-by: Geert Uytterhoeven <geert+renesas@glider.be>
---

v2: This is basically v1 with the addition of changes to the data
    types of txbuf and rxbuf and related touches.
    Also, both the title of this patch and its changelog have been
    changed to reflect the new additions.
    Although both Geert and Andy kindly allowed for their Reviewed-by
    tags to used for this patch, I have dropped them in case they want
    to jump in.

 drivers/spi/spi-rzv2m-csi.c | 46 ++++++++++++++++++-------------------
 1 file changed, 23 insertions(+), 23 deletions(-)

diff --git a/drivers/spi/spi-rzv2m-csi.c b/drivers/spi/spi-rzv2m-csi.c
index faf5898bc3e0..4dbb8c185a8a 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
@@ -86,16 +86,16 @@ struct rzv2m_csi_priv {
 	struct clk *pclk;
 	struct device *dev;
 	struct spi_controller *controller;
-	const u8 *txbuf;
-	u8 *rxbuf;
-	int buffer_len;
-	int bytes_sent;
-	int bytes_received;
-	int bytes_to_transfer;
-	int words_to_transfer;
-	unsigned char bytes_per_word;
+	const void *txbuf;
+	void *rxbuf;
+	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;
 };
 
@@ -157,18 +157,18 @@ 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;
+	unsigned int i;
 
 	if (readl(csi->base + CSI_OFIFOL))
 		return -EIO;
 
 	if (csi->bytes_per_word == 2) {
-		u16 *buf = (u16 *)csi->txbuf;
+		const u16 *buf = csi->txbuf;
 
 		for (i = 0; i < csi->words_to_transfer; i++)
 			writel(buf[i], csi->base + CSI_OFIFO);
 	} else {
-		u8 *buf = (u8 *)csi->txbuf;
+		const u8 *buf = csi->txbuf;
 
 		for (i = 0; i < csi->words_to_transfer; i++)
 			writel(buf[i], csi->base + CSI_OFIFO);
@@ -182,18 +182,18 @@ static int rzv2m_csi_fill_txfifo(struct rzv2m_csi_priv *csi)
 
 static int rzv2m_csi_read_rxfifo(struct rzv2m_csi_priv *csi)
 {
-	int i;
+	unsigned 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;
+		u16 *buf = 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;
+		u8 *buf = csi->rxbuf;
 
 		for (i = 0; i < csi->words_to_transfer; i++)
 			buf[i] = (u8)readl(csi->base + CSI_IFIFO);
@@ -207,9 +207,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)
 		/*
@@ -217,9 +217,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;
@@ -339,7 +339,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

[PATCH v2 3/4] 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>
Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
---

v2: No change.

 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 4dbb8c185a8a..62575a61608a 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)
 {
@@ -230,7 +217,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;
@@ -241,7 +228,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,
@@ -314,7 +301,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;
 
@@ -330,7 +316,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

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

[Fabrizio Castro]

Use device_set_node instead of assigning controller->dev.of_node
directly because it also sets the firmware node.

Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---

v2: Edited the changelog and added include of property.h, according
    to the feedback received from Andy.

 drivers/spi/spi-rzv2m-csi.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-rzv2m-csi.c b/drivers/spi/spi-rzv2m-csi.c
index 62575a61608a..d098aefa370d 100644
--- a/drivers/spi/spi-rzv2m-csi.c
+++ b/drivers/spi/spi-rzv2m-csi.c
@@ -12,6 +12,7 @@
 #include <linux/iopoll.h>
 #include <linux/log2.h>
 #include <linux/platform_device.h>
+#include <linux/property.h>
 #include <linux/reset.h>
 #include <linux/spi/spi.h>
 #include <linux/units.h>
@@ -589,12 +590,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 v2 2/4] spi: rzv2m-csi: Improve data types, casting and alignment

[Andy Shevchenko]

On Tue, Jul 18, 2023 at 10:25 PM Fabrizio Castro
<fabrizio.castro.jz@renesas.com> wrote:
>
> "unsigned int" is more appropriate than "int" for the members
> of "struct rzv2m_csi_priv".
> Using void* rather than u8* for txbuf and rxbuf allows for
> the removal of some type casting.
> Remove the unnecessary casting of "data" to "struct rzv2m_csi_priv*"
> in function "rzv2m_csi_irq_handler".
> Also, members "bytes_per_word" and "errors" introduce gaps
> in the structure.
> Adjust "struct rzv2m_csi_priv" and its members usage accordingly.

Hmm... A bit of fancy indentation. Why is each sentence separated?

...

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

As far as I understand Geert he wanted something like

  u32 status;
  u8 errors;

...

> -               u16 *buf = (u16 *)csi->txbuf;
> +               const u16 *buf = csi->txbuf;

> -               u8 *buf = (u8 *)csi->txbuf;
> +               const u8 *buf = csi->txbuf;

> -               u16 *buf = (u16 *)csi->rxbuf;
> +               u16 *buf = csi->rxbuf;

> -               u8 *buf = (u8 *)csi->rxbuf;
> +               u8 *buf = csi->rxbuf;

Yep, these look much better now.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Tue, Jul 18, 2023 at 10:25 PM Fabrizio Castro
<fabrizio.castro.jz@renesas.com> wrote:
>
> Use device_set_node instead of assigning controller->dev.of_node

device_set_node()

> directly because it also sets the firmware node.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/4] spi: rzv2m-csi: Improve data types, casting and alignment

[Geert Uytterhoeven]

On Tue, Jul 18, 2023 at 9:25 PM Fabrizio Castro
<fabrizio.castro.jz@renesas.com> wrote:
> "unsigned int" is more appropriate than "int" for the members
> of "struct rzv2m_csi_priv".
> Using void* rather than u8* for txbuf and rxbuf allows for
> the removal of some type casting.
> Remove the unnecessary casting of "data" to "struct rzv2m_csi_priv*"
> in function "rzv2m_csi_irq_handler".
> Also, members "bytes_per_word" and "errors" introduce gaps
> in the structure.
> Adjust "struct rzv2m_csi_priv" and its members usage accordingly.
>
> Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> Suggested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>
> v2: This is basically v1 with the addition of changes to the data
>     types of txbuf and rxbuf and related touches.
>     Also, both the title of this patch and its changelog have been
>     changed to reflect the new additions.
>     Although both Geert and Andy kindly allowed for their Reviewed-by
>     tags to used for this patch, I have dropped them in case they want
>     to jump in.

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

Re: [PATCH v2 0/4] spi: rzv2m-csi: Code refactoring

[Mark Brown]

On Tue, 18 Jul 2023 20:24:49 +0100, Fabrizio Castro wrote:
> I am sending this series to follow up on the comments received
> from the first version of this series.
> The first 5 patches of the first series have been taken by Mark
> already (thanks Mark), this second version only addresses the
> remaining patches.
> 
> I would like to highlight that I have dropped patch
> "spi: rzv2m-csi: Switch to using {read,write}s{b,w}" for now,
> and maybe I will send a follow up patch later on.
> 
> [...]

Applied to

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

Thanks!

[1/4] spi: rzv2m-csi: Squash timing settings into one statement
      commit: d5737d12779a171e76ad07635d1ed06a22009da7
[2/4] spi: rzv2m-csi: Improve data types, casting and alignment
      commit: 8dc4038a026a79b6222a43ccf7adf070c4ba54ea
[3/4] spi: rzv2m-csi: Get rid of the x_trg{_words} tables
      commit: 7b63568fce9cb34cb0ad4caf9231555eb768c8e6
[4/4] spi: rzv2m-csi: Make use of device_set_node
      commit: c5a7b66811d22a4901bd358447e59160dbda8f65

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