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

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

Hi Tarang

On Wed, Oct 21, 2026 at 03:05:06PM +0000, Tarang Raval wrote:
> 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@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
>
> 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).

Uuuuh, you're right!!

Thanks for pointing it out, I will update and resend with your tag ;)

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