[PATCH 1/2] i2c: xiic: Implement Power management

[Shubhrajyoti Datta]

Enable power management. This patch enables the clocks
before transfer and disables after the transfer.

Signed-off-by: Shubhrajyoti Datta <shubh...@xilinx.com>
---
 drivers/i2c/busses/i2c-xiic.c |   86 +---
 1 files changed, 79 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index 0b20449..b464a35 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -37,6 +37,8 @@
 #include 
 #include 
 #include 
+#include 
+#include 
 
 #define DRIVER_NAME "xiic-i2c"
 
@@ -66,6 +68,7 @@ enum xiic_endian {
  * @endianness: big/little-endian byte order
  */
 struct xiic_i2c {
+   struct device   *dev;
void __iomem*base;
wait_queue_head_t   wait;
struct i2c_adapter  adap;
@@ -77,6 +80,7 @@ struct xiic_i2c {
struct i2c_msg  *rx_msg;
int rx_pos;
enum xiic_endianendianness;
+   struct clk *clk;
 };
 
 
@@ -164,6 +168,7 @@ struct xiic_i2c {
 
 #define XIIC_RESET_MASK 0xAUL
 
+#define XIIC_PM_TIMEOUT1000/* ms */
 /*
  * The following constant is used for the device global interrupt enable
  * register, to enable all interrupts for the device, this is the only bit
@@ -676,9 +681,13 @@ static int xiic_xfer(struct i2c_adapter *adap, struct 
i2c_msg *msgs, int num)
dev_dbg(adap->dev.parent, "%s entry SR: 0x%x\n", __func__,
xiic_getreg8(i2c, XIIC_SR_REG_OFFSET));
 
+   err = pm_runtime_get_sync(i2c->dev);
+   if (err < 0)
+   return err;
+
err = xiic_busy(i2c);
if (err)
-   return err;
+   goto out;
 
i2c->tx_msg = msgs;
i2c->nmsgs = num;
@@ -686,14 +695,20 @@ static int xiic_xfer(struct i2c_adapter *adap, struct 
i2c_msg *msgs, int num)
xiic_start_xfer(i2c);
 
if (wait_event_timeout(i2c->wait, (i2c->state == STATE_ERROR) ||
-   (i2c->state == STATE_DONE), HZ))
-   return (i2c->state == STATE_DONE) ? num : -EIO;
-   else {
+   (i2c->state == STATE_DONE), HZ)) {
+   err = (i2c->state == STATE_DONE) ? num : -EIO;
+   goto out;
+   } else {
i2c->tx_msg = NULL;
i2c->rx_msg = NULL;
i2c->nmsgs = 0;
-   return -ETIMEDOUT;
+   err = -ETIMEDOUT;
+   goto out;
}
+out:
+   pm_runtime_mark_last_busy(i2c->dev);
+   pm_runtime_put_autosuspend(i2c->dev);
+   return err;
 }
 
 static u32 xiic_func(struct i2c_adapter *adap)
@@ -748,13 +763,26 @@ static int xiic_i2c_probe(struct platform_device *pdev)
spin_lock_init(>lock);
init_waitqueue_head(>wait);
 
+   i2c->clk = devm_clk_get(>dev, NULL);
+   if (IS_ERR(i2c->clk)) {
+   dev_err(>dev, "input clock not found.\n");
+   return PTR_ERR(i2c->clk);
+   }
+   ret = clk_prepare_enable(i2c->clk);
+   if (ret)
+   dev_err(>dev, "Unable to enable clock.\n");
+
+   pm_runtime_enable(i2c->dev);
+   pm_runtime_set_autosuspend_delay(i2c->dev, XIIC_PM_TIMEOUT);
+   pm_runtime_use_autosuspend(i2c->dev);
+   pm_runtime_set_active(i2c->dev);
ret = devm_request_threaded_irq(>dev, irq, xiic_isr,
xiic_process, IRQF_ONESHOT,
pdev->name, i2c);
 
if (ret < 0) {
dev_err(>dev, "Cannot claim IRQ\n");
-   return ret;
+   goto err_clk_dis;
}
 
/*
@@ -776,7 +804,7 @@ static int xiic_i2c_probe(struct platform_device *pdev)
if (ret) {
dev_err(>dev, "Failed to add adapter\n");
xiic_deinit(i2c);
-   return ret;
+   goto err_clk_dis;
}
 
if (pdata) {
@@ -786,16 +814,30 @@ static int xiic_i2c_probe(struct platform_device *pdev)
}
 
return 0;
+
+err_clk_dis:
+   clk_disable_unprepare(i2c->clk);
+   pm_runtime_set_suspended(>dev);
+   pm_runtime_disable(>dev);
+   return ret;
 }
 
 static int xiic_i2c_remove(struct platform_device *pdev)
 {
struct xiic_i2c *i2c = platform_get_drvdata(pdev);
+   int ret;
 
/* remove adapter & data */
i2c_del_adapter(>adap);
 
+   ret = clk_prepare_enable(i2c->clk);
+   if (ret) {
+   dev_err(>dev, "Unable to enable clock.\n");
+   return ret;
+   }
xiic_deinit(i2c);
+   clk_disable_unprepare(i2c->clk);
+   pm_runtime_disable(>dev);
 
return 0;
 }
@@ -808,12 +850,42 @@ static const struct of_device_id x

[PATCH 2/2] bindings: i2c: Add clock entries for i2c-xiic

[Shubhrajyoti Datta]

Add clock description for i2c-xiic

Signed-off-by: Shubhrajyoti Datta <shubh...@xilinx.com>
---
 Documentation/devicetree/bindings/i2c/i2c-xiic.txt |3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-xiic.txt 
b/Documentation/devicetree/bindings/i2c/i2c-xiic.txt
index ceabbe9..caf42e9 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-xiic.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-xiic.txt
@@ -6,14 +6,17 @@ Required properties:
 - interrupts : IIC controller unterrupt
 - #address-cells = <1>
 - #size-cells = <0>
+- clocks: Input clock specifier. Refer to common clock bindings.
 
 Optional properties:
 - Child nodes conforming to i2c bus binding
+- clock-names: Input clock name, should be 'pclk'.
 
 Example:
 
axi_iic_0: i2c@4080 {
compatible = "xlnx,xps-iic-2.00.a";
+   clocks = < 15>;
interrupts = < 1 2 >;
reg = < 0x4080 0x1 >;
 
-- 
1.7.1

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

Re: [PATCH 0/2] i2c: proper RuntimePM for the adapter device

[Shubhrajyoti Datta]

On Fri, Dec 25, 2015 at 3:55 AM, Wolfram Sang  wrote:
>> I think the below drivers
>>
>> drivers/i2c/busses/i2c-at91.c
>> drivers/i2c/busses/i2c-designware-core.c
>> drivers/i2c/busses/i2c-designware-platdrv.c
>> drivers/i2c/busses/i2c-rcar.c
>> drivers/i2c/busses/i2c-sh_mobile.c
>> drivers/i2c/busses/i2c-hix5hd2.c
>> drivers/i2c/busses/i2c-nomadik.c
>> drivers/i2c/busses/i2c-omap.c
>> drivers/i2c/busses/i2c-qup.c
>> drivers/i2c/busses/i2c-s3c2410.c
>
> They use RuntimePM on their platform device, not on the adapter device
> AFAICS.
>
Indeed my bad.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2] i2c: proper RuntimePM for the adapter device

[Shubhrajyoti Datta]

>
>> Also maybe we can add a flag that the driver enables.
>> that way for a short period till the driver removes the call
>> the double call is not there.
>
> The only driver I found was the s3c2410 and I fixed it. Is there another
> one? Then, I'd update my series because I want a consistent state in one
> go.
>
I think the below drivers

drivers/i2c/busses/i2c-at91.c
drivers/i2c/busses/i2c-designware-core.c
drivers/i2c/busses/i2c-designware-platdrv.c
drivers/i2c/busses/i2c-rcar.c
drivers/i2c/busses/i2c-sh_mobile.c
drivers/i2c/busses/i2c-hix5hd2.c
drivers/i2c/busses/i2c-nomadik.c
drivers/i2c/busses/i2c-omap.c
drivers/i2c/busses/i2c-qup.c
drivers/i2c/busses/i2c-s3c2410.c
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2] i2c: proper RuntimePM for the adapter device

[Shubhrajyoti Datta]

On Wed, Dec 23, 2015 at 10:49 PM, Wolfram Sang  wrote:
> RuntimePM for the logical adapter device should be handled in a central place
> by the core, and not by drivers. This series does exactly that.

This is good idea.

>
> This is the first step of harmonizing RuntimePM handling in I2C.
>

Also maybe we can add a flag that the driver enables.
that way for a short period till the driver removes the call
the double call is not there.

not an objection to the patch though
>
> Wolfram Sang (2):
>   i2c: always enable RuntimePM for the adapter device
>   i2c: s3c2410: remove superfluous runtime PM calls
>
>  drivers/i2c/busses/i2c-s3c2410.c | 6 --
>  drivers/i2c/i2c-core.c   | 3 +++
>  2 files changed, 3 insertions(+), 6 deletions(-)
>
> --
> 2.1.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] i2c: cadence: Move to sensible power management

[Shubhrajyoti Datta]

On Tue, Nov 24, 2015 at 12:17 AM, Sören Brinkmann
<soren.brinkm...@xilinx.com> wrote:
> On Sat, 2015-11-21 at 07:00PM +0530, Shubhrajyoti Datta wrote:
>> On Thu, Oct 29, 2015 at 8:27 PM, Shubhrajyoti Datta
>> <shubhrajyoti.da...@gmail.com> wrote:
>> > On Wed, Oct 28, 2015 at 9:48 PM, Sören Brinkmann
>> > <soren.brinkm...@xilinx.com> wrote:
>> >> Hi Shubhrajyoti,
>> >>
>> >>
>> >> On Wed, 2015-10-28 at 12:56PM +0530, Shubhrajyoti Datta wrote:
>> >>> Currently the clocks are enabled at probe and disabled at remove.
>> >>> Which keeps the clocks enabled even if no transaction is going on.
>> >>> This patch enables the clocks at the start of transfer and disables
>> >>> after it.
>> >>>
>> >>> Also adapts to runtime pm.
>> >>> Remove xi2c->suspended and use pm runtime status instead.
>> >>>
>> >>> converts dev pm to const to silence a checkpatch warning.
>> >>>
>> >>> Signed-off-by: Shubhrajyoti Datta <shubh...@xilinx.com>
>> >>
>> >> To me, this looks all good. Just one small concern below.
>> >
>> > Thanks for the review.
>> Soren ,
>> Do are you ok with the change or do you want me to resend without the
>> suspended flag change.
>
> I'm always for removing code that is not needed. If things are tested
> and well and work without throwing any warnings I'm OK with it.

It should be also having a suspended book-keeping in the driver is not
needed the pm does that for us.

I will spilt the patch and resend.

Thanks,

>
> Sören
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv3 2/2] i2c: cadence: Remove the suspended flag

[Shubhrajyoti Datta]

The suspended flag is a flag holding the device's PM status.
The runtime framework does that for us.
Use pm_runtime_suspended call instead.

Signed-off-by: Shubhrajyoti Datta <shubh...@xilinx.com>
---
v3: split the patches

 drivers/i2c/busses/i2c-cadence.c |7 +--
 1 files changed, 1 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c
index d54ad46..6b08d16 100644
--- a/drivers/i2c/busses/i2c-cadence.c
+++ b/drivers/i2c/busses/i2c-cadence.c
@@ -131,7 +131,6 @@
  * @xfer_done: Transfer complete status
  * @p_send_buf:Pointer to transmit buffer
  * @p_recv_buf:Pointer to receive buffer
- * @suspended: Flag holding the device's PM status
  * @send_count:Number of bytes still expected to send
  * @recv_count:Number of bytes still expected to receive
  * @curr_recv_count:   Number of bytes to be received in current transfer
@@ -152,7 +151,6 @@ struct cdns_i2c {
struct completion xfer_done;
unsigned char *p_send_buf;
unsigned char *p_recv_buf;
-   u8 suspended;
unsigned int send_count;
unsigned int recv_count;
unsigned int curr_recv_count;
@@ -776,7 +774,7 @@ static int cdns_i2c_clk_notifier_cb(struct notifier_block 
*nb, unsigned long
struct clk_notifier_data *ndata = data;
struct cdns_i2c *id = to_cdns_i2c(nb);
 
-   if (id->suspended)
+   if (pm_runtime_suspended(id->dev))
return NOTIFY_OK;
 
switch (event) {
@@ -830,7 +828,6 @@ static int __maybe_unused cdns_i2c_runtime_suspend(struct 
device *dev)
struct cdns_i2c *xi2c = platform_get_drvdata(pdev);
 
clk_disable(xi2c->clk);
-   xi2c->suspended = 1;
 
return 0;
 }
@@ -855,8 +852,6 @@ static int __maybe_unused cdns_i2c_runtime_resume(struct 
device *dev)
return ret;
}
 
-   xi2c->suspended = 0;
-
return 0;
 }
 
-- 
1.7.1

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

[PATCHv3 1/2] i2c: cadence: Move to sensible power management

[Shubhrajyoti Datta]

Currently the clocks are enabled at probe and disabled at remove.
Which keeps the clocks enabled even if no transaction is going on.
This patch enables the clocks at the start of transfer and disables
after it.

Also adapts to runtime pm.

converts dev pm to const to silence a checkpatch warning.

Signed-off-by: Shubhrajyoti Datta <shubh...@xilinx.com>
---
v2: update the cc list
v3: split the runtime and the suspended flag change

 drivers/i2c/busses/i2c-cadence.c |   66 ++
 1 files changed, 45 insertions(+), 21 deletions(-)

diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c
index 84deed6..d54ad46 100644
--- a/drivers/i2c/busses/i2c-cadence.c
+++ b/drivers/i2c/busses/i2c-cadence.c
@@ -18,6 +18,7 @@
 #include 
 #include 
 #include 
+#include 
 
 /* Register offsets for the I2C device. */
 #define CDNS_I2C_CR_OFFSET 0x00 /* Control Register, RW */
@@ -96,6 +97,8 @@
 CDNS_I2C_IXR_COMP)
 
 #define CDNS_I2C_TIMEOUT   msecs_to_jiffies(1000)
+/* timeout for pm runtime autosuspend */
+#define CNDS_I2C_PM_TIMEOUT1000/* ms */
 
 #define CDNS_I2C_FIFO_DEPTH16
 /* FIFO depth at which the DATA interrupt occurs */
@@ -141,6 +144,7 @@
  * @quirks:flag for broken hold bit usage in r1p10
  */
 struct cdns_i2c {
+   struct device   *dev;
void __iomem *membase;
struct i2c_adapter adap;
struct i2c_msg *p_msg;
@@ -569,9 +573,14 @@ static int cdns_i2c_master_xfer(struct i2c_adapter *adap, 
struct i2c_msg *msgs,
struct cdns_i2c *id = adap->algo_data;
bool hold_quirk;
 
+   ret = pm_runtime_get_sync(id->dev);
+   if (ret < 0)
+   return ret;
/* Check if the bus is free */
-   if (cdns_i2c_readreg(CDNS_I2C_SR_OFFSET) & CDNS_I2C_SR_BA)
-   return -EAGAIN;
+   if (cdns_i2c_readreg(CDNS_I2C_SR_OFFSET) & CDNS_I2C_SR_BA) {
+   ret = -EAGAIN;
+   goto out;
+   }
 
hold_quirk = !!(id->quirks & CDNS_I2C_BROKEN_HOLD_BIT);
/*
@@ -590,7 +599,8 @@ static int cdns_i2c_master_xfer(struct i2c_adapter *adap, 
struct i2c_msg *msgs,
if (msgs[count].flags & I2C_M_RD) {
dev_warn(adap->dev.parent,
 "Can't do repeated start after a 
receive message\n");
-   return -EOPNOTSUPP;
+   ret = -EOPNOTSUPP;
+   goto out;
}
}
id->bus_hold_flag = 1;
@@ -608,20 +618,26 @@ static int cdns_i2c_master_xfer(struct i2c_adapter *adap, 
struct i2c_msg *msgs,
 
ret = cdns_i2c_process_msg(id, msgs, adap);
if (ret)
-   return ret;
+   goto out;
 
/* Report the other error interrupts to application */
if (id->err_status) {
cdns_i2c_master_reset(adap);
 
-   if (id->err_status & CDNS_I2C_IXR_NACK)
-   return -ENXIO;
-
-   return -EIO;
+   if (id->err_status & CDNS_I2C_IXR_NACK) {
+   ret = -ENXIO;
+   goto out;
+   }
+   ret = -EIO;
+   goto out;
}
}
 
-   return num;
+   ret = num;
+out:
+   pm_runtime_mark_last_busy(id->dev);
+   pm_runtime_put_autosuspend(id->dev);
+   return ret;
 }
 
 /**
@@ -808,10 +824,9 @@ static int cdns_i2c_clk_notifier_cb(struct notifier_block 
*nb, unsigned long
  *
  * Return: 0 always
  */
