Re: [PATCH 1/2 v3] spi: s3c64xx: use "cs-gpios" from spi node instead of "cs-gpio"
On 11.06.2014 20:23, Naveen Krishna Ch wrote: > Hello Tomasz, > > On 11 June 2014 23:20, Tomasz Figa wrote: >> On 11.06.2014 19:27, Javier Martinez Canillas wrote: >>> On 06/11/2014 01:38 PM, Naveen Krishna Ch wrote: On 11 June 2014 16:43, Javier Martinez Canillas wrote: > On 06/11/2014 08:31 AM, Naveen Krishna Chatradhi wrote: >> >> [snip] >> > >> 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. >>> >>> 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. > 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. This should be handled by SPI core as well, but apparently it isn't. So the scheme of work in s3c64xx_spi_setup() changes as in pseudocode below: if (!DT) spi->cs_gpio = cs->line; if (gpio_is_valid(spi->cs_gpio)) gpio_request_one(spi->cs_gpio); and in s3c64xx_spi_cleanup(): if (gpio_is_valid(spi->cs_gpio)) gpio_free(spi->cs_gpio); if (!DT) spi->cs_gpio = -ENOENT; >> >> - 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. Hmm? Best regards, Tomasz -- HPCC Systems Open Source Big Data Platform from LexisNexis Risk Solutions Find What Matters Most in Your Big Data with HPCC Systems Open Source. Fast. Scalable. Simple. Ideal for Dirty Data. Leverages Graph Analysis for Fast Processing & Easy Data Exploration http://p.sf.net/sfu/hpccsystems ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH 1/2 v3] spi: s3c64xx: use "cs-gpios" from spi node instead of "cs-gpio"
Hello Tomasz, On 11 June 2014 23:20, Tomasz Figa wrote: > On 11.06.2014 19:27, Javier Martinez Canillas wrote: >> On 06/11/2014 01:38 PM, Naveen Krishna Ch wrote: >>> On 11 June 2014 16:43, Javier Martinez Canillas >>> wrote: On 06/11/2014 08:31 AM, Naveen Krishna Chatradhi wrote: > > [snip] > > 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. >> >> 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. 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. > > - 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. > > Best regards, > Tomasz -- Shine bright, (: Nav :) -- HPCC Systems Open Source Big Data Platform from LexisNexis Risk Solutions Find What Matters Most in Your Big Data with HPCC Systems Open Source. Fast. Scalable. Simple. Ideal for Dirty Data. Leverages Graph Analysis for Fast Processing & Easy Data Exploration http://p.sf.net/sfu/hpccsystems ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH 1/2 v3] spi: s3c64xx: use "cs-gpios" from spi node instead of "cs-gpio"
Hello Tomasz, On 06/11/2014 07:50 PM, Tomasz Figa wrote: > On 11.06.2014 19:27, Javier Martinez Canillas wrote: >> On 06/11/2014 01:38 PM, Naveen Krishna Ch wrote: >>> On 11 June 2014 16:43, Javier Martinez Canillas >>> wrote: On 06/11/2014 08:31 AM, Naveen Krishna Chatradhi wrote: > > [snip] > > 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. >> >> 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(). > Right, I was actually looking at s3c64xx_spi_setup() when I wrote that but for some reason I got confused and thought it was the probe() function. Sorry for the confusion. > 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 > Best regards, Javier -- HPCC Systems Open Source Big Data Platform from LexisNexis Risk Solutions Find What Matters Most in Your Big Data with HPCC Systems Open Source. Fast. Scalable. Simple. Ideal for Dirty Data. Leverages Graph Analysis for Fast Processing & Easy Data Exploration http://p.sf.net/sfu/hpccsystems ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH 1/2 v3] spi: s3c64xx: use "cs-gpios" from spi node instead of "cs-gpio"
On 11.06.2014 19:27, Javier Martinez Canillas wrote: > On 06/11/2014 01:38 PM, Naveen Krishna Ch wrote: >> On 11 June 2014 16:43, Javier Martinez Canillas >> wrote: >>> On 06/11/2014 08:31 AM, Naveen Krishna Chatradhi wrote: [snip] >>> 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. > > 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 -- HPCC Systems Open Source Big Data Platform from LexisNexis Risk Solutions Find What Matters Most in Your Big Data with HPCC Systems Open Source. Fast. Scalable. Simple. Ideal for Dirty Data. Leverages Graph Analysis for Fast Processing & Easy Data Exploration http://p.sf.net/sfu/hpccsystems ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH 1/2 v3] spi: s3c64xx: use "cs-gpios" from spi node instead of "cs-gpio"
Hello Naveen,
On 06/11/2014 01:38 PM, Naveen Krishna Ch wrote:
> Hello Javier,
>
> On 11 June 2014 16:43, Javier Martinez Canillas
> wrote:
>> 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.
>>
>> On 06/11/2014 08:31 AM, 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.
>>>
>>
>> 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.
>>
>>> &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.
>>
>>> 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.
>>>
>>
>> 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
>>
>>> 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).
> Will take this point for the while rewriting the commit messages
Awesome, thanks.
>>
>>> Signed-off-by: Naveen Krishna Chatradhi
>>> Acked-by: Rob Herring
>>> Cc: Javier Martinez Canillas
>>> Cc: Doug Anderson
>>> Cc: Tomasz Figa
>>> ---
>>> 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(-)
>>>
>>> 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:
>>>
Re: [PATCH 1/2 v3] spi: s3c64xx: use "cs-gpios" from spi node instead of "cs-gpio"
Hello Javier,
On 11 June 2014 16:43, Javier Martinez Canillas
wrote:
> 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.
>
> On 06/11/2014 08:31 AM, 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.
>>
>
> 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.
>
>> &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.
>
>> 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.
>>
>
> 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
>
>> 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).
Will take this point for the while rewriting the commit messages
>
>> Signed-off-by: Naveen Krishna Chatradhi
>> Acked-by: Rob Herring
>> Cc: Javier Martinez Canillas
>> Cc: Doug Anderson
>> Cc: Tomasz Figa
>> ---
>> 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(-)
>>
>> 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>;
>
Re: [PATCH 1/2 v3] spi: s3c64xx: use "cs-gpios" from spi node instead of "cs-gpio"
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.
On 06/11/2014 08:31 AM, 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.
>
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.
> &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.
> 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.
>
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.
> 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).
> Signed-off-by: Naveen Krishna Chatradhi
> Acked-by: Rob Herring
> Cc: Javier Martinez Canillas
> Cc: Doug Anderson
> Cc: Tomasz Figa
> ---
> 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(-)
>
> 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 = <1>;
>
> 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..842b148 100644
> --- a/drivers/spi/spi-s3c64xx.c
>
[PATCH 1/2 v3] spi: s3c64xx: use "cs-gpios" from 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 updates the Device tree Documentation.
Signed-off-by: Naveen Krishna Chatradhi
Acked-by: Rob Herring
Cc: Javier Martinez Canillas
Cc: Doug Anderson
Cc: Tomasz Figa
---
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(-)
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 = <1>;
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..842b148 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -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);
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);
return ERR_PTR(-EINVAL);
}
+ cs->line = spi->cs_gpio;
of_property_read_u32(data_np, "samsung,spi-feedback-delay", &fb_delay);
cs->fb_delay = fb_delay;
of_node_put(data_np);
+
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);
- gpio_free(cs->line);
+ if (cs->line)
+ gpio_free(cs->line);
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;
ret = of_alias_get_id(pdev->dev.of_node, "spi");
--
1.7.9.5
---
