Re: [PATCH v3 08/17] media: i2c: os05b10: add 12-bit RAW mode support
From: Tarang Raval
Date: Fri Jul 24 2026 - 10:30:43 EST
Hi Vladimir,
Thanks for the review.
> On 7/18/26 23:08, Tarang Raval wrote:
> > Expose a 12-bit Bayer output option in the OS05B10 V4L2 sub-device driver.
> >
> > Add a 12-bit mode table alongside the existing 10-bit mode, extend the
> > enumerated mbus codes to include RAW12, and select the correct mode table
> > based on the requested mbus format in enum_frame_size and stream enable.
> >
> > Also move OS05B10_REG_MIPI_SC_CTRL_1 programming out of the common register
> > list and program it at stream-on depending on the selected mode bpp (10/12).
> >
> > Signed-off-by: Tarang Raval <tarang.raval@xxxxxxxxxxxxxxxxx>
> > ---
> > drivers/media/i2c/os05b10.c | 112 ++++++++++++++++++++++++++++++------
> > 1 file changed, 96 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/media/i2c/os05b10.c b/drivers/media/i2c/os05b10.c
> > index 4e177eacc815..e11a3c308299 100644
> > --- a/drivers/media/i2c/os05b10.c
> > +++ b/drivers/media/i2c/os05b10.c
> > @@ -146,7 +146,6 @@ static const struct cci_reg_sequence os05b10_common_regs[] = {
> > { CCI_REG8(0x301e), 0xb4 },
> > { CCI_REG8(0x301f), 0xd0 },
> > { CCI_REG8(0x3021), 0x03 },
> > - { OS05B10_REG_MIPI_SC_CTRL_1, 0x01 },
> > { CCI_REG8(0x3107), 0xa1 },
> > { CCI_REG8(0x3108), 0x7d },
> > { CCI_REG8(0x3109), 0xfc },
> > @@ -500,6 +499,21 @@ struct os05b10_mode {
> > struct os05b10_reg_list reg_list;
> > };
> >
> > +static const struct os05b10_mode supported_modes_12bit[] = {
> > + {
> > + .width = 2592,
> > + .height = 1944,
> > + .vts = 2007,
> > + .hts = 1744,
>
> It's unusual to see .hts < .width and .vts < .height. Will is cause
> errors in hblank/vblank computations in os05b10_set_framing_limits()?
Negative hblank is accepted by the framework, so no, I'm not seeing any
errors from it.
But you're right that it doesn't make logical/physical sense. I got a
similar comment from Jai on patch 15/17 about this.
I'll update it accordingly, the datasheet doesn't have any information
about the HTS register's unit/clock domain, so I'm doing some testing on
hardware first and will update this once I have concrete numbers.
> > + .exp = 1900,
> > + .bpp = 12,
> > + .reg_list = {
> > + .num_of_regs = ARRAY_SIZE(mode_2592_1944_regs),
> > + .regs = mode_2592_1944_regs,
> > + },
> > + },
> > +};
> > +
> > static const struct os05b10_mode supported_modes_10bit[] = {
> > {
> > .width = 2592,
> > @@ -521,6 +535,7 @@ static const s64 link_frequencies[] = {
> >
> > static const u32 os05b10_mbus_codes[] = {
> > MEDIA_BUS_FMT_SBGGR10_1X10,
> > + MEDIA_BUS_FMT_SBGGR12_1X12,
> > };
> >
> > static const char * const os05b10_test_pattern_menu[] = {
> > @@ -552,14 +567,20 @@ 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 u32 os05b10_get_format_code(struct os05b10 *os05b10, u8 bpp)
> > {
> > - 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, },
> > + static const u32 codes[2][2][2] = {
> > + { /* 10 bpp */
> > + { MEDIA_BUS_FMT_SBGGR10_1X10, MEDIA_BUS_FMT_SGBRG10_1X10 },
> > + { MEDIA_BUS_FMT_SGRBG10_1X10, MEDIA_BUS_FMT_SRGGB10_1X10 },
> > + },
> > + { /* 12 bpp */
> > + { MEDIA_BUS_FMT_SBGGR12_1X12, MEDIA_BUS_FMT_SGBRG12_1X12 },
> > + { MEDIA_BUS_FMT_SGRBG12_1X12, MEDIA_BUS_FMT_SRGGB12_1X12 },
> > + },
> > };
> >
> > - return codes[os05b10->vflip->val][os05b10->hflip->val];
> > + return codes[bpp == 12][os05b10->vflip->val][os05b10->hflip->val];
> > }
> >
> > static int os05b10_update_test_pattern(struct os05b10 *os05b10, u32 pattern)
> > @@ -571,6 +592,34 @@ static int os05b10_update_test_pattern(struct os05b10 *os05b10, u32 pattern)
> > os05b10_tp_val[pattern], NULL);
> > }
> >
> > +static int get_mode_table(struct os05b10 *os05b10, unsigned int code,
> > + const struct os05b10_mode **mode_list,
> > + unsigned int *num_modes)
> > +{
> > + switch (code) {
> > + case MEDIA_BUS_FMT_SBGGR12_1X12:
> > + case MEDIA_BUS_FMT_SGBRG12_1X12:
> > + case MEDIA_BUS_FMT_SGRBG12_1X12:
> > + case MEDIA_BUS_FMT_SRGGB12_1X12:
> > + *mode_list = supported_modes_12bit;
> > + *num_modes = ARRAY_SIZE(supported_modes_12bit);
> > + return 0;
> > +
> > + case MEDIA_BUS_FMT_SBGGR10_1X10:
> > + case MEDIA_BUS_FMT_SGBRG10_1X10:
> > + case MEDIA_BUS_FMT_SGRBG10_1X10:
> > + case MEDIA_BUS_FMT_SRGGB10_1X10:
> > + *mode_list = supported_modes_10bit;
> > + *num_modes = ARRAY_SIZE(supported_modes_10bit);
> > + return 0;
> > +
> > + default:
> > + dev_err(os05b10->dev,
> > + "Unsupported media bus format: %#x\n", code);
> > + return -EINVAL;
> > + }
> > +}
> > +
> > static int os05b10_set_ctrl(struct v4l2_ctrl *ctrl)
> > {
> > struct os05b10 *os05b10 = container_of_const(ctrl->handler,
> > @@ -650,8 +699,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);
>
> os05b10_mbus_codes[code->index] == MEDIA_BUS_FMT_SBGGR12_1X12 is more
> verbose, but seems to be a better and more reliable check.
>
> Probably a simple inline function to get bpp from code->index can be added.
>
> Another option is to change the second argument of os05b10_get_format_code()
> from bpp to just media bus format, so you can write
>
> os05b10_get_format_code(os05b10, os05b10_mbus_codes[code->index]);
Okay, I will update the os05b10_get_format_code 2nd argument.
Best Regards,
Tarang