-static int __maybe_unused cdns_i2c_suspend(struct device *_dev)
+static int __maybe_unused cdns_i2c_runtime_suspend(struct device *dev)
 {
-   struct platform_device *pdev = container_of(_dev,
-   struct platform_device, dev);
+   struct platform_device *pdev = to_platform_device(dev);
struct cdns_i2c *xi2c = platform_get_drvdata(pdev);
 
clk_disable(xi2c->clk);
@@ -828,16 +843,15 @@ static int __maybe_unused cdns_i2c_suspend(struct device 
*_dev)
  *
  * Return: 0 on success and error value on error
  */
-static int __maybe_unused cdns_i2c_resume(struct device *_dev)
+static int __maybe_unused cdns_i2c_runtime_resume(struct device *dev)
 {
-   struct platform_device *pdev = container_of(_dev,
-   struct platform_device, dev);
+   struct platform_device *pdev = to_platform_device(dev);
struct cdns_i2c *xi2c = platform_get_drvdata(pdev);
int ret;
 
ret = clk_enable(xi2c->clk);
if (ret) {
-   dev_err(_dev, "Cannot enable clock.\n");
+   

Re: [PATCH v2] i2c: cadence: Move to sensible power management

[Shubhrajyoti Datta]

On Thu, Oct 29, 2015 at 8:27 PM, Shubhrajyoti Datta
<shubhrajyoti.da...@gmail.com> wrote:
> On Wed, Oct 28, 2015 at 9:48 PM, Sören Brinkmann
> <soren.brinkm...@xilinx.com> wrote:
>> Hi Shubhrajyoti,
>>
>>
>> On Wed, 2015-10-28 at 12:56PM +0530, Shubhrajyoti Datta wrote:
>>> Currently the clocks are enabled at probe and disabled at remove.
>>> Which keeps the clocks enabled even if no transaction is going on.
>>> This patch enables the clocks at the start of transfer and disables
>>> after it.
>>>
>>> Also adapts to runtime pm.
>>> Remove xi2c->suspended and use pm runtime status instead.
>>>
>>> converts dev pm to const to silence a checkpatch warning.
>>>
>>> Signed-off-by: Shubhrajyoti Datta <shubh...@xilinx.com>
>>
>> To me, this looks all good. Just one small concern below.
>
> Thanks for the review.
Soren ,
Do are you ok with the change or do you want me to resend without the
suspended flag change.

<>
>>
>> There might have been a reason to store this flag here. Did you test
>> this with lockdep and CONFIG_DEBUG_ATOMIC_SLEEP? Just to make sure that
>> nothing that can sleep is called from atomic context.
> Done now.
>
>
> Essentially this is a flag is set in suspend routine. and checked in
> the isr I use
> pm_runtime_suspended(id->dev) instead.
>
>>
>> Sören
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majord...@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] i2c: Revert "i2c: xiic: Do not reset controller before every transfer"

[Shubhrajyoti Datta]

On Wed, Nov 18, 2015 at 3:31 PM, Lars-Peter Clausen <l...@metafoo.de> wrote:
> On 11/17/2015 03:28 PM, Shubhrajyoti Datta wrote:
>> On Tue, Nov 17, 2015 at 1:04 PM, Lars-Peter Clausen <l...@metafoo.de> wrote:
>>> On 11/17/2015 06:17 AM, Shubhrajyoti Datta wrote:
>>>> On Mon, Nov 16, 2015 at 7:12 PM, Lars-Peter Clausen <l...@metafoo.de> 
>>>> wrote:
>>>>> Commit d701667bb331 ("i2c: xiic: Do not reset controller before every
>>>>> transfer") removed the reinitialization of the controller before the start
>>>>> of each transfer. Apparently this change is not safe to make and the 
>>>>> commit
>>>>> results in random I2C bus failures.
>>>>
>>>> Which is the platform and the ip version that you  saw the issue.
>>>> Did you see the issue with read and write as  well?
>>>
>>> The IP version is the axi-iic v2.0 Revision 8. I've tested this on a few
>>> platforms, custom ones and standard ones and I could reproduce it on most.
>>> One of them was on the ZED board. The one where I couldn't reproduce it was
>>> the ZC706. But that doesn't necessarily mean it doesn't happen there, just
>>> that it is not triggered by the testcase.
>> All the boards having the same version of the ip is what I have understood.
>>
>> Thanks for the info I will try to  reproduce the issue.
>>
>>>
>>> The problem is that it is random corruption,
>> Of registers?
>
> To be honest I don't know if there is corruption during I2C write transfers,
> but there is definitely corruption during read transactions.
Ok

Actually  I was thinking it is only an issue with



/* dynamic mode seem to suffer from problems if we just flushes
* fifos and the next message is a TX with len 0 (only addr)
* reset the IP instead of just flush fifos
*/
xiic_reinit(i2c);
<\quote>

I think as of now since read is also impacted we can revert it.


> - Lars
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] i2c: Revert "i2c: xiic: Do not reset controller before every transfer"

[Shubhrajyoti Datta]

On Tue, Nov 17, 2015 at 1:04 PM, Lars-Peter Clausen <l...@metafoo.de> wrote:
> On 11/17/2015 06:17 AM, Shubhrajyoti Datta wrote:
>> On Mon, Nov 16, 2015 at 7:12 PM, Lars-Peter Clausen <l...@metafoo.de> wrote:
>>> Commit d701667bb331 ("i2c: xiic: Do not reset controller before every
>>> transfer") removed the reinitialization of the controller before the start
>>> of each transfer. Apparently this change is not safe to make and the commit
>>> results in random I2C bus failures.
>>
>> Which is the platform and the ip version that you  saw the issue.
>> Did you see the issue with read and write as  well?
>
> The IP version is the axi-iic v2.0 Revision 8. I've tested this on a few
> platforms, custom ones and standard ones and I could reproduce it on most.
> One of them was on the ZED board. The one where I couldn't reproduce it was
> the ZC706. But that doesn't necessarily mean it doesn't happen there, just
> that it is not triggered by the testcase.
All the boards having the same version of the ip is what I have understood.

Thanks for the info I will try to  reproduce the issue.

>
> The problem is that it is random corruption,
Of registers?

> so some I2C devices might start
> to behave strangely at some point. The only good more or less reliable way
> to reproduce it that I found was to run i2cdetect a couple of times and at
> least one of them will produce strange behavior.
>
I will try to reproduce the issue at my end thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] i2c: xiic: Replace spinlock with mutex

[Shubhrajyoti Datta]

On Mon, Nov 16, 2015 at 7:12 PM, Lars-Peter Clausen  wrote:
> All protected sections are only called from sleep-able context, so there is
> no need to use a spinlock.

Looks good to me. Feel free to add my reviewed by.
>
> Signed-off-by: Lars-Peter Clausen 
> ---
>  drivers/i2c/busses/i2c-xiic.c | 12 ++--
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
> index 0b20449..6efd200 100644
> --- a/drivers/i2c/busses/i2c-xiic.c
> +++ b/drivers/i2c/busses/i2c-xiic.c
> @@ -70,7 +70,7 @@ struct xiic_i2c {
> wait_queue_head_t   wait;
> struct i2c_adapter  adap;
> struct i2c_msg  *tx_msg;
> -   spinlock_t  lock;
> +   struct mutexlock;
> unsigned inttx_pos;
> unsigned intnmsgs;
> enum xilinx_i2c_state   state;
> @@ -369,7 +369,7 @@ static irqreturn_t xiic_process(int irq, void *dev_id)
>  * To find which interrupts are pending; AND interrupts pending with
>  * interrupts masked.
>  */
> -   spin_lock(>lock);
> +   mutex_lock(>lock);
> isr = xiic_getreg32(i2c, XIIC_IISR_OFFSET);
> ier = xiic_getreg32(i2c, XIIC_IIER_OFFSET);
> pend = isr & ier;
> @@ -497,7 +497,7 @@ out:
> dev_dbg(i2c->adap.dev.parent, "%s clr: 0x%x\n", __func__, clr);
>
> xiic_setreg32(i2c, XIIC_IISR_OFFSET, clr);
> -   spin_unlock(>lock);
> +   mutex_unlock(>lock);
> return IRQ_HANDLED;
>  }
>
> @@ -662,10 +662,10 @@ static void __xiic_start_xfer(struct xiic_i2c *i2c)
>
>  static void xiic_start_xfer(struct xiic_i2c *i2c)
>  {
> -   spin_lock(>lock);
> +   mutex_lock(>lock);
> xiic_reinit(i2c);
> __xiic_start_xfer(i2c);
> -   spin_unlock(>lock);
> +   mutex_unlock(>lock);
>  }
>
>  static int xiic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
> @@ -745,7 +745,7 @@ static int xiic_i2c_probe(struct platform_device *pdev)
> i2c->adap.dev.parent = >dev;
> i2c->adap.dev.of_node = pdev->dev.of_node;
>
> -   spin_lock_init(>lock);
> +   mutex_init(>lock);
> init_waitqueue_head(>wait);
>
> ret = devm_request_threaded_irq(>dev, irq, xiic_isr,
> --
> 2.1.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] i2c: Revert "i2c: xiic: Do not reset controller before every transfer"

[Shubhrajyoti Datta]

On Mon, Nov 16, 2015 at 7:12 PM, Lars-Peter Clausen  wrote:
> Commit d701667bb331 ("i2c: xiic: Do not reset controller before every
> transfer") removed the reinitialization of the controller before the start
> of each transfer. Apparently this change is not safe to make and the commit
> results in random I2C bus failures.

Which is the platform and the ip version that you  saw the issue.
Did you see the issue with read and write as  well?
.

>
> An easy way to trigger the issue is to run i2cdetect.
>
> Without the patch applied:
>  0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
>  00:  -- -- -- -- -- -- -- -- -- -- -- -- --
>  10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
>  20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
>  30: -- -- -- -- -- -- -- -- UU UU -- UU 3c -- -- UU
>  40: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
>  50: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
>  60: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
>  70: -- -- -- -- -- -- -- --
>
> With the patch applied every other or so invocation:
>  0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
>  00:  03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f
>  10: 10 11 12 13 14 15 16 17 18 19 1a 1b 1c 1d 1e 1f
>  20: 20 21 22 23 24 25 26 27 28 29 2a 2b 2c 2d 2e 2f
>  30: -- -- -- -- -- -- -- -- UU UU -- UU 3c -- -- UU
>  40: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
>  50: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
>  60: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
>  70: -- -- -- -- -- -- -- --
>
I assume that you have these many peripherals.
on the bus am I right?


> So revert the commit for now.
>
> Fixes: d701667bb331 ("i2c: xiic: Do not reset controller before every 
> transfer")
> Signed-off-by: Lars-Peter Clausen 
> ---
>  drivers/i2c/busses/i2c-xiic.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
> index e23a7b0..705cf69 100644
> --- a/drivers/i2c/busses/i2c-xiic.c
> +++ b/drivers/i2c/busses/i2c-xiic.c
> @@ -662,6 +662,9 @@ static void __xiic_start_xfer(struct xiic_i2c *i2c)
>
>  static void xiic_start_xfer(struct xiic_i2c *i2c)
>  {
> +   spin_lock(>lock);
> +   xiic_reinit(i2c);
> +   spin_unlock(>lock);
>
> __xiic_start_xfer(i2c);
>  }
> --
> 2.1.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] i2c: xiic: Replace spinlock with mutex

[Shubhrajyoti Datta]

On Tue, Nov 17, 2015 at 10:48 AM, Shubhrajyoti Datta
<shubhrajyoti.da...@gmail.com> wrote:
> On Mon, Nov 16, 2015 at 7:12 PM, Lars-Peter Clausen <l...@metafoo.de> wrote:
>> All protected sections are only called from sleep-able context, so there is
>> no need to use a spinlock.
>
> Looks good to me. Feel free to add my reviewed by.

Reviewed-by: Shubhrajyoti Datta <shubh...@xilinx.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] i2c: xiic: Prevent concurrent running of the IRQ handler and __xiic_start_xfer()

[Shubhrajyoti Datta]

On Mon, Nov 16, 2015 at 7:12 PM, Lars-Peter Clausen <l...@metafoo.de> wrote:
> Prior to commit e6c9a037bc8a ("i2c: xiic: Remove the disabling of
> interrupts") IRQs where disabled when the initial __xiic_start_xfer() was
> called. After the commit the interrupt is enabled while the function is
> running, this means it is possible for the interrupt to be triggered while
> the function is still running. When this happens the internal data
> structures get corrupted and undefined behavior can occur like the
> following crash:
>
> Internal error: Oops: 17 [#1] PREEMPT SMP ARM
> Modules linked in:
> CPU: 0 PID: 2040 Comm: i2cdetect Not tainted 4.0.0-02856-g047a308 
> #10956
> Hardware name: Xilinx Zynq Platform
> task: ee0c9500 ti: e99a2000 task.ti: e99a2000
> PC is at __xiic_start_xfer+0x6c4/0x7c8
> LR is at __xiic_start_xfer+0x690/0x7c8
> pc : []lr : []psr: 800f0013
> sp : e99a3da8  ip :   fp : 
> r10: 0001  r9 : 600f0013  r8 : f018
> r7 : f018  r6 : c064e444  r5 : 0017  r4 : ee031010
> r3 :   r2 :   r1 : 600f0013  r0 : 000f
> Flags: Nzcv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment user
> Control: 18c5387d  Table: 29a5404a  DAC: 0015
> Process i2cdetect (pid: 2040, stack limit = 0xe99a2210)
> Stack: (0xe99a3da8 to 0xe99a4000)
> 3da0:   ee031010  0001 ee031020 ee031224 
> c02bc5ec
> 3dc0: ee34c604  ee0c9500 e99a3dcc e99a3dd0 e99a3dd0 e99a3dd8 
> c069f0e8
> 3de0:  ee031020 c064e100 90bb e99a3e48 c02b6590 ee031020 
> 0001
> 3e00: e99a3e48 ee031020  e99a3e63 0001 c02b6ec4  
> 
> 3e20:  c02b7320 e99a3ef0   e99e3df0  
> 
> 3e40: 0103 2814575f 003e c00a e99a3e85 0001003e ee0c 
> e99a3e63
> 3e60: eefd3578 c064e61c ee0c9500 c0041e04 056c e9a56db8 6e5a 
> b6f5c000
> 3e80: ee0c9548 eefd0040 0001 eefd3540 ee0c9500 eefd39a0 c064b540 
> ee0c9500
> 3ea0:  ee92b000  bef4862c ee34c600 e99ecdc0 0720 
> 0003
> 3ec0: e99a2000   c02b8b30    
> e99a3f24
> 3ee0: b6e8   c04257e8  e99a3f24 c02b8f08 
> 0703
> 3f00: 0003 c02116bc ee935300  bef4862c ee34c600 e99ecdc0 
> c02b91f0
> 3f20: e99ecdc0 0720 bef4862c eeb725f8 e99ecdc0 c00c9e2c 0003 
> 0003
> 3f40: ee248dc0  ee248dc8 0002 eeb7c1a8   
> c00bb360
> 3f60:   0003 ee248dc0 bef4862c e99ecdc0 e99ecdc0 
> 0720
> 3f80: 0003 e99a2000  c00c9f68   b6f22000 
> 0036
> 3fa0: c000dfa4 c000de20   0003 0720 bef4862c 
> bef4862c
> 3fc0:   b6f22000 0036   b6f6 
> 
> 3fe0: 00013040 bef48614 8cab b6ecdbe6 400f0030 0003 2f7fd821 
> 2f7fdc21
> [] (__xiic_start_xfer) from [] 
> (xiic_xfer+0x94/0x168)
> [] (xiic_xfer) from [] (__i2c_transfer+0x4c/0x7c)
> [] (__i2c_transfer) from [] 
> (i2c_transfer+0x9c/0xc4)
> [] (i2c_transfer) from [] 
> (i2c_smbus_xfer+0x3a0/0x4ec)
> [] (i2c_smbus_xfer) from [] 
> (i2cdev_ioctl_smbus+0xb0/0x214)
> [] (i2cdev_ioctl_smbus) from [] 
> (i2cdev_ioctl+0xa0/0x1d4)
> [] (i2cdev_ioctl) from [] 
> (do_vfs_ioctl+0x4b0/0x5b8)
> [] (do_vfs_ioctl) from [] (SyS_ioctl+0x34/0x5c)
> [] (SyS_ioctl) from [] (ret_fast_syscall+0x0/0x34)
> Code: e283300c e5843210 eafffe64 e5943210 (e1d320b4)
>
> The issue can easily be reproduced by performing I2C access under high
> system load or IO load.
>
> To fix the issue protect the invocation to __xiic_start_xfer() form
> xiic_start_xfer() with the same lock that is used to protect the interrupt
> handler.
>

Reviewed-by: Shubhrajyoti Datta <shubh...@xilinx.com>

Though I feel that this is the correct thing to do missed out in the
original code and exposed
by removing the disabling of interrupts. As disabling interrupts to
synchronize is crude.


> Fixes: e6c9a037bc8a ("i2c: xiic: Remove the disabling of interrupts")
> Signed-off-by: Lars-Peter Clausen <l...@metafoo.de>
> ---
>  drivers/i2c/busses/i2c-xiic.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
> index 705cf69..0b20449 100644
>

Re: [PATCH 1/3] i2c: Revert "i2c: xiic: Do not reset controller before every transfer"

[Shubhrajyoti Datta]

On Tue, Nov 17, 2015 at 10:47 AM, Shubhrajyoti Datta
<shubhrajyoti.da...@gmail.com> wrote:
> On Mon, Nov 16, 2015 at 7:12 PM, Lars-Peter Clausen <l...@metafoo.de> wrote:
>> Commit d701667bb331 ("i2c: xiic: Do not reset controller before every
>> transfer") removed the reinitialization of the controller before the start
>> of each transfer. Apparently this change is not safe to make and the commit
>> results in random I2C bus failures.
>
> Which is the platform and the ip version that you  saw the issue.
> Did you see the issue with read and write as  well?

Also having a closer look, if we reinit the status registers and bus
busy will be
cleared.

> .
>
>>
>> An easy way to trigger the issue is to run i2cdetect.
>>
>> Without the patch applied:
>>  0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
>>  00:  -- -- -- -- -- -- -- -- -- -- -- -- --
>>  10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
>>  20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
>>  30: -- -- -- -- -- -- -- -- UU UU -- UU 3c -- -- UU
>>  40: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
>>  50: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
>>  60: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
>>  70: -- -- -- -- -- -- -- --
>>
>> With the patch applied every other or so invocation:
>>  0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
>>  00:  03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f
>>  10: 10 11 12 13 14 15 16 17 18 19 1a 1b 1c 1d 1e 1f
>>  20: 20 21 22 23 24 25 26 27 28 29 2a 2b 2c 2d 2e 2f
>>  30: -- -- -- -- -- -- -- -- UU UU -- UU 3c -- -- UU
>>  40: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
>>  50: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
>>  60: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
>>  70: -- -- -- -- -- -- -- --
>>
> I assume that you have these many peripherals.
> on the bus am I right?
>
>
>> So revert the commit for now.
>>
>> Fixes: d701667bb331 ("i2c: xiic: Do not reset controller before every 
>> transfer")
>> Signed-off-by: Lars-Peter Clausen <l...@metafoo.de>
>> ---
>>  drivers/i2c/busses/i2c-xiic.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
>> index e23a7b0..705cf69 100644
>> --- a/drivers/i2c/busses/i2c-xiic.c
>> +++ b/drivers/i2c/busses/i2c-xiic.c
>> @@ -662,6 +662,9 @@ static void __xiic_start_xfer(struct xiic_i2c *i2c)
>>
>>  static void xiic_start_xfer(struct xiic_i2c *i2c)
>>  {
>> +   spin_lock(>lock);
>> +   xiic_reinit(i2c);
>> +   spin_unlock(>lock);
>>
>> __xiic_start_xfer(i2c);
>>  }
>> --
>> 2.1.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
>> the body of a message to majord...@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] i2c: cadence: Move to sensible power management

[Shubhrajyoti Datta]

On Wed, Oct 28, 2015 at 9:48 PM, Sören Brinkmann
<soren.brinkm...@xilinx.com> wrote:
> Hi Shubhrajyoti,
>
>
> On Wed, 2015-10-28 at 12:56PM +0530, Shubhrajyoti Datta wrote:
>> Currently the clocks are enabled at probe and disabled at remove.
>> Which keeps the clocks enabled even if no transaction is going on.
>> This patch enables the clocks at the start of transfer and disables
>> after it.
>>
>> Also adapts to runtime pm.
>> Remove xi2c->suspended and use pm runtime status instead.
>>
>> converts dev pm to const to silence a checkpatch warning.
>>
>> Signed-off-by: Shubhrajyoti Datta <shubh...@xilinx.com>
>
> To me, this looks all good. Just one small concern below.

Thanks for the review.

>
>> ---
>> changes since v2
>> update the cc list
>>
>>  drivers/i2c/busses/i2c-cadence.c |   73 
>> --
>>  1 files changed, 46 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-cadence.c 
>> b/drivers/i2c/busses/i2c-cadence.c
>> index 84deed6..6b08d16 100644
>> --- a/drivers/i2c/busses/i2c-cadence.c
>> +++ b/drivers/i2c/busses/i2c-cadence.c
>> @@ -18,6 +18,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>
>>  /* Register offsets for the I2C device. */
>>  #define CDNS_I2C_CR_OFFSET   0x00 /* Control Register, RW */
>> @@ -96,6 +97,8 @@
>>CDNS_I2C_IXR_COMP)
>>
>>  #define CDNS_I2C_TIMEOUT msecs_to_jiffies(1000)
>> +/* timeout for pm runtime autosuspend */
>> +#define CNDS_I2C_PM_TIMEOUT  1000/* ms */
>>
>>  #define CDNS_I2C_FIFO_DEPTH  16
>>  /* FIFO depth at which the DATA interrupt occurs */
>> @@ -128,7 +131,6 @@
>>   * @xfer_done:   Transfer complete status
>>   * @p_send_buf:  Pointer to transmit buffer
>>   * @p_recv_buf:  Pointer to receive buffer
>> - * @suspended:   Flag holding the device's PM status
>>   * @send_count:  Number of bytes still expected to send
>>   * @recv_count:  Number of bytes still expected to receive
>>   * @curr_recv_count: Number of bytes to be received in current transfer
>> @@ -141,6 +143,7 @@
>>   * @quirks:  flag for broken hold bit usage in r1p10
>>   */
>>  struct cdns_i2c {
>> + struct device   *dev;
>>   void __iomem *membase;
>>   struct i2c_adapter adap;
>>   struct i2c_msg *p_msg;
>> @@ -148,7 +151,6 @@ struct cdns_i2c {
>>   struct completion xfer_done;
>>   unsigned char *p_send_buf;
>>   unsigned char *p_recv_buf;
>> - u8 suspended;
>
> There might have been a reason to store this flag here. Did you test
> this with lockdep and CONFIG_DEBUG_ATOMIC_SLEEP? Just to make sure that
> nothing that can sleep is called from atomic context.
Done now.


Essentially this is a flag is set in suspend routine. and checked in
the isr I use
pm_runtime_suspended(id->dev) instead.

>
> Sören
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] i2c: cadence: Move to sensible power management

[Shubhrajyoti Datta]

On Sun, Oct 25, 2015 at 9:10 PM, Wolfram Sang <w...@the-dreams.de> wrote:
> On Thu, Oct 15, 2015 at 05:56:55PM +0530, Shubhrajyoti Datta wrote:
>> Currently the clocks are enabled at probe and disabled at remove.
>> Which keeps the clocks enabled even if no transaction is going on.
>> This patch enables the clocks at the start of transfer and disables
>> after it.
>>
>> Also adapts to runtime pm.
>> Remove xi2c->suspended and use pm runtime status instead.
>>
>> converts dev pm to const to silence a checkpatch warning.
>>
>> Signed-off-by: Shubhrajyoti Datta <shubh...@xilinx.com>
>
> Can you resend please with the driver maintainers in cc? Best use
> get_maintainer.pl to create the CC list. Thanks!
>
Just resent it with updated cc list.
Thanks
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] i2c: cadence: Move to sensible power management

[Shubhrajyoti Datta]

Currently the clocks are enabled at probe and disabled at remove.
Which keeps the clocks enabled even if no transaction is going on.
This patch enables the clocks at the start of transfer and disables
after it.

Also adapts to runtime pm.
Remove xi2c->suspended and use pm runtime status instead.

converts dev pm to const to silence a checkpatch warning.

Signed-off-by: Shubhrajyoti Datta <shubh...@xilinx.com>
---
changes since v2
update the cc list

 drivers/i2c/busses/i2c-cadence.c |   73 --
 1 files changed, 46 insertions(+), 27 deletions(-)

diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c
index 84deed6..6b08d16 100644
--- a/drivers/i2c/busses/i2c-cadence.c
+++ b/drivers/i2c/busses/i2c-cadence.c
@@ -18,6 +18,7 @@
 #include 
 #include 
 #include 
+#include 
 
 /* Register offsets for the I2C device. */
 #define CDNS_I2C_CR_OFFSET 0x00 /* Control Register, RW */
@@ -96,6 +97,8 @@
 CDNS_I2C_IXR_COMP)
 
 #define CDNS_I2C_TIMEOUT   msecs_to_jiffies(1000)
+/* timeout for pm runtime autosuspend */
+#define CNDS_I2C_PM_TIMEOUT1000/* ms */
 
 #define CDNS_I2C_FIFO_DEPTH16
 /* FIFO depth at which the DATA interrupt occurs */
@@ -128,7 +131,6 @@
  * @xfer_done: Transfer complete status
  * @p_send_buf:Pointer to transmit buffer
  * @p_recv_buf:Pointer to receive buffer
- * @suspended: Flag holding the device's PM status
  * @send_count:Number of bytes still expected to send
  * @recv_count:Number of bytes still expected to receive
  * @curr_recv_count:   Number of bytes to be received in current transfer
@@ -141,6 +143,7 @@
  * @quirks:flag for broken hold bit usage in r1p10
  */
 struct cdns_i2c {
+   struct device   *dev;
void __iomem *membase;
struct i2c_adapter adap;
struct i2c_msg *p_msg;
@@ -148,7 +151,6 @@ struct cdns_i2c {
struct completion xfer_done;
unsigned char *p_send_buf;
unsigned char *p_recv_buf;
-   u8 suspended;
unsigned int send_count;
unsigned int recv_count;
unsigned int curr_recv_count;
@@ -569,9 +571,14 @@ static int cdns_i2c_master_xfer(struct i2c_adapter *adap, 
struct i2c_msg *msgs,
struct cdns_i2c *id = adap->algo_data;
bool hold_quirk;
 
+   ret = pm_runtime_get_sync(id->dev);
+   if (ret < 0)
+   return ret;
/* Check if the bus is free */
-   if (cdns_i2c_readreg(CDNS_I2C_SR_OFFSET) & CDNS_I2C_SR_BA)
-   return -EAGAIN;
+   if (cdns_i2c_readreg(CDNS_I2C_SR_OFFSET) & CDNS_I2C_SR_BA) {
+   ret = -EAGAIN;
+   goto out;
+   }
 
hold_quirk = !!(id->quirks & CDNS_I2C_BROKEN_HOLD_BIT);
/*
@@ -590,7 +597,8 @@ static int cdns_i2c_master_xfer(struct i2c_adapter *adap, 
struct i2c_msg *msgs,
if (msgs[count].flags & I2C_M_RD) {
dev_warn(adap->dev.parent,
 "Can't do repeated start after a 
receive message\n");
-   return -EOPNOTSUPP;
+   ret = -EOPNOTSUPP;
+   goto out;
}
}
id->bus_hold_flag = 1;
@@ -608,20 +616,26 @@ static int cdns_i2c_master_xfer(struct i2c_adapter *adap, 
struct i2c_msg *msgs,
 
ret = cdns_i2c_process_msg(id, msgs, adap);
if (ret)
-   return ret;
+   goto out;
 
/* Report the other error interrupts to application */
if (id->err_status) {
cdns_i2c_master_reset(adap);
 
-   if (id->err_status & CDNS_I2C_IXR_NACK)
-   return -ENXIO;
-
-   return -EIO;
+   if (id->err_status & CDNS_I2C_IXR_NACK) {
+   ret = -ENXIO;
+   goto out;
+   }
+   ret = -EIO;
+   goto out;
}
}
 
-   return num;
+   ret = num;
+out:
+   pm_runtime_mark_last_busy(id->dev);
+   pm_runtime_put_autosuspend(id->dev);
+   return ret;
 }
 
 /**
@@ -760,7 +774,7 @@ static int cdns_i2c_clk_notifier_cb(struct notifier_block 
*nb, unsigned long
struct clk_notifier_data *ndata = data;
struct cdns_i2c *id = to_cdns_i2c(nb);
 
-   if (id->suspended)
+   if (pm_runtime_suspended(id->dev))
return NOTIFY_OK;
 
switch (event) {
@@ -808,14 +822,12 @@ static int cdns_i2c_clk_notifier_cb(struct notifier_block 
*nb, unsigned long
  *
  * Return: 0 always

Re: [PATCHv2] i2c: cadence: Enable power management

[Shubhrajyoti Datta]

On Wed, Oct 28, 2015 at 11:34 AM, Shubhrajyoti Datta
<shubhrajyoti.da...@xilinx.com> wrote:
> Currently the clocks are enabled at probe and disabled at remove.
> This patch enables the clocks at the start of transfer and disables
> after it.
>
> Also adapts to runtime pm.
> Remove xi2c->suspended and use pm runtime status instead.
>
> converts dev pm to const to silence a checkpatch warning.
>
Please ignore this patch
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv2] i2c: cadence: Enable power management

[Shubhrajyoti Datta]

Currently the clocks are enabled at probe and disabled at remove.
This patch enables the clocks at the start of transfer and disables
after it.

Also adapts to runtime pm.
Remove xi2c->suspended and use pm runtime status instead.

converts dev pm to const to silence a checkpatch warning.

Signed-off-by: Shubhrajyoti Datta <shubh...@xilinx.com>
---
changes since v1:
update the cc list.

 drivers/i2c/busses/i2c-cadence.c |   74 --
 1 files changed, 47 insertions(+), 27 deletions(-)

diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c
index f5227c5..ba3c67c 100644
--- a/drivers/i2c/busses/i2c-cadence.c
+++ b/drivers/i2c/busses/i2c-cadence.c
@@ -18,6 +18,7 @@
 #include 
 #include 
 #include 
+#include 
 
 /* Register offsets for the I2C device. */
 #define CDNS_I2C_CR_OFFSET 0x00 /* Control Register, RW */
@@ -98,6 +99,8 @@
 CDNS_I2C_IXR_COMP)
 
 #define CDNS_I2C_TIMEOUT   msecs_to_jiffies(1000)
+/* timeout for pm runtime autosuspend */
+#define CNDS_I2C_PM_TIMEOUT1000/* ms */
 
 #define CDNS_I2C_FIFO_DEPTH16
 /* FIFO depth at which the DATA interrupt occurs */
@@ -130,7 +133,6 @@
  * @xfer_done: Transfer complete status
  * @p_send_buf:Pointer to transmit buffer
  * @p_recv_buf:Pointer to receive buffer
- * @suspended: Flag holding the device's PM status
  * @send_count:Number of bytes still expected to send
  * @recv_count:Number of bytes still expected to receive
  * @curr_recv_count:   Number of bytes to be received in current transfer
@@ -143,6 +145,7 @@
  * @quirks:flag for broken hold bit usage in r1p10
  */
 struct cdns_i2c {
+   struct device   *dev;
void __iomem *membase;
struct i2c_adapter adap;
struct i2c_msg *p_msg;
@@ -150,7 +153,6 @@ struct cdns_i2c {
struct completion xfer_done;
unsigned char *p_send_buf;
unsigned char *p_recv_buf;
-   u8 suspended;
unsigned int send_count;
unsigned int recv_count;
unsigned int curr_recv_count;
@@ -623,10 +625,16 @@ static int cdns_i2c_master_xfer(struct i2c_adapter *adap, 
struct i2c_msg *msgs,
u32 reg;
struct cdns_i2c *id = adap->algo_data;
bool hold_quirk;
+
+   ret = pm_runtime_get_sync(id->dev);
+   if (ret < 0)
+   return ret;
/* Check if the bus is free */
if (msgs->len)
-   if (cdns_i2c_readreg(CDNS_I2C_SR_OFFSET) & CDNS_I2C_SR_BA)
-   return -EAGAIN;
+   if (cdns_i2c_readreg(CDNS_I2C_SR_OFFSET) & CDNS_I2C_SR_BA) {
+   ret = -EAGAIN;
+   goto out;
+   }
 
hold_quirk = !!(id->quirks & CDNS_I2C_BROKEN_HOLD_BIT);
/*
@@ -645,7 +653,8 @@ static int cdns_i2c_master_xfer(struct i2c_adapter *adap, 
struct i2c_msg *msgs,
if (msgs[count].flags & I2C_M_RD) {
dev_warn(adap->dev.parent,
 "Can't do repeated start after a 
receive message\n");
-   return -EOPNOTSUPP;
+   ret = -EOPNOTSUPP;
+   goto out;
}
}
id->bus_hold_flag = 1;
@@ -663,20 +672,26 @@ static int cdns_i2c_master_xfer(struct i2c_adapter *adap, 
struct i2c_msg *msgs,
 
ret = cdns_i2c_process_msg(id, msgs, adap);
if (ret)
-   return ret;
+   goto out;
 
/* Report the other error interrupts to application */
if (id->err_status) {
cdns_i2c_master_reset(adap);
 
-   if (id->err_status & CDNS_I2C_IXR_NACK)
-   return -ENXIO;
-
-   return -EIO;
+   if (id->err_status & CDNS_I2C_IXR_NACK) {
+   ret = -ENXIO;
+   goto out;
+   }
+   ret = -EIO;
+   goto out;
}
}
 
-   return num;
+   ret = num;
+out:
+   pm_runtime_mark_last_busy(id->dev);
+   pm_runtime_put_autosuspend(id->dev);
+   return ret;
 }
 
 /**
@@ -815,7 +830,7 @@ static int cdns_i2c_clk_notifier_cb(struct notifier_block 
*nb, unsigned long
struct clk_notifier_data *ndata = data;
struct cdns_i2c *id = to_cdns_i2c(nb);
 
-   if (id->suspended)
+   if (pm_runtime_suspended(id->dev))
return NOTIFY_OK;
 
switch (event) {
@@ -863,14 +878,12 @@ static int cdns_i2c_clk_notifier_cb(struct notifier_block 
*nb, unsigned long
  

Re: [PATCH] i2c: pnx: fix warnings caused by enabling unprepared clock

[Shubhrajyoti Datta]

On Mon, Oct 19, 2015 at 11:49 PM, Vladimir Zapolskiy <v...@mleia.com> wrote:
> On 19.10.2015 19:21, Shubhrajyoti Datta wrote:
>> On Sun, Oct 18, 2015 at 12:22 AM, Vladimir Zapolskiy <v...@mleia.com> wrote:
>>> If common clock framework is configured, the driver generates a warning,
>>> which is fixed by this change:
>> Maybe just describe the issue and the fix.
>> Is it just a warning or  the clk enable doesn't work ?
>>
>> I feel the crash log in the commit msg is not very informative.
>
> It is not a crash, it is a WARN_ON(1) backtrace.

ok

>
> The backtrace is informative enough IMHO, because if you check the code
> around given drivers/clk/clk.c:727 you may find the rootcause, the
> WARN_ON() and that the clock is not enabled as a result.
>
> CLK_COMMON selects HAVE_CLK_PREPARE and all drivers used on platforms
> with common clock framework must have clk_prepare/clk_unprepare, this
> information is omitted as well-known.
>
> But if you insist, I will improve the commit message, I'm interested in
> applying this trivial fix as soon as possible, then concentrate on doing
> something more fascinating.

My request will be to describe the issue.
Like the probe fails or you access registers with clocks not enabled

Or it is a warning fix and the controller works etc.
>
>>>
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] i2c: pnx: fix warnings caused by enabling unprepared clock

[Shubhrajyoti Datta]

On Sun, Oct 18, 2015 at 12:22 AM, Vladimir Zapolskiy  wrote:
> If common clock framework is configured, the driver generates a warning,
> which is fixed by this change:
Maybe just describe the issue and the fix.
Is it just a warning or  the clk enable doesn't work ?

I feel the crash log in the commit msg is not very informative.

>
> WARNING: CPU: 0 PID: 1 at drivers/clk/clk.c:727 
> clk_core_enable+0x2c/0xa4()
> Modules linked in:
> CPU: 0 PID: 1 Comm: swapper Not tainted 4.3.0-rc2+ #171
> Hardware name: LPC32XX SoC (Flattened Device Tree)
> Backtrace:
> [<>] (dump_backtrace) from [<>] (show_stack+0x18/0x1c)
> [<>] (show_stack) from [<>] (dump_stack+0x20/0x28)
> [<>] (dump_stack) from [<>] (warn_slowpath_common+0x90/0xb8)
> [<>] (warn_slowpath_common) from [<>] (warn_slowpath_null+0x24/0x2c)
> [<>] (warn_slowpath_null) from [<>] (clk_core_enable+0x2c/0xa4)
> [<>] (clk_core_enable) from [<>] (clk_enable+0x24/0x38)
> [<>] (clk_enable) from [<>] (i2c_pnx_probe+0x108/0x2a8)
> [<>] (i2c_pnx_probe) from [<>] (platform_drv_probe+0x50/0xa0)
> [<>] (platform_drv_probe) from [<>] (driver_probe_device+0x18c/0x408)
> [<>] (driver_probe_device) from [<>] (__driver_attach+0x70/0x94)
> [<>] (__driver_attach) from [<>] (bus_for_each_dev+0x74/0x98)
> [<>] (bus_for_each_dev) from [<>] (driver_attach+0x20/0x28)
> [<>] (driver_attach) from [<>] (bus_add_driver+0x11c/0x248)
> [<>] (bus_add_driver) from [<>] (driver_register+0xa4/0xe8)
> [<>] (driver_register) from [<>] (__platform_driver_register+0x50/0x64)
> [<>] (__platform_driver_register) from [<>] (i2c_adap_pnx_init+0x18/0x20)
> [<>] (i2c_adap_pnx_init) from [<>] (do_one_initcall+0x11c/0x1dc)
> [<>] (do_one_initcall) from [<>] (kernel_init_freeable+0x10c/0x1d4)
> [<>] (kernel_init_freeable) from [<>] (kernel_init+0x10/0xec)
> [<>] (kernel_init) from [<>] (ret_from_fork+0x14/0x24)
>
> Signed-off-by: Vladimir Zapolskiy 
> ---
>  drivers/i2c/busses/i2c-pnx.c | 10 +-
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-pnx.c b/drivers/i2c/busses/i2c-pnx.c
> index e814a36..6f8b446 100644
> --- a/drivers/i2c/busses/i2c-pnx.c
> +++ b/drivers/i2c/busses/i2c-pnx.c
> @@ -600,7 +600,7 @@ static int i2c_pnx_controller_suspend(struct device *dev)
>  {
> struct i2c_pnx_algo_data *alg_data = dev_get_drvdata(dev);
>
> -   clk_disable(alg_data->clk);
> +   clk_disable_unprepare(alg_data->clk);
>
> return 0;
>  }
> @@ -609,7 +609,7 @@ static int i2c_pnx_controller_resume(struct device *dev)
>  {
> struct i2c_pnx_algo_data *alg_data = dev_get_drvdata(dev);
>
> -   return clk_enable(alg_data->clk);
> +   return clk_prepare_enable(alg_data->clk);
>  }
>
>  static SIMPLE_DEV_PM_OPS(i2c_pnx_pm,
> @@ -672,7 +672,7 @@ static int i2c_pnx_probe(struct platform_device *pdev)
> if (IS_ERR(alg_data->ioaddr))
> return PTR_ERR(alg_data->ioaddr);
>
> -   ret = clk_enable(alg_data->clk);
> +   ret = clk_prepare_enable(alg_data->clk);
> if (ret)
> return ret;
>
> @@ -726,7 +726,7 @@ static int i2c_pnx_probe(struct platform_device *pdev)
> return 0;
>
>  out_clock:
> -   clk_disable(alg_data->clk);
> +   clk_disable_unprepare(alg_data->clk);
> return ret;
>  }
>
> @@ -735,7 +735,7 @@ static int i2c_pnx_remove(struct platform_device *pdev)
> struct i2c_pnx_algo_data *alg_data = platform_get_drvdata(pdev);
>
> i2c_del_adapter(_data->adapter);
> -   clk_disable(alg_data->clk);
> +   clk_disable_unprepare(alg_data->clk);
>
> return 0;
>  }
> --
> 2.1.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] i2c: cadence: Move to sensible power management

[Shubhrajyoti Datta]

On Thu, Oct 15, 2015 at 5:56 PM, Shubhrajyoti Datta
<shubhrajyoti.da...@xilinx.com> wrote:
> Currently the clocks are enabled at probe and disabled at remove.
> Which keeps the clocks enabled even if no transaction is going on.
> This patch enables the clocks at the start of transfer and disables
> after it.
>
> Also adapts to runtime pm.
> Remove xi2c->suspended and use pm runtime status instead.
>
> converts dev pm to const to silence a checkpatch warning.
>
> Signed-off-by: Shubhrajyoti Datta <shubh...@xilinx.com>

for testing suspend resume
with the below patch

http://thread.gmane.org/gmane.linux.can/8639
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] i2c: cadence: Move to sensible power management

[Shubhrajyoti Datta]

Currently the clocks are enabled at probe and disabled at remove.
Which keeps the clocks enabled even if no transaction is going on.
This patch enables the clocks at the start of transfer and disables
after it.

Also adapts to runtime pm.
Remove xi2c->suspended and use pm runtime status instead.

converts dev pm to const to silence a checkpatch warning.

Signed-off-by: Shubhrajyoti Datta <shubh...@xilinx.com>
---
 drivers/i2c/busses/i2c-cadence.c |   73 --
 1 files changed, 46 insertions(+), 27 deletions(-)

diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c
index 84deed6..6b08d16 100644
--- a/drivers/i2c/busses/i2c-cadence.c
+++ b/drivers/i2c/busses/i2c-cadence.c
@@ -18,6 +18,7 @@
 #include 
 #include 
 #include 
+#include 
 
 /* Register offsets for the I2C device. */
 #define CDNS_I2C_CR_OFFSET 0x00 /* Control Register, RW */
@@ -96,6 +97,8 @@
 CDNS_I2C_IXR_COMP)
 
 #define CDNS_I2C_TIMEOUT   msecs_to_jiffies(1000)
+/* timeout for pm runtime autosuspend */
+#define CNDS_I2C_PM_TIMEOUT1000/* ms */
 
 #define CDNS_I2C_FIFO_DEPTH16
 /* FIFO depth at which the DATA interrupt occurs */
@@ -128,7 +131,6 @@
  * @xfer_done: Transfer complete status
  * @p_send_buf:Pointer to transmit buffer
  * @p_recv_buf:Pointer to receive buffer
- * @suspended: Flag holding the device's PM status
  * @send_count:Number of bytes still expected to send
  * @recv_count:Number of bytes still expected to receive
  * @curr_recv_count:   Number of bytes to be received in current transfer
@@ -141,6 +143,7 @@
  * @quirks:flag for broken hold bit usage in r1p10
  */
 struct cdns_i2c {
+   struct device   *dev;
void __iomem *membase;
struct i2c_adapter adap;
struct i2c_msg *p_msg;
@@ -148,7 +151,6 @@ struct cdns_i2c {
struct completion xfer_done;
unsigned char *p_send_buf;
unsigned char *p_recv_buf;
-   u8 suspended;
unsigned int send_count;
unsigned int recv_count;
unsigned int curr_recv_count;
@@ -569,9 +571,14 @@ static int cdns_i2c_master_xfer(struct i2c_adapter *adap, 
struct i2c_msg *msgs,
struct cdns_i2c *id = adap->algo_data;
bool hold_quirk;
 
+   ret = pm_runtime_get_sync(id->dev);
+   if (ret < 0)
+   return ret;
/* Check if the bus is free */
-   if (cdns_i2c_readreg(CDNS_I2C_SR_OFFSET) & CDNS_I2C_SR_BA)
-   return -EAGAIN;
+   if (cdns_i2c_readreg(CDNS_I2C_SR_OFFSET) & CDNS_I2C_SR_BA) {
+   ret = -EAGAIN;
+   goto out;
+   }
 
hold_quirk = !!(id->quirks & CDNS_I2C_BROKEN_HOLD_BIT);
/*
@@ -590,7 +597,8 @@ static int cdns_i2c_master_xfer(struct i2c_adapter *adap, 
struct i2c_msg *msgs,
if (msgs[count].flags & I2C_M_RD) {
dev_warn(adap->dev.parent,
 "Can't do repeated start after a 
receive message\n");
-   return -EOPNOTSUPP;
+   ret = -EOPNOTSUPP;
+   goto out;
}
}
id->bus_hold_flag = 1;
@@ -608,20 +616,26 @@ static int cdns_i2c_master_xfer(struct i2c_adapter *adap, 
struct i2c_msg *msgs,
 
ret = cdns_i2c_process_msg(id, msgs, adap);
if (ret)
-   return ret;
+   goto out;
 
/* Report the other error interrupts to application */
if (id->err_status) {
cdns_i2c_master_reset(adap);
 
-   if (id->err_status & CDNS_I2C_IXR_NACK)
-   return -ENXIO;
-
-   return -EIO;
+   if (id->err_status & CDNS_I2C_IXR_NACK) {
+   ret = -ENXIO;
+   goto out;
+   }
+   ret = -EIO;
+   goto out;
}
}
 
-   return num;
+   ret = num;
+out:
+   pm_runtime_mark_last_busy(id->dev);
+   pm_runtime_put_autosuspend(id->dev);
+   return ret;
 }
 
 /**
@@ -760,7 +774,7 @@ static int cdns_i2c_clk_notifier_cb(struct notifier_block 
*nb, unsigned long
struct clk_notifier_data *ndata = data;
struct cdns_i2c *id = to_cdns_i2c(nb);
 
-   if (id->suspended)
+   if (pm_runtime_suspended(id->dev))
return NOTIFY_OK;
 
switch (event) {
@@ -808,14 +822,12 @@ static int cdns_i2c_clk_notifier_cb(struct notifier_block 
*nb, unsigned long
  *
  * Return: 0 always
  */
-static int __maybe_unused cdns_i2

Re: [Patch V8] i2c: imx: add runtime pm support to improve the performance

[Shubhrajyoti Datta]

clk_enable is called here.
enables the clock here.
>
> The calling sequence is:
> pm_runtime_use_autosuspend(>dev);
> pm_runtime_set_active(>dev);
> pm_runtime_enable(>dev);
> pm_runtime_get_sync(>dev);
are we not enabling it again?

there by effectively disabling pm.

Or did I missing something.

>
> The cleanup order is:
> pm_runtime_put_noidle(>dev);
> pm_runtime_disable(>dev);
> pm_runtime_set_suspended(>dev);
>
> Is it that I miss pm_runtime_dont_use_autosuspend(>dev);
>
> Thank you!
>
> Best Reguards
> Gao Pan
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH-v5 2/5] i2c: pxa: enable/disable i2c module across msg xfer

[Shubhrajyoti Datta]

hi ,
On Tue, Jul 21, 2015 at 6:11 PM, Vaibhav Hiremath
vaibhav.hirem...@linaro.org wrote:
 From: Yi Zhang yizh...@marvell.com

 Enable i2c module/unit before transmission and disable when it
 finishes.

 why?
 It's because the i2c bus may be disturbed if the slave device,
 typically a touch, powers on.

Why should that be an issue?
Is it an errata.

In which mode it is an issue slave / master or both?


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

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

 Signed-off-by: Yi Zhang yizh...@marvell.com
 Signed-off-by: Vaibhav Hiremath vaibhav.hirem...@linaro.org
 ---
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2 2/9] i2c: xiic: move the xiic_process to thread context

[Shubhrajyoti Datta]

On Fri, Jul 10, 2015 at 1:23 PM, Wolfram Sang w...@the-dreams.de wrote:
 On Fri, Jul 10, 2015 at 10:38:11AM +0530, Shubhrajyoti Datta wrote:
 On Thu, Jul 9, 2015 at 11:01 PM, Wolfram Sang w...@the-dreams.de wrote:
   static int xiic_bus_busy(struct xiic_i2c *i2c)
  @@ -602,16 +601,21 @@ static void xiic_start_send(struct xiic_i2c *i2c)
   static irqreturn_t xiic_isr(int irq, void *dev_id)
   {
struct xiic_i2c *i2c = dev_id;
  -
  - spin_lock(i2c-lock);
  + u32 pend, isr, ier;
  + irqreturn_t ret = IRQ_HANDLED;
  + /* Do not processes a devices interrupts if the device has no
  +  * interrupts pending
  +  */
 
  Shouldn't you init 'ret' to IRQ_NONE then?
 

 Indeed I missed it.

 Can you test this change on HW and report back?

I have tested it.


 Thanks,

Wolfram

--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2 6/9] i2c: xiic: Add a debug msg in case of timeout

[Shubhrajyoti Datta]

On Thu, Jul 9, 2015 at 10:59 PM, Wolfram Sang w...@the-dreams.de wrote:
 On Wed, Jun 17, 2015 at 08:48:16PM +0530, Shubhrajyoti Datta wrote:
 Currently we return silently upon a timeout.
 Adding an error message to aid debugability. Not
  a functional change.

 Signed-off-by: Shubhrajyoti Datta shubh...@xilinx.com

 I am not in favour of this. In case of a stalled bus, this can easily
 flood the logs. If it is for debugging, it should also be dev_dbg. And
 then, it probably can also be added when needed. I mean the errno is
 quite descriptive, no?

I agree I had added it for debugging.
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2 0/9] i2c: xiic: cleanups

[Shubhrajyoti Datta]

On Thu, Jul 9, 2015 at 11:05 PM, Wolfram Sang w...@the-dreams.de wrote:
 On Wed, Jun 17, 2015 at 08:48:10PM +0530, Shubhrajyoti Datta wrote:
 This is a series of cleanups for the axi iic.
 tested on zc702 and microblaze.

 Changes from v1
 Updated the commit message.


 Shubhrajyoti Datta (9):
   i2c: xiic: Remove the disabling of interrupts
   i2c: xiic: move the xiic_process to thread context
   i2c: xiic: Do not reset controller before every transfer
   i2c: xiic: Remove the disabling of interrupts
   i2c: xiic: Remove busy loop while waiting for bus busy
   i2c: xiic: Add a debug msg in case of timeout
   i2c: xiic: Remove the Addressed as slave interrupt
   i2c: xiic: Service all interrupts in isr
   i2c: xiic: Do not continue in case of errors in Rx

 Commit messages are a lot better, thanks!

 Two minor comments. If you agree to them, I can simply drop patch 6 and
 fixup patch 2
I agree anyways 2 was like a debug patch.

. Then, no need for resending.

thanks.
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 1/9] i2c: xiic: Do not continue in case of errors in Rx

