Hi,
On 1/23/23 19:46, Andy Shevchenko wrote:
> 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?
The goal is to kill one eventually yes. I'm not sure which
one to kill yet though. I have actually found a whole bunch
of bugs in the main drivers/media/i2c/ov2680.c code and
given its buggy-ness I wonder if anyone is actually using it.
I need to start an email thread about this (and a couple of
other open questions which I have), I have a bunch of notes
which I need to turn into emails for this.
> If so, why do we add something here?
Because I suspect that the atomisp version might eventually
be the one we want to keep (and move to drivers/media/i2c).
Regards,
Hans
> 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
>>
>
