[PATCH v3 0/3] Add Sony IMX471 camera sensor driver

[PATCH v3 0/3] Add Sony IMX471 camera sensor driver

[Kate Hsuan]

This patchset adds the Sony IMX471 camera sensor driver to the Linux
kernel and resolves the IPU7 camera can't work issueon Lenovo X9
laptops [1].

The patchset contains two patches:
1. Add DMI information of Lenovo X9 to the image upside-down list
2. Add Sony IMX471 image sensor driver

The IMX471 driver can be found in the Intel ipu6-drivers repository [2].
To comply with the sensor driver implementation, the clean-up work
includes:

1. Use CCI register helpers.

2. Enable and disable streams using enable_streams and disable_streams
   functions in struct v4l2_subdev_pad_ops. Invoke
   v4l2_subdev_s_stream_helper() to manage the streaming state.

3. Get rotation information from fwnode properties using
   v4l2_fwnode_device_parse().

4. Finalizes the initialization of the subdev, including allocation of
   the active state using v4l2_subdev_init_finalize().

5. Add the IMX471 driver to the Makefile and Kconfig file.

6. The mutex lock is managed by the V4l2 core.

7. Replace the supported link frequency with v4l2_link_freq_to_bitmap().

8. Drop unused codes.

[1] https://bugzilla.redhat.com/show_bug.cgi?id=2454119
[2] https://github.com/intel/ipu6-drivers/commits/master/drivers/media/i2c/imx471.c

Changes in v3:
1. Naming the register addresses and set up the value with the correct value length.
2. Implement the .get_selection().
3. Drop "identified" field from struct imx471.
4. Drop "streaming" field from struct imx471 and use the __v4l2_ctrl_grab() instead.
5. Moreover, The naming for the register can be found in a seperated patch. If we
   agree with the patch, I will squash it into one patch.

Changes in v2:
1. Change the Bayer format setting according to the vertical and horizontal flip settings.
2. Replace the self-owned mutex with the v4l2 subdev state.
3. Rework the flip control.
4. Manage the regulators using devm_regulator_bulk_get|disable|enbale API
5. Invoke devm_v4l2_sensor_clk_get to get clock-frequency


Kate Hsuan (3):
  media: ipu-bridge: Add DMI information of Lenovo X9 to the image
    upside-down list
  media: i2c: imx471: Add Sony IMX471 image sensor driver
  media: i2c: imx471: Naming the register

 MAINTAINERS                          |    6 +
 drivers/media/i2c/Kconfig            |   10 +
 drivers/media/i2c/Makefile           |    1 +
 drivers/media/i2c/imx471.c           | 1010 ++++++++++++++++++++++++++
 drivers/media/pci/intel/ipu-bridge.c |   14 +
 5 files changed, 1041 insertions(+)
 create mode 100644 drivers/media/i2c/imx471.c

-- 
2.54.0

[PATCH v3 1/3] media: ipu-bridge: Add DMI information of Lenovo X9 to the image upside-down list

[Kate Hsuan]

The Lenovo X9 has an upside-down-mounted Sony IMX471 sensor so the image
was displayed upside-down. Add the DMI information of Lenovo X9 to
resolve the issue.

Signed-off-by: Kate Hsuan <hpa@redhat.com>
---
 drivers/media/pci/intel/ipu-bridge.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
index 32cc95a766b7..1c3364451fa3 100644
--- a/drivers/media/pci/intel/ipu-bridge.c
+++ b/drivers/media/pci/intel/ipu-bridge.c
@@ -118,6 +118,20 @@ static const struct dmi_system_id upside_down_sensor_dmi_ids[] = {
 		},
 		.driver_data = "OVTI02C1",
 	},
+	{
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
+			DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad X9-14"),
+		},
+		.driver_data = "SONY471A",
+	},
+	{
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
+			DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad X9-15"),
+		},
+		.driver_data = "SONY471A",
+	},
 	{} /* Terminating entry */
 };
 
-- 
2.54.0

Re: [PATCH v3 1/3] media: ipu-bridge: Add DMI information of Lenovo X9 to the image upside-down list

[Damjan Georgievski]

On 5/22/26 05:11, Kate Hsuan wrote:
> The Lenovo X9 has an upside-down-mounted Sony IMX471 sensor so the image
> was displayed upside-down. Add the DMI information of Lenovo X9 to
> resolve the issue.
>
> Signed-off-by: Kate Hsuan <hpa@redhat.com>
> ---
>   drivers/media/pci/intel/ipu-bridge.c | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
>
> diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
> index 32cc95a766b7..1c3364451fa3 100644
> --- a/drivers/media/pci/intel/ipu-bridge.c
> +++ b/drivers/media/pci/intel/ipu-bridge.c
> @@ -118,6 +118,20 @@ static const struct dmi_system_id upside_down_sensor_dmi_ids[] = {
>   		},
>   		.driver_data = "OVTI02C1",
>   	},
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
> +			DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad X9-14"),

Isn't this going to be an issue in the future if/when a "Gen 2" appears?

Just extrapolation from my older laptop, it shows "ThinkPad X1 Carbon 
5th" for "/sys/class/dmi/id/product_version"

> +		},
> +		.driver_data = "SONY471A",
> +	},
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
> +			DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad X9-15"),
> +		},
> +		.driver_data = "SONY471A",
> +	},
>   	{} /* Terminating entry */
>   };
>   

Re: [PATCH v3 1/3] media: ipu-bridge: Add DMI information of Lenovo X9 to the image upside-down list

[Kate Hsuan]

Hi Damjan,

Thank you for your review.

On Sat, May 30, 2026 at 6:33 AM Damjan Georgievski <gdamjan@gmail.com> wrote:
>
> On 5/22/26 05:11, Kate Hsuan wrote:
> > The Lenovo X9 has an upside-down-mounted Sony IMX471 sensor so the image
> > was displayed upside-down. Add the DMI information of Lenovo X9 to
> > resolve the issue.
> >
> > Signed-off-by: Kate Hsuan <hpa@redhat.com>
> > ---
> >   drivers/media/pci/intel/ipu-bridge.c | 14 ++++++++++++++
> >   1 file changed, 14 insertions(+)
> >
> > diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
> > index 32cc95a766b7..1c3364451fa3 100644
> > --- a/drivers/media/pci/intel/ipu-bridge.c
> > +++ b/drivers/media/pci/intel/ipu-bridge.c
> > @@ -118,6 +118,20 @@ static const struct dmi_system_id upside_down_sensor_dmi_ids[] = {
> >               },
> >               .driver_data = "OVTI02C1",
> >       },
> > +     {
> > +             .matches = {
> > +                     DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
> > +                     DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad X9-14"),
>
> Isn't this going to be an issue in the future if/when a "Gen 2" appears?

You can look into the v1 [1]. The DMI_BOARD_NAME is used to
distinguish the types of X1.
v3 covers wider ranges of X9-14 and 15. If we agree with v1, I can
revert this patch to v1.

[1] https://lore.kernel.org/linux-media/20260417083214.222189-2-hpa@redhat.com/

>
> Just extrapolation from my older laptop, it shows "ThinkPad X1 Carbon
> 5th" for "/sys/class/dmi/id/product_version"
>
> > +             },
> > +             .driver_data = "SONY471A",
> > +     },
> > +     {
> > +             .matches = {
> > +                     DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
> > +                     DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad X9-15"),
> > +             },
> > +             .driver_data = "SONY471A",
> > +     },
> >       {} /* Terminating entry */
> >   };
> >
>



-- 
BR,
Kate

Re: [PATCH v3 1/3] media: ipu-bridge: Add DMI information of Lenovo X9 to the image upside-down list

[Mark Pearson]

Note - switched to my open-source friendly email account.

Hi Kate,

