Skip to contentSkip to conversation

Leadtek WinFast DTV-1800H support

PATCH In review Miroslav Šustek 4 participants 8 messages May 29, 2009 – Jun 1, 2009 1 message not in the archive Media (V4L/DVB)

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

#1 lore raw

PATCH 1/1 Leadtek WinFast DTV-1800H support

Hello,
this patch adds support for Leadtek WinFast DTV-1800H hybrid card. It enables analog/digital tv, radio and remote control trough GPIO.

Input GPIO values are extracted from INF file which is included in winxp driver. Analog audio works both through cx88-alsa and through internal cable from tv-card to sound card.

Tested by me and the people listed in patch (works well).

- Miroslav Šustek

(Shoot me! Now it's correct.)

Adds support for Leadtek WinFast DTV-1800H

From: Miroslav Sustek <[email protected]>

Enables analog/digital tv, radio and remote control (gpio).

linux/Documentation/video4linux/CARDLIST.cx88 +1 -0
diff -r 65ec132f20df linux/Documentation/video4linux/CARDLIST.cx88
@@ -79,3 +79,4 @@
  78 -> Prof 6200 DVB-S                                     [b022:3022]
  79 -> Terratec Cinergy HT PCI MKII                        [153b:1177]
  80 -> Hauppauge WinTV-IR Only                             [0070:9290]
+ 81 -> Leadtek WinFast DTV1800 Hybrid                      [107d:6654]
linux/drivers/media/video/cx88/cx88-cards.c +75 -0

111 lines not on this page · raw message

linux/drivers/media/video/cx88/cx88-dvb.c +1 -0
diff -r 65ec132f20df linux/drivers/media/video/cx88/cx88-dvb.c
@@ -1021,6 +1021,7 @@
 		}
 		break;
 	 case CX88_BOARD_PINNACLE_HYBRID_PCTV:
+	case CX88_BOARD_WINFAST_DTV1800H:
 		fe0->dvb.frontend = dvb_attach(zl10353_attach,
 					       &cx88_pinnacle_hybrid_pctv,
 					       &core->i2c_adap);
linux/drivers/media/video/cx88/cx88-input.c +2 -0
diff -r 65ec132f20df linux/drivers/media/video/cx88/cx88-input.c
@@ -92,6 +92,7 @@
 		gpio=(gpio & 0x7fd) + (auxgpio & 0xef);
 		break;
 	case CX88_BOARD_WINFAST_DTV1000:
+	case CX88_BOARD_WINFAST_DTV1800H:
 	case CX88_BOARD_WINFAST_TV2000_XP_GLOBAL:
 		gpio = (gpio & 0x6ff) | ((cx_read(MO_GP1_IO) << 8) & 0x900);
 		auxgpio = gpio;
@@ -237,6 +238,7 @@
 		ir->sampling = 1;
 		break;
 	case CX88_BOARD_WINFAST_DTV2000H:
+	case CX88_BOARD_WINFAST_DTV1800H:
 		ir_codes = ir_codes_winfast;
 		ir->gpio_addr = MO_GP0_IO;
 		ir->mask_keycode = 0x8f8;
linux/drivers/media/video/cx88/cx88.h +1 -0
diff -r 65ec132f20df linux/drivers/media/video/cx88/cx88.h
@@ -237,6 +237,7 @@
 #define CX88_BOARD_PROF_6200               78
 #define CX88_BOARD_TERRATEC_CINERGY_HT_PCI_MKII 79
 #define CX88_BOARD_HAUPPAUGE_IRONLY        80
+#define CX88_BOARD_WINFAST_DTV1800H        81
 
 enum cx88_itype {
 	CX88_VMUX_COMPOSITE1 = 1,
#2 lore raw
Quoted 45 lines · On Fri, 29 May 2009, Miroslav [UTF-8] Šustek wrote:

Hello,
this patch adds support for Leadtek WinFast DTV-1800H hybrid card. It enables analog/digital tv, radio and remote control trough GPIO.

Input GPIO values are extracted from INF file which is included in winxp driver. Analog audio works both through cx88-alsa and through internal cable from tv-card to sound card.

Tested by me and the people listed in patch (works well).

linux/drivers/media/video/cx88/cx88-cards.c +36 -0
+                *  2: mute (0=off,1=on)
+                * 12: tuner reset pin
+                * 13: audio source (0=tuner audio,1=line in)
+                * 14: FM (0=on,1=off ???)
+                */
+               .input          = {{
+                       .type   = CX88_VMUX_TELEVISION,
+                       .vmux   = 0,
+                       .gpio0  = 0x0400,       /* pin 2 = 0 */
+                       .gpio1  = 0x6040,       /* pin 13 = 0, pin 14 = 1 */
+                       .gpio2  = 0x0000,
+               }, {
+                       .type   = CX88_VMUX_COMPOSITE1,
+                       .vmux   = 1,
+                       .gpio0  = 0x0400,       /* pin 2 = 0 */
+                       .gpio1  = 0x6060,       /* pin 13 = 1, pin 14 = 1 */
+                       .gpio2  = 0x0000,
+               }, {
+                       .type   = CX88_VMUX_SVIDEO,
+                       .vmux   = 2,
+                       .gpio0  = 0x0400,       /* pin 2 = 0 */
+                       .gpio1  = 0x6060,       /* pin 13 = 1, pin 14 = 1 */
+                       .gpio2  = 0x0000,
+               } },
+               .radio = {
+                       .type   = CX88_RADIO,
+                       .gpio0  = 0x0400,       /* pin 2 = 0 */
+                       .gpio1  = 0x6000,       /* pin 13 = 0, pin 14 = 0 */
+                       .gpio2  = 0x0000,
+               },
+static int cx88_xc3028_winfast1800h_callback(struct cx88_core *core,
+                                            int command, int arg)
+{
+       switch (command) {
+       case XC2028_TUNER_RESET:
+               /* GPIO 12 (xc3028 tuner reset) */

linux/drivers/media/video/cx88/cx88-cards.c

Quoted 10 lines
linux/drivers/media/video/cx88/cx88-cards.c +10 -0
+               cx_set(MO_GP1_IO, 0x1010);
+               mdelay(50);
+               cx_clear(MO_GP1_IO, 0x10);
+               mdelay(50);
+               cx_set(MO_GP1_IO, 0x10);
+               mdelay(50);
+               return 0;
+       }
+       return -EINVAL;
+}

Instead of raising the reset line here, why not change the gpio settings in the card definition to have it high? Change gpio1 for television to 0x7050 and radio to 0x7010.

Then the reset can be done with:

	case XC2028_TUNER_RESET:
		/* GPIO 12 (xc3028 tuner reset) */
		cx_write(MO_GP1_IO, 0x101000);
		mdelay(50);
		cx_write(MO_GP1_IO, 0x101010);
		mdelay(50);
		return 0;

Though I have to wonder why each card needs its own xc2028 reset function. Shouldn't they all be the same other than what gpio they change?

@@ -2882,6 +2946,16 @@

                cx_set(MO_GP0_IO, 0x00000080); /* 702 out of reset */
                udelay(1000);
                break;
+
+       case CX88_BOARD_WINFAST_DTV1800H:
+               /* GPIO 12 (xc3028 tuner reset) */
+               cx_set(MO_GP1_IO, 0x1010);
+               mdelay(50);
+               cx_clear(MO_GP1_IO, 0x10);
+               mdelay(50);
+               cx_set(MO_GP1_IO, 0x10);
+               mdelay(50);
+               break;
        }

}

