Re: [PATCH v2 2/2] media: i2c: imx471: Add Sony IMX471 image sensor driver

[Sakari Ailus]

Hi Kate,

On Fri, May 08, 2026 at 04:51:03PM +0800, Kate Hsuan wrote:
> Hi Sakari,
> 
> Thank you for reviewing the patch.
> 
> On Wed, May 6, 2026 at 3:15 PM Sakari Ailus <sakari.ailus@xxxxxx> wrote:
> >
> > Hi Kate,
> >
> > Thanks for the update. Please see my comments below.
> >
> > On Tue, May 05, 2026 at 02:13:27PM +0800, Kate Hsuan wrote:
> > > Add a new driver for Sony imx471 camera sensor. It is based on
> > > Jimmy Su <jimmy.su@xxxxxxxxx> implementation and the driver can be found
> > > in the following URL.
> > > https://github.com/intel/ipu6-drivers/commits/master/drivers/media/i2c/imx471.c
> > >
> > > This sensor can be found on Lenovo X9-14 and X9-15 laptop and it is a part
> > > of IPU7 solution. The driver was tested on Lenovo X9-14 and X9-15 laptops.
> > >
> > > Link: https://github.com/intel/ipu6-drivers/blob/master/drivers/media/i2c/imx471.c
> > > Link: https://bugzilla.redhat.com/show_bug.cgi?id=2454119
> >
> > I'm not sure we need these links. But the extra newline here should go in
> > any case.
> If these links are unnecessary, I'll drop them.

Please.

...

> > > +static const struct cci_reg_sequence imx471_global_regs[] = {
> > > +     { CCI_REG8(0x0136), 0x13 },
> > > +     { CCI_REG8(0x0137), 0x33 },
> > > +     { CCI_REG8(0x3c7e), 0x08 },
> > > +     { CCI_REG8(0x3c7f), 0x05 },
> > > +     { CCI_REG8(0x3e35), 0x00 },
> > > +     { CCI_REG8(0x3e36), 0x00 },
> > > +     { CCI_REG8(0x3e37), 0x00 },
> > > +     { CCI_REG8(0x3f7f), 0x01 },
> >
> > A lot of these appear to be 16-bit registers. It'd make sense to write them
> > to the sensor as such.
> >
> > Quite a few of these registers also have a name, please use the names
> > instead of numerical values.
> Ok.
> >
> > The sensor is probably CCS compliant to some (unknown?) degree.
> It's pretty similar to imx214. I'll try my best to find the names of
> the registers.

If there's no name for a register, which generally is the case for the MSRs
starting at 0x3000, just use the numerical value.

...

> > > +static int imx471_get_regulators(struct device *dev, struct imx471_data *sensor)
> > > +{
> > > +     unsigned int i;
> > > +
> > > +     for (i = 0; i < IMX471_NUM_SUPPLIES; i++)
> >
> > You could declare i here.
> Something like that?
> for (unsigned int i = 0; i < IMX471_NUM_SUPPLIES; i++)

Yes, please.

...

> > > +     case V4L2_CID_HFLIP:
> > > +     case V4L2_CID_VFLIP:
> > > +             if (sensor->streaming)
> >
> > Please grab the controls when starting streaming and ungrab them when
> > stopping it.
>  v4l2_ctrl_grab() will be used to manage it when starting or stopping
> the stream.

You'll need __v4l2_ctrl_grab() as the mutex is already acquired.

...

> > > +static int imx471_set_pad_format(struct v4l2_subdev *sd,
> > > +                              struct v4l2_subdev_state *sd_state,
> > > +                              struct v4l2_subdev_format *fmt)
> > > +{
> > > +     struct imx471_data *sensor = to_imx471_data(sd);
> > > +     const struct imx471_mode *mode;
> > > +
> > > +     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;
> > > +
> > > +     if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> >
> > You can omit this check.
> Ok, since it is always an active state.
> >
> > > +             int h_blank;
> > > +             u64 pixel_rate;
> > > +
> > > +             pixel_rate = IMX471_LINK_FREQ_DEFAULT * 2 * 4;
> > > +             do_div(pixel_rate, 10);
> >
> > You could use div_u64() here.
> Okay
> >
> > > +             __v4l2_ctrl_modify_range(sensor->pixel_rate,
> > > +                                      V4L2_CID_PIXEL_RATE,
> > > +                                      pixel_rate, 1, pixel_rate);
> > > +
> > > +             __v4l2_ctrl_modify_range(sensor->vblank,
> > > +                                      mode->fll_min - mode->height,
> > > +                                      IMX471_FLL_MAX - mode->height,
> > > +                                      1,
> > > +                                      mode->fll_def - mode->height);
> > > +
> > > +             h_blank = mode->llp - mode->width;
> > > +             /*
> > > +              * Currently hblank is not changeable.
> > > +              * So FPS control is done only by vblank.
> > > +              */
> > > +             __v4l2_ctrl_modify_range(sensor->hblank, h_blank,
> > > +                                      h_blank, 1, h_blank);
> > > +     }
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +static int imx471_init_state(struct v4l2_subdev *sd,
> > > +                          struct v4l2_subdev_state *sd_state)
> > > +{
> > > +     struct v4l2_subdev_format fmt = { };
> > > +
> > > +     fmt.which =
> > > +             sd_state ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE;
> >
> > sd_state is always non-NULL here.
> I'll change to fmt.which=V4L2_SUBDEV_FORMAT_ACTIVE; here.

I'd just call imx471_update_pad_format() with imx471_modes[0].

-- 
Kind regards,

Sakari Ailus