On Wed, Sep 20, 2017 at 11:45:20AM +0300, sakari.ailus@iki.fi wrote:

quoted

quoted

quoted

@@ -743,11 +770,17 @@ static int at24_probe(struct i2c_client *client, const

struct i2c_device_id *id)

      i2c_set_clientdata(client, at24);

+     /* enable runtime pm */
+     pm_runtime_get_noresume(&client->dev);
+     pm_runtime_set_active(&client->dev);
+     pm_runtime_enable(&client->dev);

Do we need this get_noresume/set_active dance? I remember it was for
some reason needed for PCI devices, but I don't see why for I2C
anything else than just pm_runtime_enable() would be necessary.

You specifically do not need (all) this for PCI devices, but AFAIU for I²C
devices you do. The runtime PM status of a device is disabled by default
and the use count is zero, but on ACPI based systems the device is still
powered on.

quoted

Also, we enable runtime PM, but we don't provide any callbacks. If
there is no callback in any level of the hierarchy, NULL would be
returned in [3], making [2] return -ENOSYS and [1] fail. The behavior
depends on subsystem and whether the device is attached to a
pm_domain. In our particular case I'd guess the device would be in an
ACPI pm_domain and that would work, but the driver is generic and must
work in any cases.

Agreed.

I looked at the code and what actually happens here is the runtime_suspend
and runtime_resume callbacks aren't set is that the first pm_runtime_put()
call itself succeeds because checking the the runtime_suspend callback will
be done in the work queue function. This leaves the device in RPM_ACTIVE
state, which I don't think is a problem since the driver did not have
explicit functions to control the device power state.

Further pm_runtime_put() and pm_runtime_get() calls will succeed because
the device is in RPM_ACTIVE state.

So I see no reason to set the callbacks if they would not actually control
regulators, clocks or GPIOs required by the device.

Cc linux-pm.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi