Currently, spi-s3c64xx.c needs "cs-gpio" chip select GPIO to be
defined under "controller-data" node under each slave node.
&spi_x {
cs-gpios <>;
...
slave_node {
controller-data {
cs-gpio = <>;
...
};
...
};
...
};
Where as, SPI core and many other drivers uses "cs-gpios" for
from device tree node.
Hence, make changes in spi-s3c64xx.c driver to make use of
"cs-gpios" from SPI node(parent) instead of "cs-gpio" defined in
slaves "controller-data"(child) node.
Also updates the Device tree Documentation.
Signed-off-by: Naveen Krishna Chatradhi <redacted>
Acked-by: Rob Herring <robh@kernel.org>
Cc: Javier Martinez Canillas <redacted>
Cc: Doug Anderson <dianders@chromium.org>
Cc: Tomasz Figa <redacted>
---
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(-)
@@ -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.
@@ -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);returnERR_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);returnERR_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);+returncs;}
@@ -828,8 +823,6 @@ static int s3c64xx_spi_setup(struct spi_device *spi)cs->line,err);gotoerr_gpio_req;}--spi->cs_gpio=cs->line;}spi_set_ctldata(spi,cs);
This patch moves the "cs-gpio" field from "controller-data" child
node to under the spi device node.
Respective changes are preposed to spi-s3c64xx.c driver.
Signed-off-by: Naveen Krishna Chatradhi <redacted>
Acked-by: Rob Herring <robh@kernel.org>
Cc: Javier Martinez Canillas <redacted>
Cc: Doug Anderson <dianders@chromium.org>
Cc: Tomasz Figa <redacted>
---
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(-)
From: Javier Martinez Canillas <hidden> Date: 2014-06-11 11:13:26
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).
quoted hunk
Signed-off-by: Naveen Krishna Chatradhi <redacted>
Acked-by: Rob Herring <robh@kernel.org>
Cc: Javier Martinez Canillas <redacted>
Cc: Doug Anderson <dianders@chromium.org>
Cc: Tomasz Figa <redacted>
---
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(-)
@@ -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.
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.
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.
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.
@@ -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);
Same here, we can just get rid of cs->line and use gpio_free(spi->cs_gpio) instead.
quoted hunk
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.
From: Javier Martinez Canillas <hidden> Date: 2014-06-11 11:21:35
Hello Naveen,
On 06/11/2014 08:31 AM, Naveen Krishna Chatradhi wrote:
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 hunk
Respective changes are preposed to spi-s3c64xx.c driver.
Signed-off-by: Naveen Krishna Chatradhi <redacted>
Acked-by: Rob Herring <robh@kernel.org>
Cc: Javier Martinez Canillas <redacted>
Cc: Doug Anderson <dianders@chromium.org>
Cc: Tomasz Figa <redacted>
---
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(-)
Hello Javier,
On 11 June 2014 16:51, Javier Martinez Canillas
[off-list ref] wrote:
Hello Naveen,
On 06/11/2014 08:31 AM, Naveen Krishna Chatradhi wrote:
quoted
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.
Sure, Will improve the commit message with your other points aswell.
quoted
Respective changes are preposed to spi-s3c64xx.c driver.
Signed-off-by: Naveen Krishna Chatradhi <redacted>
Acked-by: Rob Herring <robh@kernel.org>
Cc: Javier Martinez Canillas <redacted>
Cc: Doug Anderson <dianders@chromium.org>
Cc: Tomasz Figa <redacted>
---
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(-)
Hello Javier,
On 11 June 2014 16:43, Javier Martinez Canillas
[off-list ref] 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:
quoted
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
&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
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
quoted
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
quoted
Signed-off-by: Naveen Krishna Chatradhi <redacted>
Acked-by: Rob Herring <robh@kernel.org>
Cc: Javier Martinez Canillas <redacted>
Cc: Doug Anderson <dianders@chromium.org>
Cc: Tomasz Figa <redacted>
---
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(-)
@@ -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.
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
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.
Actually, i wanted to move down the of_get_child_by_name() to before
of_property_read_u32() call.
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.
@@ -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);
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
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
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.
--
Shine bright,
(: Nav :)
From: Javier Martinez Canillas <hidden> Date: 2014-06-11 17:27:43
Hello Naveen,
On 06/11/2014 01:38 PM, Naveen Krishna Ch wrote:
Hello Javier,
On 11 June 2014 16:43, Javier Martinez Canillas
[off-list ref] wrote:
quoted
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:
quoted
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
&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
quoted
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
quoted
quoted
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.
quoted
quoted
Signed-off-by: Naveen Krishna Chatradhi <redacted>
Acked-by: Rob Herring <robh@kernel.org>
Cc: Javier Martinez Canillas <redacted>
Cc: Doug Anderson <dianders@chromium.org>
Cc: Tomasz Figa <redacted>
---
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(-)
@@ -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.
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
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.
Actually, i wanted to move down the of_get_child_by_name() to before
of_property_read_u32() call.
Yeah, the wrong removals seemed to me like a left over from a previous attempt
to reorganize the code.
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;
So, cs->line is only used by non-DT platforms to pass the GPIO to be used as
chip-select line. But after that use spi->cs_gpio should be used in every place.
If someday all the non-DT platforms are migrated to DT then only that assignment
has to be removed instead of several lines of code using cs->line currently.
Also is more consistent to use spi->cs_gpio like most SPI drivers do instead of
a custom structure member.
@@ -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);
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.
I don't get why you don't want to change legacy platform code if that means
improving the driver. If you don't feel confident with your changes because you
don't have access to the non-DT platform to test then you can post your patches
as RFC or ask people to with access to those platforms to test and collect some
Tested-by tags before the patch is merged.
quoted
quoted
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
sdd->cs_gpio has nothing to do with non-DT platforms AFAIU. It was introduced in
the commit that initially broke this binding and it's redundant information.
It's assigned to true on s3c64xx_spi_probe() and then set to false only if
of_find_property(pdev->dev.of_node, "cs-gpios", NULL) fails.
But you don't need an additional variable to get that info since
gpio_is_valid(spi->cs_gpio) or if (master->cs_gpios) can tell you the same AFAIK.
quoted
quoted
ret = of_alias_get_id(pdev->dev.of_node, "spi");
Best regards,
Javier
Will re-spin after waiting for a couple of more comments.
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
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
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()).
From: Mark Brown <broonie@kernel.org> Date: 2014-06-11 18:27:13
On Wed, Jun 11, 2014 at 11:53:30PM +0530, Naveen Krishna Ch wrote:
On 11 June 2014 23:20, Tomasz Figa [off-list ref] wrote:
quoted
- 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.
Ideally this code could be moved into the core too - indeed, ideally the
non-DT case could be handled by the core too except for the bit where we
look up what GPIO to use.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140611/22e2901e/attachment.sig>
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;
quoted
- 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()).