diff mbox series

[12/19] media: i2c: ov4689: Implement digital gain control

Message ID 20231211175023.1680247-13-mike.rudenko@gmail.com (mailing list archive)
State Superseded
Delegated to: Sakari Ailus
Headers
Series Omnivision OV4689 refactoring and improvements |

Commit Message

Mikhail Rudenko Dec. 11, 2023, 5:50 p.m. UTC 
  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(-)

Comments

Laurent Pinchart Dec. 11, 2023, 10:15 p.m. UTC | #1 
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);
Mikhail Rudenko Dec. 12, 2023, 12:52 p.m. UTC | #2 
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
Mikhail Rudenko Dec. 12, 2023, 12:52 p.m. UTC | #3 
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
Laurent Pinchart Dec. 18, 2023, 6:04 p.m. UTC | #4 
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);
Mikhail Rudenko Dec. 18, 2023, 8:10 p.m. UTC | #5 
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 mbox series

Patch

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);