i2c: pxa: Fixes, cleanup and support for pxa910 family
Subsystems (2)
cover i2c: pxa: Fixes, cleanup and support for pxa910 family
This patch series fixes bugs/warnings, cleans up the code and adds support for PXA910 family of devices to PXA I2C bus driver.
There has been one attempt made sometime back in 2012 to upstream some of the patches from below list, but did not get follow up later. I have consolidated all the patches, cleaned them up, splited into logical changes, added new patches and submitting it now.
I tried to maintain authorship & Signoff except where I did some significant changes to the code/logic.
Link to previous post:
http://permalink.gmane.org/gmane.linux.drivers.i2c/13557
Testing:
- Boot tested on platform based on PXA1928
- Probe is successfully passing
TODO:
- Due to lack of client driver support as of now, could not able to do functionality testing from client. I am working on PMIC driver support which is over I2C, so will have good meaningful testing then.
Jett.Zhou (3):
i2c: pxa: No need to set slave addr for i2c master mode reset
i2c: pxa: Add reset operation when i2c bus busy
i2c: pxa: Add support for pxa910/988 & new configuration features
Leilei Shang (1):
i2c: pxa: keep i2c irq ON in suspend
Rob Herring (1):
i2c: pxa: Add bus reset functionality
Shouming Wang (1):
i2c: pxa: Return I2C_RETRY when timeout in pio mode
Vaibhav Hiremath (4):
i2c: pxa: Reset i2c controller on timeout in interrupt and pio mode
i2c: pxa: enable/disable irq across message xfer
i2c: pxa: Update debug function to dump more info on error i2c:pxa: Use devm_ variants in probe function
Yi Zhang (1):
i2c: pxa: enable/disable i2c module across msg xfer
Yipeng Yao (1):
i2c: pxa: Remove compile warnning in 64bit mode
Documentation/devicetree/bindings/i2c/i2c-pxa.txt | 7 + drivers/i2c/busses/i2c-pxa.c | 272 +++++++++++++++++----- include/linux/i2c/pxa-i2c.h | 2 + 3 files changed, 225 insertions(+), 56 deletions(-)
Signature · 1 line
1.9.1
PATCH 1/12 i2c: pxa: keep i2c irq ON in suspend
From: Leilei Shang <[email protected]>
During suspend there may still be some i2c access happening, as the interrupt is shared between multiple drivers.
And if we don't keep i2c irq ON, there may be i2c access timeout if i2c is in irq mode of operation.
--- drivers/i2c/busses/i2c-pxa.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/i2c/busses/i2c-pxa.c +3 -2
@@ -1232,8 +1232,9 @@ static int i2c_pxa_probe(struct platform_device *dev) i2c->adap.algo = &i2c_pxa_pio_algorithm; }else { i2c->adap.algo = &i2c_pxa_algorithm; - ret =request_irq (irq, i2c_pxa_handler,IRQF_SHARED , -dev_name (&dev->dev), i2c); + ret =request_irq (irq, i2c_pxa_handler, +IRQF_SHARED |IRQF_NO_SUSPEND , +dev_name (&dev->dev), i2c);if (ret) goto ereqirq; }
Signature · 1 line
1.9.1
PATCH 2/12 i2c: pxa: No need to set slave addr for i2c master mode reset
In reply to Vaibhav HiremathFrom: "Jett.Zhou" <[email protected]>
Normally i2c controller works as master, so slave addr is not needed, or it will impact some slave device (eg. ST NFC chip) i2c accesses, because it has the same i2c address with controller.
--- drivers/i2c/busses/i2c-pxa.c | 2 ++ 1 file changed, 2 insertions(+)
drivers/i2c/busses/i2c-pxa.c +2 -0
@@ -459,8 +459,10 @@ static void i2c_pxa_reset(struct pxa_i2c *i2c)writel (I2C_ISR_INIT ,_ISR (i2c));writel (readl (_ICR (i2c)) & ~ICR_UR ,_ICR (i2c)); +#ifdef CONFIG_I2C_PXA_SLAVE if (i2c->reg_isar)writel (i2c->slave_addr,_ISAR (i2c)); +#endif /* set control register values */ writel (I2C_ICR_INIT | (i2c->fast_mode ?ICR_FM :0 ),_ICR (i2c));
Signature · 1 line
1.9.1
PATCH 2/12 i2c: pxa: No need to set slave addr for i2c master mode reset
drivers/i2c/busses/i2c-pxa.c static void i2c_pxa_reset(struct pxa_i2c *i2c) · line 465
Quoted 12 lines · Vaibhav Hiremath
drivers/i2c/busses/i2c-pxa.c +2
-0@@ -459,8 +459,10 @@ static void i2c_pxa_reset(struct pxa_i2c *i2c)writel (I2C_ISR_INIT ,_ISR (i2c));writel (readl (_ICR (i2c)) & ~ICR_UR ,_ICR (i2c)); +#ifdef CONFIG_I2C_PXA_SLAVE if (i2c->reg_isar)writel (i2c->slave_addr,_ISAR (i2c)); +#endif
I'd rather have :
if (i2c->reg_isar && IS_ENABLED(CONFIG_I2C_PXA_SLAVE))
writel(i2c->slave_addr, _ISAR(i2c));
Cheers.
Signature · 1 line
Robert
PATCH 2/12 i2c: pxa: No need to set slave addr for i2c master mode reset
Quoted 10 lines · Robert Jarzmik
Quoted 8 lines · Vaibhav Hiremath
drivers/i2c/busses/i2c-pxa.c +0
-0@@ -459,8 +459,10 @@ static void i2c_pxa_reset(struct pxa_i2c *i2c)writel (I2C_ISR_INIT ,_ISR (i2c));writel (readl (_ICR (i2c)) & ~ICR_UR ,_ICR (i2c));
Quoted 8 lines
drivers/i2c/busses/i2c-pxa.c static void i2c_pxa_reset(struct pxa_i2c *i2c) · line 465
Quoted 4 lines
drivers/i2c/busses/i2c-pxa.c +2
-0+#ifdef CONFIG_I2C_PXA_SLAVE if (i2c->reg_isar)writel (i2c->slave_addr,_ISAR (i2c)); +#endif I'd rather have :
if (i2c->reg_isar && IS_ENABLED(CONFIG_I2C_PXA_SLAVE)) writel(i2c->slave_addr, _ISAR(i2c));
Ok,
Will fix it in next version.
Thanks,
Vaibhav
PATCH 3/12 i2c: pxa: Add reset operation when i2c bus busy
In reply to Vaibhav HiremathFrom: "Jett.Zhou" <[email protected]>
According to some test in emei_dkb, we found some i2c slave device (eg. camera sensor ov2659 power up) introduce noise on sda, so detect i2c controller busy, and assert reset to i2c controller to recover as early as possible to avoid more latency on the entire i2c transaction.
[vaibhav.hiremath at linaro.org: Removed reduction in timeout value, as I do not have goot explanation for it. Logically it is not required. And also Updated changelog]
--- drivers/i2c/busses/i2c-pxa.c | 4 ++++ 1 file changed, 4 insertions(+)
drivers/i2c/busses/i2c-pxa.c +4 -0
@@ -314,6 +314,10 @@ static int i2c_pxa_wait_bus_not_busy(struct pxa_i2c *i2c) {int timeout =DEF_TIMEOUT ; +if (readl (_ISR (i2c)) & (ISR_IBB |ISR_UB )) +i2c_pxa_reset (i2c); + +while (timeout-- &&readl (_ISR (i2c)) & (ISR_IBB |ISR_UB )) {if ((readl (_ISR (i2c)) &ISR_SAD ) !=0 ) timeout +=4 ;
Signature · 1 line
1.9.1
PATCH 3/12 i2c: pxa: Add reset operation when i2c bus busy
Quoted 23 lines · Vaibhav Hiremath
From: "Jett.Zhou" <[email protected]>
According to some test in
emei_dkb, we found some i2c slave device (eg. camera sensor ov2659 power up) introduce noise on sda, so detect i2c controller busy, and assert reset to i2c controller to recover as early as possible to avoid more latency on the entire i2c transaction.Signed-off-by Jett.Zhou <[email protected]>[vaibhav.hiremath at linaro.org: Removed reduction in timeout value, as I do not have goot explanation for it. Logically it is not required. And also Updated changelog]
Signed-off-by Vaibhav Hiremath <[email protected]>Signed-off-by Vaibhav Hiremath <[email protected]>--- drivers/i2c/busses/i2c-pxa.c | 4 ++++ 1 file changed, 4 insertions(+)drivers/i2c/busses/i2c-pxa.c +0
-0
drivers/i2c/busses/i2c-pxa.c static int i2c_pxa_wait_bus_not_busy(struct pxa_i2c *i2c) · line 318
Quoted 5 lines
drivers/i2c/busses/i2c-pxa.c +2
-0@@ -314,6 +314,10 @@ static int i2c_pxa_wait_bus_not_busy(struct pxa_i2c *i2c) {int timeout =DEF_TIMEOUT ; +if (readl (_ISR (i2c)) & (ISR_IBB |ISR_UB )) +i2c_pxa_reset (i2c);
The pxa27x manual states in the Developer Manual, chapter 9.4.13 "Reset Conditions" :
Software must ensure that (1) the I 2 C unit is not busy before it asserts a reset
Given that, I don't agree with this patch. Moreover, reseting unconditionaly the i2c bus on each busy state on a write transaction for one single corner case is not something that has my agreement. A quirk might overcome my reluctance.
Cheers.
Signature · 1 line
Robert
PATCH 3/12 i2c: pxa: Add reset operation when i2c bus busy
Quoted 36 lines · Robert Jarzmik
Quoted 23 lines · Vaibhav Hiremath
From: "Jett.Zhou" <[email protected]>
According to some test in
emei_dkb, we found some i2c slave device (eg. camera sensor ov2659 power up) introduce noise on sda, so detect i2c controller busy, and assert reset to i2c controller to recover as early as possible to avoid more latency on the entire i2c transaction.Signed-off-by Jett.Zhou <[email protected]>[vaibhav.hiremath at linaro.org: Removed reduction in timeout value, as I do not have goot explanation for it. Logically it is not required. And also Updated changelog]
Signed-off-by Vaibhav Hiremath <[email protected]>Signed-off-by Vaibhav Hiremath <[email protected]>--- drivers/i2c/busses/i2c-pxa.c | 4 ++++ 1 file changed, 4 insertions(+)drivers/i2c/busses/i2c-pxa.c +0
-0drivers/i2c/busses/i2c-pxa.c static int i2c_pxa_wait_bus_not_busy(struct pxa_i2c *i2c) · line 318
Quoted 5 lines
drivers/i2c/busses/i2c-pxa.c +2
-0@@ -314,6 +314,10 @@ static int i2c_pxa_wait_bus_not_busy(struct pxa_i2c *i2c) {int timeout =DEF_TIMEOUT ; +if (readl (_ISR (i2c)) & (ISR_IBB |ISR_UB )) +i2c_pxa_reset (i2c);The pxa27x manual states in the Developer Manual, chapter 9.4.13 "Reset Conditions" :
Software must ensure that (1) the I 2 C unit is not busy before it asserts a reset
Quoted 1 line
Given that, I don't agree with this patch.
Hmmm,
Just saw pxa27x manual, and you are right.
In that case I am not sure how to address the issue mentioned in the changelog.
On the other side,
Check for ISR_IBB, should be ok, as, as per spec it says,
"Set when the TWSI bus is busy but the SoC TWSI is not involved in the transaction."
Also,
I believe we should be ok, as the first thing we do in i2c_pxa_reset() is abort the current transaction and then assert reset. Isn't it?
Quoted 4 lines
Moreover, reseting unconditionaly the
i2c bus on each busy state on a write transaction for one single corner case is not something that has my agreement. A quirk might overcome my reluctance.
Any condition check you can possibly think of for asserting reset???
Having said that,
Somewhere I do agree with you that we need to further debug on noise issue in HW rather hacking software. :)
I am ok to drop this patch, do further testing and revisit again if issue persist.
Thanks,
Vaibhav
PATCH 4/12 i2c: pxa: Add support for pxa910/988 & new configuration features
In reply to Vaibhav HiremathFrom: "Jett.Zhou" <[email protected]>
TWSI_ILCR & TWSI_IWCR registers are used to adjust clock rate of standard & fast mode in pxa910/988; so this patch adds these two new entries to "struct pxa_reg_layout" and "struct pxa_i2c" and also adds two new DT properties to take configuration value for the same.
Note that, DT binding document is also updated.
[vaibhav.hiremath at linaro.org: Updated kernel doc for new DT properties and updated Changelog]
--- Documentation/devicetree/bindings/i2c/i2c-pxa.txt | 7 ++++ drivers/i2c/busses/i2c-pxa.c | 44 +++++++++++++++++++++-- include/linux/i2c/pxa-i2c.h | 2 ++ 3 files changed, 51 insertions(+), 2 deletions(-)
Documentation/devicetree/bindings/i2c/i2c-pxa.txt +7 -0
@@ -18,6 +18,13 @@ Recommended properties : status register of i2c controller instead. - mrvl,i2c-fast-mode : Enable fast mode of i2c controller. +Optional Properties (Applicable to PXA910 family): + + - mrvl,i2c-ilcr : Load Count Register: Allows minor adjustment to SCL clk to + achieve std, normal and fast mode of operation. + - mrvl,i2c-iwcr : Wait Count Register - controls the setup and hold time + together with mrvl,i2c-ilcr + Examples: twsi1: i2c at d4011000 { compatible = "mrvl,mmp-twsi";
drivers/i2c/busses/i2c-pxa.c +42 -2
126 lines not on this page · raw message
include/linux/i2c/pxa-i2c.h +2 -0
@@ -70,6 +70,8 @@ struct i2c_pxa_platform_data { unsigned int high_mode:1;unsigned char master_code;unsigned long rate; +unsigned int ilcr; +unsigned int iwcr; };extern void pxa_set_i2c_info (struct i2c_pxa_platform_data *info);
Signature · 1 line
1.9.1
PATCH 4/12 i2c: pxa: Add support for pxa910/988 & new configuration features
Quoted 10 lines · Vaibhav Hiremath
drivers/i2c/busses/i2c-pxa.c +2
-0@@ -167,6 +184,8 @@ struct pxa_i2c {#define _ICR (i2c) ((i2c)->reg_icr)#define _ISR (i2c) ((i2c)->reg_isr)#define _ISAR (i2c) ((i2c)->reg_isar) +#define _ILCR (i2c) ((i2c)->reg_ilcr) +#define _IWCR (i2c) ((i2c)->reg_iwcr)/* * I2C Slave mode address
drivers/i2c/busses/i2c-pxa.c static void i2c_pxa_reset(struct pxa_i2c *i2c) · line 470
Quoted 4 lines
drivers/i2c/busses/i2c-pxa.c +0
-1@@ -467,11 +486,16 @@ static void i2c_pxa_reset(struct pxa_i2c *i2c)if (i2c->reg_isar)writel (i2c->slave_addr,_ISAR (i2c));#endif -
Not in this patch.
drivers/i2c/busses/i2c-pxa.c static void i2c_pxa_reset(struct pxa_i2c *i2c) · line 497
Quoted 9 lines
drivers/i2c/busses/i2c-pxa.c +5
-0/* set control register values */ writel (I2C_ICR_INIT | (i2c->fast_mode ?ICR_FM :0 ),_ICR (i2c));writel (readl (_ICR (i2c)) | (i2c->high_mode ?ICR_HS :0 ),_ICR (i2c)); +if (i2c->ilcr) +writel (i2c->ilcr,_ILCR (i2c)); +if (i2c->iwcr) +writel (i2c->iwcr,_IWCR (i2c)); +udelay (2 );
This is a magical 2us. Where does it come from ?
Cheers.
Signature · 1 line
Robert
PATCH 4/12 i2c: pxa: Add support for pxa910/988 & new configuration features
Quoted 9 lines · Robert Jarzmik
Quoted 7 lines · Vaibhav Hiremath
drivers/i2c/busses/i2c-pxa.c +2
-0@@ -167,6 +184,8 @@ struct pxa_i2c {#define _ICR (i2c) ((i2c)->reg_icr)#define _ISR (i2c) ((i2c)->reg_isr)#define _ISAR (i2c) ((i2c)->reg_isar) +#define _ILCR (i2c) ((i2c)->reg_ilcr) +#define _IWCR (i2c) ((i2c)->reg_iwcr)
Quoted 8 lines
drivers/i2c/busses/i2c-pxa.c static void i2c_pxa_reset(struct pxa_i2c *i2c) · line 470
Quoted 7 lines
drivers/i2c/busses/i2c-pxa.c +0
-1/* * I2C Slave mode address @@ -467,11 +486,16 @@ static void i2c_pxa_reset(struct pxa_i2c *i2c)if (i2c->reg_isar)writel (i2c->slave_addr,_ISAR (i2c));#endif -Not in this patch.
my bad, missed it.
Will fix it in next version.
Quoted 5 lines
Quoted 5 lines
drivers/i2c/busses/i2c-pxa.c +0
-0/* set control register values */ writel (I2C_ICR_INIT | (i2c->fast_mode ?ICR_FM :0 ),_ICR (i2c));writel (readl (_ICR (i2c)) | (i2c->high_mode ?ICR_HS :0 ),_ICR (i2c));
Quoted 7 lines
drivers/i2c/busses/i2c-pxa.c static void i2c_pxa_reset(struct pxa_i2c *i2c) · line 497
Quoted 5 lines
drivers/i2c/busses/i2c-pxa.c +5
-0+if (i2c->ilcr) +writel (i2c->ilcr,_ILCR (i2c)); +if (i2c->iwcr) +writel (i2c->iwcr,_IWCR (i2c)); +udelay (2 );This is a magical 2us. Where does it come from ?
I am afraid it is random number picked, required since we are configuring mode/speed related bit-fields.
Thanks,
Vaibhav
PATCH 4/12 i2c: pxa: Add support for pxa910/988 & new configuration features
In reply to Robert JarzmikQuoted 22 lines · Robert Jarzmik
Quoted 10 lines · Vaibhav Hiremath
drivers/i2c/busses/i2c-pxa.c +2
-0@@ -167,6 +184,8 @@ struct pxa_i2c {#define _ICR (i2c) ((i2c)->reg_icr)#define _ISR (i2c) ((i2c)->reg_isr)#define _ISAR (i2c) ((i2c)->reg_isar) +#define _ILCR (i2c) ((i2c)->reg_ilcr) +#define _IWCR (i2c) ((i2c)->reg_iwcr)/* * I2C Slave mode address drivers/i2c/busses/i2c-pxa.c static void i2c_pxa_reset(struct pxa_i2c *i2c) · line 470
Quoted 4 lines
drivers/i2c/busses/i2c-pxa.c +0
-1@@ -467,11 +486,16 @@ static void i2c_pxa_reset(struct pxa_i2c *i2c)if (i2c->reg_isar)writel (i2c->slave_addr,_ISAR (i2c));#endif -Not in this patch.
Quoted 4 lines
drivers/i2c/busses/i2c-pxa.c +0
-0/* set control register values */ writel (I2C_ICR_INIT | (i2c->fast_mode ?ICR_FM :0 ),_ICR (i2c));writel (readl (_ICR (i2c)) | (i2c->high_mode ?ICR_HS :0 ),_ICR (i2c));
Quoted 6 lines
drivers/i2c/busses/i2c-pxa.c static void i2c_pxa_reset(struct pxa_i2c *i2c) · line 497
Quoted 5 lines
drivers/i2c/busses/i2c-pxa.c +5
-0+if (i2c->ilcr) +writel (i2c->ilcr,_ILCR (i2c)); +if (i2c->iwcr) +writel (i2c->iwcr,_IWCR (i2c)); +udelay (2 );This is a magical 2us. Where does it come from ?
This delay can be removed, if writing LCR and WCR before enabling this i2c controller, it's safe enough; and they should not be changed when there is bus activity.
Quoted 5 lines
Cheers.
Signature · 1 line
Robert
PATCH 4/12 i2c: pxa: Add support for pxa910/988 & new configuration features
Quoted 30 lines · Yi Zhang
Quoted 22 lines · Robert Jarzmik
Quoted 10 lines · Vaibhav Hiremath
drivers/i2c/busses/i2c-pxa.c +2
-0@@ -167,6 +184,8 @@ struct pxa_i2c {#define _ICR (i2c) ((i2c)->reg_icr)#define _ISR (i2c) ((i2c)->reg_isr)#define _ISAR (i2c) ((i2c)->reg_isar) +#define _ILCR (i2c) ((i2c)->reg_ilcr) +#define _IWCR (i2c) ((i2c)->reg_iwcr)/* * I2C Slave mode address drivers/i2c/busses/i2c-pxa.c static void i2c_pxa_reset(struct pxa_i2c *i2c) · line 470
Quoted 4 lines
drivers/i2c/busses/i2c-pxa.c +0
-1@@ -467,11 +486,16 @@ static void i2c_pxa_reset(struct pxa_i2c *i2c)if (i2c->reg_isar)writel (i2c->slave_addr,_ISAR (i2c));#endif -Not in this patch.
Quoted 4 lines
drivers/i2c/busses/i2c-pxa.c +0
-0/* set control register values */ writel (I2C_ICR_INIT | (i2c->fast_mode ?ICR_FM :0 ),_ICR (i2c));writel (readl (_ICR (i2c)) | (i2c->high_mode ?ICR_HS :0 ),_ICR (i2c));Quoted 6 lines
drivers/i2c/busses/i2c-pxa.c static void i2c_pxa_reset(struct pxa_i2c *i2c) · line 497
Quoted 5 lines
drivers/i2c/busses/i2c-pxa.c +5
-0+if (i2c->ilcr) +writel (i2c->ilcr,_ILCR (i2c)); +if (i2c->iwcr) +writel (i2c->iwcr,_IWCR (i2c)); +udelay (2 );This is a magical 2us. Where does it come from ?
Quoted 3 lines
This delay can be removed, if writing LCR and WCR before enabling this i2c controller, it's safe enough; and they should not be changed when there is bus activity.
I have already removed it in my next version.
Thanks,
Vaibhav
PATCH 4/12 i2c: pxa: Add support for pxa910/988 & new configuration features
In reply to Vaibhav HiremathQuoted 8 lines · Vaibhav Hiremath
From: "Jett.Zhou" <[email protected]>
TWSI_ILCR&TWSI_IWCRregisters are used to adjust clock rate of standard & fast mode in pxa910/988; so this patch adds these two new
entries to "structpxa_reg_layout" and "structpxa_i2c" and also adds two
new DT properties to take configuration value for the same.
Mapping registers 1:1 to DT is nearly always wrong. We have some generic bindings for SCL/SDA tweaking. Check Documentation/devicetree/bindings/i2c/i2c-designware.txt for them and if they match your needs. (Yes, they should be in a more generic place).
PATCH 4/12 i2c: pxa: Add support for pxa910/988 & new configuration features
Quoted 8 lines · Wolfram Sang
Quoted 6 lines · Vaibhav Hiremath
From: "Jett.Zhou" <[email protected]>
TWSI_ILCR&TWSI_IWCRregisters are used to adjust clock rate of standard & fast mode in pxa910/988; so this patch adds these two new entries to "structpxa_reg_layout" and "structpxa_i2c" and also adds two new DT properties to take configuration value for the same.
Quoted 6 lines
Mapping registers 1:1 to DT is nearly always wrong. We have some generic bindings for SCL/SDA tweaking. Check
Documentation/devicetree/bindings/i2c/i2c-designware.txtfor them and if they match your needs. (Yes, they should be in a more generic place).
Thanks for your suggestion.
Let me check above documentation.
Thanks,
Vaibhav
PATCH 5/12 i2c: pxa: Add bus reset functionality
In reply to Vaibhav HiremathFrom: Rob Herring <[email protected]>
Since there is some problematic i2c slave devices on some platforms such as dkb (sometimes), it will drop down sda and make i2c bus hang, at that time, it need to config scl/sda into gpio to simulate "stop" sequence to recover i2c bus, so add this interface.
--- drivers/i2c/busses/i2c-pxa.c | 90 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 90 insertions(+)
drivers/i2c/busses/i2c-pxa.c +90 -0
@@ -37,6 +37,8 @@#include <linux/slab.h> #include <linux/io.h> #include <linux/i2c/pxa-i2c.h> +#include <linux/of_gpio.h> +#include <linux/pinctrl/consumer.h> #include <asm/irq.h>
@@ -177,6 +179,9 @@ struct pxa_i2c {bool highmode_enter;unsigned int ilcr;unsigned int iwcr; +struct pinctrl *pinctrl; +struct pinctrl_state *pin_i2c; +struct pinctrl_state *pin_gpio; };#define _IBMR (i2c) ((i2c)->reg_ibmr)
@@ -269,6 +274,62 @@ static void i2c_pxa_show_state(struct pxa_i2c *i2c, int lno, const char *fname)#define show_state (i2c) i2c_pxa_show_state(i2c, __LINE__, __func__) +static void i2c_bus_reset (struct pxa_i2c *i2c) +{ +int ret, ccnt, pins_scl, pins_sda; +struct device *dev = i2c->adap.dev.parent; +struct device_node *np = dev->of_node; + +if (!i2c->pinctrl) { +dev_warn (dev,"could not do i2c bus reset\n" ); +return ; + } + + ret =pinctrl_select_state (i2c->pinctrl, i2c->pin_gpio); +if (ret) { +dev_err (dev,"could not set gpio pins\n" ); +return ; + } + + pins_scl =of_get_named_gpio (np,"i2c-gpio" ,0 ); +if (!gpio_is_valid (pins_scl)) { +dev_err (dev,"i2c scl gpio not set\n" ); + goto err_out; + } + pins_sda =of_get_named_gpio (np,"i2c-gpio" ,1 ); +if (!gpio_is_valid (pins_sda)) { +dev_err (dev,"i2c sda gpio not set\n" ); + goto err_out; + } + +gpio_request (pins_scl,NULL ); +gpio_request (pins_sda,NULL ); + +gpio_direction_input (pins_sda); +for (ccnt =20 ; ccnt; ccnt--) { +gpio_direction_output (pins_scl, ccnt &0x01 ); +udelay (5 ); + } +gpio_direction_output (pins_scl,0 ); +udelay (5 ); +gpio_direction_output (pins_sda,0 ); +udelay (5 ); +/* stop signal */ +gpio_direction_output (pins_scl,1 ); +udelay (5 ); +gpio_direction_output (pins_sda,1 ); +udelay (5 ); + +gpio_free (pins_scl); +gpio_free (pins_sda); + +err_out: + ret =pinctrl_select_state (i2c->pinctrl, i2c->pin_i2c); +if (ret) +dev_err (dev,"could not set default(i2c) pins\n" ); +return ; +} +static void i2c_pxa_scream_blue_murder (struct pxa_i2c *i2c,const char *why) {unsigned int i;
@@ -281,6 +342,11 @@ static void i2c_pxa_scream_blue_murder(struct pxa_i2c *i2c, const char *why)for (i =0 ; i < i2c->irqlogidx; i++)printk ("[%08x:%08x] " , i2c->isrlog[i], i2c->icrlog[i]);printk ("\n" ); +if (strcmp (why,"exhausted retries" ) !=0 ) { +i2c_bus_reset (i2c); +/* reset i2c contorler when it's fail */ +i2c_pxa_reset (i2c); + } }#else /* ifdef DEBUG */
@@ -1301,6 +1367,30 @@ static int i2c_pxa_probe(struct platform_device *dev)platform_set_drvdata (dev, i2c); + i2c->pinctrl =devm_pinctrl_get (&dev->dev); +if (IS_ERR (i2c->pinctrl)) { + i2c->pinctrl =NULL ; +dev_warn (&dev->dev,"could not get pinctrl\n" ); + }else { + i2c->pin_i2c =pinctrl_lookup_state (i2c->pinctrl,"default" ); +if (IS_ERR (i2c->pin_i2c)) { +dev_err (&dev->dev,"could not get default(i2c) pinstate\n" ); + ret =IS_ERR (i2c->pin_i2c); + } + + i2c->pin_gpio =pinctrl_lookup_state (i2c->pinctrl,"gpio" ); +if (IS_ERR (i2c->pin_gpio)) { +dev_err (&dev->dev,"could not get gpio pinstate\n" ); + ret =IS_ERR (i2c->pin_gpio); + } + +if (ret) { + i2c->pin_i2c =NULL ; + i2c->pin_gpio =NULL ; + i2c->pinctrl =NULL ; + ret =0 ; + } + }#ifdef CONFIG_I2C_PXA_SLAVE printk (KERN_INFO "I2C: %s: PXA I2C adapter, slave address %d\n" ,dev_name (&i2c->adap.dev), i2c->slave_addr);
Signature · 1 line
1.9.1
Rob Herring
maintainerPATCH 5/12 i2c: pxa: Add bus reset functionality
Quoted 1 line · Vaibhav Hiremath
From: Rob Herring <[email protected]>
This probably should still be Leilei, but...
Quoted 40 lines
Since there is some problematic i2c slave devices on some platforms such as dkb (sometimes), it will drop down sda and make i2c bus hang, at that time, it need to config scl/sda into gpio to simulate "stop" sequence to recover i2c bus, so add this interface.
Signed-off-by Leilei Shang <[email protected]>Signed-off-by Rob Herring <[email protected]>[vaibhav.hiremath at linaro.org: Updated Changelog]Signed-off-by Vaibhav Hiremath <[email protected]>Signed-off-by Vaibhav Hiremath <[email protected]>--- drivers/i2c/busses/i2c-pxa.c | 90 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 90 insertions(+)drivers/i2c/busses/i2c-pxa.c +5
-0@@ -37,6 +37,8 @@#include <linux/slab.h> #include <linux/io.h> #include <linux/i2c/pxa-i2c.h> +#include <linux/of_gpio.h> +#include <linux/pinctrl/consumer.h> #include <asm/irq.h> @@ -177,6 +179,9 @@ struct pxa_i2c {bool highmode_enter;unsigned int ilcr;unsigned int iwcr; +struct pinctrl *pinctrl; +struct pinctrl_state *pin_i2c; +struct pinctrl_state *pin_gpio; };#define _IBMR (i2c) ((i2c)->reg_ibmr)
drivers/i2c/busses/i2c-pxa.c static void i2c_pxa_show_state(struct pxa_i2c *i2c, int lno, const char *fname) · line 277
Quoted 4 lines
drivers/i2c/busses/i2c-pxa.c +1
-0@@ -269,6 +274,62 @@ static void i2c_pxa_show_state(struct pxa_i2c *i2c, int lno, const char *fname)#define show_state (i2c) i2c_pxa_show_state(i2c, __LINE__, __func__) +static void i2c_bus_reset (struct pxa_i2c *i2c)
There's a generic mechanism in i2c_generic_gpio_recovery we should use here. It appears to be similar, but not exactly the same. The pinctrl part should probably be done by gpio driver.
Rob
PATCH 5/12 i2c: pxa: Add bus reset functionality
Quoted 6 lines · Rob Herring
Quoted 1 line · Vaibhav Hiremath
From: Rob Herring <[email protected]>
This probably should still be Leilei, but...
Ok, Since I am taking forward from your sign-off, did not change it. Anyway will fix in next version.
Quoted 41 lines
Quoted 41 lines
Since there is some problematic i2c slave devices on some platforms such as dkb (sometimes), it will drop down sda and make i2c bus hang, at that time, it need to config scl/sda into gpio to simulate "stop" sequence to recover i2c bus, so add this interface.
Signed-off-by Leilei Shang <[email protected]>Signed-off-by Rob Herring <[email protected]>[vaibhav.hiremath at linaro.org: Updated Changelog]Signed-off-by Vaibhav Hiremath <[email protected]>Signed-off-by Vaibhav Hiremath <[email protected]>--- drivers/i2c/busses/i2c-pxa.c | 90 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 90 insertions(+)drivers/i2c/busses/i2c-pxa.c +5
-0@@ -37,6 +37,8 @@#include <linux/slab.h> #include <linux/io.h> #include <linux/i2c/pxa-i2c.h> +#include <linux/of_gpio.h> +#include <linux/pinctrl/consumer.h> #include <asm/irq.h> @@ -177,6 +179,9 @@ struct pxa_i2c {bool highmode_enter;unsigned int ilcr;unsigned int iwcr; +struct pinctrl *pinctrl; +struct pinctrl_state *pin_i2c; +struct pinctrl_state *pin_gpio; };#define _IBMR (i2c) ((i2c)->reg_ibmr)@@ -269,6 +274,62 @@ static void i2c_pxa_show_state(struct pxa_i2c *i2c, int lno, const char *fname)
Quoted 8 lines
drivers/i2c/busses/i2c-pxa.c static void i2c_pxa_show_state(struct pxa_i2c *i2c, int lno, const char *fname) · line 277
Quoted 3 lines
drivers/i2c/busses/i2c-pxa.c +1
-0#define show_state (i2c) i2c_pxa_show_state(i2c, __LINE__, __func__) +static void i2c_bus_reset (struct pxa_i2c *i2c)There's a generic mechanism in
i2c_generic_gpio_recoverywe should use here. It appears to be similar, but not exactly the same. The pinctrl part should probably be done by gpio driver.
Good point.
As you mentioned, they are not exactly same.
But we can achieve exactly what we wanted using prepare & unprepare callbacks.
Generate stop signal in unprepare() callback should suffice our need.
But I am not quite sure about pinctrl handling by gpio driver. I was tracing the calls from gpio_request_one()anf gpio_free() but it seems they are not bringing back pins to default state.
correct me if I am wrong.
Thanks,
Vaibhav
PATCH 5/12 i2c: pxa: Add bus reset functionality
In reply to Vaibhav HiremathQuoted 8 lines · Vaibhav Hiremath
From: Rob Herring <[email protected]>
Since there is some problematic i2c slave devices on some platforms such as dkb (sometimes), it will drop down sda and make i2c bus hang, at that time, it need to config scl/sda into gpio to simulate "stop" sequence to recover i2c bus, so add this interface.
Quoted 6 lines
Signed-off-by Leilei Shang <[email protected]>Signed-off-by Rob Herring <[email protected]>[vaibhav.hiremath at linaro.org: Updated Changelog]Signed-off-by Vaibhav Hiremath <[email protected]>Signed-off-by Vaibhav Hiremath <[email protected]>
I'm not enthusiastic about this patch.
Linus, would you please place a comment on this patch ?
My personal feeling :
- all the udelays are just ugly
- the pinctrl binding into i2c-pxa doesn't look good to me Linus will probably comment on that
- all of this patch looks like a workaround for a single platform "poor" support. If this is dkb specific (what is dkb BTW ?), can't it be hooked into this driver, but placed in dkb platform code ?
Cheers.
--
Robert
Quoted 133 lines
--- drivers/i2c/busses/i2c-pxa.c | 90 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 90 insertions(+)drivers/i2c/busses/i2c-pxa.c +90
-0@@ -37,6 +37,8 @@#include <linux/slab.h> #include <linux/io.h> #include <linux/i2c/pxa-i2c.h> +#include <linux/of_gpio.h> +#include <linux/pinctrl/consumer.h> #include <asm/irq.h> @@ -177,6 +179,9 @@ struct pxa_i2c {bool highmode_enter;unsigned int ilcr;unsigned int iwcr; +struct pinctrl *pinctrl; +struct pinctrl_state *pin_i2c; +struct pinctrl_state *pin_gpio; };#define _IBMR (i2c) ((i2c)->reg_ibmr)@@ -269,6 +274,62 @@ static void i2c_pxa_show_state(struct pxa_i2c *i2c, int lno, const char *fname)#define show_state (i2c) i2c_pxa_show_state(i2c, __LINE__, __func__) +static void i2c_bus_reset (struct pxa_i2c *i2c) +{ +int ret, ccnt, pins_scl, pins_sda; +struct device *dev = i2c->adap.dev.parent; +struct device_node *np = dev->of_node; + +if (!i2c->pinctrl) { +dev_warn (dev,"could not do i2c bus reset\n" ); +return ; + } + + ret =pinctrl_select_state (i2c->pinctrl, i2c->pin_gpio); +if (ret) { +dev_err (dev,"could not set gpio pins\n" ); +return ; + } + + pins_scl =of_get_named_gpio (np,"i2c-gpio" ,0 ); +if (!gpio_is_valid (pins_scl)) { +dev_err (dev,"i2c scl gpio not set\n" ); + goto err_out; + } + pins_sda =of_get_named_gpio (np,"i2c-gpio" ,1 ); +if (!gpio_is_valid (pins_sda)) { +dev_err (dev,"i2c sda gpio not set\n" ); + goto err_out; + } + +gpio_request (pins_scl,NULL ); +gpio_request (pins_sda,NULL ); + +gpio_direction_input (pins_sda); +for (ccnt =20 ; ccnt; ccnt--) { +gpio_direction_output (pins_scl, ccnt &0x01 ); +udelay (5 ); + } +gpio_direction_output (pins_scl,0 ); +udelay (5 ); +gpio_direction_output (pins_sda,0 ); +udelay (5 ); +/* stop signal */ +gpio_direction_output (pins_scl,1 ); +udelay (5 ); +gpio_direction_output (pins_sda,1 ); +udelay (5 ); + +gpio_free (pins_scl); +gpio_free (pins_sda); + +err_out: + ret =pinctrl_select_state (i2c->pinctrl, i2c->pin_i2c); +if (ret) +dev_err (dev,"could not set default(i2c) pins\n" ); +return ; +} +static void i2c_pxa_scream_blue_murder (struct pxa_i2c *i2c,const char *why) {unsigned int i;@@ -281,6 +342,11 @@ static void i2c_pxa_scream_blue_murder(struct pxa_i2c *i2c, const char *why)for (i =0 ; i < i2c->irqlogidx; i++)printk ("[%08x:%08x] " , i2c->isrlog[i], i2c->icrlog[i]);printk ("\n" ); +if (strcmp (why,"exhausted retries" ) !=0 ) { +i2c_bus_reset (i2c); +/* reset i2c contorler when it's fail */ +i2c_pxa_reset (i2c); + } }#else /* ifdef DEBUG */ @@ -1301,6 +1367,30 @@ static int i2c_pxa_probe(struct platform_device *dev)platform_set_drvdata (dev, i2c); + i2c->pinctrl =devm_pinctrl_get (&dev->dev); +if (IS_ERR (i2c->pinctrl)) { + i2c->pinctrl =NULL ; +dev_warn (&dev->dev,"could not get pinctrl\n" ); + }else { + i2c->pin_i2c =pinctrl_lookup_state (i2c->pinctrl,"default" ); +if (IS_ERR (i2c->pin_i2c)) { +dev_err (&dev->dev,"could not get default(i2c) pinstate\n" ); + ret =IS_ERR (i2c->pin_i2c); + } + + i2c->pin_gpio =pinctrl_lookup_state (i2c->pinctrl,"gpio" ); +if (IS_ERR (i2c->pin_gpio)) { +dev_err (&dev->dev,"could not get gpio pinstate\n" ); + ret =IS_ERR (i2c->pin_gpio); + } + +if (ret) { + i2c->pin_i2c =NULL ; + i2c->pin_gpio =NULL ; + i2c->pinctrl =NULL ; + ret =0 ; + } + }#ifdef CONFIG_I2C_PXA_SLAVE printk (KERN_INFO "I2C: %s: PXA I2C adapter, slave address %d\n" ,dev_name (&i2c->adap.dev), i2c->slave_addr);
Signature · 1 line
Robert
PATCH 5/12 i2c: pxa: Add bus reset functionality
In reply to Vaibhav HiremathQuoted 8 lines · Vaibhav Hiremath
From: Rob Herring <[email protected]>
Since there is some problematic i2c slave devices on some platforms such as dkb (sometimes), it will drop down sda and make i2c bus hang, at that time, it need to config scl/sda into gpio to simulate "stop" sequence to recover i2c bus, so add this interface.
Quoted 6 lines
Signed-off-by Leilei Shang <[email protected]>Signed-off-by Rob Herring <[email protected]>[vaibhav.hiremath at linaro.org: Updated Changelog]Signed-off-by Vaibhav Hiremath <[email protected]>Signed-off-by Vaibhav Hiremath <[email protected]>
Double signed-off?
(...)
+#include <linux/of_gpio.h>
You should use <linux/gpio/consumer.h> and then use GPIO descriptors instead.
drivers/i2c/busses/i2c-pxa.c struct pxa_i2c { · line 184
Quoted 7 lines
drivers/i2c/busses/i2c-pxa.c +3
-0@@ -177,6 +179,9 @@ struct pxa_i2c {bool highmode_enter;unsigned int ilcr;unsigned int iwcr; +struct pinctrl *pinctrl; +struct pinctrl_state *pin_i2c; +struct pinctrl_state *pin_gpio;
Yup that's the right way. I see this is the "default" state for pin_i2c.
drivers/i2c/busses/i2c-pxa.c static void i2c_pxa_show_state(struct pxa_i2c *i2c, int lno, const char *fname) · line 279
Quoted 3 lines
drivers/i2c/busses/i2c-pxa.c +3
-0+static void i2c_bus_reset (struct pxa_i2c *i2c) +{ +int ret, ccnt, pins_scl, pins_sda;
Use GPIO descriptors.
struct gpio_desc *scl, *sda;
drivers/i2c/busses/i2c-pxa.c static void i2c_pxa_show_state(struct pxa_i2c *i2c, int lno, const char *fname) · line 292
Quoted 13 lines
drivers/i2c/busses/i2c-pxa.c +13
-0+struct device *dev = i2c->adap.dev.parent; +struct device_node *np = dev->of_node; + +if (!i2c->pinctrl) { +dev_warn (dev,"could not do i2c bus reset\n" ); +return ; + } + + ret =pinctrl_select_state (i2c->pinctrl, i2c->pin_gpio); +if (ret) { +dev_err (dev,"could not set gpio pins\n" ); +return ; + }
Exactly like that yes.
drivers/i2c/busses/i2c-pxa.c static void i2c_pxa_show_state(struct pxa_i2c *i2c, int lno, const char *fname) · line 303
Quoted 10 lines
drivers/i2c/busses/i2c-pxa.c +10
-0+ pins_scl =of_get_named_gpio (np,"i2c-gpio" ,0 ); +if (!gpio_is_valid (pins_scl)) { +dev_err (dev,"i2c scl gpio not set\n" ); + goto err_out; + } + pins_sda =of_get_named_gpio (np,"i2c-gpio" ,1 ); +if (!gpio_is_valid (pins_sda)) { +dev_err (dev,"i2c sda gpio not set\n" ); + goto err_out; + }
I would suggest just using two GPIOs in the node relying on index order. With GPIO descriptors:
scl = gpiod_get_index(dev, "i2c-gpio", 0, GPIOD_ASIS); sda = gpiod_get_index(dev, "i2c-gpio", 1, GPIOD_ASIS);
Then use gpiod* accessors below and...
Quoted 11 lines
drivers/i2c/busses/i2c-pxa.c +11
-0+ +gpio_request (pins_scl,NULL ); +gpio_request (pins_sda,NULL ); + +gpio_direction_input (pins_sda); +for (ccnt =20 ; ccnt; ccnt--) { +gpio_direction_output (pins_scl, ccnt &0x01 ); +udelay (5 ); + } +gpio_direction_output (pins_scl,0 ); +udelay (5 );
drivers/i2c/busses/i2c-pxa.c static void i2c_pxa_show_state(struct pxa_i2c *i2c, int lno, const char *fname) · line 324
Quoted 10 lines
drivers/i2c/busses/i2c-pxa.c +10
-0+gpio_direction_output (pins_sda,0 ); +udelay (5 ); +/* stop signal */ +gpio_direction_output (pins_scl,1 ); +udelay (5 ); +gpio_direction_output (pins_sda,1 ); +udelay (5 ); + +gpio_free (pins_scl); +gpio_free (pins_sda);
gpiod_put(scl);gpiod_put(sda);
drivers/i2c/busses/i2c-pxa.c static void i2c_pxa_show_state(struct pxa_i2c *i2c, int lno, const char *fname) · line 330
Quoted 5 lines
drivers/i2c/busses/i2c-pxa.c +5
-0+err_out: + ret =pinctrl_select_state (i2c->pinctrl, i2c->pin_i2c); +if (ret) +dev_err (dev,"could not set default(i2c) pins\n" ); +return ;
Nice.
Overall it looks like a real nice structured workaround using the API exactly as intended, just need to catch up with using GPIO descriptors.
Yours,
Linus Walleij
PATCH 5/12 i2c: pxa: Add bus reset functionality
Quoted 111 lines · Linus Walleij
Quoted 8 lines · Vaibhav Hiremath
From: Rob Herring <[email protected]>
Since there is some problematic i2c slave devices on some platforms such as dkb (sometimes), it will drop down sda and make i2c bus hang, at that time, it need to config scl/sda into gpio to simulate "stop" sequence to recover i2c bus, so add this interface.
Quoted 6 lines
Signed-off-by Leilei Shang <[email protected]>Signed-off-by Rob Herring <[email protected]>[vaibhav.hiremath at linaro.org: Updated Changelog]Signed-off-by Vaibhav Hiremath <[email protected]>Signed-off-by Vaibhav Hiremath <[email protected]>Double signed-off?
(...)
+#include <linux/of_gpio.h>You should use <
linux/gpio/consumer.h> and then use GPIO descriptors instead.drivers/i2c/busses/i2c-pxa.c struct pxa_i2c { · line 184
Quoted 7 lines
drivers/i2c/busses/i2c-pxa.c +3
-0@@ -177,6 +179,9 @@ struct pxa_i2c {bool highmode_enter;unsigned int ilcr;unsigned int iwcr; +struct pinctrl *pinctrl; +struct pinctrl_state *pin_i2c; +struct pinctrl_state *pin_gpio;Yup that's the right way. I see this is the "default" state for
pin_i2c.drivers/i2c/busses/i2c-pxa.c static void i2c_pxa_show_state(struct pxa_i2c *i2c, int lno, const char *fname) · line 279
Quoted 3 lines
drivers/i2c/busses/i2c-pxa.c +3
-0+static void i2c_bus_reset (struct pxa_i2c *i2c) +{ +int ret, ccnt, pins_scl, pins_sda;Use GPIO descriptors.
struct
gpio_desc*scl, *sda;drivers/i2c/busses/i2c-pxa.c static void i2c_pxa_show_state(struct pxa_i2c *i2c, int lno, const char *fname) · line 292
Quoted 13 lines
drivers/i2c/busses/i2c-pxa.c +13
-0+struct device *dev = i2c->adap.dev.parent; +struct device_node *np = dev->of_node; + +if (!i2c->pinctrl) { +dev_warn (dev,"could not do i2c bus reset\n" ); +return ; + } + + ret =pinctrl_select_state (i2c->pinctrl, i2c->pin_gpio); +if (ret) { +dev_err (dev,"could not set gpio pins\n" ); +return ; + }Exactly like that yes.
drivers/i2c/busses/i2c-pxa.c static void i2c_pxa_show_state(struct pxa_i2c *i2c, int lno, const char *fname) · line 303
Quoted 10 lines
drivers/i2c/busses/i2c-pxa.c +10
-0+ pins_scl =of_get_named_gpio (np,"i2c-gpio" ,0 ); +if (!gpio_is_valid (pins_scl)) { +dev_err (dev,"i2c scl gpio not set\n" ); + goto err_out; + } + pins_sda =of_get_named_gpio (np,"i2c-gpio" ,1 ); +if (!gpio_is_valid (pins_sda)) { +dev_err (dev,"i2c sda gpio not set\n" ); + goto err_out; + }I would suggest just using two GPIOs in the node relying on index order. With GPIO descriptors:
scl =
gpiod_get_index(dev, "i2c-gpio", 0, GPIOD_ASIS); sda =gpiod_get_index(dev, "i2c-gpio", 1, GPIOD_ASIS);Then use gpiod* accessors below and...
Quoted 11 lines
drivers/i2c/busses/i2c-pxa.c +11
-0+ +gpio_request (pins_scl,NULL ); +gpio_request (pins_sda,NULL ); + +gpio_direction_input (pins_sda); +for (ccnt =20 ; ccnt; ccnt--) { +gpio_direction_output (pins_scl, ccnt &0x01 ); +udelay (5 ); + } +gpio_direction_output (pins_scl,0 ); +udelay (5 );drivers/i2c/busses/i2c-pxa.c static void i2c_pxa_show_state(struct pxa_i2c *i2c, int lno, const char *fname) · line 324
Quoted 10 lines
drivers/i2c/busses/i2c-pxa.c +10
-0+gpio_direction_output (pins_sda,0 ); +udelay (5 ); +/* stop signal */ +gpio_direction_output (pins_scl,1 ); +udelay (5 ); +gpio_direction_output (pins_sda,1 ); +udelay (5 ); + +gpio_free (pins_scl); +gpio_free (pins_sda);
gpiod_put(scl);gpiod_put(sda);drivers/i2c/busses/i2c-pxa.c static void i2c_pxa_show_state(struct pxa_i2c *i2c, int lno, const char *fname) · line 330
Quoted 5 lines
drivers/i2c/busses/i2c-pxa.c +5
-0+err_out: + ret =pinctrl_select_state (i2c->pinctrl, i2c->pin_i2c); +if (ret) +dev_err (dev,"could not set default(i2c) pins\n" ); +return ;
Quoted 6 lines
Nice.
Overall it looks like a real nice structured workaround using the API exactly as intended, just need to catch up with using GPIO descriptors.
Thanks Linus,
I will work on this and submit V2 shortly.
Thanks,
Vaibhav
PATCH 5/12 i2c: pxa: Add bus reset functionality
In reply to Linus WalleijQuoted 11 lines · Linus Walleij
Quoted 8 lines · Vaibhav Hiremath
From: Rob Herring <[email protected]>
Since there is some problematic i2c slave devices on some platforms such as dkb (sometimes), it will drop down sda and make i2c bus hang, at that time, it need to config scl/sda into gpio to simulate "stop" sequence to recover i2c bus, so add this interface.
Quoted 9 lines
Quoted 6 lines
Signed-off-by Leilei Shang <[email protected]>Signed-off-by Rob Herring <[email protected]>[vaibhav.hiremath at linaro.org: Updated Changelog]Signed-off-by Vaibhav Hiremath <[email protected]>Signed-off-by Vaibhav Hiremath <[email protected]>Double signed-off?
Thanks for your review.
Based on review comments on the list, i will have to change this patch to use generic gpio reset implementation available in i2c-core.
Having said that, I am still sure whether GPIO lib (request/free) would handle pinctrl configuration.
Thanks,
vaibhav
PATCH 5/12 i2c: pxa: Add bus reset functionality
In reply to Vaibhav HiremathQuoted 6 lines
Since there is some problematic i2c slave devices on some platforms such as dkb (sometimes), it will drop down sda and make i2c bus hang, at that time, it need to config scl/sda into gpio to simulate "stop" sequence to recover i2c bus, so add this interface.
Please note that the i2c core has a bus recovery infrastructure. Search i2c.h for struct i2c_bus_recovery_info.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150603/352f0bd0/attachment.sig>
PATCH 5/12 i2c: pxa: Add bus reset functionality
Quoted 10 lines · Wolfram Sang
Quoted 6 lines
Since there is some problematic i2c slave devices on some platforms such as dkb (sometimes), it will drop down sda and make i2c bus hang, at that time, it need to config scl/sda into gpio to simulate "stop" sequence to recover i2c bus, so add this interface.
Please note that the i2c core has a bus recovery infrastructure. Search
i2c.hfor structi2c_bus_recovery_info.
Yeah,
I assume you are referring to "i2c_generic_gpio_recovery", right?
Rob had suggested this, and will be using it in my next version.
Thanks,
Vaibhav
PATCH 5/12 i2c: pxa: Add bus reset functionality
Quoted 3 lines
I assume you are referring to "
i2c_generic_gpio_recovery", right?Rob had suggested this, and will be using it in my next version.
Great, thanks!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150603/786cb3d1/attachment.sig>
PATCH 5/12 i2c: pxa: Add bus reset functionality
Quoted 6 lines · Wolfram Sang
Quoted 3 lines
I assume you are referring to "
i2c_generic_gpio_recovery", right?Rob had suggested this, and will be using it in my next version.
Great, thanks!
can you please also review the RFC which I submitted few days back?
http://www.spinics.net/lists/linux-i2c/msg19719.html
And also,
http://www.spinics.net/lists/arm-kernel/msg422034.html
Thanks,
Vaibhav
PATCH 5/12 i2c: pxa: Add bus reset functionality
Quoted 2 lines
can you please also review the RFC which I submitted few days back?
They are added to the queue and will be reviewed when the time comes. I won't make any promises.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150603/8a94d773/attachment.sig>
PATCH 5/12 i2c: pxa: Add bus reset functionality
Quoted 6 lines · Wolfram Sang
Quoted 2 lines
can you please also review the RFC which I submitted few days back?
They are added to the queue and will be reviewed when the time comes. I won't make any promises.
Ok, take your time.
Just wanted to make sure that they don't get buried under all email traffic.
Anyway, thanks for your review.
Thanks,
Vaibhav
PATCH 5/12 i2c: pxa: Add bus reset functionality
Can you resend the hwlock patches? I use patchwork for tracking and
those were sent when the server had a little downtime. Thanks!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150603/7edb4447/attachment.sig>
PATCH 5/12 i2c: pxa: Add bus reset functionality
Quoted 3 lines · Wolfram Sang
Can you resend the hwlock patches? I use patchwork for tracking and those were sent when the server had a little downtime. Thanks!
Just resent it. please check.
Thanks,
Vaibhav
PATCH 6/12 i2c: pxa: Return I2C_RETRY when timeout in pio mode
In reply to Vaibhav HiremathFrom: Shouming Wang <[email protected]>
In case of timeout in pio mode of operation return I2C_RETRY. This behavior will be same as interrupt mode of operation.
--- drivers/i2c/busses/i2c-pxa.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/i2c/busses/i2c-pxa.c +3 -1
@@ -841,8 +841,10 @@ static int i2c_pxa_do_pio_xfer(struct pxa_i2c *i2c, ret = i2c->msg_idx; out: -if (timeout ==0 ) +if (timeout ==0 ) {i2c_pxa_scream_blue_murder (i2c,"timeout" ); + ret =I2C_RETRY ; + }return ret; }
Signature · 1 line
1.9.1
PATCH 6/12 i2c: pxa: Return I2C_RETRY when timeout in pio mode
Quoted 19 lines · Vaibhav Hiremath
From: Shouming Wang <[email protected]>
In case of timeout in pio mode of operation return
I2C_RETRY. This behavior will be same as interrupt mode of operation.Signed-off-by Shouming Wang <[email protected]>[vaibhav.hiremath at linaro.org: Updated changelog]Signed-off-by Vaibhav Hiremath <[email protected]>Signed-off-by Vaibhav Hiremath <[email protected]>--- drivers/i2c/busses/i2c-pxa.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)drivers/i2c/busses/i2c-pxa.c +0
-0
drivers/i2c/busses/i2c-pxa.c static int i2c_pxa_do_pio_xfer(struct pxa_i2c *i2c, · line 847
Quoted 8 lines
drivers/i2c/busses/i2c-pxa.c +3
-1@@ -841,8 +841,10 @@ static int i2c_pxa_do_pio_xfer(struct pxa_i2c *i2c, ret = i2c->msg_idx; out: -if (timeout ==0 ) +if (timeout ==0 ) {i2c_pxa_scream_blue_murder (i2c,"timeout" ); + ret =I2C_RETRY ; + }
Ok, looks good to me.
As it changes the dynamic behavior of i2c_pxa_pio_xfer(), I'd like to know how it was tested (on which platform, and what was on the I2C bus).
Cheers.
Signature · 1 line
Robert
PATCH 6/12 i2c: pxa: Return I2C_RETRY when timeout in pio mode
Quoted 23 lines · Robert Jarzmik
Quoted 21 lines · Vaibhav Hiremath
From: Shouming Wang <[email protected]>
In case of timeout in pio mode of operation return
I2C_RETRY. This behavior will be same as interrupt mode of operation.Signed-off-by Shouming Wang <[email protected]>[vaibhav.hiremath at linaro.org: Updated changelog]Signed-off-by Vaibhav Hiremath <[email protected]>Signed-off-by Vaibhav Hiremath <[email protected]>--- drivers/i2c/busses/i2c-pxa.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)drivers/i2c/busses/i2c-pxa.c +0
-0@@ -841,8 +841,10 @@ static int i2c_pxa_do_pio_xfer(struct pxa_i2c *i2c, ret = i2c->msg_idx;
Quoted 10 lines
drivers/i2c/busses/i2c-pxa.c static int i2c_pxa_do_pio_xfer(struct pxa_i2c *i2c, · line 847
Quoted 6 lines
drivers/i2c/busses/i2c-pxa.c +3
-1out: -if (timeout ==0 ) +if (timeout ==0 ) {i2c_pxa_scream_blue_murder (i2c,"timeout" ); + ret =I2C_RETRY ; + }Ok, looks good to me.
As it changes the dynamic behavior ofi2c_pxa_pio_xfer(), I'd like to know how it was tested (on which platform, and what was on the I2C bus).
I am testing on PXA1928 based platform, which still not available fully on Mainline.
I have PMIC 88PM860 connected to I2C bus, with very basic features enabled. So I do probe/read/write testing.
Having said that, I still did not test RETRY path, to make sure that RETRY really happens. As I mentioned, everything is under developement, I just enabled PMIC support on my baseline to validate I2C driver.
Thanks,
Vaibhav
PATCH 7/12 i2c: pxa: Reset i2c controller on timeout in interrupt and pio mode
In reply to Vaibhav HiremathIn case of timeout during msg xfer assert reset to i2c controller for both interrupt and PIO mode of operation.
[vaibhav.hiremath at linaro.org: Split & merge patches into logical changes and update the Changelog]
--- drivers/i2c/busses/i2c-pxa.c | 8 ++++++++ 1 file changed, 8 insertions(+)
drivers/i2c/busses/i2c-pxa.c +8 -0
@@ -197,6 +197,8 @@ struct pxa_i2c {*/ #define I2C_PXA_SLAVE_ADDR 0x1 +static void i2c_pxa_reset (struct pxa_i2c *i2c); +#ifdef DEBUG struct bits {
@@ -846,6 +848,9 @@ out: ret =I2C_RETRY ; } +if (ret <0 ) +i2c_pxa_reset (i2c); +return ret; }
@@ -912,6 +917,9 @@ static int i2c_pxa_do_xfer(struct pxa_i2c *i2c, struct i2c_msg *msg, int num) } out: +if (ret <0 ) +i2c_pxa_reset (i2c); +return ret; }
Signature · 1 line
1.9.1
PATCH 7/12 i2c: pxa: Reset i2c controller on timeout in interrupt and pio mode
Quoted 9 lines · Vaibhav Hiremath
In case of timeout during msg xfer assert reset to i2c controller for both interrupt and PIO mode of operation.
Signed-off-by Jett.Zhou <[email protected]>[vaibhav.hiremath at linaro.org: Split & merge patches into logical changes and update the Changelog]
Signed-off-by Vaibhav Hiremath <[email protected]>Signed-off-by Vaibhav Hiremath <[email protected]>
I have the same comment as before.
I don't like a reset in the transfer path, especially in normal busy phases for slow I2C devices. A quirk as before.
Cheers.
--
Robert
PATCH 7/12 i2c: pxa: Reset i2c controller on timeout in interrupt and pio mode
Quoted 10 lines · Robert Jarzmik
Quoted 8 lines · Vaibhav Hiremath
In case of timeout during msg xfer assert reset to i2c controller for both interrupt and PIO mode of operation.
Signed-off-by Jett.Zhou <[email protected]>[vaibhav.hiremath at linaro.org: Split & merge patches into logical changes and update the Changelog]
Signed-off-by Vaibhav Hiremath <[email protected]>
Quoted 5 lines
Quoted 1 line
Signed-off-by Vaibhav Hiremath <[email protected]>I have the same comment as before.
I don't like a reset in the transfer path, especially in normal busy phases for slow I2C devices. A quirk as before.
Note that this assertion of reset in in case of timeout error.
Timeout error may be due to various reasons, and this is recovery mechanism.
Thanks,
Vaibhav
PATCH 8/12 i2c: pxa: enable/disable irq across message xfer
In reply to Vaibhav HiremathIn order to avoid "spurious irq" caused by CP polling mode, enable irq at the entry of i2c_pxa_xfer() fn and disable it again before exit.
Also disable it before exiting probe function.
[vaibhav.hiremath at linaro.org: Split & merge patches into logical changes and update the Changelog]
--- drivers/i2c/busses/i2c-pxa.c | 4 ++++ 1 file changed, 4 insertions(+)
drivers/i2c/busses/i2c-pxa.c +4 -0
@@ -1163,6 +1163,7 @@ static int i2c_pxa_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int numstruct pxa_i2c *i2c = adap->algo_data;int ret, i; +enable_irq (i2c->irq);for (i = adap->retries; i >=0 ; i--) { ret =i2c_pxa_do_xfer (i2c, msgs, num);if (ret !=I2C_RETRY )
@@ -1176,6 +1177,7 @@ static int i2c_pxa_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num ret = -EREMOTEIO ; out:i2c_pxa_set_slave (i2c, ret); +disable_irq (i2c->irq);return ret; }
@@ -1363,6 +1365,8 @@ static int i2c_pxa_probe(struct platform_device *dev)i2c_pxa_reset (i2c); +disable_irq (i2c->irq); + i2c->adap.algo_data = i2c; i2c->adap.dev.parent = &dev->dev;#ifdef CONFIG_OF
Signature · 1 line
1.9.1
PATCH 8/12 i2c: pxa: enable/disable irq across message xfer
Quoted 3 lines · Vaibhav Hiremath
In order to avoid "spurious irq" caused by CP polling mode, enable irq at the entry of
i2c_pxa_xfer()fn and disable it again before exit.
NAK.
It's really not nice for drivers to disable a potentially shared interrupt. If the interrupt is shared, you disable the interrupt for other users of that interrupt as well. See:
commit c66dc529194be374556d166ee7ddb84a7d1d302b
Author: Sebastian Andrzej Siewior <[email protected]> Date: Wed Feb 23 12:38:18 2011 +0100
i2c-pxa2xx: add support for shared IRQ handler
Sodaville has three of them on a single IRQ. IRQF_DISABLED is removed because it is a NOP allready and scheduled for removal.
So you're breaking Sodaville.
Signature · 2 lines
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net.
PATCH 8/12 i2c: pxa: enable/disable irq across message xfer
Quoted 17 lines · Russell King - ARM Linux
Quoted 3 lines · Vaibhav Hiremath
In order to avoid "spurious irq" caused by CP polling mode, enable irq at the entry of
i2c_pxa_xfer()fn and disable it again before exit.NAK.
It's really not nice for drivers to disable a potentially shared interrupt. If the interrupt is shared, you disable the interrupt for other users of that interrupt as well. See:
commit
c66dc529194be374556d166ee7ddb84a7d1d302bAuthor: Sebastian Andrzej Siewior <[email protected]> Date: Wed Feb 23 12:38:18 2011 +0100i2c-pxa2xx: add support for shared IRQ handler
Quoted 9 lines
Sodaville has three of them on a single IRQ.
IRQF_DISABLEDis removed because it is a NOP allready and scheduled for removal.Signed-off-by Sebastian Andrzej Siewior <[email protected]>Signed-off-by Dirk Brandewie <[email protected]>Signed-off-by Ben Dooks <[email protected]>So you're breaking Sodaville.
Got it.
I will drop this patch from the series.
Before submitting next version, will wait for review on other patches.
Thanks,
Vaibhav
PATCH 9/12 i2c: pxa: Remove compile warnning in 64bit mode
In reply to Vaibhav HiremathFrom: Yipeng Yao <[email protected]>
Fix below warning message, coming from 64 bit toolchain.
drivers/i2c/busses/i2c-pxa.c:1237:15: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
--- drivers/i2c/busses/i2c-pxa.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/i2c/busses/i2c-pxa.c +1 -1
@@ -1223,7 +1223,7 @@ static int i2c_pxa_probe_dt(struct platform_device *pdev, struct pxa_i2c *i2c, i2c->use_pio =1 ;if (of_get_property (np,"mrvl,i2c-fast-mode" ,NULL )) i2c->fast_mode =1 ; - *i2c_types = (u32)(of_id->data); + *i2c_types = (long )(of_id->data);if (of_device_is_compatible (np,"mrvl,mmp-twsi" )) { ret =of_property_read_u32 (np,"mrvl,i2c-ilcr" , &i2c->ilcr);
Signature · 1 line
1.9.1
PATCH 9/12 i2c: pxa: Remove compile warnning in 64bit mode
Quoted 4 lines · Vaibhav Hiremath
From: Yipeng Yao <[email protected]>
Fix below warning message, coming from 64 bit toolchain.
Quoted 8 lines
drivers/i2c/busses/i2c-pxa.c:1237:15: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]Signed-off-by Yipeng Yao <[email protected]>[vaibhav.hiremath at linaro.org: Updated Changelog]Signed-off-by Vaibhav Hiremath <[email protected]>Cc Wolfram Sang <[email protected]>
Cheers.
--
Robert
PATCH 10/12 i2c: pxa: Update debug function to dump more info on error
In reply to Vaibhav HiremathUpdate i2c_pxa_scream_blue_murder() fn to print more information in case of error.
[vaibhav.hiremath at linaro.org: Split patches into logical changes and update the Changelog]
--- drivers/i2c/busses/i2c-pxa.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
drivers/i2c/busses/i2c-pxa.c +8 -3
@@ -147,6 +147,7 @@ struct pxa_i2c {unsigned int msg_idx;unsigned int msg_ptr;unsigned int slave_addr; +unsigned int req_slave_addr;struct i2c_adapter adap;struct clk *clk;
@@ -335,11 +336,13 @@ err_out:static void i2c_pxa_scream_blue_murder (struct pxa_i2c *i2c,const char *why) {unsigned int i; -printk (KERN_ERR "i2c: error: %s\n" , why); +printk (KERN_ERR "i2c: <%s> slave_0x%x error: %s\n" , i2c->adap.name, + i2c->req_slave_addr >>1 , why);printk (KERN_ERR "i2c: msg_num: %d msg_idx: %d msg_ptr: %d\n" , i2c->msg_num, i2c->msg_idx, i2c->msg_ptr); -printk (KERN_ERR "i2c: ICR: %08x ISR: %08x\n" , -readl (_ICR (i2c)),readl (_ISR (i2c))); +printk (KERN_ERR "i2c: IBMR: %08x IDBR: %08x ICR: %08x ISR: %08x\n" , +readl (_IBMR (i2c)),readl (_IDBR (i2c)),readl (_ICR (i2c)), +readl (_ISR (i2c)));printk (KERN_DEBUG "i2c: log: " );for (i =0 ; i < i2c->irqlogidx; i++)printk ("[%08x:%08x] " , i2c->isrlog[i], i2c->icrlog[i]);
@@ -736,6 +739,7 @@ static inline void i2c_pxa_start_message(struct pxa_i2c *i2c)* Step 1: target slave address into IDBR */ writel (i2c_pxa_addr_byte (i2c->msg),_IDBR (i2c)); + i2c->req_slave_addr =i2c_pxa_addr_byte (i2c->msg);/* * Step 2: initiate the write.
@@ -1055,6 +1059,7 @@ static void i2c_pxa_irq_txempty(struct pxa_i2c *i2c, u32 isr)* Write the next address. */ writel (i2c_pxa_addr_byte (i2c->msg),_IDBR (i2c)); + i2c->req_slave_addr =i2c_pxa_addr_byte (i2c->msg);/* * And trigger a repeated start, and send the byte.
Signature · 1 line
1.9.1
PATCH 10/12 i2c: pxa: Update debug function to dump more info on error
Quoted 27 lines · Vaibhav Hiremath
Update
i2c_pxa_scream_blue_murder()fn to print more information in case of error.Signed-off-by Jett.Zhou <[email protected]>[vaibhav.hiremath at linaro.org: Split patches into logical changes and update the Changelog]
Signed-off-by Vaibhav Hiremath <[email protected]>Cc Wolfram Sang <[email protected]>Signed-off-by Vaibhav Hiremath <[email protected]>--- drivers/i2c/busses/i2c-pxa.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)drivers/i2c/busses/i2c-pxa.c +1
-0@@ -147,6 +147,7 @@ struct pxa_i2c {unsigned int msg_idx;unsigned int msg_ptr;unsigned int slave_addr; +unsigned int req_slave_addr;struct i2c_adapter adap;struct clk *clk;
drivers/i2c/busses/i2c-pxa.c err_out: · line 340
Quoted 6 lines
drivers/i2c/busses/i2c-pxa.c +2
-1@@ -335,11 +336,13 @@ err_out:static void i2c_pxa_scream_blue_murder (struct pxa_i2c *i2c,const char *why) {unsigned int i; -printk (KERN_ERR "i2c: error: %s\n" , why); +printk (KERN_ERR "i2c: <%s> slave_0x%x error: %s\n" , i2c->adap.name, + i2c->req_slave_addr >>1 , why);
Why not simply use dev_err(....) instead of adding manually i2c->adap.name ?
Cheers.
Signature · 1 line
Robert
PATCH 10/12 i2c: pxa: Update debug function to dump more info on error
Quoted 26 lines · Robert Jarzmik
Quoted 24 lines · Vaibhav Hiremath
Update
i2c_pxa_scream_blue_murder()fn to print more information in case of error.Signed-off-by Jett.Zhou <[email protected]>[vaibhav.hiremath at linaro.org: Split patches into logical changes and update the Changelog]
Signed-off-by Vaibhav Hiremath <[email protected]>Cc Wolfram Sang <[email protected]>Signed-off-by Vaibhav Hiremath <[email protected]>--- drivers/i2c/busses/i2c-pxa.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)drivers/i2c/busses/i2c-pxa.c +1
-0@@ -147,6 +147,7 @@ struct pxa_i2c {unsigned int msg_idx;unsigned int msg_ptr;unsigned int slave_addr; +unsigned int req_slave_addr;
Quoted 11 lines
drivers/i2c/busses/i2c-pxa.c err_out: · line 340
Quoted 9 lines
drivers/i2c/busses/i2c-pxa.c +2
-1struct i2c_adapter adap;struct clk *clk;@@ -335,11 +336,13 @@ err_out:static void i2c_pxa_scream_blue_murder (struct pxa_i2c *i2c,const char *why) {unsigned int i; -printk (KERN_ERR "i2c: error: %s\n" , why); +printk (KERN_ERR "i2c: <%s> slave_0x%x error: %s\n" , i2c->adap.name, + i2c->req_slave_addr >>1 , why);Why not simply use
dev_err(....)instead of adding manuallyi2c->adap.name?
Actually I missed this. Thought of fixing this, but somehow forgot :) Will fix it in next version.
Thanks,
Vaibhav
PATCH 11/12 i2c:pxa: Use devm_ variants in probe function
In reply to Vaibhav HiremathThis patch cleans up i2c_pxa_probe() function,
- Use devm_ variants wherever
This will clean both probe exit and i2c_pxa_remove() functions
- Check platform resource before parsing any other data from DT/platform
- Use dev_err on failure from i2c_add_numbered_adapter()
- Use pr_info instead of printk for KERN_INFO
--- drivers/i2c/busses/i2c-pxa.c | 71 ++++++++++++++++---------------------------- 1 file changed, 25 insertions(+), 46 deletions(-)
drivers/i2c/busses/i2c-pxa.c +25 -46
@@ -1270,10 +1270,17 @@ static int i2c_pxa_probe(struct platform_device *dev)struct resource *res =NULL ;int ret, irq; - i2c =kzalloc (sizeof (struct pxa_i2c ),GFP_KERNEL ); + i2c =devm_kzalloc (&dev->dev,sizeof (struct pxa_i2c ),GFP_KERNEL );if (!i2c) { - ret = -ENOMEM ; - goto emalloc; +dev_err (&dev->dev,"memory allocation failed\n" ); +return -ENOMEM ; + } + + res =platform_get_resource (dev,IORESOURCE_MEM ,0 ); + irq =platform_get_irq (dev,0 ); +if (res ==NULL || irq <0 ) { +dev_err (&dev->dev,"no mem/irq resource\n" ); +return -ENODEV ; }/* Default adapter num to device id; i2c_pxa_probe_dt can override. */
@@ -1283,19 +1290,7 @@ static int i2c_pxa_probe(struct platform_device *dev)if (ret >0 ) ret =i2c_pxa_probe_pdata (dev, i2c, &i2c_type);if (ret <0 ) - goto eclk; - - res =platform_get_resource (dev,IORESOURCE_MEM ,0 ); - irq =platform_get_irq (dev,0 ); -if (res ==NULL || irq <0 ) { - ret = -ENODEV ; - goto eclk; - } - -if (!request_mem_region (res->start,resource_size (res), res->name)) { - ret = -ENOMEM ; - goto eclk; - } +return ret; i2c->adap.owner =THIS_MODULE ; i2c->adap.retries =5 ;
@@ -1305,16 +1300,16 @@ static int i2c_pxa_probe(struct platform_device *dev)strlcpy (i2c->adap.name,"pxa_i2c-i2c" ,sizeof (i2c->adap.name)); - i2c->clk =clk_get (&dev->dev,NULL ); + i2c->clk =devm_clk_get (&dev->dev,NULL );if (IS_ERR (i2c->clk)) { - ret =PTR_ERR (i2c->clk); - goto eclk; +dev_err (&dev->dev,"failed to get the clk\n" ); +return PTR_ERR (i2c->clk); } - i2c->reg_base =ioremap (res->start,resource_size (res)); + i2c->reg_base =devm_ioremap_resource (&dev->dev, res);if (!i2c->reg_base) { - ret = -EIO ; - goto eremap; +dev_err (&dev->dev,"failed to map resource\n" ); +return -EIO ; } i2c->reg_ibmr = i2c->reg_base + pxa_reg_layout[i2c_type].ibmr;
@@ -1361,11 +1356,13 @@ static int i2c_pxa_probe(struct platform_device *dev) i2c->adap.algo = &i2c_pxa_pio_algorithm; }else { i2c->adap.algo = &i2c_pxa_algorithm; - ret =request_irq (irq, i2c_pxa_handler, + ret =devm_request_irq (&dev->dev, irq, i2c_pxa_handler,IRQF_SHARED |IRQF_NO_SUSPEND ,dev_name (&dev->dev), i2c); -if (ret) +if (ret) { +dev_err (&dev->dev,"failed to request irq\n" ); goto ereqirq; + } }i2c_pxa_reset (i2c);
@@ -1380,8 +1377,8 @@ static int i2c_pxa_probe(struct platform_device *dev) ret =i2c_add_numbered_adapter (&i2c->adap);if (ret <0 ) { -printk (KERN_INFO "I2C: Failed to add bus\n" ); - goto eadapt; +dev_err (&dev->dev,"failed to add bus\n" ); + goto ereqirq; }platform_set_drvdata (dev, i2c);
@@ -1411,26 +1408,15 @@ static int i2c_pxa_probe(struct platform_device *dev) } }#ifdef CONFIG_I2C_PXA_SLAVE -printk (KERN_INFO "I2C: %s: PXA I2C adapter, slave address %d\n" , +pr_info ("I2C: %s: PXA I2C adapter, slave address %d\n" ,dev_name (&i2c->adap.dev), i2c->slave_addr);#else -printk (KERN_INFO "I2C: %s: PXA I2C adapter\n" , -dev_name (&i2c->adap.dev)); +pr_info ("I2C: %s: PXA I2C adapter\n" ,dev_name (&i2c->adap.dev));#endif return 0 ; -eadapt: -if (!i2c->use_pio) -free_irq (irq, i2c); ereqirq:clk_disable_unprepare (i2c->clk); -iounmap (i2c->reg_base); -eremap: -clk_put (i2c->clk); -eclk: -kfree (i2c); -emalloc: -release_mem_region (res->start,resource_size (res));return ret; }
@@ -1439,15 +1425,8 @@ static int i2c_pxa_remove(struct platform_device *dev)struct pxa_i2c *i2c =platform_get_drvdata (dev);i2c_del_adapter (&i2c->adap); -if (!i2c->use_pio) -free_irq (i2c->irq, i2c);clk_disable_unprepare (i2c->clk); -clk_put (i2c->clk); - -iounmap (i2c->reg_base); -release_mem_region (i2c->iobase, i2c->iosize); -kfree (i2c);return 0 ; }
Signature · 1 line
1.9.1
PATCH 11/12 i2c:pxa: Use devm_ variants in probe function
Quoted 8 lines · Vaibhav Hiremath
drivers/i2c/busses/i2c-pxa.c +1
-2@@ -1270,10 +1270,17 @@ static int i2c_pxa_probe(struct platform_device *dev)struct resource *res =NULL ;int ret, irq; - i2c =kzalloc (sizeof (struct pxa_i2c ),GFP_KERNEL ); + i2c =devm_kzalloc (&dev->dev,sizeof (struct pxa_i2c ),GFP_KERNEL );if (!i2c) { - ret = -ENOMEM ;
drivers/i2c/busses/i2c-pxa.c static int i2c_pxa_probe(struct platform_device *dev) · line 1283
Quoted 10 lines
drivers/i2c/busses/i2c-pxa.c +9
-1- goto emalloc; +dev_err (&dev->dev,"memory allocation failed\n" ); +return -ENOMEM ; + } + + res =platform_get_resource (dev,IORESOURCE_MEM ,0 ); + irq =platform_get_irq (dev,0 ); +if (res ==NULL || irq <0 ) { +dev_err (&dev->dev,"no mem/irq resource\n" ); +return -ENODEV ;
I'd like to have the error code here, as in other part of the driver. Ie. I'd want :
dev_err(&dev->dev, "no mem/irq resource: %d\n", irq);
Moreover, return -ENODEV if res == NULL, but return irq if irq < 0 (think of -EPROBE_DEFER).
drivers/i2c/busses/i2c-pxa.c static int i2c_pxa_probe(struct platform_device *dev) · line 1310
Quoted 2 lines
drivers/i2c/busses/i2c-pxa.c +1
-0+ i2c->reg_base =devm_ioremap_resource (&dev->dev, res);if (!i2c->reg_base) {
if (IS_ERR(i2c->reg_base)) instead.
drivers/i2c/busses/i2c-pxa.c static int i2c_pxa_probe(struct platform_device *dev) · line 1312
Quoted 4 lines
drivers/i2c/busses/i2c-pxa.c +2
-2- ret = -EIO ; - goto eremap; +dev_err (&dev->dev,"failed to map resource\n" ); +return -EIO ;
Ditto, ie. include PTR_ERR(i2c->reg_base) in the dev_err(), and return PTR_ERR(i2c->reg_base);
drivers/i2c/busses/i2c-pxa.c static int i2c_pxa_probe(struct platform_device *dev) · line 1363
Quoted 11 lines
drivers/i2c/busses/i2c-pxa.c +3
-2@@ -1361,11 +1356,13 @@ static int i2c_pxa_probe(struct platform_device *dev) i2c->adap.algo = &i2c_pxa_pio_algorithm; }else { i2c->adap.algo = &i2c_pxa_algorithm; - ret =request_irq (irq, i2c_pxa_handler, + ret =devm_request_irq (&dev->dev, irq, i2c_pxa_handler,IRQF_SHARED |IRQF_NO_SUSPEND ,dev_name (&dev->dev), i2c); -if (ret) +if (ret) { +dev_err (&dev->dev,"failed to request irq\n" );
Ditto for the dev_err() and error code reporting.
drivers/i2c/busses/i2c-pxa.c static int i2c_pxa_probe(struct platform_device *dev) · line 1380
Quoted 7 lines
drivers/i2c/busses/i2c-pxa.c +1
-2@@ -1380,8 +1377,8 @@ static int i2c_pxa_probe(struct platform_device *dev) ret =i2c_add_numbered_adapter (&i2c->adap);if (ret <0 ) { -printk (KERN_INFO "I2C: Failed to add bus\n" ); - goto eadapt; +dev_err (&dev->dev,"failed to add bus\n" );
Ditto.
drivers/i2c/busses/i2c-pxa.c static int i2c_pxa_probe(struct platform_device *dev) · line 1412
Quoted 7 lines
drivers/i2c/busses/i2c-pxa.c +1
-1@@ -1411,26 +1408,15 @@ static int i2c_pxa_probe(struct platform_device *dev) } }#ifdef CONFIG_I2C_PXA_SLAVE -printk (KERN_INFO "I2C: %s: PXA I2C adapter, slave address %d\n" , +pr_info ("I2C: %s: PXA I2C adapter, slave address %d\n" ,dev_name (&i2c->adap.dev), i2c->slave_addr);
dev_info() maybe ?
drivers/i2c/busses/i2c-pxa.c static int i2c_pxa_probe(struct platform_device *dev) · line 1414
Quoted 4 lines
drivers/i2c/busses/i2c-pxa.c +1
-2#else -printk (KERN_INFO "I2C: %s: PXA I2C adapter\n" , -dev_name (&i2c->adap.dev)); +pr_info ("I2C: %s: PXA I2C adapter\n" ,dev_name (&i2c->adap.dev));
Ditto.
Cheers.
Signature · 1 line
Robert
PATCH 11/12 i2c:pxa: Use devm_ variants in probe function
Quoted 22 lines · Robert Jarzmik
Quoted 8 lines · Vaibhav Hiremath
drivers/i2c/busses/i2c-pxa.c +1
-2@@ -1270,10 +1270,17 @@ static int i2c_pxa_probe(struct platform_device *dev)struct resource *res =NULL ;int ret, irq; - i2c =kzalloc (sizeof (struct pxa_i2c ),GFP_KERNEL ); + i2c =devm_kzalloc (&dev->dev,sizeof (struct pxa_i2c ),GFP_KERNEL );if (!i2c) { - ret = -ENOMEM ;drivers/i2c/busses/i2c-pxa.c static int i2c_pxa_probe(struct platform_device *dev) · line 1283
Quoted 10 lines
drivers/i2c/busses/i2c-pxa.c +9
-1- goto emalloc; +dev_err (&dev->dev,"memory allocation failed\n" ); +return -ENOMEM ; + } + + res =platform_get_resource (dev,IORESOURCE_MEM ,0 ); + irq =platform_get_irq (dev,0 ); +if (res ==NULL || irq <0 ) { +dev_err (&dev->dev,"no mem/irq resource\n" ); +return -ENODEV ;I'd like to have the error code here, as in other part of the driver. Ie. I'd want :
Quoted 5 lines
dev_err(&dev->dev, "no mem/irq resource: %d\n", irq);Moreover, return
-ENODEVif res == NULL, but return irq if irq < 0 (think of -EPROBE_DEFER).
Good point.
I will change it in next version.
Quoted 11 lines
drivers/i2c/busses/i2c-pxa.c static int i2c_pxa_probe(struct platform_device *dev) · line 1310
Quoted 2 lines
drivers/i2c/busses/i2c-pxa.c +1
-0+ i2c->reg_base =devm_ioremap_resource (&dev->dev, res);if (!i2c->reg_base) {if (
IS_ERR(i2c->reg_base)) instead.drivers/i2c/busses/i2c-pxa.c static int i2c_pxa_probe(struct platform_device *dev) · line 1312
Quoted 4 lines
drivers/i2c/busses/i2c-pxa.c +2
-2- ret = -EIO ; - goto eremap; +dev_err (&dev->dev,"failed to map resource\n" ); +return -EIO ;Ditto, ie. include
PTR_ERR(i2c->reg_base)in thedev_err(), and returnPTR_ERR(i2c->reg_base);
Ok.
Quoted 31 lines
drivers/i2c/busses/i2c-pxa.c static int i2c_pxa_probe(struct platform_device *dev) · line 1363
Quoted 11 lines
drivers/i2c/busses/i2c-pxa.c +3
-2@@ -1361,11 +1356,13 @@ static int i2c_pxa_probe(struct platform_device *dev) i2c->adap.algo = &i2c_pxa_pio_algorithm; }else { i2c->adap.algo = &i2c_pxa_algorithm; - ret =request_irq (irq, i2c_pxa_handler, + ret =devm_request_irq (&dev->dev, irq, i2c_pxa_handler,IRQF_SHARED |IRQF_NO_SUSPEND ,dev_name (&dev->dev), i2c); -if (ret) +if (ret) { +dev_err (&dev->dev,"failed to request irq\n" );Ditto for the
dev_err()and error code reporting.drivers/i2c/busses/i2c-pxa.c static int i2c_pxa_probe(struct platform_device *dev) · line 1380
Quoted 7 lines
drivers/i2c/busses/i2c-pxa.c +1
-2@@ -1380,8 +1377,8 @@ static int i2c_pxa_probe(struct platform_device *dev) ret =i2c_add_numbered_adapter (&i2c->adap);if (ret <0 ) { -printk (KERN_INFO "I2C: Failed to add bus\n" ); - goto eadapt; +dev_err (&dev->dev,"failed to add bus\n" );Ditto.
drivers/i2c/busses/i2c-pxa.c static int i2c_pxa_probe(struct platform_device *dev) · line 1412
Quoted 7 lines
drivers/i2c/busses/i2c-pxa.c +1
-1@@ -1411,26 +1408,15 @@ static int i2c_pxa_probe(struct platform_device *dev) } }#ifdef CONFIG_I2C_PXA_SLAVE -printk (KERN_INFO "I2C: %s: PXA I2C adapter, slave address %d\n" , +pr_info ("I2C: %s: PXA I2C adapter, slave address %d\n" ,dev_name (&i2c->adap.dev), i2c->slave_addr);
dev_info()maybe ?
Quoted 6 lines
drivers/i2c/busses/i2c-pxa.c static int i2c_pxa_probe(struct platform_device *dev) · line 1414
Quoted 4 lines
drivers/i2c/busses/i2c-pxa.c +1
-2#else -printk (KERN_INFO "I2C: %s: PXA I2C adapter\n" , -dev_name (&i2c->adap.dev)); +pr_info ("I2C: %s: PXA I2C adapter\n" ,dev_name (&i2c->adap.dev));Ditto.
Agreed to all your comments.
I will change the code in next version.
Thanks,
Vaibhav
PATCH 12/12 i2c: pxa: enable/disable i2c module across msg xfer
In reply to Vaibhav HiremathFrom: Yi Zhang <[email protected]>
Enable i2c module/unit before transmission and disable when it finishes.
why?
It's because the i2c bus may be distrubed if the slave device, typically a touch, powers on.
--- drivers/i2c/busses/i2c-pxa.c | 29 +++++++++++++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-)
drivers/i2c/busses/i2c-pxa.c +27 -2
@@ -368,6 +368,16 @@ static void i2c_pxa_scream_blue_murder(struct pxa_i2c *i2c, const char *why)static void i2c_pxa_master_complete (struct pxa_i2c *i2c,int ret);static irqreturn_t i2c_pxa_handler (int this_irq,void *dev_id); +/* enable/disable i2c unit */ +static inline void i2c_pxa_enable (struct pxa_i2c *i2c,bool enable) +{ +if (enable) +writel (readl (_ICR (i2c)) |ICR_IUE ,_ICR (i2c)); +else +writel (readl (_ICR (i2c)) & ~ICR_IUE ,_ICR (i2c)); +udelay (100 ); +} +static inline int i2c_pxa_is_slavemode (struct pxa_i2c *i2c) {return !(readl (_ICR (i2c)) &ICR_SCLE );
@@ -575,8 +585,7 @@ static void i2c_pxa_reset(struct pxa_i2c *i2c)i2c_pxa_set_slave (i2c,0 );/* enable unit */ -writel (readl (_ICR (i2c)) |ICR_IUE ,_ICR (i2c)); -udelay (100 ); +i2c_pxa_enable (i2c, true); }
@@ -933,6 +942,9 @@ static int i2c_pxa_pio_xfer(struct i2c_adapter *adap,struct pxa_i2c *i2c = adap->algo_data;int ret, i; +/* Enable i2c unit */ +i2c_pxa_enable (i2c, true); +/* If the I2C controller is disabled we need to reset it (probably due to a suspend/resume destroying state). We do this here as we can then avoid worrying about resuming the
@@ -953,6 +965,10 @@ static int i2c_pxa_pio_xfer(struct i2c_adapter *adap, ret = -EREMOTEIO ; out:i2c_pxa_set_slave (i2c, ret); + +/* disable i2c unit */ +i2c_pxa_enable (i2c, false); +return ret; }
@@ -1168,6 +1184,9 @@ static int i2c_pxa_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int numstruct pxa_i2c *i2c = adap->algo_data;int ret, i; +/* Enable i2c unit */ +i2c_pxa_enable (i2c, true); +enable_irq (i2c->irq);for (i = adap->retries; i >=0 ; i--) { ret =i2c_pxa_do_xfer (i2c, msgs, num);
@@ -1183,6 +1202,9 @@ static int i2c_pxa_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num out:i2c_pxa_set_slave (i2c, ret);disable_irq (i2c->irq); +/* disable i2c unit */ +i2c_pxa_enable (i2c, false); +return ret; }
@@ -1407,6 +1429,9 @@ static int i2c_pxa_probe(struct platform_device *dev) ret =0 ; } } + +i2c_pxa_enable (i2c, false); +#ifdef CONFIG_I2C_PXA_SLAVE pr_info ("I2C: %s: PXA I2C adapter, slave address %d\n" ,dev_name (&i2c->adap.dev), i2c->slave_addr);
Signature · 1 line
1.9.1
PATCH 12/12 i2c: pxa: enable/disable i2c module across msg xfer
Quoted 7 lines · Vaibhav Hiremath
From: Yi Zhang <[email protected]>
Enable i2c module/unit before transmission and disable when it finishes.
why?
It's because the i2c bus may be distrubed if the slave device, typically a touch, powers on.
"disturbed"
I'd recommend that this is a DT property - not every platform is going to want this, and as there is rudimentary I2C slave support in this driver, this change breaks that.
Signature · 2 lines
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net.
PATCH 12/12 i2c: pxa: enable/disable i2c module across msg xfer
Quoted 9 lines · Russell King - ARM Linux
Quoted 7 lines · Vaibhav Hiremath
From: Yi Zhang <[email protected]>
Enable i2c module/unit before transmission and disable when it finishes.
why?
It's because the i2c bus may be distrubed if the slave device, typically a touch, powers on.
Quoted 6 lines
"disturbed"
I'd recommend that this is a DT property - not every platform is going to want this, and as there is rudimentary I2C slave support in this driver, this change breaks that.
I would take it as two different comments here, and I believe you also meant same,
1. Not breaking I2C slave support
Not sure whether enabling/disabling module in ISR would suffice here. To be specific, in the functions i2c_pxa_slave_start() & i2c_pxa_slave_stop()
Another important point is, I am not sure on validation part here, as the platform I have doesn't have multi masters on the bus.
2. DT property
I forsee conflict between, DT property and SLAVE mode of operation. where we may want to support master-&-slave together.
Also, we may not need this if we fix I2C Slave support issue, but testing is challenge for me (atleast).
Do you see any issues with first approach?
if you are ok with first approach, I can change the code.
Thanks,
Vaibhav
PATCH 12/12 i2c: pxa: enable/disable i2c module across msg xfer
Quoted 19 lines · Vaibhav Hiremath
Quoted 9 lines · Russell King - ARM Linux
Quoted 7 lines · Vaibhav Hiremath
From: Yi Zhang <[email protected]>
Enable i2c module/unit before transmission and disable when it finishes.
why?
It's because the i2c bus may be distrubed if the slave device, typically a touch, powers on.Quoted 6 lines
"disturbed"
I'd recommend that this is a DT property - not every platform is going to want this, and as there is rudimentary I2C slave support in this driver, this change breaks that.
Quoted 10 lines
I would take it as two different comments here, and I believe you also meant same,
1. Not breaking I2C slave support
Not sure whether enabling/disabling module in ISR would suffice here. To be specific, in the functions
i2c_pxa_slave_start()&i2c_pxa_slave_stop()
Please ignore this option, as enable is important to generate interrupt on slave start.
I will add DT property with clear note on SLAVE support issue, so that it can be disabled and SLAVE support should work as is.
Thanks,
Vaibhav
PATCH 12/12 i2c: pxa: enable/disable i2c module across msg xfer
Quoted 27 lines · Vaibhav Hiremath
Quoted 24 lines · Vaibhav Hiremath
Quoted 9 lines · Russell King - ARM Linux
Quoted 7 lines · Vaibhav Hiremath
From: Yi Zhang <[email protected]>
Enable i2c module/unit before transmission and disable when it finishes.
why?
It's because the i2c bus may be distrubed if the slave device, typically a touch, powers on.Quoted 6 lines
"disturbed"
I'd recommend that this is a DT property - not every platform is going to want this, and as there is rudimentary I2C slave support in this driver, this change breaks that.
I would take it as two different comments here, and I believe you also meant same,
1. Not breaking I2C slave support
Quoted 8 lines
Quoted 5 lines
Not sure whether enabling/disabling module in ISR would suffice here. To be specific, in the functions
i2c_pxa_slave_start()&i2c_pxa_slave_stop()Please ignore this option, as enable is important to generate interrupt on slave start.
Yes, you are right, enabling i2c controller first is must for generating the interrupt
Quoted 6 lines
I will add DT property with clear note on SLAVE support issue, so that it can be disabled and SLAVE support should work as is.
Thanks,
Vaibhav
PATCH 12/12 i2c: pxa: enable/disable i2c module across msg xfer
Quoted 34 lines · Yi Zhang
Quoted 33 lines · Vaibhav Hiremath
Quoted 29 lines · Vaibhav Hiremath
Quoted 9 lines · Russell King - ARM Linux
Quoted 7 lines · Vaibhav Hiremath
From: Yi Zhang <[email protected]>
Enable i2c module/unit before transmission and disable when it finishes.
why?
It's because the i2c bus may be distrubed if the slave device, typically a touch, powers on.Quoted 6 lines
"disturbed"
I'd recommend that this is a DT property - not every platform is going to want this, and as there is rudimentary I2C slave support in this driver, this change breaks that.
I would take it as two different comments here, and I believe you also meant same,
1. Not breaking I2C slave support
Not sure whether enabling/disabling module in ISR would suffice here. To be specific, in the functions
i2c_pxa_slave_start()&i2c_pxa_slave_stop()
Quoted 5 lines
Quoted 2 lines
Please ignore this option, as enable is important to generate interrupt on slave start.
Yes, you are right, enabling i2c controller first is must for generating the interrupt
Zang,
Probably you can help me,
I am trying to introduce standard DT propertied for sclk adjustment, meant for ILCR and IWCR register.
The datasheet I have has some confusion in terms of usage of load counters and wait counters.
Can you please help me understand on below queries -
1. Any additional calculation (any offset or something) I need to add for calculating counter values from time in nsec/usec?
2. Counters just counts the input clock (of either 26MHz or 69MHz)?
3. Are all counters to respective modes mutual exclusive? I mean, if I set fast mode, any dependency on other counters? I see IWCR.COUNT is common for both standard and fast mode, what about high speed mode?
4. IWCR.COUNT field is meant for tHD.DATA ???
Thanks,
Vaibhav
PATCH 12/12 i2c: pxa: enable/disable i2c module across msg xfer
In reply to Vaibhav HiremathQuoted 27 lines · Vaibhav Hiremath
Quoted 24 lines · Vaibhav Hiremath
Quoted 9 lines · Russell King - ARM Linux
Quoted 7 lines · Vaibhav Hiremath
From: Yi Zhang <[email protected]>
Enable i2c module/unit before transmission and disable when it finishes.
why?
It's because the i2c bus may be distrubed if the slave device, typically a touch, powers on.Quoted 6 lines
"disturbed"
I'd recommend that this is a DT property - not every platform is going to want this, and as there is rudimentary I2C slave support in this driver, this change breaks that.
I would take it as two different comments here, and I believe you also meant same,
1. Not breaking I2C slave support
Quoted 8 lines
Quoted 5 lines
Not sure whether enabling/disabling module in ISR would suffice here. To be specific, in the functions
i2c_pxa_slave_start()&i2c_pxa_slave_stop()Please ignore this option, as enable is important to generate interrupt on slave start.
Hi, Vaibhav:
sorry there is something wrong with my mailbox, so I list the LCR/WCR information here, hoping not to confuse you;
LCR: it's used to minor adjustments to the SCL, increasing it decreases the SCL frequency, vice versa;
yes, it has relation with the I2C input clock:
if input_clk = 32MHz, LCR = 0x0804_1c96
LCR[SLV] = 0x096, bit 8~0
LCR[FLV] = 0xe, bit 17~9
LCR[HLVH] = 0x1, bit 26~18
LCR[HLVL] = 0x1, bit 31~27 if input_clk = 52MHz, LCR = 0x081c_60f9
if input_clk = 62.4MHz, LCR = 0x082c_8330
WCR: it's used to control setup and hold timing during slave mode WCR[31: 15], reserved, always write 0;
WCR[14: 10], default 0x5, and should not be changed WCR[9: 5], default 0x1, and should not be changed
WCR[4:0], default 0x1a, if input_clk = 33MHz, it's 0x0a;
if input_clk = 66MHz, it's 0x14;hope it's helpful;
---
Yi Zhang <[email protected]>
Quoted 6 lines
I will add DT property with clear note on SLAVE support issue, so that it can be disabled and SLAVE support should work as is.
Thanks,
Vaibhav
PATCH 12/12 i2c: pxa: enable/disable i2c module across msg xfer
Quoted 34 lines · Yi Zhang
Quoted 33 lines · Vaibhav Hiremath
Quoted 29 lines · Vaibhav Hiremath
Quoted 9 lines · Russell King - ARM Linux
Quoted 7 lines · Vaibhav Hiremath
From: Yi Zhang <[email protected]>
Enable i2c module/unit before transmission and disable when it finishes.
why?
It's because the i2c bus may be distrubed if the slave device, typically a touch, powers on.Quoted 6 lines
"disturbed"
I'd recommend that this is a DT property - not every platform is going to want this, and as there is rudimentary I2C slave support in this driver, this change breaks that.
I would take it as two different comments here, and I believe you also meant same,
1. Not breaking I2C slave support
Not sure whether enabling/disabling module in ISR would suffice here. To be specific, in the functions
i2c_pxa_slave_start()&i2c_pxa_slave_stop()
Quoted 8 lines
Quoted 2 lines
Please ignore this option, as enable is important to generate interrupt on slave start.
Hi, Vaibhav:
sorry there is something wrong with my mailbox, so I list the LCR/WCR information here, hoping not to confuse you;genr
Thanks for the info.
Quoted 11 lines
LCR: it's used to minor adjustments to the SCL, increasing it
decreases the SCL frequency, vice versa;
yes, it has relation with the I2C input clock:
if input_clk = 32MHz, LCR = 0x0804_1c96
LCR[SLV] = 0x096, bit 8~0
LCR[FLV] = 0xe, bit 17~9
LCR[HLVH] = 0x1, bit 26~18
LCR[HLVL] = 0x1, bit 31~27
if input_clk = 52MHz, LCR = 0x081c_60f9
if input_clk = 62.4MHz, LCR = 0x082c_8330Whether they are simply counters which count downs to generate required timing profile on the bus.
So would be possible for you to provide timing values generated by this? Or please confirm below understanding,
@32MHz, SLV = 0x96,
thigh/tlow = 31nsec * 150 = 4.650 usec
thigh/tlow = 32nsec * 150 = 4.8 usec [using ceil()]
and as per I2C spec,
thigh,min = 4 usec
tlow,min = 4.7usec
I believe my understanding is correct, please confirm.
Quoted 1 line
WCR: it's used to control setup and hold timing during slave mode
This is is useful info. So in master mode we can ignore this.
Just to clarify, this bit is don't care in master-transmit and master-receive, right?
Quoted 7 lines
WCR[31: 15], reserved, always write 0; WCR[14: 10], default 0x5, and should not be changed WCR[9: 5], default 0x1, and should not be changed WCR[4:0], default 0x1a, if input_clk = 33MHz, it's 0x0a; if input_clk = 66MHz, it's 0x14;hope it's helpful;
Yes certainly. Thanks.
Thanks,
Vaibhav
Quoted 10 lines
---
Yi Zhang <[email protected]>Quoted 6 lines
I will add DT property with clear note on SLAVE support issue, so that it can be disabled and SLAVE support should work as is.
Thanks,
Vaibhav
PATCH 12/12 i2c: pxa: enable/disable i2c module across msg xfer
Quoted 73 lines · Vaibhav Hiremath
Quoted 34 lines · Yi Zhang
Quoted 33 lines · Vaibhav Hiremath
Quoted 29 lines · Vaibhav Hiremath
Quoted 9 lines · Russell King - ARM Linux
Quoted 7 lines · Vaibhav Hiremath
From: Yi Zhang <[email protected]>
Enable i2c module/unit before transmission and disable when it finishes.
why?
It's because the i2c bus may be distrubed if the slave device, typically a touch, powers on.Quoted 6 lines
"disturbed"
I'd recommend that this is a DT property - not every platform is going to want this, and as there is rudimentary I2C slave support in this driver, this change breaks that.
I would take it as two different comments here, and I believe you also meant same,
1. Not breaking I2C slave support
Not sure whether enabling/disabling module in ISR would suffice here. To be specific, in the functions
i2c_pxa_slave_start()&i2c_pxa_slave_stop()Quoted 8 lines
Quoted 2 lines
Please ignore this option, as enable is important to generate interrupt on slave start.
Hi, Vaibhav:
sorry there is something wrong with my mailbox, so I list the LCR/WCR information here, hoping not to confuse you;genr
Thanks for the info.
Quoted 11 lines
LCR: it's used to minor adjustments to the SCL, increasing it decreases the SCL frequency, vice versa;
yes, it has relation with the I2C input clock:
ifinput_clk= 32MHz, LCR = 0x0804_1c96LCR[SLV] = 0x096, bit 8~0 LCR[FLV] = 0xe, bit 17~9 LCR[HLVH] = 0x1, bit 26~18 LCR[HLVL] = 0x1, bit 31~27if
input_clk= 52MHz, LCR = 0x081c_60f9
ifinput_clk= 62.4MHz, LCR = 0x082c_8330Whether they are simply counters which count downs to generate required timing profile on the bus.
So would be possible for you to provide timing values generated by this? Or please confirm below understanding,
@32MHz, SLV =
0x96,
thigh/tlow = 31nsec * 150 = 4.650 usec
thigh/tlow = 32nsec * 150 = 4.8 usec [usingceil()]
Quoted 6 lines
and as per I2C spec,
thigh,min = 4 usec
tlow,min = 4.7usecI believe my understanding is correct, please confirm.
Yes, you are right, as the name shows it, SLV covers standard mode, FLV is for fastmode, HLVH is for high phase of high speed mode, while HLVL is for low phase of low speed mode;
Quoted 8 lines
Quoted 3 lines
WCR: it's used to control setup and hold timing during slave mode
This is is useful info. So in master mode we can ignore this.
Just to clarify, this bit is don't care in master-transmit and master-receive, right?
Yes, you are right;
Quoted 7 lines
Quoted 7 lines
WCR[31: 15], reserved, always write 0; WCR[14: 10], default 0x5, and should not be changed WCR[9: 5], default 0x1, and should not be changed WCR[4:0], default 0x1a, if input_clk = 33MHz, it's 0x0a; if input_clk = 66MHz, it's 0x14;
Quoted 17 lines
Quoted 1 line
hope it's helpful;
Yes certainly. Thanks.
Thanks,
VaibhavQuoted 10 lines
---
Yi Zhang <[email protected]>Quoted 6 lines
I will add DT property with clear note on SLAVE support issue, so that it can be disabled and SLAVE support should work as is.
Thanks,
Vaibhav
---
Yi Zhang <[email protected]>
cover i2c: pxa: Fixes, cleanup and support for pxa910 family
In reply to Vaibhav HiremathQuoted 5 lines
TODO:
- Due to lack of client driver support as of now, could not able to do functionality testing from client. I am working on PMIC driver support which is over I2C, so will have good meaningful testing then.
Okay, thanks for the info. So, unless this is done I'll consider these patches as RFC.
cover i2c: pxa: Fixes, cleanup and support for pxa910 family
Quoted 8 lines · Wolfram Sang
Quoted 5 lines
TODO:
- Due to lack of client driver support as of now, could not able to do functionality testing from client. I am working on PMIC driver support which is over I2C, so will have good meaningful testing then.Okay, thanks for the info. So, unless this is done I'll consider these patches as RFC.
I already have PMIC client driver up and doing some testing with it. So we should not have this issue for my next version.
Thanks,
Vaibhav