| Message ID | 20231211175023.1680247-13-mike.rudenko@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Omnivision OV4689 refactoring and improvements | expand |
Hi Mikhail, Thank you for the patch. On Mon, Dec 11, 2023 at 08:50:15PM +0300, Mikhail Rudenko wrote: > The OV4689 sensor supports digital gain up to 16x. Implement > corresponding control in the driver. Default digital gain value is not > modified by this patch. > > Signed-off-by: Mikhail Rudenko <mike.rudenko@gmail.com> > --- > drivers/media/i2c/ov4689.c | 16 ++++++++++++++-- > 1 file changed, 14 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/i2c/ov4689.c b/drivers/media/i2c/ov4689.c > index 62aeae43d749..ed0ce1b9e55b 100644 > --- a/drivers/media/i2c/ov4689.c > +++ b/drivers/media/i2c/ov4689.c > @@ -35,6 +35,12 @@ > #define OV4689_GAIN_STEP 1 > #define OV4689_GAIN_DEFAULT 0x80 > > +#define OV4689_REG_DIG_GAIN CCI_REG16(0x352A) Lowercase for hex constatns please. > +#define OV4689_DIG_GAIN_MIN 1 > +#define OV4689_DIG_GAIN_MAX 0x7fff > +#define OV4689_DIG_GAIN_STEP 1 > +#define OV4689_DIG_GAIN_DEFAULT 0x800 > + > #define OV4689_REG_TEST_PATTERN CCI_REG8(0x5040) > #define OV4689_TEST_PATTERN_ENABLE 0x80 > #define OV4689_TEST_PATTERN_DISABLE 0x0 > @@ -131,7 +137,6 @@ static const struct cci_reg_sequence ov4689_2688x1520_regs[] = { > > /* AEC PK */ > {CCI_REG8(0x3503), 0x04}, /* AEC_MANUAL gain_input_as_sensor_gain_format = 1 */ > - {CCI_REG8(0x352a), 0x08}, /* DIG_GAIN_FRAC_LONG dig_gain_long[14:8] = 0x08 (2x) */ Is the default value really x2 ? That's not very nice :-S It would be much nicer if the default value of the control mapped to x1, otherwise it's impossible for userspace to interpret the scale of the digital gain value in a generic way. I suppose that could break existing applications though, which isn't great. Out of curiosity, can you tell what SoC(s) you're using this sensor with ? > > /* ADC and analog control*/ > {CCI_REG8(0x3603), 0x40}, > @@ -622,6 +627,9 @@ static int ov4689_set_ctrl(struct v4l2_ctrl *ctrl) > OV4689_TIMING_FLIP_MASK, > val ? 0 : OV4689_TIMING_FLIP_BOTH, &ret); > break; > + case V4L2_CID_DIGITAL_GAIN: > + cci_write(regmap, OV4689_REG_DIG_GAIN, val, &ret); > + break; > default: > dev_warn(dev, "%s Unhandled id:0x%x, val:0x%x\n", > __func__, ctrl->id, val); > @@ -650,7 +658,7 @@ static int ov4689_initialize_controls(struct ov4689 *ov4689) > > handler = &ov4689->ctrl_handler; > mode = ov4689->cur_mode; > - ret = v4l2_ctrl_handler_init(handler, 13); > + ret = v4l2_ctrl_handler_init(handler, 14); > if (ret) > return ret; > > @@ -693,6 +701,10 @@ static int ov4689_initialize_controls(struct ov4689 *ov4689) > v4l2_ctrl_new_std(handler, &ov4689_ctrl_ops, V4L2_CID_VFLIP, 0, 1, 1, 0); > v4l2_ctrl_new_std(handler, &ov4689_ctrl_ops, V4L2_CID_HFLIP, 0, 1, 1, 0); > > + v4l2_ctrl_new_std(handler, &ov4689_ctrl_ops, V4L2_CID_DIGITAL_GAIN, > + OV4689_DIG_GAIN_MIN, OV4689_DIG_GAIN_MAX, > + OV4689_DIG_GAIN_STEP, OV4689_DIG_GAIN_DEFAULT); > + > if (handler->error) { > ret = handler->error; > dev_err(ov4689->dev, "Failed to init controls(%d)\n", ret);
On 2023-12-12 at 00:15 +02, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Hi Mikhail, > > Thank you for the patch. > > On Mon, Dec 11, 2023 at 08:50:15PM +0300, Mikhail Rudenko wrote: >> The OV4689 sensor supports digital gain up to 16x. Implement >> corresponding control in the driver. Default digital gain value is not >> modified by this patch. >> >> Signed-off-by: Mikhail Rudenko <mike.rudenko@gmail.com> >> --- >> drivers/media/i2c/ov4689.c | 16 ++++++++++++++-- >> 1 file changed, 14 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/media/i2c/ov4689.c b/drivers/media/i2c/ov4689.c >> index 62aeae43d749..ed0ce1b9e55b 100644 >> --- a/drivers/media/i2c/ov4689.c >> +++ b/drivers/media/i2c/ov4689.c >> @@ -35,6 +35,12 @@ >> #define OV4689_GAIN_STEP 1 >> #define OV4689_GAIN_DEFAULT 0x80 >> >> +#define OV4689_REG_DIG_GAIN CCI_REG16(0x352A) > > Lowercase for hex constatns please. > >> +#define OV4689_DIG_GAIN_MIN 1 >> +#define OV4689_DIG_GAIN_MAX 0x7fff >> +#define OV4689_DIG_GAIN_STEP 1 >> +#define OV4689_DIG_GAIN_DEFAULT 0x800 >> + >> #define OV4689_REG_TEST_PATTERN CCI_REG8(0x5040) >> #define OV4689_TEST_PATTERN_ENABLE 0x80 >> #define OV4689_TEST_PATTERN_DISABLE 0x0 >> @@ -131,7 +137,6 @@ static const struct cci_reg_sequence ov4689_2688x1520_regs[] = { >> >> /* AEC PK */ >> {CCI_REG8(0x3503), 0x04}, /* AEC_MANUAL gain_input_as_sensor_gain_format = 1 */ >> - {CCI_REG8(0x352a), 0x08}, /* DIG_GAIN_FRAC_LONG dig_gain_long[14:8] = 0x08 (2x) */ > > Is the default value really x2 ? That's not very nice :-S > > It would be much nicer if the default value of the control mapped to x1, > otherwise it's impossible for userspace to interpret the scale of the > digital gain value in a generic way. I suppose that could break existing > applications though, which isn't great. > > Out of curiosity, can you tell what SoC(s) you're using this sensor with > ? > >> >> /* ADC and analog control*/ >> {CCI_REG8(0x3603), 0x40}, >> @@ -622,6 +627,9 @@ static int ov4689_set_ctrl(struct v4l2_ctrl *ctrl) >> OV4689_TIMING_FLIP_MASK, >> val ? 0 : OV4689_TIMING_FLIP_BOTH, &ret); >> break; >> + case V4L2_CID_DIGITAL_GAIN: >> + cci_write(regmap, OV4689_REG_DIG_GAIN, val, &ret); >> + break; >> default: >> dev_warn(dev, "%s Unhandled id:0x%x, val:0x%x\n", >> __func__, ctrl->id, val); >> @@ -650,7 +658,7 @@ static int ov4689_initialize_controls(struct ov4689 *ov4689) >> >> handler = &ov4689->ctrl_handler; >> mode = ov4689->cur_mode; >> - ret = v4l2_ctrl_handler_init(handler, 13); >> + ret = v4l2_ctrl_handler_init(handler, 14); >> if (ret) >> return ret; >> >> @@ -693,6 +701,10 @@ static int ov4689_initialize_controls(struct ov4689 *ov4689) >> v4l2_ctrl_new_std(handler, &ov4689_ctrl_ops, V4L2_CID_VFLIP, 0, 1, 1, 0); >> v4l2_ctrl_new_std(handler, &ov4689_ctrl_ops, V4L2_CID_HFLIP, 0, 1, 1, 0); >> >> + v4l2_ctrl_new_std(handler, &ov4689_ctrl_ops, V4L2_CID_DIGITAL_GAIN, >> + OV4689_DIG_GAIN_MIN, OV4689_DIG_GAIN_MAX, >> + OV4689_DIG_GAIN_STEP, OV4689_DIG_GAIN_DEFAULT); >> + >> if (handler->error) { >> ret = handler->error; >> dev_err(ov4689->dev, "Failed to init controls(%d)\n", ret); -- Best regards, Mikhail Rudenko
On 2023-12-12 at 00:15 +02, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Hi Mikhail, > > Thank you for the patch. > > On Mon, Dec 11, 2023 at 08:50:15PM +0300, Mikhail Rudenko wrote: >> The OV4689 sensor supports digital gain up to 16x. Implement >> corresponding control in the driver. Default digital gain value is not >> modified by this patch. >> >> Signed-off-by: Mikhail Rudenko <mike.rudenko@gmail.com> >> --- >> drivers/media/i2c/ov4689.c | 16 ++++++++++++++-- >> 1 file changed, 14 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/media/i2c/ov4689.c b/drivers/media/i2c/ov4689.c >> index 62aeae43d749..ed0ce1b9e55b 100644 >> --- a/drivers/media/i2c/ov4689.c >> +++ b/drivers/media/i2c/ov4689.c >> @@ -35,6 +35,12 @@ >> #define OV4689_GAIN_STEP 1 >> #define OV4689_GAIN_DEFAULT 0x80 >> >> +#define OV4689_REG_DIG_GAIN CCI_REG16(0x352A) > > Lowercase for hex constatns please. Ah, missed it somehow. Is this convention kernel-wide or media specific? I think checkpatch could have detetected this.. >> +#define OV4689_DIG_GAIN_MIN 1 >> +#define OV4689_DIG_GAIN_MAX 0x7fff >> +#define OV4689_DIG_GAIN_STEP 1 >> +#define OV4689_DIG_GAIN_DEFAULT 0x800 >> + >> #define OV4689_REG_TEST_PATTERN CCI_REG8(0x5040) >> #define OV4689_TEST_PATTERN_ENABLE 0x80 >> #define OV4689_TEST_PATTERN_DISABLE 0x0 >> @@ -131,7 +137,6 @@ static const struct cci_reg_sequence ov4689_2688x1520_regs[] = { >> >> /* AEC PK */ >> {CCI_REG8(0x3503), 0x04}, /* AEC_MANUAL gain_input_as_sensor_gain_format = 1 */ >> - {CCI_REG8(0x352a), 0x08}, /* DIG_GAIN_FRAC_LONG dig_gain_long[14:8] = 0x08 (2x) */ > > Is the default value really x2 ? That's not very nice :-S > > It would be much nicer if the default value of the control mapped to x1, > otherwise it's impossible for userspace to interpret the scale of the > digital gain value in a generic way. I suppose that could break existing > applications though, which isn't great. The datasheet does not explicitly say how register values are mapped to the actual gain. 0x8 comes from the original register tables, and can also be found in a few other drivers for this sensor, although they do not implement digital gain control. OTOH, the power-on value of this register, and default value as found in the datasheet, is 0x4. This was the motivation behind that "(2x)" annotation. So, I'm afraid that we cannot interpret the absolute scale of the digital gain in any case, unless we have more documentation. I tend to keep the default value of 0x8 for the reasons of not (possibly) breaking userspace. > Out of curiosity, can you tell what SoC(s) you're using this sensor with > ? It's Rockchip 3399. I run most of my tests with AGC and AWB off, to be sure they do not hide some important details. > >> >> /* ADC and analog control*/ >> {CCI_REG8(0x3603), 0x40}, >> @@ -622,6 +627,9 @@ static int ov4689_set_ctrl(struct v4l2_ctrl *ctrl) >> OV4689_TIMING_FLIP_MASK, >> val ? 0 : OV4689_TIMING_FLIP_BOTH, &ret); >> break; >> + case V4L2_CID_DIGITAL_GAIN: >> + cci_write(regmap, OV4689_REG_DIG_GAIN, val, &ret); >> + break; >> default: >> dev_warn(dev, "%s Unhandled id:0x%x, val:0x%x\n", >> __func__, ctrl->id, val); >> @@ -650,7 +658,7 @@ static int ov4689_initialize_controls(struct ov4689 *ov4689) >> >> handler = &ov4689->ctrl_handler; >> mode = ov4689->cur_mode; >> - ret = v4l2_ctrl_handler_init(handler, 13); >> + ret = v4l2_ctrl_handler_init(handler, 14); >> if (ret) >> return ret; >> >> @@ -693,6 +701,10 @@ static int ov4689_initialize_controls(struct ov4689 *ov4689) >> v4l2_ctrl_new_std(handler, &ov4689_ctrl_ops, V4L2_CID_VFLIP, 0, 1, 1, 0); >> v4l2_ctrl_new_std(handler, &ov4689_ctrl_ops, V4L2_CID_HFLIP, 0, 1, 1, 0); >> >> + v4l2_ctrl_new_std(handler, &ov4689_ctrl_ops, V4L2_CID_DIGITAL_GAIN, >> + OV4689_DIG_GAIN_MIN, OV4689_DIG_GAIN_MAX, >> + OV4689_DIG_GAIN_STEP, OV4689_DIG_GAIN_DEFAULT); >> + >> if (handler->error) { >> ret = handler->error; >> dev_err(ov4689->dev, "Failed to init controls(%d)\n", ret); -- Best regards, Mikhail Rudenko
Hi Mikhail, On Tue, Dec 12, 2023 at 03:52:48PM +0300, Mikhail Rudenko wrote: > On 2023-12-12 at 00:15 +02, Laurent Pinchart wrote: > > On Mon, Dec 11, 2023 at 08:50:15PM +0300, Mikhail Rudenko wrote: > >> The OV4689 sensor supports digital gain up to 16x. Implement > >> corresponding control in the driver. Default digital gain value is not > >> modified by this patch. > >> > >> Signed-off-by: Mikhail Rudenko <mike.rudenko@gmail.com> > >> --- > >> drivers/media/i2c/ov4689.c | 16 ++++++++++++++-- > >> 1 file changed, 14 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/media/i2c/ov4689.c b/drivers/media/i2c/ov4689.c > >> index 62aeae43d749..ed0ce1b9e55b 100644 > >> --- a/drivers/media/i2c/ov4689.c > >> +++ b/drivers/media/i2c/ov4689.c > >> @@ -35,6 +35,12 @@ > >> #define OV4689_GAIN_STEP 1 > >> #define OV4689_GAIN_DEFAULT 0x80 > >> > >> +#define OV4689_REG_DIG_GAIN CCI_REG16(0x352A) > > > > Lowercase for hex constatns please. > > Ah, missed it somehow. Is this convention kernel-wide or media specific? > I think checkpatch could have detetected this.. It's media-wide :-) Lower-case hex constants are the majority through the kernel, but there's no tree-wide ban on upper-case. > >> +#define OV4689_DIG_GAIN_MIN 1 > >> +#define OV4689_DIG_GAIN_MAX 0x7fff > >> +#define OV4689_DIG_GAIN_STEP 1 > >> +#define OV4689_DIG_GAIN_DEFAULT 0x800 > >> + > >> #define OV4689_REG_TEST_PATTERN CCI_REG8(0x5040) > >> #define OV4689_TEST_PATTERN_ENABLE 0x80 > >> #define OV4689_TEST_PATTERN_DISABLE 0x0 > >> @@ -131,7 +137,6 @@ static const struct cci_reg_sequence ov4689_2688x1520_regs[] = { > >> > >> /* AEC PK */ > >> {CCI_REG8(0x3503), 0x04}, /* AEC_MANUAL gain_input_as_sensor_gain_format = 1 */ > >> - {CCI_REG8(0x352a), 0x08}, /* DIG_GAIN_FRAC_LONG dig_gain_long[14:8] = 0x08 (2x) */ > > > > Is the default value really x2 ? That's not very nice :-S > > > > It would be much nicer if the default value of the control mapped to x1, > > otherwise it's impossible for userspace to interpret the scale of the > > digital gain value in a generic way. I suppose that could break existing > > applications though, which isn't great. > > The datasheet does not explicitly say how register values are mapped to > the actual gain. 0x8 comes from the original register tables, and can > also be found in a few other drivers for this sensor, although they do > not implement digital gain control. > > OTOH, the power-on value of this register, and default value as found in > the datasheet, is 0x4. This was the motivation behind that "(2x)" > annotation. I wonder if the chip has a TPG that would be located before the digital gain. It would be a nice way to test the digital gain scale. > So, I'm afraid that we cannot interpret the absolute scale of the > digital gain in any case, unless we have more documentation. I tend to > keep the default value of 0x8 for the reasons of not (possibly) breaking > userspace. > > > Out of curiosity, can you tell what SoC(s) you're using this sensor with > > ? > > It's Rockchip 3399. I run most of my tests with AGC and AWB off, to be > sure they do not hide some important details. > > >> > >> /* ADC and analog control*/ > >> {CCI_REG8(0x3603), 0x40}, > >> @@ -622,6 +627,9 @@ static int ov4689_set_ctrl(struct v4l2_ctrl *ctrl) > >> OV4689_TIMING_FLIP_MASK, > >> val ? 0 : OV4689_TIMING_FLIP_BOTH, &ret); > >> break; > >> + case V4L2_CID_DIGITAL_GAIN: > >> + cci_write(regmap, OV4689_REG_DIG_GAIN, val, &ret); > >> + break; > >> default: > >> dev_warn(dev, "%s Unhandled id:0x%x, val:0x%x\n", > >> __func__, ctrl->id, val); > >> @@ -650,7 +658,7 @@ static int ov4689_initialize_controls(struct ov4689 *ov4689) > >> > >> handler = &ov4689->ctrl_handler; > >> mode = ov4689->cur_mode; > >> - ret = v4l2_ctrl_handler_init(handler, 13); > >> + ret = v4l2_ctrl_handler_init(handler, 14); > >> if (ret) > >> return ret; > >> > >> @@ -693,6 +701,10 @@ static int ov4689_initialize_controls(struct ov4689 *ov4689) > >> v4l2_ctrl_new_std(handler, &ov4689_ctrl_ops, V4L2_CID_VFLIP, 0, 1, 1, 0); > >> v4l2_ctrl_new_std(handler, &ov4689_ctrl_ops, V4L2_CID_HFLIP, 0, 1, 1, 0); > >> > >> + v4l2_ctrl_new_std(handler, &ov4689_ctrl_ops, V4L2_CID_DIGITAL_GAIN, > >> + OV4689_DIG_GAIN_MIN, OV4689_DIG_GAIN_MAX, > >> + OV4689_DIG_GAIN_STEP, OV4689_DIG_GAIN_DEFAULT); > >> + > >> if (handler->error) { > >> ret = handler->error; > >> dev_err(ov4689->dev, "Failed to init controls(%d)\n", ret);
On 2023-12-18 at 20:04 +02, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Hi Mikhail, > > On Tue, Dec 12, 2023 at 03:52:48PM +0300, Mikhail Rudenko wrote: >> On 2023-12-12 at 00:15 +02, Laurent Pinchart wrote: >> > On Mon, Dec 11, 2023 at 08:50:15PM +0300, Mikhail Rudenko wrote: >> >> The OV4689 sensor supports digital gain up to 16x. Implement >> >> corresponding control in the driver. Default digital gain value is not >> >> modified by this patch. >> >> >> >> Signed-off-by: Mikhail Rudenko <mike.rudenko@gmail.com> >> >> --- >> >> drivers/media/i2c/ov4689.c | 16 ++++++++++++++-- >> >> 1 file changed, 14 insertions(+), 2 deletions(-) >> >> >> >> diff --git a/drivers/media/i2c/ov4689.c b/drivers/media/i2c/ov4689.c >> >> index 62aeae43d749..ed0ce1b9e55b 100644 >> >> --- a/drivers/media/i2c/ov4689.c >> >> +++ b/drivers/media/i2c/ov4689.c >> >> @@ -35,6 +35,12 @@ >> >> #define OV4689_GAIN_STEP 1 >> >> #define OV4689_GAIN_DEFAULT 0x80 >> >> >> >> +#define OV4689_REG_DIG_GAIN CCI_REG16(0x352A) >> > >> > Lowercase for hex constatns please. >> >> Ah, missed it somehow. Is this convention kernel-wide or media specific? >> I think checkpatch could have detetected this.. > > It's media-wide :-) Lower-case hex constants are the majority through > the kernel, but there's no tree-wide ban on upper-case. > >> >> +#define OV4689_DIG_GAIN_MIN 1 >> >> +#define OV4689_DIG_GAIN_MAX 0x7fff >> >> +#define OV4689_DIG_GAIN_STEP 1 >> >> +#define OV4689_DIG_GAIN_DEFAULT 0x800 >> >> + >> >> #define OV4689_REG_TEST_PATTERN CCI_REG8(0x5040) >> >> #define OV4689_TEST_PATTERN_ENABLE 0x80 >> >> #define OV4689_TEST_PATTERN_DISABLE 0x0 >> >> @@ -131,7 +137,6 @@ static const struct cci_reg_sequence ov4689_2688x1520_regs[] = { >> >> >> >> /* AEC PK */ >> >> {CCI_REG8(0x3503), 0x04}, /* AEC_MANUAL gain_input_as_sensor_gain_format = 1 */ >> >> - {CCI_REG8(0x352a), 0x08}, /* DIG_GAIN_FRAC_LONG dig_gain_long[14:8] = 0x08 (2x) */ >> > >> > Is the default value really x2 ? That's not very nice :-S >> > >> > It would be much nicer if the default value of the control mapped to x1, >> > otherwise it's impossible for userspace to interpret the scale of the >> > digital gain value in a generic way. I suppose that could break existing >> > applications though, which isn't great. >> >> The datasheet does not explicitly say how register values are mapped to >> the actual gain. 0x8 comes from the original register tables, and can >> also be found in a few other drivers for this sensor, although they do >> not implement digital gain control. >> >> OTOH, the power-on value of this register, and default value as found in >> the datasheet, is 0x4. This was the motivation behind that "(2x)" >> annotation. > > I wonder if the chip has a TPG that would be located before the digital > gain. It would be a nice way to test the digital gain scale. Thanks for the suggestion, just tested that. Unfortunately, all the supported test patterns are not affected by digital gain at all :( But what if we set the digital gain control's default value in v4l2_ctrl_new_std to 0x400 (power-on default), right after that set ctrl->cur.val to 0x800 (default value before this series), and explain the situation in a comment? Thus we could keep the effective default value, and make it clear that it is 2x at the same time. What do you think? >> So, I'm afraid that we cannot interpret the absolute scale of the >> digital gain in any case, unless we have more documentation. I tend to >> keep the default value of 0x8 for the reasons of not (possibly) breaking >> userspace. >> >> > Out of curiosity, can you tell what SoC(s) you're using this sensor with >> > ? >> >> It's Rockchip 3399. I run most of my tests with AGC and AWB off, to be >> sure they do not hide some important details. >> >> >> >> >> /* ADC and analog control*/ >> >> {CCI_REG8(0x3603), 0x40}, >> >> @@ -622,6 +627,9 @@ static int ov4689_set_ctrl(struct v4l2_ctrl *ctrl) >> >> OV4689_TIMING_FLIP_MASK, >> >> val ? 0 : OV4689_TIMING_FLIP_BOTH, &ret); >> >> break; >> >> + case V4L2_CID_DIGITAL_GAIN: >> >> + cci_write(regmap, OV4689_REG_DIG_GAIN, val, &ret); >> >> + break; >> >> default: >> >> dev_warn(dev, "%s Unhandled id:0x%x, val:0x%x\n", >> >> __func__, ctrl->id, val); >> >> @@ -650,7 +658,7 @@ static int ov4689_initialize_controls(struct ov4689 *ov4689) >> >> >> >> handler = &ov4689->ctrl_handler; >> >> mode = ov4689->cur_mode; >> >> - ret = v4l2_ctrl_handler_init(handler, 13); >> >> + ret = v4l2_ctrl_handler_init(handler, 14); >> >> if (ret) >> >> return ret; >> >> >> >> @@ -693,6 +701,10 @@ static int ov4689_initialize_controls(struct ov4689 *ov4689) >> >> v4l2_ctrl_new_std(handler, &ov4689_ctrl_ops, V4L2_CID_VFLIP, 0, 1, 1, 0); >> >> v4l2_ctrl_new_std(handler, &ov4689_ctrl_ops, V4L2_CID_HFLIP, 0, 1, 1, 0); >> >> >> >> + v4l2_ctrl_new_std(handler, &ov4689_ctrl_ops, V4L2_CID_DIGITAL_GAIN, >> >> + OV4689_DIG_GAIN_MIN, OV4689_DIG_GAIN_MAX, >> >> + OV4689_DIG_GAIN_STEP, OV4689_DIG_GAIN_DEFAULT); >> >> + >> >> if (handler->error) { >> >> ret = handler->error; >> >> dev_err(ov4689->dev, "Failed to init controls(%d)\n", ret); -- Best regards, Mikhail Rudenko
diff --git a/drivers/media/i2c/ov4689.c b/drivers/media/i2c/ov4689.c index 62aeae43d749..ed0ce1b9e55b 100644 --- a/drivers/media/i2c/ov4689.c +++ b/drivers/media/i2c/ov4689.c @@ -35,6 +35,12 @@ #define OV4689_GAIN_STEP 1 #define OV4689_GAIN_DEFAULT 0x80 +#define OV4689_REG_DIG_GAIN CCI_REG16(0x352A) +#define OV4689_DIG_GAIN_MIN 1 +#define OV4689_DIG_GAIN_MAX 0x7fff +#define OV4689_DIG_GAIN_STEP 1 +#define OV4689_DIG_GAIN_DEFAULT 0x800 + #define OV4689_REG_TEST_PATTERN CCI_REG8(0x5040) #define OV4689_TEST_PATTERN_ENABLE 0x80 #define OV4689_TEST_PATTERN_DISABLE 0x0 @@ -131,7 +137,6 @@ static const struct cci_reg_sequence ov4689_2688x1520_regs[] = { /* AEC PK */ {CCI_REG8(0x3503), 0x04}, /* AEC_MANUAL gain_input_as_sensor_gain_format = 1 */ - {CCI_REG8(0x352a), 0x08}, /* DIG_GAIN_FRAC_LONG dig_gain_long[14:8] = 0x08 (2x) */ /* ADC and analog control*/ {CCI_REG8(0x3603), 0x40}, @@ -622,6 +627,9 @@ static int ov4689_set_ctrl(struct v4l2_ctrl *ctrl) OV4689_TIMING_FLIP_MASK, val ? 0 : OV4689_TIMING_FLIP_BOTH, &ret); break; + case V4L2_CID_DIGITAL_GAIN: + cci_write(regmap, OV4689_REG_DIG_GAIN, val, &ret); + break; default: dev_warn(dev, "%s Unhandled id:0x%x, val:0x%x\n", __func__, ctrl->id, val); @@ -650,7 +658,7 @@ static int ov4689_initialize_controls(struct ov4689 *ov4689) handler = &ov4689->ctrl_handler; mode = ov4689->cur_mode; - ret = v4l2_ctrl_handler_init(handler, 13); + ret = v4l2_ctrl_handler_init(handler, 14); if (ret) return ret; @@ -693,6 +701,10 @@ static int ov4689_initialize_controls(struct ov4689 *ov4689) v4l2_ctrl_new_std(handler, &ov4689_ctrl_ops, V4L2_CID_VFLIP, 0, 1, 1, 0); v4l2_ctrl_new_std(handler, &ov4689_ctrl_ops, V4L2_CID_HFLIP, 0, 1, 1, 0); + v4l2_ctrl_new_std(handler, &ov4689_ctrl_ops, V4L2_CID_DIGITAL_GAIN, + OV4689_DIG_GAIN_MIN, OV4689_DIG_GAIN_MAX, + OV4689_DIG_GAIN_STEP, OV4689_DIG_GAIN_DEFAULT); + if (handler->error) { ret = handler->error; dev_err(ov4689->dev, "Failed to init controls(%d)\n", ret);
The OV4689 sensor supports digital gain up to 16x. Implement corresponding control in the driver. Default digital gain value is not modified by this patch. Signed-off-by: Mikhail Rudenko <mike.rudenko@gmail.com> --- drivers/media/i2c/ov4689.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-)