[linux-dvb] [PATCH] m920x for LifeView TV Walker Twin

Nick Andrew nick-linuxtv at nick-andrew.net
Thu Mar 22 00:05:59 CET 2007


On Tue, Mar 20, 2007 at 10:46:05AM -0400, Michael Krufky wrote:

I'm about to post a revised patch, with your advice taken. I just
want to comment first about some of them:

> tvwalkertwin_0_tda10046_frontend_attach would be nicer

Or how about tvwalkertwin_tda10046_0_frontend_attach ?

Meaning "we're attaching the zeroeth tda10046" ?

> > -
> > -	deb_rc("Probed!\n");
> > -
> > -	if (((ret = dvb_usb_device_init(intf, &megasky_properties, THIS_MODULE, &d)) == 0) ||
> > -	    ((ret = dvb_usb_device_init(intf, &digivox_mini_ii_properties, THIS_MODULE, &d)) == 0))
> > +	struct m9206_inits *rc_init_seq = NULL;
> > +
> > +	deb_rc("Probing for m920x device\n");
> > +
> > +	if ((ret = dvb_usb_device_init(intf, &megasky_properties, THIS_MODULE, &d)) == 0) {
> > +		rc_init_seq = megasky_rc_init;
> >  		goto found;
> > +	}
> > +
> > +	if ((ret = dvb_usb_device_init(intf, &digivox_mini_ii_properties, THIS_MODULE, &d)) == 0) {
> > +		/* No remote control, so no rc_init_seq */
> > +		goto found;
> > +	}
> > +
> > +	if ((ret = dvb_usb_device_init(intf, &tvwalkertwin_properties, THIS_MODULE, &d)) == 0) {
> > +		rc_init_seq = tvwalkertwin_rc_init;
> > +		goto found;
> > +	}
> >  
> >  	return ret;
> >  
> 
> ^^^ I dont like this.  There is nothing wrong with it, persay, but I'd like to
> think that there could be a better, cleaner way.  I should have more time later
> on for a more thorough review.  If I can think of something better, I'll make
> suggestions.

There's more. In my next patch this function will also test which
interface on the device is being probed, and skip the tests if it
is interface 1. That correctly results in only two /dev/dvb adapters.

I have doubts about the elegance of this method, but as far as I can
see, the only alternative is to make changes to struct
dvb_usb_device_properties and alter function dvb_usb_device_init()
and that will be more intrusive for perhaps very small benefit.

> > +static struct m9206_inits megasky_rc_init [] = {
> > +	{ M9206_RC_INIT2, 0xa8 },
> > +	{ M9206_RC_INIT1, 0x51 },
> > +	{ } /* terminating entry */
> > +};
> > +
> > +static struct m9206_inits tvwalkertwin_rc_init [] = {
> > +	{ M9206_RC_INIT2, 0x00 },
> > +	{ M9206_RC_INIT1, 0xef },
> > +	{ 0xff28,         0x00 },
> > +	{ 0xff23,         0x00 },
> > +	{ 0xff21,         0x30 },
> > +	{ } /* terminating entry */
> > +};
> > +
> > +
> 
> ^^^  Skip 1 line, not 2 ...  These should be moved above to the device-specific
> areas, below the tuner_attach / frontend_attach functions.

I think it is better to keep the struct above the function which uses
the struct, and cluster all the device-specific code/data together
wherever possible.

Now in the case of m9206_inits the function using the struct is shared,
and that's the same with the remote control, so what do you think about
moving the m9206_inits underneath or above the structs which define
the remote control keys?

So the module structure would end up looking like:

  file header
  includes
  module params
  megasgk_rc_init
  megasky_rc_keys
  tvwalkertwin_rc_init
  tvwalkertwin_rc_keys
  m920[6x] functions & structs
  megasky functions & structs
  digivox functions & structs
  tv walker twin functions & structs
  m920x_probe function
  usb_device_id table
  megasky_properties
  digivox_mini_ii_properties
  tvwalkertwin_properties
  m920x_driver struct
  module init and exit functions
  module definition

That's pretty much what we've got now. I'd rather see m920x_probe()
moved to where the other m920[6x] functions are and the properties
structs moved to be close to the device-specific functions and
structs. The intent is just to group the device-specific code
and data. But none of that is required for TV Walker Twin support;
any big shuffles are probably best done at the same time as you
clean up the consistency, like those debugging messages I changed
in the previous patch.

Nick.



More information about the linux-dvb mailing list