[PATCH 00/21] spi: s3c64xx: winter cleanup and gs101 support

[PATCH 00/21] spi: s3c64xx: winter cleanup and gs101 support

[Tudor Ambarus]

Hi,

The patch set cleans a bit the driver and adds support for gs101 SPI.

Apart of the SPI patches, I added support for iowrite{8,16}_32 accessors
in asm-generic/io.h. This will allow devices that require 32 bits
register accesses to write data in chunks of 8 or 16 bits (a typical use
case is SPI, where clients can request transfers in words of 8 bits for
example). GS101 only allows 32bit register accesses otherwise it raisses
a Serror Interrupt and hangs the system, thus the accessors are needed
here. If the accessors are fine, I expect they'll be queued either to
the SPI tree or to the ASM header files tree, but by providing an
immutable tag, so that the other tree can merge them too.

The SPI patches were tested with the spi-loopback-test on the gs101
controller.

Thanks!
ta

Tudor Ambarus (21):
  spi: dt-bindings: samsung: add google,gs101-spi compatible
  spi: s3c64xx: sort headers alphabetically
  spi: s3c64xx: remove extra blank line
  spi: s3c64xx: remove unneeded (void *) casts in of_match_table
  spi: s3c64xx: explicitly include <linux/bits.h>
  spi: s3c64xx: remove else after return
  spi: s3c64xx: use bitfield access macros
  spi: s3c64xx: move error check up to avoid rechecking
  spi: s3c64xx: use full mask for {RX, TX}_FIFO_LVL
  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: simplify s3c64xx_wait_for_pio()
  spi: s3c64xx: add missing blank line after declaration
  spi: s3c64xx: downgrade dev_warn to dev_dbg for optional dt props
  asm-generic/io.h: add iowrite{8,16}_32 accessors
  spi: s3c64xx: add support for google,gs101-spi
  spi: s3c64xx: make the SPI alias optional for newer SoCs
  MAINTAINERS: add Tudor Ambarus as R for the samsung SPI driver

 .../devicetree/bindings/spi/samsung,spi.yaml  |   1 +
 MAINTAINERS                                   |   1 +
 drivers/spi/spi-s3c64xx.c                     | 447 +++++++++---------
 include/asm-generic/io.h                      |  50 ++
 4 files changed, 276 insertions(+), 223 deletions(-)

-- 
2.43.0.429.g432eaa2c6b-goog

[PATCH 01/21] spi: dt-bindings: samsung: add google,gs101-spi compatible

[Tudor Ambarus]

Add "google,gs101-spi" dedicated compatible for representing SPI of
Google GS101 SoC.

Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 Documentation/devicetree/bindings/spi/samsung,spi.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/spi/samsung,spi.yaml b/Documentation/devicetree/bindings/spi/samsung,spi.yaml
index 79da99ca0e53..386ea8b23993 100644
--- a/Documentation/devicetree/bindings/spi/samsung,spi.yaml
+++ b/Documentation/devicetree/bindings/spi/samsung,spi.yaml
@@ -17,6 +17,7 @@ properties:
   compatible:
     oneOf:
       - enum:
+          - google,gs101-spi
           - samsung,s3c2443-spi # for S3C2443, S3C2416 and S3C2450
           - samsung,s3c6410-spi
           - samsung,s5pv210-spi # for S5PV210 and S5PC110
-- 
2.43.0.429.g432eaa2c6b-goog

[PATCH 02/21] 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.

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

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 432ec60d3568..187b617e3e14 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -3,19 +3,19 @@
 // Copyright (c) 2009 Samsung Electronics Co., Ltd.
 //      Jaswinder Singh <jassi.brar@samsung.com>
 
-#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/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 03/21] spi: s3c64xx: remove extra blank line

[Tudor Ambarus]

Remove extra blank line.

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 187b617e3e14..26d389d95af9 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -16,7 +16,6 @@
 #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
-- 
2.43.0.429.g432eaa2c6b-goog

[PATCH 04/21] 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.

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

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 26d389d95af9..b350e70fd179 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -1495,29 +1495,37 @@ 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,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 05/21] spi: s3c64xx: explicitly include <linux/bits.h>

[Tudor Ambarus]

The driver uses GENMASK() but does not include <linux/bits.h>.
Include the missing header, we shall aim to have the drivers self
contained.

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 b350e70fd179..9ce56aa792ed 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/clk.h>
 #include <linux/delay.h>
 #include <linux/dma-mapping.h>
-- 
2.43.0.429.g432eaa2c6b-goog

[PATCH 06/21] spi: s3c64xx: remove else after return

[Tudor Ambarus]

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

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 9ce56aa792ed..db1e1d6ee732 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -406,12 +406,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 07/21] 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.

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

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index db1e1d6ee732..16eea56892a2 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>
@@ -17,91 +18,91 @@
 #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(15, 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)
+
+#define S3C64XX_SPI_CS_NSC_CNT_MASK		GENMASK(9, 4)
+#define S3C64XX_SPI_CS_NSC_CNT_2		2
+#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) & \
@@ -111,18 +112,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_TRAILCNT		S3C64XX_SPI_MAX_TRAILCNT
-
 #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;
@@ -341,8 +337,9 @@ static void s3c64xx_spi_set_cs(struct spi_device *spi, bool enable)
 		} else {
 			u32 ssel = readl(sdd->regs + S3C64XX_SPI_CS_REG);
 
-			ssel |= (S3C64XX_SPI_CS_AUTO |
-						S3C64XX_SPI_CS_NSC_CNT_2);
+			ssel |= S3C64XX_SPI_CS_AUTO |
+				FIELD_PREP(S3C64XX_SPI_CS_NSC_CNT_MASK,
+					   S3C64XX_SPI_CS_NSC_CNT_2);
 			writel(ssel, sdd->regs + S3C64XX_SPI_CS_REG);
 		}
 	} else {
@@ -665,16 +662,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;
 	}
 
@@ -800,7 +803,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 */
@@ -1073,8 +1076,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);
 
@@ -1090,8 +1093,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_MASK;
 	writel(val, regs + S3C64XX_SPI_MODE_CFG);
 
 	s3c64xx_flush_fifo(sdd);
-- 
2.43.0.429.g432eaa2c6b-goog

[PATCH 08/21] spi: s3c64xx: move error check up to avoid rechecking

[Tudor Ambarus]

Check the return value of wait_for_completion_timeout() immediately
after the call so that we avoid checking the return value twice.

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

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 16eea56892a2..128c3b8211ce 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -519,17 +519,17 @@ static int s3c64xx_wait_for_dma(struct s3c64xx_spi_driver_data *sdd,
 
 	val = msecs_to_jiffies(ms) + 10;
 	val = wait_for_completion_timeout(&sdd->xfer_completion, val);
-
+	if (!val)
+		return -EIO;
 	/*
-	 * If the previous xfer was completed within timeout, then
-	 * proceed further else return -EIO.
+	 * If the previous xfer was completed within timeout proceed further.
 	 * 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
 	 * needs bus transmission to finish, so we don't worry if
 	 * Xfer involved Rx(with or without Tx).
 	 */
-	if (val && !xfer->rx_buf) {
+	if (!xfer->rx_buf) {
 		val = msecs_to_loops(10);
 		status = readl(regs + S3C64XX_SPI_STATUS);
 		while ((TX_FIFO_LVL(status, sdd)
@@ -538,13 +538,8 @@ static int s3c64xx_wait_for_dma(struct s3c64xx_spi_driver_data *sdd,
 			cpu_relax();
 			status = readl(regs + S3C64XX_SPI_STATUS);
 		}
-
 	}
 
-	/* If timed out while checking rx/tx status return error */
-	if (!val)
-		return -EIO;
-
 	return 0;
 }
 
-- 
2.43.0.429.g432eaa2c6b-goog

[PATCH 09/21] spi: s3c64xx: use full mask for {RX, TX}_FIFO_LVL

[Tudor Ambarus]

SPI_STATUSn.{RX, TX}_FIFO_LVL fields  show the data level in the RX and
TX FIFOs. The IP supports FIFOs from 8 to 256 bytes, but apart from the
MODE_CFG.{RX, TX}_RDY_LVL fields that configure the {RX, TX} FIFO
trigger level in the interrupt mode, there's nothing in the registers
that configure the FIFOs depth. Is the responsibility of the SoC that
integrates the IP to dictate the FIFO depth and of the SPI driver to
make sure it doesn't bypass the FIFO length.

{RX, TX}_FIFO_LVL was used to pass the FIFO length information based on
the IP configuration in the SoC. Its value was defined so that it
includes the entire FIFO length. For example, if one wanted to specify a
64 FIFO length (0x40), it wold configure the FIFO level to 127 (0x7f).
This is not only wrong, because it doesn't respect the IP's register
fields, it's also misleading. Use the full mask for the
SPI_STATUSn.{RX, TX}_FIFO_LVL fields. No change in functionality is
expected.

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

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 128c3b8211ce..a06b83b952c6 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -77,6 +77,8 @@
 #define S3C64XX_SPI_INT_RX_FIFORDY_EN		BIT(1)
 #define S3C64XX_SPI_INT_TX_FIFORDY_EN		BIT(0)
 
+#define S3C64XX_SPI_ST_RX_FIFO_LVL		GENMASK(23, 15)
+#define S3C64XX_SPI_ST_TX_FIFO_LVL		GENMASK(14, 6)
 #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)
@@ -107,9 +109,6 @@
 #define FIFO_LVL_MASK(i) ((i)->port_conf->fifo_lvl_mask[i->port_id])
 #define S3C64XX_SPI_ST_TX_DONE(v, i) (((v) & \
 				(1 << (i)->port_conf->tx_st_done)) ? 1 : 0)
-#define TX_FIFO_LVL(v, i) (((v) >> 6) & FIFO_LVL_MASK(i))
-#define RX_FIFO_LVL(v, i) (((v) >> (i)->port_conf->rx_lvl_offset) & \
-					FIFO_LVL_MASK(i))
 #define FIFO_DEPTH(i) ((FIFO_LVL_MASK(i) >> 1) + 1)
 
 #define S3C64XX_SPI_POLLING_SIZE	32
@@ -218,7 +217,7 @@ static void s3c64xx_flush_fifo(struct s3c64xx_spi_driver_data *sdd)
 	loops = msecs_to_loops(1);
 	do {
 		val = readl(regs + S3C64XX_SPI_STATUS);
-	} while (TX_FIFO_LVL(val, sdd) && loops--);
+	} while (FIELD_GET(S3C64XX_SPI_ST_TX_FIFO_LVL, val) && loops--);
 
 	if (loops == 0)
 		dev_warn(&sdd->pdev->dev, "Timed out flushing TX FIFO\n");
