Re: [linux-dvb] [v4l-dvb-maintainer] FWD: [Patch] saa7146/budget*/dvb-ttpci: Remove V4L1 dependencies
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
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
Re: [linux-dvb] [v4l-dvb-maintainer] FWD: [Patch] saa7146/budget*/dvb-ttpci: Remove V4L1 dependencies
> 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
