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

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

On 7/24/26 17:30, Tarang Raval wrote:
> 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@siliconsignals.io>
>>> ---
>>>    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.
> 

It could be so, but in os05b10_set_framing_limits() hblank/vblank types
are unsigned (and that's correct), so there is a likely unwanted
assigment of a negative value to an unsigned variable. It's better
to correct 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.
> 

Hopefully it's not a big disadvantage, a bit of complexity will be moved
to function body from the caller's side. Thank you!

-- 
Best wishes,
Vladimir