On Tue, Jun 2, 2026, at 2:35 AM, Kate Hsuan wrote:
> Hi Damjan,
>
> Thank you for your review.
>
> On Sat, May 30, 2026 at 6:33 AM Damjan Georgievski <gdamjan@gmail.com> wrote:
>>
>> On 5/22/26 05:11, Kate Hsuan wrote:
>> > The Lenovo X9 has an upside-down-mounted Sony IMX471 sensor so the image
>> > was displayed upside-down. Add the DMI information of Lenovo X9 to
>> > resolve the issue.
>> >
>> > Signed-off-by: Kate Hsuan <hpa@redhat.com>
>> > ---
>> >   drivers/media/pci/intel/ipu-bridge.c | 14 ++++++++++++++
>> >   1 file changed, 14 insertions(+)
>> >
>> > diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
>> > index 32cc95a766b7..1c3364451fa3 100644
>> > --- a/drivers/media/pci/intel/ipu-bridge.c
>> > +++ b/drivers/media/pci/intel/ipu-bridge.c
>> > @@ -118,6 +118,20 @@ static const struct dmi_system_id upside_down_sensor_dmi_ids[] = {
>> >               },
>> >               .driver_data = "OVTI02C1",
>> >       },
>> > +     {
>> > +             .matches = {
>> > +                     DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
>> > +                     DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad X9-14"),
>>
>> Isn't this going to be an issue in the future if/when a "Gen 2" appears?
>
> You can look into the v1 [1]. The DMI_BOARD_NAME is used to
> distinguish the types of X1.
> v3 covers wider ranges of X9-14 and 15. If we agree with v1, I can
> revert this patch to v1.
>
> [1] https://lore.kernel.org/linux-media/20260417083214.222189-2-hpa@redhat.com/
>
I happily defer to Hans on things like this as he has way more experience than me, but we usually use the BOARD_NAME for identifying platforms everywhere else in the kernel so that feels to me 'safer'.

If we do go ahead with PRODUCT_VERSION it should probably be DMI_MATCH_EXACT instead. I think that will work fine, but it is not commonly used and I would worry about the FW team doing weird versions for different markets (the two board names are very standard for Thinkpads)

Mark (currently on vacation - so replies may be delayed)

Re: [PATCH v3 1/3] media: ipu-bridge: Add DMI information of Lenovo X9 to the image upside-down list

[Kate Hsuan]

Hi Mark,

On Wed, Jun 3, 2026 at 1:04 PM Mark Pearson <mpearson@squebb.ca> wrote:
>
> Note - switched to my open-source friendly email account.
>
> Hi Kate,
>
> On Tue, Jun 2, 2026, at 2:35 AM, Kate Hsuan wrote:
> > Hi Damjan,
> >
> > Thank you for your review.
> >
> > On Sat, May 30, 2026 at 6:33 AM Damjan Georgievski <gdamjan@gmail.com> wrote:
> >>
> >> On 5/22/26 05:11, Kate Hsuan wrote:
> >> > The Lenovo X9 has an upside-down-mounted Sony IMX471 sensor so the image
> >> > was displayed upside-down. Add the DMI information of Lenovo X9 to
> >> > resolve the issue.
> >> >
> >> > Signed-off-by: Kate Hsuan <hpa@redhat.com>
> >> > ---
> >> >   drivers/media/pci/intel/ipu-bridge.c | 14 ++++++++++++++
> >> >   1 file changed, 14 insertions(+)
> >> >
> >> > diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
> >> > index 32cc95a766b7..1c3364451fa3 100644
> >> > --- a/drivers/media/pci/intel/ipu-bridge.c
> >> > +++ b/drivers/media/pci/intel/ipu-bridge.c
> >> > @@ -118,6 +118,20 @@ static const struct dmi_system_id upside_down_sensor_dmi_ids[] = {
> >> >               },
> >> >               .driver_data = "OVTI02C1",
> >> >       },
> >> > +     {
> >> > +             .matches = {
> >> > +                     DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
> >> > +                     DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad X9-14"),
> >>
> >> Isn't this going to be an issue in the future if/when a "Gen 2" appears?
> >
> > You can look into the v1 [1]. The DMI_BOARD_NAME is used to
> > distinguish the types of X1.
> > v3 covers wider ranges of X9-14 and 15. If we agree with v1, I can
> > revert this patch to v1.
> >
> > [1] https://lore.kernel.org/linux-media/20260417083214.222189-2-hpa@redhat.com/
> >
> I happily defer to Hans on things like this as he has way more experience than me, but we usually use the BOARD_NAME for identifying platforms everywhere else in the kernel so that feels to me 'safer'.
>
> If we do go ahead with PRODUCT_VERSION it should probably be DMI_MATCH_EXACT instead. I think that will work fine, but it is not commonly used and I would worry about the FW team doing weird versions for different markets (the two board names are very standard for Thinkpads)
>
> Mark (currently on vacation - so replies may be delayed)
>

Thank you for your clarification.
I'll revert this patch to v1.


-- 
BR,
Kate

[PATCH v3 2/3] media: i2c: imx471: Add Sony IMX471 image sensor driver

[Kate Hsuan]

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

Signed-off-by: Kate Hsuan <hpa@redhat.com>
---
 MAINTAINERS                |    6 +
 drivers/media/i2c/Kconfig  |   10 +
 drivers/media/i2c/Makefile |    1 +
 drivers/media/i2c/imx471.c | 1006 ++++++++++++++++++++++++++++++++++++
 4 files changed, 1023 insertions(+)
 create mode 100644 drivers/media/i2c/imx471.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 1126fdd639ad..d597337e7c24 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -24735,6 +24735,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 5eb1e0e0a87a..1c28c498a9f1 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 a3a6396df3c4..0539e9171030 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..f3c7fdce2d50
--- /dev/null
+++ b/drivers/media/i2c/imx471.c
@@ -0,0 +1,1006 @@
+// 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>
+#include <media/v4l2-cci.h>
+#include <media/v4l2-ctrls.h>
+#include <media/v4l2-device.h>
+#include <media/v4l2-event.h>
+#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			0x04f6
+
+/*
+ *  the digital control register for all color control looks like:
+ *  +-----------------+------------------+
+ *  |      [7:0]      |       [15:8]     |
+ *  +-----------------+------------------+
+ *  |	  0x020f      |       0x020e     |
+ *  --------------------------------------
+ *  it is used to calculate the digital gain times value(integral + fractional)
+ *  the [15:8] bits is the fractional part and [7:0] bits is the integral
+ *  calculation equation is:
+ *      gain value (unit: times) = REG[15:8] + REG[7:0]/0x100
+ *  Only value in 0x0100 ~ 0x0FFF range is allowed.
+ *  Analog gain use 10 bits in the registers and allowed range is 0 ~ 960
+ */
+/* Analog gain control */
+#define IMX471_REG_ANALOG_GAIN			CCI_REG16(0x0204)
+#define IMX471_ANA_GAIN_MIN			0
+#define IMX471_ANA_GAIN_MAX			960
+#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
+
+#define IMX471_VALUE_08BIT			1
+
+/* HFLIP and VFLIP control */
+#define IMX471_REG_ORIENTATION			CCI_REG8(0x0101)
+#define IMX471_HFLIP_BIT			BIT(0)
+#define IMX471_VFLIP_BIT			BIT(1)
+
+/* Default exposure margin */
+#define IMX471_EXPOSURE_MARGIN			18
+
+/* Horizontal crop window offset */
+#define IMX471_REG_H_WIN_OFFSET			CCI_REG8(0x0409)
+
+/* Vertical crop window offset */
+#define IMX471_REG_V_WIN_OFFSET			CCI_REG8(0x034b)
+
+/* 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
+
+/* default link frequency and external clock */
+#define IMX471_LINK_FREQ_DEFAULT		200000000LL
+#define IMX471_EXT_CLK				19200000
+#define IMX471_LINK_FREQ_INDEX			0
+
+/* IMX471 native and active pixel array size */
+#define IMX471_NATIVE_WIDTH			4672
+#define IMX471_NATIVE_HEIGHT			3512
+#define IMX471_PIXEL_ARRAY_LEFT			8
+#define IMX471_PIXEL_ARRAY_TOP			8
+#define IMX471_PIXEL_ARRAY_WIDTH		4656
+#define IMX471_PIXEL_ARRAY_HEIGHT		3496
+
+#define to_imx471(_sd) container_of_const(_sd, struct imx471, sd)
+
+static const char * const imx471_supply_name[] = {
+	"avdd",
+};
+
+#define IMX471_NUM_SUPPLIES			ARRAY_SIZE(imx471_supply_name)
+
+/* Mode : resolution and related config&values */
+struct imx471_mode {
+	/* Frame width */
+	u32 width;
+	/* Frame height */
+	u32 height;
+
+	/* V-timing */
+	u32 fll_def;
+	u32 fll_min;
+
+	/* H-timing */
+	u32 llp;
+
+	/* index of link frequency */
+	u32 link_freq_index;
+
+	/* Default register values */
+	const struct cci_reg_sequence *default_mode_regs;
+	const int default_mode_regs_length;
+};
+
+struct imx471 {
+	struct v4l2_subdev sd;
+	struct media_pad pad;
+
+	struct v4l2_ctrl_handler ctrl_handler;
+	/* V4L2 Controls */
+	struct v4l2_ctrl *link_freq;
+	struct v4l2_ctrl *pixel_rate;
+	struct v4l2_ctrl *vblank;
+	struct v4l2_ctrl *hblank;
+	struct v4l2_ctrl *vflip;
+	struct v4l2_ctrl *hflip;
+	struct v4l2_ctrl *exposure;
+
+	struct gpio_desc *reset_gpio;
+	struct regulator_bulk_data supplies[IMX471_NUM_SUPPLIES];
+	struct clk *img_clk;
+
+	struct device *dev;
+	struct regmap *regmap;
+};
+
+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 },
+	{ CCI_REG8(0x4431), 0x04 },
+	{ CCI_REG8(0x531c), 0x01 },
+	{ CCI_REG8(0x531d), 0x02 },
+	{ CCI_REG8(0x531e), 0x04 },
+	{ CCI_REG8(0x5928), 0x00 },
+	{ CCI_REG8(0x5929), 0x2f },
+	{ CCI_REG8(0x592a), 0x00 },
+	{ CCI_REG8(0x592b), 0x85 },
+	{ CCI_REG8(0x592c), 0x00 },
+	{ CCI_REG8(0x592d), 0x32 },
+	{ CCI_REG8(0x592e), 0x00 },
+	{ CCI_REG8(0x592f), 0x88 },
+	{ CCI_REG8(0x5930), 0x00 },
+	{ CCI_REG8(0x5931), 0x3d },
+	{ CCI_REG8(0x5932), 0x00 },
+	{ CCI_REG8(0x5933), 0x93 },
+	{ CCI_REG8(0x5938), 0x00 },
+	{ CCI_REG8(0x5939), 0x24 },
+	{ CCI_REG8(0x593a), 0x00 },
+	{ CCI_REG8(0x593b), 0x7a },
+	{ CCI_REG8(0x593c), 0x00 },
+	{ CCI_REG8(0x593d), 0x24 },
+	{ CCI_REG8(0x593e), 0x00 },
+	{ CCI_REG8(0x593f), 0x7a },
+	{ CCI_REG8(0x5940), 0x00 },
+	{ CCI_REG8(0x5941), 0x2f },
+	{ CCI_REG8(0x5942), 0x00 },
+	{ CCI_REG8(0x5943), 0x85 },
+	{ CCI_REG8(0x5f0e), 0x6e },
+	{ CCI_REG8(0x5f11), 0xc6 },
+	{ CCI_REG8(0x5f17), 0x5e },
+	{ CCI_REG8(0x7990), 0x01 },
+	{ CCI_REG8(0x7993), 0x5d },
+	{ CCI_REG8(0x7994), 0x5d },
+	{ CCI_REG8(0x7995), 0xa1 },
+	{ CCI_REG8(0x799a), 0x01 },
+	{ CCI_REG8(0x799d), 0x00 },
+	{ CCI_REG8(0x8169), 0x01 },
+	{ CCI_REG8(0x8359), 0x01 },
+	{ CCI_REG8(0x9302), 0x1e },
+	{ CCI_REG8(0x9306), 0x1f },
+	{ CCI_REG8(0x930a), 0x26 },
+	{ CCI_REG8(0x930e), 0x23 },
+	{ CCI_REG8(0x9312), 0x23 },
+	{ CCI_REG8(0x9316), 0x2c },
+	{ CCI_REG8(0x9317), 0x19 },
+	{ CCI_REG8(0xb046), 0x01 },
+	{ CCI_REG8(0xb048), 0x01 },
+};
+
+static const struct cci_reg_sequence mode_1928x1088_regs[] = {
+	{ CCI_REG8(0x0101), 0x00 },
+	{ CCI_REG8(0x0112), 0x0a },
+	{ CCI_REG8(0x0113), 0x0a },
+	{ CCI_REG8(0x0114), 0x03 },
+	{ CCI_REG8(0x0342), 0x0a },
+	{ CCI_REG8(0x0343), 0x00 },
+	{ CCI_REG8(0x0340), 0x13 },
+	{ CCI_REG8(0x0341), 0xb0 },
+	{ CCI_REG8(0x0344), 0x00 },
+	{ CCI_REG8(0x0345), 0x00 },
+	{ CCI_REG8(0x0346), 0x01 },
+	{ CCI_REG8(0x0347), 0xbc },
+	{ CCI_REG8(0x0348), 0x12 },
+	{ CCI_REG8(0x0349), 0x2f },
+	{ CCI_REG8(0x034a), 0x0b },
+	{ CCI_REG8(0x034b), 0xeb },
+	{ CCI_REG8(0x0381), 0x01 },
+	{ CCI_REG8(0x0383), 0x01 },
+	{ CCI_REG8(0x0385), 0x01 },
+	{ CCI_REG8(0x0387), 0x01 },
+	{ CCI_REG8(0x0900), 0x01 },
+	{ CCI_REG8(0x0901), 0x22 },
+	{ CCI_REG8(0x0902), 0x08 },
+	{ CCI_REG8(0x3f4c), 0x81 },
+	{ CCI_REG8(0x3f4d), 0x81 },
+	{ CCI_REG8(0x0408), 0x00 },
+	{ CCI_REG8(0x0409), 0xc8 },
+	{ CCI_REG8(0x040a), 0x00 },
+	{ CCI_REG8(0x040b), 0x6c },
+	{ CCI_REG8(0x040c), 0x07 },
+	{ CCI_REG8(0x040d), 0x88 },
+	{ CCI_REG8(0x040e), 0x04 },
+	{ CCI_REG8(0x040f), 0x40 },
+	{ CCI_REG8(0x034c), 0x07 },
+	{ CCI_REG8(0x034d), 0x88 },
+	{ CCI_REG8(0x034e), 0x04 },
+	{ CCI_REG8(0x034f), 0x40 },
+	{ CCI_REG8(0x0301), 0x06 },
+	{ CCI_REG8(0x0303), 0x02 },
+	{ CCI_REG8(0x0305), 0x02 },
+	{ CCI_REG8(0x0306), 0x00 },
+	{ CCI_REG8(0x0307), 0x79 },
+	{ CCI_REG8(0x030b), 0x01 },
+	{ CCI_REG8(0x030d), 0x02 },
+	{ CCI_REG8(0x030e), 0x00 },
+	{ CCI_REG8(0x030f), 0x53 },
+	{ CCI_REG8(0x0310), 0x01 },
+	{ CCI_REG8(0x0202), 0x13 },
+	{ CCI_REG8(0x0203), 0x9e },
+	{ CCI_REG8(0x0204), 0x00 },
+	{ CCI_REG8(0x0205), 0x00 },
+	{ CCI_REG8(0x020e), 0x01 },
+	{ CCI_REG8(0x020f), 0x00 },
+	{ CCI_REG8(0x3f78), 0x01 },
+	{ CCI_REG8(0x3f79), 0x31 },
+	{ CCI_REG8(0x3ffe), 0x00 },
+	{ CCI_REG8(0x3fff), 0x8a },
+	{ CCI_REG8(0x5f0a), 0xb6 },
+};
+
+static const char * const imx471_test_pattern_menu[] = {
+	"Disabled",
+	"Solid Colour",
+	"Eight Vertical Colour Bars",
+	"Colour Bars With Fade to Grey",
+	"Pseudorandom Sequence (PN9)",
+};
+
+/*
+ * When adding more than the one below, make sure the disallowed ones will
+ * actually be disabled in the LINK_FREQ control.
+ */
+static const s64 link_freq_menu_items[] = {
+	IMX471_LINK_FREQ_DEFAULT,
+};
+
+/*
+ * The Bayer formats for the flipping.
+ * - no flip
+ * - h flip
+ * - v flip
+ * - h and v flips
+ */
+static const u32 imx471_hv_flips_bayer_order[] = {
+	MEDIA_BUS_FMT_SRGGB10_1X10,
+	MEDIA_BUS_FMT_SGRBG10_1X10,
+	MEDIA_BUS_FMT_SGBRG10_1X10,
+	MEDIA_BUS_FMT_SBGGR10_1X10,
+};
+
+/* Mode configs */
+static const struct imx471_mode imx471_modes[] = {
+	{
+		.width = 1928,
+		.height = 1088,
+		.fll_def = 1308,
+		.fll_min = 1308,
+		.llp = 2328,
+		.link_freq_index = IMX471_LINK_FREQ_INDEX,
+		.default_mode_regs = mode_1928x1088_regs,
+		.default_mode_regs_length = ARRAY_SIZE(mode_1928x1088_regs),
+	},
+};
+
+static int imx471_get_regulators(struct device *dev, struct imx471 *sensor)
+{
+	for (unsigned int  i = 0; i < IMX471_NUM_SUPPLIES; i++)
+		sensor->supplies[i].supply = imx471_supply_name[i];
+
+	return devm_regulator_bulk_get(dev, IMX471_NUM_SUPPLIES,
+				       sensor->supplies);
+}
+
+static int imx471_set_ctrl(struct v4l2_ctrl *ctrl)
+{
+	struct imx471 *sensor = container_of(ctrl->handler,
+						  struct imx471,
+						  ctrl_handler);
+	struct v4l2_subdev_state *state =
+			v4l2_subdev_get_locked_active_state(&sensor->sd);
+	const struct v4l2_mbus_framefmt *format =
+			v4l2_subdev_state_get_format(state, 0);
+	s64 exposure_max;
+	int ret;
+
+	/* Propagate change of current control to all related controls */
+	if (ctrl->id == V4L2_CID_VBLANK) {
+		/* Update max exposure while meeting expected vblanking */
+		exposure_max =
+			format->height + ctrl->val - IMX471_EXPOSURE_MARGIN;
+		__v4l2_ctrl_modify_range(sensor->exposure,
+					 sensor->exposure->minimum,
+					 exposure_max,
+					 sensor->exposure->step,
+					 exposure_max);
+	}
+
+	/* V4L2 controls values will be applied only when power is already up */
+	if (!pm_runtime_get_if_in_use(sensor->dev))
+		return 0;
+
+	switch (ctrl->id) {
+	case V4L2_CID_ANALOGUE_GAIN:
+		cci_write(sensor->regmap, IMX471_REG_ANALOG_GAIN,
+			  ctrl->val, &ret);
+		break;
+	case V4L2_CID_DIGITAL_GAIN:
+		cci_write(sensor->regmap, IMX471_REG_DIG_GAIN_GLOBAL,
+			  ctrl->val, &ret);
+		break;
+	case V4L2_CID_EXPOSURE:
+		cci_write(sensor->regmap, IMX471_REG_EXPOSURE,
+			  ctrl->val, &ret);
+		break;
+	case V4L2_CID_VBLANK:
+		/* Update FLL that meets expected vertical blanking */
+		cci_write(sensor->regmap, IMX471_REG_FLL,
+			  format->height + ctrl->val, &ret);
+		break;
+	case V4L2_CID_TEST_PATTERN:
+		cci_write(sensor->regmap, IMX471_REG_TEST_PATTERN,
+			  ctrl->val, &ret);
+		break;
+	case V4L2_CID_HFLIP:
+	case V4L2_CID_VFLIP:
+		cci_write(sensor->regmap, IMX471_REG_ORIENTATION,
+			  sensor->hflip->val | sensor->vflip->val << 1, &ret);
+		break;
+	default:
+		ret = -EINVAL;
+		dev_info(sensor->dev, "ctrl(id:0x%x,val:0x%x) is not handled",
+			 ctrl->id, ctrl->val);
+		break;
+	}
+
+	pm_runtime_put(sensor->dev);
+
+	return ret;
+}
+
+static const struct v4l2_ctrl_ops imx471_ctrl_ops = {
+	.s_ctrl = imx471_set_ctrl,
+};
+
+static u32 imx471_get_format_code(struct imx471 *sensor)
+{
+	unsigned int i;
+
+	i = (sensor->vflip->val ? 2 : 0) | (sensor->hflip->val ? 1 : 0);
+
+	return imx471_hv_flips_bayer_order[i];
+}
+
+static int imx471_enum_mbus_code(struct v4l2_subdev *sd,
+				 struct v4l2_subdev_state *sd_state,
+				 struct v4l2_subdev_mbus_code_enum *code)
+{
+	struct imx471 *sensor = to_imx471(sd);
+
+	if (code->index >= (ARRAY_SIZE(imx471_hv_flips_bayer_order) / 4))
+		return -EINVAL;
+
+	code->code = imx471_get_format_code(sensor);
+
+	return 0;
+}
+static int imx471_enum_frame_size(struct v4l2_subdev *sd,
+				  struct v4l2_subdev_state *sd_state,
+				  struct v4l2_subdev_frame_size_enum *fse)
+{
+	if (fse->index >= ARRAY_SIZE(imx471_modes))
+		return -EINVAL;
+
+	fse->min_width = imx471_modes[fse->index].width;
+	fse->max_width = fse->min_width;
+	fse->min_height = imx471_modes[fse->index].height;
+	fse->max_height = fse->min_height;
+
+	return 0;
+}
+
+static void imx471_update_pad_format(struct imx471 *sensor,
+				     const struct imx471_mode *mode,
+				     struct v4l2_subdev_format *fmt)
+{
+	fmt->format.code = imx471_get_format_code(sensor);
+	fmt->format.width = mode->width;
+	fmt->format.height = mode->height;
+	fmt->format.field = V4L2_FIELD_NONE;
+}
+
+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;
+	u64 pixel_rate;
+
+	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;
+
+	pixel_rate = IMX471_LINK_FREQ_DEFAULT * 2 * 4;
+	div_u64(pixel_rate, 10);
+	__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_get_selection(struct v4l2_subdev *sd,
+				struct v4l2_subdev_state *sd_state,
+				struct v4l2_subdev_selection *sel)
+{
+	switch (sel->target) {
+	case V4L2_SEL_TGT_CROP:
+		sel->r = *v4l2_subdev_state_get_crop(sd_state, sel->pad);
+		break;
+
+	case V4L2_SEL_TGT_NATIVE_SIZE:
+		sel->r.top = 0;
+		sel->r.left = 0;
+		sel->r.width = IMX471_NATIVE_WIDTH;
+		sel->r.height = IMX471_NATIVE_HEIGHT;
+		return 0;
+
+	case V4L2_SEL_TGT_CROP_DEFAULT:
+	case V4L2_SEL_TGT_CROP_BOUNDS:
+		sel->r.top = IMX471_PIXEL_ARRAY_TOP;
+		sel->r.left = IMX471_PIXEL_ARRAY_LEFT;
+		sel->r.width = IMX471_PIXEL_ARRAY_WIDTH;
+		sel->r.height = IMX471_PIXEL_ARRAY_HEIGHT;
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
+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,
+		.format = {
+			.code = MEDIA_BUS_FMT_SRGGB10_1X10,
+			.width = imx471_modes[0].width,
+			.height = imx471_modes[0].height,
+		},
+	};
+
+	imx471_set_pad_format(sd, sd_state, &fmt);
+
+	return 0;
+}
+
+static int imx471_identify_module(struct imx471 *sensor)
+{
+	int ret;
+	u64 val;
+
+	ret = cci_read(sensor->regmap, IMX471_REG_CHIP_ID, &val, NULL);
+	if (ret)
+		return dev_err_probe(sensor->dev, ret,
+				     "failed to read chip id\n");
+
+	if (val != IMX471_CHIP_ID)
+		return dev_err_probe(sensor->dev, -EIO,
+				     "chip id mismatch: %x!=%llx\n",
+				     IMX471_CHIP_ID, val);
+
+	return 0;
+}
+
+static int imx471_power_off(struct device *dev)
+{
+	struct v4l2_subdev *sd = dev_get_drvdata(dev);
+	struct imx471 *sensor = to_imx471(sd);
+
+	clk_disable_unprepare(sensor->img_clk);
+	gpiod_set_value_cansleep(sensor->reset_gpio, 1);
+
+	regulator_bulk_disable(IMX471_NUM_SUPPLIES, sensor->supplies);
+
+	return 0;
+}
+
+static int imx471_power_on(struct device *dev)
+{
+	struct v4l2_subdev *sd = dev_get_drvdata(dev);
+	struct imx471 *sensor = to_imx471(sd);
+	int ret;
+
+	ret = regulator_bulk_enable(IMX471_NUM_SUPPLIES, sensor->supplies);
+	if (ret < 0) {
+		dev_err(dev, "failed to enable regulators: %d\n", ret);
+		return ret;
+	}
+
+	ret = clk_prepare_enable(sensor->img_clk);
+	if (ret < 0) {
+		regulator_bulk_disable(IMX471_NUM_SUPPLIES, sensor->supplies);
+		dev_err(dev, "failed to enable imaging clock: %d", ret);
+		return ret;
+	}
+
+	gpiod_set_value_cansleep(sensor->reset_gpio, 0);
+
+	usleep_range(10000, 15000);
+
+	return 0;
+}
+
+/* Start streaming */
+static int imx471_enable_stream(struct v4l2_subdev *sd,
+				struct v4l2_subdev_state *state,
+				u32 pad, u64 streams_mask)
+{
+	struct imx471 *sensor = to_imx471(sd);
+	const struct imx471_mode *mode;
+	struct v4l2_mbus_framefmt *fmt;
+	int ret;
+
+	ret = pm_runtime_resume_and_get(sensor->dev);
+	if (ret)
+		goto error_powerdown;
+
+	ret = imx471_identify_module(sensor);
+	if (ret)
+		return ret;
+
+	/* Global Setting */
+	cci_multi_reg_write(sensor->regmap, imx471_global_regs,
+			    ARRAY_SIZE(imx471_global_regs), &ret);
+	if (ret) {
+		dev_err(sensor->dev, "failed to set global settings");
+		goto error_powerdown;
+	}
+
+	state = v4l2_subdev_get_locked_active_state(&sensor->sd);
+	fmt = v4l2_subdev_state_get_format(state, 0);
+	mode = v4l2_find_nearest_size(imx471_modes, ARRAY_SIZE(imx471_modes),
+				      width, height, fmt->width, fmt->height);
+
+	/* Apply default values of current mode */
+	cci_multi_reg_write(sensor->regmap, mode->default_mode_regs,
+			    mode->default_mode_regs_length, &ret);
+	if (ret) {
+		dev_err(sensor->dev, "failed to set mode");
+		goto error_powerdown;
+	}
+
+	/* set digital gain control to all color mode */
+	cci_write(sensor->regmap, IMX471_REG_DPGA_USE_GLOBAL_GAIN, 1, &ret);
+	if (ret)
+		goto error_powerdown;
+
+	/* Apply customized values from user */
+	ret =  __v4l2_ctrl_handler_setup(&sensor->ctrl_handler);
+	if (ret)
+		goto error_powerdown;
+
+	cci_write(sensor->regmap, IMX471_REG_MODE_SELECT,
+		  IMX471_MODE_STREAMING, &ret);
+	if (ret)
+		goto error_powerdown;
+
+	__v4l2_ctrl_grab(sensor->vflip, true);
+	__v4l2_ctrl_grab(sensor->hflip, true);
+
+	return ret;
+
+error_powerdown:
+	pm_runtime_put(sensor->dev);
+
+	return ret;
+}
+
+/* Stop streaming */
+static int imx471_disable_stream(struct v4l2_subdev *sd,
+				 struct v4l2_subdev_state *state,
+				 u32 pad, u64 streams_mask)
+{
+	struct imx471 *sensor = to_imx471(sd);
+	int ret;
+
+	cci_write(sensor->regmap, IMX471_REG_MODE_SELECT,
+		  IMX471_MODE_STANDBY, &ret);
+	pm_runtime_put(sensor->dev);
+
+	if (ret)
+		dev_err(sensor->dev,
+			"failed to disable stream with return value: %d\n",
+			ret);
+	__v4l2_ctrl_grab(sensor->vflip, false);
+	__v4l2_ctrl_grab(sensor->hflip, false);
+
+	return 0;
+}
+
+static const struct v4l2_subdev_core_ops imx471_subdev_core_ops = {
+	.subscribe_event = v4l2_ctrl_subdev_subscribe_event,
+	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
+};
+
+static const struct v4l2_subdev_video_ops imx471_video_ops = {
+	.s_stream = v4l2_subdev_s_stream_helper,
+};
+
+static const struct v4l2_subdev_pad_ops imx471_pad_ops = {
+	.enum_mbus_code = imx471_enum_mbus_code,
+	.get_fmt = v4l2_subdev_get_fmt,
+	.set_fmt = imx471_set_pad_format,
+	.get_selection = imx471_get_selection,
+	.enum_frame_size = imx471_enum_frame_size,
+	.enable_streams = imx471_enable_stream,
+	.disable_streams = imx471_disable_stream,
+};
+
+static const struct v4l2_subdev_ops imx471_subdev_ops = {
+	.core = &imx471_subdev_core_ops,
+	.video = &imx471_video_ops,
+	.pad = &imx471_pad_ops,
+};
+
+static const struct v4l2_subdev_internal_ops imx471_internal_ops = {
+	.init_state = imx471_init_state,
+};
+
+/* Initialize control handlers */
+static int imx471_init_controls(struct imx471 *sensor)
+{
+	const struct imx471_mode *mode = &imx471_modes[0];
+	struct v4l2_ctrl_handler *ctrl_hdlr;
+	struct v4l2_fwnode_device_properties props;
+	s64 exposure_max, hblank;
+	u64 pixel_rate;
+	int ret;
+
+	ctrl_hdlr = &sensor->ctrl_handler;
+	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 10);
+	if (ret)
+		return ret;
+
+	ret = v4l2_fwnode_device_parse(sensor->dev, &props);
+	if (ret) {
+		dev_err(sensor->dev, "failed to parse fwnode: %d", ret);
+		return ret;
+	}
+
+	v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &imx471_ctrl_ops, &props);
+
+	sensor->link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr,
+						   &imx471_ctrl_ops,
+						   V4L2_CID_LINK_FREQ,
+						   ARRAY_SIZE(link_freq_menu_items) - 1,
+						   0,
+						   link_freq_menu_items);
+	if (sensor->link_freq)
+		sensor->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+
+	/* pixel_rate = link_freq * 2 * nr_of_lanes / bits_per_sample */
+	pixel_rate = IMX471_LINK_FREQ_DEFAULT * 2 * 4;
+	div_u64(pixel_rate, 10);
+	/* By default, PIXEL_RATE is read only */
+	sensor->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &imx471_ctrl_ops,
+					       V4L2_CID_PIXEL_RATE, pixel_rate,
+					       pixel_rate, 1, pixel_rate);
+
+	/* Initial vblank/hblank/exposure parameters based on current mode */
+	sensor->vblank = v4l2_ctrl_new_std(ctrl_hdlr,
+					   &imx471_ctrl_ops,
+					   V4L2_CID_VBLANK,
+					   mode->fll_min - mode->height,
+					   IMX471_FLL_MAX - mode->height,
+					   1,
+					   mode->fll_def - mode->height);
+
+	hblank = mode->llp - mode->width;
+	sensor->hblank = v4l2_ctrl_new_std(ctrl_hdlr, &imx471_ctrl_ops,
+					   V4L2_CID_HBLANK, hblank, hblank,
+					   1, hblank);
+	if (sensor->hblank)
+		sensor->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+
+	/* fll >= exposure time + adjust parameter (default value is 18) */
+	exposure_max = mode->fll_def - IMX471_EXPOSURE_MARGIN;
+	sensor->exposure = v4l2_ctrl_new_std(ctrl_hdlr, &imx471_ctrl_ops,
+					     V4L2_CID_EXPOSURE,
+					     IMX471_EXPOSURE_MIN, exposure_max,
+					     IMX471_EXPOSURE_STEP,
+					     IMX471_EXPOSURE_DEFAULT);
+
+	v4l2_ctrl_new_std(ctrl_hdlr, &imx471_ctrl_ops, V4L2_CID_ANALOGUE_GAIN,
+			  IMX471_ANA_GAIN_MIN, IMX471_ANA_GAIN_MAX,
+			  IMX471_ANA_GAIN_STEP, IMX471_ANA_GAIN_DEFAULT);
+
+	/* Digital gain */
+	v4l2_ctrl_new_std(ctrl_hdlr, &imx471_ctrl_ops, V4L2_CID_DIGITAL_GAIN,
+			  IMX471_DGTL_GAIN_MIN, IMX471_DGTL_GAIN_MAX,
+			  IMX471_DGTL_GAIN_STEP, IMX471_DGTL_GAIN_DEFAULT);
+
+	v4l2_ctrl_new_std_menu_items(ctrl_hdlr, &imx471_ctrl_ops,
+				     V4L2_CID_TEST_PATTERN,
+				     ARRAY_SIZE(imx471_test_pattern_menu) - 1,
+				     0, 0, imx471_test_pattern_menu);
+
+	/* HFLIP & VFLIP */
+	sensor->hflip = v4l2_ctrl_new_std(ctrl_hdlr, &imx471_ctrl_ops,
+					  V4L2_CID_HFLIP, 0, 1, 1, 0);
+
+	sensor->vflip = v4l2_ctrl_new_std(ctrl_hdlr, &imx471_ctrl_ops,
+					  V4L2_CID_VFLIP, 0, 1, 1, 0);
+
+	if (ctrl_hdlr->error) {
+		dev_err(sensor->dev, "%s control init failed: %d",
+			__func__, ctrl_hdlr->error);
+		goto error;
+	}
+
+	sensor->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
+	sensor->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
+
+	sensor->sd.ctrl_handler = ctrl_hdlr;
+
+	return 0;
+
+error:
+	v4l2_ctrl_handler_free(ctrl_hdlr);
+
+	return ctrl_hdlr->error;
+}
+
+static int imx471_check_hwcfg(struct imx471 *sensor)
+{
+	struct v4l2_fwnode_endpoint bus_cfg = {
+		.bus_type = V4L2_MBUS_CSI2_DPHY,
+	};
+	struct fwnode_handle *ep, *fwnode = dev_fwnode(sensor->dev);
+	struct clk *clk;
+	unsigned long link_freq_bitmap;
+	int ret;
+
+	clk = devm_v4l2_sensor_clk_get(sensor->dev, NULL);
+	if (IS_ERR(clk))
+		return dev_err_probe(sensor->dev, PTR_ERR(clk),
+				     "can't get clock frequency\n");
+
+	if (clk_get_rate(clk) != IMX471_EXT_CLK)
+		return dev_err_probe(sensor->dev, -EINVAL,
+				     "external clock %lu is not supported\n",
+				     clk_get_rate(clk));
+
+	ep = fwnode_graph_get_endpoint_by_id(fwnode, 0, 0, 0);
+	ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg);
+	fwnode_handle_put(ep);
+	if (ret)
+		return dev_err_probe(sensor->dev, ret,
+				     "parsing endpoint failed");
+
+	ret = v4l2_link_freq_to_bitmap(sensor->dev, bus_cfg.link_frequencies,
+				       bus_cfg.nr_of_link_frequencies,
+				       link_freq_menu_items,
+				       ARRAY_SIZE(link_freq_menu_items),
+				       &link_freq_bitmap);
+
+	v4l2_fwnode_endpoint_free(&bus_cfg);
+
+	return ret;
+}
+
+static int imx471_probe(struct i2c_client *client)
+{
+	struct imx471 *sensor;
+	int ret;
+
+	sensor = devm_kzalloc(&client->dev, sizeof(*sensor), GFP_KERNEL);
+	if (!sensor)
+		return dev_err_probe(&client->dev, -ENOMEM,
+				     "failed to allocate memory\n");
+
+	sensor->dev = &client->dev;
+
+	/* Check HW config */
+	ret = imx471_check_hwcfg(sensor);
+	if (ret)
+		return dev_err_probe(sensor->dev, ret,
+				     "failed to check hwcfg: %d\n", ret);
+
+	ret = imx471_get_regulators(sensor->dev, sensor);
+	if (ret)
+		return dev_err_probe(sensor->dev, ret,
+				     "failed to get regulators\n");
+
+	sensor->reset_gpio = devm_gpiod_get_optional(sensor->dev, "reset",
+						     GPIOD_OUT_HIGH);
+	if (IS_ERR(sensor->reset_gpio))
+		return dev_err_probe(sensor->dev, PTR_ERR(sensor->reset_gpio),
+				     "failed to get reset gpio\n");
+
+	sensor->img_clk = devm_clk_get_optional(sensor->dev, NULL);
+	if (IS_ERR(sensor->img_clk))
+		return dev_err_probe(sensor->dev, PTR_ERR(sensor->img_clk),
+				     "failed to get imaging clock\n");
+
+	/* Initialize subdev */
+	v4l2_i2c_subdev_init(&sensor->sd, client, &imx471_subdev_ops);
+
+	/* Initialize regmap */
+	sensor->regmap = devm_cci_regmap_init_i2c(client, 16);
+	if (IS_ERR(sensor->regmap))
+		return PTR_ERR(sensor->regmap);
+
+	ret = imx471_power_on(sensor->dev);
+	if (ret)
+		return dev_err_probe(sensor->dev, ret,
+				     "failed to power on\n");
+
+	/* Check module identity */
+	ret = imx471_identify_module(sensor);
+	if (ret) {
+		dev_err(&client->dev, "failed to find sensor: %d", ret);
+		goto error_power_off;
+	}
+
+	ret = imx471_init_controls(sensor);
+	if (ret) {
+		dev_err(sensor->dev, "failed to init controls: %d", ret);
+		goto error_power_off;
+	}
+
+	/* Initialize subdev */
+	sensor->sd.internal_ops = &imx471_internal_ops;
+	sensor->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
+			    V4L2_SUBDEV_FL_HAS_EVENTS;
+	sensor->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
+
+	/* Initialize source pad */
+	sensor->pad.flags = MEDIA_PAD_FL_SOURCE;
+	ret = media_entity_pads_init(&sensor->sd.entity, 1, &sensor->pad);
+	if (ret) {
+		dev_err(&client->dev, "failed to init entity pads: %d", ret);
+		goto error_v4l2_ctrl_handler_free;
+	}
+
+	sensor->sd.state_lock = sensor->ctrl_handler.lock;
+	ret = v4l2_subdev_init_finalize(&sensor->sd);
+	if (ret < 0) {
+		dev_err(&client->dev, "failed to init subdev: %d", ret);
+		goto error_media_entity_pm;
+	}
+
+	pm_runtime_set_active(sensor->dev);
+	pm_runtime_enable(sensor->dev);
+	pm_runtime_idle(sensor->dev);
+
+	ret = v4l2_async_register_subdev_sensor(&sensor->sd);
+	if (ret < 0)
+		goto error_v4l2_subdev_cleanup;
+
+	return 0;
+
+error_v4l2_subdev_cleanup:
+	pm_runtime_disable(sensor->dev);
+	pm_runtime_set_suspended(sensor->dev);
+	v4l2_subdev_cleanup(&sensor->sd);
+
+error_media_entity_pm:
+	media_entity_cleanup(&sensor->sd.entity);
+
+error_v4l2_ctrl_handler_free:
+	v4l2_ctrl_handler_free(sensor->sd.ctrl_handler);
+
+error_power_off:
+	imx471_power_off(sensor->dev);
+
+	return ret;
+}
+
+static void imx471_remove(struct i2c_client *client)
+{
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct imx471 *sensor = to_imx471(sd);
+
+	v4l2_async_unregister_subdev(sd);
+	v4l2_subdev_cleanup(sd);
+	media_entity_cleanup(&sd->entity);
+	v4l2_ctrl_handler_free(sd->ctrl_handler);
+
+	pm_runtime_disable(&client->dev);
+
+	if (!pm_runtime_status_suspended(sensor->dev)) {
+		imx471_power_off(sensor->dev);
+		pm_runtime_set_suspended(sensor->dev);
+	}
+}
+
+static DEFINE_RUNTIME_DEV_PM_OPS(imx471_pm_ops, imx471_power_off,
+				 imx471_power_on, NULL);
+
+static const struct acpi_device_id imx471_acpi_ids[] __maybe_unused = {
+	{ "SONY471A" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(acpi, imx471_acpi_ids);
+
+static struct i2c_driver imx471_i2c_driver = {
+	.driver = {
+		.name = "imx471",
+		.acpi_match_table = ACPI_PTR(imx471_acpi_ids),
+		.pm = pm_sleep_ptr(&imx471_pm_ops),
+	},
+	.probe = imx471_probe,
+	.remove = imx471_remove,
+};
+module_i2c_driver(imx471_i2c_driver);
+
+MODULE_AUTHOR("Jimmy Su <jimmy.su@intel.com>");
+MODULE_AUTHOR("Serin Yeh <serin.yeh@intel.com>");
+MODULE_AUTHOR("Kate Hsuan <hpa@redhat.com>");
+MODULE_DESCRIPTION("Sony imx471 sensor driver");
+MODULE_LICENSE("GPL");
-- 
2.54.0

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

[Tarang Raval]

Hi Kate,

I noticed a few issues. Please check the comments below.

> 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 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.
>
> Signed-off-by: Kate Hsuan <hpa@redhat.com>
> ---
>  MAINTAINERS                |    6 +
>  drivers/media/i2c/Kconfig  |   10 +
>  drivers/media/i2c/Makefile |    1 +
>  drivers/media/i2c/imx471.c | 1006 ++++++++++++++++++++++++++++++++++++
>  4 files changed, 1023 insertions(+)
>  create mode 100644 drivers/media/i2c/imx471.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 1126fdd639ad..d597337e7c24 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -24735,6 +24735,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 5eb1e0e0a87a..1c28c498a9f1 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.
> +

The indentation is wrong here. Please fix it.

>  config VIDEO_MAX9271_LIB
> 	tristate
>
> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> index a3a6396df3c4..0539e9171030 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..f3c7fdce2d50
> --- /dev/null
> +++ b/drivers/media/i2c/imx471.c
> @@ -0,0 +1,1006 @@
> +// 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>
> +#include <media/v4l2-cci.h>
> +#include <media/v4l2-ctrls.h>
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-event.h>
> +#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                        0x04f6

Better to use a decimal value here.

> +
> +/*
> + *  the digital control register for all color control looks like:
> + *  +-----------------+------------------+
> + *  |      [7:0]      |       [15:8]     |
> + *  +-----------------+------------------+
> + *  |    0x020f      |       0x020e     |
> + *  --------------------------------------
> + *  it is used to calculate the digital gain times value(integral + fractional)
> + *  the [15:8] bits is the fractional part and [7:0] bits is the integral
> + *  calculation equation is:
> + *      gain value (unit: times) = REG[15:8] + REG[7:0]/0x100
> + *  Only value in 0x0100 ~ 0x0FFF range is allowed.
> + *  Analog gain use 10 bits in the registers and allowed range is 0 ~ 960
> + */
> +/* Analog gain control */
> +#define IMX471_REG_ANALOG_GAIN                 CCI_REG16(0x0204)
> +#define IMX471_ANA_GAIN_MIN                    0
> +#define IMX471_ANA_GAIN_MAX                    960
> +#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
> +
> +#define IMX471_VALUE_08BIT                     1

Unused macro, please remove it.

> +
> +/* HFLIP and VFLIP control */
> +#define IMX471_REG_ORIENTATION                 CCI_REG8(0x0101)
> +#define IMX471_HFLIP_BIT                       BIT(0)
> +#define IMX471_VFLIP_BIT                       BIT(1)
> +
> +/* Default exposure margin */
> +#define IMX471_EXPOSURE_MARGIN                 18

Please move this macro to the exposure block above.

> +
> +/* Horizontal crop window offset */
> +#define IMX471_REG_H_WIN_OFFSET                        CCI_REG8(0x0409)
> +
> +/* Vertical crop window offset */
> +#define IMX471_REG_V_WIN_OFFSET                        CCI_REG8(0x034b)
> +
> +/* 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
> +
> +/* default link frequency and external clock */
> +#define IMX471_LINK_FREQ_DEFAULT               200000000LL
> +#define IMX471_EXT_CLK                         19200000
> +#define IMX471_LINK_FREQ_INDEX                 0
> +
> +/* IMX471 native and active pixel array size */
> +#define IMX471_NATIVE_WIDTH                    4672
> +#define IMX471_NATIVE_HEIGHT                   3512
> +#define IMX471_PIXEL_ARRAY_LEFT                        8
> +#define IMX471_PIXEL_ARRAY_TOP                 8
> +#define IMX471_PIXEL_ARRAY_WIDTH               4656
> +#define IMX471_PIXEL_ARRAY_HEIGHT              3496
> +
> +#define to_imx471(_sd) container_of_const(_sd, struct imx471, sd)
> +
> +static const char * const imx471_supply_name[] = {
> +       "avdd",
> +};
> +
> +#define IMX471_NUM_SUPPLIES                    ARRAY_SIZE(imx471_supply_name)
> +
> +/* Mode : resolution and related config&values */
> +struct imx471_mode {
> +       /* Frame width */
> +       u32 width;
> +       /* Frame height */
> +       u32 height;
> +
> +       /* V-timing */
> +       u32 fll_def;
> +       u32 fll_min;
> +
> +       /* H-timing */
> +       u32 llp;
> +
> +       /* index of link frequency */
> +       u32 link_freq_index;
> +
> +       /* Default register values */
> +       const struct cci_reg_sequence *default_mode_regs;
> +       const int default_mode_regs_length;
> +};
> +
> +struct imx471 {
> +       struct v4l2_subdev sd;
> +       struct media_pad pad;
> +
> +       struct v4l2_ctrl_handler ctrl_handler;
> +       /* V4L2 Controls */
> +       struct v4l2_ctrl *link_freq;
> +       struct v4l2_ctrl *pixel_rate;
> +       struct v4l2_ctrl *vblank;
> +       struct v4l2_ctrl *hblank;
> +       struct v4l2_ctrl *vflip;
> +       struct v4l2_ctrl *hflip;
> +       struct v4l2_ctrl *exposure;
> +
> +       struct gpio_desc *reset_gpio;
> +       struct regulator_bulk_data supplies[IMX471_NUM_SUPPLIES];
> +       struct clk *img_clk;
> +
> +       struct device *dev;
> +       struct regmap *regmap;
> +};
> +
> +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 },
> +       { CCI_REG8(0x4431), 0x04 },
> +       { CCI_REG8(0x531c), 0x01 },
> +       { CCI_REG8(0x531d), 0x02 },
> +       { CCI_REG8(0x531e), 0x04 },
> +       { CCI_REG8(0x5928), 0x00 },
> +       { CCI_REG8(0x5929), 0x2f },
> +       { CCI_REG8(0x592a), 0x00 },
> +       { CCI_REG8(0x592b), 0x85 },
> +       { CCI_REG8(0x592c), 0x00 },
> +       { CCI_REG8(0x592d), 0x32 },
> +       { CCI_REG8(0x592e), 0x00 },
> +       { CCI_REG8(0x592f), 0x88 },
> +       { CCI_REG8(0x5930), 0x00 },
> +       { CCI_REG8(0x5931), 0x3d },
> +       { CCI_REG8(0x5932), 0x00 },
> +       { CCI_REG8(0x5933), 0x93 },
> +       { CCI_REG8(0x5938), 0x00 },
> +       { CCI_REG8(0x5939), 0x24 },
> +       { CCI_REG8(0x593a), 0x00 },
> +       { CCI_REG8(0x593b), 0x7a },
> +       { CCI_REG8(0x593c), 0x00 },
> +       { CCI_REG8(0x593d), 0x24 },
> +       { CCI_REG8(0x593e), 0x00 },
> +       { CCI_REG8(0x593f), 0x7a },
> +       { CCI_REG8(0x5940), 0x00 },
> +       { CCI_REG8(0x5941), 0x2f },
> +       { CCI_REG8(0x5942), 0x00 },
> +       { CCI_REG8(0x5943), 0x85 },
> +       { CCI_REG8(0x5f0e), 0x6e },
> +       { CCI_REG8(0x5f11), 0xc6 },
> +       { CCI_REG8(0x5f17), 0x5e },
> +       { CCI_REG8(0x7990), 0x01 },
> +       { CCI_REG8(0x7993), 0x5d },
> +       { CCI_REG8(0x7994), 0x5d },
> +       { CCI_REG8(0x7995), 0xa1 },
> +       { CCI_REG8(0x799a), 0x01 },
> +       { CCI_REG8(0x799d), 0x00 },
> +       { CCI_REG8(0x8169), 0x01 },
> +       { CCI_REG8(0x8359), 0x01 },
> +       { CCI_REG8(0x9302), 0x1e },
> +       { CCI_REG8(0x9306), 0x1f },
> +       { CCI_REG8(0x930a), 0x26 },
> +       { CCI_REG8(0x930e), 0x23 },
> +       { CCI_REG8(0x9312), 0x23 },
> +       { CCI_REG8(0x9316), 0x2c },
> +       { CCI_REG8(0x9317), 0x19 },
> +       { CCI_REG8(0xb046), 0x01 },
> +       { CCI_REG8(0xb048), 0x01 },
> +};
> +
> +static const struct cci_reg_sequence mode_1928x1088_regs[] = {
> +       { CCI_REG8(0x0101), 0x00 },
> +       { CCI_REG8(0x0112), 0x0a },
> +       { CCI_REG8(0x0113), 0x0a },
> +       { CCI_REG8(0x0114), 0x03 },
> +       { CCI_REG8(0x0342), 0x0a },
> +       { CCI_REG8(0x0343), 0x00 },
> +       { CCI_REG8(0x0340), 0x13 },
> +       { CCI_REG8(0x0341), 0xb0 },

These are V-timing settings. You can drop them since they are already
being set from the control handler.

> +       { CCI_REG8(0x0344), 0x00 },
> +       { CCI_REG8(0x0345), 0x00 },
> +       { CCI_REG8(0x0346), 0x01 },
> +       { CCI_REG8(0x0347), 0xbc },
> +       { CCI_REG8(0x0348), 0x12 },
> +       { CCI_REG8(0x0349), 0x2f },
> +       { CCI_REG8(0x034a), 0x0b },
> +       { CCI_REG8(0x034b), 0xeb },
> +       { CCI_REG8(0x0381), 0x01 },
> +       { CCI_REG8(0x0383), 0x01 },
> +       { CCI_REG8(0x0385), 0x01 },
> +       { CCI_REG8(0x0387), 0x01 },
> +       { CCI_REG8(0x0900), 0x01 },
> +       { CCI_REG8(0x0901), 0x22 },
> +       { CCI_REG8(0x0902), 0x08 },
> +       { CCI_REG8(0x3f4c), 0x81 },
> +       { CCI_REG8(0x3f4d), 0x81 },
> +       { CCI_REG8(0x0408), 0x00 },
> +       { CCI_REG8(0x0409), 0xc8 },
> +       { CCI_REG8(0x040a), 0x00 },
> +       { CCI_REG8(0x040b), 0x6c },
> +       { CCI_REG8(0x040c), 0x07 },
> +       { CCI_REG8(0x040d), 0x88 },
> +       { CCI_REG8(0x040e), 0x04 },
> +       { CCI_REG8(0x040f), 0x40 },
> +       { CCI_REG8(0x034c), 0x07 },
> +       { CCI_REG8(0x034d), 0x88 },
> +       { CCI_REG8(0x034e), 0x04 },
> +       { CCI_REG8(0x034f), 0x40 },
> +       { CCI_REG8(0x0301), 0x06 },
> +       { CCI_REG8(0x0303), 0x02 },
> +       { CCI_REG8(0x0305), 0x02 },
> +       { CCI_REG8(0x0306), 0x00 },
> +       { CCI_REG8(0x0307), 0x79 },
> +       { CCI_REG8(0x030b), 0x01 },
> +       { CCI_REG8(0x030d), 0x02 },
> +       { CCI_REG8(0x030e), 0x00 },
> +       { CCI_REG8(0x030f), 0x53 },
> +       { CCI_REG8(0x0310), 0x01 },
> +       { CCI_REG8(0x0202), 0x13 },
> +       { CCI_REG8(0x0203), 0x9e },

Similarly, these are exposure settings. You can drop.

> +       { CCI_REG8(0x0204), 0x00 },
> +       { CCI_REG8(0x0205), 0x00 },

These are digital gain settings. You can drop.

> +       { CCI_REG8(0x020e), 0x01 },
> +       { CCI_REG8(0x020f), 0x00 },

These are analog gain settings. You can drop.

> +       { CCI_REG8(0x3f78), 0x01 },
> +       { CCI_REG8(0x3f79), 0x31 },
> +       { CCI_REG8(0x3ffe), 0x00 },
> +       { CCI_REG8(0x3fff), 0x8a },
> +       { CCI_REG8(0x5f0a), 0xb6 },
> +};
> +
> +static const char * const imx471_test_pattern_menu[] = {
> +       "Disabled",
> +       "Solid Colour",
> +       "Eight Vertical Colour Bars",
> +       "Colour Bars With Fade to Grey",
> +       "Pseudorandom Sequence (PN9)",
> +};
> +
> +/*
> + * When adding more than the one below, make sure the disallowed ones will
> + * actually be disabled in the LINK_FREQ control.
> + */
> +static const s64 link_freq_menu_items[] = {
> +       IMX471_LINK_FREQ_DEFAULT,
> +};
> +
> +/*
> + * The Bayer formats for the flipping.
> + * - no flip
> + * - h flip
> + * - v flip
> + * - h and v flips
> + */
> +static const u32 imx471_hv_flips_bayer_order[] = {
> +       MEDIA_BUS_FMT_SRGGB10_1X10,
> +       MEDIA_BUS_FMT_SGRBG10_1X10,
> +       MEDIA_BUS_FMT_SGBRG10_1X10,
> +       MEDIA_BUS_FMT_SBGGR10_1X10,
> +};
> +
> +/* Mode configs */
> +static const struct imx471_mode imx471_modes[] = {
> +       {
> +               .width = 1928,
> +               .height = 1088,
> +               .fll_def = 1308,
> +               .fll_min = 1308,
> +               .llp = 2328,
> +               .link_freq_index = IMX471_LINK_FREQ_INDEX,
> +               .default_mode_regs = mode_1928x1088_regs,
> +               .default_mode_regs_length = ARRAY_SIZE(mode_1928x1088_regs),
> +       },
> +};
> +
> +static int imx471_get_regulators(struct device *dev, struct imx471 *sensor)
> +{
> +       for (unsigned int  i = 0; i < IMX471_NUM_SUPPLIES; i++)
> +               sensor->supplies[i].supply = imx471_supply_name[i];
> +
> +       return devm_regulator_bulk_get(dev, IMX471_NUM_SUPPLIES,
> +                                      sensor->supplies);
> +}
> +
> +static int imx471_set_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +       struct imx471 *sensor = container_of(ctrl->handler,
> +                                                 struct imx471,
> +                                                 ctrl_handler);

