Re: [PATCH 2/2] media: i2c: Add OmniVision OV6211 image sensor driver

[Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>]

Hello Dave.

On 7/17/25 18:07, Dave Stevenson wrote:
> Hi Vladimir and Kieran
> 
> On Thu, 17 Jul 2025 at 15:10, Kieran Bingham
> <kieran.bingham@ideasonboard.com> wrote:
>>
>> Quoting Vladimir Zapolskiy (2025-07-17 13:40:01)
>>> 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>

>>> +       { 0x3800, 0x00 },
>>> +       { 0x3801, 0x04 },
>>> +       { 0x3802, 0x00 },
>>> +       { 0x3803, 0x04 },
>>> +       { 0x3804, 0x01 },
>>> +       { 0x3805, 0x9b },
>>> +       { 0x3806, 0x01 },
>>> +       { 0x3807, 0x9b },
>>> +       { 0x3808, 0x01 },
>>> +       { 0x3809, 0x90 },
>>
>> 0x190 == 400;
>>
>>> +       { 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.
> 
>> 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.
>>
>>> +       { 0x380c, 0x05 },
>>> +       { 0x380d, 0xf2 },
>>> +       { 0x380e, 0x01 },
>>> +       { 0x380f, 0xb6 },

<snip>

>>> +};
>>> +
>>> +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.

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

<snip>

>>> +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.

>>> +       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.

>>> +}
>>> +
>>> +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.

<snip>

>>> +
>>> +       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/
> and
> https://lore.kernel.org/linux-media/20250710174808.5361-1-laurent.pinchart@ideasonboard.com/T/
> 

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.

-- 
Best wishes,
Vladimir