[PATCH] spi/s3c64xx: Update DT binding documentation to match code

[PATCH] spi/s3c64xx: Update DT binding documentation to match code

[Charles Keepax]

The following patch added support for spi controllers with a dedicated
chip select pin:

commit 3146beec21b64f4551fcf0ac148381d54dc41b1b
spi: s3c64xx: Added provision for dedicated cs pin

It updated the device tree binding to require a "cs-gpio" property to be
specified on the spi controller node if chip selects will be given as
GPIOs per slave, rather than the controller having a dedicated internal
chip select pin.

This patch updates the device tree binding documentation to match this.

Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
---

Hi,

Apologies if I missed something but there are a couple of
things I am not sure about in the original patch, there
seems to be no special handling for the dedicated case, like
set a bit that enables this or some such, which implies that
parts with dedicated cs pins will always update them even
if cs-gpio is specified. In which case wasn't what existed
before reasonable?

If the concern is that someone would try to use both the
internal chip select and a GPIO for two slaves on the same
controller, causing the first slave to always get chip
selected wouldn't a better solution be to require all slaves
to give a chip select GPIO if one does?

Finally, if we really want to specify in the DT for the
controller that it has an internal chip select wouldn't it
be better to invert the logic and  call it something like
"dedicated-cs" as then older bindings that currently use
GPIOs wouldn't break.

I thought it better to do a patch to update the
documentation rather than change the binding again, but
personally I would lean on the side of updating the binding
if others are in favour of it?

Thanks,
Charles

 .../devicetree/bindings/spi/spi-samsung.txt        |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/Documentation/devicetree/bindings/spi/spi-samsung.txt b/Documentation/devicetree/bindings/spi/spi-samsung.txt
index 86aa061..2f0167d 100644
--- a/Documentation/devicetree/bindings/spi/spi-samsung.txt
+++ b/Documentation/devicetree/bindings/spi/spi-samsung.txt
@@ -42,6 +42,11 @@ 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-gpio: Specifies that the spi controller will give a chip select gpio
+  for each slave node. Absence of this indicates that the controller has a
+  dedicated chip internal select pin and any GPIOs specified on the slaves
+  will be ignored.
+
 SPI Controller specific data in SPI slave nodes:
 
 - The spi slave nodes should provide the following information which is required
@@ -85,6 +90,7 @@ Example:
 		#size-cells = <0>;
 		pinctrl-names = "default";
 		pinctrl-0 = <&spi0_bus>;
