[PATCH 0/3 v6] spi: s3c64xx: use "cs-gpios" in spi node instead of "cs-gpio"

[PATCH 0/3 v6] spi: s3c64xx: use "cs-gpios" in spi node instead of "cs-gpio"

[Naveen Krishna Chatradhi]

Currently, spi-s3c64xx.c needs "cs-gpio" chip select GPIO to be
defined under "controller-data" node under each slave node.

&spi_x {
        cs-gpios <>;
        ...
        slave_node {

                controller-data {
                        cs-gpio = <>;
                        ...
                };
                ...
        };
        ...
};

Where as, SPI core and many other drivers uses "cs-gpios" for
from device tree node.

Hence, make changes in spi-s3c64xx.c driver to make use of
"cs-gpios" from SPI node(parent) instead of "cs-gpio" defined in
slaves "controller-data"(child) node.

Also, fixes a compilation warning and corrects the DTS nodes for
Exynos4210 based SMDKv310, Exynos4412 based Trats2, Exynos5250 based
SMDK5250 boards.

Changes since v5:
1. Fixed the "making a GPIO chip select mandatory" bug.

Naveen Krishna Chatradhi (3):
  spi: s3c64xx: fix broken "cs_gpios" usage in the driver
  spi: s3c64xx: for DT platofrms always get the chipselect info from DT
    node
  ARM: DTS: fix the chip select gpios definition in the SPI nodes

 .../devicetree/bindings/spi/spi-samsung.txt        |    8 ++--
 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                          |   41 ++++++++------------
 5 files changed, 22 insertions(+), 33 deletions(-)

-- 
1.7.9.5

[PATCH 1/3 v6] spi: s3c64xx: fix broken "cs_gpios" usage in the driver

[Naveen Krishna Chatradhi]

Since, (3146bee spi: s3c64xx: Added provision for dedicated cs pin)

spi-s3c64xx.c driver expects
1. chip select gpios from "cs-gpio"(singular) under the
   "controller-data" node of the client/slave device of the SPI.

2. "cs-gpio"(singular) entry to be present in the SPI device node.

Eg of current broken usage:
&spi_1 {
	cs-gpio <>;	/* this entry is checked during probe */
	...
	slave_node {
		controller-data {
			cs-gpio <&gpioa2 5 0>;
			/* This field is parsed during .setup() */
		}
	};
};

The following dts files which were using this driver. But,
din't have the "cs-gpio" entry under SPI node.
-- arch/arm/boot/dts/exynos4210-smdkv310.dts
-- arch/arm/boot/dts/exynos4412-trats2.dts
-- arch/arm/boot/dts/exynos5250-smdk5250.dts

Also, the SPI core and many drivers moved on to using "cs-gpios"
from SPI node and removed the gpio handling code from drivers
(including spi-s3c64xx.c).

Hence, spi-s3c64xx.c is broken since "Jun 21 11:26:12 2013" and
considering the time with no compliants about the breakage.

We are assuming it is safe to remove the "cs-gpio"(singular) usage
from device tree binding of spi-samsung.txt and makes appropriate
changes in the driver to use "cs-gpios"(plural) from
SPI device node.

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>
Tested-by: Doug Anderson <dianders@chromium.org>
Cc: Tomasz Figa <t.figa@samsung.com>
---
Changes since v5:
Fixed the "making a GPIO chip select mandatory" bug.

Changes since v4:
1. Added reviewed by from Javier and Tested by from Doug

Changes since v3:
1. Remove the sdd->cs_gpio and use gpio_is_valid(spi->cs_gpio) instead
2. Keep cs->line only for Non-DT platforms and use spi->cs_gpio
   for DT platforms
 .../devicetree/bindings/spi/spi-samsung.txt        |    8 ++---
 drivers/spi/spi-s3c64xx.c                          |   38 ++++++++------------
 2 files changed, 17 insertions(+), 29 deletions(-)

