On 23 January 2014 01:31, Tomasz Figa <tomasz.figa@xxxxxxxxx> 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@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
