[PATCH 1/3 v2] spi: s3c64xx: Delete unused boardfile helpers

[PATCH 1/3 v2] spi: s3c64xx: Delete unused boardfile helpers

[Linus Walleij]

The helpers to use SPI host 1 and 2 are unused in the kernel
and taking up space and maintenance hours. New systems should
use device tree and not this, so delete the code.

Cc: linux-samsung-soc@vger.kernel.org
Cc: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
Cc: Sylwester Nawrocki <snawrocki@kernel.org>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1-v2:
- Split out to separate patch
---
 arch/arm/mach-s3c/Kconfig                 | 12 ----
 arch/arm/mach-s3c/devs.c                  | 72 -----------------------
 arch/arm/mach-s3c/setup-spi-s3c64xx.c     |  9 ---
 arch/arm/mach-s3c/spi-core-s3c24xx.h      |  6 --
 include/linux/platform_data/spi-s3c64xx.h |  8 ---
 5 files changed, 107 deletions(-)

diff --git a/arch/arm/mach-s3c/Kconfig b/arch/arm/mach-s3c/Kconfig
index 25606e668cf9..1899fc3f44fd 100644
--- a/arch/arm/mach-s3c/Kconfig
+++ b/arch/arm/mach-s3c/Kconfig
@@ -191,18 +191,6 @@ config S3C64XX_DEV_SPI0
 	  Compile in platform device definitions for S3C64XX's type
 	  SPI controller 0
 
-config S3C64XX_DEV_SPI1
-	bool
-	help
-	  Compile in platform device definitions for S3C64XX's type
-	  SPI controller 1
-
-config S3C64XX_DEV_SPI2
-	bool
-	help
-	  Compile in platform device definitions for S3C64XX's type
-	  SPI controller 2
-
 config SAMSUNG_DEV_TS
 	bool
 	help
diff --git a/arch/arm/mach-s3c/devs.c b/arch/arm/mach-s3c/devs.c
index 06dec64848f9..9f086aee862b 100644
--- a/arch/arm/mach-s3c/devs.c
+++ b/arch/arm/mach-s3c/devs.c
@@ -1125,75 +1125,3 @@ void __init s3c64xx_spi0_set_platdata(int (*cfg_gpio)(void), int src_clk_nr,
 	s3c_set_platdata(&pd, sizeof(pd), &s3c64xx_device_spi0);
 }
 #endif /* CONFIG_S3C64XX_DEV_SPI0 */
-
-#ifdef CONFIG_S3C64XX_DEV_SPI1
-static struct resource s3c64xx_spi1_resource[] = {
-	[0] = DEFINE_RES_MEM(S3C_PA_SPI1, SZ_256),
-	[1] = DEFINE_RES_IRQ(IRQ_SPI1),
-};
-
-struct platform_device s3c64xx_device_spi1 = {
-	.name		= "s3c6410-spi",
-	.id		= 1,
-	.num_resources	= ARRAY_SIZE(s3c64xx_spi1_resource),
-	.resource	= s3c64xx_spi1_resource,
-	.dev = {
-		.dma_mask		= &samsung_device_dma_mask,
-		.coherent_dma_mask	= DMA_BIT_MASK(32),
-	},
-};
-
-void __init s3c64xx_spi1_set_platdata(int (*cfg_gpio)(void), int src_clk_nr,
-						int num_cs)
-{
-	struct s3c64xx_spi_info pd;
-
-	/* Reject invalid configuration */
-	if (!num_cs || src_clk_nr < 0) {
-		pr_err("%s: Invalid SPI configuration\n", __func__);
-		return;
-	}
-
-	pd.num_cs = num_cs;
-	pd.src_clk_nr = src_clk_nr;
-	pd.cfg_gpio = (cfg_gpio) ? cfg_gpio : s3c64xx_spi1_cfg_gpio;
-
-	s3c_set_platdata(&pd, sizeof(pd), &s3c64xx_device_spi1);
-}
-#endif /* CONFIG_S3C64XX_DEV_SPI1 */
-
-#ifdef CONFIG_S3C64XX_DEV_SPI2
-static struct resource s3c64xx_spi2_resource[] = {
-	[0] = DEFINE_RES_MEM(S3C_PA_SPI2, SZ_256),
-	[1] = DEFINE_RES_IRQ(IRQ_SPI2),
-};
-
-struct platform_device s3c64xx_device_spi2 = {
-	.name		= "s3c6410-spi",
-	.id		= 2,
-	.num_resources	= ARRAY_SIZE(s3c64xx_spi2_resource),
-	.resource	= s3c64xx_spi2_resource,
-	.dev = {
-		.dma_mask		= &samsung_device_dma_mask,
-		.coherent_dma_mask	= DMA_BIT_MASK(32),
-	},
-};
-
-void __init s3c64xx_spi2_set_platdata(int (*cfg_gpio)(void), int src_clk_nr,
-						int num_cs)
-{
-	struct s3c64xx_spi_info pd;
-
-	/* Reject invalid configuration */
-	if (!num_cs || src_clk_nr < 0) {
-		pr_err("%s: Invalid SPI configuration\n", __func__);
-		return;
-	}
-
-	pd.num_cs = num_cs;
-	pd.src_clk_nr = src_clk_nr;
-	pd.cfg_gpio = (cfg_gpio) ? cfg_gpio : s3c64xx_spi2_cfg_gpio;
-
-	s3c_set_platdata(&pd, sizeof(pd), &s3c64xx_device_spi2);
-}
-#endif /* CONFIG_S3C64XX_DEV_SPI2 */
diff --git a/arch/arm/mach-s3c/setup-spi-s3c64xx.c b/arch/arm/mach-s3c/setup-spi-s3c64xx.c
index efcf78d41585..497aff71c29c 100644
--- a/arch/arm/mach-s3c/setup-spi-s3c64xx.c
+++ b/arch/arm/mach-s3c/setup-spi-s3c64xx.c
@@ -16,12 +16,3 @@ int s3c64xx_spi0_cfg_gpio(void)
 	return 0;
 }
 #endif
-
-#ifdef CONFIG_S3C64XX_DEV_SPI1
-int s3c64xx_spi1_cfg_gpio(void)
-{
-	s3c_gpio_cfgall_range(S3C64XX_GPC(4), 3,
-				S3C_GPIO_SFN(2), S3C_GPIO_PULL_UP);
-	return 0;
-}
-#endif
diff --git a/arch/arm/mach-s3c/spi-core-s3c24xx.h b/arch/arm/mach-s3c/spi-core-s3c24xx.h
index 057667469cc3..919c5fd0c9af 100644
--- a/arch/arm/mach-s3c/spi-core-s3c24xx.h
+++ b/arch/arm/mach-s3c/spi-core-s3c24xx.h
@@ -16,12 +16,6 @@ static inline void s3c24xx_spi_setname(char *name)
 #ifdef CONFIG_S3C64XX_DEV_SPI0
 	s3c64xx_device_spi0.name = name;
 #endif
-#ifdef CONFIG_S3C64XX_DEV_SPI1
-	s3c64xx_device_spi1.name = name;
-#endif
-#ifdef CONFIG_S3C64XX_DEV_SPI2
-	s3c64xx_device_spi2.name = name;
-#endif
 }
 
 #endif /* __PLAT_S3C_SPI_CORE_S3C24XX_H */
