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

Mauro Carvalho Chehab mchehab at infradead.org
Sun Oct 28 20:31:53 CET 2007


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




More information about the linux-dvb mailing list