[1/3,v5] spi: s3c64xx: fix broken "cs_gpios" usage in the driver
diff mbox

Message ID 1402631992-500-1-git-send-email-ch.naveen@samsung.com
State New, archived
Headers show

Commit Message

Naveen Krishna Chatradhi June 13, 2014, 3:59 a.m. UTC
Since, (3146bee spi: s3c64xx: Added provision for dedicated cs pin)

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

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

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

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

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

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

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

Signed-off-by: Naveen Krishna Chatradhi <ch.naveen@samsung.com>
Acked-by: Rob Herring <robh@kernel.org>
Reviewed-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
Tested-by: Doug Anderson <dianders@chromium.org>
Cc: Tomasz Figa <t.figa@samsung.com>
---
Changes since 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                          |   41 ++++++++------------
 2 files changed, 20 insertions(+), 29 deletions(-)

Comments

Doug Anderson June 20, 2014, 9:18 p.m. UTC | #1
Mark or Kukjin,

On Thu, Jun 12, 2014 at 8:59 PM, Naveen Krishna Chatradhi
<ch.naveen@samsung.com> wrote:
> Since, (3146bee spi: s3c64xx: Added provision for dedicated cs pin)
>
> spi-s3c64xx.c driver expects
> 1. chip select gpios from "cs-gpio"(singular) under the
>    "controller-data" node of the client/slave device of the SPI.
>
> 2. "cs-gpio"(singular) entry to be present in the SPI device node.
>
> Eg of current broken usage:
> &spi_1 {
>         cs-gpio <>;     /* this entry is checked during probe */
>         ...
>         slave_node {
>                 controller-data {
>                         cs-gpio <&gpioa2 5 0>;
>                         /* This field is parsed during .setup() */
>                 }
>         };
> };
>
> The following dts files which were using this driver. But,
> din't have the "cs-gpio" entry under SPI node.
> -- arch/arm/boot/dts/exynos4210-smdkv310.dts
> -- arch/arm/boot/dts/exynos4412-trats2.dts
> -- arch/arm/boot/dts/exynos5250-smdk5250.dts
>
> Also, the SPI core and many drivers moved on to using "cs-gpios"
> from SPI node and removed the gpio handling code from drivers
> (including spi-s3c64xx.c).
>
> Hence, spi-s3c64xx.c is broken since "Jun 21 11:26:12 2013" and
> considering the time with no compliants about the breakage.
>
> We are assuming it is safe to remove the "cs-gpio"(singular) usage
> from device tree binding of spi-samsung.txt and makes appropriate
> changes in the driver to use "cs-gpios"(plural) from
> SPI device node.
>
> Signed-off-by: Naveen Krishna Chatradhi <ch.naveen@samsung.com>
> Acked-by: Rob Herring <robh@kernel.org>
> Reviewed-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> Tested-by: Doug Anderson <dianders@chromium.org>
> Cc: Tomasz Figa <t.figa@samsung.com>
> ---
> Changes since 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                          |   41 ++++++++------------
>  2 files changed, 20 insertions(+), 29 deletions(-)

Is one of you two planning to apply (parts 1 and 2)?  I know that
Kukjin needs to handle the part 3 (the .dts files)...

Ideally it seems like it could go to 3.16 since it is a bugfix...

Thanks!

-Doug
Mark Brown June 21, 2014, 9:51 a.m. UTC | #2
On Fri, Jun 20, 2014 at 02:18:08PM -0700, Doug Anderson wrote:

> Is one of you two planning to apply (parts 1 and 2)?  I know that
> Kukjin needs to handle the part 3 (the .dts files)...

> Ideally it seems like it could go to 3.16 since it is a bugfix...

Don't nag, it just means yet more mail to look at which makes things
worse not better.  Given all the problems the DT code for this driver
has I need to review carefully, and the fact that this has got to v5
before I had a chance to look at it sets off alarm bells too.

There's a couple of months to get bug fixes in yet.
Naveen Krishna Ch June 21, 2014, 5:23 p.m. UTC | #3
Hello Mark,