diff --git a/include/linux/platform_data/spi-s3c64xx.h b/include/linux/platform_data/spi-s3c64xx.h
index 773daf7915a3..19d690f34670 100644
--- a/include/linux/platform_data/spi-s3c64xx.h
+++ b/include/linux/platform_data/spi-s3c64xx.h
@@ -52,17 +52,9 @@ struct s3c64xx_spi_info {
  */
 extern void s3c64xx_spi0_set_platdata(int (*cfg_gpio)(void), int src_clk_nr,
 						int num_cs);
-extern void s3c64xx_spi1_set_platdata(int (*cfg_gpio)(void), int src_clk_nr,
-						int num_cs);
-extern void s3c64xx_spi2_set_platdata(int (*cfg_gpio)(void), int src_clk_nr,
-						int num_cs);
 
 /* defined by architecture to configure gpio */
 extern int s3c64xx_spi0_cfg_gpio(void);
-extern int s3c64xx_spi1_cfg_gpio(void);
-extern int s3c64xx_spi2_cfg_gpio(void);
 
 extern struct s3c64xx_spi_info s3c64xx_spi0_pdata;
-extern struct s3c64xx_spi_info s3c64xx_spi1_pdata;
-extern struct s3c64xx_spi_info s3c64xx_spi2_pdata;
 #endif /*__SPI_S3C64XX_H */
-- 
2.34.1

[PATCH 2/3 v2] spi: s3c64xx: Drop custom gpio setup argument

[Linus Walleij]

The SPI0 platform population function was taking a custom
gpio setup callback but the only user pass NULL as
argument so drop this argument.

Cc: linux-samsung-soc@vger.kernel.org
Cc: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
Cc: Sylwester Nawrocki <snawrocki@kernel.org>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1-v2:
- Split out to separate patch
---
 arch/arm/mach-s3c/devs.c                  | 5 ++---
 arch/arm/mach-s3c/mach-crag6410.c         | 2 +-
 include/linux/platform_data/spi-s3c64xx.h | 4 +---
 3 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/arch/arm/mach-s3c/devs.c b/arch/arm/mach-s3c/devs.c
index 9f086aee862b..1e266fc24f9b 100644
--- a/arch/arm/mach-s3c/devs.c
+++ b/arch/arm/mach-s3c/devs.c
@@ -1107,8 +1107,7 @@ struct platform_device s3c64xx_device_spi0 = {
 	},
 };
 
-void __init s3c64xx_spi0_set_platdata(int (*cfg_gpio)(void), int src_clk_nr,
-						int num_cs)
+void __init s3c64xx_spi0_set_platdata(int src_clk_nr, int num_cs)
 {
 	struct s3c64xx_spi_info pd;
 
@@ -1120,7 +1119,7 @@ void __init s3c64xx_spi0_set_platdata(int (*cfg_gpio)(void), int src_clk_nr,
 
 	pd.num_cs = num_cs;
 	pd.src_clk_nr = src_clk_nr;
-	pd.cfg_gpio = (cfg_gpio) ? cfg_gpio : s3c64xx_spi0_cfg_gpio;
+	pd.cfg_gpio = s3c64xx_spi0_cfg_gpio;
 
 	s3c_set_platdata(&pd, sizeof(pd), &s3c64xx_device_spi0);
 }
diff --git a/arch/arm/mach-s3c/mach-crag6410.c b/arch/arm/mach-s3c/mach-crag6410.c
index 4a12c75d407f..41f0aba2d2fd 100644
--- a/arch/arm/mach-s3c/mach-crag6410.c
+++ b/arch/arm/mach-s3c/mach-crag6410.c
@@ -856,7 +856,7 @@ static void __init crag6410_machine_init(void)
 	i2c_register_board_info(1, i2c_devs1, ARRAY_SIZE(i2c_devs1));
 
 	samsung_keypad_set_platdata(&crag6410_keypad_data);
-	s3c64xx_spi0_set_platdata(NULL, 0, 2);
+	s3c64xx_spi0_set_platdata(0, 2);
 
 	pwm_add_table(crag6410_pwm_lookup, ARRAY_SIZE(crag6410_pwm_lookup));
 	platform_add_devices(crag6410_devices, ARRAY_SIZE(crag6410_devices));
diff --git a/include/linux/platform_data/spi-s3c64xx.h b/include/linux/platform_data/spi-s3c64xx.h
index 19d690f34670..10890a4b55b9 100644
--- a/include/linux/platform_data/spi-s3c64xx.h
+++ b/include/linux/platform_data/spi-s3c64xx.h
@@ -43,15 +43,13 @@ struct s3c64xx_spi_info {
 /**
  * s3c64xx_spi_set_platdata - SPI Controller configure callback by the board
  *				initialization code.
- * @cfg_gpio: Pointer to gpio setup function.
  * @src_clk_nr: Clock the SPI controller is to use to generate SPI clocks.
  * @num_cs: Number of elements in the 'cs' array.
  *
  * Call this from machine init code for each SPI Controller that
  * has some chips attached to it.
  */
-extern void s3c64xx_spi0_set_platdata(int (*cfg_gpio)(void), int src_clk_nr,
-						int num_cs);
+extern void s3c64xx_spi0_set_platdata(int src_clk_nr, int num_cs);
 
 /* defined by architecture to configure gpio */
 extern int s3c64xx_spi0_cfg_gpio(void);
-- 
2.34.1

[PATCH 3/3 v2] spi: s3c64xx: Convert to use GPIO descriptors

[Linus Walleij]

Convert the S3C64xx SPI host to use GPIO descriptors.

Provide GPIO descriptor tables for the one user with CS
0 and 1.

Cc: linux-samsung-soc@vger.kernel.org
Cc: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
Cc: Sylwester Nawrocki <snawrocki@kernel.org>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1-v2:
- Split off code cleaning to separate patches
---
 arch/arm/mach-s3c/mach-crag6410-module.c  | 13 ------
 arch/arm/mach-s3c/mach-crag6410.c         | 11 +++++
 drivers/spi/spi-s3c64xx.c                 | 50 +++++------------------
 include/linux/platform_data/spi-s3c64xx.h |  4 +-
 4 files changed, 23 insertions(+), 55 deletions(-)

diff --git a/arch/arm/mach-s3c/mach-crag6410-module.c b/arch/arm/mach-s3c/mach-crag6410-module.c
index 407ad493493e..5d1d4b67a4b7 100644
--- a/arch/arm/mach-s3c/mach-crag6410-module.c
+++ b/arch/arm/mach-s3c/mach-crag6410-module.c
@@ -32,10 +32,6 @@
 
 #include "crag6410.h"
 
-static struct s3c64xx_spi_csinfo wm0010_spi_csinfo = {
-	.line = S3C64XX_GPC(3),
-};
-
 static struct wm0010_pdata wm0010_pdata = {
 	.gpio_reset = S3C64XX_GPN(6),
 	.reset_active_high = 1, /* Active high for Glenfarclas Rev 2 */
@@ -49,7 +45,6 @@ static struct spi_board_info wm1253_devs[] = {
 		.chip_select	= 0,
 		.mode		= SPI_MODE_0,
 		.irq		= S3C_EINT(4),
-		.controller_data = &wm0010_spi_csinfo,
 		.platform_data = &wm0010_pdata,
 	},
 };
@@ -62,7 +57,6 @@ static struct spi_board_info balblair_devs[] = {
 		.chip_select	= 0,
 		.mode		= SPI_MODE_0,
 		.irq		= S3C_EINT(4),
-		.controller_data = &wm0010_spi_csinfo,
 		.platform_data = &wm0010_pdata,
 	},
 };
@@ -229,10 +223,6 @@ static struct arizona_pdata wm5102_reva_pdata = {
 	},
 };
 
