Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752283AbaBXMSD (ORCPT ); Mon, 24 Feb 2014 07:18:03 -0500 Received: from mail-qc0-f172.google.com ([209.85.216.172]:36262 "EHLO mail-qc0-f172.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751787AbaBXMSB (ORCPT ); Mon, 24 Feb 2014 07:18:01 -0500 MIME-Version: 1.0 In-Reply-To: <52E062D8.1060206@gmail.com> References: <1389469372-17199-1-git-send-email-tomasz.figa@gmail.com> <1389469372-17199-5-git-send-email-tomasz.figa@gmail.com> <20140123001802.GF13785@codeaurora.org> <52E062D8.1060206@gmail.com> Date: Mon, 24 Feb 2014 13:11:00 +0100 Message-ID: Subject: Re: [PATCH RFC 04/10] base: power: Add generic OF-based power domain look-up From: Ulf Hansson To: Tomasz Figa Cc: Stephen Boyd , "linux-pm@vger.kernel.org" , Mark Rutland , devicetree@vger.kernel.org, linux-samsung-soc@vger.kernel.org, Russell King , Pawel Moll , Len Brown , Greg Kroah-Hartman , Tomasz Figa , Ian Campbell , "Rafael J. Wysocki" , "linux-kernel@vger.kernel.org" , Rob Herring , Bartlomiej Zolnierkiewicz , Kukjin Kim , Pavel Machek , Kumar Gala , Stephen Warren , "linux-arm-kernel@lists.infradead.org" , Kevin Hilman Content-Type: text/plain; charset=ISO-8859-1 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 23 January 2014 01:31, Tomasz Figa 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/ -- 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/