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

[Sakari Ailus]

Hi Kate,

On Wed, Jun 24, 2026 at 05:30:59PM +0800, Kate Hsuan wrote:
> 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;

sd_state is supposed to be always non-NULL. So you can use FORMAT_TRY
always.

-- 
Regards,

Sakari Ailus