* [PATCH 2/3] i2c: omap: Remove the redundant fifo clear
2012-10-08 9:05 [PATCH 1/3] i2c: omap: Do not enable the irq always Shubhrajyoti D
@ 2012-10-08 9:05 ` Shubhrajyoti D
2012-10-08 9:05 ` [PATCH 3/3] i2c: omap: Remove Address as slave wakeup Shubhrajyoti D
` (3 subsequent siblings)
4 siblings, 0 replies; 8+ messages in thread
From: Shubhrajyoti D @ 2012-10-08 9:05 UTC (permalink / raw)
To: linux-arm-kernel
The omap_i2c_resize_fifo already takes care of the fifo clearing so,
remove the (re)clearing of the fifo.
Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
---
drivers/i2c/busses/i2c-omap.c | 5 -----
1 files changed, 0 insertions(+), 5 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index ce41596..db0b272 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -524,11 +524,6 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
omap_i2c_write_reg(dev, OMAP_I2C_CNT_REG, dev->buf_len);
- /* Clear the FIFO Buffers */
- w = omap_i2c_read_reg(dev, OMAP_I2C_BUF_REG);
- w |= OMAP_I2C_BUF_RXFIF_CLR | OMAP_I2C_BUF_TXFIF_CLR;
- omap_i2c_write_reg(dev, OMAP_I2C_BUF_REG, w);
-
INIT_COMPLETION(dev->cmd_complete);
dev->cmd_err = 0;
--
1.7.5.4
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH 3/3] i2c: omap: Remove Address as slave wakeup
2012-10-08 9:05 [PATCH 1/3] i2c: omap: Do not enable the irq always Shubhrajyoti D
2012-10-08 9:05 ` [PATCH 2/3] i2c: omap: Remove the redundant fifo clear Shubhrajyoti D
@ 2012-10-08 9:05 ` Shubhrajyoti D
2012-10-08 9:46 ` [PATCH 1/3] i2c: omap: Do not enable the irq always Russell King - ARM Linux
` (2 subsequent siblings)
4 siblings, 0 replies; 8+ messages in thread
From: Shubhrajyoti D @ 2012-10-08 9:05 UTC (permalink / raw)
To: linux-arm-kernel
Currently the slave support is not there in the current i2c.
Lets remove the Address as slave wakeup from the Wakeup enable
register.
Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
---
drivers/i2c/busses/i2c-omap.c | 7 ++++++-
1 files changed, 6 insertions(+), 1 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index db0b272..c7b0a2c 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -128,6 +128,11 @@ enum {
OMAP_I2C_WE_DRDY_WE | OMAP_I2C_WE_ARDY_WE | \
OMAP_I2C_WE_NACK_WE | OMAP_I2C_WE_AL_WE)
+#define OMAP_I2C_WE_ENABLED (OMAP_I2C_WE_XDR_WE | OMAP_I2C_WE_RDR_WE | \
+ OMAP_I2C_WE_BF_WE | \
+ OMAP_I2C_WE_STC_WE | OMAP_I2C_WE_GC_WE | \
+ OMAP_I2C_WE_DRDY_WE | OMAP_I2C_WE_ARDY_WE | \
+ OMAP_I2C_WE_NACK_WE | OMAP_I2C_WE_AL_WE)
/* I2C Buffer Configuration Register (OMAP_I2C_BUF): */
#define OMAP_I2C_BUF_RDMA_EN (1 << 15) /* RX DMA channel enable */
#define OMAP_I2C_BUF_RXFIF_CLR (1 << 14) /* RX FIFO Clear */
@@ -326,7 +331,7 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
* WFI instruction.
* REVISIT: Some wkup sources might not be needed.
*/
- dev->westate = OMAP_I2C_WE_ALL;
+ dev->westate = OMAP_I2C_WE_ENABLED;
omap_i2c_write_reg(dev, OMAP_I2C_WE_REG,
dev->westate);
}
--
1.7.5.4
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH 1/3] i2c: omap: Do not enable the irq always
2012-10-08 9:05 [PATCH 1/3] i2c: omap: Do not enable the irq always Shubhrajyoti D
2012-10-08 9:05 ` [PATCH 2/3] i2c: omap: Remove the redundant fifo clear Shubhrajyoti D
2012-10-08 9:05 ` [PATCH 3/3] i2c: omap: Remove Address as slave wakeup Shubhrajyoti D
@ 2012-10-08 9:46 ` Russell King - ARM Linux
2012-10-12 14:26 ` Kevin Hilman
2012-10-12 14:31 ` Kevin Hilman
4 siblings, 0 replies; 8+ messages in thread
From: Russell King - ARM Linux @ 2012-10-08 9:46 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Oct 08, 2012 at 02:35:14PM +0530, Shubhrajyoti D wrote:
> Enable the irq in the transfer and disable it after the
> transfer is done.
This description is missing the most vital bits of information. In years
to come, someone will wonder "why has this changed" and there's no reason
given in the commit log.
Commit logs which are just mere translations from patch to English are
evil. Instead, good commit logs briefly state what is being done and
then go on to describe _why_ the change is necessary.
> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
> ---
> drivers/i2c/busses/i2c-omap.c | 3 +++
> 1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index b6c6b95..ce41596 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -625,6 +625,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> if (IS_ERR_VALUE(r))
> goto out;
>
> + enable_irq(dev->irq);
> r = omap_i2c_wait_for_bb(dev);
> if (r < 0)
> goto out;
> @@ -654,6 +655,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
>
> omap_i2c_wait_for_bb(dev);
> out:
> + disable_irq(dev->irq);
> pm_runtime_mark_last_busy(dev->dev);
> pm_runtime_put_autosuspend(dev->dev);
> return r;
> @@ -1179,6 +1181,7 @@ omap_i2c_probe(struct platform_device *pdev)
> goto err_unuse_clocks;
> }
>
> + disable_irq(dev->irq);
> adap = &dev->adapter;
> i2c_set_adapdata(adap, dev);
> adap->owner = THIS_MODULE;
> --
> 1.7.5.4
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/3] i2c: omap: Do not enable the irq always
2012-10-08 9:05 [PATCH 1/3] i2c: omap: Do not enable the irq always Shubhrajyoti D
` (2 preceding siblings ...)
2012-10-08 9:46 ` [PATCH 1/3] i2c: omap: Do not enable the irq always Russell King - ARM Linux
@ 2012-10-12 14:26 ` Kevin Hilman
2012-10-12 18:00 ` Shubhrajyoti Datta
2012-10-12 14:31 ` Kevin Hilman
4 siblings, 1 reply; 8+ messages in thread
From: Kevin Hilman @ 2012-10-12 14:26 UTC (permalink / raw)
To: linux-arm-kernel
+Kalle, Grygorii, Huzefa
Shubhrajyoti D <shubhrajyoti@ti.com> writes:
> Enable the irq in the transfer and disable it after the
> transfer is done.
>
> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
> ---
> drivers/i2c/busses/i2c-omap.c | 3 +++
> 1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index b6c6b95..ce41596 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -625,6 +625,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> if (IS_ERR_VALUE(r))
> goto out;
>
> + enable_irq(dev->irq);
> r = omap_i2c_wait_for_bb(dev);
> if (r < 0)
> goto out;
> @@ -654,6 +655,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
>
> omap_i2c_wait_for_bb(dev);
> out:
> + disable_irq(dev->irq);
When using runtime PM with auto-suspend timeouts, why would you disable
the IRQ before the runtime suspend handlers have run?
If you really want to do this, you probably should have these in the
runtime PM callbacks. But I'll wait until you add a more descriptive
changelog before I can really tell why this is being done. Based on the
discussion in the patch from Kalle, I'm assuming this is to prevent
interrups when I2C is being used by co-processors. If so, plese
describe in the changelog.
That being said, doesn't the runtime suspend callback already disable
IRQs at the device level instead of the INTC level?
Kevin
> pm_runtime_mark_last_busy(dev->dev);
> pm_runtime_put_autosuspend(dev->dev);
> return r;
> @@ -1179,6 +1181,7 @@ omap_i2c_probe(struct platform_device *pdev)
> goto err_unuse_clocks;
> }
>
> + disable_irq(dev->irq);
> adap = &dev->adapter;
> i2c_set_adapdata(adap, dev);
> adap->owner = THIS_MODULE;
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH 1/3] i2c: omap: Do not enable the irq always
2012-10-12 14:26 ` Kevin Hilman
@ 2012-10-12 18:00 ` Shubhrajyoti Datta
0 siblings, 0 replies; 8+ messages in thread
From: Shubhrajyoti Datta @ 2012-10-12 18:00 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Oct 12, 2012 at 7:56 PM, Kevin Hilman
<khilman@deeprootsystems.com> wrote:
> +Kalle, Grygorii, Huzefa
>
> Shubhrajyoti D <shubhrajyoti@ti.com> writes:
>
>> Enable the irq in the transfer and disable it after the
>> transfer is done.
>>
>> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
>> ---
>> drivers/i2c/busses/i2c-omap.c | 3 +++
>> 1 files changed, 3 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
>> index b6c6b95..ce41596 100644
>> --- a/drivers/i2c/busses/i2c-omap.c
>> +++ b/drivers/i2c/busses/i2c-omap.c
>> @@ -625,6 +625,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
>> if (IS_ERR_VALUE(r))
>> goto out;
>>
>> + enable_irq(dev->irq);
>> r = omap_i2c_wait_for_bb(dev);
>> if (r < 0)
>> goto out;
>> @@ -654,6 +655,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
>>
>> omap_i2c_wait_for_bb(dev);
>> out:
>> + disable_irq(dev->irq);
>
> When using runtime PM with auto-suspend timeouts, why would you disable
> the IRQ before the runtime suspend handlers have run?
>
> If you really want to do this, you probably should have these in the
> runtime PM callbacks.
OK.
> But I'll wait until you add a more descriptive
> changelog before I can really tell why this is being done. Based on the
> discussion in the patch from Kalle, I'm assuming this is to prevent
> interrups when I2C is being used by co-processors. If so, plese
> describe in the changelog.
OK.
>
> That being said, doesn't the runtime suspend callback already disable
> IRQs at the device level instead of the INTC level?
My idea is that the device may get reconfigured by the other processor.
I felt intc is the only way. do you agree?
>
> Kevin
>
>> pm_runtime_mark_last_busy(dev->dev);
>> pm_runtime_put_autosuspend(dev->dev);
>> return r;
>> @@ -1179,6 +1181,7 @@ omap_i2c_probe(struct platform_device *pdev)
>> goto err_unuse_clocks;
>> }
>>
>> + disable_irq(dev->irq);
>> adap = &dev->adapter;
>> i2c_set_adapdata(adap, dev);
>> adap->owner = THIS_MODULE;
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/3] i2c: omap: Do not enable the irq always
2012-10-08 9:05 [PATCH 1/3] i2c: omap: Do not enable the irq always Shubhrajyoti D
` (3 preceding siblings ...)
2012-10-12 14:26 ` Kevin Hilman
@ 2012-10-12 14:31 ` Kevin Hilman
2012-10-12 14:59 ` Shubhrajyoti
4 siblings, 1 reply; 8+ messages in thread
From: Kevin Hilman @ 2012-10-12 14:31 UTC (permalink / raw)
To: linux-arm-kernel
+Kalle, Grygorii, Huzefa
Shubhrajyoti D <shubhrajyoti@ti.com> writes:
> Enable the irq in the transfer and disable it after the
> transfer is done.
>
> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
> ---
> drivers/i2c/busses/i2c-omap.c | 3 +++
> 1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index b6c6b95..ce41596 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -625,6 +625,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> if (IS_ERR_VALUE(r))
> goto out;
>
> + enable_irq(dev->irq);
> r = omap_i2c_wait_for_bb(dev);
> if (r < 0)
> goto out;
> @@ -654,6 +655,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
>
> omap_i2c_wait_for_bb(dev);
> out:
> + disable_irq(dev->irq);
When using runtime PM with auto-suspend timeouts, why would you disable
the IRQ before the runtime suspend handlers have run?
If you really want to do this, you probably should have these in the
runtime PM callbacks. But I'll wait until you add a more descriptive
changelog before I can really tell why this is being done. Based on the
discussion in the patch from Kalle, I'm assuming this is to prevent
interrups when I2C is being used by co-processors. If so, plese
describe in the changelog.
That being said, doesn't the runtime suspend callback already disable
IRQs at the device level instead of the INTC level?
Kevin
> pm_runtime_mark_last_busy(dev->dev);
> pm_runtime_put_autosuspend(dev->dev);
> return r;
> @@ -1179,6 +1181,7 @@ omap_i2c_probe(struct platform_device *pdev)
> goto err_unuse_clocks;
> }
>
> + disable_irq(dev->irq);
> adap = &dev->adapter;
> i2c_set_adapdata(adap, dev);
> adap->owner = THIS_MODULE;
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH 1/3] i2c: omap: Do not enable the irq always
2012-10-12 14:31 ` Kevin Hilman
@ 2012-10-12 14:59 ` Shubhrajyoti
0 siblings, 0 replies; 8+ messages in thread
From: Shubhrajyoti @ 2012-10-12 14:59 UTC (permalink / raw)
To: linux-arm-kernel
On Friday 12 October 2012 08:01 PM, Kevin Hilman wrote:
> When using runtime PM with auto-suspend timeouts, why would you disable
> the IRQ before the runtime suspend handlers have run?
>
> If you really want to do this, you probably should have these in the
> runtime PM callbacks. But I'll wait until you add a more descriptive
> changelog before I can really tell why this is being done. Based on the
> discussion in the patch from Kalle, I'm assuming this is to prevent
> interrups when I2C is being used by co-processors. If so, plese
> describe in the changelog.
>
> That being said, doesn't the runtime suspend callback already disable
> IRQs at the device level instead of the INTC level?
I thought of not relying on the intc as the registers may be reconfigured.
> Kevin
^ permalink raw reply [flat|nested] 8+ messages in thread