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

[PATCH 00/12] 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:
  - 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(-)

-- 
1.9.1

[PATCH 01/12] 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 at linaro.org: updated Changelog]
Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
Cc: Wolfram Sang <wsa@the-dreams.de>

Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
---
 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 02/12] i2c: pxa: No need to set slave addr for i2c master mode reset

[Vaibhav Hiremath]

From: "Jett.Zhou" <jtzhou@marvell.com>

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>
[vaibhav.hiremath at linaro.org: Updated Changelog]
Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
Cc: Wolfram Sang <wsa@the-dreams.de>

Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
---
 drivers/i2c/busses/i2c-pxa.c | 2 ++
 1 file changed, 2 insertions(+)

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

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

[Robert Jarzmik]

Vaibhav Hiremath <vaibhav.hiremath@linaro.org> writes:

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

-- 
Robert

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

[Vaibhav Hiremath]

On Saturday 30 May 2015 12:51 AM, Robert Jarzmik wrote:
> Vaibhav Hiremath <vaibhav.hiremath@linaro.org> writes:
>
>> diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
>> index f4ac8c5..d4c798a 100644
>> --- a/drivers/i2c/busses/i2c-pxa.c
>> +++ b/drivers/i2c/busses/i2c-pxa.c
>> @@ -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));
>

Ok,
Will fix it in next version.

Thanks,
Vaibhav

[PATCH 03/12] i2c: pxa: Add reset operation when i2c bus busy

[Vaibhav Hiremath]

From: "Jett.Zhou" <jtzhou@marvell.com>

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 <jtzhou@marvell.com>
[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 <vaibhav.hiremath@linaro.org>

Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
---
 drivers/i2c/busses/i2c-pxa.c | 4 ++++
 1 file changed, 4 insertions(+)

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

[PATCH 03/12] i2c: pxa: Add reset operation when i2c bus busy

[Robert Jarzmik]

Vaibhav Hiremath <vaibhav.hiremath@linaro.org> writes:

> From: "Jett.Zhou" <jtzhou@marvell.com>
>
> 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 <jtzhou@marvell.com>
> [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 <vaibhav.hiremath@linaro.org>
>
> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
> ---
>  drivers/i2c/busses/i2c-pxa.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
> index d4c798a..a76c901 100644
> --- a/drivers/i2c/busses/i2c-pxa.c
> +++ b/drivers/i2c/busses/i2c-pxa.c
> @@ -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.

-- 
Robert

[PATCH 03/12] i2c: pxa: Add reset operation when i2c bus busy

[Vaibhav Hiremath]

On Saturday 30 May 2015 01:09 AM, Robert Jarzmik wrote:
> Vaibhav Hiremath <vaibhav.hiremath@linaro.org> writes:
>
>> From: "Jett.Zhou" <jtzhou@marvell.com>
>>
>> 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 <jtzhou@marvell.com>
>> [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 <vaibhav.hiremath@linaro.org>
>>
>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>> ---
>>   drivers/i2c/busses/i2c-pxa.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
>> index d4c798a..a76c901 100644
>> --- a/drivers/i2c/busses/i2c-pxa.c
>> +++ b/drivers/i2c/busses/i2c-pxa.c
>> @@ -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.


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?

> 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 04/12] i2c: pxa: Add support for pxa910/988 & new configuration features

[Vaibhav Hiremath]

From: "Jett.Zhou" <jtzhou@marvell.com>

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.

Signed-off-by: Jett.Zhou <jtzhou@marvell.com>
Signed-off-by: Yi Zhang <yizhang@marvell.com>
[vaibhav.hiremath at linaro.org: Updated kernel doc for new DT properties
and updated Changelog]
Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>

Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
---
 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(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-pxa.txt b/Documentation/devicetree/bindings/i2c/i2c-pxa.txt
index 12b78ac..5750bff 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-pxa.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-pxa.txt
@@ -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";
diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
index a76c901..8ca5552 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);
@@ -149,6 +162,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;
@@ -160,6 +175,8 @@ struct pxa_i2c {
 	unsigned char		master_code;
 	unsigned long		rate;
 	bool			highmode_enter;
+	unsigned int		ilcr;
+	unsigned int		iwcr;
 };
 
 #define _IBMR(i2c)	((i2c)->reg_ibmr)
@@ -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
@@ -467,11 +486,16 @@ static void i2c_pxa_reset(struct pxa_i2c *i2c)
 	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));
 	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);