On 21 June 2014 15:21, Mark Brown <broonie@kernel.org> wrote:
> On Fri, Jun 20, 2014 at 02:18:08PM -0700, Doug Anderson wrote:
>
>> Is one of you two planning to apply (parts 1 and 2)?  I know that
>> Kukjin needs to handle the part 3 (the .dts files)...
>
>> Ideally it seems like it could go to 3.16 since it is a bugfix...
>
> Don't nag, it just means yet more mail to look at which makes things
> worse not better.  Given all the problems the DT code for this driver
> has I need to review carefully, and the fact that this has got to v5
> before I had a chance to look at it sets off alarm bells too.

The revisions were pretty quick
v1 to v2 : Updated DT Documentation for the change
v2 to v3 : use spi->cs_gpio in the driver
v3 to v4 : use gpio_is_valid() instead of using sdd->cs_gpio (local variable,
              also remove sdd->cs_gpio)
v4 to v5 : Added Acked-by and Revewied-by from Javier and Rob Herring

Major discussion in this thread was about keeping the old DT bindings
to support legacy or move on with the new bindings.

Tomasz Figa, Javier Martinez Canillas, Doug Anderson and
Rob Herring agreed with the latter.

Kindly review and let us know your comments.

Thanks
>
> There's a couple of months to get bug fixes in yet.
Mark Brown June 22, 2014, 10:35 a.m. UTC | #4
On Sat, Jun 21, 2014 at 10:53:14PM +0530, Naveen Krishna Ch wrote:

> The revisions were pretty quick

Yes, this is part of what I'm concerned about - it often means that
there are problems, either the review wasn't very detailed or the code
is being written too hastily.  In this case this is coupled with the
fact that it's an area of the code that has had problems in general is
setting off alarm bells.

> v4 to v5 : Added Acked-by and Revewied-by from Javier and Rob Herring

Don't resend for tags - you should include tags if you do resend but
they're not a good reason to do so.

> Kindly review and let us know your comments.

I will review it when I get to it, this isn't the only thing I've got
sitting for review.
Kim Kukjin June 25, 2014, 11:29 a.m. UTC | #5
Doug Anderson wrote:
> 
> Mark or Kukjin,
> 
Hi,

> On Thu, Jun 12, 2014 at 8:59 PM, Naveen Krishna Chatradhi
> <ch.naveen@samsung.com> wrote:
> > Since, (3146bee spi: s3c64xx: Added provision for dedicated cs pin)
> >
> > spi-s3c64xx.c driver expects
> > 1. chip select gpios from "cs-gpio"(singular) under the
> >    "controller-data" node of the client/slave device of the SPI.
> >
> > 2. "cs-gpio"(singular) entry to be present in the SPI device node.
> >
> > Eg of current broken usage:
> > &spi_1 {
> >         cs-gpio <>;     /* this entry is checked during probe */
> >         ...
> >         slave_node {
> >                 controller-data {
> >                         cs-gpio <&gpioa2 5 0>;
> >                         /* This field is parsed during .setup() */
> >                 }
> >         };
> > };
> >
> > The following dts files which were using this driver. But,
> > din't have the "cs-gpio" entry under SPI node.
> > -- arch/arm/boot/dts/exynos4210-smdkv310.dts
> > -- arch/arm/boot/dts/exynos4412-trats2.dts
> > -- arch/arm/boot/dts/exynos5250-smdk5250.dts
> >
> > Also, the SPI core and many drivers moved on to using "cs-gpios"
> > from SPI node and removed the gpio handling code from drivers
> > (including spi-s3c64xx.c).
> >
> > Hence, spi-s3c64xx.c is broken since "Jun 21 11:26:12 2013" and
> > considering the time with no compliants about the breakage.
> >
> > We are assuming it is safe to remove the "cs-gpio"(singular) usage
> > from device tree binding of spi-samsung.txt and makes appropriate
> > changes in the driver to use "cs-gpios"(plural) from
> > SPI device node.
> >
> > Signed-off-by: Naveen Krishna Chatradhi <ch.naveen@samsung.com>
> > Acked-by: Rob Herring <robh@kernel.org>
> > Reviewed-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> > Tested-by: Doug Anderson <dianders@chromium.org>
> > Cc: Tomasz Figa <t.figa@samsung.com>
> > ---
> > Changes since 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                          |   41 ++++++++------------
> >  2 files changed, 20 insertions(+), 29 deletions(-)
> 
> Is one of you two planning to apply (parts 1 and 2)?  I know that
> Kukjin needs to handle the part 3 (the .dts files)...
> 
Once Mark takes spi driver related patches, I will pick DT related patches into
samsung tree.

