Re: [linux-dvb] [v4l-dvb-maintainer] FWD: [Patch] saa7146/budget*/dvb-ttpci: Remove V4L1 dependencies

2007-10-29 Thread Mauro Carvalho Chehab 
 Basically the enum is not required.
 Everything works fine without replacing VIDEO_MODE_xxx by 
 AV7110_VIDEO_MODE_xxx. (VIDEO_MODE_xxx is defined in videodev.h.)
 
 On the other hand, I like the enum because it defines the interface
 between firmware and driver in a clean way. video_tuner-mode defines
 should not be used here because anything except VIDEO_MODE_PAL or
 VIDEO_MODE_NTSC are not valid for the firmware. In the future the enum
 might be extended to display NTSC content on a PAL TV...

For me, having the enum is overkill, but this doesn't hurt.
So, let's then keep the enum if you wish.

  Also, please review the patch with scripts/checkpatch.pl.
 
 Will do so.

Ok, thanks.

-- 
Cheers,
Mauro


___
linux-dvb mailing list
linux-dvb@linuxtv.org
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb

Re: [linux-dvb] [v4l-dvb-maintainer] FWD: [Patch] saa7146/budget*/dvb-ttpci: Remove V4L1 dependencies

2007-10-28 Thread Mauro Carvalho Chehab 
Hi Oliver and Marco,

The patch looked good to me. 

Some comments:

IMO, instead of creating an emum for vidmode, I would instead just store
v4l2_std_id there.

 if (std-id  V4L2_STD_PAL) {
 -   av7110-vidmode = VIDEO_MODE_PAL;
 +   av7110-vidmode = AV7110_VIDEO_MODE_PAL;
 av7110_set_vidmode(av7110, av7110-vidmode);
 }
 else if (std-id  V4L2_STD_NTSC) {
 -   av7110-vidmode = VIDEO_MODE_NTSC;
 +   av7110-vidmode = AV7110_VIDEO_MODE_NTSC;
 av7110_set_vidmode(av7110, av7110-vidmode);
 }

I dunno if av7110 does support PAL/60, PAL/M or PAL/N. I did a quick
look on a datasheet I found at the net for
av7110(http://www.cdaniel.de/download/AV711x_3_1.pdfs). It seems that
the only supported PAL ones are PAL/BDGHI. If this is true, the above
code is perfect.

However, if the driver supports other PAL standards, the above code
won't work, since a few PAL standards are not marked as V4L2_STD PAL
[1]. If this the case, the above code is not correct. 

[1] On V4L2, V4L2_STD_PAL means only PAL/BGDKHI. IMHO, this is one of
the weird things at V4L2 API.

To support also PAL/60, and PAL/MN, a better coding would be:

if (std-id  V4L2_STD_SECAM) {
printk(KERN_ERR Secam is not supported\n);
else if (std-id  V4L2_STD_NTSC) {
av7110-vidmode = AV7110_VIDEO_MODE_NTSC;
av7110_set_vidmode(av7110, av7110-vidmode);
} else {
av7110-vidmode = AV7110_VIDEO_MODE_PAL;
av7110_set_vidmode(av7110, av7110-vidmode);
}

Or to use V4L2_STD_525_60 (for 60 Hz standards) and V4L2_STD_625_50 (for
50 Hz standards).
 
Also, please review the patch with scripts/checkpatch.pl.

Cheers,
Mauro


___
linux-dvb mailing list
linux-dvb@linuxtv.org
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb

Re: [linux-dvb] [v4l-dvb-maintainer] FWD: [Patch] saa7146/budget*/dvb-ttpci: Remove V4L1 dependencies

2007-10-28 Thread Oliver Endriss 
Mauro Carvalho Chehab wrote:
 Hi Oliver and Marco,
 
 The patch looked good to me. 
 
 Some comments:
 
 IMO, instead of creating an emum for vidmode, I would instead just store
 v4l2_std_id there.
 
  if (std-id  V4L2_STD_PAL) {
  -   av7110-vidmode = VIDEO_MODE_PAL;
  +   av7110-vidmode = AV7110_VIDEO_MODE_PAL;
  av7110_set_vidmode(av7110, av7110-vidmode);
  }
  else if (std-id  V4L2_STD_NTSC) {
  -   av7110-vidmode = VIDEO_MODE_NTSC;
  +   av7110-vidmode = AV7110_VIDEO_MODE_NTSC;
  av7110_set_vidmode(av7110, av7110-vidmode);
  }

Basically the enum is not required.
Everything works fine without replacing VIDEO_MODE_xxx by 
AV7110_VIDEO_MODE_xxx. (VIDEO_MODE_xxx is defined in videodev.h.)

On the other hand, I like the enum because it defines the interface
between firmware and driver in a clean way. video_tuner-mode defines
should not be used here because anything except VIDEO_MODE_PAL or
VIDEO_MODE_NTSC are not valid for the firmware. In the future the enum
might be extended to display NTSC content on a PAL TV...

 I dunno if av7110 does support PAL/60, PAL/M or PAL/N. I did a quick
 look on a datasheet I found at the net for
 av7110(http://www.cdaniel.de/download/AV711x_3_1.pdfs). It seems that
 the only supported PAL ones are PAL/BDGHI. If this is true, the above
 code is perfect.

It's ok. Currently we don't support those PAL standards in the firmware.

 However, if the driver supports other PAL standards, the above code
 won't work, since a few PAL standards are not marked as V4L2_STD PAL
 [1]. If this the case, the above code is not correct. 
 
 [1] On V4L2, V4L2_STD_PAL means only PAL/BGDKHI. IMHO, this is one of
 the weird things at V4L2 API.
 
 To support also PAL/60, and PAL/MN, a better coding would be:
 
 if (std-id  V4L2_STD_SECAM) {
   printk(KERN_ERR Secam is not supported\n);
 else if (std-id  V4L2_STD_NTSC) {
   av7110-vidmode = AV7110_VIDEO_MODE_NTSC;
   av7110_set_vidmode(av7110, av7110-vidmode);
 } else {
   av7110-vidmode = AV7110_VIDEO_MODE_PAL;
   av7110_set_vidmode(av7110, av7110-vidmode);
 }
 
 Or to use V4L2_STD_525_60 (for 60 Hz standards) and V4L2_STD_625_50 (for
 50 Hz standards).
  
 Also, please review the patch with scripts/checkpatch.pl.

Will do so.

CU
Oliver

-- 

VDR Remote Plugin 0.4.0: http://www.escape-edv.de/endriss/vdr/


___
linux-dvb mailing list
linux-dvb@linuxtv.org
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb