[PATCH 0/2 v2] 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.

Naveen Krishna Chatradhi (2):
based on for-next branch of spi.git
  spi: s3c64xx: use "cs-gpios" from spi node instead of "cs-gpio"

based on for-next branch of linuxsamsung.git
  ARM: DTS: move "cs-gpio" from "controller-data" to under spi node

 .../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                          |   56 ++++++++++++--------
 5 files changed, 41 insertions(+), 29 deletions(-)

-- 
1.7.9.5

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 updates the Device tree Documentation.

Signed-off-by: Naveen Krishna Chatradhi <redacted>
Cc: Javier Martinez Canillas <redacted>
Cc: Doug Anderson <dianders@chromium.org>
---
Changes since v1:
1. fixed a compilation warning thus squashing the other patch into this.
2. Updated device tree description in spi-samsung.txt

 .../devicetree/bindings/spi/spi-samsung.txt        |    8 ++-
 drivers/spi/spi-s3c64xx.c                          |   56 ++++++++++++--------
 2 files changed, 38 insertions(+), 26 deletions(-)

diff --git a/Documentation/devicetree/bindings/spi/spi-samsung.txt b/Documentation/devicetree/bindings/spi/spi-samsung.txt
index 86aa061..13bfcb5 100644
--- a/Documentation/devicetree/bindings/spi/spi-samsung.txt
+++ b/Documentation/devicetree/bindings/spi/spi-samsung.txt

@@ -42,15 +42,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 1 0 3>;
 
 		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>;
 			};
 

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 75a5696..4594dde 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c