-static struct s3c64xx_spi_csinfo codec_spi_csinfo = {
-	.line = S3C64XX_GPN(5),
-};
-
 static struct spi_board_info wm5102_reva_spi_devs[] = {
 	[0] = {
 		.modalias	= "wm5102",
@@ -242,7 +232,6 @@ static struct spi_board_info wm5102_reva_spi_devs[] = {
 		.mode		= SPI_MODE_0,
 		.irq		= GLENFARCLAS_PMIC_IRQ_BASE +
 				  WM831X_IRQ_GPIO_2,
-		.controller_data = &codec_spi_csinfo,
 		.platform_data = &wm5102_reva_pdata,
 	},
 };
@@ -275,7 +264,6 @@ static struct spi_board_info wm5102_spi_devs[] = {
 		.mode		= SPI_MODE_0,
 		.irq		= GLENFARCLAS_PMIC_IRQ_BASE +
 				  WM831X_IRQ_GPIO_2,
-		.controller_data = &codec_spi_csinfo,
 		.platform_data = &wm5102_pdata,
 	},
 };
@@ -298,7 +286,6 @@ static struct spi_board_info wm5110_spi_devs[] = {
 		.mode		= SPI_MODE_0,
 		.irq		= GLENFARCLAS_PMIC_IRQ_BASE +
 				  WM831X_IRQ_GPIO_2,
-		.controller_data = &codec_spi_csinfo,
 		.platform_data = &wm5102_reva_pdata,
 	},
 };
