Re: [PATCH v10 4/7] MFD: TWL4030: power scripts for OMAP3 boards

["Manuel, Lesly Arackal" <leslyam@ti.com>]

Hi Kevin,

On Thu, Jun 2, 2011 at 11:59 PM, Kevin Hilman <khilman@ti.com> wrote:
> Lesly A M <leslyam@ti.com> 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.

>> 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 <leslyam@ti.com>
>> Cc: Nishanth Menon <nm@ti.com>
>> Cc: David Derrick <dderrick@ti.com>
>> Cc: Samuel Ortiz <sameo@linux.intel.com>
>> ---
>>  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.

Thanks & Regards,
Lesly
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html