spi: s3c64xx: use "cs-gpios" in spi node instead of "cs-gpio"

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

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.

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

index 655b665..792efba 100644
@@ -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 at 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>;
 			};
 

index 75a5696..b61ff3d 100644
@@ -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

Hello Mark,

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.

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

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

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.
-------------- 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/20140714/ee3cad0e/attachment.sig>

Hello Mark,

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

Thanks for your review.
(: Naveen :)

That's useful, but please note that it's mainly a code review thing.
-------------- 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/20140715/b184429c/attachment.sig>

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.

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.

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

Don't top post.

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.

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).
-------------- 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/20140715/81a984f1/attachment.sig>

Hi Naveen,

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:

Best regards,
Tomasz

Hello Tomasz,

Sure, I'm working on that.

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

Thanks & Regards,
(: Naveen :)

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

Best regards,
Tomasz

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

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

index b61ff3d..a4b1af0 100644
@@ -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

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

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

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