Re: [PATCH 2/3] ARM: OMAP: Add support for TWL4030 USB Transceiver on OMAP34xx

[David Brownell <david-b@pacbell.net>]

On Wednesday 05 December 2007, Felipe Balbi wrote:
> 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.

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 ...



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)
> +#define      OPMODE_NONDRIVING       (1 << 3)
> +#define      OPMODE_DISABLE_BIT_NRZI (2 << 3)
> +#define      TERMSELECT              (1 << 2)
> +#define      XCVRSELECT_MASK         (3 << 0) /* bits 0 and 1 */
> +#define      XCVRSELECT_HS           (0 << 0)
> +#define      XCVRSELECT_FS           (1 << 0)
> +#define      XCVRSELECT_LS           (2 << 0)
> +#define      XCVRSELECT_FS4LS        (3 << 0)

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.

(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.)

> +#define twl4030_i2c_write_u8_verify(module, data, address)             \
> +do {                                                                   \
> +     u8 check;                                                         \
> +     if (!((twl4030_i2c_write_u8((module), (data), (address)) >= 0) && \
> +           (twl4030_i2c_read_u8((module), &check, (address)) >= 0) &&  \
> +           (check == data))  &&                                        \
> +         !((twl4030_i2c_write_u8((module), (data), (address)) >= 0) && \
> +           (twl4030_i2c_read_u8((module), &check, (address)) >= 0) &&  \
> +           (check == (data))))       {                                 \
> +             printk(KERN_ERR "twl4030_usb: i2c write failed,           \
> +                     line %d\n", __LINE__);                            \
> +             goto i2c_failed;                                          \
> +     }                                                                 \
> +} while (0)

Good example of something too big for a #define.  Make it a function.


No comments about the rest.

- Dave

_______________________________________________
Linux-omap-open-source mailing list
Linux-omap-open-source@linux.omap.com
http://linux.omap.com/mailman/listinfo/linux-omap-open-source