| Message ID | 1520355879-20291-1-git-send-email-hugues.fruchet@st.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Hugues, On Tue, Mar 06, 2018 at 06:04:39PM +0100, Hugues Fruchet wrote: > Fix set of missing colorspace related fields in get_/set_fmt. > Detected by v4l2-compliance tool. > > Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com> Could you confirm this is the one you intended to send? There are two others with similar content. ... > @@ -2497,16 +2504,22 @@ static int ov5640_probe(struct i2c_client *client, > struct fwnode_handle *endpoint; > struct ov5640_dev *sensor; > int ret; > + struct v4l2_mbus_framefmt *fmt; This one I'd arrange before ret. The local variable declarations should generally look like a Christmas tree but upside down. If you're happy with that, I can swap the two lines as well (no need for v2). > > sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL); > if (!sensor) > return -ENOMEM; > > sensor->i2c_client = client; > - sensor->fmt.code = MEDIA_BUS_FMT_UYVY8_2X8; > - sensor->fmt.width = 640; > - sensor->fmt.height = 480; > - sensor->fmt.field = V4L2_FIELD_NONE; > + fmt = &sensor->fmt; > + fmt->code = ov5640_formats[0].code; > + fmt->colorspace = ov5640_formats[0].colorspace; > + fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace); > + fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE; > + fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt->colorspace); > + fmt->width = 640; > + fmt->height = 480; > + fmt->field = V4L2_FIELD_NONE; > sensor->frame_interval.numerator = 1; > sensor->frame_interval.denominator = ov5640_framerates[OV5640_30_FPS]; > sensor->current_fr = OV5640_30_FPS;
Hi Sakari, On Wed, Mar 7, 2018 at 5:13 AM, Sakari Ailus <sakari.ailus@iki.fi> wrote: >> @@ -2497,16 +2504,22 @@ static int ov5640_probe(struct i2c_client *client, >> struct fwnode_handle *endpoint; >> struct ov5640_dev *sensor; >> int ret; >> + struct v4l2_mbus_framefmt *fmt; > > This one I'd arrange before ret. The local variable declarations should > generally look like a Christmas tree but upside down. It seems Mauro is not happy with reverse Christmas tree ordering: https://www.mail-archive.com/linux-media@vger.kernel.org/msg127221.html
Hi Fabio, On Wed, Mar 07, 2018 at 06:51:26AM -0300, Fabio Estevam wrote: > Hi Sakari, > > On Wed, Mar 7, 2018 at 5:13 AM, Sakari Ailus <sakari.ailus@iki.fi> wrote: > > >> @@ -2497,16 +2504,22 @@ static int ov5640_probe(struct i2c_client *client, > >> struct fwnode_handle *endpoint; > >> struct ov5640_dev *sensor; > >> int ret; > >> + struct v4l2_mbus_framefmt *fmt; > > > > This one I'd arrange before ret. The local variable declarations should > > generally look like a Christmas tree but upside down. > > It seems Mauro is not happy with reverse Christmas tree ordering: > https://www.mail-archive.com/linux-media@vger.kernel.org/msg127221.html There are other arguments supporting the change such as: - alignment with the rest of the driver and - putting similar definitions together (return value vs. pointers somewhere else).
Hi Sakari, This is the right one and it's OK to swap the lines for local variables, I'll keep this in mind for next changes. Best regards, Hugues. On 03/07/2018 09:13 AM, Sakari Ailus wrote: > Hi Hugues, > > On Tue, Mar 06, 2018 at 06:04:39PM +0100, Hugues Fruchet wrote: >> Fix set of missing colorspace related fields in get_/set_fmt. >> Detected by v4l2-compliance tool. >> >> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com> > > Could you confirm this is the one you intended to send? There are two > others with similar content. > > ... > >> @@ -2497,16 +2504,22 @@ static int ov5640_probe(struct i2c_client *client, >> struct fwnode_handle *endpoint; >> struct ov5640_dev *sensor; >> int ret; >> + struct v4l2_mbus_framefmt *fmt; > > This one I'd arrange before ret. The local variable declarations should > generally look like a Christmas tree but upside down. > > If you're happy with that, I can swap the two lines as well (no need for > v2). > >> >> sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL); >> if (!sensor) >> return -ENOMEM; >> >> sensor->i2c_client = client; >> - sensor->fmt.code = MEDIA_BUS_FMT_UYVY8_2X8; >> - sensor->fmt.width = 640; >> - sensor->fmt.height = 480; >> - sensor->fmt.field = V4L2_FIELD_NONE; >> + fmt = &sensor->fmt; >> + fmt->code = ov5640_formats[0].code; >> + fmt->colorspace = ov5640_formats[0].colorspace; >> + fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace); >> + fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE; >> + fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt->colorspace); >> + fmt->width = 640; >> + fmt->height = 480; >> + fmt->field = V4L2_FIELD_NONE; >> sensor->frame_interval.numerator = 1; >> sensor->frame_interval.denominator = ov5640_framerates[OV5640_30_FPS]; >> sensor->current_fr = OV5640_30_FPS; >
diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c index 03940f0..676f635 100644 --- a/drivers/media/i2c/ov5640.c +++ b/drivers/media/i2c/ov5640.c @@ -1874,7 +1874,13 @@ static int ov5640_try_fmt_internal(struct v4l2_subdev *sd, if (ov5640_formats[i].code == fmt->code) break; if (i >= ARRAY_SIZE(ov5640_formats)) - fmt->code = ov5640_formats[0].code; + i = 0; + + fmt->code = ov5640_formats[i].code; + fmt->colorspace = ov5640_formats[i].colorspace; + fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace); + fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE; + fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt->colorspace); return 0; } @@ -1885,6 +1891,7 @@ static int ov5640_set_fmt(struct v4l2_subdev *sd, { struct ov5640_dev *sensor = to_ov5640_dev(sd); const struct ov5640_mode_info *new_mode; + struct v4l2_mbus_framefmt *mbus_fmt = &format->format; int ret; if (format->pad != 0) @@ -1897,7 +1904,7 @@ static int ov5640_set_fmt(struct v4l2_subdev *sd, goto out; } - ret = ov5640_try_fmt_internal(sd, &format->format, + ret = ov5640_try_fmt_internal(sd, mbus_fmt, sensor->current_fr, &new_mode); if (ret) goto out; @@ -1906,12 +1913,12 @@ static int ov5640_set_fmt(struct v4l2_subdev *sd, struct v4l2_mbus_framefmt *fmt = v4l2_subdev_get_try_format(sd, cfg, 0); - *fmt = format->format; + *fmt = *mbus_fmt; goto out; } sensor->current_mode = new_mode; - sensor->fmt = format->format; + sensor->fmt = *mbus_fmt; sensor->pending_mode_change = true; out: mutex_unlock(&sensor->lock); @@ -2497,16 +2504,22 @@ static int ov5640_probe(struct i2c_client *client, struct fwnode_handle *endpoint; struct ov5640_dev *sensor; int ret; + struct v4l2_mbus_framefmt *fmt; sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL); if (!sensor) return -ENOMEM; sensor->i2c_client = client; - sensor->fmt.code = MEDIA_BUS_FMT_UYVY8_2X8; - sensor->fmt.width = 640; - sensor->fmt.height = 480; - sensor->fmt.field = V4L2_FIELD_NONE; + fmt = &sensor->fmt; + fmt->code = ov5640_formats[0].code; + fmt->colorspace = ov5640_formats[0].colorspace; + fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace); + fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE; + fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt->colorspace); + fmt->width = 640; + fmt->height = 480; + fmt->field = V4L2_FIELD_NONE; sensor->frame_interval.numerator = 1; sensor->frame_interval.denominator = ov5640_framerates[OV5640_30_FPS]; sensor->current_fr = OV5640_30_FPS;
Fix set of missing colorspace related fields in get_/set_fmt. Detected by v4l2-compliance tool. Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com> --- drivers/media/i2c/ov5640.c | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-)