+		cs-gpio;
 
 		w25q80bw@0 {
 			#address-cells = <1>;
-- 
1.7.2.5

[PATCH] spi/s3c64xx: Update DT binding documentation to match code

[Charles Keepax]

The following patch added support for spi controllers with a dedicated
chip select pin:

commit 3146beec21b64f4551fcf0ac148381d54dc41b1b
spi: s3c64xx: Added provision for dedicated cs pin

It updated the device tree binding to require a "cs-gpio" property to be
specified on the spi controller node if chip selects will be given as
GPIOs per slave, rather than the controller having a dedicated internal
chip select pin.

This patch updates the device tree binding documentation to match this.

Signed-off-by: Charles Keepax <ckeepax-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>
---

Hi,

Apologies if I missed something but there are a couple of
things I am not sure about in the original patch, there
seems to be no special handling for the dedicated case, like
set a bit that enables this or some such, which implies that
parts with dedicated cs pins will always update them even
if cs-gpio is specified. In which case wasn't what existed
before reasonable?

If the concern is that someone would try to use both the
internal chip select and a GPIO for two slaves on the same
controller, causing the first slave to always get chip
selected wouldn't a better solution be to require all slaves
to give a chip select GPIO if one does?

Finally, if we really want to specify in the DT for the
controller that it has an internal chip select wouldn't it
be better to invert the logic and  call it something like
"dedicated-cs" as then older bindings that currently use
GPIOs wouldn't break.

I thought it better to do a patch to update the
documentation rather than change the binding again, but
personally I would lean on the side of updating the binding
if others are in favour of it?

Thanks,
Charles

 .../devicetree/bindings/spi/spi-samsung.txt        |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/Documentation/devicetree/bindings/spi/spi-samsung.txt b/Documentation/devicetree/bindings/spi/spi-samsung.txt
index 86aa061..2f0167d 100644
--- a/Documentation/devicetree/bindings/spi/spi-samsung.txt
+++ b/Documentation/devicetree/bindings/spi/spi-samsung.txt
@@ -42,6 +42,11 @@ 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-gpio: Specifies that the spi controller will give a chip select gpio
+  for each slave node. Absence of this indicates that the controller has a
+  dedicated chip internal select pin and any GPIOs specified on the slaves
+  will be ignored.
+
 SPI Controller specific data in SPI slave nodes:
 
 - The spi slave nodes should provide the following information which is required
@@ -85,6 +90,7 @@ Example:
 		#size-cells = <0>;
 		pinctrl-names = "default";
 		pinctrl-0 = <&spi0_bus>;
+		cs-gpio;
 
 		w25q80bw@0 {
 			#address-cells = <1>;
-- 
1.7.2.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi/s3c64xx: Update DT binding documentation to match code

[Mark Brown]

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

On Thu, Mar 06, 2014 at 05:05:39PM +0000, Charles Keepax wrote:

> The following patch added support for spi controllers with a dedicated
> chip select pin:
> 
> commit 3146beec21b64f4551fcf0ac148381d54dc41b1b
> spi: s3c64xx: Added provision for dedicated cs pin
> 
> It updated the device tree binding to require a "cs-gpio" property to be
> specified on the spi controller node if chip selects will be given as
> GPIOs per slave, rather than the controller having a dedicated internal
> chip select pin.

No, it doesn't - it's saying that if the device has a "cs-gpio" property
then to use that as the chip select.  It's not a boolean, it's a GPIO
specifier.  Looking at the code it looks like the intention is to search
all children for a cs-gpio during the controller probe, it's possible
that this isn't working correctly.

> Apologies if I missed something but there are a couple of
> things I am not sure about in the original patch, there
> seems to be no special handling for the dedicated case, like
> set a bit that enables this or some such, which implies that
> parts with dedicated cs pins will always update them even
> if cs-gpio is specified. In which case wasn't what existed
> before reasonable?

The chip select signal within the IP always needs to be manipulated for
the hardware to work regardless of a GPIO being used, if a GPIO is being
used as the actual chip select then the expecation is that the pinmux
will be set up so that the signal isn't actually brought out of the
device. 

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

Re: [PATCH] spi/s3c64xx: Update DT binding documentation to match code

[Mark Brown]

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

On Thu, Mar 06, 2014 at 05:05:39PM +0000, Charles Keepax wrote:

> The following patch added support for spi controllers with a dedicated
> chip select pin:
> 
> commit 3146beec21b64f4551fcf0ac148381d54dc41b1b
> spi: s3c64xx: Added provision for dedicated cs pin
> 
> It updated the device tree binding to require a "cs-gpio" property to be
> specified on the spi controller node if chip selects will be given as
> GPIOs per slave, rather than the controller having a dedicated internal
> chip select pin.

No, it doesn't - it's saying that if the device has a "cs-gpio" property
then to use that as the chip select.  It's not a boolean, it's a GPIO
specifier.  Looking at the code it looks like the intention is to search
all children for a cs-gpio during the controller probe, it's possible
that this isn't working correctly.

> Apologies if I missed something but there are a couple of
> things I am not sure about in the original patch, there
> seems to be no special handling for the dedicated case, like
> set a bit that enables this or some such, which implies that
> parts with dedicated cs pins will always update them even
> if cs-gpio is specified. In which case wasn't what existed
> before reasonable?

The chip select signal within the IP always needs to be manipulated for
the hardware to work regardless of a GPIO being used, if a GPIO is being
used as the actual chip select then the expecation is that the pinmux
will be set up so that the signal isn't actually brought out of the
device. 

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

Re: [PATCH] spi/s3c64xx: Update DT binding documentation to match code

[Charles Keepax]

On Fri, Mar 07, 2014 at 10:48:41AM +0800, Mark Brown wrote:
> On Thu, Mar 06, 2014 at 05:05:39PM +0000, Charles Keepax wrote:
> 
> > The following patch added support for spi controllers with a dedicated
> > chip select pin:
> > 
> > commit 3146beec21b64f4551fcf0ac148381d54dc41b1b
> > spi: s3c64xx: Added provision for dedicated cs pin
> > 
> > It updated the device tree binding to require a "cs-gpio" property to be
> > specified on the spi controller node if chip selects will be given as
> > GPIOs per slave, rather than the controller having a dedicated internal
> > chip select pin.
> 
> No, it doesn't - it's saying that if the device has a "cs-gpio" property
> then to use that as the chip select.  It's not a boolean, it's a GPIO
> specifier.  Looking at the code it looks like the intention is to search
> all children for a cs-gpio during the controller probe, it's possible
> that this isn't working correctly.

That is basically part of my question is the current setup doing
what it is intended to? The Samsung binding has controller-data
blocks on each of the slaves that specify the gpio for that
slave.

@@ -1326,7 +1340,11 @@ 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;

This part of the original patch adds the check, I guess the
mistake could be an assumption that of_find_property will
recursively check all the children, which it won't. This will
just check the controller node itself for a cs-gpio property but
as these are specified within sub-nodes they won't be found.
Hence currently you need to add one at the controller level.

Thanks,
Charles

Re: [PATCH] spi/s3c64xx: Update DT binding documentation to match code

[Mark Brown]

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

On Fri, Mar 07, 2014 at 09:19:08AM +0000, Charles Keepax wrote:
> On Fri, Mar 07, 2014 at 10:48:41AM +0800, Mark Brown wrote:
> > On Thu, Mar 06, 2014 at 05:05:39PM +0000, Charles Keepax wrote:

> > > It updated the device tree binding to require a "cs-gpio" property to be
> > > specified on the spi controller node if chip selects will be given as
> > > GPIOs per slave, rather than the controller having a dedicated internal
> > > chip select pin.

> > No, it doesn't - it's saying that if the device has a "cs-gpio" property
> > then to use that as the chip select.  It's not a boolean, it's a GPIO
> > specifier.  Looking at the code it looks like the intention is to search
> > all children for a cs-gpio during the controller probe, it's possible
> > that this isn't working correctly.

> That is basically part of my question is the current setup doing
> what it is intended to? The Samsung binding has controller-data
> blocks on each of the slaves that specify the gpio for that
> slave.

Right, which is also clearly the intention of the code.

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

Re: [PATCH] spi/s3c64xx: Update DT binding documentation to match code

[Mark Brown]

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

On Fri, Mar 07, 2014 at 09:19:08AM +0000, Charles Keepax wrote:
> On Fri, Mar 07, 2014 at 10:48:41AM +0800, Mark Brown wrote:
> > On Thu, Mar 06, 2014 at 05:05:39PM +0000, Charles Keepax wrote:

> > > It updated the device tree binding to require a "cs-gpio" property to be
> > > specified on the spi controller node if chip selects will be given as
> > > GPIOs per slave, rather than the controller having a dedicated internal
> > > chip select pin.

> > No, it doesn't - it's saying that if the device has a "cs-gpio" property
> > then to use that as the chip select.  It's not a boolean, it's a GPIO
> > specifier.  Looking at the code it looks like the intention is to search
> > all children for a cs-gpio during the controller probe, it's possible
> > that this isn't working correctly.

> That is basically part of my question is the current setup doing
> what it is intended to? The Samsung binding has controller-data
> blocks on each of the slaves that specify the gpio for that
> slave.

Right, which is also clearly the intention of the code.

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