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

[Gao Pan]

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 consumption and i2c bus performance, runtime
pm is the good solution for it. The clk is enabled when a i2c
transfer starts, and disabled after 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

Signed-off-by: Fugang Duan 
Signed-off-by: Gao Pan 
[wsa: fixed some indentation]
Signed-off-by: Wolfram Sang 
---
V2:
As Uwe Kleine-König's suggestion, the version do below changes:
 -call clk_prepare_enable in probe to avoid never enabling clock
  if CONFIG_PM is disabled
 -enable clock before request IRQ in probe
 -remove the pm staff in i2c_imx_isr

V3:
 -pm_runtime_get_sync returns < 0 as error

V4:
 -add pm_runtime_set_active before pm_runtime_enable
 -replace pm_runtime_put_autosuspend with pm_runtime_autosuspend
  in probe
 -add error disposal when i2c_add_numbered_adapter fails

V5:
 -clean up and disable runtime PM when i2c_add_numbered_adapter fails
 -use pm_runtime_get and pm_runtime_put_autosuspend in probe

 drivers/i2c/busses/i2c-imx.c | 91 
 1 file changed, 76 insertions(+), 15 deletions(-)

diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index 785aa67..8940a89 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -53,6 +53,7 @@
 #include 
 #include 
 #include 
+#include 
 
 /** 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)
@@ -894,6 +893,10 @@ static int i2c_imx_xfer(struct i2c_adapter *adapter,
 
dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
 
+   result = pm_runtime_get_sync(i2c_imx->adapter.dev.parent);
+   if (result < 0)
+   goto out;
+
/* Start I2C transfer */
result = i2c_imx_start(i2c_imx);
if (result)
@@ -950,6 +953,10 @@ fail0:
/* Stop I2C transfer */
i2c_imx_stop(i2c_imx);
 
+out:
+   pm_runtime_mark_last_busy(i2c_imx->adapter.dev.parent);
+   pm_runtime_put_autosuspend(i2c_imx->adapter.dev.parent);
+
dev_dbg(&i2c_imx->adapter.dev, "<%s> exit with: %s: %d\n", __func__,
(result < 0) ? "error" : "success msg",
(result < 0) ? result : num);
@@ -1020,15 +1027,17 @@ static int i2c_imx_probe(struct platform_device *pdev)
 
ret = clk_prepare_enable(i2c_imx->clk);
if (ret) {
-   dev_err(&pdev->dev, "can't enable I2C clock\n");
+   dev_err(&pdev->dev, "can't enable I2C clock, ret=%d\n", ret);
return ret;
}
+
/* Request IRQ */
ret = devm_request_irq(&pdev->dev, irq, i2c_imx_isr, 0,
pdev->name, i2c_imx);
if (ret) {
dev_err(&pdev->dev, "can't claim irq %d\n", irq);
-   goto clk_disable;
+   clk_disable_unprepare(i2c_imx->clk);
+   return ret;
}
 
/* Init queue */
@@ -1037,6 +1046,18 @@ static int i2c_imx_probe(struct platform_device *pdev)
/* Set up adapter data */
i2c_set_adapdata(&i2c_imx->adapter, i2c_imx);
 
+   /* Set up platform driver data */
+   platform_set_drvdata(pdev, i2c_imx);
+
+   pm_runtime_set_autosuspend_delay(&pdev->dev, I2C_PM_TIMEOUT);
+   pm_runtime_use_autosuspend(&pdev->dev);
+   pm_runtime_set_active(&pdev->dev);
+   pm_runtime_enable(&pdev->dev);
+
+   ret = pm_runtime_get(&pdev->dev);
+   if (ret < 0)
+   

RE: [Patch V4] i2c: imx: add runtime pm support to improve the performance

[Gao Pandy]

From: Heiner Kallweit  Sent: Wednesday, August 26, 
2015 3:53 AM
> To: Gao Pan-B54642; w...@the-dreams.de
> Cc: linux-i2c@vger.kernel.org; Li Frank-B20596; Duan Fugang-B38611;
> u.kleine-koe...@pengutronix.de; ker...@pengutronix.de
> Subject: Re: [Patch V4] i2c: imx: add runtime pm support to improve the
> performance
> 
> Am 25.08.2015 um 13:09 schrieb Gao Pandy:
> > From: Heiner Kallweit  Sent: Tuesday,
> > August 25, 2015 2:37 PM
> >> To: Gao Pan-B54642; w...@the-dreams.de
> >> Cc: linux-i2c@vger.kernel.org; Li Frank-B20596; Duan Fugang-B38611;
> >> u.kleine-koe...@pengutronix.de; ker...@pengutronix.de
> >> Subject: Re: [Patch V4] i2c: imx: add runtime pm support to improve
> >> the performance
> >>
> >> Am 25.08.2015 um 04:20 schrieb Gao Pan:
> >>> 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 consumption and i2c bus performance, runtime pm
> >>> is the good solution for it. The clk is enabled when a i2c transfer
> >>> starts, and disabled after 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
> >>>
> >>> Signed-off-by: Fugang Duan 
> >>> Signed-off-by: Gao Pan 
> >>> [wsa: fixed some indentation]
> >>> Signed-off-by: Wolfram Sang 
> >>> ---
> >>> V2:
> >>> As Uwe Kleine-König's suggestion, the version do below changes:
> >>>  -call clk_prepare_enable in probe to avoid never enabling clock
> >>>   if CONFIG_PM is disabled
> >>>  -enable clock before request IRQ in probe  -remove the pm staff in
> >>> i2c_imx_isr
> >>>
> >>> V3:
> >>>  -pm_runtime_get_sync returns < 0 as error
> >>>
> >>> V4:
> >>>  -add pm_runtime_set_active before pm_runtime_enable  -replace
> >>> pm_runtime_put_autosuspend with pm_runtime_autosuspend
> >>>   in probe
> >>>  -add error disposal when i2c_add_numbered_adapter fails
> >>>
> >>>  drivers/i2c/busses/i2c-imx.c | 79
> >>> ++--
> >>>  1 file changed, 68 insertions(+), 11 deletions(-)
> >>>
> >>> diff --git a/drivers/i2c/busses/i2c-imx.c
> >>> b/drivers/i2c/busses/i2c-imx.c index 785aa67..7c63047 100644
> >>> --- a/drivers/i2c/busses/i2c-imx.c
> >>> +++ b/drivers/i2c/busses/i2c-imx.c
> >>> @@ -53,6 +53,7 @@
> >>>  #include 
> >>>  #include 
> >>>  #include 
> >>> +#include 
> >>>
> >>>  /** Defines
> >>> 
> >>>
> >>> 
> >>> **
> >>> */
> >>> @@ -118,6 +119,8 @@
> >>>  #define I2CR_IEN_OPCODE_00x0
> >>>  #define I2CR_IEN_OPCODE_1I2CR_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) @@ -894,6
> >>> +893,10 @@ static int i2c_imx_xfer(struct i2c_adapter *adapter,
> >>>
> >>>   dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
> >>>
> >>> + result = pm_runtime_get_sync(i2c_imx->adapter.dev.parent);
> >>> + if (result < 0)
> >>> + goto out;
> >>> +
> >>>   /* Start I2C transfer */
> >>>   result = i2c_imx_start(i2c_imx);
> >>>   if (result)
> >>> @@ -950,6 +953,10 @@ fail0:
> >>>   /* Stop I2C transfer */
> >>>   i2c_imx_stop(i2c_imx);
> >>>
> >>> +out:
> >>> + pm_runtime_mark_last_busy(i2c_imx->adapter.dev.parent);
> >>> + pm_runtime_put_autosuspend(i2c_imx->adapter.dev.parent);
> >>> +
> >>>   dev_dbg(&i2c_imx->adapter.dev, "<%s> exit with: %s: %d\n", __func__,
> >>>   (result < 0) ? "error" : "success msg",
> >>>   (result < 0) ? result : num);
> >>> @@ -1020,9 +1027,10 @@ static int i

Re: [PATCH] drivers: i2c: exynos5: irq spinlock rt-safe

[Thomas Gleixner]

On Tue, 25 Aug 2015, Anders Roxell wrote:

> The exynos5_i2c_message_start enables interrupts while holding the i2c
> lock which is sought by the irq handler. If an IRQ is received before
> this lock is released then a deadlock occurs.

That's crap. The interrupt handler runs in an irq thread as RT forces
all normal interrupts to that.

Thanks,

tglx

 
--
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 V4] i2c: imx: add runtime pm support to improve the performance

