Yep, nice to see an I2C driver that actually tries to cope
with errors when reading/writing registers. :)
I expect that I'll have some more comments later.
On Tuesday 02 December 2008, hvaibhav@xxxxxx wrote:
> +void tvp514x_reg_dump(struct tvp514x_decoder *decoder)
"static void" ... there probably shouldn't be any symbols
exported from this driver, except the ones handling module
init (which are in a section that's removed ASAP).
> +/*
> + * TVP5146 Init/Power on Sequence
> + */
> +static struct tvp514x_reg tvp5146_init_reg_seq[] = {
> + {TOK_WRITE, REG_VBUS_ADDRESS_ACCESS1, 0x02},
> + {TOK_WRITE, REG_VBUS_ADDRESS_ACCESS2, 0x00},
> + {TOK_WRITE, REG_VBUS_ADDRESS_ACCESS3, 0x80},
> + {TOK_WRITE, REG_VBUS_DATA_ACCESS_NO_VBUS_ADDR_INCR, 0x01},
> + ...
> +};
> +static struct tvp514x_init_seq tvp5146_init = {
> + .no_regs = ARRAY_SIZE(tvp5146_init_reg_seq),
> + .init_reg_seq = tvp5146_init_reg_seq,
> +};
I suggest making all of those be "static const", along
with their friends :)
> + {"tvp5146", (unsigned int)&tvp5146_init},
As I noted earlier, best if this were "unsigned long".
I'm surprised you're only using the driver_data to hold
a pointer to a tvp514x_init_seq structure. That would
be the natural place to store other chip-specific
data ... like whatever is needed to ensure that this
driver never tries to access a '46-specific register
on a '47 chip.
- 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
