> > I think this twl4030_usb.c driver should move to new style
> I2C. I'm adding
> > David Brownell on the loop as he has moved isp1301 to new
> style i2c driver.
> >
> > Dave, do you have any comments, here ?
>
> Yes, make it a new-style I2C driver. :)
> And pass the IRQ in from board-specific data.
The twl4030_usb.c is not really an I2C driver. It hooks into the
"drivers/i2c/chips/twl4030_core.c" driver which is the actual driver
for the twl4030.
We merely use this file to setup the USB functionality in the twl4030,
using the twl4030_i2c_{write,read}_u8 interfaces provided in the core file.
I'm not quite sure why this file needs to be made a proper new-style driver.
> I don't know the twl4030 but I suspect it should be plugging
> itself into an otg_transceiver structure instead of exporting
> global functions like this ...
musb_core.c already uses an otg_transceiver structure which we use in
omap2430.c. How do I introduce another otg_transciever here?
> Also:
>
> > +#define FUNC_CTRL (0x04)
>
> is bad style; drop the pointless parenthesis. Another bit of bad
> style (well, maybe just not-good style) is that the register fields
> aren't associated with the register ... e.g. it looks like
>
> > +#define FUNC_CTRL (0x04)
> > +#define FUNC_CTRL_SET (0x05)
> > +#define FUNC_CTRL_CLR (0x06)
> > +#define SUSPENDM (1 << 6)
> > +#define RESET (1 << 5)
> > +#define OPMODE_MASK (3 << 3) /* bits 3 and 4 */
> > +#define OPMODE_NORMAL (0 << 3)
<snip>
> most of these masks are valid for the "control" register, but I
> can't tell. And if I were to look at a line of code masking those
> together with AUTORESUME, the bug (bitfield from wrong register)
> would be almost impossible to discover. In those case a better
> convention is to use FN_CTRL_SUSPENDM and IF_CTRL_AUTORESUME,
> or something simmilar.
Ok. Fixed both points.
> (And a minor nit: "#define<TAB>..." makes trouble reading diffs,
> as lines stretch and sometimes wrap; "#define<SPACE>" doesn't.
> Either is correct, but the former can be a bit annoying.)
Fixed this too.
> > +#define twl4030_i2c_write_u8_verify(module, data, address)
<snip>
> Good example of something too big for a #define. Make it a function.
Fixed.
Thanks for these comments. I will send the reworked patch separately.
Appreciate the time you've taken on this.
Regards,
Anand
-
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