[Shubhrajyoti Datta]

 -Original Message-
 From: Wolfram Sang [mailto:w...@the-dreams.de]
 Sent: Wednesday, June 17, 2015 5:12 PM
 To: Shubhrajyoti Datta
 Cc: linux-i2c@vger.kernel.org; Shubhrajyoti Datta
 Subject: Re: [PATCH 1/9] i2c: xiic: Do not continue in case of errors in Rx

 Hi,

 On Mon, Jun 15, 2015 at 08:47:52PM +0530, Shubhrajyoti Datta wrote:
  Handle error cases in the Rx path
 
  Signed-off-by: Shubhrajyoti Datta shubh...@xilinx.com

 Your patch descriptions need improvement. They describe what you do, but
 this can be seen in the patch as well. What you need to describe is WHY you
 need this change.

In case  the slave nacks the transaction in the middle of the transfer The 
driver continues.

 What was wrong before,

The usual expectation is that in case of error / nack the transaction is halted.

what is better now.

There are 2 things my series does

1 . The interrupts were disabled in the start of the transfer and re-enabled 
again.
So if there are any error interrupts this will not be handled.

2. Secondly during the isr also the interrupts were disabled.
So if there is any nack in between that may not be respected.

3. I felt in case of all  errors we should
Call xiic_wakeup(i2c, STATE_ERROR); - wake_up(i2c-wait)
And come out of wait.


I made the patches speculatively after reading a bug report that says nacks are 
not
Respected in the middle of a transfer.

However do not have a peripheral that will nack in the middle of a transfer.
Could not test it in that sense.

 Because this driver is long in use, we must be very careful about regressions
 and every change needs a good reason.

I agree.


 So, please rework your patch descriptions.



This email and any attachments are intended for the sole use of the named 
recipient(s) and contain(s) confidential information that may be proprietary, 
privileged or copyrighted under applicable law. If you are not the intended 
recipient, do not read, copy, or forward this email message or any attachments. 
Delete this email message and any attachments immediately.

--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv2 3/9] i2c: xiic: Do not reset controller before every transfer

[Shubhrajyoti Datta]

Currently before every transfer the controller is reinitialised.
We are already resetting the controller upon errors so upon every
transfer is a performance kill.
Remove the same.

Signed-off-by: Shubhrajyoti Datta shubh...@xilinx.com
---
 drivers/i2c/busses/i2c-xiic.c |1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index 1a6e637..92ea52a 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -667,7 +667,6 @@ static void xiic_start_xfer(struct xiic_i2c *i2c)
unsigned long flags;
 
spin_lock_irqsave(i2c-lock, flags);
-   xiic_reinit(i2c);
/* disable interrupts globally */
xiic_setreg32(i2c, XIIC_DGIER_OFFSET, 0);
spin_unlock_irqrestore(i2c-lock, flags);
-- 
1.7.1

--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv2 4/9] i2c: xiic: Remove the disabling of interrupts

[Shubhrajyoti Datta]

Currently before every transfer the interrupts are disabled.
So incase the slave nacks in the middle of the transfer the
current transfer is not aborted. Upon enabling the interrupts
conditions like NACK , arbitration lost will not be masked.
Remove the disabling of the interrupts.

Signed-off-by: Shubhrajyoti Datta shubh...@xilinx.com
---
 drivers/i2c/busses/i2c-xiic.c |7 ---
 1 files changed, 0 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index 92ea52a..d9501ab 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -664,15 +664,8 @@ static void __xiic_start_xfer(struct xiic_i2c *i2c)
 
 static void xiic_start_xfer(struct xiic_i2c *i2c)
 {
-   unsigned long flags;
-
-   spin_lock_irqsave(i2c-lock, flags);
-   /* disable interrupts globally */
-   xiic_setreg32(i2c, XIIC_DGIER_OFFSET, 0);
-   spin_unlock_irqrestore(i2c-lock, flags);
 
__xiic_start_xfer(i2c);
-   xiic_setreg32(i2c, XIIC_DGIER_OFFSET, XIIC_GINTR_ENABLE_MASK);
 }
 
 static int xiic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
-- 
1.7.1

--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv2 8/9] i2c: xiic: Service all interrupts in isr

[Shubhrajyoti Datta]

Currently only one interrupt is serviced in the isr.
In case the multiple interrupts happen simultenously we service and ack
only one of them. Check for all the causes in the isr and service them.

Signed-off-by: Shubhrajyoti Datta shubh...@xilinx.com
---
 drivers/i2c/busses/i2c-xiic.c |   24 ++--
 1 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index dd23a78..182ea68 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -401,11 +401,11 @@ static irqreturn_t xiic_process(int irq, void *dev_id)
 
if (i2c-tx_msg)
xiic_wakeup(i2c, STATE_ERROR);
-
-   } else if (pend  XIIC_INTR_RX_FULL_MASK) {
+   }
+   if (pend  XIIC_INTR_RX_FULL_MASK) {
/* Receive register/FIFO is full */
 
-   clr = XIIC_INTR_RX_FULL_MASK;
+   clr |= XIIC_INTR_RX_FULL_MASK;
if (!i2c-rx_msg) {
dev_dbg(i2c-adap.dev.parent,
%s unexpexted RX IRQ\n, __func__);
@@ -438,9 +438,10 @@ static irqreturn_t xiic_process(int irq, void *dev_id)
__xiic_start_xfer(i2c);
}
}
-   } else if (pend  XIIC_INTR_BNB_MASK) {
+   }
+   if (pend  XIIC_INTR_BNB_MASK) {
/* IIC bus has transitioned to not busy */
-   clr = XIIC_INTR_BNB_MASK;
+   clr |= XIIC_INTR_BNB_MASK;
 
/* The bus is not busy, disable BusNotBusy interrupt */
xiic_irq_dis(i2c, XIIC_INTR_BNB_MASK);
@@ -453,12 +454,12 @@ static irqreturn_t xiic_process(int irq, void *dev_id)
xiic_wakeup(i2c, STATE_DONE);
else
xiic_wakeup(i2c, STATE_ERROR);
-
-   } else if (pend  (XIIC_INTR_TX_EMPTY_MASK | XIIC_INTR_TX_HALF_MASK)) {
+   }
+   if (pend  (XIIC_INTR_TX_EMPTY_MASK | XIIC_INTR_TX_HALF_MASK)) {
/* Transmit register/FIFO is empty or ½ empty */
 
-   clr = pend 
-   (XIIC_INTR_TX_EMPTY_MASK | XIIC_INTR_TX_HALF_MASK);
+   clr |= (pend 
+   (XIIC_INTR_TX_EMPTY_MASK | XIIC_INTR_TX_HALF_MASK));
 
if (!i2c-tx_msg) {
dev_dbg(i2c-adap.dev.parent,
@@ -489,11 +490,6 @@ static irqreturn_t xiic_process(int irq, void *dev_id)
 * make sure to disable tx half
 */
xiic_irq_dis(i2c, XIIC_INTR_TX_HALF_MASK);
-   } else {
-   /* got IRQ which is not acked */
-   dev_err(i2c-adap.dev.parent, %s Got unexpected IRQ\n,
-   __func__);
-   clr = pend;
}
 out:
dev_dbg(i2c-adap.dev.parent, %s clr: 0x%x\n, __func__, clr);
-- 
1.7.1

--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv2 9/9] i2c: xiic: Do not continue in case of errors in Rx

[Shubhrajyoti Datta]

In case of error conditions like Arbitration lost or NACK lets signal
the waiting process.

Handle error cases in the Rx path

Signed-off-by: Shubhrajyoti Datta shubh...@xilinx.com
---
 drivers/i2c/busses/i2c-xiic.c |2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index 182ea68..c071897 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -399,6 +399,8 @@ static irqreturn_t xiic_process(int irq, void *dev_id)
 */
xiic_reinit(i2c);
 
+   if (i2c-rx_msg)
+   xiic_wakeup(i2c, STATE_ERROR);
if (i2c-tx_msg)
xiic_wakeup(i2c, STATE_ERROR);
}
-- 
1.7.1

--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv2 6/9] i2c: xiic: Add a debug msg in case of timeout

[Shubhrajyoti Datta]

Currently we return silently upon a timeout.
Adding an error message to aid debugability. Not
 a functional change.

Signed-off-by: Shubhrajyoti Datta shubh...@xilinx.com
---
 drivers/i2c/busses/i2c-xiic.c |1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index a83f300..66c571e 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -692,6 +692,7 @@ static int xiic_xfer(struct i2c_adapter *adap, struct 
i2c_msg *msgs, int num)
i2c-tx_msg = NULL;
i2c-rx_msg = NULL;
i2c-nmsgs = 0;
+   dev_err(adap-dev.parent, Controller timed out\n);
return -ETIMEDOUT;
}
 }
-- 
1.7.1

--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv2 0/9] i2c: xiic: cleanups

[Shubhrajyoti Datta]

This is a series of cleanups for the axi iic.
tested on zc702 and microblaze.

Changes from v1
Updated the commit message.
 

Shubhrajyoti Datta (9):
  i2c: xiic: Remove the disabling of interrupts
  i2c: xiic: move the xiic_process to thread context
  i2c: xiic: Do not reset controller before every transfer
  i2c: xiic: Remove the disabling of interrupts
  i2c: xiic: Remove busy loop while waiting for bus busy
  i2c: xiic: Add a debug msg in case of timeout
  i2c: xiic: Remove the Addressed as slave interrupt
  i2c: xiic: Service all interrupts in isr
  i2c: xiic: Do not continue in case of errors in Rx

 drivers/i2c/busses/i2c-xiic.c |   75 +++--
 1 files changed, 35 insertions(+), 40 deletions(-)

--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv2 5/9] i2c: xiic: Remove busy loop while waiting for bus busy

[Shubhrajyoti Datta]

Remove the busy loop  while waiting for bus busy.
Instead let the processor sleep.

Signed-off-by: Shubhrajyoti Datta shubh...@xilinx.com
---
 drivers/i2c/busses/i2c-xiic.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index d9501ab..a83f300 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -524,7 +524,7 @@ static int xiic_busy(struct xiic_i2c *i2c)
 */
err = xiic_bus_busy(i2c);
while (err  tries--) {
-   mdelay(1);
+   msleep(1);
err = xiic_bus_busy(i2c);
}
 
-- 
1.7.1

--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv2 1/9] i2c: xiic: Remove the disabling of interrupts

[Shubhrajyoti Datta]

Currently the interrupts are disabled at the start of the
isr and enabled at the end of the isr. Remove the same.

In case the slave device NACKs the transaction while in the isr
the transfer will continue and the NACK interrupt will arrive
only after the isr is serviced.

Signed-off-by: Shubhrajyoti Datta shubh...@xilinx.com
---
 drivers/i2c/busses/i2c-xiic.c |3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index 4dda23f..912780a 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -604,14 +604,11 @@ static irqreturn_t xiic_isr(int irq, void *dev_id)
struct xiic_i2c *i2c = dev_id;
 
spin_lock(i2c-lock);
-   /* disable interrupts globally */
-   xiic_setreg32(i2c, XIIC_DGIER_OFFSET, 0);
 
dev_dbg(i2c-adap.dev.parent, %s entry\n, __func__);
 
xiic_process(i2c);
 
-   xiic_setreg32(i2c, XIIC_DGIER_OFFSET, XIIC_GINTR_ENABLE_MASK);
spin_unlock(i2c-lock);
 
return IRQ_HANDLED;
-- 
1.7.1

--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv2 7/9] i2c: xiic: Remove the Addressed as slave interrupt

[Shubhrajyoti Datta]

Currently there is no slave mode support in the driver
also in the isr we just ack it and do nothing.
So disable the AAS interrupt.

Signed-off-by: Shubhrajyoti Datta shubh...@xilinx.com
---
 drivers/i2c/busses/i2c-xiic.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index 66c571e..dd23a78 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -283,7 +283,7 @@ static void xiic_reinit(struct xiic_i2c *i2c)
/* Enable interrupts */
xiic_setreg32(i2c, XIIC_DGIER_OFFSET, XIIC_GINTR_ENABLE_MASK);
 
-   xiic_irq_clr_en(i2c, XIIC_INTR_AAS_MASK | XIIC_INTR_ARB_LOST_MASK);
+   xiic_irq_clr_en(i2c, XIIC_INTR_ARB_LOST_MASK);
 }
 
 static void xiic_deinit(struct xiic_i2c *i2c)
-- 
1.7.1

--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv2 2/9] i2c: xiic: move the xiic_process to thread context

[Shubhrajyoti Datta]

The xiic_process is a 154 line code that runs in isr context currently
move it to thread context. Also the name xiic_process suggests that the
intension was to run in process context.

Signed-off-by: Shubhrajyoti Datta shubh...@xilinx.com
---
 drivers/i2c/busses/i2c-xiic.c |   33 -
 1 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index 912780a..1a6e637 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -358,8 +358,9 @@ static void xiic_wakeup(struct xiic_i2c *i2c, int code)
wake_up(i2c-wait);
 }
 
-static void xiic_process(struct xiic_i2c *i2c)
+static irqreturn_t xiic_process(int irq, void *dev_id)
 {
+   struct xiic_i2c *i2c = dev_id;
u32 pend, isr, ier;
u32 clr = 0;
 
@@ -368,6 +369,7 @@ static void xiic_process(struct xiic_i2c *i2c)
 * To find which interrupts are pending; AND interrupts pending with
 * interrupts masked.
 */
+   spin_lock(i2c-lock);
isr = xiic_getreg32(i2c, XIIC_IISR_OFFSET);
ier = xiic_getreg32(i2c, XIIC_IIER_OFFSET);
pend = isr  ier;
@@ -378,11 +380,6 @@ static void xiic_process(struct xiic_i2c *i2c)
__func__, xiic_getreg8(i2c, XIIC_SR_REG_OFFSET),
i2c-tx_msg, i2c-nmsgs);
 
-   /* Do not processes a devices interrupts if the device has no
-* interrupts pending
-*/
-   if (!pend)
-   return;
 
/* Service requesting interrupt */
if ((pend  XIIC_INTR_ARB_LOST_MASK) ||
@@ -502,6 +499,8 @@ out:
dev_dbg(i2c-adap.dev.parent, %s clr: 0x%x\n, __func__, clr);
 
xiic_setreg32(i2c, XIIC_IISR_OFFSET, clr);
+   spin_unlock(i2c-lock);
+   return IRQ_HANDLED;
 }
 
 static int xiic_bus_busy(struct xiic_i2c *i2c)
@@ -602,16 +601,21 @@ static void xiic_start_send(struct xiic_i2c *i2c)
 static irqreturn_t xiic_isr(int irq, void *dev_id)
 {
struct xiic_i2c *i2c = dev_id;
-
-   spin_lock(i2c-lock);
+   u32 pend, isr, ier;
+   irqreturn_t ret = IRQ_HANDLED;
+   /* Do not processes a devices interrupts if the device has no
+* interrupts pending
+*/
 
dev_dbg(i2c-adap.dev.parent, %s entry\n, __func__);
 
-   xiic_process(i2c);
-
-   spin_unlock(i2c-lock);
+   isr = xiic_getreg32(i2c, XIIC_IISR_OFFSET);
+   ier = xiic_getreg32(i2c, XIIC_IIER_OFFSET);
+   pend = isr  ier;
+   if (pend)
+   ret = IRQ_WAKE_THREAD;
 
-   return IRQ_HANDLED;
+   return ret;
 }
 
 static void __xiic_start_xfer(struct xiic_i2c *i2c)
@@ -752,7 +756,10 @@ static int xiic_i2c_probe(struct platform_device *pdev)
spin_lock_init(i2c-lock);
init_waitqueue_head(i2c-wait);
 
-   ret = devm_request_irq(pdev-dev, irq, xiic_isr, 0, pdev-name, i2c);
+   ret = devm_request_threaded_irq(pdev-dev, irq, xiic_isr,
+   xiic_process, IRQF_ONESHOT,
+   pdev-name, i2c);
+
if (ret  0) {
dev_err(pdev-dev, Cannot claim IRQ\n);
return ret;
-- 
1.7.1

--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/9] i2c: xiic: cleanups

[Shubhrajyoti Datta]

This is a series of cleanups for the axi iic.
tested on zc702 and microblaze.



Shubhrajyoti Datta (9):
  i2c: xiic: Do not continue in case of errors in Rx
  i2c: xiic: Remove the disabling of interrupts
  i2c: xiic: move the xiic_process to thread context
  i2c: xiic: Do not reset controller before every transfer
  i2c: xiic: Remove the disabling of interrupts
  i2c: xiic: Remove busy loop while waiting for bus busy
  i2c: xiic: Add a msg in case of timeout
  i2c: xiic: Remove the Addressed as slave interrupt
  i2c: xiic: Service all interrupts in isr

 drivers/i2c/busses/i2c-xiic.c |   75 +++--
 1 files changed, 35 insertions(+), 40 deletions(-)

--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 6/9] i2c: xiic: Remove busy loop while waiting for bus busy

[Shubhrajyoti Datta]

Remove the busy loop  while waiting for bus busy.

Signed-off-by: Shubhrajyoti Datta shubh...@xilinx.com
---
 drivers/i2c/busses/i2c-xiic.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index 7b81aac..8142c2e 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -526,7 +526,7 @@ static int xiic_busy(struct xiic_i2c *i2c)
 */
err = xiic_bus_busy(i2c);
while (err  tries--) {
-   mdelay(1);
+   msleep(1);
err = xiic_bus_busy(i2c);
}
 
-- 
1.7.1

--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/9] i2c: xiic: Remove the disabling of interrupts

[Shubhrajyoti Datta]

Currently the interrupts are disabled at the start of the
isr and enabled at the end of the isr. Remove the same.

Signed-off-by: Shubhrajyoti Datta shubh...@xilinx.com
---
 drivers/i2c/busses/i2c-xiic.c |3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index cea842e..ce6b856 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -606,14 +606,11 @@ static irqreturn_t xiic_isr(int irq, void *dev_id)
struct xiic_i2c *i2c = dev_id;
 
spin_lock(i2c-lock);
-   /* disable interrupts globally */
-   xiic_setreg32(i2c, XIIC_DGIER_OFFSET, 0);
 
dev_dbg(i2c-adap.dev.parent, %s entry\n, __func__);
 
xiic_process(i2c);
 
-   xiic_setreg32(i2c, XIIC_DGIER_OFFSET, XIIC_GINTR_ENABLE_MASK);
spin_unlock(i2c-lock);
 
return IRQ_HANDLED;
-- 
1.7.1

--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 7/9] i2c: xiic: Add a msg in case of timeout

[Shubhrajyoti Datta]

Timing out is one of the serious errors.
Currently we return silently upon a timeout.
Adding an error message.

Signed-off-by: Shubhrajyoti Datta shubh...@xilinx.com
---
 drivers/i2c/busses/i2c-xiic.c |1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index 8142c2e..62f7138 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -694,6 +694,7 @@ static int xiic_xfer(struct i2c_adapter *adap, struct 
i2c_msg *msgs, int num)
i2c-tx_msg = NULL;
i2c-rx_msg = NULL;
i2c-nmsgs = 0;
+   dev_err(adap-dev.parent, Controller timed out\n);
return -ETIMEDOUT;
}
 }
-- 
1.7.1

--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/9] i2c: xiic: Do not continue in case of errors in Rx

[Shubhrajyoti Datta]

Handle error cases in the Rx path

Signed-off-by: Shubhrajyoti Datta shubh...@xilinx.com
---
 drivers/i2c/busses/i2c-xiic.c |2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index 4dda23f..cea842e 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -402,6 +402,8 @@ static void xiic_process(struct xiic_i2c *i2c)
 */
xiic_reinit(i2c);
 
+   if (i2c-rx_msg)
+   xiic_wakeup(i2c, STATE_ERROR);
if (i2c-tx_msg)
xiic_wakeup(i2c, STATE_ERROR);
 
-- 
1.7.1

--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 5/9] i2c: xiic: Remove the disabling of interrupts

[Shubhrajyoti Datta]

Currently before every transfer the interrupts are disabled.
Remove the same.

Signed-off-by: Shubhrajyoti Datta shubh...@xilinx.com
---
 drivers/i2c/busses/i2c-xiic.c |7 ---
 1 files changed, 0 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index 66040d3..7b81aac 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -666,15 +666,8 @@ static void __xiic_start_xfer(struct xiic_i2c *i2c)
 
 static void xiic_start_xfer(struct xiic_i2c *i2c)
 {
-   unsigned long flags;
-
-   spin_lock_irqsave(i2c-lock, flags);
-   /* disable interrupts globally */
-   xiic_setreg32(i2c, XIIC_DGIER_OFFSET, 0);
-   spin_unlock_irqrestore(i2c-lock, flags);
 
__xiic_start_xfer(i2c);
-   xiic_setreg32(i2c, XIIC_DGIER_OFFSET, XIIC_GINTR_ENABLE_MASK);
 }
 
 static int xiic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
-- 
1.7.1

--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/9] i2c: xiic: Do not reset controller before every transfer

[Shubhrajyoti Datta]

Currently before every transfer the controller is reinitialised.
Remove the same.

Signed-off-by: Shubhrajyoti Datta shubh...@xilinx.com
---
 drivers/i2c/busses/i2c-xiic.c |1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index e7df7c2..66040d3 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -669,7 +669,6 @@ static void xiic_start_xfer(struct xiic_i2c *i2c)
unsigned long flags;
 
spin_lock_irqsave(i2c-lock, flags);
-   xiic_reinit(i2c);
/* disable interrupts globally */
xiic_setreg32(i2c, XIIC_DGIER_OFFSET, 0);
spin_unlock_irqrestore(i2c-lock, flags);
-- 
1.7.1

--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 8/9] i2c: xiic: Remove the Addressed as slave interrupt

[Shubhrajyoti Datta]

Currently there is no slave mode. Do not enable the AAS interrupt.

Signed-off-by: Shubhrajyoti Datta shubh...@xilinx.com
---
 drivers/i2c/busses/i2c-xiic.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index 62f7138..69d937e 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -283,7 +283,7 @@ static void xiic_reinit(struct xiic_i2c *i2c)
/* Enable interrupts */
xiic_setreg32(i2c, XIIC_DGIER_OFFSET, XIIC_GINTR_ENABLE_MASK);
 
-   xiic_irq_clr_en(i2c, XIIC_INTR_AAS_MASK | XIIC_INTR_ARB_LOST_MASK);
+   xiic_irq_clr_en(i2c, XIIC_INTR_ARB_LOST_MASK);
 }
 
 static void xiic_deinit(struct xiic_i2c *i2c)
-- 
1.7.1

--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH-V2 03/12] i2c: pxa: Return I2C_RETRY when timeout in pio mode

[Shubhrajyoti Datta]

On Mon, Jun 15, 2015 at 9:19 PM, Vaibhav Hiremath
vaibhav.hirem...@linaro.org wrote:
 From: Shouming Wang wang...@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 wang...@marvell.com
 [vaibhav.hirem...@linaro.org: Updated changelog]
 Signed-off-by: Vaibhav Hiremath vaibhav.hirem...@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 023e59f..632008f 100644
 --- a/drivers/i2c/busses/i2c-pxa.c
 +++ b/drivers/i2c/busses/i2c-pxa.c
 @@ -745,8 +745,10 @@ static int i2c_pxa_do_pio_xfer(struct pxa_i2c *i2c,
 ret = i2c-msg_idx;

  out:
 -   if (timeout == 0)
 +   if (timeout == 0) {
 i2c_pxa_scream_blue_murder(i2c, timeout);
 +   ret = I2C_RETRY;

Can we use standard  return types.

 +   }

 return ret;
  }
 --
 1.9.1


 ___
 linux-arm-kernel mailing list
 linux-arm-ker...@lists.infradead.org
 http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Patch v1] i2c: imx: add runtime pm support to improve the performance

[Shubhrajyoti Datta]

On Thu, Jun 11, 2015 at 7:20 AM, Gao Pan b54...@freescale.com wrote:
 In our former i2c driver, i2c clk is enabled and disabled in
 xfer function, which contributes to power saving. However,
 the clk enable process brings a busy wait delay until the core
 is stable. As a result, the performance is sacrificed.

 To weigh the power consuption and i2c bus performance, runtime
 pm is the good solution for it. The clk is enabled when a i2c
 transfer starts, and disabled afer a specifically defined delay.

 Without the patch the test case (many eeprom reads) executes with approx:
 real 1m7.735s
 user 0m0.488s
 sys 0m20.040s

 With the patch the same test case (many eeprom reads) executes with approx:
 real 0m54.241s
 user 0m0.440s
 sys 0m5.920s

 From the test result, the patch get better performance.

 Signed-off-by: Fugang Duan b38...@freescale.com
 Signed-off-by: Gao Pan b54...@freescale.com
 ---
  drivers/i2c/busses/i2c-imx.c | 78 
 +---
  1 file changed, 66 insertions(+), 12 deletions(-)

 diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
 index 785aa67..cc4b5d6 100644
 --- a/drivers/i2c/busses/i2c-imx.c
 +++ b/drivers/i2c/busses/i2c-imx.c
 @@ -53,6 +53,7 @@
  #include linux/platform_device.h
  #include linux/sched.h
  #include linux/slab.h
 +#include linux/pm_runtime.h

  /** Defines 
 
  
 ***/
 @@ -118,6 +119,8 @@
  #define I2CR_IEN_OPCODE_0  0x0
  #define I2CR_IEN_OPCODE_1  I2CR_IEN

 +#define I2C_PM_TIMEOUT 10 /* ms */
 +
  /** Variables 
 **
  
 ***/

 @@ -520,9 +523,6 @@ static int i2c_imx_start(struct imx_i2c_struct *i2c_imx)

 i2c_imx_set_clk(i2c_imx);

 -   result = clk_prepare_enable(i2c_imx-clk);
 -   if (result)
 -   return result;
 imx_i2c_write_reg(i2c_imx-ifdr, i2c_imx, IMX_I2C_IFDR);
 /* Enable I2C controller */
 imx_i2c_write_reg(i2c_imx-hwdata-i2sr_clr_opcode, i2c_imx, 
 IMX_I2C_I2SR);
 @@ -575,7 +575,6 @@ static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx)
 /* Disable I2C controller */
 temp = i2c_imx-hwdata-i2cr_ien_opcode ^ I2CR_IEN,
 imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
 -   clk_disable_unprepare(i2c_imx-clk);
  }

  static irqreturn_t i2c_imx_isr(int irq, void *dev_id)
 @@ -583,6 +582,9 @@ static irqreturn_t i2c_imx_isr(int irq, void *dev_id)
 struct imx_i2c_struct *i2c_imx = dev_id;
 unsigned int temp;

 +   if (pm_runtime_suspended(i2c_imx-adapter.dev.parent))
 +   return IRQ_NONE;
 +

Didn't quite get this one.
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Patch v1] i2c: imx: add runtime pm support to improve the performance

[Shubhrajyoti Datta]

snip
   static irqreturn_t i2c_imx_isr(int irq, void *dev_id) @@ -583,6
  +582,9 @@ static irqreturn_t i2c_imx_isr(int irq, void *dev_id)
  struct imx_i2c_struct *i2c_imx = dev_id;
  unsigned int temp;
 
  +   if (pm_runtime_suspended(i2c_imx-adapter.dev.parent))
  +   return IRQ_NONE;
  +

 Didn't quite get this one.

 Yes, there don't need to add pm_runtime_suspended() check in isr handler. But 
 in some worse worse case,  like system is very
 busy and irq is blocked by others

you mean other irqs?

 that irq response coming is very late while i2c clock is gated off, the check 
 can avoid system hang.

 So I think it can be reasonable. How do you think ?

 Regards,
 Andy
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] i2c: tegra: remove warning dump if timeout happen in transfer

[Shubhrajyoti Datta]

On Thu, Feb 14, 2013 at 6:13 PM, Laxman Dewangan ldewan...@nvidia.com wrote:
 If timeout error occurs in the i2c transfer then it was dumping warning
 of call stack.

 Remove the warning dump as there is may be possibility that some slave
 devices are busy and not responding the i2c communication.

 Signed-off-by: Laxman Dewangan ldewan...@nvidia.com
 ---
 The patch is generated based on discussion happen between Stephena and
 Wolfram on the patch:
  i2c: add bcm2835 driver

 resending patch as Wolfram's email id has been changed.

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

 diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
 index ae2e027..36704e3 100644
 --- a/drivers/i2c/busses/i2c-tegra.c
 +++ b/drivers/i2c/busses/i2c-tegra.c
 @@ -587,7 +587,7 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev 
 *i2c_dev,
 ret = wait_for_completion_timeout(i2c_dev-msg_complete, 
 TEGRA_I2C_TIMEOUT);
 tegra_i2c_mask_irq(i2c_dev, int_mask);

 -   if (WARN_ON(ret == 0)) {
 +   if (ret == 0) {

I think WARN_ON has a unlikely.

If you could do a profiling and have the unlikely.

BTW thats not an objection to the patch though.

 dev_err(i2c_dev-dev, i2c transfer timed out\n);

 tegra_i2c_init(i2c_dev);
 --
 1.7.1.1

 --
 To unsubscribe from this list: send the line unsubscribe linux-i2c in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC] i2c: omap: Remove the OMAP_I2C_IP_VERSION_*

[Shubhrajyoti Datta]

On Mon, Nov 26, 2012 at 5:22 PM, Felipe Balbi ba...@ti.com wrote:
 Hi,

 On Mon, Nov 26, 2012 at 05:09:42PM +0530, Shubhrajyoti D wrote:
 The OMAP_I2C_IP_VERSION_1 and OMAP_I2C_IP_VERSION_2 was needed
 as on VER2 we were not reading all the 32-bits. Since now that
 we read the hi register we do not need the OMAP_I2C_IP_VERSION_*.
 Delete the same.

 The custom reset is also changed to detect VER2 based on the
 scheme.

 looks like this should become a series IMO.

 First patch would move the macros to common header, second patch would
 switch the Reset part to use those macros and third patch gets rid of
 OMAP_I2C_IP_VERSION_*

 other than that, looks very good to me.

OK will respin it accordingly.


 --
 balbi
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/7] i2c: s3c2410: Add devm_* apis and cleanup

[Shubhrajyoti Datta]

On Fri, Nov 23, 2012 at 11:29 AM, Tushar Behera
tushar.beh...@linaro.org wrote:
 This patchset cleans up the probe function of i2c-s3c2410 driver.
 These have been tested on Exynos4210 based Origen board.

 Tushar Behera (7):
   i2c: s3c2410: Remove unnecessary label err_noclk
   i2c: s3c2410: Convert to use devm_clk_get()
   i2c: s3c2410: Convert to use devm_request_mem_region()
   i2c: s3c2410: Convert to use devm_ioremap()
   i2c: s3c2410: Convert to use devm_request_irq()

You may want to consider request_and_ioremap.


   i2c: s3c2410: Move location of clk_prepare_enable() call in probe
 function
   i2c: s3c2410: Remove err_cpufreq label

  drivers/i2c/busses/i2c-s3c2410.c |   74 
 --
  1 files changed, 23 insertions(+), 51 deletions(-)

 --
 1.7.4.1

 --
 To unsubscribe from this list: send the line unsubscribe linux-i2c in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 065/493] i2c: remove use of __devexit_p

[Shubhrajyoti Datta]

On Mon, Nov 19, 2012 at 11:50 PM, Bill Pemberton wf...@virginia.edu wrote:
 CONFIG_HOTPLUG is going away as an option so __devexit_p is no longer
 needed.

 Reviewed-by: Shubhrajyoti D shubhrajy...@ti.com
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3] i2c: omap: Move the remove constraint

[Shubhrajyoti Datta]

On Fri, Nov 16, 2012 at 7:50 PM, Wolfram Sang w.s...@pengutronix.de wrote:
 On Thu, Nov 15, 2012 at 02:19:21PM +0530, Shubhrajyoti D wrote:
 Currently we just queue the transfer and release the
 qos constraints, however we do not wait for the transfer
 to complete to release the constraint. Move the remove
 constraint after the bus busy as we are sure that the
 transfers are completed by then.

 Acked-by: Jean Pihet j-pi...@ti.com
 Signed-off-by: Shubhrajyoti D shubhrajy...@ti.com

 Applied to for-next. Please let me know if it should go to for-current.

I feel for-next should be fine.


 --
 Pengutronix e.K.   | Wolfram Sang|
 Industrial Linux Solutions | http://www.pengutronix.de/  |
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2] i2c: omap: Move the remove constraint

[Shubhrajyoti Datta]

On Thu, Nov 15, 2012 at 1:46 PM, Jean Pihet jean.pi...@newoldbits.com wrote:
 Hi Shubhrajyoti,

 On Thu, Nov 15, 2012 at 8:34 AM, Shubhrajyoti D shubhrajy...@ti.com wrote:
 Currently we just queue the transfer and release the
 qos constraints, however we donot wait for the transfer
 Typo: donot
Just fixed and resent.


 to complete to release the constraint. Move the remove
 constraint after the bus busy as we are sure that the
 transfers are completed by then.

 Signed-off-by: Shubhrajyoti D shubhrajy...@ti.com
 Looks good!
 Acked-by: Jean Pihet j-pi...@ti.com

Thanks for review.


 Regards,
 Jean

--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] i2c: omap: don't save a value only needed for read-clearing

[Shubhrajyoti Datta]

On Thu, Nov 15, 2012 at 12:20 AM, Wolfram Sang w.s...@pengutronix.de wrote:

  This makes one of my code analyzers happy and makes me a part of the

 anything open source which we could all be using ? :-)

 'my' as in 'one of those I am using'. It was cppcheck which found that
 flaw. Its use for kernel code is limited, but it does find one or the
 other thing.

sparse did not complain though.
So it seems it helps to run multiple static tools:-)


 --
 Pengutronix e.K.   | Wolfram Sang|
 Industrial Linux Solutions | http://www.pengutronix.de/  |
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 0/7] i2c: omap: updates

[Shubhrajyoti Datta]

