[PATCH v3 0/4] i2c: tegra: Bug fixes, cleanups and M_NOSTART support

[PATCH v3 0/4] i2c: tegra: Bug fixes, cleanups and M_NOSTART support

[Laxman Dewangan]

This patch series having the:
- Handling of late register write due to Tegra PPSB design.
- support for I2C_M_NOSTART
- Use devm_* for all allocation.

Changes from V1:
- Using the readback for mask/unmask of irq also.

Changes from V2:
- Read back is done whenever write happen in i2c register other than TX FIFO.


Laxman Dewangan (4):
  i2c: tegra: make sure register writes completes
  i2c: tegra: add PROTOCOL_MANGLING as supported functionality.
  i2c: tegra: support for I2C_M_NOSTART functionality
  i2c: tegra: make all resource allocation through devm_*

 drivers/i2c/busses/i2c-tegra.c |  110 ++++++++++++++++++++++------------------
 1 files changed, 60 insertions(+), 50 deletions(-)

[PATCH V3 1/4] i2c: tegra: make sure register writes completes

[Laxman Dewangan]

The Tegra PPSB (an peripheral bus) queues writes transactions.
In order to guarantee that writes have completed before a
certain time, a read transaction to a register on the same
bus must be executed.
This is necessary in situations such as when clearing an
interrupt status or enable, so that when returning from an
interrupt handler, the HW has already de-asserted its
interrupt status output, which will avoid spurious interrupts.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
Changes from V1:
- Using the readback for mask/unmask of irq also.

Changes from V2:
- Read back is done whenever write happen in i2c register other than TX FIFO.

 drivers/i2c/busses/i2c-tegra.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 8b2e555..785f7f7 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -165,6 +165,10 @@ static void i2c_writel(struct tegra_i2c_dev *i2c_dev, u32 val,
 	unsigned long reg)
 {
 	writel(val, i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg));
+
+	/* Read back register to make sure that register writes completed */
+	if (reg != I2C_TX_FIFO)
+		readl(i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg));
 }
 
 static u32 i2c_readl(struct tegra_i2c_dev *i2c_dev, unsigned long reg)
-- 
1.7.1.1

Re: [PATCH V3 1/4] i2c: tegra: make sure register writes completes

[Stephen Warren]

On 06/13/2012 04:12 AM, Laxman Dewangan wrote:
> The Tegra PPSB (an peripheral bus) queues writes transactions.
> In order to guarantee that writes have completed before a
> certain time, a read transaction to a register on the same
> bus must be executed.
> This is necessary in situations such as when clearing an
> interrupt status or enable, so that when returning from an
> interrupt handler, the HW has already de-asserted its
> interrupt status output, which will avoid spurious interrupts.

> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c

> @@ -165,6 +165,10 @@ static void i2c_writel(struct tegra_i2c_dev *i2c_dev, u32 val,
>  	unsigned long reg)
>  {
>  	writel(val, i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg));
> +
> +	/* Read back register to make sure that register writes completed */
> +	if (reg != I2C_TX_FIFO)
> +		readl(i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg));

I guess that's fine, but it sure does seem rather heavy-weight. Don't
you only need to do the readback if you just wrote to the IRQ status or
mask registers, rather than if you wrote to /any/ register other than
the FIFO?

Re: [PATCH V3 1/4] i2c: tegra: make sure register writes completes

[Laxman Dewangan]

On Wednesday 13 June 2012 09:25 PM, Stephen Warren wrote:
> On 06/13/2012 04:12 AM, Laxman Dewangan wrote:
> @@ -165,6 +165,10 @@ static void i2c_writel(struct tegra_i2c_dev 
> *i2c_dev, u32 val,
>>   	unsigned long reg)
>>   {
>>   	writel(val, i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg));
>> +
>> +	/* Read back register to make sure that register writes completed */
>> +	if (reg != I2C_TX_FIFO)
>> +		readl(i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg));
> I guess that's fine, but it sure does seem rather heavy-weight. Don't
> you only need to do the readback if you just wrote to the IRQ status or
> mask registers, rather than if you wrote to /any/ register other than
> the FIFO?

That's what my second patch but based on your earlier review comment, I 
did for every register.

