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

[Jon Hunter <jonathanh@nvidia.com>]

On 24/05/18 13:17, Ulf Hansson wrote:
> On 24 May 2018 at 11:36, Jon Hunter <jonathanh@nvidia.com> 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