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@xxxxxxxxxxx
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