On Mon, Nov 5, 2012 at 5:53 PM, Shubhrajyoti D shubhrajy...@ti.com wrote:

 Does the followiing
 - Make the revision a 32- bit consisting of rev_lo amd rev_hi each
 of 16 bits.

 - Also use the revision register for the erratum i207.
 - Refactor the i2c_omap_init code.

 Adds a patch to remove the hardcoding sysc register. Instead
 read register ,reset and then writeback the read value.

 Also more cleanup is possible will check on that subsequently.

 Previous discussions can be found
 http://www.spinics.net/lists/linux-omap/msg81265.html


 Tested on OMAP4430sdp  ,4460 ,omap3630 ,3430 and omap2430.

 For omap2 testing the below patch was used
 [PATCH] ARM: vfp: fix save and restore when running on pre-VFPv3 and 
 CONFIG_VFPv3 set

If there are no further comments can this be considered for next.

Thanks and Regards,
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] i2c-s3c2410: Convert to devm_request_and_ioremap()

[Shubhrajyoti Datta]

On Tue, Nov 6, 2012 at 1:40 PM, Mark Brown
broo...@opensource.wolfsonmicro.com wrote:
 On Mon, Nov 05, 2012 at 05:21:03PM +0530, Shubhrajyoti Datta wrote:
 On Mon, Nov 5, 2012 at 2:03 PM, Mark Brown
 broo...@opensource.wolfsonmicro.com wrote:
  A small code saving and less error handling to worry about.

 Looks good.

 request irq could be devm_* also. Not an objection though.

 devm_ is a much worse idea for IRQs than for other resource types since
 interrupts are delivered asynchronously.  Using it safely requires that
 we do the analysis required to make sure that the hardware is totally
 idle and can't interrupt.

hmm  thats true.
thanks,
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Revert ARM: OMAP: convert I2C driver to PM QoS for MPU latency constraints

[Shubhrajyoti Datta]

On Tue, Nov 6, 2012 at 10:01 PM, Paul Walmsley p...@pwsan.com wrote:

 This reverts commit 3db11feffc1ad2ab9dea27789e6b5b3032827adc.
 This commit causes I2C timeouts to appear on several OMAP3430/3530-based
 boards:

   http://marc.info/?l=linux-arm-kernelm=135071372426971w=2
   http://marc.info/?l=linux-arm-kernelm=135067558415214w=2
   http://marc.info/?l=linux-arm-kernelm=135216013608196w=2

 and appears to have been sent for merging before one of its prerequisites
 was merged:

   http://marc.info/?l=linux-arm-kernelm=135219411617621w=2


Not a comment however was curious does merging the dependency.
make the issue go away?
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2 1/7] i2c: omap: Fix the revision register read

[Shubhrajyoti Datta]

On Mon, Nov 5, 2012 at 2:34 PM, Felipe Balbi ba...@ti.com wrote:
 Hi,

 On Mon, Nov 05, 2012 at 02:04:56PM +0530, Shubhrajyoti wrote:
  @@ -1155,7 +1187,7 @@ omap_i2c_probe(struct platform_device *pdev)
 
 dev-fifo_size = (dev-fifo_size / 2);
 
  -  if (dev-rev  OMAP_I2C_REV_ON_3630_4430)
  +  if (dev-rev  OMAP_I2C_REV_ON_3630)
 dev-b_hw = 1; /* Enable hardware fixes */
  looks like this was applicable to 4430 too, what happened ?
 No actually this can be deleted completely once the
 start - transaction - stop sequence recommendation is followed.

 yes, but we're not there just yet and this patch is changing the
 behavior

No , earlier we were truncating the register for omap4 so
OMAP_I2C_REV_ON_3630_4430 was there if we read both hi and lo for
omap4 then we donot find 3630 and 4430 value to be similar.

In this case the behavior is same as earlier also it enabled this for
lower than 3630 and
the same holds good even now.

So in essence,
Earlier  OMAP_I2C_REV_ON_3630_4430 is named to OMAP_I2C_REV_ON_3630
and omap4 rev will have a 32bit value which is greater.

 of the driver in ways which don't belong to $SUBJECT.

 --
 balbi
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] i2c-s3c2410: Convert to devm_request_and_ioremap()

[Shubhrajyoti Datta]

On Mon, Nov 5, 2012 at 2:03 PM, Mark Brown
broo...@opensource.wolfsonmicro.com wrote:
 A small code saving and less error handling to worry about.

Looks good.

request irq could be devm_* also. Not an objection though.
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 8/8] i2c: omap: cleanup the sysc write

[Shubhrajyoti Datta]

On 11/5/12, Cousson, Benoit b-cous...@ti.com wrote:
 On 11/5/2012 3:25 PM, Felipe Balbi wrote:
 Hi,

 On Mon, Nov 05, 2012 at 07:53:45PM +0530, Shubhrajyoti wrote:
 On Monday 05 November 2012 07:44 PM, Felipe Balbi wrote:
 - dev-syscstate);
 -}
 not sure if this will work. What about the first time you call reset()
 ?
 won't SYSC just contain the reset values ?
 No actually the hwmod sets the value.

 ahaaa, ok good. Let's get an Ack from Benoit, then.

 Well, in fact, I'm a little bit surprised that we still have to hack the

there was an attempt [1]

the pdata stuff may have issues with dt having to deal with more pdata [2]


 SYSC directly without using an omap_device API.

 Paul was not happy with omap device api  [3]

As far as the patch is concerned it is only getting rid of the hard coded flags
and the rev check.

 I know that we have to
 do some weird stuff for reseting that IP, but didn't we already exposed
 something to allow that?

 Regards,
 Benoit

[1]  http://www.spinics.net/lists/linux-i2c/msg06810.html
[2]  http://www.spinics.net/lists/linux-i2c/msg06937.html
[3] http://www.spinics.net/lists/linux-i2c/msg06943.html
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] i2c: s3c2410: Add fix for i2c suspend/resume

[Shubhrajyoti Datta]

On Tue, Nov 6, 2012 at 11:40 AM, Abhilash Kesavan a.kesa...@samsung.com wrote:
 The I2C driver makes a gpio_request during initialization. This request
 happens again on resume

Do you know why request and free is needed across the suspend and resume?

 and fails due to the earlier successful request.
 Modify the suspend code to free the earlier requested gpios.

 Errors on resume without this:
 [   16.02] s3c-i2c s3c2440-i2c.0: gpio [42] request failed
 [   16.02] s3c-i2c s3c2440-i2c.1: gpio [44] request failed
 [   16.02] s3c-i2c s3c2440-i2c.2: gpio [6] request failed

 Signed-off-by: Abhilash Kesavan a.kesa...@samsung.com
 ---
  drivers/i2c/busses/i2c-s3c2410.c |2 ++
  1 files changed, 2 insertions(+), 0 deletions(-)

 diff --git a/drivers/i2c/busses/i2c-s3c2410.c 
 b/drivers/i2c/busses/i2c-s3c2410.c
 index 3e0335f..9bec49e 100644
 --- a/drivers/i2c/busses/i2c-s3c2410.c
 +++ b/drivers/i2c/busses/i2c-s3c2410.c
 @@ -806,6 +806,7 @@ static int s3c24xx_i2c_parse_dt_gpio(struct s3c24xx_i2c 
 *i2c)
 dev_err(i2c-dev, invalid gpio[%d]: %d\n, idx, 
 gpio);
 goto free_gpio;
 }
 +   i2c-gpios[idx] = gpio;

 ret = gpio_request(gpio, i2c-bus);
 if (ret) {
 @@ -1125,6 +1126,7 @@ static int s3c24xx_i2c_suspend_noirq(struct device *dev)
 struct s3c24xx_i2c *i2c = platform_get_drvdata(pdev);

 i2c-suspended = 1;
 +   s3c24xx_i2c_dt_gpio_free(i2c);

 return 0;
  }
 --
 1.6.6.1

 --
 To unsubscribe from this list: send the line unsubscribe linux-i2c in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] i2c: omap: use revision check for OMAP_I2C_FLAG_APPLY_ERRATA_I207

[Shubhrajyoti Datta]

On Fri, Nov 2, 2012 at 4:37 PM, Felipe Balbi ba...@ti.com wrote:
 On Fri, Nov 02, 2012 at 04:09:45PM +0530, Shubhrajyoti D wrote:
 The errata i207 is enabled for 2430 and 3xxx. Use the revision check
 to enable the erratum instead.

 Signed-off-by: Shubhrajyoti D shubhrajy...@ti.com

 very good. I haven't read the errata but from a code standpoint, it
 looks good:

 Reviewed-by: Felipe Balbi ba...@ti.com


Also I post to this the flag may be deleted.


From: Shubhrajyoti D shubhrajy...@ti.com
Date: Fri, 2 Nov 2012 16:34:08 +0530
Subject: [PATCH] ARM: i2c: omap: Remove the i207 errata flag

The commit [i2c: omap: use revision check for OMAP_I2C_FLAG_APPLY_ERRATA_I207]
uses the revision id instead of the flag. So the flag can be safely removed.

Signed-off-by: Shubhrajyoti D shubhrajy...@ti.com
---
 arch/arm/mach-omap2/omap_hwmod_2430_data.c |3 +--
 arch/arm/mach-omap2/omap_hwmod_3xxx_data.c |9 +++--
 drivers/i2c/busses/i2c-omap.c  |3 +--
 include/linux/i2c-omap.h   |1 -
 4 files changed, 5 insertions(+), 11 deletions(-)

diff --git a/arch/arm/mach-omap2/omap_hwmod_2430_data.c
b/arch/arm/mach-omap2/omap_hwmod_2430_data.c
index c455e41..b79ccf6 100644
--- a/arch/arm/mach-omap2/omap_hwmod_2430_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_2430_data.c
@@ -76,8 +76,7 @@ static struct omap_hwmod_class i2c_class = {

 static struct omap_i2c_dev_attr i2c_dev_attr = {
.fifo_depth = 8, /* bytes */
-   .flags  = OMAP_I2C_FLAG_APPLY_ERRATA_I207 |
- OMAP_I2C_FLAG_BUS_SHIFT_2 |
+   .flags  = OMAP_I2C_FLAG_BUS_SHIFT_2 |
  OMAP_I2C_FLAG_FORCE_19200_INT_CLK,
 };

diff --git a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
index f67b7ee..943222c4 100644
--- a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
@@ -791,8 +791,7 @@ static struct omap_hwmod omap3xxx_dss_venc_hwmod = {
 /* I2C1 */
 static struct omap_i2c_dev_attr i2c1_dev_attr = {
.fifo_depth = 8, /* bytes */
-   .flags  = OMAP_I2C_FLAG_APPLY_ERRATA_I207 |
- OMAP_I2C_FLAG_RESET_REGS_POSTIDLE |
+   .flags  = OMAP_I2C_FLAG_RESET_REGS_POSTIDLE |
  OMAP_I2C_FLAG_BUS_SHIFT_2,
 };

@@ -818,8 +817,7 @@ static struct omap_hwmod omap3xxx_i2c1_hwmod = {
 /* I2C2 */
 static struct omap_i2c_dev_attr i2c2_dev_attr = {
.fifo_depth = 8, /* bytes */
-   .flags = OMAP_I2C_FLAG_APPLY_ERRATA_I207 |
-OMAP_I2C_FLAG_RESET_REGS_POSTIDLE |
+   .flags = OMAP_I2C_FLAG_RESET_REGS_POSTIDLE |
 OMAP_I2C_FLAG_BUS_SHIFT_2,
 };

@@ -845,8 +843,7 @@ static struct omap_hwmod omap3xxx_i2c2_hwmod = {
 /* I2C3 */
 static struct omap_i2c_dev_attr i2c3_dev_attr = {
.fifo_depth = 64, /* bytes */
-   .flags = OMAP_I2C_FLAG_APPLY_ERRATA_I207 |
-OMAP_I2C_FLAG_RESET_REGS_POSTIDLE |
+   .flags = OMAP_I2C_FLAG_RESET_REGS_POSTIDLE |
 OMAP_I2C_FLAG_BUS_SHIFT_2,
 };

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index dd97c14..87970fa 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -1029,8 +1029,7 @@ static const struct i2c_algorithm omap_i2c_algo = {
 #ifdef CONFIG_OF
 static struct omap_i2c_bus_platform_data omap3_pdata = {
.rev = OMAP_I2C_IP_VERSION_1,
-   .flags = OMAP_I2C_FLAG_APPLY_ERRATA_I207 |
-OMAP_I2C_FLAG_RESET_REGS_POSTIDLE |
+   .flags = OMAP_I2C_FLAG_RESET_REGS_POSTIDLE |
 OMAP_I2C_FLAG_BUS_SHIFT_2,
 };

diff --git a/include/linux/i2c-omap.h b/include/linux/i2c-omap.h
index df804ba..5c88187 100644
--- a/include/linux/i2c-omap.h
+++ b/include/linux/i2c-omap.h
@@ -21,7 +21,6 @@
 #define OMAP_I2C_FLAG_SIMPLE_CLOCK BIT(1)
 #define OMAP_I2C_FLAG_16BIT_DATA_REG   BIT(2)
 #define OMAP_I2C_FLAG_RESET_REGS_POSTIDLE  BIT(3)
-#define OMAP_I2C_FLAG_APPLY_ERRATA_I207BIT(4)
 #define OMAP_I2C_FLAG_ALWAYS_ARMXOR_CLKBIT(5)
 #define OMAP_I2C_FLAG_FORCE_19200_INT_CLK  BIT(6)
 /* how the CPU address bus must be translated for I2C unit access */
-- 
1.7.5.4
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] i2c: omap: Fix the revision register read

[Shubhrajyoti Datta]

On Fri, Nov 2, 2012 at 4:36 PM, Felipe Balbi ba...@ti.com wrote:
 Hi,

 On Fri, Nov 02, 2012 at 04:09:44PM +0530, Shubhrajyoti D wrote:
 The revision register on OMAP4 is a 16-bit lo and a 16-bit
 hi. Currently the driver reads only the lower 8-bits.
 Fix the same by preventing the truncating of the rev register
 for OMAP4.

 Also use the scheme bit ie bit-14 of the hi register to know if it
 is OMAP_I2C_IP_VERSION_2.

 On platforms previous to OMAP4 the offset 0x04 is IE register whose
 bit-14 reset value is 0, the code uses the same to its advantage.

 Also since the omap_i2c_read_reg uses reg_map_ip_* a raw_readw is done
 to fetch the revision register.

 The dev-regs is populated after reading the rev_hi. A NULL check
 has been added in the resume handler to prevent the access before
 the setting of the regs.

 tested on which platforms ?

omap4430 , 4460 and omap3.

 Signed-off-by: Shubhrajyoti D shubhrajy...@ti.com
 ---
  drivers/i2c/busses/i2c-omap.c |   59 
 -
  1 files changed, 46 insertions(+), 13 deletions(-)

 diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
 index db31eae..d8e7709 100644
 --- a/drivers/i2c/busses/i2c-omap.c
 +++ b/drivers/i2c/busses/i2c-omap.c
 @@ -49,9 +49,10 @@
  #define OMAP_I2C_OMAP1_REV_2 0x20

  /* I2C controller revisions present on specific hardware */
 -#define OMAP_I2C_REV_ON_2430 0x36
 -#define OMAP_I2C_REV_ON_3430_35300x3C
 -#define OMAP_I2C_REV_ON_3630_44300x40
 +#define OMAP_I2C_REV_ON_2430 0x0036

 are you sure this are the contents on 2430 ? Have you actually ran this
 code on that platform ?

I did not run on 2430. Will try to get a platform and run.

However the current code already has and uses the same value so
 I feel it should be fine.:-)



 +#define OMAP_I2C_REV_ON_3430_35300x003C
 +#define OMAP_I2C_REV_ON_3630 0x0040

 likewise for these two.

I verified that the 3630 returns 0x40 on my beaglexM.


 +#define OMAP_I2C_REV_ON_4430_PLUS0x5042

 what about 4460, 4470, 3530, etc etc etc ?

 @@ -490,7 +491,7 @@ static void omap_i2c_resize_fifo(struct omap_i2c_dev 
 *dev, u8 size, bool is_rx)

   omap_i2c_write_reg(dev, OMAP_I2C_BUF_REG, buf);

 - if (dev-rev  OMAP_I2C_REV_ON_3630_4430)
 + if (dev-rev  OMAP_I2C_REV_ON_3630)
   dev-b_hw = 1; /* Enable hardware fixes */

   /* calculate wakeup latency constraint for MPU */
 @@ -1052,6 +1053,14 @@ static const struct of_device_id omap_i2c_of_match[] 
 = {
  MODULE_DEVICE_TABLE(of, omap_i2c_of_match);
  #endif

 +#define OMAP_I2C_SCHEME(rev) (rev  0xc000)  14

 you miss () there to make sure no other operations will take higher
 precedence when using this macro.

Indeed. Thanks.

 @@ -1130,7 +1136,31 @@ omap_i2c_probe(struct platform_device *pdev)
   if (IS_ERR_VALUE(r))
   goto err_free_mem;

 - dev-rev = omap_i2c_read_reg(dev, OMAP_I2C_REV_REG)  0xff;
 + /*
 +  * Read the Rev hi bit-[15:14] ie scheme this is 1 indicates ver2.
 +  * On omap3 Offset 4 is IE Reg the bit [15:14] is XDR_IE which is 0
 +  * at reset. Also since the omap_i2c_read_reg uses reg_map_ip_* a
 +  * raw_readw is done.
 +  */
 + rev = __raw_readw(dev-base + 0x04);
 +
 + switch (OMAP_I2C_SCHEME(rev)) {
 + case 0:

 define macros for these two cases.

OK

 + dev-regs = (u8 *)reg_map_ip_v1;
 + dev-rev = omap_i2c_read_reg(dev, OMAP_I2C_REV_REG)  0xff;
 + minor = OMAP_I2C_REV_SCHEME_0_MAJOR(dev-rev);
 + major = OMAP_I2C_REV_SCHEME_0_MAJOR(dev-rev);
 + break;

 wrong indentation.

Yes will fix.

 --
 balbi
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC] i2c: omap: Fix the revision register read

[Shubhrajyoti Datta]

On Wed, Oct 31, 2012 at 2:29 PM, Shubhrajyoti D shubhrajy...@ti.com wrote:
 The revision register on OMAP4 is a 16-bit lo and a 16-bit
 hi. Currently the driver reads only the lower 8-bits.
 Fix the same by preventing the truncating of the rev register
 for OMAP4.

 Also use the scheme bit ie bit-14 of the hi register to know if it
 is OMAP_I2C_IP_VERSION_2.

 On platforms previous to OMAP4 the offset 0x04 is IE register whose
 bit-14 reset value is 0, the code uses the same to its advantage.

 The dev-regs is populated after reading the rev_hi. A NULL check
 has been added in the resume handler to prevent the access before
 the setting of the regs.

tested on omap4sdp, omap3630 based beagle , omap3430sdp.
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] i2c: omap: re-factor omap_i2c_init function

[Shubhrajyoti Datta]

On Thu, Oct 25, 2012 at 12:06 PM, Felipe Balbi ba...@ti.com wrote:
 Hi,

 On Thu, Oct 25, 2012 at 12:06:51PM +0530, Shubhrajyoti D wrote:
 re-factor omap_i2c_init() so that we can re-use it for resume.
 While at it also remove the bufstate variable as we write it
 in omap_i2c_resize_fifo for every transfer.

 Signed-off-by: Shubhrajyoti D shubhrajy...@ti.com
 ---
 v2 - add the iestate 0 check back.
- Remove a stray change.
 - Applies on top of Felipe's patches.
 http://www.spinics.net/lists/linux-omap/msg79995.html


 Tested with Terro sys fix + Felipe's stop sched_clock() during suspend
 on omap3636 beagle both idle and suspend.

 Functional testing on omap4sdp.

  drivers/i2c/busses/i2c-omap.c |   71 
 ++--
  1 files changed, 32 insertions(+), 39 deletions(-)

 diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
 index 5e5cefb..3d400b1 100644
 --- a/drivers/i2c/busses/i2c-omap.c
 +++ b/drivers/i2c/busses/i2c-omap.c
 @@ -209,7 +209,6 @@ struct omap_i2c_dev {
   u16 pscstate;
   u16 scllstate;
   u16 sclhstate;
 - u16 bufstate;
   u16 syscstate;
   u16 westate;
   u16 errata;
 @@ -285,9 +284,31 @@ static inline u16 omap_i2c_read_reg(struct omap_i2c_dev 
 *i2c_dev, int reg)
   }
  }

 +static void __omap_i2c_init(struct omap_i2c_dev *dev)
 +{
 +
 + omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
 + /* Setup clock prescaler to obtain approx 12MHz I2C module clock: */
 + omap_i2c_write_reg(dev, OMAP_I2C_PSC_REG, dev-pscstate);
 +
 + /* SCL low and high time values */
 + omap_i2c_write_reg(dev, OMAP_I2C_SCLL_REG, dev-scllstate);
 + omap_i2c_write_reg(dev, OMAP_I2C_SCLH_REG, dev-sclhstate);
 + if (dev-rev = OMAP_I2C_REV_ON_3430_3530)
 + omap_i2c_write_reg(dev, OMAP_I2C_WE_REG, dev-westate);
 + /* Take the I2C module out of reset: */
 + omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN);

 a few blank lines in this function wouldn't hurt and they would help
 with readability.

Will add .


 + /*
 +  * Don't write to this register if the IE state is 0 as it can
 +  * cause deadlock.
 +  */
 + if (dev-iestate)
 + omap_i2c_write_reg(dev, OMAP_I2C_IE_REG, dev-iestate);
 +}
 +
  static int omap_i2c_init(struct omap_i2c_dev *dev)
  {
 - u16 psc = 0, scll = 0, sclh = 0, buf = 0;
 + u16 psc = 0, scll = 0, sclh = 0;
   u16 fsscll = 0, fssclh = 0, hsscll = 0, hssclh = 0;
   unsigned long fclk_rate = 1200;
   unsigned long timeout;
 @@ -337,11 +358,8 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
* REVISIT: Some wkup sources might not be needed.
*/
   dev-westate = OMAP_I2C_WE_ALL;
 - omap_i2c_write_reg(dev, OMAP_I2C_WE_REG,
 - dev-westate);

 remove the comment too since now that's done by some other function ?

   }
   }
 - omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);

   if (dev-flags  OMAP_I2C_FLAG_ALWAYS_ARMXOR_CLK) {
   /*
 @@ -426,28 +444,18 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
   sclh = fclk_rate / (dev-speed * 2) - 7 + psc;
   }

 - /* Setup clock prescaler to obtain approx 12MHz I2C module clock: */
 - omap_i2c_write_reg(dev, OMAP_I2C_PSC_REG, psc);
 -
 - /* SCL low and high time values */
 - omap_i2c_write_reg(dev, OMAP_I2C_SCLL_REG, scll);
 - omap_i2c_write_reg(dev, OMAP_I2C_SCLH_REG, sclh);
 -
 - /* Take the I2C module out of reset: */
 - omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN);
 -
   /* Enable interrupts */
   dev-iestate = (OMAP_I2C_IE_XRDY | OMAP_I2C_IE_RRDY |
   OMAP_I2C_IE_NACK | OMAP_I2C_IE_AL)  |
   ((dev-fifo_size) ? (OMAP_I2C_IE_RDR |
   OMAP_I2C_IE_XDR) : 0);
 - omap_i2c_write_reg(dev, OMAP_I2C_IE_REG, dev-iestate);
 - if (dev-flags  OMAP_I2C_FLAG_RESET_REGS_POSTIDLE) {
 - dev-pscstate = psc;
 - dev-scllstate = scll;
 - dev-sclhstate = sclh;
 - dev-bufstate = buf;
 - }
 +
 + dev-pscstate = psc;
 + dev-scllstate = scll;
 + dev-sclhstate = sclh;
 +
 + __omap_i2c_init(dev);
 +
   return 0;
  }

 @@ -1268,23 +1276,8 @@ static int omap_i2c_runtime_resume(struct device *dev)
  {
   struct omap_i2c_dev *_dev = dev_get_drvdata(dev);

 - if (_dev-flags  OMAP_I2C_FLAG_RESET_REGS_POSTIDLE) {
 - omap_i2c_write_reg(_dev, OMAP_I2C_CON_REG, 0);
 - omap_i2c_write_reg(_dev, OMAP_I2C_PSC_REG, _dev-pscstate);
 - omap_i2c_write_reg(_dev, OMAP_I2C_SCLL_REG, _dev-scllstate);
 - omap_i2c_write_reg(_dev, 

Re: [PATCH] i2c: omap: ensure writes to dev-buf_len are ordered

[Shubhrajyoti Datta]

On Thu, Oct 25, 2012 at 2:30 PM, Felipe Balbi ba...@ti.com wrote:
 if we allow compiler reorder our writes, we could
 fall into a situation where dev-buf_len is reset
 for no apparent reason.

 This bug was found with a simple script which would
 transfer data to an i2c client from 1 to 1024 bytes
 (a simple for loop), when we got to transfer sizes
 bigger than the fifo size, dev-buf_len was reset
 to zero before we had an oportunity to handle XDR
 Interrupt. Because dev-buf_len was zero, we entered
 omap_i2c_transmit_data() to transfer zero bytes,
 which would mean we would just silently exit
 omap_i2c_transmit_data() without actually writing
 anything to DATA register. That would cause XDR
 IRQ to trigger forever and we would never transfer
 the remaining bytes.

 After adding the memory barrier, we also drop resetting
 dev-buf_len to zero in omap_i2c_xfer_msg() because
 both omap_i2c_transmit_data() and omap_i2c_receive_data()
 will act until dev-buf_len reaches zero, rendering the
 other write in omap_i2c_xfer_msg() redundant.

 This patch has been tested with pandaboard for a few
 iterations of the script mentioned above.

looks good to me
Acked-by: Shubhrajyoti D shubhrajy...@ti.com


 Signed-off-by: Felipe Balbi ba...@ti.com
 ---

 This bug has been there forever, but it's quite annoying.
 I think it deserves being pushed upstream during this -rc
 cycle, but if Wolfram decides to wait until v3.8, I don't
 mind.

  drivers/i2c/busses/i2c-omap.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

 diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
 index db31eae..1ec4e6e 100644
 --- a/drivers/i2c/busses/i2c-omap.c
 +++ b/drivers/i2c/busses/i2c-omap.c
 @@ -521,6 +521,7 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
 /* REVISIT: Could the STB bit of I2C_CON be used with probing? */
 dev-buf = msg-buf;
 dev-buf_len = msg-len;
 +   wmb();

 omap_i2c_write_reg(dev, OMAP_I2C_CNT_REG, dev-buf_len);

 @@ -579,7 +580,6 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
  */
 timeout = wait_for_completion_timeout(dev-cmd_complete,
 OMAP_I2C_TIMEOUT);
 -   dev-buf_len = 0;
 if (timeout == 0) {
 dev_err(dev-dev, controller timed out\n);
 omap_i2c_init(dev);
 --
 1.8.0

 --
 To unsubscribe from this list: send the line unsubscribe linux-i2c in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] i2c: omap: re-factor omap_i2c_init function

[Shubhrajyoti Datta]

On Thu, Oct 25, 2012 at 12:06 PM, Felipe Balbi ba...@ti.com wrote:

[...]
 +  * Don't write to this register if the IE state is 0 as it can
 +  * cause deadlock.
 +  */
 + if (dev-iestate)
 + omap_i2c_write_reg(dev, OMAP_I2C_IE_REG, dev-iestate);
 +}
 +
  static int omap_i2c_init(struct omap_i2c_dev *dev)
  {
 - u16 psc = 0, scll = 0, sclh = 0, buf = 0;
 + u16 psc = 0, scll = 0, sclh = 0;
   u16 fsscll = 0, fssclh = 0, hsscll = 0, hssclh = 0;
   unsigned long fclk_rate = 1200;
   unsigned long timeout;
 @@ -337,11 +358,8 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
* REVISIT: Some wkup sources might not be needed.
*/
   dev-westate = OMAP_I2C_WE_ALL;
 - omap_i2c_write_reg(dev, OMAP_I2C_WE_REG,
 - dev-westate);

 remove the comment too since now that's done by some other function ?

The comment is applicable to the OMAP_I2C_WE_ALL value.
So I thought it could be kept.

dont feel strongly though.



   }
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/8] i2c: omap: reorder exit path of omap_i2c_xfer_msg()

[Shubhrajyoti Datta]

On Mon, Oct 22, 2012 at 3:16 PM, Felipe Balbi ba...@ti.com wrote:
 just a cleanup patch trying to make exit path
 more straightforward. No changes otherwise.

 Signed-off-by: Felipe Balbi ba...@ti.com
 ---
  drivers/i2c/busses/i2c-omap.c | 26 +-
  1 file changed, 17 insertions(+), 9 deletions(-)

 diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
 index c07d9c4..bea0277 100644
 --- a/drivers/i2c/busses/i2c-omap.c
 +++ b/drivers/i2c/busses/i2c-omap.c
 @@ -505,6 +505,7 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
  {
 struct omap_i2c_dev *dev = i2c_get_adapdata(adap);
 unsigned long timeout;
 +   int ret;
 u16 w;

 dev_dbg(dev-dev, addr: 0x%04x, len: %d, flags: 0x%x, stop: %d\n,
 @@ -582,31 +583,38 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
 dev-buf_len = 0;
 if (timeout == 0) {
 dev_err(dev-dev, controller timed out\n);
 -   omap_i2c_init(dev);
 -   return -ETIMEDOUT;
 +   ret = -ETIMEDOUT;
 +   goto err_i2c_init;
 }

 -   if (likely(!dev-cmd_err))
 -   return 0;
 -
 /* We have an error */
 if (dev-cmd_err  (OMAP_I2C_STAT_AL | OMAP_I2C_STAT_ROVR |
 OMAP_I2C_STAT_XUDF)) {
 -   omap_i2c_init(dev);
 -   return -EIO;
 +   ret = -EIO;
 +   goto err_i2c_init;
 }

 if (dev-cmd_err  OMAP_I2C_STAT_NACK) {
 if (msg-flags  I2C_M_IGNORE_NAK)
 return 0;
 +
 if (stop) {
 w = omap_i2c_read_reg(dev, OMAP_I2C_CON_REG);
 w |= OMAP_I2C_CON_STP;
 omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, w);
 }
 -   return -EREMOTEIO;
 +
 +   ret = -EREMOTEIO;
 +   goto err;

This adds reset to nack may be that can be removed.


 }
 -   return -EIO;
 +
 +   return 0;
 +
 +err_i2c_init:
 +   omap_i2c_init(dev);
 +
 +err:
 +   return ret;
  }


 --
 1.8.0.rc0

 --
 To unsubscribe from this list: send the line unsubscribe linux-i2c in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] i2c: omap: re-factor omap_i2c_init function

[Shubhrajyoti Datta]

On Tue, Oct 23, 2012 at 11:27 PM, Felipe Balbi ba...@ti.com wrote:
 Hi,

 On Tue, Oct 23, 2012 at 11:26:15PM +0530, Shubhrajyoti Datta wrote:
  @@ -1268,23 +1271,8 @@ static int omap_i2c_runtime_resume(struct device 
  *dev)
   {
struct omap_i2c_dev *_dev = dev_get_drvdata(dev);
 
  - if (_dev-flags  OMAP_I2C_FLAG_RESET_REGS_POSTIDLE) {
  - omap_i2c_write_reg(_dev, OMAP_I2C_CON_REG, 0);
  - omap_i2c_write_reg(_dev, OMAP_I2C_PSC_REG, _dev-pscstate);
  - omap_i2c_write_reg(_dev, OMAP_I2C_SCLL_REG, 
  _dev-scllstate);
  - omap_i2c_write_reg(_dev, OMAP_I2C_SCLH_REG, 
  _dev-sclhstate);
  - omap_i2c_write_reg(_dev, OMAP_I2C_BUF_REG, _dev-bufstate);
  - omap_i2c_write_reg(_dev, OMAP_I2C_SYSC_REG, 
  _dev-syscstate);
  - omap_i2c_write_reg(_dev, OMAP_I2C_WE_REG, _dev-westate);
  - omap_i2c_write_reg(_dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN);
  - }
  -
  - /*
  -  * Don't write to this register if the IE state is 0 as it can
  -  * cause deadlock.
  -  */
  - if (_dev-iestate)
  - omap_i2c_write_reg(_dev, OMAP_I2C_IE_REG, _dev-iestate);
 
  this part is not on __omap_i2c_init() so you're potentially causing a
  regression here.

 iestate is set at init so cannot be zero. so the check was removed.

Hmm thinking a little more, there is a case that I missed the resume
handler may get called before
the omap_i2c_init will  restore the check and send another version.
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] i2c: omap: re-factor omap_i2c_init function

[Shubhrajyoti Datta]

On Tue, Oct 23, 2012 at 10:48 PM, Felipe Balbi ba...@ti.com wrote:
 Hi,

 On Tue, Oct 23, 2012 at 08:57:19PM +0530, Shubhrajyoti D wrote:
 re-factor omap_i2c_init() so that we can re-use it for resume.
 While at it also remove the bufstate variable as we write it
 in omap_i2c_resize_fifo for every transfer.

 Signed-off-by: Shubhrajyoti D shubhrajy...@ti.com
 ---
 Applies on Felipe's series
 http://www.spinics.net/lists/linux-omap/msg79995.html

 Tested with Terro sys fix + Felipe's stop sched_clock() during suspend
 on omap3636 beagle both idle and suspend.

 Functional testing on omap4sdp.

  drivers/i2c/busses/i2c-omap.c |   68 
 +
  1 files changed, 28 insertions(+), 40 deletions(-)

 diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
 index 5e5cefb..338cee7 100644
 --- a/drivers/i2c/busses/i2c-omap.c
 +++ b/drivers/i2c/busses/i2c-omap.c
 @@ -209,7 +209,6 @@ struct omap_i2c_dev {
   u16 pscstate;
   u16 scllstate;
   u16 sclhstate;
 - u16 bufstate;
   u16 syscstate;
   u16 westate;
   u16 errata;
 @@ -285,9 +284,26 @@ static inline u16 omap_i2c_read_reg(struct omap_i2c_dev 
 *i2c_dev, int reg)
   }
  }

 +static void __omap_i2c_init(struct omap_i2c_dev *dev)
 +{
 +
 + omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
 + /* Setup clock prescaler to obtain approx 12MHz I2C module clock: */
 + omap_i2c_write_reg(dev, OMAP_I2C_PSC_REG, dev-pscstate);
 +
 + /* SCL low and high time values */
 + omap_i2c_write_reg(dev, OMAP_I2C_SCLL_REG, dev-scllstate);
 + omap_i2c_write_reg(dev, OMAP_I2C_SCLH_REG, dev-sclhstate);
 + if (dev-rev = OMAP_I2C_REV_ON_3430_3530)
 + omap_i2c_write_reg(dev, OMAP_I2C_WE_REG, dev-westate);
 + /* Take the I2C module out of reset: */
 + omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN);
 + omap_i2c_write_reg(dev, OMAP_I2C_IE_REG, dev-iestate);
 +
 +}
  static int omap_i2c_init(struct omap_i2c_dev *dev)
  {
 - u16 psc = 0, scll = 0, sclh = 0, buf = 0;
 + u16 psc = 0, scll = 0, sclh = 0;
   u16 fsscll = 0, fssclh = 0, hsscll = 0, hssclh = 0;
   unsigned long fclk_rate = 1200;
   unsigned long timeout;
 @@ -337,11 +353,8 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
* REVISIT: Some wkup sources might not be needed.
*/
   dev-westate = OMAP_I2C_WE_ALL;
 - omap_i2c_write_reg(dev, OMAP_I2C_WE_REG,
 - dev-westate);
   }
   }
 - omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);

   if (dev-flags  OMAP_I2C_FLAG_ALWAYS_ARMXOR_CLK) {
   /*
 @@ -426,28 +439,18 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
   sclh = fclk_rate / (dev-speed * 2) - 7 + psc;
   }

 - /* Setup clock prescaler to obtain approx 12MHz I2C module clock: */
 - omap_i2c_write_reg(dev, OMAP_I2C_PSC_REG, psc);
 -
 - /* SCL low and high time values */
 - omap_i2c_write_reg(dev, OMAP_I2C_SCLL_REG, scll);
 - omap_i2c_write_reg(dev, OMAP_I2C_SCLH_REG, sclh);
 -
 - /* Take the I2C module out of reset: */
 - omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN);
 -
   /* Enable interrupts */
   dev-iestate = (OMAP_I2C_IE_XRDY | OMAP_I2C_IE_RRDY |
   OMAP_I2C_IE_NACK | OMAP_I2C_IE_AL)  |
   ((dev-fifo_size) ? (OMAP_I2C_IE_RDR |
   OMAP_I2C_IE_XDR) : 0);
 - omap_i2c_write_reg(dev, OMAP_I2C_IE_REG, dev-iestate);
 - if (dev-flags  OMAP_I2C_FLAG_RESET_REGS_POSTIDLE) {
 - dev-pscstate = psc;
 - dev-scllstate = scll;
 - dev-sclhstate = sclh;
 - dev-bufstate = buf;
 - }
 +
 + dev-pscstate = psc;
 + dev-scllstate = scll;
 + dev-sclhstate = sclh;
 +
 + __omap_i2c_init(dev);
 +
   return 0;
  }

 @@ -1136,7 +1139,7 @@ omap_i2c_probe(struct platform_device *pdev)
   if (IS_ERR_VALUE(r))
   goto err_free_mem;

 - dev-rev = omap_i2c_read_reg(dev, OMAP_I2C_REV_REG)  0xff;
 + dev-rev = omap_i2c_read_reg(dev, OMAP_I2C_REV_REG)   0xff;

 trailing change. not part of $SUBJECT

my mistake will remove.


   dev-errata = 0;

 @@ -1268,23 +1271,8 @@ static int omap_i2c_runtime_resume(struct device *dev)
  {
   struct omap_i2c_dev *_dev = dev_get_drvdata(dev);

 - if (_dev-flags  OMAP_I2C_FLAG_RESET_REGS_POSTIDLE) {
 - omap_i2c_write_reg(_dev, OMAP_I2C_CON_REG, 0);
 - omap_i2c_write_reg(_dev, OMAP_I2C_PSC_REG, _dev-pscstate);
 - omap_i2c_write_reg(_dev, OMAP_I2C_SCLL_REG, _dev-scllstate);
 - omap_i2c_write_reg(_dev, OMAP_I2C_SCLH_REG, _dev-sclhstate);
 - 

Re: [PATCH] i2c: omap: re-factor omap_i2c_init function

[Shubhrajyoti Datta]

On Tue, Oct 23, 2012 at 11:27 PM, Felipe Balbi ba...@ti.com wrote:
 Hi,

 On Tue, Oct 23, 2012 at 11:26:15PM +0530, Shubhrajyoti Datta wrote:
  @@ -1268,23 +1271,8 @@ static int omap_i2c_runtime_resume(struct device 
  *dev)
   {
struct omap_i2c_dev *_dev = dev_get_drvdata(dev);
 
  - if (_dev-flags  OMAP_I2C_FLAG_RESET_REGS_POSTIDLE) {
  - omap_i2c_write_reg(_dev, OMAP_I2C_CON_REG, 0);
  - omap_i2c_write_reg(_dev, OMAP_I2C_PSC_REG, _dev-pscstate);
  - omap_i2c_write_reg(_dev, OMAP_I2C_SCLL_REG, 
  _dev-scllstate);
  - omap_i2c_write_reg(_dev, OMAP_I2C_SCLH_REG, 
  _dev-sclhstate);
  - omap_i2c_write_reg(_dev, OMAP_I2C_BUF_REG, _dev-bufstate);
  - omap_i2c_write_reg(_dev, OMAP_I2C_SYSC_REG, 
  _dev-syscstate);
  - omap_i2c_write_reg(_dev, OMAP_I2C_WE_REG, _dev-westate);
  - omap_i2c_write_reg(_dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN);
  - }
  -
  - /*
  -  * Don't write to this register if the IE state is 0 as it can
  -  * cause deadlock.
  -  */
  - if (_dev-iestate)
  - omap_i2c_write_reg(_dev, OMAP_I2C_IE_REG, _dev-iestate);
 
  this part is not on __omap_i2c_init() so you're potentially causing a
  regression here.

 iestate is set at init so cannot be zero. so the check was removed.

 so you never read it back ? Then you need to add a note for that in
 changelog, since this is a behavior change ;-)

Indeed will do.


 --
 balbi
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] i2c: omap: adopt pinctrl support

[Shubhrajyoti Datta]

On Tue, Oct 16, 2012 at 8:53 PM, Sebastien Guiriec s-guir...@ti.com wrote:
 Some GPIO expanders need some early pin control muxing. Due to
 legacy boards sometimes the driver uses subsys_initcall instead of
 module_init. This patch takes advantage of defer probe feature
 and pin control in order to wait until pin control probing before
 GPIO driver probing. It has been tested on OMAP5 board with TCA6424
 driver.

 log:

 [0.482299] omap_i2c i2c.15: could not find pctldev for node /ocp/pinmux@4a00
 2840/pinmux_i2c5_pins, deferring probe
 [0.482330] platform i2c.15: Driver omap_i2c requests probe deferral
 [0.484466] Advanced Linux Sound Architecture Driver Initialized.

 [4.746917] omap_i2c i2c.15: bus 4 rev2.4.0 at 100 kHz
 [4.755279] gpiochip_find_base: found new base at 477
 [4.761169] gpiochip_add: registered GPIOs 477 to 500 on device: tca6424a

Thanks,

Acked-by: Shubhrajyoti D shubhrajy...@ti.com
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] i2c: omap: revert i2c: omap: switch to threaded IRQ support

[Shubhrajyoti Datta]

On Mon, Oct 15, 2012 at 7:21 AM, Paul Walmsley p...@pwsan.com wrote:

 Commit 3b2f8f82dad7d1f79cdc8fc05bd1c94baf109bde (i2c: omap: switch to
 threaded IRQ support) causes communication with I2C devices to fail
 after system suspend/resume on all OMAP3 devices:

Could you tell me which  omap3 platform

On Beagle Xm
after
mount /dev/mmcblk  /mmcfs


# mount /dev/mmcblk0p2 /mmcfs/
[  412.480041] kjournald starting.  Commit interval 5 seconds
[  412.490020] EXT3-fs (mmcblk0p2): using internal journal
[  412.495605] EXT3-fs (mmcblk0p2): mounted filesystem with ordered data mode
#


# cd /mmcfs/
#
#
# ls
bin   omap3_usb_prcm.sh usb_prcm.sh
dev   omap3_usbhs_off.shusb_uhh_show.sh
etc   omap3_usbhs_on.sh usb_uhh_tll.sh
init  proc  usbhs_clk_disable.sh
lib   readmem.dat   usbhs_clk_enable.sh
lost+foundroot  usbhs_set_sm.sh
mnt   sbin  usbhs_show.sh
modules   sys   usr
msc   tmp   var
omap3_ehcidump.sh usb_omap3.sh
#
#
# echo mem  /sys/power/state
[  464.785461] PM: Syncing filesystems ... done.
[  464.791442] PM: Preparing system for mem sleep
[  464.798034] Freezing user space processes ... (elapsed 0.02 seconds) done.
[  464.827301] Freezing remaining freezable tasks ... (elapsed 0.02
seconds) done.
[  464.858703] PM: Entering mem sleep
[  464.862304] Suspending console(s) (use no_console_suspend to debug)
[  464.994415] PM: suspend of devices complete after 121.002 msecs
[  464.998107] PM: late suspend of devices complete after 3.662 msecs
[  465.003173] PM: noirq suspend of devices complete after 5.004 msecs
[  465.003173] Disabling non-boot CPUs ...
[  466.225585] Successfully put all powerdomains to target state
[  466.228942] PM: noirq resume of devices complete after 3.051 msecs
[  466.232421] PM: early resume of devices complete after 2.349 msecs
[  467.492645] PM: resume of devices complete after 1260.131 msecs
[  467.546936] PM: Finishing wakeup.
[  467.550415] Restarting tasks ... done.
#
#
# cat /debug/pm_debug/count | grep per_pwrdm
per_pwrdm (ON),OFF:7,RET:0,INA:0,ON:8,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0
per_clkdm-per_pwrdm (17)
# echo mem  /sys/power/state
[ 1492.225311] PM: Syncing filesystems ... done.
[ 1492.232177] PM: Preparing system for mem sleep
[ 1492.238830] Freezing user space processes ... (elapsed 0.02 seconds) done.
[ 1492.268188] Freezing remaining freezable tasks ... (elapsed 0.02
seconds) done.
[ 1492.299804] PM: Entering mem sleep
[ 1492.303375] Suspending console(s) (use no_console_suspend to debug)
[ 1492.435333] PM: suspend of devices complete after 120.880 msecs
[ 1492.439025] PM: late suspend of devices complete after 3.692 msecs
[ 1492.444091] PM: noirq suspend of devices complete after 5.004 msecs
[ 1492.444091] Disabling non-boot CPUs ...
[ 1493.745544] Successfully put all powerdomains to target state
[ 1493.748901] PM: noirq resume of devices complete after 3.051 msecs
[ 1493.752319] PM: early resume of devices complete after 2.319 msecs
[ 1494.794067] PM: resume of devices complete after 1041.625 msecs
[ 1494.848388] PM: Finishing wakeup.
[ 1494.851867] Restarting tasks ... done.
#
#
# cat /debug/pm_debug/count | grep per_pwrdm
per_pwrdm (ON),OFF:8,RET:0,INA:0,ON:9,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0
per_clkdm-per_pwrdm (17)
#

Anyways will retry with fs on mmc.
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] ARM: OMAP: i2c: fix interrupt flood during resume

[Shubhrajyoti Datta]

On Mon, Oct 15, 2012 at 2:46 PM, Kalle Jokiniemi
kalle.jokini...@jollamobile.com wrote:
 ma, 2012-10-15 kello 09:21 +0300, Kalle Jokiniemi kirjoitti:
 Hi,

 pe, 2012-10-12 kello 14:46 +, Strashko, Grygorii kirjoitti:
  Hi Kevin,
 
  yep, [1] is the same fix - thanks.
 
  Hi Kalle,
 
  i've applied these changes and PM runtime fix on top of 
  git://git.kernel.org/pub/scm/linux/kernel/git/tmlind/linux-omap.git 
  (omap2plus_defconfig)
  Could you check it with your use case, pls? (just to be sure that idea is 
  right)

 Odd, it's not working. I'll add some debug prints to see what happens
 there.

 Well, seems after enabling irq 23 in the resume_noirq, someone does
 i2c_xfer and there is consequent flood of i2c_xfers and interrupts.
If there is continuous xfers, you could enable debug LL and see who is
queuing the
transfers.


 Not sure now how these IRQ numbers get mapped these days, my debug print
 says it's irq number 72 (UART1 from TRM) that is flooding (although it's
 printed from the i2c-omap isr function, so it's still I2C bus irq...).

Can you do a cat /proc/interrupts



 The irq enabling (in resume_noirq) is still slightly progressing after
 the flooding starts, but my watchdog kicks in before we get to the
 finish.

 Attached my debug prints patch and log. I used also no_console_suspend
 boot option.

 - Kalle

--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] i2c: omap: Do not enable the irq always

[Shubhrajyoti Datta]

On Fri, Oct 12, 2012 at 7:56 PM, Kevin Hilman
khil...@deeprootsystems.com wrote:
 +Kalle, Grygorii, Huzefa

 Shubhrajyoti D shubhrajy...@ti.com writes:

 Enable the irq in the transfer and disable it after the
 transfer is done.

 Signed-off-by: Shubhrajyoti D shubhrajy...@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 majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] ARM: OMAP: i2c: fix interrupt flood during resume

[Shubhrajyoti Datta]

On Fri, Oct 12, 2012 at 10:13 PM, Kevin Hilman
khil...@deeprootsystems.com wrote:
 Strashko, Grygorii grygorii.stras...@ti.com writes:

 Hi Kevin,

 yep, [1] is the same fix - thanks.

 Hi Kalle,

 i've applied these changes and PM runtime fix on top of 
 git://git.kernel.org/pub/scm/linux/kernel/git/tmlind/linux-omap.git 
 (omap2plus_defconfig)
 Could you check it with your use case, pls? (just to be sure that idea is 
 right)

 I think the ideas is right, but [1] introduces more potential problems
 since it disables the IRQ at the INTC well before being disabled at the
 device.
I fail to see this point. Current driver supports master mode only.
So unless there is a msg queued by the core. May be no interrupts should fire.

what I mean

msg - conf - intr - completion/error  - autosuspend.

  With runtime PM autosuspend timeouts, that means any IRQs
 firing during the autosuspend delay will be lost, which basically
 nullifies the use of autosuspend.

so I do not expect any interrupts between completion/error - autosuspend.


 Since Shubhrajyoti didn't respond to my runtime PM comments on the
 original patch,

my apologies for the delay.

 I'll respin this patch by moving the INTC disable/enable
 into the runtime PM callbacks and make the changelog a bit more clear.

 Grygorii, that pm_runtime_resume() change is needed to.  Can you spin a
 patch with just that change with a nice descriptive changelog about why
 it is needed?  Thanks.

 Kevin
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] i2c: omap: fix spurious IRQs: disable/enable IRQ at INTC when idle

[Shubhrajyoti Datta]

On Sat, Oct 13, 2012 at 12:10 AM, Kevin Hilman
khil...@deeprootsystems.com wrote:
 From: Kevin Hilman khil...@ti.com

 Currently, runtime PM is used to keep the device enabled only during
 active transfers and for a configurable runtime PM autosuspend timout
 after an xfer.

 In addition to idling the device, driver's -runtime_suspend() method
 currently disables device interrupts when idle.  However, on some SoCs
 (notably OMAP4+), the I2C hardware may shared with other coprocessors.
 This means that the MPU will still recieve interrupts if a coprocessor
 is using the I2C device.  To avoid this, also disable interrupts at
 the MPU INTC when idling the device in -runtime_suspend() (and
 re-enable them in -runtime_resume().)  This part based on an original
 patch from Shubhrajyoti Datta.  NOTE: for proper sharing the I2C with
 a coprocessor, this driver still needs hwspinlock support added.

 This change is also meant to address an issue reported by Kalle
 Jokiniemi where I2C bus interrupt may be enabled before an I2C device
 interrupt handler (e.g. just after noirq resume phase) causing an
 interrupt flood on the I2C bus interrupt before the device interrupt
 is enabled (e.g. interrupts coming from devices on I2C connected PMIC
 before the PMIC chained hanlder is enabled.)  This problem is addresed
 by ensuring that the I2C bus interrupt left disabled until an I2C xfer
 is requested.

