[PATCH] Leadtek WinFast DTV-1800H support

[PATCH] Leadtek WinFast DTV-1800H support

[Miroslav Šustek]

[-- Attachment #1: Type: text/plain, Size: 393 bytes --]

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


[-- Attachment #2: leadtek_winfast_dtv1800h.patch --]
[-- Type: application/octet-stream, Size: 5813 bytes --]

Adds support for Leadtek WinFast DTV-1800H

From: Miroslav Sustek <sustmidown@centrum.cz>

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

Signed-off-by: Miroslav Sustek <sustmidown@centrum.cz>
Tested-by: Marcin Wojcikowski <emtees.mts@gmail.com>
Tested-by: Karel Juhanak <karel.juhanak@warnet.cz>
Tested-by: Andrew Goff <goffa72@gmail.com>
Tested-by: Jan Novak <novak-j@seznam.cz>

diff -r ee3b79edde3f linux/Documentation/video4linux/CARDLIST.cx88
--- a/linux/Documentation/video4linux/CARDLIST.cx88	Sat May 09 21:45:37 2009 +0200
+++ b/linux/Documentation/video4linux/CARDLIST.cx88	Sun May 10 22:45:51 2009 +0200
@@ -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]
diff -r ee3b79edde3f linux/drivers/media/video/cx88/cx88-cards.c
--- a/linux/drivers/media/video/cx88/cx88-cards.c	Sat May 09 21:45:37 2009 +0200
+++ b/linux/drivers/media/video/cx88/cx88-cards.c	Sun May 10 22:45:51 2009 +0200
@@ -2009,6 +2009,47 @@
 		.tuner_addr	= ADDR_UNSET,
 		.radio_addr	= ADDR_UNSET,
 	},
+	[CX88_BOARD_WINFAST_DTV1800H] = {
+		.name           = "Leadtek WinFast DTV1800 Hybrid",
+		.tuner_type     = TUNER_XC2028,
+		.radio_type     = TUNER_XC2028,
+		.tuner_addr     = 0x61,
+		.radio_addr     = 0x61,
+		/*
+		 * GPIO setting
+		 *
+		 *  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,
+		},
+		.mpeg           = CX88_MPEG_DVB,
+	},
 };
 
 /* ------------------------------------------------------------------ */
@@ -2426,6 +2467,10 @@
 		.subvendor = 0x0070,
 		.subdevice = 0x9290,
 		.card      = CX88_BOARD_HAUPPAUGE_IRONLY,
+	}, {
+		.subvendor = 0x107d,
+		.subdevice = 0x6654,
+		.card      = CX88_BOARD_WINFAST_DTV1800H,
 	},
 };
 
@@ -2624,6 +2669,23 @@
 	return -EINVAL;
 }
 
+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) */
+		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;
+}
+
 /* ------------------------------------------------------------------- */
 /* some Divco specific stuff                                           */
 static int cx88_pv_8000gt_callback(struct cx88_core *core,
@@ -2696,6 +2758,8 @@
 	case CX88_BOARD_DVICO_FUSIONHDTV_DVB_T_PRO:
 	case CX88_BOARD_DVICO_FUSIONHDTV_5_PCI_NANO:
 		return cx88_dvico_xc2028_callback(core, command, arg);
+	case CX88_BOARD_WINFAST_DTV1800H:
+		return cx88_xc3028_winfast1800h_callback(core, command, arg);
 	}
 
 	switch (command) {
@@ -2870,6 +2934,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;
 	}
 }
 
@@ -2890,6 +2964,7 @@
 			core->i2c_algo.udelay = 16;
 		break;
 	case CX88_BOARD_DVICO_FUSIONHDTV_DVB_T_PRO:
+	case CX88_BOARD_WINFAST_DTV1800H:
 		ctl->demod = XC3028_FE_ZARLINK456;
 		break;
 	case CX88_BOARD_KWORLD_ATSC_120:
diff -r ee3b79edde3f linux/drivers/media/video/cx88/cx88-dvb.c
--- a/linux/drivers/media/video/cx88/cx88-dvb.c	Sat May 09 21:45:37 2009 +0200
+++ b/linux/drivers/media/video/cx88/cx88-dvb.c	Sun May 10 22:45:51 2009 +0200
@@ -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);
diff -r ee3b79edde3f linux/drivers/media/video/cx88/cx88-input.c
--- a/linux/drivers/media/video/cx88/cx88-input.c	Sat May 09 21:45:37 2009 +0200
+++ b/linux/drivers/media/video/cx88/cx88-input.c	Sun May 10 22:45:51 2009 +0200
@@ -92,6 +92,7 @@
 		gpio=(gpio & 0x7fd) + (auxgpio & 0xef);
 		break;
 	case CX88_BOARD_WINFAST_DTV1000:
