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

[Mika Westerberg <mika.westerberg-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>]

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

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

That's basically what PCI drivers currently do.

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

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

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

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

OK.

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

Sorry about that.

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

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

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

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