[Heiner Kallweit]

Am 25.08.2015 um 13:09 schrieb Gao Pandy:
> From: Heiner Kallweit  Sent: Tuesday, August 25, 
> 2015 2:37 PM
>> To: Gao Pan-B54642; w...@the-dreams.de
>> Cc: linux-i2c@vger.kernel.org; Li Frank-B20596; Duan Fugang-B38611;
>> u.kleine-koe...@pengutronix.de; ker...@pengutronix.de
>> Subject: Re: [Patch V4] i2c: imx: add runtime pm support to improve the
>> performance
>>
>> Am 25.08.2015 um 04:20 schrieb Gao Pan:
>>> 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 consumption and i2c bus performance, runtime pm is
>>> the good solution for it. The clk is enabled when a i2c transfer
>>> starts, and disabled after 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
>>>
>>> Signed-off-by: Fugang Duan 
>>> Signed-off-by: Gao Pan 
>>> [wsa: fixed some indentation]
>>> Signed-off-by: Wolfram Sang 
>>> ---
>>> V2:
>>> As Uwe Kleine-König's suggestion, the version do below changes:
>>>  -call clk_prepare_enable in probe to avoid never enabling clock
>>>   if CONFIG_PM is disabled
>>>  -enable clock before request IRQ in probe  -remove the pm staff in
>>> i2c_imx_isr
>>>
>>> V3:
>>>  -pm_runtime_get_sync returns < 0 as error
>>>
>>> V4:
>>>  -add pm_runtime_set_active before pm_runtime_enable  -replace
>>> pm_runtime_put_autosuspend with pm_runtime_autosuspend
>>>   in probe
>>>  -add error disposal when i2c_add_numbered_adapter fails
>>>
>>>  drivers/i2c/busses/i2c-imx.c | 79
>>> ++--
>>>  1 file changed, 68 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/i2c/busses/i2c-imx.c
>>> b/drivers/i2c/busses/i2c-imx.c index 785aa67..7c63047 100644
>>> --- a/drivers/i2c/busses/i2c-imx.c
>>> +++ b/drivers/i2c/busses/i2c-imx.c
>>> @@ -53,6 +53,7 @@
>>>  #include 
>>>  #include 
>>>  #include 
>>> +#include 
>>>
>>>  /** 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) @@ -894,6
>>> +893,10 @@ static int i2c_imx_xfer(struct i2c_adapter *adapter,
>>>
>>> dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
>>>
>>> +   result = pm_runtime_get_sync(i2c_imx->adapter.dev.parent);
>>> +   if (result < 0)
>>> +   goto out;
>>> +
>>> /* Start I2C transfer */
>>> result = i2c_imx_start(i2c_imx);
>>> if (result)
>>> @@ -950,6 +953,10 @@ fail0:
>>> /* Stop I2C transfer */
>>> i2c_imx_stop(i2c_imx);
>>>
>>> +out:
>>> +   pm_runtime_mark_last_busy(i2c_imx->adapter.dev.parent);
>>> +   pm_runtime_put_autosuspend(i2c_imx->adapter.dev.parent);
>>> +
>>> dev_dbg(&i2c_imx->adapter.dev, "<%s> exit with: %s: %d\n", __func__,
>>> (result < 0) ? "error" : "success msg",
>>> (result < 0) ? result : num);
>>> @@ -1020,9 +1027,10 @@ static int i2c_imx_probe(struct platform_device
>>> *pdev)
>>>
>>> ret = clk_prepare_enable(i2c_imx->clk);
>>> if (ret) {
>>> -   dev_err(&pdev->dev, "can't enable I2C clock\n");
>>> +   dev_err(&pdev->dev, "can't enable I2C clock, ret=%d\n", ret);
>>> return ret;
>>> }
>>> +
>>> /* Request IRQ */
>>> ret = devm_request_irq(&pdev->dev, irq, i2c_imx_isr, 0,
>>> pdev->name, i2c_imx);
>>> @@ -1037,6 +1045,14 @@ static int i2c_imx_probe(struct platform_device
>> *pdev)
>>> /* Set up adapter data */
>>> i

Re: [RFC] i2c: Revert back to old device naming for ACPI enumerated I2C slaves

[Guenter Roeck]