> Ideally it seems like it could go to 3.16 since it is a bugfix...
> 
I think so but it depends on Mark's comments ;-)

Thanks,
Kukjin
Mark Brown July 2, 2014, 4:56 p.m. UTC | #6
On Fri, Jun 13, 2014 at 09:29:50AM +0530, Naveen Krishna Chatradhi wrote:

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

I'm not clear what the breakage is?  Some boards are broken but what's
the driver issue?

>  	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;
> +		} else {
> +			dev_err(&spi->dev, "chip select gpio is invalid\n");
> +			return -EINVAL;
>  		}

This appears to be making it mandatory to specify a GPIO when previously
it was not mandatory which I believe breaks some SoCs - the native chip
select has to be used on some devices as there is no pinmux option to
make it a GPIO (this was why the native chip select support was added).

Also I'd need to check but are you sure that GPIO 0 is not valid?
Naveen Krishna Ch July 7, 2014, 6:21 a.m. UTC | #7
Hello Mark,

Thanks for the review.

On 2 July 2014 22:26, Mark Brown <broonie@kernel.org> wrote:
> On Fri, Jun 13, 2014 at 09:29:50AM +0530, Naveen Krishna Chatradhi wrote:
>
>> Hence, spi-s3c64xx.c is broken since "Jun 21 11:26:12 2013" and
>> considering the time with no compliants about the breakage.
>
> I'm not clear what the breakage is?  Some boards are broken but what's
> the driver issue?

ToT was broken for few boards
exynos4412-trats2.dts, exynos4210-smdkv310.dts and exynos5250-smdk5250.dts

With some DTS changes SPI works well, spi-s3c64xx.c driver had no issues.

>
>>       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;
>> +             } else {
>> +                     dev_err(&spi->dev, "chip select gpio is invalid\n");
>> +                     return -EINVAL;
>>               }
>
> This appears to be making it mandatory to specify a GPIO when previously
> it was not mandatory which I believe breaks some SoCs - the native chip
> select has to be used on some devices as there is no pinmux option to
> make it a GPIO (this was why the native chip select support was added).

I read the commit "3146beec21b64f4551fcf0ac148381d54dc41b1b"
"spi: s3c64xx: Added provision for dedicated cs pin"

I understand "cs_gpio" shouldn't be mandatory. Will remove the else part.

>
> Also I'd need to check but are you sure that GPIO 0 is not valid?

gpio_is_valid() returns true for
"number >= 0 && number < ARCH_NR_GPIOS"
Mark Brown July 7, 2014, 7:32 a.m. UTC | #8
On Mon, Jul 07, 2014 at 11:51:38AM +0530, Naveen Krishna Ch wrote:
> On 2 July 2014 22:26, Mark Brown <broonie@kernel.org> wrote:
> > On Fri, Jun 13, 2014 at 09:29:50AM +0530, Naveen Krishna Chatradhi wrote:

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

> > I'm not clear what the breakage is?  Some boards are broken but what's
> > the driver issue?

> ToT was broken for few boards
> exynos4412-trats2.dts, exynos4210-smdkv310.dts and exynos5250-smdk5250.dts

> With some DTS changes SPI works well, spi-s3c64xx.c driver had no issues.

No, you're not answering my question - to repeat, what is the breakage?

> > Also I'd need to check but are you sure that GPIO 0 is not valid?

> gpio_is_valid() returns true for
> "number >= 0 && number < ARCH_NR_GPIOS"

Right, so this means that any board that is using the internal chip
select with zero as default in their platform data is broken by this
change.
Naveen Krishna Ch July 7, 2014, 8:31 a.m. UTC | #9
Hello Mark,

