[PATCH 0/3] spi: s3c64xx: fix the driver to use "cs-gpios" property

[PATCH 0/3] spi: s3c64xx: fix the driver to use "cs-gpios" property

[Naveen Krishna Chatradhi]

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@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

[PATCH 1/3] spi: s3c64xx: move "cs-gpio" from subnode to SPI DT node

[Naveen Krishna Chatradhi]

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

Signed-off-by: Naveen Krishna Chatradhi <ch.naveen@samsung.com>
Acked-by: Rob Herring <robh@kernel.org>
Cc: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
Cc: Doug Anderson <dianders@chromium.org>
Cc: Tomasz Figa <t.figa@samsung.com>
---
This patch is a rework of the change @
http://www.mail-archive.com/devicetree@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(-)

diff --git a/Documentation/devicetree/bindings/spi/spi-samsung.txt b/Documentation/devicetree/bindings/spi/spi-samsung.txt
index 655b665..ff3c4c9 100644
--- a/Documentation/devicetree/bindings/spi/spi-samsung.txt
+++ b/Documentation/devicetree/bindings/spi/spi-samsung.txt
@@ -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@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>;
 			};
 
diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 75a5696..72bfba6 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -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

Re: [PATCH 1/3] spi: s3c64xx: move "cs-gpio" from subnode to SPI DT node

[Tomasz Figa]

Hi Naveen,

Please see my comments inline.

On 15.07.2014 14:20, Naveen Krishna Chatradhi wrote:
> 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
> 
> Signed-off-by: Naveen Krishna Chatradhi <ch.naveen@samsung.com>
> Acked-by: Rob Herring <robh@kernel.org>
> Cc: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> Cc: Doug Anderson <dianders@chromium.org>
> Cc: Tomasz Figa <t.figa@samsung.com>
> ---
> This patch is a rework of the change @
> http://www.mail-archive.com/devicetree@vger.kernel.org/msg34500.html
> 
> I'm not sure if i can carry forward the other Signed-offs and Tested-bys

[snip]

> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> index 75a5696..72bfba6 100644
> --- a/drivers/spi/spi-s3c64xx.c
> +++ b/drivers/spi/spi-s3c64xx.c
> @@ -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);
> -	}

Do you need to move this code block?