diff --git a/Documentation/devicetree/bindings/spi/spi-samsung.txt b/Documentation/devicetree/bindings/spi/spi-samsung.txt
index 655b665..792efba 100644
--- a/Documentation/devicetree/bindings/spi/spi-samsung.txt
+++ b/Documentation/devicetree/bindings/spi/spi-samsung.txt
@@ -39,15 +39,13 @@ 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: should specify GPIOs used for chipselects (see 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 +83,7 @@ Example:
 		#size-cells = <0>;
 		pinctrl-names = "default";
 		pinctrl-0 = <&spi0_bus>;
+		cs-gpios = <&gpa2 5 0>;
 
 		w25q80bw@0 {
 			#address-cells = <1>;
@@ -94,7 +93,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..b61ff3d 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -197,7 +197,6 @@ struct s3c64xx_spi_driver_data {
 	struct s3c64xx_spi_dma_data	tx_dma;
 	struct s3c64xx_spi_port_config	*port_conf;
 	unsigned int			port_id;
-	bool				cs_gpio;
 };
 
 static void flush_fifo(struct s3c64xx_spi_driver_data *sdd)
@@ -776,17 +775,6 @@ static struct s3c64xx_spi_csinfo *s3c64xx_get_slave_ctrldata(
 		return ERR_PTR(-ENOMEM);
 	}
 
-	/* 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);
-
-	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);
-	}
-
 	of_property_read_u32(data_np, "samsung,spi-feedback-delay", &fb_delay);
 	cs->fb_delay = fb_delay;
 	of_node_put(data_np);
@@ -812,6 +800,10 @@ static int s3c64xx_spi_setup(struct spi_device *spi)
 		spi->controller_data = cs;
 	}
 
+	/* For the non-DT platforms derive chip selects from controller data */
+	if (!spi->dev.of_node)
+		spi->cs_gpio = cs->line;
+
 	if (IS_ERR_OR_NULL(cs)) {
 		dev_err(&spi->dev, "No CS for SPI(%d)\n", spi->chip_select);
 		return -ENODEV;
@@ -819,17 +811,16 @@ static int s3c64xx_spi_setup(struct spi_device *spi)
 
 	if (!spi_get_ctldata(spi)) {
 		/* Request gpio only if cs line is asserted by gpio pins */
-		if (sdd->cs_gpio) {
-			err = gpio_request_one(cs->line, GPIOF_OUT_INIT_HIGH,
-					dev_name(&spi->dev));
+		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",
-					cs->line, err);
+					spi->cs_gpio, err);
 				goto err_gpio_req;
 			}
-
-			spi->cs_gpio = cs->line;
 		}
 
 		spi_set_ctldata(spi, cs);
@@ -884,7 +875,8 @@ setup_exit:
 	/* setup() returns with device de-selected */
 	writel(S3C64XX_SPI_SLAVE_SIG_INACT, sdd->regs + S3C64XX_SPI_SLAVE_SEL);
 
-	gpio_free(cs->line);
+	if (gpio_is_valid(spi->cs_gpio))
+		gpio_free(spi->cs_gpio);
 	spi_set_ctldata(spi, NULL);
 
 err_gpio_req:
@@ -900,10 +892,12 @@ 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);
+		else
+			spi->cs_gpio = -ENOENT;
 	}
 	spi_set_ctldata(spi, NULL);
 }
@@ -1075,11 +1069,7 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
 	sdd->cntrlr_info = sci;
 	sdd->pdev = 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))
