Re: [PATCH v2 2/2] media: i2c: mira016: Add driver for Mira016

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Jacopo,

On Mon, Sep 07, 2026 at 11:16:41AM +0200, Jacopo Mondi wrote:
> > > > > +static int mira016_set_ctrl(struct v4l2_ctrl *ctrl)
> > > > > +{
> > > > > +	struct mira016 *mira016 =
> > > > > +		container_of(ctrl->handler, struct mira016, ctrl_handler);
> > > > > +	struct i2c_client *client = v4l2_get_subdevdata(&mira016->sd);
> > > > > +	struct v4l2_subdev_state *state;
> > > > > +	struct v4l2_rect *crop;
> > > > > +	int ret = 0;
> > > > > +
> > > > > +	state = v4l2_subdev_get_locked_active_state(&mira016->sd);
> > > > > +	crop = v4l2_subdev_state_get_crop(state, 0);
> > > > > +
> > > > > +	if (ctrl->id == V4L2_CID_VBLANK) {
> > > > > +		s32 exposure_max = crop->height + ctrl->val
> > > > > +				 - MIRA016_FRAME_INTEGRATION_DIFF;
> > > > > +		s32 exposure_def = min(exposure_max,
> > > > > +				       mira016->exposure->val);
> > > > > +
> > > > > +		ret = __v4l2_ctrl_modify_range(mira016->exposure,
> > > > > +					       mira016->exposure->minimum,
> > > > > +					       exposure_max,
> > > > > +					       mira016->exposure->step,
> > > > > +					       exposure_def);
> > > > > +		if (ret)
> > > > > +			return ret;
> > > > > +	}
> > > > > +
> > > > > +	if (!pm_runtime_get_if_in_use(&client->dev))
> > > > > +		return 0;
> > > > > +
> > > > > +	switch (ctrl->id) {
> > > > > +	case V4L2_CID_EXPOSURE:
> > > > > +		ret = mira016_write_exposure_reg(mira016, ctrl->val);
> > > > > +		break;
> > > > > +	case V4L2_CID_VBLANK:
> > > > > +		ret = mira016_write_frame_duration_reg(mira016, state, ctrl->val);
> > > > > +		break;
> > > >
> > > > Is hblank part of the register lists?
> > > >
> > >
> > > These sensors (there will hopefully be more supported by this driver)
> > > do not have a real horizontal blanking.
> > >
> > > Their line length is expressed by a time base multipled by a line
> > > length which doesn't directly depend on the pixel width but rather on
> > > the ADC and PHY timings.
> > >
> > > You could expand the line duration by increasing the time base, but I
> > > wouldn't go there and use VBLANK only to control the frame duration.
> > >
> > > As you can see the HBLANK control is registered with  fixed value of
> > > 0.
> >
> > How does control frame rate then? If HBLANK is zero, the frame rate is
> > undefined, isn't it?
> >
> 
> Why do you think so ?
> 
> The row_timing is the product of the time base (in usec) multiplied by
> the row length expressed in time base cycles (see mira016_trow_psec()
> and how seq_time_base and row_length are calculated).
> 
> VBLANK is still controllable, and you can vary the frame rate by
> changing the vblank.
> 
> What have am I missing ?

Using only vertical blanking value to control the frame rate is fine, but
the correct HBLANK value is still needed by the user space to be able to
set the frame rate.

-- 
Regards,

Sakari Ailus