[PATCH RESEND 0/2] runtime PM support for I2C clients

[PATCH RESEND 0/2] runtime PM support for I2C clients

[Mika Westerberg]

[Resend with linux-arm-kernel included and added acks from Rafael]

Hi all,

With the advent of ACPI 5.0 we are starting to see I2C client devices
described in ACPI namespace that support power management by the means of
standard ACPI _PSx-methods. For example Intel Haswell based platforms might
have touch screen or sensor-hub connected to the I2C bus that can be
powered on and off by calling _PS0 and _PS3 -methods.

In order to support such I2C client devices, we decided to hook the ACPI
power management for these to the standard Linux runtime PM framework.

These patches implement runtime PM support for I2C client devices in a
similar way that is done already for the PCI bus. Just before a driver is
bound to an I2C client device the device runtime PM is being prepared for
that device.

If the device in question has an ACPI handle we attach it to the ACPI power
domain that then makes sure that the right _PSx methods are called in
response to runtime PM events the driver generates.

A driver that wants to participate in runtime PM and power manage its
device should:
 1) Implement device specific runtime PM callbacks if needed.
 2) Call pm_runtime_put() (or some variant of that) to decrease the runtime
    PM reference count.

If the driver doesn't do anything the device is regarded as runtime PM
active and powered on.

Even though this series has been developed specifically for ACPI enumerated
I2C client devices, I believe it can be useful for other I2C client drivers
because the I2C core now prepares the runtime PM on behalf of the driver
and thus reduces the amount of code a driver writer needs to add.

Aaron Lu (1):
  i2c: prepare runtime PM support for I2C client devices

Lv Zheng (1):
  i2c: attach/detach I2C client device to the ACPI power domain

 drivers/i2c/i2c-core.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 56 insertions(+), 1 deletion(-)

-- 
1.8.4.rc2

[PATCH RESEND 1/2] i2c: prepare runtime PM support for I2C client devices

[Mika Westerberg]

From: Aaron Lu <aaron.lu@intel.com>

This patch adds runtime PM support for the I2C bus in a similar way that
has been done for PCI bus already. This means that the I2C bus core
prepares runtime PM for a client device just before a driver is about to be
bound to it. Devices that are not bound to any driver are not prepared for
runtime PM.

In order to take advantage of this runtime PM support, the client device
driver needs drop the device runtime PM reference count by calling
pm_runtime_put() in its ->probe() callback and possibly implement rest of
the runtime PM callbacks.

However, this does not yet make runtime PM happen for the device, it has to
be explicitly allowed from userspace per each I2C client device. The
reason for this is that things like HID over I2C might not work as smoothly
when runtime PM is active. So we leave it to the user to balance between
performance and power efficiency.

User can allow runtime PM for the client device by running:

	# echo auto > /sys/bus/i2c/devices/<device>/power/control

and it can be forbidden again by:

	# echo on > /sys/bus/i2c/devices/<device>/power/control

Status of the device can be monitored by reading files under the device
power directory.

If the driver doesn't support runtime PM (like most of the existing I2C
client drivers), the device in question is regarded as being runtime PM
active and powered on.

The patch adds also runtime PM support for the adapter device because it is
needed to be able to runtime power manage the I2C controller device. The
adapter device is handled along with the I2C controller device (it uses
pm_runtime_no_callbacks()).

Signed-off-by: Aaron Lu <aaron.lu@intel.com>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/i2c/i2c-core.c | 48 +++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 47 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 3d44292..8fad5ac 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -254,11 +254,34 @@ static int i2c_device_probe(struct device *dev)
 					client->flags & I2C_CLIENT_WAKE);
 	dev_dbg(dev, "probe\n");
 
+	/* Make sure the adapter is active */
+	pm_runtime_get_sync(&client->adapter->dev);
+
+	/*
+	 * Enable runtime PM for the client device. If the client wants to
+	 * participate on runtime PM it should call pm_runtime_put() in its
+	 * probe() callback.
+	 *
+	 * User still needs to allow the PM runtime before it can actually
+	 * happen.
+	 */
+	pm_runtime_forbid(&client->dev);
+	pm_runtime_get_noresume(&client->dev);
+	pm_runtime_set_active(&client->dev);
+	pm_runtime_enable(&client->dev);
+
 	status = driver->probe(client, i2c_match_id(driver->id_table, client));
 	if (status) {
 		client->driver = NULL;
 		i2c_set_clientdata(client, NULL);
+
+		pm_runtime_disable(&client->dev);
+		pm_runtime_set_suspended(&client->dev);
+		pm_runtime_put_noidle(&client->dev);
 	}
+
+	pm_runtime_put(&client->adapter->dev);
+
 	return status;
 }
 
@@ -271,6 +294,8 @@ static int i2c_device_remove(struct device *dev)
 	if (!client || !dev->driver)
 		return 0;
 
+	pm_runtime_get_sync(&client->adapter->dev);
+
 	driver = to_i2c_driver(dev->driver);
 	if (driver->remove) {
 		dev_dbg(dev, "remove\n");
@@ -283,6 +308,13 @@ static int i2c_device_remove(struct device *dev)
 		client->driver = NULL;
 		i2c_set_clientdata(client, NULL);
 	}
