Thanks Albert Wang 86-21-61092656 >-----Original Message----- >From: Guennadi Liakhovetski [mailto:g.liakhovetski@xxxxxx] >Sent: Wednesday, 28 November, 2012 00:50 >To: Albert Wang >Cc: corbet@xxxxxxx; Linux Media Mailing List; Libin Yang >Subject: RE: [PATCH 11/15] [media] marvell-ccic: add soc_camera support in mcam >core > >On Tue, 27 Nov 2012, Albert Wang wrote: > >[snip] > >> >> +static int mcam_camera_set_fmt(struct soc_camera_device *icd, >> >> + struct v4l2_format *f) { >> >> + struct soc_camera_host *ici = to_soc_camera_host(icd->parent); >> >> + struct mcam_camera *mcam = ici->priv; >> >> + const struct soc_camera_format_xlate *xlate = NULL; >> >> + struct v4l2_mbus_framefmt mf; >> >> + struct v4l2_pix_format *pix = &f->fmt.pix; >> >> + int ret = 0; >> > >> >No need to initialise ret. >> > >> Yes, but it looks there is no "bad" impact if we initialize it. :) I >> just want to keep the rule: initialize it before use it. :) > >No, please, don't. Firstly, it adds bloat. Secondly, such "blind" >initialisation can hide real bugs: the variable is initialised, so the compiler doesn't >complain, then you use it in your code, but in reality, the value, that you used is >meaningless in your context and you get a hidden bug. > I have to agree with you at the second reason. So, I will double check them in our patches and remove them in the next version. >[snip] > >> >> +static int mcam_camera_try_fmt(struct soc_camera_device *icd, >> >> + struct v4l2_format *f) { >> >> + struct soc_camera_host *ici = to_soc_camera_host(icd->parent); >> >> + struct mcam_camera *mcam = ici->priv; >> >> + const struct soc_camera_format_xlate *xlate; >> >> + struct v4l2_pix_format *pix = &f->fmt.pix; >> >> + struct v4l2_mbus_framefmt mf; >> >> + __u32 pixfmt = pix->pixelformat; >> >> + int ret = 0; >> > >> >No need to initialise ret. >> > >> >> + >> >> + xlate = soc_camera_xlate_by_fourcc(icd, pixfmt); >> >> + if (!xlate) { >> >> + cam_err(mcam, "camera: format: %c not found\n", >> >> + pix->pixelformat); >> >> + return -EINVAL; >> > >> >You shouldn't fail .try_fmt() (unless something really bad happens). >> >Just pick up a default supported format. >> > >> Do you means we just need pick up the default supported format when try_fmt()? > >If you don't find the requested format - yes, just pick up any format, that you can >support. > OK. >Thanks >Guennadi >--- >Guennadi Liakhovetski, Ph.D. >Freelance Open-Source Software Developer http://www.open-technology.de/ -- 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
