Re: [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd

[Jon Hunter]

On 24/05/18 13:17, Ulf Hansson wrote:
> On 24 May 2018 at 11:36, Jon Hunter <jonathanh@xxxxxxxxxx> wrote:
> > On 24/05/18 08:04, Ulf Hansson wrote:
> >
> > ...
> >
> > > > Any reason why we could not add a 'boolean' argument to the API to
> > > > indicate
> > > > whether the new device should be linked? I think that I prefer the API
> > > > handles it, but I can see there could be instances where drivers may wish
> > > > to
> > > > handle it themselves.
> > >
> > >
> > > Coming back to this question. Both Tegra XUSB and Qcom Camera use
> > > case, would benefit from doing the linking themselves, as it needs
> > > different PM domains to be powered on depending on the current use
> > > case - as to avoid wasting power.
> > >
> > > However, I can understand that you prefer some simplicity over
> > > optimizations, as you told us. Then, does it mean that you are
> > > insisting on extending the APIs with a boolean for linking, or are you
> > > fine with the driver to call device_link_add()?
> >
> >
> > I am fine with the driver calling device_link_add(), but I just wonder if we
> > will find a several drivers doing this and then we will end up doing this
> > later anyway.
>
> Okay.
>
> > The current API is called ...
> >
> > * genpd_dev_pm_attach_by_id() - Attach a device to one of its PM domain.
> > * @dev: Device to attach.
> > * @index: The index of the PM domain.
> >
> > This naming and description is a bit misleading, because really it is not
> > attaching the device that is passed, but creating a new device to attach a
> > PM domain to. So we should consider renaming and changing the description
> > and indicate that users need to link the device.
>
> I picked the name to be consistent with the existing
> genpd_dev_pm_attach(). Do you have a better suggestion?

Well, it appears to get more of a 'get' function and so I don't see why 
we could not have 'genpd_dev_get_by_id()' and then we could have a 
genpd_dev_put() as well (which would call genpd_dev_pm_detach).

> I agree, some details is missing to the description, let me try to
> improve it. Actually, I was trying to follow existing descriptions
> from genpd_dev_pm_attach(), so perhaps that also needs a little
> update.
>
> However, do note that, neither genpd_dev_pm_attach() or
> genpd_dev_pm_attach_by_id() is supposed to be called by drivers, but
> rather only by the driver core. So description may not be so
> important.
>
> In regards to good descriptions, for sure the API added in patch9,
> dev_pm_domain_attach_by_id(), needs a good one, as this is what
> drivers should be using.

OK. Same appears to apply here to the description as I mentioned above. 
Still seems to be more of a 'get' than an attach. So I wonder if it 
should be dev_pm_domain_get_by_id() instead?

> > Finally, how is a PM domain attached via calling genpd_dev_pm_attach_by_id()
> > detached?
>
> Via the existing genpd_dev_pm_detach(), according to what I have
> described in the change log. I clarify the description in regards to
> this as well.

OK, so this bit is a to-do as that is not yet exposed AFAICT. I see that 
you said 'although we need to extend it to cover cleanup of the earlier 
registered device, via calling device_unregister().' So if we do this 
then that would be fine.

Cheers!
Jon

--
nvpublic