Couldn't you replace this with:

	case CX88_BOARD_WINFAST_DTV1800H:
		cx88_xc3028_winfast1800h_callback(code, XC2028_TUNER_RESET, 0);
		break;
author Tested-by Marcin Wojcikowski Tested-by Karel Juhanak Tested-by Andrew Goff Tested-by Jan Novak
#3 lore raw
Quoted 3 lines · Trent Piepho <xyzzy <at> speakeasy.org> writes:

Instead of raising the reset line here, why not change the gpio settings in the card definition to have it high? Change gpio1 for television to 0x7050 and radio to 0x7010.

Personally, I don't know when these .gpioX members are used (before firmware loads or after...).
But I assume that adding the high on reset pin shouldn't break anything, so we can do this.

And shouldn't we put tuner reset pin to 0 when in composite and s-video mode? These inputs don't use tuner or do they?
If I look in dmesg I can see that firmware is loaded into tuner even when these modes are used (I'm using MPlayer to select the input). And due to callbacks issued duting firmware loading, tuner is turned on (reset pin = 1) no matter if it was turned off by .gpioX setting.

And shouldn't we use the mask bits [24:16] of MO_GPX_IO
in .gpioX members too? I know only few GPIO pins and the other I don't know either what direction they should be. That means GPIO pins which I don't know are set as Hi-Z = inputs... Now, when I think of that, if it works it's safer when the other pins are in Hi-Z mode. Never mind.

Quoted 11 lines

Then the reset can be done with:

case XC2028_TUNER_RESET:
/* GPIO 12 (xc3028 tuner reset) */
cx_write(MO_GP1_IO, 0x101000);
mdelay(50);
cx_write(MO_GP1_IO, 0x101010);
mdelay(50);
return 0;

Earlier I was told to use 'cx_set' and 'cx_clear' because using 'cx_write' is risky.
see here: http://www.spinics.net/lists/linux-dvb/msg29777.html
And when you are using 'cx_set' and 'cx_clear' you need 3 calls. The first to set the direction bit, the second to set 0 on reset pin and the third to set 1 on reset pin.
If you ask me which I think is nicer I'll tell you: that one with 'cx_write'. If you ask me which one I want to use, I'll tell: I don't care. :)

Quoted 2 lines

Though I have to wonder why each card needs its own xc2028 reset function. Shouldn't they all be the same other than what gpio they change?

My English goes lame here. Do you mean that reset function shouldn't use GPIO at all?

Quoted 18 lines
(quoted diff) +0 -0
@@ -2882,6 +2946,16 @@

cx_set(MO_GP0_IO, 0x00000080); /* 702 out of reset */ udelay(1000);
break;
+
+ case CX88_BOARD_WINFAST_DTV1800H:
+ /* GPIO 12 (xc3028 tuner reset) */
+ cx_set(MO_GP1_IO, 0x1010);
+ mdelay(50);
+ cx_clear(MO_GP1_IO, 0x10);
+ mdelay(50);
+ cx_set(MO_GP1_IO, 0x10);
+ mdelay(50);
+ break;
}
}

Quoted 5 lines

Couldn't you replace this with:

case CX88_BOARD_WINFAST_DTV1800H:
cx88_xc3028_winfast1800h_callback(code, XC2028_TUNER_RESET, 0); break;

Yes, this will do the same job.
I think that 'cx88_card_setup_pre_i2c' is to be called before any I2C communication. The 'cx88_xc3028_winfast1800h_callback' (cx88_tuner_callback) is meant to be used when tuner code (during firmware loading) needs it. This is probably why others did it this way (these are separated issues even if they do the same thing) and I only obey existing form.