> -
>  	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)) {

This check is wrong when native chip select is used. However I'm not
sure how to distinguish this from a situation when invalid GPIO was
specified, because cs->line will be -ENOENT in both cases. Mark, any ideas?

>  		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;

What is this boolean flag used for now? If cs->line now either contains
a valid GPIO or a negative error, why gpio_is_valid() couldn't be used
on it? I believe it was done correctly in previous version.

Best regards,
Tomasz

Re: [PATCH 1/3] spi: s3c64xx: move "cs-gpio" from subnode to SPI DT node

[Mark Brown]

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

On Tue, Jul 15, 2014 at 07:30:08PM +0200, Tomasz Figa wrote:
> On 15.07.2014 14:20, Naveen Krishna Chatradhi wrote:

> >  	/* 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)) {

> This check is wrong when native chip select is used. However I'm not
> sure how to distinguish this from a situation when invalid GPIO was
> specified, because cs->line will be -ENOENT in both cases. Mark, any ideas?

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?

> > -		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;

> What is this boolean flag used for now? If cs->line now either contains
> a valid GPIO or a negative error, why gpio_is_valid() couldn't be used
> on it? I believe it was done correctly in previous version.

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 2/3] spi: s3c64xx: validate s3c64xx_spi_csinfo before using

[Naveen Krishna Chatradhi]

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.

Signed-off-by: Naveen Krishna Chatradhi <ch.naveen@samsung.com>
Cc: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
Cc: Doug Anderson <dianders@chromium.org>
---
 drivers/spi/spi-s3c64xx.c |   15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 72bfba6..8971076 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -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

Re: [PATCH 2/3] spi: s3c64xx: validate s3c64xx_spi_csinfo before using

[Tomasz Figa]

Hi Naveen,

Please see my comments inline.

On 15.07.2014 14:20, Naveen Krishna Chatradhi wrote:
> 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.

Missing reason of the change.

> 
> Signed-off-by: Naveen Krishna Chatradhi <ch.naveen@samsung.com>
> Cc: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> Cc: Doug Anderson <dianders@chromium.org>
> ---
>  drivers/spi/spi-s3c64xx.c |   15 ++++-----------
>  1 file changed, 4 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> index 72bfba6..8971076 100644
> --- a/drivers/spi/spi-s3c64xx.c
> +++ b/drivers/spi/spi-s3c64xx.c
> @@ -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)

This check is equivalent, i.e. cs will always be NULL whenever
spi->dev.of_node is not.

>  		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)) {

I'm not sure this is correct. It will error out even if hardware chip
select is used.

Otherwise you need to free cs here if wrong GPIO was the cause of
entering this block, so if this extra check is to stay, I'd suggest
splitting this into two separate if blocks.

>  		dev_err(&spi->dev, "No CS for SPI(%d)\n", spi->chip_select);
>  		return -ENODEV;
>  	}
> +	spi->controller_data = cs;

I'm not sure what's the point of moving this assignment here.

>  
>  	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);

I believe this is completely wrong. cs is allocated even if GPIO chip
select is not used, so the only thing that should be done conditionally
is gpio_free().

In general, I liked previous version of this series much more, as it was
doing what it should as opposed to this one.

Best regards,
Tomasz

Re: [PATCH 2/3] spi: s3c64xx: validate s3c64xx_spi_csinfo before using

[Javier Martinez Canillas]

Hello Naveen,

On 07/15/2014 07:49 PM, Tomasz Figa wrote:
> 
> In general, I liked previous version of this series much more, as it was
> doing what it should as opposed to this one.
> 
> Best regards,
> Tomasz
> 

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

[PATCH 3/3] ARM: DTS: fix the chip select gpios definition in the SPI nodes

[Naveen Krishna Chatradhi]

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

Signed-off-by: Naveen Krishna Chatradhi <ch.naveen@samsung.com>
Acked-by: Rob Herring <robh@kernel.org>
Reviewed-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
Reviewed-by: Doug Anderson <dianders@chromium.org>
Cc: Tomasz Figa <t.figa@samsung.com>
---
This change is being reviewed @

http://www.mail-archive.com/devicetree@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(-)

diff --git a/arch/arm/boot/dts/exynos4210-smdkv310.dts b/arch/arm/boot/dts/exynos4210-smdkv310.dts
index 636d166..676e6e0 100644
--- a/arch/arm/boot/dts/exynos4210-smdkv310.dts
+++ b/arch/arm/boot/dts/exynos4210-smdkv310.dts
@@ -168,6 +168,7 @@
 	};
 
 	spi_2: spi@13940000 {
+		cs-gpios = <&gpc1 2 0>;
 		status = "okay";
 
 		w25x80@0 {
@@ -178,7 +179,6 @@
 			spi-max-frequency = <1000000>;
 
 			controller-data {
-				cs-gpio = <&gpc1 2 0>;
 				samsung,spi-feedback-delay = <0>;
 			};
 
diff --git a/arch/arm/boot/dts/exynos4412-trats2.dts b/arch/arm/boot/dts/exynos4412-trats2.dts
index 7787844..11967f4 100644
--- a/arch/arm/boot/dts/exynos4412-trats2.dts
+++ b/arch/arm/boot/dts/exynos4412-trats2.dts
@@ -589,6 +589,7 @@
 	spi_1: spi@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>;
 			};
 		};
diff --git a/arch/arm/boot/dts/exynos5250-smdk5250.dts b/arch/arm/boot/dts/exynos5250-smdk5250.dts
index a794a70..0c6433a 100644
--- a/arch/arm/boot/dts/exynos5250-smdk5250.dts
+++ b/arch/arm/boot/dts/exynos5250-smdk5250.dts
@@ -316,6 +316,7 @@
 	};
 
 	spi_1: spi@12d30000 {
+		cs-gpios = <&gpa2 5 0>;
 		status = "okay";
 
 		w25q80bw@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