On 7 July 2014 13:02, Mark Brown <broonie@kernel.org> wrote:
> On Mon, Jul 07, 2014 at 11:51:38AM +0530, Naveen Krishna Ch wrote:
>> On 2 July 2014 22:26, Mark Brown <broonie@kernel.org> wrote:
>> > On Fri, Jun 13, 2014 at 09:29:50AM +0530, Naveen Krishna Chatradhi wrote:
>
>> >> Hence, spi-s3c64xx.c is broken since "Jun 21 11:26:12 2013" and
>> >> considering the time with no compliants about the breakage.
>
>> > I'm not clear what the breakage is?  Some boards are broken but what's
>> > the driver issue?
>
>> ToT was broken for few boards
>> exynos4412-trats2.dts, exynos4210-smdkv310.dts and exynos5250-smdk5250.dts
>
>> With some DTS changes SPI works well, spi-s3c64xx.c driver had no issues.
>
> No, you're not answering my question - to repeat, what is the breakage?

The Documentation/devicetree/bindings/spi/spi-samsung.txt
describes "cs-gpio" as a controller specific property.

The dts entries for SPI in exynos4412-trats2.dts, exynos4210-smdkv310.dts
and exynos5250-smdk5250.dts boards have the "cs-gpio" property defined
under controller-data node, which is inside the SPI device node
&spi_1 {
  controller-data {
    cs-gpio = <>;
  };
};

But, _probe() of spi-s3c64xx.c driver looks for "cs-gpio" in the SPI
device node and
sets a flag "sdd->cs_gpio = false" (If the property is not available)
&spi_1 {
  cs-gpio = <>;
};

the sdd->cs_gpio flag is checked before actually getting the gpios
from the controller-data node
       if (sdd->cs_gpio)
                cs->line = of_get_named_gpio(data_np, "cs-gpio", 0);

Hence, SPI was failing on those boards.

1. As the SPI core and several drivers were changed to work with
    DT property "cs-gpios" (plural) defined under SPI node.
2. Since the commit 3146beec21b64f4551fcf0ac148381d54dc41b1b
    "spi: s3c64xx: Added provision for dedicated cs pin"
    Dated:   Fri Jun 21 11:26:12 2013 +0530

For the above 2 reasons, It was decided to drop the backward compatibility
of using "cs-gpio"(singular) in controller-data.
Instead, start supporting "cs-gpios"(plural) in the SPI node.

>
>> > Also I'd need to check but are you sure that GPIO 0 is not valid?
>
>> gpio_is_valid() returns true for
>> "number >= 0 && number < ARCH_NR_GPIOS"
>
> Right, so this means that any board that is using the internal chip
> select with zero as default in their platform data is broken by this
> change.

using gpio_is_valid() to "sdd->cs_gpio" flag every where to check for the
validity was a review comment.
Which seems to fail for internal chip select with zero.

I can submit another version with"sdd->cs_gpio" flag for this purpose.
Javier Martinez Canillas July 7, 2014, 11:22 a.m. UTC | #10
Hello Naveen and Mark,

On Mon, Jul 7, 2014 at 10:31 AM, Naveen Krishna Ch
<naveenkrishna.ch@gmail.com> wrote:
> Hello Mark,
>
> On 7 July 2014 13:02, Mark Brown <broonie@kernel.org> wrote:
>> On Mon, Jul 07, 2014 at 11:51:38AM +0530, Naveen Krishna Ch wrote:
>>> On 2 July 2014 22:26, Mark Brown <broonie@kernel.org> wrote:
>>> > On Fri, Jun 13, 2014 at 09:29:50AM +0530, Naveen Krishna Chatradhi wrote:
>>
>>> >> Hence, spi-s3c64xx.c is broken since "Jun 21 11:26:12 2013" and
>>> >> considering the time with no compliants about the breakage.
>>
>>> > I'm not clear what the breakage is?  Some boards are broken but what's
>>> > the driver issue?
>>
>>> ToT was broken for few boards
>>> exynos4412-trats2.dts, exynos4210-smdkv310.dts and exynos5250-smdk5250.dts
>>
>>> With some DTS changes SPI works well, spi-s3c64xx.c driver had no issues.
>>

Correct me if I'm wrong but I think that the driver does have issues
since the commit mentioned (3146bee) broke DT backward compatibility.

