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

[Tarang Raval]

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.

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

Other than this, the driver looks good to me.

Reviewed-by: Tarang Raval <tarang.raval@xxxxxxxxxxxxxxxxx>

Best regards,
Tarang