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

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 <[email protected]>

Acked-by Rob Herring <[email protected]>

Cc Javier Martinez Canillas <[email protected]>

Cc Doug Anderson <[email protected]>

Cc Tomasz Figa <[email protected]>

---
Changes since v2:
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(-)

arch/arm/boot/dts/exynos4210-smdkv310.dts +1 -1

index 636d166..9191491 100644
@@ -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>;
 			};
 

arch/arm/boot/dts/exynos4412-trats2.dts +1 -1

index 8a558b7..204b0de 100644
@@ -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>;
 			};
 		};

arch/arm/boot/dts/exynos5250-smdk5250.dts +1 -0

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

Hello Naveen,

Quoted 3 lines · Naveen Krishna Chatradhi

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

Your patch looks good to me but I think that the commit message is inaccurate.

It does not move the cs-gpio property to the top SPI dev node but it replaces the cs-gpio from controller-data 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.

Having a proper commit message is important since out-of-tree DTS can use this commit as a reference on how to fix their FDT in case this change broke their platforms.

Quoted 72 lines

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

Signed-off-by Naveen Krishna Chatradhi <[email protected]>

Acked-by Rob Herring <[email protected]>

Cc Javier Martinez Canillas <[email protected]>

Cc Doug Anderson <[email protected]>

Cc Tomasz Figa <[email protected]>

---
Changes since v2:
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(-)

arch/arm/boot/dts/exynos4210-smdkv310.dts +1 -1

index 636d166..9191491 100644
@@ -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>;
                      };
 

arch/arm/boot/dts/exynos4412-trats2.dts +1 -1

index 8a558b7..204b0de 100644
@@ -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>;
                      };
              };

arch/arm/boot/dts/exynos5250-smdk5250.dts +1 -0

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

arch/arm/boot/dts/exynos5250-smdk5250.dts line 332

Quoted 6 lines

arch/arm/boot/dts/exynos5250-smdk5250.dts +0 -1

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

Best regards,
Javier

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 <[email protected]>

Acked-by Rob Herring <[email protected]>

Cc Javier Martinez Canillas <[email protected]>

Cc Doug Anderson <[email protected]>

Cc Tomasz Figa <[email protected]>

---
Changes since v2:
1. updated the gpios usage in Documentation
2. use the spi->cs_gpio in the driver, instead of parsing the node again.
3. Corrected error check of the of.node and during gpio_free

 .../devicetree/bindings/spi/spi-samsung.txt        |    8 +++-----
 drivers/spi/spi-s3c64xx.c                          |   18 ++++++------------
 2 files changed, 9 insertions(+), 17 deletions(-)

Documentation/devicetree/bindings/spi/spi-samsung.txt +3 -5

index 86aa061..2d29dac 100644
@@ -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 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>;
 			};
 

drivers/spi/spi-s3c64xx.c +0 -0

index 75a5696..842b148 100644

drivers/spi/spi-s3c64xx.c +0 -2

 	return cs;
 }
 

@@ -828,8 +823,6 @@ static int s3c64xx_spi_setup(struct spi_device *spi)
 					cs->line, err);
 				goto err_gpio_req;
 			}
-
-			spi->cs_gpio = cs->line;
 		}
 
 		spi_set_ctldata(spi, cs);

Hello Naveen,

Thanks a lot for your patches and sorry that I didn't review your prior two versions but I didn't have the time yesterday.

Quoted 3 lines · Naveen Krishna Chatradhi

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

I think that the commit message is not clear enough about the intentions behind your patch.

It's not only that spi-s3c64xx needs a cs-gpio chip to be defined under controller-data dev node while all other SPI drivers expects cs-gpios at the top level, but more important that currently s3c64xx driver expects to have both cs-gpio (singular) at the top level *and* cs-gpio in controller data which doesn't make too much sense.

Quoted 3 lines

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

As I said, currently it expects cs-gpio (singular) not cs-gpios (plural) as your example. It's important to have a correct commit message so future code archaeologists can have a proper picture of the situation if needed.

