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