Use container_of_const.

> +       struct v4l2_subdev_state *state =
> +                       v4l2_subdev_get_locked_active_state(&sensor->sd);
> +       const struct v4l2_mbus_framefmt *format =
> +                       v4l2_subdev_state_get_format(state, 0);
> +       s64 exposure_max;
> +       int ret;

ret = 0;

> +
> +       /* Propagate change of current control to all related controls */
> +       if (ctrl->id == V4L2_CID_VBLANK) {
> +               /* Update max exposure while meeting expected vblanking */
> +               exposure_max =
> +                       format->height + ctrl->val - IMX471_EXPOSURE_MARGIN;
> +               __v4l2_ctrl_modify_range(sensor->exposure,
> +                                        sensor->exposure->minimum,
> +                                        exposure_max,
> +                                        sensor->exposure->step,
> +                                        exposure_max);

This control operation can fail. Please check the return value.

> +       }
> +
> +       /* V4L2 controls values will be applied only when power is already up */
> +       if (!pm_runtime_get_if_in_use(sensor->dev))
> +               return 0;
> +
> +       switch (ctrl->id) {
> +       case V4L2_CID_ANALOGUE_GAIN:
> +               cci_write(sensor->regmap, IMX471_REG_ANALOG_GAIN,
> +                         ctrl->val, &ret);

You are using ret for the first time here, Please initialize ret with 0 when
declaring it.

cci_write() uses the value pointed by &ret to determine whether a previous
error has already occurred, and an uninitialized ret may contain a garbage
value, causing the write operation to fail unexpectedly.

> +               break;
> +       case V4L2_CID_DIGITAL_GAIN:
> +               cci_write(sensor->regmap, IMX471_REG_DIG_GAIN_GLOBAL,
> +                         ctrl->val, &ret);
> +               break;
> +       case V4L2_CID_EXPOSURE:
> +               cci_write(sensor->regmap, IMX471_REG_EXPOSURE,
> +                         ctrl->val, &ret);
> +               break;
> +       case V4L2_CID_VBLANK:
> +               /* Update FLL that meets expected vertical blanking */
> +               cci_write(sensor->regmap, IMX471_REG_FLL,
> +                         format->height + ctrl->val, &ret);
> +               break;
> +       case V4L2_CID_TEST_PATTERN:
> +               cci_write(sensor->regmap, IMX471_REG_TEST_PATTERN,
> +                         ctrl->val, &ret);
> +               break;
> +       case V4L2_CID_HFLIP:
> +       case V4L2_CID_VFLIP:
> +               cci_write(sensor->regmap, IMX471_REG_ORIENTATION,
> +                         sensor->hflip->val | sensor->vflip->val << 1, &ret);
> +               break;
> +       default:
> +               ret = -EINVAL;
> +               dev_info(sensor->dev, "ctrl(id:0x%x,val:0x%x) is not handled",
> +                        ctrl->id, ctrl->val);
> +               break;
> +       }
> +
> +       pm_runtime_put(sensor->dev);
> +
> +       return ret;
> +}
> +
> +static const struct v4l2_ctrl_ops imx471_ctrl_ops = {
> +       .s_ctrl = imx471_set_ctrl,
> +};
> +
> +static u32 imx471_get_format_code(struct imx471 *sensor)
> +{
> +       unsigned int i;
> +
> +       i = (sensor->vflip->val ? 2 : 0) | (sensor->hflip->val ? 1 : 0);
> +
> +       return imx471_hv_flips_bayer_order[i];
> +}
> +
> +static int imx471_enum_mbus_code(struct v4l2_subdev *sd,
> +                                struct v4l2_subdev_state *sd_state,
> +                                struct v4l2_subdev_mbus_code_enum *code)
> +{
> +       struct imx471 *sensor = to_imx471(sd);
> +
> +       if (code->index >= (ARRAY_SIZE(imx471_hv_flips_bayer_order) / 4))
> +               return -EINVAL;
> +
> +       code->code = imx471_get_format_code(sensor);
> +
> +       return 0;
> +}

Please add one extra blank line after this line.

> +static int imx471_enum_frame_size(struct v4l2_subdev *sd,
> +                                 struct v4l2_subdev_state *sd_state,
> +                                 struct v4l2_subdev_frame_size_enum *fse)
> +{
> +       if (fse->index >= ARRAY_SIZE(imx471_modes))
> +               return -EINVAL;
> +
> +       fse->min_width = imx471_modes[fse->index].width;
> +       fse->max_width = fse->min_width;
> +       fse->min_height = imx471_modes[fse->index].height;
> +       fse->max_height = fse->min_height;
> +
> +       return 0;
> +}
> +
> +static void imx471_update_pad_format(struct imx471 *sensor,
> +                                    const struct imx471_mode *mode,
> +                                    struct v4l2_subdev_format *fmt)
> +{
> +       fmt->format.code = imx471_get_format_code(sensor);
> +       fmt->format.width = mode->width;
> +       fmt->format.height = mode->height;
> +       fmt->format.field = V4L2_FIELD_NONE;
> +}
> +
> +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;
> +       u64 pixel_rate;
> +
> +       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;
> +
> +       pixel_rate = IMX471_LINK_FREQ_DEFAULT * 2 * 4;
> +       div_u64(pixel_rate, 10);

You need to store the return value, as the above operation does not update
pixel_rate.

Please use:
pixel_rate = div_u64(IMX471_LINK_FREQ_DEFAULT * 2 * 4, 10);

> +       __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);

All the above control operations can fail. Please add proper error checks for them.

> +
> +       return 0;
> +}
> +
> +static int imx471_get_selection(struct v4l2_subdev *sd,
> +                               struct v4l2_subdev_state *sd_state,
> +                               struct v4l2_subdev_selection *sel)
> +{
> +       switch (sel->target) {
> +       case V4L2_SEL_TGT_CROP:
> +               sel->r = *v4l2_subdev_state_get_crop(sd_state, sel->pad);
> +               break;
> +
> +       case V4L2_SEL_TGT_NATIVE_SIZE:
> +               sel->r.top = 0;
> +               sel->r.left = 0;
> +               sel->r.width = IMX471_NATIVE_WIDTH;
> +               sel->r.height = IMX471_NATIVE_HEIGHT;
> +               return 0;
> +
> +       case V4L2_SEL_TGT_CROP_DEFAULT:
> +       case V4L2_SEL_TGT_CROP_BOUNDS:
> +               sel->r.top = IMX471_PIXEL_ARRAY_TOP;
> +               sel->r.left = IMX471_PIXEL_ARRAY_LEFT;
> +               sel->r.width = IMX471_PIXEL_ARRAY_WIDTH;
> +               sel->r.height = IMX471_PIXEL_ARRAY_HEIGHT;
> +               return 0;
> +       }
> +
> +       return -EINVAL;
> +}
> +
> +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,
> +               .format = {
> +                       .code = MEDIA_BUS_FMT_SRGGB10_1X10,
> +                       .width = imx471_modes[0].width,
> +                       .height = imx471_modes[0].height,
> +               },
> +       };
> +
> +       imx471_set_pad_format(sd, sd_state, &fmt);
> +
> +       return 0;

imx471_set_pad_format() can fail.

Please use:
return imx471_set_pad_format(sd, sd_state, &fmt);

> +}
> +
> +static int imx471_identify_module(struct imx471 *sensor)
> +{
> +       int ret;
> +       u64 val;
> +
> +       ret = cci_read(sensor->regmap, IMX471_REG_CHIP_ID, &val, NULL);
> +       if (ret)
> +               return dev_err_probe(sensor->dev, ret,
> +                                    "failed to read chip id\n");
> +
> +       if (val != IMX471_CHIP_ID)
> +               return dev_err_probe(sensor->dev, -EIO,
> +                                    "chip id mismatch: %x!=%llx\n",
> +                                    IMX471_CHIP_ID, val);
> +
> +       return 0;
> +}
> +
> +static int imx471_power_off(struct device *dev)
> +{
> +       struct v4l2_subdev *sd = dev_get_drvdata(dev);
> +       struct imx471 *sensor = to_imx471(sd);
> +
> +       clk_disable_unprepare(sensor->img_clk);
> +       gpiod_set_value_cansleep(sensor->reset_gpio, 1);
> +
> +       regulator_bulk_disable(IMX471_NUM_SUPPLIES, sensor->supplies);
> +
> +       return 0;
> +}
> +
> +static int imx471_power_on(struct device *dev)
> +{
> +       struct v4l2_subdev *sd = dev_get_drvdata(dev);
> +       struct imx471 *sensor = to_imx471(sd);
> +       int ret;
> +
> +       ret = regulator_bulk_enable(IMX471_NUM_SUPPLIES, sensor->supplies);
> +       if (ret < 0) {
> +               dev_err(dev, "failed to enable regulators: %d\n", ret);
> +               return ret;
> +       }
> +
> +       ret = clk_prepare_enable(sensor->img_clk);
> +       if (ret < 0) {
> +               regulator_bulk_disable(IMX471_NUM_SUPPLIES, sensor->supplies);
> +               dev_err(dev, "failed to enable imaging clock: %d", ret);
> +               return ret;
> +       }
> +
> +       gpiod_set_value_cansleep(sensor->reset_gpio, 0);
> +
> +       usleep_range(10000, 15000);
> +
> +       return 0;
> +}
> +
> +/* Start streaming */
> +static int imx471_enable_stream(struct v4l2_subdev *sd,
> +                               struct v4l2_subdev_state *state,
> +                               u32 pad, u64 streams_mask)
> +{
> +       struct imx471 *sensor = to_imx471(sd);
> +       const struct imx471_mode *mode;
> +       struct v4l2_mbus_framefmt *fmt;
> +       int ret;
> +
> +       ret = pm_runtime_resume_and_get(sensor->dev);
> +       if (ret)
> +               goto error_powerdown;
> +
> +       ret = imx471_identify_module(sensor);
> +       if (ret)
> +               return ret;
> +
> +       /* Global Setting */
> +       cci_multi_reg_write(sensor->regmap, imx471_global_regs,
> +                           ARRAY_SIZE(imx471_global_regs), &ret);
> +       if (ret) {
> +               dev_err(sensor->dev, "failed to set global settings");
> +               goto error_powerdown;
> +       }
> +
> +       state = v4l2_subdev_get_locked_active_state(&sensor->sd);
> +       fmt = v4l2_subdev_state_get_format(state, 0);
> +       mode = v4l2_find_nearest_size(imx471_modes, ARRAY_SIZE(imx471_modes),
> +                                     width, height, fmt->width, fmt->height);
> +
> +       /* Apply default values of current mode */
> +       cci_multi_reg_write(sensor->regmap, mode->default_mode_regs,
> +                           mode->default_mode_regs_length, &ret);
> +       if (ret) {
> +               dev_err(sensor->dev, "failed to set mode");
> +               goto error_powerdown;
> +       }
> +
> +       /* set digital gain control to all color mode */
> +       cci_write(sensor->regmap, IMX471_REG_DPGA_USE_GLOBAL_GAIN, 1, &ret);
> +       if (ret)
> +               goto error_powerdown;
> +
> +       /* Apply customized values from user */
> +       ret =  __v4l2_ctrl_handler_setup(&sensor->ctrl_handler);
> +       if (ret)
> +               goto error_powerdown;
> +
> +       cci_write(sensor->regmap, IMX471_REG_MODE_SELECT,
> +                 IMX471_MODE_STREAMING, &ret);
> +       if (ret)
> +               goto error_powerdown;
> +
> +       __v4l2_ctrl_grab(sensor->vflip, true);
> +       __v4l2_ctrl_grab(sensor->hflip, true);
> +
> +       return ret;
> +
> +error_powerdown:
> +       pm_runtime_put(sensor->dev);
> +
> +       return ret;
> +}
> +
> +/* Stop streaming */
> +static int imx471_disable_stream(struct v4l2_subdev *sd,
> +                                struct v4l2_subdev_state *state,
> +                                u32 pad, u64 streams_mask)
> +{
> +       struct imx471 *sensor = to_imx471(sd);
> +       int ret;
> +
> +       cci_write(sensor->regmap, IMX471_REG_MODE_SELECT,
> +                 IMX471_MODE_STANDBY, &ret);

If you use ret for the cci_write() error path, it must be initialized.

Here, you can directly get the error by using:
ret = cci_write(sensor->regmap, IMX471_REG_MODE_SELECT, IMX471_MODE_STANDBY, NULL);

> +       pm_runtime_put(sensor->dev);
> +
> +       if (ret)
> +               dev_err(sensor->dev,
> +                       "failed to disable stream with return value: %d\n",
> +                       ret);
> +       __v4l2_ctrl_grab(sensor->vflip, false);
> +       __v4l2_ctrl_grab(sensor->hflip, false);
> +
> +       return 0;
> +}
> +
> +static const struct v4l2_subdev_core_ops imx471_subdev_core_ops = {
> +       .subscribe_event = v4l2_ctrl_subdev_subscribe_event,
> +       .unsubscribe_event = v4l2_event_subdev_unsubscribe,
> +};

Drop this

See: https://lore.kernel.org/linux-media/20241029162106.3005800-1-tomm.merciai@gmail.com/

> +
> +static const struct v4l2_subdev_video_ops imx471_video_ops = {
> +       .s_stream = v4l2_subdev_s_stream_helper,
> +};
> +
> +static const struct v4l2_subdev_pad_ops imx471_pad_ops = {
> +       .enum_mbus_code = imx471_enum_mbus_code,
> +       .get_fmt = v4l2_subdev_get_fmt,
> +       .set_fmt = imx471_set_pad_format,
> +       .get_selection = imx471_get_selection,
> +       .enum_frame_size = imx471_enum_frame_size,
> +       .enable_streams = imx471_enable_stream,
> +       .disable_streams = imx471_disable_stream,
> +};
> +
> +static const struct v4l2_subdev_ops imx471_subdev_ops = {
> +       .core = &imx471_subdev_core_ops,
> +       .video = &imx471_video_ops,
> +       .pad = &imx471_pad_ops,
> +};
> +
> +static const struct v4l2_subdev_internal_ops imx471_internal_ops = {
> +       .init_state = imx471_init_state,
> +};
> +
> +/* Initialize control handlers */
> +static int imx471_init_controls(struct imx471 *sensor)
> +{
> +       const struct imx471_mode *mode = &imx471_modes[0];
> +       struct v4l2_ctrl_handler *ctrl_hdlr;
> +       struct v4l2_fwnode_device_properties props;
> +       s64 exposure_max, hblank;
> +       u64 pixel_rate;
> +       int ret;
> +
> +       ctrl_hdlr = &sensor->ctrl_handler;
> +       ret = v4l2_ctrl_handler_init(ctrl_hdlr, 10);
> +       if (ret)
> +               return ret;

You can skip this error check.

Also, there are 12 controls here, 10 controls + 2 orientation controls.

> +
> +       ret = v4l2_fwnode_device_parse(sensor->dev, &props);
> +       if (ret) {
> +               dev_err(sensor->dev, "failed to parse fwnode: %d", ret);
> +               return ret;
> +       }
> +
> +       v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &imx471_ctrl_ops, &props);
> +
> +       sensor->link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr,
> +                                                  &imx471_ctrl_ops,
> +                                                  V4L2_CID_LINK_FREQ,
> +                                                  ARRAY_SIZE(link_freq_menu_items) - 1,
> +                                                  0,
> +                                                  link_freq_menu_items);
> +       if (sensor->link_freq)
> +               sensor->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +
> +       /* pixel_rate = link_freq * 2 * nr_of_lanes / bits_per_sample */
> +       pixel_rate = IMX471_LINK_FREQ_DEFAULT * 2 * 4;
> +       div_u64(pixel_rate, 10);

As I mentioned above, please store the return value of div_u64():

pixel_rate = div_u64(IMX471_LINK_FREQ_DEFAULT * 2 * 4, 10);

> +       /* By default, PIXEL_RATE is read only */
> +       sensor->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &imx471_ctrl_ops,
> +                                              V4L2_CID_PIXEL_RATE, pixel_rate,
> +                                              pixel_rate, 1, pixel_rate);
> +
> +       /* Initial vblank/hblank/exposure parameters based on current mode */
> +       sensor->vblank = v4l2_ctrl_new_std(ctrl_hdlr,
> +                                          &imx471_ctrl_ops,
> +                                          V4L2_CID_VBLANK,
> +                                          mode->fll_min - mode->height,
> +                                          IMX471_FLL_MAX - mode->height,
> +                                          1,
> +                                          mode->fll_def - mode->height);
> +
> +       hblank = mode->llp - mode->width;
> +       sensor->hblank = v4l2_ctrl_new_std(ctrl_hdlr, &imx471_ctrl_ops,
> +                                          V4L2_CID_HBLANK, hblank, hblank,
> +                                          1, hblank);
> +       if (sensor->hblank)
> +               sensor->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +
> +       /* fll >= exposure time + adjust parameter (default value is 18) */
> +       exposure_max = mode->fll_def - IMX471_EXPOSURE_MARGIN;
> +       sensor->exposure = v4l2_ctrl_new_std(ctrl_hdlr, &imx471_ctrl_ops,
> +                                            V4L2_CID_EXPOSURE,
> +                                            IMX471_EXPOSURE_MIN, exposure_max,
> +                                            IMX471_EXPOSURE_STEP,
> +                                            IMX471_EXPOSURE_DEFAULT);
> +
> +       v4l2_ctrl_new_std(ctrl_hdlr, &imx471_ctrl_ops, V4L2_CID_ANALOGUE_GAIN,
> +                         IMX471_ANA_GAIN_MIN, IMX471_ANA_GAIN_MAX,
> +                         IMX471_ANA_GAIN_STEP, IMX471_ANA_GAIN_DEFAULT);
> +
> +       /* Digital gain */
> +       v4l2_ctrl_new_std(ctrl_hdlr, &imx471_ctrl_ops, V4L2_CID_DIGITAL_GAIN,
> +                         IMX471_DGTL_GAIN_MIN, IMX471_DGTL_GAIN_MAX,
> +                         IMX471_DGTL_GAIN_STEP, IMX471_DGTL_GAIN_DEFAULT);
> +
> +       v4l2_ctrl_new_std_menu_items(ctrl_hdlr, &imx471_ctrl_ops,
> +                                    V4L2_CID_TEST_PATTERN,
> +                                    ARRAY_SIZE(imx471_test_pattern_menu) - 1,
> +                                    0, 0, imx471_test_pattern_menu);
> +
> +       /* HFLIP & VFLIP */

I can see many comments in your code that are not very important.
You can remove them if you want, it's up to you.

> +       sensor->hflip = v4l2_ctrl_new_std(ctrl_hdlr, &imx471_ctrl_ops,
> +                                         V4L2_CID_HFLIP, 0, 1, 1, 0);
> +
> +       sensor->vflip = v4l2_ctrl_new_std(ctrl_hdlr, &imx471_ctrl_ops,
> +                                         V4L2_CID_VFLIP, 0, 1, 1, 0);
> +
> +       if (ctrl_hdlr->error) {
> +               dev_err(sensor->dev, "%s control init failed: %d",
> +                       __func__, ctrl_hdlr->error);
> +               goto error;
> +       }
> +
> +       sensor->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> +       sensor->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> +
> +       sensor->sd.ctrl_handler = ctrl_hdlr;
> +
> +       return 0;
> +
> +error:
> +       v4l2_ctrl_handler_free(ctrl_hdlr);
> +
> +       return ctrl_hdlr->error;
> +}
> +
> +static int imx471_check_hwcfg(struct imx471 *sensor)
> +{
> +       struct v4l2_fwnode_endpoint bus_cfg = {
> +               .bus_type = V4L2_MBUS_CSI2_DPHY,
> +       };
> +       struct fwnode_handle *ep, *fwnode = dev_fwnode(sensor->dev);
> +       struct clk *clk;
> +       unsigned long link_freq_bitmap;
> +       int ret;

If you want, you can sort the variable declarations throughout the code, where
appropriate, by length to make them more readable.

> +
> +       clk = devm_v4l2_sensor_clk_get(sensor->dev, NULL);
> +       if (IS_ERR(clk))
> +               return dev_err_probe(sensor->dev, PTR_ERR(clk),
> +                                    "can't get clock frequency\n");
> +
> +       if (clk_get_rate(clk) != IMX471_EXT_CLK)
> +               return dev_err_probe(sensor->dev, -EINVAL,
> +                                    "external clock %lu is not supported\n",
> +                                    clk_get_rate(clk));
> +
> +       ep = fwnode_graph_get_endpoint_by_id(fwnode, 0, 0, 0);
> +       ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg);
> +       fwnode_handle_put(ep);
> +       if (ret)
> +               return dev_err_probe(sensor->dev, ret,
> +                                    "parsing endpoint failed");
> +
> +       ret = v4l2_link_freq_to_bitmap(sensor->dev, bus_cfg.link_frequencies,
> +                                      bus_cfg.nr_of_link_frequencies,
> +                                      link_freq_menu_items,
> +                                      ARRAY_SIZE(link_freq_menu_items),
> +                                      &link_freq_bitmap);

This can fail silently. Please add an error message before returning the failure.