Quoted 12 lines

      slave_node {

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

Quoted 7 lines

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.

So, the problem is not that the binding is not consistent with other SPI drivers (that would have been bad but acceptable IMHO) but that it is completely broken. And since we have to fix it which means breaking the ABI anyways, it is better to make it consistent with other drivers and SPI core.

Quoted 3 lines

Also updates the Device tree Documentation.

While I agree with others that a binding that has been broken for a year is a binding that appears to not be used a lot, we should be more vocal about this and why breaking backward compatibility is the right approach in this case.

So, in the (theoretical?) case that users find that their platform stopped to work with their current FDT, it will be easy for them to figure out what commit break it, what was the motivation to break the ABI and what changes are needed on their DTS to make it work again.

Adding the sha1 of the culprit commit (3146bee spi: s3c64xx: Added provision for dedicated cs pin) that puts us in this situation will also be useful. Since the date of that commit is part of the rationale behind this change. (e.g: nobody cared about this binding and so can be changed).

Quoted 59 lines

Signed-off-by Naveen Krishna Chatradhi <[email protected]>

Acked-by Rob Herring <[email protected]>

Cc Javier Martinez Canillas <[email protected]>

Cc Doug Anderson <[email protected]>

Cc Tomasz Figa <[email protected]>

---
Changes since v2:
1. updated the gpios usage in Documentation
2. use the spi->cs_gpio in the driver, instead of parsing the node again.
3. Corrected error check of the of.node and during gpio_free

 .../devicetree/bindings/spi/spi-samsung.txt        |    8 +++-----
 drivers/spi/spi-s3c64xx.c                          |   18 ++++++------------
 2 files changed, 9 insertions(+), 17 deletions(-)

Documentation/devicetree/bindings/spi/spi-samsung.txt +3 -5

index 86aa061..2d29dac 100644
@@ -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 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>;
                      };
 

drivers/spi/spi-s3c64xx.c +0 -0

index 75a5696..842b148 100644

