Re: [PATCH v2 2/3] media: i2c: og0ve1b: Introduce per-sensor data structure

[Wenmeng Liu <wenmeng.liu@oss.qualcomm.com>]

Hi Vladimir,

On 7/2/2026 9:10 PM, Vladimir Zapolskiy wrote:
> Hi Wenmeng.
> 
> On 7/2/26 13:52, Wenmeng Liu wrote:
>> In preparation for supporting further OmniVision sensors that share most
>> of this driver, move the sensor-specific parameters (chip id, MCLK
>> frequency, test pattern register, link frequency menu and the list of
>> supported modes) into a new struct og0ve1b_sensor_data, selected through
>> i2c_get_match_data() at probe time.
>>
>> Signed-off-by: Wenmeng Liu <wenmeng.liu@oss.qualcomm.com>
>> ---
>>   drivers/media/i2c/og0ve1b.c | 101 ++++++++++++++++++++++++++++ 
>> +---------------
>>   1 file changed, 67 insertions(+), 34 deletions(-)
>>
>> diff --git a/drivers/media/i2c/og0ve1b.c b/drivers/media/i2c/og0ve1b.c
>> index 
>> 84a28cdcade10f8fbcf945999e88f84641b9bc0d..acc06b10bf896f734926289099a70fbc2bb628d5 100644
>> --- a/drivers/media/i2c/og0ve1b.c
>> +++ b/drivers/media/i2c/og0ve1b.c
>> @@ -66,10 +66,21 @@ struct og0ve1b_mode {
>>       u32 hts;    /* Horizontal timing size */
>>       u32 vts;    /* Default vertical timing size */
>>       u32 bpp;    /* Bits per pixel */
>> +    u32 code;    /* MEDIA_BUS_FMT code */
> 
> In this particular case of grayscale only sensors there is no need for both
> bpp and media bus format code, since there is a natural one-to-one mapping.
> 
> I'm fine with "code" just replacing "bpp", and in og0ve1b_init_controls()
> simply replace "bpp" with "(code == MEDIA_BUS_FMT_Y8_1X8 ? 8 : 10)".
> 
ACK.

>>       const struct og0ve1b_reg_list reg_list;    /* Sensor register 
>> setting */
>>   };
>> +struct og0ve1b_sensor_data {
>> +    u64 chip_id;
>> +    unsigned long mclk_freq;
>> +    u32 test_pattern_reg;
>> +    const s64 *link_freq_menu;
>> +    int num_link_freqs;
>> +    const struct og0ve1b_mode *modes;
>> +    int num_modes;
>> +};
>> +
>>   static const char * const og0ve1b_test_pattern_menu[] = {
>>       "Disabled",
>>       "Vertical Colour Bars",
>> @@ -97,8 +108,7 @@ struct og0ve1b {
>>       struct v4l2_ctrl *exposure;
>>       struct v4l2_ctrl_handler ctrl_handler;
>> -    /* Saved register value */
>> -    u64 pre_isp;
> 
> I'm afraid this will break test pattern on og0ve1b, unfortunately it should
> be kept as is for now.
> 
> The register is used for something else, and IIRC even cci_update_bits() 
> API
> does not work expectedly, it certainly should be read once and written 
> later.
> 
Will keep pre_isp code for og0ve1b.

> Hence it leaves an option to keep og0ve1b_enable_test_pattern() and then
> introduce in 3/3 a new og0va1b_enable_test_pattern() function, the 
> selection
> will be done in runtime similarly to other og0ve/og0va branches.
> 
>> +    const struct og0ve1b_sensor_data *sensor;
> 
> Well, the identifier name is confusing to me, it is not "sensor", it's
> "sensor data" or just "data", no?
> 
ACK.

>>   };
>>   static const struct cci_reg_sequence og0ve1b_640x480_120fps_mode[] = {
>> @@ -254,6 +264,7 @@ static const struct og0ve1b_mode supported_modes[] 
>> = {
>>           .hts = 792,
>>           .vts = 568,
>>           .bpp = 8,
>> +        .code = MEDIA_BUS_FMT_Y8_1X8,
>>           .reg_list = {
>>               .regs = og0ve1b_640x480_120fps_mode,
>>               .num_regs = ARRAY_SIZE(og0ve1b_640x480_120fps_mode),
>> @@ -261,23 +272,39 @@ static const struct og0ve1b_mode 
>> supported_modes[] = {
>>       },
>>   };
>> +static const struct og0ve1b_sensor_data og0ve1b_data = {
>> +    .chip_id    = OG0VE1B_CHIP_ID,
>> +    .mclk_freq    = OG0VE1B_MCLK_FREQ_24MHZ,
>> +    .test_pattern_reg = OG0VE1B_REG_PRE_ISP,
>> +    .link_freq_menu    = og0ve1b_link_freq_menu,
>> +    .num_link_freqs    = ARRAY_SIZE(og0ve1b_link_freq_menu),
>> +    .modes        = supported_modes,
>> +    .num_modes    = ARRAY_SIZE(supported_modes),
> 
> Please use tab symbols or spaces before '=' sign consistently on all lines.
> 
ACK.

>> +};
>> +
>>   static int og0ve1b_enable_test_pattern(struct og0ve1b *og0ve1b, u32 
>> pattern)
>>   {
>> -    u64 val = og0ve1b->pre_isp;
>> +    u32 reg = og0ve1b->sensor->test_pattern_reg;
>> +    u64 val;
>> +    int ret;
>> +
>> +    ret = cci_read(og0ve1b->regmap, reg, &val, NULL);
>> +    if (ret)
>> +        return ret;
>>       if (pattern)
>>           val |= OG0VE1B_TEST_PATTERN_ENABLE;
>>       else
>>           val &= ~OG0VE1B_TEST_PATTERN_ENABLE;
>> -    return cci_write(og0ve1b->regmap, OG0VE1B_REG_PRE_ISP, val, NULL);
>> +    return cci_write(og0ve1b->regmap, reg, val, NULL);
>>   }
> 
> So far let's keep the function above unmodified, but call it by pointer 
> stored
> in the new struct.
> 
> Does OG0VA have also just one "Vertical Colour Bars" test pattern mode?
> 
> Let me test this v2 for test pattern regression to formally confirm it 
> shortly.
> 

For og0va1b, tpg has 4 types:
	"Standard Color Bar",
	"Top-Bottom Darker Color Bar",
	"Right-Left Darker Color Bar",
	"Bottom-Top Darker Color Bar",
will fix it in next version.

>>   static int og0ve1b_set_ctrl(struct v4l2_ctrl *ctrl)
>>   {
>>       struct og0ve1b *og0ve1b = container_of(ctrl->handler, struct 
>> og0ve1b,
>>                              ctrl_handler);
>> -    const struct og0ve1b_mode *mode = &supported_modes[0];
>> +    const struct og0ve1b_mode *mode = &og0ve1b->sensor->modes[0];
>>       s64 exposure_max;
>>       int ret;
>> @@ -333,7 +360,8 @@ static const struct v4l2_ctrl_ops og0ve1b_ctrl_ops 
>> = {
>>   static int og0ve1b_init_controls(struct og0ve1b *og0ve1b)
>>   {
>>       struct v4l2_ctrl_handler *ctrl_hdlr = &og0ve1b->ctrl_handler;
>> -    const struct og0ve1b_mode *mode = &supported_modes[0];
>> +    const struct og0ve1b_mode *mode = &og0ve1b->sensor->modes[0];
>> +    const struct og0ve1b_sensor_data *sensor = og0ve1b->sensor;
>>       s64 exposure_max, pixel_rate, h_blank, v_blank;
>>       struct v4l2_fwnode_device_properties props;
>>       struct v4l2_ctrl *ctrl;
>> @@ -343,12 +371,12 @@ static int og0ve1b_init_controls(struct og0ve1b 
>> *og0ve1b)
>>       ctrl = v4l2_ctrl_new_int_menu(ctrl_hdlr, &og0ve1b_ctrl_ops,
>>                         V4L2_CID_LINK_FREQ,
>> -                      ARRAY_SIZE(og0ve1b_link_freq_menu) - 1,
>> -                      0, og0ve1b_link_freq_menu);
>> +                      sensor->num_link_freqs - 1,
>> +                      0, sensor->link_freq_menu);
>>       if (ctrl)
>>           ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>> -    pixel_rate = og0ve1b_link_freq_menu[0] / mode->bpp;
>> +    pixel_rate = sensor->link_freq_menu[0] / mode->bpp;
> 
> Since it becomes more complex, can you please move the calculation to
> a new inline function, like os05b10_pixel_rate()?
> 

ACK.

>>       v4l2_ctrl_new_std(ctrl_hdlr, &og0ve1b_ctrl_ops, 
>> V4L2_CID_PIXEL_RATE,
>>                 0, pixel_rate, 1, pixel_rate);
>> @@ -407,7 +435,7 @@ static int og0ve1b_init_controls(struct og0ve1b 
>> *og0ve1b)
>>   static void og0ve1b_update_pad_format(const struct og0ve1b_mode *mode,
>>                         struct v4l2_mbus_framefmt *fmt)
>>   {
>> -    fmt->code = MEDIA_BUS_FMT_Y8_1X8;
>> +    fmt->code = mode->code;
>>       fmt->width = mode->width;
>>       fmt->height = mode->height;
>>       fmt->field = V4L2_FIELD_NONE;
>> @@ -421,8 +449,8 @@ static int og0ve1b_enable_streams(struct 
>> v4l2_subdev *sd,
>>                     struct v4l2_subdev_state *state, u32 pad,
>>                     u64 streams_mask)
>>   {
>> -    const struct og0ve1b_reg_list *reg_list = 
>> &supported_modes[0].reg_list;
>>       struct og0ve1b *og0ve1b = to_og0ve1b(sd);
>> +    const struct og0ve1b_reg_list *reg_list = &og0ve1b->sensor- 
>> >modes[0].reg_list;
>>       int ret;
>>       ret = pm_runtime_resume_and_get(og0ve1b->dev);
>> @@ -484,13 +512,14 @@ static int og0ve1b_set_pad_format(struct 
>> v4l2_subdev *sd,
>>                     struct v4l2_subdev_state *state,
>>                     struct v4l2_subdev_format *fmt)
>>   {
>> +    struct og0ve1b *og0ve1b = to_og0ve1b(sd);
>>       struct v4l2_mbus_framefmt *format;
>>       const struct og0ve1b_mode *mode;
>>       format = v4l2_subdev_state_get_format(state, 0);
>> -    mode = v4l2_find_nearest_size(supported_modes,
>> -                      ARRAY_SIZE(supported_modes),
>> +    mode = v4l2_find_nearest_size(og0ve1b->sensor->modes,
>> +                      og0ve1b->sensor->num_modes,
>>                         width, height,
>>                         fmt->format.width,
>>                         fmt->format.height);
>> @@ -505,10 +534,12 @@ static int og0ve1b_enum_mbus_code(struct 
>> v4l2_subdev *sd,
>>                     struct v4l2_subdev_state *sd_state,
>>                     struct v4l2_subdev_mbus_code_enum *code)
>>   {
>> +    struct og0ve1b *og0ve1b = to_og0ve1b(sd);
>> +
>>       if (code->index > 0)
>>           return -EINVAL;
>> -    code->code = MEDIA_BUS_FMT_Y8_1X8;
>> +    code->code = og0ve1b->sensor->modes[0].code;
>>       return 0;
>>   }
>> @@ -517,15 +548,18 @@ static int og0ve1b_enum_frame_size(struct 
>> v4l2_subdev *sd,
>>                      struct v4l2_subdev_state *sd_state,
>>                      struct v4l2_subdev_frame_size_enum *fse)
>>   {
>> -    if (fse->index >= ARRAY_SIZE(supported_modes))
>> +    struct og0ve1b *og0ve1b = to_og0ve1b(sd);
>> +    const struct og0ve1b_sensor_data *sensor = og0ve1b->sensor;
>> +
>> +    if (fse->index >= sensor->num_modes)
>>           return -EINVAL;
>> -    if (fse->code != MEDIA_BUS_FMT_Y8_1X8)
>> +    if (fse->code != sensor->modes[fse->index].code)
>>           return -EINVAL;
>> -    fse->min_width = supported_modes[fse->index].width;
>> +    fse->min_width = sensor->modes[fse->index].width;
>>       fse->max_width = fse->min_width;
>> -    fse->min_height = supported_modes[fse->index].height;
>> +    fse->min_height = sensor->modes[fse->index].height;
>>       fse->max_height = fse->min_height;
>>       return 0;
>> @@ -534,13 +568,14 @@ static int og0ve1b_enum_frame_size(struct 
>> v4l2_subdev *sd,
>>   static int og0ve1b_init_state(struct v4l2_subdev *sd,
>>                     struct v4l2_subdev_state *state)
>>   {
>> +    struct og0ve1b *og0ve1b = to_og0ve1b(sd);
> 
> struct og0ve1b_sensor_data *data = to_og0ve1b(sd)->sensor; // ->data 
> anticipated
> 
ACK.

>>       struct v4l2_subdev_format fmt = {
>>           .which = V4L2_SUBDEV_FORMAT_TRY,
>>           .pad = 0,
>>           .format = {
>> -            .code = MEDIA_BUS_FMT_Y8_1X8,
>> -            .width = supported_modes[0].width,
>> -            .height = supported_modes[0].height,
>> +            .code = og0ve1b->sensor->modes[0].code,
>> +            .width = og0ve1b->sensor->modes[0].width,
>> +            .height = og0ve1b->sensor->modes[0].height,
>>           },
>>       };
>> @@ -586,18 +621,13 @@ static int og0ve1b_identify_sensor(struct 
>> og0ve1b *og0ve1b)
>>           return ret;
>>       }
>> -    if (val != OG0VE1B_CHIP_ID) {
>> -        dev_err(og0ve1b->dev, "chip id mismatch: %x!=%llx\n",
>> -            OG0VE1B_CHIP_ID, val);
>> +    if (val != og0ve1b->sensor->chip_id) {
>> +        dev_err(og0ve1b->dev, "chip id mismatch: %llx!=%llx\n",
>> +            og0ve1b->sensor->chip_id, val);
>>           return -ENODEV;
>>       }
>> -    ret = cci_read(og0ve1b->regmap, OG0VE1B_REG_PRE_ISP,
>> -               &og0ve1b->pre_isp, NULL);
>> -    if (ret)
>> -        dev_err(og0ve1b->dev, "failed to read pre_isp: %d\n", ret);
>> -
>> -    return ret;
>> +    return 0;
>>   }
>>   static int og0ve1b_check_hwcfg(struct og0ve1b *og0ve1b)
>> @@ -624,8 +654,8 @@ static int og0ve1b_check_hwcfg(struct og0ve1b 
>> *og0ve1b)
>>       ret = v4l2_link_freq_to_bitmap(og0ve1b->dev,
>>                          bus_cfg.link_frequencies,
>>                          bus_cfg.nr_of_link_frequencies,
>> -                       og0ve1b_link_freq_menu,
>> -                       ARRAY_SIZE(og0ve1b_link_freq_menu),
>> +                       og0ve1b->sensor->link_freq_menu,
>> +                       og0ve1b->sensor->num_link_freqs,
>>                          &freq_bitmap);
>>       v4l2_fwnode_endpoint_free(&bus_cfg);
>> @@ -686,6 +716,9 @@ static int og0ve1b_probe(struct i2c_client *client)
>>           return -ENOMEM;
>>       og0ve1b->dev = &client->dev;
>> +    og0ve1b->sensor = i2c_get_match_data(client);
>> +    if (!og0ve1b->sensor)
>> +        return -ENODEV;
>>       v4l2_i2c_subdev_init(&og0ve1b->sd, client, &og0ve1b_subdev_ops);
>> @@ -700,7 +733,7 @@ static int og0ve1b_probe(struct i2c_client *client)
>>                        "failed to get XVCLK clock\n");
>>       freq = clk_get_rate(og0ve1b->xvclk);
>> -    if (freq && freq != OG0VE1B_MCLK_FREQ_24MHZ)
>> +    if (freq && freq != og0ve1b->sensor->mclk_freq)
>>           return dev_err_probe(og0ve1b->dev, -EINVAL,
>>                        "XVCLK clock frequency %lu is not supported\n",
>>                        freq);
>> @@ -819,7 +852,7 @@ static const struct dev_pm_ops og0ve1b_pm_ops = {
>>   };
>>   static const struct of_device_id og0ve1b_of_match[] = {
>> -    { .compatible = "ovti,og0ve1b" },
>> +    { .compatible = "ovti,og0ve1b", .data = &og0ve1b_data },
>>       { /* sentinel */ }
>>   };
>>   MODULE_DEVICE_TABLE(of, og0ve1b_of_match);
>>
> 
> Looks good overall, thank you.
> 

Thanks,
Wenmeng