@@ -750,47 +750,56 @@ static int s3c64xx_spi_transfer_one(struct spi_master *master,
 }
 
 static struct s3c64xx_spi_csinfo *s3c64xx_get_slave_ctrldata(
-				struct spi_device *spi)
+				struct spi_device *spi,
+				struct s3c64xx_spi_csinfo *cs)
 {
-	struct s3c64xx_spi_csinfo *cs;
-	struct device_node *slave_np, *data_np = NULL;
-	struct s3c64xx_spi_driver_data *sdd;
+	struct device_node *data_np = NULL;
 	u32 fb_delay = 0;
 
-	sdd = spi_master_get_devdata(spi->master);
-	slave_np = spi->dev.of_node;
-	if (!slave_np) {
-		dev_err(&spi->dev, "device node not found\n");
+	data_np = of_get_child_by_name(spi->dev.of_node, "controller-data");
+	if (!data_np) {
+		dev_err(&spi->dev, "child node 'controller-data' not found\n");
 		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");
+	of_property_read_u32(data_np, "samsung,spi-feedback-delay", &fb_delay);
+	cs->fb_delay = fb_delay;
+	of_node_put(data_np);
+
+	return cs;
+}
+
+static struct s3c64xx_spi_csinfo *s3c64xx_get_cs_gpios(struct spi_device *spi)
+{
+	struct device_node *parent_np = NULL;
+	struct s3c64xx_spi_driver_data *sdd;
+	struct s3c64xx_spi_csinfo *cs;
+
+	parent_np = of_get_parent(spi->dev.of_node);
+	if (!parent_np) {
+		dev_err(&spi->dev, "Parent node not found\n");
 		return ERR_PTR(-EINVAL);
 	}
 
+	sdd = spi_master_get_devdata(spi->master);
+
 	cs = kzalloc(sizeof(*cs), GFP_KERNEL);
 	if (!cs) {
-		of_node_put(data_np);
+		of_node_put(parent_np);
 		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);
+		cs->line = of_get_named_gpio(parent_np, "cs-gpios", 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);
+		of_node_put(parent_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);
-	return cs;
+	return s3c64xx_get_slave_ctrldata(spi, cs);
 }
 
 /*

@@ -806,9 +815,14 @@ static int s3c64xx_spi_setup(struct spi_device *spi)
 	struct s3c64xx_spi_info *sci;
 	int err;
 
+	if (!spi->dev.of_node) {
+		dev_err(&spi->dev, "device node not found\n");
+		return -EINVAL;
+	}
+
 	sdd = spi_master_get_devdata(spi->master);
 	if (!cs && spi->dev.of_node) {
-		cs = s3c64xx_get_slave_ctrldata(spi);
+		cs = s3c64xx_get_cs_gpios(spi);
 		spi->controller_data = cs;
 	}
 

@@ -1077,7 +1091,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

This patch moves the "cs-gpio" field from "controller-data" child
node to under the spi device node.

Respective changes are preposed to spi-s3c64xx.c driver.

Signed-off-by: Naveen Krishna Chatradhi <redacted>
Cc: Javier Martinez Canillas <redacted>
Cc: Doug Anderson <dianders@chromium.org>
---
Changes since v1:
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..9191491 100644
--- a/arch/arm/boot/dts/exynos4210-smdkv310.dts
+++ b/arch/arm/boot/dts/exynos4210-smdkv310.dts

@@ -169,6 +169,7 @@
 
 	spi_2: spi at 13940000 {
 		status = "okay";
+		cs-gpios = <&gpc1 2 0>;
 
 		w25x80 at 0 {
 			#address-cells = <1>;

@@ -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 8a558b7..204b0de 100644
--- a/arch/arm/boot/dts/exynos4412-trats2.dts
+++ b/arch/arm/boot/dts/exynos4412-trats2.dts

@@ -512,6 +512,7 @@
 	spi_1: spi at 13930000 {
 		pinctrl-names = "default";
 		pinctrl-0 = <&spi1_bus>;
+		cs-gpios = <&gpb 5 0>;
 		status = "okay";
 
 		s5c73m3_spi: s5c73m3 {

@@ -519,7 +520,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 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

On 10/06/14 12:08, Naveen Krishna Chatradhi wrote:

While at it, please change the GPIO specifier format, this one is not
valid any more.

Can we support both "cs-gpio" and "cs-gpios" for backward compatibility ?
After your change all DTBs using the original pattern will not work with
new kernels any more. At least I would expect such backward compatibility
maintained for few kernel releases.

Ditto.

 		ret = of_alias_get_id(pdev->dev.of_node, "spi");

--
Thanks!
Sylwester

Hello Sylwester,

Thanks for the review.
On 10 June 2014 16:09, Sylwester Nawrocki [off-list ref] wrote:

On 10/06/14 12:08, Naveen Krishna Chatradhi wrote:

quoted

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 updates the Device tree Documentation.

Signed-off-by: Naveen Krishna Chatradhi <redacted>
Cc: Javier Martinez Canillas <redacted>
Cc: Doug Anderson <dianders@chromium.org>
---
Changes since v1:
1. fixed a compilation warning thus squashing the other patch into this.
2. Updated device tree description in spi-samsung.txt

 .../devicetree/bindings/spi/spi-samsung.txt        |    8 ++-
 drivers/spi/spi-s3c64xx.c                          |   56 ++++++++++++--------
 2 files changed, 38 insertions(+), 26 deletions(-)

diff --git a/Documentation/devicetree/bindings/spi/spi-samsung.txt b/Documentation/devicetree/bindings/spi/spi-samsung.txt
index 86aa061..13bfcb5 100644
--- a/Documentation/devicetree/bindings/spi/spi-samsung.txt
+++ b/Documentation/devicetree/bindings/spi/spi-samsung.txt

@@ -42,15 +42,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 1 0 3>;

While at it, please change the GPIO specifier format, this one is not
valid any more.

Sure,  i will club it with the other comments.

quoted

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

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 75a5696..4594dde 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c

@@ -750,47 +750,56 @@ static int s3c64xx_spi_transfer_one(struct spi_master *master,
 }

 static struct s3c64xx_spi_csinfo *s3c64xx_get_slave_ctrldata(
-                             struct spi_device *spi)
+                             struct spi_device *spi,
+                             struct s3c64xx_spi_csinfo *cs)
 {
-     struct s3c64xx_spi_csinfo *cs;
-     struct device_node *slave_np, *data_np = NULL;
-     struct s3c64xx_spi_driver_data *sdd;
+     struct device_node *data_np = NULL;
      u32 fb_delay = 0;

-     sdd = spi_master_get_devdata(spi->master);
-     slave_np = spi->dev.of_node;
-     if (!slave_np) {
-             dev_err(&spi->dev, "device node not found\n");
+     data_np = of_get_child_by_name(spi->dev.of_node, "controller-data");
+     if (!data_np) {
+             dev_err(&spi->dev, "child node 'controller-data' not found\n");
              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");
+     of_property_read_u32(data_np, "samsung,spi-feedback-delay", &fb_delay);
+     cs->fb_delay = fb_delay;
+     of_node_put(data_np);
+
+     return cs;
+}
+
+static struct s3c64xx_spi_csinfo *s3c64xx_get_cs_gpios(struct spi_device *spi)
+{
+     struct device_node *parent_np = NULL;
+     struct s3c64xx_spi_driver_data *sdd;
+     struct s3c64xx_spi_csinfo *cs;
+
+     parent_np = of_get_parent(spi->dev.of_node);
+     if (!parent_np) {
+             dev_err(&spi->dev, "Parent node not found\n");
              return ERR_PTR(-EINVAL);
      }

+     sdd = spi_master_get_devdata(spi->master);
+
      cs = kzalloc(sizeof(*cs), GFP_KERNEL);
      if (!cs) {
-             of_node_put(data_np);
+             of_node_put(parent_np);
              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);
+             cs->line = of_get_named_gpio(parent_np, "cs-gpios", 0);

Can we support both "cs-gpio" and "cs-gpios" for backward compatibility ?
After your change all DTBs using the original pattern will not work with
new kernels any more. At least I would expect such backward compatibility
maintained for few kernel releases.

The reason behind removing the "cs-gpio" or not providing backward
compatibility was

1. Since spi-core started using "cs-gpios" string from spi device node
several months ago.
  The spi-s3c64xx.c driver is partially broken for more than 6 months.

2. Supporting "cs-gpio" would add extra bit of code.

I've corrected the dts files that were using "cs-gpio" under
"controller-data"(child node)
to use "cs-gpio" from spi device node (parent node).

I will make another version if you insist.

quoted

      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);
+             of_node_put(parent_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);
-     return cs;
+     return s3c64xx_get_slave_ctrldata(spi, cs);
 }

 /*

@@ -806,9 +815,14 @@ static int s3c64xx_spi_setup(struct spi_device *spi)
      struct s3c64xx_spi_info *sci;
      int err;

+     if (!spi->dev.of_node) {
+             dev_err(&spi->dev, "device node not found\n");
+             return -EINVAL;
+     }
+
      sdd = spi_master_get_devdata(spi->master);
      if (!cs && spi->dev.of_node) {
-             cs = s3c64xx_get_slave_ctrldata(spi);
+             cs = s3c64xx_get_cs_gpios(spi);
              spi->controller_data = cs;
      }

@@ -1077,7 +1091,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;

Ditto.

I can do that, once we are finalized on having backward compatibility.

quoted

              ret = of_alias_get_id(pdev->dev.of_node, "spi");

--
Thanks!
Sylwester

-- 
Shine bright,
(: Nav :)

Naveen / Sylwester,

On Tue, Jun 10, 2014 at 4:00 AM, Naveen Krishna Ch
[off-list ref] wrote:

quoted

Can we support both "cs-gpio" and "cs-gpios" for backward compatibility ?
After your change all DTBs using the original pattern will not work with
new kernels any more. At least I would expect such backward compatibility
maintained for few kernel releases.

The reason behind removing the "cs-gpio" or not providing backward
compatibility was

1. Since spi-core started using "cs-gpios" string from spi device node
several months ago.
  The spi-s3c64xx.c driver is partially broken for more than 6 months.

2. Supporting "cs-gpio" would add extra bit of code.

I've corrected the dts files that were using "cs-gpio" under
"controller-data"(child node)
to use "cs-gpio" from spi device node (parent node).

I will make another version if you insist.

Yup, as near as I can tell this has been broken since (3146bee spi:
s3c64xx: Added provision for dedicated cs pin).  That landed June 25th
of 2013 into the SPI tree.

...so clearly nobody was really testing Samsung's SPI driver on ToT
Linux.  1 year of unnoticed brokenness seems like enough time that we
could do away with the old code.  If someone comes out of the woodwork
and claims a dire need to support the old binding then it can be added
then.

In-tree users of this appear to be:

arch/arm/boot/dts/exynos4210-smdkv310.dts:
 cs-gpio = <&gpc1 2 0>;
arch/arm/boot/dts/exynos4412-trats2.dts:
 cs-gpio = <&gpb 5 0>;
arch/arm/boot/dts/exynos5250-smdk5250.dts:
 cs-gpio = <&gpa2 5 0>;

...and I guess nobody has bothered using the SPI flash on those boards
for the last year?

Naveen,

Not a full review, but a few quick things I happened to notice:

On Tue, Jun 10, 2014 at 3:08 AM, Naveen Krishna Chatradhi
[off-list ref] wrote:

In a future patch (not this one!) it might make sense to also move
spi-feedback-delay out.

+static struct s3c64xx_spi_csinfo *s3c64xx_get_cs_gpios(struct spi_device *spi)

I believe that you can make use of the fact that the SPI core already
got these GPIOs for you.  See of_spi_register_master().  Everything
you need ought to be in master->num_chipselect and master->cs_gpios.

Strangely, it doesn't look like any other drivers use this, so
possibly I'm confused...

This seems incredibly broken.  You're saying that the SPI driver no
longer works without the device tree?  I don't think that's desirable,
but I suppose I could be wrong.

Also note that you still left an "if" test below for trying to deal
with the fact that there is no "of_node".


-Doug

On 10.06.2014 20:26, Doug Anderson wrote:

Naveen,

Not a full review, but a few quick things I happened to notice:

On Tue, Jun 10, 2014 at 3:08 AM, Naveen Krishna Chatradhi
[off-list ref] wrote:

quoted

@@ -94,7 +93,6 @@ Example:
                        spi-max-frequency = <10000>;

                        controller-data {
-                               cs-gpio = <&gpa2 5 1 0 3>;
                                samsung,spi-feedback-delay = <0>;

In a future patch (not this one!) it might make sense to also move
spi-feedback-delay out.

quoted

+static struct s3c64xx_spi_csinfo *s3c64xx_get_cs_gpios(struct spi_device *spi)

I believe that you can make use of the fact that the SPI core already
got these GPIOs for you.  See of_spi_register_master().  Everything
you need ought to be in master->num_chipselect and master->cs_gpios.

Strangely, it doesn't look like any other drivers use this, so
possibly I'm confused...

quoted

@@ -806,9 +815,14 @@ static int s3c64xx_spi_setup(struct spi_device *spi)
        struct s3c64xx_spi_info *sci;
        int err;

+       if (!spi->dev.of_node) {
+               dev_err(&spi->dev, "device node not found\n");
+               return -EINVAL;
+       }

This seems incredibly broken.  You're saying that the SPI driver no
longer works without the device tree?  I don't think that's desirable,
but I suppose I could be wrong.

Also note that you still left an "if" test below for trying to deal
with the fact that there is no "of_node".

This driver can be also used by non-DT platforms (namely s3c2443,
s3c64xx and s5p*) and so this assumption is wrong.

Best regards,
Tomasz

On 10.06.2014 20:09, Doug Anderson wrote:

Naveen / Sylwester,

On Tue, Jun 10, 2014 at 4:00 AM, Naveen Krishna Ch
[off-list ref] wrote:

quoted

quoted

Can we support both "cs-gpio" and "cs-gpios" for backward compatibility ?
After your change all DTBs using the original pattern will not work with
new kernels any more. At least I would expect such backward compatibility
maintained for few kernel releases.

The reason behind removing the "cs-gpio" or not providing backward
compatibility was

1. Since spi-core started using "cs-gpios" string from spi device node
several months ago.
  The spi-s3c64xx.c driver is partially broken for more than 6 months.

2. Supporting "cs-gpio" would add extra bit of code.

I've corrected the dts files that were using "cs-gpio" under
"controller-data"(child node)
to use "cs-gpio" from spi device node (parent node).

I will make another version if you insist.

Yup, as near as I can tell this has been broken since (3146bee spi:
s3c64xx: Added provision for dedicated cs pin).  That landed June 25th
of 2013 into the SPI tree.

...so clearly nobody was really testing Samsung's SPI driver on ToT
Linux.  1 year of unnoticed brokenness seems like enough time that we
could do away with the old code.  If someone comes out of the woodwork
and claims a dire need to support the old binding then it can be added
then.

In-tree users of this appear to be:

arch/arm/boot/dts/exynos4210-smdkv310.dts:
 cs-gpio = <&gpc1 2 0>;
arch/arm/boot/dts/exynos4412-trats2.dts:
 cs-gpio = <&gpb 5 0>;
arch/arm/boot/dts/exynos5250-smdk5250.dts:
 cs-gpio = <&gpa2 5 0>;

...and I guess nobody has bothered using the SPI flash on those boards
for the last year?

On Trats2, it's actually a camera sensor, not SPI flash...

Still, that's really a shame that:

a) such patch went in (i.e. its brokenness was not spotted in review),
b) the regression was not spotted.

Anyway, IMHO this justifies removing the old binding then.

Best regards,
Tomasz

Hi Naveen,

On 10.06.2014 12:08, Naveen Krishna Chatradhi wrote:

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

[snip]

While at it, you might also update the example to something more
appropriate on Samsung platforms, e.g. <&gpa2 5 0>;

 
 		w25q80bw at 0 {
 			#address-cells = <1>;

[snip]

+static struct s3c64xx_spi_csinfo *s3c64xx_get_cs_gpios(struct spi_device *spi)
+{
+	struct device_node *parent_np = NULL;
+	struct s3c64xx_spi_driver_data *sdd;
+	struct s3c64xx_spi_csinfo *cs;
+
+	parent_np = of_get_parent(spi->dev.of_node);
+	if (!parent_np) {
+		dev_err(&spi->dev, "Parent node not found\n");
 		return ERR_PTR(-EINVAL);
 	}
 
+	sdd = spi_master_get_devdata(spi->master);
+
 	cs = kzalloc(sizeof(*cs), GFP_KERNEL);
 	if (!cs) {
-		of_node_put(data_np);
+		of_node_put(parent_np);
 		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);
+		cs->line = of_get_named_gpio(parent_np, "cs-gpios", 0);

This is wrong. The "cs-gpios" property is supposed to be an array,
indexed by chip select number of SPI devices (indicated by their "reg"
properties).

Moreover, is there a need to parse this manually in this driver? I can
see respective parsing code in of_spi_register_master().

Best regards,
Tomasz

Tomasz,

On Tue, Jun 10, 2014 at 12:49 PM, Tomasz Figa [off-list ref] wrote:

This is wrong. The "cs-gpios" property is supposed to be an array,
indexed by chip select number of SPI devices (indicated by their "reg"
properties).

Moreover, is there a need to parse this manually in this driver? I can
see respective parsing code in of_spi_register_master().

I noticed this too (see my response), but I was confused about the
fact that nobody else uses the array created by
of_spi_register_master().  Any idea why?

-Doug

On 10.06.2014 21:58, Doug Anderson wrote:

Tomasz,

On Tue, Jun 10, 2014 at 12:49 PM, Tomasz Figa [off-list ref] wrote:

quoted

This is wrong. The "cs-gpios" property is supposed to be an array,
indexed by chip select number of SPI devices (indicated by their "reg"
properties).

Moreover, is there a need to parse this manually in this driver? I can
see respective parsing code in of_spi_register_master().

I noticed this too (see my response), but I was confused about the
fact that nobody else uses the array created by
of_spi_register_master().  Any idea why?

Hmm, I can see of_spi_register_master() assigning allocated pointer to
master->cs_gpios, which is then used in spi_add_device() as follows:

	if (master->cs_gpios)
		spi->cs_gpio = master->cs_gpios[spi->chip_select];

Best regards,
Tomasz

Tomasz,

On Tue, Jun 10, 2014 at 12:59 PM, Tomasz Figa [off-list ref] wrote:

On 10.06.2014 21:58, Doug Anderson wrote:

quoted

Tomasz,

On Tue, Jun 10, 2014 at 12:49 PM, Tomasz Figa [off-list ref] wrote:

quoted

This is wrong. The "cs-gpios" property is supposed to be an array,
indexed by chip select number of SPI devices (indicated by their "reg"
properties).

Moreover, is there a need to parse this manually in this driver? I can
see respective parsing code in of_spi_register_master().

I noticed this too (see my response), but I was confused about the
fact that nobody else uses the array created by
of_spi_register_master().  Any idea why?

Hmm, I can see of_spi_register_master() assigning allocated pointer to
master->cs_gpios, which is then used in spi_add_device() as follows:

        if (master->cs_gpios)
                spi->cs_gpio = master->cs_gpios[spi->chip_select];

OK.  I haven't traced through all the code, but I did notice:

* spi-gpio.c: has its own cs_gpios[0], initted with of_get_named_gpio()
* spi-dw-mmio.c, spi-efm32.c, spi-imx.c, spi-pl022.c, spi-sirf.c all
seem to be looking at "cs-gpios" directly.

...but I see now that you're right that most drivers don't need to
actually look at cs_gpios and can just look at the device itself.

-Doug

On Tue, Jun 10, 2014 at 1:09 PM, Doug Anderson [off-list ref] wrote:

Naveen / Sylwester,

On Tue, Jun 10, 2014 at 4:00 AM, Naveen Krishna Ch
[off-list ref] wrote:

quoted

quoted

Can we support both "cs-gpio" and "cs-gpios" for backward compatibility ?
After your change all DTBs using the original pattern will not work with
new kernels any more. At least I would expect such backward compatibility
maintained for few kernel releases.

The reason behind removing the "cs-gpio" or not providing backward
compatibility was

1. Since spi-core started using "cs-gpios" string from spi device node
several months ago.
  The spi-s3c64xx.c driver is partially broken for more than 6 months.

2. Supporting "cs-gpio" would add extra bit of code.

I've corrected the dts files that were using "cs-gpio" under
"controller-data"(child node)
to use "cs-gpio" from spi device node (parent node).

I will make another version if you insist.

Yup, as near as I can tell this has been broken since (3146bee spi:
s3c64xx: Added provision for dedicated cs pin).  That landed June 25th
of 2013 into the SPI tree.

...so clearly nobody was really testing Samsung's SPI driver on ToT
Linux.  1 year of unnoticed brokenness seems like enough time that we
could do away with the old code.  If someone comes out of the woodwork
and claims a dire need to support the old binding then it can be added
then.

In-tree users of this appear to be:

arch/arm/boot/dts/exynos4210-smdkv310.dts:
 cs-gpio = <&gpc1 2 0>;
arch/arm/boot/dts/exynos4412-trats2.dts:
 cs-gpio = <&gpb 5 0>;
arch/arm/boot/dts/exynos5250-smdk5250.dts:
 cs-gpio = <&gpa2 5 0>;

...and I guess nobody has bothered using the SPI flash on those boards
for the last year?

I always try to warn people if they are breaking compatibility, but in
this case I agree it is okay. Presumably no one expects BSD or vxworks
support for this platform either?

For the binding change:

Acked-by: Rob Herring <robh@kernel.org>

Rob

Hello Tomasz,

On 11 June 2014 01:19, Tomasz Figa [off-list ref] wrote:

Hi Naveen,

On 10.06.2014 12:08, Naveen Krishna Chatradhi wrote:

quoted

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

[snip]

quoted

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

While at it, you might also update the example to something more
appropriate on Samsung platforms, e.g. <&gpa2 5 0>;

Sylwester gave this comment, will club with the other changes.

quoted

              w25q80bw at 0 {
                      #address-cells = <1>;

[snip]

quoted

+static struct s3c64xx_spi_csinfo *s3c64xx_get_cs_gpios(struct spi_device *spi)
+{
+     struct device_node *parent_np = NULL;
+     struct s3c64xx_spi_driver_data *sdd;
+     struct s3c64xx_spi_csinfo *cs;
+
+     parent_np = of_get_parent(spi->dev.of_node);
+     if (!parent_np) {
+             dev_err(&spi->dev, "Parent node not found\n");
              return ERR_PTR(-EINVAL);
      }

+     sdd = spi_master_get_devdata(spi->master);
+
      cs = kzalloc(sizeof(*cs), GFP_KERNEL);
      if (!cs) {
-             of_node_put(data_np);
+             of_node_put(parent_np);
              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);
+             cs->line = of_get_named_gpio(parent_np, "cs-gpios", 0);

This is wrong. The "cs-gpios" property is supposed to be an array,
indexed by chip select number of SPI devices (indicated by their "reg"
properties).

Moreover, is there a need to parse this manually in this driver? I can
see respective parsing code in of_spi_register_master().

Right, spi_add_device() parses the "cs-gpios" and gives the "spi->cs_gpio"
before calling the driver setup() function.

Will respin with appropriate changes.

Best regards,
Tomasz

-- 
Shine bright,
(: Nav :)

On 10/06/14 22:32, Rob Herring wrote:

On Tue, Jun 10, 2014 at 1:09 PM, Doug Anderson [off-list ref] wrote:

quoted

Naveen / Sylwester,

On Tue, Jun 10, 2014 at 4:00 AM, Naveen Krishna Ch
[off-list ref] wrote:

quoted

quoted

Can we support both "cs-gpio" and "cs-gpios" for backward compatibility ?
After your change all DTBs using the original pattern will not work with
new kernels any more. At least I would expect such backward compatibility
maintained for few kernel releases.

The reason behind removing the "cs-gpio" or not providing backward
compatibility was

1. Since spi-core started using "cs-gpios" string from spi device node
several months ago.
  The spi-s3c64xx.c driver is partially broken for more than 6 months.

2. Supporting "cs-gpio" would add extra bit of code.

I've corrected the dts files that were using "cs-gpio" under
"controller-data"(child node)
to use "cs-gpio" from spi device node (parent node).

Normally properties get removed from existing dts files and then
marked in the binding documentation as deprecated.  Or least in the
commit messages it would be good to have clearly explained what was
replaced with what and what were the main reasons.  This is especially
useful for out-of-tree dts files, when you port a board to newer
kernel, things suddenly stop working and you're left with little
or no clue why.

quoted

quoted

I will make another version if you insist.

I'm OK with dropping the old binding, I'm generally not a proponent
of a strict DT backward compatibility.  But we _must_ have things
documented properly, so it all doesn't turn into a debugging nightmare.

quoted

Yup, as near as I can tell this has been broken since (3146bee spi:
s3c64xx: Added provision for dedicated cs pin).  That landed June 25th
of 2013 into the SPI tree.

...so clearly nobody was really testing Samsung's SPI driver on ToT
Linux.  1 year of unnoticed brokenness seems like enough time that we
could do away with the old code.  If someone comes out of the woodwork
and claims a dire need to support the old binding then it can be added
then.

In-tree users of this appear to be:

arch/arm/boot/dts/exynos4210-smdkv310.dts:
 cs-gpio = <&gpc1 2 0>;
arch/arm/boot/dts/exynos4412-trats2.dts:
 cs-gpio = <&gpb 5 0>;
arch/arm/boot/dts/exynos5250-smdk5250.dts:
 cs-gpio = <&gpa2 5 0>;

...and I guess nobody has bothered using the SPI flash on those boards
for the last year?

I guess the SMDK boards are not really well maintained now.  I've got a
working kernel for trats2 based on v3.15-rc4, the SPI is working fine
there, despite the offending commit you point out.

It seems the GPB[5]/SPI_1_nSS pin is in the reset default state, i.e.
an input with pull down enabled (gpb-5 is not a part of the SPI
interface pinctrl group).  So in case we don't pass the CS GPIO in the
SPI controller node or the driver doesn't handle it properly for any
other reason the chip select signal is always active and the SPI
communication works.

I always try to warn people if they are breaking compatibility, but in
this case I agree it is okay. Presumably no one expects BSD or vxworks
support for this platform either?

For the binding change:

Acked-by: Rob Herring <robh@kernel.org>

Rob

--
Thanks,
Sylwester