On Tue, Jun 28, 2011 at 4:25 AM, Rafael J. Wysocki <rjw@xxxxxxx> wrote:
> On Monday, June 27, 2011, Magnus Damm wrote:
>> On Sun, Jun 26, 2011 at 6:31 AM, Rafael J. Wysocki <rjw@xxxxxxx> wrote:
>> > From: Rafael J. Wysocki <rjw@xxxxxxx>
>> >
>> > Use the generic power domains support introduced by the previous
>> > patch to implement support for power domains on SH7372.
>> >
>> > Signed-off-by: Rafael J. Wysocki <rjw@xxxxxxx>
>> > Acked-by: Paul Mundt <lethal@xxxxxxxxxxxx>
>> > ---
>>
>> Hi Rafael,
>>
>> Thanks for your work on this. I've been working on up-porting my A3RV
>> prototype, and I came across these minor details:
>>
>> > --- linux-2.6.orig/arch/arm/mach-shmobile/board-mackerel.c
>> > +++ linux-2.6/arch/arm/mach-shmobile/board-mackerel.c
>> > @@ -1582,6 +1582,10 @@ static void __init mackerel_init(void)
>> >
>> > platform_add_devices(mackerel_devices, ARRAY_SIZE(mackerel_devices));
>> >
>> > + sh7372_init_pm_domain(SH7372_A4LC);
>> > + sh7372_add_device_to_domain(SH7372_A4LC, &lcdc_device);
>> > + sh7372_add_device_to_domain(SH7372_A4LC, &hdmi_lcdc_device);
>> > +
>> > hdmi_init_pm_clock();
>> > sh7372_pm_init();
>> > }
>> > Index: linux-2.6/arch/arm/mach-shmobile/include/mach/sh7372.h
>> > ===================================================================
>> > --- linux-2.6.orig/arch/arm/mach-shmobile/include/mach/sh7372.h
>> > +++ linux-2.6/arch/arm/mach-shmobile/include/mach/sh7372.h
>> > @@ -12,6 +12,7 @@
>> > #define __ASM_SH7372_H__
>> >
>> > #include <linux/sh_clk.h>
>> > +#include <linux/pm_domain.h>
>> >
>> > /*
>> > * Pin Function Controller:
>> > @@ -470,4 +471,31 @@ extern struct clk sh7372_fsibck_clk;
>> > extern struct clk sh7372_fsidiva_clk;
>> > extern struct clk sh7372_fsidivb_clk;
>> >
>> > +struct platform_device;
>> > +
>> > +struct sh7372_pm_domain {
>> > + struct generic_pm_domain genpd;
>> > + unsigned int bit_shift;
>> > +};
>> > +
>> > +static inline struct sh7372_pm_domain *to_sh7372_pd(struct generic_pm_domain *d)
>> > +{
>> > + return container_of(d, struct sh7372_pm_domain, genpd);
>> > +}
>> > +
>> > +#ifdef CONFIG_PM
>> > +extern struct sh7372_pm_domain sh7372_a4lc_domain;
>> > +#define SH7372_A4LC (&sh7372_a4lc_domain)
>> > +
>> > +extern void sh7372_init_pm_domain(struct sh7372_pm_domain *sh7372_pd);
>> > +extern void sh7372_add_device_to_domain(struct sh7372_pm_domain *sh7372_pd,
>> > + struct platform_device *pdev);
>> > +#else
>> > +#define SH7372_A4LC NULL
>> > +
>> > +static inline void sh7372_init_pm_domain(struct sh7372_pm_domain *sh7372_pd) {}
>> > +static inline void sh7372_add_device_to_domain(struct sh7372_pm_domain *pd,
>> > + struct platform_device *pdev) {}
>> > +#endif /* CONFIG_PM */
>> > +
>> > #endif /* __ASM_SH7372_H__ */
>>
>> Wouldn't it be easier to simply get rid of SH7372_A4LC?
>
> Not really, because the code won't build for both CONFIG_PM_RUNTIME and
> CONFIG_SUSPEND unset (resulting in CONFIG_PM unset).
>
>> Perhaps you have some special intention behind your #define, but for me the
>> following change is working just fine:
>
> Well, if CONFIG_PM is unset, sh7372_a4lc is not defined.
True, but in the CONFIG_PM=n case sh7372_a4lc is never used by
sh7372_init_pm_domain() or sh7372_add_device_to_domain().
How about letting the preprocessor do the work for us instead? This
certainly builds without SH7372_A4LC in case of CONFIG_PM=n:
#else
#define sh7372_init_pm_domain(pd) do { } while(0)
#define sh7372_add_device_to_domain(pd, pdev) do { } while(0)
#endif /* CONFIG_PM */
Cheers,
/ magnus
_______________________________________________
linux-pm mailing list
linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/linux-pm
