[PATCH RFC 04/10] base: power: Add generic OF-based power domain look-up

[Ulf Hansson]

On 23 January 2014 01:31, Tomasz Figa <tomasz.figa at gmail.com> wrote:
> Hi Stephen,
>
>
> On 23.01.2014 01:18, Stephen Boyd wrote:
>>
>> On 01/11, Tomasz Figa wrote:
>>>
>>> +
>>> +/**
>>> + * of_genpd_lock() - Lock access to of_genpd_providers list
>>> + */
>>> +static void of_genpd_lock(void)
>>> +{
>>> +       mutex_lock(&of_genpd_mutex);
>>> +}
>>> +
>>> +/**
>>> + * of_genpd_unlock() - Unlock access to of_genpd_providers list
>>> + */
>>> +static void of_genpd_unlock(void)
>>> +{
>>> +       mutex_unlock(&of_genpd_mutex);
>>> +}
>>
>>
>> Why do we need these functions? Can't we just call
>> mutex_lock/unlock directly?
>
>
> That would be fine as well, I guess. Just duplicated the pattern used in
> CCF, but can remove them in next version if it's found to be better.
>
>
>>
>>> +
>>> +/**
>>> + * of_genpd_add_provider() - Register a domain provider for a node
>>> + * @np: Device node pointer associated with domain provider
>>> + * @genpd_src_get: callback for decoding domain
>>> + * @data: context pointer for @genpd_src_get callback.
>>
>>
>> These look a little outdated.
>
>
> Oops, missed this.
>
>
>>
>>> + */
>>> +int of_genpd_add_provider(struct device_node *np, genpd_xlate_t xlate,
>>> +                         void *data)
>>> +{
>>> +       struct of_genpd_provider *cp;
>>> +
>>> +       cp = kzalloc(sizeof(struct of_genpd_provider), GFP_KERNEL);
>>
>>
>> Please use sizeof(*cp) instead.
>
>
> Right.
>
>
>>
>>> +       if (!cp)
>>> +               return -ENOMEM;
>>> +
>>> +       cp->node = of_node_get(np);
>>> +       cp->data = data;
>>> +       cp->xlate = xlate;
>>> +
>>> +       of_genpd_lock();
>>> +       list_add(&cp->link, &of_genpd_providers);
>>> +       of_genpd_unlock();
>>> +       pr_debug("Added domain provider from %s\n", np->full_name);
>>> +
>>> +       return 0;
>>> +}
>>> +EXPORT_SYMBOL_GPL(of_genpd_add_provider);
>>> +
>>
>> [...]
>>>
>>> +
>>> +/* See of_genpd_get_from_provider(). */
>>> +static struct generic_pm_domain *__of_genpd_get_from_provider(
>>> +                                       struct of_phandle_args
>>> *genpdspec)
>>> +{
>>> +       struct of_genpd_provider *provider;
>>> +       struct generic_pm_domain *genpd = ERR_PTR(-ENOENT);
>>
>>
>> Can this be -EPROBE_DEFER so that we can defer probe until a
>> later time if the power domain provider hasn't registered yet?
>
>
> Yes, this could be useful. Makes me wonder why clock code (on which I based
> this code) doesn't have it done this way.
>
>
>>
>>> +
>>> +       /* Check if we have such a provider in our array */
>>> +       list_for_each_entry(provider, &of_genpd_providers, link) {
>>> +               if (provider->node == genpdspec->np)
>>> +                       genpd = provider->xlate(genpdspec,
>>> provider->data);
>>> +               if (!IS_ERR(genpd))
>>> +                       break;
>>> +       }
>>> +
>>> +       return genpd;
>>> +}
>>> +
>>
>> [...]
>>>
>>> +static int of_genpd_notifier_call(struct notifier_block *nb,
>>> +                                 unsigned long event, void *data)
>>> +{
>>> +       struct device *dev = data;
>>> +       int ret;
>>> +
>>> +       if (!dev->of_node)
>>> +               return NOTIFY_DONE;
>>> +
>>> +       switch (event) {
>>> +       case BUS_NOTIFY_BIND_DRIVER:
>>> +               ret = of_genpd_add_to_domain(dev);
>>> +               break;
>>> +
>>> +       case BUS_NOTIFY_UNBOUND_DRIVER:
>>> +               ret = of_genpd_del_from_domain(dev);
>>> +               break;
>>> +
>>> +       default:
>>> +               return NOTIFY_DONE;
>>> +       }
>>> +
>>> +       return notifier_from_errno(ret);
>>> +}
>>> +
>>> +static struct notifier_block of_genpd_notifier_block = {
>>> +       .notifier_call = of_genpd_notifier_call,
>>> +};
>>> +
>>> +static int of_genpd_init(void)
>>> +{
>>> +       return bus_register_notifier(&platform_bus_type,
>>> +                                       &of_genpd_notifier_block);
>>> +}
>>> +core_initcall(of_genpd_init);
>>
>>
>> Would it be possible to call the of_genpd_add_to_domain() and
>> of_genpd_del_from_domain() functions directly in the driver core,
>> similar to how the pinctrl framework has a hook in there? That
>> way we're not relying on any initcall ordering for this.
>
>
> Hmm, the initcall here just registers a notifier, which needs to be done
> just before any driver registers. So, IMHO, current variant is safe, given
> an early enough initcall level is used.
>
> However, doing it the pinctrl way might still have an advantage of not
> relying on specific bus type, so this is worth consideration indeed. I'd
> like to hear Rafael's and Kevin's opinions on this (and other comments above
> too).

As you say; certainly there will be other bus types that we need to
support as well. For example the amba bus (drivers/amba/bus.c).

Additionally I believe similar reasons, why we added the pinctrl
handling to driver core, applies to generic power domains. So I think
we should give it a try!

Kind regards
Ulf Hansson

>
> Best regards,
> Tomasz
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/