[i2c] [patch 2.6.22] tps65010 new-style driver updates, part 2

[Jean Delvare]

Hi David,

On Sat, 11 Aug 2007 11:17:14 -0700, David Brownell wrote:
> > > Also remove some now-superfluous #includes.
> >
> > You've been overeager, see below.
> 
> It compiles and runs fine.  In what sense could it be over-eager?
> 
> Explicitly including every single file that's referenced is not a
> requirement anywhere ... it's OK to let other headers include them
> for you.  In fact that's common practice throughout Linux.

It's a bad practice, causing random breakage when someone attempts to
cleanup includes in header files, or removes an include which is no
longer needed (but was relied upon to draw one that is needed), or
reorders includes, etc. Ideally you should always include "all" the
header files that declare something you use. I'm not saying that every
Linux driver does this, just that it's the right way to do it. Given
that the tps65010 driver does things the right way at the moment (at
least for the headers I've pointed out), I simply don't want to apply
your patch which removes needed includes. Please send an updated patch.

> > > @@ -397,6 +413,14 @@ static void __init osk_init(void)
> > >  	omap_board_config_size = ARRAY_SIZE(osk_config);
> > >  	USB_TRANSCEIVER_CTRL_REG |= (3 << 1);
> > >  
> > > +	/* irq for tps65010 chip */
> > > +	/* omap_cfg_reg(U19_1610_MPUIO1); */
> >
> > Why is this line commented out? Either it's needed, or it's not.
> 
> Call it documentation; and moving *everything* from the old "driver
> does setup" to the preferred "board init does setup" model.  It just
> happens that U19_1610_MPUIO1 is not defined, so that line couldn't
> compile.  That setup code happens to work so far, since that mux
> setting is the reset default and wasn't overridden by the particular
> bootloader that's most often used on this board.  But that's the pinmux
> setting needed for this board to operate...
> 
> It was there in the first place since it's really quite a PITA (*) to
> find the correct pin mux options ... this way that info is ready in
> case it's needed, maybe with an updated bootloader.  Bootloaders are
> not generally worth trusting for anything more than starting Linux.
> 
> Maybe you'd just like the comment to say some of that?

I was initially just surprised that the h2 code was calling
omap_cfg_reg() and the osk code wasn't, I thought the code was really
meant to be live and was commented out by accident. But if it is really
meant as a comment, that's fine with me.

Thanks,
-- 
Jean Delvare