| Message ID | 20230123125205.622152-34-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: atomisp: Big power-management changes + lots of fixes | expand |
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@kernel.org> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > .../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 >
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@kernel.org> > >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> .../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 >> >
On Tue, Jan 24, 2023 at 12:27:55PM +0100, Hans de Goede wrote: > 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). Fine, just add a few words into cover letter. Btw, do you use `b4` tool to handle patch(es) series? It has a nice feature to handle a series as a PR. In that case the cover letter becomes a merge-commit message which is cool feature in my opinion.
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; };
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. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- .../media/atomisp/i2c/atomisp-ov2680.c | 33 +++++++++++++++++++ drivers/staging/media/atomisp/i2c/ov2680.h | 3 ++ 2 files changed, 36 insertions(+)