[PATCH-v3 00/11] i2c: pxa: Fixes, cleanup and support for pxa910 family

[PATCH-v3 00/11] i2c: pxa: Fixes, cleanup and support for pxa910 family

[Vaibhav Hiremath]

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:
  - Basic testing on PMIC device on I2C-0 interface
  - Boot tested on platform based on PXA1928
  - Probe is successfully passing
  - Read few registers of PMIC (RTC, ID, etc...) during boot

V2 => V3
=======
Link to V2: http://www.spinics.net/lists/linux-i2c/msg20059.html

  - Removed PATCH [4/12] related to reset of I2C module.
    Suggested by "Robert Jarzmik"
  - Updated commit description for,
     PATCH [11/12]: Mentioned reasoning about moment of clk_get code.
     PATCH [12/12]: for DT property node.
  - Added Acked by "Robert Jarzmik" to patched which he acked.

V1 => V2:
========
Link to V1 - http://lists.infradead.org/pipermail/linux-arm-kernel/2015-May/347012.html

  - Fixed all comments from "Robert Jarzmik" and "Wolfram Sang"
  - Dropped Patch
	05/12: using core bus reset implementation - under work.
               Will submit shortly.
	08/12: NAKed and dropped
  - Separated DT binding patch from driver changes, for easy merge



Leilei Shang (1):
  i2c: pxa: keep i2c irq ON in suspend

Shouming Wang (1):
  i2c: pxa: Return I2C_RETRY when timeout in pio mode

Vaibhav Hiremath (7):
  i2c: pxa: No need to set slave addr for i2c master mode reset
  i2c: pxa: Update debug function to dump more info on error
  i2c:pxa: Use devm_ variants in probe function
  Documentation: binding: add new property 'disable_after_xfer' to
    i2c-pxa
  i2c: pxa: Add support for pxa910/988 & new configuration features
  i2c: pxa: Add ILCR (tLow & tHigh) configuration support
  Documentation: binding: add sclk adjustment properties to i2c-pxa

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 |  18 ++
 drivers/i2c/busses/i2c-pxa.c                      | 262 ++++++++++++++++------
 2 files changed, 215 insertions(+), 65 deletions(-)

-- 
1.9.1

[PATCH-v3 01/11] i2c: pxa: keep i2c irq ON in suspend

[Vaibhav Hiremath]

From: Leilei Shang <shangll@marvell.com>

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.

Signed-off-by: Raul Xiong <xjian@marvell.com>
Signed-off-by: Xiaofan Tian <tianxf@marvell.com>
[vaibhav.hiremath@linaro.org: updated Changelog]
Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
Cc: Wolfram Sang <wsa@the-dreams.de>
---
 drivers/i2c/busses/i2c-pxa.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
index d9c0d6a..f4ac8c5 100644
--- a/drivers/i2c/busses/i2c-pxa.c
+++ b/drivers/i2c/busses/i2c-pxa.c
@@ -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;
 	}
-- 
1.9.1

[PATCH-v3 02/11] i2c: pxa: No need to set slave addr for i2c master mode reset

[Vaibhav Hiremath]

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.

Signed-off-by: Jett.Zhou <jtzhou@marvell.com>
Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
Acked-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
 drivers/i2c/busses/i2c-pxa.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
index f4ac8c5..023e59f 100644
--- a/drivers/i2c/busses/i2c-pxa.c
+++ b/drivers/i2c/busses/i2c-pxa.c
@@ -459,7 +459,7 @@ static void i2c_pxa_reset(struct pxa_i2c *i2c)
 	writel(I2C_ISR_INIT, _ISR(i2c));
 	writel(readl(_ICR(i2c)) & ~ICR_UR, _ICR(i2c));
 
-	if (i2c->reg_isar)
+	if (i2c->reg_isar && IS_ENABLED(CONFIG_I2C_PXA_SLAVE))
 		writel(i2c->slave_addr, _ISAR(i2c));
 
 	/* set control register values */
-- 
1.9.1

Re: [PATCH-v3 02/11] i2c: pxa: No need to set slave addr for i2c master mode reset

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1370 bytes --]

On Tue, Jul 07, 2015 at 12:54:46AM +0530, Vaibhav Hiremath wrote:
> 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.

Just to make sure: Does it? As I read the code, slave interrupts are
enabled later only when slave mode is selected? Is that a HW bug? And if
so, can't the code just be moved into this #ifdef block later?

> 
> Signed-off-by: Jett.Zhou <jtzhou-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Acked-by: Robert Jarzmik <robert.jarzmik-GANU6spQydw@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-pxa.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
> index f4ac8c5..023e59f 100644
> --- a/drivers/i2c/busses/i2c-pxa.c
> +++ b/drivers/i2c/busses/i2c-pxa.c
> @@ -459,7 +459,7 @@ static void i2c_pxa_reset(struct pxa_i2c *i2c)
>  	writel(I2C_ISR_INIT, _ISR(i2c));
>  	writel(readl(_ICR(i2c)) & ~ICR_UR, _ICR(i2c));
>  
> -	if (i2c->reg_isar)
> +	if (i2c->reg_isar && IS_ENABLED(CONFIG_I2C_PXA_SLAVE))
>  		writel(i2c->slave_addr, _ISAR(i2c));
>  
>  	/* set control register values */
> -- 
> 1.9.1
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH-v3 02/11] i2c: pxa: No need to set slave addr for i2c master mode reset

[Vaibhav Hiremath]

On Friday 10 July 2015 01:41 PM, Wolfram Sang wrote:
> On Tue, Jul 07, 2015 at 12:54:46AM +0530, Vaibhav Hiremath wrote:
>> 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.
>
> Just to make sure: Does it? As I read the code, slave interrupts are
> enabled later only when slave mode is selected? Is that a HW bug? And if
> so, can't the code just be moved into this #ifdef block later?
>

