spi: s3c64xx: fix the driver to use "cs-gpios" property

Current SPI core has generic implementation for configuring the Chip select gpios during .setup() & .cleanup(). By modifying the spi-s3c64xx.c driver to expect the "cs-gpios" property in SPI device node instead of the subnode "controller-data".

This way we can avoid parsing the "cs-gpios" in the driver.

This patch set also does the following
1. Validate the "struct s3c64xx_spi_csinfo *cs" object and the
CS gpio for both DT and NON-DT before using them.
2. Correct the dt-bindings for exynos4210-smdkv310.dts, exynos4412-trats2.dts and exynos5250-smdk5250.dts
3. Updates the DT bindings.

Note:
This patchset is a rework of the changes under review @
http://www.mail-archive.com/devicetree at vger.kernel.org/msg34501.html

Tested on Exynos5420 and Exynos5250 based Peach PIT, PI and Snow boards respectively using the flashrom utility to access SPI flash.

This patchset is needed by the changes
http://www.gossamer-threads.com/lists/linux/kernel/1951607

Tested-by on boards based on Exynos4, S5P, S3C series SoCs would be appreciated.

Naveen Krishna Chatradhi (3):
spi: s3c64xx: move property "cs-gpio" from controller_data subnode
to SPI DT node
spi: s3c64xx: validate s3c64xx_spi_csinfo before using
ARM: DTS: fix the chip select gpios definition in the SPI nodes

 .../devicetree/bindings/spi/spi-samsung.txt        |   10 ++++----
 arch/arm/boot/dts/exynos4210-smdkv310.dts          |    2 +-
 arch/arm/boot/dts/exynos4412-trats2.dts            |    2 +-
 arch/arm/boot/dts/exynos5250-smdk5250.dts          |    2 +-
 drivers/spi/spi-s3c64xx.c                          |   27 +++++++-------------
 5 files changed, 17 insertions(+), 26 deletions(-)

1.7.9.5

This patch modifies the spi-s3c64xx.c driver to fetch the Chip select or Slave select gpio line property "cs-gpios" from SPI node instead of "controller_data" subnode.

Rename the property "cs-gpio" to "cs-gpios" in accordance with the SPI core. Such that s3c64xx.c can use spi->cs_gpio instead of parsing the property in the driver.

Update the dt-bindings ion spi/spi-samsung.txt

---
This patch is a rework of the change @
http://www.mail-archive.com/devicetree at vger.kernel.org/msg34500.html

I'm not sure if i can carry forward the other Signed-offs and Tested-bys

 .../devicetree/bindings/spi/spi-samsung.txt        |   10 +++++-----
 drivers/spi/spi-s3c64xx.c                          |   18 ++++++++----------
 2 files changed, 13 insertions(+), 15 deletions(-)

index 655b665..ff3c4c9 100644
@@ -39,15 +39,15 @@ Optional Board Specific Properties:
 - num-cs: Specifies the number of chip select lines supported. If
   not specified, the default number of chip select lines is set to 1.
 
+- cs-gpios: A gpio specifier that specifies the gpio line used as
+  the slave select line by the spi controller. The format of the gpio
+  specifier depends on the gpio controller (Also read spi-bus.txt).
+
 SPI Controller specific data in SPI slave nodes:
 
 - The spi slave nodes should provide the following information which is required
   by the spi controller.
 
-  - cs-gpio: A gpio specifier that specifies the gpio line used as
-    the slave select line by the spi controller. The format of the gpio
-    specifier depends on the gpio controller.
-
   - samsung,spi-feedback-delay: The sampling phase shift to be applied on the
     miso line (to account for any lag in the miso line). The following are the
     valid values.

@@ -85,6 +85,7 @@ Example:
 		#size-cells = <0>;
 		pinctrl-names = "default";
 		pinctrl-0 = <&spi0_bus>;
