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