+	case CX88_BOARD_WINFAST_DTV1800H:
 		gpio = (gpio & 0x6ff) | ((cx_read(MO_GP1_IO) << 8) & 0x900);
 		auxgpio = gpio;
 		break;
@@ -236,6 +237,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;
diff -r ee3b79edde3f linux/drivers/media/video/cx88/cx88.h
--- a/linux/drivers/media/video/cx88/cx88.h	Sat May 09 21:45:37 2009 +0200
+++ b/linux/drivers/media/video/cx88/cx88.h	Sun May 10 22:45:51 2009 +0200
@@ -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,

Re: [PATCH] Leadtek WinFast DTV-1800H support

[Miroslav Šustek]

Any problem with this patch?
I'm trying to get WinFast DTV-1800H support into repository for seven months.
(see:
http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/1125/match=1800h
)

Re: [PATCH] Leadtek WinFast DTV-1800H support

[hermann pitton]

Hi Miroslav,

Am Donnerstag, den 28.05.2009, 18:44 +0000 schrieb Miroslav Šustek:
> Any problem with this patch?
> I'm trying to get WinFast DTV-1800H support into repository for seven months.
> (see:
> http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/1125/match=1800h
> )
> 

just saw this patch on another machine and it reminded me also on your
patch to select different frontends on multiple multi frontends card at
insmod, which can be quite useful and was tested by me.

They must pass the checks "patchwork" does, this includes to run "make
checkpatch" on them previously and then you will see them very soon in

http://patchwork.kernel.org/project/linux-media/list

They seem not to be there, also not with filters "any" set, and you
should have seen an automated commit message, if they should have made
it into v4l-dvb.

Please resend and check short time later, if they are at patchwork
linux-media.

Some sort of reject message would of course be fine on such and make
life easier, but does not happen yet.

Cheers,
Hermann

[PATCH] Leadtek WinFast DTV-1800H support

[Miroslav Šustek]

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

(Sorry for double-post, but I was told to do it so. Nobody noticed the previous post(s).)

Re: [PATCH] Leadtek WinFast DTV-1800H support

[CityK]

Miroslav Šustek wrote:
> Any problem with this patch?
> I'm trying to get WinFast DTV-1800H support into repository for seven months.
> (see:
> http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/1125/match=1800h
>
>   

Hi Miro,

Its unfortunate that the patch hasn't been added yet, but I do see a
problem (in its current form) that explains why it hasn't been picked
up.   For the sake of thoroughness, here's an audit trail of the entire
history:

1) http://linuxtv.org/pipermail/linux-dvb/2008-October/029859.html
- Steve picked up some style flaws
- (Although it is more desirable to include patches inline as opposed to
as attachments, I note that the attached patch is of type
"text/x-patch", which is fine)

2) http://linuxtv.org/pipermail/linux-dvb/2008-November/030362.html
- I noticed your missing SOB
- (Again, I note that the attached patch is of type "text/x-patch",
which is fine)

3)
http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/1125/match=1800h
- You note in your message that your prior patch didn't get picked up
possibly because of the switch of mail lists.  That is quite possible,
but whatever the case, your patch was, indeed, lost.
- Herman responded with some suggestions, as well as noting that your
SOB was absent again with the attached patches (though, I know you had
resubmitted back in Nov with a SOB)
- But here starts the most recent problem: unfortunately, the attached
patches where of type "application/octet-stream", which the patchwork
tool will NOT pick up.  Please see:
http://www.linuxtv.org/wiki/index.php/Development:_How_to_submit_patches
This is a summary explanation of the submitting process (which includes
links to several of the documents you've undoubtedly already read) and
touches upon attachments.

4) http://www.mail-archive.com/linux-media@vger.kernel.org/msg05856.html
-  in Hermann's follow up, he correctly notes that the patches never
made it onto the patchwork queue (see my explanation directly above for
why they were not).

5) http://www.mail-archive.com/linux-media@vger.kernel.org/msg05888.html
> Nobody noticed the previous post(s).
>   
- Ha, I beg to differ! -- at the very least, Steve, myself and Hermann
have all spotted your prior patches and have commented.
- Unfortunately, the attached patch is again of the type
"application/octet-stream" and will NOT be automatically picked up by
patchwork