I only want to finally add the support for this card.
You know many people (not developers) don't care "if this function is used or that function is used twice, instead". They just want to install they distro and watch the tv.
I classify myself as an programmer rather than ordinary user, so I care how the code looks like. I'm open to the discussion about these things, but this can take long time and I just want the card to be supported asap. There are more cards which has code like this so if linuxtv developers realize eg. to not use callbacks or use only cx_set and cx_clear (instead of cx_write) they'll do it all at once (not every card separately).

I attached modified patch:
- .gpioX members of inputs which use tuner have reset pin 1 (tuner enabled)
- .gpioX members of inputs which don't use tuner have reset pin 0 (tuner disabled)
- resets (in callback and the one in pre_i2c) use only two 'cx_write' calls

I'm keeping the "tested-by" lines even if this modified version of patch wasn't tested by those people (the previous version was). I trust this changes can't break the functionality.
If you think it's too audacious then drop them.

It's on linuxtv developers which of these two patches will be chosen.

- Miroslav Šustek

Adds support for Leadtek WinFast DTV-1800H

From: Miroslav Sustek <[email protected]>

Enables analog/digital tv, radio and remote control (gpio).

linux/Documentation/video4linux/CARDLIST.cx88 +1 -0
diff -r 25bc0580359a linux/Documentation/video4linux/CARDLIST.cx88
@@ -79,3 +79,4 @@
  78 -> Prof 6200 DVB-S                                     [b022:3022]
  79 -> Terratec Cinergy HT PCI MKII                        [153b:1177]
  80 -> Hauppauge WinTV-IR Only                             [0070:9290]
+ 81 -> Leadtek WinFast DTV1800 Hybrid                      [107d:6654]
linux/drivers/media/video/cx88/cx88-cards.c +71 -0

107 lines not on this page · raw message

linux/drivers/media/video/cx88/cx88-dvb.c +1 -0
diff -r 25bc0580359a linux/drivers/media/video/cx88/cx88-dvb.c
@@ -1021,6 +1021,7 @@
 		}
 		break;
 	 case CX88_BOARD_PINNACLE_HYBRID_PCTV:
+	case CX88_BOARD_WINFAST_DTV1800H:
 		fe0->dvb.frontend = dvb_attach(zl10353_attach,
 					       &cx88_pinnacle_hybrid_pctv,
 					       &core->i2c_adap);
linux/drivers/media/video/cx88/cx88-input.c +2 -0
diff -r 25bc0580359a linux/drivers/media/video/cx88/cx88-input.c
@@ -92,6 +92,7 @@
 		gpio=(gpio & 0x7fd) + (auxgpio & 0xef);
 		break;
 	case CX88_BOARD_WINFAST_DTV1000:
+	case CX88_BOARD_WINFAST_DTV1800H:
 	case CX88_BOARD_WINFAST_TV2000_XP_GLOBAL:
 		gpio = (gpio & 0x6ff) | ((cx_read(MO_GP1_IO) << 8) & 0x900);
 		auxgpio = gpio;
@@ -237,6 +238,7 @@
 		ir->sampling = 1;
 		break;
 	case CX88_BOARD_WINFAST_DTV2000H:
+	case CX88_BOARD_WINFAST_DTV1800H:
 		ir_codes = ir_codes_winfast;
 		ir->gpio_addr = MO_GP0_IO;
 		ir->mask_keycode = 0x8f8;
linux/drivers/media/video/cx88/cx88.h +1 -0
diff -r 25bc0580359a linux/drivers/media/video/cx88/cx88.h
@@ -237,6 +237,7 @@
 #define CX88_BOARD_PROF_6200               78
 #define CX88_BOARD_TERRATEC_CINERGY_HT_PCI_MKII 79
 #define CX88_BOARD_HAUPPAUGE_IRONLY        80
+#define CX88_BOARD_WINFAST_DTV1800H        81
 
 enum cx88_itype {
 	CX88_VMUX_COMPOSITE1 = 1,
#4 lore raw

Em Sun, 31 May 2009 19:39:15 +0200
"Miroslav Šustek" <[email protected]> escreveu:

Quoted 94 lines
Quoted 3 lines · Trent Piepho <xyzzy <at> speakeasy.org> writes:

Instead of raising the reset line here, why not change the gpio settings in the card definition to have it high? Change gpio1 for television to 0x7050 and radio to 0x7010.

Personally, I don't know when these .gpioX members are used (before firmware loads or after...).
But I assume that adding the high on reset pin shouldn't break anything, so we can do this.

And shouldn't we put tuner reset pin to 0 when in composite and s-video mode? These inputs don't use tuner or do they?
If I look in dmesg I can see that firmware is loaded into tuner even when these modes are used (I'm using MPlayer to select the input). And due to callbacks issued duting firmware loading, tuner is turned on (reset pin = 1) no matter if it was turned off by .gpioX setting.

And shouldn't we use the mask bits [24:16] of MO_GPX_IO
in .gpioX members too? I know only few GPIO pins and the other I don't know either what direction they should be. That means GPIO pins which I don't know are set as Hi-Z = inputs... Now, when I think of that, if it works it's safer when the other pins are in Hi-Z mode. Never mind.

Quoted 11 lines

Then the reset can be done with:

case XC2028_TUNER_RESET:
/* GPIO 12 (xc3028 tuner reset) */
cx_write(MO_GP1_IO, 0x101000);
mdelay(50);
cx_write(MO_GP1_IO, 0x101010);
mdelay(50);
return 0;

Earlier I was told to use 'cx_set' and 'cx_clear' because using 'cx_write' is risky.
see here: http://www.spinics.net/lists/linux-dvb/msg29777.html
And when you are using 'cx_set' and 'cx_clear' you need 3 calls. The first to set the direction bit, the second to set 0 on reset pin and the third to set 1 on reset pin.
If you ask me which I think is nicer I'll tell you: that one with 'cx_write'. If you ask me which one I want to use, I'll tell: I don't care. :)

Quoted 2 lines

Though I have to wonder why each card needs its own xc2028 reset function. Shouldn't they all be the same other than what gpio they change?

My English goes lame here. Do you mean that reset function shouldn't use GPIO at all?

Quoted 18 lines
(quoted diff) +0 -0
@@ -2882,6 +2946,16 @@

cx_set(MO_GP0_IO, 0x00000080); /* 702 out of reset */ udelay(1000);
break;
+
+ case CX88_BOARD_WINFAST_DTV1800H:
+ /* GPIO 12 (xc3028 tuner reset) */
+ cx_set(MO_GP1_IO, 0x1010);
+ mdelay(50);
+ cx_clear(MO_GP1_IO, 0x10);
+ mdelay(50);
+ cx_set(MO_GP1_IO, 0x10);
+ mdelay(50);
+ break;
}
}