Yes we could, infact I thought about it;
but I would break recommended sequence here.

As per spec, it only says,

The SocC writes this register before it enable TWSI operations.

I decided not to break the existing sequence and just fix the code, as
I do can not validate slave mode of operation functionally. I do not
have slave mode setup with me.

Thanks,
Vaibhav

>>
>> Signed-off-by: Jett.Zhou <jtzhou-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> Acked-by: Robert Jarzmik <robert.jarzmik-GANU6spQydw@public.gmane.org>
>> ---
>>   drivers/i2c/busses/i2c-pxa.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
>> index f4ac8c5..023e59f 100644
>> --- a/drivers/i2c/busses/i2c-pxa.c
>> +++ b/drivers/i2c/busses/i2c-pxa.c
>> @@ -459,7 +459,7 @@ static void i2c_pxa_reset(struct pxa_i2c *i2c)
>>   	writel(I2C_ISR_INIT, _ISR(i2c));
>>   	writel(readl(_ICR(i2c)) & ~ICR_UR, _ICR(i2c));
>>
>> -	if (i2c->reg_isar)
>> +	if (i2c->reg_isar && IS_ENABLED(CONFIG_I2C_PXA_SLAVE))
>>   		writel(i2c->slave_addr, _ISAR(i2c));
>>
>>   	/* set control register values */
>> --
>> 1.9.1
>>

Re: [PATCH-v3 02/11] i2c: pxa: No need to set slave addr for i2c master mode reset

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 896 bytes --]

On Fri, Jul 10, 2015 at 06:08:43PM +0530, Vaibhav Hiremath wrote:
> 
> 
> On Friday 10 July 2015 01:41 PM, Wolfram Sang wrote:
> >On Tue, Jul 07, 2015 at 12:54:46AM +0530, Vaibhav Hiremath wrote:
> >>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.
> >
> >Just to make sure: Does it? As I read the code, slave interrupts are
> >enabled later only when slave mode is selected? Is that a HW bug? And if
> >so, can't the code just be moved into this #ifdef block later?
> >
> 
> Yes we could, infact I thought about it;
> but I would break recommended sequence here.

And did you set the "own slave address" to a value which one of your
existing i2c slaves also has (without enabling slave mode)? Did it
disturb communication?


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH-v3 02/11] i2c: pxa: No need to set slave addr for i2c master mode reset

[Vaibhav Hiremath]

On Friday 10 July 2015 07:44 PM, Wolfram Sang wrote:
> On Fri, Jul 10, 2015 at 06:08:43PM +0530, Vaibhav Hiremath wrote:
>>
>>
>> On Friday 10 July 2015 01:41 PM, Wolfram Sang wrote:
>>> On Tue, Jul 07, 2015 at 12:54:46AM +0530, Vaibhav Hiremath wrote:
>>>> 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.
>>>
>>> Just to make sure: Does it? As I read the code, slave interrupts are
>>> enabled later only when slave mode is selected? Is that a HW bug? And if
>>> so, can't the code just be moved into this #ifdef block later?
>>>
>>
>> Yes we could, infact I thought about it;
>> but I would break recommended sequence here.
>
> And did you set the "own slave address" to a value which one of your
> existing i2c slaves also has (without enabling slave mode)? Did it
> disturb communication?
>

Since slave and master mode are mutual exclusive,
I did not try this.

But I can try.

Thanks,
Vaibhav
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH-v3 02/11] i2c: pxa: No need to set slave addr for i2c master mode reset

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1319 bytes --]

On Fri, Jul 10, 2015 at 07:55:31PM +0530, Vaibhav Hiremath wrote:
> 
> 
> On Friday 10 July 2015 07:44 PM, Wolfram Sang wrote:
> >On Fri, Jul 10, 2015 at 06:08:43PM +0530, Vaibhav Hiremath wrote:
> >>
> >>
> >>On Friday 10 July 2015 01:41 PM, Wolfram Sang wrote:
> >>>On Tue, Jul 07, 2015 at 12:54:46AM +0530, Vaibhav Hiremath wrote:
> >>>>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.
> >>>
> >>>Just to make sure: Does it? As I read the code, slave interrupts are
> >>>enabled later only when slave mode is selected? Is that a HW bug? And if
> >>>so, can't the code just be moved into this #ifdef block later?
> >>>
> >>
> >>Yes we could, infact I thought about it;
> >>but I would break recommended sequence here.
> >
> >And did you set the "own slave address" to a value which one of your
> >existing i2c slaves also has (without enabling slave mode)? Did it
> >disturb communication?
> >
> 
> Since slave and master mode are mutual exclusive,
> I did not try this.

Ehrm, what I meant was. Did you see the issue mentioned in the above
commit message? Can you reproduce it? You don't need to enable slave
mode for that, no?


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH-v3 02/11] i2c: pxa: No need to set slave addr for i2c master mode reset

[Vaibhav Hiremath]

On Friday 10 July 2015 08:26 PM, Wolfram Sang wrote:
> On Fri, Jul 10, 2015 at 07:55:31PM +0530, Vaibhav Hiremath wrote:
>>
>>
>> On Friday 10 July 2015 07:44 PM, Wolfram Sang wrote:
>>> On Fri, Jul 10, 2015 at 06:08:43PM +0530, Vaibhav Hiremath wrote:
>>>>
>>>>
>>>> On Friday 10 July 2015 01:41 PM, Wolfram Sang wrote:
>>>>> On Tue, Jul 07, 2015 at 12:54:46AM +0530, Vaibhav Hiremath wrote:
>>>>>> 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.
>>>>>
>>>>> Just to make sure: Does it? As I read the code, slave interrupts are
>>>>> enabled later only when slave mode is selected? Is that a HW bug? And if
>>>>> so, can't the code just be moved into this #ifdef block later?
>>>>>
>>>>
>>>> Yes we could, infact I thought about it;
>>>> but I would break recommended sequence here.
>>>
>>> And did you set the "own slave address" to a value which one of your
>>> existing i2c slaves also has (without enabling slave mode)? Did it
>>> disturb communication?
>>>
>>
>> Since slave and master mode are mutual exclusive,
>> I did not try this.
>
> Ehrm, what I meant was. Did you see the issue mentioned in the above
> commit message? Can you reproduce it? You don't need to enable slave
> mode for that, no?
>

Ohh yes,

I am able to reproduce this issue.

I have pxa1928 based board, where I have only PMIC 88PM860 connected to
I2C bus, and whenever I set slave address to 0x30 (PMIC slave address)
all I2C transaction for PMIC goes for toss.


Thanks,
Vaibhav

[PATCH-v3 03/11] i2c: pxa: Return I2C_RETRY when timeout in pio mode

[Vaibhav Hiremath]

From: Shouming Wang <wangshm-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>

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 <wangshm-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
[vaibhav.hiremath-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org: Updated changelog]
Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Acked-by: Robert Jarzmik <robert.jarzmik-GANU6spQydw@public.gmane.org>
---
 drivers/i2c/busses/i2c-pxa.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
index 023e59f..632008f 100644
--- a/drivers/i2c/busses/i2c-pxa.c
+++ b/drivers/i2c/busses/i2c-pxa.c
@@ -745,8 +745,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;
 }
-- 
1.9.1

[PATCH-v3 09/11] i2c: pxa: Add support for pxa910/988 & new configuration features

[Vaibhav Hiremath]

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".

As discussed in the previous patch-series, the idea here is to add standard
DT properties for ilcr and iwcr configuration fields.
In case of Master ilcr is used for low/high time and in case of slave mode
of operation iwcr is used for setup/hold time.

Signed-off-by: Jett.Zhou <jtzhou-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
Signed-off-by: Yi Zhang <yizhang-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/i2c/busses/i2c-pxa.c | 42 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 41 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
index dfd1dd0..d5cf6f5 100644
--- a/drivers/i2c/busses/i2c-pxa.c
+++ b/drivers/i2c/busses/i2c-pxa.c
@@ -46,12 +46,15 @@ struct pxa_reg_layout {
 	u32 icr;
 	u32 isr;
 	u32 isar;
+	u32 ilcr;
+	u32 iwcr;
 };
 
 enum pxa_i2c_types {
 	REGS_PXA2XX,
 	REGS_PXA3XX,
 	REGS_CE4100,
+	REGS_PXA910,
 };
 
 /*
@@ -79,12 +82,22 @@ static struct pxa_reg_layout pxa_reg_layout[] = {
 		.isr =	0x04,
 		/* no isar register */
 	},
+	[REGS_PXA910] = {
+		.ibmr = 0x00,
+		.idbr = 0x08,
+		.icr =	0x10,
+		.isr =	0x18,
+		.isar = 0x20,
+		.ilcr = 0x28,
+		.iwcr = 0x30,
+	},
 };
 
 static const struct platform_device_id i2c_pxa_id_table[] = {
 	{ "pxa2xx-i2c",		REGS_PXA2XX },
 	{ "pxa3xx-pwri2c",	REGS_PXA3XX },
 	{ "ce4100-i2c",		REGS_CE4100 },
+	{ "pxa910-i2c",		REGS_PXA910 },
 	{ },
 };
 MODULE_DEVICE_TABLE(platform, i2c_pxa_id_table);
@@ -124,6 +137,24 @@ MODULE_DEVICE_TABLE(platform, i2c_pxa_id_table);
 #define ISR_SAD		(1 << 9)	   /* slave address detected */
 #define ISR_BED		(1 << 10)	   /* bus error no ACK/NAK */
 
