On Mon, Jan 23, 2023 at 01:51:41PM +0100, Hans de Goede wrote:
> Add a test pattern control. This is a 1:1 copy of the test pattern
> control in the main drivers/media/i2c/ov2680.c driver.
Hmm... I'm not sure I understand the trend of the changes.
We have two drivers of the same sensor, correct?
So, the idea is to move the AtomISP-specific one to be like
the generic and then kill it eventually?
If so, why do we add something here?
Code-wise it's okay change, but see above.
Reviewed-by: Andy Shevchenko <andy@xxxxxxxxxx>
> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> ---
> .../media/atomisp/i2c/atomisp-ov2680.c | 33 +++++++++++++++++++
> drivers/staging/media/atomisp/i2c/ov2680.h | 3 ++
> 2 files changed, 36 insertions(+)
>
> diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
> index 14002a1c22d2..6ca2a5bb0700 100644
> --- a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
> +++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
> @@ -127,6 +127,24 @@ static int ov2680_gain_set(struct ov2680_device *sensor, u32 gain)
> return ovxxxx_write_reg16(sensor->client, OV2680_REG_GAIN_PK, gain);
> }
>
> +static int ov2680_test_pattern_set(struct ov2680_device *sensor, int value)
> +{
> + int ret;
> +
> + if (!value)
> + return ovxxxx_mod_reg(sensor->client, OV2680_REG_ISP_CTRL00, BIT(7), 0);
> +
> + ret = ovxxxx_mod_reg(sensor->client, OV2680_REG_ISP_CTRL00, 0x03, value - 1);
> + if (ret < 0)
> + return ret;
> +
> + ret = ovxxxx_mod_reg(sensor->client, OV2680_REG_ISP_CTRL00, BIT(7), BIT(7));
> + if (ret < 0)
> + return ret;
> +
> + return 0;
> +}
> +
> static int ov2680_s_ctrl(struct v4l2_ctrl *ctrl)
> {
> struct v4l2_subdev *sd = ctrl_to_sd(ctrl);
> @@ -151,6 +169,9 @@ static int ov2680_s_ctrl(struct v4l2_ctrl *ctrl)
> case V4L2_CID_GAIN:
> ret = ov2680_gain_set(sensor, ctrl->val);
> break;
> + case V4L2_CID_TEST_PATTERN:
> + ret = ov2680_test_pattern_set(sensor, ctrl->val);
> + break;
> default:
> ret = -EINVAL;
> }
> @@ -644,6 +665,13 @@ static const struct v4l2_subdev_ops ov2680_ops = {
>
> static int ov2680_init_controls(struct ov2680_device *sensor)
> {
> + static const char * const test_pattern_menu[] = {
> + "Disabled",
> + "Color Bars",
> + "Random Data",
> + "Square",
> + "Black Image",
> + };
> const struct v4l2_ctrl_ops *ops = &ov2680_ctrl_ops;
> struct ov2680_ctrls *ctrls = &sensor->ctrls;
> struct v4l2_ctrl_handler *hdl = &ctrls->handler;
> @@ -658,6 +686,11 @@ static int ov2680_init_controls(struct ov2680_device *sensor)
> ctrls->exposure = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_EXPOSURE,
> 0, exp_max, 1, exp_max);
> ctrls->gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_GAIN, 0, 1023, 1, 250);
> + ctrls->test_pattern =
> + v4l2_ctrl_new_std_menu_items(hdl,
> + &ov2680_ctrl_ops, V4L2_CID_TEST_PATTERN,
> + ARRAY_SIZE(test_pattern_menu) - 1,
> + 0, 0, test_pattern_menu);
>
> ctrls->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> ctrls->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> diff --git a/drivers/staging/media/atomisp/i2c/ov2680.h b/drivers/staging/media/atomisp/i2c/ov2680.h
> index e3ad20a7ffd5..45526477b612 100644
> --- a/drivers/staging/media/atomisp/i2c/ov2680.h
> +++ b/drivers/staging/media/atomisp/i2c/ov2680.h
> @@ -120,6 +120,8 @@
> #define OV2680_MWB_BLUE_GAIN_H 0x5008/*0x3404*/
> #define OV2680_MWB_GAIN_MAX 0x0fff
>
> +#define OV2680_REG_ISP_CTRL00 0x5080
> +
> #define OV2680_START_STREAMING 0x01
> #define OV2680_STOP_STREAMING 0x00
>
> @@ -171,6 +173,7 @@ struct ov2680_device {
> struct v4l2_ctrl *vflip;
> struct v4l2_ctrl *exposure;
> struct v4l2_ctrl *gain;
> + struct v4l2_ctrl *test_pattern;
> } ctrls;
> };
>
> --
> 2.39.0
>
--
With Best Regards,
Andy Shevchenko
