"Manuel, Lesly Arackal" <[email protected]> writes: > Hi Kevin, > > On Thu, Jun 2, 2011 at 11:59 PM, Kevin Hilman <[email protected]> wrote: >> Lesly A M <[email protected]> writes: >> >>> Power bus message sequence for TWL4030 to enter sleep/wakeup/warm_reset. >>> >>> TWL4030 power scripts which can be used by different OMAP3 boards >>> with the power companion chip (TWL4030 series). >>> >>> The twl4030 generic script can be used by any board file to update >>> the power data in twl4030_platform_data. >> >> What about existing board files that are using their own scripts? >> >> On n900 for example, board-rx51-peripherals.c has its own custom >> scripts which are registered at board init time. These are then >> over written by your new driver which loads later. >> >> So first, can you verify if the n900 scripts can/should be replaced by >> these generic ones. And second, please find a way for board files to >> override these scripts at runtime instead of using a Kconfig. > > Customer boards have to modify the script for their board > (may be using modem which is not present in SDP). > Is it ok to add a flag to check whether the board file has already > initialized the script.
Sounds fine. >>> Since the TWL4030 power script has dependency with APIs in twl4030-power.c >>> removing the __init for these APIs. >> >> I think separating this part out into a separate patch, with a better >> description would make the rest of this patch much more readable. As it >> is, it doesn't make much sense since all the script functions and data >> are __init or __initdata. > Ok > >> These changes also introduce a new complier warning: >> >> CC drivers/mfd/twl4030-power.o >> /work/kernel/omap/pm/drivers/mfd/twl4030-power.c: In function >> 'twl4030_power_init': >> /work/kernel/omap/pm/drivers/mfd/twl4030-power.c:457:5: warning: 'err' may >> be used uninitialized in this function >> /work/kernel/omap/pm/drivers/mfd/twl4030-power.c:171:6: note: 'err' was >> declared here > > Ok, I will fix it >> >>> For more information please see: >>> http://omapedia.org/wiki/TWL4030_power_scripts >> >> This is an excellent wiki, thank for the detailed description! >> >>> Signed-off-by: Lesly A M <[email protected]> >>> Cc: Nishanth Menon <[email protected]> >>> Cc: David Derrick <[email protected]> >>> Cc: Samuel Ortiz <[email protected]> >>> --- >>> arch/arm/configs/omap2plus_defconfig | 1 + >>> arch/arm/mach-omap2/devices.c | 15 ++ >>> drivers/mfd/Kconfig | 11 + >>> drivers/mfd/Makefile | 1 + >>> drivers/mfd/twl4030-power.c | 31 ++-- >>> drivers/mfd/twl4030-script-omap.c | 373 >>> ++++++++++++++++++++++++++++++++++ >>> include/linux/i2c/twl.h | 41 ++++- >>> 7 files changed, 454 insertions(+), 19 deletions(-) >>> create mode 100644 drivers/mfd/twl4030-script-omap.c >>> >>> diff --git a/arch/arm/configs/omap2plus_defconfig >>> b/arch/arm/configs/omap2plus_defconfig >>> index 076db52..d9b9858 100644 >>> --- a/arch/arm/configs/omap2plus_defconfig >>> +++ b/arch/arm/configs/omap2plus_defconfig >>> @@ -184,6 +184,7 @@ CONFIG_TWL4030_WATCHDOG=y >>> CONFIG_MENELAUS=y >>> CONFIG_TWL4030_CORE=y >>> CONFIG_TWL4030_POWER=y >>> +CONFIG_TWL4030_SCRIPT=m >>> CONFIG_REGULATOR=y >>> CONFIG_REGULATOR_TWL4030=y >>> CONFIG_REGULATOR_TPS65023=y >>> diff --git a/arch/arm/mach-omap2/devices.c b/arch/arm/mach-omap2/devices.c >>> index 7b85585..7653329 100644 >>> --- a/arch/arm/mach-omap2/devices.c >>> +++ b/arch/arm/mach-omap2/devices.c >>> @@ -329,6 +329,20 @@ static void omap_init_audio(void) >>> static inline void omap_init_audio(void) {} >>> #endif >>> >>> +#ifdef CONFIG_ARCH_OMAP3 >> >> Shouldn't this depend on CONFIG_TWL4030_SCRIPT ? > Ok > >>> +static struct platform_device omap_twl4030_script = { >>> + .name = "twl4030_script", >>> + .id = -1, >>> +}; >>> + >>> +static void omap_init_twl4030_script(void) >>> +{ >>> + platform_device_register(&omap_twl4030_script); >>> +} >>> +#else >>> +static inline void omap_init_twl4030_script(void) {} >>> +#endif >> >> I guess this gets to the debate about whether these scripts should be in >> drivers/mfd or in arch/arm/mach-omap2, but wherever the script data >> lives, this platform_device definition and register should be also. >> >> IOW, there's not a clean separation between driver and device currently. >> IMO, The platform_driver part should just be part of twl4030-power.c, >> and all the scripts and platform_device creation/registration should be >> part of the script file. >> >> As far as where the script device data should go, I'd vote for >> drivers/mfd, where it can be compiled as a module along with the rest of >> the twl4030 code. > > Any final conclusion ? > >>> #if defined(CONFIG_SPI_OMAP24XX) || defined(CONFIG_SPI_OMAP24XX_MODULE) >>> >>> #include <plat/mcspi.h> >>> @@ -691,6 +705,7 @@ static int __init omap2_init_devices(void) >>> omap_init_sham(); >>> omap_init_aes(); >>> omap_init_vout(); >>> + omap_init_twl4030_script(); >>> >>> return 0; >>> } >>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig >>> index 3ed3ff0..bed88ce 100644 >>> --- a/drivers/mfd/Kconfig >>> +++ b/drivers/mfd/Kconfig >>> @@ -204,6 +204,17 @@ config TWL4030_POWER >>> and load scripts controlling which resources are switched off/on >>> or reset when a sleep, wakeup or warm reset event occurs. >>> >>> +config TWL4030_SCRIPT >>> + tristate "Support TWL4030 script for OMAP3 boards" >>> + depends on TWL4030_CORE && TWL4030_POWER >>> + help >>> + Say yes here if you want to use the twl4030 power scripts >>> + for OMAP3 boards. Power bus message sequence for >>> + TWL4030 to enter sleep/wakeup/warm_reset. >>> + >>> + TWL4030 power scripts which can be used by different >>> + OMAP3 boards with the power companion chip (TWL4030 series). >>> + >> >> Why do we need another Kconfig for this? It should be enabled and >> overridden at runtime via board files. > > Kconfig option was added when script was changed to insert-able module, > which was suggested by Tony as comment for last version. Yes, it needs to be a loadable module, but doesn't need Kconfig. IMO, it should just be build whenever CONFIG_TWL4030_POWER=y && CONFIG_ARCH_OMAP2PLUS=y Adding another build option means needing to ensure more build-option testing coverage. Kevin -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