drivers/spi/spi-s3c64xx.c static struct s3c64xx_spi_csinfo *s3c64xx_get_slave_ctrldata( · line 775

Quoted 4 lines

drivers/spi/spi-s3c64xx.c +0 -1

@@ -772,24 +772,19 @@ static struct s3c64xx_spi_csinfo *s3c64xx_get_slave_ctrldata(
 
      cs = kzalloc(sizeof(*cs), GFP_KERNEL);
      if (!cs) {
-             of_node_put(data_np);

Why are you removing this?

You are not removing the call to of_get_child_by_name(slave_np, "controller-data") so on failure data_np reference counter still needs to be decremented.

drivers/spi/spi-s3c64xx.c static struct s3c64xx_spi_csinfo *s3c64xx_get_slave_ctrldata( · line 786

Quoted 12 lines

drivers/spi/spi-s3c64xx.c +1 -7

              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)) {
+     if (!gpio_is_valid(spi->cs_gpio)) {
              dev_err(&spi->dev, "chip select gpio is not specified or invalid\n");
-             kfree(cs);
-             of_node_put(data_np);

Again, it's wrong to remove this of_node_put(data_np) call.

drivers/spi/spi-s3c64xx.c static struct s3c64xx_spi_csinfo *s3c64xx_get_slave_ctrldata( · line 783

Quoted 5 lines

drivers/spi/spi-s3c64xx.c +1 -0

              return ERR_PTR(-EINVAL);
      }
+     cs->line = spi->cs_gpio;
 

I wonder why are you keeping cs->line? AFAICT it's only used in s3c64xx_spi_setup() to request the GPIO and since you get the struct spi_device pointer as a parameter then you can just use spi->s_gpio instead.

drivers/spi/spi-s3c64xx.c static struct s3c64xx_spi_csinfo *s3c64xx_get_slave_ctrldata( · line 787

Quoted 5 lines

drivers/spi/spi-s3c64xx.c +1 -0

      of_property_read_u32(data_np, "samsung,spi-feedback-delay", &fb_delay);
      cs->fb_delay = fb_delay;
      of_node_put(data_np);
+

Usually is better to not mix style and functionality changes in the same patch since it makes harder to review.

Quoted 16 lines

drivers/spi/spi-s3c64xx.c +0 -2

      return cs;
 }
 

@@ -828,8 +823,6 @@ static int s3c64xx_spi_setup(struct spi_device *spi)
                                      cs->line, err);
                              goto err_gpio_req;
                      }
-
-                     spi->cs_gpio = cs->line;
              }
 
              spi_set_ctldata(spi, cs);

@@ -884,7 +877,8 @@ setup_exit:
      /* setup() returns with device de-selected */
      writel(S3C64XX_SPI_SLAVE_SIG_INACT, sdd->regs + S3C64XX_SPI_SLAVE_SEL);
 

drivers/spi/spi-s3c64xx.c setup_exit: · line 881

Quoted 3 lines

drivers/spi/spi-s3c64xx.c +2 -1

-     gpio_free(cs->line);
+     if (cs->line)
+             gpio_free(cs->line);

Same here, we can just get rid of cs->line and use gpio_free(spi->cs_gpio) instead.

drivers/spi/spi-s3c64xx.c static int s3c64xx_spi_probe(struct platform_device *pdev) · line 1075

Quoted 11 lines

drivers/spi/spi-s3c64xx.c +1 -1

      spi_set_ctldata(spi, NULL);
 
 err_gpio_req:

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

I think we can get rid of sdd->cs_gpio as well. You can always use gpio_is_valid(spi->cs_gpio) or if (master->cs_gpios) to figure out if the SPI device has a cs-gpios array associated.

drivers/spi/spi-s3c64xx.c static int s3c64xx_spi_probe(struct platform_device *pdev) · line 1077

Quoted 4 lines

drivers/spi/spi-s3c64xx.c +0 -0

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

Best regards,
Javier

Hello Javier,

Quoted 19 lines · Javier Martinez Canillas

Hello Naveen,

Thanks a lot for your patches and sorry that I didn't review your prior two versions but I didn't have the time yesterday.

Quoted 3 lines · Naveen Krishna Chatradhi

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

I think that the commit message is not clear enough about the intentions behind your patch.

It's not only that spi-s3c64xx needs a cs-gpio chip to be defined under controller-data dev node while all other SPI drivers expects cs-gpios at the top level, but more important that currently s3c64xx driver expects to have both cs-gpio (singular) at the top level *and* cs-gpio in controller data which doesn't make too much sense.

Quoted 7 lines

Quoted 3 lines

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

As I said, currently it expects cs-gpio (singular) not cs-gpios (plural) as your example. It's important to have a correct commit message so future code archaeologists can have a proper picture of the situation if needed.

Will add some explanation in the example, for it to be clear.

Quoted 20 lines

Quoted 12 lines

      slave_node {

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

Quoted 7 lines

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.

Quoted 4 lines

So, the problem is not that the binding is not consistent with other SPI drivers (that would have been bad but acceptable IMHO) but that it is completely broken. And since we have to fix it which means breaking the ABI anyways, it is better to make it consistent with other drivers and SPI core.

Will take this point for the while rewriting the commit messages

Quoted 13 lines

Quoted 3 lines

Also updates the Device tree Documentation.

While I agree with others that a binding that has been broken for a year is a binding that appears to not be used a lot, we should be more vocal about this and why breaking backward compatibility is the right approach in this case.

So, in the (theoretical?) case that users find that their platform stopped to work with their current FDT, it will be easy for them to figure out what commit break it, what was the motivation to break the ABI and what changes are needed on their DTS to make it work again.

Quoted 4 lines

Adding the sha1 of the culprit commit (3146bee spi: s3c64xx: Added provision for dedicated cs pin) that puts us in this situation will also be useful. Since the date of that commit is part of the rationale behind this change. (e.g: nobody cared about this binding and so can be changed).

Will take this point for the while rewriting the commit messages

Quoted 83 lines

Quoted 59 lines

Signed-off-by Naveen Krishna Chatradhi <[email protected]>

Acked-by Rob Herring <[email protected]>

Cc Javier Martinez Canillas <[email protected]>

Cc Doug Anderson <[email protected]>

Cc Tomasz Figa <[email protected]>

---
Changes since v2:
1. updated the gpios usage in Documentation
2. use the spi->cs_gpio in the driver, instead of parsing the node again.
3. Corrected error check of the of.node and during gpio_free

 .../devicetree/bindings/spi/spi-samsung.txt        |    8 +++-----
 drivers/spi/spi-s3c64xx.c                          |   18 ++++++------------
 2 files changed, 9 insertions(+), 17 deletions(-)

Documentation/devicetree/bindings/spi/spi-samsung.txt +3 -5

index 86aa061..2d29dac 100644
@@ -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 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>;
                      };
 

drivers/spi/spi-s3c64xx.c +0 -0

index 75a5696..842b148 100644

drivers/spi/spi-s3c64xx.c static struct s3c64xx_spi_csinfo *s3c64xx_get_slave_ctrldata( · line 775

Quoted 4 lines

drivers/spi/spi-s3c64xx.c +0 -1

@@ -772,24 +772,19 @@ static struct s3c64xx_spi_csinfo *s3c64xx_get_slave_ctrldata(
 
      cs = kzalloc(sizeof(*cs), GFP_KERNEL);
      if (!cs) {
-             of_node_put(data_np);

Why are you removing this?

You are not removing the call to of_get_child_by_name(slave_np, "controller-data") so on failure data_np reference counter still needs to be decremented.

Quoted 12 lines

drivers/spi/spi-s3c64xx.c +1 -7

              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)) {
+     if (!gpio_is_valid(spi->cs_gpio)) {
              dev_err(&spi->dev, "chip select gpio is not specified or invalid\n");
-             kfree(cs);
-             of_node_put(data_np);

Quoted 1 line

Again, it's wrong to remove this of_node_put(data_np) call.

Actually, i wanted to move down the of_get_child_by_name() to before of_property_read_u32() call.

Quoted 6 lines

Quoted 5 lines

drivers/spi/spi-s3c64xx.c +1 -0

              return ERR_PTR(-EINVAL);
      }
+     cs->line = spi->cs_gpio;
 

Quoted 3 lines

I wonder why are you keeping cs->line? AFAICT it's only used in s3c64xx_spi_setup() to request the GPIO and since you get the struct spi_device pointer as a parameter then you can just use spi->s_gpio instead.

I'm trying not to touch the non-DT part of the code.

struct s3c64xx_spi_csinfo *cs = spi->controller_data;

This will update the cs->line and cs->fb_delay in case of non-DT.

Quoted 25 lines

drivers/spi/spi-s3c64xx.c static struct s3c64xx_spi_csinfo *s3c64xx_get_slave_ctrldata( · line 787

Quoted 5 lines

drivers/spi/spi-s3c64xx.c +1 -0

      of_property_read_u32(data_np, "samsung,spi-feedback-delay", &fb_delay);
      cs->fb_delay = fb_delay;
      of_node_put(data_np);
+

Usually is better to not mix style and functionality changes in the same patch since it makes harder to review.

Quoted 16 lines

drivers/spi/spi-s3c64xx.c +0 -2

      return cs;
 }
 

@@ -828,8 +823,6 @@ static int s3c64xx_spi_setup(struct spi_device *spi)
                                      cs->line, err);
                              goto err_gpio_req;
                      }
-
-                     spi->cs_gpio = cs->line;
              }
 
              spi_set_ctldata(spi, cs);