Quoted 5 lines

Couldn't you replace this with:

case CX88_BOARD_WINFAST_DTV1800H:
cx88_xc3028_winfast1800h_callback(code, XC2028_TUNER_RESET, 0); break;

Yes, this will do the same job.
I think that 'cx88_card_setup_pre_i2c' is to be called before any I2C communication. The 'cx88_xc3028_winfast1800h_callback' (cx88_tuner_callback) is meant to be used when tuner code (during firmware loading) needs it. This is probably why others did it this way (these are separated issues even if they do the same thing) and I only obey existing form.

I only want to finally add the support for this card.
You know many people (not developers) don't care "if this function is used or that function is used twice, instead". They just want to install they distro and watch the tv.
I classify myself as an programmer rather than ordinary user, so I care how the code looks like. I'm open to the discussion about these things, but this can take long time and I just want the card to be supported asap. There are more cards which has code like this so if linuxtv developers realize eg. to not use callbacks or use only cx_set and cx_clear (instead of cx_write) they'll do it all at once (not every card separately).

I attached modified patch:
- .gpioX members of inputs which use tuner have reset pin 1 (tuner enabled)
- .gpioX members of inputs which don't use tuner have reset pin 0 (tuner disabled)
- resets (in callback and the one in pre_i2c) use only two 'cx_write' calls

Quoted 6 lines

I'm keeping the "tested-by" lines even if this modified version of patch wasn't tested by those people (the previous version was). I trust this changes can't break the functionality.
If you think it's too audacious then drop them.

It's on linuxtv developers which of these two patches will be chosen.

For the sake of not loosing this patch again, I've applied it as-is. I hope I got the latest version. It is hard to track patches that aren't got by patchwork.

I agree with Trent's proposals for optimizing the code: It is better to just call xc3028_winfast1800h_callback() if you ever need to reset xc3028 before the tuner driver. I suspect, however, that you don't need to do such reset, since the tuner driver will do it during its initialization, especially if you've properly initialized the gpio lines.

Cheers,
Mauro

#5 lore raw

Am Sonntag, den 31.05.2009, 16:58 -0300 schrieb Mauro Carvalho Chehab:

Quoted 104 lines

Em Sun, 31 May 2009 19:39:15 +0200
"Miroslav Šustek" <[email protected]> escreveu:

Quoted 94 lines
Quoted 3 lines · Trent Piepho <xyzzy <at> speakeasy.org> writes:

Instead of raising the reset line here, why not change the gpio settings in the card definition to have it high? Change gpio1 for television to 0x7050 and radio to 0x7010.

Personally, I don't know when these .gpioX members are used (before firmware loads or after...).
But I assume that adding the high on reset pin shouldn't break anything, so we can do this.

And shouldn't we put tuner reset pin to 0 when in composite and s-video mode? These inputs don't use tuner or do they?
If I look in dmesg I can see that firmware is loaded into tuner even when these modes are used (I'm using MPlayer to select the input). And due to callbacks issued duting firmware loading, tuner is turned on (reset pin = 1) no matter if it was turned off by .gpioX setting.

And shouldn't we use the mask bits [24:16] of MO_GPX_IO
in .gpioX members too? I know only few GPIO pins and the other I don't know either what direction they should be. That means GPIO pins which I don't know are set as Hi-Z = inputs... Now, when I think of that, if it works it's safer when the other pins are in Hi-Z mode. Never mind.

Quoted 11 lines

Then the reset can be done with:

case XC2028_TUNER_RESET:
/* GPIO 12 (xc3028 tuner reset) */
cx_write(MO_GP1_IO, 0x101000);
mdelay(50);
cx_write(MO_GP1_IO, 0x101010);
mdelay(50);
return 0;

Earlier I was told to use 'cx_set' and 'cx_clear' because using 'cx_write' is risky.
see here: http://www.spinics.net/lists/linux-dvb/msg29777.html
And when you are using 'cx_set' and 'cx_clear' you need 3 calls. The first to set the direction bit, the second to set 0 on reset pin and the third to set 1 on reset pin.
If you ask me which I think is nicer I'll tell you: that one with 'cx_write'. If you ask me which one I want to use, I'll tell: I don't care. :)

Quoted 2 lines

Though I have to wonder why each card needs its own xc2028 reset function. Shouldn't they all be the same other than what gpio they change?

My English goes lame here. Do you mean that reset function shouldn't use GPIO at all?

Quoted 18 lines
(quoted diff) +0 -0
@@ -2882,6 +2946,16 @@

cx_set(MO_GP0_IO, 0x00000080); /* 702 out of reset */ udelay(1000);
break;
+
+ case CX88_BOARD_WINFAST_DTV1800H:
+ /* GPIO 12 (xc3028 tuner reset) */
+ cx_set(MO_GP1_IO, 0x1010);
+ mdelay(50);
+ cx_clear(MO_GP1_IO, 0x10);
+ mdelay(50);
+ cx_set(MO_GP1_IO, 0x10);
+ mdelay(50);
+ break;
}
}