@@ -227,7 +226,7 @@ static void s3c64xx_flush_fifo(struct s3c64xx_spi_driver_data *sdd)
 	loops = msecs_to_loops(1);
 	do {
 		val = readl(regs + S3C64XX_SPI_STATUS);
-		if (RX_FIFO_LVL(val, sdd))
+		if (FIELD_GET(S3C64XX_SPI_ST_RX_FIFO_LVL, val))
 			readl(regs + S3C64XX_SPI_RX_DATA);
 		else
 			break;
@@ -498,10 +497,11 @@ static u32 s3c64xx_spi_wait_for_timeout(struct s3c64xx_spi_driver_data *sdd,
 
 	do {
 		status = readl(regs + S3C64XX_SPI_STATUS);
-	} while (RX_FIFO_LVL(status, sdd) < max_fifo && --val);
+	} while (FIELD_GET(S3C64XX_SPI_ST_RX_FIFO_LVL, status) < max_fifo &&
+		 --val);
 
 	/* return the actual received data length */
-	return RX_FIFO_LVL(status, sdd);
+	return FIELD_GET(S3C64XX_SPI_ST_RX_FIFO_LVL, status);
 }
 
 static int s3c64xx_wait_for_dma(struct s3c64xx_spi_driver_data *sdd,
@@ -532,7 +532,7 @@ static int s3c64xx_wait_for_dma(struct s3c64xx_spi_driver_data *sdd,
 	if (!xfer->rx_buf) {
 		val = msecs_to_loops(10);
 		status = readl(regs + S3C64XX_SPI_STATUS);
-		while ((TX_FIFO_LVL(status, sdd)
+		while ((FIELD_GET(S3C64XX_SPI_ST_TX_FIFO_LVL, status)
 			|| !S3C64XX_SPI_ST_TX_DONE(status, sdd))
 		       && --val) {
 			cpu_relax();
@@ -562,7 +562,7 @@ static int s3c64xx_wait_for_pio(struct s3c64xx_spi_driver_data *sdd,
 
 	/* sleep during signal transfer time */
 	status = readl(regs + S3C64XX_SPI_STATUS);
-	if (RX_FIFO_LVL(status, sdd) < xfer->len)
+	if (FIELD_GET(S3C64XX_SPI_ST_RX_FIFO_LVL, status) < xfer->len)
 		usleep_range(time_us / 2, time_us);
 
 	if (use_irq) {
@@ -574,7 +574,8 @@ static int s3c64xx_wait_for_pio(struct s3c64xx_spi_driver_data *sdd,
 	val = msecs_to_loops(ms);
 	do {
 		status = readl(regs + S3C64XX_SPI_STATUS);
-	} while (RX_FIFO_LVL(status, sdd) < xfer->len && --val);
+	} while (FIELD_GET(S3C64XX_SPI_ST_RX_FIFO_LVL, status) < xfer->len &&
+		 --val);
 
 	if (!val)
 		return -EIO;
-- 
2.43.0.429.g432eaa2c6b-goog

[PATCH 10/21] spi: s3c64xx: move common code outside if else

[Tudor Ambarus]

Move common code outside if else to avoid code duplication.

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 a06b83b952c6..4b13252da8b8 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -285,20 +285,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 11/21] spi: s3c64xx: check return code of dmaengine_slave_config()

[Tudor Ambarus]

Check the return code of dmaengine_slave_config().

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 4b13252da8b8..cc33647eab14 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -296,7 +296,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 12/21] spi: s3c64xx: propagate the dma_submit_error() error code

[Tudor Ambarus]

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 cc33647eab14..2b088a190ab9 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -315,7 +315,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 13/21] spi: s3c64xx: rename prepare_dma() to s3c64xx_prepare_dma()

[Tudor Ambarus]

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

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 2b088a190ab9..8897b5895cde 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -272,8 +272,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;
@@ -439,7 +439,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:
@@ -471,7 +471,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 14/21] 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 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 8897b5895cde..fd0e62ff8968 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -520,7 +520,7 @@ static int s3c64xx_wait_for_dma(struct s3c64xx_spi_driver_data *sdd,
 	val = msecs_to_jiffies(ms) + 10;
 	val = wait_for_completion_timeout(&sdd->xfer_completion, val);
 	if (!val)
-		return -EIO;
+		return -ETIMEDOUT;
 	/*
 	 * If the previous xfer was completed within timeout proceed further.
 	 * DmaTx returns after simply writing data in the FIFO,
@@ -568,7 +568,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 15/21] spi: s3c64xx: simplify s3c64xx_wait_for_pio()

[Tudor Ambarus]

s3c64xx_spi_transfer_one() makes sure that for PIO the xfer->len is
always smaller than the fifo size. Clean s3c64xx_wait_for_pio() and
emphasize that it works with lengths smaller than the fifo size.

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

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index fd0e62ff8968..f5474f3b3920 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -484,26 +484,6 @@ static int s3c64xx_enable_datapath(struct s3c64xx_spi_driver_data *sdd,
 	return 0;
 }
 
-static u32 s3c64xx_spi_wait_for_timeout(struct s3c64xx_spi_driver_data *sdd,
-					int timeout_ms)
-{
-	void __iomem *regs = sdd->regs;
-	unsigned long val = 1;
-	u32 status;
-	u32 max_fifo = FIFO_DEPTH(sdd);
-
-	if (timeout_ms)
-		val = msecs_to_loops(timeout_ms);
-
-	do {
-		status = readl(regs + S3C64XX_SPI_STATUS);
-	} while (FIELD_GET(S3C64XX_SPI_ST_RX_FIFO_LVL, status) < max_fifo &&
-		 --val);
-
-	/* return the actual received data length */
-	return FIELD_GET(S3C64XX_SPI_ST_RX_FIFO_LVL, status);
-}
-
 static int s3c64xx_wait_for_dma(struct s3c64xx_spi_driver_data *sdd,
 				struct spi_transfer *xfer)
 {
@@ -547,13 +527,11 @@ static int s3c64xx_wait_for_pio(struct s3c64xx_spi_driver_data *sdd,
 				struct spi_transfer *xfer, bool use_irq)
 {
 	void __iomem *regs = sdd->regs;
+	u8 *buf = xfer->rx_buf;
+	unsigned long time_us;
 	unsigned long val;
-	u32 status;
-	int loops;
-	u32 cpy_len;
-	u8 *buf;
+	u32 status, len;
 	int ms;
-	unsigned long time_us;
 
 	/* microsecs to xfer 'len' bytes @ 'cur_speed' */
 	time_us = (xfer->len * 8 * 1000 * 1000) / sdd->cur_speed;
@@ -576,48 +554,29 @@ static int s3c64xx_wait_for_pio(struct s3c64xx_spi_driver_data *sdd,
 		status = readl(regs + S3C64XX_SPI_STATUS);
 	} while (FIELD_GET(S3C64XX_SPI_ST_RX_FIFO_LVL, status) < xfer->len &&
 		 --val);
-
 	if (!val)
 		return -EIO;
 
 	/* If it was only Tx */
-	if (!xfer->rx_buf) {
+	if (!buf) {
 		sdd->state &= ~TXBUSY;
 		return 0;
 	}
 
-	/*
-	 * If the receive length is bigger than the controller fifo
-	 * size, calculate the loops and read the fifo as many times.
-	 * loops = length / max fifo size (calculated by using the
-	 * fifo mask).
-	 * For any size less than the fifo size the below code is
-	 * executed atleast once.
-	 */
-	loops = xfer->len / FIFO_DEPTH(sdd);
-	buf = xfer->rx_buf;
-	do {
-		/* wait for data to be received in the fifo */
-		cpy_len = s3c64xx_spi_wait_for_timeout(sdd,
-						       (loops ? ms : 0));
-
-		switch (sdd->cur_bpw) {
-		case 32:
-			ioread32_rep(regs + S3C64XX_SPI_RX_DATA,
-				     buf, cpy_len / 4);
-			break;
-		case 16:
-			ioread16_rep(regs + S3C64XX_SPI_RX_DATA,
-				     buf, cpy_len / 2);
-			break;
-		default:
-			ioread8_rep(regs + S3C64XX_SPI_RX_DATA,
-				    buf, cpy_len);
-			break;
-		}
+	len = FIELD_GET(S3C64XX_SPI_ST_RX_FIFO_LVL, status);
+
+	switch (sdd->cur_bpw) {
+	case 32:
+		ioread32_rep(regs + S3C64XX_SPI_RX_DATA, buf, len / 4);
+		break;
+	case 16:
+		ioread16_rep(regs + S3C64XX_SPI_RX_DATA, buf, len / 2);
+		break;
+	default:
+		ioread8_rep(regs + S3C64XX_SPI_RX_DATA, buf, len);
+		break;
+	}
 
-		buf = buf + cpy_len;
-	} while (loops--);
 	sdd->state &= ~RXBUSY;
 
 	return 0;
-- 
2.43.0.429.g432eaa2c6b-goog

[PATCH 16/21] spi: s3c64xx: add missing blank line after declaration

[Tudor Ambarus]

Add missing blank line after declaration. Move initialization in the
body of the function.

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 f5474f3b3920..2abf5994080a 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -1273,8 +1273,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 17/21] 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.

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 2abf5994080a..62671b2d594a 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -1065,14 +1065,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 18/21] asm-generic/io.h: add iowrite{8,16}_32 accessors

[Tudor Ambarus]

This will allow devices that require 32 bits register accesses to write
data in chunks of 8 or 16 bits.

One SoC that requires 32 bit register accesses is the google gs101. A
typical use case is SPI, where the clients can request transfers in words
of 8 bits.

Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 include/asm-generic/io.h | 50 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
index bac63e874c7b..1e224d1ccc98 100644
--- a/include/asm-generic/io.h
+++ b/include/asm-generic/io.h
@@ -476,6 +476,21 @@ static inline void writesb(volatile void __iomem *addr, const void *buffer,
 }
 #endif
 
+#ifndef writesb_l
+#define writesb_l writesb_l
+static inline void writesb_l(volatile void __iomem *addr, const void *buffer,
+			     unsigned int count)
+{
+	if (count) {
+		const u8 *buf = buffer;
+
+		do {
+			__raw_writel(*buf++, addr);
+		} while (--count);
+	}
+}
+#endif
+
 #ifndef writesw
 #define writesw writesw
 static inline void writesw(volatile void __iomem *addr, const void *buffer,
@@ -491,6 +506,21 @@ static inline void writesw(volatile void __iomem *addr, const void *buffer,
 }
 #endif
 
+#ifndef writesw_l
+#define writesw_l writesw_l
+static inline void writesw_l(volatile void __iomem *addr, const void *buffer,
+			     unsigned int count)
+{
+	if (count) {
+		const u16 *buf = buffer;
+
+		do {
+			__raw_writel(*buf++, addr);
+		} while (--count);
+	}
+}
+#endif
+
 #ifndef writesl
 #define writesl writesl
 static inline void writesl(volatile void __iomem *addr, const void *buffer,
@@ -956,6 +986,16 @@ static inline void iowrite8_rep(volatile void __iomem *addr,
 }
 #endif
 
+#ifndef iowrite8_32_rep
+#define iowrite8_32_rep iowrite8_32_rep
+static inline void iowrite8_32_rep(volatile void __iomem *addr,
+				   const void *buffer,
+				   unsigned int count)
+{
+	writesb_l(addr, buffer, count);
+}
+#endif
+
 #ifndef iowrite16_rep
 #define iowrite16_rep iowrite16_rep
 static inline void iowrite16_rep(volatile void __iomem *addr,
@@ -966,6 +1006,16 @@ static inline void iowrite16_rep(volatile void __iomem *addr,
 }
 #endif
 
+#ifndef iowrite16_32_rep
+#define iowrite16_32_rep iowrite16_32_rep
+static inline void iowrite16_32_rep(volatile void __iomem *addr,
+				    const void *buffer,
+				    unsigned int count)
+{
+	writesw_l(addr, buffer, count);
+}
+#endif
+
 #ifndef iowrite32_rep
 #define iowrite32_rep iowrite32_rep
 static inline void iowrite32_rep(volatile void __iomem *addr,
-- 
2.43.0.429.g432eaa2c6b-goog

[PATCH 19/21] spi: s3c64xx: add support for google,gs101-spi

[Tudor Ambarus]

Add support for GS101 SPI. All the SPI nodes on GS101 have 64 bytes
FIFOs, infer the FIFO size from the compatible. GS101 allows just 32bit
register accesses, otherwise a Serror Interrupt is raised. Do the write
reg accesses in 32 bits.

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

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 62671b2d594a..c4ddd2859ba4 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -20,6 +20,7 @@
 
 #define MAX_SPI_PORTS				12
 #define S3C64XX_SPI_QUIRK_CS_AUTO		BIT(1)
+#define S3C64XX_SPI_GS1O1_32BIT_REG_IO_WIDTH	BIT(2)
 #define AUTOSUSPEND_TIMEOUT			2000
 
 /* Registers and bit-fields */
@@ -131,6 +132,7 @@ struct s3c64xx_spi_dma_data {
  * @rx_lvl_offset: Bit offset of RX_FIFO_LVL bits in SPI_STATUS regiter.
  * @tx_st_done: Bit offset of TX_DONE bit in SPI_STATUS regiter.
  * @clk_div: Internal clock divider
+ * @fifosize: size of the FIFO
  * @quirks: Bitmask of known quirks
  * @high_speed: True, if the controller supports HIGH_SPEED_EN bit.
  * @clk_from_cmu: True, if the controller does not include a clock mux and
@@ -149,6 +151,7 @@ struct s3c64xx_spi_port_config {
 	int	tx_st_done;
 	int	quirks;
 	int	clk_div;
+	unsigned int fifosize;
 	bool	high_speed;
 	bool	clk_from_cmu;
 	bool	clk_ioclk;
@@ -175,6 +178,7 @@ struct s3c64xx_spi_port_config {
  * @tx_dma: Local transmit DMA data (e.g. chan and direction)
  * @port_conf: Local SPI port configuartion data
  * @port_id: Port identification number
+ * @fifosize: size of the FIFO for this port
  */
 struct s3c64xx_spi_driver_data {
 	void __iomem                    *regs;
@@ -194,6 +198,7 @@ struct s3c64xx_spi_driver_data {
 	struct s3c64xx_spi_dma_data	tx_dma;
 	const struct s3c64xx_spi_port_config	*port_conf;
 	unsigned int			port_id;
+	unsigned int			fifosize;
 };
 
 static void s3c64xx_flush_fifo(struct s3c64xx_spi_driver_data *sdd)
@@ -403,7 +408,7 @@ static bool s3c64xx_spi_can_dma(struct spi_controller *host,
 	struct s3c64xx_spi_driver_data *sdd = spi_controller_get_devdata(host);
 
 	if (sdd->rx_dma.ch && sdd->tx_dma.ch)
-		return xfer->len > FIFO_DEPTH(sdd);
+		return xfer->len > sdd->fifosize;
 
 	return false;
 }
@@ -447,12 +452,22 @@ static int s3c64xx_enable_datapath(struct s3c64xx_spi_driver_data *sdd,
 					xfer->tx_buf, xfer->len / 4);
 				break;
 			case 16:
-				iowrite16_rep(regs + S3C64XX_SPI_TX_DATA,
-					xfer->tx_buf, xfer->len / 2);
+				if (sdd->port_conf->quirks &
+				    S3C64XX_SPI_GS1O1_32BIT_REG_IO_WIDTH)
+					iowrite16_32_rep(regs + S3C64XX_SPI_TX_DATA,
+							 xfer->tx_buf, xfer->len / 2);
+				else
+					iowrite16_rep(regs + S3C64XX_SPI_TX_DATA,
+						      xfer->tx_buf, xfer->len / 2);
 				break;
 			default:
-				iowrite8_rep(regs + S3C64XX_SPI_TX_DATA,
-					xfer->tx_buf, xfer->len);
+				if (sdd->port_conf->quirks &
+				    S3C64XX_SPI_GS1O1_32BIT_REG_IO_WIDTH)
+					iowrite8_32_rep(regs + S3C64XX_SPI_TX_DATA,
+							xfer->tx_buf, xfer->len);
+				else
+					iowrite8_rep(regs + S3C64XX_SPI_TX_DATA,
+						     xfer->tx_buf, xfer->len);
 				break;
 			}
 		}
@@ -696,7 +711,7 @@ static int s3c64xx_spi_transfer_one(struct spi_controller *host,
 				    struct spi_transfer *xfer)
 {
 	struct s3c64xx_spi_driver_data *sdd = spi_controller_get_devdata(host);
-	const unsigned int fifo_len = FIFO_DEPTH(sdd);
+	const unsigned int fifo_len = sdd->fifosize;
 	const void *tx_buf = NULL;
 	void *rx_buf = NULL;
 	int target_len = 0, origin_len = 0;
@@ -1145,6 +1160,11 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
 		sdd->port_id = pdev->id;
 	}
 
+	if (sdd->port_conf->fifosize)
+		sdd->fifosize = sdd->port_conf->fifosize;
+	else
+		sdd->fifosize = FIFO_DEPTH(sdd);
+
 	sdd->cur_bpw = 8;
 
 	sdd->tx_dma.direction = DMA_MEM_TO_DEV;
@@ -1234,7 +1254,7 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
 	dev_dbg(&pdev->dev, "Samsung SoC SPI Driver loaded for Bus SPI-%d with %d Targets attached\n",
 					sdd->port_id, host->num_chipselect);
 	dev_dbg(&pdev->dev, "\tIOmem=[%pR]\tFIFO %dbytes\n",
-					mem_res, FIFO_DEPTH(sdd));
+					mem_res, sdd->fifosize);
 
 	pm_runtime_mark_last_busy(&pdev->dev);
 	pm_runtime_put_autosuspend(&pdev->dev);
@@ -1362,6 +1382,18 @@ static const struct dev_pm_ops s3c64xx_spi_pm = {
 			   s3c64xx_spi_runtime_resume, NULL)
 };
 
+static const struct s3c64xx_spi_port_config gs101_spi_port_config = {
+	.fifosize	= 64,
+	.rx_lvl_offset  = 15,
+	.tx_st_done     = 25,
+	.clk_div        = 4,
+	.high_speed	= true,
+	.clk_from_cmu	= true,
+	.has_loopback	= true,
+	.quirks		= S3C64XX_SPI_QUIRK_CS_AUTO |
+			  S3C64XX_SPI_GS1O1_32BIT_REG_IO_WIDTH,
+};
+
 static const struct s3c64xx_spi_port_config s3c2443_spi_port_config = {
 	.fifo_lvl_mask	= { 0x7f },
 	.rx_lvl_offset	= 13,
@@ -1452,6 +1484,10 @@ static const struct platform_device_id s3c64xx_spi_driver_ids[] = {
 };
 
 static const struct of_device_id s3c64xx_spi_dt_match[] = {
+	{
+		.compatible = "google,gs101-spi",
+		.data = &gs101_spi_port_config,
+	},
 	{
 		.compatible = "samsung,s3c2443-spi",
 		.data = &s3c2443_spi_port_config,
-- 
2.43.0.429.g432eaa2c6b-goog

[PATCH 20/21] spi: s3c64xx: make the SPI alias optional for newer SoCs

[Tudor Ambarus]

The alias was used to initialize the port_id, which unfortunately is
used for older SoCs to determine the FIFO size from
``s3c64xx_spi_port_config.fifo_lvl_mask``. This is wrong all the way as
we shouldn't make a driver dependable of an alias, or the order of
probe. If multiple FIFO sizes are supported across the SPI IPs, one
shall instead introduce a fifosize device tree property. Make the SPI
alias optional for the newer SoCs and mark the ``port_id`` and
``fifo_lvl_mask`` as deprecated.

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

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index c4ddd2859ba4..9cd64fd3058a 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -128,7 +128,8 @@ struct s3c64xx_spi_dma_data {
 
 /**
  * struct s3c64xx_spi_port_config - SPI Controller hardware info
- * @fifo_lvl_mask: Bit-mask for {TX|RX}_FIFO_LVL bits in SPI_STATUS register.
+ * @fifo_lvl_mask: [DEPRECATED] Bit-mask for {TX|RX}_FIFO_LVL bits in
+ *                 SPI_STATUS register.
  * @rx_lvl_offset: Bit offset of RX_FIFO_LVL bits in SPI_STATUS regiter.
  * @tx_st_done: Bit offset of TX_DONE bit in SPI_STATUS regiter.
  * @clk_div: Internal clock divider
@@ -177,7 +178,7 @@ struct s3c64xx_spi_port_config {
  * @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_id: Port identification number
+ * @port_id: [DEPRECATED] Port identification number
  * @fifosize: size of the FIFO for this port
  */
 struct s3c64xx_spi_driver_data {
@@ -1152,7 +1153,7 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
 	sdd->pdev = pdev;
 	if (pdev->dev.of_node) {
 		ret = of_alias_get_id(pdev->dev.of_node, "spi");
-		if (ret < 0)
+		if (ret < 0 && !sdd->port_conf->fifosize)
 			return dev_err_probe(&pdev->dev, ret,
 					     "Failed to get alias id\n");
 		sdd->port_id = ret;
@@ -1171,7 +1172,7 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
 	sdd->rx_dma.direction = DMA_DEV_TO_MEM;
 
 	host->dev.of_node = pdev->dev.of_node;
-	host->bus_num = sdd->port_id;
+	host->bus_num = -1;
 	host->setup = s3c64xx_spi_setup;
 	host->cleanup = s3c64xx_spi_cleanup;
 	host->prepare_transfer_hardware = s3c64xx_spi_prepare_transfer;
@@ -1252,7 +1253,7 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
 	}
 
 	dev_dbg(&pdev->dev, "Samsung SoC SPI Driver loaded for Bus SPI-%d with %d Targets attached\n",
-					sdd->port_id, host->num_chipselect);
+		host->bus_num, host->num_chipselect);
 	dev_dbg(&pdev->dev, "\tIOmem=[%pR]\tFIFO %dbytes\n",
 					mem_res, sdd->fifosize);
 
-- 
2.43.0.429.g432eaa2c6b-goog

[PATCH 21/21] MAINTAINERS: add Tudor Ambarus as R for the samsung SPI driver

[Tudor Ambarus]

I'm working with the samsung SPI driver and I'd like to review further
patches on this driver. Add myself as reviewer.

Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 8d1052fa6a69..b9cde7ed8489 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19404,6 +19404,7 @@ F:	include/linux/clk/samsung.h
 
 SAMSUNG SPI DRIVERS
 M:	Andi Shyti <andi.shyti@kernel.org>
+R:	Tudor Ambarus <tudor.ambarus@linaro.org>
 L:	linux-spi@vger.kernel.org
 L:	linux-samsung-soc@vger.kernel.org
 S:	Maintained
-- 
2.43.0.429.g432eaa2c6b-goog

Re: [PATCH 21/21] MAINTAINERS: add Tudor Ambarus as R for the samsung SPI driver

[Sam Protsenko]

On Tue, Jan 23, 2024 at 9:34 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
> I'm working with the samsung SPI driver and I'd like to review further
> patches on this driver. Add myself as reviewer.
>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> ---

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

>  MAINTAINERS | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 8d1052fa6a69..b9cde7ed8489 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -19404,6 +19404,7 @@ F:      include/linux/clk/samsung.h
>
>  SAMSUNG SPI DRIVERS
>  M:     Andi Shyti <andi.shyti@kernel.org>
> +R:     Tudor Ambarus <tudor.ambarus@linaro.org>
>  L:     linux-spi@vger.kernel.org
>  L:     linux-samsung-soc@vger.kernel.org
>  S:     Maintained
> --
> 2.43.0.429.g432eaa2c6b-goog
>

Re: [PATCH 00/21] spi: s3c64xx: winter cleanup and gs101 support

[Mark Brown]

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

On Tue, Jan 23, 2024 at 03:33:59PM +0000, Tudor Ambarus wrote:

> The patch set cleans a bit the driver and adds support for gs101 SPI.
> 
> Apart of the SPI patches, I added support for iowrite{8,16}_32 accessors
> in asm-generic/io.h. This will allow devices that require 32 bits
> register accesses to write data in chunks of 8 or 16 bits (a typical use
> case is SPI, where clients can request transfers in words of 8 bits for
> example). GS101 only allows 32bit register accesses otherwise it raisses
> a Serror Interrupt and hangs the system, thus the accessors are needed
> here. If the accessors are fine, I expect they'll be queued either to
> the SPI tree or to the ASM header files tree, but by providing an
> immutable tag, so that the other tree can merge them too.
> 
> The SPI patches were tested with the spi-loopback-test on the gs101
> controller.

The reformatting in this series will conflict with the SPI changes in:

   https://lore.kernel.org/r/20240120012948.8836-1-semen.protsenko@linaro.org

Can you please pull those into this series or otherwise coordinate?

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

Re: [PATCH 00/21] spi: s3c64xx: winter cleanup and gs101 support

[Sam Protsenko]

On Tue, Jan 23, 2024 at 9:34 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
> Hi,
>
> The patch set cleans a bit the driver and adds support for gs101 SPI.
>

It might be more convenient (for review purposes) to extract all the
cleanup patches into a separate series, and base it on top of the
gs101 SPI enablement series.

> Apart of the SPI patches, I added support for iowrite{8,16}_32 accessors
> in asm-generic/io.h. This will allow devices that require 32 bits
> register accesses to write data in chunks of 8 or 16 bits (a typical use
> case is SPI, where clients can request transfers in words of 8 bits for
> example). GS101 only allows 32bit register accesses otherwise it raisses
> a Serror Interrupt and hangs the system, thus the accessors are needed
> here. If the accessors are fine, I expect they'll be queued either to
> the SPI tree or to the ASM header files tree, but by providing an
> immutable tag, so that the other tree can merge them too.
>
> The SPI patches were tested with the spi-loopback-test on the gs101
> controller.
>
> Thanks!
> ta
>
> Tudor Ambarus (21):
>   spi: dt-bindings: samsung: add google,gs101-spi compatible
>   spi: s3c64xx: sort headers alphabetically
>   spi: s3c64xx: remove extra blank line
>   spi: s3c64xx: remove unneeded (void *) casts in of_match_table
>   spi: s3c64xx: explicitly include <linux/bits.h>
>   spi: s3c64xx: remove else after return
>   spi: s3c64xx: use bitfield access macros
>   spi: s3c64xx: move error check up to avoid rechecking
>   spi: s3c64xx: use full mask for {RX, TX}_FIFO_LVL
>   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: simplify s3c64xx_wait_for_pio()
>   spi: s3c64xx: add missing blank line after declaration
>   spi: s3c64xx: downgrade dev_warn to dev_dbg for optional dt props
>   asm-generic/io.h: add iowrite{8,16}_32 accessors
>   spi: s3c64xx: add support for google,gs101-spi
>   spi: s3c64xx: make the SPI alias optional for newer SoCs
>   MAINTAINERS: add Tudor Ambarus as R for the samsung SPI driver
>
>  .../devicetree/bindings/spi/samsung,spi.yaml  |   1 +
>  MAINTAINERS                                   |   1 +
>  drivers/spi/spi-s3c64xx.c                     | 447 +++++++++---------
>  include/asm-generic/io.h                      |  50 ++
>  4 files changed, 276 insertions(+), 223 deletions(-)
>
> --
> 2.43.0.429.g432eaa2c6b-goog
>

Re: [PATCH 01/21] spi: dt-bindings: samsung: add google,gs101-spi compatible

[Sam Protsenko]

On Tue, Jan 23, 2024 at 9:34 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
> Add "google,gs101-spi" dedicated compatible for representing SPI of
> Google GS101 SoC.
>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> ---

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

>  Documentation/devicetree/bindings/spi/samsung,spi.yaml | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/Documentation/devicetree/bindings/spi/samsung,spi.yaml b/Documentation/devicetree/bindings/spi/samsung,spi.yaml
> index 79da99ca0e53..386ea8b23993 100644
> --- a/Documentation/devicetree/bindings/spi/samsung,spi.yaml
> +++ b/Documentation/devicetree/bindings/spi/samsung,spi.yaml
> @@ -17,6 +17,7 @@ properties:
>    compatible:
>      oneOf:
>        - enum:
> +          - google,gs101-spi
>            - samsung,s3c2443-spi # for S3C2443, S3C2416 and S3C2450
>            - samsung,s3c6410-spi
>            - samsung,s5pv210-spi # for S5PV210 and S5PC110
> --
> 2.43.0.429.g432eaa2c6b-goog
>

Re: [PATCH 03/21] spi: s3c64xx: remove extra blank line

[Sam Protsenko]

On Tue, Jan 23, 2024 at 9:34 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
> Remove extra blank line.
>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> ---

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 187b617e3e14..26d389d95af9 100644
> --- a/drivers/spi/spi-s3c64xx.c
> +++ b/drivers/spi/spi-s3c64xx.c
> @@ -16,7 +16,6 @@
>  #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
> --
> 2.43.0.429.g432eaa2c6b-goog
>

Re: [PATCH 06/21] spi: s3c64xx: remove else after return

[Sam Protsenko]

On Tue, Jan 23, 2024 at 9:34 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
> Else case is not needed after a return, remove it.
>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> ---

Reviewed-by: Sam Protsenko <semen.protsenko@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 9ce56aa792ed..db1e1d6ee732 100644
> --- a/drivers/spi/spi-s3c64xx.c
> +++ b/drivers/spi/spi-s3c64xx.c
> @@ -406,12 +406,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
>

Re: [PATCH 19/21] spi: s3c64xx: add support for google,gs101-spi

[Sam Protsenko]

On Tue, Jan 23, 2024 at 9:34 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
> Add support for GS101 SPI. All the SPI nodes on GS101 have 64 bytes
> FIFOs, infer the FIFO size from the compatible. GS101 allows just 32bit
> register accesses, otherwise a Serror Interrupt is raised. Do the write
> reg accesses in 32 bits.
>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> ---

I counted 3 different features in this patch. Would be better to split
it correspondingly into 3 patches, to make patches atomic:

  1. I/O width
  2. FIFO size
  3. Adding support for gs101

And I'm not really convinced about FIFO size change.

>  drivers/spi/spi-s3c64xx.c | 50 +++++++++++++++++++++++++++++++++------
>  1 file changed, 43 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> index 62671b2d594a..c4ddd2859ba4 100644
> --- a/drivers/spi/spi-s3c64xx.c
> +++ b/drivers/spi/spi-s3c64xx.c
> @@ -20,6 +20,7 @@
>
>  #define MAX_SPI_PORTS                          12
>  #define S3C64XX_SPI_QUIRK_CS_AUTO              BIT(1)
> +#define S3C64XX_SPI_GS1O1_32BIT_REG_IO_WIDTH   BIT(2)
>  #define AUTOSUSPEND_TIMEOUT                    2000
>
>  /* Registers and bit-fields */
> @@ -131,6 +132,7 @@ struct s3c64xx_spi_dma_data {
>   * @rx_lvl_offset: Bit offset of RX_FIFO_LVL bits in SPI_STATUS regiter.
>   * @tx_st_done: Bit offset of TX_DONE bit in SPI_STATUS regiter.
>   * @clk_div: Internal clock divider
> + * @fifosize: size of the FIFO
>   * @quirks: Bitmask of known quirks
>   * @high_speed: True, if the controller supports HIGH_SPEED_EN bit.
>   * @clk_from_cmu: True, if the controller does not include a clock mux and
> @@ -149,6 +151,7 @@ struct s3c64xx_spi_port_config {
>         int     tx_st_done;
>         int     quirks;
>         int     clk_div;
> +       unsigned int fifosize;
>         bool    high_speed;
>         bool    clk_from_cmu;
>         bool    clk_ioclk;
> @@ -175,6 +178,7 @@ struct s3c64xx_spi_port_config {
>   * @tx_dma: Local transmit DMA data (e.g. chan and direction)
>   * @port_conf: Local SPI port configuartion data
>   * @port_id: Port identification number
> + * @fifosize: size of the FIFO for this port
>   */
>  struct s3c64xx_spi_driver_data {
>         void __iomem                    *regs;
> @@ -194,6 +198,7 @@ struct s3c64xx_spi_driver_data {
>         struct s3c64xx_spi_dma_data     tx_dma;
>         const struct s3c64xx_spi_port_config    *port_conf;
>         unsigned int                    port_id;
> +       unsigned int                    fifosize;
>  };
>
>  static void s3c64xx_flush_fifo(struct s3c64xx_spi_driver_data *sdd)
> @@ -403,7 +408,7 @@ static bool s3c64xx_spi_can_dma(struct spi_controller *host,
>         struct s3c64xx_spi_driver_data *sdd = spi_controller_get_devdata(host);
>
>         if (sdd->rx_dma.ch && sdd->tx_dma.ch)
> -               return xfer->len > FIFO_DEPTH(sdd);
> +               return xfer->len > sdd->fifosize;
>
>         return false;
>  }
> @@ -447,12 +452,22 @@ static int s3c64xx_enable_datapath(struct s3c64xx_spi_driver_data *sdd,
>                                         xfer->tx_buf, xfer->len / 4);
>                                 break;
>                         case 16:
> -                               iowrite16_rep(regs + S3C64XX_SPI_TX_DATA,
> -                                       xfer->tx_buf, xfer->len / 2);
> +                               if (sdd->port_conf->quirks &
> +                                   S3C64XX_SPI_GS1O1_32BIT_REG_IO_WIDTH)
> +                                       iowrite16_32_rep(regs + S3C64XX_SPI_TX_DATA,
> +                                                        xfer->tx_buf, xfer->len / 2);
> +                               else
> +                                       iowrite16_rep(regs + S3C64XX_SPI_TX_DATA,
> +                                                     xfer->tx_buf, xfer->len / 2);
>                                 break;
>                         default:
> -                               iowrite8_rep(regs + S3C64XX_SPI_TX_DATA,
> -                                       xfer->tx_buf, xfer->len);
> +                               if (sdd->port_conf->quirks &
> +                                   S3C64XX_SPI_GS1O1_32BIT_REG_IO_WIDTH)
> +                                       iowrite8_32_rep(regs + S3C64XX_SPI_TX_DATA,
> +                                                       xfer->tx_buf, xfer->len);
> +                               else
> +                                       iowrite8_rep(regs + S3C64XX_SPI_TX_DATA,
> +                                                    xfer->tx_buf, xfer->len);
>                                 break;
>                         }
>                 }
> @@ -696,7 +711,7 @@ static int s3c64xx_spi_transfer_one(struct spi_controller *host,
>                                     struct spi_transfer *xfer)
>  {
>         struct s3c64xx_spi_driver_data *sdd = spi_controller_get_devdata(host);
> -       const unsigned int fifo_len = FIFO_DEPTH(sdd);
> +       const unsigned int fifo_len = sdd->fifosize;
>         const void *tx_buf = NULL;
>         void *rx_buf = NULL;
>         int target_len = 0, origin_len = 0;
> @@ -1145,6 +1160,11 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
>                 sdd->port_id = pdev->id;
>         }
>
> +       if (sdd->port_conf->fifosize)
> +               sdd->fifosize = sdd->port_conf->fifosize;
> +       else
> +               sdd->fifosize = FIFO_DEPTH(sdd);
> +
>         sdd->cur_bpw = 8;
>
>         sdd->tx_dma.direction = DMA_MEM_TO_DEV;
> @@ -1234,7 +1254,7 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
>         dev_dbg(&pdev->dev, "Samsung SoC SPI Driver loaded for Bus SPI-%d with %d Targets attached\n",
>                                         sdd->port_id, host->num_chipselect);
>         dev_dbg(&pdev->dev, "\tIOmem=[%pR]\tFIFO %dbytes\n",
> -                                       mem_res, FIFO_DEPTH(sdd));
> +                                       mem_res, sdd->fifosize);
>
>         pm_runtime_mark_last_busy(&pdev->dev);
>         pm_runtime_put_autosuspend(&pdev->dev);
> @@ -1362,6 +1382,18 @@ static const struct dev_pm_ops s3c64xx_spi_pm = {
>                            s3c64xx_spi_runtime_resume, NULL)
>  };
>
> +static const struct s3c64xx_spi_port_config gs101_spi_port_config = {
> +       .fifosize       = 64,

I think if you rework the the .fifo_lvl_mask, replacing it with
.fifosize, you should also do next things in this series:
  1. Rework it for all supported (existing) chips in this driver
  2. Provide fifosize property for each SPI node for all existing dts
that use this driver
  3. Get rid of .fifo_lvl_mask for good. But the compatibility with
older kernels has to be taken into the account here as well.

Otherwise it looks like a half attempt and not finished, only creating
a duplicated property/struct field for the same (already existing)
thing. Because it's completely possible to do the same using just
.fifo_lvl_mask without introducing new fields or properties. If it
seems to much -- maybe just use .fifo_lvl_mask for now, and do all
that reworking properly later, in a separate patch series?

> +       .rx_lvl_offset  = 15,
> +       .tx_st_done     = 25,
> +       .clk_div        = 4,
> +       .high_speed     = true,
> +       .clk_from_cmu   = true,
> +       .has_loopback   = true,
> +       .quirks         = S3C64XX_SPI_QUIRK_CS_AUTO |
> +                         S3C64XX_SPI_GS1O1_32BIT_REG_IO_WIDTH,
> +};
> +
>  static const struct s3c64xx_spi_port_config s3c2443_spi_port_config = {
>         .fifo_lvl_mask  = { 0x7f },
>         .rx_lvl_offset  = 13,
> @@ -1452,6 +1484,10 @@ static const struct platform_device_id s3c64xx_spi_driver_ids[] = {
>  };
>
>  static const struct of_device_id s3c64xx_spi_dt_match[] = {
> +       {
> +               .compatible = "google,gs101-spi",
> +               .data = &gs101_spi_port_config,
> +       },
>         {
>                 .compatible = "samsung,s3c2443-spi",
>                 .data = &s3c2443_spi_port_config,
> --
> 2.43.0.429.g432eaa2c6b-goog
>

Re: [PATCH 16/21] spi: s3c64xx: add missing blank line after declaration

[Sam Protsenko]

On Tue, Jan 23, 2024 at 9:34 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
> Add missing blank line after declaration. Move initialization in the
> body of the function.
>
> 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 f5474f3b3920..2abf5994080a 100644
> --- a/drivers/spi/spi-s3c64xx.c
> +++ b/drivers/spi/spi-s3c64xx.c
> @@ -1273,8 +1273,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);

Why not just moving the empty line below the declaration block,
keeping the initialization on the variable declaration line?

>         if (ret)
>                 return ret;
>
> --
> 2.43.0.429.g432eaa2c6b-goog
>

Re: [PATCH 01/21] spi: dt-bindings: samsung: add google,gs101-spi compatible

[Krzysztof Kozlowski]

On 23/01/2024 16:34, Tudor Ambarus wrote:
> Add "google,gs101-spi" dedicated compatible for representing SPI of
> Google GS101 SoC.
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH 01/21] spi: dt-bindings: samsung: add google,gs101-spi compatible

[Andi Shyti]

Hi Tudor,

On Tue, Jan 23, 2024 at 03:34:00PM +0000, Tudor Ambarus wrote:
> Add "google,gs101-spi" dedicated compatible for representing SPI of
> Google GS101 SoC.
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>

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

Thanks,
Andi

Re: [PATCH 02/21] spi: s3c64xx: sort headers alphabetically

[Andi Shyti]

Hi Tudor,

On Tue, Jan 23, 2024 at 03:34:01PM +0000, Tudor Ambarus wrote:
> Sorting headers alphabetically helps locating duplicates,
> and makes it easier to figure out where to insert new headers.
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>

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

Thanks,
Andi

Re: [PATCH 03/21] spi: s3c64xx: remove extra blank line

[Andi Shyti]

Hi Tudor,

On Tue, Jan 23, 2024 at 03:34:02PM +0000, Tudor Ambarus wrote:
> Remove extra blank line.
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>

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

Thanks,
Andi

Re: [PATCH 04/21] spi: s3c64xx: remove unneeded (void *) casts in of_match_table

[Andi Shyti]

Hi Tudor,

On Tue, Jan 23, 2024 at 03:34:03PM +0000, Tudor Ambarus 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.
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>

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

Thanks,
Andi

Re: [PATCH 05/21] spi: s3c64xx: explicitly include <linux/bits.h>

[Andi Shyti]

Hi Tudor,

On Tue, Jan 23, 2024 at 03:34:04PM +0000, Tudor Ambarus wrote:
> The driver uses GENMASK() but does not include <linux/bits.h>.
> Include the missing header, we shall aim to have the drivers self
> contained.
> 
> 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 b350e70fd179..9ce56aa792ed 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>

I don't see why this should be included. Are there cases when
not having bits.h produces any compilation error?

Andi

>  #include <linux/clk.h>
>  #include <linux/delay.h>
>  #include <linux/dma-mapping.h>
> -- 
> 2.43.0.429.g432eaa2c6b-goog
> 

Re: [PATCH 06/21] spi: s3c64xx: remove else after return

[Andi Shyti]

Hi Tudor,

On Tue, Jan 23, 2024 at 03:34:05PM +0000, Tudor Ambarus wrote:
> Else case is not needed after a return, remove it.
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>

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

Thanks,
Andi

Re: [PATCH 05/21] spi: s3c64xx: explicitly include <linux/bits.h>

[Mark Brown]

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

On Tue, Jan 23, 2024 at 11:28:31PM +0100, Andi Shyti wrote:
> On Tue, Jan 23, 2024 at 03:34:04PM +0000, Tudor Ambarus wrote:

> > +#include <linux/bits.h>

> I don't see why this should be included. Are there cases when
> not having bits.h produces any compilation error?

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.

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

Re: [PATCH 00/21] spi: s3c64xx: winter cleanup and gs101 support

[Tudor Ambarus]

On 1/23/24 19:00, Mark Brown wrote:
> On Tue, Jan 23, 2024 at 03:33:59PM +0000, Tudor Ambarus wrote:
> 
>> The patch set cleans a bit the driver and adds support for gs101 SPI.
>>
>> Apart of the SPI patches, I added support for iowrite{8,16}_32 accessors
>> in asm-generic/io.h. This will allow devices that require 32 bits
>> register accesses to write data in chunks of 8 or 16 bits (a typical use
>> case is SPI, where clients can request transfers in words of 8 bits for
>> example). GS101 only allows 32bit register accesses otherwise it raisses
>> a Serror Interrupt and hangs the system, thus the accessors are needed
>> here. If the accessors are fine, I expect they'll be queued either to
>> the SPI tree or to the ASM header files tree, but by providing an
>> immutable tag, so that the other tree can merge them too.
>>
>> The SPI patches were tested with the spi-loopback-test on the gs101
>> controller.
> 
> The reformatting in this series will conflict with the SPI changes in:
> 
>    https://lore.kernel.org/r/20240120012948.8836-1-semen.protsenko@linaro.org
> 
> Can you please pull those into this series or otherwise coordinate?

ah, I haven't noticed Sam's updates. I'll rebase on top of his set and
adapt if necessary. I'll review that set in a sec.

Cheers,
ta

Re: [PATCH 08/21] spi: s3c64xx: move error check up to avoid rechecking

[André Draszik]

Hi Tudor,

On Tue, 2024-01-23 at 15:34 +0000, Tudor Ambarus wrote:
> @@ -538,13 +538,8 @@ static int s3c64xx_wait_for_dma(struct s3c64xx_spi_driver_data *sdd,
>  			cpu_relax();
>  			status = readl(regs + S3C64XX_SPI_STATUS);
>  		}
> -
>  	}
>  
> -	/* If timed out while checking rx/tx status return error */
> -	if (!val)
> -		return -EIO;
> -

This change behaviour of this function. The loop just above adjusts val and it is used to
determine if there was a timeout or not:

	if (val && !xfer->rx_buf) {
		val = msecs_to_loops(10);
		status = readl(regs + S3C64XX_SPI_STATUS);
		while ((TX_FIFO_LVL(status, sdd)
			|| !S3C64XX_SPI_ST_TX_DONE(status, sdd))
		       && --val) {
			cpu_relax();
			status = readl(regs + S3C64XX_SPI_STATUS);
		}


That doesn't work anymore now.

Cheers,
Andre'

Re: [PATCH 08/21] spi: s3c64xx: move error check up to avoid rechecking

[Tudor Ambarus]

On 1/24/24 09:21, André Draszik wrote:
> Hi Tudor,
> 

Hi!

> On Tue, 2024-01-23 at 15:34 +0000, Tudor Ambarus wrote:
>> @@ -538,13 +538,8 @@ static int s3c64xx_wait_for_dma(struct s3c64xx_spi_driver_data *sdd,
>>  			cpu_relax();
>>  			status = readl(regs + S3C64XX_SPI_STATUS);
>>  		}
>> -
>>  	}
>>  
>> -	/* If timed out while checking rx/tx status return error */
>> -	if (!val)
>> -		return -EIO;
>> -
> 
> This change behaviour of this function. The loop just above adjusts val and it is used to
> determine if there was a timeout or not:
> 
> 	if (val && !xfer->rx_buf) {
> 		val = msecs_to_loops(10);
> 		status = readl(regs + S3C64XX_SPI_STATUS);
> 		while ((TX_FIFO_LVL(status, sdd)
> 			|| !S3C64XX_SPI_ST_TX_DONE(status, sdd))
> 		       && --val) {
> 			cpu_relax();
> 			status = readl(regs + S3C64XX_SPI_STATUS);
> 		}
>
Oh, yes, the timeout in this block. You're right, I'll drop the patch.
Thanks!

ta

Re: [PATCH 16/21] spi: s3c64xx: add missing blank line after declaration

[Tudor Ambarus]

On 1/23/24 19:28, Sam Protsenko wrote:
> On Tue, Jan 23, 2024 at 9:34 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>>
>> Add missing blank line after declaration. Move initialization in the
>> body of the function.
>>
>> 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 f5474f3b3920..2abf5994080a 100644
>> --- a/drivers/spi/spi-s3c64xx.c
>> +++ b/drivers/spi/spi-s3c64xx.c
>> @@ -1273,8 +1273,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);
> 
> Why not just moving the empty line below the declaration block,
> keeping the initialization on the variable declaration line?
> 

just a matter of personal preference. I find it ugly to do an
initialization at variable declaration and then to immediately check the
return value in the body of the function. But I'll do as you say, just
cosmetics anyway.

Re: [PATCH 19/21] spi: s3c64xx: add support for google,gs101-spi

[Tudor Ambarus]

Hi, Sam! Thanks for the review!

On 1/23/24 19:25, Sam Protsenko wrote:
> On Tue, Jan 23, 2024 at 9:34 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>>
>> Add support for GS101 SPI. All the SPI nodes on GS101 have 64 bytes
>> FIFOs, infer the FIFO size from the compatible. GS101 allows just 32bit
>> register accesses, otherwise a Serror Interrupt is raised. Do the write
>> reg accesses in 32 bits.
>>
>> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
>> ---
> 
> I counted 3 different features in this patch. Would be better to split
> it correspondingly into 3 patches, to make patches atomic:
> 
>   1. I/O width
>   2. FIFO size

I kept these 2 in the same patch as gs101 to exemplify their use by
gs101. But I'm also fine splitting the patch in 3, will do in v2.

>   3. Adding support for gs101
> 
> And I'm not really convinced about FIFO size change.

I'll explain why it's needed below.

> 
>>  drivers/spi/spi-s3c64xx.c | 50 +++++++++++++++++++++++++++++++++------
>>  1 file changed, 43 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
>> index 62671b2d594a..c4ddd2859ba4 100644
>> --- a/drivers/spi/spi-s3c64xx.c
>> +++ b/drivers/spi/spi-s3c64xx.c
>> @@ -20,6 +20,7 @@
>>
>>  #define MAX_SPI_PORTS                          12
>>  #define S3C64XX_SPI_QUIRK_CS_AUTO              BIT(1)
>> +#define S3C64XX_SPI_GS1O1_32BIT_REG_IO_WIDTH   BIT(2)
>>  #define AUTOSUSPEND_TIMEOUT                    2000
>>
>>  /* Registers and bit-fields */
>> @@ -131,6 +132,7 @@ struct s3c64xx_spi_dma_data {
>>   * @rx_lvl_offset: Bit offset of RX_FIFO_LVL bits in SPI_STATUS regiter.
>>   * @tx_st_done: Bit offset of TX_DONE bit in SPI_STATUS regiter.
>>   * @clk_div: Internal clock divider
>> + * @fifosize: size of the FIFO
>>   * @quirks: Bitmask of known quirks
>>   * @high_speed: True, if the controller supports HIGH_SPEED_EN bit.
>>   * @clk_from_cmu: True, if the controller does not include a clock mux and
>> @@ -149,6 +151,7 @@ struct s3c64xx_spi_port_config {
>>         int     tx_st_done;
>>         int     quirks;
>>         int     clk_div;
>> +       unsigned int fifosize;
>>         bool    high_speed;
>>         bool    clk_from_cmu;
>>         bool    clk_ioclk;
>> @@ -175,6 +178,7 @@ struct s3c64xx_spi_port_config {
>>   * @tx_dma: Local transmit DMA data (e.g. chan and direction)
>>   * @port_conf: Local SPI port configuartion data
>>   * @port_id: Port identification number
>> + * @fifosize: size of the FIFO for this port
>>   */
>>  struct s3c64xx_spi_driver_data {
>>         void __iomem                    *regs;
>> @@ -194,6 +198,7 @@ struct s3c64xx_spi_driver_data {
>>         struct s3c64xx_spi_dma_data     tx_dma;
>>         const struct s3c64xx_spi_port_config    *port_conf;
>>         unsigned int                    port_id;
>> +       unsigned int                    fifosize;
>>  };
>>
>>  static void s3c64xx_flush_fifo(struct s3c64xx_spi_driver_data *sdd)
>> @@ -403,7 +408,7 @@ static bool s3c64xx_spi_can_dma(struct spi_controller *host,
>>         struct s3c64xx_spi_driver_data *sdd = spi_controller_get_devdata(host);
>>
>>         if (sdd->rx_dma.ch && sdd->tx_dma.ch)
>> -               return xfer->len > FIFO_DEPTH(sdd);
>> +               return xfer->len > sdd->fifosize;
>>
>>         return false;
>>  }
>> @@ -447,12 +452,22 @@ static int s3c64xx_enable_datapath(struct s3c64xx_spi_driver_data *sdd,
>>                                         xfer->tx_buf, xfer->len / 4);
>>                                 break;
>>                         case 16:
>> -                               iowrite16_rep(regs + S3C64XX_SPI_TX_DATA,
>> -                                       xfer->tx_buf, xfer->len / 2);
>> +                               if (sdd->port_conf->quirks &
>> +                                   S3C64XX_SPI_GS1O1_32BIT_REG_IO_WIDTH)
>> +                                       iowrite16_32_rep(regs + S3C64XX_SPI_TX_DATA,
>> +                                                        xfer->tx_buf, xfer->len / 2);
>> +                               else
>> +                                       iowrite16_rep(regs + S3C64XX_SPI_TX_DATA,
>> +                                                     xfer->tx_buf, xfer->len / 2);
>>                                 break;
>>                         default:
>> -                               iowrite8_rep(regs + S3C64XX_SPI_TX_DATA,
>> -                                       xfer->tx_buf, xfer->len);
>> +                               if (sdd->port_conf->quirks &
>> +                                   S3C64XX_SPI_GS1O1_32BIT_REG_IO_WIDTH)
>> +                                       iowrite8_32_rep(regs + S3C64XX_SPI_TX_DATA,
>> +                                                       xfer->tx_buf, xfer->len);
>> +                               else
>> +                                       iowrite8_rep(regs + S3C64XX_SPI_TX_DATA,
>> +                                                    xfer->tx_buf, xfer->len);
>>                                 break;
>>                         }
>>                 }
>> @@ -696,7 +711,7 @@ static int s3c64xx_spi_transfer_one(struct spi_controller *host,
>>                                     struct spi_transfer *xfer)
>>  {
>>         struct s3c64xx_spi_driver_data *sdd = spi_controller_get_devdata(host);
>> -       const unsigned int fifo_len = FIFO_DEPTH(sdd);
>> +       const unsigned int fifo_len = sdd->fifosize;
>>         const void *tx_buf = NULL;
>>         void *rx_buf = NULL;
>>         int target_len = 0, origin_len = 0;
>> @@ -1145,6 +1160,11 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
>>                 sdd->port_id = pdev->id;
>>         }
>>
>> +       if (sdd->port_conf->fifosize)
>> +               sdd->fifosize = sdd->port_conf->fifosize;
>> +       else
>> +               sdd->fifosize = FIFO_DEPTH(sdd);
>> +
>>         sdd->cur_bpw = 8;
>>
>>         sdd->tx_dma.direction = DMA_MEM_TO_DEV;
>> @@ -1234,7 +1254,7 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
>>         dev_dbg(&pdev->dev, "Samsung SoC SPI Driver loaded for Bus SPI-%d with %d Targets attached\n",
>>                                         sdd->port_id, host->num_chipselect);
>>         dev_dbg(&pdev->dev, "\tIOmem=[%pR]\tFIFO %dbytes\n",
>> -                                       mem_res, FIFO_DEPTH(sdd));
>> +                                       mem_res, sdd->fifosize);
>>
>>         pm_runtime_mark_last_busy(&pdev->dev);
>>         pm_runtime_put_autosuspend(&pdev->dev);
>> @@ -1362,6 +1382,18 @@ static const struct dev_pm_ops s3c64xx_spi_pm = {
>>                            s3c64xx_spi_runtime_resume, NULL)
>>  };
>>
>> +static const struct s3c64xx_spi_port_config gs101_spi_port_config = {
>> +       .fifosize       = 64,
> 
> I think if you rework the the .fifo_lvl_mask, replacing it with
> .fifosize, you should also do next things in this series:
>   1. Rework it for all supported (existing) chips in this driver
>   2. Provide fifosize property for each SPI node for all existing dts
> that use this driver
>   3. Get rid of .fifo_lvl_mask for good. But the compatibility with
> older kernels has to be taken into the account here as well.

We can't get rid of the .fifo_lvl_mask entirely because we need to be
backward compatible with the device tree files that we have now.

> 
> Otherwise it looks like a half attempt and not finished, only creating
> a duplicated property/struct field for the same (already existing)
> thing. Because it's completely possible to do the same using just
> .fifo_lvl_mask without introducing new fields or properties. If it

Using fifo_lvl_mask works but is wrong on multiple levels.
As the code is now, the device tree spi alias is used as an index in the
fifo_lvl_mask to determine the FIFO depth. I find it unacceptable to
have a dependency on an alias in a driver. Not specifying an alias will
make the probe fail, which is even worse. Also, the fifo_lvl_mask value
does not reflect the FIFO level reg field. This is incorrect as we use
partial register fields and is misleading. Other problem is that the
fifo_lvl_mask value is used to determine the FIFO depth which is also
incorrect. The FIFO depth is dictated by the SoC implementing the IP,
not by the FIFO_LVL register field. Having in mind these reasons I
marked the fifo_lvl_mask and the port_id as deprecated in the next
patch, we shouldn't use fifo_lvl_mask or the alias anymore.

In what concerns your first 2 points, to rework all the compatibles and
to introduce a fifosize property, I agree it would be nice to do it, but
it's not mandatory, we can work in an incremental fashion. Emphasizing
what is wrong, marking things as deprecated and guiding contributors on
how things should be handled is good too, which I tried in the next
patch. Anyway, I'll check what the reworking would involve, and if I
think it wouldn't take me a terrible amount of time, I'll do it.

> seems to much -- maybe just use .fifo_lvl_mask for now, and do all
> that reworking properly later, in a separate patch series?
> 

But that means to add gs101 and then to come with patches updating what
I just proposed, and I'm not thrilled about it.

Cheers,
ta

Re: [PATCH 19/21] spi: s3c64xx: add support for google,gs101-spi

[Sam Protsenko]

On Wed, Jan 24, 2024 at 4:40 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
> Hi, Sam! Thanks for the review!
>
> On 1/23/24 19:25, Sam Protsenko wrote:
> > On Tue, Jan 23, 2024 at 9:34 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
> >>
> >> Add support for GS101 SPI. All the SPI nodes on GS101 have 64 bytes
> >> FIFOs, infer the FIFO size from the compatible. GS101 allows just 32bit
> >> register accesses, otherwise a Serror Interrupt is raised. Do the write
> >> reg accesses in 32 bits.
> >>
> >> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> >> ---
> >
> > I counted 3 different features in this patch. Would be better to split
> > it correspondingly into 3 patches, to make patches atomic:
> >
> >   1. I/O width
> >   2. FIFO size
>
> I kept these 2 in the same patch as gs101 to exemplify their use by
> gs101. But I'm also fine splitting the patch in 3, will do in v2.
>
> >   3. Adding support for gs101
> >
> > And I'm not really convinced about FIFO size change.
>
> I'll explain why it's needed below.
>
> >
> >>  drivers/spi/spi-s3c64xx.c | 50 +++++++++++++++++++++++++++++++++------
> >>  1 file changed, 43 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> >> index 62671b2d594a..c4ddd2859ba4 100644
> >> --- a/drivers/spi/spi-s3c64xx.c
> >> +++ b/drivers/spi/spi-s3c64xx.c
> >> @@ -20,6 +20,7 @@
> >>
> >>  #define MAX_SPI_PORTS                          12
> >>  #define S3C64XX_SPI_QUIRK_CS_AUTO              BIT(1)
> >> +#define S3C64XX_SPI_GS1O1_32BIT_REG_IO_WIDTH   BIT(2)
> >>  #define AUTOSUSPEND_TIMEOUT                    2000
> >>
> >>  /* Registers and bit-fields */
> >> @@ -131,6 +132,7 @@ struct s3c64xx_spi_dma_data {
> >>   * @rx_lvl_offset: Bit offset of RX_FIFO_LVL bits in SPI_STATUS regiter.
> >>   * @tx_st_done: Bit offset of TX_DONE bit in SPI_STATUS regiter.
> >>   * @clk_div: Internal clock divider
> >> + * @fifosize: size of the FIFO
> >>   * @quirks: Bitmask of known quirks
> >>   * @high_speed: True, if the controller supports HIGH_SPEED_EN bit.
> >>   * @clk_from_cmu: True, if the controller does not include a clock mux and
> >> @@ -149,6 +151,7 @@ struct s3c64xx_spi_port_config {
> >>         int     tx_st_done;
> >>         int     quirks;
> >>         int     clk_div;
> >> +       unsigned int fifosize;
> >>         bool    high_speed;
> >>         bool    clk_from_cmu;
> >>         bool    clk_ioclk;
> >> @@ -175,6 +178,7 @@ struct s3c64xx_spi_port_config {
> >>   * @tx_dma: Local transmit DMA data (e.g. chan and direction)
> >>   * @port_conf: Local SPI port configuartion data
> >>   * @port_id: Port identification number
> >> + * @fifosize: size of the FIFO for this port
> >>   */
> >>  struct s3c64xx_spi_driver_data {
> >>         void __iomem                    *regs;
> >> @@ -194,6 +198,7 @@ struct s3c64xx_spi_driver_data {
> >>         struct s3c64xx_spi_dma_data     tx_dma;
> >>         const struct s3c64xx_spi_port_config    *port_conf;
> >>         unsigned int                    port_id;
> >> +       unsigned int                    fifosize;
> >>  };
> >>
> >>  static void s3c64xx_flush_fifo(struct s3c64xx_spi_driver_data *sdd)
> >> @@ -403,7 +408,7 @@ static bool s3c64xx_spi_can_dma(struct spi_controller *host,
> >>         struct s3c64xx_spi_driver_data *sdd = spi_controller_get_devdata(host);
> >>
> >>         if (sdd->rx_dma.ch && sdd->tx_dma.ch)
> >> -               return xfer->len > FIFO_DEPTH(sdd);
> >> +               return xfer->len > sdd->fifosize;
> >>
> >>         return false;
> >>  }
> >> @@ -447,12 +452,22 @@ static int s3c64xx_enable_datapath(struct s3c64xx_spi_driver_data *sdd,
> >>                                         xfer->tx_buf, xfer->len / 4);
> >>                                 break;
> >>                         case 16:
> >> -                               iowrite16_rep(regs + S3C64XX_SPI_TX_DATA,
> >> -                                       xfer->tx_buf, xfer->len / 2);
> >> +                               if (sdd->port_conf->quirks &
> >> +                                   S3C64XX_SPI_GS1O1_32BIT_REG_IO_WIDTH)
> >> +                                       iowrite16_32_rep(regs + S3C64XX_SPI_TX_DATA,
> >> +                                                        xfer->tx_buf, xfer->len / 2);
> >> +                               else
> >> +                                       iowrite16_rep(regs + S3C64XX_SPI_TX_DATA,
> >> +                                                     xfer->tx_buf, xfer->len / 2);
> >>                                 break;
> >>                         default:
> >> -                               iowrite8_rep(regs + S3C64XX_SPI_TX_DATA,
> >> -                                       xfer->tx_buf, xfer->len);
> >> +                               if (sdd->port_conf->quirks &
> >> +                                   S3C64XX_SPI_GS1O1_32BIT_REG_IO_WIDTH)
> >> +                                       iowrite8_32_rep(regs + S3C64XX_SPI_TX_DATA,
> >> +                                                       xfer->tx_buf, xfer->len);
> >> +                               else
> >> +                                       iowrite8_rep(regs + S3C64XX_SPI_TX_DATA,
> >> +                                                    xfer->tx_buf, xfer->len);
> >>                                 break;
> >>                         }
> >>                 }
> >> @@ -696,7 +711,7 @@ static int s3c64xx_spi_transfer_one(struct spi_controller *host,
> >>                                     struct spi_transfer *xfer)
> >>  {
> >>         struct s3c64xx_spi_driver_data *sdd = spi_controller_get_devdata(host);
> >> -       const unsigned int fifo_len = FIFO_DEPTH(sdd);
> >> +       const unsigned int fifo_len = sdd->fifosize;
> >>         const void *tx_buf = NULL;
> >>         void *rx_buf = NULL;
> >>         int target_len = 0, origin_len = 0;
> >> @@ -1145,6 +1160,11 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
> >>                 sdd->port_id = pdev->id;
> >>         }
> >>
> >> +       if (sdd->port_conf->fifosize)
> >> +               sdd->fifosize = sdd->port_conf->fifosize;
> >> +       else
> >> +               sdd->fifosize = FIFO_DEPTH(sdd);
> >> +
> >>         sdd->cur_bpw = 8;
> >>
> >>         sdd->tx_dma.direction = DMA_MEM_TO_DEV;
> >> @@ -1234,7 +1254,7 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
> >>         dev_dbg(&pdev->dev, "Samsung SoC SPI Driver loaded for Bus SPI-%d with %d Targets attached\n",
> >>                                         sdd->port_id, host->num_chipselect);
> >>         dev_dbg(&pdev->dev, "\tIOmem=[%pR]\tFIFO %dbytes\n",
> >> -                                       mem_res, FIFO_DEPTH(sdd));
> >> +                                       mem_res, sdd->fifosize);
> >>
> >>         pm_runtime_mark_last_busy(&pdev->dev);
> >>         pm_runtime_put_autosuspend(&pdev->dev);
> >> @@ -1362,6 +1382,18 @@ static const struct dev_pm_ops s3c64xx_spi_pm = {
> >>                            s3c64xx_spi_runtime_resume, NULL)
> >>  };
> >>
> >> +static const struct s3c64xx_spi_port_config gs101_spi_port_config = {
> >> +       .fifosize       = 64,
> >
> > I think if you rework the the .fifo_lvl_mask, replacing it with
> > .fifosize, you should also do next things in this series:
> >   1. Rework it for all supported (existing) chips in this driver
> >   2. Provide fifosize property for each SPI node for all existing dts
> > that use this driver
> >   3. Get rid of .fifo_lvl_mask for good. But the compatibility with
> > older kernels has to be taken into the account here as well.
>
> We can't get rid of the .fifo_lvl_mask entirely because we need to be
> backward compatible with the device tree files that we have now.
>
> >
> > Otherwise it looks like a half attempt and not finished, only creating
> > a duplicated property/struct field for the same (already existing)
> > thing. Because it's completely possible to do the same using just
> > .fifo_lvl_mask without introducing new fields or properties. If it
>
> Using fifo_lvl_mask works but is wrong on multiple levels.
> As the code is now, the device tree spi alias is used as an index in the
> fifo_lvl_mask to determine the FIFO depth. I find it unacceptable to
> have a dependency on an alias in a driver. Not specifying an alias will
> make the probe fail, which is even worse. Also, the fifo_lvl_mask value

Ok, I think that's a valid point. I probably missed the alias part
when reading the patch description. I also understand we can't just
remove .fifo_lvl_mask right now, as we have to keep the compatibility
with older/existing out-of-tree device trees, so that the user can
update the kernel image separately.

> does not reflect the FIFO level reg field. This is incorrect as we use
> partial register fields and is misleading. Other problem is that the
> fifo_lvl_mask value is used to determine the FIFO depth which is also
> incorrect. The FIFO depth is dictated by the SoC implementing the IP,
> not by the FIFO_LVL register field. Having in mind these reasons I
> marked the fifo_lvl_mask and the port_id as deprecated in the next
> patch, we shouldn't use fifo_lvl_mask or the alias anymore.
>
> In what concerns your first 2 points, to rework all the compatibles and
> to introduce a fifosize property, I agree it would be nice to do it, but
> it's not mandatory, we can work in an incremental fashion. Emphasizing
> what is wrong, marking things as deprecated and guiding contributors on
> how things should be handled is good too, which I tried in the next
> patch. Anyway, I'll check what the reworking would involve, and if I
> think it wouldn't take me a terrible amount of time, I'll do it.
>

From what I understand, that shouldn't be very hard to do, just a
matter of adding fifosize property to all dts's existing upstream.
That would also provide a good example to follow for anyone who wants
to add the support for new compatibles. But of course I can't ask you
to do the extra work. My point is, with that item done, the first
transition step would be finished right away. And the remaining step
would be to have a strategy for .fifo_lvl_mask removal. I wonder what
maintainers can suggest on that matter, and if it's doable at all.

Btw, just a thought: maybe also add "deprecated" comment to each line
of code where .fifo_lvl_mask is being assigned, just to make sure
noone follows that style in the future (as people often tend to
copy-paste existing implementation)? Because obviously we can't remove
those lines for now.

> > seems to much -- maybe just use .fifo_lvl_mask for now, and do all
> > that reworking properly later, in a separate patch series?
> >
>
> But that means to add gs101 and then to come with patches updating what
> I just proposed, and I'm not thrilled about it.
>

Got it. That's fine with me. I think we don't have to have everything
super-granular w.r.t. patch series split. But I'd still argue that
splitting this particular patch by 3 patches would make things more
atomic and thus better.

> Cheers,
> ta

Re: [PATCH 16/21] spi: s3c64xx: add missing blank line after declaration

[Sam Protsenko]

On Wed, Jan 24, 2024 at 3:54 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
>
>
> On 1/23/24 19:28, Sam Protsenko wrote:
> > On Tue, Jan 23, 2024 at 9:34 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
> >>
> >> Add missing blank line after declaration. Move initialization in the
> >> body of the function.
> >>
> >> 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 f5474f3b3920..2abf5994080a 100644
> >> --- a/drivers/spi/spi-s3c64xx.c
> >> +++ b/drivers/spi/spi-s3c64xx.c
> >> @@ -1273,8 +1273,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);
> >
> > Why not just moving the empty line below the declaration block,
> > keeping the initialization on the variable declaration line?
> >
>
> just a matter of personal preference. I find it ugly to do an
> initialization at variable declaration and then to immediately check the
> return value in the body of the function. But I'll do as you say, just
> cosmetics anyway.

That's not like "do as I say", I'm just a mere reviewer anyway, so
it's just my opinion :) You can leave it as is, and I kinda can see
your point now (having actual logical operation executed in main body
rather than in initialization list):

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

Re: [PATCH 19/21] spi: s3c64xx: add support for google,gs101-spi

[Mark Brown]

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

On Wed, Jan 24, 2024 at 01:43:55PM -0600, Sam Protsenko wrote:
> On Wed, Jan 24, 2024 at 4:40 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:

> > Using fifo_lvl_mask works but is wrong on multiple levels.
> > As the code is now, the device tree spi alias is used as an index in the
> > fifo_lvl_mask to determine the FIFO depth. I find it unacceptable to
> > have a dependency on an alias in a driver. Not specifying an alias will
> > make the probe fail, which is even worse. Also, the fifo_lvl_mask value

> Ok, I think that's a valid point. I probably missed the alias part
> when reading the patch description. I also understand we can't just
> remove .fifo_lvl_mask right now, as we have to keep the compatibility
> with older/existing out-of-tree device trees, so that the user can
> update the kernel image separately.

I don't really agree here, for a given compatible the FIFO depth is
known so it's redundant to specify and it's much simpler to correct
issues if we're not overspecifying things in the DT.

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

Re: [PATCH 00/21] spi: s3c64xx: winter cleanup and gs101 support

[Andi Shyti]

Hi Tudor,

> >> The patch set cleans a bit the driver and adds support for gs101 SPI.
> >>
> >> Apart of the SPI patches, I added support for iowrite{8,16}_32 accessors
> >> in asm-generic/io.h. This will allow devices that require 32 bits
> >> register accesses to write data in chunks of 8 or 16 bits (a typical use
> >> case is SPI, where clients can request transfers in words of 8 bits for
> >> example). GS101 only allows 32bit register accesses otherwise it raisses
> >> a Serror Interrupt and hangs the system, thus the accessors are needed
> >> here. If the accessors are fine, I expect they'll be queued either to
> >> the SPI tree or to the ASM header files tree, but by providing an
> >> immutable tag, so that the other tree can merge them too.
> >>
> >> The SPI patches were tested with the spi-loopback-test on the gs101
> >> controller.
> > 
> > The reformatting in this series will conflict with the SPI changes in:
> > 
> >    https://lore.kernel.org/r/20240120012948.8836-1-semen.protsenko@linaro.org
> > 
> > Can you please pull those into this series or otherwise coordinate?
> 
> ah, I haven't noticed Sam's updates. I'll rebase on top of his set and
> adapt if necessary. I'll review that set in a sec.

it's a long series, please give it a few days before resending
it.

Thanks,
Andi

Re: [PATCH 00/21] spi: s3c64xx: winter cleanup and gs101 support

[Sam Protsenko]

On Thu, Jan 25, 2024 at 4:25 PM Andi Shyti <andi.shyti@kernel.org> wrote:
>
> Hi Tudor,
>
> > >> The patch set cleans a bit the driver and adds support for gs101 SPI.
> > >>
> > >> Apart of the SPI patches, I added support for iowrite{8,16}_32 accessors
> > >> in asm-generic/io.h. This will allow devices that require 32 bits
> > >> register accesses to write data in chunks of 8 or 16 bits (a typical use
> > >> case is SPI, where clients can request transfers in words of 8 bits for
> > >> example). GS101 only allows 32bit register accesses otherwise it raisses
> > >> a Serror Interrupt and hangs the system, thus the accessors are needed
> > >> here. If the accessors are fine, I expect they'll be queued either to
> > >> the SPI tree or to the ASM header files tree, but by providing an
> > >> immutable tag, so that the other tree can merge them too.
> > >>
> > >> The SPI patches were tested with the spi-loopback-test on the gs101
> > >> controller.
> > >
> > > The reformatting in this series will conflict with the SPI changes in:
> > >
> > >    https://lore.kernel.org/r/20240120012948.8836-1-semen.protsenko@linaro.org
> > >
> > > Can you please pull those into this series or otherwise coordinate?
> >
> > ah, I haven't noticed Sam's updates. I'll rebase on top of his set and
> > adapt if necessary. I'll review that set in a sec.
>
> it's a long series, please give it a few days before resending
> it.
>

Also, I recommend splitting it up in a way I suggested before:

  1. First add gs101 support with minimal amount of patches (without
.fifosize introduction, etc)
  2. Then do all those cleanups and reworks on top of that

The reason why I think it's better than doing that vice-versa is that
I feel (2) can take a lot of time/review rounds to get polished and
accepted. So this way you can make sure gs101 support gets applied
sooner. It'll also make it easier to do the backporting work later, if
that's ever needed.

> Thanks,
> Andi

Re: [PATCH 07/21] spi: s3c64xx: use bitfield access macros

[Andi Shyti]

Hi Tudor,

On Tue, Jan 23, 2024 at 03:34:06PM +0000, Tudor Ambarus wrote:
> Use the bitfield access macros in order to clean and to make the driver
> easier to read.

most of the changes done here are allignment. I would mention it
in the log.

> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> ---

...

> -#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(15, 0)

I find it easier as 0xff to be honest, but I'm not going to be
picky.

> +
> +#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

I personally find this pattern harder to read. Perhaps you can
already define these as FIELD_PREP here.

> +#define S3C64XX_SPI_MAX_TRAILCNT_MASK		GENMASK(28, 19)
> +#define S3C64XX_SPI_MODE_BUS_TSZ_MASK		GENMASK(18, 17)

...

> -#define S3C64XX_SPI_FBCLK_MSK			(3<<0)
> +#define S3C64XX_SPI_FBCLK_MASK			GENMASK(1, 0)

0x3 to me is more understandable than (3<<0) and GENMASK(1, 0).
Bit operation defines should be used when they really simplify
the reading. But when they make it more difficult, then, I don't
see the point.

Overall looks good, though.

Andi