@@ -884,7 +877,8 @@ setup_exit:
      /* setup() returns with device de-selected */
      writel(S3C64XX_SPI_SLAVE_SIG_INACT, sdd->regs + S3C64XX_SPI_SLAVE_SEL);
 

Quoted 5 lines

drivers/spi/spi-s3c64xx.c setup_exit: · line 881

Quoted 3 lines

drivers/spi/spi-s3c64xx.c +2 -1

-     gpio_free(cs->line);
+     if (cs->line)
+             gpio_free(cs->line);

Same here, we can just get rid of cs->line and use gpio_free(spi->cs_gpio) instead.

This is to keep the non-DT part untouched.

Quoted 15 lines

drivers/spi/spi-s3c64xx.c static int s3c64xx_spi_probe(struct platform_device *pdev) · line 1075

Quoted 11 lines

drivers/spi/spi-s3c64xx.c +1 -1

      spi_set_ctldata(spi, NULL);
 
 err_gpio_req:

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

I think we can get rid of sdd->cs_gpio as well. You can always use gpio_is_valid(spi->cs_gpio) or if (master->cs_gpios) to figure out if the SPI device has a cs-gpios array associated.

This is to keep the non-DT part untouched

Quoted 7 lines

drivers/spi/spi-s3c64xx.c static int s3c64xx_spi_probe(struct platform_device *pdev) · line 1077