> +
> +       v4l2_fwnode_endpoint_free(&bus_cfg);
> +
> +       return ret;
> +}
> +
> +static int imx471_probe(struct i2c_client *client)
> +{
> +       struct imx471 *sensor;
> +       int ret;
> +
> +       sensor = devm_kzalloc(&client->dev, sizeof(*sensor), GFP_KERNEL);
> +       if (!sensor)
> +               return dev_err_probe(&client->dev, -ENOMEM,
> +                                    "failed to allocate memory\n");
> +
> +       sensor->dev = &client->dev;
> +
> +       /* Check HW config */
> +       ret = imx471_check_hwcfg(sensor);
> +       if (ret)
> +               return dev_err_probe(sensor->dev, ret,
> +                                    "failed to check hwcfg: %d\n", ret);
> +
> +       ret = imx471_get_regulators(sensor->dev, sensor);
> +       if (ret)
> +               return dev_err_probe(sensor->dev, ret,
> +                                    "failed to get regulators\n");
> +
> +       sensor->reset_gpio = devm_gpiod_get_optional(sensor->dev, "reset",
> +                                                    GPIOD_OUT_HIGH);
> +       if (IS_ERR(sensor->reset_gpio))
> +               return dev_err_probe(sensor->dev, PTR_ERR(sensor->reset_gpio),
> +                                    "failed to get reset gpio\n");
> +
> +       sensor->img_clk = devm_clk_get_optional(sensor->dev, NULL);
> +       if (IS_ERR(sensor->img_clk))
> +               return dev_err_probe(sensor->dev, PTR_ERR(sensor->img_clk),
> +                                    "failed to get imaging clock\n");
> +
> +       /* Initialize subdev */
> +       v4l2_i2c_subdev_init(&sensor->sd, client, &imx471_subdev_ops);
> +
> +       /* Initialize regmap */
> +       sensor->regmap = devm_cci_regmap_init_i2c(client, 16);
> +       if (IS_ERR(sensor->regmap))
> +               return PTR_ERR(sensor->regmap);

Please add an error message before returning the failure.

> +
> +       ret = imx471_power_on(sensor->dev);
> +       if (ret)
> +               return dev_err_probe(sensor->dev, ret,
> +                                    "failed to power on\n");
> +
> +       /* Check module identity */
> +       ret = imx471_identify_module(sensor);
> +       if (ret) {
> +               dev_err(&client->dev, "failed to find sensor: %d", ret);

Use dev_err_probe();

Also, add a newline character at the end of the print statement.

> +               goto error_power_off;
> +       }
> +
> +       ret = imx471_init_controls(sensor);
> +       if (ret) {
> +               dev_err(sensor->dev, "failed to init controls: %d", ret);

Same here.

> +               goto error_power_off;
> +       }
> +
> +       /* Initialize subdev */
> +       sensor->sd.internal_ops = &imx471_internal_ops;
> +       sensor->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
> +                           V4L2_SUBDEV_FL_HAS_EVENTS;

See: https://lore.kernel.org/linux-media/20241029162106.3005800-1-tomm.merciai@gmail.com/

> +       sensor->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> +
> +       /* Initialize source pad */
> +       sensor->pad.flags = MEDIA_PAD_FL_SOURCE;
> +       ret = media_entity_pads_init(&sensor->sd.entity, 1, &sensor->pad);
> +       if (ret) {
> +               dev_err(&client->dev, "failed to init entity pads: %d", ret);

Use dev_err_probe.

> +               goto error_v4l2_ctrl_handler_free;
> +       }
> +
> +       sensor->sd.state_lock = sensor->ctrl_handler.lock;
> +       ret = v4l2_subdev_init_finalize(&sensor->sd);
> +       if (ret < 0) {
> +               dev_err(&client->dev, "failed to init subdev: %d", ret);

Same here.

> +               goto error_media_entity_pm;
> +       }
> +
> +       pm_runtime_set_active(sensor->dev);
> +       pm_runtime_enable(sensor->dev);
> +       pm_runtime_idle(sensor->dev);
> +
> +       ret = v4l2_async_register_subdev_sensor(&sensor->sd);
> +       if (ret < 0)
> +               goto error_v4l2_subdev_cleanup;
> +
> +       return 0;
> +
> +error_v4l2_subdev_cleanup:
> +       pm_runtime_disable(sensor->dev);
> +       pm_runtime_set_suspended(sensor->dev);
> +       v4l2_subdev_cleanup(&sensor->sd);
> +
> +error_media_entity_pm:
> +       media_entity_cleanup(&sensor->sd.entity);
> +
> +error_v4l2_ctrl_handler_free:
> +       v4l2_ctrl_handler_free(sensor->sd.ctrl_handler);
> +
> +error_power_off:
> +       imx471_power_off(sensor->dev);
> +
> +       return ret;
> +}
> +
> +static void imx471_remove(struct i2c_client *client)
> +{
> +       struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +       struct imx471 *sensor = to_imx471(sd);
> +
> +       v4l2_async_unregister_subdev(sd);
> +       v4l2_subdev_cleanup(sd);
> +       media_entity_cleanup(&sd->entity);
> +       v4l2_ctrl_handler_free(sd->ctrl_handler);
> +
> +       pm_runtime_disable(&client->dev);
> +
> +       if (!pm_runtime_status_suspended(sensor->dev)) {
> +               imx471_power_off(sensor->dev);
> +               pm_runtime_set_suspended(sensor->dev);

You can use client->dev here and drop the struct imx471 *sensor = to_imx471(sd); line.

> +       }
> +}
> +
> +static DEFINE_RUNTIME_DEV_PM_OPS(imx471_pm_ops, imx471_power_off,
> +                                imx471_power_on, NULL);
> +
> +static const struct acpi_device_id imx471_acpi_ids[] __maybe_unused = {
> +       { "SONY471A" },
> +       { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(acpi, imx471_acpi_ids);
> +
> +static struct i2c_driver imx471_i2c_driver = {
> +       .driver = {
> +               .name = "imx471",
> +               .acpi_match_table = ACPI_PTR(imx471_acpi_ids),
> +               .pm = pm_sleep_ptr(&imx471_pm_ops),
> +       },
> +       .probe = imx471_probe,
> +       .remove = imx471_remove,
> +};
> +module_i2c_driver(imx471_i2c_driver);
> +
> +MODULE_AUTHOR("Jimmy Su <jimmy.su@intel.com>");
> +MODULE_AUTHOR("Serin Yeh <serin.yeh@intel.com>");
> +MODULE_AUTHOR("Kate Hsuan <hpa@redhat.com>");
> +MODULE_DESCRIPTION("Sony imx471 sensor driver");
> +MODULE_LICENSE("GPL");
> --
> 2.54.0

Best Regards,
Tarang

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

[Sakari Ailus]

Hi Tarang, Kate,

On Fri, May 22, 2026 at 11:12:53AM +0000, Tarang Raval wrote:
> > +/* Exposure control */
> > +#define IMX471_REG_EXPOSURE                    CCI_REG16(0x0202)
> > +#define IMX471_EXPOSURE_MIN                    1
> > +#define IMX471_EXPOSURE_STEP                   1
> > +#define IMX471_EXPOSURE_DEFAULT                        0x04f6
> 
> Better to use a decimal value here.

I'd rather just initialise this to the maximum instead of a fixed value.


...

> > +static int imx471_set_ctrl(struct v4l2_ctrl *ctrl)
> > +{
> > +       struct imx471 *sensor = container_of(ctrl->handler,
> > +                                                 struct imx471,
> > +                                                 ctrl_handler);
> 
> Use container_of_const.
> 
> > +       struct v4l2_subdev_state *state =
> > +                       v4l2_subdev_get_locked_active_state(&sensor->sd);
> > +       const struct v4l2_mbus_framefmt *format =
> > +                       v4l2_subdev_state_get_format(state, 0);
> > +       s64 exposure_max;
> > +       int ret;
> 
> ret = 0;

Or assign the return value to ret below. Either works.

> 
> > +
> > +       /* Propagate change of current control to all related controls */
> > +       if (ctrl->id == V4L2_CID_VBLANK) {
> > +               /* Update max exposure while meeting expected vblanking */
> > +               exposure_max =
> > +                       format->height + ctrl->val - IMX471_EXPOSURE_MARGIN;
> > +               __v4l2_ctrl_modify_range(sensor->exposure,
> > +                                        sensor->exposure->minimum,
> > +                                        exposure_max,
> > +                                        sensor->exposure->step,
> > +                                        exposure_max);
> 
> This control operation can fail. Please check the return value.
> 
> > +       }
> > +
> > +       /* V4L2 controls values will be applied only when power is already up */
> > +       if (!pm_runtime_get_if_in_use(sensor->dev))
> > +               return 0;
> > +
> > +       switch (ctrl->id) {
> > +       case V4L2_CID_ANALOGUE_GAIN:
> > +               cci_write(sensor->regmap, IMX471_REG_ANALOG_GAIN,
> > +                         ctrl->val, &ret);
> 
> You are using ret for the first time here, Please initialize ret with 0 when
> declaring it.
> 
> cci_write() uses the value pointed by &ret to determine whether a previous
> error has already occurred, and an uninitialized ret may contain a garbage
> value, causing the write operation to fail unexpectedly.
> 
> > +               break;
> > +       case V4L2_CID_DIGITAL_GAIN:
> > +               cci_write(sensor->regmap, IMX471_REG_DIG_GAIN_GLOBAL,
> > +                         ctrl->val, &ret);
> > +               break;
> > +       case V4L2_CID_EXPOSURE:
> > +               cci_write(sensor->regmap, IMX471_REG_EXPOSURE,
> > +                         ctrl->val, &ret);
> > +               break;
> > +       case V4L2_CID_VBLANK:
> > +               /* Update FLL that meets expected vertical blanking */
> > +               cci_write(sensor->regmap, IMX471_REG_FLL,
> > +                         format->height + ctrl->val, &ret);
> > +               break;
> > +       case V4L2_CID_TEST_PATTERN:
> > +               cci_write(sensor->regmap, IMX471_REG_TEST_PATTERN,
> > +                         ctrl->val, &ret);
> > +               break;
> > +       case V4L2_CID_HFLIP:
> > +       case V4L2_CID_VFLIP:
> > +               cci_write(sensor->regmap, IMX471_REG_ORIENTATION,
> > +                         sensor->hflip->val | sensor->vflip->val << 1, &ret);
> > +               break;
> > +       default:
> > +               ret = -EINVAL;
> > +               dev_info(sensor->dev, "ctrl(id:0x%x,val:0x%x) is not handled",
> > +                        ctrl->id, ctrl->val);
> > +               break;
> > +       }
> > +
> > +       pm_runtime_put(sensor->dev);
> > +
> > +       return ret;
> > +}

...

> > +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;
> > +       u64 pixel_rate;
> > +
> > +       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;
> > +
> > +       pixel_rate = IMX471_LINK_FREQ_DEFAULT * 2 * 4;
> > +       div_u64(pixel_rate, 10);
> 
> You need to store the return value, as the above operation does not update
> pixel_rate.
> 
> Please use:
> pixel_rate = div_u64(IMX471_LINK_FREQ_DEFAULT * 2 * 4, 10);
> 
> > +       __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);
> 
> All the above control operations can fail. Please add proper error checks for them.
> 
> > +
> > +       return 0;
> > +}
> > +
> > +static int imx471_get_selection(struct v4l2_subdev *sd,
> > +                               struct v4l2_subdev_state *sd_state,
> > +                               struct v4l2_subdev_selection *sel)
> > +{
> > +       switch (sel->target) {
> > +       case V4L2_SEL_TGT_CROP:
> > +               sel->r = *v4l2_subdev_state_get_crop(sd_state, sel->pad);
> > +               break;
> > +
> > +       case V4L2_SEL_TGT_NATIVE_SIZE:
> > +               sel->r.top = 0;
> > +               sel->r.left = 0;
> > +               sel->r.width = IMX471_NATIVE_WIDTH;
> > +               sel->r.height = IMX471_NATIVE_HEIGHT;
> > +               return 0;
> > +
> > +       case V4L2_SEL_TGT_CROP_DEFAULT:
> > +       case V4L2_SEL_TGT_CROP_BOUNDS:
> > +               sel->r.top = IMX471_PIXEL_ARRAY_TOP;
> > +               sel->r.left = IMX471_PIXEL_ARRAY_LEFT;
> > +               sel->r.width = IMX471_PIXEL_ARRAY_WIDTH;
> > +               sel->r.height = IMX471_PIXEL_ARRAY_HEIGHT;
> > +               return 0;
> > +       }
> > +
> > +       return -EINVAL;
> > +}
> > +
> > +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,

You shouldn't be setting the active format here.

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

...

> > +static int imx471_check_hwcfg(struct imx471 *sensor)
> > +{
> > +       struct v4l2_fwnode_endpoint bus_cfg = {
> > +               .bus_type = V4L2_MBUS_CSI2_DPHY,
> > +       };
> > +       struct fwnode_handle *ep, *fwnode = dev_fwnode(sensor->dev);
> > +       struct clk *clk;
> > +       unsigned long link_freq_bitmap;
> > +       int ret;
> 
> If you want, you can sort the variable declarations throughout the code, where
> appropriate, by length to make them more readable.
> 
> > +
> > +       clk = devm_v4l2_sensor_clk_get(sensor->dev, NULL);
> > +       if (IS_ERR(clk))
> > +               return dev_err_probe(sensor->dev, PTR_ERR(clk),
> > +                                    "can't get clock frequency\n");
> > +
> > +       if (clk_get_rate(clk) != IMX471_EXT_CLK)
> > +               return dev_err_probe(sensor->dev, -EINVAL,
> > +                                    "external clock %lu is not supported\n",
> > +                                    clk_get_rate(clk));
> > +
> > +       ep = fwnode_graph_get_endpoint_by_id(fwnode, 0, 0, 0);
> > +       ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg);
> > +       fwnode_handle_put(ep);
> > +       if (ret)
> > +               return dev_err_probe(sensor->dev, ret,
> > +                                    "parsing endpoint failed");
> > +
> > +       ret = v4l2_link_freq_to_bitmap(sensor->dev, bus_cfg.link_frequencies,
> > +                                      bus_cfg.nr_of_link_frequencies,
> > +                                      link_freq_menu_items,
> > +                                      ARRAY_SIZE(link_freq_menu_items),
> > +                                      &link_freq_bitmap);
> 
> This can fail silently. Please add an error message before returning the failure.

v4l2_link_freq_to_bitmap() does print errors already, don't do it here.

> 
> > +
> > +       v4l2_fwnode_endpoint_free(&bus_cfg);
> > +
> > +       return ret;
> > +}

-- 
Regards,

Sakari Ailus

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

[Kate Hsuan]

On Sat, May 23, 2026 at 6:55 AM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Tarang, Kate,
>
> On Fri, May 22, 2026 at 11:12:53AM +0000, Tarang Raval wrote:
> > > +/* Exposure control */
> > > +#define IMX471_REG_EXPOSURE                    CCI_REG16(0x0202)
> > > +#define IMX471_EXPOSURE_MIN                    1
> > > +#define IMX471_EXPOSURE_STEP                   1
> > > +#define IMX471_EXPOSURE_DEFAULT                        0x04f6
> >
> > Better to use a decimal value here.
>
> I'd rather just initialise this to the maximum instead of a fixed value.
I can set the max value when creating the exposure control and drop
the default value.

>
>
> ...
>
> > > +static int imx471_set_ctrl(struct v4l2_ctrl *ctrl)
> > > +{
> > > +       struct imx471 *sensor = container_of(ctrl->handler,
> > > +                                                 struct imx471,
> > > +                                                 ctrl_handler);
> >
> > Use container_of_const.
> >
> > > +       struct v4l2_subdev_state *state =
> > > +                       v4l2_subdev_get_locked_active_state(&sensor->sd);
> > > +       const struct v4l2_mbus_framefmt *format =
> > > +                       v4l2_subdev_state_get_format(state, 0);
> > > +       s64 exposure_max;
> > > +       int ret;
> >
> > ret = 0;
>
> Or assign the return value to ret below. Either works.
will fix it through ret = cci_write(...);
>
> >
> > > +
> > > +       /* Propagate change of current control to all related controls */
> > > +       if (ctrl->id == V4L2_CID_VBLANK) {
> > > +               /* Update max exposure while meeting expected vblanking */
> > > +               exposure_max =
> > > +                       format->height + ctrl->val - IMX471_EXPOSURE_MARGIN;
> > > +               __v4l2_ctrl_modify_range(sensor->exposure,
> > > +                                        sensor->exposure->minimum,
> > > +                                        exposure_max,
> > > +                                        sensor->exposure->step,
> > > +                                        exposure_max);
> >
> > This control operation can fail. Please check the return value.
> >
> > > +       }
> > > +
> > > +       /* V4L2 controls values will be applied only when power is already up */
> > > +       if (!pm_runtime_get_if_in_use(sensor->dev))
> > > +               return 0;
> > > +
> > > +       switch (ctrl->id) {
> > > +       case V4L2_CID_ANALOGUE_GAIN:
> > > +               cci_write(sensor->regmap, IMX471_REG_ANALOG_GAIN,
> > > +                         ctrl->val, &ret);
> >
> > You are using ret for the first time here, Please initialize ret with 0 when
> > declaring it.
> >
> > cci_write() uses the value pointed by &ret to determine whether a previous
> > error has already occurred, and an uninitialized ret may contain a garbage
> > value, causing the write operation to fail unexpectedly.
> >
> > > +               break;
> > > +       case V4L2_CID_DIGITAL_GAIN:
> > > +               cci_write(sensor->regmap, IMX471_REG_DIG_GAIN_GLOBAL,
> > > +                         ctrl->val, &ret);
> > > +               break;
> > > +       case V4L2_CID_EXPOSURE:
> > > +               cci_write(sensor->regmap, IMX471_REG_EXPOSURE,
> > > +                         ctrl->val, &ret);
> > > +               break;
> > > +       case V4L2_CID_VBLANK:
> > > +               /* Update FLL that meets expected vertical blanking */
> > > +               cci_write(sensor->regmap, IMX471_REG_FLL,
> > > +                         format->height + ctrl->val, &ret);
> > > +               break;
> > > +       case V4L2_CID_TEST_PATTERN:
> > > +               cci_write(sensor->regmap, IMX471_REG_TEST_PATTERN,
> > > +                         ctrl->val, &ret);
> > > +               break;
> > > +       case V4L2_CID_HFLIP:
> > > +       case V4L2_CID_VFLIP:
> > > +               cci_write(sensor->regmap, IMX471_REG_ORIENTATION,
> > > +                         sensor->hflip->val | sensor->vflip->val << 1, &ret);
> > > +               break;
> > > +       default:
> > > +               ret = -EINVAL;
> > > +               dev_info(sensor->dev, "ctrl(id:0x%x,val:0x%x) is not handled",
> > > +                        ctrl->id, ctrl->val);
> > > +               break;
> > > +       }
> > > +
> > > +       pm_runtime_put(sensor->dev);
> > > +
> > > +       return ret;
> > > +}
>
> ...
>
> > > +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;
> > > +       u64 pixel_rate;
> > > +
> > > +       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;
> > > +
> > > +       pixel_rate = IMX471_LINK_FREQ_DEFAULT * 2 * 4;
> > > +       div_u64(pixel_rate, 10);
> >
> > You need to store the return value, as the above operation does not update
> > pixel_rate.
> >
> > Please use:
> > pixel_rate = div_u64(IMX471_LINK_FREQ_DEFAULT * 2 * 4, 10);
> >
> > > +       __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);
> >
> > All the above control operations can fail. Please add proper error checks for them.
> >
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +static int imx471_get_selection(struct v4l2_subdev *sd,
> > > +                               struct v4l2_subdev_state *sd_state,
> > > +                               struct v4l2_subdev_selection *sel)
> > > +{
> > > +       switch (sel->target) {
> > > +       case V4L2_SEL_TGT_CROP:
> > > +               sel->r = *v4l2_subdev_state_get_crop(sd_state, sel->pad);
> > > +               break;
> > > +
> > > +       case V4L2_SEL_TGT_NATIVE_SIZE:
> > > +               sel->r.top = 0;
> > > +               sel->r.left = 0;
> > > +               sel->r.width = IMX471_NATIVE_WIDTH;
> > > +               sel->r.height = IMX471_NATIVE_HEIGHT;
> > > +               return 0;
> > > +
> > > +       case V4L2_SEL_TGT_CROP_DEFAULT:
> > > +       case V4L2_SEL_TGT_CROP_BOUNDS:
> > > +               sel->r.top = IMX471_PIXEL_ARRAY_TOP;
> > > +               sel->r.left = IMX471_PIXEL_ARRAY_LEFT;
> > > +               sel->r.width = IMX471_PIXEL_ARRAY_WIDTH;
> > > +               sel->r.height = IMX471_PIXEL_ARRAY_HEIGHT;
> > > +               return 0;
> > > +       }
> > > +
> > > +       return -EINVAL;
> > > +}
> > > +
> > > +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,
>
> You shouldn't be setting the active format here.
I'll drop it.

>
> > > +               .format = {
> > > +                       .code = MEDIA_BUS_FMT_SRGGB10_1X10,
> > > +                       .width = imx471_modes[0].width,
> > > +                       .height = imx471_modes[0].height,
> > > +               },
> > > +       };
> > > +
> > > +       imx471_set_pad_format(sd, sd_state, &fmt);
> > > +
> > > +       return 0;
>
> ...
>
> > > +static int imx471_check_hwcfg(struct imx471 *sensor)
> > > +{
> > > +       struct v4l2_fwnode_endpoint bus_cfg = {
> > > +               .bus_type = V4L2_MBUS_CSI2_DPHY,
> > > +       };
> > > +       struct fwnode_handle *ep, *fwnode = dev_fwnode(sensor->dev);
> > > +       struct clk *clk;
> > > +       unsigned long link_freq_bitmap;
> > > +       int ret;
> >
> > If you want, you can sort the variable declarations throughout the code, where
> > appropriate, by length to make them more readable.
> >
> > > +
> > > +       clk = devm_v4l2_sensor_clk_get(sensor->dev, NULL);
> > > +       if (IS_ERR(clk))
> > > +               return dev_err_probe(sensor->dev, PTR_ERR(clk),
> > > +                                    "can't get clock frequency\n");
> > > +
> > > +       if (clk_get_rate(clk) != IMX471_EXT_CLK)
> > > +               return dev_err_probe(sensor->dev, -EINVAL,
> > > +                                    "external clock %lu is not supported\n",
> > > +                                    clk_get_rate(clk));
> > > +
> > > +       ep = fwnode_graph_get_endpoint_by_id(fwnode, 0, 0, 0);
> > > +       ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg);
> > > +       fwnode_handle_put(ep);
> > > +       if (ret)
> > > +               return dev_err_probe(sensor->dev, ret,
> > > +                                    "parsing endpoint failed");
> > > +
> > > +       ret = v4l2_link_freq_to_bitmap(sensor->dev, bus_cfg.link_frequencies,
> > > +                                      bus_cfg.nr_of_link_frequencies,
> > > +                                      link_freq_menu_items,
> > > +                                      ARRAY_SIZE(link_freq_menu_items),
> > > +                                      &link_freq_bitmap);
> >
> > This can fail silently. Please add an error message before returning the failure.
>
> v4l2_link_freq_to_bitmap() does print errors already, don't do it here.
>
> >
> > > +
> > > +       v4l2_fwnode_endpoint_free(&bus_cfg);
> > > +
> > > +       return ret;
> > > +}
>
> --
> Regards,
>
> Sakari Ailus
>


-- 
BR,
Kate

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

[Kate Hsuan]

Hi Tarang,

Thank you for reviewing.

On Fri, May 22, 2026 at 7:13 PM Tarang Raval
<tarang.raval@siliconsignals.io> wrote:
>
> Hi Kate,
>
> I noticed a few issues. Please check the comments below.
>
> > 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 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.
> >
> > Signed-off-by: Kate Hsuan <hpa@redhat.com>
> > ---
> >  MAINTAINERS                |    6 +
> >  drivers/media/i2c/Kconfig  |   10 +
> >  drivers/media/i2c/Makefile |    1 +
> >  drivers/media/i2c/imx471.c | 1006 ++++++++++++++++++++++++++++++++++++
> >  4 files changed, 1023 insertions(+)
> >  create mode 100644 drivers/media/i2c/imx471.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 1126fdd639ad..d597337e7c24 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -24735,6 +24735,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 5eb1e0e0a87a..1c28c498a9f1 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.
> > +
>
> The indentation is wrong here. Please fix it.
Okay.



>
> >  config VIDEO_MAX9271_LIB
> >       tristate
> >
> > diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> > index a3a6396df3c4..0539e9171030 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..f3c7fdce2d50
> > --- /dev/null
> > +++ b/drivers/media/i2c/imx471.c
> > @@ -0,0 +1,1006 @@
> > +// 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>
> > +#include <media/v4l2-cci.h>
> > +#include <media/v4l2-ctrls.h>
> > +#include <media/v4l2-device.h>
> > +#include <media/v4l2-event.h>
> > +#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                        0x04f6
>
> Better to use a decimal value here.
As Sakari mentioned, I'll try to find the max exposure in decimal.
It shoule be 1290.

>
> > +
> > +/*
> > + *  the digital control register for all color control looks like:
> > + *  +-----------------+------------------+
> > + *  |      [7:0]      |       [15:8]     |
> > + *  +-----------------+------------------+
> > + *  |    0x020f      |       0x020e     |
> > + *  --------------------------------------
> > + *  it is used to calculate the digital gain times value(integral + fractional)
> > + *  the [15:8] bits is the fractional part and [7:0] bits is the integral
> > + *  calculation equation is:
> > + *      gain value (unit: times) = REG[15:8] + REG[7:0]/0x100
> > + *  Only value in 0x0100 ~ 0x0FFF range is allowed.
> > + *  Analog gain use 10 bits in the registers and allowed range is 0 ~ 960
> > + */
> > +/* Analog gain control */
> > +#define IMX471_REG_ANALOG_GAIN                 CCI_REG16(0x0204)
> > +#define IMX471_ANA_GAIN_MIN                    0
> > +#define IMX471_ANA_GAIN_MAX                    960
> > +#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
> > +
> > +#define IMX471_VALUE_08BIT                     1
>
> Unused macro, please remove it.
ok

>
> > +
> > +/* HFLIP and VFLIP control */
> > +#define IMX471_REG_ORIENTATION                 CCI_REG8(0x0101)
> > +#define IMX471_HFLIP_BIT                       BIT(0)
> > +#define IMX471_VFLIP_BIT                       BIT(1)
> > +
> > +/* Default exposure margin */
> > +#define IMX471_EXPOSURE_MARGIN                 18
>
> Please move this macro to the exposure block above.
ok