I think it will not matter much as we dont write all register with every 
transaction, only during initialization.
Then for each transfer we write manly on Tx fifo and interrupt 
mask/status register and hence not too much overweight.

Re: [PATCH V3 1/4] i2c: tegra: make sure register writes completes

[Stephen Warren]

On 06/14/2012 06:35 AM, Laxman Dewangan wrote:
> On Wednesday 13 June 2012 09:25 PM, Stephen Warren wrote:
>> On 06/13/2012 04:12 AM, Laxman Dewangan wrote:
>> @@ -165,6 +165,10 @@ static void i2c_writel(struct tegra_i2c_dev
>> *i2c_dev, u32 val,
>>>       unsigned long reg)
>>>   {
>>>       writel(val, i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg));
>>> +
>>> +    /* Read back register to make sure that register writes
>>> completed */
>>> +    if (reg != I2C_TX_FIFO)
>>> +        readl(i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg));
>> I guess that's fine, but it sure does seem rather heavy-weight. Don't
>> you only need to do the readback if you just wrote to the IRQ status or
>> mask registers, rather than if you wrote to /any/ register other than
>> the FIFO?
> 
> That's what my second patch but based on your earlier review comment, I
> did for every register.

Well, just for the record, in that comment you refer to, I was talking
about the location in the code where the readback should be implemented,
not the set of registers that the readback should happen for. But, it
probably doesn't impact performance. It'd be more self-documenting if
this readback was limited to the specific registers where it was needed
though.

Re: [PATCH V3 1/4] i2c: tegra: make sure register writes completes

[Wolfram Sang]

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


> probably doesn't impact performance. It'd be more self-documenting if
> this readback was limited to the specific registers where it was needed
> though.

I'd think this is easier to maintain in case code gets
rewritten/shuffled around.

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

[PATCH V3 2/4] i2c: tegra: add PROTOCOL_MANGLING as supported functionality.

[Laxman Dewangan]