Quoted 4 lines

drivers/spi/spi-s3c64xx.c +0 -0

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

Best regards,
Javier

Will re-spin after waiting for a couple of more comments.

Thanks for your review.

Quoted 3 lines · Naveen Krishna Ch

Quoted 1 line · Javier Martinez Canillas

On 06/11/2014 08:31 AM, Naveen Krishna Chatradhi wrote:

Quoted 11 lines

Quoted 9 lines

Quoted 5 lines

              return ERR_PTR(-EINVAL);
      }

+ cs->line = spi->cs_gpio;

I wonder why are you keeping cs->line? AFAICT it's only used in s3c64xx_spi_setup() to request the GPIO and since you get the struct spi_device pointer as a parameter then you can just use spi->s_gpio instead.

I'm trying not to touch the non-DT part of the code.

Quoted 3 lines

struct s3c64xx_spi_csinfo *cs = spi->controller_data;

This will update the cs->line and cs->fb_delay in case of non-DT.

I see, then I prefer the opposite and do something like this on s3c64xx_spi_probe():

if (!pdev->dev.of_node)

       spi->cs_gpio = cs->line;

Quoted 4 lines · Javier Martinez Canillas

Quoted 3 lines · Naveen Krishna Ch

Quoted 1 line · Javier Martinez Canillas

On 06/11/2014 08:31 AM, Naveen Krishna Chatradhi wrote:

[snip]

Quoted 15 lines

Quoted 14 lines

Quoted 9 lines

Quoted 5 lines

              return ERR_PTR(-EINVAL);
      }

+ cs->line = spi->cs_gpio;

I wonder why are you keeping cs->line? AFAICT it's only used in s3c64xx_spi_setup() to request the GPIO and since you get the struct spi_device pointer as a parameter then you can just use spi->s_gpio instead.

I'm trying not to touch the non-DT part of the code.

struct s3c64xx_spi_csinfo *cs = spi->controller_data;

This will update the cs->line and cs->fb_delay in case of non-DT.

Quoted 4 lines

I see, then I prefer the opposite and do something like this on s3c64xx_spi_probe():

if (!pdev->dev.of_node)

       spi->cs_gpio = cs->line;

Hmm, as far as I understand, spi here is spi_device, not spi_master. I don't think you have access to SPI devices on your bus in controller probe().

What I think could work is reworking the driver to:

- in DT case, don't do anything in the driver about the GPIO chip select, because it will be handled automatically by the core.

- in non-DT case, in s3c64xx_spi_setup(), just take the GPIO pin from s3c64xx_spi_csinfo struct passed through spi->controller_data, request it and save it to spi->cs_gpio,

- in non-DT case, in s3c64xx_spi_cleanup(), free the GPIO requested in s3c64xx_spi_setup() and set spi->cs_gpio to -ENOENT (as done initially in spi_alloc_device()).

Best regards,
Tomasz

Quoted 4 lines · Javier Martinez Canillas

