Re: [PATCH v3 06/17] media: i2c: os05b10: Add H/V flip support

[Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>]

On 7/18/26 23:08, 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@siliconsignals.io>
> ---
>   drivers/media/i2c/os05b10.c | 56 ++++++++++++++++++++++++++++++++++---
>   1 file changed, 52 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/i2c/os05b10.c b/drivers/media/i2c/os05b10.c
> index abf3d2970900..61c51844a91e 100644
> --- a/drivers/media/i2c/os05b10.c
> +++ b/drivers/media/i2c/os05b10.c
> @@ -96,6 +96,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
> +
>   #define OS05B10_REG_FORMAT2		CCI_REG8(0x3821)
>   
>   #define OS05B10_LINK_FREQ_600MHZ	(600 * HZ_PER_MHZ)
> @@ -231,7 +235,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 },
> @@ -465,6 +468,8 @@ struct os05b10 {
>   	struct v4l2_ctrl *vblank;
>   	struct v4l2_ctrl *gain;
>   	struct v4l2_ctrl *exposure;
> +	struct v4l2_ctrl *vflip;
> +	struct v4l2_ctrl *hflip;
>   
>   	u32 link_freq_index;
>   	u32 data_lanes;
> @@ -513,6 +518,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];
> +}
> +
>   static int os05b10_set_ctrl(struct v4l2_ctrl *ctrl)
>   {
>   	struct os05b10 *os05b10 = container_of_const(ctrl->handler,
> @@ -556,6 +571,20 @@ 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 = cci_update_bits(os05b10->cci, OS05B10_REG_FORMAT1,
> +				      OS05B10_MIRROR | OS05B10_FLIP,
> +				      (!os05b10->hflip->val ?
> +				       OS05B10_MIRROR : 0) |
> +				      (os05b10->vflip->val ?
> +				       OS05B10_FLIP : 0), NULL);
> +
> +		ret |= cci_write(os05b10->cci, OS05B10_REG_ANALOG_FLIP,
> +				 (os05b10->vflip->val == 1) ?
> +				 OS05B10_FLIP_ENABLE : OS05B10_FLIP_DISABLE,
> +				 NULL);

Above please follow the review comment given by Mehdi.

It's expected to see

cci_update_bits(<...>, &ret);
cci_write(<...>, &ret);

> +		break;
>   	default:
>   		ret = -EINVAL;
>   		break;
> @@ -570,10 +599,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);
>   
>   	return 0;
>   }
> @@ -712,6 +743,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:
> @@ -732,6 +766,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(os05b10->dev);
>   
>   	return 0;
> @@ -740,6 +777,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;
>   
> @@ -747,7 +785,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;
> @@ -928,7 +966,7 @@ static int os05b10_init_controls(struct os05b10 *os05b10)
>   	int 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,
> @@ -974,6 +1012,16 @@ 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);
> +	if (os05b10->hflip)
> +		os05b10->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> +
> +	os05b10->vflip = v4l2_ctrl_new_std(ctrl_hdlr, &os05b10_ctrl_ops,
> +					   V4L2_CID_VFLIP, 0, 1, 1, 0);
> +	if (os05b10->vflip)
> +		os05b10->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> +

As a micro optimization againt the error path, setting of these flags can be
done at the end of the function, for instance check how it's done in ov2680.c

>   	if (ctrl_hdlr->error) {
>   		ret = ctrl_hdlr->error;
>   		dev_err(os05b10->dev, "control init failed (%d)\n", ret);

-- 
Best wishes,
Vladimir