by Mark Brown

[permalink] [raw]

Subject: Re: [PATCH RFC 01/10] ARM: s3c64xx: pm: Use name field of generic_pm_domain

On Sat, Jan 11, 2014 at 08:42:43PM +0100, Tomasz Figa wrote:

> static struct s3c64xx_pm_domain s3c64xx_pm_irom = {
> - .name = "IROM",
> .ena = S3C64XX_NORMALCFG_IROM_ON,
> .pd = {
> + .name = "domain_irom",

This is nitpicking a bit but are you sure this is actually a
beautification of the name? It's mainly the domain_ bit, mostly since
I'd expect that if it's not obvious that this is a power domain the core
logging would be changed rather than tweaking the name of every power
domain user.

Attachments:

(No filename) (504.00 B)
signature.asc (836.00 B)
Digital signature Download all attachments

2014-01-12 19:25:11

On Sat, Jan 11, 2014 at 08:42:48PM +0100, Tomasz Figa wrote:

> This patch adds support for registering power domains of S3C64xx SoCs
> and binding devices to them using device tree.

> +#ifdef CONFIG_OF
> +static struct of_device_id s3c64xx_pd_matches[] = {
> + { .compatible = "samsung,s3c6400-clock", },
> + { .compatible = "samsung,s3c6410-clock", },
> + { },
> };

Looks like a rebasing issue? Otherwise this looks nice.

Attachments:

(No filename) (428.00 B)
signature.asc (836.00 B)
Digital signature Download all attachments

2014-01-12 19:34:20

by Tomasz Figa

[permalink] [raw]

Subject: Re: [PATCH RFC 06/10] ARM: s3c64xx: pm: Add device tree based power domain instantiation

On 12.01.2014 20:29, Mark Brown wrote:
> On Sat, Jan 11, 2014 at 08:42:48PM +0100, Tomasz Figa wrote:
>
>> This patch adds support for registering power domains of S3C64xx SoCs
>> and binding devices to them using device tree.
>
>> +#ifdef CONFIG_OF
>> +static struct of_device_id s3c64xx_pd_matches[] = {
>> + { .compatible = "samsung,s3c6400-clock", },
>> + { .compatible = "samsung,s3c6410-clock", },
>> + { },
>> };
>
> Looks like a rebasing issue? Otherwise this looks nice.

Hmm, what's the problem here? Maybe I'm just blind, but I fail to see
anything wrong (except that the array should be const...).

Thanks for reviewing this.

Best regards,
Tomasz

2014-01-13 11:10:12

[permalink] [raw]

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

On 23 January 2014 01:31, Tomasz Figa <[email protected]> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/