Hi Hardik, Just a few comments on your comments :-) > Hi Hans, > My Comments inlined, > Most of the comments are taken care off. > Thanks for review. > > Hardik, > >> -----Original Message----- >> From: Hans Verkuil [mailto:hverkuil@xxxxxxxxx] >> Sent: Saturday, April 18, 2009 7:29 PM >> To: Shah, Hardik >> Cc: linux-media@xxxxxxxxxxxxxxx; linux-omap@xxxxxxxxxxxxxxx; Jadav, >> Brijesh R; >> Hiremath, Vaibhav >> Subject: Re: [Review PATCH 3/3] OMAP2/3 V4L2 Display Driver >> > +config VID2_LCD_MANAGER >> > + bool "Use LCD Managaer" >> > + help >> > + Select this option if you want VID2 pipeline on LCD Overlay >> manager >> > +endchoice >> > + >> > +choice >> > + prompt "TV Mode" >> > + depends on VID2_TV_MANAGER || VID1_TV_MANAGER >> > + default NTSC_M >> > + >> > +config NTSC_M >> > + bool "Use NTSC_M mode" >> > + help >> > + Select this option if you want NTSC_M mode on TV >> > + >> > +config PAL_BDGHI >> > + bool "Use PAL_BDGHI mode" >> > + help >> > + Select this option if you want PAL_BDGHI mode on TV >> >> Terminology: PAL and NTSC etc. refer to broadcast standards. That is no >> generally applicable to omap. When it comes to streaming digital video >> there is only the 50 and 60 Hz SDTV standards. For output over a >> Composite >> or S-Video connector you can also choose between PAL and SECAM. There >> are some differences between the two, although I'm not sure about the >> details. The common saa7128 i2c device definitely has support for both. >> >> In this particular case you probably mean 50 or 60 Hz SDTV rather than >> NTSC/PAL. >> > [Shah, Hardik] OMAP DSS is having the internal video encoder for > converting the digital to analog standards like NTSC and PAL with a > s-video and composite output. Currently DSS does not support changing of > the TV standards dynamically so I have made it as a compile time option. > Once DSS will support that I will add the S_STD and G_STD for standards. > Internally it will call the DSS2 library APIs to change the standard. I don't have a problem with these config options, it's more the names NTSC_M and PAL_BDGHI that are misleading IMHO, since it's really a matter of 50Hz vs 60Hz and not of NTSC vs PAL. >> > + if (1 == vout->mirror && vout->rotation >= 0) { >> > + rotation_deg = (vout->rotation == 1) ? >> > + 3 : (vout->rotation == 3) ? >> > + 1 : (vout->rotation == 2) ? >> > + 0 : 2; >> >> Or: rotation = (4 - vout->rotation) % 4; > [Shah, Hardik] It will not work when rotation will be 0 and I want 2 > because of mirroring. (4-0) % 2 is 0 I want 2 here. So not changing it. You are right. Sorry about that. >> > +static int vidioc_s_ctrl(struct file *file, void *fh, struct >> v4l2_control >> *a) >> > +{ >> > + struct omap_vout_device *vout = ((struct omap_vout_fh *) >> fh)->vout; >> > + >> > + switch (a->id) { >> > + case V4L2_CID_ROTATE: >> > + { >> > + int rotation = a->value; >> > + >> > + if (vout->pix.pixelformat == V4L2_PIX_FMT_RGB24 && >> > + rotation != -1) >> >> Huh? Shouldn't this be rotation != 0? > [Shah, Hardik] Rotation 0 means the rotation using Virtual Frame Buffer > Rotation (VRFB) engine. It does not support rotation with packed RGB24 > format. -1 means VRFB is not used. So it should be -1; This really isn't right. a->value is the rotate control value. And that's defined as 0, 90, 180 or 270 according to queryctrl. Not -1. The value -1 is a purely internal value which is also why I am opposed to its use. It is very confusing. Regards, Hans -- Hans Verkuil - video4linux developer - sponsored by TANDBERG -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