+
+	/* Undo the runtime PM done in i2c_probe() */
+	pm_runtime_disable(&client->dev);
+	pm_runtime_set_suspended(&client->dev);
+	pm_runtime_put_noidle(&client->dev);
+
+	pm_runtime_put(&client->adapter->dev);
 	return status;
 }
 
@@ -294,8 +326,11 @@ static void i2c_device_shutdown(struct device *dev)
 	if (!client || !dev->driver)
 		return;
 	driver = to_i2c_driver(dev->driver);
-	if (driver->shutdown)
+	if (driver->shutdown) {
+		pm_runtime_get_sync(&client->adapter->dev);
 		driver->shutdown(client);
+		pm_runtime_put(&client->adapter->dev);
+	}
 }
 
 #ifdef CONFIG_PM_SLEEP
@@ -1263,6 +1298,15 @@ exit_recovery:
 	bus_for_each_drv(&i2c_bus_type, NULL, adap, __process_new_adapter);
 	mutex_unlock(&core_lock);
 
+	/*
+	 * Make sure the adapter runtime PM follows the parent device (the
+	 * host controller) so that we can suspend it once there aren't any
+	 * active clients anymore.
+	 */
+	pm_runtime_set_active(&adap->dev);
+	pm_runtime_no_callbacks(&adap->dev);
+	pm_runtime_enable(&adap->dev);
+
 	return 0;
 
 out_list:
@@ -1427,6 +1471,8 @@ void i2c_del_adapter(struct i2c_adapter *adap)
 		return;
 	}
 
+	pm_runtime_disable(&adap->dev);
+
 	/* Tell drivers about this removal */
 	mutex_lock(&core_lock);
 	bus_for_each_drv(&i2c_bus_type, NULL, adap,
-- 
1.8.4.rc2

Re: [PATCH RESEND 1/2] i2c: prepare runtime PM support for I2C client devices

[Mark Brown]

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

On Mon, Sep 09, 2013 at 04:34:38PM +0300, Mika Westerberg wrote:

> +	/*
> +	 * Enable runtime PM for the client device. If the client wants to
> +	 * participate on runtime PM it should call pm_runtime_put() in its
> +	 * probe() callback.
> +	 *
> +	 * User still needs to allow the PM runtime before it can actually
> +	 * happen.
> +	 */
> +	pm_runtime_forbid(&client->dev);
> +	pm_runtime_get_noresume(&client->dev);
> +	pm_runtime_set_active(&client->dev);
> +	pm_runtime_enable(&client->dev);

How is this going to interact with client devices which are already
enabling runtime PM for themselves, and what are the advantages of doing
this over having the client device enable runtime PM for itself (given
that the client still needs an explicit put() adding)?

Given that it's relatively common for devices to have both I2C and SPI
control it seems like it'd be sensible to keep the policy common between
the two buses to simplify driver implementation.

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

Re: [PATCH RESEND 1/2] i2c: prepare runtime PM support for I2C client devices

[Mika Westerberg]

On Mon, Sep 09, 2013 at 04:40:28PM +0100, Mark Brown wrote:
> On Mon, Sep 09, 2013 at 04:34:38PM +0300, Mika Westerberg wrote:
> 
> > +	/*
> > +	 * Enable runtime PM for the client device. If the client wants to
> > +	 * participate on runtime PM it should call pm_runtime_put() in its
> > +	 * probe() callback.
> > +	 *
> > +	 * User still needs to allow the PM runtime before it can actually
> > +	 * happen.
> > +	 */
> > +	pm_runtime_forbid(&client->dev);
> > +	pm_runtime_get_noresume(&client->dev);
> > +	pm_runtime_set_active(&client->dev);
> > +	pm_runtime_enable(&client->dev);
> 
> How is this going to interact with client devices which are already
> enabling runtime PM for themselves, and what are the advantages of doing
> this over having the client device enable runtime PM for itself (given
> that the client still needs an explicit put() adding)?

My understanding is that you can call pm_runtime_enable() several times
(provided that pm_runtime_disable() is called as many times). So that
should have no effect on the current drivers that already take advantage of
runtime PM.

There is one difference though -- runtime PM is now blocked by default and
it needs to be unblocked from the userspace per each device.

For the advantages compared to each driver handling it completely
themselves:

	- Few lines less as you only need to call _put().
	- It follows what is already been done for other buses, like PCI
	  and AMBA .
	- The I2C core makes sure that the device is available (from bus
	  point of view) when the driver ->probe() is called.

> Given that it's relatively common for devices to have both I2C and SPI
> control it seems like it'd be sensible to keep the policy common between
> the two buses to simplify driver implementation.

Yes and IMHO if I2C and SPI follows what has already been done for other
buses it should make the driver writer's job easier as the usage is similar
from one bus to another.

Re: [PATCH RESEND 1/2] i2c: prepare runtime PM support for I2C client devices

[Rafael J. Wysocki]

On Tuesday, September 10, 2013 10:51:00 AM Mika Westerberg wrote:
> On Mon, Sep 09, 2013 at 04:40:28PM +0100, Mark Brown wrote:
> > On Mon, Sep 09, 2013 at 04:34:38PM +0300, Mika Westerberg wrote:
> > 
> > > +	/*
> > > +	 * Enable runtime PM for the client device. If the client wants to
> > > +	 * participate on runtime PM it should call pm_runtime_put() in its
> > > +	 * probe() callback.
> > > +	 *
> > > +	 * User still needs to allow the PM runtime before it can actually
> > > +	 * happen.
> > > +	 */
> > > +	pm_runtime_forbid(&client->dev);
> > > +	pm_runtime_get_noresume(&client->dev);
> > > +	pm_runtime_set_active(&client->dev);
> > > +	pm_runtime_enable(&client->dev);
> > 
> > How is this going to interact with client devices which are already
> > enabling runtime PM for themselves, and what are the advantages of doing
> > this over having the client device enable runtime PM for itself (given
> > that the client still needs an explicit put() adding)?
> 
> My understanding is that you can call pm_runtime_enable() several times
> (provided that pm_runtime_disable() is called as many times). So that
> should have no effect on the current drivers that already take advantage of
> runtime PM.

That's correct.

> There is one difference though -- runtime PM is now blocked by default and
> it needs to be unblocked from the userspace per each device.
> 
> For the advantages compared to each driver handling it completely
> themselves:
> 
> 	- Few lines less as you only need to call _put().
> 	- It follows what is already been done for other buses, like PCI
> 	  and AMBA .
> 	- The I2C core makes sure that the device is available (from bus
> 	  point of view) when the driver ->probe() is called.
> 
> > Given that it's relatively common for devices to have both I2C and SPI
> > control it seems like it'd be sensible to keep the policy common between
> > the two buses to simplify driver implementation.
> 
> Yes and IMHO if I2C and SPI follows what has already been done for other
> buses it should make the driver writer's job easier as the usage is similar
> from one bus to another.

I agree here, FWIW.

Thanks!


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH RESEND 1/2] i2c: prepare runtime PM support for I2C client devices

