[PATCH v2 2/2] ARM: Exynos: Hook up power domains to generic power domain infrastructure

[Thomas Abraham]

Hi Sylwester.

On 7 January 2012 20:14, Sylwester Nawrocki <snjw23 at gmail.com> wrote:
[...]

>> diff --git a/arch/arm/mach-exynos/pm_domains.c b/arch/arm/mach-exynos/pm_domains.c
>> new file mode 100644
>> index 0000000..95a7c55
>> --- /dev/null
>> +++ b/arch/arm/mach-exynos/pm_domains.c
>> @@ -0,0 +1,183 @@
>> +/*
>> + * Exynos4 Generic power domain support.
>> + *
>> + * Copyright (c) 2011 Samsung Electronics Co., Ltd.
>
> 2012 ?

Ok. I will change this.

>
>> + *           http://www.samsung.com

[...]

>> +static int exynos4_pd_power(struct generic_pm_domain *domain, bool power_on)
>> +{
>> +     struct exynos4_pm_domain *pd;
>> +     void __iomem *base;
>> +     u32 timeout, pwr;
>> +     char *op;
>> +
>> +     pd = container_of(domain, struct exynos4_pm_domain, pd);
>> +     base = pd->base;
>> +
>> +     pwr = (power_on) ? S5P_INT_LOCAL_PWR_EN : 0;
>
> Is there any value in parentheses around 'power_on' ?

No. I will remove the parentheses around 'power_on'.

>
>> +     __raw_writel(pwr, base);
>> +
>> +     /* Wait max 1ms */
>> +     timeout = 10;
>> +
>> +     while ((__raw_readl(base + 0x4)&  S5P_INT_LOCAL_PWR_EN) != pwr) {
>> +             if (!timeout) {
>> +                     op = (power_on) ? "enable" : "disable";
>> +                     pr_err("Power domain %s %s failed\n", op, domain->name);
>
> How about just:
>       pr_err("%s power domain state change (%d) failed\n",
>              domain->name, power_on);
> ?

>From the message it will not be clear which state change failed. The
state (enable/disable) would be helpful.



>> +                     return -ETIMEDOUT;
>> +             }
>> +             timeout--;
>> +             cpu_relax();
>
> Does cpu_relax() make any difference here ?

I need to check on this.

>
>> +             usleep_range(80, 100);
>> +     }
>> +     return 0;
>> +}
>> +

[...]

>> +             pd = kzalloc(sizeof(*pd), GFP_KERNEL);
>> +             if (!pd) {
>> +                     pr_err("exynos4_pm_init_power_domain: failed to "
>> +                                     "allocate memory for domain\n");
>
> nit: what about:
>                        pr_err("%s: failed to allocate memory for domain\n",
>                                __func__);
> ?

Yes. This is better. I will change this.

>> +                     return -ENOMEM;
>> +             }
>> +

[...]

>> +#ifdef CONFIG_S5P_DEV_CSIS0
>> +     if (pm_genpd_add_device(&exynos4_pd_cam.pd,&s5p_device_mipi_csis0.dev))
>> +             pr_info("error in adding csis0 to cam power domain\n");
>> +#endif
>
> Could you add CSIS1 as well ? Some boards will be using both MIPI-CSI receivers.

Ok. I will add CSIS1 here.

Thanks for your review.

Regards,
Thomas.

[...]