diff mbox series

[v3,12/20] media: i2c: ov4689: Implement vflip/hflip controls

Message ID 20240229165333.227484-13-mike.rudenko@gmail.com (mailing list archive)
State New, archived
Headers show
Series Omnivision OV4689 refactoring and improvements | expand

Commit Message

Mikhail Rudenko Feb. 29, 2024, 4:53 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

Kieran Bingham March 4, 2024, 4:54 p.m. UTC | #1 
Quoting Mikhail Rudenko (2024-02-29 16:53:25)
> 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(-)
> 
> diff --git a/drivers/media/i2c/ov4689.c b/drivers/media/i2c/ov4689.c
> index 8283a9ac86c9..01ee8cadb7c9 100644
> --- a/drivers/media/i2c/ov4689.c
> +++ b/drivers/media/i2c/ov4689.c
> @@ -42,6 +42,14 @@
>  #define OV4689_REG_VTS                 CCI_REG16(0x380e)
>  #define OV4689_VTS_MAX                 0x7fff
>  
> +#define OV4689_REG_TIMING_FORMAT1      CCI_REG8(0x3820)
> +#define OV4689_REG_TIMING_FORMAT2      CCI_REG8(0x3821)

I assume these registerse are called TIMING_FORMAT1 and TIMING_FORMAT2,
and they don't have any named reference to H/V?

It's a shame, but I might add /* Horizontal */ and /* Vertical */
accordingly to these definitions, though not specifically required.


Looks like a good breakdown of the control though.

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> +#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_REG_TEST_PATTERN                CCI_REG8(0x5040)
>  #define OV4689_TEST_PATTERN_ENABLE     0x80
>  #define OV4689_TEST_PATTERN_DISABLE    0x0
> @@ -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,
> @@ -607,6 +614,16 @@ static int ov4689_set_ctrl(struct v4l2_ctrl *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,
> +                               ctrl->val ? OV4689_TIMING_FLIP_BOTH : 0, &ret);
> +               break;
> +       case V4L2_CID_HFLIP:
> +               cci_update_bits(regmap, OV4689_REG_TIMING_FORMAT2,
> +                               OV4689_TIMING_FLIP_MASK,
> +                               ctrl->val ? 0 : OV4689_TIMING_FLIP_BOTH, &ret);
> +               break;
>         default:
>                 dev_warn(dev, "%s Unhandled id:0x%x, val:0x%x\n",
>                          __func__, ctrl->id, ctrl->val);
> @@ -637,7 +654,7 @@ static int ov4689_initialize_controls(struct ov4689 *ov4689)
>  
>         handler = &ov4689->ctrl_handler;
>         mode = ov4689->cur_mode;
> -       ret = v4l2_ctrl_handler_init(handler, 10);
> +       ret = v4l2_ctrl_handler_init(handler, 12);
>         if (ret)
>                 return ret;
>  
> @@ -677,6 +694,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);
> -- 
> 2.43.0
>
Mikhail Rudenko March 4, 2024, 8:19 p.m. UTC | #2 
Hi Kieran,

and thanks for the review!

On 2024-03-04 at 16:54 GMT, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote:

> Quoting Mikhail Rudenko (2024-02-29 16:53:25)
>> 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(-)
>>
>> diff --git a/drivers/media/i2c/ov4689.c b/drivers/media/i2c/ov4689.c
>> index 8283a9ac86c9..01ee8cadb7c9 100644
>> --- a/drivers/media/i2c/ov4689.c
>> +++ b/drivers/media/i2c/ov4689.c
>> @@ -42,6 +42,14 @@
>>  #define OV4689_REG_VTS                 CCI_REG16(0x380e)
>>  #define OV4689_VTS_MAX                 0x7fff
>>
>> +#define OV4689_REG_TIMING_FORMAT1      CCI_REG8(0x3820)
>> +#define OV4689_REG_TIMING_FORMAT2      CCI_REG8(0x3821)
>
> I assume these registerse are called TIMING_FORMAT1 and TIMING_FORMAT2,
> and they don't have any named reference to H/V?

Exactly.

> It's a shame, but I might add /* Horizontal */ and /* Vertical */
> accordingly to these definitions, though not specifically required.

Thanks for the tip, will do it in v3.

> Looks like a good breakdown of the control though.
>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
>> +#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_REG_TEST_PATTERN                CCI_REG8(0x5040)
>>  #define OV4689_TEST_PATTERN_ENABLE     0x80
>>  #define OV4689_TEST_PATTERN_DISABLE    0x0
>> @@ -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,
>> @@ -607,6 +614,16 @@ static int ov4689_set_ctrl(struct v4l2_ctrl *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,
>> +                               ctrl->val ? OV4689_TIMING_FLIP_BOTH : 0, &ret);
>> +               break;
>> +       case V4L2_CID_HFLIP:
>> +               cci_update_bits(regmap, OV4689_REG_TIMING_FORMAT2,
>> +                               OV4689_TIMING_FLIP_MASK,
>> +                               ctrl->val ? 0 : OV4689_TIMING_FLIP_BOTH, &ret);
>> +               break;
>>         default:
>>                 dev_warn(dev, "%s Unhandled id:0x%x, val:0x%x\n",
>>                          __func__, ctrl->id, ctrl->val);
>> @@ -637,7 +654,7 @@ static int ov4689_initialize_controls(struct ov4689 *ov4689)
>>
>>         handler = &ov4689->ctrl_handler;
>>         mode = ov4689->cur_mode;
>> -       ret = v4l2_ctrl_handler_init(handler, 10);
>> +       ret = v4l2_ctrl_handler_init(handler, 12);
>>         if (ret)
>>                 return ret;
>>
>> @@ -677,6 +694,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);
>> --
>> 2.43.0
>>


--
Best regards,
Mikhail Rudenko
diff mbox series

Patch

diff --git a/drivers/media/i2c/ov4689.c b/drivers/media/i2c/ov4689.c
index 8283a9ac86c9..01ee8cadb7c9 100644
--- a/drivers/media/i2c/ov4689.c
+++ b/drivers/media/i2c/ov4689.c
@@ -42,6 +42,14 @@ 
 #define OV4689_REG_VTS			CCI_REG16(0x380e)
 #define OV4689_VTS_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_REG_TEST_PATTERN		CCI_REG8(0x5040)
 #define OV4689_TEST_PATTERN_ENABLE	0x80
 #define OV4689_TEST_PATTERN_DISABLE	0x0
@@ -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,
@@ -607,6 +614,16 @@  static int ov4689_set_ctrl(struct v4l2_ctrl *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,
+				ctrl->val ? OV4689_TIMING_FLIP_BOTH : 0, &ret);
+		break;
+	case V4L2_CID_HFLIP:
+		cci_update_bits(regmap, OV4689_REG_TIMING_FORMAT2,
+				OV4689_TIMING_FLIP_MASK,
+				ctrl->val ? 0 : OV4689_TIMING_FLIP_BOTH, &ret);
+		break;
 	default:
 		dev_warn(dev, "%s Unhandled id:0x%x, val:0x%x\n",
 			 __func__, ctrl->id, ctrl->val);
@@ -637,7 +654,7 @@  static int ov4689_initialize_controls(struct ov4689 *ov4689)
 
 	handler = &ov4689->ctrl_handler;
 	mode = ov4689->cur_mode;
-	ret = v4l2_ctrl_handler_init(handler, 10);
+	ret = v4l2_ctrl_handler_init(handler, 12);
 	if (ret)
 		return ret;
 
@@ -677,6 +694,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);