lkml.org 
[lkml]   [2014]   [Feb]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH RFC 04/10] base: power: Add generic OF-based power domain look-up
    From
    On 23 January 2014 01:31, Tomasz Figa <tomasz.figa@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@vger.kernel.org
    > More majordomo info at http://vger.kernel.org/majordomo-info.html
    > Please read the FAQ at http://www.tux.org/lkml/


    \
     
     \ /
      Last update: 2014-02-24 13:41    [W:2.321 / U:0.364 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site