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

[Mehdi Djait <mehdi.djait@linux.intel.com>]

Hi Tarang,

On Sun, Jul 19, 2026 at 01:38:57AM +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@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);

I don't know about the ret |= assignment

You can use the last parameter of the cci_*() helpers. That would make
this cleaner and not combine two error codes to produce something weird.

If you pass &ret to cci_write() the helper will check if an error
already occured and return it.

> +		break;
>  	default:
>  		ret = -EINVAL;
>  		break;

--
Kind Regards
Mehdi Djait