>> No, you're not answering my question - to repeat, what is the breakage?
>

As far as I understand, the breakage is that any DTS that followed the
DT binding documented in
Documentation/devicetree/bindings/spi/spi-samsung.txt is not working
with the current driver. So is not that some boards are broken, is
that the driver is broken and it has been broken for more than a year
(the commit date is Jun 21 2013).

> The Documentation/devicetree/bindings/spi/spi-samsung.txt
> describes "cs-gpio" as a controller specific property.
>
> The dts entries for SPI in exynos4412-trats2.dts, exynos4210-smdkv310.dts
> and exynos5250-smdk5250.dts boards have the "cs-gpio" property defined
> under controller-data node, which is inside the SPI device node
> &spi_1 {
>   controller-data {
>     cs-gpio = <>;
>   };
> };
>
> But, _probe() of spi-s3c64xx.c driver looks for "cs-gpio" in the SPI
> device node and
> sets a flag "sdd->cs_gpio = false" (If the property is not available)
> &spi_1 {
>   cs-gpio = <>;
> };
>
> the sdd->cs_gpio flag is checked before actually getting the gpios
> from the controller-data node
>        if (sdd->cs_gpio)
>                 cs->line = of_get_named_gpio(data_np, "cs-gpio", 0);
>

I think that if changing the binding is not possible, at least we
should document this new "cs-gpio" property that is looked in the top
level SPI node after commit 3146bee and also revert the default in
order to allow DTs using the old binding to keep working.

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.

Of course in that case the property name does not make too much sense,
so probably should be changed to "cs-native" or something like that.
But I still don't understand why this is needed in the first place
since according to Documentation/devicetree/bindings/spi/spi-bus.txt
you can use the cs-gpios property to specify that a native chip-select
will be used instead of a GPIO by doing:

cs-gpios = <&gpio1 0 0> <0>

cs0 : &gpio1 0 0
cs1 : native

> Hence, SPI was failing on those boards.
>
> 1. As the SPI core and several drivers were changed to work with
>     DT property "cs-gpios" (plural) defined under SPI node.
> 2. Since the commit 3146beec21b64f4551fcf0ac148381d54dc41b1b
>     "spi: s3c64xx: Added provision for dedicated cs pin"
>     Dated:   Fri Jun 21 11:26:12 2013 +0530
>
> For the above 2 reasons, It was decided to drop the backward compatibility
> of using "cs-gpio"(singular) in controller-data.
> Instead, start supporting "cs-gpios"(plural) in the SPI node.
>

Right, since the DT binding has been broken for a year and because is
not consistent with the bindings used by all other SPI drivers, many
agreed that it was one of the exceptional cases where the DT binding
can be rethought and changed to use the generic "cs-gpios" property
already supported by SPI core. It breaks backward compatibility that's
true but the DT binding has been broken anyways and nobody noticed
before.

The other option is what I said above, fixing the DT binding
compatibility breakage while keeping the custom binding for this SPI
driver.

>>
>>> > Also I'd need to check but are you sure that GPIO 0 is not valid?
>>
>>> gpio_is_valid() returns true for
>>> "number >= 0 && number < ARCH_NR_GPIOS"
>>
>> Right, so this means that any board that is using the internal chip
>> select with zero as default in their platform data is broken by this
>> change.
>

I think this problem could be present in other SPI drivers as well? So
maybe the right fix for this is to convert the SPI core gpio handling
to use the new descriptor-based gpio API instead of the integer-base
one?

> using gpio_is_valid() to "sdd->cs_gpio" flag every where to check for the
> validity was a review comment.
> Which seems to fail for internal chip select with zero.
>
> I can submit another version with"sdd->cs_gpio" flag for this purpose.
>
> --
> Thanks & Regards,
> (: Nav :)
> --

Best regards,
Javier
Javier Martinez Canillas July 11, 2014, 11:04 a.m. UTC | #11
Hello Naveen and Mark,