Quoted 5 lines

Couldn't you replace this with:

case CX88_BOARD_WINFAST_DTV1800H:
cx88_xc3028_winfast1800h_callback(code, XC2028_TUNER_RESET, 0); break;

Yes, this will do the same job.
I think that 'cx88_card_setup_pre_i2c' is to be called before any I2C communication. The 'cx88_xc3028_winfast1800h_callback' (cx88_tuner_callback) is meant to be used when tuner code (during firmware loading) needs it. This is probably why others did it this way (these are separated issues even if they do the same thing) and I only obey existing form.

I only want to finally add the support for this card.
You know many people (not developers) don't care "if this function is used or that function is used twice, instead". They just want to install they distro and watch the tv.
I classify myself as an programmer rather than ordinary user, so I care how the code looks like. I'm open to the discussion about these things, but this can take long time and I just want the card to be supported asap. There are more cards which has code like this so if linuxtv developers realize eg. to not use callbacks or use only cx_set and cx_clear (instead of cx_write) they'll do it all at once (not every card separately).

I attached modified patch:
- .gpioX members of inputs which use tuner have reset pin 1 (tuner enabled)
- .gpioX members of inputs which don't use tuner have reset pin 0 (tuner disabled)
- resets (in callback and the one in pre_i2c) use only two 'cx_write' calls

Quoted 6 lines

I'm keeping the "tested-by" lines even if this modified version of patch wasn't tested by those people (the previous version was). I trust this changes can't break the functionality.
If you think it's too audacious then drop them.

It's on linuxtv developers which of these two patches will be chosen.

Quoted 9 lines

For the sake of not loosing this patch again, I've applied it as-is. I hope I got the latest version. It is hard to track patches that aren't got by patchwork.

I agree with Trent's proposals for optimizing the code: It is better to just call xc3028_winfast1800h_callback() if you ever need to reset xc3028 before the tuner driver. I suspect, however, that you don't need to do such reset, since the tuner driver will do it during its initialization, especially if you've properly initialized the gpio lines.

Mauro,

it is also hard to track down whatever happens on your patchwork stuff.

I think we had enough cases for now and I know about more.

The absolute minimum is, that people see a reject message, if failing on submitting patches.

Do you deny that?

To be honest, I'm slowly getting it sick.

Cheers,
Hermann

#6 lore raw

Em Mon, 01 Jun 2009 03:58:25 +0200
hermann pitton <[email protected]> escreveu:

Quoted 120 lines

Am Sonntag, den 31.05.2009, 16:58 -0300 schrieb Mauro Carvalho Chehab:

Quoted 104 lines

Em Sun, 31 May 2009 19:39:15 +0200
"Miroslav Šustek" <[email protected]> escreveu:

Quoted 94 lines
Quoted 3 lines · Trent Piepho <xyzzy <at> speakeasy.org> writes:

Instead of raising the reset line here, why not change the gpio settings in the card definition to have it high? Change gpio1 for television to 0x7050 and radio to 0x7010.

Personally, I don't know when these .gpioX members are used (before firmware loads or after...).
But I assume that adding the high on reset pin shouldn't break anything, so we can do this.

And shouldn't we put tuner reset pin to 0 when in composite and s-video mode? These inputs don't use tuner or do they?
If I look in dmesg I can see that firmware is loaded into tuner even when these modes are used (I'm using MPlayer to select the input). And due to callbacks issued duting firmware loading, tuner is turned on (reset pin = 1) no matter if it was turned off by .gpioX setting.

And shouldn't we use the mask bits [24:16] of MO_GPX_IO
in .gpioX members too? I know only few GPIO pins and the other I don't know either what direction they should be. That means GPIO pins which I don't know are set as Hi-Z = inputs... Now, when I think of that, if it works it's safer when the other pins are in Hi-Z mode. Never mind.

Quoted 11 lines

Then the reset can be done with:

case XC2028_TUNER_RESET:
/* GPIO 12 (xc3028 tuner reset) */
cx_write(MO_GP1_IO, 0x101000);
mdelay(50);
cx_write(MO_GP1_IO, 0x101010);
mdelay(50);
return 0;

Earlier I was told to use 'cx_set' and 'cx_clear' because using 'cx_write' is risky.
see here: http://www.spinics.net/lists/linux-dvb/msg29777.html
And when you are using 'cx_set' and 'cx_clear' you need 3 calls. The first to set the direction bit, the second to set 0 on reset pin and the third to set 1 on reset pin.
If you ask me which I think is nicer I'll tell you: that one with 'cx_write'. If you ask me which one I want to use, I'll tell: I don't care. :)

Quoted 2 lines

Though I have to wonder why each card needs its own xc2028 reset function. Shouldn't they all be the same other than what gpio they change?

My English goes lame here. Do you mean that reset function shouldn't use GPIO at all?

Quoted 18 lines
(quoted diff) +0 -0
@@ -2882,6 +2946,16 @@

cx_set(MO_GP0_IO, 0x00000080); /* 702 out of reset */ udelay(1000);
break;
+
+ case CX88_BOARD_WINFAST_DTV1800H:
+ /* GPIO 12 (xc3028 tuner reset) */
+ cx_set(MO_GP1_IO, 0x1010);
+ mdelay(50);
+ cx_clear(MO_GP1_IO, 0x10);
+ mdelay(50);
+ cx_set(MO_GP1_IO, 0x10);
+ mdelay(50);
+ break;
}
}

