Hi,
This is second version of the patches. The previous version can be found
here:
http://www.spinics.net/lists/linux-i2c/msg13152.html
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.
In the previous version we did this only for the I2C bus. However, Mark
Brown suggested that the same thing should be done also for the SPI bus.
This series now attempts to address that as well.
In addition we convert the existing I2C client drivers to use this model to
prevent any regressions from happening. I split the patches per subsystem
and the maintainers should be in CC list.
Note that I don't have hardware to test these existing I2C drivers so they
are only compile tested (well, except the s5p-tv which I wasn't able to
compile at all on x86).
Since the I2C patches are dependent on the core support I suggest that if this
gets merged, they would go through the I2C tree.
The SPI part could go separately via SPI tree as there are no existing
runtime PM users.
Changes to the previous version:
* Runtime PM is now unblocked by default.
* The existing I2C drivers have been converted to this model.
* Added runtime PM support for SPI devices.
[ Rafael ACKed the previous version but I've changed the patches a bit, so I
only kept his ACK in the second patch (it is unchanged). ]
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
Mika Westerberg (7):
Input: misc - convert existing I2C client drivers to use I2C core runtime PM
[media] s5p-tv: convert to use I2C core runtime PM
drivers/misc: convert existing I2C clients driver to use I2C core runtime PM
mfd: wm8994: convert to use I2C core runtime PM
ASoC: codecs: convert existing I2C client drivers to use I2C core runtime PM
spi: prepare runtime PM support for SPI devices
spi: attach/detach SPI device to the ACPI power domain
drivers/i2c/i2c-core.c | 53 +++++++++++++++++++++++++-
drivers/input/misc/bma150.c | 4 +-
drivers/input/misc/mpu3050.c | 16 ++------
drivers/media/platform/s5p-tv/sii9234_drv.c | 30 +++------------
drivers/mfd/wm8994-core.c | 5 +--
drivers/misc/apds9802als.c | 7 +---
drivers/misc/apds990x.c | 16 +++-----
drivers/misc/bh1770glc.c | 12 +++---
drivers/misc/fsa9480.c | 2 -
drivers/misc/isl29020.c | 3 +-
drivers/spi/spi.c | 58 ++++++++++++++++++++++++++++-
sound/soc/codecs/wm2200.c | 12 +++---
sound/soc/codecs/wm5100.c | 8 ++--
sound/soc/codecs/wm8962.c | 5 +--
14 files changed, 147 insertions(+), 84 deletions(-)
--
1.8.4.rc3
From: Lv Zheng <[email protected]>
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
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 <[email protected]>
Signed-off-by: Mika Westerberg <[email protected]>
Acked-by: Rafael J. Wysocki <[email protected]>
---
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 44374b4..ac4ea1f 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -251,6 +251,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
@@ -268,6 +271,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);
@@ -304,6 +310,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.rc3
If the SPI device is enumerated from ACPI namespace (it has an ACPI handle)
it might have ACPI methods that needs to be called in order to transition
the device to different power states (such as _PSx).
We follow what has been done for platform and I2C buses here and attach the
SPI device to the ACPI power domain if the device has an ACPI handle. This
makes sure that the device is powered on when its ->probe() is called.
For non-ACPI devices this patch is a no-op.
Signed-off-by: Mika Westerberg <[email protected]>
---
drivers/spi/spi.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 94ebab9..cac0ca2 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -246,6 +246,9 @@ static int spi_drv_probe(struct device *dev)
/* Make sure that the master is powered on */
pm_runtime_get_sync(&spi->master->dev);
+ if (ACPI_HANDLE(&spi->dev))
+ acpi_dev_pm_attach(&spi->dev, true);
+
/*
* Enable runtime PM for the SPI device. The SPI device driver can
* participate in runtime PM by calling pm_runtime_put() in its
@@ -260,6 +263,9 @@ static int spi_drv_probe(struct device *dev)
pm_runtime_disable(&spi->dev);
pm_runtime_set_suspended(&spi->dev);
pm_runtime_put_noidle(&spi->dev);
+
+ if (ACPI_HANDLE(&spi->dev))
+ acpi_dev_pm_detach(&spi->dev, true);
}
pm_runtime_put(&spi->master->dev);
@@ -282,6 +288,9 @@ static int spi_drv_remove(struct device *dev)
pm_runtime_set_suspended(&spi->dev);
pm_runtime_put_noidle(&spi->dev);
+ if (ACPI_HANDLE(&spi->dev))
+ acpi_dev_pm_detach(&spi->dev, true);
+
pm_runtime_put(&spi->master->dev);
return ret;
--
1.8.4.rc3
The I2C core now prepares runtime PM on behalf of the I2C client device, so
only thing the driver needs to do is to call pm_runtime_put() at the end of
its ->probe().
This patch converts I2C client drivers under drivers/misc to use this
model.
Signed-off-by: Mika Westerberg <[email protected]>
---
drivers/misc/apds9802als.c | 7 +------
drivers/misc/apds990x.c | 16 ++++++----------
drivers/misc/bh1770glc.c | 12 +++++-------
drivers/misc/fsa9480.c | 2 --
drivers/misc/isl29020.c | 3 ++-
5 files changed, 14 insertions(+), 26 deletions(-)
diff --git a/drivers/misc/apds9802als.c b/drivers/misc/apds9802als.c
index 0c6e037..7a177cd 100644
--- a/drivers/misc/apds9802als.c
+++ b/drivers/misc/apds9802als.c
@@ -246,8 +246,7 @@ static int apds9802als_probe(struct i2c_client *client,
als_set_default_config(client);
mutex_init(&data->mutex);
- pm_runtime_set_active(&client->dev);
- pm_runtime_enable(&client->dev);
+ pm_runtime_put(&client->dev);
return res;
als_error1:
@@ -264,10 +263,6 @@ static int apds9802als_remove(struct i2c_client *client)
als_set_power_state(client, false);
sysfs_remove_group(&client->dev.kobj, &m_als_gr);
- pm_runtime_disable(&client->dev);
- pm_runtime_set_suspended(&client->dev);
- pm_runtime_put_noidle(&client->dev);
-
kfree(data);
return 0;
}
diff --git a/drivers/misc/apds990x.c b/drivers/misc/apds990x.c
index 868a30a..3bb87a8 100644
--- a/drivers/misc/apds990x.c
+++ b/drivers/misc/apds990x.c
@@ -1140,14 +1140,10 @@ static int apds990x_probe(struct i2c_client *client,
goto fail3;
}
- pm_runtime_set_active(&client->dev);
-
apds990x_configure(chip);
apds990x_set_arate(chip, APDS_LUX_DEFAULT_RATE);
apds990x_mode_on(chip);
- pm_runtime_enable(&client->dev);
-
if (chip->pdata->setup_resources) {
err = chip->pdata->setup_resources();
if (err) {
@@ -1173,6 +1169,9 @@ static int apds990x_probe(struct i2c_client *client,
client->irq);
goto fail5;
}
+
+ pm_runtime_put(&client->dev);
+
return err;
fail5:
sysfs_remove_group(&chip->client->dev.kobj,
@@ -1193,6 +1192,8 @@ static int apds990x_remove(struct i2c_client *client)
{
struct apds990x_chip *chip = i2c_get_clientdata(client);
+ pm_runtime_get_sync(&client->dev);
+
free_irq(client->irq, chip);
sysfs_remove_group(&chip->client->dev.kobj,
apds990x_attribute_group);
@@ -1200,12 +1201,7 @@ static int apds990x_remove(struct i2c_client *client)
if (chip->pdata && chip->pdata->release_resources)
chip->pdata->release_resources();
- if (!pm_runtime_suspended(&client->dev))
- apds990x_chip_off(chip);
-
- pm_runtime_disable(&client->dev);
- pm_runtime_set_suspended(&client->dev);
-
+ apds990x_chip_off(chip);
regulator_bulk_free(ARRAY_SIZE(chip->regs), chip->regs);
kfree(chip);
diff --git a/drivers/misc/bh1770glc.c b/drivers/misc/bh1770glc.c
index 99a0468..7a85328 100644
--- a/drivers/misc/bh1770glc.c
+++ b/drivers/misc/bh1770glc.c
@@ -1245,8 +1245,6 @@ static int bh1770_probe(struct i2c_client *client,
/* Start chip */
bh1770_chip_on(chip);
- pm_runtime_set_active(&client->dev);
- pm_runtime_enable(&client->dev);
chip->lux_corr = bh1770_get_corr_value(chip);
if (chip->lux_corr == 0) {
@@ -1286,6 +1284,8 @@ static int bh1770_probe(struct i2c_client *client,
goto fail5;
}
regulator_bulk_disable(ARRAY_SIZE(chip->regs), chip->regs);
+ pm_runtime_put(&client->dev);
+
return err;
fail5:
sysfs_remove_group(&chip->client->dev.kobj,
@@ -1306,6 +1306,8 @@ static int bh1770_remove(struct i2c_client *client)
{
struct bh1770_chip *chip = i2c_get_clientdata(client);
+ pm_runtime_get_sync(&client->dev);
+
free_irq(client->irq, chip);
sysfs_remove_group(&chip->client->dev.kobj,
@@ -1316,11 +1318,7 @@ static int bh1770_remove(struct i2c_client *client)
cancel_delayed_work_sync(&chip->prox_work);
- if (!pm_runtime_suspended(&client->dev))
- bh1770_chip_off(chip);
-
- pm_runtime_disable(&client->dev);
- pm_runtime_set_suspended(&client->dev);
+ bh1770_chip_off(chip);
regulator_bulk_free(ARRAY_SIZE(chip->regs), chip->regs);
kfree(chip);
diff --git a/drivers/misc/fsa9480.c b/drivers/misc/fsa9480.c
index a725c79..1a7dc8b 100644
--- a/drivers/misc/fsa9480.c
+++ b/drivers/misc/fsa9480.c
@@ -450,8 +450,6 @@ static int fsa9480_probe(struct i2c_client *client,
/* device detection */
fsa9480_detect_dev(usbsw, INT_ATTACH);
- pm_runtime_set_active(&client->dev);
-
return 0;
fail2:
diff --git a/drivers/misc/isl29020.c b/drivers/misc/isl29020.c
index b7f84da..7bf15b8 100644
--- a/drivers/misc/isl29020.c
+++ b/drivers/misc/isl29020.c
@@ -179,12 +179,13 @@ static int isl29020_probe(struct i2c_client *client,
}
dev_info(&client->dev, "%s isl29020: ALS chip found\n", client->name);
als_set_power_state(client, 0);
- pm_runtime_enable(&client->dev);
+ pm_runtime_put(&client->dev);
return res;
}
static int isl29020_remove(struct i2c_client *client)
{
+ pm_runtime_get(&client->dev);
sysfs_remove_group(&client->dev.kobj, &m_als_gr);
return 0;
}
--
1.8.4.rc3
This patch adds runtime PM support for the SPI bus analogous to what has
been done for the I2C bus. This means that the SPI 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 participate the runtime PM of the SPI bus a device driver needs
to 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.
If the driver doesn't support runtime PM, the device in question is
regarded as being runtime PM active and powered on.
This patch adds also runtime PM support for the SPI master device because
it is needed to be able to runtime power manage the SPI controller device.
The SPI master device is handled along with the SPI controller device.
Signed-off-by: Mika Westerberg <[email protected]>
---
drivers/spi/spi.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 47 insertions(+), 2 deletions(-)
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 978dda2..94ebab9 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -240,22 +240,61 @@ EXPORT_SYMBOL_GPL(spi_bus_type);
static int spi_drv_probe(struct device *dev)
{
const struct spi_driver *sdrv = to_spi_driver(dev->driver);
+ struct spi_device *spi = to_spi_device(dev);
+ int ret;
- return sdrv->probe(to_spi_device(dev));
+ /* Make sure that the master is powered on */
+ pm_runtime_get_sync(&spi->master->dev);
+
+ /*
+ * Enable runtime PM for the SPI device. The SPI device driver can
+ * participate in runtime PM by calling pm_runtime_put() in its
+ * probe() callback.
+ */
+ pm_runtime_get_noresume(&spi->dev);
+ pm_runtime_set_active(&spi->dev);
+ pm_runtime_enable(&spi->dev);
+
+ ret = sdrv->probe(spi);
+ if (ret) {
+ pm_runtime_disable(&spi->dev);
+ pm_runtime_set_suspended(&spi->dev);
+ pm_runtime_put_noidle(&spi->dev);
+ }
+
+ pm_runtime_put(&spi->master->dev);
+
+ return ret;
}
static int spi_drv_remove(struct device *dev)
{
const struct spi_driver *sdrv = to_spi_driver(dev->driver);
+ struct spi_device *spi = to_spi_device(dev);
+ int ret;
+
+ pm_runtime_get_sync(&spi->master->dev);
+
+ ret = sdrv->remove(spi);
+
+ /* Undo the runtime PM done in spi_drv_probe() */
+ pm_runtime_disable(&spi->dev);
+ pm_runtime_set_suspended(&spi->dev);
+ pm_runtime_put_noidle(&spi->dev);
- return sdrv->remove(to_spi_device(dev));
+ pm_runtime_put(&spi->master->dev);
+
+ return ret;
}
static void spi_drv_shutdown(struct device *dev)
{
const struct spi_driver *sdrv = to_spi_driver(dev->driver);
+ struct spi_device *spi = to_spi_device(dev);
+ pm_runtime_get_sync(&spi->master->dev);
sdrv->shutdown(to_spi_device(dev));
+ pm_runtime_put(&spi->master->dev);
}
/**
@@ -1174,6 +1213,10 @@ int spi_register_master(struct spi_master *master)
}
}
+ pm_runtime_set_active(&master->dev);
+ pm_runtime_no_callbacks(&master->dev);
+ pm_runtime_enable(&master->dev);
+
mutex_lock(&board_lock);
list_add_tail(&master->list, &spi_master_list);
list_for_each_entry(bi, &board_list, list)
@@ -1217,6 +1260,8 @@ void spi_unregister_master(struct spi_master *master)
list_del(&master->list);
mutex_unlock(&board_lock);
+ pm_runtime_disable(&master->dev);
+
dummy = device_for_each_child(&master->dev, NULL, __unregister);
device_unregister(&master->dev);
}
--
1.8.4.rc3
From: Aaron Lu <[email protected]>
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.
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 <[email protected]>
Signed-off-by: Mika Westerberg <[email protected]>
---
drivers/i2c/i2c-core.c | 44 +++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 43 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index f32ca29..44374b4 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -248,11 +248,30 @@ 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.
+ */
+ 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;
}
@@ -265,6 +284,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");
@@ -277,6 +298,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;
}
@@ -288,8 +316,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
@@ -1066,6 +1097,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:
@@ -1230,6 +1270,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.rc3
The I2C core now prepares runtime PM on behalf of the I2C client device, so
only thing the driver needs to do is to call pm_runtime_put() at the end of
its ->probe().
This patch converts wm8994 driver to use this model.
Signed-off-by: Mika Westerberg <[email protected]>
---
drivers/mfd/wm8994-core.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/mfd/wm8994-core.c b/drivers/mfd/wm8994-core.c
index 3fdee90..9928bb1 100644
--- a/drivers/mfd/wm8994-core.c
+++ b/drivers/mfd/wm8994-core.c
@@ -706,8 +706,7 @@ static int wm8994_device_init(struct wm8994 *wm8994, int irq)
goto err_irq;
}
- pm_runtime_enable(wm8994->dev);
- pm_runtime_idle(wm8994->dev);
+ pm_runtime_put(wm8994->dev);
return 0;
@@ -723,7 +722,7 @@ err:
static void wm8994_device_exit(struct wm8994 *wm8994)
{
- pm_runtime_disable(wm8994->dev);
+ pm_runtime_get(wm8994->dev);
mfd_remove_devices(wm8994->dev);
wm8994_irq_exit(wm8994);
regulator_bulk_disable(wm8994->num_supplies,
--
1.8.4.rc3
The I2C core now prepares runtime PM on behalf of the I2C client device, so
only thing the driver needs to do is to call pm_runtime_put() at the end of
its ->probe().
This patch converts ASoC codec drivers to use this model.
Signed-off-by: Mika Westerberg <[email protected]>
---
sound/soc/codecs/wm2200.c | 12 +++++-------
sound/soc/codecs/wm5100.c | 8 ++++----
sound/soc/codecs/wm8962.c | 5 ++---
3 files changed, 11 insertions(+), 14 deletions(-)
diff --git a/sound/soc/codecs/wm2200.c b/sound/soc/codecs/wm2200.c
index 57ba315..469b67f 100644
--- a/sound/soc/codecs/wm2200.c
+++ b/sound/soc/codecs/wm2200.c
@@ -2407,21 +2407,17 @@ static int wm2200_i2c_probe(struct i2c_client *i2c,
i2c->irq, ret);
}
- pm_runtime_set_active(&i2c->dev);
- pm_runtime_enable(&i2c->dev);
- pm_request_idle(&i2c->dev);
-
ret = snd_soc_register_codec(&i2c->dev, &soc_codec_wm2200,
&wm2200_dai, 1);
if (ret != 0) {
dev_err(&i2c->dev, "Failed to register CODEC: %d\n", ret);
- goto err_pm_runtime;
+ goto err_reset;
}
+ pm_runtime_put(&i2c->dev);
+
return 0;
-err_pm_runtime:
- pm_runtime_disable(&i2c->dev);
err_reset:
if (wm2200->pdata.reset)
gpio_set_value_cansleep(wm2200->pdata.reset, 0);
@@ -2438,6 +2434,8 @@ static int wm2200_i2c_remove(struct i2c_client *i2c)
{
struct wm2200_priv *wm2200 = i2c_get_clientdata(i2c);
+ pm_runtime_get(&i2c->dev);
+
snd_soc_unregister_codec(&i2c->dev);
if (i2c->irq)
free_irq(i2c->irq, wm2200);
diff --git a/sound/soc/codecs/wm5100.c b/sound/soc/codecs/wm5100.c
index ac1745d..5d3b22e 100644
--- a/sound/soc/codecs/wm5100.c
+++ b/sound/soc/codecs/wm5100.c
@@ -2612,10 +2612,6 @@ static int wm5100_i2c_probe(struct i2c_client *i2c,
}
}
- pm_runtime_set_active(&i2c->dev);
- pm_runtime_enable(&i2c->dev);
- pm_request_idle(&i2c->dev);
-
ret = snd_soc_register_codec(&i2c->dev,
&soc_codec_dev_wm5100, wm5100_dai,
ARRAY_SIZE(wm5100_dai));
@@ -2624,6 +2620,8 @@ static int wm5100_i2c_probe(struct i2c_client *i2c,
goto err_reset;
}
+ pm_runtime_put(&i2c->dev);
+
return ret;
err_reset:
@@ -2650,6 +2648,8 @@ static int wm5100_i2c_remove(struct i2c_client *i2c)
{
struct wm5100_priv *wm5100 = i2c_get_clientdata(i2c);
+ pm_runtime_get(&i2c->dev);
+
snd_soc_unregister_codec(&i2c->dev);
if (i2c->irq)
free_irq(i2c->irq, wm5100);
diff --git a/sound/soc/codecs/wm8962.c b/sound/soc/codecs/wm8962.c
index e2de9ec..4fcf7db 100644
--- a/sound/soc/codecs/wm8962.c
+++ b/sound/soc/codecs/wm8962.c
@@ -3715,9 +3715,6 @@ static int wm8962_i2c_probe(struct i2c_client *i2c,
ret);
}
- pm_runtime_enable(&i2c->dev);
- pm_request_idle(&i2c->dev);
-
ret = snd_soc_register_codec(&i2c->dev,
&soc_codec_dev_wm8962, &wm8962_dai, 1);
if (ret < 0)
@@ -3725,6 +3722,7 @@ static int wm8962_i2c_probe(struct i2c_client *i2c,
/* The drivers should power up as needed */
regulator_bulk_disable(ARRAY_SIZE(wm8962->supplies), wm8962->supplies);
+ pm_runtime_put(&i2c->dev);
return 0;
@@ -3736,6 +3734,7 @@ err:
static int wm8962_i2c_remove(struct i2c_client *client)
{
+ pm_runtime_get(&client->dev);
snd_soc_unregister_codec(&client->dev);
return 0;
}
--
1.8.4.rc3
The I2C core now prepares runtime PM on behalf of the I2C client device, so
only thing the driver needs to do is to call pm_runtime_put() at the end of
its ->probe().
This patch converts s5p-tv driver to use this model.
Signed-off-by: Mika Westerberg <[email protected]>
---
drivers/media/platform/s5p-tv/sii9234_drv.c | 30 ++++++-----------------------
1 file changed, 6 insertions(+), 24 deletions(-)
diff --git a/drivers/media/platform/s5p-tv/sii9234_drv.c b/drivers/media/platform/s5p-tv/sii9234_drv.c
index 3dd762e..c0e8d42 100644
--- a/drivers/media/platform/s5p-tv/sii9234_drv.c
+++ b/drivers/media/platform/s5p-tv/sii9234_drv.c
@@ -325,8 +325,7 @@ static int sii9234_probe(struct i2c_client *client,
ctx = devm_kzalloc(&client->dev, sizeof(*ctx), GFP_KERNEL);
if (!ctx) {
dev_err(dev, "out of memory\n");
- ret = -ENOMEM;
- goto fail;
+ return -ENOMEM;
}
ctx->client = client;
@@ -345,42 +344,25 @@ static int sii9234_probe(struct i2c_client *client,
v4l2_i2c_subdev_init(&ctx->sd, client, &sii9234_ops);
- pm_runtime_enable(dev);
-
- /* enable device */
- ret = pm_runtime_get_sync(dev);
- if (ret)
- goto fail_pm;
-
/* verify chip version */
ret = sii9234_verify_version(client);
- if (ret)
- goto fail_pm_get;
+ if (ret) {
+ dev_err(dev, "failed to verify chip version\n");
+ return ret;
+ }
- /* stop processing */
pm_runtime_put(dev);
dev_info(dev, "probe successful\n");
return 0;
-
-fail_pm_get:
- pm_runtime_put_sync(dev);
-
-fail_pm:
- pm_runtime_disable(dev);
-
-fail:
- dev_err(dev, "probe failed\n");
-
- return ret;
}
static int sii9234_remove(struct i2c_client *client)
{
struct device *dev = &client->dev;
- pm_runtime_disable(dev);
+ pm_runtime_get(dev);
dev_info(dev, "remove successful\n");
--
1.8.4.rc3
The I2C core now prepares runtime PM on behalf of the I2C client device, so
only thing the driver needs to do is to call pm_runtime_put() at the end of
->probe().
This patch converts bma150 and mpu3050 input drivers to use this model.
While we are there remove call to pm_runtime_set_autosuspend_delay() in
mpu3050 driver because the driver doesn't seem to use autosuspend anyway.
Signed-off-by: Mika Westerberg <[email protected]>
---
drivers/input/misc/bma150.c | 4 ++--
drivers/input/misc/mpu3050.c | 16 ++++------------
2 files changed, 6 insertions(+), 14 deletions(-)
diff --git a/drivers/input/misc/bma150.c b/drivers/input/misc/bma150.c
index 865c2f9..f1bf5e9 100644
--- a/drivers/input/misc/bma150.c
+++ b/drivers/input/misc/bma150.c
@@ -592,7 +592,7 @@ static int bma150_probe(struct i2c_client *client,
i2c_set_clientdata(client, bma150);
- pm_runtime_enable(&client->dev);
+ pm_runtime_put(&client->dev);
return 0;
@@ -605,7 +605,7 @@ static int bma150_remove(struct i2c_client *client)
{
struct bma150_data *bma150 = i2c_get_clientdata(client);
- pm_runtime_disable(&client->dev);
+ pm_runtime_get(&client->dev);
if (client->irq > 0) {
free_irq(client->irq, bma150);
diff --git a/drivers/input/misc/mpu3050.c b/drivers/input/misc/mpu3050.c
index dce0d95..9f2ead5 100644
--- a/drivers/input/misc/mpu3050.c
+++ b/drivers/input/misc/mpu3050.c
@@ -43,8 +43,6 @@
#define MPU3050_CHIP_ID 0x69
-#define MPU3050_AUTO_DELAY 1000
-
#define MPU3050_MIN_VALUE -32768
#define MPU3050_MAX_VALUE 32767
@@ -359,11 +357,9 @@ static int mpu3050_probe(struct i2c_client *client,
input_set_drvdata(idev, sensor);
- pm_runtime_set_active(&client->dev);
-
error = mpu3050_hw_init(sensor);
if (error)
- goto err_pm_set_suspended;
+ goto err_free_mem;
error = request_threaded_irq(client->irq,
NULL, mpu3050_interrupt_thread,
@@ -372,7 +368,7 @@ static int mpu3050_probe(struct i2c_client *client,
if (error) {
dev_err(&client->dev,
"can't get IRQ %d, error %d\n", client->irq, error);
- goto err_pm_set_suspended;
+ goto err_free_mem;
}
error = input_register_device(idev);
@@ -381,15 +377,12 @@ static int mpu3050_probe(struct i2c_client *client,
goto err_free_irq;
}
- pm_runtime_enable(&client->dev);
- pm_runtime_set_autosuspend_delay(&client->dev, MPU3050_AUTO_DELAY);
+ pm_runtime_put(&client->dev);
return 0;
err_free_irq:
free_irq(client->irq, sensor);
-err_pm_set_suspended:
- pm_runtime_set_suspended(&client->dev);
err_free_mem:
input_free_device(idev);
kfree(sensor);
@@ -406,8 +399,7 @@ static int mpu3050_remove(struct i2c_client *client)
{
struct mpu3050_sensor *sensor = i2c_get_clientdata(client);
- pm_runtime_disable(&client->dev);
- pm_runtime_set_suspended(&client->dev);
+ pm_runtime_get(&client->dev);
free_irq(client->irq, sensor);
input_unregister_device(sensor->idev);
--
1.8.4.rc3
On Wed, Sep 11, 2013 at 06:32:38PM +0300, Mika Westerberg wrote:
> The I2C core now prepares runtime PM on behalf of the I2C client device, so
> only thing the driver needs to do is to call pm_runtime_put() at the end of
> its ->probe().
>
> This patch converts ASoC codec drivers to use this model.
Acked-by: Mark Brown <[email protected]>
On Wed, Sep 11, 2013 at 06:32:39PM +0300, Mika Westerberg wrote:
> This patch adds runtime PM support for the SPI bus analogous to what has
> been done for the I2C bus. This means that the SPI 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.
Acked-by: Mark Brown <[email protected]>
I would be able to have this and the other patch in the SPI tree in case
it overlaps with other work - I'm not sure what the plan will be for
merging this stuff but if there were a branch which I could merge into
the SPI tree that'd be good.
On Wed, Sep 11, 2013 at 06:32:40PM +0300, Mika Westerberg wrote:
> If the SPI device is enumerated from ACPI namespace (it has an ACPI handle)
> it might have ACPI methods that needs to be called in order to transition
> the device to different power states (such as _PSx).
Acked-by: Mark Brown <[email protected]>
On Wed, Sep 11, 2013 at 06:32:31PM +0300, Mika Westerberg wrote:
> Hi,
>
> This is second version of the patches. The previous version can be found
> here:
>
> http://www.spinics.net/lists/linux-i2c/msg13152.html
Looks good to me now:
Reviwed-by: Mark Brown <[email protected]>
for what it's worth.
Hi Mika,
On Wed, Sep 11, 2013 at 06:32:37PM +0300, Mika Westerberg wrote:
> The I2C core now prepares runtime PM on behalf of the I2C client device, so
> only thing the driver needs to do is to call pm_runtime_put() at the end of
> its ->probe().
>
> This patch converts wm8994 driver to use this model.
>
> Signed-off-by: Mika Westerberg <[email protected]>
Acked-by: Samuel Ortiz <[email protected]>
I think it would make more sense for you to merge that one together with
the related i2c changes. If you prefer that I take it through MFD,
please let me know.
Cheers,
Samuel.
--
Intel Open Source Technology Centre
http://oss.intel.com/
On Wed, Sep 11, 2013 at 06:12:43PM +0200, Samuel Ortiz wrote:
> Hi Mika,
>
> On Wed, Sep 11, 2013 at 06:32:37PM +0300, Mika Westerberg wrote:
> > The I2C core now prepares runtime PM on behalf of the I2C client device, so
> > only thing the driver needs to do is to call pm_runtime_put() at the end of
> > its ->probe().
> >
> > This patch converts wm8994 driver to use this model.
> >
> > Signed-off-by: Mika Westerberg <[email protected]>
> Acked-by: Samuel Ortiz <[email protected]>
Thanks!
> I think it would make more sense for you to merge that one together with
> the related i2c changes. If you prefer that I take it through MFD,
> please let me know.
I'm hoping that Wolfram takes all the I2C related patches to his tree.
On Wed, Sep 11, 2013 at 04:51:20PM +0100, Mark Brown wrote:
> On Wed, Sep 11, 2013 at 06:32:39PM +0300, Mika Westerberg wrote:
> > This patch adds runtime PM support for the SPI bus analogous to what has
> > been done for the I2C bus. This means that the SPI 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.
>
> Acked-by: Mark Brown <[email protected]>
Thanks!
> I would be able to have this and the other patch in the SPI tree in case
> it overlaps with other work - I'm not sure what the plan will be for
> merging this stuff but if there were a branch which I could merge into
> the SPI tree that'd be good.
I think these two can go via your SPI tree as they shouldn't have
dependencies to the I2C tree.
On Thu, Sep 12, 2013 at 12:24:47PM +0300, Mika Westerberg wrote:
> On Wed, Sep 11, 2013 at 06:12:43PM +0200, Samuel Ortiz wrote:
> > I think it would make more sense for you to merge that one together with
> > the related i2c changes. If you prefer that I take it through MFD,
> > please let me know.
>
> I'm hoping that Wolfram takes all the I2C related patches to his tree.
If Wolfram prepares an immutable branch for that, I can pull it in
mfd-next and then merge the MFD patch on top of it.
Cheers,
Samuel.
--
Intel Open Source Technology Centre
http://oss.intel.com/
On Thu, Sep 12, 2013 at 12:27:43PM +0300, Mika Westerberg wrote:
> On Wed, Sep 11, 2013 at 04:51:20PM +0100, Mark Brown wrote:
> > I would be able to have this and the other patch in the SPI tree in case
> > it overlaps with other work - I'm not sure what the plan will be for
> > merging this stuff but if there were a branch which I could merge into
> > the SPI tree that'd be good.
> I think these two can go via your SPI tree as they shouldn't have
> dependencies to the I2C tree.
There's all the driver changes though - it seems best to push the whole
series through one branch so there's fewer bisection problems.
On Thu, Sep 12, 2013 at 10:31:45AM +0100, Mark Brown wrote:
> On Thu, Sep 12, 2013 at 12:27:43PM +0300, Mika Westerberg wrote:
> > On Wed, Sep 11, 2013 at 04:51:20PM +0100, Mark Brown wrote:
>
> > > I would be able to have this and the other patch in the SPI tree in case
> > > it overlaps with other work - I'm not sure what the plan will be for
> > > merging this stuff but if there were a branch which I could merge into
> > > the SPI tree that'd be good.
>
> > I think these two can go via your SPI tree as they shouldn't have
> > dependencies to the I2C tree.
>
> There's all the driver changes though - it seems best to push the whole
> series through one branch so there's fewer bisection problems.
Ah, right. Then I suppose the right tree would be the I2C tree (as majority
of the patches are I2C related)?
Wolfram, are you OK with this?
On Thursday, September 12, 2013 12:43:02 PM Mika Westerberg wrote:
> On Thu, Sep 12, 2013 at 10:31:45AM +0100, Mark Brown wrote:
> > On Thu, Sep 12, 2013 at 12:27:43PM +0300, Mika Westerberg wrote:
> > > On Wed, Sep 11, 2013 at 04:51:20PM +0100, Mark Brown wrote:
> >
> > > > I would be able to have this and the other patch in the SPI tree in case
> > > > it overlaps with other work - I'm not sure what the plan will be for
> > > > merging this stuff but if there were a branch which I could merge into
> > > > the SPI tree that'd be good.
> >
> > > I think these two can go via your SPI tree as they shouldn't have
> > > dependencies to the I2C tree.
> >
> > There's all the driver changes though - it seems best to push the whole
> > series through one branch so there's fewer bisection problems.
>
> Ah, right. Then I suppose the right tree would be the I2C tree (as majority
> of the patches are I2C related)?
>
> Wolfram, are you OK with this?
Alternatively, I can apply them too if everyone is OK with that.
They are PM+ACPI changes after all ...
Thanks,
Rafael
On Thu, Sep 12, 2013 at 01:04:55PM +0200, Rafael J. Wysocki wrote:
> On Thursday, September 12, 2013 12:43:02 PM Mika Westerberg wrote:
> > On Thu, Sep 12, 2013 at 10:31:45AM +0100, Mark Brown wrote:
> > > On Thu, Sep 12, 2013 at 12:27:43PM +0300, Mika Westerberg wrote:
> > > > On Wed, Sep 11, 2013 at 04:51:20PM +0100, Mark Brown wrote:
> > >
> > > > > I would be able to have this and the other patch in the SPI tree in case
> > > > > it overlaps with other work - I'm not sure what the plan will be for
> > > > > merging this stuff but if there were a branch which I could merge into
> > > > > the SPI tree that'd be good.
> > >
> > > > I think these two can go via your SPI tree as they shouldn't have
> > > > dependencies to the I2C tree.
> > >
> > > There's all the driver changes though - it seems best to push the whole
> > > series through one branch so there's fewer bisection problems.
> >
> > Ah, right. Then I suppose the right tree would be the I2C tree (as majority
> > of the patches are I2C related)?
> >
> > Wolfram, are you OK with this?
>
> Alternatively, I can apply them too if everyone is OK with that.
>
> They are PM+ACPI changes after all ...
I'd like to give at least an ACK. But I need to find some time for that.
Is it urgent? Looks like 3.13 material to me...
On Thu, Sep 12, 2013 at 01:04:28PM +0200, Wolfram Sang wrote:
> On Thu, Sep 12, 2013 at 01:04:55PM +0200, Rafael J. Wysocki wrote:
> > On Thursday, September 12, 2013 12:43:02 PM Mika Westerberg wrote:
> > > On Thu, Sep 12, 2013 at 10:31:45AM +0100, Mark Brown wrote:
> > > > On Thu, Sep 12, 2013 at 12:27:43PM +0300, Mika Westerberg wrote:
> > > > > On Wed, Sep 11, 2013 at 04:51:20PM +0100, Mark Brown wrote:
> > > >
> > > > > > I would be able to have this and the other patch in the SPI tree in case
> > > > > > it overlaps with other work - I'm not sure what the plan will be for
> > > > > > merging this stuff but if there were a branch which I could merge into
> > > > > > the SPI tree that'd be good.
> > > >
> > > > > I think these two can go via your SPI tree as they shouldn't have
> > > > > dependencies to the I2C tree.
> > > >
> > > > There's all the driver changes though - it seems best to push the whole
> > > > series through one branch so there's fewer bisection problems.
> > >
> > > Ah, right. Then I suppose the right tree would be the I2C tree (as majority
> > > of the patches are I2C related)?
> > >
> > > Wolfram, are you OK with this?
> >
> > Alternatively, I can apply them too if everyone is OK with that.
> >
> > They are PM+ACPI changes after all ...
>
> I'd like to give at least an ACK. But I need to find some time for that.
> Is it urgent? Looks like 3.13 material to me...
It's not urgent. Please take your time.
On Thu, Sep 12, 2013 at 01:04:55PM +0200, Rafael J. Wysocki wrote:
> On Thursday, September 12, 2013 12:43:02 PM Mika Westerberg wrote:
> > On Thu, Sep 12, 2013 at 10:31:45AM +0100, Mark Brown wrote:
> > > On Thu, Sep 12, 2013 at 12:27:43PM +0300, Mika Westerberg wrote:
> > > > On Wed, Sep 11, 2013 at 04:51:20PM +0100, Mark Brown wrote:
> > >
> > > > > I would be able to have this and the other patch in the SPI tree in case
> > > > > it overlaps with other work - I'm not sure what the plan will be for
> > > > > merging this stuff but if there were a branch which I could merge into
> > > > > the SPI tree that'd be good.
> > >
> > > > I think these two can go via your SPI tree as they shouldn't have
> > > > dependencies to the I2C tree.
> > >
> > > There's all the driver changes though - it seems best to push the whole
> > > series through one branch so there's fewer bisection problems.
> >
> > Ah, right. Then I suppose the right tree would be the I2C tree (as majority
> > of the patches are I2C related)?
> >
> > Wolfram, are you OK with this?
>
> Alternatively, I can apply them too if everyone is OK with that.
Sounds good to me, thanks!
On Wed, Sep 11, 2013 at 06:32:36PM +0300, Mika Westerberg wrote:
> The I2C core now prepares runtime PM on behalf of the I2C client device, so
> only thing the driver needs to do is to call pm_runtime_put() at the end of
> its ->probe().
>
> This patch converts I2C client drivers under drivers/misc to use this
> model.
>
> Signed-off-by: Mika Westerberg <[email protected]>
Acked-by: Greg Kroah-Hartman <[email protected]>
Mika Westerberg <[email protected]> writes:
> From: Aaron Lu <[email protected]>
>
> 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.
>
> 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.
But for existing drivers which already support runtime PM (at least 7 by
a quick grep), they will be stuck runtime enabled and stop hitting
low-power states after this patch.
> 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 <[email protected]>
> Signed-off-by: Mika Westerberg <[email protected]>
> ---
> drivers/i2c/i2c-core.c | 44 +++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 43 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index f32ca29..44374b4 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -248,11 +248,30 @@ 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.
> + */
> + pm_runtime_get_noresume(&client->dev);
> + pm_runtime_set_active(&client->dev);
Why the set_active here?
For hardware that is disabled/powered-off on startup, there will now be
a mismatch between the hardware state an the RPM core state.
Kevin
On Thu, Sep 12, 2013 at 2:34 PM, Kevin Hilman <[email protected]> wrote:
> Mika Westerberg <[email protected]> writes:
>
>> From: Aaron Lu <[email protected]>
>>
>> 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.
>>
>> 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.
>
> But for existing drivers which already support runtime PM (at least 7 by
> a quick grep), they will be stuck runtime enabled and stop hitting
> low-power states after this patch.
Oops, nevermind. I was mixing this up with runtime PM on the i2c
adapter but this is for the i2c client.
Sorry for the noise.
Kevin
On 09/11/2013 05:32 PM, Mika Westerberg wrote:
> From: Aaron Lu<[email protected]>
>
> 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.
>
> 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<[email protected]>
> Signed-off-by: Mika Westerberg<[email protected]>
> ---
> drivers/i2c/i2c-core.c | 44 +++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 43 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index f32ca29..44374b4 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -248,11 +248,30 @@ 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);
So there is currently no way to avoid this behaviour, i.e. to have the
adapter
not activated before any of its client devices is probed, but only later on,
after explicit call to pm_runtime_get*(&client->dev) in the client driver ?
--
Thanks,
Sylwester
On 09/13/2013 06:06 AM, Sylwester Nawrocki wrote:
> On 09/11/2013 05:32 PM, Mika Westerberg wrote:
>> From: Aaron Lu<[email protected]>
>>
>> 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.
>>
>> 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<[email protected]>
>> Signed-off-by: Mika Westerberg<[email protected]>
>> ---
>> drivers/i2c/i2c-core.c | 44 +++++++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 43 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
>> index f32ca29..44374b4 100644
>> --- a/drivers/i2c/i2c-core.c
>> +++ b/drivers/i2c/i2c-core.c
>> @@ -248,11 +248,30 @@ 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);
>
> So there is currently no way to avoid this behaviour, i.e. to have the
> adapter
> not activated before any of its client devices is probed, but only later on,
> after explicit call to pm_runtime_get*(&client->dev) in the client driver ?
The above pm_runtime_get_sync is used to make sure when the client I2C
device is going to be probed, its host adapter device is turned on(or we
will fail the probe). It doesn't affect the adapter's status before the
probe of I2C client device.
Thanks,
Aaron
On Thu, Sep 12, 2013 at 02:34:21PM -0700, Kevin Hilman wrote:
> > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> > index f32ca29..44374b4 100644
> > --- a/drivers/i2c/i2c-core.c
> > +++ b/drivers/i2c/i2c-core.c
> > @@ -248,11 +248,30 @@ 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.
> > + */
> > + pm_runtime_get_noresume(&client->dev);
> > + pm_runtime_set_active(&client->dev);
>
> Why the set_active here?
>
> For hardware that is disabled/powered-off on startup, there will now be
> a mismatch between the hardware state an the RPM core state.
The call to pm_runtime_get_noresume() should make sure that the device is
in active state (at least in state where it can access the bus) if I'm
understanding this right.
On Fri, Sep 13, 2013 at 09:54:34AM +0300, Mika Westerberg wrote:
> On Thu, Sep 12, 2013 at 02:34:21PM -0700, Kevin Hilman wrote:
> > For hardware that is disabled/powered-off on startup, there will now be
> > a mismatch between the hardware state an the RPM core state.
> The call to pm_runtime_get_noresume() should make sure that the device is
> in active state (at least in state where it can access the bus) if I'm
> understanding this right.
Accessing the bus isn't an issue for I2C outside of ACPI, the power
management of the device is totally disassociated from the bus and the
controller is responsible for ensuring it is available during transfers.
On Fri, Sep 13, 2013 at 09:14:20AM +0800, Aaron Lu wrote:
> On 09/13/2013 06:06 AM, Sylwester Nawrocki wrote:
> > So there is currently no way to avoid this behaviour, i.e. to have the
> > adapter
> > not activated before any of its client devices is probed, but only later on,
> > after explicit call to pm_runtime_get*(&client->dev) in the client driver ?
> The above pm_runtime_get_sync is used to make sure when the client I2C
> device is going to be probed, its host adapter device is turned on(or we
> will fail the probe). It doesn't affect the adapter's status before the
> probe of I2C client device.
The expecation is that if the adaptor needs to do anything to transfer
it'll do that when asked to transfer - that way it can sit in a low
power state when the bus is idle.
On Fri, Sep 13, 2013 at 10:59:50AM +0100, Mark Brown wrote:
> On Fri, Sep 13, 2013 at 09:54:34AM +0300, Mika Westerberg wrote:
> > On Thu, Sep 12, 2013 at 02:34:21PM -0700, Kevin Hilman wrote:
>
> > > For hardware that is disabled/powered-off on startup, there will now be
> > > a mismatch between the hardware state an the RPM core state.
>
> > The call to pm_runtime_get_noresume() should make sure that the device is
> > in active state (at least in state where it can access the bus) if I'm
> > understanding this right.
>
> Accessing the bus isn't an issue for I2C outside of ACPI, the power
> management of the device is totally disassociated from the bus and the
> controller is responsible for ensuring it is available during transfers.
Yes, but since we want to support ACPI as well, we must make sure that the
adapter (and the associated controller) is available when client ->probe()
is called.
On Fri, Sep 13, 2013 at 01:16:11PM +0300, Mika Westerberg wrote:
> On Fri, Sep 13, 2013 at 10:59:50AM +0100, Mark Brown wrote:
> > Accessing the bus isn't an issue for I2C outside of ACPI, the power
> > management of the device is totally disassociated from the bus and the
> > controller is responsible for ensuring it is available during transfers.
> Yes, but since we want to support ACPI as well, we must make sure that the
> adapter (and the associated controller) is available when client ->probe()
> is called.
Right, but this probably needs to be highlighted more since it's a very
surprising thing for I2C and is causing confusion.
On Fri, Sep 13, 2013 at 11:31:52AM +0100, Mark Brown wrote:
> On Fri, Sep 13, 2013 at 01:16:11PM +0300, Mika Westerberg wrote:
> > On Fri, Sep 13, 2013 at 10:59:50AM +0100, Mark Brown wrote:
>
> > > Accessing the bus isn't an issue for I2C outside of ACPI, the power
> > > management of the device is totally disassociated from the bus and the
> > > controller is responsible for ensuring it is available during transfers.
>
> > Yes, but since we want to support ACPI as well, we must make sure that the
> > adapter (and the associated controller) is available when client ->probe()
> > is called.
>
> Right, but this probably needs to be highlighted more since it's a very
> surprising thing for I2C and is causing confusion.
By highlighted more, do you mean something like adding a comment in the
code about this or?
On Fri, Sep 13, 2013 at 02:50:35PM +0300, Mika Westerberg wrote:
> On Fri, Sep 13, 2013 at 11:31:52AM +0100, Mark Brown wrote:
> > Right, but this probably needs to be highlighted more since it's a very
> > surprising thing for I2C and is causing confusion.
> By highlighted more, do you mean something like adding a comment in the
> code about this or?
Perhaps, yes. Or possibly the commit log is going to be enough going
forwards.
Mika Westerberg <[email protected]> writes:
> On Thu, Sep 12, 2013 at 02:34:21PM -0700, Kevin Hilman wrote:
>> > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
>> > index f32ca29..44374b4 100644
>> > --- a/drivers/i2c/i2c-core.c
>> > +++ b/drivers/i2c/i2c-core.c
>> > @@ -248,11 +248,30 @@ 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.
>> > + */
>> > + pm_runtime_get_noresume(&client->dev);
>> > + pm_runtime_set_active(&client->dev);
>>
>> Why the set_active here?
>>
>> For hardware that is disabled/powered-off on startup, there will now be
>> a mismatch between the hardware state an the RPM core state.
>
> The call to pm_runtime_get_noresume() should make sure that the device is
> in active state (at least in state where it can access the bus) if I'm
> understanding this right.
No, after _get_noresume(), nothing happens to the hardware. It simply
increments the usecount. From pm_runtime.h:
static inline void pm_runtime_get_noresume(struct device *dev)
{
atomic_inc(&dev->power.usage_count);
}
So after the _get_noresume() and _set_active() you're very likely to
have a disconnect between the hardware state and what state RPM thinks
the hardware is in.
Kevin
On Fri, Sep 13, 2013 at 07:30:55AM -0700, Kevin Hilman wrote:
> Mika Westerberg <[email protected]> writes:
>
> > On Thu, Sep 12, 2013 at 02:34:21PM -0700, Kevin Hilman wrote:
> >> > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> >> > index f32ca29..44374b4 100644
> >> > --- a/drivers/i2c/i2c-core.c
> >> > +++ b/drivers/i2c/i2c-core.c
> >> > @@ -248,11 +248,30 @@ 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.
> >> > + */
> >> > + pm_runtime_get_noresume(&client->dev);
> >> > + pm_runtime_set_active(&client->dev);
> >>
> >> Why the set_active here?
> >>
> >> For hardware that is disabled/powered-off on startup, there will now be
> >> a mismatch between the hardware state an the RPM core state.
> >
> > The call to pm_runtime_get_noresume() should make sure that the device is
> > in active state (at least in state where it can access the bus) if I'm
> > understanding this right.
>
> No, after _get_noresume(), nothing happens to the hardware. It simply
> increments the usecount. From pm_runtime.h:
>
> static inline void pm_runtime_get_noresume(struct device *dev)
> {
> atomic_inc(&dev->power.usage_count);
> }
>
> So after the _get_noresume() and _set_active() you're very likely to
> have a disconnect between the hardware state and what state RPM thinks
> the hardware is in.
Good point.
I suppose calling pm_runtime_get() here would work (and make the state
active in all case)? I used _noresume() here because at this point the
driver itself hasn't had change to install it's RPM hooks.
On 09/13/2013 08:54 AM, Mika Westerberg wrote:
> On Thu, Sep 12, 2013 at 02:34:21PM -0700, Kevin Hilman wrote:
>>> > > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
>>> > > index f32ca29..44374b4 100644
>>> > > --- a/drivers/i2c/i2c-core.c
>>> > > +++ b/drivers/i2c/i2c-core.c
>>> > > @@ -248,11 +248,30 @@ 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.
>>> > > + */
>>> > > + pm_runtime_get_noresume(&client->dev);
>>> > > + pm_runtime_set_active(&client->dev);
>> >
>> > Why the set_active here?
>> >
>> > For hardware that is disabled/powered-off on startup, there will now be
>> > a mismatch between the hardware state an the RPM core state.
>
> The call to pm_runtime_get_noresume() should make sure that the device is
> in active state (at least in state where it can access the bus) if I'm
> understanding this right.
I can't see how this would happen. How runtime_resume/runtime_suspend
callbacks would get invoked with this code, if, e.g. originally driver called
pm_runtime_enable(), pm_runtime_get_sync(), pm_runtime_put_sync() in probe() ?
pm_runtime_get_noresume() merely increments usage counter of a device.
It seems that these changes will break the s5p-tv driver. I might be missing
something though.
As Mark pointed out this is currently unwanted behaviour to runtime PM
activate a bus controller device manually in the core for when the client's
probe() is executed, since i2c core will activate the bus controller for when
transfer is being carried out.
But I can understand this is needed for ACPI and it shouldn't break existing
drivers, that do runtime PM activate the client device in probe().
Now I'm sure this will break power management of the drivers/media/exynos4-is
driver, due to incorrect power sequence (power domain and clocks handling).
I'll try to take care of it in separate patch, as I have some patches pending,
that move most of code from drivers/media/exynos4-is/fimc-is-sensor.c to
drivers/media/i2c/s5k6a3.c.
--
Regards,
Sylwester
On Fri, Sep 13, 2013 at 05:14:56PM +0200, Sylwester Nawrocki wrote:
> On 09/13/2013 08:54 AM, Mika Westerberg wrote:
> > On Thu, Sep 12, 2013 at 02:34:21PM -0700, Kevin Hilman wrote:
> >>> > > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> >>> > > index f32ca29..44374b4 100644
> >>> > > --- a/drivers/i2c/i2c-core.c
> >>> > > +++ b/drivers/i2c/i2c-core.c
> >>> > > @@ -248,11 +248,30 @@ 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.
> >>> > > + */
> >>> > > + pm_runtime_get_noresume(&client->dev);
> >>> > > + pm_runtime_set_active(&client->dev);
> >> >
> >> > Why the set_active here?
> >> >
> >> > For hardware that is disabled/powered-off on startup, there will now be
> >> > a mismatch between the hardware state an the RPM core state.
> >
> > The call to pm_runtime_get_noresume() should make sure that the device is
> > in active state (at least in state where it can access the bus) if I'm
> > understanding this right.
>
> I can't see how this would happen. How runtime_resume/runtime_suspend
> callbacks would get invoked with this code, if, e.g. originally driver called
> pm_runtime_enable(), pm_runtime_get_sync(), pm_runtime_put_sync() in probe() ?
The driver callbacks are not called but if the device has been attached to
a power domain (like we do with ACPI) the power domain callbacks get called
and it brings the "bus" to such state that we are able to access the
device. That also was the reason I used _noresume() but didn't look too
close the implementation.
> pm_runtime_get_noresume() merely increments usage counter of a device.
> It seems that these changes will break the s5p-tv driver. I might be missing
> something though.
You are right and Kevin also mentioned this. It should be pm_runtime_get(),
if I'm not mistaken.
> As Mark pointed out this is currently unwanted behaviour to runtime PM
> activate a bus controller device manually in the core for when the client's
> probe() is executed, since i2c core will activate the bus controller for when
> transfer is being carried out.
>
> But I can understand this is needed for ACPI and it shouldn't break existing
> drivers, that do runtime PM activate the client device in probe().
Indeed, we don't want to break anything (and we still need something like
this for ACPI).
> Now I'm sure this will break power management of the drivers/media/exynos4-is
> driver, due to incorrect power sequence (power domain and clocks handling).
> I'll try to take care of it in separate patch, as I have some patches pending,
> that move most of code from drivers/media/exynos4-is/fimc-is-sensor.c to
> drivers/media/i2c/s5k6a3.c.
I missed that code when I converted existing users to this method. Sorry
about that (I can handle that in the next version).
I quickly looked at it and I don't see anything that could break (once
converted). What it does is this:
pm_runtime_no_callbacks(dev);
pm_runtime_enable(dev);
changing that to:
pm_runtime_no_callbacks(dev);
pm_runtime_put(dev);
shouldn't cause problems AFAICT.
On Fri, Sep 13, 2013 at 05:50:22PM +0300, Mika Westerberg wrote:
> On Fri, Sep 13, 2013 at 07:30:55AM -0700, Kevin Hilman wrote:
> > Mika Westerberg <[email protected]> writes:
> >
> > > On Thu, Sep 12, 2013 at 02:34:21PM -0700, Kevin Hilman wrote:
> > >> > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> > >> > index f32ca29..44374b4 100644
> > >> > --- a/drivers/i2c/i2c-core.c
> > >> > +++ b/drivers/i2c/i2c-core.c
> > >> > @@ -248,11 +248,30 @@ 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.
> > >> > + */
> > >> > + pm_runtime_get_noresume(&client->dev);
> > >> > + pm_runtime_set_active(&client->dev);
> > >>
> > >> Why the set_active here?
> > >>
> > >> For hardware that is disabled/powered-off on startup, there will now be
> > >> a mismatch between the hardware state an the RPM core state.
> > >
> > > The call to pm_runtime_get_noresume() should make sure that the device is
> > > in active state (at least in state where it can access the bus) if I'm
> > > understanding this right.
> >
> > No, after _get_noresume(), nothing happens to the hardware. It simply
> > increments the usecount. From pm_runtime.h:
> >
> > static inline void pm_runtime_get_noresume(struct device *dev)
> > {
> > atomic_inc(&dev->power.usage_count);
> > }
> >
> > So after the _get_noresume() and _set_active() you're very likely to
> > have a disconnect between the hardware state and what state RPM thinks
> > the hardware is in.
>
> Good point.
>
> I suppose calling pm_runtime_get() here would work (and make the state
> active in all case)? I used _noresume() here because at this point the
> driver itself hasn't had change to install it's RPM hooks.
I take that back.
[ It has been some time since this code was originally written so I can't
remember all the details :-/ ]
The pm_runtime_get_noresume() is there just to increment the refcount. It
has nothing to do to "activate" the device in question. Sorry about the
confusion from my part.
This is what happens in case the device is enumerated from ACPI:
// This makes sure that the controller itself is powered on
// (adapter device follows its parent which is the controller). The
// controller is attached to the ACPI power domain so it is
// brought to D0 now.
pm_runtime_get_sync(&client->adapter->dev);
// This binds the client device to the ACPI power domain, and in
// addition to that brings the client device to D0.
if (ACPI_HANDLE(&client->dev))
acpi_dev_pm_attach(&client->dev, true);
// Increase the refcount so that client can start runtime PM
// transitions when it calls _put().
pm_runtime_get_noresume(&client->dev);
// Mark the device being active as
// 1) In ACPI case we know that is true as we just powered the
// device on.
// 2) We treat the device by default to be runtime PM active and
// powered on (that's in the changelog and should follow what
// the PCI bus did).
pm_runtime_set_active(&client->dev);
// Enable runtime PM but nothing happens yet as long as the client
// driver doesn't call _put().
pm_runtime_enable(&client->dev);
So, yes there might be a disconnect between the runtime PM state and the
device HW state now (same is with default to suspended).
When the driver ->probe() is called, it needs to power on the device (which
it probably needs to do anyway to be able to talk to the device and probe
its registers, etc.). Once the driver calls _put() the device is eventually
moved to suspended state (and its RPM hooks are called). I might be missing
something but is there a case where this is not beneficial?
Mika Westerberg <[email protected]> writes:
> On Fri, Sep 13, 2013 at 05:50:22PM +0300, Mika Westerberg wrote:
>> On Fri, Sep 13, 2013 at 07:30:55AM -0700, Kevin Hilman wrote:
>> > Mika Westerberg <[email protected]> writes:
>> >
>> > > On Thu, Sep 12, 2013 at 02:34:21PM -0700, Kevin Hilman wrote:
>> > >> > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
>> > >> > index f32ca29..44374b4 100644
>> > >> > --- a/drivers/i2c/i2c-core.c
>> > >> > +++ b/drivers/i2c/i2c-core.c
>> > >> > @@ -248,11 +248,30 @@ 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.
>> > >> > + */
>> > >> > + pm_runtime_get_noresume(&client->dev);
>> > >> > + pm_runtime_set_active(&client->dev);
>> > >>
>> > >> Why the set_active here?
>> > >>
>> > >> For hardware that is disabled/powered-off on startup, there will now be
>> > >> a mismatch between the hardware state an the RPM core state.
>> > >
>> > > The call to pm_runtime_get_noresume() should make sure that the device is
>> > > in active state (at least in state where it can access the bus) if I'm
>> > > understanding this right.
>> >
>> > No, after _get_noresume(), nothing happens to the hardware. It simply
>> > increments the usecount. From pm_runtime.h:
>> >
>> > static inline void pm_runtime_get_noresume(struct device *dev)
>> > {
>> > atomic_inc(&dev->power.usage_count);
>> > }
>> >
>> > So after the _get_noresume() and _set_active() you're very likely to
>> > have a disconnect between the hardware state and what state RPM thinks
>> > the hardware is in.
>>
>> Good point.
>>
>> I suppose calling pm_runtime_get() here would work (and make the state
>> active in all case)? I used _noresume() here because at this point the
>> driver itself hasn't had change to install it's RPM hooks.
>
> I take that back.
>
> [ It has been some time since this code was originally written so I can't
> remember all the details :-/ ]
>
> The pm_runtime_get_noresume() is there just to increment the refcount. It
> has nothing to do to "activate" the device in question. Sorry about the
> confusion from my part.
>
> This is what happens in case the device is enumerated from ACPI:
>
> // This makes sure that the controller itself is powered on
> // (adapter device follows its parent which is the controller). The
> // controller is attached to the ACPI power domain so it is
> // brought to D0 now.
> pm_runtime_get_sync(&client->adapter->dev);
>
> // This binds the client device to the ACPI power domain, and in
> // addition to that brings the client device to D0.
OK, then here is where the problem is, because you're building ACPI
assumptions into the core. For non-ACPI devices, this part is a nop, so
the client device is still powered off, which breaks the assumptions
below.
> if (ACPI_HANDLE(&client->dev))
> acpi_dev_pm_attach(&client->dev, true);
>
> // Increase the refcount so that client can start runtime PM
> // transitions when it calls _put().
> pm_runtime_get_noresume(&client->dev);
> // Mark the device being active as
> // 1) In ACPI case we know that is true as we just powered the
> // device on.
> // 2) We treat the device by default to be runtime PM active and
> // powered on (that's in the changelog and should follow what
> // the PCI bus did).
> pm_runtime_set_active(&client->dev);
>
> // Enable runtime PM but nothing happens yet as long as the client
> // driver doesn't call _put().
> pm_runtime_enable(&client->dev);
>
> So, yes there might be a disconnect between the runtime PM state and the
> device HW state now (same is with default to suspended).
Yes, but until now, default to suspended has been assumed, so any
changes to that will likely require more thorough auditing of other drivers.
Kevin
> When the driver ->probe() is called, it needs to power on the device (which
> it probably needs to do anyway to be able to talk to the device and probe
> its registers, etc.). Once the driver calls _put() the device is eventually
> moved to suspended state (and its RPM hooks are called). I might be missing
> something but is there a case where this is not beneficial?
On Fri, Sep 13, 2013 at 02:10:43PM -0700, Kevin Hilman wrote:
> >
> > // This makes sure that the controller itself is powered on
> > // (adapter device follows its parent which is the controller). The
> > // controller is attached to the ACPI power domain so it is
> > // brought to D0 now.
> > pm_runtime_get_sync(&client->adapter->dev);
> >
> > // This binds the client device to the ACPI power domain, and in
> > // addition to that brings the client device to D0.
>
> OK, then here is where the problem is, because you're building ACPI
> assumptions into the core. For non-ACPI devices, this part is a nop, so
> the client device is still powered off, which breaks the assumptions
> below.
We expect that once the driver ->probe() is called, and it doesn't
participate the runtime PM prepared here, the device is regarded as powered
on, runtime PM active.
If the driver participates in runtime PM, it needs to power on the device
and then call pm_runtime_put() to suspend the device.
> > if (ACPI_HANDLE(&client->dev))
> > acpi_dev_pm_attach(&client->dev, true);
> >
> > // Increase the refcount so that client can start runtime PM
> > // transitions when it calls _put().
> > pm_runtime_get_noresume(&client->dev);
>
> > // Mark the device being active as
> > // 1) In ACPI case we know that is true as we just powered the
> > // device on.
> > // 2) We treat the device by default to be runtime PM active and
> > // powered on (that's in the changelog and should follow what
> > // the PCI bus did).
> > pm_runtime_set_active(&client->dev);
> >
> > // Enable runtime PM but nothing happens yet as long as the client
> > // driver doesn't call _put().
> > pm_runtime_enable(&client->dev);
> >
> > So, yes there might be a disconnect between the runtime PM state and the
> > device HW state now (same is with default to suspended).
>
> Yes, but until now, default to suspended has been assumed, so any
> changes to that will likely require more thorough auditing of other drivers.
I agree. And it looks like I missed few existing drivers as well. I'm going
to update them in the next version of the series.
There's also a less intrusive way of fixing the problem we see with ACPI
enabled I2C devices:
1. In I2C core i2c_device_probe() we power on the I2C controller
and attach the client device to the ACPI power domain. Just like in
this patch but we don't touch the I2C client device runtime PM.
-> This should allow the existing drivers to keep using whatever
runtime PM strategy they have chosen.
2. For ACPI enumerated I2C client devices drivers we need to
implement the runtime PM in order to save power (otherwise the
devices will be left powered on).
and do the same for SPI devices as well.
Then only thing that changes for non-ACPI devices is that the controller
will be powered on during the client device probe (well, and during
remove).
Thoughts?
On Sun, Sep 15, 2013 at 09:41:39AM +0300, Mika Westerberg wrote:
> There's also a less intrusive way of fixing the problem we see with ACPI
> enabled I2C devices:
> 1. In I2C core i2c_device_probe() we power on the I2C controller
> and attach the client device to the ACPI power domain. Just like in
> this patch but we don't touch the I2C client device runtime PM.
> -> This should allow the existing drivers to keep using whatever
> runtime PM strategy they have chosen.
There should be no explicit need to power on the I2C controller if it's
implemented the same way the existing ones are - just have it power
itself on when it is doing a transfer.
> 2. For ACPI enumerated I2C client devices drivers we need to
> implement the runtime PM in order to save power (otherwise the
> devices will be left powered on).
> and do the same for SPI devices as well.
> Then only thing that changes for non-ACPI devices is that the controller
> will be powered on during the client device probe (well, and during
> remove).
> Thoughts?
This is definitely less intrusive than the current proposal, there's one
ACPI I2C device binding queued for -next after the merge window (rt5640)
which will need an update.
On Sun, Sep 15, 2013 at 01:47:44PM +0100, Mark Brown wrote:
> On Sun, Sep 15, 2013 at 09:41:39AM +0300, Mika Westerberg wrote:
>
> > There's also a less intrusive way of fixing the problem we see with ACPI
> > enabled I2C devices:
>
> > 1. In I2C core i2c_device_probe() we power on the I2C controller
> > and attach the client device to the ACPI power domain. Just like in
> > this patch but we don't touch the I2C client device runtime PM.
>
> > -> This should allow the existing drivers to keep using whatever
> > runtime PM strategy they have chosen.
>
> There should be no explicit need to power on the I2C controller if it's
> implemented the same way the existing ones are - just have it power
> itself on when it is doing a transfer.
The problem is that the ACPI child device can't be in higher power state
than its parent (and this is also what the runtime PM expects). If we don't
power the I2C controller device before we power on the I2C client device
that rule is violated and we get an complaint to the console.
> > 2. For ACPI enumerated I2C client devices drivers we need to
> > implement the runtime PM in order to save power (otherwise the
> > devices will be left powered on).
>
> > and do the same for SPI devices as well.
>
> > Then only thing that changes for non-ACPI devices is that the controller
> > will be powered on during the client device probe (well, and during
> > remove).
>
> > Thoughts?
>
> This is definitely less intrusive than the current proposal, there's one
> ACPI I2C device binding queued for -next after the merge window (rt5640)
> which will need an update.
OK, thanks for the heads up. I'll check it.
On 09/13/2013 05:40 PM, Mika Westerberg wrote:
[...]
>>> The call to pm_runtime_get_noresume() should make sure that the device is
>>> in active state (at least in state where it can access the bus) if I'm
>>> understanding this right.
>>
>> I can't see how this would happen. How runtime_resume/runtime_suspend
>> callbacks would get invoked with this code, if, e.g. originally driver called
>> pm_runtime_enable(), pm_runtime_get_sync(), pm_runtime_put_sync() in probe() ?
>
> The driver callbacks are not called but if the device has been attached to
> a power domain (like we do with ACPI) the power domain callbacks get called
> and it brings the "bus" to such state that we are able to access the
> device. That also was the reason I used _noresume() but didn't look too
> close the implementation.
OK, but if a client driver assumes default inactive power state it will
expect
its callbacks to get called. Otherwise exisiting code might break. So, e.g.
in case of s5p-tv it would rather need to be something like:
pm_runtime_put()
pm_runtime_get_sync()
sii9234_verify_version()
pm_runtime_put(dev)
>> pm_runtime_get_noresume() merely increments usage counter of a device.
>> It seems that these changes will break the s5p-tv driver. I might be missing
>> something though.
>
> You are right and Kevin also mentioned this. It should be pm_runtime_get(),
> if I'm not mistaken.
Note that client drivers usually call pm_runtime_enable() only when it
is safe
to call their driver's runtime PM callbacks. By enabling runtime PM
before the
client's driver has completely initialized we may risk that the
callbacks are
executed with uninitialized data, if I understand things correctly.
>> As Mark pointed out this is currently unwanted behaviour to runtime PM
>> activate a bus controller device manually in the core for when the client's
>> probe() is executed, since i2c core will activate the bus controller for when
>> transfer is being carried out.
>>
>> But I can understand this is needed for ACPI and it shouldn't break existing
>> drivers, that do runtime PM activate the client device in probe().
>
> Indeed, we don't want to break anything (and we still need something like
> this for ACPI).
>
>> Now I'm sure this will break power management of the drivers/media/exynos4-is
>> driver, due to incorrect power sequence (power domain and clocks handling).
>> I'll try to take care of it in separate patch, as I have some patches pending,
>> that move most of code from drivers/media/exynos4-is/fimc-is-sensor.c to
>> drivers/media/i2c/s5k6a3.c.
>
> I missed that code when I converted existing users to this method. Sorry
> about that (I can handle that in the next version).
>
> I quickly looked at it and I don't see anything that could break (once
> converted). What it does is this:
>
> pm_runtime_no_callbacks(dev);
> pm_runtime_enable(dev);
>
> changing that to:
>
> pm_runtime_no_callbacks(dev);
> pm_runtime_put(dev);
>
> shouldn't cause problems AFAICT.
Yes, considering this driver in isolation it should be fine.
However, I observed system suspend issues when the I2C bus controller was
being activated (which would happen in the I2C core after your changes)
before some other driver has initialized.
So to ensure things continue to work the "fimc-isp-i2c" driver would need
to be registered after the "exynos4-fimc-is" driver has initialized. Or the
"exynos4-fimc-is" would need to call of_platform_populate() to instantiate
its all children devices as specified in device tree (see arch/arm/boot/dts/
exynos4x12.dtsi in -next). "simple-bus" would then have to be not listed in
the compatible property of that top level device. So to avoid regressions
some additional changes would be needed, outside of this particular I2C
client driver. I guess this could be avoided by better design of the
exynos4-is driver right from the beginning. But it's all some times tricky
when there is some many IP blocks involved and the hardware behaviour/device
interactions are not always well documented.
On Sun, Sep 15, 2013 at 03:48:12PM +0200, Sylwester Nawrocki wrote:
> On 09/13/2013 05:40 PM, Mika Westerberg wrote:
> [...]
> >>>The call to pm_runtime_get_noresume() should make sure that the device is
> >>>in active state (at least in state where it can access the bus) if I'm
> >>>understanding this right.
> >>
> >>I can't see how this would happen. How runtime_resume/runtime_suspend
> >>callbacks would get invoked with this code, if, e.g. originally driver called
> >>pm_runtime_enable(), pm_runtime_get_sync(), pm_runtime_put_sync() in probe() ?
> >
> >The driver callbacks are not called but if the device has been attached to
> >a power domain (like we do with ACPI) the power domain callbacks get called
> >and it brings the "bus" to such state that we are able to access the
> >device. That also was the reason I used _noresume() but didn't look too
> >close the implementation.
>
> OK, but if a client driver assumes default inactive power state it
> will expect
> its callbacks to get called. Otherwise exisiting code might break. So, e.g.
> in case of s5p-tv it would rather need to be something like:
>
> pm_runtime_put()
>
> pm_runtime_get_sync()
> sii9234_verify_version()
> pm_runtime_put(dev)
Yes, or even call directly its runtime_resume callback to bring the device
to the active state.
> >>pm_runtime_get_noresume() merely increments usage counter of a device.
> >>It seems that these changes will break the s5p-tv driver. I might be missing
> >>something though.
> >
> >You are right and Kevin also mentioned this. It should be pm_runtime_get(),
> >if I'm not mistaken.
>
> Note that client drivers usually call pm_runtime_enable() only when
> it is safe
> to call their driver's runtime PM callbacks. By enabling runtime PM
> before the
> client's driver has completely initialized we may risk that the
> callbacks are
> executed with uninitialized data, if I understand things correctly.
I think that calling pm_runtime_enable() on behalf of the client driver
shouldn't cause any problems. There is no PM events queued for that device
yet (and we have prevented that from happening when we called
_get_noresume() for the device).
Only when the client driver calls _put() things start to happen and at that
phase the runtime PM hooks should be usable.
> >>As Mark pointed out this is currently unwanted behaviour to runtime PM
> >>activate a bus controller device manually in the core for when the client's
> >>probe() is executed, since i2c core will activate the bus controller for when
> >>transfer is being carried out.
> >>
> >>But I can understand this is needed for ACPI and it shouldn't break existing
> >>drivers, that do runtime PM activate the client device in probe().
> >
> >Indeed, we don't want to break anything (and we still need something like
> >this for ACPI).
> >
> >>Now I'm sure this will break power management of the drivers/media/exynos4-is
> >>driver, due to incorrect power sequence (power domain and clocks handling).
> >>I'll try to take care of it in separate patch, as I have some patches pending,
> >>that move most of code from drivers/media/exynos4-is/fimc-is-sensor.c to
> >>drivers/media/i2c/s5k6a3.c.
> >
> >I missed that code when I converted existing users to this method. Sorry
> >about that (I can handle that in the next version).
> >
> >I quickly looked at it and I don't see anything that could break (once
> >converted). What it does is this:
> >
> > pm_runtime_no_callbacks(dev);
> > pm_runtime_enable(dev);
> >
> >changing that to:
> >
> > pm_runtime_no_callbacks(dev);
> > pm_runtime_put(dev);
> >
> >shouldn't cause problems AFAICT.
>
> Yes, considering this driver in isolation it should be fine.
>
> However, I observed system suspend issues when the I2C bus controller was
> being activated (which would happen in the I2C core after your changes)
> before some other driver has initialized.
>
> So to ensure things continue to work the "fimc-isp-i2c" driver would need
> to be registered after the "exynos4-fimc-is" driver has initialized. Or the
> "exynos4-fimc-is" would need to call of_platform_populate() to instantiate
> its all children devices as specified in device tree (see arch/arm/boot/dts/
> exynos4x12.dtsi in -next). "simple-bus" would then have to be not listed in
> the compatible property of that top level device. So to avoid regressions
> some additional changes would be needed, outside of this particular I2C
> client driver. I guess this could be avoided by better design of the
> exynos4-is driver right from the beginning. But it's all some times tricky
> when there is some many IP blocks involved and the hardware behaviour/device
> interactions are not always well documented.
OK.
I'm actually thinking that it is probably better now if we don't touch the
client runtime PM at all in the I2C core.
I proposed a less intrusive solution in this same thread where we power the
I2C controller briefly at the client ->probe() (In order to have all the
ACPI power resources etc. and the controller on) and let the client driver
handle their own runtime PM as they do currently.
Do you think that would work better wrt. fimc-isp-i2c driver?
On Sun, Sep 15, 2013 at 04:28:24PM +0300, Mika Westerberg wrote:
> On Sun, Sep 15, 2013 at 01:47:44PM +0100, Mark Brown wrote:
> > On Sun, Sep 15, 2013 at 09:41:39AM +0300, Mika Westerberg wrote:
> > > 1. In I2C core i2c_device_probe() we power on the I2C controller
> > > and attach the client device to the ACPI power domain. Just like in
> > > this patch but we don't touch the I2C client device runtime PM.
> > > -> This should allow the existing drivers to keep using whatever
> > > runtime PM strategy they have chosen.
> > There should be no explicit need to power on the I2C controller if it's
> > implemented the same way the existing ones are - just have it power
> > itself on when it is doing a transfer.
> The problem is that the ACPI child device can't be in higher power state
> than its parent (and this is also what the runtime PM expects). If we don't
> power the I2C controller device before we power on the I2C client device
> that rule is violated and we get an complaint to the console.
That's definitely an ACPI specific (probably x86 specific ACPI?)
requirement not a generic one, on some systems it would increase power
consumption since the controller will need to sit on while the device is
functioning autonomously. Even though the controller power consumption
is going to be minimal the power domain it is in may be relatively
large. Can't the power domains for ACPI deal with this requirement, for
example by making the I2C slave power domains children of the controller
power domain?
On Mon, Sep 16, 2013 at 11:12:49AM +0100, Mark Brown wrote:
> That's definitely an ACPI specific (probably x86 specific ACPI?)
> requirement not a generic one, on some systems it would increase power
> consumption since the controller will need to sit on while the device is
> functioning autonomously.
Yes, the ACPI 5.0 spec says that the device cannot be in higher D-state
than its parent. This is not x86 specific, though I'm not sure if this is
implemented elsewhere.
> Even though the controller power consumption is going to be minimal the
> power domain it is in may be relatively large. Can't the power domains
> for ACPI deal with this requirement, for example by making the I2C slave
> power domains children of the controller power domain?
We'll look into this. Thanks for the suggestion.
On Mon, Sep 16, 2013 at 05:38:12PM +0300, Mika Westerberg wrote:
> On Mon, Sep 16, 2013 at 11:12:49AM +0100, Mark Brown wrote:
> > That's definitely an ACPI specific (probably x86 specific ACPI?)
> > requirement not a generic one, on some systems it would increase power
> > consumption since the controller will need to sit on while the device is
> > functioning autonomously.
>
> Yes, the ACPI 5.0 spec says that the device cannot be in higher D-state
> than its parent. This is not x86 specific, though I'm not sure if this is
> implemented elsewhere.
>
I do not think this stops the OS fine controlling the power of the device
though. It is only a mechanism to make sure the tree of D states is vaguely
sane from the ACPI point of view. What happens in each D state is never
actually defined in the ACPI spec.
Graeme
On Mon, Sep 16, 2013 at 03:46:16PM +0100, Graeme Gregory wrote:
> On Mon, Sep 16, 2013 at 05:38:12PM +0300, Mika Westerberg wrote:
> > On Mon, Sep 16, 2013 at 11:12:49AM +0100, Mark Brown wrote:
> > > That's definitely an ACPI specific (probably x86 specific ACPI?)
> > > requirement not a generic one, on some systems it would increase power
> > > consumption since the controller will need to sit on while the device is
> > > functioning autonomously.
> >
> > Yes, the ACPI 5.0 spec says that the device cannot be in higher D-state
> > than its parent. This is not x86 specific, though I'm not sure if this is
> > implemented elsewhere.
> >
> I do not think this stops the OS fine controlling the power of the device
> though. It is only a mechanism to make sure the tree of D states is vaguely
> sane from the ACPI point of view. What happens in each D state is never
> actually defined in the ACPI spec.
I think there's a pretty good definition of the D-states in chapter 2.3 of
the ACPI 5.0 spec.
In our case the problem is that the I2C controller is in D3Cold (off) and
we try to move the I2C client device to D0 (on) it violates the ACPI spec.
Anyway we are looking if we can somehow make this work in such way that it
doesn't prevent non-ACPI devices from functioning as they expect now.
On Monday, September 16, 2013 11:47:08 AM Mika Westerberg wrote:
> On Sun, Sep 15, 2013 at 03:48:12PM +0200, Sylwester Nawrocki wrote:
> > On 09/13/2013 05:40 PM, Mika Westerberg wrote:
> > [...]
> > >>>The call to pm_runtime_get_noresume() should make sure that the device is
> > >>>in active state (at least in state where it can access the bus) if I'm
> > >>>understanding this right.
> > >>
> > >>I can't see how this would happen. How runtime_resume/runtime_suspend
> > >>callbacks would get invoked with this code, if, e.g. originally driver called
> > >>pm_runtime_enable(), pm_runtime_get_sync(), pm_runtime_put_sync() in probe() ?
> > >
> > >The driver callbacks are not called but if the device has been attached to
> > >a power domain (like we do with ACPI) the power domain callbacks get called
> > >and it brings the "bus" to such state that we are able to access the
> > >device. That also was the reason I used _noresume() but didn't look too
> > >close the implementation.
> >
> > OK, but if a client driver assumes default inactive power state it
> > will expect
> > its callbacks to get called. Otherwise exisiting code might break. So, e.g.
> > in case of s5p-tv it would rather need to be something like:
> >
> > pm_runtime_put()
> >
> > pm_runtime_get_sync()
> > sii9234_verify_version()
> > pm_runtime_put(dev)
>
> Yes, or even call directly its runtime_resume callback to bring the device
> to the active state.
>
> > >>pm_runtime_get_noresume() merely increments usage counter of a device.
> > >>It seems that these changes will break the s5p-tv driver. I might be missing
> > >>something though.
> > >
> > >You are right and Kevin also mentioned this. It should be pm_runtime_get(),
> > >if I'm not mistaken.
> >
> > Note that client drivers usually call pm_runtime_enable() only when
> > it is safe
> > to call their driver's runtime PM callbacks. By enabling runtime PM
> > before the
> > client's driver has completely initialized we may risk that the
> > callbacks are
> > executed with uninitialized data, if I understand things correctly.
>
> I think that calling pm_runtime_enable() on behalf of the client driver
> shouldn't cause any problems. There is no PM events queued for that device
> yet (and we have prevented that from happening when we called
> _get_noresume() for the device).
That only is the case if the device in RPM_ACTIVE when we enable runtime PM for
it. If it is RPM_SUSPENDED at that point, it still is possible that the resume
callback will be executed then.
> Only when the client driver calls _put() things start to happen and at that
> phase the runtime PM hooks should be usable.
>
> > >>As Mark pointed out this is currently unwanted behaviour to runtime PM
> > >>activate a bus controller device manually in the core for when the client's
> > >>probe() is executed, since i2c core will activate the bus controller for when
> > >>transfer is being carried out.
> > >>
> > >>But I can understand this is needed for ACPI and it shouldn't break existing
> > >>drivers, that do runtime PM activate the client device in probe().
> > >
> > >Indeed, we don't want to break anything (and we still need something like
> > >this for ACPI).
> > >
> > >>Now I'm sure this will break power management of the drivers/media/exynos4-is
> > >>driver, due to incorrect power sequence (power domain and clocks handling).
> > >>I'll try to take care of it in separate patch, as I have some patches pending,
> > >>that move most of code from drivers/media/exynos4-is/fimc-is-sensor.c to
> > >>drivers/media/i2c/s5k6a3.c.
> > >
> > >I missed that code when I converted existing users to this method. Sorry
> > >about that (I can handle that in the next version).
> > >
> > >I quickly looked at it and I don't see anything that could break (once
> > >converted). What it does is this:
> > >
> > > pm_runtime_no_callbacks(dev);
> > > pm_runtime_enable(dev);
> > >
> > >changing that to:
> > >
> > > pm_runtime_no_callbacks(dev);
> > > pm_runtime_put(dev);
> > >
> > >shouldn't cause problems AFAICT.
> >
> > Yes, considering this driver in isolation it should be fine.
> >
> > However, I observed system suspend issues when the I2C bus controller was
> > being activated (which would happen in the I2C core after your changes)
> > before some other driver has initialized.
> >
> > So to ensure things continue to work the "fimc-isp-i2c" driver would need
> > to be registered after the "exynos4-fimc-is" driver has initialized. Or the
> > "exynos4-fimc-is" would need to call of_platform_populate() to instantiate
> > its all children devices as specified in device tree (see arch/arm/boot/dts/
> > exynos4x12.dtsi in -next). "simple-bus" would then have to be not listed in
> > the compatible property of that top level device. So to avoid regressions
> > some additional changes would be needed, outside of this particular I2C
> > client driver. I guess this could be avoided by better design of the
> > exynos4-is driver right from the beginning. But it's all some times tricky
> > when there is some many IP blocks involved and the hardware behaviour/device
> > interactions are not always well documented.
>
> OK.
>
> I'm actually thinking that it is probably better now if we don't touch the
> client runtime PM at all in the I2C core.
>
> I proposed a less intrusive solution in this same thread where we power the
> I2C controller briefly at the client ->probe() (In order to have all the
> ACPI power resources etc. and the controller on) and let the client driver
> handle their own runtime PM as they do currently.
I'm not sure if the I2C core needs to power up the controller at the probe time.
That may be left to the client driver altogether. I mean, if the client wants
the controller to be powered up, it should just call
pm_runtime_get_sync(controller device) at a suitable place (and then do the
corresponding _put when the controller is not necessary anu more) from its
->probe() callback.
Or the core can just check if the device is in the ACPI PM domain and power up
the controller if that's the case. However, that may not match the case when
the I2C client is not a direct descendant of the controller (it may just use
an I2C resource pointing to the controller via a namespace path).
Thanks,
Rafael
On Mon, Sep 16, 2013 at 09:07:07PM +0200, Rafael J. Wysocki wrote:
> That may be left to the client driver altogether. I mean, if the client wants
> the controller to be powered up, it should just call
> pm_runtime_get_sync(controller device) at a suitable place (and then do the
> corresponding _put when the controller is not necessary anu more) from its
> ->probe() callback.
It shouldn't even need to do that, it should just be able to rely on the
controller to power itself up when asked to do work. This is how the
existing implementations are done - the controller power management is
totally transparent to the slave.
On Tuesday, September 17, 2013 12:31:11 AM Mark Brown wrote:
> On Mon, Sep 16, 2013 at 09:07:07PM +0200, Rafael J. Wysocki wrote:
>
> > That may be left to the client driver altogether. I mean, if the client wants
> > the controller to be powered up, it should just call
> > pm_runtime_get_sync(controller device) at a suitable place (and then do the
> > corresponding _put when the controller is not necessary anu more) from its
> > ->probe() callback.
>
> It shouldn't even need to do that, it should just be able to rely on the
> controller to power itself up when asked to do work. This is how the
> existing implementations are done - the controller power management is
> totally transparent to the slave.
If both the I2C client and I2C controller have corresponding objects in the
ACPI namespace and the client's object is a child of the controller's object,
then in order to power up the client we need to power up the controller even
if no transactions are going to be carried out. That's what the spec simply
requires us to do in that case.
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On Tue, Sep 17, 2013 at 03:25:25AM +0200, Rafael J. Wysocki wrote:
> On Tuesday, September 17, 2013 12:31:11 AM Mark Brown wrote:
> > It shouldn't even need to do that, it should just be able to rely on the
> > controller to power itself up when asked to do work. This is how the
> > existing implementations are done - the controller power management is
> > totally transparent to the slave.
> If both the I2C client and I2C controller have corresponding objects in the
> ACPI namespace and the client's object is a child of the controller's object,
> then in order to power up the client we need to power up the controller even
> if no transactions are going to be carried out. That's what the spec simply
> requires us to do in that case.
Like I said I think this should be handled by the power domains (or
otherwise in the ACPI specific code) - we shouldn't be needing to modify
individual drivers to work around thoughtlessness in the ACPI spec.
On Mon, Sep 16, 2013 at 09:07:07PM +0200, Rafael J. Wysocki wrote:
> On Monday, September 16, 2013 11:47:08 AM Mika Westerberg wrote:
> > On Sun, Sep 15, 2013 at 03:48:12PM +0200, Sylwester Nawrocki wrote:
> > > On 09/13/2013 05:40 PM, Mika Westerberg wrote:
> > > [...]
> > > >>>The call to pm_runtime_get_noresume() should make sure that the device is
> > > >>>in active state (at least in state where it can access the bus) if I'm
> > > >>>understanding this right.
> > > >>
> > > >>I can't see how this would happen. How runtime_resume/runtime_suspend
> > > >>callbacks would get invoked with this code, if, e.g. originally driver called
> > > >>pm_runtime_enable(), pm_runtime_get_sync(), pm_runtime_put_sync() in probe() ?
> > > >
> > > >The driver callbacks are not called but if the device has been attached to
> > > >a power domain (like we do with ACPI) the power domain callbacks get called
> > > >and it brings the "bus" to such state that we are able to access the
> > > >device. That also was the reason I used _noresume() but didn't look too
> > > >close the implementation.
> > >
> > > OK, but if a client driver assumes default inactive power state it
> > > will expect
> > > its callbacks to get called. Otherwise exisiting code might break. So, e.g.
> > > in case of s5p-tv it would rather need to be something like:
> > >
> > > pm_runtime_put()
> > >
> > > pm_runtime_get_sync()
> > > sii9234_verify_version()
> > > pm_runtime_put(dev)
> >
> > Yes, or even call directly its runtime_resume callback to bring the device
> > to the active state.
> >
> > > >>pm_runtime_get_noresume() merely increments usage counter of a device.
> > > >>It seems that these changes will break the s5p-tv driver. I might be missing
> > > >>something though.
> > > >
> > > >You are right and Kevin also mentioned this. It should be pm_runtime_get(),
> > > >if I'm not mistaken.
> > >
> > > Note that client drivers usually call pm_runtime_enable() only when
> > > it is safe
> > > to call their driver's runtime PM callbacks. By enabling runtime PM
> > > before the
> > > client's driver has completely initialized we may risk that the
> > > callbacks are
> > > executed with uninitialized data, if I understand things correctly.
> >
> > I think that calling pm_runtime_enable() on behalf of the client driver
> > shouldn't cause any problems. There is no PM events queued for that device
> > yet (and we have prevented that from happening when we called
> > _get_noresume() for the device).
>
> That only is the case if the device in RPM_ACTIVE when we enable runtime PM for
> it. If it is RPM_SUSPENDED at that point, it still is possible that the resume
> callback will be executed then.
OK, thanks for the clarification.
> > Only when the client driver calls _put() things start to happen and at that
> > phase the runtime PM hooks should be usable.
> >
> > > >>As Mark pointed out this is currently unwanted behaviour to runtime PM
> > > >>activate a bus controller device manually in the core for when the client's
> > > >>probe() is executed, since i2c core will activate the bus controller for when
> > > >>transfer is being carried out.
> > > >>
> > > >>But I can understand this is needed for ACPI and it shouldn't break existing
> > > >>drivers, that do runtime PM activate the client device in probe().
> > > >
> > > >Indeed, we don't want to break anything (and we still need something like
> > > >this for ACPI).
> > > >
> > > >>Now I'm sure this will break power management of the drivers/media/exynos4-is
> > > >>driver, due to incorrect power sequence (power domain and clocks handling).
> > > >>I'll try to take care of it in separate patch, as I have some patches pending,
> > > >>that move most of code from drivers/media/exynos4-is/fimc-is-sensor.c to
> > > >>drivers/media/i2c/s5k6a3.c.
> > > >
> > > >I missed that code when I converted existing users to this method. Sorry
> > > >about that (I can handle that in the next version).
> > > >
> > > >I quickly looked at it and I don't see anything that could break (once
> > > >converted). What it does is this:
> > > >
> > > > pm_runtime_no_callbacks(dev);
> > > > pm_runtime_enable(dev);
> > > >
> > > >changing that to:
> > > >
> > > > pm_runtime_no_callbacks(dev);
> > > > pm_runtime_put(dev);
> > > >
> > > >shouldn't cause problems AFAICT.
> > >
> > > Yes, considering this driver in isolation it should be fine.
> > >
> > > However, I observed system suspend issues when the I2C bus controller was
> > > being activated (which would happen in the I2C core after your changes)
> > > before some other driver has initialized.
> > >
> > > So to ensure things continue to work the "fimc-isp-i2c" driver would need
> > > to be registered after the "exynos4-fimc-is" driver has initialized. Or the
> > > "exynos4-fimc-is" would need to call of_platform_populate() to instantiate
> > > its all children devices as specified in device tree (see arch/arm/boot/dts/
> > > exynos4x12.dtsi in -next). "simple-bus" would then have to be not listed in
> > > the compatible property of that top level device. So to avoid regressions
> > > some additional changes would be needed, outside of this particular I2C
> > > client driver. I guess this could be avoided by better design of the
> > > exynos4-is driver right from the beginning. But it's all some times tricky
> > > when there is some many IP blocks involved and the hardware behaviour/device
> > > interactions are not always well documented.
> >
> > OK.
> >
> > I'm actually thinking that it is probably better now if we don't touch the
> > client runtime PM at all in the I2C core.
> >
> > I proposed a less intrusive solution in this same thread where we power the
> > I2C controller briefly at the client ->probe() (In order to have all the
> > ACPI power resources etc. and the controller on) and let the client driver
> > handle their own runtime PM as they do currently.
>
> I'm not sure if the I2C core needs to power up the controller at the probe time.
> That may be left to the client driver altogether. I mean, if the client wants
> the controller to be powered up, it should just call
> pm_runtime_get_sync(controller device) at a suitable place (and then do the
> corresponding _put when the controller is not necessary anu more) from its
> ->probe() callback.
>
> Or the core can just check if the device is in the ACPI PM domain and power up
> the controller if that's the case. However, that may not match the case when
> the I2C client is not a direct descendant of the controller (it may just use
> an I2C resource pointing to the controller via a namespace path).
I sure hope we don't have to deal with such devices.
What if we make runtime PM enabled for the I2C adapter device only in case
of ACPI enumerated devices? That way runtime PM itself keeps the adapter
powered on when it has any active kids so we don't violate the ACPI spec,
and still let non-ACPI systems to use I2C as they do today.
Then we could do something like:
static int i2c_device_probe(struct device *dev)
{
...
/*
* Make sure that the adapter is powered on when the client is
* probed.
*
* Note that this is no-op on non-ACPI systems as runtime PM for
* the adapter is not enabled.
*/
pm_runtime_get_sync(&client->adapter->dev);
acpi_dev_pm_attach(&client->dev, true);
status = driver->probe(client, i2c_match_id(driver->id_table, client));
if (status) {
...
and enable runtime PM only when we find that there are ACPI I2C devices
behind the controller and they are power manageable.
On 09/16/2013 10:47 AM, Mika Westerberg wrote:
> I'm actually thinking that it is probably better now if we don't touch the
> client runtime PM at all in the I2C core.
>
> I proposed a less intrusive solution in this same thread where we power the
> I2C controller briefly at the client ->probe() (In order to have all the
> ACPI power resources etc. and the controller on) and let the client driver
> handle their own runtime PM as they do currently.
>
> Do you think that would work better wrt. fimc-isp-i2c driver?
That would be no different for this particular driver, as long as the
I2C bus controller is activated right before the I2C client's probe().
In general I would expect such additional device activation not to be
harmful. For that particular driver I'm going to prepare patches to
ensure that the I2C bus controller device and its driver is registered
only when a driver it depends on has initialized. This should have been
ensured right from the beginning. So don't need to worry about this
particular case. I'm just not sure what other devices could be similarly
affected.
On Tuesday, September 17, 2013 02:00:22 PM Mika Westerberg wrote:
> On Mon, Sep 16, 2013 at 09:07:07PM +0200, Rafael J. Wysocki wrote:
> > On Monday, September 16, 2013 11:47:08 AM Mika Westerberg wrote:
> > > On Sun, Sep 15, 2013 at 03:48:12PM +0200, Sylwester Nawrocki wrote:
> > > > On 09/13/2013 05:40 PM, Mika Westerberg wrote:
> > > > [...]
> > > > >>>The call to pm_runtime_get_noresume() should make sure that the device is
> > > > >>>in active state (at least in state where it can access the bus) if I'm
> > > > >>>understanding this right.
> > > > >>
> > > > >>I can't see how this would happen. How runtime_resume/runtime_suspend
> > > > >>callbacks would get invoked with this code, if, e.g. originally driver called
> > > > >>pm_runtime_enable(), pm_runtime_get_sync(), pm_runtime_put_sync() in probe() ?
> > > > >
> > > > >The driver callbacks are not called but if the device has been attached to
> > > > >a power domain (like we do with ACPI) the power domain callbacks get called
> > > > >and it brings the "bus" to such state that we are able to access the
> > > > >device. That also was the reason I used _noresume() but didn't look too
> > > > >close the implementation.
> > > >
> > > > OK, but if a client driver assumes default inactive power state it
> > > > will expect
> > > > its callbacks to get called. Otherwise exisiting code might break. So, e.g.
> > > > in case of s5p-tv it would rather need to be something like:
> > > >
> > > > pm_runtime_put()
> > > >
> > > > pm_runtime_get_sync()
> > > > sii9234_verify_version()
> > > > pm_runtime_put(dev)
> > >
> > > Yes, or even call directly its runtime_resume callback to bring the device
> > > to the active state.
> > >
> > > > >>pm_runtime_get_noresume() merely increments usage counter of a device.
> > > > >>It seems that these changes will break the s5p-tv driver. I might be missing
> > > > >>something though.
> > > > >
> > > > >You are right and Kevin also mentioned this. It should be pm_runtime_get(),
> > > > >if I'm not mistaken.
> > > >
> > > > Note that client drivers usually call pm_runtime_enable() only when
> > > > it is safe
> > > > to call their driver's runtime PM callbacks. By enabling runtime PM
> > > > before the
> > > > client's driver has completely initialized we may risk that the
> > > > callbacks are
> > > > executed with uninitialized data, if I understand things correctly.
> > >
> > > I think that calling pm_runtime_enable() on behalf of the client driver
> > > shouldn't cause any problems. There is no PM events queued for that device
> > > yet (and we have prevented that from happening when we called
> > > _get_noresume() for the device).
> >
> > That only is the case if the device in RPM_ACTIVE when we enable runtime PM for
> > it. If it is RPM_SUSPENDED at that point, it still is possible that the resume
> > callback will be executed then.
>
> OK, thanks for the clarification.
>
> > > Only when the client driver calls _put() things start to happen and at that
> > > phase the runtime PM hooks should be usable.
> > >
> > > > >>As Mark pointed out this is currently unwanted behaviour to runtime PM
> > > > >>activate a bus controller device manually in the core for when the client's
> > > > >>probe() is executed, since i2c core will activate the bus controller for when
> > > > >>transfer is being carried out.
> > > > >>
> > > > >>But I can understand this is needed for ACPI and it shouldn't break existing
> > > > >>drivers, that do runtime PM activate the client device in probe().
> > > > >
> > > > >Indeed, we don't want to break anything (and we still need something like
> > > > >this for ACPI).
> > > > >
> > > > >>Now I'm sure this will break power management of the drivers/media/exynos4-is
> > > > >>driver, due to incorrect power sequence (power domain and clocks handling).
> > > > >>I'll try to take care of it in separate patch, as I have some patches pending,
> > > > >>that move most of code from drivers/media/exynos4-is/fimc-is-sensor.c to
> > > > >>drivers/media/i2c/s5k6a3.c.
> > > > >
> > > > >I missed that code when I converted existing users to this method. Sorry
> > > > >about that (I can handle that in the next version).
> > > > >
> > > > >I quickly looked at it and I don't see anything that could break (once
> > > > >converted). What it does is this:
> > > > >
> > > > > pm_runtime_no_callbacks(dev);
> > > > > pm_runtime_enable(dev);
> > > > >
> > > > >changing that to:
> > > > >
> > > > > pm_runtime_no_callbacks(dev);
> > > > > pm_runtime_put(dev);
> > > > >
> > > > >shouldn't cause problems AFAICT.
> > > >
> > > > Yes, considering this driver in isolation it should be fine.
> > > >
> > > > However, I observed system suspend issues when the I2C bus controller was
> > > > being activated (which would happen in the I2C core after your changes)
> > > > before some other driver has initialized.
> > > >
> > > > So to ensure things continue to work the "fimc-isp-i2c" driver would need
> > > > to be registered after the "exynos4-fimc-is" driver has initialized. Or the
> > > > "exynos4-fimc-is" would need to call of_platform_populate() to instantiate
> > > > its all children devices as specified in device tree (see arch/arm/boot/dts/
> > > > exynos4x12.dtsi in -next). "simple-bus" would then have to be not listed in
> > > > the compatible property of that top level device. So to avoid regressions
> > > > some additional changes would be needed, outside of this particular I2C
> > > > client driver. I guess this could be avoided by better design of the
> > > > exynos4-is driver right from the beginning. But it's all some times tricky
> > > > when there is some many IP blocks involved and the hardware behaviour/device
> > > > interactions are not always well documented.
> > >
> > > OK.
> > >
> > > I'm actually thinking that it is probably better now if we don't touch the
> > > client runtime PM at all in the I2C core.
> > >
> > > I proposed a less intrusive solution in this same thread where we power the
> > > I2C controller briefly at the client ->probe() (In order to have all the
> > > ACPI power resources etc. and the controller on) and let the client driver
> > > handle their own runtime PM as they do currently.
> >
> > I'm not sure if the I2C core needs to power up the controller at the probe time.
> > That may be left to the client driver altogether. I mean, if the client wants
> > the controller to be powered up, it should just call
> > pm_runtime_get_sync(controller device) at a suitable place (and then do the
> > corresponding _put when the controller is not necessary anu more) from its
> > ->probe() callback.
> >
> > Or the core can just check if the device is in the ACPI PM domain and power up
> > the controller if that's the case. However, that may not match the case when
> > the I2C client is not a direct descendant of the controller (it may just use
> > an I2C resource pointing to the controller via a namespace path).
>
> I sure hope we don't have to deal with such devices.
>
> What if we make runtime PM enabled for the I2C adapter device only in case
> of ACPI enumerated devices? That way runtime PM itself keeps the adapter
> powered on when it has any active kids so we don't violate the ACPI spec,
> and still let non-ACPI systems to use I2C as they do today.
>
> Then we could do something like:
>
> static int i2c_device_probe(struct device *dev)
> {
> ...
> /*
> * Make sure that the adapter is powered on when the client is
> * probed.
> *
> * Note that this is no-op on non-ACPI systems as runtime PM for
> * the adapter is not enabled.
> */
> pm_runtime_get_sync(&client->adapter->dev);
> acpi_dev_pm_attach(&client->dev, true);
I would make the code indicate the ACPI special case, like:
if (ACPI_HANDLE(&client->dev)) {
/* Power up the controller, if necessary, to follow the ACPI spec. */
pm_runtime_get_sync(&client->adapter->dev);
acpi_dev_pm_attach(&client->dev, true);
}
>
> status = driver->probe(client, i2c_match_id(driver->id_table, client));
> if (status) {
> ...
>
and of course you need to do the corresponding pm_runtime_put() for the
controller somewhere.
And because ACPI_HANDLE() is simply false for CONFIG_ACPI unset, that would
cover that case too.
> and enable runtime PM only when we find that there are ACPI I2C devices
> behind the controller and they are power manageable.
We need to power up the controller regardless of whether or not the child
devices are power manageable. If a client device we want to access has an
ACPI handle, the controller has to be in D0 at that point.
Thanks,
Rafael
On Tue, Sep 17, 2013 at 01:07:37PM +0200, Sylwester Nawrocki wrote:
> On 09/16/2013 10:47 AM, Mika Westerberg wrote:
> > I'm actually thinking that it is probably better now if we don't touch the
> > client runtime PM at all in the I2C core.
> >
> > I proposed a less intrusive solution in this same thread where we power the
> > I2C controller briefly at the client ->probe() (In order to have all the
> > ACPI power resources etc. and the controller on) and let the client driver
> > handle their own runtime PM as they do currently.
> >
> > Do you think that would work better wrt. fimc-isp-i2c driver?
>
> That would be no different for this particular driver, as long as the
> I2C bus controller is activated right before the I2C client's probe().
Rafael pointed out that we can use ->ignore_children for the I2C adapter
device for everything else than ACPI devices. That should keep the existing
behavior.
For ACPI devices we don't set that flag and the runtime PM core will
activate the adapter whenever we try to power on I2C client device. This
will make ACPI happy as well.
If there are no objections, I'm going to send a new version based on the
above.
Thanks everyone for valuable input :)