[PATCH] i2c: designware: Suppress error message if platform_get_irq() returns -EPROBE_DEFER
There's no point in printing error message if platform_get_irq()
returns -EPROBE_DEFER because probe deferring subsystem already outputs
message in bootlog like this:
---8---
platform e001d000.i2c: Driver i2c_designware requests probe deferral
---8---
Moreover in case of probe deferral following message may mislead user:
---8---
i2c_designware e001d000.i2c: no irq resource?
---8---
even though it's expected that platform_get_irq() may return
-EPROBE_DEFER.
Signed-off-by: Alexey Brodkin abrod...@synopsys.com
Cc: Vineet Gupta vgu...@synopsys.com
Cc: Christian Ruppert christian.rupp...@abilis.com
Cc: Mika Westerberg mika.westerb...@linux.intel.com
Cc: Wolfram Sang w...@the-dreams.de
---
drivers/i2c/busses/i2c-designware-platdrv.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
b/drivers/i2c/busses/i2c-designware-platdrv.c
index c270f5f..01c1b17 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -144,7 +144,8 @@ static int dw_i2c_probe(struct platform_device *pdev)
irq = platform_get_irq(pdev, 0);
if (irq 0) {
- dev_err(pdev-dev, no irq resource?\n);
+ if (irq != -EPROBE_DEFER)
+ dev_err(pdev-dev, no irq resource?\n);
return irq; /* -ENXIO */
}
--
2.1.0
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c: designware: Suppress error message if platform_get_irq() returns -EPROBE_DEFER
Hi Christian,
On Tue, 2015-03-03 at 17:28 +0100, Christian Ruppert wrote:
On 2015-03-03 16:27, Alexey Brodkin wrote:
There's no point in printing error message if platform_get_irq()
returns -EPROBE_DEFER because probe deferring subsystem already outputs
message in bootlog like this:
---8---
platform e001d000.i2c: Driver i2c_designware requests probe deferral
---8---
Moreover in case of probe deferral following message may mislead user:
---8---
i2c_designware e001d000.i2c: no irq resource?
---8---
even though it's expected that platform_get_irq() may return
-EPROBE_DEFER.
irq = platform_get_irq(pdev, 0);
if (irq 0) {
- dev_err(pdev-dev, no irq resource?\n);
+ if (irq != -EPROBE_DEFER)
+ dev_err(pdev-dev, no irq resource?\n);
Presented like this I wonder if this merits being a dev_err at all.
Wouldn't dev_dbg be more adequate? This might remove the need for the
condition and also avoid bothering everyone if something in the platform
device structures or device tree is not right.
return irq; /* -ENXIO */
}
We've just had similar discussion related to DW APB UART with Andy here
https://lkml.org/lkml/2015/3/3/412
So yes probably we may safely remove error message from here completely.
-Alexey
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c: designware: Suppress error message if platform_get_irq() returns -EPROBE_DEFER
Hello Alexey,
Alexey Brodkin alexey.brod...@synopsys.com wrote on 03/03/2015 05:37:31
PM:
From:
Alexey Brodkin alexey.brod...@synopsys.com
To:
christian.rupp...@alitech.com christian.rupp...@alitech.com,
Cc:
mika.westerb...@linux.intel.com mika.westerb...@linux.intel.com,
linux-ker...@vger.kernel.org linux-ker...@vger.kernel.org,
vineet.gup...@synopsys.com vineet.gup...@synopsys.com, wsa@the-
dreams.de w...@the-dreams.de, andriy.shevche...@linux.intel.com
andriy.shevche...@linux.intel.com, linux-i2c@vger.kernel.org
linux-i2c@vger.kernel.org, christian.rupp...@abilis.com
christian.rupp...@abilis.com
Date:
03/03/2015 05:38 PM
Subject:
Re: [PATCH] i2c: designware: Suppress error message if
platform_get_irq() returns -EPROBE_DEFER
Hi Christian,
[...]
irq = platform_get_irq(pdev, 0);
if (irq 0) {
- dev_err(pdev-dev, no irq resource?\n);
+ if (irq != -EPROBE_DEFER)
+ dev_err(pdev-dev, no irq resource?\n);
Presented like this I wonder if this merits being a dev_err at all.
Wouldn't dev_dbg be more adequate? This might remove the need for the
condition and also avoid bothering everyone if something in the
platform
device structures or device tree is not right.
return irq; /* -ENXIO */
}
We've just had similar discussion related to DW APB UART with Andy here
https://lkml.org/lkml/2015/3/3/412
So yes probably we may safely remove error message from here completely.
I agree. Although you do have a point (in the other thread) when saying
this
kind of messages can be useful in some situations. The process of
debugging
device tree and platform device setup is definitely more painful for
drivers
which omit this type of messages completely. Andy's proposal of
centralising
this looks like a very good solution here (and on top of that removes many
useless strings from the kernel binary).
Greetings,
Christian
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c: designware: Suppress error message if platform_get_irq() returns -EPROBE_DEFER
On 2015-03-03 16:27, Alexey Brodkin wrote:
There's no point in printing error message if platform_get_irq()
returns -EPROBE_DEFER because probe deferring subsystem already outputs
message in bootlog like this:
---8---
platform e001d000.i2c: Driver i2c_designware requests probe deferral
---8---
Moreover in case of probe deferral following message may mislead user:
---8---
i2c_designware e001d000.i2c: no irq resource?
---8---
even though it's expected that platform_get_irq() may return
-EPROBE_DEFER.
Signed-off-by: Alexey Brodkin abrod...@synopsys.com
Cc: Vineet Gupta vgu...@synopsys.com
Cc: Christian Ruppert christian.rupp...@abilis.com
Cc: Mika Westerberg mika.westerb...@linux.intel.com
Cc: Wolfram Sang w...@the-dreams.de
---
drivers/i2c/busses/i2c-designware-platdrv.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
b/drivers/i2c/busses/i2c-designware-platdrv.c
index c270f5f..01c1b17 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -144,7 +144,8 @@ static int dw_i2c_probe(struct platform_device *pdev)
irq = platform_get_irq(pdev, 0);
if (irq 0) {
- dev_err(pdev-dev, no irq resource?\n);
+ if (irq != -EPROBE_DEFER)
+ dev_err(pdev-dev, no irq resource?\n);
Presented like this I wonder if this merits being a dev_err at all.
Wouldn't dev_dbg be more adequate? This might remove the need for the
condition and also avoid bothering everyone if something in the platform
device structures or device tree is not right.
return irq; /* -ENXIO */
}
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] i2c: imx: add required clocks property to binding
A clock specifier is required for i.MX I2C and is provided in all DTS implementations. Add this to the list of required properties in the binding. Signed-off-by: Matt Porter mpor...@konsulko.com --- Documentation/devicetree/bindings/i2c/i2c-imx.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/Documentation/devicetree/bindings/i2c/i2c-imx.txt b/Documentation/devicetree/bindings/i2c/i2c-imx.txt index 52d37fd..ce4311d 100644 --- a/Documentation/devicetree/bindings/i2c/i2c-imx.txt +++ b/Documentation/devicetree/bindings/i2c/i2c-imx.txt @@ -7,6 +7,7 @@ Required properties: - fsl,vf610-i2c for I2C compatible with the one integrated on Vybrid vf610 SoC - reg : Should contain I2C/HS-I2C registers location and length - interrupts : Should contain I2C/HS-I2C interrupt +- clocks : Should contain the I2C/HS-I2C clock specifier Optional properties: - clock-frequency : Constains desired I2C/HS-I2C bus clock frequency in Hz. -- 1.8.4 -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c: designware: Suppress error message if platform_get_irq() returns -EPROBE_DEFER
which omit this type of messages completely. Andy's proposal of centralising this looks like a very good solution here (and on top of that removes many useless strings from the kernel binary). I am all for centralizing printouts. I recommended this at my ELCE talk last year, too. However, you need to keep in mind that irqs are sometimes optional and you don't want error messages for those irqs. IMO worthwhile, but not a low hanging fruit... signature.asc Description: Digital signature
Re: [PATCH] i2c: designware: Suppress error message if platform_get_irq() returns -EPROBE_DEFER
On Tue, 2015-03-03 at 18:46 +0100, Christian Ruppert wrote: On 2015-03-03 18:21, Wolfram Sang wrote: which omit this type of messages completely. Andy's proposal of centralising this looks like a very good solution here (and on top of that removes many useless strings from the kernel binary). I am all for centralizing printouts. I recommended this at my ELCE talk last year, too. However, you need to keep in mind that irqs are sometimes optional and you don't want error messages for those irqs. IMO worthwhile, but not a low hanging fruit... There is a lot of truth in that. Thus the initial dev_dbg() suggestion to go half way. I still think that Andy's proposal (or a variation thereof to catch the optional irqs case) should be the ultimate goal but I agree that this is more than a quick patch and that it's probably way out of scope here. Yes, I was thinking even about some wrapper on top of platform_get_irq() since it seems there are no messaging done inside platform.c, though devm_* functions usually have it. -- Andy Shevchenko andriy.shevche...@intel.com Intel Finland Oy -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c: designware: Suppress error message if platform_get_irq() returns -EPROBE_DEFER
On Tue, 2015-03-03 at 20:11 +0100, Wolfram Sang wrote: Yes, I was thinking even about some wrapper on top of platform_get_irq() since it seems there are no messaging done inside platform.c, though devm_* functions usually have it. When I had a look a few months ago, the situation with devm_* was messy. Some rightfully printed errors, some rightfully didn't, some vice versa, some the other way around, and some did something else... For driver authors, it is hard to see/remember which devm function does it and which doesn't. IMO a good cleanup will get rid of this mess. I started sketching something but especially clks and irqs are basically everywhere and so it easily grew out of the fun-time project scale, sadly. Yeah, same for me. I've checked the situation with platform_get_irq() and estimate the amount of drivers about 300. That's why I discourage to create another one that needs to be fixed in the future. -- Andy Shevchenko andriy.shevche...@intel.com Intel Finland Oy -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c: designware: Suppress error message if platform_get_irq() returns -EPROBE_DEFER
I am all for centralizing printouts. I recommended this at my ELCE talk last year, too. However, you need to keep in mind that irqs are sometimes optional and you don't want error messages for those irqs. IMO worthwhile, but not a low hanging fruit... There is a lot of truth in that. Thus the initial dev_dbg() suggestion to go half way. I still think that Andy's proposal (or a variation thereof to catch the optional irqs case) should be the ultimate goal but I agree that this is more than a quick patch and that it's probably way out of scope here. Yes, I was thinking even about some wrapper on top of platform_get_irq() since it seems there are no messaging done inside platform.c, though devm_* functions usually have it. When I had a look a few months ago, the situation with devm_* was messy. Some rightfully printed errors, some rightfully didn't, some vice versa, some the other way around, and some did something else... For driver authors, it is hard to see/remember which devm function does it and which doesn't. IMO a good cleanup will get rid of this mess. I started sketching something but especially clks and irqs are basically everywhere and so it easily grew out of the fun-time project scale, sadly. signature.asc Description: Digital signature
Re: Bug in i2c-core?
Hi Dmitry, On Friday 27 February 2015 08:59:44 Dmitry Torokhov wrote: On Fri, Feb 27, 2015 at 12:09:51PM +0100, Sébastien SZYMANSKI wrote: Hi, I am writing an I2C touchscreen driver for an i.MX6 based board. I compiled it as a module and when I unload it, I get the following warning: # modprobe sx8654 [ 46.261494] input: SX8654 I2C Touchscreen as /devices/soc0/soc/210.aips-bus/21a.i2c/i2c-0/0-0048/input/input1 # rmmod sx8654 [ 76.435223] [ cut here ] [ 76.439909] WARNING: CPU: 0 PID: 134 at fs/proc/generic.c:552 remove_proc_entry+0x148/0x164() [ 76.448582] remove_proc_entry: removing non-empty directory 'irq/208', leaking at least 'sx8654' ... When I revert commit e4df3a0 (i2c: core: Dispose OF IRQ mapping at client removal time) I don't get the warning. Is this a bug in the i2c-core or am I doing something wrong in my driver? Yes, this commit breaks all drivers using devm* for IRQ management on OF-based systemsi because devm* cleanup happens in device code, after bus's remove() method returns. I'd recommend reverting and finding a better way (making cleanup a custom devm action as well?). Ouch, my bad. Wolfram, any opinion ? The original patch fixes a real bug, so we shouldn't just revert it. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Bug in i2c-core?
Yes, this commit breaks all drivers using devm* for IRQ management on OF-based systemsi because devm* cleanup happens in device code, after bus's remove() method returns. I'd recommend reverting and finding a better way (making cleanup a custom devm action as well?). Ouch, my bad. Wolfram, any opinion ? The original patch fixes a real bug, so we shouldn't just revert it. I guess the usual rules apply. Let's try to fix it for v4.0, otherwise I have to revert it. signature.asc Description: Digital signature
[PATCH v2 2/2] i2c: Support for Netlogic XLP9XX/5XX I2C controller.
From: Subhendu Sekhar Behera sbeh...@broadcom.com
Add an I2C bus driver i2c-xlp9xx.c to support the I2C block in the
XLP9xx/XLP5xx MIPS SoC. Update Kconfig and Makefile to add the
CONFIG_I2C_XLP9XX option.
Also add document Documentation/devicetree/bindings/i2c/i2c-xlp9xx.txt
for DT compatible string netlogic,xlp9xx-i2c.
Signed-off-by: Subhendu Sekhar Behera sbeh...@broadcom.com
Signed-off-by: Jayachandran C jchan...@broadcom.com
---
.../devicetree/bindings/i2c/i2c-xlp9xx.txt | 22 +
drivers/i2c/busses/Kconfig | 10 +
drivers/i2c/busses/Makefile| 1 +
drivers/i2c/busses/i2c-xlp9xx.c| 446 +
4 files changed, 479 insertions(+)
create mode 100644 Documentation/devicetree/bindings/i2c/i2c-xlp9xx.txt
create mode 100644 drivers/i2c/busses/i2c-xlp9xx.c
diff --git a/Documentation/devicetree/bindings/i2c/i2c-xlp9xx.txt
b/Documentation/devicetree/bindings/i2c/i2c-xlp9xx.txt
new file mode 100644
index ..f23ae87
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-xlp9xx.txt
@@ -0,0 +1,22 @@
+Device tree configuration for the I2C controller on the XLP9xx/5xx SoC
+
+Required properties:
+- compatible : should be netlogic,xlp9xx-i2c
+- reg : bus address start and address range size of device
+- interrupts : interrupt number
+
+Optional properties:
+- clock-frequency : frequency of bus clock in Hz
+Defaults to 100 KHz when the property is not specified
+
+Example:
+
+i2c0: i2c@113100 {
+ compatible = netlogic,xlp9xx-i2c;
+ #address-cells = 1;
+ #size-cells = 0;
+ reg = 0 0x113100 0x100;
+ clock-frequency = 40;
+ interrupts = 30;
+ interrupt-parent = pic;
+};
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 22da9c2..dde4648 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -898,6 +898,16 @@ config I2C_XLR
This driver can also be built as a module. If so, the module
will be called i2c-xlr.
+config I2C_XLP9XX
+ tristate XLP9XX I2C support
+ depends on CPU_XLP || COMPILE_TEST
+ help
+ This driver enables support for the on-chip I2C interface of
+ the Broadcom XLP9xx/XLP5xx MIPS processors.
+
+ This driver can also be built as a module. If so, the module will
+ be called i2c-xlp9xx.
+
config I2C_RCAR
tristate Renesas R-Car I2C Controller
depends on ARCH_SHMOBILE || COMPILE_TEST
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 3638feb..f8e0f0e 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -87,6 +87,7 @@ obj-$(CONFIG_I2C_WMT) += i2c-wmt.o
obj-$(CONFIG_I2C_OCTEON) += i2c-octeon.o
obj-$(CONFIG_I2C_XILINX) += i2c-xiic.o
obj-$(CONFIG_I2C_XLR) += i2c-xlr.o
+obj-$(CONFIG_I2C_XLP9XX) += i2c-xlp9xx.o
obj-$(CONFIG_I2C_RCAR) += i2c-rcar.o
# External I2C/SMBus adapter drivers
diff --git a/drivers/i2c/busses/i2c-xlp9xx.c b/drivers/i2c/busses/i2c-xlp9xx.c
new file mode 100644
index ..6886add
--- /dev/null
+++ b/drivers/i2c/busses/i2c-xlp9xx.c
@@ -0,0 +1,446 @@
+/*
+ * Copyright (c) 2003-2015 Broadcom Corporation
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed as is without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include linux/completion.h
+#include linux/errno.h
+#include linux/hardirq.h
+#include linux/i2c.h
+#include linux/init.h
+#include linux/interrupt.h
+#include linux/io.h
+#include linux/kernel.h
+#include linux/module.h
+#include linux/platform_device.h
+#include linux/slab.h
+
+#define XLP9XX_I2C_DIV 0x0
+#define XLP9XX_I2C_CTRL0x1
+#define XLP9XX_I2C_CMD 0x2
+#define XLP9XX_I2C_STATUS 0x3
+#define XLP9XX_I2C_MTXFIFO 0x4
+#define XLP9XX_I2C_MRXFIFO 0x5
+#define XLP9XX_I2C_MFIFOCTRL 0x6
+#define XLP9XX_I2C_STXFIFO 0x7
+#define XLP9XX_I2C_SRXFIFO 0x8
+#define XLP9XX_I2C_SFIFOCTRL 0x9
+#define XLP9XX_I2C_SLAVEADDR 0xA
+#define XLP9XX_I2C_OWNADDR 0xB
+#define XLP9XX_I2C_FIFOWCNT0xC
+#define XLP9XX_I2C_INTEN 0xD
+#define XLP9XX_I2C_INTST 0xE
+#define XLP9XX_I2C_WAITCNT 0xF
+#define XLP9XX_I2C_TIMEOUT 0X10
+#define XLP9XX_I2C_GENCALLADDR 0x11
+
+#define XLP9XX_I2C_CTRL_MCTLEN_SHIFT 16
+#define XLP9XX_I2C_MFIFOCTRL_HITH_SHIFT8
+#define XLP9XX_I2C_MFIFOCTRL_LOTH_SHIFT0
+#define XLP9XX_I2C_SLAVEADDR_ADDR_SHIFT1
+
+#define XLP9XX_I2C_CMD_START BIT(7)
+#define XLP9XX_I2C_CMD_STOPBIT(6)
+#define XLP9XX_I2C_CMD_READBIT(5)
+#define XLP9XX_I2C_CMD_WRITE
[PATCH v2 0/2] i2c: Support for Netlogic XLP9XX/5XX I2C controller.
Here is an updated version of the i2c controller driver on Broadcom XLP9xx MIPS SoC. We have taken care of the review comments for v1, and have made some additional fixes while going thru the process. The Changelog is below. Please let me know if there are any comments or suggestions. Thanks, JC. Changes v1-v2: * Implement changes from Ray Jui's review - Support for 0 length transfers - remove .owner assignment in platform_driver and .data assignment in of_device_id table - add synchronize_irq and disable interrupt in .remove - Move most prints to dev_dbg - add COMPILE_TEST - align wrapped line in function definitions - IRQ_NONE return if interrupt is not ours. - error check frequency setting in DTB - fix check of return of devm_ioremap_resource() and remove unnecessary prints - fix incorrect irq check - include file ordering fixed - few unneeded variables taken out - use u32 consistently for unsigned 32 bit int - add comment for prescale value calculation * Change clock-frequency parameter to take the bus frequency as expected in i2c subsystem * Rearrage register definitions to group them better * Add I2C_FUNC_10BIT_ADDR to xlp9xx_i2c_functionality() * use reinit_completion before each transfer * use i2c_add_adapter instead of doing i2c_add_numbered_adapter Subhendu Sekhar Behera (2): of: Add vendor prefix 'netlogic' i2c: Support for Netlogic XLP9XX/5XX I2C controller. .../devicetree/bindings/i2c/i2c-xlp9xx.txt | 19 + .../devicetree/bindings/vendor-prefixes.txt| 1 + drivers/i2c/busses/Kconfig | 10 + drivers/i2c/busses/Makefile| 1 + drivers/i2c/busses/i2c-xlp9xx.c| 446 + 5 files changed, 477 insertions(+) create mode 100644 Documentation/devicetree/bindings/i2c/i2c-xlp9xx.txt create mode 100644 drivers/i2c/busses/i2c-xlp9xx.c -- 1.9.1 -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/2] of: Add vendor prefix 'netlogic'
From: Subhendu Sekhar Behera sbeh...@broadcom.com Add vendor name netlogic in vendor-prefixes.txt, which will be used for the Netlogic XLP and XLPII MIPS SoCs. These processors were from NetLogic Microsystems which is now part of Broadcom Corporation. Signed-off-by: Subhendu Sekhar Behera sbeh...@broadcom.com Signed-off-by: Jayachandran C jchan...@broadcom.com --- Documentation/devicetree/bindings/vendor-prefixes.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt index 389ca13..a718eb1 100644 --- a/Documentation/devicetree/bindings/vendor-prefixes.txt +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt @@ -123,6 +123,7 @@ mxicy Macronix International Co., Ltd. national National Semiconductor neonodeNeonode Inc. netgearNETGEAR +netlogic Broadcom Corporation (formerly NetLogic Microsystems) newhaven Newhaven Display International nintendo Nintendo nokia Nokia -- 1.9.1 -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC] i2c: rcar: Support ACK by HW auto restart after NACK
Hi Wolfram, On Mon, Feb 23, 2015 at 02:08:42PM +0100, Wolfram Sang wrote: Hi, On 2015-02-15 15:44, Yoshihiro Kaneko wrote: From: Ryo Kataoka ryo.kataoka...@renesas.com Even if R-Car I2C received NACK, after that it might receive ACK by HW auto restart. In case of that, driver would continue process. If R-Car I2C didn't receive ACK, the driver would detect timeout and would report NACK as -ENXIO. Signed-off-by: Ryo Kataoka ryo.kataoka...@renesas.com Signed-off-by: Yoshihiro Kaneko ykaneko0...@gmail.com Excuse me, but what exactly is HW auto restart in this case? Is it a feature of the I2C slave? I asked Kataoka-san about this and his response was as follows: It is a feature of the i2c-rcar(H/W) master. If system(CPU) is busy, NACK procedure may have interrupt latency. Since the clear of ICMCR.ESG bit is delayed, i2c-rcar(H/W) may auto-restart after NACK. Please refer to ESG bit of H/W UM section 55.3.5. For example, this is I2C write transmitting. 1.Start / 2.SlaveAddr,ACK / 3.RegAddr,ACK / 4.RegData,ACK / 5.Stop If No.2 has NACK and interruption has delay, this transmitting is as follows. 1.Start / 2.SlaveAddr,NACK/ 1x.auto-restart / 2x.SlaveAddr,ACK / 3.RegAddr,ACK / 4.RegData,ACK / 5.Stop NACK of No.2 is invalidated by ACK of No.2x. It means recover. -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC] i2c: rcar: Fix order of restart and clear status
Hi Wolfram, On Mon, Feb 23, 2015 at 02:07:18PM +0100, Wolfram Sang wrote: Hi, On 2015-02-15 15:43, Yoshihiro Kaneko wrote: From: Ryo Kataoka ryo.kataoka...@renesas.com In case of repeated START condition, the restart has to be kicked before clear status (MSR register). If it is kicked after clear status, R-Car I2C may transfer data (TXD register) or receive data (RXD register) instead of transferring slave address (MAR register). Signed-off-by: Ryo Kataoka ryo.kataoka...@renesas.com Signed-off-by: Yoshihiro Kaneko ykaneko0...@gmail.com Thanks for the patch! I wondered if we couldn't always kick the start/restart before clearing the status register? I asked Kataoka-san about this and his response was as follows: If system(CPU) is busy, the driver can't clear the status register soon after kicking start. If sequence of first start is as follows, there is a problem. Because H/W starts by 1. But sequence of re-start is as follows, there is no problem. Because H/W starts by 2. 1. Issue START condition by ESG bit of ICMCR register. --- If there is too much time, H/W finish transmitting and set status in status register. 2. Clear interrupt status (ICMSR). 3. Open interrupt mask. 4. Wait interrupt. --- If status is cleared, interrupt does not occur. I am not sure if this simulates the flaw accurately, but I inserted a udelay(500) in the original code after clearing the status register. Then, I couldn't access the audio codec on my Lager board anymore. By always first kicking and then clearing, access was possible again. But as said, I am not sure if my scenario matches yours. Have you considered to treat start and repeated start the same and always kick the condition before clearing the status? -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c: designware: Suppress error message if platform_get_irq() returns -EPROBE_DEFER
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 2015-03-03 18:21, Wolfram Sang wrote: which omit this type of messages completely. Andy's proposal of centralising this looks like a very good solution here (and on top of that removes many useless strings from the kernel binary). I am all for centralizing printouts. I recommended this at my ELCE talk last year, too. However, you need to keep in mind that irqs are sometimes optional and you don't want error messages for those irqs. IMO worthwhile, but not a low hanging fruit... There is a lot of truth in that. Thus the initial dev_dbg() suggestion to go half way. I still think that Andy's proposal (or a variation thereof to catch the optional irqs case) should be the ultimate goal but I agree that this is more than a quick patch and that it's probably way out of scope here. -BEGIN PGP SIGNATURE- Version: GnuPG v2.0.14 (GNU/Linux) iQIcBAEBAgAGBQJU9fONAAoJENOBSR1q+OdQLh0P/3f9NIBGyvpr3ao+UVhiW26f 8ikcd1lHezjGZuILPgsobb4bpePwyV0IYLoqediGtwOSlbe6IlxC5DTzwlcJhUe5 BDtker32yPTohA8uc27PFlnZ3secJ3xngmF43QwsP4waQxdLsufQ85YQ1/P4SXhQ gkxZpE1J1T4jVshtrf4lWlMJ3okdN2n/aFILRkNPCGc5QV9FjAMDwzYJLhBJw8wj 0Xe0X6zflgLlbUGfIeUIPoS7Stn9JtLZt/ltRAPBfFdKwBLUWOvmqGWl5XBcgzRi UCEEefQl7T/H5HXOEdUyLuCFNsa7rMLEGX8HVSpqb1516dMM4L4vQomISKQyW5ED SHLaCwzIsQTLdaY4gfNdKjJQMkuZSktz5zG8k3SRAR1PSqzNn7isUcc12HmoyRUd 2lONuN79r3z2iM591FofG69uC64RAJc5DHBj9gHbyvPwp48SMOQThO2vUunZvDoZ E9rfux3Dj0txpxeh4GqGUjtDXmIhcLm9uXMYQ2UBMH/vaz3adWcX3rH76MLTIZCS a0ilv+7Jr8VDgJcik2hiFIsSURSZqKSub6bXefvUYc8jOscREa0SnBQvGT3xkl+p n57+5d+lWpkT4a5LSVRqkhhAtG+g/Ti09YOfhR2Z3C2xvWxM7VyVbxY69UDPXW5Z sBDHtNkqCZUcx5wdkPec =cvA7 -END PGP SIGNATURE- -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
