diff mbox series

[11/19] media: i2c: ov4689: Implement vflip/hflip controls

Message ID 20231211175023.1680247-12-mike.rudenko@gmail.com
State New
Headers
Series Omnivision OV4689 refactoring and improvements |

Commit Message

Mikhail Rudenko Dec. 11, 2023, 5:50 p.m. UTC 
  The OV4689 sensor supports horizontal and vertical flipping. Add
appropriate controls to the driver. Toggling both array flip and
digital flip bits allows to achieve flipping while maintaining output
Bayer order. Note that the default value of hflip control corresponds
to both bits set, as it was before this patch.

Signed-off-by: Mikhail Rudenko <mike.rudenko@gmail.com>
---
 drivers/media/i2c/ov4689.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

Comments

Laurent Pinchart Dec. 11, 2023, 10:10 p.m. UTC | #1 
Hi Mikhail,

Thank you for the patch.

On Mon, Dec 11, 2023 at 08:50:14PM +0300, Mikhail Rudenko wrote:
> The OV4689 sensor supports horizontal and vertical flipping. Add
> appropriate controls to the driver. Toggling both array flip and
> digital flip bits allows to achieve flipping while maintaining output
> Bayer order. Note that the default value of hflip control corresponds
> to both bits set, as it was before this patch.

What happens if only hlip or vflip is set, does the bayer pattern change
?

Sakari, I think this patch could use your attention.

