[PATCH] i2c-designware: add optional regulator support

[PATCH] i2c-designware: add optional regulator support

[Mika Westerberg]

It is possible that some of the i2c busses are supplied by a separate
regulator which needs to be enabled before we can access the bus. Thus add
support for an optional regulator.

We also implement enabling/disabling the regulator on system and runtime PM
hooks.

Signed-off-by: Mika Westerberg <mika.westerberg-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Signed-off-by: Jani Nikula <jani.nikula-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Signed-off-by: Kirill A. Shutemov <kirill.shutemov-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Cc: Dirk Brandewie <dirk.brandewie-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/i2c/busses/i2c-designware-pcidrv.c |   19 +++++++++++++++++++
 1 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c
index 37f4211..26bcdf3 100644
--- a/drivers/i2c/busses/i2c-designware-pcidrv.c
+++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
@@ -39,6 +39,7 @@
 #include <linux/slab.h>
 #include <linux/pci.h>
 #include <linux/pm_runtime.h>
+#include <linux/regulator/consumer.h>
 #include "i2c-designware-core.h"
 
 #define DRIVER_NAME "i2c-designware-pci"
@@ -62,6 +63,7 @@ struct dw_pci_controller {
 	u32 tx_fifo_depth;
 	u32 rx_fifo_depth;
 	u32 clk_khz;
+	struct regulator *regulator;
 };
 
 #define INTEL_MID_STD_CFG  (DW_IC_CON_MASTER |			\
@@ -144,6 +146,8 @@ static int i2c_dw_pci_suspend(struct device *dev)
 	struct dw_i2c_dev *i2c = pci_get_drvdata(pdev);
 	int err;
 
+	if (i2c->controller->regulator)
+		regulator_disable(i2c->controller->regulator);
 
 	i2c_dw_disable(i2c);
 
@@ -169,6 +173,9 @@ static int i2c_dw_pci_resume(struct device *dev)
 	int err;
 	u32 enabled;
 
+	if (i2c->controller->regulator)
+		regulator_enable(i2c->controller->regulator);
+
 	enabled = i2c_dw_is_enabled(i2c);
 	if (enabled)
 		return 0;
@@ -281,6 +288,12 @@ const struct pci_device_id *id)
 
 	pci_set_drvdata(pdev, dev);
 
+	controller->regulator = regulator_get(&pdev->dev, "v-i2c");
+	if (IS_ERR(controller->regulator))
+		controller->regulator = NULL;
+	else
+		regulator_enable(controller->regulator);
+
 	dev->tx_fifo_depth = controller->tx_fifo_depth;
 	dev->rx_fifo_depth = controller->rx_fifo_depth;
 	r = i2c_dw_init(dev);
