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

[Mark Brown <broonie@kernel.org>]

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

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

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

> > ...as you say.

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

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

> That's basically what PCI drivers currently do.

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

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

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

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

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

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

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

> Sorry about that.

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

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

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

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

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

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

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