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

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

Hi Kieran.

On 7/17/25 17:09, Kieran Bingham wrote:
> Quoting Vladimir Zapolskiy (2025-07-17 13:40:01)

<snip>

>> +       { 0x3808, 0x01 },
>> +       { 0x3809, 0x90 },
> 
> 0x190 == 400;
> 
>> +       { 0x380a, 0x01 },
>> +       { 0x380b, 0x90 },
> 
> So I bet these are the width and height registers.
> 
> 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.

No, I don't have a datasheet for this sensor.

What's imporatant the sensor controls are similar (identical?) to an old
OV6710 sensor (a guess from chip ID also), and there is a public driver
for that sensor written for the OLPC project, a number of controls may
be reused from that driver, however since it requires such an extensive
testing, it can be done later on.

I kindly ask to review the displayed supported modes/configurations
at the moment. FWIW your guess of mapping the registers to width/height
is correct.

<snip>

>> +static int ov6211_write_reg_list(struct ov6211 *ov6211,
>> +                                const struct ov6211_reg_list *r_list)
>> +{
>> +       struct i2c_client *client = v4l2_get_subdevdata(&ov6211->sd);
>> +       unsigned int i;
>> +       int ret;
>> +
>> +       for (i = 0; i < r_list->num_of_regs; i++) {
>> +               ret = ov6211_write_reg(ov6211, r_list->regs[i].address, 1,
>> +                                      r_list->regs[i].val);
>> +               if (ret) {
>> +                       dev_err_ratelimited(&client->dev,
>> +                                           "failed to write reg 0x%4.4x. error = %d\n",
>> +                                           r_list->regs[i].address, ret);
>> +                       return ret;
>> +               }
>> +       }
>> +
>> +       return 0;
>> +}
>> +
> 
> Please use CCI helpers now.
> 

It makes sense, thank you.

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

I don't have any information how to set/get this setting, the OV6710 driver
also does not provide any hints, so it should be omitted, I believe.

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

To the best of my knowledge there is no crops, so I believe this is irrelevant.

This particular sensor is extremely simple.

-- 
Best wishes,
Vladimir