> Signed-off-by: Mikhail Rudenko <mike.rudenko@gmail.com>
> ---
>  drivers/media/i2c/ov4689.c | 24 ++++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov4689.c b/drivers/media/i2c/ov4689.c
> index 67d4004bdcfb..62aeae43d749 100644
> --- a/drivers/media/i2c/ov4689.c
> +++ b/drivers/media/i2c/ov4689.c
> @@ -46,6 +46,14 @@
>  #define OV4689_HTS_DIVIDER		4
>  #define OV4689_HTS_MAX			0x7fff
>  
> +#define OV4689_REG_TIMING_FORMAT1	CCI_REG8(0x3820)
> +#define OV4689_REG_TIMING_FORMAT2	CCI_REG8(0x3821)
> +#define OV4689_TIMING_FLIP_MASK		GENMASK(2, 1)
> +#define OV4689_TIMING_FLIP_ARRAY	BIT(1)
> +#define OV4689_TIMING_FLIP_DIGITAL	BIT(2)
> +#define OV4689_TIMING_FLIP_BOTH		(OV4689_TIMING_FLIP_ARRAY |\
> +					 OV4689_TIMING_FLIP_DIGITAL)
> +
>  #define OV4689_LANES			4
>  #define OV4689_XVCLK_FREQ		24000000
>  
> @@ -183,7 +191,6 @@ static const struct cci_reg_sequence ov4689_2688x1520_regs[] = {
>  	{CCI_REG8(0x3811), 0x08}, /* H_WIN_OFF_L h_win_off[7:0] = 0x08*/
>  	{CCI_REG8(0x3813), 0x04}, /* V_WIN_OFF_L v_win_off[7:0] = 0x04 */
>  	{CCI_REG8(0x3819), 0x01}, /* VSYNC_END_L vsync_end_point[7:0] = 0x01 */
> -	{CCI_REG8(0x3821), 0x06}, /* TIMING_FORMAT2 array_h_mirror = 1, digital_h_mirror = 1 */
>  
>  	/* OTP control */
>  	{CCI_REG8(0x3d85), 0x36}, /* OTP_REG85 OTP_power_up_load_setting_enable = 1,
> @@ -605,6 +612,16 @@ static int ov4689_set_ctrl(struct v4l2_ctrl *ctrl)
>  			  (val + ov4689->cur_mode->width) /
>  			  OV4689_HTS_DIVIDER, &ret);
>  		break;
> +	case V4L2_CID_VFLIP:
> +		cci_update_bits(regmap, OV4689_REG_TIMING_FORMAT1,
> +				OV4689_TIMING_FLIP_MASK,
> +				val ? OV4689_TIMING_FLIP_BOTH : 0, &ret);
> +		break;
> +	case V4L2_CID_HFLIP:
> +		cci_update_bits(regmap, OV4689_REG_TIMING_FORMAT2,
> +				OV4689_TIMING_FLIP_MASK,
> +				val ? 0 : OV4689_TIMING_FLIP_BOTH, &ret);
> +		break;
>  	default:
>  		dev_warn(dev, "%s Unhandled id:0x%x, val:0x%x\n",
>  			 __func__, ctrl->id, val);
> @@ -633,7 +650,7 @@ static int ov4689_initialize_controls(struct ov4689 *ov4689)
>  
>  	handler = &ov4689->ctrl_handler;
>  	mode = ov4689->cur_mode;
> -	ret = v4l2_ctrl_handler_init(handler, 11);
> +	ret = v4l2_ctrl_handler_init(handler, 13);

This should be 12 if my comment on 10/19 is correct. Further patches in
the series may need to be adjusted too.

>  	if (ret)
>  		return ret;
>  
> @@ -673,6 +690,9 @@ static int ov4689_initialize_controls(struct ov4689 *ov4689)
>  				     ARRAY_SIZE(ov4689_test_pattern_menu) - 1,
>  				     0, 0, ov4689_test_pattern_menu);
>  
> +	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);
> +
>  	if (handler->error) {
>  		ret = handler->error;
>  		dev_err(ov4689->dev, "Failed to init controls(%d)\n", ret);
Mikhail Rudenko Dec. 12, 2023, 12:42 p.m. UTC | #2 
On 2023-12-12 at 00:10 +02, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:

> Hi Mikhail,
>
> Thank you for the patch.
>
> On Mon, Dec 11, 2023 at 08:50:14PM +0300, Mikhail Rudenko wrote:
>> The OV4689 sensor supports horizontal and vertical flipping. Add
>> appropriate controls to the driver. Toggling both array flip and
>> digital flip bits allows to achieve flipping while maintaining output
>> Bayer order. Note that the default value of hflip control corresponds
>> to both bits set, as it was before this patch.
>
> What happens if only hlip or vflip is set, does the bayer pattern change
> ?

If one changes both digital and analog flip bits (and this is what this
driver does), bayer pattern stay the same for all possible vflip/hflip
combinations. Unfortunately, the datasheet does not say much about these
bits, but I have a hypothesis that the on-sensor ISP can somehow reorder
pixels to keep the same bayer battern when asked.


> Sakari, I think this patch could use your attention.
>
>> Signed-off-by: Mikhail Rudenko <mike.rudenko@gmail.com>
>> ---
>>  drivers/media/i2c/ov4689.c | 24 ++++++++++++++++++++++--
>>  1 file changed, 22 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/i2c/ov4689.c b/drivers/media/i2c/ov4689.c
>> index 67d4004bdcfb..62aeae43d749 100644
>> --- a/drivers/media/i2c/ov4689.c
>> +++ b/drivers/media/i2c/ov4689.c
>> @@ -46,6 +46,14 @@
>>  #define OV4689_HTS_DIVIDER		4
>>  #define OV4689_HTS_MAX			0x7fff
>>
>> +#define OV4689_REG_TIMING_FORMAT1	CCI_REG8(0x3820)
>> +#define OV4689_REG_TIMING_FORMAT2	CCI_REG8(0x3821)
>> +#define OV4689_TIMING_FLIP_MASK		GENMASK(2, 1)
>> +#define OV4689_TIMING_FLIP_ARRAY	BIT(1)
>> +#define OV4689_TIMING_FLIP_DIGITAL	BIT(2)
>> +#define OV4689_TIMING_FLIP_BOTH		(OV4689_TIMING_FLIP_ARRAY |\
>> +					 OV4689_TIMING_FLIP_DIGITAL)
>> +
>>  #define OV4689_LANES			4
>>  #define OV4689_XVCLK_FREQ		24000000
>>
>> @@ -183,7 +191,6 @@ static const struct cci_reg_sequence ov4689_2688x1520_regs[] = {
>>  	{CCI_REG8(0x3811), 0x08}, /* H_WIN_OFF_L h_win_off[7:0] = 0x08*/
>>  	{CCI_REG8(0x3813), 0x04}, /* V_WIN_OFF_L v_win_off[7:0] = 0x04 */
>>  	{CCI_REG8(0x3819), 0x01}, /* VSYNC_END_L vsync_end_point[7:0] = 0x01 */
>> -	{CCI_REG8(0x3821), 0x06}, /* TIMING_FORMAT2 array_h_mirror = 1, digital_h_mirror = 1 */
>>
>>  	/* OTP control */
>>  	{CCI_REG8(0x3d85), 0x36}, /* OTP_REG85 OTP_power_up_load_setting_enable = 1,
>> @@ -605,6 +612,16 @@ static int ov4689_set_ctrl(struct v4l2_ctrl *ctrl)
>>  			  (val + ov4689->cur_mode->width) /
>>  			  OV4689_HTS_DIVIDER, &ret);
>>  		break;
>> +	case V4L2_CID_VFLIP:
>> +		cci_update_bits(regmap, OV4689_REG_TIMING_FORMAT1,
>> +				OV4689_TIMING_FLIP_MASK,
>> +				val ? OV4689_TIMING_FLIP_BOTH : 0, &ret);
>> +		break;
>> +	case V4L2_CID_HFLIP:
>> +		cci_update_bits(regmap, OV4689_REG_TIMING_FORMAT2,
>> +				OV4689_TIMING_FLIP_MASK,
>> +				val ? 0 : OV4689_TIMING_FLIP_BOTH, &ret);
>> +		break;
>>  	default:
>>  		dev_warn(dev, "%s Unhandled id:0x%x, val:0x%x\n",
>>  			 __func__, ctrl->id, val);
>> @@ -633,7 +650,7 @@ static int ov4689_initialize_controls(struct ov4689 *ov4689)
>>
>>  	handler = &ov4689->ctrl_handler;
>>  	mode = ov4689->cur_mode;
>> -	ret = v4l2_ctrl_handler_init(handler, 11);
>> +	ret = v4l2_ctrl_handler_init(handler, 13);
>
> This should be 12 if my comment on 10/19 is correct. Further patches in
> the series may need to be adjusted too.

Ack, will fix in v2.

>>  	if (ret)
>>  		return ret;
>>
>> @@ -673,6 +690,9 @@ static int ov4689_initialize_controls(struct ov4689 *ov4689)
>>  				     ARRAY_SIZE(ov4689_test_pattern_menu) - 1,
>>  				     0, 0, ov4689_test_pattern_menu);
>>
>> +	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);
>> +
>>  	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 67d4004bdcfb..62aeae43d749 100644
--- a/drivers/media/i2c/ov4689.c
+++ b/drivers/media/i2c/ov4689.c
@@ -46,6 +46,14 @@ 
 #define OV4689_HTS_DIVIDER		4
 #define OV4689_HTS_MAX			0x7fff
 
+#define OV4689_REG_TIMING_FORMAT1	CCI_REG8(0x3820)
+#define OV4689_REG_TIMING_FORMAT2	CCI_REG8(0x3821)
+#define OV4689_TIMING_FLIP_MASK		GENMASK(2, 1)
+#define OV4689_TIMING_FLIP_ARRAY	BIT(1)
+#define OV4689_TIMING_FLIP_DIGITAL	BIT(2)
+#define OV4689_TIMING_FLIP_BOTH		(OV4689_TIMING_FLIP_ARRAY |\
+					 OV4689_TIMING_FLIP_DIGITAL)
+
 #define OV4689_LANES			4
 #define OV4689_XVCLK_FREQ		24000000
 
@@ -183,7 +191,6 @@  static const struct cci_reg_sequence ov4689_2688x1520_regs[] = {
 	{CCI_REG8(0x3811), 0x08}, /* H_WIN_OFF_L h_win_off[7:0] = 0x08*/
 	{CCI_REG8(0x3813), 0x04}, /* V_WIN_OFF_L v_win_off[7:0] = 0x04 */
 	{CCI_REG8(0x3819), 0x01}, /* VSYNC_END_L vsync_end_point[7:0] = 0x01 */
-	{CCI_REG8(0x3821), 0x06}, /* TIMING_FORMAT2 array_h_mirror = 1, digital_h_mirror = 1 */
 
 	/* OTP control */
 	{CCI_REG8(0x3d85), 0x36}, /* OTP_REG85 OTP_power_up_load_setting_enable = 1,
@@ -605,6 +612,16 @@  static int ov4689_set_ctrl(struct v4l2_ctrl *ctrl)
 			  (val + ov4689->cur_mode->width) /
 			  OV4689_HTS_DIVIDER, &ret);
 		break;
+	case V4L2_CID_VFLIP:
+		cci_update_bits(regmap, OV4689_REG_TIMING_FORMAT1,
+				OV4689_TIMING_FLIP_MASK,
+				val ? OV4689_TIMING_FLIP_BOTH : 0, &ret);
+		break;
+	case V4L2_CID_HFLIP:
+		cci_update_bits(regmap, OV4689_REG_TIMING_FORMAT2,
+				OV4689_TIMING_FLIP_MASK,
+				val ? 0 : OV4689_TIMING_FLIP_BOTH, &ret);
+		break;
 	default:
 		dev_warn(dev, "%s Unhandled id:0x%x, val:0x%x\n",
 			 __func__, ctrl->id, val);
@@ -633,7 +650,7 @@  static int ov4689_initialize_controls(struct ov4689 *ov4689)
 
 	handler = &ov4689->ctrl_handler;
 	mode = ov4689->cur_mode;
-	ret = v4l2_ctrl_handler_init(handler, 11);
+	ret = v4l2_ctrl_handler_init(handler, 13);
 	if (ret)
 		return ret;
 
@@ -673,6 +690,9 @@  static int ov4689_initialize_controls(struct ov4689 *ov4689)
 				     ARRAY_SIZE(ov4689_test_pattern_menu) - 1,
 				     0, 0, ov4689_test_pattern_menu);
 
+	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);
+
 	if (handler->error) {
 		ret = handler->error;
 		dev_err(ov4689->dev, "Failed to init controls(%d)\n", ret);