[i2c] [patch 2.6.21-rc5-git +i2c 2/2] convert tps65010 to new-style driver

[David Brownell]

On Thursday 05 April 2007 12:05 pm, Jean Delvare wrote:

> > Tested on OSK ... which should now behave, mostly, with the upstream kernel.
> > Note that the previous patch (1/2) doesn't depend on the I2C stack updates,
> > but this one does.  And I've arranged the patch so it won't conflict with
> > what's pending in the linux-omap tree ... so if this gets added to the merge
> > queue for 2.6.22-early, I anticipate few consequential problems.
> 
> Then I suggest that you keep the patches in your local stack for now,
> and resend them when then can be merged.

OK.  "Few" meaning "none" actually -- I tested it! -- but this could wait
until some more of Tony's mountain of patches merge.



> > +static struct i2c_board_info __initdata h2_i2c_board_info[] = {
> > +{
> > +	I2C_BOARD_INFO("tps65010", 0x48),
> > +	.type		= "tps65010",
> > +	.irq		= OMAP_GPIO_IRQ(58),
> > +},
> 
> You're missing one level of indentation here, and same for the other
> declarations later on.

I don't see anything in Documentation/CodingStyle saying that ...



> > +	/* gpio3 for SD, gpio4 for VDD_DSP */
> > +	// FIXME power DSP iff it's configured
> 
> No C++-style comments please.

If you insist ... but then it's likely to *NEVER* get fixed!  :(



> > +#ifdef CONFIG_TPS65010
> > +static int __init h3_tps_init(void)
> > +{
> > +	if (!machine_is_omap_h3())
> > +		return 0;
> > +
> > +	/* gpio4 for SD, gpio3 for VDD_DSP */
> > +	// FIXME power DSP iff it's configured
> > +
> > +#ifdef CONFIG_PM
> > +	/* Enable LOW_PWR */
> > +	tps65013_set_low_pwr(ON);
> > +#endif
> 
> The H2 and OSK cases don't have this #ifdef, should it be added?

More likely removed from H3.  H2 and OSK worked fine without it.
But without an H3 to test, I'm unwilling to take that risk ...



> > @@ -462,66 +456,51 @@ static irqreturn_t tps65010_irq(int irq,
> >  
> >  static struct tps65010 *the_tps;
> >  
> > -static int __exit tps65010_detach_client(struct i2c_client *client)
> > +static int __exit tps65010_remove(struct i2c_client *client)
> 
> Shouldn't this be __devexit?

And likewise with __init/__devinit ... in practice we always want this
and i2c-omap statically linked, so that "cleanup" just wastes memory;
but it's a cost we seem stuck with for now.

- Dave