+/* bit field shift & mask */
+#define ILCR_SLV_SHIFT		0
+#define ILCR_SLV_MASK		(0x1FF << ILCR_SLV_SHIFT)
+#define ILCR_FLV_SHIFT		9
+#define ILCR_FLV_MASK		(0x1FF << ILCR_FLV_SHIFT)
+#define ILCR_HLVL_SHIFT		18
+#define ILCR_HLVL_MASK		(0x1FF << ILCR_HLVL_SHIFT)
+#define ILCR_HLVH_SHIFT		27
+#define ILCR_HLVH_MASK		(0x1F << ILCR_HLVH_SHIFT)
+
+#define IWCR_CNT_SHIFT		0
+#define IWCR_CNT_MASK		(0x1F << IWCR_CNT_SHIFT)
+#define IWCR_HS_CNT1_SHIFT	5
+#define IWCR_HS_CNT1_MASK	(0x1F << IWCR_HS_CNT1_SHIFT)
+#define IWCR_HS_CNT2_SHIFT	10
+#define IWCR_HS_CNT2_MASK	(0x1F << IWCR_HS_CNT2_SHIFT)
+
+
 struct pxa_i2c {
 	spinlock_t		lock;
 	wait_queue_head_t	wait;
@@ -150,6 +181,8 @@ struct pxa_i2c {
 	void __iomem		*reg_icr;
 	void __iomem		*reg_isr;
 	void __iomem		*reg_isar;
+	void __iomem		*reg_ilcr;
+	void __iomem		*reg_iwcr;
 
 	unsigned long		iobase;
 	unsigned long		iosize;
@@ -169,6 +202,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
@@ -1135,7 +1170,7 @@ static const struct i2c_algorithm i2c_pxa_pio_algorithm = {
 static const struct of_device_id i2c_pxa_dt_ids[] = {
 	{ .compatible = "mrvl,pxa-i2c", .data = (void *)REGS_PXA2XX },
 	{ .compatible = "mrvl,pwri2c", .data = (void *)REGS_PXA3XX },
-	{ .compatible = "mrvl,mmp-twsi", .data = (void *)REGS_PXA2XX },
+	{ .compatible = "mrvl,mmp-twsi", .data = (void *)REGS_PXA910 },
 	{}
 };
 MODULE_DEVICE_TABLE(of, i2c_pxa_dt_ids);
@@ -1250,6 +1285,11 @@ static int i2c_pxa_probe(struct platform_device *dev)
 	if (i2c_type != REGS_CE4100)
 		i2c->reg_isar = i2c->reg_base + pxa_reg_layout[i2c_type].isar;
 
+	if (i2c_type == REGS_PXA910) {
+		i2c->reg_ilcr = i2c->reg_base + pxa_reg_layout[i2c_type].ilcr;
+		i2c->reg_iwcr = i2c->reg_base + pxa_reg_layout[i2c_type].iwcr;
+	}
+
 	i2c->iobase = res->start;
 	i2c->iosize = resource_size(res);
 
-- 
1.9.1

[PATCH-v3 04/11] i2c: pxa: Remove compile warnning in 64bit mode

[Vaibhav Hiremath]

From: Yipeng Yao <ypyao@marvell.com>

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]

Signed-off-by: Yipeng Yao <ypyao@marvell.com>
[vaibhav.hiremath@linaro.org: Updated Changelog]
Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
Cc: Wolfram Sang <wsa@the-dreams.de>
Acked-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
 drivers/i2c/busses/i2c-pxa.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
index 632008f..4c92694 100644
--- a/drivers/i2c/busses/i2c-pxa.c
+++ b/drivers/i2c/busses/i2c-pxa.c
@@ -1116,7 +1116,9 @@ 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);
+
 	return 0;
 }
 
-- 
1.9.1

Re: [PATCH-v3 04/11] i2c: pxa: Remove compile warnning in 64bit mode

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1422 bytes --]

On Tue, Jul 07, 2015 at 12:54:48AM +0530, Vaibhav Hiremath wrote:
> From: Yipeng Yao <ypyao-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
> 
> 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]
> 
> Signed-off-by: Yipeng Yao <ypyao-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
> [vaibhav.hiremath-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org: Updated Changelog]
> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Cc: Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>
> Acked-by: Robert Jarzmik <robert.jarzmik-GANU6spQydw@public.gmane.org>

Huh? Why long? Shouldn't that be casted to enum pxa_i2c_types?

> ---
>  drivers/i2c/busses/i2c-pxa.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
> index 632008f..4c92694 100644
> --- a/drivers/i2c/busses/i2c-pxa.c
> +++ b/drivers/i2c/busses/i2c-pxa.c
> @@ -1116,7 +1116,9 @@ 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);
> +
>  	return 0;
>  }
>  
> -- 
> 1.9.1
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH-v3 04/11] i2c: pxa: Remove compile warnning in 64bit mode

[Vaibhav Hiremath]

On Friday 10 July 2015 01:43 PM, Wolfram Sang wrote:
> On Tue, Jul 07, 2015 at 12:54:48AM +0530, Vaibhav Hiremath wrote:
>> From: Yipeng Yao <ypyao-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
>>
>> 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]
>>
>> Signed-off-by: Yipeng Yao <ypyao-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
>> [vaibhav.hiremath-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org: Updated Changelog]
>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> Cc: Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>
>> Acked-by: Robert Jarzmik <robert.jarzmik-GANU6spQydw@public.gmane.org>
>
> Huh? Why long? Shouldn't that be casted to enum pxa_i2c_types?
>

My bad.
Indeed it can be (and should be) enum pxa_i2c_types.

Will wait to close on the discussion on slave mode patch PATCH [02/11]
before pushing V4.


Thanks,
Vaibhav

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH-v3 04/11] i2c: pxa: Remove compile warnning in 64bit mode

[Uwe Kleine-König]

Hello,

$Subject ~= s/warnning/warning/

and I'd rather write "Fix" instead of "Remove".

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH-v3 05/11] i2c: pxa: Update debug function to dump more info on error

[Vaibhav Hiremath]

Update i2c_pxa_scream_blue_murder() fn to print more information
in case of error.
Also, use dev_err variants instead of printk.

Signed-off-by: Jett.Zhou <jtzhou@marvell.com>
Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
Cc: Wolfram Sang <wsa@the-dreams.de>
---
 drivers/i2c/busses/i2c-pxa.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
index 4c92694..040effa 100644
--- a/drivers/i2c/busses/i2c-pxa.c
+++ b/drivers/i2c/busses/i2c-pxa.c
@@ -132,6 +132,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;
@@ -253,15 +254,20 @@ static void i2c_pxa_show_state(struct pxa_i2c *i2c, int lno, const char *fname)
 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: msg_num: %d msg_idx: %d msg_ptr: %d\n",
+	struct device *dev = &i2c->adap.dev;
+
+	dev_err(dev, "slave_0x%x error: %s\n",
+		i2c->req_slave_addr >> 1, why);
+	dev_err(dev, "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_DEBUG "i2c: log: ");
+	dev_err(dev, "IBMR: %08x IDBR: %08x ICR: %08x ISR: %08x\n",
+		readl(_IBMR(i2c)), readl(_IDBR(i2c)), readl(_ICR(i2c)),
+		readl(_ISR(i2c)));
+	dev_dbg(dev, "log: ");
 	for (i = 0; i < i2c->irqlogidx; i++)
-		printk("[%08x:%08x] ", i2c->isrlog[i], i2c->icrlog[i]);
-	printk("\n");
+		pr_debug("[%08x:%08x] ", i2c->isrlog[i], i2c->icrlog[i]);
+
+	pr_debug("\n");
 }
 
 #else /* ifdef DEBUG */