>
> > +
> > +/* Horizontal crop window offset */
> > +#define IMX471_REG_H_WIN_OFFSET                        CCI_REG8(0x0409)
> > +
> > +/* Vertical crop window offset */
> > +#define IMX471_REG_V_WIN_OFFSET                        CCI_REG8(0x034b)
> > +
> > +/* 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
> > +
> > +/* default link frequency and external clock */
> > +#define IMX471_LINK_FREQ_DEFAULT               200000000LL
> > +#define IMX471_EXT_CLK                         19200000
> > +#define IMX471_LINK_FREQ_INDEX                 0
> > +
> > +/* IMX471 native and active pixel array size */
> > +#define IMX471_NATIVE_WIDTH                    4672
> > +#define IMX471_NATIVE_HEIGHT                   3512
> > +#define IMX471_PIXEL_ARRAY_LEFT                        8
> > +#define IMX471_PIXEL_ARRAY_TOP                 8
> > +#define IMX471_PIXEL_ARRAY_WIDTH               4656
> > +#define IMX471_PIXEL_ARRAY_HEIGHT              3496
> > +
> > +#define to_imx471(_sd) container_of_const(_sd, struct imx471, sd)
> > +
> > +static const char * const imx471_supply_name[] = {
> > +       "avdd",
> > +};
> > +
> > +#define IMX471_NUM_SUPPLIES                    ARRAY_SIZE(imx471_supply_name)
> > +
> > +/* Mode : resolution and related config&values */
> > +struct imx471_mode {
> > +       /* Frame width */
> > +       u32 width;
> > +       /* Frame height */
> > +       u32 height;
> > +
> > +       /* V-timing */
> > +       u32 fll_def;
> > +       u32 fll_min;
> > +
> > +       /* H-timing */
> > +       u32 llp;
> > +
> > +       /* index of link frequency */
> > +       u32 link_freq_index;
> > +
> > +       /* Default register values */
> > +       const struct cci_reg_sequence *default_mode_regs;
> > +       const int default_mode_regs_length;
> > +};
> > +
> > +struct imx471 {
> > +       struct v4l2_subdev sd;
> > +       struct media_pad pad;
> > +
> > +       struct v4l2_ctrl_handler ctrl_handler;
> > +       /* V4L2 Controls */
> > +       struct v4l2_ctrl *link_freq;
> > +       struct v4l2_ctrl *pixel_rate;
> > +       struct v4l2_ctrl *vblank;
> > +       struct v4l2_ctrl *hblank;
> > +       struct v4l2_ctrl *vflip;
> > +       struct v4l2_ctrl *hflip;
> > +       struct v4l2_ctrl *exposure;
> > +
> > +       struct gpio_desc *reset_gpio;
> > +       struct regulator_bulk_data supplies[IMX471_NUM_SUPPLIES];
> > +       struct clk *img_clk;
> > +
> > +       struct device *dev;
> > +       struct regmap *regmap;
> > +};
> > +
> > +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 },
> > +       { CCI_REG8(0x4431), 0x04 },
> > +       { CCI_REG8(0x531c), 0x01 },
> > +       { CCI_REG8(0x531d), 0x02 },
> > +       { CCI_REG8(0x531e), 0x04 },
> > +       { CCI_REG8(0x5928), 0x00 },
> > +       { CCI_REG8(0x5929), 0x2f },
> > +       { CCI_REG8(0x592a), 0x00 },
> > +       { CCI_REG8(0x592b), 0x85 },
> > +       { CCI_REG8(0x592c), 0x00 },
> > +       { CCI_REG8(0x592d), 0x32 },
> > +       { CCI_REG8(0x592e), 0x00 },
> > +       { CCI_REG8(0x592f), 0x88 },
> > +       { CCI_REG8(0x5930), 0x00 },
> > +       { CCI_REG8(0x5931), 0x3d },
> > +       { CCI_REG8(0x5932), 0x00 },
> > +       { CCI_REG8(0x5933), 0x93 },
> > +       { CCI_REG8(0x5938), 0x00 },
> > +       { CCI_REG8(0x5939), 0x24 },
> > +       { CCI_REG8(0x593a), 0x00 },
> > +       { CCI_REG8(0x593b), 0x7a },
> > +       { CCI_REG8(0x593c), 0x00 },
> > +       { CCI_REG8(0x593d), 0x24 },
> > +       { CCI_REG8(0x593e), 0x00 },
> > +       { CCI_REG8(0x593f), 0x7a },
> > +       { CCI_REG8(0x5940), 0x00 },
> > +       { CCI_REG8(0x5941), 0x2f },
> > +       { CCI_REG8(0x5942), 0x00 },
> > +       { CCI_REG8(0x5943), 0x85 },
> > +       { CCI_REG8(0x5f0e), 0x6e },
> > +       { CCI_REG8(0x5f11), 0xc6 },
> > +       { CCI_REG8(0x5f17), 0x5e },
> > +       { CCI_REG8(0x7990), 0x01 },
> > +       { CCI_REG8(0x7993), 0x5d },
> > +       { CCI_REG8(0x7994), 0x5d },
> > +       { CCI_REG8(0x7995), 0xa1 },
> > +       { CCI_REG8(0x799a), 0x01 },
> > +       { CCI_REG8(0x799d), 0x00 },
> > +       { CCI_REG8(0x8169), 0x01 },
> > +       { CCI_REG8(0x8359), 0x01 },
> > +       { CCI_REG8(0x9302), 0x1e },
> > +       { CCI_REG8(0x9306), 0x1f },
> > +       { CCI_REG8(0x930a), 0x26 },
> > +       { CCI_REG8(0x930e), 0x23 },
> > +       { CCI_REG8(0x9312), 0x23 },
> > +       { CCI_REG8(0x9316), 0x2c },
> > +       { CCI_REG8(0x9317), 0x19 },
> > +       { CCI_REG8(0xb046), 0x01 },
> > +       { CCI_REG8(0xb048), 0x01 },
> > +};
> > +
> > +static const struct cci_reg_sequence mode_1928x1088_regs[] = {
> > +       { CCI_REG8(0x0101), 0x00 },
> > +       { CCI_REG8(0x0112), 0x0a },
> > +       { CCI_REG8(0x0113), 0x0a },
> > +       { CCI_REG8(0x0114), 0x03 },
> > +       { CCI_REG8(0x0342), 0x0a },
> > +       { CCI_REG8(0x0343), 0x00 },
> > +       { CCI_REG8(0x0340), 0x13 },
> > +       { CCI_REG8(0x0341), 0xb0 },
>
> These are V-timing settings. You can drop them since they are already
> being set from the control handler.
>
> > +       { CCI_REG8(0x0344), 0x00 },
> > +       { CCI_REG8(0x0345), 0x00 },
> > +       { CCI_REG8(0x0346), 0x01 },
> > +       { CCI_REG8(0x0347), 0xbc },
> > +       { CCI_REG8(0x0348), 0x12 },
> > +       { CCI_REG8(0x0349), 0x2f },
> > +       { CCI_REG8(0x034a), 0x0b },
> > +       { CCI_REG8(0x034b), 0xeb },
> > +       { CCI_REG8(0x0381), 0x01 },
> > +       { CCI_REG8(0x0383), 0x01 },
> > +       { CCI_REG8(0x0385), 0x01 },
> > +       { CCI_REG8(0x0387), 0x01 },
> > +       { CCI_REG8(0x0900), 0x01 },
> > +       { CCI_REG8(0x0901), 0x22 },
> > +       { CCI_REG8(0x0902), 0x08 },
> > +       { CCI_REG8(0x3f4c), 0x81 },
> > +       { CCI_REG8(0x3f4d), 0x81 },
> > +       { CCI_REG8(0x0408), 0x00 },
> > +       { CCI_REG8(0x0409), 0xc8 },
> > +       { CCI_REG8(0x040a), 0x00 },
> > +       { CCI_REG8(0x040b), 0x6c },
> > +       { CCI_REG8(0x040c), 0x07 },
> > +       { CCI_REG8(0x040d), 0x88 },
> > +       { CCI_REG8(0x040e), 0x04 },
> > +       { CCI_REG8(0x040f), 0x40 },
> > +       { CCI_REG8(0x034c), 0x07 },
> > +       { CCI_REG8(0x034d), 0x88 },
> > +       { CCI_REG8(0x034e), 0x04 },
> > +       { CCI_REG8(0x034f), 0x40 },
> > +       { CCI_REG8(0x0301), 0x06 },
> > +       { CCI_REG8(0x0303), 0x02 },
> > +       { CCI_REG8(0x0305), 0x02 },
> > +       { CCI_REG8(0x0306), 0x00 },
> > +       { CCI_REG8(0x0307), 0x79 },
> > +       { CCI_REG8(0x030b), 0x01 },
> > +       { CCI_REG8(0x030d), 0x02 },
> > +       { CCI_REG8(0x030e), 0x00 },
> > +       { CCI_REG8(0x030f), 0x53 },
> > +       { CCI_REG8(0x0310), 0x01 },
> > +       { CCI_REG8(0x0202), 0x13 },
> > +       { CCI_REG8(0x0203), 0x9e },
>
> Similarly, these are exposure settings. You can drop.
>
> > +       { CCI_REG8(0x0204), 0x00 },
> > +       { CCI_REG8(0x0205), 0x00 },
>
> These are digital gain settings. You can drop.
>
> > +       { CCI_REG8(0x020e), 0x01 },
> > +       { CCI_REG8(0x020f), 0x00 },
>
> These are analog gain settings. You can drop.
Okay. I'll drop them.

>
> > +       { CCI_REG8(0x3f78), 0x01 },
> > +       { CCI_REG8(0x3f79), 0x31 },
> > +       { CCI_REG8(0x3ffe), 0x00 },
> > +       { CCI_REG8(0x3fff), 0x8a },
> > +       { CCI_REG8(0x5f0a), 0xb6 },
> > +};
> > +
> > +static const char * const imx471_test_pattern_menu[] = {
> > +       "Disabled",
> > +       "Solid Colour",
> > +       "Eight Vertical Colour Bars",
> > +       "Colour Bars With Fade to Grey",
> > +       "Pseudorandom Sequence (PN9)",
> > +};
> > +
> > +/*
> > + * When adding more than the one below, make sure the disallowed ones will
> > + * actually be disabled in the LINK_FREQ control.
> > + */
> > +static const s64 link_freq_menu_items[] = {
> > +       IMX471_LINK_FREQ_DEFAULT,
> > +};
> > +
> > +/*
> > + * The Bayer formats for the flipping.
> > + * - no flip
> > + * - h flip
> > + * - v flip
> > + * - h and v flips
> > + */
> > +static const u32 imx471_hv_flips_bayer_order[] = {
> > +       MEDIA_BUS_FMT_SRGGB10_1X10,
> > +       MEDIA_BUS_FMT_SGRBG10_1X10,
> > +       MEDIA_BUS_FMT_SGBRG10_1X10,
> > +       MEDIA_BUS_FMT_SBGGR10_1X10,
> > +};
> > +
> > +/* Mode configs */
> > +static const struct imx471_mode imx471_modes[] = {
> > +       {
> > +               .width = 1928,
> > +               .height = 1088,
> > +               .fll_def = 1308,
> > +               .fll_min = 1308,
> > +               .llp = 2328,
> > +               .link_freq_index = IMX471_LINK_FREQ_INDEX,
> > +               .default_mode_regs = mode_1928x1088_regs,
> > +               .default_mode_regs_length = ARRAY_SIZE(mode_1928x1088_regs),
> > +       },
> > +};
> > +
> > +static int imx471_get_regulators(struct device *dev, struct imx471 *sensor)
> > +{
> > +       for (unsigned int  i = 0; i < IMX471_NUM_SUPPLIES; i++)
> > +               sensor->supplies[i].supply = imx471_supply_name[i];
> > +
> > +       return devm_regulator_bulk_get(dev, IMX471_NUM_SUPPLIES,
> > +                                      sensor->supplies);
> > +}
> > +
> > +static int imx471_set_ctrl(struct v4l2_ctrl *ctrl)
> > +{
> > +       struct imx471 *sensor = container_of(ctrl->handler,
> > +                                                 struct imx471,
> > +                                                 ctrl_handler);
>
> Use container_of_const.
OK
>
> > +       struct v4l2_subdev_state *state =
> > +                       v4l2_subdev_get_locked_active_state(&sensor->sd);
> > +       const struct v4l2_mbus_framefmt *format =
> > +                       v4l2_subdev_state_get_format(state, 0);
> > +       s64 exposure_max;
> > +       int ret;
>
> ret = 0;
See below.
>
> > +
> > +       /* Propagate change of current control to all related controls */
> > +       if (ctrl->id == V4L2_CID_VBLANK) {
> > +               /* Update max exposure while meeting expected vblanking */
> > +               exposure_max =
> > +                       format->height + ctrl->val - IMX471_EXPOSURE_MARGIN;
> > +               __v4l2_ctrl_modify_range(sensor->exposure,
> > +                                        sensor->exposure->minimum,
> > +                                        exposure_max,
> > +                                        sensor->exposure->step,
> > +                                        exposure_max);
>
> This control operation can fail. Please check the return value.
>
> > +       }
> > +
> > +       /* V4L2 controls values will be applied only when power is already up */
> > +       if (!pm_runtime_get_if_in_use(sensor->dev))
> > +               return 0;
> > +
> > +       switch (ctrl->id) {
> > +       case V4L2_CID_ANALOGUE_GAIN:
> > +               cci_write(sensor->regmap, IMX471_REG_ANALOG_GAIN,
> > +                         ctrl->val, &ret);
This will be
ret = cci_write(sensor->regmap, IMX471_REG_ANALOG_GAIN, ctrl->val, NULL);
>
> You are using ret for the first time here, Please initialize ret with 0 when
> declaring it.
>
> cci_write() uses the value pointed by &ret to determine whether a previous
> error has already occurred, and an uninitialized ret may contain a garbage
> value, causing the write operation to fail unexpectedly.
I noticed this thank you :)
>
> > +               break;
> > +       case V4L2_CID_DIGITAL_GAIN:
> > +               cci_write(sensor->regmap, IMX471_REG_DIG_GAIN_GLOBAL,
> > +                         ctrl->val, &ret);
> > +               break;
> > +       case V4L2_CID_EXPOSURE:
> > +               cci_write(sensor->regmap, IMX471_REG_EXPOSURE,
> > +                         ctrl->val, &ret);
> > +               break;
> > +       case V4L2_CID_VBLANK:
> > +               /* Update FLL that meets expected vertical blanking */
> > +               cci_write(sensor->regmap, IMX471_REG_FLL,
> > +                         format->height + ctrl->val, &ret);
> > +               break;
> > +       case V4L2_CID_TEST_PATTERN:
> > +               cci_write(sensor->regmap, IMX471_REG_TEST_PATTERN,
> > +                         ctrl->val, &ret);
> > +               break;
> > +       case V4L2_CID_HFLIP:
> > +       case V4L2_CID_VFLIP:
> > +               cci_write(sensor->regmap, IMX471_REG_ORIENTATION,
> > +                         sensor->hflip->val | sensor->vflip->val << 1, &ret);
> > +               break;
> > +       default:
> > +               ret = -EINVAL;
> > +               dev_info(sensor->dev, "ctrl(id:0x%x,val:0x%x) is not handled",
> > +                        ctrl->id, ctrl->val);
> > +               break;
> > +       }
> > +
> > +       pm_runtime_put(sensor->dev);
> > +
> > +       return ret;
> > +}
> > +
> > +static const struct v4l2_ctrl_ops imx471_ctrl_ops = {
> > +       .s_ctrl = imx471_set_ctrl,
> > +};
> > +
> > +static u32 imx471_get_format_code(struct imx471 *sensor)
> > +{
> > +       unsigned int i;
> > +
> > +       i = (sensor->vflip->val ? 2 : 0) | (sensor->hflip->val ? 1 : 0);
> > +
> > +       return imx471_hv_flips_bayer_order[i];
> > +}
> > +
> > +static int imx471_enum_mbus_code(struct v4l2_subdev *sd,
> > +                                struct v4l2_subdev_state *sd_state,
> > +                                struct v4l2_subdev_mbus_code_enum *code)
> > +{
> > +       struct imx471 *sensor = to_imx471(sd);
> > +
> > +       if (code->index >= (ARRAY_SIZE(imx471_hv_flips_bayer_order) / 4))
> > +               return -EINVAL;
> > +
> > +       code->code = imx471_get_format_code(sensor);
> > +
> > +       return 0;
> > +}
>
> Please add one extra blank line after this line.
OK

>
> > +static int imx471_enum_frame_size(struct v4l2_subdev *sd,
> > +                                 struct v4l2_subdev_state *sd_state,
> > +                                 struct v4l2_subdev_frame_size_enum *fse)
> > +{
> > +       if (fse->index >= ARRAY_SIZE(imx471_modes))
> > +               return -EINVAL;
> > +
> > +       fse->min_width = imx471_modes[fse->index].width;
> > +       fse->max_width = fse->min_width;
> > +       fse->min_height = imx471_modes[fse->index].height;
> > +       fse->max_height = fse->min_height;
> > +
> > +       return 0;
> > +}
> > +
> > +static void imx471_update_pad_format(struct imx471 *sensor,
> > +                                    const struct imx471_mode *mode,
> > +                                    struct v4l2_subdev_format *fmt)
> > +{
> > +       fmt->format.code = imx471_get_format_code(sensor);
> > +       fmt->format.width = mode->width;
> > +       fmt->format.height = mode->height;
> > +       fmt->format.field = V4L2_FIELD_NONE;
> > +}
> > +
> > +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;
> > +       u64 pixel_rate;
> > +
> > +       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;
> > +
> > +       pixel_rate = IMX471_LINK_FREQ_DEFAULT * 2 * 4;
> > +       div_u64(pixel_rate, 10);
>
> You need to store the return value, as the above operation does not update
> pixel_rate.
>
> Please use:
> pixel_rate = div_u64(IMX471_LINK_FREQ_DEFAULT * 2 * 4, 10);
Got it.

>
> > +       __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);
>
> All the above control operations can fail. Please add proper error checks for them.
okay.
>
> > +
> > +       return 0;
> > +}
> > +
> > +static int imx471_get_selection(struct v4l2_subdev *sd,
> > +                               struct v4l2_subdev_state *sd_state,
> > +                               struct v4l2_subdev_selection *sel)
> > +{
> > +       switch (sel->target) {
> > +       case V4L2_SEL_TGT_CROP:
> > +               sel->r = *v4l2_subdev_state_get_crop(sd_state, sel->pad);
> > +               break;
> > +
> > +       case V4L2_SEL_TGT_NATIVE_SIZE:
> > +               sel->r.top = 0;
> > +               sel->r.left = 0;
> > +               sel->r.width = IMX471_NATIVE_WIDTH;
> > +               sel->r.height = IMX471_NATIVE_HEIGHT;
> > +               return 0;
> > +
> > +       case V4L2_SEL_TGT_CROP_DEFAULT:
> > +       case V4L2_SEL_TGT_CROP_BOUNDS:
> > +               sel->r.top = IMX471_PIXEL_ARRAY_TOP;
> > +               sel->r.left = IMX471_PIXEL_ARRAY_LEFT;
> > +               sel->r.width = IMX471_PIXEL_ARRAY_WIDTH;
> > +               sel->r.height = IMX471_PIXEL_ARRAY_HEIGHT;
> > +               return 0;
> > +       }
> > +
> > +       return -EINVAL;
> > +}
> > +
> > +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,
> > +               .format = {
> > +                       .code = MEDIA_BUS_FMT_SRGGB10_1X10,
> > +                       .width = imx471_modes[0].width,
> > +                       .height = imx471_modes[0].height,
> > +               },
> > +       };
> > +
> > +       imx471_set_pad_format(sd, sd_state, &fmt);
> > +
> > +       return 0;
>
> imx471_set_pad_format() can fail.
>
> Please use:
> return imx471_set_pad_format(sd, sd_state, &fmt);
Okay

>
> > +}
> > +
> > +static int imx471_identify_module(struct imx471 *sensor)
> > +{
> > +       int ret;
> > +       u64 val;
> > +
> > +       ret = cci_read(sensor->regmap, IMX471_REG_CHIP_ID, &val, NULL);
> > +       if (ret)
> > +               return dev_err_probe(sensor->dev, ret,
> > +                                    "failed to read chip id\n");
> > +
> > +       if (val != IMX471_CHIP_ID)
> > +               return dev_err_probe(sensor->dev, -EIO,
> > +                                    "chip id mismatch: %x!=%llx\n",
> > +                                    IMX471_CHIP_ID, val);
> > +
> > +       return 0;
> > +}
> > +
> > +static int imx471_power_off(struct device *dev)
> > +{
> > +       struct v4l2_subdev *sd = dev_get_drvdata(dev);
> > +       struct imx471 *sensor = to_imx471(sd);
> > +
> > +       clk_disable_unprepare(sensor->img_clk);
> > +       gpiod_set_value_cansleep(sensor->reset_gpio, 1);
> > +
> > +       regulator_bulk_disable(IMX471_NUM_SUPPLIES, sensor->supplies);
> > +
> > +       return 0;
> > +}
> > +
> > +static int imx471_power_on(struct device *dev)
> > +{
> > +       struct v4l2_subdev *sd = dev_get_drvdata(dev);
> > +       struct imx471 *sensor = to_imx471(sd);
> > +       int ret;
> > +
> > +       ret = regulator_bulk_enable(IMX471_NUM_SUPPLIES, sensor->supplies);
> > +       if (ret < 0) {
> > +               dev_err(dev, "failed to enable regulators: %d\n", ret);
> > +               return ret;
> > +       }
> > +
> > +       ret = clk_prepare_enable(sensor->img_clk);
> > +       if (ret < 0) {
> > +               regulator_bulk_disable(IMX471_NUM_SUPPLIES, sensor->supplies);
> > +               dev_err(dev, "failed to enable imaging clock: %d", ret);
> > +               return ret;
> > +       }
> > +
> > +       gpiod_set_value_cansleep(sensor->reset_gpio, 0);
> > +
> > +       usleep_range(10000, 15000);
> > +
> > +       return 0;
> > +}
> > +
> > +/* Start streaming */
> > +static int imx471_enable_stream(struct v4l2_subdev *sd,
> > +                               struct v4l2_subdev_state *state,
> > +                               u32 pad, u64 streams_mask)
> > +{
> > +       struct imx471 *sensor = to_imx471(sd);
> > +       const struct imx471_mode *mode;
> > +       struct v4l2_mbus_framefmt *fmt;
> > +       int ret;
> > +
> > +       ret = pm_runtime_resume_and_get(sensor->dev);
> > +       if (ret)
> > +               goto error_powerdown;
> > +
> > +       ret = imx471_identify_module(sensor);
> > +       if (ret)
> > +               return ret;
> > +
> > +       /* Global Setting */
> > +       cci_multi_reg_write(sensor->regmap, imx471_global_regs,
> > +                           ARRAY_SIZE(imx471_global_regs), &ret);
> > +       if (ret) {
> > +               dev_err(sensor->dev, "failed to set global settings");
> > +               goto error_powerdown;
> > +       }
> > +
> > +       state = v4l2_subdev_get_locked_active_state(&sensor->sd);
> > +       fmt = v4l2_subdev_state_get_format(state, 0);
> > +       mode = v4l2_find_nearest_size(imx471_modes, ARRAY_SIZE(imx471_modes),
> > +                                     width, height, fmt->width, fmt->height);
> > +
> > +       /* Apply default values of current mode */
> > +       cci_multi_reg_write(sensor->regmap, mode->default_mode_regs,
> > +                           mode->default_mode_regs_length, &ret);
> > +       if (ret) {
> > +               dev_err(sensor->dev, "failed to set mode");
> > +               goto error_powerdown;
> > +       }
> > +
> > +       /* set digital gain control to all color mode */
> > +       cci_write(sensor->regmap, IMX471_REG_DPGA_USE_GLOBAL_GAIN, 1, &ret);
> > +       if (ret)
> > +               goto error_powerdown;
> > +
> > +       /* Apply customized values from user */
> > +       ret =  __v4l2_ctrl_handler_setup(&sensor->ctrl_handler);
> > +       if (ret)
> > +               goto error_powerdown;
> > +
> > +       cci_write(sensor->regmap, IMX471_REG_MODE_SELECT,
> > +                 IMX471_MODE_STREAMING, &ret);
> > +       if (ret)
> > +               goto error_powerdown;
> > +
> > +       __v4l2_ctrl_grab(sensor->vflip, true);
> > +       __v4l2_ctrl_grab(sensor->hflip, true);
> > +
> > +       return ret;
> > +
> > +error_powerdown:
> > +       pm_runtime_put(sensor->dev);
> > +
> > +       return ret;
> > +}
> > +
> > +/* Stop streaming */
> > +static int imx471_disable_stream(struct v4l2_subdev *sd,
> > +                                struct v4l2_subdev_state *state,
> > +                                u32 pad, u64 streams_mask)
> > +{
> > +       struct imx471 *sensor = to_imx471(sd);
> > +       int ret;
> > +
> > +       cci_write(sensor->regmap, IMX471_REG_MODE_SELECT,
> > +                 IMX471_MODE_STANDBY, &ret);
>
> If you use ret for the cci_write() error path, it must be initialized.
>
> Here, you can directly get the error by using:
> ret = cci_write(sensor->regmap, IMX471_REG_MODE_SELECT, IMX471_MODE_STANDBY, NULL);
I'll correct this.

>
> > +       pm_runtime_put(sensor->dev);
> > +
> > +       if (ret)
> > +               dev_err(sensor->dev,
> > +                       "failed to disable stream with return value: %d\n",
> > +                       ret);
> > +       __v4l2_ctrl_grab(sensor->vflip, false);
> > +       __v4l2_ctrl_grab(sensor->hflip, false);
> > +
> > +       return 0;
> > +}
> > +
> > +static const struct v4l2_subdev_core_ops imx471_subdev_core_ops = {
> > +       .subscribe_event = v4l2_ctrl_subdev_subscribe_event,
> > +       .unsubscribe_event = v4l2_event_subdev_unsubscribe,
> > +};
>
> Drop this
>
> See: https://lore.kernel.org/linux-media/20241029162106.3005800-1-tomm.merciai@gmail.com/
Okay

>
> > +
> > +static const struct v4l2_subdev_video_ops imx471_video_ops = {
> > +       .s_stream = v4l2_subdev_s_stream_helper,
> > +};
> > +
> > +static const struct v4l2_subdev_pad_ops imx471_pad_ops = {
> > +       .enum_mbus_code = imx471_enum_mbus_code,
> > +       .get_fmt = v4l2_subdev_get_fmt,
> > +       .set_fmt = imx471_set_pad_format,
> > +       .get_selection = imx471_get_selection,
> > +       .enum_frame_size = imx471_enum_frame_size,
> > +       .enable_streams = imx471_enable_stream,
> > +       .disable_streams = imx471_disable_stream,
> > +};
> > +
> > +static const struct v4l2_subdev_ops imx471_subdev_ops = {
> > +       .core = &imx471_subdev_core_ops,
> > +       .video = &imx471_video_ops,
> > +       .pad = &imx471_pad_ops,
> > +};
> > +
> > +static const struct v4l2_subdev_internal_ops imx471_internal_ops = {
> > +       .init_state = imx471_init_state,
> > +};
> > +
> > +/* Initialize control handlers */
> > +static int imx471_init_controls(struct imx471 *sensor)
> > +{
> > +       const struct imx471_mode *mode = &imx471_modes[0];
> > +       struct v4l2_ctrl_handler *ctrl_hdlr;
> > +       struct v4l2_fwnode_device_properties props;
> > +       s64 exposure_max, hblank;
> > +       u64 pixel_rate;
> > +       int ret;
> > +
> > +       ctrl_hdlr = &sensor->ctrl_handler;
> > +       ret = v4l2_ctrl_handler_init(ctrl_hdlr, 10);
> > +       if (ret)
> > +               return ret;
>
> You can skip this error check.
>
> Also, there are 12 controls here, 10 controls + 2 orientation controls.

ok

>
> > +
> > +       ret = v4l2_fwnode_device_parse(sensor->dev, &props);
> > +       if (ret) {
> > +               dev_err(sensor->dev, "failed to parse fwnode: %d", ret);
> > +               return ret;
> > +       }
> > +
> > +       v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &imx471_ctrl_ops, &props);
> > +
> > +       sensor->link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr,
> > +                                                  &imx471_ctrl_ops,
> > +                                                  V4L2_CID_LINK_FREQ,
> > +                                                  ARRAY_SIZE(link_freq_menu_items) - 1,
> > +                                                  0,
> > +                                                  link_freq_menu_items);
> > +       if (sensor->link_freq)
> > +               sensor->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > +
> > +       /* pixel_rate = link_freq * 2 * nr_of_lanes / bits_per_sample */
> > +       pixel_rate = IMX471_LINK_FREQ_DEFAULT * 2 * 4;
> > +       div_u64(pixel_rate, 10);
>
> As I mentioned above, please store the return value of div_u64():
>
> pixel_rate = div_u64(IMX471_LINK_FREQ_DEFAULT * 2 * 4, 10);
I'll fix them.
>
> > +       /* By default, PIXEL_RATE is read only */
> > +       sensor->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &imx471_ctrl_ops,
> > +                                              V4L2_CID_PIXEL_RATE, pixel_rate,
> > +                                              pixel_rate, 1, pixel_rate);
> > +
> > +       /* Initial vblank/hblank/exposure parameters based on current mode */
> > +       sensor->vblank = v4l2_ctrl_new_std(ctrl_hdlr,
> > +                                          &imx471_ctrl_ops,
> > +                                          V4L2_CID_VBLANK,
> > +                                          mode->fll_min - mode->height,
> > +                                          IMX471_FLL_MAX - mode->height,
> > +                                          1,
> > +                                          mode->fll_def - mode->height);
> > +
> > +       hblank = mode->llp - mode->width;
> > +       sensor->hblank = v4l2_ctrl_new_std(ctrl_hdlr, &imx471_ctrl_ops,
> > +                                          V4L2_CID_HBLANK, hblank, hblank,
> > +                                          1, hblank);
> > +       if (sensor->hblank)
> > +               sensor->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > +
> > +       /* fll >= exposure time + adjust parameter (default value is 18) */
> > +       exposure_max = mode->fll_def - IMX471_EXPOSURE_MARGIN;
> > +       sensor->exposure = v4l2_ctrl_new_std(ctrl_hdlr, &imx471_ctrl_ops,
> > +                                            V4L2_CID_EXPOSURE,
> > +                                            IMX471_EXPOSURE_MIN, exposure_max,
> > +                                            IMX471_EXPOSURE_STEP,
> > +                                            IMX471_EXPOSURE_DEFAULT);
> > +
> > +       v4l2_ctrl_new_std(ctrl_hdlr, &imx471_ctrl_ops, V4L2_CID_ANALOGUE_GAIN,
> > +                         IMX471_ANA_GAIN_MIN, IMX471_ANA_GAIN_MAX,
> > +                         IMX471_ANA_GAIN_STEP, IMX471_ANA_GAIN_DEFAULT);
> > +
> > +       /* Digital gain */
> > +       v4l2_ctrl_new_std(ctrl_hdlr, &imx471_ctrl_ops, V4L2_CID_DIGITAL_GAIN,
> > +                         IMX471_DGTL_GAIN_MIN, IMX471_DGTL_GAIN_MAX,
> > +                         IMX471_DGTL_GAIN_STEP, IMX471_DGTL_GAIN_DEFAULT);
> > +
> > +       v4l2_ctrl_new_std_menu_items(ctrl_hdlr, &imx471_ctrl_ops,
> > +                                    V4L2_CID_TEST_PATTERN,
> > +                                    ARRAY_SIZE(imx471_test_pattern_menu) - 1,
> > +                                    0, 0, imx471_test_pattern_menu);
> > +
> > +       /* HFLIP & VFLIP */
>
> I can see many comments in your code that are not very important.
> You can remove them if you want, it's up to you.
Sure.