[Mark Brown]

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

On Tue, Sep 10, 2013 at 10:51:00AM +0300, Mika Westerberg wrote:
> On Mon, Sep 09, 2013 at 04:40:28PM +0100, Mark Brown wrote:

> > How is this going to interact with client devices which are already
> > enabling runtime PM for themselves, and what are the advantages of doing
> > this over having the client device enable runtime PM for itself (given
> > that the client still needs an explicit put() adding)?

> My understanding is that you can call pm_runtime_enable() several times
> (provided that pm_runtime_disable() is called as many times). So that
> should have no effect on the current drivers that already take advantage of
> runtime PM.

Not quite...

> There is one difference though -- runtime PM is now blocked by default and
> it needs to be unblocked from the userspace per each device.

...as you say.

This seems crazy, why on earth would we want to have userspace be forced
to manually go through every single device and manually enable power
saving?  I can't see anyone finding that helpful and it's going to be a
pain to deploy.

However I had thought it was just a case of the drivers doing a put()
instead of their current code to enable runtime PM (you mention that
later on)?  That seems both sensible and doable, though it would need
doing as part of the conversion to avoid regressions and I'd expect it
does mean that SPI needs to be converted at the same time.

> For the advantages compared to each driver handling it completely
> themselves:

> 	- Few lines less as you only need to call _put().
> 	- It follows what is already been done for other buses, like PCI
> 	  and AMBA .
> 	- The I2C core makes sure that the device is available (from bus
> 	  point of view) when the driver ->probe() is called.

I can't understand your last point here at all, sorry.  Can you expand
please?

> > Given that it's relatively common for devices to have both I2C and SPI
> > control it seems like it'd be sensible to keep the policy common between
> > the two buses to simplify driver implementation.

> Yes and IMHO if I2C and SPI follows what has already been done for other
> buses it should make the driver writer's job easier as the usage is similar
> from one bus to another.

So then the obvious followup question is why this is even something that
needs to be implemented per bus?  Shouldn't we be enhancing the driver
core to do this, or is that the long term plan and this is a step on the
road to doing that?

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

Re: [PATCH RESEND 1/2] i2c: prepare runtime PM support for I2C client devices

[Mika Westerberg]

On Tue, Sep 10, 2013 at 01:27:54PM +0100, Mark Brown wrote:
> On Tue, Sep 10, 2013 at 10:51:00AM +0300, Mika Westerberg wrote:
> > On Mon, Sep 09, 2013 at 04:40:28PM +0100, Mark Brown wrote:
> 
> > > How is this going to interact with client devices which are already
> > > enabling runtime PM for themselves, and what are the advantages of doing
> > > this over having the client device enable runtime PM for itself (given
> > > that the client still needs an explicit put() adding)?
> 
> > My understanding is that you can call pm_runtime_enable() several times
> > (provided that pm_runtime_disable() is called as many times). So that
> > should have no effect on the current drivers that already take advantage of
> > runtime PM.
> 
> Not quite...
> 
> > There is one difference though -- runtime PM is now blocked by default and
> > it needs to be unblocked from the userspace per each device.
> 
> ...as you say.
> 
> This seems crazy, why on earth would we want to have userspace be forced
> to manually go through every single device and manually enable power
> saving?  I can't see anyone finding that helpful and it's going to be a
> pain to deploy.