@@ -638,6 +644,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.
@@ -951,6 +958,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.
-- 
1.9.1

[PATCH-v3 06/11] i2c:pxa: Use devm_ variants in probe function

[Vaibhav Hiremath]

This 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

Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
Acked-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
 drivers/i2c/busses/i2c-pxa.c | 82 ++++++++++++++++++--------------------------
 1 file changed, 34 insertions(+), 48 deletions(-)

diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
index 040effa..2de9300 100644
--- a/drivers/i2c/busses/i2c-pxa.c
+++ b/drivers/i2c/busses/i2c-pxa.c
@@ -1158,10 +1158,22 @@ 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);
+	if (!res) {
+		dev_err(&dev->dev, "no mem resource\n");
+		return -ENODEV;
+	}
+
+	irq = platform_get_irq(dev, 0);
+	if (irq < 0) {
+		dev_err(&dev->dev, "no irq resource: %d\n", irq);
+		return irq;
 	}
 
 	/* Default adapter num to device id; i2c_pxa_probe_dt can override. */
@@ -1171,19 +1183,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;
@@ -1193,16 +1193,18 @@ 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: %ld\n",
+			PTR_ERR(i2c->clk));
+		return PTR_ERR(i2c->clk);
 	}
 
-	i2c->reg_base = ioremap(res->start, resource_size(res));
-	if (!i2c->reg_base) {
-		ret = -EIO;
-		goto eremap;
+	i2c->reg_base = devm_ioremap_resource(&dev->dev, res);
+	if (IS_ERR(i2c->reg_base)) {
+		dev_err(&dev->dev, "failed to map resource: %ld\n",
+			PTR_ERR(i2c->reg_base));
+		return PTR_ERR(i2c->reg_base);
 	}
 
 	i2c->reg_ibmr = i2c->reg_base + pxa_reg_layout[i2c_type].ibmr;
@@ -1244,11 +1246,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: %d\n", ret);
 			goto ereqirq;
+		}
 	}
 
 	i2c_pxa_reset(i2c);
@@ -1261,33 +1265,22 @@ 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: %d\n", ret);
+		goto ereqirq;
 	}
 
 	platform_set_drvdata(dev, i2c);
 
 #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);
+	dev_info(&i2c->adap.dev, " PXA I2C adapter, slave address %d\n",
+		i2c->slave_addr);
 #else
-	printk(KERN_INFO "I2C: %s: PXA I2C adapter\n",
-	       dev_name(&i2c->adap.dev));
+	dev_info(&i2c->adap.dev, " PXA I2C adapter\n");
 #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;
 }
 
@@ -1296,15 +1289,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;
 }
-- 
1.9.1

Re: [PATCH-v3 06/11] i2c:pxa: Use devm_ variants in probe function

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 721 bytes --]

> -	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");

No message here, we get a dump anyhow.

> +	res = platform_get_resource(dev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(&dev->dev, "no mem resource\n");
> +		return -ENODEV;
> +	}

You can skip this check, devm_ioremap_resource will do it.

> +		dev_err(&dev->dev, "failed to get the clk: %ld\n",
> +			PTR_ERR(i2c->clk));

Sidenote for all patches: I am not so strict with the 80 char limit. I'd
think the above code would be more readable if it was one line. But you
decide.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH-v3 07/11] i2c: pxa: enable/disable i2c module across msg xfer

[Vaibhav Hiremath]

From: Yi Zhang <yizhang@marvell.com>

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.

As we do not want to break slave mode support, this patch introduces
DT property to control disable of the I2C module after xfer in master mode
of operation.

i2c-disable-after-xfer : If set, driver will disable I2C module after msg xfer

Signed-off-by: Yi Zhang <yizhang@marvell.com>
Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
---
 drivers/i2c/busses/i2c-pxa.c | 43 +++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 41 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
index 2de9300..dfd1dd0 100644
--- a/drivers/i2c/busses/i2c-pxa.c
+++ b/drivers/i2c/busses/i2c-pxa.c
@@ -161,6 +161,7 @@ struct pxa_i2c {
 	unsigned char		master_code;
 	unsigned long		rate;
 	bool			highmode_enter;
+	bool			disable_after_xfer;
 };
 
 #define _IBMR(i2c)	((i2c)->reg_ibmr)
@@ -284,6 +285,24 @@ 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 int i2c_pxa_is_enabled(struct pxa_i2c *i2c)
+{
+	return (readl(_ICR(i2c)) & ICR_IUE);
+}
+
+static inline void i2c_pxa_enable(struct pxa_i2c *i2c, bool enable)
+{
+	if (enable) {
+		if (!i2c_pxa_is_enabled(i2c)) {
+			writel(readl(_ICR(i2c)) | ICR_IUE, _ICR(i2c));
+			udelay(100);
+		}
+	} else {
+		writel(readl(_ICR(i2c)) & ~ICR_IUE, _ICR(i2c));
+	}
+}
+
 static inline int i2c_pxa_is_slavemode(struct pxa_i2c *i2c)
 {
 	return !(readl(_ICR(i2c)) & ICR_SCLE);
@@ -480,8 +499,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);
 }
 
 
@@ -832,6 +850,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
@@ -852,6 +873,11 @@ static int i2c_pxa_pio_xfer(struct i2c_adapter *adap,
 	ret = -EREMOTEIO;
  out:
 	i2c_pxa_set_slave(i2c, ret);
+
+	/* disable i2c unit */
+	if (i2c->disable_after_xfer)
+		i2c_pxa_enable(i2c, false);
+
 	return ret;
 }
 
@@ -1067,6 +1093,9 @@ static int i2c_pxa_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num
 	struct pxa_i2c *i2c = adap->algo_data;
 	int ret, i;
 
+	/* Enable i2c unit */
+	i2c_pxa_enable(i2c, true);
+
 	for (i = adap->retries; i >= 0; i--) {
 		ret = i2c_pxa_do_xfer(i2c, msgs, num);
 		if (ret != I2C_RETRY)
@@ -1080,6 +1109,10 @@ 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 i2c unit */
+	if (i2c->disable_after_xfer)
+		i2c_pxa_enable(i2c, false);
+
 	return ret;
 }
 
@@ -1120,6 +1153,9 @@ static int i2c_pxa_probe_dt(struct platform_device *pdev, struct pxa_i2c *i2c,
 	/* For device tree we always use the dynamic or alias-assigned ID */
 	i2c->adap.nr = -1;
 
+	i2c->disable_after_xfer = of_property_read_bool(np,
+				"i2c-disable-after-xfer");
+
 	if (of_get_property(np, "mrvl,i2c-polling", NULL))
 		i2c->use_pio = 1;
 	if (of_get_property(np, "mrvl,i2c-fast-mode", NULL))
@@ -1271,6 +1307,9 @@ static int i2c_pxa_probe(struct platform_device *dev)
 
 	platform_set_drvdata(dev, i2c);
 
+	if (i2c->disable_after_xfer)
+		i2c_pxa_enable(i2c, false);
+
 #ifdef CONFIG_I2C_PXA_SLAVE
 	dev_info(&i2c->adap.dev, " PXA I2C adapter, slave address %d\n",
 		i2c->slave_addr);
-- 
1.9.1

Re: [PATCH-v3 07/11] i2c: pxa: enable/disable i2c module across msg xfer

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 4677 bytes --]

On Tue, Jul 07, 2015 at 12:54:51AM +0530, Vaibhav Hiremath wrote:
> From: Yi Zhang <yizhang-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
> 
> 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.
> 
> As we do not want to break slave mode support, this patch introduces
> DT property to control disable of the I2C module after xfer in master mode
> of operation.
> 
> i2c-disable-after-xfer : If set, driver will disable I2C module after msg xfer

Hmm, I am not sure this property fits into the "describing hardware"
category. And we can't make the below behaviour default, because the
udelay(100) will cause quite some latency.

> 
> Signed-off-by: Yi Zhang <yizhang-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-pxa.c | 43 +++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 41 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
> index 2de9300..dfd1dd0 100644
> --- a/drivers/i2c/busses/i2c-pxa.c
> +++ b/drivers/i2c/busses/i2c-pxa.c
> @@ -161,6 +161,7 @@ struct pxa_i2c {
>  	unsigned char		master_code;
>  	unsigned long		rate;
>  	bool			highmode_enter;
> +	bool			disable_after_xfer;
>  };
>  
>  #define _IBMR(i2c)	((i2c)->reg_ibmr)
> @@ -284,6 +285,24 @@ 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 int i2c_pxa_is_enabled(struct pxa_i2c *i2c)
> +{
> +	return (readl(_ICR(i2c)) & ICR_IUE);
> +}
> +
> +static inline void i2c_pxa_enable(struct pxa_i2c *i2c, bool enable)
> +{
> +	if (enable) {
> +		if (!i2c_pxa_is_enabled(i2c)) {
> +			writel(readl(_ICR(i2c)) | ICR_IUE, _ICR(i2c));
> +			udelay(100);
> +		}
> +	} else {
> +		writel(readl(_ICR(i2c)) & ~ICR_IUE, _ICR(i2c));
> +	}
> +}
> +
>  static inline int i2c_pxa_is_slavemode(struct pxa_i2c *i2c)
>  {
>  	return !(readl(_ICR(i2c)) & ICR_SCLE);
> @@ -480,8 +499,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);
>  }
>  
>  
> @@ -832,6 +850,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
> @@ -852,6 +873,11 @@ static int i2c_pxa_pio_xfer(struct i2c_adapter *adap,
>  	ret = -EREMOTEIO;
>   out:
>  	i2c_pxa_set_slave(i2c, ret);
> +
> +	/* disable i2c unit */
> +	if (i2c->disable_after_xfer)
> +		i2c_pxa_enable(i2c, false);
> +
>  	return ret;
>  }
>  
> @@ -1067,6 +1093,9 @@ static int i2c_pxa_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num
>  	struct pxa_i2c *i2c = adap->algo_data;
>  	int ret, i;
>  
> +	/* Enable i2c unit */
> +	i2c_pxa_enable(i2c, true);
> +
>  	for (i = adap->retries; i >= 0; i--) {
>  		ret = i2c_pxa_do_xfer(i2c, msgs, num);
>  		if (ret != I2C_RETRY)
> @@ -1080,6 +1109,10 @@ 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 i2c unit */
> +	if (i2c->disable_after_xfer)
> +		i2c_pxa_enable(i2c, false);
> +
>  	return ret;
>  }
>  
> @@ -1120,6 +1153,9 @@ static int i2c_pxa_probe_dt(struct platform_device *pdev, struct pxa_i2c *i2c,
>  	/* For device tree we always use the dynamic or alias-assigned ID */
>  	i2c->adap.nr = -1;
>  
> +	i2c->disable_after_xfer = of_property_read_bool(np,
> +				"i2c-disable-after-xfer");
> +
>  	if (of_get_property(np, "mrvl,i2c-polling", NULL))
>  		i2c->use_pio = 1;
>  	if (of_get_property(np, "mrvl,i2c-fast-mode", NULL))
> @@ -1271,6 +1307,9 @@ static int i2c_pxa_probe(struct platform_device *dev)
>  
>  	platform_set_drvdata(dev, i2c);
>  
> +	if (i2c->disable_after_xfer)
> +		i2c_pxa_enable(i2c, false);
> +
>  #ifdef CONFIG_I2C_PXA_SLAVE
>  	dev_info(&i2c->adap.dev, " PXA I2C adapter, slave address %d\n",
>  		i2c->slave_addr);
> -- 
> 1.9.1
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH-v3 07/11] i2c: pxa: enable/disable i2c module across msg xfer