The Tegra i2c driver supports the I2C_M_IGNORE_NAK and hence
returning I2C_FUNC_PROTOCOL_MANGLING as supported functionality.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
No Changes from V1 and V2.

 drivers/i2c/busses/i2c-tegra.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 785f7f7..68433ae 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -563,7 +563,8 @@ static int tegra_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
 
 static u32 tegra_i2c_func(struct i2c_adapter *adap)
 {
-	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | I2C_FUNC_10BIT_ADDR;
+	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | I2C_FUNC_10BIT_ADDR |
+		I2C_FUNC_PROTOCOL_MANGLING;
 }
 
 static const struct i2c_algorithm tegra_i2c_algo = {
-- 
1.7.1.1

[PATCH V3 3/4] i2c: tegra: support for I2C_M_NOSTART functionality

[Laxman Dewangan]

Adding support for functionality I2C_M_NOSTART.
When multiple message transfer request made through i2c
and if any message is flagged with I2C_M_NOSTART then
it will not send the start/repeat-start and address of
that message i.e. sends data directly.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
This is rebased of earlier patch on same support.
At this time using the new flag I2C_FUNC_NOSTART.

Changes form V1:
- Rebased on linux-next.

Changes from V2:
- Rebased based on Wolfram's tree.


 drivers/i2c/busses/i2c-tegra.c |   31 ++++++++++++++++++++++++++-----
 1 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 68433ae..c4593a2 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -97,8 +97,21 @@
 #define I2C_HEADER_10BIT_ADDR			(1<<18)
 #define I2C_HEADER_IE_ENABLE			(1<<17)
 #define I2C_HEADER_REPEAT_START			(1<<16)
+#define I2C_HEADER_CONTINUE_XFER		(1<<15)
 #define I2C_HEADER_MASTER_ADDR_SHIFT		12
 #define I2C_HEADER_SLAVE_ADDR_SHIFT		1
+/*
+ * msg_end_type: The bus control which need to be send at end of transfer.
+ * @MSG_END_STOP: Send stop pulse at end of transfer.
+ * @MSG_END_REPEAT_START: Send repeat start at end of transfer.
+ * @MSG_END_CONTINUE: The following on message is coming and so do not send
+ *		stop or repeat start.
+ */
+enum msg_end_type {
+	MSG_END_STOP,
+	MSG_END_REPEAT_START,
+	MSG_END_CONTINUE,
+};
 
 /**
  * struct tegra_i2c_dev	- per device i2c context
@@ -453,7 +466,7 @@ err:
 }
 
 static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
-	struct i2c_msg *msg, int stop)
+	struct i2c_msg *msg, enum msg_end_type end_state)
 {
 	u32 packet_header;
 	u32 int_mask;
@@ -480,7 +493,9 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 	i2c_writel(i2c_dev, packet_header, I2C_TX_FIFO);
 
 	packet_header = I2C_HEADER_IE_ENABLE;
-	if (!stop)
+	if (end_state == MSG_END_CONTINUE)
+		packet_header |= I2C_HEADER_CONTINUE_XFER;
+	else if (end_state == MSG_END_REPEAT_START)
 		packet_header |= I2C_HEADER_REPEAT_START;
 	if (msg->flags & I2C_M_TEN) {
 		packet_header |= msg->addr;
@@ -552,8 +567,14 @@ static int tegra_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
 
 	clk_enable(i2c_dev->clk);
 	for (i = 0; i < num; i++) {
-		int stop = (i == (num - 1)) ? 1  : 0;
-		ret = tegra_i2c_xfer_msg(i2c_dev, &msgs[i], stop);
+		enum msg_end_type end_type = MSG_END_STOP;
+		if (i < (num - 1)) {
+			if (msgs[i + 1].flags & I2C_M_NOSTART)
+				end_type = MSG_END_CONTINUE;
+			else
+				end_type = MSG_END_REPEAT_START;
+		}
+		ret = tegra_i2c_xfer_msg(i2c_dev, &msgs[i], end_type);
 		if (ret)
 			break;
 	}
@@ -564,7 +585,7 @@ static int tegra_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
 static u32 tegra_i2c_func(struct i2c_adapter *adap)
 {
 	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | I2C_FUNC_10BIT_ADDR |
-		I2C_FUNC_PROTOCOL_MANGLING;
+		I2C_FUNC_PROTOCOL_MANGLING | I2C_FUNC_NOSTART;
 }
 
 static const struct i2c_algorithm tegra_i2c_algo = {
-- 
1.7.1.1

[PATCH V3 4/4] i2c: tegra: make all resource allocation through devm_*

[Laxman Dewangan]

Use the devm_* for the memory region allocation, interrupt request,
clock handler request.
By doing this, it does not require to explicitly free it and hence
saving some code.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
This is rebased of earlier similar patch and sending as
part of this series.

Changes form V1:
- Rebased on linux-next.

Changes from V2:
- Rebased based on Wolfram's tree.

 drivers/i2c/busses/i2c-tegra.c |   62 +++++++++++-----------------------------
 1 files changed, 17 insertions(+), 45 deletions(-)

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index c4593a2..9f4e22c 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -598,7 +598,6 @@ static int __devinit tegra_i2c_probe(struct platform_device *pdev)
 	struct tegra_i2c_dev *i2c_dev;
 	struct tegra_i2c_platform_data *pdata = pdev->dev.platform_data;
 	struct resource *res;
-	struct resource *iomem;
 	struct clk *clk;
 	struct clk *i2c_clk;
 	const unsigned int *prop;
@@ -611,50 +610,41 @@ static int __devinit tegra_i2c_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev, "no mem resource\n");
 		return -EINVAL;
 	}
-	iomem = request_mem_region(res->start, resource_size(res), pdev->name);
-	if (!iomem) {
-		dev_err(&pdev->dev, "I2C region already claimed\n");
-		return -EBUSY;
-	}
 
-	base = ioremap(iomem->start, resource_size(iomem));
+	base = devm_request_and_ioremap(&pdev->dev, res);
 	if (!base) {
-		dev_err(&pdev->dev, "Cannot ioremap I2C region\n");
-		return -ENOMEM;
+		dev_err(&pdev->dev, "Cannot request/ioremap I2C registers\n");
+		return -EADDRNOTAVAIL;
 	}
 
 	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
 	if (!res) {
 		dev_err(&pdev->dev, "no irq resource\n");
-		ret = -EINVAL;
-		goto err_iounmap;
+		return -EINVAL;
 	}
 	irq = res->start;
 
-	clk = clk_get(&pdev->dev, NULL);
+	clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(clk)) {
 		dev_err(&pdev->dev, "missing controller clock");
-		ret = PTR_ERR(clk);
-		goto err_release_region;
+		return PTR_ERR(clk);
 	}
 
-	i2c_clk = clk_get(&pdev->dev, "i2c");
+	i2c_clk = devm_clk_get(&pdev->dev, "i2c");
 	if (IS_ERR(i2c_clk)) {
 		dev_err(&pdev->dev, "missing bus clock");
-		ret = PTR_ERR(i2c_clk);
-		goto err_clk_put;
+		return PTR_ERR(i2c_clk);
 	}
 
-	i2c_dev = kzalloc(sizeof(struct tegra_i2c_dev), GFP_KERNEL);
+	i2c_dev = devm_kzalloc(&pdev->dev, sizeof(*i2c_dev), GFP_KERNEL);
 	if (!i2c_dev) {
-		ret = -ENOMEM;
-		goto err_i2c_clk_put;
+		dev_err(&pdev->dev, "Could not allocate struct tegra_i2c_dev");
+		return -ENOMEM;
 	}
 
 	i2c_dev->base = base;
 	i2c_dev->clk = clk;
 	i2c_dev->i2c_clk = i2c_clk;
-	i2c_dev->iomem = iomem;
 	i2c_dev->adapter.algo = &tegra_i2c_algo;
 	i2c_dev->irq = irq;
 	i2c_dev->cont_id = pdev->id;
@@ -683,13 +673,14 @@ static int __devinit tegra_i2c_probe(struct platform_device *pdev)
 	ret = tegra_i2c_init(i2c_dev);
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to initialize i2c controller");
-		goto err_free;
+		return ret;
 	}
 
-	ret = request_irq(i2c_dev->irq, tegra_i2c_isr, 0, pdev->name, i2c_dev);
+	ret = devm_request_irq(&pdev->dev, i2c_dev->irq,
+			tegra_i2c_isr, 0, pdev->name, i2c_dev);
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to request irq %i\n", i2c_dev->irq);
-		goto err_free;
+		return ret;
 	}
 
 	clk_enable(i2c_dev->i2c_clk);
@@ -707,38 +698,19 @@ static int __devinit tegra_i2c_probe(struct platform_device *pdev)
 	ret = i2c_add_numbered_adapter(&i2c_dev->adapter);
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to add I2C adapter\n");
-		goto err_free_irq;
+		clk_disable(i2c_dev->i2c_clk);
+		return ret;
 	}
 
 	of_i2c_register_devices(&i2c_dev->adapter);
 
 	return 0;
-err_free_irq:
-	free_irq(i2c_dev->irq, i2c_dev);
-err_free:
-	kfree(i2c_dev);
-err_i2c_clk_put:
-	clk_put(i2c_clk);
-err_clk_put:
-	clk_put(clk);
-err_release_region:
-	release_mem_region(iomem->start, resource_size(iomem));
-err_iounmap:
-	iounmap(base);
-	return ret;
 }
 
 static int __devexit tegra_i2c_remove(struct platform_device *pdev)
 {
 	struct tegra_i2c_dev *i2c_dev = platform_get_drvdata(pdev);
 	i2c_del_adapter(&i2c_dev->adapter);
-	free_irq(i2c_dev->irq, i2c_dev);
-	clk_put(i2c_dev->i2c_clk);
-	clk_put(i2c_dev->clk);
-	release_mem_region(i2c_dev->iomem->start,
-		resource_size(i2c_dev->iomem));
-	iounmap(i2c_dev->base);
-	kfree(i2c_dev);
 	return 0;
 }
 
-- 
1.7.1.1

Re: [PATCH v3 0/4] i2c: tegra: Bug fixes, cleanups and M_NOSTART support

[Wolfram Sang]

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

On Wed, Jun 13, 2012 at 03:42:35PM +0530, Laxman Dewangan wrote:
> This patch series having the:
> - Handling of late register write due to Tegra PPSB design.
> - support for I2C_M_NOSTART
> - Use devm_* for all allocation.

Thanks. 1/4 applied to for-current, rest to for-next.

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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