6) http://www.mail-archive.com/linux-media@vger.kernel.org/msg05890.html
> (Shoot me! Now it's correct.)
>   
- Nope.  The attached patch is still a type "application/octet-stream",
and that is why it is not showing up on the patchwork queue list
- I have also just noticed that Trent has also now commented on the
patches (so add another to the list!)

/End of audit trail

I understand your frustration and I don't mean to be bureaucratic (I
have zero say in what patches get picked up), but I hope I have shed
some light upon what has gone wrong over the last several months. 
Although I'd rather suspect that Mauro is now well aware of the issue,
I'd urge you to resubmit (following the recommendations from the link I
provided above) so that it gets picked up and placed upon the patchwork
list.

[PATCH] Leadtek WinFast DTV-1800H support

[Miroslav Šustek]

[-- Attachment #1: Type: text/plain, Size: 432 bytes --]

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


[-- Attachment #2: leadtek_winfast_dtv1800h.patch --]
[-- Type: application/octet-stream, Size: 5847 bytes --]

Adds support for Leadtek WinFast DTV-1800H

From: Miroslav Sustek <sustmidown@centrum.cz>

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

Signed-off-by: Miroslav Sustek <sustmidown@centrum.cz>
Tested-by: Marcin Wojcikowski <emtees.mts@gmail.com>
Tested-by: Karel Juhanak <karel.juhanak@warnet.cz>
Tested-by: Andrew Goff <goffa72@gmail.com>
Tested-by: Jan Novak <novak-j@seznam.cz>

diff -r 65ec132f20df linux/Documentation/video4linux/CARDLIST.cx88
--- a/linux/Documentation/video4linux/CARDLIST.cx88	Wed May 27 15:53:00 2009 -0300
+++ b/linux/Documentation/video4linux/CARDLIST.cx88	Fri May 29 16:11:14 2009 +0200
@@ -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]
diff -r 65ec132f20df linux/drivers/media/video/cx88/cx88-cards.c
--- a/linux/drivers/media/video/cx88/cx88-cards.c	Wed May 27 15:53:00 2009 -0300
+++ b/linux/drivers/media/video/cx88/cx88-cards.c	Fri May 29 16:11:14 2009 +0200
@@ -2009,6 +2009,47 @@
 		.tuner_addr	= ADDR_UNSET,
 		.radio_addr	= ADDR_UNSET,
 	},
+	[CX88_BOARD_WINFAST_DTV1800H] = {
+		.name           = "Leadtek WinFast DTV1800 Hybrid",
+		.tuner_type     = TUNER_XC2028,
+		.radio_type     = TUNER_XC2028,
+		.tuner_addr     = 0x61,
+		.radio_addr     = 0x61,
+		/*
+		 * GPIO setting
+		 *
+		 *  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,
+		},
+		.mpeg           = CX88_MPEG_DVB,
+	},
 };
 
 /* ------------------------------------------------------------------ */
@@ -2426,6 +2467,10 @@
 		.subvendor = 0x0070,
 		.subdevice = 0x9290,
 		.card      = CX88_BOARD_HAUPPAUGE_IRONLY,
+	}, {
+		.subvendor = 0x107d,
+		.subdevice = 0x6654,
+		.card      = CX88_BOARD_WINFAST_DTV1800H,
 	},
 };
 
@@ -2624,6 +2669,23 @@
 	return -EINVAL;
 }
 
+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) */
+		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;
+}
+
 /* ------------------------------------------------------------------- */
 /* some Divco specific stuff                                           */
 static int cx88_pv_8000gt_callback(struct cx88_core *core,
@@ -2696,6 +2758,8 @@
 	case CX88_BOARD_DVICO_FUSIONHDTV_DVB_T_PRO:
 	case CX88_BOARD_DVICO_FUSIONHDTV_5_PCI_NANO:
 		return cx88_dvico_xc2028_callback(core, command, arg);
+	case CX88_BOARD_WINFAST_DTV1800H:
+		return cx88_xc3028_winfast1800h_callback(core, command, arg);
 	}
 
 	switch (command) {
@@ -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;
 	}
 }
 
@@ -2902,6 +2976,7 @@
 			core->i2c_algo.udelay = 16;
 		break;
 	case CX88_BOARD_DVICO_FUSIONHDTV_DVB_T_PRO:
+	case CX88_BOARD_WINFAST_DTV1800H:
 		ctl->demod = XC3028_FE_ZARLINK456;
 		break;
 	case CX88_BOARD_KWORLD_ATSC_120:
