Re: [PATCH v2 2/3] media: i2c: og0ve1b: Introduce per-sensor data structure
From: Vladimir Zapolskiy
Date: Thu Jul 02 2026 - 09:25:32 EST
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@xxxxxxxxxxxxxxxx>
---
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)".
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.
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?
};
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.
+};
+
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.
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()?
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
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.
--
Best wishes,
Vladimir