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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Mirela,

On Mon, Feb 03, 2025 at 10:02:14AM +0200, Mirela Rabulea wrote:
> 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.

Ah, looks like I missed that when reading the code.

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

I think that would be less confusing. Even better would be to calculate
the pixel rate from the PLL parameters, but Iknow there's such a thing
as too much yak shaving (even if it would be a useful improvement).

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

-- 
Regards,

Laurent Pinchart