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

[Ilkka Koskinen]

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
> appropriate.
> - 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... :(

> Cheers,
> Samuel.
>
>
> > +	} 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  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/