Nick Andrew wrote:
> Here's my patch so far.
>
> Two remaining problems:
>
>
> 1 - device is probed twice and I end up with 4 /dev/dvb adapters
> rather than two. The reason is that the USB configuration defines
> two interfaces, so the probe function is called once for each
> interface.
>
> I don't yet know what to do about this. Presumably the right
> solution is to configure tuner/demod 0 on the first call to
> m920x_probe(), and tuner/demod 1 on the second call. But I don't
> know how to achieve this because all the configuring happens inside
> dvb_usb_device_init() and it's mostly hardcoded inside the @adapter
> array in dvb_usb_device_properties.
>
> I wrote a quick workaround (which _isn't_ in this patch) which
> detects if it is probing bInterface == 1, and if so then returns
> without probing, and that proves my analysis (only 2 adapters
> are created under /dev/dvb).
>
>
> 2 - I haven't been able to stream anything with 'dvbstream',
> although kaffeine works fine. I expect this is not going to
> prevent acceptance of my patch.
>
>
> description:
> m920x: add support for LifeView TV Walker Twin
>
> From: Nick Andrew <nick@xxxxxxxxxxxxxxx>
>
> Add support for "LifeView TV Walker Twin" (USB IDs 10fd:0514, 10fd:0513)
>
> Signed-off-by: Nick Andrew <nick@xxxxxxxxxxxxxxx>
>
> Nick.
>
>
> ------------------------------------------------------------------------
>
> diff -r 0832954f4b9d linux/drivers/media/dvb/dvb-usb/dvb-usb-ids.h
> --- a/linux/drivers/media/dvb/dvb-usb/dvb-usb-ids.h Sun Mar 18 18:54:07 2007 -0400
> +++ b/linux/drivers/media/dvb/dvb-usb/dvb-usb-ids.h Tue Mar 20 11:49:12 2007 +1100
> @@ -142,6 +142,8 @@
> #define USB_PID_GENPIX_8PSK_WARM 0x0201
> #define USB_PID_SIGMATEK_DVB_110 0x6610
> #define USB_PID_MSI_DIGI_VOX_MINI_II 0x1513
> +#define USB_PID_LIFEVIEW_TV_WALKER_TWIN_COLD 0x0514
> +#define USB_PID_LIFEVIEW_TV_WALKER_TWIN_WARM 0x0513
>
>
> #endif
> diff -r 0832954f4b9d linux/drivers/media/dvb/dvb-usb/m920x.c
> --- a/linux/drivers/media/dvb/dvb-usb/m920x.c Sun Mar 18 18:54:07 2007 -0400
> +++ b/linux/drivers/media/dvb/dvb-usb/m920x.c Tue Mar 20 11:49:12 2007 +1100
> @@ -5,6 +5,8 @@
> * This program is free software; you can redistribute it and/or modify it
> * under the terms of the GNU General Public License as published by the Free
> * Software Foundation, version 2.
> + *
> + * LifeView TV Walker Twin support by Nick Andrew <nick@xxxxxxxxxxxxxxx>
> *
> * see Documentation/dvb/README.dvb-usb for more information
> */
^^ Is this really necessary? When the patch gets committed, your author
information will be stored in the changeset meta-data. This is how we track
authorship. If we added a by-line for every card support patch, we would have
more by-lines than code!
If you really find it necessary to do this, then my preference would be to add a
/* comment */ directly above the dvb_usb_device_properties struct being added.
> @@ -41,6 +43,26 @@ static struct dvb_usb_rc_key megasky_rc_
> { 0x0, 0x0e, KEY_COFFEE }, /* "MTS" */
> };
>
> +static struct dvb_usb_rc_key tvwalkertwin_rc_keys [] = {
> + { 0x0, 0x01, KEY_ZOOM }, /* Full Screen */
> + { 0x0, 0x02, KEY_CAMERA }, /* snapshot */
> + { 0x0, 0x03, KEY_MUTE },
> + { 0x0, 0x04, KEY_REWIND },
> + { 0x0, 0x05, KEY_PLAYPAUSE }, /* Play/Pause */
> + { 0x0, 0x06, KEY_FASTFORWARD },
> + { 0x0, 0x07, KEY_RECORD },
> + { 0x0, 0x08, KEY_STOP },
> + { 0x0, 0x09, KEY_TIME }, /* Timeshift */
> + { 0x0, 0x0c, KEY_COFFEE }, /* Recall */
> + { 0x0, 0x0e, KEY_CHANNELUP },
> + { 0x0, 0x12, KEY_POWER },
> + { 0x0, 0x15, KEY_MENU }, /* source */
> + { 0x0, 0x18, KEY_CYCLEWINDOWS }, /* TWIN PIP */
> + { 0x0, 0x1a, KEY_CHANNELDOWN },
> + { 0x0, 0x1b, KEY_VOLUMEDOWN },
> + { 0x0, 0x1e, KEY_VOLUMEUP },
> +};
> +
> static inline int m9206_read(struct usb_device *udev, u8 request, u16 value,\
> u16 index, void *data, int size)
> {
> @@ -50,12 +72,12 @@ static inline int m9206_read(struct usb_
> request, USB_TYPE_VENDOR | USB_DIR_IN,
> value, index, data, size, 2000);
> if (ret < 0) {
> - printk(KERN_INFO "m920x_read = error: %d\n", ret);
> + printk(KERN_INFO "m9206_read = error: %d\n", ret);
> return ret;
> }
>
> if (ret != size) {
> - deb_rc("m920x_read = no data\n");
> + deb_rc("m9206_read = no data\n");
> return -EIO;
> }
>
^^^^ These trivial changes have nothing to do with your device-support patch,
and should be removed, perhaps submitted separately.
I am aware that the m920x.c needs some naming consistency cleanups, and I have
not applied a pending patch from Aapo, which cleans that stuff up, due to the
potential of merge clashes amongst the rest of the m920x developers.
This can be taken care of later. Separate changes should always be submitted in
separate patches, and codingstyle cleanups should be submitted separately from
functional changes.
> @@ -74,17 +96,23 @@ static inline int m9206_write(struct usb
> return ret;
> }
>
> -static int m9206_init(struct dvb_usb_device *d)
> +static int m9206_init(struct dvb_usb_device *d, struct m9206_inits *rc_seq)
> {
> int ret = 0;
>
> /* Remote controller init. */
> if (d->props.rc_query) {
> - if ((ret = m9206_write(d->udev, M9206_CORE, 0xa8, M9206_RC_INIT2)) != 0)
> - return ret;
> -
> - if ((ret = m9206_write(d->udev, M9206_CORE, 0x51, M9206_RC_INIT1)) != 0)
> - return ret;
> + deb_rc("Initialising remote control\n");
> + while (rc_seq->address) {
> + if ((ret = m9206_write(d->udev, M9206_CORE, rc_seq->data, rc_seq->address)) != 0) {
> + deb_rc("Initialising remote control failed\n");
> + return ret;
> + }
> +
> + rc_seq++;
> + }
> +
> + deb_rc("Initialising remote control success\n");
> }
>
> return ret;
I'm going to have to review this part a bit closer later on.
> @@ -109,6 +137,14 @@ static int m9206_rc_query(struct dvb_usb
> switch(rc_state[0]) {
> case 0x80:
> *state = REMOTE_NO_KEY_PRESSED;
> + goto unlock;
> +
> + case 0x88: /* framing error or "invalid code" */
> + case 0x99:
> + case 0xc0:
> + case 0xd8:
> + *state = REMOTE_NO_KEY_PRESSED;
> + m->rep_count = 0;
> goto unlock;
>
> case 0x93:
> @@ -118,20 +154,22 @@ static int m9206_rc_query(struct dvb_usb
> goto unlock;
>
> case 0x91:
> - /* For comfort. */
> + /* prevent immediate auto-repeat */
> if (++m->rep_count > 2)
> *state = REMOTE_KEY_REPEAT;
> + else
> + *state = REMOTE_NO_KEY_PRESSED;
> goto unlock;
>
> default:
> - deb_rc("Unexpected rc response %x\n", rc_state[0]);
> + deb_rc("Unexpected rc state %02x\n", rc_state[0]);
> *state = REMOTE_NO_KEY_PRESSED;
> goto unlock;
> }
> }
>
> if (rc_state[1] != 0)
> - deb_rc("Unknown rc key %x\n", rc_state[1]);
> + deb_rc("Unknown rc key %02x\n", rc_state[1]);
>
> *state = REMOTE_NO_KEY_PRESSED;
>
> @@ -447,9 +485,100 @@ static int digivox_tda8275_tuner_attach(
> return 0;
> }
>
> +/* LifeView TV Walker Twin has 1 x M9206, 2 x TDA10046, 2 x TDA8275A
> + * TDA10046 #0 is located at i2c address 0x08
> + * TDA10046 #1 is located at i2c address 0x0b
> + * TDA8275A #0 is located at i2c address 0x60
> + * TDA8275A #1 is located at i2c address 0x61
> + */
> +
> +static struct tda1004x_config tvwalkertwin_tda10046_config0 = {
> + .demod_address = 0x08,
> + .invert = 0,
> + .invert_oclk = 0,
> + .ts_mode = TDA10046_TS_SERIAL,
> + .xtal_freq = TDA10046_XTAL_16M,
> + .if_freq = TDA10046_FREQ_045,
> + .agc_config = TDA10046_AGC_TDA827X,
> + .gpio_config = TDA10046_GPTRI,
> + .request_firmware = NULL, /* uses firmware EEPROM */
> +};
> +
> +static struct tda1004x_config tvwalkertwin_tda10046_config1 = {
> + .demod_address = 0x0b,
> + .invert = 0,
> + .invert_oclk = 0,
> + .ts_mode = TDA10046_TS_SERIAL,
> + .xtal_freq = TDA10046_XTAL_16M,
> + .if_freq = TDA10046_FREQ_045,
> + .agc_config = TDA10046_AGC_TDA827X,
> + .gpio_config = TDA10046_GPTRI,
> + .request_firmware = NULL, /* uses firmware EEPROM */
> +};
> +
> +static int tvwalkertwin_tda10046_frontend_attach0(struct dvb_usb_adapter *adap)
tvwalkertwin_0_tda10046_frontend_attach would be nicer
> +{
> + deb_rc("tvwalkertwin_tda10046_frontend_attach0!\n");
> +
> + if ((adap->fe = dvb_attach(tda10046_attach, &tvwalkertwin_tda10046_config0, &adap->dev->i2c_adap)) == NULL)
> + return -EIO;
> +
> + deb_rc("Attached demod 0 at address %02x\n", tvwalkertwin_tda10046_config0.demod_address);
> +
> + return 0;
> +}
> +
> +static int tvwalkertwin_tda10046_frontend_attach1(struct dvb_usb_adapter *adap)
tvwalkertwin_1_tda10046_frontend_attach would look nicer
> +{
> + deb_rc("tvwalkertwin_tda10046_frontend_attach1!\n");
> +
> + if ((adap->fe = dvb_attach(tda10046_attach, &tvwalkertwin_tda10046_config1, &adap->dev->i2c_adap)) == NULL)
> + return -EIO;
> +
> + deb_rc("Attached demod 1 at address %02x\n", tvwalkertwin_tda10046_config1.demod_address);
> +
> + return 0;
> +}
> +
> +static struct tda827x_config tvwalkertwin_tda8275_config = {
> +};
^^^ shouldnt be needed. Pass NULL into tda827x_attach() instead of this empty
config struct. After some more cleanups in saa7134-dvb.c, we hopefully will do
away with the tda827x_config entirely.
> +
> +static int tvwalkertwin_tda8275_tuner_attach0(struct dvb_usb_adapter *adap)
tvwalkertwin_0_tda8275_tuner_attach
> +{
> + int address = 0x60;
> +
> + deb_rc("tvwalkertwin_tda8275_tuner_attach0!\n");
> +
> + if (dvb_attach(tda827x_attach, adap->fe, address, &adap->dev->i2c_adap,
> + &tvwalkertwin_tda8275_config) == NULL)
+ NULL) == NULL)
> + return -ENODEV;
> +
> + deb_rc("Attached tuner 0 at address %02x\n", address);
> +
> + return 0;
> +}
> +
> +static int tvwalkertwin_tda8275_tuner_attach1(struct dvb_usb_adapter *adap)
tvwalkertwin_1_tda8275_tuner_attach
> +{
> + int address = 0x61;
> +
> + deb_rc("tvwalkertwin_tda8275_tuner_attach1!\n");
> +
> + if (dvb_attach(tda827x_attach, adap->fe, address, &adap->dev->i2c_adap,
> + &tvwalkertwin_tda8275_config) == NULL)
> + return -ENODEV;
> +
> + deb_rc("Attached tuner 1 at address %02x\n", address);
> +
> + return 0;
> +}
> +
> /* DVB USB Driver stuff */
> static struct dvb_usb_device_properties megasky_properties;
> static struct dvb_usb_device_properties digivox_mini_ii_properties;
> +static struct dvb_usb_device_properties tvwalkertwin_properties;
> +static struct m9206_inits megasky_rc_init [];
> +static struct m9206_inits tvwalkertwin_rc_init [];
>
> static int m920x_probe(struct usb_interface *intf,
> const struct usb_device_id *id)
> @@ -457,12 +586,24 @@ static int m920x_probe(struct usb_interf
> struct dvb_usb_device *d;
> struct usb_host_interface *alt;
> int ret;
> -
> - 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.
> @@ -478,7 +619,7 @@ found:
> if (ret < 0)
> return ret;
>
> - if ((ret = m9206_init(d)) != 0)
> + if ((ret = m9206_init(d, rc_init_seq)) != 0)
> return ret;
>
> return ret;
> @@ -488,6 +629,8 @@ static struct usb_device_id m920x_table
> { USB_DEVICE(USB_VID_MSI, USB_PID_MSI_MEGASKY580) },
> { USB_DEVICE(USB_VID_ANUBIS_ELECTRONIC,
> USB_PID_MSI_DIGI_VOX_MINI_II) },
> + { USB_DEVICE(USB_VID_ANUBIS_ELECTRONIC, USB_PID_LIFEVIEW_TV_WALKER_TWIN_COLD) },
> + { USB_DEVICE(USB_VID_ANUBIS_ELECTRONIC, USB_PID_LIFEVIEW_TV_WALKER_TWIN_WARM) },
> { } /* Terminating entry */
> };
> MODULE_DEVICE_TABLE (usb, m920x_table);
...so much for the 80-column limit :-/
> @@ -585,6 +728,95 @@ static struct dvb_usb_device_properties
> }
> };
>
> +static struct dvb_usb_device_properties tvwalkertwin_properties = {
> + .caps = DVB_USB_IS_AN_I2C_ADAPTER,
> +
> + .usb_ctrl = DEVICE_SPECIFIC,
> + .firmware = "dvb-usb-tvwalkert.fw",
> + .download_firmware = m9206_firmware_download,
> +
> + .rc_interval = 100,
> + .rc_key_map = tvwalkertwin_rc_keys,
> + .rc_key_map_size = ARRAY_SIZE(tvwalkertwin_rc_keys),
> + .rc_query = m9206_rc_query,
> +
> + .size_of_priv = sizeof(struct m9206_state),
> +
> + .identify_state = m920x_identify_state,
> + .num_adapters = 2,
> + .adapter = {
> + {
> + .caps = DVB_USB_ADAP_HAS_PID_FILTER |
> + DVB_USB_ADAP_PID_FILTER_CAN_BE_TURNED_OFF,
> +
> + .pid_filter_count = 8,
> + .pid_filter = m9206_pid_filter,
> + .pid_filter_ctrl = m9206_pid_filter_ctrl,
> +
> + .frontend_attach = tvwalkertwin_tda10046_frontend_attach0,
> + .tuner_attach = tvwalkertwin_tda8275_tuner_attach0,
> +
> + .stream = {
> + .type = USB_BULK,
> + .count = 8,
> + .endpoint = 0x81,
> + .u = {
> + .bulk = {
> + .buffersize = 512,
> + }
> + }
> + },
> + },
> + {
> + .caps = DVB_USB_ADAP_HAS_PID_FILTER |
> + DVB_USB_ADAP_PID_FILTER_CAN_BE_TURNED_OFF,
> +
> + .pid_filter_count = 8,
> + .pid_filter = m9206_pid_filter,
> + .pid_filter_ctrl = m9206_pid_filter_ctrl,
> +
> + .frontend_attach = tvwalkertwin_tda10046_frontend_attach1,
> + .tuner_attach = tvwalkertwin_tda8275_tuner_attach1,
> +
> + .stream = {
> + .type = USB_BULK,
> + .count = 8,
> + .endpoint = 0x82,
> + .u = {
> + .bulk = {
> + .buffersize = 512,
> + }
> + }
> + },
> + }
> + },
> + .i2c_algo = &m9206_i2c_algo,
> +
> + .num_device_descs = 1,
> + .devices = {
> + { .name = "LifeView TV Walker Twin DVB-T USB2.0",
> + .cold_ids = { &m920x_table[2], NULL },
> + .warm_ids = { &m920x_table[3], NULL },
> + },
> + }
> +};
> +
> +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.
> static struct usb_driver m920x_driver = {
> #if LINUX_VERSION_CODE <= KERNEL_VERSION(2,6,15)
> .owner = THIS_MODULE,
> @@ -618,6 +850,6 @@ module_exit (m920x_module_exit);
> module_exit (m920x_module_exit);
>
> MODULE_AUTHOR("Aapo Tahkola <aet@xxxxxxxxxxxxxx>");
> -MODULE_DESCRIPTION("Driver MSI Mega Sky 580 DVB-T USB2.0 / Uli m920x");
> +MODULE_DESCRIPTION("Driver ULI M920x DVB-T USB2.0 / MSI Mega Sky / DigiVox / LifeView");
> MODULE_VERSION("0.1");
> MODULE_LICENSE("GPL");
^^^^ I've been meaning to change this, myself. Something more appropriate would
be as simple as:
+MODULE_DESCRIPTION("DVB Driver for ULI M920x");
The driver is not specific to MSI Megasky / Digivox / Lifeview -- we might see
more vendors using these chips in future products, and since it has a pid
filter, it may be used in usb1.1 or usb 2.0.
> diff -r 0832954f4b9d linux/drivers/media/dvb/dvb-usb/m920x.h
> --- a/linux/drivers/media/dvb/dvb-usb/m920x.h Sun Mar 18 18:54:07 2007 -0400
> +++ b/linux/drivers/media/dvb/dvb-usb/m920x.h Tue Mar 20 11:39:12 2007 +1100
> @@ -64,4 +64,13 @@ struct m9206_state {
> int filtering_enabled;
> int rep_count;
> };
> +
> +/* Initialisation data for the m920x
> + */
> +
> +struct m9206_inits {
> + u16 address;
> + u8 data;
> +};
> +
> #endif
I'll look over the rest when I have more time, and sent you my comments.
Cheers,
Michael Krufky
_______________________________________________
linux-dvb mailing list
linux-dvb@xxxxxxxxxxx
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
