On 4/4/07, Domen Puncer <[EMAIL PROTECTED]> wrote: > Hi! > > How about something like the following. > Changes: > - lots of code moved from asm to C > - add compatible "mpc5200" to lite5200x soc (already is this > way on efika). And change Efika's soc device_type to "soc". > - add wakeup supported with RTC (1 to 24*60-1 minutes) > - each board now configures it's wakeup mode and possibly > board suspend and resume functions it needs to call (USB on lite)
Sorry it took so long for me to review and get back to you on this patch. I got wrapped up in other tasks. I mostly like the approach that you're taking here to allow each board to select it's own wakeup method, but I think it should be taken a step farther. Instead of providing a set of stock mpc52xx_set_wakeup_*() functions, I think it would make for simpler and more flexible code if each board takes care of its own wakeup configuration in the board_suspend_prepare hook. In fact, the mpc52xx_set_wakeup_*() functions themselves make a lot of sense, and are useful utility functions. But rather than trying to codify which pins/rtc to configure for wakeup via the .mask, .level, .pin and .delay items, those functions should be called directly by lite5200_suspend_prepare() and efika_suspend_prepare(). Doing this will drop 19 lines of common code in exchange for adding 2 lines to each board port. As an added bonus, doing it this way allows multiple pins to be used for wakeup. :-) > This code survived > 70k suspend/resume cycles :-) That's pretty good evidence. :-) > Comments? A few more comments below > --- /dev/null > +++ grant.git/arch/powerpc/platforms/52xx/mpc52xx_pm.c <snip> > +extern void mpc52xx_ds_sram(void); > +extern const long mpc52xx_ds_sram_size; > +extern void mpc52xx_ds_cached(void); > +extern const long mpc52xx_ds_cached_size; This *looks* like dangerous code (function prototypes not in a shared header). You should add a big comment here to the fact that these are assembly functions that are only ever referenced in this file. =================================================================== > --- grant.git.orig/arch/powerpc/boot/dts/lite5200.dts > +++ grant.git/arch/powerpc/boot/dts/lite5200.dts > @@ -49,6 +49,7 @@ > > [EMAIL PROTECTED] { > model = "fsl,mpc5200"; > + compatible = "mpc5200"; > revision = "" // from bootloader > #interrupt-cells = <3>; > device_type = "soc"; > Index: grant.git/arch/powerpc/boot/dts/lite5200b.dts > =================================================================== > --- grant.git.orig/arch/powerpc/boot/dts/lite5200b.dts > +++ grant.git/arch/powerpc/boot/dts/lite5200b.dts > @@ -49,6 +49,7 @@ > > [EMAIL PROTECTED] { > model = "fsl,mpc5200b"; > + compatible = "mpc5200"; > revision = ""; // from bootloader > #interrupt-cells = <3>; > device_type = "soc"; Put these two changes in a seperate patch, and I'll ACK them right away =================================================================== > --- grant.git.orig/arch/powerpc/kernel/prom_init.c > +++ grant.git/arch/powerpc/kernel/prom_init.c > @@ -2142,7 +2142,7 @@ static void __init fixup_device_tree_efi > 3,12,0, 3,13,0, 3,14,0, 3,15,0 }; > struct subst_entry efika_subst_table[] = { > { "/", "device_type", prop_cstr("efika") }, > - { "/builtin", "compatible", prop_cstr("soc") }, > + { "/builtin", "device_type", prop_cstr("soc") }, > { "/builtin/ata", "compatible", > prop_cstr("mpc5200b-ata\0mpc5200-ata"), }, > { "/builtin/bestcomm", "compatible", > prop_cstr("mpc5200b-bestcomm\0mpc5200-bestcomm") }, > { "/builtin/bestcomm", "interrupts", prop_bcomm_irq, > sizeof(prop_bcomm_irq) }, > This should also be a seperate patch. Cheers, g. -- Grant Likely, B.Sc. P.Eng. Secret Lab Technologies Ltd. [EMAIL PROTECTED] (403) 399-0195 _______________________________________________ Linuxppc-embedded mailing list Linuxppc-embedded@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-embedded