The driver suffers from a few compliance and implementation issues:
- crop rectangle is affected by .set_fmt() pad operation callback,
- frame scaling is not reset on modification of crop rectangle,
- V4L2_SUBDEV_FORMAT_TRY support in .set_fmt() uses active crop
  rectangle, not the one from a pad config, as a reference.

For easy resolution of those issues, ov6650_s_fmt() will first be
refactored.

The ov6650_s_fmt() helper function, mostly called form .set_fmt() pad
operation callback, now takes a decision if half scaling is applicable
for current crop rectangle and requested frame size, then possibly
adjusts hardware crop settings, frame scaling and media bus frame
format.  It accepts a struct v4l2_mbus_framefmt argument passed by a
user to .set_fmt().

Move the decision on applicability of half scaling up to .set_fmt(),
then modify the ov6650_s_fmt() API so it accepts a half scaling flag.
In order to avoid passing full struct v4l2_mbus_framefmt argument to it
when called from functions other than .set_fmt(), also accept a target
pixel code as an argument and make the struct v4l2_mbus_framefmt used
for crop rectangle modification optional.

Signed-off-by: Janusz Krzysztofik <jmkrzy...@gmail.com>
---
 drivers/media/i2c/ov6650.c | 38 ++++++++++++++++++++------------------
 1 file changed, 20 insertions(+), 18 deletions(-)

diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
index 1b02479b616f..8cb30f3e4851 100644
--- a/drivers/media/i2c/ov6650.c
+++ b/drivers/media/i2c/ov6650.c
@@ -570,25 +570,18 @@ static u8 to_clkrc(struct v4l2_fract *timeperframe,
 }
 
 /* set the format we will capture in */
-static int ov6650_s_fmt(struct v4l2_subdev *sd, struct v4l2_mbus_framefmt *mf)
+static int ov6650_s_fmt(struct v4l2_subdev *sd, struct v4l2_mbus_framefmt *mf,
+                       u32 code, bool half_scale)
 {
        struct i2c_client *client = v4l2_get_subdevdata(sd);
        struct ov6650 *priv = to_ov6650(client);
-       bool half_scale = !is_unscaled_ok(mf->width, mf->height, &priv->rect);
        struct v4l2_subdev_selection sel = {
                .which = V4L2_SUBDEV_FORMAT_ACTIVE,
                .target = V4L2_SEL_TGT_CROP,
-               .r.left = priv->rect.left + (priv->rect.width >> 1) -
-                       (mf->width >> (1 - half_scale)),
-               .r.top = priv->rect.top + (priv->rect.height >> 1) -
-                       (mf->height >> (1 - half_scale)),
-               .r.width = mf->width << half_scale,
-               .r.height = mf->height << half_scale,
        };
-       u32 code = mf->code;
        unsigned long mclk, pclk;
        u8 coma_set = 0, coma_mask = 0, coml_set, coml_mask, clkrc;
-       int ret;
+       int ret = 0;
 
        /* select color matrix configuration for given color encoding */
        switch (code) {
@@ -668,7 +661,16 @@ static int ov6650_s_fmt(struct v4l2_subdev *sd, struct 
v4l2_mbus_framefmt *mf)
        dev_dbg(&client->dev, "pixel clock divider: %ld.%ld\n",
                        mclk / pclk, 10 * mclk % pclk / pclk);
 
-       ret = ov6650_set_selection(sd, NULL, &sel);
+       if (mf) {
+               sel.r.left = priv->rect.left + (priv->rect.width >> 1) -
+                       (mf->width >> (1 - half_scale)),
+               sel.r.top = priv->rect.top + (priv->rect.height >> 1) -
+                       (mf->height >> (1 - half_scale)),
+               sel.r.width = mf->width << half_scale,
+               sel.r.height = mf->height << half_scale,
+
+               ret = ov6650_set_selection(sd, NULL, &sel);
+       }
        if (!ret)
                ret = ov6650_reg_rmw(client, REG_COMA, coma_set, coma_mask);
        if (!ret)
@@ -691,11 +693,14 @@ static int ov6650_set_fmt(struct v4l2_subdev *sd,
        struct v4l2_mbus_framefmt *mf = &format->format;
        struct i2c_client *client = v4l2_get_subdevdata(sd);
        struct ov6650 *priv = to_ov6650(client);
+       bool half_scale;
 
        if (format->pad)
                return -EINVAL;
 
-       if (is_unscaled_ok(mf->width, mf->height, &priv->rect))
+       half_scale = !is_unscaled_ok(mf->width, mf->height, &priv->rect);
+
+       if (!half_scale)
                v4l_bound_align_image(&mf->width, 2, W_CIF, 1,
                                &mf->height, 2, H_CIF, 1, 0);
 
@@ -730,7 +735,7 @@ static int ov6650_set_fmt(struct v4l2_subdev *sd,
 
        } else {
                /* apply new media bus format code and frame size */
-               int ret = ov6650_s_fmt(sd, mf);
+               int ret = ov6650_s_fmt(sd, mf, mf->code, half_scale);
 
                if (ret)
                        return ret;
@@ -885,11 +890,8 @@ static int ov6650_video_probe(struct v4l2_subdev *sd)
        ret = ov6650_reset(client);
        if (!ret)
                ret = ov6650_prog_dflt(client);
-       if (!ret) {
-               struct v4l2_mbus_framefmt mf = ov6650_def_fmt;
-
-               ret = ov6650_s_fmt(sd, &mf);
-       }
+       if (!ret)
+               ret = ov6650_s_fmt(sd, NULL, ov6650_def_fmt.code, false);
        if (!ret)
                ret = v4l2_ctrl_handler_setup(&priv->hdl);
 
-- 
2.21.0

Reply via email to