diff -r 65ec132f20df linux/drivers/media/video/cx88/cx88-dvb.c
--- a/linux/drivers/media/video/cx88/cx88-dvb.c	Wed May 27 15:53:00 2009 -0300
+++ b/linux/drivers/media/video/cx88/cx88-dvb.c	Fri May 29 16:11:14 2009 +0200
@@ -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);
diff -r 65ec132f20df linux/drivers/media/video/cx88/cx88-input.c
--- a/linux/drivers/media/video/cx88/cx88-input.c	Wed May 27 15:53:00 2009 -0300
+++ b/linux/drivers/media/video/cx88/cx88-input.c	Fri May 29 16:11:14 2009 +0200
@@ -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;
diff -r 65ec132f20df linux/drivers/media/video/cx88/cx88.h
--- a/linux/drivers/media/video/cx88/cx88.h	Wed May 27 15:53:00 2009 -0300
+++ b/linux/drivers/media/video/cx88/cx88.h	Fri May 29 16:11:14 2009 +0200
@@ -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,

Re: [PATCH] Leadtek WinFast DTV-1800H support

[Trent Piepho]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=X-UNKNOWN, Size: 3623 bytes --]

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

> +                *  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) */
> +               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;

Re: [PATCH] Leadtek WinFast DTV-1800H support

[Miroslav Šustek]

[-- Attachment #1: Type: text/plain, Size: 4186 bytes --]

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.

>
> 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. :)

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

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


[-- Attachment #2: leadtek_winfast_dtv1800h_v2.patch --]
[-- Type: application/octet-stream, Size: 5789 bytes --]

Adds support for Leadtek WinFast DTV-1800H

From: Miroslav Sustek <sustmidown@centrum.cz>

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

Signed-off-by: Miroslav Sustek <sustmidown@centrum.cz>
Tested-by: Marcin Wojcikowski <emtees.mts@gmail.com>
Tested-by: Karel Juhanak <karel.juhanak@warnet.cz>
Tested-by: Andrew Goff <goffa72@gmail.com>
Tested-by: Jan Novak <novak-j@seznam.cz>

diff -r 25bc0580359a linux/Documentation/video4linux/CARDLIST.cx88
--- a/linux/Documentation/video4linux/CARDLIST.cx88	Fri May 29 17:03:31 2009 -0300
+++ b/linux/Documentation/video4linux/CARDLIST.cx88	Sun May 31 18:44:05 2009 +0200
@@ -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]
diff -r 25bc0580359a linux/drivers/media/video/cx88/cx88-cards.c
--- a/linux/drivers/media/video/cx88/cx88-cards.c	Fri May 29 17:03:31 2009 -0300
+++ b/linux/drivers/media/video/cx88/cx88-cards.c	Sun May 31 18:44:05 2009 +0200
@@ -2009,6 +2009,47 @@
 		.tuner_addr	= ADDR_UNSET,
 		.radio_addr	= ADDR_UNSET,
 	},
+	[CX88_BOARD_WINFAST_DTV1800H] = {
+		.name           = "Leadtek WinFast DTV1800 Hybrid",
+		.tuner_type     = TUNER_XC2028,
+		.radio_type     = TUNER_XC2028,
+		.tuner_addr     = 0x61,
+		.radio_addr     = 0x61,
+		/*
+		 * GPIO setting
+		 *
+		 *  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  = 0x7050,       /* p12 = 1, p13 = 0, p14 = 1 */
+			.gpio2  = 0x0000,
+		}, {
+			.type   = CX88_VMUX_COMPOSITE1,
+			.vmux   = 1,
+			.gpio0  = 0x0400,       /* pin 2 = 0 */
+			.gpio1  = 0x7060,       /* p12 = 0, p13 = 1, p14 = 1 */
+			.gpio2  = 0x0000,
+		}, {
+			.type   = CX88_VMUX_SVIDEO,
+			.vmux   = 2,
+			.gpio0  = 0x0400,       /* pin 2 = 0 */
+			.gpio1  = 0x7060,       /* p12 = 0, p13 = 1, p14 = 1 */
+			.gpio2  = 0x0000,
+		} },
+		.radio = {
+			.type   = CX88_RADIO,
+			.gpio0  = 0x0400,       /* pin 2 = 0 */
+			.gpio1  = 0x7010,       /* p12 = 1, p13 = 0, p14 = 0 */
+			.gpio2  = 0x0000,
+		},
+		.mpeg           = CX88_MPEG_DVB,
+	},
 };
 
 /* ------------------------------------------------------------------ */