+		cs-gpios = <&gpa2 5 1 0 3>;
 
 		w25q80bw at 0 {
 			#address-cells = <1>;

@@ -94,7 +95,6 @@ Example:
 			spi-max-frequency = <10000>;
 
 			controller-data {
-				cs-gpio = <&gpa2 5 1 0 3>;
 				samsung,spi-feedback-delay = <0>;
 			};
 

index 75a5696..72bfba6 100644
@@ -764,12 +764,6 @@ static struct s3c64xx_spi_csinfo *s3c64xx_get_slave_ctrldata(
 		return ERR_PTR(-EINVAL);
 	}
 
-	data_np = of_get_child_by_name(slave_np, "controller-data");
-	if (!data_np) {
-		dev_err(&spi->dev, "child node 'controller-data' not found\n");
-		return ERR_PTR(-EINVAL);
-	}
-
 	cs = kzalloc(sizeof(*cs), GFP_KERNEL);
 	if (!cs) {
 		of_node_put(data_np);

@@ -777,13 +771,17 @@ static struct s3c64xx_spi_csinfo *s3c64xx_get_slave_ctrldata(
 	}
 
 	/* The CS line is asserted/deasserted by the gpio pin */
-	if (sdd->cs_gpio)
-		cs->line = of_get_named_gpio(data_np, "cs-gpio", 0);
+	cs->line = spi->cs_gpio;
 
 	if (!gpio_is_valid(cs->line)) {
 		dev_err(&spi->dev, "chip select gpio is not specified or invalid\n");
 		kfree(cs);
-		of_node_put(data_np);
+		return ERR_PTR(-EINVAL);
+	}
+
+	data_np = of_get_child_by_name(slave_np, "controller-data");
+	if (!data_np) {
+		dev_err(&spi->dev, "child node 'controller-data' not found\n");
 		return ERR_PTR(-EINVAL);
 	}
 

@@ -1077,7 +1075,7 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
 	sdd->sfr_start = mem_res->start;
 	sdd->cs_gpio = true;
 	if (pdev->dev.of_node) {
-		if (!of_find_property(pdev->dev.of_node, "cs-gpio", NULL))
+		if (!of_find_property(pdev->dev.of_node, "cs-gpios", NULL))
 			sdd->cs_gpio = false;
 
 		ret = of_alias_get_id(pdev->dev.of_node, "spi");

1.7.9.5

Hi Naveen,

Please see my comments inline.

[snip]

Hrm. I'd *hope* that of_get_named_gpio() would distinguish between the property being there but unparsable and the property being absent. Does it not do this?

We ought to handle the errors differently depending on where they came
from - an unspecified GPIO means use the physical chip select but a GPIO
we fail to obtain is an error we should handle.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140716/b89bae1f/attachment.sig>

This patch validates the cs->line (Chip select gpio) and struct s3c64xx_spi_csinfo *cs object for both DT and NON-DT platforms before using in .setup().

Also, check gpio_is_valid(spi->cs_gpio) in cleanup() before freeing up.

---
 drivers/spi/spi-s3c64xx.c |   15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

index 72bfba6..8971076 100644
@@ -773,12 +773,6 @@ static struct s3c64xx_spi_csinfo *s3c64xx_get_slave_ctrldata(
 	/* The CS line is asserted/deasserted by the gpio pin */
 	cs->line = spi->cs_gpio;
 
-	if (!gpio_is_valid(cs->line)) {
-		dev_err(&spi->dev, "chip select gpio is not specified or invalid\n");
-		kfree(cs);
-		return ERR_PTR(-EINVAL);
-	}
-
 	data_np = of_get_child_by_name(slave_np, "controller-data");
 	if (!data_np) {
 		dev_err(&spi->dev, "child node 'controller-data' not found\n");

@@ -805,15 +799,14 @@ static int s3c64xx_spi_setup(struct spi_device *spi)
 	int err;
 
 	sdd = spi_master_get_devdata(spi->master);
-	if (!cs && spi->dev.of_node) {
+	if (spi->dev.of_node)
 		cs = s3c64xx_get_slave_ctrldata(spi);
-		spi->controller_data = cs;
-	}
 
-	if (IS_ERR_OR_NULL(cs)) {
+	if (IS_ERR_OR_NULL(cs) || !gpio_is_valid(cs->line)) {
 		dev_err(&spi->dev, "No CS for SPI(%d)\n", spi->chip_select);
 		return -ENODEV;
 	}
+	spi->controller_data = cs;
 
 	if (!spi_get_ctldata(spi)) {
 		/* Request gpio only if cs line is asserted by gpio pins */

@@ -898,7 +891,7 @@ static void s3c64xx_spi_cleanup(struct spi_device *spi)
 	struct s3c64xx_spi_driver_data *sdd;
 
 	sdd = spi_master_get_devdata(spi->master);
-	if (spi->cs_gpio) {
+	if (gpio_is_valid(spi->cs_gpio)) {
 		gpio_free(spi->cs_gpio);
 		if (spi->dev.of_node)
 			kfree(cs);

1.7.9.5

Hi Naveen,

Please see my comments inline.

Missing reason of the change.

Hello Naveen,

I agree with Tomasz. I think version v6 of your series was (almost) correct while this is one is not. You only had to address Mark's concerns about using gpio_is_valid(spi->cs_gpio).

Also, you need to work out your commit messages since I they are still not clearly explaining the motivation for the changes. You are explaining what the patches are changing but you have to explain why the changes are needed in the first place.

Best regards,
Javier

This patch replaces the "cs-gpio" from "controller-data" node as was specified in the old binding and use the standard "cs-gpios" property expected by the SPI core as is defined in the new binding.

Respective changes are preposed to spi-s3c64xx.c driver. @ http://www.spinics.net/lists/linux-samsung-soc/msg32282.html

---
This change is being reviewed @

http://www.mail-archive.com/devicetree at vger.kernel.org/msg34498.html

 arch/arm/boot/dts/exynos4210-smdkv310.dts |    2 +-
 arch/arm/boot/dts/exynos4412-trats2.dts   |    2 +-
 arch/arm/boot/dts/exynos5250-smdk5250.dts |    2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

index 636d166..676e6e0 100644
@@ -168,6 +168,7 @@
 	};
 
 	spi_2: spi at 13940000 {
+		cs-gpios = <&gpc1 2 0>;
 		status = "okay";
 
 		w25x80 at 0 {

@@ -178,7 +179,6 @@
 			spi-max-frequency = <1000000>;
 
 			controller-data {
-				cs-gpio = <&gpc1 2 0>;
 				samsung,spi-feedback-delay = <0>;
 			};
 

index 7787844..11967f4 100644
@@ -589,6 +589,7 @@
 	spi_1: spi at 13930000 {
 		pinctrl-names = "default";
 		pinctrl-0 = <&spi1_bus>;
+		cs-gpios = <&gpb 5 0>;
 		status = "okay";
 
 		s5c73m3_spi: s5c73m3 {

@@ -596,7 +597,6 @@
 			spi-max-frequency = <50000000>;
 			reg = <0>;
 			controller-data {
-				cs-gpio = <&gpb 5 0>;
 				samsung,spi-feedback-delay = <2>;
 			};
 		};

index a794a70..0c6433a 100644
@@ -316,6 +316,7 @@
 	};
 
 	spi_1: spi at 12d30000 {
+		cs-gpios = <&gpa2 5 0>;
 		status = "okay";
 
 		w25q80bw at 0 {

@@ -326,7 +327,6 @@
 			spi-max-frequency = <1000000>;
 
 			controller-data {
-				cs-gpio = <&gpa2 5 0>;
 				samsung,spi-feedback-delay = <0>;
 			};
 

1.7.9.5