Hi Vladimir

On Fri, 18 Jul 2025 at 16:27, Vladimir Zapolskiy
[off-list ref] wrote:

Hello Dave.

On 7/17/25 18:07, Dave Stevenson wrote:

quoted

Hi Vladimir and Kieran

On Thu, 17 Jul 2025 at 15:10, Kieran Bingham
[off-list ref] wrote:

quoted

Quoting Vladimir Zapolskiy (2025-07-17 13:40:01)

quoted

OmniVision OV6211 is a monochrome image sensor, which produces frames in
8/10-bit raw output format and supports 400x400, 200x200 and 100x100
output image resolution modes.

Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
---

<snip>

quoted

quoted

quoted

+       { 0x3800, 0x00 },
+       { 0x3801, 0x04 },
+       { 0x3802, 0x00 },
+       { 0x3803, 0x04 },
+       { 0x3804, 0x01 },
+       { 0x3805, 0x9b },
+       { 0x3806, 0x01 },
+       { 0x3807, 0x9b },
+       { 0x3808, 0x01 },
+       { 0x3809, 0x90 },

0x190 == 400;

quoted

+       { 0x380a, 0x01 },
+       { 0x380b, 0x90 },

So I bet these are the width and height registers.

Almost certainly as that also matches OV7251 and OV9281.
0x3800/1 is X_ADDR_START
0x3802/3 is Y_ADDR_START
0x3804/5 is X_ADDR_END
0x3806/7 is Y_ADDR_END
0x3808/9 is X_OUTPUT_SIZE
0x380a/b is Y_OUTPUT_SIZE.
Those almost all fit here, although it does imply that it's reading
(0x019b - 0x0004 + 1 = ) 408 x408 from the array, but only outputting
400x400.

quoted

Have you got a data sheet for this ? It would be /really/ far more
helpful to break out the specific register updates here for the mode
configuration at least.

quoted

+       { 0x380c, 0x05 },
+       { 0x380d, 0xf2 },
+       { 0x380e, 0x01 },
+       { 0x380f, 0xb6 },

<snip>

quoted

quoted

quoted

+};
+
+static const struct ov6211_mode supported_modes[] = {
+       {
+               .width = 400,
+               .height = 400,
+               .hts = 1522,
+               .vts = 438,

This implies we do have the blanking values, but don't expose them.
The values don't appear to be used for anything though.

0x380c and 0x380e registers are to set hts/vts, for reference I've
added a human readable representation over here, however the values
are formally unused anywhere in the driver, they could be removed,
or left as is.

Keep them but add the V4L2_VID_[HV]BLANK controls with the corresponding values.

Your commit text says that the sensor also supports 100x100 and
200x200, so keeping it as a per mode value is perfectly valid.

quoted

quoted

quoted

+               .reg_list = {
+                       .regs = mode_400x400_regs,
+                       .num_of_regs = ARRAY_SIZE(mode_400x400_regs),
+               },
+       },
+};

<snip>

quoted

quoted

quoted

+static int ov6211_set_ctrl(struct v4l2_ctrl *ctrl)
+{
+       struct ov6211 *ov6211 = container_of(ctrl->handler, struct ov6211,
+                                            ctrl_handler);
+       struct i2c_client *client = v4l2_get_subdevdata(&ov6211->sd);
+       int ret;
+
+       /* V4L2 controls values will be applied only when power is already up */
+       if (!pm_runtime_get_if_in_use(&client->dev))
+               return 0;
+
+       switch (ctrl->id) {
+       case V4L2_CID_ANALOGUE_GAIN:
+               ret = ov6211_write_reg(ov6211, OV6211_REG_ANALOGUE_GAIN,
+                                      OV6211_REG_VALUE_16BIT, ctrl->val);
+               break;
+       case V4L2_CID_EXPOSURE:
+               ret = ov6211_write_reg(ov6211, OV6211_REG_EXPOSURE,
+                                      OV6211_REG_VALUE_24BIT, ctrl->val << 4);
+               break;

What about V4L2_CID_HBLANK and V4L2_CID_VBLANK ?

It's also missing V4L2_CID_PIXEL_RATE, so even with the blanking
values you can't control frame rate.

The single given sensor configuration setting above sets 120fps.

I may add it with a V4L2_CTRL_FLAG_READ_ONLY control though.

The use of PIXEL_RATE, VBLANK, and HBLANK are the official way for raw
camera sensors to report (or control) the frame rate [1].
PIXEL_RATE defaults to read only anyway [2].
Having them all as read only is valid, but gives userspace the frame
rate information.

[1] https://www.kernel.org/doc/html/latest/userspace-api/media/drivers/camera-sensor.html#raw-camera-sensors
[2] https://elixir.bootlin.com/linux/v6.15.7/source/drivers/media/v4l2-core/v4l2-ctrls-defs.c#L1515

quoted

quoted

quoted

+       default:
+               ret = -EINVAL;
+               break;
+       }
+
+       pm_runtime_put(&client->dev);
+
+       return ret;
+}
+
+static const struct v4l2_ctrl_ops ov6211_ctrl_ops = {
+       .s_ctrl = ov6211_set_ctrl,
+};
+
+static int ov6211_init_controls(struct ov6211 *ov6211)
+{
+       struct v4l2_ctrl_handler *ctrl_hdlr;
+       s64 exposure_max;
+       int ret;
+
+       ctrl_hdlr = &ov6211->ctrl_handler;
+       ret = v4l2_ctrl_handler_init(ctrl_hdlr, 3);
+       if (ret)
+               return ret;
+
+       ctrl_hdlr->lock = &ov6211->mutex;
+
+       ov6211->link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr, &ov6211_ctrl_ops,
+                                       V4L2_CID_LINK_FREQ,
+                                       ARRAY_SIZE(link_freq_menu_items) - 1,
+                                       0, link_freq_menu_items);
+       if (ov6211->link_freq)
+               ov6211->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+
+       v4l2_ctrl_new_std(ctrl_hdlr, &ov6211_ctrl_ops, V4L2_CID_ANALOGUE_GAIN,
+                         OV6211_ANALOGUE_GAIN_MIN, OV6211_ANALOGUE_GAIN_MAX,
+                         OV6211_ANALOGUE_GAIN_STEP,
+                         OV6211_ANALOGUE_GAIN_DEFAULT);
+
+       exposure_max = (ov6211->cur_mode->vts - OV6211_EXPOSURE_MAX_MARGIN);
+       ov6211->exposure = v4l2_ctrl_new_std(ctrl_hdlr, &ov6211_ctrl_ops,
+                                            V4L2_CID_EXPOSURE,
+                                            OV6211_EXPOSURE_MIN, exposure_max,
+                                            OV6211_EXPOSURE_STEP,
+                                            OV6211_EXPOSURE_DEFAULT);
+

