Re: [PATCH v5 09/15] media: i2c: os05b10: Add H/V flip support
From: Sakari Ailus
Date: Mon Nov 02 2026 - 08:13:53 EST
Hi Tarang,
On Fri, Oct 30, 2026 at 04:17:58PM +0530, Tarang Raval wrote:
> Add HFLIP and VFLIP controls, lock them while streaming, and update
> the reported Bayer format based on the flip state.
>
> Signed-off-by: Tarang Raval <tarang.raval@xxxxxxxxxxxxxxxxx>
> ---
> drivers/media/i2c/os05b10.c | 59 ++++++++++++++++++++++++++++++++++---
> 1 file changed, 55 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/i2c/os05b10.c b/drivers/media/i2c/os05b10.c
> index 14046f1c9324..4b87ef60a2a7 100644
> --- a/drivers/media/i2c/os05b10.c
> +++ b/drivers/media/i2c/os05b10.c
> @@ -92,6 +92,10 @@
> #define OS05B10_MIRROR BIT(3)
> #define OS05B10_FLIP GENMASK(5, 4)
>
> +#define OS05B10_REG_ANALOG_FLIP CCI_REG8(0x3716)
> +#define OS05B10_FLIP_ENABLE 0x04
> +#define OS05B10_FLIP_DISABLE 0x24
I'd use the register name as prefix here, omitting "_REG".
0x4 has nothing to do with flip configuration, so I'd just use another
macro for that, e.g. OS05B10_ANALOG_FLIP_MAGIC, unless we know more about
that bit.
> +
> #define OS05B10_REG_FORMAT2 CCI_REG8(0x3821)
>
> #define OS05B10_LINK_FREQ_600MHZ (600 * HZ_PER_MHZ)
> @@ -227,7 +231,6 @@ static const struct cci_reg_sequence os05b10_common_regs[] = {
> { CCI_REG8(0x370f), 0x1c },
> { CCI_REG8(0x3710), 0x00 },
> { CCI_REG8(0x3713), 0x00 },
> - { CCI_REG8(0x3716), 0x24 },
> { CCI_REG8(0x371a), 0x1e },
> { CCI_REG8(0x3724), 0x09 },
> { CCI_REG8(0x3725), 0xb2 },
> @@ -459,6 +462,8 @@ struct os05b10 {
> struct v4l2_ctrl *hblank;
> struct v4l2_ctrl *vblank;
> struct v4l2_ctrl *exposure;
> + struct v4l2_ctrl *vflip;
> + struct v4l2_ctrl *hflip;
>
> u32 link_freq_index;
> u32 data_lanes;
> @@ -507,6 +512,16 @@ static inline struct os05b10 *to_os05b10(struct v4l2_subdev *sd)
> return container_of_const(sd, struct os05b10, sd);
> };
>
> +static u32 os05b10_get_format_code(struct os05b10 *os05b10)
> +{
> + static const u32 codes[2][2] = {
> + { MEDIA_BUS_FMT_SBGGR10_1X10, MEDIA_BUS_FMT_SGBRG10_1X10, },
> + { MEDIA_BUS_FMT_SGRBG10_1X10, MEDIA_BUS_FMT_SRGGB10_1X10, },
> + };
> +
> + return codes[os05b10->vflip->val][os05b10->hflip->val];
Note that accessing the control values requires holding the handler's mutex
here. That's not the case when called from enum_mbus_code() I think.
> +}
> +
> static int os05b10_set_ctrl(struct v4l2_ctrl *ctrl)
> {
> struct os05b10 *os05b10 = container_of_const(ctrl->handler,
> @@ -550,6 +565,25 @@ static int os05b10_set_ctrl(struct v4l2_ctrl *ctrl)
> ret = cci_write(os05b10->cci, OS05B10_REG_EXPOSURE,
> ctrl->val, NULL);
> break;
> + case V4L2_CID_HFLIP:
> + case V4L2_CID_VFLIP:
> + ret = 0;
> + cci_update_bits(os05b10->cci, OS05B10_REG_FORMAT1,
> + OS05B10_MIRROR | OS05B10_FLIP,
> + (!os05b10->hflip->val ?
> + OS05B10_MIRROR : 0) |
> + (os05b10->vflip->val ?
> + OS05B10_FLIP : 0), &ret);
> +
> + cci_write(os05b10->cci, OS05B10_REG_ANALOG_FLIP,
> + (os05b10->vflip->val == 1) ?
> + OS05B10_FLIP_ENABLE : OS05B10_FLIP_DISABLE,
> + &ret);
Indentation. The last line can also be unwrapped.
> + if (ret)
> + break;
> +
> + fmt->code = os05b10_get_format_code(os05b10);
> + break;
> default:
> ret = -EINVAL;
> break;
> @@ -564,10 +598,12 @@ static int os05b10_enum_mbus_code(struct v4l2_subdev *sd,
> struct v4l2_subdev_state *sd_state,
> struct v4l2_subdev_mbus_code_enum *code)
> {
> + struct os05b10 *os05b10 = to_os05b10(sd);
> +
> if (code->index >= ARRAY_SIZE(os05b10_mbus_codes))
> return -EINVAL;
>
> - code->code = os05b10_mbus_codes[code->index];
> + code->code = os05b10_get_format_code(os05b10);
If this is the case, then any non-zero value of code->index should yield
-EINVAL.
>
> return 0;
> }
> @@ -708,6 +744,9 @@ static int os05b10_enable_streams(struct v4l2_subdev *sd,
> if (ret)
> goto err_rpm_put;
>
> + __v4l2_ctrl_grab(os05b10->vflip, true);
> + __v4l2_ctrl_grab(os05b10->hflip, true);
> +
> return 0;
>
> err_rpm_put:
> @@ -728,6 +767,9 @@ static int os05b10_disable_streams(struct v4l2_subdev *sd,
> if (ret)
> dev_err(os05b10->dev, "failed to set stream off\n");
>
> + __v4l2_ctrl_grab(os05b10->vflip, false);
> + __v4l2_ctrl_grab(os05b10->hflip, false);
> +
> pm_runtime_put_autosuspend(os05b10->dev);
>
> return 0;
> @@ -736,6 +778,7 @@ static int os05b10_disable_streams(struct v4l2_subdev *sd,
> static int os05b10_init_state(struct v4l2_subdev *sd,
> struct v4l2_subdev_state *state)
> {
> + struct os05b10 *os05b10 = to_os05b10(sd);
> struct v4l2_mbus_framefmt *format;
> const struct os05b10_mode *mode;
>
> @@ -743,7 +786,7 @@ static int os05b10_init_state(struct v4l2_subdev *sd,
> format = v4l2_subdev_state_get_format(state, 0);
>
> mode = &supported_modes_10bit[0];
> - format->code = MEDIA_BUS_FMT_SBGGR10_1X10;
> + format->code = os05b10_get_format_code(os05b10);
>
> /* Update image pad formate */
> format->width = mode->width;
> @@ -929,7 +972,7 @@ static int os05b10_init_controls(struct os05b10 *os05b10)
> return ret;
>
> ctrl_hdlr = &os05b10->handler;
> - v4l2_ctrl_handler_init(ctrl_hdlr, 9);
> + v4l2_ctrl_handler_init(ctrl_hdlr, 11);
>
> pixel_rate = os05b10_pixel_rate(os05b10, mode);
> v4l2_ctrl_new_std(ctrl_hdlr, &os05b10_ctrl_ops, V4L2_CID_PIXEL_RATE,
> @@ -967,6 +1010,12 @@ static int os05b10_init_controls(struct os05b10 *os05b10)
> OS05B10_DIGITAL_GAIN_MIN, OS05B10_DIGITAL_GAIN_MAX,
> OS05B10_DIGITAL_GAIN_STEP, OS05B10_DIGITAL_GAIN_DEFAULT);
>
> + os05b10->hflip = v4l2_ctrl_new_std(ctrl_hdlr, &os05b10_ctrl_ops,
> + V4L2_CID_HFLIP, 0, 1, 1, 0);
> +
> + os05b10->vflip = v4l2_ctrl_new_std(ctrl_hdlr, &os05b10_ctrl_ops,
> + V4L2_CID_VFLIP, 0, 1, 1, 0);
> +
> ret = v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &os05b10_ctrl_ops,
> &props);
> if (ret) {
> @@ -976,6 +1025,8 @@ static int os05b10_init_controls(struct os05b10 *os05b10)
>
> link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> os05b10->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> + os05b10->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> + os05b10->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
>
> os05b10->sd.ctrl_handler = ctrl_hdlr;
>
--
Regards,
Sakari Ailus