From: Bryan O'Donoghue <bod@kernel.org>
To: Wenmeng Liu <wenmeng.liu@oss.qualcomm.com>,
Mauro Carvalho Chehab <mchehab@kernel.org>,
Rob Herring <robh@kernel.org>,
Krzysztof Kozlowski <krzk+dt@kernel.org>,
Conor Dooley <conor+dt@kernel.org>,
Sakari Ailus <sakari.ailus@linux.intel.com>,
Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
Cc: linux-media@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 2/3] media: i2c: og0ve1b: Introduce per-sensor data structure
Date: Thu, 2 Jul 2026 17:31:04 +0100 [thread overview]
Message-ID: <5019684c-ace6-4202-97ca-98016df77d86@kernel.org> (raw)
In-Reply-To: <20260702-og0va1b-v2-2-0071442caa2a@oss.qualcomm.com>
On 02/07/2026 11: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 */
>
> 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;
> + const struct og0ve1b_sensor_data *sensor;
> };
>
> 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),
> +};
> +
> 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);
> }
>
> 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;
> 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 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;
The one thing that is not immediately clear to me is if we get
equivalent logic WRT OG0VE1B_REG_PRE_ISP after this change.
> }
>
> 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);
>
> --
> 2.34.1
>
>
Other than that LTGM.
---
bod
next prev parent reply other threads:[~2026-07-02 16:31 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-07-02 10:52 [PATCH v2 0/3] media: i2c: Add OmniVision OG0VA1B camera sensor driver Wenmeng Liu
2026-07-02 10:52 ` [PATCH v2 1/3] dt-bindings: media: i2c: og0ve1b: Add OmniVision OG0VA1B camera sensor Wenmeng Liu
2026-07-02 12:24 ` Vladimir Zapolskiy
2026-07-02 19:21 ` Conor Dooley
2026-07-02 10:52 ` [PATCH v2 2/3] media: i2c: og0ve1b: Introduce per-sensor data structure Wenmeng Liu
2026-07-02 11:04 ` sashiko-bot
2026-07-02 13:10 ` Vladimir Zapolskiy
2026-07-02 16:31 ` Bryan O'Donoghue [this message]
2026-07-02 10:52 ` [PATCH v2 3/3] media: i2c: og0ve1b: Add support for OmniVision OG0VA1B Wenmeng Liu
2026-07-02 13:16 ` Vladimir Zapolskiy
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5019684c-ace6-4202-97ca-98016df77d86@kernel.org \
--to=bod@kernel.org \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=krzk+dt@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=mchehab@kernel.org \
--cc=robh@kernel.org \
--cc=sakari.ailus@linux.intel.com \
--cc=vladimir.zapolskiy@linaro.org \
--cc=wenmeng.liu@oss.qualcomm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox