[PATCH v3 00/17] spi: s3c64xx: straightforward cleanup

[PATCH v3 00/17] spi: s3c64xx: straightforward cleanup

[Tudor Ambarus]

Straightforward patches that clean the driver. Just compiled tested.

Stands up just the patch that updates the driver to use the bitfield
access macros. The bit operations shall be identical after the patch.
Sam and Andi have some concerns on whether using the bitfield access
macros are just a matter of taste, or they are actually necessary.
I think they are necessary. Here are the concerns/discussions:
https://lore.kernel.org/linux-arm-kernel/ee4107c3-1141-45ab-874c-03474d8ec18d@linaro.org/
https://lore.kernel.org/linux-arm-kernel/ri7gerw4ov4jnmmkhtumhhtgfgxtr6kpsopdxjlx6fylbqznna@3qgvejyhjirw/

Cheers,
ta

v3:
- reworked the bitfied access macros patch so that the bit operations
  are the same as before the patch. Fix S3C64XX_SPI_PSR_MASK value,
  drop S3C64XX_SPI_CS_NSC_CNT_MASK.
- add a new patches to explicitly remove a duplicated definition and to
  drop a superfluous bitwise NOT operation.
- collect R-b tags

v2:
https://lore.kernel.org/linux-arm-kernel/36a664b1-666d-4fc4-90d9-35b42e56973d@linaro.org/

Tudor Ambarus (17):
  spi: s3c64xx: explicitly include <linux/io.h>
  spi: s3c64xx: explicitly include <linux/bits.h>
  spi: s3c64xx: avoid possible negative array index
  spi: s3c64xx: fix typo, s/configuartion/configuration
  spi: s3c64xx: sort headers alphabetically
  spi: s3c64xx: remove unneeded (void *) casts in of_match_table
  spi: s3c64xx: remove else after return
  spi: s3c64xx: move common code outside if else
  spi: s3c64xx: check return code of dmaengine_slave_config()
  spi: s3c64xx: propagate the dma_submit_error() error code
  spi: s3c64xx: rename prepare_dma() to s3c64xx_prepare_dma()
  spi: s3c64xx: return ETIMEDOUT for wait_for_completion_timeout()
  spi: s3c64xx: drop blank line between declarations
  spi: s3c64xx: downgrade dev_warn to dev_dbg for optional dt props
  spi: s3c64xx: remove duplicated definition
  spi: s3c64xx: drop a superfluous bitwise NOT operation
  spi: s3c64xx: use bitfield access macros

 drivers/spi/spi-s3c64xx.c | 298 ++++++++++++++++++++------------------
 1 file changed, 158 insertions(+), 140 deletions(-)

-- 
2.43.0.429.g432eaa2c6b-goog

[PATCH v3 01/17] spi: s3c64xx: explicitly include <linux/io.h>

[Tudor Ambarus]

The driver uses readl() but does not include <linux/io.h>.

It is good practice to directly include all headers used, it avoids
implicit dependencies and spurious breakage if someone rearranges
headers and causes the implicit include to vanish.

Include the missing header.

Fixes: 230d42d422e7 ("spi: Add s3c64xx SPI Controller driver")
Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 drivers/spi/spi-s3c64xx.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 7f7eb8f742e4..c1cbc4780a3b 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -10,6 +10,7 @@
 #include <linux/clk.h>
 #include <linux/dma-mapping.h>
 #include <linux/dmaengine.h>
+#include <linux/io.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/spi/spi.h>
-- 
2.43.0.429.g432eaa2c6b-goog

[PATCH v3 02/17] spi: s3c64xx: explicitly include <linux/bits.h>

[Tudor Ambarus]

The driver uses GENMASK() but does not include <linux/bits.h>.

It is good practice to directly include all headers used, it avoids
implicit dependencies and spurious breakage if someone rearranges
headers and causes the implicit include to vanish.

Include the missing header.

Fixes: 1224e29572f6 ("spi: s3c64xx: Fix large transfers with DMA")
Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 drivers/spi/spi-s3c64xx.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index c1cbc4780a3b..2b5bb7604526 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -3,6 +3,7 @@
 // Copyright (c) 2009 Samsung Electronics Co., Ltd.
 //      Jaswinder Singh <jassi.brar@samsung.com>
 
+#include <linux/bits.h>
 #include <linux/init.h>
 #include <linux/module.h>
 #include <linux/interrupt.h>
-- 
2.43.0.429.g432eaa2c6b-goog

[PATCH v3 03/17] spi: s3c64xx: avoid possible negative array index

[Tudor Ambarus]

The platform id is used as an index into the fifo_lvl_mask array.
Platforms can come with a negative device ID, PLATFORM_DEVID_NONE (-1),
thus we risked a negative array index. Catch such cases and fail to
probe.

Fixes: 2b90807549e5 ("spi: s3c64xx: add device tree support")
Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>
Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 drivers/spi/spi-s3c64xx.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 2b5bb7604526..c3176a510643 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -1189,6 +1189,9 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
 					     "Failed to get alias id\n");
 		sdd->port_id = ret;
 	} else {
+		if (pdev->id < 0)
+			return dev_err_probe(&pdev->dev, -EINVAL,
+					     "Negative platform ID is not allowed\n");
 		sdd->port_id = pdev->id;
 	}
 
-- 
2.43.0.429.g432eaa2c6b-goog

[PATCH v3 04/17] spi: s3c64xx: fix typo, s/configuartion/configuration

[Tudor Ambarus]

Fix typo, s/configuartion/configuration.

Fixes: 6b8d1e4739f4 ("spi: spi-s3c64xx: Add missing entries for structs 's3c64xx_spi_dma_data' and 's3c64xx_spi_dma_data'")
Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>
Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 drivers/spi/spi-s3c64xx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index c3176a510643..3df4906bba34 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -180,7 +180,7 @@ struct s3c64xx_spi_port_config {
  * @cur_speed: Current clock speed
  * @rx_dma: Local receive DMA data (e.g. chan and direction)
  * @tx_dma: Local transmit DMA data (e.g. chan and direction)
- * @port_conf: Local SPI port configuartion data
+ * @port_conf: Local SPI port configuration data
  * @port_id: Port identification number
  */
 struct s3c64xx_spi_driver_data {
-- 
2.43.0.429.g432eaa2c6b-goog

[PATCH v3 05/17] spi: s3c64xx: sort headers alphabetically

[Tudor Ambarus]

Sorting headers alphabetically helps locating duplicates,
and makes it easier to figure out where to insert new headers.

Reviewed-by: Andi Shyti <andi.shyti@kernel.org>
Signed-off-by: Tudor Ambarus <tudor.ambarus@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 3df4906bba34..ccb700312d64 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -4,20 +4,19 @@
 //      Jaswinder Singh <jassi.brar@samsung.com>
 
 #include <linux/bits.h>
-#include <linux/init.h>
-#include <linux/module.h>
-#include <linux/interrupt.h>
-#include <linux/delay.h>
 #include <linux/clk.h>
+#include <linux/delay.h>
 #include <linux/dma-mapping.h>
 #include <linux/dmaengine.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
 #include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_data/spi-s3c64xx.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/spi/spi.h>
-#include <linux/of.h>
-
-#include <linux/platform_data/spi-s3c64xx.h>
 
 #define MAX_SPI_PORTS		12
 #define S3C64XX_SPI_QUIRK_CS_AUTO	(1 << 1)
-- 
2.43.0.429.g432eaa2c6b-goog

[PATCH v3 06/17] spi: s3c64xx: remove unneeded (void *) casts in of_match_table

[Tudor Ambarus]

of_device_id::data is an opaque pointer. No explicit cast is needed.
Remove unneeded (void *) casts in of_match_table. While here align the
compatible and data members.

Reviewed-by: Andi Shyti <andi.shyti@kernel.org>
Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 drivers/spi/spi-s3c64xx.c | 45 +++++++++++++++++++++++----------------
 1 file changed, 27 insertions(+), 18 deletions(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index ccb700312d64..9bf54c1044b3 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -1511,32 +1511,41 @@ static const struct platform_device_id s3c64xx_spi_driver_ids[] = {
 };
 
 static const struct of_device_id s3c64xx_spi_dt_match[] = {
-	{ .compatible = "samsung,s3c2443-spi",
-			.data = (void *)&s3c2443_spi_port_config,
+	{
+		.compatible = "samsung,s3c2443-spi",
+		.data = &s3c2443_spi_port_config,
 	},
-	{ .compatible = "samsung,s3c6410-spi",
-			.data = (void *)&s3c6410_spi_port_config,
+	{
+		.compatible = "samsung,s3c6410-spi",
+		.data = &s3c6410_spi_port_config,
 	},
-	{ .compatible = "samsung,s5pv210-spi",
-			.data = (void *)&s5pv210_spi_port_config,
+	{
+		.compatible = "samsung,s5pv210-spi",
+		.data = &s5pv210_spi_port_config,
 	},
-	{ .compatible = "samsung,exynos4210-spi",
-			.data = (void *)&exynos4_spi_port_config,
+	{
+		.compatible = "samsung,exynos4210-spi",
+		.data = &exynos4_spi_port_config,
 	},
-	{ .compatible = "samsung,exynos7-spi",
-			.data = (void *)&exynos7_spi_port_config,
+	{
+		.compatible = "samsung,exynos7-spi",
+		.data = &exynos7_spi_port_config,
 	},
-	{ .compatible = "samsung,exynos5433-spi",
-			.data = (void *)&exynos5433_spi_port_config,
+	{
+		.compatible = "samsung,exynos5433-spi",
+		.data = &exynos5433_spi_port_config,
 	},
-	{ .compatible = "samsung,exynos850-spi",
-			.data = (void *)&exynos850_spi_port_config,
+	{
+		.compatible = "samsung,exynos850-spi",
+		.data = &exynos850_spi_port_config,
 	},
-	{ .compatible = "samsung,exynosautov9-spi",
-			.data = (void *)&exynosautov9_spi_port_config,
+	{
+		.compatible = "samsung,exynosautov9-spi",
+		.data = &exynosautov9_spi_port_config,
 	},
-	{ .compatible = "tesla,fsd-spi",
-			.data = (void *)&fsd_spi_port_config,
+	{
+		.compatible = "tesla,fsd-spi",
+		.data = &fsd_spi_port_config,
 	},
 	{ },
 };
-- 
2.43.0.429.g432eaa2c6b-goog

[PATCH v3 07/17] spi: s3c64xx: remove else after return

[Tudor Ambarus]

Else case is not needed after a return, remove it.

Reviewed-by: Andi Shyti <andi.shyti@kernel.org>
Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>
Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 drivers/spi/spi-s3c64xx.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 9bf54c1044b3..bd2ac875af59 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -407,12 +407,10 @@ 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) {
+	if (sdd->rx_dma.ch && sdd->tx_dma.ch)
 		return xfer->len > FIFO_DEPTH(sdd);
-	} else {
-		return false;
-	}
 
+	return false;
 }
 
 static int s3c64xx_enable_datapath(struct s3c64xx_spi_driver_data *sdd,
-- 
2.43.0.429.g432eaa2c6b-goog

[PATCH v3 08/17] spi: s3c64xx: move common code outside if else

[Tudor Ambarus]

Move common code outside if else to avoid code duplication.

Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>
Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 drivers/spi/spi-s3c64xx.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index bd2ac875af59..bbbc4795bcbf 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -291,20 +291,18 @@ static int prepare_dma(struct s3c64xx_spi_dma_data *dma,
 	if (dma->direction == DMA_DEV_TO_MEM) {
 		sdd = container_of((void *)dma,
 			struct s3c64xx_spi_driver_data, rx_dma);
-		config.direction = dma->direction;
 		config.src_addr = sdd->sfr_start + S3C64XX_SPI_RX_DATA;
 		config.src_addr_width = sdd->cur_bpw / 8;
 		config.src_maxburst = 1;
-		dmaengine_slave_config(dma->ch, &config);
 	} else {
 		sdd = container_of((void *)dma,
 			struct s3c64xx_spi_driver_data, tx_dma);
-		config.direction = dma->direction;
 		config.dst_addr = sdd->sfr_start + S3C64XX_SPI_TX_DATA;
 		config.dst_addr_width = sdd->cur_bpw / 8;
 		config.dst_maxburst = 1;
-		dmaengine_slave_config(dma->ch, &config);
 	}
+	config.direction = dma->direction;
+	dmaengine_slave_config(dma->ch, &config);
 
 	desc = dmaengine_prep_slave_sg(dma->ch, sgt->sgl, sgt->nents,
 				       dma->direction, DMA_PREP_INTERRUPT);
-- 
2.43.0.429.g432eaa2c6b-goog

[PATCH v3 09/17] spi: s3c64xx: check return code of dmaengine_slave_config()

[Tudor Ambarus]

Check the return code of dmaengine_slave_config().

Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>
Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 drivers/spi/spi-s3c64xx.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index bbbc4795bcbf..6268790bbcff 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -302,7 +302,9 @@ static int prepare_dma(struct s3c64xx_spi_dma_data *dma,
 		config.dst_maxburst = 1;
 	}
 	config.direction = dma->direction;
-	dmaengine_slave_config(dma->ch, &config);
+	ret = dmaengine_slave_config(dma->ch, &config);
+	if (ret)
+		return ret;
 
 	desc = dmaengine_prep_slave_sg(dma->ch, sgt->sgl, sgt->nents,
 				       dma->direction, DMA_PREP_INTERRUPT);
-- 
2.43.0.429.g432eaa2c6b-goog

[PATCH v3 10/17] spi: s3c64xx: propagate the dma_submit_error() error code

[Tudor Ambarus]

DMA submit should just add the dma descriptor to a queue, without firing
it. EIO is misleading and hides what happens in DMA. Propagate the
dma_submit_error() error code, don't overwrite it.

Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 drivers/spi/spi-s3c64xx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 6268790bbcff..64daf944b245 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -321,7 +321,7 @@ static int prepare_dma(struct s3c64xx_spi_dma_data *dma,
 	ret = dma_submit_error(dma->cookie);
 	if (ret) {
 		dev_err(&sdd->pdev->dev, "DMA submission failed");
-		return -EIO;
+		return ret;
 	}
 
 	dma_async_issue_pending(dma->ch);
-- 
2.43.0.429.g432eaa2c6b-goog

[PATCH v3 11/17] spi: s3c64xx: rename prepare_dma() to s3c64xx_prepare_dma()

[Tudor Ambarus]

Don't monopolize the name. Prepend the driver prefix to the function
name.

Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>
Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 drivers/spi/spi-s3c64xx.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 64daf944b245..76fa378ab5ab 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -278,8 +278,8 @@ static void s3c64xx_spi_dmacb(void *data)
 	spin_unlock_irqrestore(&sdd->lock, flags);
 }
 
-static int prepare_dma(struct s3c64xx_spi_dma_data *dma,
-			struct sg_table *sgt)
+static int s3c64xx_prepare_dma(struct s3c64xx_spi_dma_data *dma,
+			       struct sg_table *sgt)
 {
 	struct s3c64xx_spi_driver_data *sdd;
 	struct dma_slave_config config;
@@ -444,7 +444,7 @@ static int s3c64xx_enable_datapath(struct s3c64xx_spi_driver_data *sdd,
 		chcfg |= S3C64XX_SPI_CH_TXCH_ON;
 		if (dma_mode) {
 			modecfg |= S3C64XX_SPI_MODE_TXDMA_ON;
-			ret = prepare_dma(&sdd->tx_dma, &xfer->tx_sg);
+			ret = s3c64xx_prepare_dma(&sdd->tx_dma, &xfer->tx_sg);
 		} else {
 			switch (sdd->cur_bpw) {
 			case 32:
@@ -476,7 +476,7 @@ static int s3c64xx_enable_datapath(struct s3c64xx_spi_driver_data *sdd,
 			writel(((xfer->len * 8 / sdd->cur_bpw) & 0xffff)
 					| S3C64XX_SPI_PACKET_CNT_EN,
 					regs + S3C64XX_SPI_PACKET_CNT);
-			ret = prepare_dma(&sdd->rx_dma, &xfer->rx_sg);
+			ret = s3c64xx_prepare_dma(&sdd->rx_dma, &xfer->rx_sg);
 		}
 	}
 
-- 
2.43.0.429.g432eaa2c6b-goog

[PATCH v3 12/17] spi: s3c64xx: return ETIMEDOUT for wait_for_completion_timeout()

[Tudor Ambarus]

ETIMEDOUT is more specific than EIO, use it for
wait_for_completion_timeout().

Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 drivers/spi/spi-s3c64xx.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 76fa378ab5ab..2f2c4ad35df4 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -526,7 +526,7 @@ static int s3c64xx_wait_for_dma(struct s3c64xx_spi_driver_data *sdd,
 
 	/*
 	 * If the previous xfer was completed within timeout, then
-	 * proceed further else return -EIO.
+	 * proceed further else return -ETIMEDOUT.
 	 * DmaTx returns after simply writing data in the FIFO,
 	 * w/o waiting for real transmission on the bus to finish.
 	 * DmaRx returns only after Dma read data from FIFO which
@@ -547,7 +547,7 @@ static int s3c64xx_wait_for_dma(struct s3c64xx_spi_driver_data *sdd,
 
 	/* If timed out while checking rx/tx status return error */
 	if (!val)
-		return -EIO;
+		return -ETIMEDOUT;
 
 	return 0;
 }
@@ -577,7 +577,7 @@ static int s3c64xx_wait_for_pio(struct s3c64xx_spi_driver_data *sdd,
 	if (use_irq) {
 		val = msecs_to_jiffies(ms);
 		if (!wait_for_completion_timeout(&sdd->xfer_completion, val))
-			return -EIO;
+			return -ETIMEDOUT;
 	}
 
 	val = msecs_to_loops(ms);
-- 
2.43.0.429.g432eaa2c6b-goog

[PATCH v3 13/17] spi: s3c64xx: drop blank line between declarations

[Tudor Ambarus]

Drop the blank line and move the logical operation in the body of the
function rather than in initialization list.

Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>
Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 drivers/spi/spi-s3c64xx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 2f2c4ad35df4..08ba14adb428 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -1320,8 +1320,9 @@ static int s3c64xx_spi_suspend(struct device *dev)
 {
 	struct spi_controller *host = dev_get_drvdata(dev);
 	struct s3c64xx_spi_driver_data *sdd = spi_controller_get_devdata(host);
+	int ret;
 
-	int ret = spi_controller_suspend(host);
+	ret = spi_controller_suspend(host);
 	if (ret)
 		return ret;
 
-- 
2.43.0.429.g432eaa2c6b-goog

[PATCH v3 14/17] spi: s3c64xx: downgrade dev_warn to dev_dbg for optional dt props

[Tudor Ambarus]

"samsung,spi-src-clk" and "num-cs" are optional dt properties. Downgrade
the message from warning to debug message.

Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>
Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 drivers/spi/spi-s3c64xx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 08ba14adb428..dc779d5305a5 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -1109,14 +1109,14 @@ static struct s3c64xx_spi_info *s3c64xx_spi_parse_dt(struct device *dev)
 		return ERR_PTR(-ENOMEM);
 
 	if (of_property_read_u32(dev->of_node, "samsung,spi-src-clk", &temp)) {
-		dev_warn(dev, "spi bus clock parent not specified, using clock at index 0 as parent\n");
+		dev_dbg(dev, "spi bus clock parent not specified, using clock at index 0 as parent\n");
 		sci->src_clk_nr = 0;
 	} else {
 		sci->src_clk_nr = temp;
 	}
 
 	if (of_property_read_u32(dev->of_node, "num-cs", &temp)) {
-		dev_warn(dev, "number of chip select lines not specified, assuming 1 chip select line\n");
+		dev_dbg(dev, "number of chip select lines not specified, assuming 1 chip select line\n");
 		sci->num_cs = 1;
 	} else {
 		sci->num_cs = temp;
-- 
2.43.0.429.g432eaa2c6b-goog

[PATCH v3 15/17] spi: s3c64xx: remove duplicated definition

[Tudor Ambarus]

S3C64XX_SPI_TRAILCNT brings no benefit in terms of name over
S3C64XX_SPI_MAX_TRAILCNT. Remove the duplicated definition.

Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 drivers/spi/spi-s3c64xx.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index dc779d5305a5..e9344fe71e56 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -115,8 +115,6 @@
 #define S3C64XX_SPI_MAX_TRAILCNT	0x3ff
 #define S3C64XX_SPI_TRAILCNT_OFF	19
 
-#define S3C64XX_SPI_TRAILCNT		S3C64XX_SPI_MAX_TRAILCNT
-
 #define S3C64XX_SPI_POLLING_SIZE	32
 
 #define msecs_to_loops(t) (loops_per_jiffy / 1000 * HZ * t)
@@ -1092,7 +1090,7 @@ static void s3c64xx_spi_hwinit(struct s3c64xx_spi_driver_data *sdd)
 	val = readl(regs + S3C64XX_SPI_MODE_CFG);
 	val &= ~S3C64XX_SPI_MODE_4BURST;
 	val &= ~(S3C64XX_SPI_MAX_TRAILCNT << S3C64XX_SPI_TRAILCNT_OFF);
-	val |= (S3C64XX_SPI_TRAILCNT << S3C64XX_SPI_TRAILCNT_OFF);
+	val |= (S3C64XX_SPI_MAX_TRAILCNT << S3C64XX_SPI_TRAILCNT_OFF);
 	writel(val, regs + S3C64XX_SPI_MODE_CFG);
 
 	s3c64xx_flush_fifo(sdd);
-- 
2.43.0.429.g432eaa2c6b-goog

[PATCH v3 16/17] spi: s3c64xx: drop a superfluous bitwise NOT operation

[Tudor Ambarus]

val &= ~mask;
val |= mask;

is equivalent to:
val |= mask;

Drop the superfluous bitwise NOT operation.

Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 drivers/spi/spi-s3c64xx.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index e9344fe71e56..43b888c8812e 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -1089,7 +1089,6 @@ static void s3c64xx_spi_hwinit(struct s3c64xx_spi_driver_data *sdd)
 
 	val = readl(regs + S3C64XX_SPI_MODE_CFG);
 	val &= ~S3C64XX_SPI_MODE_4BURST;
-	val &= ~(S3C64XX_SPI_MAX_TRAILCNT << S3C64XX_SPI_TRAILCNT_OFF);
 	val |= (S3C64XX_SPI_MAX_TRAILCNT << S3C64XX_SPI_TRAILCNT_OFF);
 	writel(val, regs + S3C64XX_SPI_MODE_CFG);
 
-- 
2.43.0.429.g432eaa2c6b-goog

[PATCH v3 17/17] spi: s3c64xx: use bitfield access macros

[Tudor Ambarus]

Use the bitfield access macros in order to clean and to make the driver
easier to read. Introduce S3C64XX_SPI_MAX_TRAILCNT_MASK to replace value
and offset equivalents (S3C64XX_SPI_MAX_TRAILCNT,
S3C64XX_SPI_TRAILCNT_OFF). While touching the register definitions, align
their values to the same offset.

No functional change intended, the bit operations shall be equivalent.

Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 drivers/spi/spi-s3c64xx.c | 193 ++++++++++++++++++++------------------
 1 file changed, 101 insertions(+), 92 deletions(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 43b888c8812e..7f052d6cd2ba 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -4,6 +4,7 @@
 //      Jaswinder Singh <jassi.brar@samsung.com>
 
 #include <linux/bits.h>
+#include <linux/bitfield.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/dma-mapping.h>
@@ -18,91 +19,96 @@
 #include <linux/pm_runtime.h>
 #include <linux/spi/spi.h>
 
-#define MAX_SPI_PORTS		12
-#define S3C64XX_SPI_QUIRK_CS_AUTO	(1 << 1)
-#define AUTOSUSPEND_TIMEOUT	2000
+#define MAX_SPI_PORTS				12
+#define S3C64XX_SPI_QUIRK_CS_AUTO		BIT(1)
+#define AUTOSUSPEND_TIMEOUT			2000
 
 /* Registers and bit-fields */
 
-#define S3C64XX_SPI_CH_CFG		0x00
-#define S3C64XX_SPI_CLK_CFG		0x04
-#define S3C64XX_SPI_MODE_CFG		0x08
-#define S3C64XX_SPI_CS_REG		0x0C
-#define S3C64XX_SPI_INT_EN		0x10
-#define S3C64XX_SPI_STATUS		0x14
-#define S3C64XX_SPI_TX_DATA		0x18
-#define S3C64XX_SPI_RX_DATA		0x1C
-#define S3C64XX_SPI_PACKET_CNT		0x20
-#define S3C64XX_SPI_PENDING_CLR		0x24
-#define S3C64XX_SPI_SWAP_CFG		0x28
-#define S3C64XX_SPI_FB_CLK		0x2C
-
-#define S3C64XX_SPI_CH_HS_EN		(1<<6)	/* High Speed Enable */
-#define S3C64XX_SPI_CH_SW_RST		(1<<5)
-#define S3C64XX_SPI_CH_SLAVE		(1<<4)
-#define S3C64XX_SPI_CPOL_L		(1<<3)
-#define S3C64XX_SPI_CPHA_B		(1<<2)
-#define S3C64XX_SPI_CH_RXCH_ON		(1<<1)
-#define S3C64XX_SPI_CH_TXCH_ON		(1<<0)
-
-#define S3C64XX_SPI_CLKSEL_SRCMSK	(3<<9)
-#define S3C64XX_SPI_CLKSEL_SRCSHFT	9
-#define S3C64XX_SPI_ENCLK_ENABLE	(1<<8)
-#define S3C64XX_SPI_PSR_MASK		0xff
-
-#define S3C64XX_SPI_MODE_CH_TSZ_BYTE		(0<<29)
-#define S3C64XX_SPI_MODE_CH_TSZ_HALFWORD	(1<<29)
-#define S3C64XX_SPI_MODE_CH_TSZ_WORD		(2<<29)
-#define S3C64XX_SPI_MODE_CH_TSZ_MASK		(3<<29)
-#define S3C64XX_SPI_MODE_BUS_TSZ_BYTE		(0<<17)
-#define S3C64XX_SPI_MODE_BUS_TSZ_HALFWORD	(1<<17)
-#define S3C64XX_SPI_MODE_BUS_TSZ_WORD		(2<<17)
-#define S3C64XX_SPI_MODE_BUS_TSZ_MASK		(3<<17)
+#define S3C64XX_SPI_CH_CFG			0x00
+#define S3C64XX_SPI_CLK_CFG			0x04
+#define S3C64XX_SPI_MODE_CFG			0x08
+#define S3C64XX_SPI_CS_REG			0x0C
+#define S3C64XX_SPI_INT_EN			0x10
+#define S3C64XX_SPI_STATUS			0x14
+#define S3C64XX_SPI_TX_DATA			0x18
+#define S3C64XX_SPI_RX_DATA			0x1C
+#define S3C64XX_SPI_PACKET_CNT			0x20
+#define S3C64XX_SPI_PENDING_CLR			0x24
+#define S3C64XX_SPI_SWAP_CFG			0x28
+#define S3C64XX_SPI_FB_CLK			0x2C
+
+#define S3C64XX_SPI_CH_HS_EN			BIT(6)	/* High Speed Enable */
+#define S3C64XX_SPI_CH_SW_RST			BIT(5)
+#define S3C64XX_SPI_CH_SLAVE			BIT(4)
+#define S3C64XX_SPI_CPOL_L			BIT(3)
+#define S3C64XX_SPI_CPHA_B			BIT(2)
+#define S3C64XX_SPI_CH_RXCH_ON			BIT(1)
+#define S3C64XX_SPI_CH_TXCH_ON			BIT(0)
+
+#define S3C64XX_SPI_CLKSEL_SRCMSK		GENMASK(10, 9)
+#define S3C64XX_SPI_ENCLK_ENABLE		BIT(8)
+#define S3C64XX_SPI_PSR_MASK			GENMASK(7, 0)
+
+#define S3C64XX_SPI_MODE_CH_TSZ_MASK		GENMASK(30, 29)
+#define S3C64XX_SPI_MODE_CH_TSZ_BYTE		0
+#define S3C64XX_SPI_MODE_CH_TSZ_HALFWORD	1
+#define S3C64XX_SPI_MODE_CH_TSZ_WORD		2
+#define S3C64XX_SPI_MAX_TRAILCNT_MASK		GENMASK(28, 19)
+#define S3C64XX_SPI_MODE_BUS_TSZ_MASK		GENMASK(18, 17)
+#define S3C64XX_SPI_MODE_BUS_TSZ_BYTE		0
+#define S3C64XX_SPI_MODE_BUS_TSZ_HALFWORD	1
+#define S3C64XX_SPI_MODE_BUS_TSZ_WORD		2
 #define S3C64XX_SPI_MODE_RX_RDY_LVL		GENMASK(16, 11)
-#define S3C64XX_SPI_MODE_RX_RDY_LVL_SHIFT	11
-#define S3C64XX_SPI_MODE_SELF_LOOPBACK		(1<<3)
-#define S3C64XX_SPI_MODE_RXDMA_ON		(1<<2)
-#define S3C64XX_SPI_MODE_TXDMA_ON		(1<<1)
-#define S3C64XX_SPI_MODE_4BURST			(1<<0)
-
-#define S3C64XX_SPI_CS_NSC_CNT_2		(2<<4)
-#define S3C64XX_SPI_CS_AUTO			(1<<1)
-#define S3C64XX_SPI_CS_SIG_INACT		(1<<0)
-
-#define S3C64XX_SPI_INT_TRAILING_EN		(1<<6)
-#define S3C64XX_SPI_INT_RX_OVERRUN_EN		(1<<5)
-#define S3C64XX_SPI_INT_RX_UNDERRUN_EN		(1<<4)
-#define S3C64XX_SPI_INT_TX_OVERRUN_EN		(1<<3)
-#define S3C64XX_SPI_INT_TX_UNDERRUN_EN		(1<<2)
-#define S3C64XX_SPI_INT_RX_FIFORDY_EN		(1<<1)
-#define S3C64XX_SPI_INT_TX_FIFORDY_EN		(1<<0)
-
-#define S3C64XX_SPI_ST_RX_OVERRUN_ERR		(1<<5)
-#define S3C64XX_SPI_ST_RX_UNDERRUN_ERR		(1<<4)
-#define S3C64XX_SPI_ST_TX_OVERRUN_ERR		(1<<3)
-#define S3C64XX_SPI_ST_TX_UNDERRUN_ERR		(1<<2)
-#define S3C64XX_SPI_ST_RX_FIFORDY		(1<<1)
-#define S3C64XX_SPI_ST_TX_FIFORDY		(1<<0)
-
-#define S3C64XX_SPI_PACKET_CNT_EN		(1<<16)
+#define S3C64XX_SPI_MODE_SELF_LOOPBACK		BIT(3)
+#define S3C64XX_SPI_MODE_RXDMA_ON		BIT(2)
+#define S3C64XX_SPI_MODE_TXDMA_ON		BIT(1)
+#define S3C64XX_SPI_MODE_4BURST			BIT(0)
+
+/*
+ * S3C64XX_SPI_CS_NSC_CNT_2 is a value into the NCS_TIME_COUNT field. In newer
+ * datasheets this field is defined as GENMASK(9, 4). We don't know if this mask
+ * applies to all the versions of the IP, thus we can't yet define
+ * S3C64XX_SPI_CS_NSC_CNT_2 as a value and the register field as a mask.
+ */
+#define S3C64XX_SPI_CS_NSC_CNT_2		(2 << 4)
+#define S3C64XX_SPI_CS_AUTO			BIT(1)
+#define S3C64XX_SPI_CS_SIG_INACT		BIT(0)
+
+#define S3C64XX_SPI_INT_TRAILING_EN		BIT(6)
+#define S3C64XX_SPI_INT_RX_OVERRUN_EN		BIT(5)
+#define S3C64XX_SPI_INT_RX_UNDERRUN_EN		BIT(4)
+#define S3C64XX_SPI_INT_TX_OVERRUN_EN		BIT(3)
+#define S3C64XX_SPI_INT_TX_UNDERRUN_EN		BIT(2)
+#define S3C64XX_SPI_INT_RX_FIFORDY_EN		BIT(1)
+#define S3C64XX_SPI_INT_TX_FIFORDY_EN		BIT(0)
+
+#define S3C64XX_SPI_ST_RX_OVERRUN_ERR		BIT(5)
+#define S3C64XX_SPI_ST_RX_UNDERRUN_ERR		BIT(4)
+#define S3C64XX_SPI_ST_TX_OVERRUN_ERR		BIT(3)
+#define S3C64XX_SPI_ST_TX_UNDERRUN_ERR		BIT(2)
+#define S3C64XX_SPI_ST_RX_FIFORDY		BIT(1)
+#define S3C64XX_SPI_ST_TX_FIFORDY		BIT(0)
+
+#define S3C64XX_SPI_PACKET_CNT_EN		BIT(16)
 #define S3C64XX_SPI_PACKET_CNT_MASK		GENMASK(15, 0)
 
-#define S3C64XX_SPI_PND_TX_UNDERRUN_CLR		(1<<4)
-#define S3C64XX_SPI_PND_TX_OVERRUN_CLR		(1<<3)
-#define S3C64XX_SPI_PND_RX_UNDERRUN_CLR		(1<<2)
-#define S3C64XX_SPI_PND_RX_OVERRUN_CLR		(1<<1)
-#define S3C64XX_SPI_PND_TRAILING_CLR		(1<<0)
+#define S3C64XX_SPI_PND_TX_UNDERRUN_CLR		BIT(4)
+#define S3C64XX_SPI_PND_TX_OVERRUN_CLR		BIT(3)
+#define S3C64XX_SPI_PND_RX_UNDERRUN_CLR		BIT(2)
+#define S3C64XX_SPI_PND_RX_OVERRUN_CLR		BIT(1)
+#define S3C64XX_SPI_PND_TRAILING_CLR		BIT(0)
 
-#define S3C64XX_SPI_SWAP_RX_HALF_WORD		(1<<7)
-#define S3C64XX_SPI_SWAP_RX_BYTE		(1<<6)
-#define S3C64XX_SPI_SWAP_RX_BIT			(1<<5)
-#define S3C64XX_SPI_SWAP_RX_EN			(1<<4)
-#define S3C64XX_SPI_SWAP_TX_HALF_WORD		(1<<3)
-#define S3C64XX_SPI_SWAP_TX_BYTE		(1<<2)
-#define S3C64XX_SPI_SWAP_TX_BIT			(1<<1)
-#define S3C64XX_SPI_SWAP_TX_EN			(1<<0)
+#define S3C64XX_SPI_SWAP_RX_HALF_WORD		BIT(7)
+#define S3C64XX_SPI_SWAP_RX_BYTE		BIT(6)
+#define S3C64XX_SPI_SWAP_RX_BIT			BIT(5)
+#define S3C64XX_SPI_SWAP_RX_EN			BIT(4)
+#define S3C64XX_SPI_SWAP_TX_HALF_WORD		BIT(3)
+#define S3C64XX_SPI_SWAP_TX_BYTE		BIT(2)
+#define S3C64XX_SPI_SWAP_TX_BIT			BIT(1)
+#define S3C64XX_SPI_SWAP_TX_EN			BIT(0)
 
-#define S3C64XX_SPI_FBCLK_MSK			(3<<0)
+#define S3C64XX_SPI_FBCLK_MASK			GENMASK(1, 0)
 
 #define FIFO_LVL_MASK(i) ((i)->port_conf->fifo_lvl_mask[i->port_id])
 #define S3C64XX_SPI_ST_TX_DONE(v, i) (((v) & \
@@ -112,16 +118,13 @@
 					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
-
 #define S3C64XX_SPI_POLLING_SIZE	32
 
 #define msecs_to_loops(t) (loops_per_jiffy / 1000 * HZ * t)
 #define is_polling(x)	(x->cntrlr_info->polling)
 
-#define RXBUSY    (1<<2)
-#define TXBUSY    (1<<3)
+#define RXBUSY    BIT(2)
+#define TXBUSY    BIT(3)
 
 struct s3c64xx_spi_dma_data {
 	struct dma_chan *ch;
@@ -664,16 +667,22 @@ static int s3c64xx_spi_config(struct s3c64xx_spi_driver_data *sdd)
 
 	switch (sdd->cur_bpw) {
 	case 32:
-		val |= S3C64XX_SPI_MODE_BUS_TSZ_WORD;
-		val |= S3C64XX_SPI_MODE_CH_TSZ_WORD;
+		val |= FIELD_PREP(S3C64XX_SPI_MODE_BUS_TSZ_MASK,
+				  S3C64XX_SPI_MODE_BUS_TSZ_WORD) |
+		       FIELD_PREP(S3C64XX_SPI_MODE_CH_TSZ_MASK,
+				  S3C64XX_SPI_MODE_CH_TSZ_WORD);
 		break;
 	case 16:
-		val |= S3C64XX_SPI_MODE_BUS_TSZ_HALFWORD;
-		val |= S3C64XX_SPI_MODE_CH_TSZ_HALFWORD;
+		val |= FIELD_PREP(S3C64XX_SPI_MODE_BUS_TSZ_MASK,
+				  S3C64XX_SPI_MODE_BUS_TSZ_HALFWORD) |
+		       FIELD_PREP(S3C64XX_SPI_MODE_CH_TSZ_MASK,
+				  S3C64XX_SPI_MODE_CH_TSZ_HALFWORD);
 		break;
 	default:
-		val |= S3C64XX_SPI_MODE_BUS_TSZ_BYTE;
-		val |= S3C64XX_SPI_MODE_CH_TSZ_BYTE;
+		val |= FIELD_PREP(S3C64XX_SPI_MODE_BUS_TSZ_MASK,
+				  S3C64XX_SPI_MODE_BUS_TSZ_BYTE) |
+		       FIELD_PREP(S3C64XX_SPI_MODE_CH_TSZ_MASK,
+				  S3C64XX_SPI_MODE_CH_TSZ_BYTE);
 		break;
 	}
 
@@ -799,7 +808,7 @@ static int s3c64xx_spi_transfer_one(struct spi_controller *host,
 
 			val = readl(sdd->regs + S3C64XX_SPI_MODE_CFG);
 			val &= ~S3C64XX_SPI_MODE_RX_RDY_LVL;
-			val |= (rdy_lv << S3C64XX_SPI_MODE_RX_RDY_LVL_SHIFT);
+			val |= FIELD_PREP(S3C64XX_SPI_MODE_RX_RDY_LVL, rdy_lv);
 			writel(val, sdd->regs + S3C64XX_SPI_MODE_CFG);
 
 			/* Enable FIFO_RDY_EN IRQ */
@@ -1072,8 +1081,8 @@ static void s3c64xx_spi_hwinit(struct s3c64xx_spi_driver_data *sdd)
 	writel(0, regs + S3C64XX_SPI_INT_EN);
 
 	if (!sdd->port_conf->clk_from_cmu)
-		writel(sci->src_clk_nr << S3C64XX_SPI_CLKSEL_SRCSHFT,
-				regs + S3C64XX_SPI_CLK_CFG);
+		writel(FIELD_PREP(S3C64XX_SPI_CLKSEL_SRCMSK, sci->src_clk_nr),
+		       regs + S3C64XX_SPI_CLK_CFG);
 	writel(0, regs + S3C64XX_SPI_MODE_CFG);
 	writel(0, regs + S3C64XX_SPI_PACKET_CNT);
 
@@ -1089,7 +1098,7 @@ static void s3c64xx_spi_hwinit(struct s3c64xx_spi_driver_data *sdd)
 
 	val = readl(regs + S3C64XX_SPI_MODE_CFG);
 	val &= ~S3C64XX_SPI_MODE_4BURST;
-	val |= (S3C64XX_SPI_MAX_TRAILCNT << S3C64XX_SPI_TRAILCNT_OFF);
+	val |= S3C64XX_SPI_MAX_TRAILCNT_MASK;
 	writel(val, regs + S3C64XX_SPI_MODE_CFG);
 
 	s3c64xx_flush_fifo(sdd);
-- 
2.43.0.429.g432eaa2c6b-goog

Re: [PATCH v3 15/17] spi: s3c64xx: remove duplicated definition

[Sam Protsenko]

On Fri, Jan 26, 2024 at 11:16 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
> S3C64XX_SPI_TRAILCNT brings no benefit in terms of name over
> S3C64XX_SPI_MAX_TRAILCNT. Remove the duplicated definition.
>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> ---

Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>

>  drivers/spi/spi-s3c64xx.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> index dc779d5305a5..e9344fe71e56 100644
> --- a/drivers/spi/spi-s3c64xx.c
> +++ b/drivers/spi/spi-s3c64xx.c
> @@ -115,8 +115,6 @@
>  #define S3C64XX_SPI_MAX_TRAILCNT       0x3ff
>  #define S3C64XX_SPI_TRAILCNT_OFF       19
>
> -#define S3C64XX_SPI_TRAILCNT           S3C64XX_SPI_MAX_TRAILCNT
> -
>  #define S3C64XX_SPI_POLLING_SIZE       32
>
>  #define msecs_to_loops(t) (loops_per_jiffy / 1000 * HZ * t)
> @@ -1092,7 +1090,7 @@ static void s3c64xx_spi_hwinit(struct s3c64xx_spi_driver_data *sdd)
>         val = readl(regs + S3C64XX_SPI_MODE_CFG);
>         val &= ~S3C64XX_SPI_MODE_4BURST;
>         val &= ~(S3C64XX_SPI_MAX_TRAILCNT << S3C64XX_SPI_TRAILCNT_OFF);
> -       val |= (S3C64XX_SPI_TRAILCNT << S3C64XX_SPI_TRAILCNT_OFF);
> +       val |= (S3C64XX_SPI_MAX_TRAILCNT << S3C64XX_SPI_TRAILCNT_OFF);
>         writel(val, regs + S3C64XX_SPI_MODE_CFG);
>
>         s3c64xx_flush_fifo(sdd);
> --
> 2.43.0.429.g432eaa2c6b-goog
>

Re: [PATCH v3 16/17] spi: s3c64xx: drop a superfluous bitwise NOT operation

[Sam Protsenko]

On Fri, Jan 26, 2024 at 11:16 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
> val &= ~mask;
> val |= mask;
>
> is equivalent to:
> val |= mask;
>
> Drop the superfluous bitwise NOT operation.
>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> ---

That's much more clear. Also I'm pretty sure if you compare .o file
before and after the patch, they would be identical -- another way to
argue the patch has no functional change.

Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>

>  drivers/spi/spi-s3c64xx.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> index e9344fe71e56..43b888c8812e 100644
> --- a/drivers/spi/spi-s3c64xx.c
> +++ b/drivers/spi/spi-s3c64xx.c
> @@ -1089,7 +1089,6 @@ static void s3c64xx_spi_hwinit(struct s3c64xx_spi_driver_data *sdd)
>
>         val = readl(regs + S3C64XX_SPI_MODE_CFG);
>         val &= ~S3C64XX_SPI_MODE_4BURST;
> -       val &= ~(S3C64XX_SPI_MAX_TRAILCNT << S3C64XX_SPI_TRAILCNT_OFF);
>         val |= (S3C64XX_SPI_MAX_TRAILCNT << S3C64XX_SPI_TRAILCNT_OFF);
>         writel(val, regs + S3C64XX_SPI_MODE_CFG);
>
> --
> 2.43.0.429.g432eaa2c6b-goog
>

Re: [PATCH v3 17/17] spi: s3c64xx: use bitfield access macros

[Sam Protsenko]

On Fri, Jan 26, 2024 at 11:16 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
> Use the bitfield access macros in order to clean and to make the driver
> easier to read. Introduce S3C64XX_SPI_MAX_TRAILCNT_MASK to replace value
> and offset equivalents (S3C64XX_SPI_MAX_TRAILCNT,
> S3C64XX_SPI_TRAILCNT_OFF). While touching the register definitions, align
> their values to the same offset.
>
> No functional change intended, the bit operations shall be equivalent.
>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> ---
>  drivers/spi/spi-s3c64xx.c | 193 ++++++++++++++++++++------------------
>  1 file changed, 101 insertions(+), 92 deletions(-)
>
> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> index 43b888c8812e..7f052d6cd2ba 100644
> --- a/drivers/spi/spi-s3c64xx.c
> +++ b/drivers/spi/spi-s3c64xx.c
> @@ -4,6 +4,7 @@
>  //      Jaswinder Singh <jassi.brar@samsung.com>
>
>  #include <linux/bits.h>
> +#include <linux/bitfield.h>
>  #include <linux/clk.h>
>  #include <linux/delay.h>
>  #include <linux/dma-mapping.h>
> @@ -18,91 +19,96 @@
>  #include <linux/pm_runtime.h>
>  #include <linux/spi/spi.h>
>
> -#define MAX_SPI_PORTS          12
> -#define S3C64XX_SPI_QUIRK_CS_AUTO      (1 << 1)
> -#define AUTOSUSPEND_TIMEOUT    2000
> +#define MAX_SPI_PORTS                          12
> +#define S3C64XX_SPI_QUIRK_CS_AUTO              BIT(1)
> +#define AUTOSUSPEND_TIMEOUT                    2000
>
>  /* Registers and bit-fields */
>
> -#define S3C64XX_SPI_CH_CFG             0x00
> -#define S3C64XX_SPI_CLK_CFG            0x04
> -#define S3C64XX_SPI_MODE_CFG           0x08
> -#define S3C64XX_SPI_CS_REG             0x0C
> -#define S3C64XX_SPI_INT_EN             0x10
> -#define S3C64XX_SPI_STATUS             0x14
> -#define S3C64XX_SPI_TX_DATA            0x18
> -#define S3C64XX_SPI_RX_DATA            0x1C
> -#define S3C64XX_SPI_PACKET_CNT         0x20
> -#define S3C64XX_SPI_PENDING_CLR                0x24
> -#define S3C64XX_SPI_SWAP_CFG           0x28
> -#define S3C64XX_SPI_FB_CLK             0x2C
> -
> -#define S3C64XX_SPI_CH_HS_EN           (1<<6)  /* High Speed Enable */
> -#define S3C64XX_SPI_CH_SW_RST          (1<<5)
> -#define S3C64XX_SPI_CH_SLAVE           (1<<4)
> -#define S3C64XX_SPI_CPOL_L             (1<<3)
> -#define S3C64XX_SPI_CPHA_B             (1<<2)
> -#define S3C64XX_SPI_CH_RXCH_ON         (1<<1)
> -#define S3C64XX_SPI_CH_TXCH_ON         (1<<0)
> -
> -#define S3C64XX_SPI_CLKSEL_SRCMSK      (3<<9)
> -#define S3C64XX_SPI_CLKSEL_SRCSHFT     9
> -#define S3C64XX_SPI_ENCLK_ENABLE       (1<<8)
> -#define S3C64XX_SPI_PSR_MASK           0xff
> -
> -#define S3C64XX_SPI_MODE_CH_TSZ_BYTE           (0<<29)
> -#define S3C64XX_SPI_MODE_CH_TSZ_HALFWORD       (1<<29)
> -#define S3C64XX_SPI_MODE_CH_TSZ_WORD           (2<<29)
> -#define S3C64XX_SPI_MODE_CH_TSZ_MASK           (3<<29)
> -#define S3C64XX_SPI_MODE_BUS_TSZ_BYTE          (0<<17)
> -#define S3C64XX_SPI_MODE_BUS_TSZ_HALFWORD      (1<<17)
> -#define S3C64XX_SPI_MODE_BUS_TSZ_WORD          (2<<17)
> -#define S3C64XX_SPI_MODE_BUS_TSZ_MASK          (3<<17)
> +#define S3C64XX_SPI_CH_CFG                     0x00
> +#define S3C64XX_SPI_CLK_CFG                    0x04
> +#define S3C64XX_SPI_MODE_CFG                   0x08
> +#define S3C64XX_SPI_CS_REG                     0x0C
> +#define S3C64XX_SPI_INT_EN                     0x10
> +#define S3C64XX_SPI_STATUS                     0x14
> +#define S3C64XX_SPI_TX_DATA                    0x18
> +#define S3C64XX_SPI_RX_DATA                    0x1C
> +#define S3C64XX_SPI_PACKET_CNT                 0x20
> +#define S3C64XX_SPI_PENDING_CLR                        0x24
> +#define S3C64XX_SPI_SWAP_CFG                   0x28
> +#define S3C64XX_SPI_FB_CLK                     0x2C
> +
> +#define S3C64XX_SPI_CH_HS_EN                   BIT(6)  /* High Speed Enable */
> +#define S3C64XX_SPI_CH_SW_RST                  BIT(5)
> +#define S3C64XX_SPI_CH_SLAVE                   BIT(4)
> +#define S3C64XX_SPI_CPOL_L                     BIT(3)
> +#define S3C64XX_SPI_CPHA_B                     BIT(2)
> +#define S3C64XX_SPI_CH_RXCH_ON                 BIT(1)
> +#define S3C64XX_SPI_CH_TXCH_ON                 BIT(0)
> +
> +#define S3C64XX_SPI_CLKSEL_SRCMSK              GENMASK(10, 9)
> +#define S3C64XX_SPI_ENCLK_ENABLE               BIT(8)
> +#define S3C64XX_SPI_PSR_MASK                   GENMASK(7, 0)
> +
> +#define S3C64XX_SPI_MODE_CH_TSZ_MASK           GENMASK(30, 29)
> +#define S3C64XX_SPI_MODE_CH_TSZ_BYTE           0
> +#define S3C64XX_SPI_MODE_CH_TSZ_HALFWORD       1
> +#define S3C64XX_SPI_MODE_CH_TSZ_WORD           2
> +#define S3C64XX_SPI_MAX_TRAILCNT_MASK          GENMASK(28, 19)
> +#define S3C64XX_SPI_MODE_BUS_TSZ_MASK          GENMASK(18, 17)
> +#define S3C64XX_SPI_MODE_BUS_TSZ_BYTE          0
> +#define S3C64XX_SPI_MODE_BUS_TSZ_HALFWORD      1
> +#define S3C64XX_SPI_MODE_BUS_TSZ_WORD          2
>  #define S3C64XX_SPI_MODE_RX_RDY_LVL            GENMASK(16, 11)
> -#define S3C64XX_SPI_MODE_RX_RDY_LVL_SHIFT      11
> -#define S3C64XX_SPI_MODE_SELF_LOOPBACK         (1<<3)
> -#define S3C64XX_SPI_MODE_RXDMA_ON              (1<<2)
> -#define S3C64XX_SPI_MODE_TXDMA_ON              (1<<1)
> -#define S3C64XX_SPI_MODE_4BURST                        (1<<0)
> -
> -#define S3C64XX_SPI_CS_NSC_CNT_2               (2<<4)
> -#define S3C64XX_SPI_CS_AUTO                    (1<<1)
> -#define S3C64XX_SPI_CS_SIG_INACT               (1<<0)
> -
> -#define S3C64XX_SPI_INT_TRAILING_EN            (1<<6)
> -#define S3C64XX_SPI_INT_RX_OVERRUN_EN          (1<<5)
> -#define S3C64XX_SPI_INT_RX_UNDERRUN_EN         (1<<4)
> -#define S3C64XX_SPI_INT_TX_OVERRUN_EN          (1<<3)
> -#define S3C64XX_SPI_INT_TX_UNDERRUN_EN         (1<<2)
> -#define S3C64XX_SPI_INT_RX_FIFORDY_EN          (1<<1)
> -#define S3C64XX_SPI_INT_TX_FIFORDY_EN          (1<<0)
> -
> -#define S3C64XX_SPI_ST_RX_OVERRUN_ERR          (1<<5)
> -#define S3C64XX_SPI_ST_RX_UNDERRUN_ERR         (1<<4)
> -#define S3C64XX_SPI_ST_TX_OVERRUN_ERR          (1<<3)
> -#define S3C64XX_SPI_ST_TX_UNDERRUN_ERR         (1<<2)
> -#define S3C64XX_SPI_ST_RX_FIFORDY              (1<<1)
> -#define S3C64XX_SPI_ST_TX_FIFORDY              (1<<0)
> -
> -#define S3C64XX_SPI_PACKET_CNT_EN              (1<<16)
> +#define S3C64XX_SPI_MODE_SELF_LOOPBACK         BIT(3)
> +#define S3C64XX_SPI_MODE_RXDMA_ON              BIT(2)
> +#define S3C64XX_SPI_MODE_TXDMA_ON              BIT(1)
> +#define S3C64XX_SPI_MODE_4BURST                        BIT(0)
> +
> +/*
> + * S3C64XX_SPI_CS_NSC_CNT_2 is a value into the NCS_TIME_COUNT field. In newer
> + * datasheets this field is defined as GENMASK(9, 4). We don't know if this mask
> + * applies to all the versions of the IP, thus we can't yet define
> + * S3C64XX_SPI_CS_NSC_CNT_2 as a value and the register field as a mask.
> + */
> +#define S3C64XX_SPI_CS_NSC_CNT_2               (2 << 4)
> +#define S3C64XX_SPI_CS_AUTO                    BIT(1)
> +#define S3C64XX_SPI_CS_SIG_INACT               BIT(0)
> +
> +#define S3C64XX_SPI_INT_TRAILING_EN            BIT(6)
> +#define S3C64XX_SPI_INT_RX_OVERRUN_EN          BIT(5)
> +#define S3C64XX_SPI_INT_RX_UNDERRUN_EN         BIT(4)
> +#define S3C64XX_SPI_INT_TX_OVERRUN_EN          BIT(3)
> +#define S3C64XX_SPI_INT_TX_UNDERRUN_EN         BIT(2)
> +#define S3C64XX_SPI_INT_RX_FIFORDY_EN          BIT(1)
> +#define S3C64XX_SPI_INT_TX_FIFORDY_EN          BIT(0)
> +
> +#define S3C64XX_SPI_ST_RX_OVERRUN_ERR          BIT(5)
> +#define S3C64XX_SPI_ST_RX_UNDERRUN_ERR         BIT(4)
> +#define S3C64XX_SPI_ST_TX_OVERRUN_ERR          BIT(3)
> +#define S3C64XX_SPI_ST_TX_UNDERRUN_ERR         BIT(2)
> +#define S3C64XX_SPI_ST_RX_FIFORDY              BIT(1)
> +#define S3C64XX_SPI_ST_TX_FIFORDY              BIT(0)
> +
> +#define S3C64XX_SPI_PACKET_CNT_EN              BIT(16)
>  #define S3C64XX_SPI_PACKET_CNT_MASK            GENMASK(15, 0)
>
> -#define S3C64XX_SPI_PND_TX_UNDERRUN_CLR                (1<<4)
> -#define S3C64XX_SPI_PND_TX_OVERRUN_CLR         (1<<3)
> -#define S3C64XX_SPI_PND_RX_UNDERRUN_CLR                (1<<2)
> -#define S3C64XX_SPI_PND_RX_OVERRUN_CLR         (1<<1)
> -#define S3C64XX_SPI_PND_TRAILING_CLR           (1<<0)
> +#define S3C64XX_SPI_PND_TX_UNDERRUN_CLR                BIT(4)
> +#define S3C64XX_SPI_PND_TX_OVERRUN_CLR         BIT(3)
> +#define S3C64XX_SPI_PND_RX_UNDERRUN_CLR                BIT(2)
> +#define S3C64XX_SPI_PND_RX_OVERRUN_CLR         BIT(1)
> +#define S3C64XX_SPI_PND_TRAILING_CLR           BIT(0)
>
> -#define S3C64XX_SPI_SWAP_RX_HALF_WORD          (1<<7)
> -#define S3C64XX_SPI_SWAP_RX_BYTE               (1<<6)
> -#define S3C64XX_SPI_SWAP_RX_BIT                        (1<<5)
> -#define S3C64XX_SPI_SWAP_RX_EN                 (1<<4)
> -#define S3C64XX_SPI_SWAP_TX_HALF_WORD          (1<<3)
> -#define S3C64XX_SPI_SWAP_TX_BYTE               (1<<2)
> -#define S3C64XX_SPI_SWAP_TX_BIT                        (1<<1)
> -#define S3C64XX_SPI_SWAP_TX_EN                 (1<<0)
> +#define S3C64XX_SPI_SWAP_RX_HALF_WORD          BIT(7)
> +#define S3C64XX_SPI_SWAP_RX_BYTE               BIT(6)
> +#define S3C64XX_SPI_SWAP_RX_BIT                        BIT(5)
> +#define S3C64XX_SPI_SWAP_RX_EN                 BIT(4)
> +#define S3C64XX_SPI_SWAP_TX_HALF_WORD          BIT(3)
> +#define S3C64XX_SPI_SWAP_TX_BYTE               BIT(2)
> +#define S3C64XX_SPI_SWAP_TX_BIT                        BIT(1)
> +#define S3C64XX_SPI_SWAP_TX_EN                 BIT(0)
>
> -#define S3C64XX_SPI_FBCLK_MSK                  (3<<0)
> +#define S3C64XX_SPI_FBCLK_MASK                 GENMASK(1, 0)
>
>  #define FIFO_LVL_MASK(i) ((i)->port_conf->fifo_lvl_mask[i->port_id])
>  #define S3C64XX_SPI_ST_TX_DONE(v, i) (((v) & \
> @@ -112,16 +118,13 @@
>                                         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
> -
>  #define S3C64XX_SPI_POLLING_SIZE       32
>
>  #define msecs_to_loops(t) (loops_per_jiffy / 1000 * HZ * t)
>  #define is_polling(x)  (x->cntrlr_info->polling)
>
> -#define RXBUSY    (1<<2)
> -#define TXBUSY    (1<<3)
> +#define RXBUSY    BIT(2)
> +#define TXBUSY    BIT(3)
>
>  struct s3c64xx_spi_dma_data {
>         struct dma_chan *ch;
> @@ -664,16 +667,22 @@ static int s3c64xx_spi_config(struct s3c64xx_spi_driver_data *sdd)
>
>         switch (sdd->cur_bpw) {
>         case 32:
> -               val |= S3C64XX_SPI_MODE_BUS_TSZ_WORD;
> -               val |= S3C64XX_SPI_MODE_CH_TSZ_WORD;
> +               val |= FIELD_PREP(S3C64XX_SPI_MODE_BUS_TSZ_MASK,
> +                                 S3C64XX_SPI_MODE_BUS_TSZ_WORD) |
> +                      FIELD_PREP(S3C64XX_SPI_MODE_CH_TSZ_MASK,
> +                                 S3C64XX_SPI_MODE_CH_TSZ_WORD);
>                 break;
>         case 16:
> -               val |= S3C64XX_SPI_MODE_BUS_TSZ_HALFWORD;
> -               val |= S3C64XX_SPI_MODE_CH_TSZ_HALFWORD;
> +               val |= FIELD_PREP(S3C64XX_SPI_MODE_BUS_TSZ_MASK,
> +                                 S3C64XX_SPI_MODE_BUS_TSZ_HALFWORD) |
> +                      FIELD_PREP(S3C64XX_SPI_MODE_CH_TSZ_MASK,
> +                                 S3C64XX_SPI_MODE_CH_TSZ_HALFWORD);

Two people complained it makes the code harder to read. Yet it's not
addressed in v3. Please see my comments for your previous submission
explaining what can be done, and also Andi's comment on that matter.
Also I think new patch series are being submitted a bit too fast,
people might not have enough time to provide the review.

>                 break;
>         default:
> -               val |= S3C64XX_SPI_MODE_BUS_TSZ_BYTE;
> -               val |= S3C64XX_SPI_MODE_CH_TSZ_BYTE;
> +               val |= FIELD_PREP(S3C64XX_SPI_MODE_BUS_TSZ_MASK,
> +                                 S3C64XX_SPI_MODE_BUS_TSZ_BYTE) |
> +                      FIELD_PREP(S3C64XX_SPI_MODE_CH_TSZ_MASK,
> +                                 S3C64XX_SPI_MODE_CH_TSZ_BYTE);
>                 break;
>         }
>
> @@ -799,7 +808,7 @@ static int s3c64xx_spi_transfer_one(struct spi_controller *host,
>
>                         val = readl(sdd->regs + S3C64XX_SPI_MODE_CFG);
>                         val &= ~S3C64XX_SPI_MODE_RX_RDY_LVL;
> -                       val |= (rdy_lv << S3C64XX_SPI_MODE_RX_RDY_LVL_SHIFT);
> +                       val |= FIELD_PREP(S3C64XX_SPI_MODE_RX_RDY_LVL, rdy_lv);
>                         writel(val, sdd->regs + S3C64XX_SPI_MODE_CFG);
>
>                         /* Enable FIFO_RDY_EN IRQ */
> @@ -1072,8 +1081,8 @@ static void s3c64xx_spi_hwinit(struct s3c64xx_spi_driver_data *sdd)
>         writel(0, regs + S3C64XX_SPI_INT_EN);
>
>         if (!sdd->port_conf->clk_from_cmu)
> -               writel(sci->src_clk_nr << S3C64XX_SPI_CLKSEL_SRCSHFT,
> -                               regs + S3C64XX_SPI_CLK_CFG);
> +               writel(FIELD_PREP(S3C64XX_SPI_CLKSEL_SRCMSK, sci->src_clk_nr),
> +                      regs + S3C64XX_SPI_CLK_CFG);
>         writel(0, regs + S3C64XX_SPI_MODE_CFG);
>         writel(0, regs + S3C64XX_SPI_PACKET_CNT);
>
> @@ -1089,7 +1098,7 @@ static void s3c64xx_spi_hwinit(struct s3c64xx_spi_driver_data *sdd)
>
>         val = readl(regs + S3C64XX_SPI_MODE_CFG);
>         val &= ~S3C64XX_SPI_MODE_4BURST;
> -       val |= (S3C64XX_SPI_MAX_TRAILCNT << S3C64XX_SPI_TRAILCNT_OFF);
> +       val |= S3C64XX_SPI_MAX_TRAILCNT_MASK;
>         writel(val, regs + S3C64XX_SPI_MODE_CFG);
>
>         s3c64xx_flush_fifo(sdd);
> --
> 2.43.0.429.g432eaa2c6b-goog
>

Re: [PATCH v3 12/17] spi: s3c64xx: return ETIMEDOUT for wait_for_completion_timeout()

[Sam Protsenko]

On Fri, Jan 26, 2024 at 11:16 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
> ETIMEDOUT is more specific than EIO, use it for
> wait_for_completion_timeout().
>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> ---

Looks like you missed my R-b tag I added to this patch in your
previous submission.

>  drivers/spi/spi-s3c64xx.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> index 76fa378ab5ab..2f2c4ad35df4 100644
> --- a/drivers/spi/spi-s3c64xx.c
> +++ b/drivers/spi/spi-s3c64xx.c
> @@ -526,7 +526,7 @@ static int s3c64xx_wait_for_dma(struct s3c64xx_spi_driver_data *sdd,
>
>         /*
>          * If the previous xfer was completed within timeout, then
> -        * proceed further else return -EIO.
> +        * proceed further else return -ETIMEDOUT.
>          * DmaTx returns after simply writing data in the FIFO,
>          * w/o waiting for real transmission on the bus to finish.
>          * DmaRx returns only after Dma read data from FIFO which
> @@ -547,7 +547,7 @@ static int s3c64xx_wait_for_dma(struct s3c64xx_spi_driver_data *sdd,
>
>         /* If timed out while checking rx/tx status return error */
>         if (!val)
> -               return -EIO;
> +               return -ETIMEDOUT;
>
>         return 0;
>  }
> @@ -577,7 +577,7 @@ static int s3c64xx_wait_for_pio(struct s3c64xx_spi_driver_data *sdd,
>         if (use_irq) {
>                 val = msecs_to_jiffies(ms);
>                 if (!wait_for_completion_timeout(&sdd->xfer_completion, val))
> -                       return -EIO;
> +                       return -ETIMEDOUT;
>         }
>
>         val = msecs_to_loops(ms);
> --
> 2.43.0.429.g432eaa2c6b-goog
>

Re: [PATCH v3 06/17] spi: s3c64xx: remove unneeded (void *) casts in of_match_table

[Sam Protsenko]

On Fri, Jan 26, 2024 at 11:15 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
> of_device_id::data is an opaque pointer. No explicit cast is needed.
> Remove unneeded (void *) casts in of_match_table. While here align the
> compatible and data members.
>
> Reviewed-by: Andi Shyti <andi.shyti@kernel.org>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> ---
>  drivers/spi/spi-s3c64xx.c | 45 +++++++++++++++++++++++----------------
>  1 file changed, 27 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> index ccb700312d64..9bf54c1044b3 100644
> --- a/drivers/spi/spi-s3c64xx.c
> +++ b/drivers/spi/spi-s3c64xx.c
> @@ -1511,32 +1511,41 @@ static const struct platform_device_id s3c64xx_spi_driver_ids[] = {
>  };
>
>  static const struct of_device_id s3c64xx_spi_dt_match[] = {
> -       { .compatible = "samsung,s3c2443-spi",
> -                       .data = (void *)&s3c2443_spi_port_config,
> +       {
> +               .compatible = "samsung,s3c2443-spi",
> +               .data = &s3c2443_spi_port_config,
>         },
> -       { .compatible = "samsung,s3c6410-spi",
> -                       .data = (void *)&s3c6410_spi_port_config,
> +       {

The braces style is not fixed. Yet that's a style patch, which on one
hand fixes the issue (unnecessary void * cast), but OTOH brings
another issue (non-canonical braces placement). Please see my comments
for your previous submission.

> +               .compatible = "samsung,s3c6410-spi",
> +               .data = &s3c6410_spi_port_config,
>         },
> -       { .compatible = "samsung,s5pv210-spi",
> -                       .data = (void *)&s5pv210_spi_port_config,
> +       {
> +               .compatible = "samsung,s5pv210-spi",
> +               .data = &s5pv210_spi_port_config,
>         },
> -       { .compatible = "samsung,exynos4210-spi",
> -                       .data = (void *)&exynos4_spi_port_config,
> +       {
> +               .compatible = "samsung,exynos4210-spi",
> +               .data = &exynos4_spi_port_config,
>         },
> -       { .compatible = "samsung,exynos7-spi",
> -                       .data = (void *)&exynos7_spi_port_config,
> +       {
> +               .compatible = "samsung,exynos7-spi",
> +               .data = &exynos7_spi_port_config,
>         },
> -       { .compatible = "samsung,exynos5433-spi",
> -                       .data = (void *)&exynos5433_spi_port_config,
> +       {
> +               .compatible = "samsung,exynos5433-spi",
> +               .data = &exynos5433_spi_port_config,
>         },
> -       { .compatible = "samsung,exynos850-spi",
> -                       .data = (void *)&exynos850_spi_port_config,
> +       {
> +               .compatible = "samsung,exynos850-spi",
> +               .data = &exynos850_spi_port_config,
>         },
> -       { .compatible = "samsung,exynosautov9-spi",
> -                       .data = (void *)&exynosautov9_spi_port_config,
> +       {
> +               .compatible = "samsung,exynosautov9-spi",
> +               .data = &exynosautov9_spi_port_config,
>         },
> -       { .compatible = "tesla,fsd-spi",
> -                       .data = (void *)&fsd_spi_port_config,
> +       {
> +               .compatible = "tesla,fsd-spi",
> +               .data = &fsd_spi_port_config,
>         },
>         { },
>  };
> --
> 2.43.0.429.g432eaa2c6b-goog
>

Re: [PATCH v3 17/17] spi: s3c64xx: use bitfield access macros

[Tudor Ambarus]

Hi, Sam,

Thanks for the review feedback!

On 1/26/24 20:12, Sam Protsenko wrote:
>> -               val |= S3C64XX_SPI_MODE_BUS_TSZ_HALFWORD;
>> -               val |= S3C64XX_SPI_MODE_CH_TSZ_HALFWORD;
>> +               val |= FIELD_PREP(S3C64XX_SPI_MODE_BUS_TSZ_MASK,
>> +                                 S3C64XX_SPI_MODE_BUS_TSZ_HALFWORD) |
>> +                      FIELD_PREP(S3C64XX_SPI_MODE_CH_TSZ_MASK,
>> +                                 S3C64XX_SPI_MODE_CH_TSZ_HALFWORD);
> Two people complained it makes the code harder to read. Yet it's not
> addressed in v3. Please see my comments for your previous submission
> explaining what can be done, and also Andi's comment on that matter.

I kept these intentionally. Please read my reply on that matter or the
cover letter to this patch set.

> Also I think new patch series are being submitted a bit too fast,
> people might not have enough time to provide the review.

This patch set contains patches that are already reviewed or too simple
to being lagged.

Cheers,
ta

Re: [PATCH v3 12/17] spi: s3c64xx: return ETIMEDOUT for wait_for_completion_timeout()

[Tudor Ambarus]

On 1/26/24 20:13, Sam Protsenko wrote:
>> ETIMEDOUT is more specific than EIO, use it for
>> wait_for_completion_timeout().
>>
>> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
>> ---
> Looks like you missed my R-b tag I added to this patch in your
> previous submission.

My apologies. Adding it here should do the trick, here it is:

Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>

Re: [PATCH v3 17/17] spi: s3c64xx: use bitfield access macros

[Sam Protsenko]

On Fri, Jan 26, 2024 at 9:23 PM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
> Hi, Sam,
>
> Thanks for the review feedback!
>
> On 1/26/24 20:12, Sam Protsenko wrote:
> >> -               val |= S3C64XX_SPI_MODE_BUS_TSZ_HALFWORD;
> >> -               val |= S3C64XX_SPI_MODE_CH_TSZ_HALFWORD;
> >> +               val |= FIELD_PREP(S3C64XX_SPI_MODE_BUS_TSZ_MASK,
> >> +                                 S3C64XX_SPI_MODE_BUS_TSZ_HALFWORD) |
> >> +                      FIELD_PREP(S3C64XX_SPI_MODE_CH_TSZ_MASK,
> >> +                                 S3C64XX_SPI_MODE_CH_TSZ_HALFWORD);
> > Two people complained it makes the code harder to read. Yet it's not
> > addressed in v3. Please see my comments for your previous submission
> > explaining what can be done, and also Andi's comment on that matter.
>
> I kept these intentionally. Please read my reply on that matter or the
> cover letter to this patch set.
>

I read it. But still don't like it :) I'm sure it's possible to do
this modification, but at the same time keep the code clean an easy to
read. The code above -- I don't like at all, sorry. It was much better
before this patch, IMHO.

> > Also I think new patch series are being submitted a bit too fast,
> > people might not have enough time to provide the review.
>
> This patch set contains patches that are already reviewed or too simple
> to being lagged.
>

Ok, agreed.

> Cheers,
> ta

Re: [PATCH v3 06/17] spi: s3c64xx: remove unneeded (void *) casts in of_match_table

[Tudor Ambarus]

Hi, Sam,

Thanks for the review feedback.

On 1/26/24 20:17, Sam Protsenko wrote:
>>  static const struct of_device_id s3c64xx_spi_dt_match[] = {
>> -       { .compatible = "samsung,s3c2443-spi",
>> -                       .data = (void *)&s3c2443_spi_port_config,
>> +       {
>> +               .compatible = "samsung,s3c2443-spi",
>> +               .data = &s3c2443_spi_port_config,
>>         },
>> -       { .compatible = "samsung,s3c6410-spi",
>> -                       .data = (void *)&s3c6410_spi_port_config,
>> +       {
> The braces style is not fixed. Yet that's a style patch, which on one
> hand fixes the issue (unnecessary void * cast), but OTOH brings
> another issue (non-canonical braces placement). Please see my comments
> for your previous submission.

I've read your email and replied there that the braces were one on top
of the other even before my patch and since I don't have a preference on
whether to choose a style or the other, I kept the style as it were.

But I learnt my lesson. Mark can ignore this path when applying and I'll
submit a new version of the patch where I'll refrain myself making
alignments or changing the style.

Cheers,
ta

Re: [PATCH v3 17/17] spi: s3c64xx: use bitfield access macros

[Tudor Ambarus]

On 1/27/24 03:38, Sam Protsenko wrote:
>>>> -               val |= S3C64XX_SPI_MODE_BUS_TSZ_HALFWORD;
>>>> -               val |= S3C64XX_SPI_MODE_CH_TSZ_HALFWORD;
>>>> +               val |= FIELD_PREP(S3C64XX_SPI_MODE_BUS_TSZ_MASK,
>>>> +                                 S3C64XX_SPI_MODE_BUS_TSZ_HALFWORD) |
>>>> +                      FIELD_PREP(S3C64XX_SPI_MODE_CH_TSZ_MASK,
>>>> +                                 S3C64XX_SPI_MODE_CH_TSZ_HALFWORD);
>>> Two people complained it makes the code harder to read. Yet it's not
>>> addressed in v3. Please see my comments for your previous submission
>>> explaining what can be done, and also Andi's comment on that matter.
>> I kept these intentionally. Please read my reply on that matter or the
>> cover letter to this patch set.
>>
> I read it. But still don't like it 🙂 I'm sure it's possible to do
> this modification, but at the same time keep the code clean an easy to
> read. The code above -- I don't like at all, sorry. It was much better
> before this patch, IMHO.

Yeah, I guess Mark will tip the scale.

Cheers,
ta

Re: [PATCH v3 17/17] spi: s3c64xx: use bitfield access macros

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1386 bytes --]

On Sat, Jan 27, 2024 at 03:44:24AM +0000, Tudor Ambarus wrote:
> On 1/27/24 03:38, Sam Protsenko wrote:

> >>>> -               val |= S3C64XX_SPI_MODE_BUS_TSZ_HALFWORD;
> >>>> -               val |= S3C64XX_SPI_MODE_CH_TSZ_HALFWORD;
> >>>> +               val |= FIELD_PREP(S3C64XX_SPI_MODE_BUS_TSZ_MASK,
> >>>> +                                 S3C64XX_SPI_MODE_BUS_TSZ_HALFWORD) |
> >>>> +                      FIELD_PREP(S3C64XX_SPI_MODE_CH_TSZ_MASK,
> >>>> +                                 S3C64XX_SPI_MODE_CH_TSZ_HALFWORD);

> >>> Two people complained it makes the code harder to read. Yet it's not
> >>> addressed in v3. Please see my comments for your previous submission
> >>> explaining what can be done, and also Andi's comment on that matter.

> >> I kept these intentionally. Please read my reply on that matter or the
> >> cover letter to this patch set.

> > I read it. But still don't like it 🙂 I'm sure it's possible to do
> > this modification, but at the same time keep the code clean an easy to
> > read. The code above -- I don't like at all, sorry. It was much better
> > before this patch, IMHO.

> Yeah, I guess Mark will tip the scale.

All other things being equal I tend to try not to get too involved with
minor coding style stuff in drivers.  People do seem to like
FIELD_PREP() but I have a hard time getting *too* excited.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 17/17] spi: s3c64xx: use bitfield access macros

[Tudor Ambarus]

On 1/29/24 16:42, Mark Brown wrote:
> On Sat, Jan 27, 2024 at 03:44:24AM +0000, Tudor Ambarus wrote:
>> On 1/27/24 03:38, Sam Protsenko wrote:
> 
>>>>>> -               val |= S3C64XX_SPI_MODE_BUS_TSZ_HALFWORD;
>>>>>> -               val |= S3C64XX_SPI_MODE_CH_TSZ_HALFWORD;
>>>>>> +               val |= FIELD_PREP(S3C64XX_SPI_MODE_BUS_TSZ_MASK,
>>>>>> +                                 S3C64XX_SPI_MODE_BUS_TSZ_HALFWORD) |
>>>>>> +                      FIELD_PREP(S3C64XX_SPI_MODE_CH_TSZ_MASK,
>>>>>> +                                 S3C64XX_SPI_MODE_CH_TSZ_HALFWORD);
> 
>>>>> Two people complained it makes the code harder to read. Yet it's not
>>>>> addressed in v3. Please see my comments for your previous submission
>>>>> explaining what can be done, and also Andi's comment on that matter.
> 
>>>> I kept these intentionally. Please read my reply on that matter or the
>>>> cover letter to this patch set.
> 
>>> I read it. But still don't like it 🙂 I'm sure it's possible to do
>>> this modification, but at the same time keep the code clean an easy to
>>> read. The code above -- I don't like at all, sorry. It was much better
>>> before this patch, IMHO.
> 
>> Yeah, I guess Mark will tip the scale.
> 
> All other things being equal I tend to try not to get too involved with
> minor coding style stuff in drivers.  People do seem to like
> FIELD_PREP() but I have a hard time getting *too* excited.

Ok, I'll remove FIELD_PREP. Would you please consider the other patches,
all are simple. There's another "controversy" on 6/17. You can ignore
that as well maybe, and I'll resend it where I refrain myself to just
removing the cast.