Re: [PATCH 2/2] TVP514x Driver with Review comments fixed [V4]

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

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@ti.com 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html