Thanks, Vaibhav Hiremath
> -----Original Message----- > From: Trilok Soni [mailto:[EMAIL PROTECTED] > Sent: Friday, November 21, 2008 11:43 PM > To: Hiremath, Vaibhav > Cc: [EMAIL PROTECTED]; linux-omap@vger.kernel.org; > [EMAIL PROTECTED] > Subject: Re: [PATCH 2/2] TVP514x V4L int device driver support > > Hi Vaibhav, > > On Fri, Nov 21, 2008 at 8:52 PM, <[EMAIL PROTECTED]> wrote: > > From: Vaibhav Hiremath <[EMAIL PROTECTED]> > > > > Added new V4L2 slave driver for TVP514x. > > > > The Driver interface has been tested on OMAP3EVM board > > with TI daughter card (TVP5146). Soon the patch for Daughter card > will > > be posted on community. > > You may want to add some of the TVP5146 video decoder capabilities > in > commit text. Useful for someone who just sees this chip for first > time. > [Hiremath, Vaibhav] Will take care next time. > > > > Signed-off-by: Brijesh Jadav <[EMAIL PROTECTED]> > > Hardik Shah <[EMAIL PROTECTED]> > > Manjunath Hadli <[EMAIL PROTECTED]> > > R Sivaraj <[EMAIL PROTECTED]> > > Vaibhav Hiremath <[EMAIL PROTECTED]> > > Karicheri Muralidharan <[EMAIL PROTECTED]> > > I suggested to change this in another email. > [Hiremath, Vaibhav] Point taken. > > + > > +#include <linux/i2c.h> > > +#include <linux/delay.h> > > +#include <linux/videodev2.h> > > +#include <media/v4l2-int-device.h> > > Convention is is to put empty line between #include <linux/xxx.h> > and > first #include <nonlinux/xxx.h> which is #include > <media/v4l2-int-device.h> > [Hiremath, Vaibhav] I had referred to the other driver files under media/video/ (e.g. tvp5150) which doesn't follow this. Not sure about actual convention, but it looks like line break is expected between standard include (linux or non-linux) and local include. E.g. include <linux/xxx.h> include <media/yyy.h> Include "zzz.h" > > +#include <media/tvp514x.h> > > + > > > + > > +/* List of image formats supported by TVP5146/47 decoder > > + * Currently we are using 8 bit mode only, but can be > > + * extended to 10/20 bit mode. > > + */ > > +static const struct v4l2_fmtdesc tvp514x_fmt_list[] = { > > + { > > + .index = 0, > > + .type = V4L2_BUF_TYPE_VIDEO_CAPTURE, > > + .flags = 0, > > + .description = "8-bit UYVY 4:2:2 Format", > > + .pixelformat = V4L2_PIX_FMT_UYVY, > > + } > > Good to add "," after the last element. > > > +static struct tvp514x_std_info tvp514x_std_list[] = { > > + { > > + .width = NTSC_NUM_ACTIVE_PIXELS, > > + .height = NTSC_NUM_ACTIVE_LINES, > > + .video_std = VIDEO_STD_NTSC_MJ_BIT, > > + .standard = { > > + .index = 0, > > + .id = V4L2_STD_NTSC, > > + .name = "NTSC", > > + .frameperiod = {1001, 30000}, > > + .framelines = 525} > > "{" after 525 looks weird. > > > + }, > > + { > > You can put "{" with "}" to save one line . Like this > > "}, {" > [Hiremath, Vaibhav] Point taken. > You may want to make similar changes at other places in the patch. > > > +static enum tvp514x_std tvp514x_get_current_std(struct > tvp514x_decoder > > + *decoder) > > +{ > > + u8 std, std_status; > > + > > + if (tvp514x_read_reg(decoder->client, REG_VIDEO_STD, > &std)) > > + return STD_INVALID; > > + > > + if ((std & VIDEO_STD_MASK) == VIDEO_STD_AUTO_SWITCH_BIT) { > > + /* use the standard status register */ > > + if (tvp514x_read_reg(decoder->client, > REG_VIDEO_STD_STATUS, > > + &std_status)) > > + return STD_INVALID; > > + } else > > + std_status = std; /* use the standard > register itself */ > > + > > + switch (std_status & VIDEO_STD_MASK) { > > + case VIDEO_STD_NTSC_MJ_BIT: > > + return STD_NTSC_MJ; > > + break; > > No need of " break" here. > [Hiremath, Vaibhav] Point taken. > > + > > + case VIDEO_STD_PAL_BDGHIN_BIT: > > + return STD_PAL_BDGHIN; > > + break; > > Ditto. > > > + > > + default: > > + return STD_INVALID; > > + break; > > Tritto? > > > + } > > + > > + return STD_INVALID; > > +} > > + > > > + > > +static int > > +tvp514x_probe(struct i2c_client *client, const struct > i2c_device_id *id) > > +{ > > Mark this as __init please. > [Hiremath, Vaibhav] Again valid point. I will update the patch with review comments and post it again. Please let me know if there are any more comments. > -- > ---Trilok Soni > http://triloksoni.wordpress.com > http://www.linkedin.com/in/triloksoni -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