On 08/25/2015 09:18 AM, Wolfram Sang wrote:

On Tue, Aug 25, 2015 at 08:18:30AM -0700, Guenter Roeck wrote:

On 08/25/2015 07:57 AM, Wolfram Sang wrote:

On Tue, Aug 25, 2015 at 06:25:13AM +0100, Mark Brown wrote:

On Mon, Aug 24, 2015 at 01:52:02PM +0300, Jarkko Nikula wrote:

Commit 70762abb9f89 ("i2c: Use stable dev_name for ACPI enumerated I2C
slaves") broke the lm-sensors which relies on I2C hwmon slave devices under
/sys/bus/i2c/devices/ to be named as "x-00yz". However if those hwmon
devices are ACPI 5 enumerated their name became "i2c-INTABCD:ij" and sysfs
code in lm-sensors does not find them anymore:


Acked-by: Mark Brown 


Don't you think there will be regressions given that the new naming
scheme was around for 18 months?



acpi is pretty long term. New bindings don't show up quickly.


The key is, there might be userspace applications that got used to our
new "export" of ACPI bindings. The bindings were already existing, our
"export" did change.


So I am not surprised that this only shows up now.


I am, to be honest. It shows running lm-sensors with ACPI on a kernel
newer than 18 months. Not a rare scenario, so I thought.


With i2c (sensor) devices enumerated in acpi ? Yes, I think that is quite rare.
This is not about the kernel, it is about such device bindings showing up
in acpi data.

Guenter

--
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: [RFC] i2c: Revert back to old device naming for ACPI enumerated I2C slaves

[Wolfram Sang]

> > So I am not surprised that this only shows up now.
> 
> I am, to be honest. It shows running lm-sensors with ACPI on a kernel
> newer than 18 months. Not a rare scenario, so I thought.

And thanks to Mark's recent post I understand now that this is only
relevant for ACPI5 enumerated devices. I lost that detail.



signature.asc
Description: Digital signature

Re: [RFC] i2c: Revert back to old device naming for ACPI enumerated I2C slaves

[Wolfram Sang]

On Tue, Aug 25, 2015 at 08:18:30AM -0700, Guenter Roeck wrote:
> On 08/25/2015 07:57 AM, Wolfram Sang wrote:
> >On Tue, Aug 25, 2015 at 06:25:13AM +0100, Mark Brown wrote:
> >>On Mon, Aug 24, 2015 at 01:52:02PM +0300, Jarkko Nikula wrote:
> >>>Commit 70762abb9f89 ("i2c: Use stable dev_name for ACPI enumerated I2C
> >>>slaves") broke the lm-sensors which relies on I2C hwmon slave devices under
> >>>/sys/bus/i2c/devices/ to be named as "x-00yz". However if those hwmon
> >>>devices are ACPI 5 enumerated their name became "i2c-INTABCD:ij" and sysfs
> >>>code in lm-sensors does not find them anymore:
> >>
> >>Acked-by: Mark Brown 
> >
> >Don't you think there will be regressions given that the new naming
> >scheme was around for 18 months?
> >
> 
> acpi is pretty long term. New bindings don't show up quickly.

The key is, there might be userspace applications that got used to our
new "export" of ACPI bindings. The bindings were already existing, our
"export" did change.

> So I am not surprised that this only shows up now.

I am, to be honest. It shows running lm-sensors with ACPI on a kernel
newer than 18 months. Not a rare scenario, so I thought.

> Will there be regressions ? Who knows. What we do know is
> that there are regressions today due to the original change.

Because we didn't pay enough attention. I wouldn't like to do the same
mistake again. Or?



signature.asc
Description: Digital signature

Re: [RFC] i2c: Revert back to old device naming for ACPI enumerated I2C slaves

[Mark Brown]

On Tue, Aug 25, 2015 at 04:57:56PM +0200, Wolfram Sang wrote:
> On Tue, Aug 25, 2015 at 06:25:13AM +0100, Mark Brown wrote:
> > On Mon, Aug 24, 2015 at 01:52:02PM +0300, Jarkko Nikula wrote:

> > > Commit 70762abb9f89 ("i2c: Use stable dev_name for ACPI enumerated I2C
> > > slaves") broke the lm-sensors which relies on I2C hwmon slave devices 
> > > under
> > > /sys/bus/i2c/devices/ to be named as "x-00yz". However if those hwmon
> > > devices are ACPI 5 enumerated their name became "i2c-INTABCD:ij" and sysfs
> > > code in lm-sensors does not find them anymore:

> > Acked-by: Mark Brown 

> Don't you think there will be regressions given that the new naming
> scheme was around for 18 months?

That's a "whatever" from the ASoC point of view.  I don't particularly
care about the userspace ABI, on the one hand there aren't that many
ACPI 5 devices that might be affected and there's other devices that
won't work anyway.  On the other hand I guess there's other devices that
would have worked with the old kernel.


signature.asc
Description: Digital signature

Re: [RFC] i2c: Revert back to old device naming for ACPI enumerated I2C slaves

[Guenter Roeck]

On 08/25/2015 07:57 AM, Wolfram Sang wrote:

On Tue, Aug 25, 2015 at 06:25:13AM +0100, Mark Brown wrote:

On Mon, Aug 24, 2015 at 01:52:02PM +0300, Jarkko Nikula wrote:

Commit 70762abb9f89 ("i2c: Use stable dev_name for ACPI enumerated I2C
slaves") broke the lm-sensors which relies on I2C hwmon slave devices under
/sys/bus/i2c/devices/ to be named as "x-00yz". However if those hwmon
devices are ACPI 5 enumerated their name became "i2c-INTABCD:ij" and sysfs
code in lm-sensors does not find them anymore:


Acked-by: Mark Brown 


Don't you think there will be regressions given that the new naming
scheme was around for 18 months?



acpi is pretty long term. New bindings don't show up quickly.
So I am not surprised that this only shows up now.

Will there be regressions ? Who knows. What we do know is
that there are regressions today due to the original change.

Guenter


--
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: [RFC] i2c: Revert back to old device naming for ACPI enumerated I2C slaves

[Wolfram Sang]

> In this particular case it looks like we need a mechanism to use the old
> naming scheme for hwmon and the new one for everything else or something
> similar.  I'm afraid I am not familiar enough with the i2c core to suggest
> anything specific ATM, though.

Well, it boils down to directory names in sysfs. So, to support both
naming schemes, I'd simply suggest symlinks.



signature.asc
Description: Digital signature

Re: [RFC] i2c: Revert back to old device naming for ACPI enumerated I2C slaves

[Wolfram Sang]

On Tue, Aug 25, 2015 at 06:25:13AM +0100, Mark Brown wrote:
> On Mon, Aug 24, 2015 at 01:52:02PM +0300, Jarkko Nikula wrote:
> > Commit 70762abb9f89 ("i2c: Use stable dev_name for ACPI enumerated I2C
> > slaves") broke the lm-sensors which relies on I2C hwmon slave devices under
> > /sys/bus/i2c/devices/ to be named as "x-00yz". However if those hwmon
> > devices are ACPI 5 enumerated their name became "i2c-INTABCD:ij" and sysfs
> > code in lm-sensors does not find them anymore:
> 
> Acked-by: Mark Brown 

Don't you think there will be regressions given that the new naming
scheme was around for 18 months?



signature.asc
Description: Digital signature

Re: [RFC] i2c: Revert back to old device naming for ACPI enumerated I2C slaves

[Jarkko Nikula]

On 08/25/2015 08:03 AM, Dustin Byford wrote:

On Mon Aug 24 13:52, Jarkko Nikula wrote:


I don't know how common ACPI enumerated I2C hwmon devices are but I guess
they exists since Dustin notices this issue and wrote "With that change,
/sys/bus/i2c/devices/i2c-0-004a, for example, became
/sys/bus/i2c/devices/i2c-PNP:xx".


I wouldn't take my particular use case as evidence they are common, or
even they really exist.  I got here because I loaded a hwmon driver
using the _DSD/PRP0001 mechanism in ACPI.  In that case, those devices
have never worked with 'sensors'  Therefore, in that context, this
change is just fixing a bug.

So does this then fall more into new use case rather than being a 
regression as such?


What I'm trying to understand what kind of regression we actually do 
have here? Is it about listing the devices or does it also cease to show 
for instance temperature sensor readings?


If it requires to add a match table to existing driver in order to 
trigger this then at least stable tree is not worry here.


--
Jarkko
--
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: imx: implement bus recovery

[Linus Walleij]

On Wed, Aug 19, 2015 at 9:02 AM, Uwe Kleine-König
 wrote:
> On Wed, Aug 19, 2015 at 03:44:49AM +, Gao Pandy wrote:
>> From: Uwe Kleine-König  Sent: 
>> Thursday, August 13, 2015 4:15 PM
>> > > +static void i2c_imx_prepare_recovery(struct i2c_adapter *adap) {
>> > > + struct imx_i2c_struct *i2c_imx;
>> > > +
>> > > + i2c_imx = container_of(adap, struct imx_i2c_struct, adapter);
>> > > + if (i2c_imx->pins.sda && i2c_imx->pins.scl) {
>> > > + pinctrl_pm_select_sleep_state(&adap->dev);
>> >
>> > Your requirement that the sleep state should configure the pins as gpio
>> > is strange. Maybe better introduce a dedicated state for recovery? At
>> > least you should document this requirement.
>>
>> In general, pinctrl sleep mode is gpio function. I will add this in binding 
>> doc. Thanks.
>
> Linus, do you have to say something here? It might be right to have the
> gpio function as configuration for sleep mode, but it doesn't look right
> for me to use this for recovery purposes.

What it usually means is that the pin has a function mode such
that an asynchronous edge detector is connected to it on the
outer pad ring, maybe in tristate or some pull-up/down configuration.
This makes is possible for the system to power
off completely until an event occurs there with only some
very peripheral electronics powered up.

I think the terminology depend on the use case. See the section
"GPIO mode pitfalls" in Documentation/pinctrl.txt

If the use case is around the i2c traffic, it is a mode related to I2C,
and if this mode is called "GPIO mode" in the data sheet
is irrelevant, because it is obviously not used for the generic
input/output but the specific I2C. The terminology should be
made familiar to whoever needs to go in and read the code later.

Yours,
Linus Walleij
--
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 V4] i2c: imx: add runtime pm support to improve the performance

[Gao Pandy]

> From: linux-i2c-ow...@vger.kernel.org [mailto:linux-i2c-
> ow...@vger.kernel.org] On Behalf Of Gao Pandy
> Sent: Tuesday, August 25, 2015 7:09 PM
> To: Heiner Kallweit; w...@the-dreams.de
> Cc: linux-i2c@vger.kernel.org; Li Frank-B20596; Duan Fugang-B38611;
> u.kleine-koe...@pengutronix.de; ker...@pengutronix.de
> Subject: RE: [Patch V4] i2c: imx: add runtime pm support to improve the
> performance
> 
> From: Heiner Kallweit  Sent: Tuesday, August
> 25, 2015 2:37 PM
> > To: Gao Pan-B54642; w...@the-dreams.de
> > Cc: linux-i2c@vger.kernel.org; Li Frank-B20596; Duan Fugang-B38611;
> > u.kleine-koe...@pengutronix.de; ker...@pengutronix.de
> > Subject: Re: [Patch V4] i2c: imx: add runtime pm support to improve
> > the performance
> >
> > Am 25.08.2015 um 04:20 schrieb Gao Pan:
> > > 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 consumption and i2c bus performance, runtime pm
> > > is the good solution for it. The clk is enabled when a i2c transfer
> > > starts, and disabled after 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
> > >
> > > Signed-off-by: Fugang Duan 
> > > Signed-off-by: Gao Pan 
> > > [wsa: fixed some indentation]
> > > Signed-off-by: Wolfram Sang 
> > > ---
> > > V2:
> > > As Uwe Kleine-König's suggestion, the version do below changes:
> > >  -call clk_prepare_enable in probe to avoid never enabling clock
> > >   if CONFIG_PM is disabled
> > >  -enable clock before request IRQ in probe  -remove the pm staff in
> > > i2c_imx_isr
> > >
> > > V3:
> > >  -pm_runtime_get_sync returns < 0 as error
> > >
> > > V4:
> > >  -add pm_runtime_set_active before pm_runtime_enable  -replace
> > > pm_runtime_put_autosuspend with pm_runtime_autosuspend
> > >   in probe
> > >  -add error disposal when i2c_add_numbered_adapter fails
> > >
> > >  drivers/i2c/busses/i2c-imx.c | 79
> > > ++--
> > >  1 file changed, 68 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/drivers/i2c/busses/i2c-imx.c
> > > b/drivers/i2c/busses/i2c-imx.c index 785aa67..7c63047 100644
> > > --- a/drivers/i2c/busses/i2c-imx.c
> > > +++ b/drivers/i2c/busses/i2c-imx.c
> > > @@ -53,6 +53,7 @@
> > >  #include 
> > >  #include 
> > >  #include 
> > > +#include 
> > >
> > >  /** Defines
> > > 
> > >
> > > 
> > > **
> > > */
> > > @@ -118,6 +119,8 @@
> > >  #define I2CR_IEN_OPCODE_00x0
> > >  #define I2CR_IEN_OPCODE_1I2CR_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) @@ -894,6
> > > +893,10 @@ static int i2c_imx_xfer(struct i2c_adapter *adapter,
> > >
> > >   dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
> > >
> > > + result = pm_runtime_get_sync(i2c_imx->adapter.dev.parent);
> > > + if (result < 0)
> > > + goto out;
> > > +
> > >   /* Start I2C transfer */
> > >   result = i2c_imx_start(i2c_imx);
> > >   if (result)
> > > @@ -950,6 +953,10 @@ fail0:
> > >   /* Stop I2C transfer */
> > >   i2c_imx_stop(i2c_imx);
> > >
> > > +out:
> > > + pm_runtime_mark_last_busy(i2c_imx->adapter.dev.parent);
> > > + pm_runtime_put_autosuspend(i2c_imx->adapter.dev.parent);
> > > +
> > >   dev_dbg(&i2c_imx->adapter.dev, "<%s> exit with: %s: %d\n", __func__,
> > >   (result < 0) ? "error" : "success msg",
> > >   (result < 0) ? result : num);
> > > @@ -1020,9 +1027,10 @@ static int i2c_

RE: [Patch V4] i2c: imx: add runtime pm support to improve the performance

[Gao Pandy]

From: Heiner Kallweit  Sent: Tuesday, August 25, 
2015 2:37 PM
> To: Gao Pan-B54642; w...@the-dreams.de
> Cc: linux-i2c@vger.kernel.org; Li Frank-B20596; Duan Fugang-B38611;
> u.kleine-koe...@pengutronix.de; ker...@pengutronix.de
> Subject: Re: [Patch V4] i2c: imx: add runtime pm support to improve the
> performance
> 
> Am 25.08.2015 um 04:20 schrieb Gao Pan:
> > 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 consumption and i2c bus performance, runtime pm is
> > the good solution for it. The clk is enabled when a i2c transfer
> > starts, and disabled after 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
> >
> > Signed-off-by: Fugang Duan 
> > Signed-off-by: Gao Pan 
> > [wsa: fixed some indentation]
> > Signed-off-by: Wolfram Sang 
> > ---
> > V2:
> > As Uwe Kleine-König's suggestion, the version do below changes:
> >  -call clk_prepare_enable in probe to avoid never enabling clock
> >   if CONFIG_PM is disabled
> >  -enable clock before request IRQ in probe  -remove the pm staff in
> > i2c_imx_isr
> >
> > V3:
> >  -pm_runtime_get_sync returns < 0 as error
> >
> > V4:
> >  -add pm_runtime_set_active before pm_runtime_enable  -replace
> > pm_runtime_put_autosuspend with pm_runtime_autosuspend
> >   in probe
> >  -add error disposal when i2c_add_numbered_adapter fails
> >
> >  drivers/i2c/busses/i2c-imx.c | 79
> > ++--
> >  1 file changed, 68 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-imx.c
> > b/drivers/i2c/busses/i2c-imx.c index 785aa67..7c63047 100644
> > --- a/drivers/i2c/busses/i2c-imx.c
> > +++ b/drivers/i2c/busses/i2c-imx.c
> > @@ -53,6 +53,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >
> >  /** 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) @@ -894,6
> > +893,10 @@ static int i2c_imx_xfer(struct i2c_adapter *adapter,
> >
> > dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
> >
> > +   result = pm_runtime_get_sync(i2c_imx->adapter.dev.parent);
> > +   if (result < 0)
> > +   goto out;
> > +
> > /* Start I2C transfer */
> > result = i2c_imx_start(i2c_imx);
> > if (result)
> > @@ -950,6 +953,10 @@ fail0:
> > /* Stop I2C transfer */
> > i2c_imx_stop(i2c_imx);
> >
> > +out:
> > +   pm_runtime_mark_last_busy(i2c_imx->adapter.dev.parent);
> > +   pm_runtime_put_autosuspend(i2c_imx->adapter.dev.parent);
> > +
> > dev_dbg(&i2c_imx->adapter.dev, "<%s> exit with: %s: %d\n", __func__,
> > (result < 0) ? "error" : "success msg",
> > (result < 0) ? result : num);
> > @@ -1020,9 +1027,10 @@ static int i2c_imx_probe(struct platform_device
> > *pdev)
> >
> > ret = clk_prepare_enable(i2c_imx->clk);
> > if (ret) {
> > -   dev_err(&pdev->dev, "can't enable I2C clock\n");
> > +   dev_err(&pdev->dev, "can't enable I2C clock, ret=%d\n", ret);
> > return ret;
> > }
> > +
> > /* Request IRQ */
> > ret = devm_request_irq(&pdev->dev, irq, i2c_imx_isr, 0,
> > pdev->name, i2c_imx);
> > @@ -1037,6 +1045,14 @@ static int i2c_imx_probe(struct platform_device
> *pdev)
> > /* Set up adapter data */
> > i2c_set_adapdata(&i2c_imx->adapter, i2c_imx);
> >
> > +  

[PATCH 0/4] Support multiplexed main SMBus interface on SB800

[Christian Fetzer]

This is an attempt to upstream the patches created by Thomas Brandon and
Eddi De Pieri to support the multiplexed main SMBus interface on the SB800
chipset. (https://www.mail-archive.com/linux-i2c@vger.kernel.org/msg06757.html)

I have mainly rebased the latest patch version and tested the driver on a
HP ProLiant MicroServer G7 N54L (where this patch allows to access sensor data
from a w83795adg).

The patched driver is running stable on the machine, given that ic2_piix4 is
loaded before jc42 and w83795. If jc42 is loaded before i2c_piix4 calling
sensors triggers some errors:
ERROR: Can't get value of subfeature temp1_min_alarm: Can't read

While the kernel log shows:
i2c i2c-1: Transaction (pre): CNT=0c, CMD=05, ADD=31, DAT0=03, DAT1=c0
i2c i2c-1: Error: no response!
i2c i2c-1: Transaction (post): CNT=0c, CMD=05, ADD=31, DAT0=ff, DAT1=ff
Unfortunately I don't know how to tackle this specific issue.

Please review and let me know required changes in order to get this upstream
finally.

Eddi, Thomas, it would be great if you could verify the changes on your
machines.

Regards,
Christian

Christian Fetzer (4):
  i2c-piix4: Optionally release smba in piix4_adap_remove
  i2c-piix4: Convert piix4_main_adapter to array
  i2c-piix4: Add support for multiplexed main adapter in SB800
  i2c-piix4: Add adapter port name support for SB800 chipset

 drivers/i2c/busses/i2c-piix4.c | 151 -
 1 file changed, 134 insertions(+), 17 deletions(-)

-- 
1.9.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/4] i2c-piix4: Add adapter port name support for SB800 chipset

[Christian Fetzer]

This patch adds support for port names for the SB800 chipset.
Since the chipset supports a multiplexed main SMBus controller, adding
the channel name to the adapter name is necessary to differentiate the
ports better (for example in sensors output).

Signed-off-by: Christian Fetzer 
---
 drivers/i2c/busses/i2c-piix4.c | 18 +-
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index e344abd..366878e 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -131,6 +131,10 @@ static const struct dmi_system_id piix4_dmi_ibm[] = {
 
 /* SB800 globals */
 DEFINE_MUTEX(piix4_mutex_sb800);
+static const char *piix4_main_port_names_sb800[4] = {
+   "SDA0", "SDA2", "SDA3", "SDA4"
+};
+static const char *piix4_aux_port_name_sb800 = "SDA1";
 
 struct i2c_piix4_adapdata {
unsigned short smba;
@@ -626,7 +630,7 @@ static struct i2c_adapter 
*piix4_main_adapters[PIIX4_MAX_ADAPTERS];
 static struct i2c_adapter *piix4_aux_adapter;
 
 static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
-struct i2c_adapter **padap)
+const char *name, struct i2c_adapter **padap)
 {
struct i2c_adapter *adap;
struct i2c_piix4_adapdata *adapdata;
@@ -655,7 +659,7 @@ static int piix4_add_adapter(struct pci_dev *dev, unsigned 
short smba,
adap->dev.parent = &dev->dev;
 
snprintf(adap->name, sizeof(adap->name),
-   "SMBus PIIX4 adapter at %04x", smba);
+   "SMBus PIIX4 adapter %s at %04x", name, smba);
 
i2c_set_adapdata(adap, adapdata);
 
@@ -679,7 +683,9 @@ static int piix4_add_adapters_sb800(struct pci_dev *dev, 
unsigned short smba)
struct i2c_piix4_adapdata *adapdata;
 
for (port = 0; port < PIIX4_MAX_ADAPTERS; port++) {
-   retval = piix4_add_adapter(dev, smba, 
&piix4_main_adapters[port]);
+   retval = piix4_add_adapter(dev, smba,
+  piix4_main_port_names_sb800[port],
+  &piix4_main_adapters[port]);
if (retval < 0)
goto ERROR;
 
@@ -729,7 +735,8 @@ static int piix4_probe(struct pci_dev *dev, const struct 
pci_device_id *id)
return retval;
 
/* Try to register main SMBus adapter, give up if we can't */
-   retval = piix4_add_adapter(dev, retval, 
&piix4_main_adapters[0]);
+   retval = piix4_add_adapter(dev, retval, "",
+  &piix4_main_adapters[0]);
}
 
/* If no main SMBus found, give up */
@@ -757,7 +764,8 @@ static int piix4_probe(struct pci_dev *dev, const struct 
pci_device_id *id)
if (retval > 0) {
/* Try to add the aux adapter if it exists,
 * piix4_add_adapter will clean up if this fails */
-   piix4_add_adapter(dev, retval, &piix4_aux_adapter);
+   piix4_add_adapter(dev, retval, piix4_aux_port_name_sb800,
+ &piix4_aux_adapter);
}
 
return 0;
-- 
1.9.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/4] i2c-piix4: Optionally release smba in piix4_adap_remove

[Christian Fetzer]

This is in preparation to support the multiplexed SMBus main controller
in the SB800 chipset where the controller address is shared among
the four multiplexed ports. As such the address region should be
only freed for the first multiplexed adapter to avoid double free
warnings.

Signed-off-by: Christian Fetzer 
---
 drivers/i2c/busses/i2c-piix4.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index 630bce6..97d2165 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -660,13 +660,14 @@ static int piix4_probe(struct pci_dev *dev, const struct 
pci_device_id *id)
return 0;
 }
 
-static void piix4_adap_remove(struct i2c_adapter *adap)
+static void piix4_adap_remove(struct i2c_adapter *adap, int free_smba)
 {
struct i2c_piix4_adapdata *adapdata = i2c_get_adapdata(adap);
 
if (adapdata->smba) {
i2c_del_adapter(adap);
-   release_region(adapdata->smba, SMBIOSIZE);
+   if (free_smba)
+   release_region(adapdata->smba, SMBIOSIZE);
kfree(adapdata);
kfree(adap);
}
@@ -675,12 +676,12 @@ static void piix4_adap_remove(struct i2c_adapter *adap)
 static void piix4_remove(struct pci_dev *dev)
 {
if (piix4_main_adapter) {
-   piix4_adap_remove(piix4_main_adapter);
+   piix4_adap_remove(piix4_main_adapter, 1);
piix4_main_adapter = NULL;
}
 
if (piix4_aux_adapter) {
-   piix4_adap_remove(piix4_aux_adapter);
+   piix4_adap_remove(piix4_aux_adapter, 1);
piix4_aux_adapter = NULL;
}
 }
-- 
1.9.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/4] i2c-piix4: Convert piix4_main_adapter to array

[Christian Fetzer]

The SB800 chipset supports a multiplexed main SMBus controller with
four ports. Therefore the static variable piix4_main_adapter is
converted into a piix4_main_adapters array that can hold one
i2c_adapter for each multiplexed port.

The auxiliary adapter remains unchanged since it represents the second
(not multiplexed) SMBus controller on the SB800 chipset.

Signed-off-by: Christian Fetzer 
---
 drivers/i2c/busses/i2c-piix4.c | 18 +-
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index 97d2165..8934a32 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -75,6 +75,9 @@
 #define PIIX4_WORD_DATA0x0C
 #define PIIX4_BLOCK_DATA   0x14
 
+/* Multi-port constants */
+#define PIIX4_MAX_ADAPTERS 4
+
 /* insmod parameters */
 
 /* If force is set to anything different from 0, we forcibly enable the
@@ -561,7 +564,7 @@ static const struct pci_device_id piix4_ids[] = {
 
 MODULE_DEVICE_TABLE (pci, piix4_ids);
 
-static struct i2c_adapter *piix4_main_adapter;
+static struct i2c_adapter *piix4_main_adapters[PIIX4_MAX_ADAPTERS];
 static struct i2c_adapter *piix4_aux_adapter;
 
 static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
@@ -629,7 +632,7 @@ static int piix4_probe(struct pci_dev *dev, const struct 
pci_device_id *id)
return retval;
 
/* Try to register main SMBus adapter, give up if we can't */
-   retval = piix4_add_adapter(dev, retval, &piix4_main_adapter);
+   retval = piix4_add_adapter(dev, retval, &piix4_main_adapters[0]);
if (retval < 0)
return retval;
 
@@ -675,9 +678,14 @@ static void piix4_adap_remove(struct i2c_adapter *adap, 
int free_smba)
 
 static void piix4_remove(struct pci_dev *dev)
 {
-   if (piix4_main_adapter) {
-   piix4_adap_remove(piix4_main_adapter, 1);
-   piix4_main_adapter = NULL;
+   int port;
+
+   for (port = 0; port < PIIX4_MAX_ADAPTERS; port++) {
+   if (piix4_main_adapters[port]) {
+   piix4_adap_remove(piix4_main_adapters[port],
+ port == 0);
+   piix4_main_adapters[port] = NULL;
+   }
}
 
if (piix4_aux_adapter) {
-- 
1.9.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 3/4] i2c-piix4: Add support for multiplexed main adapter in SB800

[Christian Fetzer]

The SB800 chipset supports a multiplexed main SMBus controller with
four ports. The multiplexed ports share the same SMBus address and
register set. The port is selected by bits 2:1 of the smb_en register
(0x2C).

Only one port can be active at any point in time therefore a mutex is
needed in order to synchronize access.

Tested on HP ProLiant MicroServer G7 N54L (where this patch adds
support to access sensor data from the w83795adg).

Cc: Thomas Brandon 
Cc: Eddi De Pieri 
Signed-off-by: Christian Fetzer 
---
 drivers/i2c/busses/i2c-piix4.c | 114 ++---
 1 file changed, 107 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index 8934a32..e344abd 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -23,6 +23,9 @@
 
Note: we assume there can only be one device, with one or more
SMBus interfaces.
+   The device can register multiple i2c_adapters (up to PIIX4_MAX_ADAPTERS).
+   For devices supporting multiple ports the i2c_adapter should provide
+   an i2c_algorithm to access them.
 */
 
 #include 
@@ -37,6 +40,7 @@
 #include 
 #include 
 #include 
+#include 
 
 
 /* PIIX4 SMBus address offsets */
@@ -125,8 +129,12 @@ static const struct dmi_system_id piix4_dmi_ibm[] = {
{ },
 };
 
+/* SB800 globals */
+DEFINE_MUTEX(piix4_mutex_sb800);
+
 struct i2c_piix4_adapdata {
unsigned short smba;
+   unsigned short port;
 };
 
 static int piix4_setup(struct pci_dev *PIIX4_dev,
@@ -313,6 +321,8 @@ static int piix4_setup_sb800(struct pci_dev *PIIX4_dev,
else
dev_dbg(&PIIX4_dev->dev, "Using SMI# for SMBus\n");
 
+   mutex_init(&piix4_mutex_sb800);
+
dev_info(&PIIX4_dev->dev,
 "SMBus Host Controller at 0x%x, revision %d\n",
 piix4_smba, i2ccfg >> 4);
@@ -527,6 +537,49 @@ static s32 piix4_access(struct i2c_adapter * adap, u16 
addr,
return 0;
 }
 
+/* Handles access to multiple SMBus ports on the SB800.
+ * The port is selected by bits 2:1 of the smb_en register (0x2C).
+ * NOTE: The selected port must be returned to the initial selection to
+ * avoid problems on certain systems.
+ * Return negative errno on error.
+ */
+static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr,
+unsigned short flags, char read_write,
+u8 command, int size, union i2c_smbus_data *data)
+{
+   struct i2c_piix4_adapdata *adapdata = i2c_get_adapdata(adap);
+   unsigned short smba_idx = 0xcd6;
+   u8 smba_en_lo, smb_en = 0x2c;
+   u8 port;
+   int retval;
+
+   mutex_lock(&piix4_mutex_sb800);
+
+   if (!request_region(smba_idx, 2, "smba_idx")) {
+   dev_err(&adap->dev, "SMBus base address index region "
+   "0x%x already in use!\n", smba_idx);
+   retval = -EBUSY;
+   goto ERROR;
+   }
+   outb_p(smb_en, smba_idx);
+   smba_en_lo = inb_p(smba_idx + 1);
+
+   port = adapdata->port;
+   if ((smba_en_lo & 6) != (port << 1))
+   outb_p((smba_en_lo & ~6) | (port << 1), smba_idx + 1);
+
+   retval = piix4_access(adap, addr, flags, read_write,
+ command, size, data);
+
+   outb_p(smba_en_lo, smba_idx + 1);
+   release_region(smba_idx, 2);
+
+ERROR:
+   mutex_unlock(&piix4_mutex_sb800);
+
+   return retval;
+}
+
 static u32 piix4_func(struct i2c_adapter *adapter)
 {
return I2C_FUNC_SMBUS_QUICK | I2C_FUNC_SMBUS_BYTE |
@@ -539,6 +592,11 @@ static const struct i2c_algorithm smbus_algorithm = {
.functionality  = piix4_func,
 };
 
+static const struct i2c_algorithm piix4_smbus_algorithm_sb800 = {
+   .smbus_xfer = piix4_access_sb800,
+   .functionality  = piix4_func,
+};
+
 static const struct pci_device_id piix4_ids[] = {
{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82371AB_3) },
{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82443MX_3) },
@@ -614,6 +672,41 @@ static int piix4_add_adapter(struct pci_dev *dev, unsigned 
short smba,
return 0;
 }
 
+static int piix4_add_adapters_sb800(struct pci_dev *dev, unsigned short smba)
+{
+   unsigned short port;
+   int retval;
+   struct i2c_piix4_adapdata *adapdata;
+
+   for (port = 0; port < PIIX4_MAX_ADAPTERS; port++) {
+   retval = piix4_add_adapter(dev, smba, 
&piix4_main_adapters[port]);
+   if (retval < 0)
+   goto ERROR;
+
+   piix4_main_adapters[port]->algo = &piix4_smbus_algorithm_sb800;
+
+   adapdata = i2c_get_adapdata(piix4_main_adapters[port]);
+   adapdata->port = port;
+   }
+
+   return retval;
+
+ERROR:
+   dev_err(&dev->dev, "Error setting up SB800 adapters. "
+   "Unregistering all adapters!\n");
+   for (port--; port >= 0; port--) {
+   adapdata = i2c_get_adapdata(

[PATCH] drivers: i2c: exynos5: irq spinlock rt-safe

[Anders Roxell]

The exynos5_i2c_message_start enables interrupts while holding the i2c
lock which is sought by the irq handler. If an IRQ is received before
this lock is released then a deadlock occurs.

This is only seen on an RT patched kernel, due to the transformation of
spinlocks into sleeping locks. By using raw_spinlocks here the same code
can run in mainline or an RT patched kernel. No change for !RT kenrels

[   10.992238] kernel BUG at ../kernel/locking/rtmutex.c:998!
[   10.992243] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP ARM
[   10.992250] Modules linked in:
[   10.992258] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.1.5-rt5
[   10.992263] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
[   10.992268] task: ed88 ti: ed888000 task.ti: ed888000
[   10.992281] PC is at rt_spin_lock_slowlock+0xa4/0x2ec
[   10.992288] LR is at rt_spin_lock_slowlock+0x54/0x2ec
[   10.992296] pc : []lr : []psr: 6193
[   10.992296] sp : ed889a28  ip : ed880001  fp : 0089
[   10.992300] r10: ed889a28  r9 : c0f55654  r8 : 0060
[   10.992305] r7 : ed88  r6 :   r5 : 0001  r4 : ed9f7288
[   10.992310] r3 : ed88  r2 :   r1 : ed88  r0 : 
[   10.992316] Flags: nZCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM
Segment kernel
...
...
[   10.992662] [] (rt_spin_lock_slowlock) from []
(exynos5_i2c_irq+0x20/0x2b0)
[   10.992678] [] (exynos5_i2c_irq) from []
(handle_irq_event_percpu+0x68/0x158)
[   10.992690] [] (handle_irq_event_percpu) from []
(handle_irq_event+0x68/0xa8)
[   10.992702] [] (handle_irq_event) from []
(handle_fasteoi_irq+0x11c/0x1d4)
[   10.992713] [] (handle_fasteoi_irq) from []
(generic_handle_irq+0x20/0x30)
[   10.992724] [] (generic_handle_irq) from []
(__handle_domain_irq+0x6c/0xe4)
[   10.992734] [] (__handle_domain_irq) from []
(gic_handle_irq+0x2c/0x68)
[   10.992744] [] (gic_handle_irq) from []
(__irq_svc+0x40/0x88)
[   10.992749] Exception stack(0xed889b28 to 0xed889b70)
...
...

Signed-off-by: Anders Roxell 
---
 drivers/i2c/busses/i2c-exynos5.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/busses/i2c-exynos5.c b/drivers/i2c/busses/i2c-exynos5.c
index b29c750..b12e77e 100644
--- a/drivers/i2c/busses/i2c-exynos5.c
+++ b/drivers/i2c/busses/i2c-exynos5.c
@@ -170,7 +170,7 @@ struct exynos5_i2c {
struct device   *dev;
int state;
 
-   spinlock_t  lock;   /* IRQ synchronization */
+   raw_spinlock_t  lock;   /* IRQ synchronization */
 
/*
 * Since the TRANS_DONE bit is cleared on read, and we may read it
@@ -433,7 +433,7 @@ static irqreturn_t exynos5_i2c_irq(int irqno, void *dev_id)
 
i2c->state = -EINVAL;
 
-   spin_lock(&i2c->lock);
+   raw_spin_lock(&i2c->lock);
 
int_status = readl(i2c->regs + HSI2C_INT_STATUS);
writel(int_status, i2c->regs + HSI2C_INT_STATUS);
@@ -521,7 +521,7 @@ static irqreturn_t exynos5_i2c_irq(int irqno, void *dev_id)
complete(&i2c->msg_complete);
}
 
-   spin_unlock(&i2c->lock);
+   raw_spin_unlock(&i2c->lock);
 
return IRQ_HANDLED;
 }
@@ -610,7 +610,7 @@ static void exynos5_i2c_message_start(struct exynos5_i2c 
*i2c, int stop)
 * Enable interrupts before starting the transfer so that we don't
 * miss any INT_I2C interrupts.
 */
-   spin_lock_irqsave(&i2c->lock, flags);
+   raw_spin_lock_irqsave(&i2c->lock, flags);
writel(int_en, i2c->regs + HSI2C_INT_ENABLE);
 
if (stop == 1)
@@ -618,7 +618,7 @@ static void exynos5_i2c_message_start(struct exynos5_i2c 
*i2c, int stop)
i2c_auto_conf |= i2c->msg->len;
i2c_auto_conf |= HSI2C_MASTER_RUN;
writel(i2c_auto_conf, i2c->regs + HSI2C_AUTO_CONF);
-   spin_unlock_irqrestore(&i2c->lock, flags);
+   raw_spin_unlock_irqrestore(&i2c->lock, flags);
 }
 
 static int exynos5_i2c_xfer_msg(struct exynos5_i2c *i2c,
@@ -763,7 +763,7 @@ static int exynos5_i2c_probe(struct platform_device *pdev)
/* Clear pending interrupts from u-boot or misc causes */
exynos5_i2c_clr_pend_irq(i2c);
 
-   spin_lock_init(&i2c->lock);
+   raw_spin_lock_init(&i2c->lock);
init_completion(&i2c->msg_complete);
 
i2c->irq = ret = platform_get_irq(pdev, 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