Looks good to me.
Will wait for Kalle though.


 Cc: Kalle Jokiniemi kalle.jokini...@jollamobile.com
 Cc: Grygorii Strashko grygorii.stras...@ti.com
 Cc: Shubhrajyoti Datta shubhrajy...@ti.com,
 Cc: Huzefa Kankroliwala huzef...@ti.com
 Cc: Nishanth Menon n...@ti.com
 Signed-off-by: Kevin Hilman khil...@ti.com
 ---
  drivers/i2c/busses/i2c-omap.c |3 +++
  1 file changed, 3 insertions(+)

 diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
 index db31eae..e6413e8 100644
 --- a/drivers/i2c/busses/i2c-omap.c
 +++ b/drivers/i2c/busses/i2c-omap.c
 @@ -1255,6 +1255,7 @@ static int omap_i2c_runtime_suspend(struct device *dev)
 /* Flush posted write */
 omap_i2c_read_reg(_dev, OMAP_I2C_STAT_REG);
 }
 +   disable_irq(_dev-irq);

 return 0;
  }
 @@ -1275,6 +1276,8 @@ static int omap_i2c_runtime_resume(struct device *dev)
 omap_i2c_write_reg(_dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN);
 }

 +   enable_irq(_dev-irq);
 +
 /*
  * Don't write to this register if the IE state is 0 as it can
  * cause deadlock.
 --
 1.7.9.2

 --
 To unsubscribe from this list: send the line unsubscribe linux-i2c in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] ARM: OMAP: i2c: fix interrupt flood during resume

[Shubhrajyoti Datta]

On Wed, Oct 10, 2012 at 5:48 PM, Kalle Jokiniemi
kalle.jokini...@jollamobile.com wrote:
 The resume_noirq enables interrupts one-by-one starting from
 first one. Now if the wake up event for suspend came from i2c
 device, the i2c bus irq gets enabled before the threaded
 i2c device irq, causing a flood of i2c bus interrupts as the
 threaded irq that should clear the event is not enabled yet.

 Fixed the issue by adding suspend_noirq and resume_early
 functions that keep i2c bus interrupts disabled until
 resume_noirq has run completely.

 Issue was detected doing a wake up from autosleep with
 twl4030 power key on N9. Patch tested on N9.

 Signed-off-by: Kalle Jokiniemi kalle.jokini...@jollamobile.com

Thanks for rebasing however since you were actually interested in one
of the older
stable releases how about cc stable?
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] char/tpm: Convert struct i2c_msg initialization to C99 format

[Shubhrajyoti Datta]

On Wed, Oct 10, 2012 at 2:36 PM, Jean Delvare kh...@linux-fr.org wrote:
 On Tue, 9 Oct 2012 17:12:31 +0530, Shubhrajyoti D wrote:
Thanks for review updated patch below
From f3786807bbaafe1f0dfcf2d94e3bee1c273296a4 Mon Sep 17 00:00:00 2001
From: Shubhrajyoti D shubhrajy...@ti.com
Date: Mon, 17 Sep 2012 19:34:37 +0530
Subject: [PATCHv2] char/tpm: Convert struct i2c_msg initialization to C99
 format

Convert the struct i2c_msg initialization to C99 format. This makes
maintaining and editing the code simpler. Also helps once other fields
like transferred are added in future.

Thanks to Julia Lawall julia.law...@lip6.fr  for automating the conversion

Acked-by: Jean Delvare kh...@linux-fr.org
Signed-off-by: Shubhrajyoti D shubhrajy...@ti.com
---
v2: removed zero initialisation of flags.

 drivers/char/tpm/tpm_i2c_infineon.c |   19 ---
 1 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/char/tpm/tpm_i2c_infineon.c
b/drivers/char/tpm/tpm_i2c_infineon.c
index 5a831ae..cbf72e1 100644
--- a/drivers/char/tpm/tpm_i2c_infineon.c
+++ b/drivers/char/tpm/tpm_i2c_infineon.c
@@ -90,8 +90,17 @@ static struct i2c_driver tpm_tis_i2c_driver;
 static int iic_tpm_read(u8 addr, u8 *buffer, size_t len)
 {

-   struct i2c_msg msg1 = { tpm_dev.client-addr, 0, 1, addr };
-   struct i2c_msg msg2 = { tpm_dev.client-addr, I2C_M_RD, len, buffer };
+   struct i2c_msg msg1 = {
+   .addr = tpm_dev.client-addr,
+   .len = 1,
+   .buf = addr
+   };
+   struct i2c_msg msg2 = {
+   .addr = tpm_dev.client-addr,
+   .flags = I2C_M_RD,
+   .len = len,
+   .buf = buffer
+   };

int rc;
int count;
@@ -138,7 +147,11 @@ static int iic_tpm_write_generic(u8 addr, u8
*buffer, size_t len,
int rc = -EIO;
int count;

-   struct i2c_msg msg1 = { tpm_dev.client-addr, 0, len + 1, tpm_dev.buf };
+   struct i2c_msg msg1 = {
+   .addr = tpm_dev.client-addr,
+   .len = len + 1,
+   .buf = tpm_dev.buf
+   };

if (len  TPM_BUFSIZE)
return -EINVAL;
-- 
1.7.5.4
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] matroxfb: Convert struct i2c_msg initialization to C99 format

[Shubhrajyoti Datta]

On Wed, Oct 10, 2012 at 2:35 PM, Jean Delvare kh...@linux-fr.org wrote:
 On Tue, 9 Oct 2012 17:07:48 +0530, Shubhrajyoti D wrote:
Thanks updated patch below.
From 99073779197f5759a76e65c3f4ef2ad4e9c88eaf Mon Sep 17 00:00:00 2001
From: Shubhrajyoti D shubhrajy...@ti.com
Date: Mon, 17 Sep 2012 21:19:32 +0530
Subject: [PATCHv2] matroxfb: Convert struct i2c_msg initialization to C99
 format

Convert the struct i2c_msg initialization to C99 format. This makes
maintaining and editing the code simpler. Also helps once other fields
like transferred are added in future.

Thanks to Julia Lawall julia.law...@lip6.fr  for automating the conversion

Acked-by: Jean Delvare kh...@linux-fr.org
Signed-off-by: Shubhrajyoti D shubhrajy...@ti.com
---
 drivers/video/matrox/matroxfb_maven.c |   16 ++--
 1 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/video/matrox/matroxfb_maven.c
b/drivers/video/matrox/matroxfb_maven.c
index 217678e..f66c34c 100644
--- a/drivers/video/matrox/matroxfb_maven.c
+++ b/drivers/video/matrox/matroxfb_maven.c
@@ -137,8 +137,20 @@ static int* get_ctrl_ptr(struct maven_data* md, int idx) {

 static int maven_get_reg(struct i2c_client* c, char reg) {
char dst;
-   struct i2c_msg msgs[] = {{ c-addr, I2C_M_REV_DIR_ADDR, sizeof(reg), 
reg },
-{ c-addr, I2C_M_RD | I2C_M_NOSTART, 
sizeof(dst), dst }};
+   struct i2c_msg msgs[] = {
+   {
+   .addr = c-addr,
+   .flags = I2C_M_REV_DIR_ADDR,
+   .len = sizeof(reg),
+   .buf = reg
+   },
+   {
+   .addr = c-addr,
+   .flags = I2C_M_RD | I2C_M_NOSTART,
+   .len = sizeof(dst),
+   .buf = dst
+   }
+   };
s32 err;

err = i2c_transfer(c-adapter, msgs, 2);
-- 
1.7.5.4
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/4] input: Convert struct i2c_msg initialization to C99 format

[Shubhrajyoti Datta]

On Wed, Oct 10, 2012 at 2:32 PM, Jean Delvare kh...@linux-fr.org wrote:
 On Tue, 9 Oct 2012 17:01:18 +0530, Shubhrajyoti D wrote:
[...]

 Acked-by: Jean Delvare kh...@linux-fr.org


thanks updated patch below
From 6638ecfa7982f95815382922c50573712c9626d7 Mon Sep 17 00:00:00 2001
From: Shubhrajyoti D shubhrajy...@ti.com
Date: Mon, 17 Sep 2012 19:37:17 +0530
Subject: [PATCHv2 1/2] input: Convert struct i2c_msg initialization to C99
 format

Convert the struct i2c_msg initialization to C99 format. This makes
maintaining and editing the code simpler. Also helps once other fields
like transferred are added in future.

Thanks to Julia Lawall julia.law...@lip6.fr  for automating the conversion

Acked-by: Jean Delvare kh...@linux-fr.org
Signed-off-by: Shubhrajyoti D shubhrajy...@ti.com
---
 drivers/input/touchscreen/cy8ctmg110_ts.c |   13 +++--
 1 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/input/touchscreen/cy8ctmg110_ts.c
b/drivers/input/touchscreen/cy8ctmg110_ts.c
index 464f1bf..ad6a664 100644
--- a/drivers/input/touchscreen/cy8ctmg110_ts.c
+++ b/drivers/input/touchscreen/cy8ctmg110_ts.c
@@ -99,9 +99,18 @@ static int cy8ctmg110_read_regs(struct cy8ctmg110 *tsc,
int ret;
struct i2c_msg msg[2] = {
/* first write slave position to i2c devices */
-   { client-addr, 0, 1, cmd },
+   {
+   .addr = client-addr,
+   .len = 1,
+   .buf = cmd
+   },
/* Second read data from position */
-   { client-addr, I2C_M_RD, len, data }
+   {
+   .addr = client-addr,
+   .flags = I2C_M_RD,
+   .len = len,
+   .buf = data
+   }
};

ret = i2c_transfer(client-adapter, msg, 2);
-- 
1.7.5.4
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] i2c: omap: Prevent NULL pointer dereference in remove

[Shubhrajyoti Datta]

On Thu, Sep 6, 2012 at 5:51 PM, Jean Delvare kh...@linux-fr.org wrote:
 On Thu, 6 Sep 2012 17:31:56 +0530, Shubhrajyoti D wrote:
 Prevent the NULL pointer access by moving the platform_set_drvdata function
 after the access of the pdev.

 [  654.961761] Unable to handle kernel NULL pointer dereference at virtual 
 address 0070
 [  654.970611] pgd = df254000
 [  654.973480] [0070] *pgd=9f1da831, *pte=, *ppte=
 [  654.980163] Internal error: Oops: 17 [#1] SMP ARM
 [  654.985076] Modules linked in:
 [  654.988281] CPU: 1Not tainted  (3.6.0-rc1-00031-ge547de1-dirty #339)
 [  654.995330] PC is at omap_i2c_runtime_resume+0x8/0x148
 [  655.000732] LR is at omap_i2c_runtime_resume+0x8/0x148

 Signed-off-by: Shubhrajyoti D shubhrajy...@ti.com

 Acked-by: Jean Delvare kh...@linux-fr.org

 (This will rather go in Ben's or Wolfram's i2c tree.)

ping.


 ---
 v2: Implement Jean's suggestion of not deleting instead make it NULL at a 
 safer time.

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

 diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
 index 5d19a49..c4ddb08 100644
 --- a/drivers/i2c/busses/i2c-omap.c
 +++ b/drivers/i2c/busses/i2c-omap.c
 @@ -1112,8 +1112,6 @@ static int __devexit omap_i2c_remove(struct 
 platform_device *pdev)
   struct resource *mem;
   int ret;

 - platform_set_drvdata(pdev, NULL);
 -
   free_irq(dev-irq, dev);
   i2c_del_adapter(dev-adapter);
   ret = pm_runtime_get_sync(pdev-dev);
 @@ -1127,6 +1125,7 @@ static int __devexit omap_i2c_remove(struct 
 platform_device *pdev)
   kfree(dev);
   mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
   release_mem_region(mem-start, resource_size(mem));
 + platform_set_drvdata(pdev, NULL);
   return 0;
  }



 --
 Jean Delvare
 --
 To unsubscribe from this list: send the line unsubscribe linux-i2c in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1] i2c-hid: introduce HID over i2c specification implementation

[Shubhrajyoti Datta]

On Fri, Sep 14, 2012 at 7:11 PM, benjamin.tissoires
benjamin.tissoi...@gmail.com wrote:
 From: Benjamin Tissoires benjamin.tissoi...@enac.fr

 Microsoft published the protocol specification of HID over i2c:
 http://msdn.microsoft.com/en-us/library/windows/hardware/hh852380.aspx

 This patch introduces an implementation of this protocol.

 This implementation does not includes the ACPI part of the specification.
 This will come when ACPI 5.0 devices will be available.

 Once the ACPI part will be done, OEM will not have to declare HID over I2C
 devices in their platform specific driver.

 Signed-off-by: Benjamin Tissoires benjamin.tissoi...@enac.fr
 ---

 Hi,

 this is finally my first implementation of HID over I2C.

 This has been tested on an Elan Microelectronics HID over I2C device, with
 a Samsung Exynos 4412 board.

 Any comments are welcome.

 Cheers,
 Benjamin

  drivers/i2c/Kconfig |8 +
  drivers/i2c/Makefile|1 +
  drivers/i2c/i2c-hid.c   | 1027 
 +++
  include/linux/i2c/i2c-hid.h |   35 ++
  4 files changed, 1071 insertions(+)
  create mode 100644 drivers/i2c/i2c-hid.c
  create mode 100644 include/linux/i2c/i2c-hid.h

 diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig
 index 5a3bb3d..5adf65a 100644
 --- a/drivers/i2c/Kconfig
 +++ b/drivers/i2c/Kconfig
 @@ -47,6 +47,14 @@ config I2C_CHARDEV
   This support is also available as a module.  If so, the module
   will be called i2c-dev.

 +config I2C_HID
 +   tristate HID over I2C bus
 +   help
 + Say Y here to use the HID over i2c protocol implementation.
 +
 + This support is also available as a module.  If so, the module
 + will be called i2c-hid.
 +
  config I2C_MUX
 tristate I2C bus multiplexing support
 help
 diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile
 index beee6b2..8f38116 100644
 --- a/drivers/i2c/Makefile
 +++ b/drivers/i2c/Makefile
 @@ -6,6 +6,7 @@ obj-$(CONFIG_I2C_BOARDINFO) += i2c-boardinfo.o
  obj-$(CONFIG_I2C)  += i2c-core.o
  obj-$(CONFIG_I2C_SMBUS)+= i2c-smbus.o
  obj-$(CONFIG_I2C_CHARDEV)  += i2c-dev.o
 +obj-$(CONFIG_I2C_HID)  += i2c-hid.o
  obj-$(CONFIG_I2C_MUX)  += i2c-mux.o
  obj-y  += algos/ busses/ muxes/

 diff --git a/drivers/i2c/i2c-hid.c b/drivers/i2c/i2c-hid.c
 new file mode 100644
 index 000..eb17d8c
 --- /dev/null
 +++ b/drivers/i2c/i2c-hid.c
 @@ -0,0 +1,1027 @@
 +/*
 + * HID over I2C protocol implementation
 + *
 + * Copyright (c) 2012 Benjamin Tissoires benjamin.tissoi...@gmail.com
 + * Copyright (c) 2012 Ecole Nationale de l'Aviation Civile, France
 + *
 + * This code is partly based on USB HID support for Linux:
 + *
 + *  Copyright (c) 1999 Andreas Gal
 + *  Copyright (c) 2000-2005 Vojtech Pavlik vojt...@suse.cz
 + *  Copyright (c) 2005 Michael Haboustak mi...@cinci.rr.com for Concept2, 
 Inc
 + *  Copyright (c) 2007-2008 Oliver Neukum
 + *  Copyright (c) 2006-2010 Jiri Kosina
 + *
 + * This file is subject to the terms and conditions of the GNU General Public
 + * License.  See the file COPYING in the main directory of this archive for
 + * more details.
 + */
 +
 +#include linux/module.h
 +#include linux/i2c.h
 +#include linux/irq.h
 +#include linux/gpio.h
 +#include linux/interrupt.h
 +#include linux/input.h
 +#include linux/delay.h
 +#include linux/slab.h
 +#include linux/pm.h
 +
 +#include linux/i2c/i2c-hid.h
 +
 +#include linux/hid.h
 +#include linux/hiddev.h
 +#include linux/hidraw.h
 +
 +#define DRIVER_NAMEi2chid
 +#define DRIVER_DESCHID over I2C core driver
 +
 +#define I2C_HID_COMMAND_TRIES  3
 +
 +/* flags */
 +#define I2C_HID_STARTED(1  0)
 +#define I2C_HID_OUT_RUNNING(1  1)
 +#define I2C_HID_IN_RUNNING (1  2)
 +#define I2C_HID_RESET_PENDING  (1  3)
 +#define I2C_HID_SUSPENDED  (1  4)
 +
 +#define I2C_HID_PWR_ON 0x00
 +#define I2C_HID_PWR_SLEEP  0x01
 +
 +/* debug option */
 +static bool debug = false;
 +module_param(debug, bool, 0444);
 +MODULE_PARM_DESC(debug, print a lot of debug informations);
 +
 +struct i2chid_desc {
 +   __le16 wHIDDescLength;
 +   __le16 bcdVersion;
 +   __le16 wReportDescLength;
 +   __le16 wReportDescRegister;
 +   __le16 wInputRegister;
 +   __le16 wMaxInputLength;
 +   __le16 wOutputRegister;
 +   __le16 wMaxOutputLength;
 +   __le16 wCommandRegister;
 +   __le16 wDataRegister;
 +   __le16 wVendorID;
 +   __le16 wProductID;
 +   __le16 wVersionID;
 +} __packed;
 +
 +struct i2chid_cmd {
 +   enum {
 +   /* fecth HID descriptor */
 +   HID_DESCR_CMD,
 +
 +   /* fetch report descriptors */
 +   HID_REPORT_DESCR_CMD,
 +
 +   /* commands */
 +   HID_RESET_CMD,
 +   HID_GET_REPORT_CMD,
 +   HID_SET_REPORT_CMD,
 +   HID_GET_IDLE_CMD,

Re: [PATCH v1] i2c-hid: introduce HID over i2c specification implementation

[Shubhrajyoti Datta]

On Wed, Oct 3, 2012 at 9:03 PM, Benjamin Tissoires
benjamin.tissoi...@gmail.com wrote:
 Hi,

 thanks also for the review. Two in the same day! I was about to send a
 ping on that patch ;-)

 On Wed, Oct 3, 2012 at 8:05 AM, Shubhrajyoti Datta
 omaplinuxker...@gmail.com wrote:
 On Fri, Sep 14, 2012 at 7:11 PM, benjamin.tissoires
 benjamin.tissoi...@gmail.com wrote:
 From: Benjamin Tissoires benjamin.tissoi...@enac.fr

 Microsoft published the protocol specification of HID over i2c:
 http://msdn.microsoft.com/en-us/library/windows/hardware/hh852380.aspx

 This patch introduces an implementation of this protocol.

 This implementation does not includes the ACPI part of the specification.
 This will come when ACPI 5.0 devices will be available.

 Once the ACPI part will be done, OEM will not have to declare HID over I2C
 devices in their platform specific driver.

 Signed-off-by: Benjamin Tissoires benjamin.tissoi...@enac.fr
 ---

 Hi,

 this is finally my first implementation of HID over I2C.

 This has been tested on an Elan Microelectronics HID over I2C device, with
 a Samsung Exynos 4412 board.

 Any comments are welcome.

 Cheers,
 Benjamin

  drivers/i2c/Kconfig |8 +
  drivers/i2c/Makefile|1 +
  drivers/i2c/i2c-hid.c   | 1027 
 +++
  include/linux/i2c/i2c-hid.h |   35 ++
  4 files changed, 1071 insertions(+)
  create mode 100644 drivers/i2c/i2c-hid.c
  create mode 100644 include/linux/i2c/i2c-hid.h

 diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig

[...]

 same thing, will change it in v2.


 +
 +   if (debug)
 +   dev_err(client-dev, resetting...\n);
 this is a little uncustomary.

 May be consider  bdg

 Sorry for that. I don't get your point here. You don't like the whole
 if(debug) dev_err(...) or just the dev_err(...) call?

Apologies for the typo dev_dbg.
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] i2c: add Renesas R-Car I2C driver

[Shubhrajyoti Datta]

On Fri, Sep 28, 2012 at 11:26 AM, Kuninori Morimoto
kuninori.morimoto...@renesas.com wrote:

 Dear Shubhrajyoti

 Thank you for your comment.

 Hi A few suggestions,

 On Tue, Aug 28, 2012 at 2:07 PM, Kuninori Morimoto
 kuninori.morimoto...@renesas.com wrote:
  R-Car I2C is similar with SH7760 I2C.
  But the SH7760 I2C driver had many workaround operations, since H/W had 
  bugs.
  Thus, it was pointless to keep compatible between SH7760 and R-Car I2C 
  drivers.
  This patch creates new Renesas R-Car I2C driver.
 
  Signed-off-by: Kuninori Morimoto kuninori.morimoto...@renesas.com
  ---
  v1 - v2
 
   - removed #if 0 function
   - add explanation on rcar_i2c_bus_barrier()
   - removed IGNORE_NAK support
   - rename rcar_i2c_soft_reset() - rcar_i2c_init()
   - removed devm_kfree/devm_iounmap
   - __raw_writel/readl = writel/readl
   - removed un-needed return from rcar_i2c_bus_phase()
   - tidyup calculation method on rcar_i2c_clock_calculate()
   - tidyup English type
   - tidyup comment to i2c device disabled
 
   drivers/i2c/busses/Kconfig|   10 +
   drivers/i2c/busses/Makefile   |1 +
   drivers/i2c/busses/i2c-rcar.c |  715 
  +
   include/linux/i2c/i2c-rcar.h  |   10 +
   4 files changed, 736 insertions(+), 0 deletions(-)
   create mode 100644 drivers/i2c/busses/i2c-rcar.c
   create mode 100644 include/linux/i2c/i2c-rcar.h
 
  diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
  index b4aaa1b..51baa08 100644
  --- a/drivers/i2c/busses/Kconfig
  +++ b/drivers/i2c/busses/Kconfig
  @@ -709,6 +709,16 @@ config I2C_XLR
This driver can also be built as a module.  If so, the module
will be called i2c-xlr.
 
  +config I2C_RCAR
  +   tristate Renesas R-Car I2C Controller
  +   depends on ARCH_SHMOBILE  I2C
  +   help
  + If you say yes to this option, support will be included for the
  + R-Car I2C controller.
  +
  + This driver can also be built as a module.  If so, the module
  + will be called i2c-rcar.
  +
   comment External I2C/SMBus adapter drivers
 
   config I2C_DIOLAN_U2C
  diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
  index ce3c2be..e98ff51 100644
  --- a/drivers/i2c/busses/Makefile
  +++ b/drivers/i2c/busses/Makefile
  @@ -70,6 +70,7 @@ obj-$(CONFIG_I2C_VERSATILE)   += i2c-versatile.o
   obj-$(CONFIG_I2C_OCTEON)   += i2c-octeon.o
   obj-$(CONFIG_I2C_XILINX)   += i2c-xiic.o
   obj-$(CONFIG_I2C_XLR)  += i2c-xlr.o
  +obj-$(CONFIG_I2C_RCAR) += i2c-rcar.o
 
   # External I2C/SMBus adapter drivers
   obj-$(CONFIG_I2C_DIOLAN_U2C)   += i2c-diolan-u2c.o
  diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
  new file mode 100644
  index 000..bd8fcf1
  --- /dev/null
  +++ b/drivers/i2c/busses/i2c-rcar.c
  @@ -0,0 +1,715 @@
  +/*
  + *  drivers/i2c/busses/i2c-rcar.c
  + *
  + * Copyright (C) 2012 Renesas Solutions Corp.
  + * Kuninori Morimoto kuninori.morimoto...@renesas.com
  + *
  + * This file is based on the drivers/i2c/busses/i2c-sh7760.c
  + * (c) 2005-2008 MSC Vertriebsges.m.b.H, Manuel Lauss m...@msc-ge.com
  + *
  + * This file used out-of-tree driver i2c-rcar.c
  + * Copyright (C) 2011-2012 Renesas Electronics Corporation
  + *
  + * This program is free software; you can redistribute it and/or modify
  + * it under the terms of the GNU General Public License as published by
  + * the Free Software Foundation; either version 2 of the License
  + *
  + * This program is distributed in the hope that it will be useful,
  + * but WITHOUT ANY WARRANTY; without even the implied warranty of
  + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  + * GNU General Public License for more details.
  + *
  + * You should have received a copy of the GNU General Public License
  + * along with this program; if not, write to the Free Software
  + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  
  USA
  + */
  +#include linux/clk.h
  +#include linux/delay.h
  +#include linux/err.h
  +#include linux/init.h
  +#include linux/interrupt.h
  +#include linux/io.h
  +#include linux/i2c.h
  +#include linux/i2c/i2c-rcar.h
  +#include linux/kernel.h
  +#include linux/module.h
  +#include linux/platform_device.h
  +#include linux/pm_runtime.h
  +#include linux/slab.h
  +#include linux/spinlock.h
  +
  +/* register offsets */
  +#define ICSCR  0x00/* slave ctrl */
  +#define ICMCR  0x04/* master ctrl */
  +#define ICSSR  0x08/* slave status */
  +#define ICMSR  0x0C/* master status */
  +#define ICSIER 0x10/* slave irq enable */
  +#define ICMIER 0x14/* master irq enable */
  +#define ICCCR  0x18/* clock dividers */
  +#define ICSAR  0x1C/* slave address */
  +#define ICMAR  0x20/* master address */
  +#define ICRXTX 0x24/* data port */
  +
  +/* ICMCR */
  +#define MDBS   (1  7)/* non-fifo mode switch */
  +#define FSCL   (1  6) 

Re: [PATCH v2] i2c: add Renesas R-Car I2C driver

[Shubhrajyoti Datta]

Hi A few suggestions,

