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 2/3] spi: s3c64xx: validate s3c64xx_spi_csinfo before using
Hello Naveen, On 07/15/2014 07:49 PM, Tomasz Figa wrote: In general, I liked previous version of this series much more, as it was doing what it should as opposed to this one. Best regards, Tomasz I agree with Tomasz. I think version v6 of your series was (almost) correct while this is one is not. You only had to address Mark's concerns about using gpio_is_valid(spi-cs_gpio). Also, you need to work out your commit messages since I they are still not clearly explaining the motivation for the changes. You are explaining what the patches are changing but you have to explain why the changes are needed in the first place. Best regards, Javier -- 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 v5] spi: s3c64xx: fix broken cs_gpios usage in the driver
Hello Naveen and Mark,
On Mon, Jul 7, 2014 at 1:22 PM, Javier Martinez Canillas
jav...@dowhile0.org wrote:
On Mon, Jul 7, 2014 at 10:31 AM, Naveen Krishna Ch
naveenkrishna...@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 withsdd-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
Re: [PATCH 1/3 v5] spi: s3c64xx: fix broken cs_gpios usage in the driver
Hello Naveen and Mark,
On Mon, Jul 7, 2014 at 10:31 AM, Naveen Krishna Ch
naveenkrishna...@gmail.com wrote:
Hello Mark,
On 7 July 2014 13:02, Mark Brown broo...@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 broo...@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 withsdd-cs_gpio flag for this purpose.
--
Re: [PATCH 1/3 v4] spi: s3c64xx: fix broken cs_gpios usage in the driver
Hello Naveen,
On 06/12/2014 03:13 PM, 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.
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
Cc: Javier Martinez Canillas javier.marti...@collabora.co.uk
Cc: Doug Anderson diand...@chromium.org
Cc: Tomasz Figa t.f...@samsung.com
---
Usually when you send a new version of a series is good to keep a history of the
patch-set so reviewers don't have to look at the previous threads in order to
see what changed from version to version.
Anything that is between --- and the actual diff is not part of a commit and
tools like git am omits that part so that's were you usually describe the
history.
.../devicetree/bindings/spi/spi-samsung.txt|8 ++--
drivers/spi/spi-s3c64xx.c | 41
2 files changed, 20 insertions(+), 29 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..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 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
Re: [PATCH 2/3 v4] spi: s3c64xx: for DT platofrms always get the chipselect info from DT node
Hello Naveen,
On 06/12/2014 03:13 PM, Naveen Krishna Chatradhi wrote:
Use controller_data structure only for the Non Device tree platforms.
For Device tree platforms, always derive the chipselect info from
DT node.
Signed-off-by: Naveen Krishna Chatradhi ch.nav...@samsung.com
Cc: Javier Martinez Canillas javier.marti...@collabora.co.uk
Cc: Doug Anderson diand...@chromium.org
Cc: Tomasz Figa t.f...@samsung.com
---
drivers/spi/spi-s3c64xx.c | 11 ++-
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index b888c66..f27e15d 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -795,14 +795,15 @@ static int s3c64xx_spi_setup(struct spi_device *spi)
int err;
sdd = spi_master_get_devdata(spi-master);
- if (!cs spi-dev.of_node) {
+ if (spi-dev.of_node) {
cs = s3c64xx_get_slave_ctrldata(spi);
spi-controller_data = cs;
- }
-
- /* For the non-DT platforms derive chip selects from controller data */
- if (!spi-dev.of_node)
+ } else {
+ /* For the non-DT platforms derive chip
+ * selects from controller data
+ */
spi-cs_gpio = cs-line;
+ }
if (IS_ERR_OR_NULL(cs)) {
dev_err(spi-dev, No CS for SPI(%d)\n, spi-chip_select);
Personally I wouldn't have this change as a separate patch since it's too
related to what you changed in Patch 1. But it's just a nitpick.
Reviewed-by: Javier Martinez Canillas javier.marti...@collabora.co.uk
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 3/3 v4] ARM: DTS: fix the chip select gpios definition in the SPI nodes
Hello Naveen,
On 06/12/2014 03:13 PM, Naveen Krishna Chatradhi wrote:
This patch replaces the cs-gpio from controller-data node
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.
Respective changes are preposed to spi-s3c64xx.c driver.
@ http://www.spinics.net/lists/linux-samsung-soc/msg32282.html
Signed-off-by: Naveen Krishna Chatradhi ch.nav...@samsung.com
Acked-by: Rob Herring r...@kernel.org
Cc: Javier Martinez Canillas javier.marti...@collabora.co.uk
Cc: Doug Anderson diand...@chromium.org
Cc: Tomasz Figa t.f...@samsung.com
---
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(-)
diff --git a/arch/arm/boot/dts/exynos4210-smdkv310.dts
b/arch/arm/boot/dts/exynos4210-smdkv310.dts
index 636d166..9191491 100644
--- a/arch/arm/boot/dts/exynos4210-smdkv310.dts
+++ b/arch/arm/boot/dts/exynos4210-smdkv310.dts
@@ -169,6 +169,7 @@
spi_2: spi@1394 {
status = okay;
+ cs-gpios = gpc1 2 0;
w25x80@0 {
#address-cells = 1;
@@ -178,7 +179,6 @@
spi-max-frequency = 100;
controller-data {
- cs-gpio = gpc1 2 0;
samsung,spi-feedback-delay = 0;
};
diff --git a/arch/arm/boot/dts/exynos4412-trats2.dts
b/arch/arm/boot/dts/exynos4412-trats2.dts
index 8a558b7..204b0de 100644
--- a/arch/arm/boot/dts/exynos4412-trats2.dts
+++ b/arch/arm/boot/dts/exynos4412-trats2.dts
@@ -512,6 +512,7 @@
spi_1: spi@1393 {
pinctrl-names = default;
pinctrl-0 = spi1_bus;
+ cs-gpios = gpb 5 0;
status = okay;
s5c73m3_spi: s5c73m3 {
@@ -519,7 +520,6 @@
spi-max-frequency = 5000;
reg = 0;
controller-data {
- cs-gpio = gpb 5 0;
samsung,spi-feedback-delay = 2;
};
};
diff --git a/arch/arm/boot/dts/exynos5250-smdk5250.dts
b/arch/arm/boot/dts/exynos5250-smdk5250.dts
index a794a70..0c6433a 100644
--- a/arch/arm/boot/dts/exynos5250-smdk5250.dts
+++ b/arch/arm/boot/dts/exynos5250-smdk5250.dts
@@ -316,6 +316,7 @@
};
spi_1: spi@12d3 {
+ cs-gpios = gpa2 5 0;
status = okay;
w25q80bw@0 {
@@ -326,7 +327,6 @@
spi-max-frequency = 100;
controller-data {
- cs-gpio = gpa2 5 0;
samsung,spi-feedback-delay = 0;
};
Looks good to me.
Reviewed-by: Javier Martinez Canillas javier.marti...@collabora.co.uk
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
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 ch.nav...@samsung.com
Acked-by: Rob Herring r...@kernel.org
Cc: Javier Martinez Canillas javier.marti...@collabora.co.uk
Cc: Doug Anderson diand...@chromium.org
Cc: Tomasz Figa t.f...@samsung.com
---
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
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
javier.marti...@collabora.co.uk 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 ch.nav...@samsung.com
Acked-by: Rob Herring r...@kernel.org
Cc: Javier Martinez Canillas javier.marti...@collabora.co.uk
Cc: Doug Anderson diand...@chromium.org
Cc: Tomasz Figa t.f...@samsung.com
---
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
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 javier.marti...@collabora.co.uk 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