As well as the blanking - I think this driver is missing reporting the
crop selection implementation to report the sensor crops.

A call to v4l2_fwnode_device_parse and v4l2_ctrl_new_fwnode_properties
wouldn't go amiss to provide the standard orientation and rotation
properties wouldn't go amiss either.

To my knowledge the sensor does not support orientation or rotation
configuration.

v4l2_ctrl_new_fwnode_properties adds V4L2_CID_CAMERA_ORIENTATION and
V4L2_CID_CAMERA_SENSOR_ROTATION which provide userspace with
information over where and how the sensor is mounted in the device, as
read from device-tree or similar. They mean that a JPEG can have a
suitable EXIF orientation tag to get the viewer to rotate the image
appropriately, or it can identify where physically the sensor is
mounted.

Actual flips would normally be controlled V4L2_CID_[HV]FLIP, and not
supporting those is fine.

quoted

quoted

quoted

+}
+
+static int ov6211_check_hwcfg(struct ov6211 *ov6211)
+{
+       struct i2c_client *client = v4l2_get_subdevdata(&ov6211->sd);
+       struct device *dev = &client->dev;
+       struct fwnode_handle *fwnode = dev_fwnode(dev), *ep;
+       struct v4l2_fwnode_endpoint bus_cfg = {
+               .bus_type = V4L2_MBUS_CSI2_DPHY
+       };
+       int ret;
+
+       if (!fwnode)
+               return -ENXIO;
+
+       ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
+       if (!ep)
+               return -ENXIO;
+
+       ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg);
+       fwnode_handle_put(ep);
+       if (ret)
+               return ret;
+
+       if (bus_cfg.bus.mipi_csi2.num_data_lanes &&

Is it permitted to omit the data-lanes configuration?

That's a question to linux-media maintainers.

This sensor does not have configurable data-lanes, it's a single lane
sensor, so adding a constant 'data-lanes' property does not help in any
way to describe a hardware wiring, the property becomes a redundant one.

There are some MIPI CSI2 sensors, which does not have 'data-lanes'
property, for instance ovti,ov2680.

ov2680 doesn't validate the number of data lanes though.

<snip>

quoted

quoted

quoted

+
+       ov6211->xvclk = devm_clk_get_optional(&client->dev, NULL);
+       if (IS_ERR(ov6211->xvclk)) {
+               ret = PTR_ERR(ov6211->xvclk);
+               dev_err(&client->dev, "failed to get XVCLK clock: %d\n", ret);
+               return ret;
+       }
+
+       freq = clk_get_rate(ov6211->xvclk);
+       if (freq && freq != OV6211_MCLK_FREQ_24MHZ)
+               return dev_err_probe(&client->dev, -EINVAL,
+                               "XVCLK clock frequency %lu is not supported\n",
+                               freq);

This would be nicer to make use of the cleanups that have just been
implemented in
https://lore.kernel.org/linux-media/cover.1750942967.git.mehdi.djait@linux.intel.com/ (local)
and
https://lore.kernel.org/linux-media/20250710174808.5361-1-laurent.pinchart@ideasonboard.com/T/ (local)

Actually I've already checked it before publishing the code, as a summary:

1. to my understanding the introduced API is still under review, I didn't
find it in media/master or linux-next,

2. the only needed change to get support of the new helper is to replace
the single line of devm_clk_get_optional() with devm_v4l2_sensor_clk_get(),
no more than that,

3. the internal complexity of devm_v4l2_sensor_clk_get() seems excessive
right over here, what's worse I can not test devm_v4l2_sensor_clk_get()
in this driver on any ACPI platform...

To sum up and to minimize the overall complexity, I'd rather prefer to
stick to devm_clk_get_optional() at the moment, the switch to the new
proposed API can be done, when it's ready.

Responded to by Mehdi.
I would expect devm_v4l2_sensor_clk_get to be merged before this
driver, at which point you're likely going to get asked to make use of
it anyway. Otherwise you're just asking someone else to write the
cleanup patch at a later date.

  Dave