Re: [PATCH v2] TWL4030: Initial support for TWL5031

From: Ilkka Koskinen
Date: Fri Nov 06 2009 - 09:34:30 EST

Hi Samuel,

Thanks for comments. I'll fix them but I'd
still have a couple of comments

On Wed, 4 Nov 2009, Samuel Ortiz wrote:

Hi Ilkka,

Some comments below:

On Fri, Oct 16, 2009 at 05:12:40PM +0300, Ilkka Koskinen wrote:
TWL5031 introduces two new interrupts in PIH. Moreover, BCI
has changed remarkably and, thus, it's disabled when TWL5031
is in use.

Signed-off-by: Ilkka Koskinen <ilkka.koskinen@xxxxxxxxx>
drivers/mfd/twl4030-core.c | 12 ++++-
drivers/mfd/twl4030-irq.c | 127 +++++++++++++++++++++++++++++++++++++++++--
include/linux/i2c/twl4030.h | 47 ++++++++++++++--
3 files changed, 173 insertions(+), 13 deletions(-)

@@ -284,11 +367,20 @@ static int twl4030_init_sih_modules(unsigned line)
/* disable all interrupts on our line */
memset(buf, 0xff, sizeof buf);
sih = sih_modules;
- for (i = 0; i < ARRAY_SIZE(sih_modules); i++, sih++) {
+ for (i = 0; i < nr_sih_modules; i++, sih++) {

/* skip USB -- it's funky */
- if (!sih->bytes_ixr)
+ /* But don't skip TWL5031's ACI */
+ if (!sih->bytes_ixr) {
+ if (chip_is_twl5031 && !strcmp(sih->name, "aci")) {
+ twl4030_i2c_write(TWL5031_MODULE_ACCESSORY,
+ buf, TWL5031_ACIIMR_LSB, 1);
+ twl4030_i2c_write(TWL5031_MODULE_ACCESSORY,
+ buf, TWL5031_ACIIMR_MSB, 1);
+ }
A few comments on this:
- Why do we have to do that for the accessory ? Some comments would be
- You could use twl4030_i2c_write_u8(), couldnt you ?
- I'd like it to be more generic: TWL5031_MODULE_ACCESSORY should be replaced
by sih->module, and the TWL5031_ACII* register should be somehow integrated in
the sih struct. The idea is to get rid of this chip_is_twl5031.

Very good point. Actually, I could change it to use isr/imr offsets if I add another variable in sih structure describing a number of supported irq lines. (ACI supports just one...) I think it would be a lot cleaner approach that way.

@@ -756,3 +857,17 @@ int twl_exit_irq(void)
return 0;
+int twl_init_chip_irq(const char *chip)
+ if (!strcmp(chip, "twl5031")) {
+ chip_is_twl5031 = 1;
+ sih_modules = sih_modules_twl5031;
+ nr_sih_modules = ARRAY_SIZE(sih_modules_twl5031);
I dont think you need nr_sih_modules at all. sih_modules is pointing to the
right array, and then you can keep the ARRAY_SIZE(sih_modules) call instead of
defining yet another static variable.

What comes to adding another static variable, I don't like it either.

But, as far as I can see ARRAY_SIZE is using sizeof operator, which is calculated in compilation time, right? Number of sih modules is decided in probing time based on the used twl chip and, thus, ARRAY_SIZE cannot be used. Or did I miss something?

Talking about static variables and such, I really think the twl driver needs
some cleanup. It should really be allocating a private device structure at
probe time containing all those static crap. As this driver is growing and
supporting more and more HW, it starts to be messy.

That would be great. If I only had some time... :(


+ } else {
+ sih_modules = sih_modules_twl4030;
+ nr_sih_modules = ARRAY_SIZE(sih_modules_twl4030);
+ }
+ return 0;

Cheers, Ilkka
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at
Please read the FAQ at