There are things like HID over I2C devices (e.g touch screen) where going
to lower power states too aggressively makes the touch experience really
sluggish. However, other HID over I2C devices like sensor hubs it doesn't
matter that much. In order to get the best performance we have runtime PM
blocked by default (and leave it up to the user to unblock to get power
savings).

That's basically what PCI drivers currently do.

> However I had thought it was just a case of the drivers doing a put()
> instead of their current code to enable runtime PM (you mention that
> later on)?

User still needs to unblock runtime PM for the device. The driver can call
the runtime PM functions but they don't have any effect until runtime PM is
unblocked by the user.

However, I don't have problems dropping the call to pm_runtime_forbid() in
this patch and leave it up to the user to decide whether runtime PM should
be blocked for the device.

> That seems both sensible and doable, though it would need
> doing as part of the conversion to avoid regressions and I'd expect it
> does mean that SPI needs to be converted at the same time.

OK.

> > For the advantages compared to each driver handling it completely
> > themselves:
> 
> > 	- Few lines less as you only need to call _put().
> > 	- It follows what is already been done for other buses, like PCI
> > 	  and AMBA .
> > 	- The I2C core makes sure that the device is available (from bus
> > 	  point of view) when the driver ->probe() is called.
> 
> I can't understand your last point here at all, sorry.  Can you expand
> please?

Sorry about that.

At least with ACPI enumerated I2C client devices, they might be powered off
by the BIOS (there are power resources attached to the devices). So when
the driver ->probe() is called we can't access the device's registers etc.

So we bind the device to the ACPI power domain (the second patch in this
series) and then call pm_runtime_get() for the device. That makes sure that
the device is accessible when ->probe() is called.

> 
> > > Given that it's relatively common for devices to have both I2C and SPI
> > > control it seems like it'd be sensible to keep the policy common between
> > > the two buses to simplify driver implementation.
> 
> > Yes and IMHO if I2C and SPI follows what has already been done for other
> > buses it should make the driver writer's job easier as the usage is similar
> > from one bus to another.
> 
> So then the obvious followup question is why this is even something that
> needs to be implemented per bus?  Shouldn't we be enhancing the driver
> core to do this, or is that the long term plan and this is a step on the
> road to doing that?

If we end up all buses implementing the same mechanism, I think it makes
sense to move it to the driver core.

Re: [PATCH RESEND 1/2] i2c: prepare runtime PM support for I2C client devices

[Mark Brown]

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

On Tue, Sep 10, 2013 at 05:26:31PM +0300, Mika Westerberg wrote:
> On Tue, Sep 10, 2013 at 01:27:54PM +0100, Mark Brown wrote:

> > > There is one difference though -- runtime PM is now blocked by default and
> > > it needs to be unblocked from the userspace per each device.

> > ...as you say.

> > This seems crazy, why on earth would we want to have userspace be forced
> > to manually go through every single device and manually enable power
> > saving?  I can't see anyone finding that helpful and it's going to be a
> > pain to deploy.

> There are things like HID over I2C devices (e.g touch screen) where going
> to lower power states too aggressively makes the touch experience really
> sluggish. However, other HID over I2C devices like sensor hubs it doesn't
> matter that much. In order to get the best performance we have runtime PM
> blocked by default (and leave it up to the user to unblock to get power
> savings).

> That's basically what PCI drivers currently do.

So those specific devices need to implement runtime PM in an appropriate
fashion.  That's no need to implement a poor default for every single
device to work around poor implementations from some, especially when it
requires a userspace update to get acceptable performance again from the
unaffected devices.

> > However I had thought it was just a case of the drivers doing a put()
> > instead of their current code to enable runtime PM (you mention that
> > later on)?

> User still needs to unblock runtime PM for the device. The driver can call
> the runtime PM functions but they don't have any effect until runtime PM is
> unblocked by the user.

> However, I don't have problems dropping the call to pm_runtime_forbid() in
> this patch and leave it up to the user to decide whether runtime PM should
> be blocked for the device.

I think this is essential - we can't really go around forcing userspace
updates to restore runtime power management, nobody is going to thank us
for that and it sounds like the issue you're trying to solve is device
specific anyway.

> > > 	- The I2C core makes sure that the device is available (from bus
> > > 	  point of view) when the driver ->probe() is called.

> > I can't understand your last point here at all, sorry.  Can you expand
> > please?

> Sorry about that.

> At least with ACPI enumerated I2C client devices, they might be powered off
> by the BIOS (there are power resources attached to the devices). So when
> the driver ->probe() is called we can't access the device's registers etc.

> So we bind the device to the ACPI power domain (the second patch in this
> series) and then call pm_runtime_get() for the device. That makes sure that
> the device is accessible when ->probe() is called.

OK, that is very much not the model which embedded systems follow, in
embedded systems the driver for the device is fully in control of its
own power.  It gets resources like GPIOs and regulators which allow it
to make fine grained decisions.

> > So then the obvious followup question is why this is even something that
> > needs to be implemented per bus?  Shouldn't we be enhancing the driver
> > core to do this, or is that the long term plan and this is a step on the
> > road to doing that?