>
> > +       sensor->hflip = v4l2_ctrl_new_std(ctrl_hdlr, &imx471_ctrl_ops,
> > +                                         V4L2_CID_HFLIP, 0, 1, 1, 0);
> > +
> > +       sensor->vflip = v4l2_ctrl_new_std(ctrl_hdlr, &imx471_ctrl_ops,
> > +                                         V4L2_CID_VFLIP, 0, 1, 1, 0);
> > +
> > +       if (ctrl_hdlr->error) {
> > +               dev_err(sensor->dev, "%s control init failed: %d",
> > +                       __func__, ctrl_hdlr->error);
> > +               goto error;
> > +       }
> > +
> > +       sensor->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> > +       sensor->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> > +
> > +       sensor->sd.ctrl_handler = ctrl_hdlr;
> > +
> > +       return 0;
> > +
> > +error:
> > +       v4l2_ctrl_handler_free(ctrl_hdlr);
> > +
> > +       return ctrl_hdlr->error;
> > +}
> > +
> > +static int imx471_check_hwcfg(struct imx471 *sensor)
> > +{
> > +       struct v4l2_fwnode_endpoint bus_cfg = {
> > +               .bus_type = V4L2_MBUS_CSI2_DPHY,
> > +       };
> > +       struct fwnode_handle *ep, *fwnode = dev_fwnode(sensor->dev);
> > +       struct clk *clk;
> > +       unsigned long link_freq_bitmap;
> > +       int ret;
>
> If you want, you can sort the variable declarations throughout the code, where
> appropriate, by length to make them more readable.
okay

>
> > +
> > +       clk = devm_v4l2_sensor_clk_get(sensor->dev, NULL);
> > +       if (IS_ERR(clk))
> > +               return dev_err_probe(sensor->dev, PTR_ERR(clk),
> > +                                    "can't get clock frequency\n");
> > +
> > +       if (clk_get_rate(clk) != IMX471_EXT_CLK)
> > +               return dev_err_probe(sensor->dev, -EINVAL,
> > +                                    "external clock %lu is not supported\n",
> > +                                    clk_get_rate(clk));
> > +
> > +       ep = fwnode_graph_get_endpoint_by_id(fwnode, 0, 0, 0);
> > +       ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg);
> > +       fwnode_handle_put(ep);
> > +       if (ret)
> > +               return dev_err_probe(sensor->dev, ret,
> > +                                    "parsing endpoint failed");
> > +
> > +       ret = v4l2_link_freq_to_bitmap(sensor->dev, bus_cfg.link_frequencies,
> > +                                      bus_cfg.nr_of_link_frequencies,
> > +                                      link_freq_menu_items,
> > +                                      ARRAY_SIZE(link_freq_menu_items),
> > +                                      &link_freq_bitmap);
>
> This can fail silently. Please add an error message before returning the failure.
The error messages are in the v4l2_link_freq_to_bitmap() therefore I
don't put additinal error messages for it.

>
> > +
> > +       v4l2_fwnode_endpoint_free(&bus_cfg);
> > +
> > +       return ret;
> > +}
> > +
> > +static int imx471_probe(struct i2c_client *client)
> > +{
> > +       struct imx471 *sensor;
> > +       int ret;
> > +
> > +       sensor = devm_kzalloc(&client->dev, sizeof(*sensor), GFP_KERNEL);
> > +       if (!sensor)
> > +               return dev_err_probe(&client->dev, -ENOMEM,
> > +                                    "failed to allocate memory\n");
> > +
> > +       sensor->dev = &client->dev;
> > +
> > +       /* Check HW config */
> > +       ret = imx471_check_hwcfg(sensor);
> > +       if (ret)
> > +               return dev_err_probe(sensor->dev, ret,
> > +                                    "failed to check hwcfg: %d\n", ret);
> > +
> > +       ret = imx471_get_regulators(sensor->dev, sensor);
> > +       if (ret)
> > +               return dev_err_probe(sensor->dev, ret,
> > +                                    "failed to get regulators\n");
> > +
> > +       sensor->reset_gpio = devm_gpiod_get_optional(sensor->dev, "reset",
> > +                                                    GPIOD_OUT_HIGH);
> > +       if (IS_ERR(sensor->reset_gpio))
> > +               return dev_err_probe(sensor->dev, PTR_ERR(sensor->reset_gpio),
> > +                                    "failed to get reset gpio\n");
> > +
> > +       sensor->img_clk = devm_clk_get_optional(sensor->dev, NULL);
> > +       if (IS_ERR(sensor->img_clk))
> > +               return dev_err_probe(sensor->dev, PTR_ERR(sensor->img_clk),
> > +                                    "failed to get imaging clock\n");
> > +
> > +       /* Initialize subdev */
> > +       v4l2_i2c_subdev_init(&sensor->sd, client, &imx471_subdev_ops);
> > +
> > +       /* Initialize regmap */
> > +       sensor->regmap = devm_cci_regmap_init_i2c(client, 16);
> > +       if (IS_ERR(sensor->regmap))
> > +               return PTR_ERR(sensor->regmap);
>
> Please add an error message before returning the failure.

Ok

>
> > +
> > +       ret = imx471_power_on(sensor->dev);
> > +       if (ret)
> > +               return dev_err_probe(sensor->dev, ret,
> > +                                    "failed to power on\n");
> > +
> > +       /* Check module identity */
> > +       ret = imx471_identify_module(sensor);
> > +       if (ret) {
> > +               dev_err(&client->dev, "failed to find sensor: %d", ret);
>
> Use dev_err_probe();
>
> Also, add a newline character at the end of the print statement.
ok

>
> > +               goto error_power_off;
> > +       }
> > +
> > +       ret = imx471_init_controls(sensor);
> > +       if (ret) {
> > +               dev_err(sensor->dev, "failed to init controls: %d", ret);
>
> Same here.
got it.
>
> > +               goto error_power_off;
> > +       }
> > +
> > +       /* Initialize subdev */
> > +       sensor->sd.internal_ops = &imx471_internal_ops;
> > +       sensor->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
> > +                           V4L2_SUBDEV_FL_HAS_EVENTS;
>
> See: https://lore.kernel.org/linux-media/20241029162106.3005800-1-tomm.merciai@gmail.com/
Ok. Drop V4L2_SUBDEV_FL_HAS_EVENTS.
>
> > +       sensor->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> > +
> > +       /* Initialize source pad */
> > +       sensor->pad.flags = MEDIA_PAD_FL_SOURCE;
> > +       ret = media_entity_pads_init(&sensor->sd.entity, 1, &sensor->pad);
> > +       if (ret) {
> > +               dev_err(&client->dev, "failed to init entity pads: %d", ret);
>
> Use dev_err_probe.
>
> > +               goto error_v4l2_ctrl_handler_free;
> > +       }
> > +
> > +       sensor->sd.state_lock = sensor->ctrl_handler.lock;
> > +       ret = v4l2_subdev_init_finalize(&sensor->sd);
> > +       if (ret < 0) {
> > +               dev_err(&client->dev, "failed to init subdev: %d", ret);
>
> Same here.

Ok

>
> > +               goto error_media_entity_pm;
> > +       }
> > +
> > +       pm_runtime_set_active(sensor->dev);
> > +       pm_runtime_enable(sensor->dev);
> > +       pm_runtime_idle(sensor->dev);
> > +
> > +       ret = v4l2_async_register_subdev_sensor(&sensor->sd);
> > +       if (ret < 0)
> > +               goto error_v4l2_subdev_cleanup;
> > +
> > +       return 0;
> > +
> > +error_v4l2_subdev_cleanup:
> > +       pm_runtime_disable(sensor->dev);
> > +       pm_runtime_set_suspended(sensor->dev);
> > +       v4l2_subdev_cleanup(&sensor->sd);
> > +
> > +error_media_entity_pm:
> > +       media_entity_cleanup(&sensor->sd.entity);
> > +
> > +error_v4l2_ctrl_handler_free:
> > +       v4l2_ctrl_handler_free(sensor->sd.ctrl_handler);
> > +
> > +error_power_off:
> > +       imx471_power_off(sensor->dev);
> > +
> > +       return ret;
> > +}
> > +
> > +static void imx471_remove(struct i2c_client *client)
> > +{
> > +       struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > +       struct imx471 *sensor = to_imx471(sd);
> > +
> > +       v4l2_async_unregister_subdev(sd);
> > +       v4l2_subdev_cleanup(sd);
> > +       media_entity_cleanup(&sd->entity);
> > +       v4l2_ctrl_handler_free(sd->ctrl_handler);
> > +
> > +       pm_runtime_disable(&client->dev);
> > +
> > +       if (!pm_runtime_status_suspended(sensor->dev)) {
> > +               imx471_power_off(sensor->dev);
> > +               pm_runtime_set_suspended(sensor->dev);
>
> You can use client->dev here and drop the struct imx471 *sensor = to_imx471(sd); line.
Okay
>
> > +       }
> > +}
> > +
> > +static DEFINE_RUNTIME_DEV_PM_OPS(imx471_pm_ops, imx471_power_off,
> > +                                imx471_power_on, NULL);
> > +
> > +static const struct acpi_device_id imx471_acpi_ids[] __maybe_unused = {
> > +       { "SONY471A" },
> > +       { /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(acpi, imx471_acpi_ids);
> > +
> > +static struct i2c_driver imx471_i2c_driver = {
> > +       .driver = {
> > +               .name = "imx471",
> > +               .acpi_match_table = ACPI_PTR(imx471_acpi_ids),
> > +               .pm = pm_sleep_ptr(&imx471_pm_ops),
> > +       },
> > +       .probe = imx471_probe,
> > +       .remove = imx471_remove,
> > +};
> > +module_i2c_driver(imx471_i2c_driver);
> > +
> > +MODULE_AUTHOR("Jimmy Su <jimmy.su@intel.com>");
> > +MODULE_AUTHOR("Serin Yeh <serin.yeh@intel.com>");
> > +MODULE_AUTHOR("Kate Hsuan <hpa@redhat.com>");
> > +MODULE_DESCRIPTION("Sony imx471 sensor driver");
> > +MODULE_LICENSE("GPL");
> > --
> > 2.54.0
>
> Best Regards,
> Tarang
>

I'll propose the v4 patch to include the fixes. :)

-- 
BR,
Kate

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

[Sakari Ailus]

Hi Kate,

Thanks for the update.

On Fri, May 22, 2026 at 11:11:20AM +0800, 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 X9-14 and X9-15 laptop and it is a part

s/laptop\K/s/

> of IPU7 solution. The driver was tested on Lenovo X9-14 and X9-15 laptops.
> 
> Signed-off-by: Kate Hsuan <hpa@redhat.com>
> ---
>  MAINTAINERS                |    6 +
>  drivers/media/i2c/Kconfig  |   10 +
>  drivers/media/i2c/Makefile |    1 +
>  drivers/media/i2c/imx471.c | 1006 ++++++++++++++++++++++++++++++++++++
>  4 files changed, 1023 insertions(+)
>  create mode 100644 drivers/media/i2c/imx471.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 1126fdd639ad..d597337e7c24 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -24735,6 +24735,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/imx471.c b/drivers/media/i2c/imx471.c
> new file mode 100644
> index 000000000000..f3c7fdce2d50
> --- /dev/null
> +++ b/drivers/media/i2c/imx471.c

...

> +
> +#define IMX471_NUM_SUPPLIES			ARRAY_SIZE(imx471_supply_name)

Please just use ARRAY_SIZE() where you need it.

> +
> +/* Mode : resolution and related config&values */
> +struct imx471_mode {
> +	/* Frame width */
> +	u32 width;
> +	/* Frame height */
> +	u32 height;
> +
> +	/* V-timing */
> +	u32 fll_def;
> +	u32 fll_min;
> +
> +	/* H-timing */
> +	u32 llp;
> +
> +	/* index of link frequency */
> +	u32 link_freq_index;
> +
> +	/* Default register values */
> +	const struct cci_reg_sequence *default_mode_regs;
> +	const int default_mode_regs_length;

unsigned int?

const-ness here doesn't really matter as the entire struct is const.

> +};
> +
> +struct imx471 {
> +	struct v4l2_subdev sd;
> +	struct media_pad pad;
> +
> +	struct v4l2_ctrl_handler ctrl_handler;
> +	/* V4L2 Controls */
> +	struct v4l2_ctrl *link_freq;
> +	struct v4l2_ctrl *pixel_rate;
> +	struct v4l2_ctrl *vblank;
> +	struct v4l2_ctrl *hblank;
> +	struct v4l2_ctrl *vflip;
> +	struct v4l2_ctrl *hflip;
> +	struct v4l2_ctrl *exposure;

Do you need all these? At least link_freq remains effectively unused.

> +
> +	struct gpio_desc *reset_gpio;
> +	struct regulator_bulk_data supplies[IMX471_NUM_SUPPLIES];
> +	struct clk *img_clk;
> +
> +	struct device *dev;
> +	struct regmap *regmap;
> +};
> +
> +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 },
> +	{ CCI_REG8(0x4431), 0x04 },
> +	{ CCI_REG8(0x531c), 0x01 },
> +	{ CCI_REG8(0x531d), 0x02 },
> +	{ CCI_REG8(0x531e), 0x04 },
> +	{ CCI_REG8(0x5928), 0x00 },
> +	{ CCI_REG8(0x5929), 0x2f },
> +	{ CCI_REG8(0x592a), 0x00 },
> +	{ CCI_REG8(0x592b), 0x85 },
> +	{ CCI_REG8(0x592c), 0x00 },
> +	{ CCI_REG8(0x592d), 0x32 },
> +	{ CCI_REG8(0x592e), 0x00 },
> +	{ CCI_REG8(0x592f), 0x88 },
> +	{ CCI_REG8(0x5930), 0x00 },
> +	{ CCI_REG8(0x5931), 0x3d },
> +	{ CCI_REG8(0x5932), 0x00 },
> +	{ CCI_REG8(0x5933), 0x93 },
> +	{ CCI_REG8(0x5938), 0x00 },
> +	{ CCI_REG8(0x5939), 0x24 },
> +	{ CCI_REG8(0x593a), 0x00 },
> +	{ CCI_REG8(0x593b), 0x7a },
> +	{ CCI_REG8(0x593c), 0x00 },
> +	{ CCI_REG8(0x593d), 0x24 },
> +	{ CCI_REG8(0x593e), 0x00 },
> +	{ CCI_REG8(0x593f), 0x7a },
> +	{ CCI_REG8(0x5940), 0x00 },
> +	{ CCI_REG8(0x5941), 0x2f },
> +	{ CCI_REG8(0x5942), 0x00 },
> +	{ CCI_REG8(0x5943), 0x85 },
> +	{ CCI_REG8(0x5f0e), 0x6e },
> +	{ CCI_REG8(0x5f11), 0xc6 },
> +	{ CCI_REG8(0x5f17), 0x5e },
> +	{ CCI_REG8(0x7990), 0x01 },
> +	{ CCI_REG8(0x7993), 0x5d },
> +	{ CCI_REG8(0x7994), 0x5d },
> +	{ CCI_REG8(0x7995), 0xa1 },
> +	{ CCI_REG8(0x799a), 0x01 },
> +	{ CCI_REG8(0x799d), 0x00 },
> +	{ CCI_REG8(0x8169), 0x01 },
> +	{ CCI_REG8(0x8359), 0x01 },
> +	{ CCI_REG8(0x9302), 0x1e },
> +	{ CCI_REG8(0x9306), 0x1f },
> +	{ CCI_REG8(0x930a), 0x26 },
> +	{ CCI_REG8(0x930e), 0x23 },
> +	{ CCI_REG8(0x9312), 0x23 },
> +	{ CCI_REG8(0x9316), 0x2c },
> +	{ CCI_REG8(0x9317), 0x19 },
> +	{ CCI_REG8(0xb046), 0x01 },
> +	{ CCI_REG8(0xb048), 0x01 },
> +};
> +
> +static const struct cci_reg_sequence mode_1928x1088_regs[] = {
> +	{ CCI_REG8(0x0101), 0x00 },
> +	{ CCI_REG8(0x0112), 0x0a },
> +	{ CCI_REG8(0x0113), 0x0a },
> +	{ CCI_REG8(0x0114), 0x03 },
> +	{ CCI_REG8(0x0342), 0x0a },
> +	{ CCI_REG8(0x0343), 0x00 },
> +	{ CCI_REG8(0x0340), 0x13 },
> +	{ CCI_REG8(0x0341), 0xb0 },
> +	{ CCI_REG8(0x0344), 0x00 },
> +	{ CCI_REG8(0x0345), 0x00 },
> +	{ CCI_REG8(0x0346), 0x01 },
> +	{ CCI_REG8(0x0347), 0xbc },
> +	{ CCI_REG8(0x0348), 0x12 },
> +	{ CCI_REG8(0x0349), 0x2f },
> +	{ CCI_REG8(0x034a), 0x0b },
> +	{ CCI_REG8(0x034b), 0xeb },
> +	{ CCI_REG8(0x0381), 0x01 },
> +	{ CCI_REG8(0x0383), 0x01 },
> +	{ CCI_REG8(0x0385), 0x01 },
> +	{ CCI_REG8(0x0387), 0x01 },
> +	{ CCI_REG8(0x0900), 0x01 },
> +	{ CCI_REG8(0x0901), 0x22 },
> +	{ CCI_REG8(0x0902), 0x08 },
> +	{ CCI_REG8(0x3f4c), 0x81 },
> +	{ CCI_REG8(0x3f4d), 0x81 },
> +	{ CCI_REG8(0x0408), 0x00 },
> +	{ CCI_REG8(0x0409), 0xc8 },
> +	{ CCI_REG8(0x040a), 0x00 },
> +	{ CCI_REG8(0x040b), 0x6c },
> +	{ CCI_REG8(0x040c), 0x07 },
> +	{ CCI_REG8(0x040d), 0x88 },
> +	{ CCI_REG8(0x040e), 0x04 },
> +	{ CCI_REG8(0x040f), 0x40 },
> +	{ CCI_REG8(0x034c), 0x07 },
> +	{ CCI_REG8(0x034d), 0x88 },
> +	{ CCI_REG8(0x034e), 0x04 },
> +	{ CCI_REG8(0x034f), 0x40 },
> +	{ CCI_REG8(0x0301), 0x06 },
> +	{ CCI_REG8(0x0303), 0x02 },
> +	{ CCI_REG8(0x0305), 0x02 },
> +	{ CCI_REG8(0x0306), 0x00 },
> +	{ CCI_REG8(0x0307), 0x79 },
> +	{ CCI_REG8(0x030b), 0x01 },
> +	{ CCI_REG8(0x030d), 0x02 },
> +	{ CCI_REG8(0x030e), 0x00 },
> +	{ CCI_REG8(0x030f), 0x53 },
> +	{ CCI_REG8(0x0310), 0x01 },
> +	{ CCI_REG8(0x0202), 0x13 },
> +	{ CCI_REG8(0x0203), 0x9e },
> +	{ CCI_REG8(0x0204), 0x00 },
> +	{ CCI_REG8(0x0205), 0x00 },
> +	{ CCI_REG8(0x020e), 0x01 },
> +	{ CCI_REG8(0x020f), 0x00 },
> +	{ CCI_REG8(0x3f78), 0x01 },
> +	{ CCI_REG8(0x3f79), 0x31 },
> +	{ CCI_REG8(0x3ffe), 0x00 },
> +	{ CCI_REG8(0x3fff), 0x8a },
> +	{ CCI_REG8(0x5f0a), 0xb6 },
> +};
> +
> +static const char * const imx471_test_pattern_menu[] = {
> +	"Disabled",
> +	"Solid Colour",
> +	"Eight Vertical Colour Bars",
> +	"Colour Bars With Fade to Grey",
> +	"Pseudorandom Sequence (PN9)",
> +};
> +
> +/*
> + * When adding more than the one below, make sure the disallowed ones will
> + * actually be disabled in the LINK_FREQ control.
> + */
> +static const s64 link_freq_menu_items[] = {
> +	IMX471_LINK_FREQ_DEFAULT,
> +};
> +
> +/*
> + * The Bayer formats for the flipping.
> + * - no flip
> + * - h flip
> + * - v flip
> + * - h and v flips
> + */
> +static const u32 imx471_hv_flips_bayer_order[] = {
> +	MEDIA_BUS_FMT_SRGGB10_1X10,
> +	MEDIA_BUS_FMT_SGRBG10_1X10,
> +	MEDIA_BUS_FMT_SGBRG10_1X10,
> +	MEDIA_BUS_FMT_SBGGR10_1X10,
> +};
> +
> +/* Mode configs */
> +static const struct imx471_mode imx471_modes[] = {
> +	{
> +		.width = 1928,
> +		.height = 1088,
> +		.fll_def = 1308,
> +		.fll_min = 1308,
> +		.llp = 2328,
> +		.link_freq_index = IMX471_LINK_FREQ_INDEX,
> +		.default_mode_regs = mode_1928x1088_regs,
> +		.default_mode_regs_length = ARRAY_SIZE(mode_1928x1088_regs),
> +	},
> +};
> +
> +static int imx471_get_regulators(struct device *dev, struct imx471 *sensor)
> +{
> +	for (unsigned int  i = 0; i < IMX471_NUM_SUPPLIES; i++)

s/ \K //

> +		sensor->supplies[i].supply = imx471_supply_name[i];
> +
> +	return devm_regulator_bulk_get(dev, IMX471_NUM_SUPPLIES,
> +				       sensor->supplies);
> +}

...

> +/* Start streaming */
> +static int imx471_enable_stream(struct v4l2_subdev *sd,
> +				struct v4l2_subdev_state *state,
> +				u32 pad, u64 streams_mask)
> +{
> +	struct imx471 *sensor = to_imx471(sd);
> +	const struct imx471_mode *mode;
> +	struct v4l2_mbus_framefmt *fmt;
> +	int ret;
> +
> +	ret = pm_runtime_resume_and_get(sensor->dev);
> +	if (ret)

Just return the error code here -- pm_runtime_resume_and_get() won't
increment the usage count unless it succeeds.

> +		goto error_powerdown;
> +
> +	ret = imx471_identify_module(sensor);
> +	if (ret)
> +		return ret;
> +
> +	/* Global Setting */
> +	cci_multi_reg_write(sensor->regmap, imx471_global_regs,
> +			    ARRAY_SIZE(imx471_global_regs), &ret);
> +	if (ret) {
> +		dev_err(sensor->dev, "failed to set global settings");
> +		goto error_powerdown;
> +	}
> +
> +	state = v4l2_subdev_get_locked_active_state(&sensor->sd);
> +	fmt = v4l2_subdev_state_get_format(state, 0);
> +	mode = v4l2_find_nearest_size(imx471_modes, ARRAY_SIZE(imx471_modes),
> +				      width, height, fmt->width, fmt->height);
> +
> +	/* Apply default values of current mode */
> +	cci_multi_reg_write(sensor->regmap, mode->default_mode_regs,
> +			    mode->default_mode_regs_length, &ret);
> +	if (ret) {
> +		dev_err(sensor->dev, "failed to set mode");
> +		goto error_powerdown;
> +	}
> +
> +	/* set digital gain control to all color mode */
> +	cci_write(sensor->regmap, IMX471_REG_DPGA_USE_GLOBAL_GAIN, 1, &ret);
> +	if (ret)
> +		goto error_powerdown;
> +
> +	/* Apply customized values from user */
> +	ret =  __v4l2_ctrl_handler_setup(&sensor->ctrl_handler);
> +	if (ret)
> +		goto error_powerdown;
> +
> +	cci_write(sensor->regmap, IMX471_REG_MODE_SELECT,
> +		  IMX471_MODE_STREAMING, &ret);
> +	if (ret)
> +		goto error_powerdown;
> +
> +	__v4l2_ctrl_grab(sensor->vflip, true);
> +	__v4l2_ctrl_grab(sensor->hflip, true);
> +
> +	return ret;
> +
> +error_powerdown:
> +	pm_runtime_put(sensor->dev);
> +
> +	return ret;
> +}
> +
> +/* Stop streaming */
> +static int imx471_disable_stream(struct v4l2_subdev *sd,
> +				 struct v4l2_subdev_state *state,
> +				 u32 pad, u64 streams_mask)
> +{
> +	struct imx471 *sensor = to_imx471(sd);
> +	int ret;
> +
> +	cci_write(sensor->regmap, IMX471_REG_MODE_SELECT,
> +		  IMX471_MODE_STANDBY, &ret);
> +	pm_runtime_put(sensor->dev);
> +
> +	if (ret)
> +		dev_err(sensor->dev,
> +			"failed to disable stream with return value: %d\n",
> +			ret);
> +	__v4l2_ctrl_grab(sensor->vflip, false);
> +	__v4l2_ctrl_grab(sensor->hflip, false);
> +
> +	return 0;
> +}
> +
> +static const struct v4l2_subdev_core_ops imx471_subdev_core_ops = {
> +	.subscribe_event = v4l2_ctrl_subdev_subscribe_event,
> +	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
> +};
> +
> +static const struct v4l2_subdev_video_ops imx471_video_ops = {
> +	.s_stream = v4l2_subdev_s_stream_helper,
> +};
> +
> +static const struct v4l2_subdev_pad_ops imx471_pad_ops = {
> +	.enum_mbus_code = imx471_enum_mbus_code,
> +	.get_fmt = v4l2_subdev_get_fmt,
> +	.set_fmt = imx471_set_pad_format,
> +	.get_selection = imx471_get_selection,
> +	.enum_frame_size = imx471_enum_frame_size,
> +	.enable_streams = imx471_enable_stream,
> +	.disable_streams = imx471_disable_stream,
> +};
> +
> +static const struct v4l2_subdev_ops imx471_subdev_ops = {
> +	.core = &imx471_subdev_core_ops,
> +	.video = &imx471_video_ops,
> +	.pad = &imx471_pad_ops,
> +};
> +
> +static const struct v4l2_subdev_internal_ops imx471_internal_ops = {
> +	.init_state = imx471_init_state,
> +};
> +
> +/* Initialize control handlers */
> +static int imx471_init_controls(struct imx471 *sensor)
> +{
> +	const struct imx471_mode *mode = &imx471_modes[0];
> +	struct v4l2_ctrl_handler *ctrl_hdlr;
> +	struct v4l2_fwnode_device_properties props;
> +	s64 exposure_max, hblank;
> +	u64 pixel_rate;
> +	int ret;
> +
> +	ctrl_hdlr = &sensor->ctrl_handler;
> +	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 10);
> +	if (ret)
> +		return ret;
> +
> +	ret = v4l2_fwnode_device_parse(sensor->dev, &props);
> +	if (ret) {
> +		dev_err(sensor->dev, "failed to parse fwnode: %d", ret);
> +		return ret;
> +	}
> +
> +	v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &imx471_ctrl_ops, &props);
> +
> +	sensor->link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr,
> +						   &imx471_ctrl_ops,
> +						   V4L2_CID_LINK_FREQ,
> +						   ARRAY_SIZE(link_freq_menu_items) - 1,
> +						   0,
> +						   link_freq_menu_items);
> +	if (sensor->link_freq)
> +		sensor->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +
> +	/* pixel_rate = link_freq * 2 * nr_of_lanes / bits_per_sample */
> +	pixel_rate = IMX471_LINK_FREQ_DEFAULT * 2 * 4;
> +	div_u64(pixel_rate, 10);
> +	/* By default, PIXEL_RATE is read only */
> +	sensor->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &imx471_ctrl_ops,
> +					       V4L2_CID_PIXEL_RATE, pixel_rate,
> +					       pixel_rate, 1, pixel_rate);
> +
> +	/* Initial vblank/hblank/exposure parameters based on current mode */
> +	sensor->vblank = v4l2_ctrl_new_std(ctrl_hdlr,
> +					   &imx471_ctrl_ops,
> +					   V4L2_CID_VBLANK,
> +					   mode->fll_min - mode->height,
> +					   IMX471_FLL_MAX - mode->height,
> +					   1,
> +					   mode->fll_def - mode->height);
> +
> +	hblank = mode->llp - mode->width;
> +	sensor->hblank = v4l2_ctrl_new_std(ctrl_hdlr, &imx471_ctrl_ops,
> +					   V4L2_CID_HBLANK, hblank, hblank,
> +					   1, hblank);
> +	if (sensor->hblank)
> +		sensor->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +
> +	/* fll >= exposure time + adjust parameter (default value is 18) */
> +	exposure_max = mode->fll_def - IMX471_EXPOSURE_MARGIN;
> +	sensor->exposure = v4l2_ctrl_new_std(ctrl_hdlr, &imx471_ctrl_ops,
> +					     V4L2_CID_EXPOSURE,
> +					     IMX471_EXPOSURE_MIN, exposure_max,
> +					     IMX471_EXPOSURE_STEP,
> +					     IMX471_EXPOSURE_DEFAULT);
> +
> +	v4l2_ctrl_new_std(ctrl_hdlr, &imx471_ctrl_ops, V4L2_CID_ANALOGUE_GAIN,
> +			  IMX471_ANA_GAIN_MIN, IMX471_ANA_GAIN_MAX,
> +			  IMX471_ANA_GAIN_STEP, IMX471_ANA_GAIN_DEFAULT);
> +
> +	/* Digital gain */
> +	v4l2_ctrl_new_std(ctrl_hdlr, &imx471_ctrl_ops, V4L2_CID_DIGITAL_GAIN,
> +			  IMX471_DGTL_GAIN_MIN, IMX471_DGTL_GAIN_MAX,
> +			  IMX471_DGTL_GAIN_STEP, IMX471_DGTL_GAIN_DEFAULT);
> +
> +	v4l2_ctrl_new_std_menu_items(ctrl_hdlr, &imx471_ctrl_ops,
> +				     V4L2_CID_TEST_PATTERN,
> +				     ARRAY_SIZE(imx471_test_pattern_menu) - 1,
> +				     0, 0, imx471_test_pattern_menu);
> +
> +	/* HFLIP & VFLIP */
> +	sensor->hflip = v4l2_ctrl_new_std(ctrl_hdlr, &imx471_ctrl_ops,
> +					  V4L2_CID_HFLIP, 0, 1, 1, 0);
> +
> +	sensor->vflip = v4l2_ctrl_new_std(ctrl_hdlr, &imx471_ctrl_ops,
> +					  V4L2_CID_VFLIP, 0, 1, 1, 0);
> +
> +	if (ctrl_hdlr->error) {
> +		dev_err(sensor->dev, "%s control init failed: %d",
> +			__func__, ctrl_hdlr->error);
> +		goto error;
> +	}
> +
> +	sensor->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> +	sensor->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;