@@ -332,11 +345,17 @@ exit:
 static void __devexit i2c_dw_pci_remove(struct pci_dev *pdev)
 {
 	struct dw_i2c_dev *dev = pci_get_drvdata(pdev);
+	struct regulator *regulator = dev->controller->regulator;
 
 	i2c_dw_disable(dev);
 	pm_runtime_forbid(&pdev->dev);
 	pm_runtime_get_noresume(&pdev->dev);
 
+	if (regulator) {
+		regulator_disable(regulator);
+		regulator_put(regulator);
+	}
+
 	pci_set_drvdata(pdev, NULL);
 	i2c_del_adapter(&dev->adapter);
 	put_device(&pdev->dev);
-- 
1.7.9.1

Re: [PATCH] i2c-designware: add optional regulator support

[Mark Brown]

On Mon, Mar 19, 2012 at 02:46:20PM +0200, Mika Westerberg wrote:

> It is possible that some of the i2c busses are supplied by a separate
> regulator which needs to be enabled before we can access the bus. Thus add
> support for an optional regulator.

No, support for critical supplies should not be optional.  I'm getting
rather fed up with having to point this out.  There's always a regulator
there supplying the bus, the driver should just assume that the system
provides it.

> We also implement enabling/disabling the regulator on system and runtime PM
> hooks.

This is suspicious too...

If this is just the external buffer/reference voltage for the bus then
I'd really not expect the controller to have to worry about it, the
drivers for the target devices ought to be making sure they've powered
up the devices prior to trying to talk to them anyway (and most of the
time will need the supplies on for *much* longer than just the time
spent doing I2C transactions).  There's no harm in having support in the
controller driver but I'd be surprised if the clients were happy with
having their I/O voltage bounced on them...

> +	if (i2c->controller->regulator)
> +		regulator_enable(i2c->controller->regulator);

> +	controller->regulator = regulator_get(&pdev->dev, "v-i2c");
> +	if (IS_ERR(controller->regulator))
> +		controller->regulator = NULL;
> +	else
> +		regulator_enable(controller->regulator);

There's no error checking at all in this ccode - if it fails to request
the supply the driver silently ignores the error and if the regulator is
there but fails to enable the error will also be silently ignored.

Re: [PATCH] i2c-designware: add optional regulator support

[Mika Westerberg]

On Mon, Mar 19, 2012 at 02:28:07PM +0000, Mark Brown wrote:
> On Mon, Mar 19, 2012 at 02:46:20PM +0200, Mika Westerberg wrote:
> 
> > It is possible that some of the i2c busses are supplied by a separate
> > regulator which needs to be enabled before we can access the bus. Thus add
> > support for an optional regulator.
> 
> No, support for critical supplies should not be optional.  I'm getting
> rather fed up with having to point this out.  There's always a regulator
> there supplying the bus, the driver should just assume that the system
> provides it.

Right, but not all platforms have regulator for this. That's why we made it
optional.

On the other hand if we assume it is always provided by the system then I
guess there is no way to turn it off for example during system sleep which
might result unneeded power consumption.

> > We also implement enabling/disabling the regulator on system and runtime PM
> > hooks.
> 
> This is suspicious too...
> 
> If this is just the external buffer/reference voltage for the bus then
> I'd really not expect the controller to have to worry about it, the
> drivers for the target devices ought to be making sure they've powered
> up the devices prior to trying to talk to them anyway (and most of the
> time will need the supplies on for *much* longer than just the time
> spent doing I2C transactions).  There's no harm in having support in the
> controller driver but I'd be surprised if the clients were happy with
> having their I/O voltage bounced on them...

It is meant for controlling bus voltage for given I2C bus in case some
other platform using designware I2C controller might have use for such.

Having the controller controlling the bus voltage should not cause problems
for its clients as they get suspended before the bus itself and vice versa
in case of resume. Or have I misunderstood something?

On one Medfield platform we actually have a LCD panel which misbehaves when
its supply voltage is cut off (it drives the I2C lines low). This made the
whole bus unusable.

So we worked it around by adding an artificial regulator to I2C controller
which then drives LCD supply voltage as well (it is shared with LCD driver
and I2C controller driver). This way we were able to make sure that the bus
is functional when any I2C transactions are performed.

In our case we really want to turn off the LCD voltage when not needed as
it consumes ~500mW power.

> > +	if (i2c->controller->regulator)
> > +		regulator_enable(i2c->controller->regulator);
> 
> > +	controller->regulator = regulator_get(&pdev->dev, "v-i2c");
> > +	if (IS_ERR(controller->regulator))
> > +		controller->regulator = NULL;
> > +	else
> > +		regulator_enable(controller->regulator);
> 
> There's no error checking at all in this ccode - if it fails to request
> the supply the driver silently ignores the error and if the regulator is
> there but fails to enable the error will also be silently ignored.

The first case is because the regulator is optional - if we don't get it we
don't try to use it. The other case is overlook from our side and should be
fixed to handle the errors correctly.

Re: [PATCH] i2c-designware: add optional regulator support

[Mark Brown]

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

On Mon, Mar 19, 2012 at 05:41:30PM +0200, Mika Westerberg wrote:
> On Mon, Mar 19, 2012 at 02:28:07PM +0000, Mark Brown wrote:

> > No, support for critical supplies should not be optional.  I'm getting
> > rather fed up with having to point this out.  There's always a regulator
> > there supplying the bus, the driver should just assume that the system
> > provides it.

> Right, but not all platforms have regulator for this. That's why we made it
> optional.

No, really.  They'll have a regulator - the buffer voltage isn't just
randomly going to appear without something supplying it.  It might not
be a software controlled supply but there will be something there.  This
all applies to essentially any supply and is therefore handled in the
core, it compiles itself out transparently when not enabled and there's
facilities for providing stub regulators too.  Just ignoring the errors
isn't a good way of handling this.

> > If this is just the external buffer/reference voltage for the bus then
> > I'd really not expect the controller to have to worry about it, the
> > drivers for the target devices ought to be making sure they've powered
> > up the devices prior to trying to talk to them anyway (and most of the

> Having the controller controlling the bus voltage should not cause problems
> for its clients as they get suspended before the bus itself and vice versa
> in case of resume. Or have I misunderstood something?

That's extremely unusual for an I2C controller and probably suboptimal
for your system - due to the nature of I2C the devices don't need the
controller to be around except when the controller wants to interact
with them.  Normally the runtime power management for I2C controllers
suspends the controller whenever it's not actively talking on the bus
ignoring the state of the children, allowing the SoC to save power on a
controller that's just sitting there idle.

> On one Medfield platform we actually have a LCD panel which misbehaves when
> its supply voltage is cut off (it drives the I2C lines low). This made the
> whole bus unusable.

> So we worked it around by adding an artificial regulator to I2C controller
> which then drives LCD supply voltage as well (it is shared with LCD driver
> and I2C controller driver). This way we were able to make sure that the bus
> is functional when any I2C transactions are performed.

> In our case we really want to turn off the LCD voltage when not needed as
> it consumes ~500mW power.

This is all exactly what I'd expect and really sounds exactly like the
case I described - the panel needs the supply so the panel driver should
be managing it when required.  There's nothing system or controller
specific about the fact that the panel needs all its supplies up to run,
and indeed there's already some LCD and backlight drivers which manage
their supplies.

> > > +	if (i2c->controller->regulator)
> > > +		regulator_enable(i2c->controller->regulator);

> > > +	controller->regulator = regulator_get(&pdev->dev, "v-i2c");
> > > +	if (IS_ERR(controller->regulator))
> > > +		controller->regulator = NULL;
> > > +	else
> > > +		regulator_enable(controller->regulator);

> > There's no error checking at all in this ccode - if it fails to request
> > the supply the driver silently ignores the error and if the regulator is
> > there but fails to enable the error will also be silently ignored.

> The first case is because the regulator is optional - if we don't get it we
> don't try to use it. The other case is overlook from our side and should be

So what happens in a system where the regulator is required but fails to
appear for some reason is that the driver happily runs on without saying
anything which isn't going to be great for diagnostics and will produce
some interesting races when we start to deploy deferred probe.

There's really nothing sensible an individual driver can do except for
assume that the device needs power, the concept of things being supplied
by fixed voltage regulators outside of software control is so widespread
that it'd be crazy to implement support for it in individual drivers.

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

Re: [PATCH] i2c-designware: add optional regulator support

[Mika Westerberg]

On Mon, Mar 19, 2012 at 04:24:34PM +0000, Mark Brown wrote:

> This is all exactly what I'd expect and really sounds exactly like the
> case I described - the panel needs the supply so the panel driver should
> be managing it when required.  There's nothing system or controller
> specific about the fact that the panel needs all its supplies up to run,
> and indeed there's already some LCD and backlight drivers which manage
> their supplies.

We already have the panel driver to manage its regulator and it works as
should. The problem is that when we blank the display and switch off the
panel power, it drives the whole I2C bus to low (due to HW bug in the
panel). This effectively prevents any other driver to talk to their device
on the same bus :-/

This is the reason we shared the same panel regulator with I2C controller -
power is only cut off when both drivers do regulator_disable().

> So what happens in a system where the regulator is required but fails to
> appear for some reason is that the driver happily runs on without saying
> anything which isn't going to be great for diagnostics and will produce
> some interesting races when we start to deploy deferred probe.

Ah, true - it should print some sort of warning in that case instead of
silently failing.

I understand your point wrt. this patch and I think we can go forward by
keeping this in our local kernel tree as a workaround for particular HW
issue.

Thank you for your comments.

Re: [PATCH] i2c-designware: add optional regulator support

[Mark Brown]

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

On Tue, Mar 20, 2012 at 10:46:19AM +0200, Mika Westerberg wrote:

> We already have the panel driver to manage its regulator and it works as
> should. The problem is that when we blank the display and switch off the
> panel power, it drives the whole I2C bus to low (due to HW bug in the
> panel). This effectively prevents any other driver to talk to their device
> on the same bus :-/

> This is the reason we shared the same panel regulator with I2C controller -
> power is only cut off when both drivers do regulator_disable().

Right, but the more normal thing here would be to add support to the
other client drivers rather than the controller (though there is no harm
at all in adding support to the controller, it's just a little odd
rather than wrong).  Like I say as far as the controller is concerned
you can generally turn the buffer voltage off whenever you're not
actually doing an I2C transaction which probably isn't what you want.

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