Re: Re: [PATCH v2 2/4] media: ox05b1s: Add omnivision OX05B1S raw sensor driver

[Mirela Rabulea]

Hi Laurent,

...
> > > > +
> > > > +struct ox05b1s_mode {
> > > > +     u32 index;
> > > > +     u32 width;
> > > > +     u32 height;
> > > > +     u32 code;
> > > > +     u32 bpp;
> > > > +     u32 vts; /* default VTS */
> > > > +     u32 hts; /* default HTS */
> > > > +     u32 exp; /* max exposure */
> > > > +     bool h_bin; /* horizontal binning */
> > > > +     u32 fps;
> > > > +     struct ox05b1s_reglist *reg_data;
> > > > +};
> > > > +
...
> > > > +
> > > > +static struct ox05b1s_mode ox05b1s_supported_modes[] = {
> > > > +     {
> > > > +             .index          = 0,
> > > > +             .width          = 2592,
> > > > +             .height         = 1944,
> > > > +             .code           = MEDIA_BUS_FMT_SGRBG10_1X10,
> > > > +             .bpp            = 10,
> > > > +             .vts            = 0x850,
> > > > +             .hts            = 0x2f0,
> > > > +             .exp            = 0x850 - 8,
> > > > +             .h_bin          = false,
> > > > +             .fps            = 30,
> > > > +             .reg_data       = ox05b1s_reglist_2592x1944,
> > > > +     }, {
> > > > +             /* sentinel */
> > > > +     }
> > > > +};
> > > > +
...
> > > > +
> > > > +static int ox05b1s_set_vts(struct ox05b1s *sensor, u32 vts)
> > > > +{
> > > > +     u8 values[2] = { (u8)(vts >> 8) & 0xff, (u8)(vts & 0xff) };
> > > > +
> > > > +     return regmap_bulk_write(sensor->regmap, OX05B1S_REG_TIMING_VTS_H, values, 2);
> > > > +}
> > > > +
...
> > > > +
> > > > +static int ox05b1s_s_ctrl(struct v4l2_ctrl *ctrl)
> > > > +{
> > > > +     struct v4l2_subdev *sd = ctrl_to_sd(ctrl);
> > > > +     struct i2c_client *client = v4l2_get_subdevdata(sd);
> > > > +     struct ox05b1s *sensor = client_to_ox05b1s(client);
> > > > +     u32 w = sensor->mode->width;
> > > > +     u32 h = sensor->mode->height;
> > > > +     int ret = 0;
> > > > +
> > > > +     /* apply V4L2 controls values only if power is already up */
> > > > +     if (!pm_runtime_get_if_in_use(&client->dev))
> > > > +             return 0;
> > > > +
> > > > +     /* s_ctrl holds sensor lock */
> > > > +     switch (ctrl->id) {
> > > > +     case V4L2_CID_VBLANK:
> > > > +             ret = ox05b1s_set_vts(sensor, h + ctrl->val);
> > > > +             break;
> > > > +     case V4L2_CID_HBLANK:
> > > > +             if (sensor->mode->h_bin)
> > > > +                     ret = ox05b1s_set_hts(sensor, w + ctrl->val);
> > > > +             else
> > > > +                     ret = ox05b1s_set_hts(sensor, (w + ctrl->val) / 2);
> > > > +             break;
> > > > +     case V4L2_CID_PIXEL_RATE:
> > > > +             /* Read-only, but we adjust it based on mode. */
> > > > +             break;
> > > > +     case V4L2_CID_ANALOGUE_GAIN:
> > > > +             ret = ox05b1s_set_analog_gain(sensor, ctrl->val);
> > > > +             break;
> > > > +     case V4L2_CID_EXPOSURE:
> > > > +             ret = ox05b1s_set_exp(sensor, ctrl->val);
> > > > +             break;
> > > > +     default:
> > > > +             ret = -EINVAL;
> > > > +             break;
> > > > +     }
> > > > +
> > > > +     pm_runtime_put(&client->dev);
> > > > +
> > > > +     return ret;
> > > > +}
> > > > +
...
> > > > +
> > > > +/* Update control ranges based on current streaming mode, needs sensor lock */
> > > > +static int ox05b1s_update_controls(struct ox05b1s *sensor)
> > > > +{
> > > > +     int ret;
> > > > +     struct device *dev = &sensor->i2c_client->dev;
> > > > +     u32 hts = sensor->mode->hts;
> > > > +     u32 hblank;
> > > > +     u32 vts = sensor->mode->vts;
> > > > +     u32 vblank = vts - sensor->mode->height;
> > > > +     u32 fps = sensor->mode->fps;
> > > > +     u64 pixel_rate = (sensor->mode->h_bin) ? hts * vts * fps : 2 * hts * vts * fps;
> > > Unless the sensor adjusts the pixel rate when you write the blanking
> > > registers, this doesn't seem correct.
> > I'm not sure I understand.
> >
> > The pixel_rate value calculated here is fixed per mode, as the
> > hts,vts,fps are the default values per mode.
> >
> > The hblank is basically read-only (min=max value). If vblank is changed
> > by user, the frame rate will change, but the pixel_rate value will
> > remain the same.
> Yes, the frame rate will change, but the fps variable above won't. It
> will still be set to the default fps for the mode, while the actual
> frame rate produced by the sensor will differ. The pixel rate
> calculation here will therefore give an incorrect value.

Yes, the real frame rate produced  by the sensor is different than the 
value that is hold in the fps field of the mode structure (that is the 
default fps, I can add a comment to clarify that, or change the names of 
those fields to def_fps, def_hts, def_vts). But that only happens as a 
result of the actual vts change, and the actual vts is not used in the 
pixel_rate calculation above, the calculation only uses the default 
values from the mode structure. The real vts is written into the sensor 
register when V4L2_CID_VBLANK control is changed, but the vts field in 
the mode structure remains a default value.

Or, I could add a pixel_rate field in the ox05b1s_mode, and use that 
instead of a formula, maybe it would be more straightforward.

Let me know what you think.

...
> > > > +     {0x0100, 0x00},
> > > Please use register macros for the registers that are documented.
> > Do you mean to add a define for all register numbers in the init list,
> > or just use the define in the init list, in case it was defined already
> > for other usages elsewhere in the code?
> The latter, using OX05B1S_REG_SW_STB here. I'd like a macro for each
> register in the init list, but macros such as OX05B1S_REG_0107 are
> useless. Registers that are documented in the datasheet should be named
> with macros, registers that are not can use numerical addresses and
> values.
Thanks, I'll add macros for the documented registers.


Thanks,

Mirela