From: Wenmeng Liu <wenmeng.liu@oss.qualcomm.com>
To: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>,
Rob Herring <robh@kernel.org>,
Krzysztof Kozlowski <krzk+dt@kernel.org>,
Conor Dooley <conor+dt@kernel.org>,
Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>,
linux-media@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v6 3/4] media: i2c: og0ve1b: Introduce per-sensor data structure
Date: Thu, 10 Sep 2026 16:14:49 +0800 [thread overview]
Message-ID: <20853e7e-0884-49ae-a6ce-48c0c826ea27@oss.qualcomm.com> (raw)
In-Reply-To: <ap_v94pUrTo1C1Go@kekkonen.localdomain>
Hi Sakari,
On 9/8/2026 7:22 PM, Sakari Ailus wrote:
> Hi Wenmeng,
>
> On Tue, Sep 01, 2026 at 03:40:06PM +0800, Wenmeng Liu wrote:
>> Refactor the driver to separate sensor-specific data from the common
>> driver code. Introduce a sensor_data structure, populated through
>> i2c_get_match_data(), to hold the chip ID, MCLK frequency, link
>> frequencies and supported modes.
>>
>> Test pattern programming is described by a per-sensor callback, and each
>> mode stores its media bus format code (rather than a bits-per-pixel
>> value) so that sensors with a different output format can be supported.
>>
>> Reviewed-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
>> Tested-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
>> Signed-off-by: Wenmeng Liu <wenmeng.liu@oss.qualcomm.com>
>> ---
>> drivers/media/i2c/og0ve1b.c | 100 +++++++++++++++++++++++++++++++-------------
>> 1 file changed, 72 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/media/i2c/og0ve1b.c b/drivers/media/i2c/og0ve1b.c
>> index da5c8e49d4a811f55736bd89c6922856f2e7391c..f560d5d90fda94abf4bc622c5e4f861b078b811f 100644
>> --- a/drivers/media/i2c/og0ve1b.c
>> +++ b/drivers/media/i2c/og0ve1b.c
>> @@ -5,6 +5,7 @@
>> #include <linux/delay.h>
>> #include <linux/gpio/consumer.h>
>> #include <linux/i2c.h>
>> +#include <linux/math64.h>
>> #include <linux/module.h>
>> #include <linux/pm_runtime.h>
>> #include <linux/regulator/consumer.h>
>> @@ -65,11 +66,23 @@ struct og0ve1b_mode {
>> u32 height; /* Frame height in pixels */
>> 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;
>> +
>> +struct og0ve1b_sensor_data {
>> + u64 chip_id;
>
> This one seems to be unused.
>
It is currently used by og0ve1b_identify_sensor().
The intention of moving chip_id into the sensor_data structure is to
prepare for support of additional sensors sharing the common driver
code. The next patch in the series adds OG0VA1B support, which uses a
different chip ID. Keeping chip_id in the per-sensor data allows the
common identification code to be reused unchanged.
Thanks,
Wenmeng
>> + unsigned long mclk_freq;
>> + int (*enable_test_pattern)(struct og0ve1b *og0ve1b, u32 pattern);
>> + 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",
>> "Standard Test Bar",
>> @@ -99,6 +112,8 @@ struct og0ve1b {
>>
>> /* Saved register value */
>> u64 pre_isp;
>> +
>> + const struct og0ve1b_sensor_data *data;
>> };
>>
>> static const struct cci_reg_sequence og0ve1b_640x480_120fps_mode[] = {
>> @@ -247,13 +262,13 @@ static const struct cci_reg_sequence og0ve1b_640x480_120fps_mode[] = {
>> { CCI_REG8(0x3f47), 0x35 },
>> };
>>
>> -static const struct og0ve1b_mode supported_modes[] = {
>> +static const struct og0ve1b_mode og0ve1b_supported_modes[] = {
>> {
>> .width = 640,
>> .height = 480,
>> .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),
>> @@ -273,11 +288,21 @@ static int og0ve1b_enable_test_pattern(struct og0ve1b *og0ve1b, u32 pattern)
>> return cci_write(og0ve1b->regmap, OG0VE1B_REG_PRE_ISP, val, NULL);
>> }
>>
>> +static const struct og0ve1b_sensor_data og0ve1b_data = {
>> + .chip_id = OG0VE1B_CHIP_ID,
>> + .mclk_freq = OG0VE1B_MCLK_FREQ_24MHZ,
>> + .enable_test_pattern = og0ve1b_enable_test_pattern,
>> + .link_freq_menu = og0ve1b_link_freq_menu,
>> + .num_link_freqs = ARRAY_SIZE(og0ve1b_link_freq_menu),
>> + .modes = og0ve1b_supported_modes,
>> + .num_modes = ARRAY_SIZE(og0ve1b_supported_modes),
>> +};
>> +
>> 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->data->modes[0];
>> s64 exposure_max;
>> int ret;
>>
>> @@ -314,7 +339,7 @@ static int og0ve1b_set_ctrl(struct v4l2_ctrl *ctrl)
>> ctrl->val + mode->height, NULL);
>> break;
>> case V4L2_CID_TEST_PATTERN:
>> - ret = og0ve1b_enable_test_pattern(og0ve1b, ctrl->val);
>> + ret = og0ve1b->data->enable_test_pattern(og0ve1b, ctrl->val);
>> break;
>> default:
>> ret = -EINVAL;
>> @@ -330,10 +355,19 @@ static const struct v4l2_ctrl_ops og0ve1b_ctrl_ops = {
>> .s_ctrl = og0ve1b_set_ctrl,
>> };
>>
>> +static s64 og0ve1b_pixel_rate(const struct og0ve1b_sensor_data *data)
>> +{
>> + const struct og0ve1b_mode *mode = &data->modes[0];
>> + unsigned int bpp = mode->code == MEDIA_BUS_FMT_Y8_1X8 ? 8 : 10;
>> +
>> + return div_u64(data->link_freq_menu[0], bpp);
>> +}
>> +
>> 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->data->modes[0];
>> + const struct og0ve1b_sensor_data *data = og0ve1b->data;
>> s64 exposure_max, pixel_rate, h_blank, v_blank;
>> struct v4l2_fwnode_device_properties props;
>> struct v4l2_ctrl *ctrl;
>> @@ -343,12 +377,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);
>> + data->num_link_freqs - 1,
>> + 0, data->link_freq_menu);
>> if (ctrl)
>> ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>>
>> - pixel_rate = og0ve1b_link_freq_menu[0] / mode->bpp;
>> + pixel_rate = og0ve1b_pixel_rate(data);
>> v4l2_ctrl_new_std(ctrl_hdlr, &og0ve1b_ctrl_ops, V4L2_CID_PIXEL_RATE,
>> 0, pixel_rate, 1, pixel_rate);
>>
>> @@ -407,7 +441,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 +455,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->data->modes[0].reg_list;
>> int ret;
>>
>> ret = pm_runtime_resume_and_get(og0ve1b->dev);
>> @@ -484,13 +518,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->data->modes,
>> + og0ve1b->data->num_modes,
>> width, height,
>> fmt->format.width,
>> fmt->format.height);
>> @@ -505,10 +540,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->data->modes[0].code;
>>
>> return 0;
>> }
>> @@ -517,15 +554,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 *data = og0ve1b->data;
>> +
>> + if (fse->index >= data->num_modes)
>> return -EINVAL;
>>
>> - if (fse->code != MEDIA_BUS_FMT_Y8_1X8)
>> + if (fse->code != data->modes[fse->index].code)
>> return -EINVAL;
>>
>> - fse->min_width = supported_modes[fse->index].width;
>> + fse->min_width = data->modes[fse->index].width;
>> fse->max_width = fse->min_width;
>> - fse->min_height = supported_modes[fse->index].height;
>> + fse->min_height = data->modes[fse->index].height;
>> fse->max_height = fse->min_height;
>>
>> return 0;
>> @@ -534,13 +574,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)
>> {
>> + const struct og0ve1b_mode *mode = &to_og0ve1b(sd)->data->modes[0];
>> 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 = mode->code,
>> + .width = mode->width,
>> + .height = mode->height,
>> },
>> };
>>
>> @@ -586,9 +627,9 @@ 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->data->chip_id) {
>> + dev_err(og0ve1b->dev, "chip id mismatch: %llx!=%llx\n",
>> + og0ve1b->data->chip_id, val);
>> return -ENODEV;
>> }
>>
>> @@ -624,8 +665,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->data->link_freq_menu,
>> + og0ve1b->data->num_link_freqs,
>> &freq_bitmap);
>>
>> v4l2_fwnode_endpoint_free(&bus_cfg);
>> @@ -686,6 +727,9 @@ static int og0ve1b_probe(struct i2c_client *client)
>> return -ENOMEM;
>>
>> og0ve1b->dev = &client->dev;
>> + og0ve1b->data = i2c_get_match_data(client);
>> + if (!og0ve1b->data)
>> + return -ENODEV;
>>
>> v4l2_i2c_subdev_init(&og0ve1b->sd, client, &og0ve1b_subdev_ops);
>>
>> @@ -700,7 +744,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->data->mclk_freq)
>> return dev_err_probe(og0ve1b->dev, -EINVAL,
>> "XVCLK clock frequency %lu is not supported\n",
>> freq);
>> @@ -819,7 +863,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);
>>
>
next prev parent reply other threads:[~2026-09-10 8:14 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-09-01 7:40 [PATCH v6 0/4] media: i2c: Add OmniVision OG0VA1B camera sensor driver Wenmeng Liu
2026-09-01 7:40 ` [PATCH v6 1/4] dt-bindings: media: i2c: og0ve1b: Add OmniVision OG0VA1B camera sensor Wenmeng Liu
2026-09-01 7:40 ` [PATCH v6 2/4] media: i2c: og0ve1b: Use monochrome-appropriate test pattern name Wenmeng Liu
2026-09-08 11:14 ` Sakari Ailus
2026-09-10 7:34 ` Wenmeng Liu
2026-09-01 7:40 ` [PATCH v6 3/4] media: i2c: og0ve1b: Introduce per-sensor data structure Wenmeng Liu
2026-09-08 11:22 ` Sakari Ailus
2026-09-10 8:14 ` Wenmeng Liu [this message]
2026-09-10 8:28 ` Sakari Ailus
2026-09-01 7:40 ` [PATCH v6 4/4] media: i2c: og0ve1b: Add support for OmniVision OG0VA1B Wenmeng Liu
2026-09-08 11:19 ` Sakari Ailus
2026-09-10 8:16 ` Wenmeng Liu
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=20853e7e-0884-49ae-a6ce-48c0c826ea27@oss.qualcomm.com \
--to=wenmeng.liu@oss.qualcomm.com \
--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 \
/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