Quoted 5 lines

Couldn't you replace this with:

case CX88_BOARD_WINFAST_DTV1800H:
cx88_xc3028_winfast1800h_callback(code, XC2028_TUNER_RESET, 0); break;

Yes, this will do the same job.
I think that 'cx88_card_setup_pre_i2c' is to be called before any I2C communication. The 'cx88_xc3028_winfast1800h_callback' (cx88_tuner_callback) is meant to be used when tuner code (during firmware loading) needs it. This is probably why others did it this way (these are separated issues even if they do the same thing) and I only obey existing form.

I only want to finally add the support for this card.
You know many people (not developers) don't care "if this function is used or that function is used twice, instead". They just want to install they distro and watch the tv.
I classify myself as an programmer rather than ordinary user, so I care how the code looks like. I'm open to the discussion about these things, but this can take long time and I just want the card to be supported asap. There are more cards which has code like this so if linuxtv developers realize eg. to not use callbacks or use only cx_set and cx_clear (instead of cx_write) they'll do it all at once (not every card separately).

I attached modified patch:
- .gpioX members of inputs which use tuner have reset pin 1 (tuner enabled)
- .gpioX members of inputs which don't use tuner have reset pin 0 (tuner disabled)
- resets (in callback and the one in pre_i2c) use only two 'cx_write' calls

Quoted 6 lines

I'm keeping the "tested-by" lines even if this modified version of patch wasn't tested by those people (the previous version was). I trust this changes can't break the functionality.
If you think it's too audacious then drop them.

It's on linuxtv developers which of these two patches will be chosen.

Quoted 9 lines

For the sake of not loosing this patch again, I've applied it as-is. I hope I got the latest version. It is hard to track patches that aren't got by patchwork.

I agree with Trent's proposals for optimizing the code: It is better to just call xc3028_winfast1800h_callback() if you ever need to reset xc3028 before the tuner driver. I suspect, however, that you don't need to do such reset, since the tuner driver will do it during its initialization, especially if you've properly initialized the gpio lines.

Mauro,

it is also hard to track down whatever happens on your patchwork stuff.

Quoted 8 lines

I think we had enough cases for now and I know about more.

The absolute minimum is, that people see a reject message, if failing on submitting patches.

Do you deny that?

To be honest, I'm slowly getting it sick.

Hermann,

If you have suggestions to improve patchwork, please forward it to the right list:
https://ozlabs.org/mailman/listinfo/patchwork

I'm not the maintainer of it, so all I can do is to propose some code just like any other.

I'm not sure what you're meaning by "reject message". If what you want is that patchwork would generate a reject message if it can't found a patch on an email, this won't work, since it will send a message to every message that doesn't contain a patch.

Although I've already proposed a few patches to patchwork, I don't have deep knowledge about its internals. As far as I understand, patchwork has a logic to detect if an email has a patch. If it doesn't detect a patch, it do nothing.

Probably, the logic it uses is similar to the logic at diffstat. So, if a patch got mangled by line-wrap, the logic won't be able to detect it as a patch.

Patchwork works fine if the patch is sent via the official, recommended way:
send a patch in-lined.

Send a patch enclosed as an attachment is evil, since it is harder for developers to comment about the patch, especially if they are using some text-mode emailer like mutt and pine (those are very frequent among kernel hackers).

Yet, it tries to do its best to analyze an attachment and checks for a patch. In order to do that, it analyzes the applicable mime types for a text that can contain a patch:
text/x-patch
text/x-diff

It also provides a fallback method of analyzing pure text attachments (text/plain) for those email software that are broken enough to not recognize a patch.

It makes no sense to analyze any other mime type since what would be the sense of having a patch inside a gif picture, an openoffice doc or a binary application?

So, it properly discards any other type of documents.

What happened with Miroslav's email is that the email software he is using is so broken that it can't even recognize a patch as a text! It thinks that the patch is a binary application (application/octet-stream), as pointed by CityK, and even encodes it with base64.

This is what you see if you open the source code of his email:

...
X-Mailer: Centrum Mail 5.0
...
Content-Type: application/octet-stream; name="leadtek_winfast_dtv1800h.patch"
Content-Transfer-Encoding: base64

QWRkcyBzdXBwb3J0IGZvciBMZWFkdGVrIFdpbkZhc3QgRFRWLTE4MDBICgpGcm9tOiBNaXJv c2xhdiBTdXN0ZWsgPHN1c3RtaWRvd25AY2VudHJ1bS5jej4KCkVuYWJsZXMgYW5hbG9nL2Rp
...

The proper approach here is to complain with the email software provider (Centrum Mail?) for they to fix their software to not mark a patch as if it where an application, and to send patches as inlined.

That's said, the default behavior configured on several listservers and on list mirrors is to deny any email with application/octet-stream mime type, to avoid that someone could use a list or a web server as a place to store trojan horses.

So, nobody should use an emailer that encodes an attachment as application/octet-stream for sending such emails to a public ML.

Cheers,
Mauro

#7 lore raw

Am Montag, den 01.06.2009, 06:07 -0300 schrieb Mauro Carvalho Chehab:

Quoted 197 lines

Em Mon, 01 Jun 2009 03:58:25 +0200
hermann pitton <[email protected]> escreveu:

Quoted 120 lines

Am Sonntag, den 31.05.2009, 16:58 -0300 schrieb Mauro Carvalho Chehab:

Quoted 104 lines

Em Sun, 31 May 2009 19:39:15 +0200
"Miroslav Šustek" <[email protected]> escreveu:

Quoted 94 lines
Quoted 3 lines · Trent Piepho <xyzzy <at> speakeasy.org> writes:

Instead of raising the reset line here, why not change the gpio settings in the card definition to have it high? Change gpio1 for television to 0x7050 and radio to 0x7010.

Personally, I don't know when these .gpioX members are used (before firmware loads or after...).
But I assume that adding the high on reset pin shouldn't break anything, so we can do this.

And shouldn't we put tuner reset pin to 0 when in composite and s-video mode? These inputs don't use tuner or do they?
If I look in dmesg I can see that firmware is loaded into tuner even when these modes are used (I'm using MPlayer to select the input). And due to callbacks issued duting firmware loading, tuner is turned on (reset pin = 1) no matter if it was turned off by .gpioX setting.

And shouldn't we use the mask bits [24:16] of MO_GPX_IO
in .gpioX members too? I know only few GPIO pins and the other I don't know either what direction they should be. That means GPIO pins which I don't know are set as Hi-Z = inputs... Now, when I think of that, if it works it's safer when the other pins are in Hi-Z mode. Never mind.

Quoted 11 lines

Then the reset can be done with:

case XC2028_TUNER_RESET:
/* GPIO 12 (xc3028 tuner reset) */
cx_write(MO_GP1_IO, 0x101000);
mdelay(50);
cx_write(MO_GP1_IO, 0x101010);
mdelay(50);
return 0;

Earlier I was told to use 'cx_set' and 'cx_clear' because using 'cx_write' is risky.
see here: http://www.spinics.net/lists/linux-dvb/msg29777.html
And when you are using 'cx_set' and 'cx_clear' you need 3 calls. The first to set the direction bit, the second to set 0 on reset pin and the third to set 1 on reset pin.
If you ask me which I think is nicer I'll tell you: that one with 'cx_write'. If you ask me which one I want to use, I'll tell: I don't care. :)

Quoted 2 lines

Though I have to wonder why each card needs its own xc2028 reset function. Shouldn't they all be the same other than what gpio they change?

My English goes lame here. Do you mean that reset function shouldn't use GPIO at all?

Quoted 18 lines
(quoted diff) +0 -0
@@ -2882,6 +2946,16 @@

cx_set(MO_GP0_IO, 0x00000080); /* 702 out of reset */ udelay(1000);
break;
+
+ case CX88_BOARD_WINFAST_DTV1800H:
+ /* GPIO 12 (xc3028 tuner reset) */
+ cx_set(MO_GP1_IO, 0x1010);
+ mdelay(50);
+ cx_clear(MO_GP1_IO, 0x10);
+ mdelay(50);
+ cx_set(MO_GP1_IO, 0x10);
+ mdelay(50);
+ break;
}
}

Quoted 5 lines

Couldn't you replace this with:

case CX88_BOARD_WINFAST_DTV1800H:
cx88_xc3028_winfast1800h_callback(code, XC2028_TUNER_RESET, 0); break;

Yes, this will do the same job.
I think that 'cx88_card_setup_pre_i2c' is to be called before any I2C communication. The 'cx88_xc3028_winfast1800h_callback' (cx88_tuner_callback) is meant to be used when tuner code (during firmware loading) needs it. This is probably why others did it this way (these are separated issues even if they do the same thing) and I only obey existing form.

I only want to finally add the support for this card.
You know many people (not developers) don't care "if this function is used or that function is used twice, instead". They just want to install they distro and watch the tv.
I classify myself as an programmer rather than ordinary user, so I care how the code looks like. I'm open to the discussion about these things, but this can take long time and I just want the card to be supported asap. There are more cards which has code like this so if linuxtv developers realize eg. to not use callbacks or use only cx_set and cx_clear (instead of cx_write) they'll do it all at once (not every card separately).

I attached modified patch:
- .gpioX members of inputs which use tuner have reset pin 1 (tuner enabled)
- .gpioX members of inputs which don't use tuner have reset pin 0 (tuner disabled)
- resets (in callback and the one in pre_i2c) use only two 'cx_write' calls

Quoted 6 lines

I'm keeping the "tested-by" lines even if this modified version of patch wasn't tested by those people (the previous version was). I trust this changes can't break the functionality.
If you think it's too audacious then drop them.

It's on linuxtv developers which of these two patches will be chosen.

Quoted 9 lines

For the sake of not loosing this patch again, I've applied it as-is. I hope I got the latest version. It is hard to track patches that aren't got by patchwork.

I agree with Trent's proposals for optimizing the code: It is better to just call xc3028_winfast1800h_callback() if you ever need to reset xc3028 before the tuner driver. I suspect, however, that you don't need to do such reset, since the tuner driver will do it during its initialization, especially if you've properly initialized the gpio lines.

Mauro,

it is also hard to track down whatever happens on your patchwork stuff.

Quoted 8 lines

I think we had enough cases for now and I know about more.

The absolute minimum is, that people see a reject message, if failing on submitting patches.

Do you deny that?

To be honest, I'm slowly getting it sick.