On Tue, Aug 28, 2012 at 2:07 PM, Kuninori Morimoto
kuninori.morimoto...@renesas.com wrote:
 R-Car I2C is similar with SH7760 I2C.
 But the SH7760 I2C driver had many workaround operations, since H/W had bugs.
 Thus, it was pointless to keep compatible between SH7760 and R-Car I2C 
 drivers.
 This patch creates new Renesas R-Car I2C driver.

 Signed-off-by: Kuninori Morimoto kuninori.morimoto...@renesas.com
 ---
 v1 - v2

  - removed #if 0 function
  - add explanation on rcar_i2c_bus_barrier()
  - removed IGNORE_NAK support
  - rename rcar_i2c_soft_reset() - rcar_i2c_init()
  - removed devm_kfree/devm_iounmap
  - __raw_writel/readl = writel/readl
  - removed un-needed return from rcar_i2c_bus_phase()
  - tidyup calculation method on rcar_i2c_clock_calculate()
  - tidyup English type
  - tidyup comment to i2c device disabled

  drivers/i2c/busses/Kconfig|   10 +
  drivers/i2c/busses/Makefile   |1 +
  drivers/i2c/busses/i2c-rcar.c |  715 
 +
  include/linux/i2c/i2c-rcar.h  |   10 +
  4 files changed, 736 insertions(+), 0 deletions(-)
  create mode 100644 drivers/i2c/busses/i2c-rcar.c
  create mode 100644 include/linux/i2c/i2c-rcar.h

 diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
 index b4aaa1b..51baa08 100644
 --- a/drivers/i2c/busses/Kconfig
 +++ b/drivers/i2c/busses/Kconfig
 @@ -709,6 +709,16 @@ config I2C_XLR
   This driver can also be built as a module.  If so, the module
   will be called i2c-xlr.

 +config I2C_RCAR
 +   tristate Renesas R-Car I2C Controller
 +   depends on ARCH_SHMOBILE  I2C
 +   help
 + If you say yes to this option, support will be included for the
 + R-Car I2C controller.
 +
 + This driver can also be built as a module.  If so, the module
 + will be called i2c-rcar.
 +
  comment External I2C/SMBus adapter drivers

  config I2C_DIOLAN_U2C
 diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
 index ce3c2be..e98ff51 100644
 --- a/drivers/i2c/busses/Makefile
 +++ b/drivers/i2c/busses/Makefile
 @@ -70,6 +70,7 @@ obj-$(CONFIG_I2C_VERSATILE)   += i2c-versatile.o
  obj-$(CONFIG_I2C_OCTEON)   += i2c-octeon.o
  obj-$(CONFIG_I2C_XILINX)   += i2c-xiic.o
  obj-$(CONFIG_I2C_XLR)  += i2c-xlr.o
 +obj-$(CONFIG_I2C_RCAR) += i2c-rcar.o

  # External I2C/SMBus adapter drivers
  obj-$(CONFIG_I2C_DIOLAN_U2C)   += i2c-diolan-u2c.o
 diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
 new file mode 100644
 index 000..bd8fcf1
 --- /dev/null
 +++ b/drivers/i2c/busses/i2c-rcar.c
 @@ -0,0 +1,715 @@
 +/*
 + *  drivers/i2c/busses/i2c-rcar.c
 + *
 + * Copyright (C) 2012 Renesas Solutions Corp.
 + * Kuninori Morimoto kuninori.morimoto...@renesas.com
 + *
 + * This file is based on the drivers/i2c/busses/i2c-sh7760.c
 + * (c) 2005-2008 MSC Vertriebsges.m.b.H, Manuel Lauss m...@msc-ge.com
 + *
 + * This file used out-of-tree driver i2c-rcar.c
 + * Copyright (C) 2011-2012 Renesas Electronics Corporation
 + *
 + * This program is free software; you can redistribute it and/or modify
 + * it under the terms of the GNU General Public License as published by
 + * the Free Software Foundation; either version 2 of the License
 + *
 + * This program is distributed in the hope that it will be useful,
 + * but WITHOUT ANY WARRANTY; without even the implied warranty of
 + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 + * GNU General Public License for more details.
 + *
 + * You should have received a copy of the GNU General Public License
 + * along with this program; if not, write to the Free Software
 + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
 + */
 +#include linux/clk.h
 +#include linux/delay.h
 +#include linux/err.h
 +#include linux/init.h
 +#include linux/interrupt.h
 +#include linux/io.h
 +#include linux/i2c.h
 +#include linux/i2c/i2c-rcar.h
 +#include linux/kernel.h
 +#include linux/module.h
 +#include linux/platform_device.h
 +#include linux/pm_runtime.h
 +#include linux/slab.h
 +#include linux/spinlock.h
 +
 +/* register offsets */
 +#define ICSCR  0x00/* slave ctrl */
 +#define ICMCR  0x04/* master ctrl */
 +#define ICSSR  0x08/* slave status */
 +#define ICMSR  0x0C/* master status */
 +#define ICSIER 0x10/* slave irq enable */
 +#define ICMIER 0x14/* master irq enable */
 +#define ICCCR  0x18/* clock dividers */
 +#define ICSAR  0x1C/* slave address */
 +#define ICMAR  0x20/* master address */
 +#define ICRXTX 0x24/* data port */
 +
 +/* ICMCR */
 +#define MDBS   (1  7)/* non-fifo mode switch */
 +#define FSCL   (1  6)/* override SCL pin */
 +#define FSDA   (1  5)/* override SDA pin */
 +#define OBPC   (1  4)/* override pins */
 +#define MIE(1  3)/* master if enable */
 +#define TSBE   (1  2)
 +#define FSB(1  1)/* force stop bit */
 +#define ESG  

Re: [PATCH] ARM: OMAP: convert I2C driver to PM QoS for MPU latency constraints

[Shubhrajyoti Datta]

On Thu, Sep 20, 2012 at 9:38 PM, Jean Pihet jean.pi...@newoldbits.com wrote:
 Convert the driver from the outdated omap_pm_set_max_mpu_wakeup_lat
 API to the new PM QoS API.
 Since the constraint is on the MPU subsystem, use the PM_QOS_CPU_DMA_LATENCY
 class of PM QoS. The resulting MPU constraints are used by cpuidle to
 decide the next power state of the MPU subsystem.

 The I2C device latency timing is derived from the FIFO size and the
 clock speed and so is applicable to all OMAP SoCs.
Agree thanks for doing that.


 Signed-off-by: Jean Pihet j-pi...@ti.com
 ---
 Rebased on git://git.pengutronix.de/git/wsa/linux.git, branch
 i2c-embedded/for-next
thanks ,
tested with i2c tools  on omap4sdp and omap3sdp

Acked-by: Shubhrajyoti D shubhrajy...@ti.com

 ---
  arch/arm/plat-omap/i2c.c  |   21 -
  drivers/i2c/busses/i2c-omap.c |   32 ++--
  include/linux/i2c-omap.h  |1 -
  3 files changed, 18 insertions(+), 36 deletions(-)

--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv7 00/23]I2C big cleanup

[Shubhrajyoti Datta]

On Wed, Sep 12, 2012 at 7:14 PM, Wolfram Sang w.s...@pengutronix.de wrote:
 On Wed, Sep 12, 2012 at 12:18:50PM +0200, Wolfram Sang wrote:

  I donot see the warning. Am I missing something?

 I deleted my logfiles already. Ignore it for now, if it comes up again
 with your new series, I will give a more detailed pointer.

 Sorry, the section mismatch was not related to I2C it seems:

Thanks for the report just sent a patch fixing that.


 WARNING: vmlinux.o(.data+0x30958): Section mismatch in reference from the 
 variable rx51_si4713_dev to the (unknown reference) .init.data:(unknown)
 The variable rx51_si4713_dev references
 the (unknown reference) __initdata (unknown)
 If the reference is valid then annotate the
 variable with __init* or __refdata (see linux/init.h) or name the variable:
 *_template, *_timer, *_sht, *_ops, *_probe, *_probe_one, *_console

 Got it with an allnoconfig and then selecting MMU and OMAP.

 Regards,

Wolfram

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

 -BEGIN PGP SIGNATURE-
 Version: GnuPG v1.4.10 (GNU/Linux)

 iEYEARECAAYFAlBQkbsACgkQD27XaX1/VRtbuACgkBa0lOIN551eec9TSetVPsCE
 Ew0AoKizKon3DIILpERWJIwzAXdgRVDc
 =T4Yq
 -END PGP SIGNATURE-

--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] i2c: omap: Prevent NULL pointer dereference in remove

[Shubhrajyoti Datta]

Hi Jean,
Thanks for the review.

On Thu, Aug 30, 2012 at 12:17 AM, Jean Delvare kh...@linux-fr.org wrote:
 On Thu, 23 Aug 2012 19:51:26 +0530, Shubhrajyoti D wrote:
 Prevent the NULL pointer access of pdev-dev in remove. The platform_device 
 is anyways
 deleted so remove  platform_set_drvdata(pdev, NULL);.

[...]

 @@ -1112,8 +,6 @@ static int __devexit omap_i2c_remove(struct 
 platform_device *pdev)
   struct resource *mem;
   int ret;

 - platform_set_drvdata(pdev, NULL);
 -

 This OTOH is a good catch. But the problem isn't with calling
 platform_set_drvdata(pdev, NULL) per se. The problem is with calling it
 too early. It should be called after i2c_del_adapter(), and ideally
 before freeing the memory.


Just  sent a patch implementing your suggestion.
thanks,
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] i2c: add Renesas R-Car I2C driver

[Shubhrajyoti Datta]

On Wed, Jul 25, 2012 at 12:06 PM, Kuninori Morimoto
kuninori.morimoto...@renesas.com wrote:
 R-Car I2C is similar with SH7760 I2C.
 But the SH7760 I2C driver had many workaround operations, since H/W had bugs.
 Thus, it was pointless to keep compatible between SH7760 and R-Car I2C 
 drivers.
 This patch creates new Renesas R-Car I2C driver.

 Signed-off-by: Kuninori Morimoto kuninori.morimoto...@renesas.com
 ---
  drivers/i2c/busses/Kconfig|   10 +
  drivers/i2c/busses/Makefile   |1 +
  drivers/i2c/busses/i2c-rcar.c |  741 
 +
  include/linux/i2c/i2c-rcar.h  |   10 +
  4 files changed, 762 insertions(+), 0 deletions(-)
  create mode 100644 drivers/i2c/busses/i2c-rcar.c
  create mode 100644 include/linux/i2c/i2c-rcar.h

 diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
 index 7244c8b..3175c81 100644
 --- a/drivers/i2c/busses/Kconfig
 +++ b/drivers/i2c/busses/Kconfig
 @@ -704,6 +704,16 @@ config I2C_XLR
   This driver can also be built as a module.  If so, the module
   will be called i2c-xlr.

 +config I2C_RCAR
 +   tristate Renesas R-Car I2C Controller
 +   depends on ARCH_SHMOBILE  I2C
 +   help
 + If you say yes to this option, support will be included for the
 + R-Car I2C controller.
 +
 + This driver can also be built as a module.  If so, the module
 + will be called i2c-rcar.
 +
  comment External I2C/SMBus adapter drivers

  config I2C_DIOLAN_U2C
 diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
 index ce3c2be..e98ff51 100644
 --- a/drivers/i2c/busses/Makefile
 +++ b/drivers/i2c/busses/Makefile
 @@ -70,6 +70,7 @@ obj-$(CONFIG_I2C_VERSATILE)   += i2c-versatile.o
  obj-$(CONFIG_I2C_OCTEON)   += i2c-octeon.o
  obj-$(CONFIG_I2C_XILINX)   += i2c-xiic.o
  obj-$(CONFIG_I2C_XLR)  += i2c-xlr.o
 +obj-$(CONFIG_I2C_RCAR) += i2c-rcar.o

  # External I2C/SMBus adapter drivers
  obj-$(CONFIG_I2C_DIOLAN_U2C)   += i2c-diolan-u2c.o
 diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
 new file mode 100644
 index 000..2993cab
 --- /dev/null
 +++ b/drivers/i2c/busses/i2c-rcar.c
 @@ -0,0 +1,741 @@
 +/*
 + *  drivers/i2c/busses/i2c-rcar.c
 + *
 + * Copyright (C) 2012 Renesas Solutions Corp.
 + * Kuninori Morimoto kuninori.morimoto...@renesas.com
 + *
 + * This file is based on the drivers/i2c/busses/i2c-sh7760.c
 + * (c) 2005-2008 MSC Vertriebsges.m.b.H, Manuel Lauss m...@msc-ge.com
 + *
 + * This file used out-of-tree driver i2c-rcar.c
 + * Copyright (C) 2011-2012 Renesas Electronics Corporation
 + *
 + * This program is free software; you can redistribute it and/or modify
 + * it under the terms of the GNU General Public License as published by
 + * the Free Software Foundation; either version 2 of the License
 + *
 + * This program is distributed in the hope that it will be useful,
 + * but WITHOUT ANY WARRANTY; without even the implied warranty of
 + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 + * GNU General Public License for more details.
 + *
 + * You should have received a copy of the GNU General Public License
 + * along with this program; if not, write to the Free Software
 + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
 + */
 +#include linux/clk.h
 +#include linux/delay.h
 +#include linux/err.h
 +#include linux/init.h
 +#include linux/interrupt.h
 +#include linux/io.h
 +#include linux/i2c.h
 +#include linux/i2c/i2c-rcar.h
 +#include linux/kernel.h
 +#include linux/module.h
 +#include linux/platform_device.h
 +#include linux/pm_runtime.h
 +#include linux/slab.h
 +#include linux/spinlock.h
 +
 +/* register offsets */
 +#define ICSCR  0x00/* slave ctrl */
 +#define ICMCR  0x04/* master ctrl */
 +#define ICSSR  0x08/* slave status */
 +#define ICMSR  0x0C/* master status */
 +#define ICSIER 0x10/* slave irq enable */
 +#define ICMIER 0x14/* master irq enable */
 +#define ICCCR  0x18/* clock dividers */
 +#define ICSAR  0x1C/* slave address */
 +#define ICMAR  0x20/* master address */
 +#define ICRXTX 0x24/* data port */
 +
 +/* ICMCR */
 +#define MDBS   (1  7)/* non-fifo mode switch */
 +#define FSCL   (1  6)/* override SCL pin */
 +#define FSDA   (1  5)/* override SDA pin */
 +#define OBPC   (1  4)/* override pins */
 +#define MIE(1  3)/* master if enable */
 +#define TSBE   (1  2)
 +#define FSB(1  1)/* force stop bit */
 +#define ESG(1  0)/* en startbit gen */
 +
 +/* ICMSR */
 +#define MNR(1  6)/* nack received */
 +#define MAL(1  5)/* arbitration lost */
 +#define MST(1  4)/* sent a stop */
 +#define MDE(1  3)
 +#define MDT(1  2)
 +#define MDR(1  1)
 +#define MAT(1  0)/* slave addr xfer done */
 +
 +/* ICMIE */
 +#define MNRE   (1  6)/* nack irq en */
 +#define MALE   (1  5)/* 

Re: [PATCH] i2c: add Renesas R-Car I2C driver

[Shubhrajyoti Datta]

On Tue, Aug 28, 2012 at 11:03 AM, Kuninori Morimoto
kuninori.morimoto...@renesas.com wrote:

 Hi Shubhrajyoti

 Thank you for checking patch.
 I create v2 patch and post it soon.
 There is not big reason, but I would like to keep adap-retries for now.

Or maybe just wait for Jean's advice on the same.
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/6] i2c-xiic: Fix a possible NULL pointer access

[Shubhrajyoti Datta]

On Sat, Aug 18, 2012 at 3:47 PM, Wolfram Sang w.s...@pengutronix.de wrote:
 On Thu, Aug 09, 2012 at 07:07:43PM +0530, Shubhrajyoti D wrote:
 platform_get_resource uses pdev so move the function
 platform_set_drvdata(pdev, NULL) after the get_resource.

Yes agree.

 This descriptions sounds as you'd assume pdev is set to NULL here?
 Careful, platform_set_drvdata() does not clear pdev, but driver_data of
 pdev-dev!

 So, I guess the rest of this series is bogus?

 It would be worth checking if other drivers have the same pattern as the
 omap driver where the fix was valid for other reasons. Are you
 interested in doing that (perfectly fine to say no).

Yes I can take that , lest someone with the platform/ test set-up can
take that up.

 Regards,

Wolfram

--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/6] i2c: omap: Prevent NULL pointer dereference in remove

[Shubhrajyoti Datta]

On Sat, Aug 18, 2012 at 3:39 PM, Wolfram Sang w.s...@pengutronix.de wrote:
 On Thu, Aug 09, 2012 at 07:07:42PM +0530, Shubhrajyoti D wrote:
 Prevent the NULL pointer access by moving the platform_set_drvdata function
 after the access of the pdev.

 [  654.961761] Unable to handle kernel NULL pointer dereference at virtual 
 address 0070
 [  654.970611] pgd = df254000
 [  654.973480] [0070] *pgd=9f1da831, *pte=, *ppte=
 [  654.980163] Internal error: Oops: 17 [#1] SMP ARM
 [  654.985076] Modules linked in:
 [  654.988281] CPU: 1Not tainted  (3.6.0-rc1-00031-ge547de1-dirty #339)
 [  654.995330] PC is at omap_i2c_runtime_resume+0x8/0x148
 [  655.000732] LR is at omap_i2c_runtime_resume+0x8/0x148

 Signed-off-by: Shubhrajyoti D shubhrajy...@ti.com
 ---
  drivers/i2c/busses/i2c-omap.c |4 ++--
  1 files changed, 2 insertions(+), 2 deletions(-)

 diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
 index c8e3886..0c593d4 100644
 --- a/drivers/i2c/busses/i2c-omap.c
 +++ b/drivers/i2c/busses/i2c-omap.c
 @@ -1217,8 +1217,6 @@ static int __devexit omap_i2c_remove(struct 
 platform_device *pdev)
   struct omap_i2c_dev *dev = platform_get_drvdata(pdev);
   int ret;

 - platform_set_drvdata(pdev, NULL);
 -
   i2c_del_adapter(dev-adapter);
   ret = pm_runtime_get_sync(pdev-dev);
   if (IS_ERR_VALUE(ret))
 @@ -1227,6 +1225,8 @@ static int __devexit omap_i2c_remove(struct 
 platform_device *pdev)
   omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
   pm_runtime_put(pdev-dev);
   pm_runtime_disable(pdev-dev);
 + platform_set_drvdata(pdev, NULL);
 +
   return 0;
  }

 I think this patch is correct, because drvdata is used in the PM code of
 the driver and thus cleared too early.

 As such, this is a bugfix and should be not based on the big cleanup
 since it should go into this rc series.

 I will pick it as soon as the comments on the other patches are
 answered.
BTW it was pointed out that the The
platform_device object will be destroyed.

Do you agree with

http://www.spinics.net/lists/linux-omap/msg75553.html


 --
 Pengutronix e.K.   | Wolfram Sang|
 Industrial Linux Solutions | http://www.pengutronix.de/  |
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/6] i2c-xiic: Fix a possible NULL pointer access

[Shubhrajyoti Datta]

On Sat, Aug 18, 2012 at 6:52 PM, Shubhrajyoti Datta
omaplinuxker...@gmail.com wrote:
 On Sat, Aug 18, 2012 at 3:47 PM, Wolfram Sang w.s...@pengutronix.de wrote:
 On Thu, Aug 09, 2012 at 07:07:43PM +0530, Shubhrajyoti D wrote:
 platform_get_resource uses pdev so move the function
 platform_set_drvdata(pdev, NULL) after the get_resource.

 Yes agree.

 This descriptions sounds as you'd assume pdev is set to NULL here?
 Careful, platform_set_drvdata() does not clear pdev, but driver_data of
 pdev-dev!

 So, I guess the rest of this series is bogus?

 It would be worth checking if other drivers have the same pattern as the
 omap driver where the fix was valid for other reasons. Are you
 interested in doing that (perfectly fine to say no).

 Yes I can take that , lest someone with the platform/ test set-up can
 take that up.

By the way davinci and designware seem to have the issue
however having a closer look even the probe err handling looks wrong.


 Regards,

Wolfram

--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] I2C: MIPS: lantiq: add FALC-ON i2c bus master

[Shubhrajyoti Datta]

Hi John ,

On Thu, Aug 16, 2012 at 1:04 PM, John Crispin blo...@openwrt.org wrote:
 This patch adds the driver needed to make the I2C bus work on FALC-ON SoCs.

 Signed-off-by: Thomas Langer thomas.lan...@lantiq.com
 Signed-off-by: John Crispin blo...@openwrt.org
 ---
  drivers/i2c/busses/Kconfig  |   10 +
  drivers/i2c/busses/Makefile |1 +
  drivers/i2c/busses/i2c-lantiq.c |  752 
 +++
  drivers/i2c/busses/i2c-lantiq.h |  234 
  4 files changed, 997 insertions(+), 0 deletions(-)
  create mode 100644 drivers/i2c/busses/i2c-lantiq.c
  create mode 100644 drivers/i2c/busses/i2c-lantiq.h

 diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
 index b4aaa1b..1e80198 100644
 --- a/drivers/i2c/busses/Kconfig
 +++ b/drivers/i2c/busses/Kconfig
 @@ -449,6 +449,16 @@ config I2C_IOP3XX
   This driver can also be built as a module.  If so, the module
   will be called i2c-iop3xx.

 +config I2C_LANTIQ
 +   tristate Lantiq I2C interface
 +   depends on LANTIQ  SOC_FALCON
 +   help
 + If you say yes to this option, support will be included for the
 + Lantiq I2C core.
 +
 + This driver can also be built as a module. If so, the module
 + will be called i2c-lantiq.
 +
  config I2C_MPC
 tristate MPC107/824x/85xx/512x/52xx/83xx/86xx
 depends on PPC
 diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
 index ce3c2be..da72247 100644
 --- a/drivers/i2c/busses/Makefile
 +++ b/drivers/i2c/busses/Makefile
 @@ -44,6 +44,7 @@ obj-$(CONFIG_I2C_IBM_IIC) += i2c-ibm_iic.o
  obj-$(CONFIG_I2C_IMX)  += i2c-imx.o
  obj-$(CONFIG_I2C_INTEL_MID)+= i2c-intel-mid.o
  obj-$(CONFIG_I2C_IOP3XX)   += i2c-iop3xx.o
 +obj-$(CONFIG_I2C_LANTIQ)   += i2c-lantiq.o
  obj-$(CONFIG_I2C_MPC)  += i2c-mpc.o
  obj-$(CONFIG_I2C_MV64XXX)  += i2c-mv64xxx.o
  obj-$(CONFIG_I2C_MXS)  += i2c-mxs.o
 diff --git a/drivers/i2c/busses/i2c-lantiq.c b/drivers/i2c/busses/i2c-lantiq.c
 new file mode 100644
 index 000..e0afa8c
 --- /dev/null
 +++ b/drivers/i2c/busses/i2c-lantiq.c
 @@ -0,0 +1,752 @@
 +/*
 + * Lantiq I2C bus adapter
 + *
 + * Parts based on i2c-designware.c and other i2c drivers from Linux 2.6.33
 + *
 + * This program is free software; you can redistribute it and/or modify
 + * it under the terms of the GNU General Public License as published by
 + * the Free Software Foundation; either version 2 of the License, or
 + * (at your option) any later version.
 + *
 + * This program is distributed in the hope that it will be useful,
 + * but WITHOUT ANY WARRANTY; without even the implied warranty of
 + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 + * GNU General Public License for more details.
 + *
 + * You should have received a copy of the GNU General Public License
 + * along with this program; if not, write to the Free Software
 + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
 + *
 + * Copyright (C) 2012 Thomas Langer thomas.lan...@lantiq.com
 + */
 +
 +#include linux/kernel.h
 +#include linux/module.h
 +#include linux/delay.h
 +#include linux/slab.h /* for kzalloc, kfree */
 +#include linux/i2c.h
 +#include linux/errno.h
 +#include linux/completion.h
 +#include linux/interrupt.h
 +#include linux/platform_device.h
 +#include linux/io.h
 +#include linux/of_irq.h
 +#include linux/of_i2c.h
 +
 +#include lantiq_soc.h
 +#include i2c-lantiq.h
 +
 +/* CURRENT ISSUES:
 + * - no high speed support
 + * - ten bit mode is not tested (no slave devices)
 + */
 +
 +/* access macros */
 +#define i2c_r32(reg)   \
 +   __raw_readl((priv-membase)-reg)
 +#define i2c_w32(val, reg)  \
 +   __raw_writel(val, (priv-membase)-reg)
 +#define i2c_w32_mask(clear, set, reg)  \
 +   i2c_w32((i2c_r32(reg)  ~(clear)) | (set), reg)
 +
 +#define DRV_NAME i2c-lantiq
 +#define DRV_VERSION 1.00
 +
 +#define LTQ_I2C_BUSY_TIMEOUT   20 /* ms */
 +
 +#ifdef DEBUG
 +#define LTQ_I2C_XFER_TIMEOUT   (25*HZ)
 +#else
 +#define LTQ_I2C_XFER_TIMEOUT   HZ
 +#endif
 +#if defined(DEBUG)  0
 +#define PRINTK(arg...) pr_debug(arg)
 +#else
 +#define PRINTK(arg...) do {} while (0)
 +#endif
 +
 +#define LTQ_I2C_IMSC_DEFAULT_MASK  (I2C_IMSC_I2C_P_INT_EN | \
 +I2C_IMSC_I2C_ERR_INT_EN)
 +
 +#define LTQ_I2C_ARB_LOST   (1  0)
 +#define LTQ_I2C_NACK   (1  1)
 +#define LTQ_I2C_RX_UFL (1  2)
 +#define LTQ_I2C_RX_OFL (1  3)
 +#define LTQ_I2C_TX_UFL (1  4)
 +#define LTQ_I2C_TX_OFL (1  5)
 +
 +struct lantiq_i2c {
 +   struct mutex mutex;
 +
 +   /* active clock settings */
 +   unsigned int input_clock;   /* clock input for i2c hardware block 
 */
 +   unsigned int i2c_clock; /* approximated bus clock in kHz */
 +
 +   struct clk *clk_gate;
 +   struct clk *clk_input;
 +
 +