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

[Tarang Raval]

Hi Jacopo,

> 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@xxxxxxxxxxxxxxxx>
> > > ---
> > >  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

Okay, let's keep them for future use.

> >
> > > +               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;

You're right that min_exposure_time (in ns) doesn't depend on
row_length, it only depends on the data rate.

My concern was about the control value, which is expressed in lines:
mira016_nsec_to_lines() divides by seq_time_base * row_length, so the
minimum in lines is 7 with row_length 1062 (Y8/Y10), but would be 5 with
row_length 1504 (Y12).

Best regards,
Tarang

> 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@xxxxxxxxxxxxxxxxx>
>
> Thank you!
>
> >
> > Best regards,
> > Tarang