Re: [PATCH v2 08/15] media: i2c: os05b10: add 12-bit RAW mode support

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

Hi Tarang,

On Wed, Mar 25, 2026 at 05:13:54PM +0530, Tarang Raval wrote:

[..]

> -static u32 os05b10_get_format_code(struct os05b10 *os05b10)
> +static u32 os05b10_get_format_code(struct os05b10 *os05b10, u8 bpp)
>  {
> -	static const u32 codes[2][2] = {
> +	static const u32 codes_12[2][2] = {
> +		{ MEDIA_BUS_FMT_SBGGR12_1X12, MEDIA_BUS_FMT_SGBRG12_1X12, },
> +		{ MEDIA_BUS_FMT_SGRBG12_1X12, MEDIA_BUS_FMT_SRGGB12_1X12, },
> +	};
> +
> +	static const u32 codes_10[2][2] = {
>  		{ MEDIA_BUS_FMT_SBGGR10_1X10, MEDIA_BUS_FMT_SGBRG10_1X10, },
>  		{ MEDIA_BUS_FMT_SGRBG10_1X10, MEDIA_BUS_FMT_SRGGB10_1X10, },
>  	};
>  
> +	const u32 (*codes)[2] = (bpp == 12) ? codes_12 : codes_10;
> +

I don't know about the code above..

How about one array:

        static const u32 codes[2][2][2] = {
                {
                        { MEDIA_BUS_FMT_SBGGR10_1X10, MEDIA_BUS_FMT_SGBRG10_1X10, },
                        { MEDIA_BUS_FMT_SGRBG10_1X10, MEDIA_BUS_FMT_SRGGB10_1X10, },
                }, {
                        { MEDIA_BUS_FMT_SBGGR12_1X12, MEDIA_BUS_FMT_SGBRG12_1X12, },
                        { MEDIA_BUS_FMT_SGRBG12_1X12, MEDIA_BUS_FMT_SRGGB12_1X12, },
                },
        };

And the first index will be dependent on the bpp == 12 or not.

or
        static const u32 codes[4][2];

and have an offset of 2. I think the first is cleaner.

>  	u32 code = codes[os05b10->vflip->val][os05b10->hflip->val];
>  
>  	return code;
> @@ -654,8 +676,8 @@ static int os05b10_enum_mbus_code(struct v4l2_subdev *sd,
>  	if (code->index >= ARRAY_SIZE(os05b10_mbus_codes))
>  		return -EINVAL;
>  
> -	code->code = os05b10_get_format_code(os05b10);
> -
> +	code->code = os05b10_get_format_code(os05b10,
> +					     (code->index == 1) ? 12 : 10);
>  	return 0;
>  }
>  
> @@ -684,15 +706,42 @@ static int os05b10_set_framing_limits(struct os05b10 *os05b10,
>  					OS05B10_EXPOSURE_STEP, mode->exp);
>  }
>  
> +static inline void get_mode_table(unsigned int code,
> +				  const struct os05b10_mode **mode_list,
> +				  unsigned int *num_modes)
> +{
> +	switch (code) {
> +	case MEDIA_BUS_FMT_SBGGR12_1X12:
> +		*mode_list = supported_modes_12bit;
> +		*num_modes = ARRAY_SIZE(supported_modes_12bit);
> +		break;
> +
> +	case MEDIA_BUS_FMT_SBGGR10_1X10:
> +		*mode_list = supported_modes_10bit;
> +		*num_modes = ARRAY_SIZE(supported_modes_10bit);
> +		break;
> +	default:
> +		*mode_list = NULL;
> +		*num_modes = 0;
> +		break;
> +	}
> +}
> +
>  static int os05b10_set_pad_format(struct v4l2_subdev *sd,
>  				  struct v4l2_subdev_state *sd_state,
>  				  struct v4l2_subdev_format *fmt)
>  {
> -	const struct os05b10_mode *mode = &supported_modes_10bit[0];
>  	struct os05b10 *os05b10 = to_os05b10(sd);
> +	const struct os05b10_mode *mode_list;
>  	struct v4l2_mbus_framefmt *format;
> +	const struct os05b10_mode *mode;
> +	unsigned int num_modes;
>  	int ret;
>  
> +	get_mode_table(fmt->format.code, &mode_list, &num_modes);
> +	mode = v4l2_find_nearest_size(mode_list, num_modes, width, height,
> +				      fmt->format.width, fmt->format.height);
> +

What would happen here if get_mode_table returns NULL and 0 for
mode_list and num_modes ?

>  	fmt->format.width = mode->width;

Will this cause a kernel panic ?

>  	fmt->format.height = mode->height;
>  	fmt->format.field = V4L2_FIELD_NONE;
> @@ -735,12 +784,17 @@ static int os05b10_enum_frame_size(struct v4l2_subdev *sd,
>  				   struct v4l2_subdev_state *sd_state,
>  				   struct v4l2_subdev_frame_size_enum *fse)
> +	const struct os05b10_mode *mode_list;
> +	unsigned int num_modes;
> +
> +	get_mode_table(fse->code, &mode_list, &num_modes);
> +
> +	if (fse->index >= num_modes)
>  		return -EINVAL;
>  
> -	fse->min_width = supported_modes_10bit[fse->index].width;
> +	fse->min_width = mode_list[fse->index].width;
>  	fse->max_width = fse->min_width;
> -	fse->min_height = supported_modes_10bit[fse->index].height;
> +	fse->min_height = mode_list[fse->index].height;
>  	fse->max_height = fse->min_height;
>  
>  	return 0;

--
Kind Regards
Mehdi Djait