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

[Kate Hsuan]

Hi Vladimir,

Thank you for reviewing.

On Wed, Jun 24, 2026 at 6:12 AM Vladimir Zapolskiy
<vladimir.zapolskiy@xxxxxxxxxx> wrote:
>
> On 6/24/26 06:35, 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 X1 Carbon G14, X9-14 and X9-15 laptops
> > and it is a part of IPU7 solution. The driver was tested on Lenovo X1
> > Carbon G14, X9-14 and X9-15 laptops.
> >
> > Signed-off-by: Kate Hsuan <hpa@xxxxxxxxxx>
>
> Please find a few minor nitpicks below.
>
> > ---
> >   MAINTAINERS                |   6 +
> >   drivers/media/i2c/Kconfig  |  10 +
> >   drivers/media/i2c/Makefile |   1 +
> >   drivers/media/i2c/imx471.c | 971 +++++++++++++++++++++++++++++++++++++
> >   4 files changed, 988 insertions(+)
> >   create mode 100644 drivers/media/i2c/imx471.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 6b4560681b51..586958b1816d 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -25219,6 +25219,12 @@ T:   git git://linuxtv.org/media.git
> >   F:  Documentation/devicetree/bindings/media/i2c/sony,imx415.yaml
> >   F:  drivers/media/i2c/imx415.c
> >
> > +SONY IMX471 SENSOR DRIVER
> > +M:   Kate Hsuan <hpa@xxxxxxxxxx>
> > +L:   linux-media@xxxxxxxxxxxxxxx
> > +S:   Maintained
> > +F:   drivers/media/i2c/imx471.c
> > +
> >   SONY MEMORYSTICK SUBSYSTEM
> >   M:  Maxim Levitsky <maximlevitsky@xxxxxxxxx>
> >   M:  Alex Dubov <oakad@xxxxxxxxx>
> > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> > index 5d173e0ecf42..b7199f9f5a0c 100644
> > --- a/drivers/media/i2c/Kconfig
> > +++ b/drivers/media/i2c/Kconfig
> > @@ -287,6 +287,16 @@ config VIDEO_IMX415
> >         To compile this driver as a module, choose M here: the
> >         module will be called imx415.
> >
> > +config VIDEO_IMX471
> > +     tristate "Sony IMX471 sensor support"
> > +     select V4L2_CCI_I2C
> > +     help
> > +       This is a Video4Linux2 sensor driver for the Sony
> > +       IMX471 camera.
> > +
> > +       To compile this driver as a module, choose M here: the
> > +       module will be called imx471.
> > +
> >   config VIDEO_MAX9271_LIB
> >       tristate
> >
> > diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> > index e45359efe0e4..acbd321fc12e 100644
> > --- a/drivers/media/i2c/Makefile
> > +++ b/drivers/media/i2c/Makefile
> > @@ -61,6 +61,7 @@ obj-$(CONFIG_VIDEO_IMX335) += imx335.o
> >   obj-$(CONFIG_VIDEO_IMX355) += imx355.o
> >   obj-$(CONFIG_VIDEO_IMX412) += imx412.o
> >   obj-$(CONFIG_VIDEO_IMX415) += imx415.o
> > +obj-$(CONFIG_VIDEO_IMX471) += imx471.o
> >   obj-$(CONFIG_VIDEO_IR_I2C) += ir-kbd-i2c.o
> >   obj-$(CONFIG_VIDEO_ISL7998X) += isl7998x.o
> >   obj-$(CONFIG_VIDEO_KS0127) += ks0127.o
> > diff --git a/drivers/media/i2c/imx471.c b/drivers/media/i2c/imx471.c
> > new file mode 100644
> > index 000000000000..1e1bff69ea3d
> > --- /dev/null
> > +++ b/drivers/media/i2c/imx471.c
> > @@ -0,0 +1,971 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * imx471.c - imx471 sensor driver
> > + *
> > + * Copyright (C) 2025 Intel Corporation
> > + * Copyright (C) 2026 Kate Hsuan <hpa@xxxxxxxxxx>
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
> > +#include <linux/i2c.h>
> > +#include <linux/module.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/unaligned.h>
>
> No declarations coming from linux/unaligned.h header are used in the driver.
>
> > +#include <media/v4l2-cci.h>
> > +#include <media/v4l2-ctrls.h>
> > +#include <media/v4l2-device.h>
>
> No declarations coming from media/v4l-event.h header are used in the driver,
> likely it's intended to include media/v4l2-subdev.h instead.
>
> > +#include <media/v4l2-event.h>
>
> No declarations coming from media/v4l-event.h header are used in the driver.