diff --git a/arch/arm/mach-s3c/mach-crag6410.c b/arch/arm/mach-s3c/mach-crag6410.c
index 41f0aba2d2fd..e3e0fe897bcc 100644
--- a/arch/arm/mach-s3c/mach-crag6410.c
+++ b/arch/arm/mach-s3c/mach-crag6410.c
@@ -825,6 +825,15 @@ static const struct gpio_led_platform_data gpio_leds_pdata = {
 
 static struct dwc2_hsotg_plat crag6410_hsotg_pdata;
 
+static struct gpiod_lookup_table crag_spi0_gpiod_table = {
+	.dev_id = "s3c6410-spi.0",
+	.table = {
+		GPIO_LOOKUP_IDX("GPIOC", 3, "cs", 0, GPIO_ACTIVE_LOW),
+		GPIO_LOOKUP_IDX("GPION", 5, "cs", 1, GPIO_ACTIVE_LOW),
+		{ },
+	},
+};
+
 static void __init crag6410_machine_init(void)
 {
 	/* Open drain IRQs need pullups */
@@ -856,6 +865,8 @@ static void __init crag6410_machine_init(void)
 	i2c_register_board_info(1, i2c_devs1, ARRAY_SIZE(i2c_devs1));
 
 	samsung_keypad_set_platdata(&crag6410_keypad_data);
+
+	gpiod_add_lookup_table(&crag_spi0_gpiod_table);
 	s3c64xx_spi0_set_platdata(0, 2);
 
 	pwm_add_table(crag6410_pwm_lookup, ARRAY_SIZE(crag6410_pwm_lookup));
diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 8755cd85e83c..5e03c9144bfe 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -13,10 +13,8 @@
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/spi/spi.h>
-#include <linux/gpio.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
-#include <linux/of_gpio.h>
 
 #include <linux/platform_data/spi-s3c64xx.h>
 
@@ -655,6 +653,9 @@ static int s3c64xx_spi_prepare_message(struct spi_master *master,
 	struct spi_device *spi = msg->spi;
 	struct s3c64xx_spi_csinfo *cs = spi->controller_data;
 
+	if (!cs)
+		return 0;
+
 	/* Configure feedback delay */
 	writel(cs->fb_delay & 0x3, sdd->regs + S3C64XX_SPI_FB_CLK);
 
@@ -830,34 +831,16 @@ static int s3c64xx_spi_setup(struct spi_device *spi)
 	if (spi->dev.of_node) {
 		cs = s3c64xx_get_slave_ctrldata(spi);
 		spi->controller_data = cs;
-	} else if (cs) {
-		/* On non-DT platforms the SPI core will set spi->cs_gpio
-		 * to -ENOENT. The GPIO pin used to drive the chip select
-		 * is defined by using platform data so spi->cs_gpio value
-		 * has to be override to have the proper GPIO pin number.
-		 */
-		spi->cs_gpio = cs->line;
 	}
 
-	if (IS_ERR_OR_NULL(cs)) {
+	/* NULL is fine, we just avoid using the FB delay (=0) */
+	if (IS_ERR(cs)) {
 		dev_err(&spi->dev, "No CS for SPI(%d)\n", spi->chip_select);
 		return -ENODEV;
 	}
 
-	if (!spi_get_ctldata(spi)) {
-		if (gpio_is_valid(spi->cs_gpio)) {
-			err = gpio_request_one(spi->cs_gpio, GPIOF_OUT_INIT_HIGH,
-					       dev_name(&spi->dev));
-			if (err) {
-				dev_err(&spi->dev,
-					"Failed to get /CS gpio [%d]: %d\n",
-					spi->cs_gpio, err);
-				goto err_gpio_req;
-			}
-		}
-
+	if (!spi_get_ctldata(spi))
 		spi_set_ctldata(spi, cs);
-	}
 
 	pm_runtime_get_sync(&sdd->pdev->dev);
 
@@ -909,11 +892,9 @@ static int s3c64xx_spi_setup(struct spi_device *spi)
 	/* setup() returns with device de-selected */
 	s3c64xx_spi_set_cs(spi, false);
 
-	if (gpio_is_valid(spi->cs_gpio))
-		gpio_free(spi->cs_gpio);
 	spi_set_ctldata(spi, NULL);
 
-err_gpio_req:
+	/* This was dynamically allocated on the DT path */
 	if (spi->dev.of_node)
 		kfree(cs);
 
@@ -924,19 +905,9 @@ static void s3c64xx_spi_cleanup(struct spi_device *spi)
 {
 	struct s3c64xx_spi_csinfo *cs = spi_get_ctldata(spi);
 
-	if (gpio_is_valid(spi->cs_gpio)) {
-		gpio_free(spi->cs_gpio);
-		if (spi->dev.of_node)
-			kfree(cs);
-		else {
-			/* On non-DT platforms, the SPI core sets
-			 * spi->cs_gpio to -ENOENT and .setup()
-			 * overrides it with the GPIO pin value
-			 * passed using platform data.
-			 */
-			spi->cs_gpio = -ENOENT;
-		}
-	}
+	/* This was dynamically allocated on the DT path */
+	if (spi->dev.of_node)
+		kfree(cs);
 
 	spi_set_ctldata(spi, NULL);
 }
@@ -1131,6 +1102,7 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
 	master->prepare_message = s3c64xx_spi_prepare_message;
 	master->transfer_one = s3c64xx_spi_transfer_one;
 	master->num_chipselect = sci->num_cs;
+	master->use_gpio_descriptors = true;
 	master->dma_alignment = 8;
 	master->bits_per_word_mask = SPI_BPW_MASK(32) | SPI_BPW_MASK(16) |
 					SPI_BPW_MASK(8);
diff --git a/include/linux/platform_data/spi-s3c64xx.h b/include/linux/platform_data/spi-s3c64xx.h
index 10890a4b55b9..391f4dc68742 100644
--- a/include/linux/platform_data/spi-s3c64xx.h
+++ b/include/linux/platform_data/spi-s3c64xx.h
@@ -16,15 +16,13 @@ struct platform_device;
  * struct s3c64xx_spi_csinfo - ChipSelect description
  * @fb_delay: Slave specific feedback delay.
  *            Refer to FB_CLK_SEL register definition in SPI chapter.
- * @line: Custom 'identity' of the CS line.
  *
- * This is per SPI-Slave Chipselect information.
+ * This is per SPI-Slave FB delay information.
  * Allocate and initialize one in machine init code and make the
  * spi_board_info.controller_data point to it.
  */
 struct s3c64xx_spi_csinfo {
 	u8 fb_delay;
-	unsigned line;
 };
 
 /**
-- 
2.34.1

Re: [PATCH 1/3 v2] spi: s3c64xx: Delete unused boardfile helpers

[Krzysztof Kozlowski]

On 18/01/2022 12:09, Linus Walleij wrote:
> The helpers to use SPI host 1 and 2 are unused in the kernel
> and taking up space and maintenance hours. New systems should
> use device tree and not this, so delete the code.
> 
> Cc: linux-samsung-soc@vger.kernel.org
> Cc: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
> Cc: Sylwester Nawrocki <snawrocki@kernel.org>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1-v2:
> - Split out to separate patch
> ---
>  arch/arm/mach-s3c/Kconfig                 | 12 ----
>  arch/arm/mach-s3c/devs.c                  | 72 -----------------------
>  arch/arm/mach-s3c/setup-spi-s3c64xx.c     |  9 ---
>  arch/arm/mach-s3c/spi-core-s3c24xx.h      |  6 --
>  include/linux/platform_data/spi-s3c64xx.h |  8 ---
>  5 files changed, 107 deletions(-)
> 


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>


Best regards,
Krzysztof

Re: [PATCH 2/3 v2] spi: s3c64xx: Drop custom gpio setup argument

[Krzysztof Kozlowski]

On 18/01/2022 12:09, Linus Walleij wrote:
> The SPI0 platform population function was taking a custom
> gpio setup callback but the only user pass NULL as
> argument so drop this argument.
> 
> Cc: linux-samsung-soc@vger.kernel.org
> Cc: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
> Cc: Sylwester Nawrocki <snawrocki@kernel.org>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1-v2:
> - Split out to separate patch
> ---
>  arch/arm/mach-s3c/devs.c                  | 5 ++---
>  arch/arm/mach-s3c/mach-crag6410.c         | 2 +-
>  include/linux/platform_data/spi-s3c64xx.h | 4 +---
>  3 files changed, 4 insertions(+), 7 deletions(-)
> 


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>


Best regards,
Krzysztof

Re: [PATCH 3/3 v2] spi: s3c64xx: Convert to use GPIO descriptors

[Krzysztof Kozlowski]

On 18/01/2022 12:09, Linus Walleij wrote:
> Convert the S3C64xx SPI host to use GPIO descriptors.
> 
> Provide GPIO descriptor tables for the one user with CS
> 0 and 1.
> 
> Cc: linux-samsung-soc@vger.kernel.org
> Cc: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
> Cc: Sylwester Nawrocki <snawrocki@kernel.org>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1-v2:
> - Split off code cleaning to separate patches
> ---

Much easier to read, thanks!


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>


Best regards,
Krzysztof

Re: [PATCH 1/3 v2] spi: s3c64xx: Delete unused boardfile helpers

[Sam Protsenko]

On Tue, 18 Jan 2022 at 13:11, Linus Walleij <linus.walleij@linaro.org> wrote:
>
> The helpers to use SPI host 1 and 2 are unused in the kernel
> and taking up space and maintenance hours. New systems should
> use device tree and not this, so delete the code.
>
> Cc: linux-samsung-soc@vger.kernel.org
> Cc: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
> Cc: Sylwester Nawrocki <snawrocki@kernel.org>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1-v2:
> - Split out to separate patch
> ---

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

>  arch/arm/mach-s3c/Kconfig                 | 12 ----
>  arch/arm/mach-s3c/devs.c                  | 72 -----------------------
>  arch/arm/mach-s3c/setup-spi-s3c64xx.c     |  9 ---
>  arch/arm/mach-s3c/spi-core-s3c24xx.h      |  6 --
>  include/linux/platform_data/spi-s3c64xx.h |  8 ---
>  5 files changed, 107 deletions(-)
>
> diff --git a/arch/arm/mach-s3c/Kconfig b/arch/arm/mach-s3c/Kconfig
> index 25606e668cf9..1899fc3f44fd 100644
> --- a/arch/arm/mach-s3c/Kconfig
> +++ b/arch/arm/mach-s3c/Kconfig
> @@ -191,18 +191,6 @@ config S3C64XX_DEV_SPI0
>           Compile in platform device definitions for S3C64XX's type
>           SPI controller 0
>
> -config S3C64XX_DEV_SPI1
> -       bool
> -       help
> -         Compile in platform device definitions for S3C64XX's type
> -         SPI controller 1
> -
> -config S3C64XX_DEV_SPI2
> -       bool
> -       help
> -         Compile in platform device definitions for S3C64XX's type
> -         SPI controller 2
> -
>  config SAMSUNG_DEV_TS
>         bool
>         help
> diff --git a/arch/arm/mach-s3c/devs.c b/arch/arm/mach-s3c/devs.c
> index 06dec64848f9..9f086aee862b 100644
> --- a/arch/arm/mach-s3c/devs.c
> +++ b/arch/arm/mach-s3c/devs.c
> @@ -1125,75 +1125,3 @@ void __init s3c64xx_spi0_set_platdata(int (*cfg_gpio)(void), int src_clk_nr,
>         s3c_set_platdata(&pd, sizeof(pd), &s3c64xx_device_spi0);
>  }
>  #endif /* CONFIG_S3C64XX_DEV_SPI0 */
> -
> -#ifdef CONFIG_S3C64XX_DEV_SPI1
> -static struct resource s3c64xx_spi1_resource[] = {
> -       [0] = DEFINE_RES_MEM(S3C_PA_SPI1, SZ_256),
> -       [1] = DEFINE_RES_IRQ(IRQ_SPI1),
> -};
> -
> -struct platform_device s3c64xx_device_spi1 = {
> -       .name           = "s3c6410-spi",
> -       .id             = 1,
> -       .num_resources  = ARRAY_SIZE(s3c64xx_spi1_resource),
> -       .resource       = s3c64xx_spi1_resource,
> -       .dev = {
> -               .dma_mask               = &samsung_device_dma_mask,
> -               .coherent_dma_mask      = DMA_BIT_MASK(32),
> -       },
> -};
> -
> -void __init s3c64xx_spi1_set_platdata(int (*cfg_gpio)(void), int src_clk_nr,
> -                                               int num_cs)
> -{
> -       struct s3c64xx_spi_info pd;
> -
> -       /* Reject invalid configuration */
> -       if (!num_cs || src_clk_nr < 0) {
> -               pr_err("%s: Invalid SPI configuration\n", __func__);
> -               return;
> -       }
> -
> -       pd.num_cs = num_cs;
> -       pd.src_clk_nr = src_clk_nr;
> -       pd.cfg_gpio = (cfg_gpio) ? cfg_gpio : s3c64xx_spi1_cfg_gpio;
> -
> -       s3c_set_platdata(&pd, sizeof(pd), &s3c64xx_device_spi1);
> -}
> -#endif /* CONFIG_S3C64XX_DEV_SPI1 */
> -
> -#ifdef CONFIG_S3C64XX_DEV_SPI2
> -static struct resource s3c64xx_spi2_resource[] = {
> -       [0] = DEFINE_RES_MEM(S3C_PA_SPI2, SZ_256),
> -       [1] = DEFINE_RES_IRQ(IRQ_SPI2),
> -};
> -
> -struct platform_device s3c64xx_device_spi2 = {
> -       .name           = "s3c6410-spi",
> -       .id             = 2,
> -       .num_resources  = ARRAY_SIZE(s3c64xx_spi2_resource),
> -       .resource       = s3c64xx_spi2_resource,
> -       .dev = {
> -               .dma_mask               = &samsung_device_dma_mask,
> -               .coherent_dma_mask      = DMA_BIT_MASK(32),
> -       },
> -};
> -
> -void __init s3c64xx_spi2_set_platdata(int (*cfg_gpio)(void), int src_clk_nr,
> -                                               int num_cs)
> -{
> -       struct s3c64xx_spi_info pd;
> -
> -       /* Reject invalid configuration */
> -       if (!num_cs || src_clk_nr < 0) {
> -               pr_err("%s: Invalid SPI configuration\n", __func__);
> -               return;
> -       }
> -
> -       pd.num_cs = num_cs;
> -       pd.src_clk_nr = src_clk_nr;
> -       pd.cfg_gpio = (cfg_gpio) ? cfg_gpio : s3c64xx_spi2_cfg_gpio;
> -
> -       s3c_set_platdata(&pd, sizeof(pd), &s3c64xx_device_spi2);
> -}
> -#endif /* CONFIG_S3C64XX_DEV_SPI2 */
> diff --git a/arch/arm/mach-s3c/setup-spi-s3c64xx.c b/arch/arm/mach-s3c/setup-spi-s3c64xx.c
> index efcf78d41585..497aff71c29c 100644
> --- a/arch/arm/mach-s3c/setup-spi-s3c64xx.c
> +++ b/arch/arm/mach-s3c/setup-spi-s3c64xx.c
> @@ -16,12 +16,3 @@ int s3c64xx_spi0_cfg_gpio(void)
>         return 0;
>  }
>  #endif
> -
> -#ifdef CONFIG_S3C64XX_DEV_SPI1
> -int s3c64xx_spi1_cfg_gpio(void)
> -{
> -       s3c_gpio_cfgall_range(S3C64XX_GPC(4), 3,
> -                               S3C_GPIO_SFN(2), S3C_GPIO_PULL_UP);
> -       return 0;
> -}
> -#endif
> diff --git a/arch/arm/mach-s3c/spi-core-s3c24xx.h b/arch/arm/mach-s3c/spi-core-s3c24xx.h
> index 057667469cc3..919c5fd0c9af 100644
> --- a/arch/arm/mach-s3c/spi-core-s3c24xx.h
> +++ b/arch/arm/mach-s3c/spi-core-s3c24xx.h
> @@ -16,12 +16,6 @@ static inline void s3c24xx_spi_setname(char *name)
>  #ifdef CONFIG_S3C64XX_DEV_SPI0
>         s3c64xx_device_spi0.name = name;
>  #endif
> -#ifdef CONFIG_S3C64XX_DEV_SPI1
> -       s3c64xx_device_spi1.name = name;
> -#endif
> -#ifdef CONFIG_S3C64XX_DEV_SPI2
> -       s3c64xx_device_spi2.name = name;
> -#endif
>  }
>
>  #endif /* __PLAT_S3C_SPI_CORE_S3C24XX_H */
> diff --git a/include/linux/platform_data/spi-s3c64xx.h b/include/linux/platform_data/spi-s3c64xx.h
> index 773daf7915a3..19d690f34670 100644
> --- a/include/linux/platform_data/spi-s3c64xx.h
> +++ b/include/linux/platform_data/spi-s3c64xx.h
> @@ -52,17 +52,9 @@ struct s3c64xx_spi_info {
>   */
>  extern void s3c64xx_spi0_set_platdata(int (*cfg_gpio)(void), int src_clk_nr,
>                                                 int num_cs);
> -extern void s3c64xx_spi1_set_platdata(int (*cfg_gpio)(void), int src_clk_nr,
> -                                               int num_cs);
> -extern void s3c64xx_spi2_set_platdata(int (*cfg_gpio)(void), int src_clk_nr,
> -                                               int num_cs);
>
>  /* defined by architecture to configure gpio */
>  extern int s3c64xx_spi0_cfg_gpio(void);
> -extern int s3c64xx_spi1_cfg_gpio(void);
> -extern int s3c64xx_spi2_cfg_gpio(void);
>
>  extern struct s3c64xx_spi_info s3c64xx_spi0_pdata;
> -extern struct s3c64xx_spi_info s3c64xx_spi1_pdata;
> -extern struct s3c64xx_spi_info s3c64xx_spi2_pdata;
>  #endif /*__SPI_S3C64XX_H */
> --
> 2.34.1
>

Re: [PATCH 2/3 v2] spi: s3c64xx: Drop custom gpio setup argument

[Sam Protsenko]

On Tue, 18 Jan 2022 at 13:11, Linus Walleij <linus.walleij@linaro.org> wrote:
>
> The SPI0 platform population function was taking a custom
> gpio setup callback but the only user pass NULL as
> argument so drop this argument.
>
> Cc: linux-samsung-soc@vger.kernel.org
> Cc: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
> Cc: Sylwester Nawrocki <snawrocki@kernel.org>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1-v2:
> - Split out to separate patch
> ---

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

>  arch/arm/mach-s3c/devs.c                  | 5 ++---
>  arch/arm/mach-s3c/mach-crag6410.c         | 2 +-
>  include/linux/platform_data/spi-s3c64xx.h | 4 +---
>  3 files changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/arch/arm/mach-s3c/devs.c b/arch/arm/mach-s3c/devs.c
> index 9f086aee862b..1e266fc24f9b 100644
> --- a/arch/arm/mach-s3c/devs.c
> +++ b/arch/arm/mach-s3c/devs.c
> @@ -1107,8 +1107,7 @@ struct platform_device s3c64xx_device_spi0 = {
>         },
>  };
>
> -void __init s3c64xx_spi0_set_platdata(int (*cfg_gpio)(void), int src_clk_nr,
> -                                               int num_cs)
> +void __init s3c64xx_spi0_set_platdata(int src_clk_nr, int num_cs)
>  {
>         struct s3c64xx_spi_info pd;
>
> @@ -1120,7 +1119,7 @@ void __init s3c64xx_spi0_set_platdata(int (*cfg_gpio)(void), int src_clk_nr,
>
>         pd.num_cs = num_cs;
>         pd.src_clk_nr = src_clk_nr;
> -       pd.cfg_gpio = (cfg_gpio) ? cfg_gpio : s3c64xx_spi0_cfg_gpio;
> +       pd.cfg_gpio = s3c64xx_spi0_cfg_gpio;
>
>         s3c_set_platdata(&pd, sizeof(pd), &s3c64xx_device_spi0);
>  }
> diff --git a/arch/arm/mach-s3c/mach-crag6410.c b/arch/arm/mach-s3c/mach-crag6410.c
> index 4a12c75d407f..41f0aba2d2fd 100644
> --- a/arch/arm/mach-s3c/mach-crag6410.c
> +++ b/arch/arm/mach-s3c/mach-crag6410.c
> @@ -856,7 +856,7 @@ static void __init crag6410_machine_init(void)
>         i2c_register_board_info(1, i2c_devs1, ARRAY_SIZE(i2c_devs1));
>
>         samsung_keypad_set_platdata(&crag6410_keypad_data);
> -       s3c64xx_spi0_set_platdata(NULL, 0, 2);
> +       s3c64xx_spi0_set_platdata(0, 2);
>
>         pwm_add_table(crag6410_pwm_lookup, ARRAY_SIZE(crag6410_pwm_lookup));
>         platform_add_devices(crag6410_devices, ARRAY_SIZE(crag6410_devices));
> diff --git a/include/linux/platform_data/spi-s3c64xx.h b/include/linux/platform_data/spi-s3c64xx.h
> index 19d690f34670..10890a4b55b9 100644
> --- a/include/linux/platform_data/spi-s3c64xx.h
> +++ b/include/linux/platform_data/spi-s3c64xx.h
> @@ -43,15 +43,13 @@ struct s3c64xx_spi_info {
>  /**
>   * s3c64xx_spi_set_platdata - SPI Controller configure callback by the board
>   *                             initialization code.
> - * @cfg_gpio: Pointer to gpio setup function.
>   * @src_clk_nr: Clock the SPI controller is to use to generate SPI clocks.
>   * @num_cs: Number of elements in the 'cs' array.
>   *
>   * Call this from machine init code for each SPI Controller that
>   * has some chips attached to it.
>   */
> -extern void s3c64xx_spi0_set_platdata(int (*cfg_gpio)(void), int src_clk_nr,
> -                                               int num_cs);
> +extern void s3c64xx_spi0_set_platdata(int src_clk_nr, int num_cs);
>
>  /* defined by architecture to configure gpio */
>  extern int s3c64xx_spi0_cfg_gpio(void);
> --
> 2.34.1
>

Re: [PATCH 3/3 v2] spi: s3c64xx: Convert to use GPIO descriptors

[Sam Protsenko]

On Tue, 18 Jan 2022 at 13:11, Linus Walleij <linus.walleij@linaro.org> wrote:
>
> Convert the S3C64xx SPI host to use GPIO descriptors.
>
> Provide GPIO descriptor tables for the one user with CS
> 0 and 1.
>
> Cc: linux-samsung-soc@vger.kernel.org
> Cc: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
> Cc: Sylwester Nawrocki <snawrocki@kernel.org>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1-v2:
> - Split off code cleaning to separate patches
> ---
>  arch/arm/mach-s3c/mach-crag6410-module.c  | 13 ------
>  arch/arm/mach-s3c/mach-crag6410.c         | 11 +++++
>  drivers/spi/spi-s3c64xx.c                 | 50 +++++------------------
>  include/linux/platform_data/spi-s3c64xx.h |  4 +-
>  4 files changed, 23 insertions(+), 55 deletions(-)
>
> diff --git a/arch/arm/mach-s3c/mach-crag6410-module.c b/arch/arm/mach-s3c/mach-crag6410-module.c
> index 407ad493493e..5d1d4b67a4b7 100644
> --- a/arch/arm/mach-s3c/mach-crag6410-module.c
> +++ b/arch/arm/mach-s3c/mach-crag6410-module.c
> @@ -32,10 +32,6 @@
>
>  #include "crag6410.h"
>
> -static struct s3c64xx_spi_csinfo wm0010_spi_csinfo = {
> -       .line = S3C64XX_GPC(3),
> -};
> -
>  static struct wm0010_pdata wm0010_pdata = {
>         .gpio_reset = S3C64XX_GPN(6),
>         .reset_active_high = 1, /* Active high for Glenfarclas Rev 2 */
> @@ -49,7 +45,6 @@ static struct spi_board_info wm1253_devs[] = {
>                 .chip_select    = 0,
>                 .mode           = SPI_MODE_0,
>                 .irq            = S3C_EINT(4),
> -               .controller_data = &wm0010_spi_csinfo,
>                 .platform_data = &wm0010_pdata,
>         },
>  };
> @@ -62,7 +57,6 @@ static struct spi_board_info balblair_devs[] = {
>                 .chip_select    = 0,
>                 .mode           = SPI_MODE_0,
>                 .irq            = S3C_EINT(4),
> -               .controller_data = &wm0010_spi_csinfo,
>                 .platform_data = &wm0010_pdata,
>         },
>  };
> @@ -229,10 +223,6 @@ static struct arizona_pdata wm5102_reva_pdata = {
>         },
>  };
>
> -static struct s3c64xx_spi_csinfo codec_spi_csinfo = {
> -       .line = S3C64XX_GPN(5),
> -};
> -
>  static struct spi_board_info wm5102_reva_spi_devs[] = {
>         [0] = {
>                 .modalias       = "wm5102",
> @@ -242,7 +232,6 @@ static struct spi_board_info wm5102_reva_spi_devs[] = {
>                 .mode           = SPI_MODE_0,
>                 .irq            = GLENFARCLAS_PMIC_IRQ_BASE +
>                                   WM831X_IRQ_GPIO_2,
> -               .controller_data = &codec_spi_csinfo,
>                 .platform_data = &wm5102_reva_pdata,
>         },
>  };
> @@ -275,7 +264,6 @@ static struct spi_board_info wm5102_spi_devs[] = {
>                 .mode           = SPI_MODE_0,
>                 .irq            = GLENFARCLAS_PMIC_IRQ_BASE +
>                                   WM831X_IRQ_GPIO_2,
> -               .controller_data = &codec_spi_csinfo,
>                 .platform_data = &wm5102_pdata,
>         },
>  };
> @@ -298,7 +286,6 @@ static struct spi_board_info wm5110_spi_devs[] = {
>                 .mode           = SPI_MODE_0,
>                 .irq            = GLENFARCLAS_PMIC_IRQ_BASE +
>                                   WM831X_IRQ_GPIO_2,
> -               .controller_data = &codec_spi_csinfo,
>                 .platform_data = &wm5102_reva_pdata,
>         },
>  };
> diff --git a/arch/arm/mach-s3c/mach-crag6410.c b/arch/arm/mach-s3c/mach-crag6410.c
> index 41f0aba2d2fd..e3e0fe897bcc 100644
> --- a/arch/arm/mach-s3c/mach-crag6410.c
> +++ b/arch/arm/mach-s3c/mach-crag6410.c
> @@ -825,6 +825,15 @@ static const struct gpio_led_platform_data gpio_leds_pdata = {
>
>  static struct dwc2_hsotg_plat crag6410_hsotg_pdata;
>
> +static struct gpiod_lookup_table crag_spi0_gpiod_table = {
> +       .dev_id = "s3c6410-spi.0",
> +       .table = {
> +               GPIO_LOOKUP_IDX("GPIOC", 3, "cs", 0, GPIO_ACTIVE_LOW),
> +               GPIO_LOOKUP_IDX("GPION", 5, "cs", 1, GPIO_ACTIVE_LOW),
> +               { },
> +       },
> +};
> +
>  static void __init crag6410_machine_init(void)
>  {
>         /* Open drain IRQs need pullups */
> @@ -856,6 +865,8 @@ static void __init crag6410_machine_init(void)
>         i2c_register_board_info(1, i2c_devs1, ARRAY_SIZE(i2c_devs1));
>
>         samsung_keypad_set_platdata(&crag6410_keypad_data);
> +
> +       gpiod_add_lookup_table(&crag_spi0_gpiod_table);
>         s3c64xx_spi0_set_platdata(0, 2);
>
>         pwm_add_table(crag6410_pwm_lookup, ARRAY_SIZE(crag6410_pwm_lookup));
> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> index 8755cd85e83c..5e03c9144bfe 100644
> --- a/drivers/spi/spi-s3c64xx.c
> +++ b/drivers/spi/spi-s3c64xx.c
> @@ -13,10 +13,8 @@
>  #include <linux/platform_device.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/spi/spi.h>
> -#include <linux/gpio.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
> -#include <linux/of_gpio.h>
>
>  #include <linux/platform_data/spi-s3c64xx.h>
>
> @@ -655,6 +653,9 @@ static int s3c64xx_spi_prepare_message(struct spi_master *master,
>         struct spi_device *spi = msg->spi;
>         struct s3c64xx_spi_csinfo *cs = spi->controller_data;
>
> +       if (!cs)
> +               return 0;

Shouldn't some error code be returned in that case? Or is it normal case?

> +
>         /* Configure feedback delay */
>         writel(cs->fb_delay & 0x3, sdd->regs + S3C64XX_SPI_FB_CLK);
>
> @@ -830,34 +831,16 @@ static int s3c64xx_spi_setup(struct spi_device *spi)
>         if (spi->dev.of_node) {
>                 cs = s3c64xx_get_slave_ctrldata(spi);
>                 spi->controller_data = cs;
> -       } else if (cs) {
> -               /* On non-DT platforms the SPI core will set spi->cs_gpio
> -                * to -ENOENT. The GPIO pin used to drive the chip select
> -                * is defined by using platform data so spi->cs_gpio value
> -                * has to be override to have the proper GPIO pin number.
> -                */
> -               spi->cs_gpio = cs->line;
>         }
>
> -       if (IS_ERR_OR_NULL(cs)) {
> +       /* NULL is fine, we just avoid using the FB delay (=0) */
> +       if (IS_ERR(cs)) {
>                 dev_err(&spi->dev, "No CS for SPI(%d)\n", spi->chip_select);
>                 return -ENODEV;
>         }
>
> -       if (!spi_get_ctldata(spi)) {
> -               if (gpio_is_valid(spi->cs_gpio)) {
> -                       err = gpio_request_one(spi->cs_gpio, GPIOF_OUT_INIT_HIGH,
> -                                              dev_name(&spi->dev));
> -                       if (err) {
> -                               dev_err(&spi->dev,
> -                                       "Failed to get /CS gpio [%d]: %d\n",
> -                                       spi->cs_gpio, err);
> -                               goto err_gpio_req;
> -                       }
> -               }
> -
> +       if (!spi_get_ctldata(spi))
>                 spi_set_ctldata(spi, cs);
> -       }
>
>         pm_runtime_get_sync(&sdd->pdev->dev);
>
> @@ -909,11 +892,9 @@ static int s3c64xx_spi_setup(struct spi_device *spi)
>         /* setup() returns with device de-selected */
>         s3c64xx_spi_set_cs(spi, false);
>
> -       if (gpio_is_valid(spi->cs_gpio))
> -               gpio_free(spi->cs_gpio);
>         spi_set_ctldata(spi, NULL);
>
> -err_gpio_req:
> +       /* This was dynamically allocated on the DT path */
>         if (spi->dev.of_node)
>                 kfree(cs);
>
> @@ -924,19 +905,9 @@ static void s3c64xx_spi_cleanup(struct spi_device *spi)
>  {
>         struct s3c64xx_spi_csinfo *cs = spi_get_ctldata(spi);
>
> -       if (gpio_is_valid(spi->cs_gpio)) {
> -               gpio_free(spi->cs_gpio);
> -               if (spi->dev.of_node)
> -                       kfree(cs);
> -               else {
> -                       /* On non-DT platforms, the SPI core sets
> -                        * spi->cs_gpio to -ENOENT and .setup()
> -                        * overrides it with the GPIO pin value
> -                        * passed using platform data.
> -                        */
> -                       spi->cs_gpio = -ENOENT;
> -               }
> -       }
> +       /* This was dynamically allocated on the DT path */
> +       if (spi->dev.of_node)
> +               kfree(cs);
>
>         spi_set_ctldata(spi, NULL);
>  }
> @@ -1131,6 +1102,7 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
>         master->prepare_message = s3c64xx_spi_prepare_message;
>         master->transfer_one = s3c64xx_spi_transfer_one;
>         master->num_chipselect = sci->num_cs;
> +       master->use_gpio_descriptors = true;
>         master->dma_alignment = 8;
>         master->bits_per_word_mask = SPI_BPW_MASK(32) | SPI_BPW_MASK(16) |
>                                         SPI_BPW_MASK(8);
> diff --git a/include/linux/platform_data/spi-s3c64xx.h b/include/linux/platform_data/spi-s3c64xx.h
> index 10890a4b55b9..391f4dc68742 100644
> --- a/include/linux/platform_data/spi-s3c64xx.h
> +++ b/include/linux/platform_data/spi-s3c64xx.h
> @@ -16,15 +16,13 @@ struct platform_device;
>   * struct s3c64xx_spi_csinfo - ChipSelect description
>   * @fb_delay: Slave specific feedback delay.
>   *            Refer to FB_CLK_SEL register definition in SPI chapter.
> - * @line: Custom 'identity' of the CS line.
>   *
> - * This is per SPI-Slave Chipselect information.
> + * This is per SPI-Slave FB delay information.

Not sure if this change is needed: this comments will be incorrect if
some new fields are added in future. For example, downstream trees
like [1] have more fields there, not only feedback delay one.

[1] https://android.googlesource.com/kernel/gs/+/refs/heads/android-gs-raviole-5.10-android12-d1/include/linux/platform_data/spi-s3c64xx.h#53

>   * Allocate and initialize one in machine init code and make the
>   * spi_board_info.controller_data point to it.
>   */
>  struct s3c64xx_spi_csinfo {
>         u8 fb_delay;
> -       unsigned line;
>  };
>
>  /**
> --
> 2.34.1
>

Other than minor comments above:

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

Btw, this SPI driver is used in Exynos850 SoC, which I'm working on
right now. It's the shame I didn't enable it yet on my board, so I
can't test the code. But I'm happy to see it's being modernized, kudos
for doing that!

Re: [PATCH 3/3 v2] spi: s3c64xx: Convert to use GPIO descriptors

[Linus Walleij]

On Tue, Jan 18, 2022 at 4:27 PM Sam Protsenko
<semen.protsenko@linaro.org> wrote:

> > +       if (!cs)
> > +               return 0;
>
> Shouldn't some error code be returned in that case? Or is it normal case?

No, because it just means the platform didn't define any fb_delay.

> >         /* Configure feedback delay */
> >         writel(cs->fb_delay & 0x3, sdd->regs + S3C64XX_SPI_FB_CLK);

But maybe I should always write 0 into this then.

> > - * This is per SPI-Slave Chipselect information.
> > + * This is per SPI-Slave FB delay information.
>
> Not sure if this change is needed: this comments will be incorrect if
> some new fields are added in future. For example, downstream trees
> like [1] have more fields there, not only feedback delay one.

OK I fix

> Other than minor comments above:
>
> Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>
>
> Btw, this SPI driver is used in Exynos850 SoC, which I'm working on
> right now. It's the shame I didn't enable it yet on my board, so I
> can't test the code. But I'm happy to see it's being modernized, kudos
> for doing that!

Yeah I saw it being used in the Tesla SoC and also Exynos850 so as
I now know this is not going away any time soon we might as well just
fix it up.

Yours,
Linus Walleij

Re: [PATCH 3/3 v2] spi: s3c64xx: Convert to use GPIO descriptors

[Mark Brown]

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

On Tue, Jan 18, 2022 at 05:27:27PM +0200, Sam Protsenko wrote:
> On Tue, 18 Jan 2022 at 13:11, Linus Walleij <linus.walleij@linaro.org> wrote:
> >
> > Convert the S3C64xx SPI host to use GPIO descriptors.
> >
> > Provide GPIO descriptor tables for the one user with CS
> > 0 and 1.
> >
> > Cc: linux-samsung-soc@vger.kernel.org

Please delete unneeded context from mails when replying.  Doing this
makes it much easier to find your reply in the message, helping ensure
it won't be missed by people scrolling through the irrelevant quoted
material.

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

Re: [PATCH 3/3 v2] spi: s3c64xx: Convert to use GPIO descriptors

[Mark Brown]

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

On Tue, Jan 18, 2022 at 12:09:28PM +0100, Linus Walleij wrote:

> @@ -655,6 +653,9 @@ static int s3c64xx_spi_prepare_message(struct spi_master *master,
>  	struct spi_device *spi = msg->spi;
>  	struct s3c64xx_spi_csinfo *cs = spi->controller_data;
>  
> +	if (!cs)
> +		return 0;
> +
>  	/* Configure feedback delay */
>  	writel(cs->fb_delay & 0x3, sdd->regs + S3C64XX_SPI_FB_CLK);
>  

If you do revise this that should probably just be a guard on
configuring feedback delay rather than aborting out of the entire
function since otherwise this isn't extensible if anything else gets
added to the function.

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

Re: [PATCH 3/3 v2] spi: s3c64xx: Convert to use GPIO descriptors

[Sam Protsenko]

On Tue, 18 Jan 2022 at 18:59, Mark Brown <broonie@kernel.org> wrote:

[snip]

>
> Please delete unneeded context from mails when replying.  Doing this
> makes it much easier to find your reply in the message, helping ensure
> it won't be missed by people scrolling through the irrelevant quoted
> material.

Sure. Thanks for pointing that out. Just forgot to do so this time :)

Re: [PATCH 3/3 v2] spi: s3c64xx: Convert to use GPIO descriptors

[Sam Protsenko]

On Tue, 18 Jan 2022 at 18:08, Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Tue, Jan 18, 2022 at 4:27 PM Sam Protsenko
> <semen.protsenko@linaro.org> wrote:
>
> > > +       if (!cs)
> > > +               return 0;
> >
> > Shouldn't some error code be returned in that case? Or is it normal case?
>
> No, because it just means the platform didn't define any fb_delay.
>
> > >         /* Configure feedback delay */
> > >         writel(cs->fb_delay & 0x3, sdd->regs + S3C64XX_SPI_FB_CLK);
>
> But maybe I should always write 0 into this then.
>

Yeah, probably makes sense, as some other slave node may have
configured fb_delay on the same bus / driver instance. And of course 0
is correct value, just checked in Exynos850 TRM, means "do not use
this feedback clock".

> > > - * This is per SPI-Slave Chipselect information.
> > > + * This is per SPI-Slave FB delay information.
> >
> > Not sure if this change is needed: this comments will be incorrect if
> > some new fields are added in future. For example, downstream trees
> > like [1] have more fields there, not only feedback delay one.
>
> OK I fix
>
> > Other than minor comments above:
> >
> > Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>
> >
> > Btw, this SPI driver is used in Exynos850 SoC, which I'm working on
> > right now. It's the shame I didn't enable it yet on my board, so I
> > can't test the code. But I'm happy to see it's being modernized, kudos
> > for doing that!
>
> Yeah I saw it being used in the Tesla SoC and also Exynos850 so as
> I now know this is not going away any time soon we might as well just
> fix it up.
>
> Yours,
> Linus Walleij

Re: [PATCH 3/3 v2] spi: s3c64xx: Convert to use GPIO descriptors

[Andy Shevchenko]

On Thu, Jan 20, 2022 at 9:40 AM Linus Walleij <linus.walleij@linaro.org> wrote:

...

> +static struct gpiod_lookup_table crag_spi0_gpiod_table = {
> +       .dev_id = "s3c6410-spi.0",
> +       .table = {
> +               GPIO_LOOKUP_IDX("GPIOC", 3, "cs", 0, GPIO_ACTIVE_LOW),
> +               GPIO_LOOKUP_IDX("GPION", 5, "cs", 1, GPIO_ACTIVE_LOW),

> +               { },

Since it will be v3 of the series, can we remove the comma in the
terminator line?

> +       },
> +};

...

> +       /* This was dynamically allocated on the DT path */

> +       if (spi->dev.of_node)

I believe we may survive w/o this check(s).

> +               kfree(cs);

--
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/3 v2] spi: s3c64xx: Delete unused boardfile helpers

[Mark Brown]

On Tue, 18 Jan 2022 12:09:26 +0100, Linus Walleij wrote:
> The helpers to use SPI host 1 and 2 are unused in the kernel
> and taking up space and maintenance hours. New systems should
> use device tree and not this, so delete the code.
> 
> 

Applied to

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

Thanks!

[1/3] spi: s3c64xx: Delete unused boardfile helpers
      commit: f1ba938e4f98941dc2b77795062e49444ec1fee1
[2/3] spi: s3c64xx: Drop custom gpio setup argument
      commit: 3b5529ae7f3578da633e8ae2ec0715a55a248f9f
[3/3] spi: s3c64xx: Convert to use GPIO descriptors
      commit: a45cf3cc72dd9cfde9db8af32cdf9c431f53f9bc

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

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

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

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

Thanks,
Mark