[Vaibhav Hiremath]

On Friday 10 July 2015 01:54 PM, Wolfram Sang wrote:
> On Tue, Jul 07, 2015 at 12:54:51AM +0530, Vaibhav Hiremath wrote:
>> From: Yi Zhang <yizhang-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
>>
>> 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.
>>
>> As we do not want to break slave mode support, this patch introduces
>> DT property to control disable of the I2C module after xfer in master mode
>> of operation.
>>
>> i2c-disable-after-xfer : If set, driver will disable I2C module after msg xfer
>
> Hmm, I am not sure this property fits into the "describing hardware"
> category. And we can't make the below behaviour default, because the
> udelay(100) will cause quite some latency.
>
>>

Sorry for delayed response,
I thought I responded to this, but when I was preparing for V4,
realized it that I did not :)

Yes, I agree that, this doesn't look like HW description. But I could
not find any other mechanism to handle this.
And also during V1 discussion, I did not get any objection to this
approach.


Thanks,
Vaibhav

[PATCH-v3 08/11] Documentation: binding: add new property 'disable_after_xfer' to i2c-pxa

[Vaibhav Hiremath]

Driver now supports enable/disable across msg xfer, which user
can control it by new DT property -

i2c-disable-after-xfer : If set, driver will disable I2C module after msg
 xfer and enable it back before xfer.

Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
---
 Documentation/devicetree/bindings/i2c/i2c-pxa.txt | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-pxa.txt b/Documentation/devicetree/bindings/i2c/i2c-pxa.txt
index 12b78ac..9657db5 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-pxa.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-pxa.txt
@@ -18,6 +18,11 @@ Recommended properties :
    status register of i2c controller instead.
  - mrvl,i2c-fast-mode : Enable fast mode of i2c controller.
 