I'll drop them.

>
> > +#include <media/v4l2-fwnode.h>
> > +
> > +#define IMX471_REG_MODE_SELECT                       CCI_REG8(0x0100)
> > +#define IMX471_MODE_STANDBY                  0x00
> > +#define IMX471_MODE_STREAMING                        0x01
> > +
> > +/* Chip ID */
> > +#define IMX471_REG_CHIP_ID                   CCI_REG16(0x0016)
> > +#define IMX471_CHIP_ID                               0x0471
> > +
> > +/* V_TIMING internal */
> > +#define IMX471_REG_FLL                               CCI_REG16(0x0340)
> > +#define IMX471_FLL_MAX                               0xffff
> > +
> > +/* Exposure control */
> > +#define IMX471_REG_EXPOSURE                  CCI_REG16(0x0202)
> > +#define IMX471_EXPOSURE_MIN                  1
> > +#define IMX471_EXPOSURE_STEP                 1
> > +#define IMX471_EXPOSURE_DEFAULT                      1270
> > +
> > +/* Default exposure margin */
> > +#define IMX471_EXPOSURE_MARGIN                       18
> > +
> > +/* Analog gain control */
> > +#define IMX471_REG_ANALOG_GAIN                       CCI_REG16(0x0204)
> > +#define IMX471_ANA_GAIN_MIN                  0
> > +#define IMX471_ANA_GAIN_MAX                  800
> > +#define IMX471_ANA_GAIN_STEP                 1
> > +#define IMX471_ANA_GAIN_DEFAULT                      0
> > +
> > +/* Digital gain control */
> > +#define IMX471_REG_DPGA_USE_GLOBAL_GAIN              CCI_REG16(0x3ff9)
> > +#define IMX471_REG_DIG_GAIN_GLOBAL           CCI_REG16(0x020e)
> > +#define IMX471_DGTL_GAIN_MIN                 256
> > +#define IMX471_DGTL_GAIN_MAX                 4095
> > +#define IMX471_DGTL_GAIN_STEP                        1
> > +#define IMX471_DGTL_GAIN_DEFAULT             256
> > +
> > +/* HFLIP and VFLIP control */
> > +#define IMX471_REG_ORIENTATION                       CCI_REG8(0x0101)
> > +#define IMX471_HFLIP_BIT                     BIT(0)
> > +#define IMX471_VFLIP_BIT                     BIT(1)
>
> IMX471_HFLIP_BIT and IMX471_VFLIP_BIT macro do not have any users in the driver.
I'll drop them.

>
> > +
> > +/* Test Pattern Control */
> > +#define IMX471_REG_TEST_PATTERN                      CCI_REG8(0x0600)
> > +#define IMX471_TEST_PATTERN_DISABLED         0
> > +#define IMX471_TEST_PATTERN_SOLID_COLOR              1
> > +#define IMX471_TEST_PATTERN_COLOR_BARS               2
> > +#define IMX471_TEST_PATTERN_GRAY_COLOR_BARS  3
> > +#define IMX471_TEST_PATTERN_PN9                      4
>
> IMX471_TEST_PATTERN_* macro do not have any users in the driver.
I'll drop them.

>
> > +
> > +/* default link frequency and external clock */
> > +#define IMX471_LINK_FREQ_DEFAULT             200000000LL
> > +#define IMX471_EXT_CLK                               19200000
> > +#define IMX471_LINK_FREQ_INDEX                       0
>
> Please drop IMX471_LINK_FREQ_INDEX macro along with .link_freq_index from
> struct imx471_mode.
Okay

>
> --
> Best wishes,
> Vladimir
>


-- 
BR,
Kate