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

[Jacopo Mondi <jacopo.mondi@ideasonboard.com>]

Hi Tarang

On Fri, Oct 16, 2026 at 10:29:51AM +0000, Tarang Raval wrote:
> Hi Jacopo,
>
> Sorry, I overlooked one point in my previous review.
>
> > Add driver for the ams OSRAM Mira016 sensor.
> >
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > ---
> >  MAINTAINERS                 |    1 +
> >  drivers/media/i2c/Kconfig   |   12 +
> >  drivers/media/i2c/Makefile  |    1 +
> >  drivers/media/i2c/mira016.c | 2358 +++++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 2372 insertions(+)
>
> ...
>
> > +       struct {
> > +               u8 n;
> > +               u8 m;
> > +               u8 cp_div;
> > +               u8 otp_gran;
> > +               u8 gran_tg;
> > +               u8 lut_del;
> > +               u32 esc_h;
> > +               u32 esc_l;
> > +               u32 esc_period;
> > +               u32 byte_period;
>
> nit: pll.esc_period and pll.byte_period are never used, so you
> can drop them.

They are anyway calculated based on the PLL configuration and will be
required to support multiple link frequencies.

I wouldn't remove them if nobody really insist.

>
> > +               u32 data_rate_mbps;
> > +       } pll;
> > +
>
> ...
>
> > +static int mira016_set_ctrl(struct v4l2_ctrl *ctrl)
> > +{
> > +       struct mira016 *mira016 =
> > +               container_of(ctrl->handler, struct mira016, ctrl_handler);
> > +       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->minimum isn't updated when a format change alters row_length.

I don't think the min exposure time changes with row_length

It's actually computed as

	mira016->timings.min_exposure_time = (151 + mira016->pll.lut_del)
					   * mira016->pll.gran_tg
					   * mira016->timings.seq_time_base
					   / HZ_PER_KHZ;

And all the parameters used in the computation depend solely on the
output data rate which can't change at run time

	gran_tg = DIV_ROUND_DOWN_ULL(data_rate_mbps * KHZ_PER_MHZ, 1500);
	mira016->pll.gran_tg = DIV_ROUND_UP_ULL(gran_tg * 50, KHZ_PER_MHZ);

	mira016->pll.lut_del = DIV_ROUND_UP_ULL(7000, mira016->pll.gran_tg) - 140;

	mira016->timings.seq_time_base = 8 * HZ_PER_MHZ
				       / mira016->pll.data_rate_mbps;

>
> Other than this, the driver looks good to me.
>
> Reviewed-by: Tarang Raval <tarang.raval@siliconsignals.io>

Thank you!

>
> Best regards,
> Tarang