Re: [PATCH v5 3/3] media: i2c: imx471: Add Sony IMX471 image sensor driver

[Kate Hsuan]

Hi Sakari,

Thank you for reviewing.

On Wed, Jun 24, 2026 at 4:56 PM Sakari Ailus
<sakari.ailus@xxxxxxxxxxxxxxx> wrote:
>
> Hi Kate,
>
> Thanks for the update.
>
> On Wed, Jun 24, 2026 at 11:35:08AM +0800, Kate Hsuan wrote:
> ...
>
> > +static int imx471_set_pad_format(struct v4l2_subdev *sd,
> > +                              struct v4l2_subdev_state *sd_state,
> > +                              struct v4l2_subdev_format *fmt)
> > +{
> > +     struct imx471 *sensor = to_imx471(sd);
> > +     const struct imx471_mode *mode;
> > +     int h_blank, ret;
> > +
> > +     mode = v4l2_find_nearest_size(imx471_modes, ARRAY_SIZE(imx471_modes),
> > +                                   width, height, fmt->format.width,
> > +                                   fmt->format.height);
> > +
> > +     imx471_update_pad_format(sensor, mode, fmt);
> > +
> > +     *v4l2_subdev_state_get_format(sd_state, fmt->pad) = fmt->format;
> > +
> > +     if (fmt->which == V4L2_SUBDEV_FORMAT_TRY)
> > +             return 0;
> > +
> > +     if (media_entity_is_streaming(&sensor->sd.entity))
> > +             return -EBUSY;
> > +
> > +     ret = __v4l2_ctrl_modify_range(sensor->vblank,
> > +                                    mode->fll_min - mode->height,
> > +                                    IMX471_FLL_MAX - mode->height,
> > +                                    1,
> > +                                    mode->fll_def - mode->height);
> > +     if (ret)
> > +             return ret;
> > +
> > +     h_blank = mode->llp - mode->width;
> > +     /*
> > +      * Currently hblank is not changeable.
> > +      * So FPS control is done only by vblank.
> > +      */
> > +     return __v4l2_ctrl_modify_range(sensor->hblank, h_blank,
> > +                                     h_blank, 1, h_blank);
> > +}
>
> ...
>
> > +
> > +static int imx471_init_state(struct v4l2_subdev *sd,
> > +                          struct v4l2_subdev_state *sd_state)
> > +{
> > +     struct v4l2_subdev_format fmt = {
> > +             .which = V4L2_SUBDEV_FORMAT_ACTIVE,
>
> The purpose of the init_state op is to initialise the provided state only,
> it's not allowed to change the sensor configuration.

The input parameter sd_state could initialise it. I'll change this as follows
.which = sd_state ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE;

>
> > +             .format = {
> > +                     .code = MEDIA_BUS_FMT_SRGGB10_1X10,
> > +                     .width = imx471_modes[0].width,
> > +                     .height = imx471_modes[0].height,
> > +             },
> > +     };
> > +
> > +     return imx471_set_pad_format(sd, sd_state, &fmt);
> > +}
>
> --
> Kind regards,
>
> Sakari Ailus
>


-- 
BR,
Kate