+Optional properties :
+
+ - i2c-disable-after-xfer : If set, driver will disable I2C module
+   after msg xfer and enable it again before xfer.
+
 Examples:
 	twsi1: i2c@d4011000 {
 		compatible = "mrvl,mmp-twsi";
-- 
1.9.1

[PATCH-v3 10/11] i2c: pxa: Add ILCR (tLow & tHigh) configuration support

[Vaibhav Hiremath]

With addition of PXA910 family of devices, the TWSI module supports
SCL clock adjustment using ILCR register.

This patch enables the control and configuration of ICLR through DT
properties,

i2c-sclk-high-time-ns:
  SCLK high time (tHigh), for standard/fast/high speed mode
i2c-sclk-low-time-ns:
  SCLK low time (tLow), for standard/fast/high speed mode

Note that in case of standard and fast mod, the tLow and tHigh counters
are same, and software will use tLow value.

Also, brought up devm_clk_get() fn above i2c_pxa_probe_dt(), as it
uses clk rate for timing calculations.

Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
Signed-off-by: Jett.Zhou <jtzhou@marvell.com>
Signed-off-by: Yi Zhang <yizhang@marvell.com>
---
 drivers/i2c/busses/i2c-pxa.c | 68 +++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 61 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
index d5cf6f5..537555a 100644
--- a/drivers/i2c/busses/i2c-pxa.c
+++ b/drivers/i2c/busses/i2c-pxa.c
@@ -195,6 +195,9 @@ struct pxa_i2c {
 	unsigned long		rate;
 	bool			highmode_enter;
 	bool			disable_after_xfer;
+
+	unsigned int		sclk_thigh_load_cnt;
+	unsigned int		sclk_tlow_load_cnt;
 };
 
 #define _IBMR(i2c)	((i2c)->reg_ibmr)
@@ -507,6 +510,33 @@ static void i2c_pxa_set_slave(struct pxa_i2c *i2c, int errcode)
 #define i2c_pxa_set_slave(i2c, err)	do { } while (0)
 #endif
 
+static void i2c_pxa_do_sclk_adj(struct pxa_i2c *i2c)
+{
+	unsigned int reg_ilcr;
+
+	reg_ilcr = readl(_ILCR(i2c));
+
+	/* For standard/fast mode tlow and thigh counters are same */
+	if (i2c->sclk_tlow_load_cnt) {
+		unsigned int mask, shift;
+
+		mask = i2c->high_mode ? ILCR_HLVL_MASK :
+			i2c->fast_mode ? ILCR_FLV_MASK : ILCR_SLV_MASK;
+		shift = i2c->high_mode ? ILCR_HLVL_SHIFT :
+			i2c->fast_mode ? ILCR_FLV_SHIFT : ILCR_SLV_SHIFT;
+
+		reg_ilcr &= ~mask;
+		reg_ilcr |= i2c->sclk_tlow_load_cnt << shift;
+	}
+
+	if (i2c->high_mode && i2c->sclk_thigh_load_cnt) {
+		reg_ilcr &= ~ILCR_HLVH_MASK;
+		reg_ilcr |= i2c->sclk_thigh_load_cnt << ILCR_HLVH_SHIFT;
+	}
+
+	writel(reg_ilcr, _ILCR(i2c));
+}
+
 static void i2c_pxa_reset(struct pxa_i2c *i2c)
 {
 	pr_debug("Resetting I2C Controller Unit\n");
@@ -526,6 +556,8 @@ static void i2c_pxa_reset(struct pxa_i2c *i2c)
 	writel(I2C_ICR_INIT | (i2c->fast_mode ? ICR_FM : 0), _ICR(i2c));
 	writel(readl(_ICR(i2c)) | (i2c->high_mode ? ICR_HS : 0), _ICR(i2c));
 
+	i2c_pxa_do_sclk_adj(i2c);
+
 #ifdef CONFIG_I2C_PXA_SLAVE
 	dev_info(&i2c->adap.dev, "Enabling slave mode\n");
 	writel(readl(_ICR(i2c)) | ICR_SADIE | ICR_ALDIE | ICR_SSDIE, _ICR(i2c));
@@ -1198,6 +1230,26 @@ static int i2c_pxa_probe_dt(struct platform_device *pdev, struct pxa_i2c *i2c,
 
 	*i2c_types = (long)(of_id->data);
 
+	/* optional properties */
+	if (of_device_is_compatible(np, "mrvl,mmp-twsi")) {
+		unsigned int tlow = 0, thigh = 0;
+		unsigned int clk_ns;
+
+		/* clock time in nsec */
+		clk_ns = 1000000 / (i2c->rate / 1000);
+
+		of_property_read_u32(np, "i2c-sclk-high-time-ns", &thigh);
+		i2c->sclk_thigh_load_cnt = thigh / clk_ns;
+
+		of_property_read_u32(np, "i2c-sclk-low-time-ns", &tlow);
+		i2c->sclk_tlow_load_cnt = tlow / clk_ns;
+
+		/* For std/fast mode tlow & thigh have same bit-fields */
+		if (!i2c->high_mode &&
+			(i2c->sclk_tlow_load_cnt != i2c->sclk_thigh_load_cnt))
+			dev_warn(&i2c->adap.dev,
+				"mismatch of tLow & tHigh values, using tLow\n");
+	}
 	return 0;
 }
 
@@ -1247,6 +1299,15 @@ static int i2c_pxa_probe(struct platform_device *dev)
 		return irq;
 	}
 
+	i2c->clk = devm_clk_get(&dev->dev, NULL);
+	if (IS_ERR(i2c->clk)) {
+		dev_err(&dev->dev, "failed to get the clk: %ld\n",
+			PTR_ERR(i2c->clk));
+		return PTR_ERR(i2c->clk);
+	}
+
+	i2c->rate = clk_get_rate(i2c->clk);
+
 	/* Default adapter num to device id; i2c_pxa_probe_dt can override. */
 	i2c->adap.nr = dev->id;
 
@@ -1264,13 +1325,6 @@ static int i2c_pxa_probe(struct platform_device *dev)
 
 	strlcpy(i2c->adap.name, "pxa_i2c-i2c", sizeof(i2c->adap.name));
 
-	i2c->clk = devm_clk_get(&dev->dev, NULL);
-	if (IS_ERR(i2c->clk)) {
-		dev_err(&dev->dev, "failed to get the clk: %ld\n",
-			PTR_ERR(i2c->clk));
-		return PTR_ERR(i2c->clk);
-	}
-
 	i2c->reg_base = devm_ioremap_resource(&dev->dev, res);
 	if (IS_ERR(i2c->reg_base)) {
 		dev_err(&dev->dev, "failed to map resource: %ld\n",
-- 
1.9.1

[PATCH-v3 11/11] Documentation: binding: add sclk adjustment properties to i2c-pxa

[Vaibhav Hiremath]

With addition of PXA910 family of devices, the TWSI module supports
new feature which allows us to adjust SCLK. i2c-pxa driver takes input
configuration in nsec and converts it to respective bit-fields,

 - i2c-sclk-low-time-ns : SCLK low time (tlow)
   This property is used along with mode selection.
 - i2c-sclk-high-time-ns : SCLK high time (thigh)
 - i2c-start-hold-time-ns : Used in case of high speed mode for start bit
   hold/setup wait counter.
 - i2c-stop-hold-time-ns : Used in case of high speed mode for stop bit
   hold/setup wait counter.
 - i2c-sda-hold-time-ns : Used to calculate hold/setup wait counter for
   standard and fast mode.

Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
---
 Documentation/devicetree/bindings/i2c/i2c-pxa.txt | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-pxa.txt b/Documentation/devicetree/bindings/i2c/i2c-pxa.txt
index 9657db5..bb4df60 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-pxa.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-pxa.txt
@@ -23,12 +23,25 @@ Optional properties :
  - i2c-disable-after-xfer : If set, driver will disable I2C module
    after msg xfer and enable it again before xfer.
 
+   (Applicable to PXA910 family):
+
+ - i2c-sclk-low-time-ns : SCLK low time (tlow), for standard/fast/high
+   speed mode.
+   This property is used along with mode selection. Driver uses this property
+   to set low/high time for standard and fast speed mode, as HW counter
+   bit-field is same for both.
+ - i2c-sclk-high-time-ns : SCLK high time (thigh), Used in case of high speed
+   mode only.
+
 Examples:
 	twsi1: i2c@d4011000 {
 		compatible = "mrvl,mmp-twsi";
 		reg = <0xd4011000 0x1000>;
 		interrupts = <7>;
 		mrvl,i2c-fast-mode;
+
+		i2c-sclk-low-time-ns = <988>;
+		i2c-sclk-high-time-ns = <988>;
 	};
 	
 	twsi2: i2c@d4025000 {
-- 
1.9.1