> If we end up all buses implementing the same mechanism, I think it makes
> sense to move it to the driver core.

If we're starting to get a reasonable number of buses following the same
pattern it seems like we're in a position to start 

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

Re: [PATCH RESEND 1/2] i2c: prepare runtime PM support for I2C client devices

[Rafael J. Wysocki]

On Tuesday, September 10, 2013 05:13:21 PM Mark Brown wrote:
> On Tue, Sep 10, 2013 at 05:26:31PM +0300, Mika Westerberg wrote:
> > On Tue, Sep 10, 2013 at 01:27:54PM +0100, Mark Brown wrote:
> 
> > > > There is one difference though -- runtime PM is now blocked by default and
> > > > it needs to be unblocked from the userspace per each device.
> 
> > > ...as you say.
> 
> > > This seems crazy, why on earth would we want to have userspace be forced
> > > to manually go through every single device and manually enable power
> > > saving?  I can't see anyone finding that helpful and it's going to be a
> > > pain to deploy.
> 
> > There are things like HID over I2C devices (e.g touch screen) where going
> > to lower power states too aggressively makes the touch experience really
> > sluggish. However, other HID over I2C devices like sensor hubs it doesn't
> > matter that much. In order to get the best performance we have runtime PM
> > blocked by default (and leave it up to the user to unblock to get power
> > savings).
> 
> > That's basically what PCI drivers currently do.
> 
> So those specific devices need to implement runtime PM in an appropriate
> fashion.  That's no need to implement a poor default for every single
> device to work around poor implementations from some, especially when it
> requires a userspace update to get acceptable performance again from the
> unaffected devices.
> 
> > > However I had thought it was just a case of the drivers doing a put()
> > > instead of their current code to enable runtime PM (you mention that
> > > later on)?
> 
> > User still needs to unblock runtime PM for the device. The driver can call
> > the runtime PM functions but they don't have any effect until runtime PM is
> > unblocked by the user.
> 
> > However, I don't have problems dropping the call to pm_runtime_forbid() in
> > this patch and leave it up to the user to decide whether runtime PM should
> > be blocked for the device.
> 
> I think this is essential - we can't really go around forcing userspace
> updates to restore runtime power management, nobody is going to thank us
> for that and it sounds like the issue you're trying to solve is device
> specific anyway.

In fact, that's all about what the default should be, because user space
can very easily change it either way (it takes one loop in shell code to do
that for all devices on the given bus).

And I'm fine with defaulting to "auto".

> > > > 	- The I2C core makes sure that the device is available (from bus
> > > > 	  point of view) when the driver ->probe() is called.
> 
> > > I can't understand your last point here at all, sorry.  Can you expand
> > > please?
> 
> > Sorry about that.
> 
> > At least with ACPI enumerated I2C client devices, they might be powered off
> > by the BIOS (there are power resources attached to the devices). So when
> > the driver ->probe() is called we can't access the device's registers etc.
> 
> > So we bind the device to the ACPI power domain (the second patch in this
> > series) and then call pm_runtime_get() for the device. That makes sure that
> > the device is accessible when ->probe() is called.
> 
> OK, that is very much not the model which embedded systems follow, in
> embedded systems the driver for the device is fully in control of its
> own power.  It gets resources like GPIOs and regulators which allow it
> to make fine grained decisions.

There are platforms where those resources are simply not available for
direct manipulation and we need to use ACPI methods for power management.

Now, since those methods are used in pretty much the same way for all I2C
devices, we add a PM domain for that to avoid duplicating the same code in
all of the drivers in question (patch [2/2]).  Does that make sense to you?

> > > So then the obvious followup question is why this is even something that
> > > needs to be implemented per bus?  Shouldn't we be enhancing the driver
> > > core to do this, or is that the long term plan and this is a step on the
> > > road to doing that?
> 
> > If we end up all buses implementing the same mechanism, I think it makes
> > sense to move it to the driver core.
> 
> If we're starting to get a reasonable number of buses following the same
> pattern it seems like we're in a position to start 

We need that for exactly 3 buses: platform (already done), I2C and SPI.

