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
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] spi: s3c64xx: move cs-gpio from subnode to SPI DT node
Hi Naveen,
Please see my comments inline.
On 15.07.2014 14:20, Naveen Krishna Chatradhi wrote:
This patch modifies the spi-s3c64xx.c driver to fetch the
Chip select or Slave select gpio line property cs-gpios
from SPI node instead of controller_data subnode.
Rename the property cs-gpio to cs-gpios in accordance
with the SPI core. Such that s3c64xx.c can use spi-cs_gpio
instead of parsing the property in the driver.
Update the dt-bindings ion spi/spi-samsung.txt
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
---
This patch is a rework of the change @
http://www.mail-archive.com/devicetree@vger.kernel.org/msg34500.html
I'm not sure if i can carry forward the other Signed-offs and Tested-bys
[snip]
diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 75a5696..72bfba6 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -764,12 +764,6 @@ static struct s3c64xx_spi_csinfo
*s3c64xx_get_slave_ctrldata(
return ERR_PTR(-EINVAL);
}
- data_np = of_get_child_by_name(slave_np, controller-data);
- if (!data_np) {
- dev_err(spi-dev, child node 'controller-data' not found\n);
- return ERR_PTR(-EINVAL);
- }
Do you need to move this code block?
-
cs = kzalloc(sizeof(*cs), GFP_KERNEL);
if (!cs) {
of_node_put(data_np);
@@ -777,13 +771,17 @@ static struct s3c64xx_spi_csinfo
*s3c64xx_get_slave_ctrldata(
}
/* 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);
+ cs-line = spi-cs_gpio;
if (!gpio_is_valid(cs-line)) {
This check is wrong when native chip select is used. However I'm not
sure how to distinguish this from a situation when invalid GPIO was
specified, because cs-line will be -ENOENT in both cases. Mark, any ideas?
dev_err(spi-dev, chip select gpio is not specified or
invalid\n);
kfree(cs);
- of_node_put(data_np);
+ return ERR_PTR(-EINVAL);
+ }
+
+ data_np = of_get_child_by_name(slave_np, controller-data);
+ if (!data_np) {
+ dev_err(spi-dev, child node 'controller-data' not found\n);
return ERR_PTR(-EINVAL);
}
@@ -1077,7 +1075,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;
What is this boolean flag used for now? If cs-line now either contains
a valid GPIO or a negative error, why gpio_is_valid() couldn't be used
on it? I believe it was done correctly in previous version.
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 2/3] spi: s3c64xx: validate s3c64xx_spi_csinfo before using
Hi Naveen,
Please see my comments inline.
On 15.07.2014 14:20, Naveen Krishna Chatradhi wrote:
This patch validates the cs-line (Chip select gpio) and
struct s3c64xx_spi_csinfo *cs object for both DT and NON-DT
platforms before using in .setup().
Also, check gpio_is_valid(spi-cs_gpio) in cleanup() before
freeing up.
Missing reason of the change.
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
---
drivers/spi/spi-s3c64xx.c | 15 ---
1 file changed, 4 insertions(+), 11 deletions(-)
diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 72bfba6..8971076 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -773,12 +773,6 @@ static struct s3c64xx_spi_csinfo
*s3c64xx_get_slave_ctrldata(
/* The CS line is asserted/deasserted by the gpio pin */
cs-line = spi-cs_gpio;
- if (!gpio_is_valid(cs-line)) {
- dev_err(spi-dev, chip select gpio is not specified or
invalid\n);
- kfree(cs);
- return ERR_PTR(-EINVAL);
- }
-
data_np = of_get_child_by_name(slave_np, controller-data);
if (!data_np) {
dev_err(spi-dev, child node 'controller-data' not found\n);
@@ -805,15 +799,14 @@ 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)
This check is equivalent, i.e. cs will always be NULL whenever
spi-dev.of_node is not.
cs = s3c64xx_get_slave_ctrldata(spi);
- spi-controller_data = cs;
- }
- if (IS_ERR_OR_NULL(cs)) {
+ if (IS_ERR_OR_NULL(cs) || !gpio_is_valid(cs-line)) {
I'm not sure this is correct. It will error out even if hardware chip
select is used.
Otherwise you need to free cs here if wrong GPIO was the cause of
entering this block, so if this extra check is to stay, I'd suggest
splitting this into two separate if blocks.
dev_err(spi-dev, No CS for SPI(%d)\n, spi-chip_select);
return -ENODEV;
}
+ spi-controller_data = cs;
I'm not sure what's the point of moving this assignment here.
if (!spi_get_ctldata(spi)) {
/* Request gpio only if cs line is asserted by gpio pins */
@@ -898,7 +891,7 @@ static void s3c64xx_spi_cleanup(struct spi_device *spi)
struct s3c64xx_spi_driver_data *sdd;
sdd = spi_master_get_devdata(spi-master);
- if (spi-cs_gpio) {
+ if (gpio_is_valid(spi-cs_gpio)) {
gpio_free(spi-cs_gpio);
if (spi-dev.of_node)
kfree(cs);
I believe this is completely wrong. cs is allocated even if GPIO chip
select is not used, so the only thing that should be done conditionally
is gpio_free().
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
--
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/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 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(). 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
On 11.06.2014 20:23, Naveen Krishna Ch wrote: Hello Tomasz, On 11 June 2014 23:20, Tomasz Figa tomasz.f...@gmail.com 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(). 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 v2] spi: s3c64xx: use cs-gpios from spi node instead of cs-gpio
On 10.06.2014 20:26, Doug Anderson wrote:
Naveen,
Not a full review, but a few quick things I happened to notice:
On Tue, Jun 10, 2014 at 3:08 AM, Naveen Krishna Chatradhi
ch.nav...@samsung.com wrote:
@@ -94,7 +93,6 @@ Example:
spi-max-frequency = 1;
controller-data {
- cs-gpio = gpa2 5 1 0 3;
samsung,spi-feedback-delay = 0;
In a future patch (not this one!) it might make sense to also move
spi-feedback-delay out.
+static struct s3c64xx_spi_csinfo *s3c64xx_get_cs_gpios(struct spi_device
*spi)
I believe that you can make use of the fact that the SPI core already
got these GPIOs for you. See of_spi_register_master(). Everything
you need ought to be in master-num_chipselect and master-cs_gpios.
Strangely, it doesn't look like any other drivers use this, so
possibly I'm confused...
@@ -806,9 +815,14 @@ static int s3c64xx_spi_setup(struct spi_device *spi)
struct s3c64xx_spi_info *sci;
int err;
+ if (!spi-dev.of_node) {
+ dev_err(spi-dev, device node not found\n);
+ return -EINVAL;
+ }
This seems incredibly broken. You're saying that the SPI driver no
longer works without the device tree? I don't think that's desirable,
but I suppose I could be wrong.
Also note that you still left an if test below for trying to deal
with the fact that there is no of_node.
This driver can be also used by non-DT platforms (namely s3c2443,
s3c64xx and s5p*) and so this assumption is wrong.
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 v2] spi: s3c64xx: use cs-gpios from spi node instead of cs-gpio
On 10.06.2014 20:09, Doug Anderson wrote: Naveen / Sylwester, On Tue, Jun 10, 2014 at 4:00 AM, Naveen Krishna Ch naveenkrishna...@gmail.com wrote: Can we support both cs-gpio and cs-gpios for backward compatibility ? After your change all DTBs using the original pattern will not work with new kernels any more. At least I would expect such backward compatibility maintained for few kernel releases. The reason behind removing the cs-gpio or not providing backward compatibility was 1. Since spi-core started using cs-gpios string from spi device node several months ago. The spi-s3c64xx.c driver is partially broken for more than 6 months. 2. Supporting cs-gpio would add extra bit of code. I've corrected the dts files that were using cs-gpio under controller-data(child node) to use cs-gpio from spi device node (parent node). I will make another version if you insist. Yup, as near as I can tell this has been broken since (3146bee spi: s3c64xx: Added provision for dedicated cs pin). That landed June 25th of 2013 into the SPI tree. ...so clearly nobody was really testing Samsung's SPI driver on ToT Linux. 1 year of unnoticed brokenness seems like enough time that we could do away with the old code. If someone comes out of the woodwork and claims a dire need to support the old binding then it can be added then. In-tree users of this appear to be: arch/arm/boot/dts/exynos4210-smdkv310.dts: cs-gpio = gpc1 2 0; arch/arm/boot/dts/exynos4412-trats2.dts: cs-gpio = gpb 5 0; arch/arm/boot/dts/exynos5250-smdk5250.dts: cs-gpio = gpa2 5 0; ...and I guess nobody has bothered using the SPI flash on those boards for the last year? On Trats2, it's actually a camera sensor, not SPI flash... Still, that's really a shame that: a) such patch went in (i.e. its brokenness was not spotted in review), b) the regression was not spotted. Anyway, IMHO this justifies removing the old binding then. 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 v2] spi: s3c64xx: use cs-gpios from spi node instead of cs-gpio
Hi Naveen,
On 10.06.2014 12:08, 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.
[snip]
@@ -85,6 +83,7 @@ Example:
#size-cells = 0;
pinctrl-names = default;
pinctrl-0 = spi0_bus;
+ cs-gpios = gpa2 5 1 0 3;
While at it, you might also update the example to something more
appropriate on Samsung platforms, e.g. gpa2 5 0;
w25q80bw@0 {
#address-cells = 1;
[snip]
+static struct s3c64xx_spi_csinfo *s3c64xx_get_cs_gpios(struct spi_device
*spi)
+{
+ struct device_node *parent_np = NULL;
+ struct s3c64xx_spi_driver_data *sdd;
+ struct s3c64xx_spi_csinfo *cs;
+
+ parent_np = of_get_parent(spi-dev.of_node);
+ if (!parent_np) {
+ dev_err(spi-dev, Parent node not found\n);
return ERR_PTR(-EINVAL);
}
+ sdd = spi_master_get_devdata(spi-master);
+
cs = kzalloc(sizeof(*cs), GFP_KERNEL);
if (!cs) {
- of_node_put(data_np);
+ of_node_put(parent_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);
+ cs-line = of_get_named_gpio(parent_np, cs-gpios, 0);
This is wrong. The cs-gpios property is supposed to be an array,
indexed by chip select number of SPI devices (indicated by their reg
properties).
Moreover, is there a need to parse this manually in this driver? I can
see respective parsing code in of_spi_register_master().
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 v2] spi: s3c64xx: use cs-gpios from spi node instead of cs-gpio
On 10.06.2014 21:58, Doug Anderson wrote: Tomasz, On Tue, Jun 10, 2014 at 12:49 PM, Tomasz Figa tomasz.f...@gmail.com wrote: This is wrong. The cs-gpios property is supposed to be an array, indexed by chip select number of SPI devices (indicated by their reg properties). Moreover, is there a need to parse this manually in this driver? I can see respective parsing code in of_spi_register_master(). I noticed this too (see my response), but I was confused about the fact that nobody else uses the array created by of_spi_register_master(). Any idea why? Hmm, I can see of_spi_register_master() assigning allocated pointer to master-cs_gpios, which is then used in spi_add_device() as follows: if (master-cs_gpios) spi-cs_gpio = master-cs_gpios[spi-chip_select]; 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/4] spi: s3c64xx: modified error interrupt handling and init
On Thursday 07 of February 2013 09:46:58 Girish KS wrote:
On Thu, Feb 7, 2013 at 3:09 AM, Tomasz Figa t.f...@samsung.com wrote:
Hi Girish,
On Wednesday 06 of February 2013 12:12:29 Girish KS wrote:
On Wed, Feb 6, 2013 at 2:26 AM, Grant Likely
grant.lik...@secretlab.ca
wrote:
On Tue, 5 Feb 2013 15:09:41 -0800, Girish K S
girishks2...@gmail.com wrote:
The status of the interrupt is available in the status register,
so reading the clear pending register and writing back the same
value will not actually clear the pending interrupts. This patch
modifies the interrupt handler to read the status register and
clear the corresponding pending bit in the clear pending register.
Modified the hwInit function to clear all the pending interrupts.
Signed-off-by: Girish K S ks.g...@samsung.com
---
drivers/spi/spi-s3c64xx.c | 41
+ 1 file changed, 25
insertions(+), 16 deletions(-)
diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index ad93231..b770f88 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -997,25 +997,30 @@ static irqreturn_t s3c64xx_spi_irq(int irq,
void *data)
{
struct s3c64xx_spi_driver_data *sdd = data;
struct spi_master *spi = sdd-master;
- unsigned int val;
+ unsigned int val, clr = 0;
- val = readl(sdd-regs + S3C64XX_SPI_PENDING_CLR);
+ val = readl(sdd-regs + S3C64XX_SPI_STATUS);
- val = S3C64XX_SPI_PND_RX_OVERRUN_CLR |
- S3C64XX_SPI_PND_RX_UNDERRUN_CLR |
- S3C64XX_SPI_PND_TX_OVERRUN_CLR |
- S3C64XX_SPI_PND_TX_UNDERRUN_CLR;
-
- writel(val, sdd-regs + S3C64XX_SPI_PENDING_CLR);
-
- if (val S3C64XX_SPI_PND_RX_OVERRUN_CLR)
+ if (val S3C64XX_SPI_ST_RX_OVERRUN_ERR) {
+ clr = S3C64XX_SPI_PND_RX_OVERRUN_CLR;
dev_err(spi-dev, RX overrun\n);
- if (val S3C64XX_SPI_PND_RX_UNDERRUN_CLR)
+ }
+ if (val S3C64XX_SPI_ST_RX_UNDERRUN_ERR) {
+ clr |= S3C64XX_SPI_PND_RX_UNDERRUN_CLR;
dev_err(spi-dev, RX underrun\n);
- if (val S3C64XX_SPI_PND_TX_OVERRUN_CLR)
+ }
+ if (val S3C64XX_SPI_ST_TX_OVERRUN_ERR) {
+ clr |= S3C64XX_SPI_PND_TX_OVERRUN_CLR;
dev_err(spi-dev, TX overrun\n);
- if (val S3C64XX_SPI_PND_TX_UNDERRUN_CLR)
+ }
+ if (val S3C64XX_SPI_ST_TX_UNDERRUN_ERR) {
+ clr |= S3C64XX_SPI_PND_TX_UNDERRUN_CLR;
dev_err(spi-dev, TX underrun\n);
+ }
+
+ /* Clear the pending irq by setting and then clearing it */
+ writel(clr, sdd-regs + S3C64XX_SPI_PENDING_CLR);
+ writel(clr ~clr, sdd-regs + S3C64XX_SPI_PENDING_CLR);
Wait, what? clr ~clr == 0 Always. What are you actually
trying
to do here?
The user manual says, wirting 1 to the pending clear register clears
the interrupt (its not auto clear to 0). so i need to explicitly
reset
those bits thats what the 2nd write does
I have looked through user's manuals of different Samsung SoCs. All of
them said that writing 1 to a bit clears the corresponding interrupt,
but none of them contain any note that it must be manually cleared to
0.
What i meant was the clear pending bit will not clear automatically.
When I set the
clear pending bit, it remains set. This is a problem for the next
interrupt cycle.
How did you check that it does not clear automatically?
In addition the expression
clr ~clr
makes no sense, because it is equal to 0.
It makes sense, because we are not disturbing the interrupt pending
bit at position 0, which is a trailing clr bit.
You either seem to misunderstand the problem I'm mentioning or not
understanding it at all.
If you take a variable named clr, no matter what value it is set to, and
you AND it with bitwise negation of the same variable, you will get 0.
See on this example:
Bits: 7 | 6 | 5 | 4 | 3 | 2 | 1 | 0
---
Values: 1 | 1 | 0 | 0 | 1 | 0 | 0 | 1
---
Negation: 0 | 0 | 1 | 1 | 0 | 1 | 1 | 0
---
AND: 0 | 0 | 0 | 0 | 0 | 0 | 0 | 0
Now, can you see that (clr ~clr) is the same as (0)?
Best regards,
Tomasz
--
Free Next-Gen Firewall Hardware Offer
Buy your Sophos next-gen firewall before the end March 2013
and get the hardware for free! Learn more.
http://p.sf.net/sfu/sophos-d2d-feb
___
spi-devel-general mailing list
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH 1/4] spi: s3c64xx: modified error interrupt handling and init
Hi Girish,
On Wednesday 06 of February 2013 12:12:29 Girish KS wrote:
On Wed, Feb 6, 2013 at 2:26 AM, Grant Likely grant.lik...@secretlab.ca
wrote:
On Tue, 5 Feb 2013 15:09:41 -0800, Girish K S
girishks2...@gmail.com wrote:
The status of the interrupt is available in the status register,
so reading the clear pending register and writing back the same
value will not actually clear the pending interrupts. This patch
modifies the interrupt handler to read the status register and
clear the corresponding pending bit in the clear pending register.
Modified the hwInit function to clear all the pending interrupts.
Signed-off-by: Girish K S ks.g...@samsung.com
---
drivers/spi/spi-s3c64xx.c | 41
+ 1 file changed, 25
insertions(+), 16 deletions(-)
diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index ad93231..b770f88 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -997,25 +997,30 @@ static irqreturn_t s3c64xx_spi_irq(int irq,
void *data)
{
struct s3c64xx_spi_driver_data *sdd = data;
struct spi_master *spi = sdd-master;
- unsigned int val;
+ unsigned int val, clr = 0;
- val = readl(sdd-regs + S3C64XX_SPI_PENDING_CLR);
+ val = readl(sdd-regs + S3C64XX_SPI_STATUS);
- val = S3C64XX_SPI_PND_RX_OVERRUN_CLR |
- S3C64XX_SPI_PND_RX_UNDERRUN_CLR |
- S3C64XX_SPI_PND_TX_OVERRUN_CLR |
- S3C64XX_SPI_PND_TX_UNDERRUN_CLR;
-
- writel(val, sdd-regs + S3C64XX_SPI_PENDING_CLR);
-
- if (val S3C64XX_SPI_PND_RX_OVERRUN_CLR)
+ if (val S3C64XX_SPI_ST_RX_OVERRUN_ERR) {
+ clr = S3C64XX_SPI_PND_RX_OVERRUN_CLR;
dev_err(spi-dev, RX overrun\n);
- if (val S3C64XX_SPI_PND_RX_UNDERRUN_CLR)
+ }
+ if (val S3C64XX_SPI_ST_RX_UNDERRUN_ERR) {
+ clr |= S3C64XX_SPI_PND_RX_UNDERRUN_CLR;
dev_err(spi-dev, RX underrun\n);
- if (val S3C64XX_SPI_PND_TX_OVERRUN_CLR)
+ }
+ if (val S3C64XX_SPI_ST_TX_OVERRUN_ERR) {
+ clr |= S3C64XX_SPI_PND_TX_OVERRUN_CLR;
dev_err(spi-dev, TX overrun\n);
- if (val S3C64XX_SPI_PND_TX_UNDERRUN_CLR)
+ }
+ if (val S3C64XX_SPI_ST_TX_UNDERRUN_ERR) {
+ clr |= S3C64XX_SPI_PND_TX_UNDERRUN_CLR;
dev_err(spi-dev, TX underrun\n);
+ }
+
+ /* Clear the pending irq by setting and then clearing it */
+ writel(clr, sdd-regs + S3C64XX_SPI_PENDING_CLR);
+ writel(clr ~clr, sdd-regs + S3C64XX_SPI_PENDING_CLR);
Wait, what? clr ~clr == 0 Always. What are you actually trying
to do here?
The user manual says, wirting 1 to the pending clear register clears
the interrupt (its not auto clear to 0). so i need to explicitly reset
those bits thats what the 2nd write does
I have looked through user's manuals of different Samsung SoCs. All of
them said that writing 1 to a bit clears the corresponding interrupt, but
none of them contain any note that it must be manually cleared to 0.
In addition the expression
clr ~clr
makes no sense, because it is equal to 0.
If you really need to clear those bits manually (and I don't think so),
you should replace this expression with 0.
Best regards,
--
Tomasz Figa
Samsung Poland RD Center
SW Solution Development, Linux Platform
--
Free Next-Gen Firewall Hardware Offer
Buy your Sophos next-gen firewall before the end March 2013
and get the hardware for free! Learn more.
http://p.sf.net/sfu/sophos-d2d-feb
___
spi-devel-general mailing list
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general
