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

[Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>]

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@intel.com> 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@redhat.com>

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@redhat.com>
> +L:	linux-media@vger.kernel.org
> +S:	Maintained
> +F:	drivers/media/i2c/imx471.c
> +
>   SONY MEMORYSTICK SUBSYSTEM
>   M:	Maxim Levitsky <maximlevitsky@gmail.com>
>   M:	Alex Dubov <oakad@yahoo.com>
> 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@redhat.com>
> + */
> +
> +#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.

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

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

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

-- 
Best wishes,
Vladimir