No other bus types are going to use ACPI this way for PM, at least for the
time being, simply because PCI, USB and SATA/IDE have their own ways of doing
this (which are bus-specific) and the spec doesn't cover other bus types
directly (it defines support for UART, but we don't have a UART bus type).

Moreover, because PCI and USB use ACPI for PM in their own ways, moving that
thing up to the driver core would be rather inconvenient.

Thanks,
Rafael

Re: [PATCH RESEND 1/2] i2c: prepare runtime PM support for I2C client devices

[Mark Brown]

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

On Tue, Sep 10, 2013 at 10:04:21PM +0200, Rafael J. Wysocki wrote:
> On Tuesday, September 10, 2013 05:13:21 PM Mark Brown wrote:

> > OK, that is very much not the model which embedded systems follow, in
> > embedded systems the driver for the device is fully in control of its
> > own power.  It gets resources like GPIOs and regulators which allow it
> > to make fine grained decisions.

> There are platforms where those resources are simply not available for
> direct manipulation and we need to use ACPI methods for power management.

> Now, since those methods are used in pretty much the same way for all I2C
> devices, we add a PM domain for that to avoid duplicating the same code in
> all of the drivers in question (patch [2/2]).  Does that make sense to you?

It doesn't seem like a particular problem, but the existing usage does
need to be preserved for the systems that use it so things like having
auto as the default and updating the drivers seem like they're needed.

> > If we're starting to get a reasonable number of buses following the same
> > pattern it seems like we're in a position to start 

> We need that for exactly 3 buses: platform (already done), I2C and SPI.

> No other bus types are going to use ACPI this way for PM, at least for the
> time being, simply because PCI, USB and SATA/IDE have their own ways of doing
> this (which are bus-specific) and the spec doesn't cover other bus types
> directly (it defines support for UART, but we don't have a UART bus type).

> Moreover, because PCI and USB use ACPI for PM in their own ways, moving that
> thing up to the driver core would be rather inconvenient.

That only applies to the power domains though, what Mika was saying was
that the process for enabling runtime PM (just drop a reference) also
becomes easier with this method regardless of anything else - that makes
sense to me as something we might want to end up with so do we want to
just move towards making that a default?

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

Re: [PATCH RESEND 1/2] i2c: prepare runtime PM support for I2C client devices

[Rafael J. Wysocki]

On Tuesday, September 10, 2013 10:35:22 PM Mark Brown wrote:
> On Tue, Sep 10, 2013 at 10:04:21PM +0200, Rafael J. Wysocki wrote:
> > On Tuesday, September 10, 2013 05:13:21 PM Mark Brown wrote:
> 
> > > OK, that is very much not the model which embedded systems follow, in
> > > embedded systems the driver for the device is fully in control of its
> > > own power.  It gets resources like GPIOs and regulators which allow it
> > > to make fine grained decisions.
> 
> > There are platforms where those resources are simply not available for
> > direct manipulation and we need to use ACPI methods for power management.
> 
> > Now, since those methods are used in pretty much the same way for all I2C
> > devices, we add a PM domain for that to avoid duplicating the same code in
> > all of the drivers in question (patch [2/2]).  Does that make sense to you?
> 
> It doesn't seem like a particular problem, but the existing usage does
> need to be preserved for the systems that use it so things like having
> auto as the default and updating the drivers seem like they're needed.
> 
> > > If we're starting to get a reasonable number of buses following the same
> > > pattern it seems like we're in a position to start 
> 
> > We need that for exactly 3 buses: platform (already done), I2C and SPI.
> 
> > No other bus types are going to use ACPI this way for PM, at least for the
> > time being, simply because PCI, USB and SATA/IDE have their own ways of doing
> > this (which are bus-specific) and the spec doesn't cover other bus types
> > directly (it defines support for UART, but we don't have a UART bus type).
> 
> > Moreover, because PCI and USB use ACPI for PM in their own ways, moving that
> > thing up to the driver core would be rather inconvenient.
> 
> That only applies to the power domains though, what Mika was saying was
> that the process for enabling runtime PM (just drop a reference) also
> becomes easier with this method regardless of anything else - that makes
> sense to me as something we might want to end up with so do we want to
> just move towards making that a default?

Possibly. :-)

At least I don't see any fundamental obstacles ...


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH RESEND 1/2] i2c: prepare runtime PM support for I2C client devices

[Aaron Lu]

On 09/11/2013 06:32 AM, Rafael J. Wysocki wrote:
> On Tuesday, September 10, 2013 10:35:22 PM Mark Brown wrote:
>> On Tue, Sep 10, 2013 at 10:04:21PM +0200, Rafael J. Wysocki wrote:
>>> On Tuesday, September 10, 2013 05:13:21 PM Mark Brown wrote:
>>
>>>> OK, that is very much not the model which embedded systems follow, in
>>>> embedded systems the driver for the device is fully in control of its
>>>> own power.  It gets resources like GPIOs and regulators which allow it
>>>> to make fine grained decisions.
>>
>>> There are platforms where those resources are simply not available for
>>> direct manipulation and we need to use ACPI methods for power management.
>>
>>> Now, since those methods are used in pretty much the same way for all I2C
>>> devices, we add a PM domain for that to avoid duplicating the same code in
>>> all of the drivers in question (patch [2/2]).  Does that make sense to you?
>>
>> It doesn't seem like a particular problem, but the existing usage does
>> need to be preserved for the systems that use it so things like having
>> auto as the default and updating the drivers seem like they're needed.
>>
>>>> If we're starting to get a reasonable number of buses following the same
>>>> pattern it seems like we're in a position to start 
>>
>>> We need that for exactly 3 buses: platform (already done), I2C and SPI.
>>
>>> No other bus types are going to use ACPI this way for PM, at least for the
>>> time being, simply because PCI, USB and SATA/IDE have their own ways of doing
>>> this (which are bus-specific) and the spec doesn't cover other bus types
>>> directly (it defines support for UART, but we don't have a UART bus type).
>>
>>> Moreover, because PCI and USB use ACPI for PM in their own ways, moving that
>>> thing up to the driver core would be rather inconvenient.
>>
>> That only applies to the power domains though, what Mika was saying was
>> that the process for enabling runtime PM (just drop a reference) also
>> becomes easier with this method regardless of anything else - that makes
>> sense to me as something we might want to end up with so do we want to
>> just move towards making that a default?
> 
> Possibly. :-)
> 
> At least I don't see any fundamental obstacles ...

Looks like, it all boils down to how many I2C devices should be allowed
for runtime PM by default and how many I2C devices should be forbidden.
, and then we allow/forbid runtime PM for the majority case in I2C core
while individual driver can still forbid/allow it in their own code.

So if the majority case is runtime PM should be allowed by default, I'm
also OK to not forbid runtime PM for I2C client device in I2C core. My
original intention to forbid runtime PM by default is to make sure no
adverse effect would occur to some I2C devices that used to work well
before runtime PM.

Thanks,
Aaron

Re: [PATCH RESEND 1/2] i2c: prepare runtime PM support for I2C client devices

[Mark Brown]

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

On Wed, Sep 11, 2013 at 09:01:16AM +0800, Aaron Lu wrote:

> Looks like, it all boils down to how many I2C devices should be allowed
> for runtime PM by default and how many I2C devices should be forbidden.
> , and then we allow/forbid runtime PM for the majority case in I2C core
> while individual driver can still forbid/allow it in their own code.

> So if the majority case is runtime PM should be allowed by default, I'm
> also OK to not forbid runtime PM for I2C client device in I2C core. My
> original intention to forbid runtime PM by default is to make sure no
> adverse effect would occur to some I2C devices that used to work well
> before runtime PM.

The really big problem here is that there are I2C devices currently
using runtime PM quite happily and forbidding it by default will break
them.

In general though requiring userspace to manually activate power saving
features isn't going to make people happy.

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

Re: [PATCH RESEND 1/2] i2c: prepare runtime PM support for I2C client devices

[Mika Westerberg]

On Wed, Sep 11, 2013 at 10:55:52AM +0100, Mark Brown wrote:
> On Wed, Sep 11, 2013 at 09:01:16AM +0800, Aaron Lu wrote:
> 
> > Looks like, it all boils down to how many I2C devices should be allowed
> > for runtime PM by default and how many I2C devices should be forbidden.
> > , and then we allow/forbid runtime PM for the majority case in I2C core
> > while individual driver can still forbid/allow it in their own code.
> 
> > So if the majority case is runtime PM should be allowed by default, I'm
> > also OK to not forbid runtime PM for I2C client device in I2C core. My
> > original intention to forbid runtime PM by default is to make sure no
> > adverse effect would occur to some I2C devices that used to work well
> > before runtime PM.
> 
> The really big problem here is that there are I2C devices currently
> using runtime PM quite happily and forbidding it by default will break
> them.
> 
> In general though requiring userspace to manually activate power saving
> features isn't going to make people happy.

Yeah, we are going change that in the next revision (default to RPM
unblocked).

I'll also look into converting the existing I2C client drivers to use this
method. One question, though, is it better to have the conversion in the
same patch that introduces the I2C core runtime PM change or as a separate
patch?

Re: [PATCH RESEND 1/2] i2c: prepare runtime PM support for I2C client devices

[Mark Brown]

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

On Wed, Sep 11, 2013 at 02:05:43PM +0300, Mika Westerberg wrote:

> I'll also look into converting the existing I2C client drivers to use this
> method. One question, though, is it better to have the conversion in the
> same patch that introduces the I2C core runtime PM change or as a separate
> patch?

In theory it ought to be part of the same patch but in practice a brief
bit of bisection breakage on a single branch is probably worth the ease
of review from my point of view, others may disagree though.  Like I say
I think you'll need to convert SPI at the same time due to the devices
with both buses sharing code.

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

Re: [PATCH RESEND 1/2] i2c: prepare runtime PM support for I2C client devices

[Mika Westerberg]

On Wed, Sep 11, 2013 at 12:14:09PM +0100, Mark Brown wrote:
> On Wed, Sep 11, 2013 at 02:05:43PM +0300, Mika Westerberg wrote:
> 
> > I'll also look into converting the existing I2C client drivers to use this
> > method. One question, though, is it better to have the conversion in the
> > same patch that introduces the I2C core runtime PM change or as a separate
> > patch?
> 
> In theory it ought to be part of the same patch but in practice a brief
> bit of bisection breakage on a single branch is probably worth the ease
> of review from my point of view, others may disagree though.

OK, I'll keep them separate, unless there are objections.

> Like I say I think you'll need to convert SPI at the same time due to the
> devices with both buses sharing code.

Yes, I'm looking into that :)

Re: [PATCH RESEND 1/2] i2c: prepare runtime PM support for I2C client devices

[Kevin Hilman]

Aaron Lu <aaron.lu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> writes:

> On 09/11/2013 06:32 AM, Rafael J. Wysocki wrote:
>> On Tuesday, September 10, 2013 10:35:22 PM Mark Brown wrote:
>>> On Tue, Sep 10, 2013 at 10:04:21PM +0200, Rafael J. Wysocki wrote:
>>>> On Tuesday, September 10, 2013 05:13:21 PM Mark Brown wrote:
>>>
>>>>> OK, that is very much not the model which embedded systems follow, in
>>>>> embedded systems the driver for the device is fully in control of its
>>>>> own power.  It gets resources like GPIOs and regulators which allow it
>>>>> to make fine grained decisions.
>>>
>>>> There are platforms where those resources are simply not available for
>>>> direct manipulation and we need to use ACPI methods for power management.
>>>
>>>> Now, since those methods are used in pretty much the same way for all I2C
>>>> devices, we add a PM domain for that to avoid duplicating the same code in
>>>> all of the drivers in question (patch [2/2]).  Does that make sense to you?
>>>
>>> It doesn't seem like a particular problem, but the existing usage does
>>> need to be preserved for the systems that use it so things like having
>>> auto as the default and updating the drivers seem like they're needed.
>>>
>>>>> If we're starting to get a reasonable number of buses following the same
>>>>> pattern it seems like we're in a position to start 
>>>
>>>> We need that for exactly 3 buses: platform (already done), I2C and SPI.
>>>
>>>> No other bus types are going to use ACPI this way for PM, at least for the
>>>> time being, simply because PCI, USB and SATA/IDE have their own ways of doing
>>>> this (which are bus-specific) and the spec doesn't cover other bus types
>>>> directly (it defines support for UART, but we don't have a UART bus type).
>>>
>>>> Moreover, because PCI and USB use ACPI for PM in their own ways, moving that
>>>> thing up to the driver core would be rather inconvenient.
>>>
>>> That only applies to the power domains though, what Mika was saying was
>>> that the process for enabling runtime PM (just drop a reference) also
>>> becomes easier with this method regardless of anything else - that makes
>>> sense to me as something we might want to end up with so do we want to
>>> just move towards making that a default?
>> 
>> Possibly. :-)
>> 
>> At least I don't see any fundamental obstacles ...
>
> Looks like, it all boils down to how many I2C devices should be allowed
> for runtime PM by default and how many I2C devices should be forbidden.
> , and then we allow/forbid runtime PM for the majority case in I2C core
> while individual driver can still forbid/allow it in their own code.
>
> So if the majority case is runtime PM should be allowed by default, I'm
> also OK to not forbid runtime PM for I2C client device in I2C core. My
> original intention to forbid runtime PM by default is to make sure no
> adverse effect would occur to some I2C devices that used to work well
> before runtime PM.

IMO, this decision belongs to the PM domain, not to the core.  We have
an established legacy with the current core default (auto) and changing
that means lots of breakage.

The "forbid by default" can just as easily be handled in the PM domain
for the group of devices that need it, so why not do it there?

Kevin

Re: [PATCH RESEND 1/2] i2c: prepare runtime PM support for I2C client devices

[Mark Brown]

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

On Thu, Sep 12, 2013 at 02:07:48PM -0700, Kevin Hilman wrote:

> IMO, this decision belongs to the PM domain, not to the core.  We have
> an established legacy with the current core default (auto) and changing
> that means lots of breakage.

Yup.

> The "forbid by default" can just as easily be handled in the PM domain
> for the group of devices that need it, so why not do it there?

Or at the device level - I'd guess most I2C devices won't end up in a
domain outside of ACPI.  Mika's latest version of the patches address
this issue, the default is left alone.

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

[PATCH RESEND 2/2] i2c: attach/detach I2C client device to the ACPI power domain

[Mika Westerberg]

From: Lv Zheng <lv.zheng@intel.com>

If the I2C client device is enumerated from ACPI namespace it might have
ACPI methods that needs to be called in order to transition the device to
a different power states (such as _PSx).

Implement this for I2C client devices by checking if the device has an ACPI
handle and if that's the case, attach it to the ACPI power domain. In
addition we make sure that the device is fully powered when its ->probe()
function gets called.

For non-ACPI devices this patch is a no-op.

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/i2c/i2c-core.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 8fad5ac..fdf086b 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -257,6 +257,9 @@ static int i2c_device_probe(struct device *dev)
 	/* Make sure the adapter is active */
 	pm_runtime_get_sync(&client->adapter->dev);
 
+	if (ACPI_HANDLE(&client->dev))
+		acpi_dev_pm_attach(&client->dev, true);
+
 	/*
 	 * Enable runtime PM for the client device. If the client wants to
 	 * participate on runtime PM it should call pm_runtime_put() in its
@@ -278,6 +281,9 @@ static int i2c_device_probe(struct device *dev)
 		pm_runtime_disable(&client->dev);
 		pm_runtime_set_suspended(&client->dev);
 		pm_runtime_put_noidle(&client->dev);
+
+		if (ACPI_HANDLE(&client->dev))
+			acpi_dev_pm_detach(&client->dev, true);
 	}
 
 	pm_runtime_put(&client->adapter->dev);
@@ -314,6 +320,9 @@ static int i2c_device_remove(struct device *dev)
 	pm_runtime_set_suspended(&client->dev);
 	pm_runtime_put_noidle(&client->dev);
 
+	if (ACPI_HANDLE(&client->dev))
+		acpi_dev_pm_detach(&client->dev, true);
+
 	pm_runtime_put(&client->adapter->dev);
 	return status;
 }
-- 
1.8.4.rc2