-			sdd->cs_gpio = false;
-
 		ret = of_alias_get_id(pdev->dev.of_node, "spi");
 		if (ret < 0) {
 			dev_err(&pdev->dev, "failed to get alias id, errno %d\n",
-- 
1.7.9.5

[PATCH 2/3 v6] spi: s3c64xx: for DT platofrms always get the chipselect info from DT node

[Naveen Krishna Chatradhi]

Use controller_data structure only for the Non Device tree  platforms.
For Device tree platforms, always derive the chipselect info from
DT node.

Signed-off-by: Naveen Krishna Chatradhi <ch.naveen@samsung.com>
Reviewed-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
Tested-by: Doug Anderson <dianders@chromium.org>
---
Changes since v5:
None

Changes since v4:
1. Added reviewed by from Javier and Tested by from Doug

Changes since v3:
New change
 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 b61ff3d..a4b1af0 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -795,14 +795,15 @@ 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;
-	}
-
-	/* For the non-DT platforms derive chip selects from controller data */
-	if (!spi->dev.of_node)
+	} else {
+		/* For the non-DT platforms derive chip
+		 * selects from controller data
+		 */
 		spi->cs_gpio = cs->line;
+	}
 
 	if (IS_ERR_OR_NULL(cs)) {
 		dev_err(&spi->dev, "No CS for SPI(%d)\n", spi->chip_select);
-- 
1.7.9.5

[PATCH 3/3 v6] 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>
---
Changes since v5:
None

Changes since v4:
1. Added reviewed by from Javier and Doug.
2. Maintained the "status" and "cs-gpios" ordering

Changes since v3:
None
 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

Re: [PATCH 1/3 v6] spi: s3c64xx: fix broken "cs_gpios" usage in the driver

[Mark Brown]

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

On Mon, Jul 14, 2014 at 11:11:44AM +0530, Naveen Krishna Chatradhi wrote:

> @@ -812,6 +800,10 @@ static int s3c64xx_spi_setup(struct spi_device *spi)
>  		spi->controller_data = cs;
>  	}
>  
> +	/* For the non-DT platforms derive chip selects from controller data */
> +	if (!spi->dev.of_node)
> +		spi->cs_gpio = cs->line;
> +
>  	if (IS_ERR_OR_NULL(cs)) {
>  		dev_err(&spi->dev, "No CS for SPI(%d)\n", spi->chip_select);
>  		return -ENODEV;
> @@ -819,17 +811,16 @@ static int s3c64xx_spi_setup(struct spi_device *spi)
>  
>  	if (!spi_get_ctldata(spi)) {
>  		/* Request gpio only if cs line is asserted by gpio pins */
> -		if (sdd->cs_gpio) {
> -			err = gpio_request_one(cs->line, GPIOF_OUT_INIT_HIGH,
> -					dev_name(&spi->dev));
> +		if (gpio_is_valid(spi->cs_gpio)) {

As previously mentioned gpio_is_valid() is *not* a direct substitute for
checking if the boolean flag cs_gpio has been set since 0 is a valid
GPIO on at least some of these platforms and as discussed several times
already some of the SoCs require the use of the built in chip select.

In general it's quite hard to tie the description in the patch to the
code changes, not helped by the decision to do separate refactorings
like this conversion to gpio_is_valid() as part of the one patch.  The
description of the patch now makes some statements about what the
problem that's intended to be fixed is but it still doesn't seem
entirely clear that everything has been thought through fully and tied
to the code.

The original code appears to be buggy which isn't helping anything but
it's hard to have confidence that this isn't going to break some other
use case that currently works given the lack of clarity and the number
of revisions that have been required so far.

I think some combination of smaller changes and a clearer working
through of the before and after states for both DT and non DT cases to
show that everything has been considered would help a lot.  I may have
another stare at this but it's worrying how hard I'm needing to think.

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

Re: [PATCH 1/3 v6] spi: s3c64xx: fix broken "cs_gpios" usage in the driver

[Naveen Krishna Ch]

Hello Mark,

On 14 July 2014 22:55, Mark Brown <broonie@kernel.org> wrote:
> On Mon, Jul 14, 2014 at 11:11:44AM +0530, Naveen Krishna Chatradhi wrote:
>
>> @@ -812,6 +800,10 @@ static int s3c64xx_spi_setup(struct spi_device *spi)
>>               spi->controller_data = cs;
>>       }
>>
>> +     /* For the non-DT platforms derive chip selects from controller data */
>> +     if (!spi->dev.of_node)
>> +             spi->cs_gpio = cs->line;
>> +
>>       if (IS_ERR_OR_NULL(cs)) {
>>               dev_err(&spi->dev, "No CS for SPI(%d)\n", spi->chip_select);
>>               return -ENODEV;
>> @@ -819,17 +811,16 @@ static int s3c64xx_spi_setup(struct spi_device *spi)
>>
>>       if (!spi_get_ctldata(spi)) {
>>               /* Request gpio only if cs line is asserted by gpio pins */
>> -             if (sdd->cs_gpio) {
>> -                     err = gpio_request_one(cs->line, GPIOF_OUT_INIT_HIGH,
>> -                                     dev_name(&spi->dev));
>> +             if (gpio_is_valid(spi->cs_gpio)) {
>
> As previously mentioned gpio_is_valid() is *not* a direct substitute for
> checking if the boolean flag cs_gpio has been set since 0 is a valid
> GPIO on at least some of these platforms and as discussed several times
> already some of the SoCs require the use of the built in chip select.
Yes, gpio_is_valid() is not a direct substitute for boolean cs_gpio.

It was a review comment to use gpio_is_valid() and quickly iterated
another version with that change.  Now that we see gpio_is_valid() is
breaking the native chip select for gpio number 0. Shall i go back to
using the boolean or do you have a better way to do this.

>
> In general it's quite hard to tie the description in the patch to the
> code changes, not helped by the decision to do separate refactorings
> like this conversion to gpio_is_valid() as part of the one patch.  The
> description of the patch now makes some statements about what the
> problem that's intended to be fixed is but it still doesn't seem
> entirely clear that everything has been thought through fully and tied
> to the code.
>
> The original code appears to be buggy which isn't helping anything but
> it's hard to have confidence that this isn't going to break some other
> use case that currently works given the lack of clarity and the number
> of revisions that have been required so far.

spi-s3c64xx.c is supporting a wide range of SoCs ranging from s3c64xx
series followed by s5p series and then exynos4 and exynos5 series.
However, Exynos4 and Exynos5 series currently available are all DT based.
Support for DT and non-DT was taken good care.

Revision number went to 5 for minor changes like
1. Missing documentation
2. Using gpio_is_valid() instead of boolean cs_gpio
3. Adding SignedOff-By and Acked-By

>
> I think some combination of smaller changes and a clearer working
> through of the before and after states for both DT and non DT cases to
> show that everything has been considered would help a lot.  I may have
> another stare at this but it's worrying how hard I'm needing to think.


The intention of the patch was to fix the broken platforms by implementing
dt bindings similar to generic SPI core in spi-s3c64xx.c.

Current dt bindings:
spi-s3c64xx.c is expects the cs-gpio" property in the SPI DT node
and also defined under the "controller-data".

Solution 1:
Add support for "cs-gpios" property in the SPI DT node and
remove "cs-gpio" from sub node "controller-data".

Solution 2:
As Javier suggested
//
Inverting the default of cs_gpio. and By default not having the
"cs-gpio" property in the SPI dev node should mean that the "cs-gpio"
property in the controller-data node should be used to signal the
chip-select and having the "cs-gpio" property in the SPI node should
mean that the native chip select should be used instead of a GPIO.
That preserves the old DT binding semantic while making the GPIO to be optional
//

in this case spi-s3c64xx.c will continue to ignore the generic SPI "cs-gpios"
implementation.

I'm willing to implement any suggestion to fix this issue.

-- 
Regards,
(: Naveen :)

Re: [PATCH 1/3 v6] spi: s3c64xx: fix broken "cs_gpios" usage in the driver

[Mark Brown]

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

On Tue, Jul 15, 2014 at 12:31:32AM +0530, Naveen Krishna Ch wrote:

> in this case spi-s3c64xx.c will continue to ignore the generic SPI "cs-gpios"
> implementation.

> I'm willing to implement any suggestion to fix this issue.

The problem isn't what you're trying to do, the problem is verifying
that it has been done correctly - making sure that everything has been
accounted for in the change itself.

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

Re: [PATCH 1/3 v6] spi: s3c64xx: fix broken "cs_gpios" usage in the driver

[Naveen Krishna Ch]

Hello Mark,

On 15 July 2014 00:45, Mark Brown <broonie@kernel.org> wrote:
> On Tue, Jul 15, 2014 at 12:31:32AM +0530, Naveen Krishna Ch wrote:
>
>> in this case spi-s3c64xx.c will continue to ignore the generic SPI "cs-gpios"
>> implementation.
>
>> I'm willing to implement any suggestion to fix this issue.
>
> The problem isn't what you're trying to do, the problem is verifying
> that it has been done correctly - making sure that everything has been
> accounted for in the change itself.

Understand your concern. I will try to get it tested on as many platforms.

-- 
Thanks for your review.
(: Naveen :)

Re: [PATCH 1/3 v6] spi: s3c64xx: fix broken "cs_gpios" usage in the driver

[Javier Martinez Canillas]

Hello Mark,

I agree that the commit message could have a better description and I
understand your concerns. I'm not an SPI expert by any means but I did
my best to review the patches and provide feedback to Naveen on the
first iterations of the series.

On Mon, Jul 14, 2014 at 7:25 PM, Mark Brown <broonie@kernel.org> wrote:
> On Mon, Jul 14, 2014 at 11:11:44AM +0530, Naveen Krishna Chatradhi wrote:
>
>> @@ -812,6 +800,10 @@ static int s3c64xx_spi_setup(struct spi_device *spi)
>>               spi->controller_data = cs;
>>       }
>>
>> +     /* For the non-DT platforms derive chip selects from controller data */
>> +     if (!spi->dev.of_node)
>> +             spi->cs_gpio = cs->line;
>> +
>>       if (IS_ERR_OR_NULL(cs)) {
>>               dev_err(&spi->dev, "No CS for SPI(%d)\n", spi->chip_select);
>>               return -ENODEV;
>> @@ -819,17 +811,16 @@ static int s3c64xx_spi_setup(struct spi_device *spi)
>>
>>       if (!spi_get_ctldata(spi)) {
>>               /* Request gpio only if cs line is asserted by gpio pins */
>> -             if (sdd->cs_gpio) {
>> -                     err = gpio_request_one(cs->line, GPIOF_OUT_INIT_HIGH,
>> -                                     dev_name(&spi->dev));
>> +             if (gpio_is_valid(spi->cs_gpio)) {
>
> As previously mentioned gpio_is_valid() is *not* a direct substitute for
> checking if the boolean flag cs_gpio has been set since 0 is a valid
> GPIO on at least some of these platforms and as discussed several times
> already some of the SoCs require the use of the built in chip select.
>

Please correct me if I'm wrong but in this case I think that using
gpio_is_valid(spi->cs_gpio) is the right thing to do. The SPI core
will set spi->cs_gpio to -ENOENT if a Device Tree didn't use the
"cs-gpios" property of if the format (explained in
Documentation/devicetree/bindings/spi/spi-bus.txt) to specify a
native/built-in line chip select is used:

cs-gpios = <&gpio1 0 0> <0>
cs0 : &gpio1 0 0
cs1 : native

So in that case gpio_is_valid(spi->cs_gpio) will return false which is
what Naveen wants in his patch.

Now, a problem is that in the non-DT case the patch does not rely on
the spi->cs_gpio value set by the SPI core but instead overwrites it
with the value in cs->line. So as you said this would have been an
issue if legacy non-DT board code used s3c64xx_spi_csinfo .line to
specify that the built-in chip select should be used instead of a
GPIO.

But looking at the board files that sets struct spi_board_info
.controller_data to struct s3c64xx_spi_csinfo I see that the the .line
field is actually used to specify a GPIO pin and not a chip select.

In fact there is only one user in mainline
(arch/arm/mach-s3c64xx/mach-crag6410-module.c) that do this:

#define S3C64XX_GPC(_nr)      (S3C64XX_GPIO_C_START + (_nr))

static struct s3c64xx_spi_csinfo wm0010_spi_csinfo = {
        .line = S3C64XX_GPC(3),
};

static struct spi_board_info wm1253_devs[] = {
        [0] = {
                .modalias       = "wm0010",
                .max_speed_hz   = 26 * 1000 * 1000,
                .bus_num        = 0,
                .chip_select    = 0,
                .mode           = SPI_MODE_0,
                .irq            = S3C_EINT(4),
                .controller_data = &wm0010_spi_csinfo,
                .platform_data = &wm0010_pdata,
        },
};

So, the .line field is used to specify a GPIO, not a chip select. So
gpio_is_valid() should also be used to check that cs->line is a valid
GPIO. If board file does not want to use a GPIO and wants to use a
built-in chip select then it should either not use a struct
s3c64xx_spi_csinfo at all or set .line to -ENOENT in order to be
consistent with the SPI core.

Now, looking at this I realize that there is a bug in the spi-s3c64xx
driver since it does not check if spi->controller_data is NULL in the
non-DT case which I believe it can be true.

> In general it's quite hard to tie the description in the patch to the
> code changes, not helped by the decision to do separate refactorings
> like this conversion to gpio_is_valid() as part of the one patch.  The
> description of the patch now makes some statements about what the
> problem that's intended to be fixed is but it still doesn't seem
> entirely clear that everything has been thought through fully and tied
> to the code.
>

AFAICT most of the refactoring is actually removing the buggy code
that was introduced in commit 3146bee ("spi: s3c64xx: Added provision
for dedicated cs pin"). So maybe Naveen can try doing a new series
first reverting the offending commit and then on top of that adding
the support for the generic "cs-gpios" DT property used by the SPI
core?

As I said before this patch is fixing a bug in the SPI driver, the
first version of the series was posted on June, 10 so many other
patches already depend on this fix. So it would be great if we can
move this forward since this is hurting the platform support as a
whole.

Thanks a lot and best regards,
Javier

Re: [PATCH 1/3 v6] spi: s3c64xx: fix broken "cs_gpios" usage in the driver

[Mark Brown]

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

On Tue, Jul 15, 2014 at 09:33:21AM +0530, Naveen Krishna Ch wrote:
> On 15 July 2014 00:45, Mark Brown <broonie@kernel.org> wrote:

> > The problem isn't what you're trying to do, the problem is verifying
> > that it has been done correctly - making sure that everything has been
> > accounted for in the change itself.

> Understand your concern. I will try to get it tested on as many platforms.

That's useful, but please note that it's mainly a code review thing.

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

Re: [PATCH 1/3 v6] spi: s3c64xx: fix broken "cs_gpios" usage in the driver

[Mark Brown]

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

On Tue, Jul 15, 2014 at 12:38:58PM +0200, Javier Martinez Canillas wrote:
> Hello Mark,

Don't top post.

> On Mon, Jul 14, 2014 at 7:25 PM, Mark Brown <broonie@kernel.org> wrote:
> > On Mon, Jul 14, 2014 at 11:11:44AM +0530, Naveen Krishna Chatradhi wrote:

> So, the .line field is used to specify a GPIO, not a chip select. So
> gpio_is_valid() should also be used to check that cs->line is a valid
> GPIO. If board file does not want to use a GPIO and wants to use a
> built-in chip select then it should either not use a struct
> s3c64xx_spi_csinfo at all or set .line to -ENOENT in order to be
> consistent with the SPI core.

This sort of "let's make everyone instabuggy" change is not good
practice, especially when it's mixed in with some other not obviously
related change, has an unclear benefit and is barely mentioned in the
commit log - I found this through code review, not through it being a
clearly intentional and considered part of the change.

> As I said before this patch is fixing a bug in the SPI driver, the
> first version of the series was posted on June, 10 so many other
> patches already depend on this fix. So it would be great if we can
> move this forward since this is hurting the platform support as a
> whole.

So provide a clear, easy to understand patch series then.  As I've said
before this series is setting off lots of alarm bells - the large number
of versions, the difficulty in understanding what it was supposed to do
both for the overall goal of the series and the individual changes, and
the fact that several people have found bugs for use cases other than
your own (all the way up to removing non-DT support) are all saying that
this is something that needs careful review and also making that review
difficult.

Code review is an important part of the process, we need people to work
to make that review easy, address review comments and allow a reasonable
time for the review to happen (something that's obviously going to be
quicker with submissions that are easier to review).

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

Re: [PATCH 1/3 v6] spi: s3c64xx: fix broken "cs_gpios" usage in the driver

[Tomasz Figa]

Hi Naveen,

On 14.07.2014 07:41, Naveen Krishna Chatradhi wrote:
> Since, (3146bee spi: s3c64xx: Added provision for dedicated cs pin)
> 
> spi-s3c64xx.c driver expects
> 1. chip select gpios from "cs-gpio"(singular) under the
>    "controller-data" node of the client/slave device of the SPI.

Please work a bit more on readability of your commit messages,
especially in terms of using correct terminology to avoid unnecessary
confusion.

s/'chip select gpios'/'chip select GPIO pins'/
s/'"cs-gpio"(singular)'/'"cs-gpio" property'/
s/'client/slave device of the SPI'/'SPI device'/

More things like this follow in rest of the text below.

Otherwise I believe the code after this patch does the right thing, so:

Reviewed-by: Tomasz Figa <t.figa@samsung.com>

Best regards,
Tomasz

Re: [PATCH 1/3 v6] spi: s3c64xx: fix broken "cs_gpios" usage in the driver

[Naveen Krishna Ch]

Hello Tomasz,

On 15 July 2014 22:25, Tomasz Figa <t.figa@samsung.com> wrote:
> Hi Naveen,
>
> On 14.07.2014 07:41, Naveen Krishna Chatradhi wrote:
>> Since, (3146bee spi: s3c64xx: Added provision for dedicated cs pin)
>>
>> spi-s3c64xx.c driver expects
>> 1. chip select gpios from "cs-gpio"(singular) under the
>>    "controller-data" node of the client/slave device of the SPI.
>
> Please work a bit more on readability of your commit messages,
> especially in terms of using correct terminology to avoid unnecessary
> confusion.
>
> s/'chip select gpios'/'chip select GPIO pins'/
> s/'"cs-gpio"(singular)'/'"cs-gpio" property'/
> s/'client/slave device of the SPI'/'SPI device'/

Sure, I'm working on that.

>
> More things like this follow in rest of the text below.
>
> Otherwise I believe the code after this patch does the right thing, so:
>
> Reviewed-by: Tomasz Figa <t.figa@samsung.com>

I've submitted another patch set which does the same functionality
as this patch set. I believe the new series is more cleaner in terms of
patch separation and documentation.

Kindly, let me know your comments on that series.
http://www.spinics.net/lists/arm-kernel/msg347634.html

>
> Best regards,
> Tomasz
-- 
Thanks & Regards,
(: Naveen :)

Re: [PATCH 1/3 v6] spi: s3c64xx: fix broken "cs_gpios" usage in the driver

[Tomasz Figa]

On 15.07.2014 19:21, Naveen Krishna Ch wrote:
> Hello Tomasz,
> 
> On 15 July 2014 22:25, Tomasz Figa <t.figa@samsung.com> wrote:
>> Hi Naveen,
>>
>> On 14.07.2014 07:41, Naveen Krishna Chatradhi wrote:
>>> Since, (3146bee spi: s3c64xx: Added provision for dedicated cs pin)
>>>
>>> spi-s3c64xx.c driver expects
>>> 1. chip select gpios from "cs-gpio"(singular) under the
>>>    "controller-data" node of the client/slave device of the SPI.
>>
>> Please work a bit more on readability of your commit messages,
>> especially in terms of using correct terminology to avoid unnecessary
>> confusion.
>>
>> s/'chip select gpios'/'chip select GPIO pins'/
>> s/'"cs-gpio"(singular)'/'"cs-gpio" property'/
>> s/'client/slave device of the SPI'/'SPI device'/
> 
> Sure, I'm working on that.
> 
>>
>> More things like this follow in rest of the text below.
>>
>> Otherwise I believe the code after this patch does the right thing, so:
>>
>> Reviewed-by: Tomasz Figa <t.figa@samsung.com>
> 
> I've submitted another patch set which does the same functionality
> as this patch set. I believe the new series is more cleaner in terms of
> patch separation and documentation.
> 
> Kindly, let me know your comments on that series.
> http://www.spinics.net/lists/arm-kernel/msg347634.html

Right. I noticed it just now and am looking at it.

Best regards,
Tomasz