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