Either set all flags where controls are created or here. I think I'd move
them here.

> +
> +	sensor->sd.ctrl_handler = ctrl_hdlr;
> +
> +	return 0;
> +
> +error:
> +	v4l2_ctrl_handler_free(ctrl_hdlr);
> +
> +	return ctrl_hdlr->error;
> +}
> +
> +static int imx471_check_hwcfg(struct imx471 *sensor)
> +{
> +	struct v4l2_fwnode_endpoint bus_cfg = {
> +		.bus_type = V4L2_MBUS_CSI2_DPHY,
> +	};
> +	struct fwnode_handle *ep, *fwnode = dev_fwnode(sensor->dev);
> +	struct clk *clk;
> +	unsigned long link_freq_bitmap;
> +	int ret;
> +
> +	clk = devm_v4l2_sensor_clk_get(sensor->dev, NULL);
> +	if (IS_ERR(clk))
> +		return dev_err_probe(sensor->dev, PTR_ERR(clk),
> +				     "can't get clock frequency\n");
> +
> +	if (clk_get_rate(clk) != IMX471_EXT_CLK)
> +		return dev_err_probe(sensor->dev, -EINVAL,
> +				     "external clock %lu is not supported\n",
> +				     clk_get_rate(clk));
> +
> +	ep = fwnode_graph_get_endpoint_by_id(fwnode, 0, 0, 0);
> +	ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg);
> +	fwnode_handle_put(ep);
> +	if (ret)
> +		return dev_err_probe(sensor->dev, ret,
> +				     "parsing endpoint failed");
> +
> +	ret = v4l2_link_freq_to_bitmap(sensor->dev, bus_cfg.link_frequencies,
> +				       bus_cfg.nr_of_link_frequencies,
> +				       link_freq_menu_items,
> +				       ARRAY_SIZE(link_freq_menu_items),
> +				       &link_freq_bitmap);
> +
> +	v4l2_fwnode_endpoint_free(&bus_cfg);
> +
> +	return ret;
> +}
> +
> +static int imx471_probe(struct i2c_client *client)
> +{
> +	struct imx471 *sensor;
> +	int ret;
> +
> +	sensor = devm_kzalloc(&client->dev, sizeof(*sensor), GFP_KERNEL);
> +	if (!sensor)
> +		return dev_err_probe(&client->dev, -ENOMEM,
> +				     "failed to allocate memory\n");
> +
> +	sensor->dev = &client->dev;
> +
> +	/* Check HW config */
> +	ret = imx471_check_hwcfg(sensor);
> +	if (ret)
> +		return dev_err_probe(sensor->dev, ret,
> +				     "failed to check hwcfg: %d\n", ret);
> +
> +	ret = imx471_get_regulators(sensor->dev, sensor);
> +	if (ret)
> +		return dev_err_probe(sensor->dev, ret,
> +				     "failed to get regulators\n");
> +
> +	sensor->reset_gpio = devm_gpiod_get_optional(sensor->dev, "reset",
> +						     GPIOD_OUT_HIGH);
> +	if (IS_ERR(sensor->reset_gpio))
> +		return dev_err_probe(sensor->dev, PTR_ERR(sensor->reset_gpio),
> +				     "failed to get reset gpio\n");
> +
> +	sensor->img_clk = devm_clk_get_optional(sensor->dev, NULL);
> +	if (IS_ERR(sensor->img_clk))
> +		return dev_err_probe(sensor->dev, PTR_ERR(sensor->img_clk),
> +				     "failed to get imaging clock\n");
> +
> +	/* Initialize subdev */
> +	v4l2_i2c_subdev_init(&sensor->sd, client, &imx471_subdev_ops);
> +
> +	/* Initialize regmap */
> +	sensor->regmap = devm_cci_regmap_init_i2c(client, 16);
> +	if (IS_ERR(sensor->regmap))
> +		return PTR_ERR(sensor->regmap);
> +
> +	ret = imx471_power_on(sensor->dev);
> +	if (ret)
> +		return dev_err_probe(sensor->dev, ret,
> +				     "failed to power on\n");
> +
> +	/* Check module identity */
> +	ret = imx471_identify_module(sensor);
> +	if (ret) {
> +		dev_err(&client->dev, "failed to find sensor: %d", ret);
> +		goto error_power_off;
> +	}
> +
> +	ret = imx471_init_controls(sensor);
> +	if (ret) {
> +		dev_err(sensor->dev, "failed to init controls: %d", ret);
> +		goto error_power_off;
> +	}
> +
> +	/* Initialize subdev */
> +	sensor->sd.internal_ops = &imx471_internal_ops;
> +	sensor->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
> +			    V4L2_SUBDEV_FL_HAS_EVENTS;
> +	sensor->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> +
> +	/* Initialize source pad */
> +	sensor->pad.flags = MEDIA_PAD_FL_SOURCE;
> +	ret = media_entity_pads_init(&sensor->sd.entity, 1, &sensor->pad);
> +	if (ret) {
> +		dev_err(&client->dev, "failed to init entity pads: %d", ret);
> +		goto error_v4l2_ctrl_handler_free;
> +	}
> +
> +	sensor->sd.state_lock = sensor->ctrl_handler.lock;
> +	ret = v4l2_subdev_init_finalize(&sensor->sd);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "failed to init subdev: %d", ret);
> +		goto error_media_entity_pm;
> +	}
> +
> +	pm_runtime_set_active(sensor->dev);
> +	pm_runtime_enable(sensor->dev);
> +	pm_runtime_idle(sensor->dev);

Move the pm_runtime_idle() call after v4l2_async_register_subdev_sensor().
Otherwise Runtime PM may power off the sensor on error and the driver still
calls imx471_power_off().

> +
> +	ret = v4l2_async_register_subdev_sensor(&sensor->sd);
> +	if (ret < 0)
> +		goto error_v4l2_subdev_cleanup;
> +
> +	return 0;
> +
> +error_v4l2_subdev_cleanup:
> +	pm_runtime_disable(sensor->dev);
> +	pm_runtime_set_suspended(sensor->dev);
> +	v4l2_subdev_cleanup(&sensor->sd);
> +
> +error_media_entity_pm:
> +	media_entity_cleanup(&sensor->sd.entity);
> +
> +error_v4l2_ctrl_handler_free:
> +	v4l2_ctrl_handler_free(sensor->sd.ctrl_handler);
> +
> +error_power_off:
> +	imx471_power_off(sensor->dev);
> +
> +	return ret;
> +}
> +
> +static void imx471_remove(struct i2c_client *client)
> +{
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct imx471 *sensor = to_imx471(sd);
> +
> +	v4l2_async_unregister_subdev(sd);
> +	v4l2_subdev_cleanup(sd);
> +	media_entity_cleanup(&sd->entity);
> +	v4l2_ctrl_handler_free(sd->ctrl_handler);
> +
> +	pm_runtime_disable(&client->dev);
> +
> +	if (!pm_runtime_status_suspended(sensor->dev)) {
> +		imx471_power_off(sensor->dev);
> +		pm_runtime_set_suspended(sensor->dev);
> +	}
> +}
> +
> +static DEFINE_RUNTIME_DEV_PM_OPS(imx471_pm_ops, imx471_power_off,
> +				 imx471_power_on, NULL);
> +
> +static const struct acpi_device_id imx471_acpi_ids[] __maybe_unused = {
> +	{ "SONY471A" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(acpi, imx471_acpi_ids);
> +
> +static struct i2c_driver imx471_i2c_driver = {
> +	.driver = {
> +		.name = "imx471",
> +		.acpi_match_table = ACPI_PTR(imx471_acpi_ids),
> +		.pm = pm_sleep_ptr(&imx471_pm_ops),
> +	},
> +	.probe = imx471_probe,
> +	.remove = imx471_remove,
> +};
> +module_i2c_driver(imx471_i2c_driver);
> +
> +MODULE_AUTHOR("Jimmy Su <jimmy.su@intel.com>");
> +MODULE_AUTHOR("Serin Yeh <serin.yeh@intel.com>");
> +MODULE_AUTHOR("Kate Hsuan <hpa@redhat.com>");
> +MODULE_DESCRIPTION("Sony imx471 sensor driver");
> +MODULE_LICENSE("GPL");

-- 
Kind regards,

Sakari Ailus

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

[Kate Hsuan]

Hi Sakari,

Thank you for reviewing.

On Sat, May 23, 2026 at 6:55 AM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Kate,
>
> Thanks for the update.
>
> On Fri, May 22, 2026 at 11:11:20AM +0800, 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 X9-14 and X9-15 laptop and it is a part
>
> s/laptop\K/s/
>
> > of IPU7 solution. The driver was tested on Lenovo X9-14 and X9-15 laptops.
> >
> > Signed-off-by: Kate Hsuan <hpa@redhat.com>
> > ---
> >  MAINTAINERS                |    6 +
> >  drivers/media/i2c/Kconfig  |   10 +
> >  drivers/media/i2c/Makefile |    1 +
> >  drivers/media/i2c/imx471.c | 1006 ++++++++++++++++++++++++++++++++++++
> >  4 files changed, 1023 insertions(+)
> >  create mode 100644 drivers/media/i2c/imx471.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 1126fdd639ad..d597337e7c24 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -24735,6 +24735,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/imx471.c b/drivers/media/i2c/imx471.c
> > new file mode 100644
> > index 000000000000..f3c7fdce2d50
> > --- /dev/null
> > +++ b/drivers/media/i2c/imx471.c
>
> ...
>
> > +
> > +#define IMX471_NUM_SUPPLIES                  ARRAY_SIZE(imx471_supply_name)
>
> Please just use ARRAY_SIZE() where you need it.
>
> > +
> > +/* Mode : resolution and related config&values */
> > +struct imx471_mode {
> > +     /* Frame width */
> > +     u32 width;
> > +     /* Frame height */
> > +     u32 height;
> > +
> > +     /* V-timing */
> > +     u32 fll_def;
> > +     u32 fll_min;
> > +
> > +     /* H-timing */
> > +     u32 llp;
> > +
> > +     /* index of link frequency */
> > +     u32 link_freq_index;
> > +
> > +     /* Default register values */
> > +     const struct cci_reg_sequence *default_mode_regs;
> > +     const int default_mode_regs_length;
>
> unsigned int?
>
> const-ness here doesn't really matter as the entire struct is const.

unsigned int and drop const :)
>
> > +};
> > +
> > +struct imx471 {
> > +     struct v4l2_subdev sd;
> > +     struct media_pad pad;
> > +
> > +     struct v4l2_ctrl_handler ctrl_handler;
> > +     /* V4L2 Controls */
> > +     struct v4l2_ctrl *link_freq;
> > +     struct v4l2_ctrl *pixel_rate;
> > +     struct v4l2_ctrl *vblank;
> > +     struct v4l2_ctrl *hblank;
> > +     struct v4l2_ctrl *vflip;
> > +     struct v4l2_ctrl *hflip;
> > +     struct v4l2_ctrl *exposure;
>
> Do you need all these? At least link_freq remains effectively unused.
I'll tweak these ctrl based on
https://libcamera.org/sensor_driver_requirements.html.


>
> > +
> > +     struct gpio_desc *reset_gpio;
> > +     struct regulator_bulk_data supplies[IMX471_NUM_SUPPLIES];
> > +     struct clk *img_clk;
> > +
> > +     struct device *dev;
> > +     struct regmap *regmap;
> > +};
> > +
> > +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 },
> > +     { CCI_REG8(0x4431), 0x04 },
> > +     { CCI_REG8(0x531c), 0x01 },
> > +     { CCI_REG8(0x531d), 0x02 },
> > +     { CCI_REG8(0x531e), 0x04 },
> > +     { CCI_REG8(0x5928), 0x00 },
> > +     { CCI_REG8(0x5929), 0x2f },
> > +     { CCI_REG8(0x592a), 0x00 },
> > +     { CCI_REG8(0x592b), 0x85 },
> > +     { CCI_REG8(0x592c), 0x00 },
> > +     { CCI_REG8(0x592d), 0x32 },
> > +     { CCI_REG8(0x592e), 0x00 },
> > +     { CCI_REG8(0x592f), 0x88 },
> > +     { CCI_REG8(0x5930), 0x00 },
> > +     { CCI_REG8(0x5931), 0x3d },
> > +     { CCI_REG8(0x5932), 0x00 },
> > +     { CCI_REG8(0x5933), 0x93 },
> > +     { CCI_REG8(0x5938), 0x00 },
> > +     { CCI_REG8(0x5939), 0x24 },
> > +     { CCI_REG8(0x593a), 0x00 },
> > +     { CCI_REG8(0x593b), 0x7a },
> > +     { CCI_REG8(0x593c), 0x00 },
> > +     { CCI_REG8(0x593d), 0x24 },
> > +     { CCI_REG8(0x593e), 0x00 },
> > +     { CCI_REG8(0x593f), 0x7a },
> > +     { CCI_REG8(0x5940), 0x00 },
> > +     { CCI_REG8(0x5941), 0x2f },
> > +     { CCI_REG8(0x5942), 0x00 },
> > +     { CCI_REG8(0x5943), 0x85 },
> > +     { CCI_REG8(0x5f0e), 0x6e },
> > +     { CCI_REG8(0x5f11), 0xc6 },
> > +     { CCI_REG8(0x5f17), 0x5e },
> > +     { CCI_REG8(0x7990), 0x01 },
> > +     { CCI_REG8(0x7993), 0x5d },
> > +     { CCI_REG8(0x7994), 0x5d },
> > +     { CCI_REG8(0x7995), 0xa1 },
> > +     { CCI_REG8(0x799a), 0x01 },
> > +     { CCI_REG8(0x799d), 0x00 },
> > +     { CCI_REG8(0x8169), 0x01 },
> > +     { CCI_REG8(0x8359), 0x01 },
> > +     { CCI_REG8(0x9302), 0x1e },
> > +     { CCI_REG8(0x9306), 0x1f },
> > +     { CCI_REG8(0x930a), 0x26 },
> > +     { CCI_REG8(0x930e), 0x23 },
> > +     { CCI_REG8(0x9312), 0x23 },
> > +     { CCI_REG8(0x9316), 0x2c },
> > +     { CCI_REG8(0x9317), 0x19 },
> > +     { CCI_REG8(0xb046), 0x01 },
> > +     { CCI_REG8(0xb048), 0x01 },
> > +};
> > +
> > +static const struct cci_reg_sequence mode_1928x1088_regs[] = {
> > +     { CCI_REG8(0x0101), 0x00 },
> > +     { CCI_REG8(0x0112), 0x0a },
> > +     { CCI_REG8(0x0113), 0x0a },
> > +     { CCI_REG8(0x0114), 0x03 },
> > +     { CCI_REG8(0x0342), 0x0a },
> > +     { CCI_REG8(0x0343), 0x00 },
> > +     { CCI_REG8(0x0340), 0x13 },
> > +     { CCI_REG8(0x0341), 0xb0 },
> > +     { CCI_REG8(0x0344), 0x00 },
> > +     { CCI_REG8(0x0345), 0x00 },
> > +     { CCI_REG8(0x0346), 0x01 },
> > +     { CCI_REG8(0x0347), 0xbc },
> > +     { CCI_REG8(0x0348), 0x12 },
> > +     { CCI_REG8(0x0349), 0x2f },
> > +     { CCI_REG8(0x034a), 0x0b },
> > +     { CCI_REG8(0x034b), 0xeb },
> > +     { CCI_REG8(0x0381), 0x01 },
> > +     { CCI_REG8(0x0383), 0x01 },
> > +     { CCI_REG8(0x0385), 0x01 },
> > +     { CCI_REG8(0x0387), 0x01 },
> > +     { CCI_REG8(0x0900), 0x01 },
> > +     { CCI_REG8(0x0901), 0x22 },
> > +     { CCI_REG8(0x0902), 0x08 },
> > +     { CCI_REG8(0x3f4c), 0x81 },
> > +     { CCI_REG8(0x3f4d), 0x81 },
> > +     { CCI_REG8(0x0408), 0x00 },
> > +     { CCI_REG8(0x0409), 0xc8 },
> > +     { CCI_REG8(0x040a), 0x00 },
> > +     { CCI_REG8(0x040b), 0x6c },
> > +     { CCI_REG8(0x040c), 0x07 },
> > +     { CCI_REG8(0x040d), 0x88 },
> > +     { CCI_REG8(0x040e), 0x04 },
> > +     { CCI_REG8(0x040f), 0x40 },
> > +     { CCI_REG8(0x034c), 0x07 },
> > +     { CCI_REG8(0x034d), 0x88 },
> > +     { CCI_REG8(0x034e), 0x04 },
> > +     { CCI_REG8(0x034f), 0x40 },
> > +     { CCI_REG8(0x0301), 0x06 },
> > +     { CCI_REG8(0x0303), 0x02 },
> > +     { CCI_REG8(0x0305), 0x02 },
> > +     { CCI_REG8(0x0306), 0x00 },
> > +     { CCI_REG8(0x0307), 0x79 },
> > +     { CCI_REG8(0x030b), 0x01 },
> > +     { CCI_REG8(0x030d), 0x02 },
> > +     { CCI_REG8(0x030e), 0x00 },
> > +     { CCI_REG8(0x030f), 0x53 },
> > +     { CCI_REG8(0x0310), 0x01 },
> > +     { CCI_REG8(0x0202), 0x13 },
> > +     { CCI_REG8(0x0203), 0x9e },
> > +     { CCI_REG8(0x0204), 0x00 },
> > +     { CCI_REG8(0x0205), 0x00 },
> > +     { CCI_REG8(0x020e), 0x01 },
> > +     { CCI_REG8(0x020f), 0x00 },
> > +     { CCI_REG8(0x3f78), 0x01 },
> > +     { CCI_REG8(0x3f79), 0x31 },
> > +     { CCI_REG8(0x3ffe), 0x00 },
> > +     { CCI_REG8(0x3fff), 0x8a },
> > +     { CCI_REG8(0x5f0a), 0xb6 },
> > +};
> > +
> > +static const char * const imx471_test_pattern_menu[] = {
> > +     "Disabled",
> > +     "Solid Colour",
> > +     "Eight Vertical Colour Bars",
> > +     "Colour Bars With Fade to Grey",
> > +     "Pseudorandom Sequence (PN9)",
> > +};
> > +
> > +/*
> > + * When adding more than the one below, make sure the disallowed ones will
> > + * actually be disabled in the LINK_FREQ control.
> > + */
> > +static const s64 link_freq_menu_items[] = {
> > +     IMX471_LINK_FREQ_DEFAULT,
> > +};
> > +
> > +/*
> > + * The Bayer formats for the flipping.
> > + * - no flip
> > + * - h flip
> > + * - v flip
> > + * - h and v flips
> > + */
> > +static const u32 imx471_hv_flips_bayer_order[] = {
> > +     MEDIA_BUS_FMT_SRGGB10_1X10,
> > +     MEDIA_BUS_FMT_SGRBG10_1X10,
> > +     MEDIA_BUS_FMT_SGBRG10_1X10,
> > +     MEDIA_BUS_FMT_SBGGR10_1X10,
> > +};
> > +
> > +/* Mode configs */
> > +static const struct imx471_mode imx471_modes[] = {
> > +     {
> > +             .width = 1928,
> > +             .height = 1088,
> > +             .fll_def = 1308,
> > +             .fll_min = 1308,
> > +             .llp = 2328,
> > +             .link_freq_index = IMX471_LINK_FREQ_INDEX,
> > +             .default_mode_regs = mode_1928x1088_regs,
> > +             .default_mode_regs_length = ARRAY_SIZE(mode_1928x1088_regs),
> > +     },
> > +};
> > +
> > +static int imx471_get_regulators(struct device *dev, struct imx471 *sensor)
> > +{
> > +     for (unsigned int  i = 0; i < IMX471_NUM_SUPPLIES; i++)
>
> s/ \K //
ops

>
> > +             sensor->supplies[i].supply = imx471_supply_name[i];
> > +
> > +     return devm_regulator_bulk_get(dev, IMX471_NUM_SUPPLIES,
> > +                                    sensor->supplies);
> > +}
>
> ...
>
> > +/* Start streaming */
> > +static int imx471_enable_stream(struct v4l2_subdev *sd,
> > +                             struct v4l2_subdev_state *state,
> > +                             u32 pad, u64 streams_mask)
> > +{
> > +     struct imx471 *sensor = to_imx471(sd);
> > +     const struct imx471_mode *mode;
> > +     struct v4l2_mbus_framefmt *fmt;
> > +     int ret;
> > +
> > +     ret = pm_runtime_resume_and_get(sensor->dev);
> > +     if (ret)
>
> Just return the error code here -- pm_runtime_resume_and_get() won't
> increment the usage count unless it succeeds.
okay.


>
> > +             goto error_powerdown;
> > +
> > +     ret = imx471_identify_module(sensor);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /* Global Setting */
> > +     cci_multi_reg_write(sensor->regmap, imx471_global_regs,
> > +                         ARRAY_SIZE(imx471_global_regs), &ret);
> > +     if (ret) {
> > +             dev_err(sensor->dev, "failed to set global settings");
> > +             goto error_powerdown;
> > +     }
> > +
> > +     state = v4l2_subdev_get_locked_active_state(&sensor->sd);
> > +     fmt = v4l2_subdev_state_get_format(state, 0);
> > +     mode = v4l2_find_nearest_size(imx471_modes, ARRAY_SIZE(imx471_modes),
> > +                                   width, height, fmt->width, fmt->height);
> > +
> > +     /* Apply default values of current mode */
> > +     cci_multi_reg_write(sensor->regmap, mode->default_mode_regs,
> > +                         mode->default_mode_regs_length, &ret);
> > +     if (ret) {
> > +             dev_err(sensor->dev, "failed to set mode");
> > +             goto error_powerdown;
> > +     }
> > +
> > +     /* set digital gain control to all color mode */
> > +     cci_write(sensor->regmap, IMX471_REG_DPGA_USE_GLOBAL_GAIN, 1, &ret);
> > +     if (ret)
> > +             goto error_powerdown;
> > +
> > +     /* Apply customized values from user */
> > +     ret =  __v4l2_ctrl_handler_setup(&sensor->ctrl_handler);
> > +     if (ret)
> > +             goto error_powerdown;
> > +
> > +     cci_write(sensor->regmap, IMX471_REG_MODE_SELECT,
> > +               IMX471_MODE_STREAMING, &ret);
> > +     if (ret)
> > +             goto error_powerdown;
> > +
> > +     __v4l2_ctrl_grab(sensor->vflip, true);
> > +     __v4l2_ctrl_grab(sensor->hflip, true);
> > +
> > +     return ret;
> > +
> > +error_powerdown:
> > +     pm_runtime_put(sensor->dev);
> > +
> > +     return ret;
> > +}
> > +
> > +/* Stop streaming */
> > +static int imx471_disable_stream(struct v4l2_subdev *sd,
> > +                              struct v4l2_subdev_state *state,
> > +                              u32 pad, u64 streams_mask)
> > +{
> > +     struct imx471 *sensor = to_imx471(sd);
> > +     int ret;
> > +
> > +     cci_write(sensor->regmap, IMX471_REG_MODE_SELECT,
> > +               IMX471_MODE_STANDBY, &ret);
> > +     pm_runtime_put(sensor->dev);
> > +
> > +     if (ret)
> > +             dev_err(sensor->dev,
> > +                     "failed to disable stream with return value: %d\n",
> > +                     ret);
> > +     __v4l2_ctrl_grab(sensor->vflip, false);
> > +     __v4l2_ctrl_grab(sensor->hflip, false);
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct v4l2_subdev_core_ops imx471_subdev_core_ops = {
> > +     .subscribe_event = v4l2_ctrl_subdev_subscribe_event,
> > +     .unsubscribe_event = v4l2_event_subdev_unsubscribe,
> > +};
> > +
> > +static const struct v4l2_subdev_video_ops imx471_video_ops = {
> > +     .s_stream = v4l2_subdev_s_stream_helper,
> > +};
> > +
> > +static const struct v4l2_subdev_pad_ops imx471_pad_ops = {
> > +     .enum_mbus_code = imx471_enum_mbus_code,
> > +     .get_fmt = v4l2_subdev_get_fmt,
> > +     .set_fmt = imx471_set_pad_format,
> > +     .get_selection = imx471_get_selection,
> > +     .enum_frame_size = imx471_enum_frame_size,
> > +     .enable_streams = imx471_enable_stream,
> > +     .disable_streams = imx471_disable_stream,
> > +};
> > +
> > +static const struct v4l2_subdev_ops imx471_subdev_ops = {
> > +     .core = &imx471_subdev_core_ops,
> > +     .video = &imx471_video_ops,
> > +     .pad = &imx471_pad_ops,
> > +};
> > +
> > +static const struct v4l2_subdev_internal_ops imx471_internal_ops = {
> > +     .init_state = imx471_init_state,
> > +};
> > +
> > +/* Initialize control handlers */
> > +static int imx471_init_controls(struct imx471 *sensor)
> > +{
> > +     const struct imx471_mode *mode = &imx471_modes[0];
> > +     struct v4l2_ctrl_handler *ctrl_hdlr;
> > +     struct v4l2_fwnode_device_properties props;
> > +     s64 exposure_max, hblank;
> > +     u64 pixel_rate;
> > +     int ret;
> > +
> > +     ctrl_hdlr = &sensor->ctrl_handler;
> > +     ret = v4l2_ctrl_handler_init(ctrl_hdlr, 10);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = v4l2_fwnode_device_parse(sensor->dev, &props);
> > +     if (ret) {
> > +             dev_err(sensor->dev, "failed to parse fwnode: %d", ret);
> > +             return ret;
> > +     }
> > +
> > +     v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &imx471_ctrl_ops, &props);
> > +
> > +     sensor->link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr,
> > +                                                &imx471_ctrl_ops,
> > +                                                V4L2_CID_LINK_FREQ,
> > +                                                ARRAY_SIZE(link_freq_menu_items) - 1,
> > +                                                0,
> > +                                                link_freq_menu_items);
> > +     if (sensor->link_freq)
> > +             sensor->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > +
> > +     /* pixel_rate = link_freq * 2 * nr_of_lanes / bits_per_sample */
> > +     pixel_rate = IMX471_LINK_FREQ_DEFAULT * 2 * 4;
> > +     div_u64(pixel_rate, 10);
> > +     /* By default, PIXEL_RATE is read only */
> > +     sensor->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &imx471_ctrl_ops,
> > +                                            V4L2_CID_PIXEL_RATE, pixel_rate,
> > +                                            pixel_rate, 1, pixel_rate);
> > +
> > +     /* Initial vblank/hblank/exposure parameters based on current mode */
> > +     sensor->vblank = v4l2_ctrl_new_std(ctrl_hdlr,
> > +                                        &imx471_ctrl_ops,
> > +                                        V4L2_CID_VBLANK,
> > +                                        mode->fll_min - mode->height,
> > +                                        IMX471_FLL_MAX - mode->height,
> > +                                        1,
> > +                                        mode->fll_def - mode->height);
> > +
> > +     hblank = mode->llp - mode->width;
> > +     sensor->hblank = v4l2_ctrl_new_std(ctrl_hdlr, &imx471_ctrl_ops,
> > +                                        V4L2_CID_HBLANK, hblank, hblank,
> > +                                        1, hblank);
> > +     if (sensor->hblank)
> > +             sensor->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > +
> > +     /* fll >= exposure time + adjust parameter (default value is 18) */
> > +     exposure_max = mode->fll_def - IMX471_EXPOSURE_MARGIN;
> > +     sensor->exposure = v4l2_ctrl_new_std(ctrl_hdlr, &imx471_ctrl_ops,
> > +                                          V4L2_CID_EXPOSURE,
> > +                                          IMX471_EXPOSURE_MIN, exposure_max,
> > +                                          IMX471_EXPOSURE_STEP,
> > +                                          IMX471_EXPOSURE_DEFAULT);
> > +
> > +     v4l2_ctrl_new_std(ctrl_hdlr, &imx471_ctrl_ops, V4L2_CID_ANALOGUE_GAIN,
> > +                       IMX471_ANA_GAIN_MIN, IMX471_ANA_GAIN_MAX,
> > +                       IMX471_ANA_GAIN_STEP, IMX471_ANA_GAIN_DEFAULT);
> > +
> > +     /* Digital gain */
> > +     v4l2_ctrl_new_std(ctrl_hdlr, &imx471_ctrl_ops, V4L2_CID_DIGITAL_GAIN,
> > +                       IMX471_DGTL_GAIN_MIN, IMX471_DGTL_GAIN_MAX,
> > +                       IMX471_DGTL_GAIN_STEP, IMX471_DGTL_GAIN_DEFAULT);
> > +
> > +     v4l2_ctrl_new_std_menu_items(ctrl_hdlr, &imx471_ctrl_ops,
> > +                                  V4L2_CID_TEST_PATTERN,
> > +                                  ARRAY_SIZE(imx471_test_pattern_menu) - 1,
> > +                                  0, 0, imx471_test_pattern_menu);
> > +
> > +     /* HFLIP & VFLIP */
> > +     sensor->hflip = v4l2_ctrl_new_std(ctrl_hdlr, &imx471_ctrl_ops,
> > +                                       V4L2_CID_HFLIP, 0, 1, 1, 0);
> > +
> > +     sensor->vflip = v4l2_ctrl_new_std(ctrl_hdlr, &imx471_ctrl_ops,
> > +                                       V4L2_CID_VFLIP, 0, 1, 1, 0);
> > +
> > +     if (ctrl_hdlr->error) {
> > +             dev_err(sensor->dev, "%s control init failed: %d",
> > +                     __func__, ctrl_hdlr->error);
> > +             goto error;
> > +     }
> > +
> > +     sensor->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> > +     sensor->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
>
> Either set all flags where controls are created or here. I think I'd move
> them here.

