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

Oliver Endriss o.endriss at gmx.de
Mon Oct 29 03:10:56 CET 2007


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



More information about the linux-dvb mailing list