Re: [PATCH 1/3 v6] spi: s3c64xx: fix broken cs_gpios usage in the driver
Hello Mark,
I agree that the commit message could have a better description and I
understand your concerns. I'm not an SPI expert by any means but I did
my best to review the patches and provide feedback to Naveen on the
first iterations of the series.
On Mon, Jul 14, 2014 at 7:25 PM, Mark Brown broo...@kernel.org wrote:
On Mon, Jul 14, 2014 at 11:11:44AM +0530, Naveen Krishna Chatradhi wrote:
@@ -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,16 @@ 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)) {
As previously mentioned gpio_is_valid() is *not* a direct substitute for
checking if the boolean flag cs_gpio has been set since 0 is a valid
GPIO on at least some of these platforms and as discussed several times
already some of the SoCs require the use of the built in chip select.
Please correct me if I'm wrong but in this case I think that using
gpio_is_valid(spi-cs_gpio) is the right thing to do. The SPI core
will set spi-cs_gpio to -ENOENT if a Device Tree didn't use the
cs-gpios property of if the format (explained in
Documentation/devicetree/bindings/spi/spi-bus.txt) to specify a
native/built-in line chip select is used:
cs-gpios = gpio1 0 0 0
cs0 : gpio1 0 0
cs1 : native
So in that case gpio_is_valid(spi-cs_gpio) will return false which is
what Naveen wants in his patch.
Now, a problem is that in the non-DT case the patch does not rely on
the spi-cs_gpio value set by the SPI core but instead overwrites it
with the value in cs-line. So as you said this would have been an
issue if legacy non-DT board code used s3c64xx_spi_csinfo .line to
specify that the built-in chip select should be used instead of a
GPIO.
But looking at the board files that sets struct spi_board_info
.controller_data to struct s3c64xx_spi_csinfo I see that the the .line
field is actually used to specify a GPIO pin and not a chip select.
In fact there is only one user in mainline
(arch/arm/mach-s3c64xx/mach-crag6410-module.c) that do this:
#define S3C64XX_GPC(_nr) (S3C64XX_GPIO_C_START + (_nr))
static struct s3c64xx_spi_csinfo wm0010_spi_csinfo = {
.line = S3C64XX_GPC(3),
};
static struct spi_board_info wm1253_devs[] = {
[0] = {
.modalias = wm0010,
.max_speed_hz = 26 * 1000 * 1000,
.bus_num= 0,
.chip_select= 0,
.mode = SPI_MODE_0,
.irq= S3C_EINT(4),
.controller_data = wm0010_spi_csinfo,
.platform_data = wm0010_pdata,
},
};
So, the .line field is used to specify a GPIO, not a chip select. So
gpio_is_valid() should also be used to check that cs-line is a valid
GPIO. If board file does not want to use a GPIO and wants to use a
built-in chip select then it should either not use a struct
s3c64xx_spi_csinfo at all or set .line to -ENOENT in order to be
consistent with the SPI core.
Now, looking at this I realize that there is a bug in the spi-s3c64xx
driver since it does not check if spi-controller_data is NULL in the
non-DT case which I believe it can be true.
In general it's quite hard to tie the description in the patch to the
code changes, not helped by the decision to do separate refactorings
like this conversion to gpio_is_valid() as part of the one patch. The
description of the patch now makes some statements about what the
problem that's intended to be fixed is but it still doesn't seem
entirely clear that everything has been thought through fully and tied
to the code.
AFAICT most of the refactoring is actually removing the buggy code
that was introduced in commit 3146bee (spi: s3c64xx: Added provision
for dedicated cs pin). So maybe Naveen can try doing a new series
first reverting the offending commit and then on top of that adding
the support for the generic cs-gpios DT property used by the SPI
core?
As I said before this patch is fixing a bug in the SPI driver, the
first version of the series was posted on June, 10 so many other
patches already depend on this fix. So it would be great if we can
move this forward since this is hurting the platform support as a
whole.
Thanks a lot and best regards,
Javier
--
Want fast and easy access
Re: [PATCH 1/3 v6] spi: s3c64xx: fix broken cs_gpios usage in the driver
Hi Naveen, On 14.07.2014 07:41, Naveen Krishna Chatradhi 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. Please work a bit more on readability of your commit messages, especially in terms of using correct terminology to avoid unnecessary confusion. s/'chip select gpios'/'chip select GPIO pins'/ s/'cs-gpio(singular)'/'cs-gpio property'/ s/'client/slave device of the SPI'/'SPI device'/ More things like this follow in rest of the text below. Otherwise I believe the code after this patch does the right thing, so: Reviewed-by: Tomasz Figa t.f...@samsung.com Best regards, Tomasz -- Want fast and easy access to all the code in your enterprise? Index and search up to 200,000 lines of code with a free copy of Black Duck Code Sight - the same software that powers the world's largest code search on Ohloh, the Black Duck Open Hub! Try it now. http://p.sf.net/sfu/bds ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH 1/3 v6] spi: s3c64xx: fix broken cs_gpios usage in the driver
Hello Tomasz, On 15 July 2014 22:25, Tomasz Figa t.f...@samsung.com wrote: Hi Naveen, On 14.07.2014 07:41, Naveen Krishna Chatradhi 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. Please work a bit more on readability of your commit messages, especially in terms of using correct terminology to avoid unnecessary confusion. s/'chip select gpios'/'chip select GPIO pins'/ s/'cs-gpio(singular)'/'cs-gpio property'/ s/'client/slave device of the SPI'/'SPI device'/ Sure, I'm working on that. More things like this follow in rest of the text below. Otherwise I believe the code after this patch does the right thing, so: Reviewed-by: Tomasz Figa t.f...@samsung.com I've submitted another patch set which does the same functionality as this patch set. I believe the new series is more cleaner in terms of patch separation and documentation. Kindly, let me know your comments on that series. http://www.spinics.net/lists/arm-kernel/msg347634.html Best regards, Tomasz -- Thanks Regards, (: Naveen :) -- Want fast and easy access to all the code in your enterprise? Index and search up to 200,000 lines of code with a free copy of Black Duck Code Sight - the same software that powers the world's largest code search on Ohloh, the Black Duck Open Hub! Try it now. http://p.sf.net/sfu/bds ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH 1/3 v6] spi: s3c64xx: fix broken cs_gpios usage in the driver
On 15.07.2014 19:21, Naveen Krishna Ch wrote: Hello Tomasz, On 15 July 2014 22:25, Tomasz Figa t.f...@samsung.com wrote: Hi Naveen, On 14.07.2014 07:41, Naveen Krishna Chatradhi 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. Please work a bit more on readability of your commit messages, especially in terms of using correct terminology to avoid unnecessary confusion. s/'chip select gpios'/'chip select GPIO pins'/ s/'cs-gpio(singular)'/'cs-gpio property'/ s/'client/slave device of the SPI'/'SPI device'/ Sure, I'm working on that. More things like this follow in rest of the text below. Otherwise I believe the code after this patch does the right thing, so: Reviewed-by: Tomasz Figa t.f...@samsung.com I've submitted another patch set which does the same functionality as this patch set. I believe the new series is more cleaner in terms of patch separation and documentation. Kindly, let me know your comments on that series. http://www.spinics.net/lists/arm-kernel/msg347634.html Right. I noticed it just now and am looking at it. Best regards, Tomasz -- Want fast and easy access to all the code in your enterprise? Index and search up to 200,000 lines of code with a free copy of Black Duck Code Sight - the same software that powers the world's largest code search on Ohloh, the Black Duck Open Hub! Try it now. http://p.sf.net/sfu/bds ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH 1/3 v6] spi: s3c64xx: fix broken cs_gpios usage in the driver
Hello Mark,
On 14 July 2014 22:55, Mark Brown broo...@kernel.org wrote:
On Mon, Jul 14, 2014 at 11:11:44AM +0530, Naveen Krishna Chatradhi wrote:
@@ -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,16 @@ 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)) {
As previously mentioned gpio_is_valid() is *not* a direct substitute for
checking if the boolean flag cs_gpio has been set since 0 is a valid
GPIO on at least some of these platforms and as discussed several times
already some of the SoCs require the use of the built in chip select.
Yes, gpio_is_valid() is not a direct substitute for boolean cs_gpio.
It was a review comment to use gpio_is_valid() and quickly iterated
another version with that change. Now that we see gpio_is_valid() is
breaking the native chip select for gpio number 0. Shall i go back to
using the boolean or do you have a better way to do this.
In general it's quite hard to tie the description in the patch to the
code changes, not helped by the decision to do separate refactorings
like this conversion to gpio_is_valid() as part of the one patch. The
description of the patch now makes some statements about what the
problem that's intended to be fixed is but it still doesn't seem
entirely clear that everything has been thought through fully and tied
to the code.
The original code appears to be buggy which isn't helping anything but
it's hard to have confidence that this isn't going to break some other
use case that currently works given the lack of clarity and the number
of revisions that have been required so far.
spi-s3c64xx.c is supporting a wide range of SoCs ranging from s3c64xx
series followed by s5p series and then exynos4 and exynos5 series.
However, Exynos4 and Exynos5 series currently available are all DT based.
Support for DT and non-DT was taken good care.
Revision number went to 5 for minor changes like
1. Missing documentation
2. Using gpio_is_valid() instead of boolean cs_gpio
3. Adding SignedOff-By and Acked-By
I think some combination of smaller changes and a clearer working
through of the before and after states for both DT and non DT cases to
show that everything has been considered would help a lot. I may have
another stare at this but it's worrying how hard I'm needing to think.
The intention of the patch was to fix the broken platforms by implementing
dt bindings similar to generic SPI core in spi-s3c64xx.c.
Current dt bindings:
spi-s3c64xx.c is expects the cs-gpio property in the SPI DT node
and also defined under the controller-data.
Solution 1:
Add support for cs-gpios property in the SPI DT node and
remove cs-gpio from sub node controller-data.
Solution 2:
As Javier suggested
//
Inverting the default of cs_gpio. and 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
//
in this case spi-s3c64xx.c will continue to ignore the generic SPI cs-gpios
implementation.
I'm willing to implement any suggestion to fix this issue.
--
Regards,
(: Naveen :)
--
Want fast and easy access to all the code in your enterprise? Index and
search up to 200,000 lines of code with a free copy of Black Duck#174;
Code Sight#153; - the same software that powers the world's largest code
search on Ohloh, the Black Duck Open Hub! Try it now.
http://p.sf.net/sfu/bds
___
spi-devel-general mailing list
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH 1/3 v6] spi: s3c64xx: fix broken cs_gpios usage in the driver
Hello Mark, On 15 July 2014 00:45, Mark Brown broo...@kernel.org wrote: On Tue, Jul 15, 2014 at 12:31:32AM +0530, Naveen Krishna Ch wrote: in this case spi-s3c64xx.c will continue to ignore the generic SPI cs-gpios implementation. I'm willing to implement any suggestion to fix this issue. The problem isn't what you're trying to do, the problem is verifying that it has been done correctly - making sure that everything has been accounted for in the change itself. Understand your concern. I will try to get it tested on as many platforms. -- Thanks for your review. (: Naveen :) -- Want fast and easy access to all the code in your enterprise? Index and search up to 200,000 lines of code with a free copy of Black Duck Code Sight - the same software that powers the world's largest code search on Ohloh, the Black Duck Open Hub! Try it now. http://p.sf.net/sfu/bds ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
[PATCH 1/3 v6] spi: s3c64xx: fix broken cs_gpios usage in the driver
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.nav...@samsung.com
Acked-by: Rob Herring r...@kernel.org
Reviewed-by: Javier Martinez Canillas javier.marti...@collabora.co.uk
Tested-by: Doug Anderson diand...@chromium.org
Cc: Tomasz Figa t.f...@samsung.com
---
Changes since v5:
Fixed the making a GPIO chip select mandatory bug.
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 | 38
2 files changed, 17 insertions(+), 29 deletions(-)
diff --git a/Documentation/devicetree/bindings/spi/spi-samsung.txt
b/Documentation/devicetree/bindings/spi/spi-samsung.txt
index 655b665..792efba 100644
--- a/Documentation/devicetree/bindings/spi/spi-samsung.txt
+++ b/Documentation/devicetree/bindings/spi/spi-samsung.txt
@@ -39,15 +39,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..b61ff3d 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 intport_id;
- boolcs_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