Hermann,

If you have suggestions to improve patchwork, please forward it to the right list:
https://ozlabs.org/mailman/listinfo/patchwork

I'm not the maintainer of it, so all I can do is to propose some code just like any other.

I'm not sure what you're meaning by "reject message". If what you want is that patchwork would generate a reject message if it can't found a patch on an email, this won't work, since it will send a message to every message that doesn't contain a patch.

Although I've already proposed a few patches to patchwork, I don't have deep knowledge about its internals. As far as I understand, patchwork has a logic to detect if an email has a patch. If it doesn't detect a patch, it do nothing.

Probably, the logic it uses is similar to the logic at diffstat. So, if a patch got mangled by line-wrap, the logic won't be able to detect it as a patch.

Patchwork works fine if the patch is sent via the official, recommended way:
send a patch in-lined.

Send a patch enclosed as an attachment is evil, since it is harder for developers to comment about the patch, especially if they are using some text-mode emailer like mutt and pine (those are very frequent among kernel hackers).

Yet, it tries to do its best to analyze an attachment and checks for a patch. In order to do that, it analyzes the applicable mime types for a text that can contain a patch:
text/x-patch
text/x-diff

It also provides a fallback method of analyzing pure text attachments (text/plain) for those email software that are broken enough to not recognize a patch.

It makes no sense to analyze any other mime type since what would be the sense of having a patch inside a gif picture, an openoffice doc or a binary application?

So, it properly discards any other type of documents.

What happened with Miroslav's email is that the email software he is using is so broken that it can't even recognize a patch as a text! It thinks that the patch is a binary application (application/octet-stream), as pointed by CityK, and even encodes it with base64.

This is what you see if you open the source code of his email:

...
X-Mailer: Centrum Mail 5.0
...
Content-Type: application/octet-stream; name="leadtek_winfast_dtv1800h.patch"
Content-Transfer-Encoding: base64

QWRkcyBzdXBwb3J0IGZvciBMZWFkdGVrIFdpbkZhc3QgRFRWLTE4MDBICgpGcm9tOiBNaXJv c2xhdiBTdXN0ZWsgPHN1c3RtaWRvd25AY2VudHJ1bS5jej4KCkVuYWJsZXMgYW5hbG9nL2Rp
...

The proper approach here is to complain with the email software provider (Centrum Mail?) for they to fix their software to not mark a patch as if it where an application, and to send patches as inlined.

Quoted 6 lines

That's said, the default behavior configured on several listservers and on list mirrors is to deny any email with application/octet-stream mime type, to avoid that someone could use a list or a web server as a place to store trojan horses.

So, nobody should use an emailer that encodes an attachment as application/octet-stream for sending such emails to a public ML.

Hi Mauro,

that is all agreed.

But that [PATCH] in subject all lost or with unclear status as a minimum have.

Is that not enough to provide the sender and the list with some message, that his patch was not recognized and he should try again?

I would not complain, if there would be anything in sight improving the situation, but in README.patches the word "patchwork" does not even exist until now.

Especially there needs to be a hint, that patches already discussed and with a tested-by on the list are treated like they did never exist, if they do not make it into "patchwork".

Also for chapter 7 about patches from the community, BTW 8 and 7 are swapped, that "make checkpatch" thing should be mentioned. You will complain anyway later if stuff is not conform to it.

And SOBs already given at any list, I guess this includes LKML, are worthless if not on the version that makes it into "patchwork"?

I get by far too much posts off lists from people just trying to add some lines for new hardware, and I already had the fear Miroslav gave up after all and we start losing patches. He did not try off list and is wondering for months now.

My compliments go to CityK. I can see he followed everything carefully and tries to give best possible instructions on the wiki. At least something to point to now.

That patchwork transition was really a bumpy and unpaved road for close to half a year now.

Cheers,
Hermann

#8 lore raw
In reply to Trent Piepho

Trent Piepho píše v Ne 31. 05. 2009 v 06:34 -0700:

Quoted 3 lines

Instead of raising the reset line here, why not change the gpio settings in the card definition to have it high? Change gpio1 for television to 0x7050 and radio to 0x7010.

Personally, I don't know when these .gpioX members are used (before firmware loads or after...). But I assume that adding the high on reset pin shouldn't break anything, so we can do this.

Quoted 37 lines

Then the reset can be done with:

	case XC2028_TUNER_RESET:
		/* GPIO 12 (xc3028 tuner reset) */
		cx_write(MO_GP1_IO, 0x101000);
		mdelay(50);
		cx_write(MO_GP1_IO, 0x101010);
		mdelay(50);
		return 0;

Though I have to wonder why each card needs its own xc2028 reset function. Shouldn't they all be the same other than what gpio they change?

linux/drivers/media/video/cx88/cx88-cards.c +10 -0
@@ -2882,6 +2946,16 @@
                cx_set(MO_GP0_IO, 0x00000080); /* 702 out of reset */
                udelay(1000);
                break;
+
+       case CX88_BOARD_WINFAST_DTV1800H:
+               /* GPIO 12 (xc3028 tuner reset) */
+               cx_set(MO_GP1_IO, 0x1010);
+               mdelay(50);
+               cx_clear(MO_GP1_IO, 0x10);
+               mdelay(50);
+               cx_set(MO_GP1_IO, 0x10);
+               mdelay(50);
+               break;
        }
 }
 

Couldn't you replace this with:

	case CX88_BOARD_WINFAST_DTV1800H:
		cx88_xc3028_winfast1800h_callback(code, XC2028_TUNER_RESET, 0);
		break;