Sound good. I'll move the flag settings to here.
>
> > +
> > +     sensor->sd.ctrl_handler = ctrl_hdlr;
> > +
> > +     return 0;
> > +
> > +error:
> > +     v4l2_ctrl_handler_free(ctrl_hdlr);
> > +
> > +     return ctrl_hdlr->error;
> > +}
> > +
> > +static int imx471_check_hwcfg(struct imx471 *sensor)
> > +{
> > +     struct v4l2_fwnode_endpoint bus_cfg = {
> > +             .bus_type = V4L2_MBUS_CSI2_DPHY,
> > +     };
> > +     struct fwnode_handle *ep, *fwnode = dev_fwnode(sensor->dev);
> > +     struct clk *clk;
> > +     unsigned long link_freq_bitmap;
> > +     int ret;
> > +
> > +     clk = devm_v4l2_sensor_clk_get(sensor->dev, NULL);
> > +     if (IS_ERR(clk))
> > +             return dev_err_probe(sensor->dev, PTR_ERR(clk),
> > +                                  "can't get clock frequency\n");
> > +
> > +     if (clk_get_rate(clk) != IMX471_EXT_CLK)
> > +             return dev_err_probe(sensor->dev, -EINVAL,
> > +                                  "external clock %lu is not supported\n",
> > +                                  clk_get_rate(clk));
> > +
> > +     ep = fwnode_graph_get_endpoint_by_id(fwnode, 0, 0, 0);
> > +     ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg);
> > +     fwnode_handle_put(ep);
> > +     if (ret)
> > +             return dev_err_probe(sensor->dev, ret,
> > +                                  "parsing endpoint failed");
> > +
> > +     ret = v4l2_link_freq_to_bitmap(sensor->dev, bus_cfg.link_frequencies,
> > +                                    bus_cfg.nr_of_link_frequencies,
> > +                                    link_freq_menu_items,
> > +                                    ARRAY_SIZE(link_freq_menu_items),
> > +                                    &link_freq_bitmap);
> > +
> > +     v4l2_fwnode_endpoint_free(&bus_cfg);
> > +
> > +     return ret;
> > +}
> > +
> > +static int imx471_probe(struct i2c_client *client)
> > +{
> > +     struct imx471 *sensor;
> > +     int ret;
> > +
> > +     sensor = devm_kzalloc(&client->dev, sizeof(*sensor), GFP_KERNEL);
> > +     if (!sensor)
> > +             return dev_err_probe(&client->dev, -ENOMEM,
> > +                                  "failed to allocate memory\n");
> > +
> > +     sensor->dev = &client->dev;
> > +
> > +     /* Check HW config */
> > +     ret = imx471_check_hwcfg(sensor);
> > +     if (ret)
> > +             return dev_err_probe(sensor->dev, ret,
> > +                                  "failed to check hwcfg: %d\n", ret);
> > +
> > +     ret = imx471_get_regulators(sensor->dev, sensor);
> > +     if (ret)
> > +             return dev_err_probe(sensor->dev, ret,
> > +                                  "failed to get regulators\n");
> > +
> > +     sensor->reset_gpio = devm_gpiod_get_optional(sensor->dev, "reset",
> > +                                                  GPIOD_OUT_HIGH);
> > +     if (IS_ERR(sensor->reset_gpio))
> > +             return dev_err_probe(sensor->dev, PTR_ERR(sensor->reset_gpio),
> > +                                  "failed to get reset gpio\n");
> > +
> > +     sensor->img_clk = devm_clk_get_optional(sensor->dev, NULL);
> > +     if (IS_ERR(sensor->img_clk))
> > +             return dev_err_probe(sensor->dev, PTR_ERR(sensor->img_clk),
> > +                                  "failed to get imaging clock\n");
> > +
> > +     /* Initialize subdev */
> > +     v4l2_i2c_subdev_init(&sensor->sd, client, &imx471_subdev_ops);
> > +
> > +     /* Initialize regmap */
> > +     sensor->regmap = devm_cci_regmap_init_i2c(client, 16);
> > +     if (IS_ERR(sensor->regmap))
> > +             return PTR_ERR(sensor->regmap);
> > +
> > +     ret = imx471_power_on(sensor->dev);
> > +     if (ret)
> > +             return dev_err_probe(sensor->dev, ret,
> > +                                  "failed to power on\n");
> > +
> > +     /* Check module identity */
> > +     ret = imx471_identify_module(sensor);
> > +     if (ret) {
> > +             dev_err(&client->dev, "failed to find sensor: %d", ret);
> > +             goto error_power_off;
> > +     }
> > +
> > +     ret = imx471_init_controls(sensor);
> > +     if (ret) {
> > +             dev_err(sensor->dev, "failed to init controls: %d", ret);
> > +             goto error_power_off;
> > +     }
> > +
> > +     /* Initialize subdev */
> > +     sensor->sd.internal_ops = &imx471_internal_ops;
> > +     sensor->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
> > +                         V4L2_SUBDEV_FL_HAS_EVENTS;
> > +     sensor->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> > +
> > +     /* Initialize source pad */
> > +     sensor->pad.flags = MEDIA_PAD_FL_SOURCE;
> > +     ret = media_entity_pads_init(&sensor->sd.entity, 1, &sensor->pad);
> > +     if (ret) {
> > +             dev_err(&client->dev, "failed to init entity pads: %d", ret);
> > +             goto error_v4l2_ctrl_handler_free;
> > +     }
> > +
> > +     sensor->sd.state_lock = sensor->ctrl_handler.lock;
> > +     ret = v4l2_subdev_init_finalize(&sensor->sd);
> > +     if (ret < 0) {
> > +             dev_err(&client->dev, "failed to init subdev: %d", ret);
> > +             goto error_media_entity_pm;
> > +     }
> > +
> > +     pm_runtime_set_active(sensor->dev);
> > +     pm_runtime_enable(sensor->dev);
> > +     pm_runtime_idle(sensor->dev);
>
> Move the pm_runtime_idle() call after v4l2_async_register_subdev_sensor().
> Otherwise Runtime PM may power off the sensor on error and the driver still
> calls imx471_power_off().
Ok. got it.
>
> > +
> > +     ret = v4l2_async_register_subdev_sensor(&sensor->sd);
> > +     if (ret < 0)
> > +             goto error_v4l2_subdev_cleanup;
> > +
> > +     return 0;
> > +
> > +error_v4l2_subdev_cleanup:
> > +     pm_runtime_disable(sensor->dev);
> > +     pm_runtime_set_suspended(sensor->dev);
> > +     v4l2_subdev_cleanup(&sensor->sd);
> > +
> > +error_media_entity_pm:
> > +     media_entity_cleanup(&sensor->sd.entity);
> > +
> > +error_v4l2_ctrl_handler_free:
> > +     v4l2_ctrl_handler_free(sensor->sd.ctrl_handler);
> > +
> > +error_power_off:
> > +     imx471_power_off(sensor->dev);
> > +
> > +     return ret;
> > +}
> > +
> > +static void imx471_remove(struct i2c_client *client)
> > +{
> > +     struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > +     struct imx471 *sensor = to_imx471(sd);
> > +
> > +     v4l2_async_unregister_subdev(sd);
> > +     v4l2_subdev_cleanup(sd);
> > +     media_entity_cleanup(&sd->entity);
> > +     v4l2_ctrl_handler_free(sd->ctrl_handler);
> > +
> > +     pm_runtime_disable(&client->dev);
> > +
> > +     if (!pm_runtime_status_suspended(sensor->dev)) {
> > +             imx471_power_off(sensor->dev);
> > +             pm_runtime_set_suspended(sensor->dev);
> > +     }
> > +}
> > +
> > +static DEFINE_RUNTIME_DEV_PM_OPS(imx471_pm_ops, imx471_power_off,
> > +                              imx471_power_on, NULL);
> > +
> > +static const struct acpi_device_id imx471_acpi_ids[] __maybe_unused = {
> > +     { "SONY471A" },
> > +     { /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(acpi, imx471_acpi_ids);
> > +
> > +static struct i2c_driver imx471_i2c_driver = {
> > +     .driver = {
> > +             .name = "imx471",
> > +             .acpi_match_table = ACPI_PTR(imx471_acpi_ids),
> > +             .pm = pm_sleep_ptr(&imx471_pm_ops),
> > +     },
> > +     .probe = imx471_probe,
> > +     .remove = imx471_remove,
> > +};
> > +module_i2c_driver(imx471_i2c_driver);
> > +
> > +MODULE_AUTHOR("Jimmy Su <jimmy.su@intel.com>");
> > +MODULE_AUTHOR("Serin Yeh <serin.yeh@intel.com>");
> > +MODULE_AUTHOR("Kate Hsuan <hpa@redhat.com>");
> > +MODULE_DESCRIPTION("Sony imx471 sensor driver");
> > +MODULE_LICENSE("GPL");
>
> --
> Kind regards,
>
> Sakari Ailus
>


-- 
BR,
Kate

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

[Sakari Ailus]

Hi Kate,

On Wed, May 27, 2026 at 02:14:24PM +0800, Kate Hsuan wrote:
> > > +struct imx471 {
> > > +     struct v4l2_subdev sd;
> > > +     struct media_pad pad;
> > > +
> > > +     struct v4l2_ctrl_handler ctrl_handler;
> > > +     /* V4L2 Controls */
> > > +     struct v4l2_ctrl *link_freq;
> > > +     struct v4l2_ctrl *pixel_rate;
> > > +     struct v4l2_ctrl *vblank;
> > > +     struct v4l2_ctrl *hblank;
> > > +     struct v4l2_ctrl *vflip;
> > > +     struct v4l2_ctrl *hflip;
> > > +     struct v4l2_ctrl *exposure;
> >
> > Do you need all these? At least link_freq remains effectively unused.
> I'll tweak these ctrl based on
> https://libcamera.org/sensor_driver_requirements.html.

I rather meant that you're assigning all of these fields but then not using
them. You could thus remove the fields and the assignments. But I can't say
which ones, apart from link_freq.

-- 
Kind regards,

Sakari Ailus

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

[Kate Hsuan]

Hi Sakari,

On Wed, May 27, 2026 at 7:42 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Kate,
>
> On Wed, May 27, 2026 at 02:14:24PM +0800, Kate Hsuan wrote:
> > > > +struct imx471 {
> > > > +     struct v4l2_subdev sd;
> > > > +     struct media_pad pad;
> > > > +
> > > > +     struct v4l2_ctrl_handler ctrl_handler;
> > > > +     /* V4L2 Controls */
> > > > +     struct v4l2_ctrl *link_freq;
> > > > +     struct v4l2_ctrl *pixel_rate;
> > > > +     struct v4l2_ctrl *vblank;
> > > > +     struct v4l2_ctrl *hblank;
> > > > +     struct v4l2_ctrl *vflip;
> > > > +     struct v4l2_ctrl *hflip;
> > > > +     struct v4l2_ctrl *exposure;
> > >
> > > Do you need all these? At least link_freq remains effectively unused.
> > I'll tweak these ctrl based on
> > https://libcamera.org/sensor_driver_requirements.html.
>
> I rather meant that you're assigning all of these fields but then not using
> them. You could thus remove the fields and the assignments. But I can't say
> which ones, apart from link_freq.

I dropped link_freq from the driver but ipu7 complains of errors
regarding link_freq, shown as follows.

[ 3628.195722] intel_ipu7_isys.isys intel_ipu7.isys.40: bind imx471
0-0010 nlanes is 4 port is 0
[ 3628.196207] intel_ipu7_isys.isys intel_ipu7.isys.40: All sensor
registration completed.
[ 3631.754550] intel_ipu7_isys.isys intel_ipu7.isys.40: get link freq
failed (-2)
[ 3631.754562] intel_ipu7_isys.isys intel_ipu7.isys.40: CSI-0 PHY
power up failed -2
[ 3631.754565] intel_ipu7_isys.isys intel_ipu7.isys.40: enable streams
Intel IPU7 CSI2 0 failed with -2

So, I'll keep link_freq. :)

>
> --
> Kind regards,
>
> Sakari Ailus
>


-- 
BR,
Kate

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

[Tarang Raval]

Hi Kate,

> On Wed, May 27, 2026 at 7:42 PM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> >
> > Hi Kate,
> >
> > On Wed, May 27, 2026 at 02:14:24PM +0800, Kate Hsuan wrote:
> > > > > +struct imx471 {
> > > > > +     struct v4l2_subdev sd;
> > > > > +     struct media_pad pad;
> > > > > +
> > > > > +     struct v4l2_ctrl_handler ctrl_handler;
> > > > > +     /* V4L2 Controls */
> > > > > +     struct v4l2_ctrl *link_freq;
> > > > > +     struct v4l2_ctrl *pixel_rate;
> > > > > +     struct v4l2_ctrl *vblank;
> > > > > +     struct v4l2_ctrl *hblank;
> > > > > +     struct v4l2_ctrl *vflip;
> > > > > +     struct v4l2_ctrl *hflip;
> > > > > +     struct v4l2_ctrl *exposure;
> > > >
> > > > Do you need all these? At least link_freq remains effectively unused.
> > > I'll tweak these ctrl based on
> > > https://libcamera.org/sensor_driver_requirements.html.
> >
> > I rather meant that you're assigning all of these fields but then not using
> > them. You could thus remove the fields and the assignments. But I can't say
> > which ones, apart from link_freq.
>
> I dropped link_freq from the driver but ipu7 complains of errors
> regarding link_freq, shown as follows.
>
> [ 3628.195722] intel_ipu7_isys.isys intel_ipu7.isys.40: bind imx471
> 0-0010 nlanes is 4 port is 0
> [ 3628.196207] intel_ipu7_isys.isys intel_ipu7.isys.40: All sensor
> registration completed.
> [ 3631.754550] intel_ipu7_isys.isys intel_ipu7.isys.40: get link freq
> failed (-2)
> [ 3631.754562] intel_ipu7_isys.isys intel_ipu7.isys.40: CSI-0 PHY
> power up failed -2
> [ 3631.754565] intel_ipu7_isys.isys intel_ipu7.isys.40: enable streams
> Intel IPU7 CSI2 0 failed with -2
>
> So, I'll keep link_freq. :)

I believe Sakari's suggestion was to remove only the struct v4l2_ctrl *link_freq member.

Since the driver does not use the control pointer later, there is no need
to store the returned pointer in struct imx471.

In other words, the suggestion was to remove the unused link_freq field, 
rather than the V4L2_CID_LINK_FREQ control itself.

Best Regards,
Tarang

[PATCH v3 3/3] media: i2c: imx471: Naming the register

[Kate Hsuan]

Name the register addresses and set up the value with correct value
length.

Signed-off-by: Kate Hsuan <hpa@redhat.com>
---
 drivers/media/i2c/imx471.c | 96 ++++++++++++++++++++------------------
 1 file changed, 50 insertions(+), 46 deletions(-)

diff --git a/drivers/media/i2c/imx471.c b/drivers/media/i2c/imx471.c
index f3c7fdce2d50..e8bc3487370f 100644
--- a/drivers/media/i2c/imx471.c
+++ b/drivers/media/i2c/imx471.c
@@ -76,12 +76,6 @@
 /* Default exposure margin */
 #define IMX471_EXPOSURE_MARGIN			18
 
-/* Horizontal crop window offset */
-#define IMX471_REG_H_WIN_OFFSET			CCI_REG8(0x0409)
-
-/* Vertical crop window offset */
-#define IMX471_REG_V_WIN_OFFSET			CCI_REG8(0x034b)
-
 /* Test Pattern Control */
 #define IMX471_REG_TEST_PATTERN			CCI_REG8(0x0600)
 #define IMX471_TEST_PATTERN_DISABLED		0
@@ -103,6 +97,32 @@
 #define IMX471_PIXEL_ARRAY_WIDTH		4656
 #define IMX471_PIXEL_ARRAY_HEIGHT		3496
 
+#define IMX471_REG_EXCK_FREQ			CCI_REG16(0x0136)
+#define IMX471_EXCK_FREQ(n)			((n) * 256)	/* n in MHz */
+
+#define IMX471_REG_CSI_DATA_FORMAT		CCI_REG16(0x0112)
+#define IMX471_CSI_DATA_FORMAT_RAW10		0x0a0a
+
+#define IMX471_REG_CSI_LANE_MODE		CCI_REG8(0x0114)
+#define IMX471_CSI_2_LANE_MODE			1
+#define IMX471_CSI_4_LANE_MODE			3
+
+#define IMX471_REG_X_ADD_STA			CCI_REG16(0x0344)
+#define IMX471_REG_Y_ADD_STA			CCI_REG16(0x0346)
+#define IMX471_REG_X_ADD_END			CCI_REG16(0x0348)
+#define IMX471_REG_Y_ADD_END			CCI_REG16(0x034a)
+#define IMX471_REG_X_OUTPUT_SIZE		CCI_REG16(0x034c)
+#define IMX471_REG_Y_OUTPUT_SIZE		CCI_REG16(0x034e)
+#define IMX471_REG_X_EVEN_INC			CCI_REG8(0x0381)
+#define IMX471_REG_X_ODD_INC			CCI_REG8(0x0383)
+#define IMX471_REG_Y_EVEN_INC			CCI_REG8(0x0385)
+#define IMX471_REG_Y_ODD_INC			CCI_REG8(0x0387)
+
+#define IMX471_REG_DIG_CROP_X_OFFSET		CCI_REG16(0x0408)
+#define IMX471_REG_DIG_CROP_Y_OFFSET		CCI_REG16(0x040a)
+#define IMX471_REG_DIG_CROP_WIDTH		CCI_REG16(0x040c)
+#define IMX471_REG_DIG_CROP_HEIGHT		CCI_REG16(0x040e)
+
 #define to_imx471(_sd) container_of_const(_sd, struct imx471, sd)
 
 static const char * const imx471_supply_name[] = {
@@ -156,8 +176,7 @@ struct imx471 {
 };
 
 static const struct cci_reg_sequence imx471_global_regs[] = {
-	{ CCI_REG8(0x0136), 0x13 },
-	{ CCI_REG8(0x0137), 0x33 },
+	{ IMX471_REG_EXCK_FREQ, IMX471_EXCK_FREQ(19.2) },
 	{ CCI_REG8(0x3c7e), 0x08 },
 	{ CCI_REG8(0x3c7f), 0x05 },
 	{ CCI_REG8(0x3e35), 0x00 },
@@ -215,43 +234,29 @@ static const struct cci_reg_sequence imx471_global_regs[] = {
 };
 
 static const struct cci_reg_sequence mode_1928x1088_regs[] = {
-	{ CCI_REG8(0x0101), 0x00 },
-	{ CCI_REG8(0x0112), 0x0a },
-	{ CCI_REG8(0x0113), 0x0a },
-	{ CCI_REG8(0x0114), 0x03 },
+	{ IMX471_REG_ORIENTATION, 0x00 },
+	{ IMX471_REG_CSI_DATA_FORMAT, IMX471_CSI_DATA_FORMAT_RAW10 },
+	{ IMX471_REG_CSI_LANE_MODE, IMX471_CSI_4_LANE_MODE },
 	{ CCI_REG8(0x0342), 0x0a },
 	{ CCI_REG8(0x0343), 0x00 },
-	{ CCI_REG8(0x0340), 0x13 },
-	{ CCI_REG8(0x0341), 0xb0 },
-	{ CCI_REG8(0x0344), 0x00 },
-	{ CCI_REG8(0x0345), 0x00 },
-	{ CCI_REG8(0x0346), 0x01 },
-	{ CCI_REG8(0x0347), 0xbc },
-	{ CCI_REG8(0x0348), 0x12 },
-	{ CCI_REG8(0x0349), 0x2f },
-	{ CCI_REG8(0x034a), 0x0b },
-	{ CCI_REG8(0x034b), 0xeb },
-	{ CCI_REG8(0x0381), 0x01 },
-	{ CCI_REG8(0x0383), 0x01 },
-	{ CCI_REG8(0x0385), 0x01 },
-	{ CCI_REG8(0x0387), 0x01 },
+	{ IMX471_REG_FLL, 0x13b0 },
+	{ IMX471_REG_X_ADD_STA, 8 },
+	{ IMX471_REG_Y_ADD_STA, 444 },
+	{ IMX471_REG_X_ADD_END, 4647 },
+	{ IMX471_REG_Y_ADD_END, 3051 },
+	{ IMX471_REG_X_EVEN_INC, 1 },
+	{ IMX471_REG_X_ODD_INC, 1 },
+	{ IMX471_REG_Y_EVEN_INC, 1 },
+	{ IMX471_REG_Y_ODD_INC, 1 },
 	{ CCI_REG8(0x0900), 0x01 },
 	{ CCI_REG8(0x0901), 0x22 },
 	{ CCI_REG8(0x0902), 0x08 },
-	{ CCI_REG8(0x3f4c), 0x81 },
-	{ CCI_REG8(0x3f4d), 0x81 },
-	{ CCI_REG8(0x0408), 0x00 },
-	{ CCI_REG8(0x0409), 0xc8 },
-	{ CCI_REG8(0x040a), 0x00 },
-	{ CCI_REG8(0x040b), 0x6c },
-	{ CCI_REG8(0x040c), 0x07 },
-	{ CCI_REG8(0x040d), 0x88 },
-	{ CCI_REG8(0x040e), 0x04 },
-	{ CCI_REG8(0x040f), 0x40 },
-	{ CCI_REG8(0x034c), 0x07 },
-	{ CCI_REG8(0x034d), 0x88 },
-	{ CCI_REG8(0x034e), 0x04 },
-	{ CCI_REG8(0x034f), 0x40 },
+	{ IMX471_REG_DIG_CROP_X_OFFSET, 208 },
+	{ IMX471_REG_DIG_CROP_Y_OFFSET, 108 },
+	{ IMX471_REG_DIG_CROP_WIDTH, 1928 },
+	{ IMX471_REG_DIG_CROP_HEIGHT, 1088 },
+	{ IMX471_REG_X_OUTPUT_SIZE, 1928 },
+	{ IMX471_REG_Y_OUTPUT_SIZE, 1088 },
 	{ CCI_REG8(0x0301), 0x06 },
 	{ CCI_REG8(0x0303), 0x02 },
 	{ CCI_REG8(0x0305), 0x02 },
@@ -262,12 +267,11 @@ static const struct cci_reg_sequence mode_1928x1088_regs[] = {
 	{ CCI_REG8(0x030e), 0x00 },
 	{ CCI_REG8(0x030f), 0x53 },
 	{ CCI_REG8(0x0310), 0x01 },
-	{ CCI_REG8(0x0202), 0x13 },
-	{ CCI_REG8(0x0203), 0x9e },
-	{ CCI_REG8(0x0204), 0x00 },
-	{ CCI_REG8(0x0205), 0x00 },
-	{ CCI_REG8(0x020e), 0x01 },
-	{ CCI_REG8(0x020f), 0x00 },
+	{ IMX471_REG_EXPOSURE, 5022 },
+	{ IMX471_REG_ANALOG_GAIN, 0 },
+	{ IMX471_REG_DIG_GAIN_GLOBAL, 256 },
+	{ CCI_REG8(0x3f4c), 0x81 },
+	{ CCI_REG8(0x3f4d), 0x81 },
 	{ CCI_REG8(0x3f78), 0x01 },
 	{ CCI_REG8(0x3f79), 0x31 },
 	{ CCI_REG8(0x3ffe), 0x00 },
-- 
2.54.0

Re: [PATCH v3 3/3] media: i2c: imx471: Naming the register

[Sakari Ailus]

Hi Kate,

On Fri, May 22, 2026 at 11:11:21AM +0800, Kate Hsuan wrote:
> Name the register addresses and set up the value with correct value
> length.
> 
> Signed-off-by: Kate Hsuan <hpa@redhat.com>

Please squash this to the 2nd patch.

-- 
Regards,

Sakari Ailus