+
 #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));
@@ -1098,7 +1122,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);
@@ -1106,6 +1130,7 @@ MODULE_DEVICE_TABLE(of, i2c_pxa_dt_ids);
 static int i2c_pxa_probe_dt(struct platform_device *pdev, struct pxa_i2c *i2c,
 			    enum pxa_i2c_types *i2c_types)
 {
+	int ret;
 	struct device_node *np = pdev->dev.of_node;
 	const struct of_device_id *of_id =
 			of_match_device(i2c_pxa_dt_ids, &pdev->dev);
@@ -1121,6 +1146,16 @@ static int i2c_pxa_probe_dt(struct platform_device *pdev, struct pxa_i2c *i2c,
 	if (of_get_property(np, "mrvl,i2c-fast-mode", NULL))
 		i2c->fast_mode = 1;
 	*i2c_types = (u32)(of_id->data);
+
+	if (of_device_is_compatible(np, "mrvl,mmp-twsi")) {
+		ret = of_property_read_u32(np, "mrvl,i2c-ilcr", &i2c->ilcr);
+		if (ret)
+			return ret;
+		ret = of_property_read_u32(np, "mrvl,i2c-iwcr", &i2c->iwcr);
+		if (ret)
+			return ret;
+	}
+
 	return 0;
 }
 
@@ -1206,6 +1241,9 @@ 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;
 
+	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);
 
@@ -1219,6 +1257,8 @@ static int i2c_pxa_probe(struct platform_device *dev)
 		i2c->slave_addr = plat->slave_addr;
 		i2c->slave = plat->slave;
 #endif
+		i2c->ilcr = plat->ilcr;
+		i2c->iwcr = plat->iwcr;
 		i2c->adap.class = plat->class;
 	}
 
diff --git a/include/linux/i2c/pxa-i2c.h b/include/linux/i2c/pxa-i2c.h
index 53aab24..d1a44e8 100644
--- a/include/linux/i2c/pxa-i2c.h
+++ b/include/linux/i2c/pxa-i2c.h
@@ -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);
-- 
1.9.1

[PATCH 04/12] i2c: pxa: Add support for pxa910/988 & new configuration features

[Robert Jarzmik]

Vaibhav Hiremath <vaibhav.hiremath@linaro.org> writes:

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

