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.
_______________________________________________
linux-dvb mailing list
linux-dvb@xxxxxxxxxxx
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