On Mon, Jul 7, 2014 at 1:22 PM, Javier Martinez Canillas
<javier@dowhile0.org> wrote:
> On Mon, Jul 7, 2014 at 10:31 AM, Naveen Krishna Ch
> <naveenkrishna.ch@gmail.com> wrote:
>>>
>>>> >> Hence, spi-s3c64xx.c is broken since "Jun 21 11:26:12 2013" and
>>>> >> considering the time with no compliants about the breakage.
>>>
>>>> > I'm not clear what the breakage is?  Some boards are broken but what's
>>>> > the driver issue?
>>>
>>>> ToT was broken for few boards
>>>> exynos4412-trats2.dts, exynos4210-smdkv310.dts and exynos5250-smdk5250.dts
>>>
>>>> With some DTS changes SPI works well, spi-s3c64xx.c driver had no issues.
>>>
>
> Correct me if I'm wrong but I think that the driver does have issues
> since the commit mentioned (3146bee) broke DT backward compatibility.
>
>>> No, you're not answering my question - to repeat, what is the breakage?
>>
>
> As far as I understand, the breakage is that any DTS that followed the
> DT binding documented in
> Documentation/devicetree/bindings/spi/spi-samsung.txt is not working
> with the current driver. So is not that some boards are broken, is
> that the driver is broken and it has been broken for more than a year
> (the commit date is Jun 21 2013).
>
>> The Documentation/devicetree/bindings/spi/spi-samsung.txt
>> describes "cs-gpio" as a controller specific property.
>>
>> The dts entries for SPI in exynos4412-trats2.dts, exynos4210-smdkv310.dts
>> and exynos5250-smdk5250.dts boards have the "cs-gpio" property defined
>> under controller-data node, which is inside the SPI device node
>> &spi_1 {
>>   controller-data {
>>     cs-gpio = <>;
>>   };
>> };
>>
>> But, _probe() of spi-s3c64xx.c driver looks for "cs-gpio" in the SPI
>> device node and
>> sets a flag "sdd->cs_gpio = false" (If the property is not available)
>> &spi_1 {
>>   cs-gpio = <>;
>> };
>>
>> the sdd->cs_gpio flag is checked before actually getting the gpios
>> from the controller-data node
>>        if (sdd->cs_gpio)
>>                 cs->line = of_get_named_gpio(data_np, "cs-gpio", 0);
>>
>
> I think that if changing the binding is not possible, at least we
> should document this new "cs-gpio" property that is looked in the top
> level SPI node after commit 3146bee and also revert the default in
> order to allow DTs using the old binding to keep working.
>
> 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.
>
> Of course in that case the property name does not make too much sense,
> so probably should be changed to "cs-native" or something like that.
> But I still don't understand why this is needed in the first place
> since according to Documentation/devicetree/bindings/spi/spi-bus.txt
> you can use the cs-gpios property to specify that a native chip-select
> will be used instead of a GPIO by doing:
>
> cs-gpios = <&gpio1 0 0> <0>
>
> cs0 : &gpio1 0 0
> cs1 : native
>
>> Hence, SPI was failing on those boards.
>>
>> 1. As the SPI core and several drivers were changed to work with
>>     DT property "cs-gpios" (plural) defined under SPI node.
>> 2. Since the commit 3146beec21b64f4551fcf0ac148381d54dc41b1b
>>     "spi: s3c64xx: Added provision for dedicated cs pin"
>>     Dated:   Fri Jun 21 11:26:12 2013 +0530
>>
>> For the above 2 reasons, It was decided to drop the backward compatibility
>> of using "cs-gpio"(singular) in controller-data.
>> Instead, start supporting "cs-gpios"(plural) in the SPI node.
>>
>
> Right, since the DT binding has been broken for a year and because is
> not consistent with the bindings used by all other SPI drivers, many
> agreed that it was one of the exceptional cases where the DT binding
> can be rethought and changed to use the generic "cs-gpios" property
> already supported by SPI core. It breaks backward compatibility that's
> true but the DT binding has been broken anyways and nobody noticed
> before.
>
> The other option is what I said above, fixing the DT binding
> compatibility breakage while keeping the custom binding for this SPI
> driver.
>
>>>
>>>> > Also I'd need to check but are you sure that GPIO 0 is not valid?
>>>
>>>> gpio_is_valid() returns true for
>>>> "number >= 0 && number < ARCH_NR_GPIOS"
>>>
>>> Right, so this means that any board that is using the internal chip
>>> select with zero as default in their platform data is broken by this
>>> change.
>>
>
> I think this problem could be present in other SPI drivers as well? So
> maybe the right fix for this is to convert the SPI core gpio handling
> to use the new descriptor-based gpio API instead of the integer-base
> one?
>
>> using gpio_is_valid() to "sdd->cs_gpio" flag every where to check for the
>> validity was a review comment.
>> Which seems to fail for internal chip select with zero.
>>
>> I can submit another version with"sdd->cs_gpio" flag for this purpose.
>>
>> --
>> Thanks & Regards,
>> (: Nav :)
>> --