Quoted 3 lines · Naveen Krishna Ch

Quoted 1 line · Javier Martinez Canillas

On 06/11/2014 08:31 AM, Naveen Krishna Chatradhi wrote:

[snip]

Quoted 11 lines

Quoted 11 lines

Quoted 9 lines

Quoted 5 lines

              return ERR_PTR(-EINVAL);
      }

+ cs->line = spi->cs_gpio;

I wonder why are you keeping cs->line? AFAICT it's only used in s3c64xx_spi_setup() to request the GPIO and since you get the struct spi_device pointer as a parameter then you can just use spi->s_gpio instead.

I'm trying not to touch the non-DT part of the code.

Quoted 8 lines

Quoted 3 lines

struct s3c64xx_spi_csinfo *cs = spi->controller_data;

This will update the cs->line and cs->fb_delay in case of non-DT.

I see, then I prefer the opposite and do something like this on s3c64xx_spi_probe():

if (!pdev->dev.of_node)

       spi->cs_gpio = cs->line;

Hmm, as far as I understand, spi here is spi_device, not spi_master. I don't think you have access to SPI devices on your bus in controller probe().

What I think could work is reworking the driver to:

- in DT case, don't do anything in the driver about the GPIO chip select, because it will be handled automatically by the core.

- in non-DT case, in s3c64xx_spi_setup(), just take the GPIO pin from s3c64xx_spi_csinfo struct passed through spi->controller_data, request it and save it to spi->cs_gpio,

- in non-DT case, in s3c64xx_spi_cleanup(), free the GPIO requested in s3c64xx_spi_setup() and set spi->cs_gpio to -ENOENT (as done initially in spi_alloc_device()).

Best regards,
Tomasz

Quoted 2 lines · Tomasz Figa

- in DT case, don't do anything in the driver about the GPIO chip select, because it will be handled automatically by the core.

But, i see that gpio_request_one is needed in _setup function in the driver. Except that no other gpio related operation is needed in the driver.

Hello Tomasz,

Quoted 32 lines · Tomasz Figa

Quoted 4 lines · Javier Martinez Canillas

Quoted 3 lines · Naveen Krishna Ch

Quoted 1 line · Javier Martinez Canillas

On 06/11/2014 08:31 AM, Naveen Krishna Chatradhi wrote:

[snip]

Quoted 11 lines

Quoted 11 lines

Quoted 9 lines

Quoted 5 lines

              return ERR_PTR(-EINVAL);
      }

+ cs->line = spi->cs_gpio;

I wonder why are you keeping cs->line? AFAICT it's only used in s3c64xx_spi_setup() to request the GPIO and since you get the struct spi_device pointer as a parameter then you can just use spi->s_gpio instead.

I'm trying not to touch the non-DT part of the code.

Quoted 8 lines

Quoted 3 lines

struct s3c64xx_spi_csinfo *cs = spi->controller_data;

This will update the cs->line and cs->fb_delay in case of non-DT.

I see, then I prefer the opposite and do something like this on s3c64xx_spi_probe():

if (!pdev->dev.of_node)

       spi->cs_gpio = cs->line;

Hmm, as far as I understand, spi here is spi_device, not spi_master. I don't think you have access to SPI devices on your bus in controller probe().

Quoted 4 lines

What I think could work is reworking the driver to:

- in DT case, don't do anything in the driver about the GPIO chip select, because it will be handled automatically by the core.

But, i see that gpio_request_one is needed in _setup function in the driver. Except that no other gpio related operation is needed in the driver.

Quoted 8 lines

- in non-DT case, in s3c64xx_spi_setup(), just take the GPIO pin from s3c64xx_spi_csinfo struct passed through spi->controller_data, request it and save it to spi->cs_gpio,

- in non-DT case, in s3c64xx_spi_cleanup(), free the GPIO requested in s3c64xx_spi_setup() and set spi->cs_gpio to -ENOENT (as done initially in spi_alloc_device()).

Its better going back to the working way.