@@ -2426,6 +2467,10 @@
 		.subvendor = 0x0070,
 		.subdevice = 0x9290,
 		.card      = CX88_BOARD_HAUPPAUGE_IRONLY,
+	}, {
+		.subvendor = 0x107d,
+		.subdevice = 0x6654,
+		.card      = CX88_BOARD_WINFAST_DTV1800H,
 	},
 };
 
@@ -2624,6 +2669,21 @@
 	return -EINVAL;
 }
 
+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) */
+		cx_write(MO_GP1_IO, 0x101000);
+		mdelay(50);
+		cx_write(MO_GP1_IO, 0x101010);
+		mdelay(50);
+		return 0;
+	}
+	return -EINVAL;
+}
+
 /* ------------------------------------------------------------------- */
 /* some Divco specific stuff                                           */
 static int cx88_pv_8000gt_callback(struct cx88_core *core,
@@ -2696,6 +2756,8 @@
 	case CX88_BOARD_DVICO_FUSIONHDTV_DVB_T_PRO:
 	case CX88_BOARD_DVICO_FUSIONHDTV_5_PCI_NANO:
 		return cx88_dvico_xc2028_callback(core, command, arg);
+	case CX88_BOARD_WINFAST_DTV1800H:
+		return cx88_xc3028_winfast1800h_callback(core, command, arg);
 	}
 
 	switch (command) {
@@ -2882,6 +2944,14 @@
 		cx_set(MO_GP0_IO, 0x00000080); /* 702 out of reset */
 		udelay(1000);
 		break;
+
+	case CX88_BOARD_WINFAST_DTV1800H:
+		/* GPIO 12 (xc3028 tuner reset) */
+		cx_write(MO_GP1_IO, 0x101000);
+		mdelay(50);
+		cx_write(MO_GP1_IO, 0x101010);
+		mdelay(50);
+		break;
 	}
 }
 
@@ -2902,6 +2972,7 @@
 			core->i2c_algo.udelay = 16;
 		break;
 	case CX88_BOARD_DVICO_FUSIONHDTV_DVB_T_PRO:
+	case CX88_BOARD_WINFAST_DTV1800H:
 		ctl->demod = XC3028_FE_ZARLINK456;
 		break;
 	case CX88_BOARD_KWORLD_ATSC_120:
diff -r 25bc0580359a linux/drivers/media/video/cx88/cx88-dvb.c
--- a/linux/drivers/media/video/cx88/cx88-dvb.c	Fri May 29 17:03:31 2009 -0300
+++ b/linux/drivers/media/video/cx88/cx88-dvb.c	Sun May 31 18:44:05 2009 +0200
@@ -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);
diff -r 25bc0580359a linux/drivers/media/video/cx88/cx88-input.c
--- a/linux/drivers/media/video/cx88/cx88-input.c	Fri May 29 17:03:31 2009 -0300
+++ b/linux/drivers/media/video/cx88/cx88-input.c	Sun May 31 18:44:05 2009 +0200
@@ -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;
diff -r 25bc0580359a linux/drivers/media/video/cx88/cx88.h
--- a/linux/drivers/media/video/cx88/cx88.h	Fri May 29 17:03:31 2009 -0300
+++ b/linux/drivers/media/video/cx88/cx88.h	Sun May 31 18:44:05 2009 +0200
@@ -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,

Re: [PATCH] Leadtek WinFast DTV-1800H support

[Mauro Carvalho Chehab]

Em Sun, 31 May 2009 19:39:15 +0200
"Miroslav  Šustek" <sustmidown@centrum.cz> escreveu:

> 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.
> 
> >
> > 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. :)
> 
> > 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?
> 
> >
> > @@ -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;
> 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.

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

Re: [PATCH] Leadtek WinFast DTV-1800H support

[hermann pitton]

Am Sonntag, den 31.05.2009, 16:58 -0300 schrieb Mauro Carvalho Chehab:
> Em Sun, 31 May 2009 19:39:15 +0200
> "Miroslav  Šustek" <sustmidown@centrum.cz> escreveu:
> 
> > 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.
> > 
> > >
> > > 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. :)
> > 
> > > 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?
> > 
> > >
> > > @@ -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;
> > 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.
> 
> 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

Re: [PATCH] Leadtek WinFast DTV-1800H support

[Mauro Carvalho Chehab]

Em Mon, 01 Jun 2009 03:58:25 +0200
hermann pitton <hermann-pitton@arcor.de> escreveu:

> 
> Am Sonntag, den 31.05.2009, 16:58 -0300 schrieb Mauro Carvalho Chehab:
> > Em Sun, 31 May 2009 19:39:15 +0200
> > "Miroslav  Šustek" <sustmidown@centrum.cz> escreveu:
> > 
> > > 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.
> > > 
> > > >
> > > > 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. :)
> > > 
> > > > 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?
> > > 
> > > >
> > > > @@ -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;
> > > 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.
> > 
> > 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.

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

Re: [PATCH] Leadtek WinFast DTV-1800H support

[hermann pitton]

Am Montag, den 01.06.2009, 06:07 -0300 schrieb Mauro Carvalho Chehab:
> Em Mon, 01 Jun 2009 03:58:25 +0200
> hermann pitton <hermann-pitton@arcor.de> escreveu:
> 
> > 
> > Am Sonntag, den 31.05.2009, 16:58 -0300 schrieb Mauro Carvalho Chehab:
> > > Em Sun, 31 May 2009 19:39:15 +0200
> > > "Miroslav  Šustek" <sustmidown@centrum.cz> escreveu:
> > > 
> > > > 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.
> > > > 
> > > > >
> > > > > 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. :)
> > > > 
> > > > > 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?
> > > > 
> > > > >
> > > > > @@ -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;
> > > > 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.
> > > 
> > > 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.
> 
> 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.

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

Re: [PATCH] Leadtek WinFast DTV-1800H support

[Miroslav Šustek]

Trent Piepho píše v Ne 31. 05. 2009 v 06:34 -0700:
> 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.

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

Re: [PATCH] Leadtek WinFast DTV-1800H support

[Trent Piepho]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=X-UNKNOWN, Size: 7382 bytes --]

On Sun, 31 May 2009, Miroslav [UTF-8]  Šustek wrote:
> 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.

They are used whenever the video mux is set.  That will happen when
changing inputs and when the cx8800 driver first initializes the card.
Opening the radio device does an implicit change to the radio input.

It looks like firmware is loaded when needed as part of setting the tuner
frequency.  I think that makes it safe to assume that the gpios will be set
before firmware is loaded.   Though it might be possible if the cx8802
driver is loaded before the cx8800 driver.

Since you have the hardware, it would be easy to check.  Add printk's in
the reset callback code you wrote so you'll know when it's called.  Then
set video_debug to 1 and look for "video_mux:  " lines, which indicate the
card gpio values are being set.

It seems clear that the xc2028 has an active low reset line.  To reset, the
line must be lowered for some time period (probably very short) and then
raised to enable the chip, at which point there should be a delay (probably
longer) while waiting for the chip to come out of reset.  If you think
about it, it does not matter what state the reset line is in before we
lower it.  It can be high, it can be low, the chip be reset either way.

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

It should be unnecessary to do that, but might help with power consumption.
To do it, change the composite and s-video gpio1 values to 0x7060.

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

It seems like firmware loaded should only happen on frequency change.

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

Normally all the unused gpio lines are just set as inputs.

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

Steven is wrong there and you are right.  The cx88 gpio lines have the mask
bits in the 3rd byte, which allow changing a gpio without modifying the
others with a single write.  It is better to use this than to do a
read-modify-write.  That is actually less safe, since it's not an atomic
operation.

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

You could use cx_andor() to set the direction bit and lower the reset pin
in one call.  cx_set/cx_clear are just calls to cx_andor().  But using the
mask bit and cx_write is best.

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

There is other code for xc2028 reset for different cards, e.g.
cx88_dvico_xc2028_callback, cx88_xc3028_geniatech_tuner_callback,
cx88_xc2028_tuner_callback, cx88_pv_8000gt_callback, and even
cx88_xc5000_tuner_callback.  Shouldn't these functions all do the same
thing other than what gpio line 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;
> 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.

The reason cx88_card_setup_pre_i2c() needs to do something is to reset
(actually, un-reset) the xc2028 since the chip will mess up i2c if it is in
reset state.  So really it is the same thing.  I think in this function we
could probably just say:

	if (core->board.tuner_type == TUNER_XC2028)
		cx88_xc2028_tuner_callback(core, XC2028_TUNER_RESET, 0);

That would handle all xc2028 cards at once and there would be no need for
each xc2028 to put a block of code here.

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

They will care if the driver becomes a broken mess and is full of bugs and
no one will work on it anymore.  They will care if linux is as bloated
as vista.  So developers need to take care to do things right.

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