Any more opinions on this issue?

It would be great if we can move this forward since there are other
series that depend on these fixes.

Best regards,
Javier
Mark Brown July 11, 2014, 1:48 p.m. UTC | #12
On Fri, Jul 11, 2014 at 01:04:07PM +0200, Javier Martinez Canillas wrote:
> Hello Naveen and Mark,
> On Mon, Jul 7, 2014 at 1:22 PM, Javier Martinez Canillas
> <javier@dowhile0.org> wrote:
> > On Mon, Jul 7, 2014 at 10:31 AM, Naveen Krishna Ch
> > <naveenkrishna.ch@gmail.com> wrote:

Please delete irrelevant context from your replies, this mail is over a
hundred lines long and has only three lines of new content.

> Any more opinions on this issue?

> It would be great if we can move this forward since there are other
> series that depend on these fixes.

Well, the bug with making a GPIO chip select mandatory needs to be
fixed.

Patch
diff mbox

diff --git a/Documentation/devicetree/bindings/spi/spi-samsung.txt b/Documentation/devicetree/bindings/spi/spi-samsung.txt
index 86aa061..2d29dac 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 0>;
 
 		w25q80bw@0 {
 			#address-cells = <1>;
@@ -94,7 +93,6 @@  Example:
 			spi-max-frequency = <10000>;
 
 			controller-data {
-				cs-gpio = <&gpa2 5 1 0 3>;
 				samsung,spi-feedback-delay = <0>;
 			};
 
diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 75a5696..b888c66 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -197,7 +197,6 @@  struct s3c64xx_spi_driver_data {
 	struct s3c64xx_spi_dma_data	tx_dma;
 	struct s3c64xx_spi_port_config	*port_conf;
 	unsigned int			port_id;
-	bool				cs_gpio;
 };
 
 static void flush_fifo(struct s3c64xx_spi_driver_data *sdd)
@@ -776,17 +775,6 @@  static struct s3c64xx_spi_csinfo *s3c64xx_get_slave_ctrldata(
 		return ERR_PTR(-ENOMEM);
 	}
 
-	/* The CS line is asserted/deasserted by the gpio pin */
-	if (sdd->cs_gpio)
-		cs->line = of_get_named_gpio(data_np, "cs-gpio", 0);
-
-	if (!gpio_is_valid(cs->line)) {
-		dev_err(&spi->dev, "chip select gpio is not specified or invalid\n");
-		kfree(cs);
-		of_node_put(data_np);
-		return ERR_PTR(-EINVAL);
-	}
-
 	of_property_read_u32(data_np, "samsung,spi-feedback-delay", &fb_delay);
 	cs->fb_delay = fb_delay;
 	of_node_put(data_np);
@@ -812,6 +800,10 @@  static int s3c64xx_spi_setup(struct spi_device *spi)
 		spi->controller_data = cs;
 	}
 
+	/* For the non-DT platforms derive chip selects from controller data */
+	if (!spi->dev.of_node)
+		spi->cs_gpio = cs->line;
+
 	if (IS_ERR_OR_NULL(cs)) {
 		dev_err(&spi->dev, "No CS for SPI(%d)\n", spi->chip_select);
 		return -ENODEV;
@@ -819,17 +811,19 @@  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;
+		} else {
+			dev_err(&spi->dev, "chip select gpio is invalid\n");
+			return -EINVAL;
 		}
 
 		spi_set_ctldata(spi, cs);
@@ -884,7 +878,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 +895,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 +1072,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",