>  	/* 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.

-- 
Robert

[PATCH 04/12] i2c: pxa: Add support for pxa910/988 & new configuration features

[Vaibhav Hiremath]

On Saturday 30 May 2015 01:52 AM, Robert Jarzmik wrote:
> Vaibhav Hiremath <vaibhav.hiremath@linaro.org> writes:
>
>> @@ -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
>> @@ -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.

>
>>   	/* 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 ?
>

I am afraid it is random number picked, required since we are
configuring mode/speed related bit-fields.

Thanks,
Vaibhav

[PATCH 04/12] i2c: pxa: Add support for pxa910/988 & new configuration features

[Yi Zhang]

On Fri, May 29, 2015 at 10:22:27PM +0200, Robert Jarzmik wrote:
> Vaibhav Hiremath <vaibhav.hiremath@linaro.org> writes:
> 
> > @@ -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
> > @@ -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.
> 
> >  	/* 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 ?

  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.
> 
> Cheers.
> 
> -- 
> Robert

[PATCH 04/12] i2c: pxa: Add support for pxa910/988 & new configuration features

[Vaibhav Hiremath]

On Thursday 04 June 2015 08:01 AM, Yi Zhang wrote:
> On Fri, May 29, 2015 at 10:22:27PM +0200, Robert Jarzmik wrote:
>> Vaibhav Hiremath <vaibhav.hiremath@linaro.org> writes:
>>
>>> @@ -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
>>> @@ -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.
>>
>>>   	/* 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 ?
>
>    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 04/12] i2c: pxa: Add support for pxa910/988 & new configuration features

[Wolfram Sang]

On 2015-05-28 22:03, Vaibhav Hiremath wrote:
> From: "Jett.Zhou" <jtzhou@marvell.com>
>
> 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.

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 04/12] i2c: pxa: Add support for pxa910/988 & new configuration features

[Vaibhav Hiremath]

On Monday 01 June 2015 05:43 AM, Wolfram Sang wrote:
> On 2015-05-28 22:03, Vaibhav Hiremath wrote:
>> From: "Jett.Zhou" <jtzhou@marvell.com>
>>
>> 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.
>
> 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).
>
>


Thanks for your suggestion.

Let me check above documentation.

Thanks,
Vaibhav

[PATCH 05/12] i2c: pxa: Add bus reset functionality

[Vaibhav Hiremath]

From: Rob Herring <robh@kernel.org>

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 <shangll@marvell.com>
Signed-off-by: Rob Herring <robh@kernel.org>
[vaibhav.hiremath at linaro.org: Updated Changelog]
Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>

Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
---
 drivers/i2c/busses/i2c-pxa.c | 90 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 90 insertions(+)

diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
index 8ca5552..eb09071 100644
--- a/drivers/i2c/busses/i2c-pxa.c
+++ b/drivers/i2c/busses/i2c-pxa.c
@@ -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);
-- 
1.9.1

[PATCH 05/12] i2c: pxa: Add bus reset functionality

[Rob Herring]

On Thu, May 28, 2015 at 8:03 AM, Vaibhav Hiremath
<vaibhav.hiremath@linaro.org> wrote:
> From: Rob Herring <robh@kernel.org>

This probably should still be Leilei, but...

> 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 <shangll@marvell.com>
> Signed-off-by: Rob Herring <robh@kernel.org>
> [vaibhav.hiremath at linaro.org: Updated Changelog]
> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>
> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
> ---
>  drivers/i2c/busses/i2c-pxa.c | 90 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 90 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
> index 8ca5552..eb09071 100644
> --- a/drivers/i2c/busses/i2c-pxa.c
> +++ b/drivers/i2c/busses/i2c-pxa.c
> @@ -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)

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 05/12] i2c: pxa: Add bus reset functionality

[Vaibhav Hiremath]

On Friday 29 May 2015 07:29 PM, Rob Herring wrote:
> On Thu, May 28, 2015 at 8:03 AM, Vaibhav Hiremath
> <vaibhav.hiremath@linaro.org> wrote:
>> From: Rob Herring <robh@kernel.org>
>
> 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.

>> 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 <shangll@marvell.com>
>> Signed-off-by: Rob Herring <robh@kernel.org>
>> [vaibhav.hiremath at linaro.org: Updated Changelog]
>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>>
>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>> ---
>>   drivers/i2c/busses/i2c-pxa.c | 90 ++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 90 insertions(+)
>>
>> diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
>> index 8ca5552..eb09071 100644
>> --- a/drivers/i2c/busses/i2c-pxa.c
>> +++ b/drivers/i2c/busses/i2c-pxa.c
>> @@ -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)
>
> 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.
>


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 05/12] i2c: pxa: Add bus reset functionality

[Robert Jarzmik]

Vaibhav Hiremath <vaibhav.hiremath@linaro.org> writes:

> From: Rob Herring <robh@kernel.org>
>
> 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 <shangll@marvell.com>
> Signed-off-by: Rob Herring <robh@kernel.org>
> [vaibhav.hiremath at linaro.org: Updated Changelog]
> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>
> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
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

> ---
>  drivers/i2c/busses/i2c-pxa.c | 90 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 90 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
> index 8ca5552..eb09071 100644
> --- a/drivers/i2c/busses/i2c-pxa.c
> +++ b/drivers/i2c/busses/i2c-pxa.c
> @@ -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);

-- 
Robert

[PATCH 05/12] i2c: pxa: Add bus reset functionality

[Linus Walleij]

On Thu, May 28, 2015 at 3:03 PM, Vaibhav Hiremath
<vaibhav.hiremath@linaro.org> wrote:

> From: Rob Herring <robh@kernel.org>
>
> 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 <shangll@marvell.com>
> Signed-off-by: Rob Herring <robh@kernel.org>
> [vaibhav.hiremath at linaro.org: Updated Changelog]
> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>
> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>

Double signed-off?

(...)
+#include <linux/of_gpio.h>

You should use <linux/gpio/consumer.h> and then use
GPIO descriptors instead.

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

> +static void i2c_bus_reset(struct pxa_i2c *i2c)
> +{
> +       int ret, ccnt, pins_scl, pins_sda;

Use GPIO descriptors.

struct gpio_desc *scl, *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;
> +       }

Exactly like that yes.

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

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

gpiod_put(scl);
gpiod_put(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;

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 05/12] i2c: pxa: Add bus reset functionality

[Vaibhav Hiremath]

On Tuesday 02 June 2015 06:42 PM, Linus Walleij wrote:
> On Thu, May 28, 2015 at 3:03 PM, Vaibhav Hiremath
> <vaibhav.hiremath@linaro.org> wrote:
>
>> From: Rob Herring <robh@kernel.org>
>>
>> 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 <shangll@marvell.com>
>> Signed-off-by: Rob Herring <robh@kernel.org>
>> [vaibhav.hiremath at linaro.org: Updated Changelog]
>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>>
>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>
> Double signed-off?
>
> (...)
> +#include <linux/of_gpio.h>
>
> You should use <linux/gpio/consumer.h> and then use
> GPIO descriptors instead.
>
>> @@ -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.
>
>> +static void i2c_bus_reset(struct pxa_i2c *i2c)
>> +{
>> +       int ret, ccnt, pins_scl, pins_sda;
>
> Use GPIO descriptors.
>
> struct gpio_desc *scl, *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;
>> +       }
>
> Exactly like that yes.
>
>> +       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...
>
>> +
>> +       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);
>
> gpiod_put(scl);
> gpiod_put(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;
>
> 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 05/12] i2c: pxa: Add bus reset functionality

[Vaibhav Hiremath]

On Tuesday 02 June 2015 06:42 PM, Linus Walleij wrote:
> On Thu, May 28, 2015 at 3:03 PM, Vaibhav Hiremath
> <vaibhav.hiremath@linaro.org> wrote:
>
>> From: Rob Herring <robh@kernel.org>
>>
>> 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 <shangll@marvell.com>
>> Signed-off-by: Rob Herring <robh@kernel.org>
>> [vaibhav.hiremath at linaro.org: Updated Changelog]
>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>>
>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>
> 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 05/12] i2c: pxa: Add bus reset functionality

[Wolfram Sang]

> 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 05/12] i2c: pxa: Add bus reset functionality

[Vaibhav Hiremath]

On Tuesday 02 June 2015 11:03 PM, Wolfram Sang wrote:
>
>> 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.
>


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 05/12] i2c: pxa: Add bus reset functionality

[Wolfram Sang]

> 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 05/12] i2c: pxa: Add bus reset functionality

[Vaibhav Hiremath]

On Tuesday 02 June 2015 11:18 PM, Wolfram Sang wrote:
>> 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 05/12] i2c: pxa: Add bus reset functionality

[Wolfram Sang]

> 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 05/12] i2c: pxa: Add bus reset functionality

[Vaibhav Hiremath]

On Tuesday 02 June 2015 11:32 PM, Wolfram Sang wrote:
>
>> 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 05/12] i2c: pxa: Add bus reset functionality

[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!
-------------- 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 05/12] i2c: pxa: Add bus reset functionality

[Vaibhav Hiremath]

On Tuesday 02 June 2015 11:54 PM, Wolfram Sang wrote:
> 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 06/12] i2c: pxa: Return I2C_RETRY when timeout in pio mode

[Vaibhav Hiremath]

From: Shouming Wang <wangshm@marvell.com>

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@marvell.com>
[vaibhav.hiremath at linaro.org: Updated changelog]
Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>

Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.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 eb09071..2777d5c 100644
--- a/drivers/i2c/busses/i2c-pxa.c
+++ b/drivers/i2c/busses/i2c-pxa.c
@@ -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;
 }
-- 
1.9.1

[PATCH 06/12] i2c: pxa: Return I2C_RETRY when timeout in pio mode

[Robert Jarzmik]

Vaibhav Hiremath <vaibhav.hiremath@linaro.org> writes:

> From: Shouming Wang <wangshm@marvell.com>
>
> 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@marvell.com>
> [vaibhav.hiremath at linaro.org: Updated changelog]
> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>
> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.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 eb09071..2777d5c 100644
> --- a/drivers/i2c/busses/i2c-pxa.c
> +++ b/drivers/i2c/busses/i2c-pxa.c
> @@ -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.

-- 
Robert

[PATCH 06/12] i2c: pxa: Return I2C_RETRY when timeout in pio mode

[Vaibhav Hiremath]

On Saturday 30 May 2015 02:16 AM, Robert Jarzmik wrote:
> Vaibhav Hiremath <vaibhav.hiremath@linaro.org> writes:
>
>> From: Shouming Wang <wangshm@marvell.com>
>>
>> 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@marvell.com>
>> [vaibhav.hiremath at linaro.org: Updated changelog]
>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>>
>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.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 eb09071..2777d5c 100644
>> --- a/drivers/i2c/busses/i2c-pxa.c
>> +++ b/drivers/i2c/busses/i2c-pxa.c
>> @@ -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).
>

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 07/12] i2c: pxa: Reset i2c controller on timeout in interrupt and pio mode

[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 <jtzhou@marvell.com>
[vaibhav.hiremath at linaro.org: Split & merge patches into logical changes
and update the Changelog]
Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>

Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
---
 drivers/i2c/busses/i2c-pxa.c | 8 ++++++++
 1 file changed, 8 insertions(+)

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

[PATCH 07/12] i2c: pxa: Reset i2c controller on timeout in interrupt and pio mode

[Robert Jarzmik]

Vaibhav Hiremath <vaibhav.hiremath@linaro.org> writes:

> 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 <jtzhou@marvell.com>
> [vaibhav.hiremath at linaro.org: Split & merge patches into logical changes
> and update the Changelog]
> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>
> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
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 07/12] i2c: pxa: Reset i2c controller on timeout in interrupt and pio mode

[Vaibhav Hiremath]

On Saturday 30 May 2015 02:43 AM, Robert Jarzmik wrote:
> Vaibhav Hiremath <vaibhav.hiremath@linaro.org> writes:
>
>> 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 <jtzhou@marvell.com>
>> [vaibhav.hiremath at linaro.org: Split & merge patches into logical changes
>> and update the Changelog]
>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>>
>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
> 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 08/12] i2c: pxa: enable/disable irq across message xfer

[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.
Also disable it before exiting probe function.

Signed-off-by: Jett.Zhou <jtzhou@marvell.com>
[vaibhav.hiremath at linaro.org: Split & merge patches into logical changes
and update the Changelog]
Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>

Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
---
 drivers/i2c/busses/i2c-pxa.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
index 3c6ebb5..a3ac97c 100644
--- a/drivers/i2c/busses/i2c-pxa.c
+++ b/drivers/i2c/busses/i2c-pxa.c
@@ -1163,6 +1163,7 @@ 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_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
-- 
1.9.1

[PATCH 08/12] i2c: pxa: enable/disable irq across message xfer

[Russell King - ARM Linux]

On Thu, May 28, 2015 at 06:33:40PM +0530, Vaibhav Hiremath wrote:
> 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 <bigeasy@linutronix.de>
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.

    Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
    Signed-off-by: Dirk Brandewie <dirk.brandewie@gmail.com>
    Signed-off-by: Ben Dooks <ben-linux@fluff.org>

So you're breaking Sodaville.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH 08/12] i2c: pxa: enable/disable irq across message xfer

[Vaibhav Hiremath]

On Thursday 28 May 2015 06:47 PM, Russell King - ARM Linux wrote:
> On Thu, May 28, 2015 at 06:33:40PM +0530, Vaibhav Hiremath wrote:
>> 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 <bigeasy@linutronix.de>
> 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.
>
>      Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>      Signed-off-by: Dirk Brandewie <dirk.brandewie@gmail.com>
>      Signed-off-by: Ben Dooks <ben-linux@fluff.org>
>
> 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 09/12] 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 at linaro.org: Updated Changelog]
Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
Cc: Wolfram Sang <wsa@the-dreams.de>

Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.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 a3ac97c..cf6c383 100644
--- a/drivers/i2c/busses/i2c-pxa.c
+++ b/drivers/i2c/busses/i2c-pxa.c
@@ -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);
-- 
1.9.1

[PATCH 09/12] i2c: pxa: Remove compile warnning in 64bit mode

[Robert Jarzmik]

Vaibhav Hiremath <vaibhav.hiremath@linaro.org> writes:

> 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 at 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>

Cheers.

--
Robert

[PATCH 10/12] 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.

Signed-off-by: Jett.Zhou <jtzhou@marvell.com>
[vaibhav.hiremath at linaro.org: Split patches into logical changes
and update the Changelog]
Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
Cc: Wolfram Sang <wsa@the-dreams.de>

Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
---
 drivers/i2c/busses/i2c-pxa.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
index cf6c383..065e647 100644
--- a/drivers/i2c/busses/i2c-pxa.c
+++ b/drivers/i2c/busses/i2c-pxa.c
@@ -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.
-- 
1.9.1

[PATCH 10/12] i2c: pxa: Update debug function to dump more info on error

[Robert Jarzmik]

Vaibhav Hiremath <vaibhav.hiremath@linaro.org> writes:

> Update i2c_pxa_scream_blue_murder() fn to print more information
> in case of error.
>
> Signed-off-by: Jett.Zhou <jtzhou@marvell.com>
> [vaibhav.hiremath at linaro.org: Split patches into logical changes
> and update the Changelog]
> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
> Cc: Wolfram Sang <wsa@the-dreams.de>
>
> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
> ---
>  drivers/i2c/busses/i2c-pxa.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
> index cf6c383..065e647 100644
> --- a/drivers/i2c/busses/i2c-pxa.c
> +++ b/drivers/i2c/busses/i2c-pxa.c
> @@ -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);
Why not simply use dev_err(....) instead of adding manually i2c->adap.name ?

Cheers.

-- 
Robert

[PATCH 10/12] i2c: pxa: Update debug function to dump more info on error

[Vaibhav Hiremath]

On Saturday 30 May 2015 03:12 AM, Robert Jarzmik wrote:
> Vaibhav Hiremath <vaibhav.hiremath@linaro.org> writes:
>
>> Update i2c_pxa_scream_blue_murder() fn to print more information
>> in case of error.
>>
>> Signed-off-by: Jett.Zhou <jtzhou@marvell.com>
>> [vaibhav.hiremath at linaro.org: Split patches into logical changes
>> and update the Changelog]
>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>> Cc: Wolfram Sang <wsa@the-dreams.de>
>>
>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>> ---
>>   drivers/i2c/busses/i2c-pxa.c | 11 ++++++++---
>>   1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
>> index cf6c383..065e647 100644
>> --- a/drivers/i2c/busses/i2c-pxa.c
>> +++ b/drivers/i2c/busses/i2c-pxa.c
>> @@ -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);
> Why not simply use dev_err(....) instead of adding manually i2c->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

[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>
---
 drivers/i2c/busses/i2c-pxa.c | 71 ++++++++++++++++----------------------------
 1 file changed, 25 insertions(+), 46 deletions(-)

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

[PATCH 11/12] i2c:pxa: Use devm_ variants in probe function

[Robert Jarzmik]

Vaibhav Hiremath <vaibhav.hiremath@linaro.org> writes:

> @@ -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;
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).

> +	i2c->reg_base = devm_ioremap_resource(&dev->dev, res);
>  	if (!i2c->reg_base) {
if (IS_ERR(i2c->reg_base)) instead.

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

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

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

> @@ -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 ?

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

-- 
Robert

[PATCH 11/12] i2c:pxa: Use devm_ variants in probe function

[Vaibhav Hiremath]

On Saturday 30 May 2015 09:23 PM, Robert Jarzmik wrote:
> Vaibhav Hiremath <vaibhav.hiremath@linaro.org> writes:
>
>> @@ -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;
> 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).
>

Good point.

I will change it in next version.


>> +	i2c->reg_base = devm_ioremap_resource(&dev->dev, res);
>>   	if (!i2c->reg_base) {
> if (IS_ERR(i2c->reg_base)) instead.
>
>> -		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);
>

Ok.

>> @@ -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.
>
>> @@ -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.
>
>> @@ -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 ?
>
>>   #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

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

Signed-off-by: Yi Zhang <yizhang@marvell.com>
[vaibhav.hiremath at linaro.org: ported to latest kernel & also improved changelog]
Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>

Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
---
 drivers/i2c/busses/i2c-pxa.c | 29 +++++++++++++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
index 844e1fc..f51d512 100644
--- a/drivers/i2c/busses/i2c-pxa.c
+++ b/drivers/i2c/busses/i2c-pxa.c
@@ -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 num
 	struct 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);
-- 
1.9.1

[PATCH 12/12] i2c: pxa: enable/disable i2c module across msg xfer

[Russell King - ARM Linux]

On Thu, May 28, 2015 at 06:33:44PM +0530, Vaibhav Hiremath wrote:
> 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.

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

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

[Vaibhav Hiremath]

On Thursday 28 May 2015 06:53 PM, Russell King - ARM Linux wrote:
> On Thu, May 28, 2015 at 06:33:44PM +0530, Vaibhav Hiremath wrote:
>> 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.
>
> "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

[Vaibhav Hiremath]

On Tuesday 02 June 2015 10:22 PM, Vaibhav Hiremath wrote:
>
>
> On Thursday 28 May 2015 06:53 PM, Russell King - ARM Linux wrote:
>> On Thu, May 28, 2015 at 06:33:44PM +0530, Vaibhav Hiremath wrote:
>>> 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.
>>
>> "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()
>
>

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

[Yi Zhang]

On Tue, Jun 02, 2015 at 10:29:23PM +0530, Vaibhav Hiremath wrote:
> 
> 
> On Tuesday 02 June 2015 10:22 PM, Vaibhav Hiremath wrote:
> >
> >
> >On Thursday 28 May 2015 06:53 PM, Russell King - ARM Linux wrote:
> >>On Thu, May 28, 2015 at 06:33:44PM +0530, Vaibhav Hiremath wrote:
> >>>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.
> >>
> >>"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()
> >
> >
> 
> 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
> 
> 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

[Vaibhav Hiremath]

On Wednesday 03 June 2015 04:26 PM, Yi Zhang wrote:
> On Tue, Jun 02, 2015 at 10:29:23PM +0530, Vaibhav Hiremath wrote:
>>
>>
>> On Tuesday 02 June 2015 10:22 PM, Vaibhav Hiremath wrote:
>>>
>>>
>>> On Thursday 28 May 2015 06:53 PM, Russell King - ARM Linux wrote:
>>>> On Thu, May 28, 2015 at 06:33:44PM +0530, Vaibhav Hiremath wrote:
>>>>> 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.
>>>>
>>>> "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()
>>>
>>>
>>
>> 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

[Yi Zhang]

On Wed, Jun 03, 2015 at 12:59:23AM +0800, Vaibhav Hiremath wrote:
> 
> 
> On Tuesday 02 June 2015 10:22 PM, Vaibhav Hiremath wrote:
> >
> >
> > On Thursday 28 May 2015 06:53 PM, Russell King - ARM Linux wrote:
> >> On Thu, May 28, 2015 at 06:33:44PM +0530, Vaibhav Hiremath wrote:
> >>> 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.
> >>
> >> "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()
> >
> >
> 
> 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 <yizhang@marvell.com>

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

[Vaibhav Hiremath]

On Thursday 04 June 2015 11:59 AM, Yi Zhang wrote:
> On Wed, Jun 03, 2015 at 12:59:23AM +0800, Vaibhav Hiremath wrote:
>>
>>
>> On Tuesday 02 June 2015 10:22 PM, Vaibhav Hiremath wrote:
>>>
>>>
>>> On Thursday 28 May 2015 06:53 PM, Russell King - ARM Linux wrote:
>>>> On Thu, May 28, 2015 at 06:33:44PM +0530, Vaibhav Hiremath wrote:
>>>>> 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.
>>>>
>>>> "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()
>>>
>>>
>>
>> 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.


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

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


>    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?

>    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
>
>    ---
>    Yi Zhang <yizhang@marvell.com>
>
>>
>> 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

[Yi Zhang]

On Thu, Jun 04, 2015 at 12:49:48PM +0530, Vaibhav Hiremath wrote:
> 
> 
> On Thursday 04 June 2015 11:59 AM, Yi Zhang wrote:
> >On Wed, Jun 03, 2015 at 12:59:23AM +0800, Vaibhav Hiremath wrote:
> >>
> >>
> >>On Tuesday 02 June 2015 10:22 PM, Vaibhav Hiremath wrote:
> >>>
> >>>
> >>>On Thursday 28 May 2015 06:53 PM, Russell King - ARM Linux wrote:
> >>>>On Thu, May 28, 2015 at 06:33:44PM +0530, Vaibhav Hiremath wrote:
> >>>>>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.
> >>>>
> >>>>"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()
> >>>
> >>>
> >>
> >>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.
> 
> 
> >   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
> >
> 
> Whether 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.

  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;
> 
> 
> >   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;
> 
> >   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
> >
> >   ---
> >   Yi Zhang <yizhang@marvell.com>
> >
> >>
> >>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 <yizhang@marvell.com>

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

[Wolfram Sang]

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

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

[Vaibhav Hiremath]

On Monday 01 June 2015 05:37 AM, Wolfram Sang wrote:
>> 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