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

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

On Monday 01 December 2008, Hiremath, Vaibhav wrote:
> > 
> > Another common use of driver_data is to hold a pointer
> > to a struct holding chip-specific data that doesn't fit
> > into a simple bitmask.
> > 
> [Hiremath, Vaibhav] I am trying to use/save complete init sequence in 
> id->driver_data -
> 
> static const struct i2c_device_id tvp514x_id[] = {
>         {"tvp5146", (unsigned int)&tvp5146_init},

Well, kernel_ulong_t ...

>         {"tvp5146m2", (unsigned int)&tvp514xm_init},
>         {"tvp5147", (unsigned int)&tvp5147_init},
>         {"tvp5147m1", (unsigned int)&tvp514xm_init},
>         {},
> };
> 
> NOTE: Please note that init sequence for 46, 47 are different.
> 
> But I came to know that, client structure doesn't have any parameter
> which will provide me the index under this id table. The only
> differentiating parameter we have is "name" (decoder->client->driver->name).  

Right; why would you need an index, if you've got the pointer you
were going to use to look it up anyway?

	struct tvp_init {
		enum tvp_id	id;		/* tvp5146, tvp5146m2, etc */
		short		*entries;	/* ((addr << 8) | value, etc */
		unsigned	n_entries;
	};

	struct tvp_init tvp515xm_init = { ... };
	... etc

 
> I can use "id->driver_data" only in my probe function without any index. 
> 
> So left with only following options -
> 
> 1) 
>         if (strcmp(id->name, "tvp5146") == 0)
>                 /* original 46 init seq */;
>         else if (strcmp(id->name, "tvp5147") == 0) 
>                 /* original 47 init seq */
>         else if ((strmcp(id->name, "tvp5146m2") == 0) || 
>                         (strmcp(id->name, "tvp5147m1") == 0))
>                 /* New 46/47 init seq */

Avoid that; it's already been done before you, if you pass the
right info in driver_data.

 
> 2)
> 
> Driver specific structure must contain either of

By "driver specific structure" do you mean what the
board init code stores in i2c_client.dev.platform_data?

I'm assuming that's what you mean ... but of course, a
second driver-specific structure is what you'd normally
store in probe() and retrieve with i2c_get_clientdata().


>         - Index of i2c_device_id table, use this to get the driver_data.
>           (This also requires string compare to get the index.) 

No, you're given the i2c_device_id entry as a probe() parameter.
Just dereference id to get driver_data; no stable index numbers
needed.  Store it away along through i2c_get_clientdata() if
you need it later.


>         - Pointer to init_reg_seq, which is pointer to array of structure
>           for tvp514x_regs. This is little bit ugly, since will have to 
>           export tvp514x_regs structure.

The platform_data should not hold such stuff; it's not board-specific.
I'd expect platform_data to hold regulator_init_data as needed to
instantiate the regulator; and maybe other stuff needed on this board
too.  Floor and ceiling parameters, maybe, unless they change at runtime.

The init sequence wouldn't matter at all for i2c_get_clientdata(),
since it should only kick in during probe().


>         - Or have pointer to i2c_device_id itself. (Implemented and tested)

Hmm, now I'm not sure what you mean.  Were you talking about
the i2c_get_clientdata() stuff instead?
 

> I prefer to use second option, instead of comparing the name string in
> s_power every time. And it will be very easy to add even more chips
> providing generic solution; we need to just add entry to i2c_device_id
> with expected init sequence and you are done.   

If I interpret your words correctly, I agree.  The whole point of
the id table is to let probe just use the "id" directly to get at
descriptors with chip-specific data.

- Dave
 

> Any suggestions or inputs appreciated???


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