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

[PATCH v4 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 v4:
1. Add TBE20A0 (found on Lenovo X1 Carbon G14) to the supported sensors list.
2. Revert the sensor upside-down list to v1.
3. Decrease the max analog gain to 800 to mitigate the image flickering problem.
4. Return the error value when __v4l2_ctrl_modify_range() fails.
5. Fix indentation issue in Kconfig.
6. Fix the cci error value issue.
7. Drop unnecessary comments.
8. Drop unused link_freq control variable.

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: ipu-bridge: Add Sony IMX471 for Lenovo X1 Carbon G14
  media: i2c: imx471: Add Sony IMX471 image sensor driver

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

-- 
2.54.0

[PATCH v4 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 | 32 ++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
index fc6608e33de4..9e24aaceecdf 100644
--- a/drivers/media/pci/intel/ipu-bridge.c
+++ b/drivers/media/pci/intel/ipu-bridge.c
@@ -134,6 +134,38 @@ static const struct dmi_system_id upside_down_sensor_dmi_ids[] = {
 		},
 		.driver_data = "OVTI02C1",
 	},
+	{
+		/* Lenovo X9-14 */
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
+			DMI_MATCH(DMI_BOARD_NAME, "21QA"),
+		},
+		.driver_data = "SONY471A",
+	},
+	{
+		/* Lenovo X9-14 */
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
+			DMI_MATCH(DMI_BOARD_NAME, "21QB"),
+		},
+		.driver_data = "SONY471A",
+	},
+	{
+		/* Lenovo X9-15 */
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
+			DMI_MATCH(DMI_BOARD_NAME, "21Q6"),
+		},
+		.driver_data = "SONY471A",
+	},
+	{
+		/* Lenovo X9-15 */
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
+			DMI_MATCH(DMI_BOARD_NAME, "21Q7"),
+		},
+		.driver_data = "SONY471A",
+	},
 	{} /* Terminating entry */
 };
 
-- 
2.54.0

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

[Sakari Ailus]

Hi Kate,

I know several people have given you different advices but...

On Tue, Jun 09, 2026 at 08:49:24PM +0800, 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 | 32 ++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
> index fc6608e33de4..9e24aaceecdf 100644
> --- a/drivers/media/pci/intel/ipu-bridge.c
> +++ b/drivers/media/pci/intel/ipu-bridge.c
> @@ -134,6 +134,38 @@ static const struct dmi_system_id upside_down_sensor_dmi_ids[] = {
>  		},
>  		.driver_data = "OVTI02C1",
>  	},
> +	{
> +		/* Lenovo X9-14 */
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
> +			DMI_MATCH(DMI_BOARD_NAME, "21QA"),
> +		},
> +		.driver_data = "SONY471A",
> +	},
> +	{
> +		/* Lenovo X9-14 */

How are the two X9-14's different? It'd be good to have some comment here
which model this actually is: the board name is only available (typically
at least) in DMI.

> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
> +			DMI_MATCH(DMI_BOARD_NAME, "21QB"),
> +		},
> +		.driver_data = "SONY471A",
> +	},
> +	{
> +		/* Lenovo X9-15 */
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
> +			DMI_MATCH(DMI_BOARD_NAME, "21Q6"),
> +		},
> +		.driver_data = "SONY471A",
> +	},
> +	{
> +		/* Lenovo X9-15 */
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
> +			DMI_MATCH(DMI_BOARD_NAME, "21Q7"),
> +		},
> +		.driver_data = "SONY471A",
> +	},
>  	{} /* Terminating entry */
>  };
>  

-- 
Kind regards,

Sakari Ailus

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

[Kate Hsuan]

Hi Sakari and Mark,

On Thu, Jun 11, 2026 at 5:59 AM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Kate,
>
> I know several people have given you different advices but...
>
> On Tue, Jun 09, 2026 at 08:49:24PM +0800, 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 | 32 ++++++++++++++++++++++++++++
> >  1 file changed, 32 insertions(+)
> >
> > diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
> > index fc6608e33de4..9e24aaceecdf 100644
> > --- a/drivers/media/pci/intel/ipu-bridge.c
> > +++ b/drivers/media/pci/intel/ipu-bridge.c
> > @@ -134,6 +134,38 @@ static const struct dmi_system_id upside_down_sensor_dmi_ids[] = {
> >               },
> >               .driver_data = "OVTI02C1",
> >       },
> > +     {
> > +             /* Lenovo X9-14 */
> > +             .matches = {
> > +                     DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
> > +                     DMI_MATCH(DMI_BOARD_NAME, "21QA"),
> > +             },
> > +             .driver_data = "SONY471A",
> > +     },
> > +     {
> > +             /* Lenovo X9-14 */
>
> How are the two X9-14's different? It'd be good to have some comment here
> which model this actually is: the board name is only available (typically
> at least) in DMI.

According to the datasheet of X9-14 and 15. Lenovo offers 2 screen
sizes (14-inch and 15-inch) and 2 CPU types (Core Ultra 5 and 7).
They may ship customised firmware for a specific purpose with a random
DMI_PRODUCT_VERSION.
So, using the DMI_BOARD_NAME prefix to identify the laptop SKU is
safer and easier.

I think DMI_MATCH(DMI_BOARD_NAME, "21Q") covers all X9 but I don't
know the side effect :(
They may propose a new model with a different MIPI camera or HID.

>
> > +             .matches = {
> > +                     DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
> > +                     DMI_MATCH(DMI_BOARD_NAME, "21QB"),
> > +             },
> > +             .driver_data = "SONY471A",
> > +     },
> > +     {
> > +             /* Lenovo X9-15 */
> > +             .matches = {
> > +                     DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
> > +                     DMI_MATCH(DMI_BOARD_NAME, "21Q6"),
> > +             },
> > +             .driver_data = "SONY471A",
> > +     },
> > +     {
> > +             /* Lenovo X9-15 */
> > +             .matches = {
> > +                     DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
> > +                     DMI_MATCH(DMI_BOARD_NAME, "21Q7"),
> > +             },
> > +             .driver_data = "SONY471A",
> > +     },
> >       {} /* Terminating entry */
> >  };
> >
>
> --
> Kind regards,
>
> Sakari Ailus
>


-- 
BR,
Kate

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

[Sakari Ailus]

Hi Kate,

On Thu, Jun 11, 2026 at 03:25:46PM +0800, Kate Hsuan wrote:
> Hi Sakari and Mark,
> 
> On Thu, Jun 11, 2026 at 5:59 AM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> >
> > Hi Kate,
> >
> > I know several people have given you different advices but...
> >
> > On Tue, Jun 09, 2026 at 08:49:24PM +0800, 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 | 32 ++++++++++++++++++++++++++++
> > >  1 file changed, 32 insertions(+)
> > >
> > > diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
> > > index fc6608e33de4..9e24aaceecdf 100644
> > > --- a/drivers/media/pci/intel/ipu-bridge.c
> > > +++ b/drivers/media/pci/intel/ipu-bridge.c
> > > @@ -134,6 +134,38 @@ static const struct dmi_system_id upside_down_sensor_dmi_ids[] = {
> > >               },
> > >               .driver_data = "OVTI02C1",
> > >       },
> > > +     {
> > > +             /* Lenovo X9-14 */
> > > +             .matches = {
> > > +                     DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
> > > +                     DMI_MATCH(DMI_BOARD_NAME, "21QA"),
> > > +             },
> > > +             .driver_data = "SONY471A",
> > > +     },
> > > +     {
> > > +             /* Lenovo X9-14 */
> >
> > How are the two X9-14's different? It'd be good to have some comment here
> > which model this actually is: the board name is only available (typically
> > at least) in DMI.
> 
> According to the datasheet of X9-14 and 15. Lenovo offers 2 screen
> sizes (14-inch and 15-inch) and 2 CPU types (Core Ultra 5 and 7).

Is the CPU SKU the difference here? If so, can you add that to the comment?

> They may ship customised firmware for a specific purpose with a random
> DMI_PRODUCT_VERSION.
> So, using the DMI_BOARD_NAME prefix to identify the laptop SKU is
> safer and easier.
> 
> I think DMI_MATCH(DMI_BOARD_NAME, "21Q") covers all X9 but I don't
> know the side effect :(
> They may propose a new model with a different MIPI camera or HID.

Indeed. Is the BOARD_NAME guaranteed to be unique by Lenovo? I think I'd
use DMI_EXACT_MATCH(), too.

> 
> >
> > > +             .matches = {
> > > +                     DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
> > > +                     DMI_MATCH(DMI_BOARD_NAME, "21QB"),
> > > +             },
> > > +             .driver_data = "SONY471A",
> > > +     },
> > > +     {
> > > +             /* Lenovo X9-15 */
> > > +             .matches = {
> > > +                     DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
> > > +                     DMI_MATCH(DMI_BOARD_NAME, "21Q6"),
> > > +             },
> > > +             .driver_data = "SONY471A",
> > > +     },
> > > +     {
> > > +             /* Lenovo X9-15 */
> > > +             .matches = {
> > > +                     DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
> > > +                     DMI_MATCH(DMI_BOARD_NAME, "21Q7"),
> > > +             },
> > > +             .driver_data = "SONY471A",
> > > +     },
> > >       {} /* Terminating entry */
> > >  };
> > >
> >

-- 
Kind regards,

Sakari Ailus

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

[Damjan Georgievski]

On Thu, 11 Jun 2026 at 10:24, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
>
> Hi Kate,
>
> On Thu, Jun 11, 2026 at 03:25:46PM +0800, Kate Hsuan wrote:
> > Hi Sakari and Mark,
> >
> > On Thu, Jun 11, 2026 at 5:59 AM Sakari Ailus
> > <sakari.ailus@linux.intel.com> wrote:
> > >
> > > Hi Kate,
> > >
> > > I know several people have given you different advices but...
> > >
> > > On Tue, Jun 09, 2026 at 08:49:24PM +0800, 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 | 32 ++++++++++++++++++++++++++++
> > > >  1 file changed, 32 insertions(+)
> > > >
> > > > diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
> > > > index fc6608e33de4..9e24aaceecdf 100644
> > > > --- a/drivers/media/pci/intel/ipu-bridge.c
> > > > +++ b/drivers/media/pci/intel/ipu-bridge.c
> > > > @@ -134,6 +134,38 @@ static const struct dmi_system_id upside_down_sensor_dmi_ids[] = {
> > > >               },
> > > >               .driver_data = "OVTI02C1",
> > > >       },
> > > > +     {
> > > > +             /* Lenovo X9-14 */
> > > > +             .matches = {
> > > > +                     DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
> > > > +                     DMI_MATCH(DMI_BOARD_NAME, "21QA"),
> > > > +             },
> > > > +             .driver_data = "SONY471A",
> > > > +     },
> > > > +     {
> > > > +             /* Lenovo X9-14 */
> > >
> > > How are the two X9-14's different? It'd be good to have some comment here
> > > which model this actually is: the board name is only available (typically
> > > at least) in DMI.
> >
> > According to the datasheet of X9-14 and 15. Lenovo offers 2 screen
> > sizes (14-inch and 15-inch) and 2 CPU types (Core Ultra 5 and 7).
>
> Is the CPU SKU the difference here? If so, can you add that to the comment?
>
> > They may ship customised firmware for a specific purpose with a random
> > DMI_PRODUCT_VERSION.
> > So, using the DMI_BOARD_NAME prefix to identify the laptop SKU is
> > safer and easier.
> >
> > I think DMI_MATCH(DMI_BOARD_NAME, "21Q") covers all X9 but I don't
> > know the side effect :(
> > They may propose a new model with a different MIPI camera or HID.
>
> Indeed. Is the BOARD_NAME guaranteed to be unique by Lenovo? I think I'd
> use DMI_EXACT_MATCH(), too.

The full board_name is 21QA0048RM (on mine), so 21QA is just the prefix.
The lenovo support site only requires this prefix to get you to the
support page for the laptops and they refer to it as "Type 21QA".

Is there DMI_MATCH_PREFIX, for these (if not all) Thinkpads it might
make the most sense ?


-- 
damjan

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

[Sakari Ailus]

Hi Damjan,

On Thu, Jun 11, 2026 at 11:41:19AM +0200, Damjan Georgievski wrote:
> On Thu, 11 Jun 2026 at 10:24, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
> >
> > Hi Kate,
> >
> > On Thu, Jun 11, 2026 at 03:25:46PM +0800, Kate Hsuan wrote:
> > > Hi Sakari and Mark,
> > >
> > > On Thu, Jun 11, 2026 at 5:59 AM Sakari Ailus
> > > <sakari.ailus@linux.intel.com> wrote:
> > > >
> > > > Hi Kate,
> > > >
> > > > I know several people have given you different advices but...
> > > >
> > > > On Tue, Jun 09, 2026 at 08:49:24PM +0800, 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 | 32 ++++++++++++++++++++++++++++
> > > > >  1 file changed, 32 insertions(+)
> > > > >
> > > > > diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
> > > > > index fc6608e33de4..9e24aaceecdf 100644
> > > > > --- a/drivers/media/pci/intel/ipu-bridge.c
> > > > > +++ b/drivers/media/pci/intel/ipu-bridge.c
> > > > > @@ -134,6 +134,38 @@ static const struct dmi_system_id upside_down_sensor_dmi_ids[] = {
> > > > >               },
> > > > >               .driver_data = "OVTI02C1",
> > > > >       },
> > > > > +     {
> > > > > +             /* Lenovo X9-14 */
> > > > > +             .matches = {
> > > > > +                     DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
> > > > > +                     DMI_MATCH(DMI_BOARD_NAME, "21QA"),
> > > > > +             },
> > > > > +             .driver_data = "SONY471A",
> > > > > +     },
> > > > > +     {
> > > > > +             /* Lenovo X9-14 */
> > > >
> > > > How are the two X9-14's different? It'd be good to have some comment here
> > > > which model this actually is: the board name is only available (typically
> > > > at least) in DMI.
> > >
> > > According to the datasheet of X9-14 and 15. Lenovo offers 2 screen
> > > sizes (14-inch and 15-inch) and 2 CPU types (Core Ultra 5 and 7).
> >
> > Is the CPU SKU the difference here? If so, can you add that to the comment?
> >
> > > They may ship customised firmware for a specific purpose with a random
> > > DMI_PRODUCT_VERSION.
> > > So, using the DMI_BOARD_NAME prefix to identify the laptop SKU is
> > > safer and easier.
> > >
> > > I think DMI_MATCH(DMI_BOARD_NAME, "21Q") covers all X9 but I don't
> > > know the side effect :(
> > > They may propose a new model with a different MIPI camera or HID.
> >
> > Indeed. Is the BOARD_NAME guaranteed to be unique by Lenovo? I think I'd
> > use DMI_EXACT_MATCH(), too.
> 
> The full board_name is 21QA0048RM (on mine), so 21QA is just the prefix.
> The lenovo support site only requires this prefix to get you to the
> support page for the laptops and they refer to it as "Type 21QA".
> 
> Is there DMI_MATCH_PREFIX, for these (if not all) Thinkpads it might
> make the most sense ?

DMI_MATCH() only matches for prefix whereas DMI_EXACT_MATCH() requires an
exact match.

-- 
Sakari Ailus

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

[Mark Pearson]

On Thu, Jun 11, 2026, at 8:44 AM, Sakari Ailus wrote:
> Hi Damjan,
>
> On Thu, Jun 11, 2026 at 11:41:19AM +0200, Damjan Georgievski wrote:
>> On Thu, 11 Jun 2026 at 10:24, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
>> >
>> > Hi Kate,
>> >
>> > On Thu, Jun 11, 2026 at 03:25:46PM +0800, Kate Hsuan wrote:
>> > > Hi Sakari and Mark,
>> > >
>> > > On Thu, Jun 11, 2026 at 5:59 AM Sakari Ailus
>> > > <sakari.ailus@linux.intel.com> wrote:
>> > > >
>> > > > Hi Kate,
>> > > >
>> > > > I know several people have given you different advices but...
>> > > >
>> > > > On Tue, Jun 09, 2026 at 08:49:24PM +0800, 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 | 32 ++++++++++++++++++++++++++++
>> > > > >  1 file changed, 32 insertions(+)
>> > > > >
>> > > > > diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
>> > > > > index fc6608e33de4..9e24aaceecdf 100644
>> > > > > --- a/drivers/media/pci/intel/ipu-bridge.c
>> > > > > +++ b/drivers/media/pci/intel/ipu-bridge.c
>> > > > > @@ -134,6 +134,38 @@ static const struct dmi_system_id upside_down_sensor_dmi_ids[] = {
>> > > > >               },
>> > > > >               .driver_data = "OVTI02C1",
>> > > > >       },
>> > > > > +     {
>> > > > > +             /* Lenovo X9-14 */
>> > > > > +             .matches = {
>> > > > > +                     DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
>> > > > > +                     DMI_MATCH(DMI_BOARD_NAME, "21QA"),
>> > > > > +             },
>> > > > > +             .driver_data = "SONY471A",
>> > > > > +     },
>> > > > > +     {
>> > > > > +             /* Lenovo X9-14 */
>> > > >
>> > > > How are the two X9-14's different? It'd be good to have some comment here
>> > > > which model this actually is: the board name is only available (typically
>> > > > at least) in DMI.
>> > >
>> > > According to the datasheet of X9-14 and 15. Lenovo offers 2 screen
>> > > sizes (14-inch and 15-inch) and 2 CPU types (Core Ultra 5 and 7).
>> >
>> > Is the CPU SKU the difference here? If so, can you add that to the comment?
>> >
>> > > They may ship customised firmware for a specific purpose with a random
>> > > DMI_PRODUCT_VERSION.
>> > > So, using the DMI_BOARD_NAME prefix to identify the laptop SKU is
>> > > safer and easier.
>> > >
>> > > I think DMI_MATCH(DMI_BOARD_NAME, "21Q") covers all X9 but I don't
>> > > know the side effect :(
>> > > They may propose a new model with a different MIPI camera or HID.
>> >
>> > Indeed. Is the BOARD_NAME guaranteed to be unique by Lenovo? I think I'd
>> > use DMI_EXACT_MATCH(), too.
>> 
>> The full board_name is 21QA0048RM (on mine), so 21QA is just the prefix.
>> The lenovo support site only requires this prefix to get you to the
>> support page for the laptops and they refer to it as "Type 21QA".
>> 
>> Is there DMI_MATCH_PREFIX, for these (if not all) Thinkpads it might
>> make the most sense ?
>
> DMI_MATCH() only matches for prefix whereas DMI_EXACT_MATCH() requires an
> exact match.
>
There are two BOARD_NAME's for every Thinkpad (which one your system has depends on the channel thru which the platform is sold).

The first 4 chars are the important bit - they are defined and won't change (they are used internally here). The rest of the BOARD_NAME is based on the config and can vary.
I'm not so sure on the PRODUCT_VERSION I'm afraid which is why I'm hesitant to use it, even though doing the match on two IDs is annoying. I'm trying to confirm if it's a good idea or not.

A useful trick, for released platforms, is to go to https://psref.lenovo.com and type your platform in the search bar and it will come up with the match and also list the two board codes.
So if you put in X9-14 you should see 21QA and 21QB - these are the two DMI matches you would need to do (X9-15 uses 21Q6 & 21Q7)

Don't do a match on 21Q - that will definitely match up with other platforms that won't want this change.

Hope that helps
Mark

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

[Kate Hsuan]

Hi Mark and Sakari,

On Thu, Jun 11, 2026 at 10:55 PM Mark Pearson <mpearson@squebb.ca> wrote:
>
>
> On Thu, Jun 11, 2026, at 8:44 AM, Sakari Ailus wrote:
> > Hi Damjan,
> >
> > On Thu, Jun 11, 2026 at 11:41:19AM +0200, Damjan Georgievski wrote:
> >> On Thu, 11 Jun 2026 at 10:24, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
> >> >
> >> > Hi Kate,
> >> >
> >> > On Thu, Jun 11, 2026 at 03:25:46PM +0800, Kate Hsuan wrote:
> >> > > Hi Sakari and Mark,
> >> > >
> >> > > On Thu, Jun 11, 2026 at 5:59 AM Sakari Ailus
> >> > > <sakari.ailus@linux.intel.com> wrote:
> >> > > >
> >> > > > Hi Kate,
> >> > > >
> >> > > > I know several people have given you different advices but...
> >> > > >
> >> > > > On Tue, Jun 09, 2026 at 08:49:24PM +0800, 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 | 32 ++++++++++++++++++++++++++++
> >> > > > >  1 file changed, 32 insertions(+)
> >> > > > >
> >> > > > > diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
> >> > > > > index fc6608e33de4..9e24aaceecdf 100644
> >> > > > > --- a/drivers/media/pci/intel/ipu-bridge.c
> >> > > > > +++ b/drivers/media/pci/intel/ipu-bridge.c
> >> > > > > @@ -134,6 +134,38 @@ static const struct dmi_system_id upside_down_sensor_dmi_ids[] = {
> >> > > > >               },
> >> > > > >               .driver_data = "OVTI02C1",
> >> > > > >       },
> >> > > > > +     {
> >> > > > > +             /* Lenovo X9-14 */
> >> > > > > +             .matches = {
> >> > > > > +                     DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
> >> > > > > +                     DMI_MATCH(DMI_BOARD_NAME, "21QA"),
> >> > > > > +             },
> >> > > > > +             .driver_data = "SONY471A",
> >> > > > > +     },
> >> > > > > +     {
> >> > > > > +             /* Lenovo X9-14 */
> >> > > >
> >> > > > How are the two X9-14's different? It'd be good to have some comment here
> >> > > > which model this actually is: the board name is only available (typically
> >> > > > at least) in DMI.
> >> > >
> >> > > According to the datasheet of X9-14 and 15. Lenovo offers 2 screen
> >> > > sizes (14-inch and 15-inch) and 2 CPU types (Core Ultra 5 and 7).
> >> >
> >> > Is the CPU SKU the difference here? If so, can you add that to the comment?
> >> >
> >> > > They may ship customised firmware for a specific purpose with a random
> >> > > DMI_PRODUCT_VERSION.
> >> > > So, using the DMI_BOARD_NAME prefix to identify the laptop SKU is
> >> > > safer and easier.
> >> > >
> >> > > I think DMI_MATCH(DMI_BOARD_NAME, "21Q") covers all X9 but I don't
> >> > > know the side effect :(
> >> > > They may propose a new model with a different MIPI camera or HID.
> >> >
> >> > Indeed. Is the BOARD_NAME guaranteed to be unique by Lenovo? I think I'd
> >> > use DMI_EXACT_MATCH(), too.
> >>
> >> The full board_name is 21QA0048RM (on mine), so 21QA is just the prefix.
> >> The lenovo support site only requires this prefix to get you to the
> >> support page for the laptops and they refer to it as "Type 21QA".
> >>
> >> Is there DMI_MATCH_PREFIX, for these (if not all) Thinkpads it might
> >> make the most sense ?
> >
> > DMI_MATCH() only matches for prefix whereas DMI_EXACT_MATCH() requires an
> > exact match.
> >
> There are two BOARD_NAME's for every Thinkpad (which one your system has depends on the channel thru which the platform is sold).
>
> The first 4 chars are the important bit - they are defined and won't change (they are used internally here). The rest of the BOARD_NAME is based on the config and can vary.
> I'm not so sure on the PRODUCT_VERSION I'm afraid which is why I'm hesitant to use it, even though doing the match on two IDs is annoying. I'm trying to confirm if it's a good idea or not.
>
> A useful trick, for released platforms, is to go to https://psref.lenovo.com and type your platform in the search bar and it will come up with the match and also list the two board codes.
> So if you put in X9-14 you should see 21QA and 21QB - these are the two DMI matches you would need to do (X9-15 uses 21Q6 & 21Q7)
>
> Don't do a match on 21Q - that will definitely match up with other platforms that won't want this change.

Thank you for your clarification and comment.

In summary, v1 is a better revision for now, and I'll update the
comments to describe the reasons for the implementation.

>
> Hope that helps
> Mark
>


-- 
BR,
Kate

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

[Kate Hsuan]

Hi Sakari,

On Thu, Jun 11, 2026 at 4:24 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Kate,
>
> On Thu, Jun 11, 2026 at 03:25:46PM +0800, Kate Hsuan wrote:
> > Hi Sakari and Mark,
> >
> > On Thu, Jun 11, 2026 at 5:59 AM Sakari Ailus
> > <sakari.ailus@linux.intel.com> wrote:
> > >
> > > Hi Kate,
> > >
> > > I know several people have given you different advices but...
> > >
> > > On Tue, Jun 09, 2026 at 08:49:24PM +0800, 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 | 32 ++++++++++++++++++++++++++++
> > > >  1 file changed, 32 insertions(+)
> > > >
> > > > diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
> > > > index fc6608e33de4..9e24aaceecdf 100644
> > > > --- a/drivers/media/pci/intel/ipu-bridge.c
> > > > +++ b/drivers/media/pci/intel/ipu-bridge.c
> > > > @@ -134,6 +134,38 @@ static const struct dmi_system_id upside_down_sensor_dmi_ids[] = {
> > > >               },
> > > >               .driver_data = "OVTI02C1",
> > > >       },
> > > > +     {
> > > > +             /* Lenovo X9-14 */
> > > > +             .matches = {
> > > > +                     DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
> > > > +                     DMI_MATCH(DMI_BOARD_NAME, "21QA"),
> > > > +             },
> > > > +             .driver_data = "SONY471A",
> > > > +     },
> > > > +     {
> > > > +             /* Lenovo X9-14 */
> > >
> > > How are the two X9-14's different? It'd be good to have some comment here
> > > which model this actually is: the board name is only available (typically
> > > at least) in DMI.
> >
> > According to the datasheet of X9-14 and 15. Lenovo offers 2 screen
> > sizes (14-inch and 15-inch) and 2 CPU types (Core Ultra 5 and 7).
>
> Is the CPU SKU the difference here? If so, can you add that to the comment?
I looked into the datasheet and it is hard to describe the difference
between 21QA and 21QB (X9-14) or 21Q6 and 21Q7 (X9-15).
The 21QA and 21QB cover many kinds of configurations.

>
> > They may ship customised firmware for a specific purpose with a random
> > DMI_PRODUCT_VERSION.
> > So, using the DMI_BOARD_NAME prefix to identify the laptop SKU is
> > safer and easier.
> >
> > I think DMI_MATCH(DMI_BOARD_NAME, "21Q") covers all X9 but I don't
> > know the side effect :(
> > They may propose a new model with a different MIPI camera or HID.
>
> Indeed. Is the BOARD_NAME guaranteed to be unique by Lenovo? I think I'd
> use DMI_EXACT_MATCH(), too.

The board name is a 10-character string, and the first four characters
are used to identify the SKU, called "machine type". For example, my
X9 is 21QBZCK... so DMI_MATCH() matches the first 4 characters (21QB)
to identify that it is a X9-14.

> >
> > >
> > > > +             .matches = {
> > > > +                     DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
> > > > +                     DMI_MATCH(DMI_BOARD_NAME, "21QB"),
> > > > +             },
> > > > +             .driver_data = "SONY471A",
> > > > +     },
> > > > +     {
> > > > +             /* Lenovo X9-15 */
> > > > +             .matches = {
> > > > +                     DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
> > > > +                     DMI_MATCH(DMI_BOARD_NAME, "21Q6"),
> > > > +             },
> > > > +             .driver_data = "SONY471A",
> > > > +     },
> > > > +     {
> > > > +             /* Lenovo X9-15 */
> > > > +             .matches = {
> > > > +                     DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
> > > > +                     DMI_MATCH(DMI_BOARD_NAME, "21Q7"),
> > > > +             },
> > > > +             .driver_data = "SONY471A",
> > > > +     },
> > > >       {} /* Terminating entry */
> > > >  };
> > > >
> > >
>
> --
> Kind regards,
>
> Sakari Ailus
>


-- 
BR,
Kate

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

[Sakari Ailus]

Hi Kate,

On Thu, Jun 11, 2026 at 05:42:04PM +0800, Kate Hsuan wrote:
> Hi Sakari,
> 
> On Thu, Jun 11, 2026 at 4:24 PM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> >
> > Hi Kate,
> >
> > On Thu, Jun 11, 2026 at 03:25:46PM +0800, Kate Hsuan wrote:
> > > Hi Sakari and Mark,
> > >
> > > On Thu, Jun 11, 2026 at 5:59 AM Sakari Ailus
> > > <sakari.ailus@linux.intel.com> wrote:
> > > >
> > > > Hi Kate,
> > > >
> > > > I know several people have given you different advices but...
> > > >
> > > > On Tue, Jun 09, 2026 at 08:49:24PM +0800, 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 | 32 ++++++++++++++++++++++++++++
> > > > >  1 file changed, 32 insertions(+)
> > > > >
> > > > > diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
> > > > > index fc6608e33de4..9e24aaceecdf 100644
> > > > > --- a/drivers/media/pci/intel/ipu-bridge.c
> > > > > +++ b/drivers/media/pci/intel/ipu-bridge.c
> > > > > @@ -134,6 +134,38 @@ static const struct dmi_system_id upside_down_sensor_dmi_ids[] = {
> > > > >               },
> > > > >               .driver_data = "OVTI02C1",
> > > > >       },
> > > > > +     {
> > > > > +             /* Lenovo X9-14 */
> > > > > +             .matches = {
> > > > > +                     DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
> > > > > +                     DMI_MATCH(DMI_BOARD_NAME, "21QA"),
> > > > > +             },
> > > > > +             .driver_data = "SONY471A",
> > > > > +     },
> > > > > +     {
> > > > > +             /* Lenovo X9-14 */
> > > >
> > > > How are the two X9-14's different? It'd be good to have some comment here
> > > > which model this actually is: the board name is only available (typically
> > > > at least) in DMI.
> > >
> > > According to the datasheet of X9-14 and 15. Lenovo offers 2 screen
> > > sizes (14-inch and 15-inch) and 2 CPU types (Core Ultra 5 and 7).
> >
> > Is the CPU SKU the difference here? If so, can you add that to the comment?
> I looked into the datasheet and it is hard to describe the difference
> between 21QA and 21QB (X9-14) or 21Q6 and 21Q7 (X9-15).

Ack.

> The 21QA and 21QB cover many kinds of configurations.

I think it'd be good to say that in a comment, too.

Some Lenovo models have UVC webcams, too, and often it's hard to tell which
laptop has which camera. :-(

> 
> >
> > > They may ship customised firmware for a specific purpose with a random
> > > DMI_PRODUCT_VERSION.
> > > So, using the DMI_BOARD_NAME prefix to identify the laptop SKU is
> > > safer and easier.
> > >
> > > I think DMI_MATCH(DMI_BOARD_NAME, "21Q") covers all X9 but I don't
> > > know the side effect :(
> > > They may propose a new model with a different MIPI camera or HID.
> >
> > Indeed. Is the BOARD_NAME guaranteed to be unique by Lenovo? I think I'd
> > use DMI_EXACT_MATCH(), too.
> 
> The board name is a 10-character string, and the first four characters
> are used to identify the SKU, called "machine type". For example, my
> X9 is 21QBZCK... so DMI_MATCH() matches the first 4 characters (21QB)
> to identify that it is a X9-14.

Ack.

-- 
Kind regards,

Sakari Ailus

[PATCH v4 2/3] media: ipu-bridge: Add Sony IMX471 for Lenovo X1 Carbon G14

[Kate Hsuan]

The HID for Sony IMX471 is TBE20A0 on Lenovo X1 Carbon G14.

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

diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
index 9e24aaceecdf..1bf1e4d5f4b6 100644
--- a/drivers/media/pci/intel/ipu-bridge.c
+++ b/drivers/media/pci/intel/ipu-bridge.c
@@ -97,6 +97,8 @@ static const struct ipu_sensor_config ipu_supported_sensors[] = {
 	IPU_SENSOR_CONFIG("OVTI8856", 3, 180000000, 360000000, 720000000),
 	/* Sony IMX471 */
 	IPU_SENSOR_CONFIG("SONY471A", 1, 200000000),
+	/* Sony IMX471 (found on Lenovo X1 Carbon G14) */
+	IPU_SENSOR_CONFIG("TBE20A0", 1, 200000000),
 	/* Toshiba T4KA3 */
 	IPU_SENSOR_CONFIG("XMCC0003", 1, 321468000),
 };
-- 
2.54.0

[PATCH v4 3/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 X1 Carbon G14, X9-14 and X9-15 laptops
and it is a part of IPU7 solution. The driver was tested on Lenovo X1
Carbon G14, X9-14 and X9-15 laptops.

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

diff --git a/MAINTAINERS b/MAINTAINERS
index 9ec290e38b44..f4bb6b528fa1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -24963,6 +24963,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 8f2ba4121586..e1143579ae3d 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 90b276a7417a..f5c9fa93dc47 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..4d570cf7a91d
--- /dev/null
+++ b/drivers/media/i2c/imx471.c
@@ -0,0 +1,949 @@
+// 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			1270
+
+/* Default exposure margin */
+#define IMX471_EXPOSURE_MARGIN			18
+
+/* Analog gain control */
+#define IMX471_REG_ANALOG_GAIN			CCI_REG16(0x0204)
+#define IMX471_ANA_GAIN_MIN			0
+#define IMX471_ANA_GAIN_MAX			800
+#define IMX471_ANA_GAIN_STEP			1
+#define IMX471_ANA_GAIN_DEFAULT			0
+
+/* Digital gain control */
+#define IMX471_REG_DPGA_USE_GLOBAL_GAIN		CCI_REG16(0x3ff9)
+#define IMX471_REG_DIG_GAIN_GLOBAL		CCI_REG16(0x020e)
+#define IMX471_DGTL_GAIN_MIN			256
+#define IMX471_DGTL_GAIN_MAX			4095
+#define IMX471_DGTL_GAIN_STEP			1
+#define IMX471_DGTL_GAIN_DEFAULT		256
+
+/* HFLIP and VFLIP control */
+#define IMX471_REG_ORIENTATION			CCI_REG8(0x0101)
+#define IMX471_HFLIP_BIT			BIT(0)
+#define IMX471_VFLIP_BIT			BIT(1)
+
+/* 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 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[] = {
+	"avdd",
+};
+
+struct imx471_mode {
+	u32 width;
+	u32 height;
+
+	/* V-timing */
+	u32 fll_def;
+	u32 fll_min;
+
+	/* H-timing */
+	u32 llp;
+
+	u32 link_freq_index;
+
+	const struct cci_reg_sequence *default_mode_regs;
+	unsigned int default_mode_regs_length;
+};
+
+struct imx471 {
+	struct v4l2_subdev sd;
+	struct media_pad pad;
+
+	struct v4l2_ctrl_handler ctrl_handler;
+	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[ARRAY_SIZE(imx471_supply_name)];
+	struct clk *img_clk;
+
+	struct device *dev;
+	struct regmap *regmap;
+};
+
+static const struct cci_reg_sequence imx471_global_regs[] = {
+	{ IMX471_REG_EXCK_FREQ, IMX471_EXCK_FREQ(19.2) },
+	{ 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[] = {
+	{ IMX471_REG_X_ADD_STA, 8 },
+	{ IMX471_REG_Y_ADD_STA, 408 },
+	{ 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 },
+	{ 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 },
+	{ 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 },
+	{ IMX471_REG_EXPOSURE, IMX471_EXPOSURE_DEFAULT },
+	{ CCI_REG8(0x3f4c), 0x81 },
+	{ CCI_REG8(0x3f4d), 0x81 },
+	{ 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)",
+};
+
+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,
+};
+
+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 < ARRAY_SIZE(imx471_supply_name); i++)
+		sensor->supplies[i].supply = imx471_supply_name[i];
+
+	return devm_regulator_bulk_get(dev, ARRAY_SIZE(imx471_supply_name),
+				       sensor->supplies);
+}
+
+static int imx471_set_ctrl(struct v4l2_ctrl *ctrl)
+{
+	struct imx471 *sensor = container_of_const(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;
+
+	if (ctrl->id == V4L2_CID_VBLANK) {
+		exposure_max =
+			format->height + ctrl->val - IMX471_EXPOSURE_MARGIN;
+		ret = __v4l2_ctrl_modify_range(sensor->exposure,
+					       sensor->exposure->minimum,
+					       exposure_max,
+					       sensor->exposure->step,
+					       exposure_max);
+		if (ret)
+			return ret;
+	}
+
+	/* 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:
+		ret = cci_write(sensor->regmap, IMX471_REG_ANALOG_GAIN,
+				ctrl->val, NULL);
+		break;
+	case V4L2_CID_DIGITAL_GAIN:
+		ret = cci_write(sensor->regmap, IMX471_REG_DIG_GAIN_GLOBAL,
+				ctrl->val, NULL);
+		break;
+	case V4L2_CID_EXPOSURE:
+		ret = cci_write(sensor->regmap, IMX471_REG_EXPOSURE,
+				ctrl->val, &ret);
+		break;
+	case V4L2_CID_VBLANK:
+		/* Update FLL that meets expected vertical blanking */
+		ret = cci_write(sensor->regmap, IMX471_REG_FLL,
+				format->height + ctrl->val, &ret);
+		break;
+	case V4L2_CID_TEST_PATTERN:
+		ret = cci_write(sensor->regmap, IMX471_REG_TEST_PATTERN,
+				ctrl->val, NULL);
+		break;
+	case V4L2_CID_HFLIP:
+	case V4L2_CID_VFLIP:
+		ret = cci_write(sensor->regmap, IMX471_REG_ORIENTATION,
+				sensor->hflip->val | sensor->vflip->val << 1, NULL);
+		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;
+	u64 pixel_rate;
+	int h_blank;
+	int ret;
+
+	mode = v4l2_find_nearest_size(imx471_modes,
+				      ARRAY_SIZE(imx471_modes),
+				      width, height,
+				      fmt->format.width, fmt->format.height);
+
+	imx471_update_pad_format(sensor, mode, fmt);
+
+	*v4l2_subdev_state_get_format(sd_state, fmt->pad) = fmt->format;
+
+	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY)
+		return 0;
+
+	if (media_entity_is_streaming(&sensor->sd.entity))
+		return -EBUSY;
+
+	pixel_rate = div_u64(IMX471_LINK_FREQ_DEFAULT * 2 * 4, 10);
+	ret = __v4l2_ctrl_modify_range(sensor->pixel_rate,
+				       V4L2_CID_PIXEL_RATE,
+				       pixel_rate, 1, pixel_rate);
+	if (ret)
+		return ret;
+
+	ret = __v4l2_ctrl_modify_range(sensor->vblank,
+				       mode->fll_min - mode->height,
+				       IMX471_FLL_MAX - mode->height,
+				       1,
+				       mode->fll_def - mode->height);
+	if (ret)
+		return ret;
+
+	h_blank = mode->llp - mode->width;
+	/*
+	 * Currently hblank is not changeable.
+	 * So FPS control is done only by vblank.
+	 */
+	return __v4l2_ctrl_modify_range(sensor->hblank, h_blank,
+					h_blank, 1, h_blank);
+}
+
+static int imx471_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,
+		},
+	};
+
+	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(ARRAY_SIZE(imx471_supply_name), 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(ARRAY_SIZE(imx471_supply_name), 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(ARRAY_SIZE(imx471_supply_name), 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;
+}
+
+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)
+		return ret;
+
+	ret = imx471_identify_module(sensor);
+	if (ret)
+		return ret;
+
+	ret = cci_multi_reg_write(sensor->regmap, imx471_global_regs,
+				  ARRAY_SIZE(imx471_global_regs), NULL);
+	if (ret) {
+		dev_err(sensor->dev, "failed to set global settings: %d", ret);
+		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);
+
+	ret = cci_multi_reg_write(sensor->regmap, mode->default_mode_regs,
+				  mode->default_mode_regs_length, NULL);
+	if (ret) {
+		dev_err(sensor->dev, "failed to set mode: %d", ret);
+		goto error_powerdown;
+	}
+
+	ret = cci_write(sensor->regmap, IMX471_REG_DPGA_USE_GLOBAL_GAIN, 1, NULL);
+	if (ret)
+		goto error_powerdown;
+
+	ret = __v4l2_ctrl_handler_setup(&sensor->ctrl_handler);
+	if (ret)
+		goto error_powerdown;
+
+	ret = cci_write(sensor->regmap, IMX471_REG_MODE_SELECT,
+			IMX471_MODE_STREAMING, NULL);
+	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;
+}
+
+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;
+
+	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_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 = {
+	.video = &imx471_video_ops,
+	.pad = &imx471_pad_ops,
+};
+
+static const struct v4l2_subdev_internal_ops imx471_internal_ops = {
+	.init_state = imx471_init_state,
+};
+
+static int imx471_init_controls(struct imx471 *sensor)
+{
+	const struct imx471_mode *mode = &imx471_modes[0];
+	struct v4l2_fwnode_device_properties props;
+	struct v4l2_ctrl_handler *ctrl_hdlr;
+	struct v4l2_ctrl *link_freq;
+	s64 exposure_max, hblank;
+	u64 pixel_rate;
+	int ret;
+
+	ctrl_hdlr = &sensor->ctrl_handler;
+	v4l2_ctrl_handler_init(ctrl_hdlr, 12);
+
+	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);
+
+	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);
+
+	/* pixel_rate = link_freq * 2 * nr_of_lanes / bits_per_sample */
+	pixel_rate = div_u64(IMX471_LINK_FREQ_DEFAULT * 2 * 4, 10);
+
+	sensor->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &imx471_ctrl_ops,
+					       V4L2_CID_PIXEL_RATE, pixel_rate,
+					       pixel_rate, 1, pixel_rate);
+
+	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);
+
+	/* 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);
+
+	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);
+
+	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;
+	}
+
+	link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+	sensor->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+	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);
+	unsigned long link_freq_bitmap;
+	struct clk *clk;
+	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;
+
+	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");
+
+	v4l2_i2c_subdev_init(&sensor->sd, client, &imx471_subdev_ops);
+
+	sensor->regmap = devm_cci_regmap_init_i2c(client, 16);
+	if (IS_ERR(sensor->regmap))
+		return dev_err_probe(sensor->dev, PTR_ERR(sensor->regmap),
+				     "failed to initialize CCI\n");
+
+	ret = imx471_power_on(sensor->dev);
+	if (ret)
+		return dev_err_probe(sensor->dev, ret,
+				     "failed to power on\n");
+
+	ret = imx471_identify_module(sensor);
+	if (ret) {
+		dev_err_probe(sensor->dev, ret, "failed to find sensor: %d", ret);
+		goto error_power_off;
+	}
+
+	ret = imx471_init_controls(sensor);
+	if (ret) {
+		dev_err_probe(sensor->dev, ret, "failed to init controls: %d", ret);
+		goto error_power_off;
+	}
+
+	sensor->sd.internal_ops = &imx471_internal_ops;
+	sensor->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+	sensor->pad.flags = MEDIA_PAD_FL_SOURCE;
+	sensor->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
+
+	ret = media_entity_pads_init(&sensor->sd.entity, 1, &sensor->pad);
+	if (ret) {
+		dev_err_probe(sensor->dev, ret, "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_probe(sensor->dev, ret, "failed to init subdev: %d", ret);
+		goto error_media_entity_pm;
+	}
+
+	pm_runtime_set_active(sensor->dev);
+	pm_runtime_enable(sensor->dev);
+
+	ret = v4l2_async_register_subdev_sensor(&sensor->sd);
+	if (ret < 0)
+		goto error_v4l2_subdev_cleanup;
+
+	pm_runtime_idle(sensor->dev);
+
+	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);
+
+	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(&client->dev)) {
+		imx471_power_off(&client->dev);
+		pm_runtime_set_suspended(&client->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" },
+	{ "TBE20A0" },
+	{ /* 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 v4 3/3] media: i2c: imx471: Add Sony IMX471 image sensor driver

[Tarang Raval]

Hi Kate,

I noticed a few more issues. Could you please check the comments below?

Sorry, I missed these in my first review.

> Add a new driver for Sony imx471 camera sensor. It is based on
> Jimmy Su <jimmy.su@intel.com> implementation and the driver can be found
> in the following URL.
> https://github.com/intel/ipu6-drivers/commits/master/drivers/media/i2c/imx471.c
>
> This sensor can be found on Lenovo X1 Carbon G14, X9-14 and X9-15 laptops
> and it is a part of IPU7 solution. The driver was tested on Lenovo X1
> Carbon G14, X9-14 and X9-15 laptops.
>
> Signed-off-by: Kate Hsuan <hpa@redhat.com>

...

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

The CSI data format (0x0112) and lane mode (0x0114) registers are defined but
never programmed.

The out-of-tree driver mentioned in the cover letter configures these registers
during initialization. Shouldn't the same settings be applied here as part of
the global register configuration?

...

> +static const char * const imx471_supply_name[] = {
> +       "avdd",
> +};

Only avdd is defined as a regulator supply.

According to the datasheet, are there any additional power rails
(e.g. dvdd or iovdd) required by the sensor?


...

> +       { CCI_REG8(0x0307), 0x79 },
> +       { CCI_REG8(0x030b), 0x01 },
> +       { CCI_REG8(0x030d), 0x02 },
> +       { CCI_REG8(0x030e), 0x00 },
> +       { CCI_REG8(0x030f), 0x53 },
> +       { CCI_REG8(0x0310), 0x01 },
> +       { IMX471_REG_EXPOSURE, IMX471_EXPOSURE_DEFAULT },

drop this setting above.

> +       { CCI_REG8(0x3f4c), 0x81 },
> +       { CCI_REG8(0x3f4d), 0x81 },
> +       { CCI_REG8(0x3f78), 0x01 },
> +       { CCI_REG8(0x3f79), 0x31 },
> +       { CCI_REG8(0x3ffe), 0x00 },
> +       { CCI_REG8(0x3fff), 0x8a },
> +       { CCI_REG8(0x5f0a), 0xb6 },
> +};

...

> +static int imx471_set_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +       struct imx471 *sensor = container_of_const(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;
> +
> +       if (ctrl->id == V4L2_CID_VBLANK) {
> +               exposure_max =
> +                       format->height + ctrl->val - IMX471_EXPOSURE_MARGIN;
> +               ret = __v4l2_ctrl_modify_range(sensor->exposure,
> +                                              sensor->exposure->minimum,
> +                                              exposure_max,
> +                                              sensor->exposure->step,
> +                                              exposure_max);
> +               if (ret)
> +                       return ret;
> +       }
> +
> +       /* V4L2 controls values will be applied only when power is already up */
> +       if (!pm_runtime_get_if_in_use(sensor->dev))

Use pm_runtime_get_if_active() or update the comment. With pm_runtime_get_if_in_use(),
the comment should say "applied only when the device is in use".

> +               return 0;
> +
> +       switch (ctrl->id) {
> +       case V4L2_CID_ANALOGUE_GAIN:
> +               ret = cci_write(sensor->regmap, IMX471_REG_ANALOG_GAIN,
> +                               ctrl->val, NULL);
> +               break;
> +       case V4L2_CID_DIGITAL_GAIN:
> +               ret = cci_write(sensor->regmap, IMX471_REG_DIG_GAIN_GLOBAL,
> +                               ctrl->val, NULL);
> +               break;
> +       case V4L2_CID_EXPOSURE:
> +               ret = cci_write(sensor->regmap, IMX471_REG_EXPOSURE,
> +                               ctrl->val, &ret);
> +               break;
> +       case V4L2_CID_VBLANK:
> +               /* Update FLL that meets expected vertical blanking */
> +               ret = cci_write(sensor->regmap, IMX471_REG_FLL,
> +                               format->height + ctrl->val, &ret);
> +               break;
> +       case V4L2_CID_TEST_PATTERN:
> +               ret = cci_write(sensor->regmap, IMX471_REG_TEST_PATTERN,
> +                               ctrl->val, NULL);
> +               break;
> +       case V4L2_CID_HFLIP:
> +       case V4L2_CID_VFLIP:
> +               ret = cci_write(sensor->regmap, IMX471_REG_ORIENTATION,
> +                               sensor->hflip->val | sensor->vflip->val << 1, NULL);
> +               break;
> +       default:
> +               ret = -EINVAL;
> +               dev_info(sensor->dev, "ctrl(id:0x%x,val:0x%x) is not handled",

Use dev_err.

> +                        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;
> +       u64 pixel_rate;
> +       int h_blank;
> +       int ret;

int h_blank, ret;

> +
> +       mode = v4l2_find_nearest_size(imx471_modes,
> +                                     ARRAY_SIZE(imx471_modes),
> +                                     width, height,
> +                                     fmt->format.width, fmt->format.height);
> +
> +       imx471_update_pad_format(sensor, mode, fmt);
> +
> +       *v4l2_subdev_state_get_format(sd_state, fmt->pad) = fmt->format;
> +
> +       if (fmt->which == V4L2_SUBDEV_FORMAT_TRY)
> +               return 0;
> +
> +       if (media_entity_is_streaming(&sensor->sd.entity))
> +               return -EBUSY;
> +
> +       pixel_rate = div_u64(IMX471_LINK_FREQ_DEFAULT * 2 * 4, 10);
> +       ret = __v4l2_ctrl_modify_range(sensor->pixel_rate,
> +                                      V4L2_CID_PIXEL_RATE,
> +                                      pixel_rate, 1, pixel_rate);
> +       if (ret)
> +               return ret;

Why do we need this? You have a fixed pixel_rate. Drop the settings above.

> +
> +       ret = __v4l2_ctrl_modify_range(sensor->vblank,
> +                                      mode->fll_min - mode->height,
> +                                      IMX471_FLL_MAX - mode->height,
> +                                      1,
> +                                      mode->fll_def - mode->height);
> +       if (ret)
> +               return ret;
> +
> +       h_blank = mode->llp - mode->width;
> +       /*
> +        * Currently hblank is not changeable.
> +        * So FPS control is done only by vblank.
> +        */
> +       return __v4l2_ctrl_modify_range(sensor->hblank, h_blank,
> +                                       h_blank, 1, h_blank);
> +}

...

> +static int imx471_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)
> +               return ret;
> +
> +       ret = imx471_identify_module(sensor);
> +       if (ret)
> +               return ret;

Runtime PM leak. Please call error_powerdown at return path.

> +
> +       ret = cci_multi_reg_write(sensor->regmap, imx471_global_regs,
> +                                 ARRAY_SIZE(imx471_global_regs), NULL);
> +       if (ret) {
> +               dev_err(sensor->dev, "failed to set global settings: %d", ret);
> +               goto error_powerdown;
> +       }
> +
> +       state = v4l2_subdev_get_locked_active_state(&sensor->sd);

Why is the active state retrieved again here? state is already passed to enable_stream()

Drop this line.

> +       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);
> +
> +       ret = cci_multi_reg_write(sensor->regmap, mode->default_mode_regs,
> +                                 mode->default_mode_regs_length, NULL);
> +       if (ret) {
> +               dev_err(sensor->dev, "failed to set mode: %d", ret);
> +               goto error_powerdown;
> +       }
> +
> +       ret = cci_write(sensor->regmap, IMX471_REG_DPGA_USE_GLOBAL_GAIN, 1, NULL);
> +       if (ret)
> +               goto error_powerdown;
> +
> +       ret = __v4l2_ctrl_handler_setup(&sensor->ctrl_handler);
> +       if (ret)
> +               goto error_powerdown;
> +
> +       ret = cci_write(sensor->regmap, IMX471_REG_MODE_SELECT,
> +                       IMX471_MODE_STREAMING, NULL);
> +       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;

...

> +static int imx471_init_controls(struct imx471 *sensor)
> +{
> +       const struct imx471_mode *mode = &imx471_modes[0];
> +       struct v4l2_fwnode_device_properties props;
> +       struct v4l2_ctrl_handler *ctrl_hdlr;
> +       struct v4l2_ctrl *link_freq;
> +       s64 exposure_max, hblank;
> +       u64 pixel_rate;
> +       int ret;
> +
> +       ctrl_hdlr = &sensor->ctrl_handler;
> +       v4l2_ctrl_handler_init(ctrl_hdlr, 12);
> +
> +       ret = v4l2_fwnode_device_parse(sensor->dev, &props);
> +       if (ret) {
> +               dev_err(sensor->dev, "failed to parse fwnode: %d", ret);
> +               return ret;

Memory leak, Use goto error here.

> +       }
> +
> +       v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &imx471_ctrl_ops, &props);
> +
> +       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);
> +
> +       /* pixel_rate = link_freq * 2 * nr_of_lanes / bits_per_sample */
> +       pixel_rate = div_u64(IMX471_LINK_FREQ_DEFAULT * 2 * 4, 10);
> +
> +       sensor->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &imx471_ctrl_ops,
> +                                              V4L2_CID_PIXEL_RATE, pixel_rate,
> +                                              pixel_rate, 1, pixel_rate);

You can drop pixel_rate from struct sensor. it is not used anywhere in the driver.

> +
> +       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);
> +
> +       /* 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);
> +
> +       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);
> +
> +       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;
> +       }
> +
> +       link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +       sensor->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +       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);
> +       unsigned long link_freq_bitmap;
> +       struct clk *clk;
> +       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);

The sensor supports both 2 and 4 CSI-2 data lanes, but this driver only
implements the 4-lane configuration. Please validate the data-lanes property
and reject anything other than 4 lanes.

> +
> +       return ret;
> +}

...

> +static const struct acpi_device_id imx471_acpi_ids[] __maybe_unused = {
> +       { "SONY471A" },
> +       { "TBE20A0" },
> +       { /* 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),

Could you please add .of_match_table as well? The driver can also be used
on DT-based systems.

> +               .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 v4 3/3] media: i2c: imx471: Add Sony IMX471 image sensor driver

[Sakari Ailus]

Hi Tarang, Kate,

On Wed, Jun 10, 2026 at 07:11:39AM +0000, Tarang Raval wrote:
> > +       /* V4L2 controls values will be applied only when power is already up */
> > +       if (!pm_runtime_get_if_in_use(sensor->dev))
> 
> Use pm_runtime_get_if_active() or update the comment. With pm_runtime_get_if_in_use(),
> the comment should say "applied only when the device is in use".

Using pm_runtime_get_if_active() would allow more sparing I²C writes, which
I understand the driver always does in enable_streams(). It's not a bug
though.

> > +static const struct acpi_device_id imx471_acpi_ids[] __maybe_unused = {
> > +       { "SONY471A" },
> > +       { "TBE20A0" },
> > +       { /* 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),
> 
> Could you please add .of_match_table as well? The driver can also be used
> on DT-based systems.

I'd leave this up to someone who needs this to work on DT. It'd require
bindings, too, and the support wouldn't be testable at this time anyway.

-- 
Regards,

Sakari Ailus

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

[Tarang Raval]

Hi Sakari,

> On Wed, Jun 10, 2026 at 07:11:39AM +0000, Tarang Raval wrote:
> > > +       /* V4L2 controls values will be applied only when power is already up */
> > > +       if (!pm_runtime_get_if_in_use(sensor->dev))
> >
> > Use pm_runtime_get_if_active() or update the comment. With pm_runtime_get_if_in_use(),
> > the comment should say "applied only when the device is in use".
>
> Using pm_runtime_get_if_active() would allow more sparing I²C writes, which
> I understand the driver always does in enable_streams(). It's not a bug
> though.

Yes, you are right. My concern is only about the comment, as it does not accurately 
describe the behavior when using pm_runtime_get_if_in_use(). That's why I said either
approach is fine, but if pm_runtime_get_if_in_use() is used, the comment should be 
updated accordingly.

Best Regards,
Tarang

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

[Sakari Ailus]

Hi Tarang,

On Thu, Jun 11, 2026 at 05:35:29AM +0000, Tarang Raval wrote:
> Hi Sakari,
> 
> > On Wed, Jun 10, 2026 at 07:11:39AM +0000, Tarang Raval wrote:
> > > > +       /* V4L2 controls values will be applied only when power is already up */
> > > > +       if (!pm_runtime_get_if_in_use(sensor->dev))
> > >
> > > Use pm_runtime_get_if_active() or update the comment. With pm_runtime_get_if_in_use(),
> > > the comment should say "applied only when the device is in use".
> >
> > Using pm_runtime_get_if_active() would allow more sparing I²C writes, which
> > I understand the driver always does in enable_streams(). It's not a bug
> > though.
> 
> Yes, you are right. My concern is only about the comment, as it does not accurately 
> describe the behavior when using pm_runtime_get_if_in_use(). That's why I said either
> approach is fine, but if pm_runtime_get_if_in_use() is used, the comment should be 
> updated accordingly.

I'd just drop the comment.

Please also don't use more than ~ 75 characters per line in e-mails.

-- 
Regards,

Sakari Ailus

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

[Kate Hsuan]

Hi Tarang,

Thank you for reviewing it.

On Wed, Jun 10, 2026 at 3:11 PM Tarang Raval
<tarang.raval@siliconsignals.io> wrote:
>
> Hi Kate,
>
> I noticed a few more issues. Could you please check the comments below?
>
> Sorry, I missed these in my first review.
>
> > Add a new driver for Sony imx471 camera sensor. It is based on
> > Jimmy Su <jimmy.su@intel.com> implementation and the driver can be found
> > in the following URL.
> > https://github.com/intel/ipu6-drivers/commits/master/drivers/media/i2c/imx471.c
> >
> > This sensor can be found on Lenovo X1 Carbon G14, X9-14 and X9-15 laptops
> > and it is a part of IPU7 solution. The driver was tested on Lenovo X1
> > Carbon G14, X9-14 and X9-15 laptops.
> >
> > Signed-off-by: Kate Hsuan <hpa@redhat.com>
>
> ...
>
> > +#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
>
> The CSI data format (0x0112) and lane mode (0x0114) registers are defined but
> never programmed.
>
> The out-of-tree driver mentioned in the cover letter configures these registers
> during initialization. Shouldn't the same settings be applied here as part of
> the global register configuration?

ops. I didn't put them back in the cci_reg_sequence when I changed the
register names.
I'll check the register values.

>
> ...
>
> > +static const char * const imx471_supply_name[] = {
> > +       "avdd",
> > +};
>
> Only avdd is defined as a regulator supply.
>
> According to the datasheet, are there any additional power rails
> (e.g. dvdd or iovdd) required by the sensor?
I only found avdd on X1 Carbon G14, my X9-14 and 15. I only have this
one regulator to test and ensure it runs.
According to the datasheet, I found "VNAN, VDIG, and VIF". Should I
add them to the list?

>
>
> ...
>
> > +       { CCI_REG8(0x0307), 0x79 },
> > +       { CCI_REG8(0x030b), 0x01 },
> > +       { CCI_REG8(0x030d), 0x02 },
> > +       { CCI_REG8(0x030e), 0x00 },
> > +       { CCI_REG8(0x030f), 0x53 },
> > +       { CCI_REG8(0x0310), 0x01 },
> > +       { IMX471_REG_EXPOSURE, IMX471_EXPOSURE_DEFAULT },
>
> drop this setting above.
ok

>
> > +       { CCI_REG8(0x3f4c), 0x81 },
> > +       { CCI_REG8(0x3f4d), 0x81 },
> > +       { CCI_REG8(0x3f78), 0x01 },
> > +       { CCI_REG8(0x3f79), 0x31 },
> > +       { CCI_REG8(0x3ffe), 0x00 },
> > +       { CCI_REG8(0x3fff), 0x8a },
> > +       { CCI_REG8(0x5f0a), 0xb6 },
> > +};
>
> ...
>
> > +static int imx471_set_ctrl(struct v4l2_ctrl *ctrl)
> > +{
> > +       struct imx471 *sensor = container_of_const(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;
> > +
> > +       if (ctrl->id == V4L2_CID_VBLANK) {
> > +               exposure_max =
> > +                       format->height + ctrl->val - IMX471_EXPOSURE_MARGIN;
> > +               ret = __v4l2_ctrl_modify_range(sensor->exposure,
> > +                                              sensor->exposure->minimum,
> > +                                              exposure_max,
> > +                                              sensor->exposure->step,
> > +                                              exposure_max);
> > +               if (ret)
> > +                       return ret;
> > +       }
> > +
> > +       /* V4L2 controls values will be applied only when power is already up */
> > +       if (!pm_runtime_get_if_in_use(sensor->dev))
>
> Use pm_runtime_get_if_active() or update the comment. With pm_runtime_get_if_in_use(),
> the comment should say "applied only when the device is in use".

I'll drop the comment.

>
> > +               return 0;
> > +
> > +       switch (ctrl->id) {
> > +       case V4L2_CID_ANALOGUE_GAIN:
> > +               ret = cci_write(sensor->regmap, IMX471_REG_ANALOG_GAIN,
> > +                               ctrl->val, NULL);
> > +               break;
> > +       case V4L2_CID_DIGITAL_GAIN:
> > +               ret = cci_write(sensor->regmap, IMX471_REG_DIG_GAIN_GLOBAL,
> > +                               ctrl->val, NULL);
> > +               break;
> > +       case V4L2_CID_EXPOSURE:
> > +               ret = cci_write(sensor->regmap, IMX471_REG_EXPOSURE,
> > +                               ctrl->val, &ret);
> > +               break;
> > +       case V4L2_CID_VBLANK:
> > +               /* Update FLL that meets expected vertical blanking */
> > +               ret = cci_write(sensor->regmap, IMX471_REG_FLL,
> > +                               format->height + ctrl->val, &ret);
> > +               break;
> > +       case V4L2_CID_TEST_PATTERN:
> > +               ret = cci_write(sensor->regmap, IMX471_REG_TEST_PATTERN,
> > +                               ctrl->val, NULL);
> > +               break;
> > +       case V4L2_CID_HFLIP:
> > +       case V4L2_CID_VFLIP:
> > +               ret = cci_write(sensor->regmap, IMX471_REG_ORIENTATION,
> > +                               sensor->hflip->val | sensor->vflip->val << 1, NULL);
> > +               break;
> > +       default:
> > +               ret = -EINVAL;
> > +               dev_info(sensor->dev, "ctrl(id:0x%x,val:0x%x) is not handled",
>
> Use dev_err.
sure.

>
> > +                        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;
> > +       u64 pixel_rate;
> > +       int h_blank;
> > +       int ret;
>
> int h_blank, ret;
Okay.

>
> > +
> > +       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 = div_u64(IMX471_LINK_FREQ_DEFAULT * 2 * 4, 10);
> > +       ret = __v4l2_ctrl_modify_range(sensor->pixel_rate,
> > +                                      V4L2_CID_PIXEL_RATE,
> > +                                      pixel_rate, 1, pixel_rate);
> > +       if (ret)
> > +               return ret;
>
> Why do we need this? You have a fixed pixel_rate. Drop the settings above.

There is only one mode for this driver. This can be dropped.

>
> > +
> > +       ret = __v4l2_ctrl_modify_range(sensor->vblank,
> > +                                      mode->fll_min - mode->height,
> > +                                      IMX471_FLL_MAX - mode->height,
> > +                                      1,
> > +                                      mode->fll_def - mode->height);
> > +       if (ret)
> > +               return ret;
> > +
> > +       h_blank = mode->llp - mode->width;
> > +       /*
> > +        * Currently hblank is not changeable.
> > +        * So FPS control is done only by vblank.
> > +        */
> > +       return __v4l2_ctrl_modify_range(sensor->hblank, h_blank,
> > +                                       h_blank, 1, h_blank);
> > +}
>
> ...
>
> > +static int imx471_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)
> > +               return ret;
> > +
> > +       ret = imx471_identify_module(sensor);
> > +       if (ret)
> > +               return ret;
>
> Runtime PM leak. Please call error_powerdown at return path.
OK. I'll check the rest of the rumtime PM operations.

>
> > +
> > +       ret = cci_multi_reg_write(sensor->regmap, imx471_global_regs,
> > +                                 ARRAY_SIZE(imx471_global_regs), NULL);
> > +       if (ret) {
> > +               dev_err(sensor->dev, "failed to set global settings: %d", ret);
> > +               goto error_powerdown;
> > +       }
> > +
> > +       state = v4l2_subdev_get_locked_active_state(&sensor->sd);
>
> Why is the active state retrieved again here? state is already passed to enable_stream()
>
> Drop this line.

OK

>
> > +       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);
> > +
> > +       ret = cci_multi_reg_write(sensor->regmap, mode->default_mode_regs,
> > +                                 mode->default_mode_regs_length, NULL);
> > +       if (ret) {
> > +               dev_err(sensor->dev, "failed to set mode: %d", ret);
> > +               goto error_powerdown;
> > +       }
> > +
> > +       ret = cci_write(sensor->regmap, IMX471_REG_DPGA_USE_GLOBAL_GAIN, 1, NULL);
> > +       if (ret)
> > +               goto error_powerdown;
> > +
> > +       ret = __v4l2_ctrl_handler_setup(&sensor->ctrl_handler);
> > +       if (ret)
> > +               goto error_powerdown;
> > +
> > +       ret = cci_write(sensor->regmap, IMX471_REG_MODE_SELECT,
> > +                       IMX471_MODE_STREAMING, NULL);
> > +       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;
>
> ...
>
> > +static int imx471_init_controls(struct imx471 *sensor)
> > +{
> > +       const struct imx471_mode *mode = &imx471_modes[0];
> > +       struct v4l2_fwnode_device_properties props;
> > +       struct v4l2_ctrl_handler *ctrl_hdlr;
> > +       struct v4l2_ctrl *link_freq;
> > +       s64 exposure_max, hblank;
> > +       u64 pixel_rate;
> > +       int ret;
> > +
> > +       ctrl_hdlr = &sensor->ctrl_handler;
> > +       v4l2_ctrl_handler_init(ctrl_hdlr, 12);
> > +
> > +       ret = v4l2_fwnode_device_parse(sensor->dev, &props);
> > +       if (ret) {
> > +               dev_err(sensor->dev, "failed to parse fwnode: %d", ret);
> > +               return ret;
>
> Memory leak, Use goto error here.
I'll do it here as follows.
   v4l2_ctrl_handler_free(ctrl_hdlr);
   return ret;

>
> > +       }
> > +
> > +       v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &imx471_ctrl_ops, &props);
> > +
> > +       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);
> > +
> > +       /* pixel_rate = link_freq * 2 * nr_of_lanes / bits_per_sample */
> > +       pixel_rate = div_u64(IMX471_LINK_FREQ_DEFAULT * 2 * 4, 10);
> > +
> > +       sensor->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &imx471_ctrl_ops,
> > +                                              V4L2_CID_PIXEL_RATE, pixel_rate,
> > +                                              pixel_rate, 1, pixel_rate);
>
> You can drop pixel_rate from struct sensor. it is not used anywhere in the driver.
Ok
>
> > +
> > +       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);
> > +
> > +       /* 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);
> > +
> > +       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);
> > +
> > +       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;
> > +       }
> > +
> > +       link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > +       sensor->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > +       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);
> > +       unsigned long link_freq_bitmap;
> > +       struct clk *clk;
> > +       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);
>
> The sensor supports both 2 and 4 CSI-2 data lanes, but this driver only
> implements the 4-lane configuration. Please validate the data-lanes property
> and reject anything other than 4 lanes.

sure.

>
> > +
> > +       return ret;
> > +}
>
> ...
>
> > +static const struct acpi_device_id imx471_acpi_ids[] __maybe_unused = {
> > +       { "SONY471A" },
> > +       { "TBE20A0" },
> > +       { /* 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),
>
> Could you please add .of_match_table as well? The driver can also be used
> on DT-based systems.

I don't have the hardware to verify it. I think it is better if
someone needs it and adds it.



On Wed, Jun 10, 2026 at 3:11 PM Tarang Raval
<tarang.raval@siliconsignals.io> wrote:
>
> Hi Kate,
>
> I noticed a few more issues. Could you please check the comments below?
>
> Sorry, I missed these in my first review.
>
> > Add a new driver for Sony imx471 camera sensor. It is based on
> > Jimmy Su <jimmy.su@intel.com> implementation and the driver can be found
> > in the following URL.
> > https://github.com/intel/ipu6-drivers/commits/master/drivers/media/i2c/imx471.c
> >
> > This sensor can be found on Lenovo X1 Carbon G14, X9-14 and X9-15 laptops
> > and it is a part of IPU7 solution. The driver was tested on Lenovo X1
> > Carbon G14, X9-14 and X9-15 laptops.
> >
> > Signed-off-by: Kate Hsuan <hpa@redhat.com>
>
> ...
>
> > +#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
>
> The CSI data format (0x0112) and lane mode (0x0114) registers are defined but
> never programmed.
>
> The out-of-tree driver mentioned in the cover letter configures these registers
> during initialization. Shouldn't the same settings be applied here as part of
> the global register configuration?
>
> ...
>
> > +static const char * const imx471_supply_name[] = {
> > +       "avdd",
> > +};
>
> Only avdd is defined as a regulator supply.
>
> According to the datasheet, are there any additional power rails
> (e.g. dvdd or iovdd) required by the sensor?
>
>
> ...
>
> > +       { CCI_REG8(0x0307), 0x79 },
> > +       { CCI_REG8(0x030b), 0x01 },
> > +       { CCI_REG8(0x030d), 0x02 },
> > +       { CCI_REG8(0x030e), 0x00 },
> > +       { CCI_REG8(0x030f), 0x53 },
> > +       { CCI_REG8(0x0310), 0x01 },
> > +       { IMX471_REG_EXPOSURE, IMX471_EXPOSURE_DEFAULT },
>
> drop this setting above.
>
> > +       { CCI_REG8(0x3f4c), 0x81 },
> > +       { CCI_REG8(0x3f4d), 0x81 },
> > +       { CCI_REG8(0x3f78), 0x01 },
> > +       { CCI_REG8(0x3f79), 0x31 },
> > +       { CCI_REG8(0x3ffe), 0x00 },
> > +       { CCI_REG8(0x3fff), 0x8a },
> > +       { CCI_REG8(0x5f0a), 0xb6 },
> > +};
>
> ...
>
> > +static int imx471_set_ctrl(struct v4l2_ctrl *ctrl)
> > +{
> > +       struct imx471 *sensor = container_of_const(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;
> > +
> > +       if (ctrl->id == V4L2_CID_VBLANK) {
> > +               exposure_max =
> > +                       format->height + ctrl->val - IMX471_EXPOSURE_MARGIN;
> > +               ret = __v4l2_ctrl_modify_range(sensor->exposure,
> > +                                              sensor->exposure->minimum,
> > +                                              exposure_max,
> > +                                              sensor->exposure->step,
> > +                                              exposure_max);
> > +               if (ret)
> > +                       return ret;
> > +       }
> > +
> > +       /* V4L2 controls values will be applied only when power is already up */
> > +       if (!pm_runtime_get_if_in_use(sensor->dev))
>
> Use pm_runtime_get_if_active() or update the comment. With pm_runtime_get_if_in_use(),
> the comment should say "applied only when the device is in use".
>
> > +               return 0;
> > +
> > +       switch (ctrl->id) {
> > +       case V4L2_CID_ANALOGUE_GAIN:
> > +               ret = cci_write(sensor->regmap, IMX471_REG_ANALOG_GAIN,
> > +                               ctrl->val, NULL);
> > +               break;
> > +       case V4L2_CID_DIGITAL_GAIN:
> > +               ret = cci_write(sensor->regmap, IMX471_REG_DIG_GAIN_GLOBAL,
> > +                               ctrl->val, NULL);
> > +               break;
> > +       case V4L2_CID_EXPOSURE:
> > +               ret = cci_write(sensor->regmap, IMX471_REG_EXPOSURE,
> > +                               ctrl->val, &ret);
> > +               break;
> > +       case V4L2_CID_VBLANK:
> > +               /* Update FLL that meets expected vertical blanking */
> > +               ret = cci_write(sensor->regmap, IMX471_REG_FLL,
> > +                               format->height + ctrl->val, &ret);
> > +               break;
> > +       case V4L2_CID_TEST_PATTERN:
> > +               ret = cci_write(sensor->regmap, IMX471_REG_TEST_PATTERN,
> > +                               ctrl->val, NULL);
> > +               break;
> > +       case V4L2_CID_HFLIP:
> > +       case V4L2_CID_VFLIP:
> > +               ret = cci_write(sensor->regmap, IMX471_REG_ORIENTATION,
> > +                               sensor->hflip->val | sensor->vflip->val << 1, NULL);
> > +               break;
> > +       default:
> > +               ret = -EINVAL;
> > +               dev_info(sensor->dev, "ctrl(id:0x%x,val:0x%x) is not handled",
>
> Use dev_err.
>
> > +                        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;
> > +       u64 pixel_rate;
> > +       int h_blank;
> > +       int ret;
>
> int h_blank, ret;
>
> > +
> > +       mode = v4l2_find_nearest_size(imx471_modes,
> > +                                     ARRAY_SIZE(imx471_modes),
> > +                                     width, height,
> > +                                     fmt->format.width, fmt->format.height);
> > +
> > +       imx471_update_pad_format(sensor, mode, fmt);
> > +
> > +       *v4l2_subdev_state_get_format(sd_state, fmt->pad) = fmt->format;
> > +
> > +       if (fmt->which == V4L2_SUBDEV_FORMAT_TRY)
> > +               return 0;
> > +
> > +       if (media_entity_is_streaming(&sensor->sd.entity))
> > +               return -EBUSY;
> > +
> > +       pixel_rate = div_u64(IMX471_LINK_FREQ_DEFAULT * 2 * 4, 10);
> > +       ret = __v4l2_ctrl_modify_range(sensor->pixel_rate,
> > +                                      V4L2_CID_PIXEL_RATE,
> > +                                      pixel_rate, 1, pixel_rate);
> > +       if (ret)
> > +               return ret;
>
> Why do we need this? You have a fixed pixel_rate. Drop the settings above.
>
> > +
> > +       ret = __v4l2_ctrl_modify_range(sensor->vblank,
> > +                                      mode->fll_min - mode->height,
> > +                                      IMX471_FLL_MAX - mode->height,
> > +                                      1,
> > +                                      mode->fll_def - mode->height);
> > +       if (ret)
> > +               return ret;
> > +
> > +       h_blank = mode->llp - mode->width;
> > +       /*
> > +        * Currently hblank is not changeable.
> > +        * So FPS control is done only by vblank.
> > +        */
> > +       return __v4l2_ctrl_modify_range(sensor->hblank, h_blank,
> > +                                       h_blank, 1, h_blank);
> > +}
>
> ...
>
> > +static int imx471_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)
> > +               return ret;
> > +
> > +       ret = imx471_identify_module(sensor);
> > +       if (ret)
> > +               return ret;
>
> Runtime PM leak. Please call error_powerdown at return path.
>
> > +
> > +       ret = cci_multi_reg_write(sensor->regmap, imx471_global_regs,
> > +                                 ARRAY_SIZE(imx471_global_regs), NULL);
> > +       if (ret) {
> > +               dev_err(sensor->dev, "failed to set global settings: %d", ret);
> > +               goto error_powerdown;
> > +       }
> > +
> > +       state = v4l2_subdev_get_locked_active_state(&sensor->sd);
>
> Why is the active state retrieved again here? state is already passed to enable_stream()
>
> Drop this line.
>
> > +       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);
> > +
> > +       ret = cci_multi_reg_write(sensor->regmap, mode->default_mode_regs,
> > +                                 mode->default_mode_regs_length, NULL);
> > +       if (ret) {
> > +               dev_err(sensor->dev, "failed to set mode: %d", ret);
> > +               goto error_powerdown;
> > +       }
> > +
> > +       ret = cci_write(sensor->regmap, IMX471_REG_DPGA_USE_GLOBAL_GAIN, 1, NULL);
> > +       if (ret)
> > +               goto error_powerdown;
> > +
> > +       ret = __v4l2_ctrl_handler_setup(&sensor->ctrl_handler);
> > +       if (ret)
> > +               goto error_powerdown;
> > +
> > +       ret = cci_write(sensor->regmap, IMX471_REG_MODE_SELECT,
> > +                       IMX471_MODE_STREAMING, NULL);
> > +       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;
>
> ...
>
> > +static int imx471_init_controls(struct imx471 *sensor)
> > +{
> > +       const struct imx471_mode *mode = &imx471_modes[0];
> > +       struct v4l2_fwnode_device_properties props;
> > +       struct v4l2_ctrl_handler *ctrl_hdlr;
> > +       struct v4l2_ctrl *link_freq;
> > +       s64 exposure_max, hblank;
> > +       u64 pixel_rate;
> > +       int ret;
> > +
> > +       ctrl_hdlr = &sensor->ctrl_handler;
> > +       v4l2_ctrl_handler_init(ctrl_hdlr, 12);
> > +
> > +       ret = v4l2_fwnode_device_parse(sensor->dev, &props);
> > +       if (ret) {
> > +               dev_err(sensor->dev, "failed to parse fwnode: %d", ret);
> > +               return ret;
>
> Memory leak, Use goto error here.
>
> > +       }
> > +
> > +       v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &imx471_ctrl_ops, &props);
> > +
> > +       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);
> > +
> > +       /* pixel_rate = link_freq * 2 * nr_of_lanes / bits_per_sample */
> > +       pixel_rate = div_u64(IMX471_LINK_FREQ_DEFAULT * 2 * 4, 10);
> > +
> > +       sensor->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &imx471_ctrl_ops,
> > +                                              V4L2_CID_PIXEL_RATE, pixel_rate,
> > +                                              pixel_rate, 1, pixel_rate);
>
> You can drop pixel_rate from struct sensor. it is not used anywhere in the driver.
>
> > +
> > +       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);
> > +
> > +       /* 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);
> > +
> > +       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);
> > +
> > +       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;
> > +       }
> > +
> > +       link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > +       sensor->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > +       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);
> > +       unsigned long link_freq_bitmap;
> > +       struct clk *clk;
> > +       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);
>
> The sensor supports both 2 and 4 CSI-2 data lanes, but this driver only
> implements the 4-lane configuration. Please validate the data-lanes property
> and reject anything other than 4 lanes.
>
> > +
> > +       return ret;
> > +}
>
> ...
>
> > +static const struct acpi_device_id imx471_acpi_ids[] __maybe_unused = {
> > +       { "SONY471A" },
> > +       { "TBE20A0" },
> > +       { /* 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),
>
> Could you please add .of_match_table as well? The driver can also be used
> on DT-based systems.
>
> > +               .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
>


--
BR,
Kate

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

[Tarang Raval]

Hi Kate,

> > > +static const char * const imx471_supply_name[] = {
> > > +       "avdd",
> > > +};
> >
> > Only avdd is defined as a regulator supply.
> >
> > According to the datasheet, are there any additional power rails
> > (e.g. dvdd or iovdd) required by the sensor?
> I only found avdd on X1 Carbon G14, my X9-14 and 15. I only have this
> one regulator to test and ensure it runs.
> According to the datasheet, I found "VNAN, VDIG, and VIF". Should I
> add them to the list?

If I'm not mistaken, avdd corresponds to the sensor's analog supply (vnan),
So it may be better to name it vnan. 
                                                                           
For the other supplies, I don't think we should add them without hardware
validation, as additional regulators can affect the power-up sequence.     

Best Regards,
Tarang

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

[Sakari Ailus]

Hi Kate,

Thanks for the update. I think we're nearly there.

Please also run

	$ ./scripts/checkpatch.pl --strict --max-line-length=80

on this.

Also see my commens below...

On Tue, Jun 09, 2026 at 08:49:26PM +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 X1 Carbon G14, X9-14 and X9-15 laptops
> and it is a part of IPU7 solution. The driver was tested on Lenovo X1
> Carbon G14, X9-14 and X9-15 laptops.
> 
> Signed-off-by: Kate Hsuan <hpa@redhat.com>
> ---
>  MAINTAINERS                |   6 +
>  drivers/media/i2c/Kconfig  |  10 +
>  drivers/media/i2c/Makefile |   1 +
>  drivers/media/i2c/imx471.c | 949 +++++++++++++++++++++++++++++++++++++
>  4 files changed, 966 insertions(+)
>  create mode 100644 drivers/media/i2c/imx471.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 9ec290e38b44..f4bb6b528fa1 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -24963,6 +24963,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 8f2ba4121586..e1143579ae3d 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 90b276a7417a..f5c9fa93dc47 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..4d570cf7a91d
> --- /dev/null
> +++ b/drivers/media/i2c/imx471.c
> @@ -0,0 +1,949 @@
> +// 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			1270
> +
> +/* Default exposure margin */
> +#define IMX471_EXPOSURE_MARGIN			18
> +
> +/* Analog gain control */
> +#define IMX471_REG_ANALOG_GAIN			CCI_REG16(0x0204)
> +#define IMX471_ANA_GAIN_MIN			0
> +#define IMX471_ANA_GAIN_MAX			800
> +#define IMX471_ANA_GAIN_STEP			1
> +#define IMX471_ANA_GAIN_DEFAULT			0
> +
> +/* Digital gain control */
> +#define IMX471_REG_DPGA_USE_GLOBAL_GAIN		CCI_REG16(0x3ff9)
> +#define IMX471_REG_DIG_GAIN_GLOBAL		CCI_REG16(0x020e)
> +#define IMX471_DGTL_GAIN_MIN			256
> +#define IMX471_DGTL_GAIN_MAX			4095
> +#define IMX471_DGTL_GAIN_STEP			1
> +#define IMX471_DGTL_GAIN_DEFAULT		256
> +
> +/* HFLIP and VFLIP control */
> +#define IMX471_REG_ORIENTATION			CCI_REG8(0x0101)
> +#define IMX471_HFLIP_BIT			BIT(0)
> +#define IMX471_VFLIP_BIT			BIT(1)
> +
> +/* 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 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[] = {
> +	"avdd",
> +};
> +
> +struct imx471_mode {
> +	u32 width;
> +	u32 height;
> +
> +	/* V-timing */
> +	u32 fll_def;
> +	u32 fll_min;
> +
> +	/* H-timing */
> +	u32 llp;
> +
> +	u32 link_freq_index;
> +
> +	const struct cci_reg_sequence *default_mode_regs;
> +	unsigned int default_mode_regs_length;
> +};
> +
> +struct imx471 {
> +	struct v4l2_subdev sd;
> +	struct media_pad pad;
> +
> +	struct v4l2_ctrl_handler ctrl_handler;
> +	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[ARRAY_SIZE(imx471_supply_name)];
> +	struct clk *img_clk;
> +
> +	struct device *dev;
> +	struct regmap *regmap;
> +};
> +
> +static const struct cci_reg_sequence imx471_global_regs[] = {
> +	{ IMX471_REG_EXCK_FREQ, IMX471_EXCK_FREQ(19.2) },
> +	{ 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[] = {
> +	{ IMX471_REG_X_ADD_STA, 8 },
> +	{ IMX471_REG_Y_ADD_STA, 408 },
> +	{ 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 },

I believe these registers have names, too.

> +	{ 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 },
> +	{ 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 },

Ditto.

> +	{ IMX471_REG_EXPOSURE, IMX471_EXPOSURE_DEFAULT },
> +	{ CCI_REG8(0x3f4c), 0x81 },
> +	{ CCI_REG8(0x3f4d), 0x81 },
> +	{ 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)",
> +};
> +
> +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,
> +};
> +
> +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 < ARRAY_SIZE(imx471_supply_name); i++)
> +		sensor->supplies[i].supply = imx471_supply_name[i];
> +
> +	return devm_regulator_bulk_get(dev, ARRAY_SIZE(imx471_supply_name),
> +				       sensor->supplies);
> +}
> +
> +static int imx471_set_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct imx471 *sensor = container_of_const(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;
> +
> +	if (ctrl->id == V4L2_CID_VBLANK) {

You can declare exposure_max here.

> +		exposure_max =
> +			format->height + ctrl->val - IMX471_EXPOSURE_MARGIN;
> +		ret = __v4l2_ctrl_modify_range(sensor->exposure,
> +					       sensor->exposure->minimum,
> +					       exposure_max,
> +					       sensor->exposure->step,
> +					       exposure_max);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	/* 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:
> +		ret = cci_write(sensor->regmap, IMX471_REG_ANALOG_GAIN,
> +				ctrl->val, NULL);
> +		break;
> +	case V4L2_CID_DIGITAL_GAIN:
> +		ret = cci_write(sensor->regmap, IMX471_REG_DIG_GAIN_GLOBAL,
> +				ctrl->val, NULL);
> +		break;
> +	case V4L2_CID_EXPOSURE:
> +		ret = cci_write(sensor->regmap, IMX471_REG_EXPOSURE,
> +				ctrl->val, &ret);
> +		break;
> +	case V4L2_CID_VBLANK:
> +		/* Update FLL that meets expected vertical blanking */
> +		ret = cci_write(sensor->regmap, IMX471_REG_FLL,
> +				format->height + ctrl->val, &ret);
> +		break;
> +	case V4L2_CID_TEST_PATTERN:
> +		ret = cci_write(sensor->regmap, IMX471_REG_TEST_PATTERN,
> +				ctrl->val, NULL);
> +		break;
> +	case V4L2_CID_HFLIP:
> +	case V4L2_CID_VFLIP:
> +		ret = cci_write(sensor->regmap, IMX471_REG_ORIENTATION,
> +				sensor->hflip->val | sensor->vflip->val << 1, NULL);
> +		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;
> +	u64 pixel_rate;
> +	int h_blank;
> +	int ret;
> +
> +	mode = v4l2_find_nearest_size(imx471_modes,
> +				      ARRAY_SIZE(imx471_modes),
> +				      width, height,
> +				      fmt->format.width, fmt->format.height);

Fits on fewer lines.

> +
> +	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 = div_u64(IMX471_LINK_FREQ_DEFAULT * 2 * 4, 10);
> +	ret = __v4l2_ctrl_modify_range(sensor->pixel_rate,
> +				       V4L2_CID_PIXEL_RATE,
> +				       pixel_rate, 1, pixel_rate);
> +	if (ret)
> +		return ret;
> +
> +	ret = __v4l2_ctrl_modify_range(sensor->vblank,
> +				       mode->fll_min - mode->height,
> +				       IMX471_FLL_MAX - mode->height,
> +				       1,
> +				       mode->fll_def - mode->height);
> +	if (ret)
> +		return ret;
> +
> +	h_blank = mode->llp - mode->width;
> +	/*
> +	 * Currently hblank is not changeable.
> +	 * So FPS control is done only by vblank.
> +	 */
> +	return __v4l2_ctrl_modify_range(sensor->hblank, h_blank,
> +					h_blank, 1, h_blank);
> +}
> +
> +static int imx471_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,
> +		},
> +	};
> +
> +	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(ARRAY_SIZE(imx471_supply_name), 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(ARRAY_SIZE(imx471_supply_name), 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(ARRAY_SIZE(imx471_supply_name), 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;
> +}
> +
> +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)
> +		return ret;
> +
> +	ret = imx471_identify_module(sensor);
> +	if (ret)
> +		return ret;
> +
> +	ret = cci_multi_reg_write(sensor->regmap, imx471_global_regs,
> +				  ARRAY_SIZE(imx471_global_regs), NULL);
> +	if (ret) {
> +		dev_err(sensor->dev, "failed to set global settings: %d", ret);
> +		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);
> +
> +	ret = cci_multi_reg_write(sensor->regmap, mode->default_mode_regs,
> +				  mode->default_mode_regs_length, NULL);
> +	if (ret) {
> +		dev_err(sensor->dev, "failed to set mode: %d", ret);
> +		goto error_powerdown;
> +	}
> +
> +	ret = cci_write(sensor->regmap, IMX471_REG_DPGA_USE_GLOBAL_GAIN, 1, NULL);
> +	if (ret)
> +		goto error_powerdown;
> +
> +	ret = __v4l2_ctrl_handler_setup(&sensor->ctrl_handler);
> +	if (ret)
> +		goto error_powerdown;
> +
> +	ret = cci_write(sensor->regmap, IMX471_REG_MODE_SELECT,
> +			IMX471_MODE_STREAMING, NULL);
> +	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;
> +}
> +
> +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;
> +
> +	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_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 = {
> +	.video = &imx471_video_ops,
> +	.pad = &imx471_pad_ops,
> +};
> +
> +static const struct v4l2_subdev_internal_ops imx471_internal_ops = {
> +	.init_state = imx471_init_state,
> +};
> +
> +static int imx471_init_controls(struct imx471 *sensor)
> +{
> +	const struct imx471_mode *mode = &imx471_modes[0];
> +	struct v4l2_fwnode_device_properties props;
> +	struct v4l2_ctrl_handler *ctrl_hdlr;
> +	struct v4l2_ctrl *link_freq;
> +	s64 exposure_max, hblank;
> +	u64 pixel_rate;
> +	int ret;
> +
> +	ctrl_hdlr = &sensor->ctrl_handler;

You can initialise this in declaration.

> +	v4l2_ctrl_handler_init(ctrl_hdlr, 12);

Initialising the handler allocates memory so pelase move under
v4l2_fwnode_device_parse() below.

> +
> +	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);
> +
> +	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);
> +
> +	/* pixel_rate = link_freq * 2 * nr_of_lanes / bits_per_sample */
> +	pixel_rate = div_u64(IMX471_LINK_FREQ_DEFAULT * 2 * 4, 10);
> +
> +	sensor->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &imx471_ctrl_ops,
> +					       V4L2_CID_PIXEL_RATE, pixel_rate,
> +					       pixel_rate, 1, pixel_rate);
> +
> +	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);
> +
> +	/* 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);
> +
> +	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);
> +
> +	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;
> +	}
> +
> +	link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +	sensor->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +	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);
> +	unsigned long link_freq_bitmap;
> +	struct clk *clk;
> +	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;
> +
> +	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");
> +
> +	v4l2_i2c_subdev_init(&sensor->sd, client, &imx471_subdev_ops);
> +
> +	sensor->regmap = devm_cci_regmap_init_i2c(client, 16);
> +	if (IS_ERR(sensor->regmap))
> +		return dev_err_probe(sensor->dev, PTR_ERR(sensor->regmap),
> +				     "failed to initialize CCI\n");
> +
> +	ret = imx471_power_on(sensor->dev);
> +	if (ret)
> +		return dev_err_probe(sensor->dev, ret,
> +				     "failed to power on\n");
> +
> +	ret = imx471_identify_module(sensor);
> +	if (ret) {
> +		dev_err_probe(sensor->dev, ret, "failed to find sensor: %d", ret);
> +		goto error_power_off;
> +	}
> +
> +	ret = imx471_init_controls(sensor);
> +	if (ret) {
> +		dev_err_probe(sensor->dev, ret, "failed to init controls: %d", ret);
> +		goto error_power_off;
> +	}
> +
> +	sensor->sd.internal_ops = &imx471_internal_ops;
> +	sensor->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> +	sensor->pad.flags = MEDIA_PAD_FL_SOURCE;
> +	sensor->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> +
> +	ret = media_entity_pads_init(&sensor->sd.entity, 1, &sensor->pad);
> +	if (ret) {
> +		dev_err_probe(sensor->dev, ret, "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_probe(sensor->dev, ret, "failed to init subdev: %d", ret);
> +		goto error_media_entity_pm;
> +	}
> +
> +	pm_runtime_set_active(sensor->dev);
> +	pm_runtime_enable(sensor->dev);
> +
> +	ret = v4l2_async_register_subdev_sensor(&sensor->sd);
> +	if (ret < 0)
> +		goto error_v4l2_subdev_cleanup;
> +
> +	pm_runtime_idle(sensor->dev);
> +
> +	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);
> +
> +	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(&client->dev)) {
> +		imx471_power_off(&client->dev);
> +		pm_runtime_set_suspended(&client->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" },
> +	{ "TBE20A0" },
> +	{ /* 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 v4 3/3] media: i2c: imx471: Add Sony IMX471 image sensor driver

[Kate Hsuan]

Hi Sakari,

Thank you for reviewing.

On Thu, Jun 11, 2026 at 6:11 AM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Kate,
>
> Thanks for the update. I think we're nearly there.
>
> Please also run
>
>         $ ./scripts/checkpatch.pl --strict --max-line-length=80

sorry
My bad. I ran checkpactch.pl without max-line-length parameter. :(

>
> on this.
>
> Also see my commens below...
>
> On Tue, Jun 09, 2026 at 08:49:26PM +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 X1 Carbon G14, X9-14 and X9-15 laptops
> > and it is a part of IPU7 solution. The driver was tested on Lenovo X1
> > Carbon G14, X9-14 and X9-15 laptops.
> >
> > Signed-off-by: Kate Hsuan <hpa@redhat.com>
> > ---
> >  MAINTAINERS                |   6 +
> >  drivers/media/i2c/Kconfig  |  10 +
> >  drivers/media/i2c/Makefile |   1 +
> >  drivers/media/i2c/imx471.c | 949 +++++++++++++++++++++++++++++++++++++
> >  4 files changed, 966 insertions(+)
> >  create mode 100644 drivers/media/i2c/imx471.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 9ec290e38b44..f4bb6b528fa1 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -24963,6 +24963,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 8f2ba4121586..e1143579ae3d 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 90b276a7417a..f5c9fa93dc47 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..4d570cf7a91d
> > --- /dev/null
> > +++ b/drivers/media/i2c/imx471.c
> > @@ -0,0 +1,949 @@
> > +// 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                      1270
> > +
> > +/* Default exposure margin */
> > +#define IMX471_EXPOSURE_MARGIN                       18
> > +
> > +/* Analog gain control */
> > +#define IMX471_REG_ANALOG_GAIN                       CCI_REG16(0x0204)
> > +#define IMX471_ANA_GAIN_MIN                  0
> > +#define IMX471_ANA_GAIN_MAX                  800
> > +#define IMX471_ANA_GAIN_STEP                 1
> > +#define IMX471_ANA_GAIN_DEFAULT                      0
> > +
> > +/* Digital gain control */
> > +#define IMX471_REG_DPGA_USE_GLOBAL_GAIN              CCI_REG16(0x3ff9)
> > +#define IMX471_REG_DIG_GAIN_GLOBAL           CCI_REG16(0x020e)
> > +#define IMX471_DGTL_GAIN_MIN                 256
> > +#define IMX471_DGTL_GAIN_MAX                 4095
> > +#define IMX471_DGTL_GAIN_STEP                        1
> > +#define IMX471_DGTL_GAIN_DEFAULT             256
> > +
> > +/* HFLIP and VFLIP control */
> > +#define IMX471_REG_ORIENTATION                       CCI_REG8(0x0101)
> > +#define IMX471_HFLIP_BIT                     BIT(0)
> > +#define IMX471_VFLIP_BIT                     BIT(1)
> > +
> > +/* 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 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[] = {
> > +     "avdd",
> > +};
> > +
> > +struct imx471_mode {
> > +     u32 width;
> > +     u32 height;
> > +
> > +     /* V-timing */
> > +     u32 fll_def;
> > +     u32 fll_min;
> > +
> > +     /* H-timing */
> > +     u32 llp;
> > +
> > +     u32 link_freq_index;
> > +
> > +     const struct cci_reg_sequence *default_mode_regs;
> > +     unsigned int default_mode_regs_length;
> > +};
> > +
> > +struct imx471 {
> > +     struct v4l2_subdev sd;
> > +     struct media_pad pad;
> > +
> > +     struct v4l2_ctrl_handler ctrl_handler;
> > +     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[ARRAY_SIZE(imx471_supply_name)];
> > +     struct clk *img_clk;
> > +
> > +     struct device *dev;
> > +     struct regmap *regmap;
> > +};
> > +
> > +static const struct cci_reg_sequence imx471_global_regs[] = {
> > +     { IMX471_REG_EXCK_FREQ, IMX471_EXCK_FREQ(19.2) },
> > +     { 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[] = {
> > +     { IMX471_REG_X_ADD_STA, 8 },
> > +     { IMX471_REG_Y_ADD_STA, 408 },
> > +     { 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 },
>
> I believe these registers have names, too.
They may be for image binning. I'll test and name the register.

>
> > +     { 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 },
> > +     { 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 },
>
> Ditto.
>
> > +     { IMX471_REG_EXPOSURE, IMX471_EXPOSURE_DEFAULT },
> > +     { CCI_REG8(0x3f4c), 0x81 },
> > +     { CCI_REG8(0x3f4d), 0x81 },
> > +     { 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)",
> > +};
> > +
> > +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,
> > +};
> > +
> > +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 < ARRAY_SIZE(imx471_supply_name); i++)
> > +             sensor->supplies[i].supply = imx471_supply_name[i];
> > +
> > +     return devm_regulator_bulk_get(dev, ARRAY_SIZE(imx471_supply_name),
> > +                                    sensor->supplies);
> > +}
> > +
> > +static int imx471_set_ctrl(struct v4l2_ctrl *ctrl)
> > +{
> > +     struct imx471 *sensor = container_of_const(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;
> > +
> > +     if (ctrl->id == V4L2_CID_VBLANK) {
>
> You can declare exposure_max here.
Ok

>
> > +             exposure_max =
> > +                     format->height + ctrl->val - IMX471_EXPOSURE_MARGIN;
> > +             ret = __v4l2_ctrl_modify_range(sensor->exposure,
> > +                                            sensor->exposure->minimum,
> > +                                            exposure_max,
> > +                                            sensor->exposure->step,
> > +                                            exposure_max);
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +
> > +     /* 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:
> > +             ret = cci_write(sensor->regmap, IMX471_REG_ANALOG_GAIN,
> > +                             ctrl->val, NULL);
> > +             break;
> > +     case V4L2_CID_DIGITAL_GAIN:
> > +             ret = cci_write(sensor->regmap, IMX471_REG_DIG_GAIN_GLOBAL,
> > +                             ctrl->val, NULL);
> > +             break;
> > +     case V4L2_CID_EXPOSURE:
> > +             ret = cci_write(sensor->regmap, IMX471_REG_EXPOSURE,
> > +                             ctrl->val, &ret);
> > +             break;
> > +     case V4L2_CID_VBLANK:
> > +             /* Update FLL that meets expected vertical blanking */
> > +             ret = cci_write(sensor->regmap, IMX471_REG_FLL,
> > +                             format->height + ctrl->val, &ret);
> > +             break;
> > +     case V4L2_CID_TEST_PATTERN:
> > +             ret = cci_write(sensor->regmap, IMX471_REG_TEST_PATTERN,
> > +                             ctrl->val, NULL);
> > +             break;
> > +     case V4L2_CID_HFLIP:
> > +     case V4L2_CID_VFLIP:
> > +             ret = cci_write(sensor->regmap, IMX471_REG_ORIENTATION,
> > +                             sensor->hflip->val | sensor->vflip->val << 1, NULL);
> > +             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;
> > +     u64 pixel_rate;
> > +     int h_blank;
> > +     int ret;
> > +
> > +     mode = v4l2_find_nearest_size(imx471_modes,
> > +                                   ARRAY_SIZE(imx471_modes),
> > +                                   width, height,
> > +                                   fmt->format.width, fmt->format.height);
>
> Fits on fewer lines.
OK

>
> > +
> > +     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 = div_u64(IMX471_LINK_FREQ_DEFAULT * 2 * 4, 10);
> > +     ret = __v4l2_ctrl_modify_range(sensor->pixel_rate,
> > +                                    V4L2_CID_PIXEL_RATE,
> > +                                    pixel_rate, 1, pixel_rate);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = __v4l2_ctrl_modify_range(sensor->vblank,
> > +                                    mode->fll_min - mode->height,
> > +                                    IMX471_FLL_MAX - mode->height,
> > +                                    1,
> > +                                    mode->fll_def - mode->height);
> > +     if (ret)
> > +             return ret;
> > +
> > +     h_blank = mode->llp - mode->width;
> > +     /*
> > +      * Currently hblank is not changeable.
> > +      * So FPS control is done only by vblank.
> > +      */
> > +     return __v4l2_ctrl_modify_range(sensor->hblank, h_blank,
> > +                                     h_blank, 1, h_blank);
> > +}
> > +
> > +static int imx471_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,
> > +             },
> > +     };
> > +
> > +     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(ARRAY_SIZE(imx471_supply_name), 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(ARRAY_SIZE(imx471_supply_name), 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(ARRAY_SIZE(imx471_supply_name), 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;
> > +}
> > +
> > +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)
> > +             return ret;
> > +
> > +     ret = imx471_identify_module(sensor);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = cci_multi_reg_write(sensor->regmap, imx471_global_regs,
> > +                               ARRAY_SIZE(imx471_global_regs), NULL);
> > +     if (ret) {
> > +             dev_err(sensor->dev, "failed to set global settings: %d", ret);
> > +             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);
> > +
> > +     ret = cci_multi_reg_write(sensor->regmap, mode->default_mode_regs,
> > +                               mode->default_mode_regs_length, NULL);
> > +     if (ret) {
> > +             dev_err(sensor->dev, "failed to set mode: %d", ret);
> > +             goto error_powerdown;
> > +     }
> > +
> > +     ret = cci_write(sensor->regmap, IMX471_REG_DPGA_USE_GLOBAL_GAIN, 1, NULL);
> > +     if (ret)
> > +             goto error_powerdown;
> > +
> > +     ret = __v4l2_ctrl_handler_setup(&sensor->ctrl_handler);
> > +     if (ret)
> > +             goto error_powerdown;
> > +
> > +     ret = cci_write(sensor->regmap, IMX471_REG_MODE_SELECT,
> > +                     IMX471_MODE_STREAMING, NULL);
> > +     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;
> > +}
> > +
> > +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;
> > +
> > +     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_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 = {
> > +     .video = &imx471_video_ops,
> > +     .pad = &imx471_pad_ops,
> > +};
> > +
> > +static const struct v4l2_subdev_internal_ops imx471_internal_ops = {
> > +     .init_state = imx471_init_state,
> > +};
> > +
> > +static int imx471_init_controls(struct imx471 *sensor)
> > +{
> > +     const struct imx471_mode *mode = &imx471_modes[0];
> > +     struct v4l2_fwnode_device_properties props;
> > +     struct v4l2_ctrl_handler *ctrl_hdlr;
> > +     struct v4l2_ctrl *link_freq;
> > +     s64 exposure_max, hblank;
> > +     u64 pixel_rate;
> > +     int ret;
> > +
> > +     ctrl_hdlr = &sensor->ctrl_handler;
>
> You can initialise this in declaration.
>
> > +     v4l2_ctrl_handler_init(ctrl_hdlr, 12);
>
> Initialising the handler allocates memory so pelase move under
> v4l2_fwnode_device_parse() below.

OK. I'll rework these lines.

>
> > +
> > +     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);
> > +
> > +     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);
> > +
> > +     /* pixel_rate = link_freq * 2 * nr_of_lanes / bits_per_sample */
> > +     pixel_rate = div_u64(IMX471_LINK_FREQ_DEFAULT * 2 * 4, 10);
> > +
> > +     sensor->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &imx471_ctrl_ops,
> > +                                            V4L2_CID_PIXEL_RATE, pixel_rate,
> > +                                            pixel_rate, 1, pixel_rate);
> > +
> > +     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);
> > +
> > +     /* 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);
> > +
> > +     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);
> > +
> > +     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;
> > +     }
> > +
> > +     link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > +     sensor->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > +     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);
> > +     unsigned long link_freq_bitmap;
> > +     struct clk *clk;
> > +     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;
> > +
> > +     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");
> > +
> > +     v4l2_i2c_subdev_init(&sensor->sd, client, &imx471_subdev_ops);
> > +
> > +     sensor->regmap = devm_cci_regmap_init_i2c(client, 16);
> > +     if (IS_ERR(sensor->regmap))
> > +             return dev_err_probe(sensor->dev, PTR_ERR(sensor->regmap),
> > +                                  "failed to initialize CCI\n");
> > +
> > +     ret = imx471_power_on(sensor->dev);
> > +     if (ret)
> > +             return dev_err_probe(sensor->dev, ret,
> > +                                  "failed to power on\n");
> > +
> > +     ret = imx471_identify_module(sensor);
> > +     if (ret) {
> > +             dev_err_probe(sensor->dev, ret, "failed to find sensor: %d", ret);
> > +             goto error_power_off;
> > +     }
> > +
> > +     ret = imx471_init_controls(sensor);
> > +     if (ret) {
> > +             dev_err_probe(sensor->dev, ret, "failed to init controls: %d", ret);
> > +             goto error_power_off;
> > +     }
> > +
> > +     sensor->sd.internal_ops = &imx471_internal_ops;
> > +     sensor->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> > +     sensor->pad.flags = MEDIA_PAD_FL_SOURCE;
> > +     sensor->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> > +
> > +     ret = media_entity_pads_init(&sensor->sd.entity, 1, &sensor->pad);
> > +     if (ret) {
> > +             dev_err_probe(sensor->dev, ret, "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_probe(sensor->dev, ret, "failed to init subdev: %d", ret);
> > +             goto error_media_entity_pm;
> > +     }
> > +
> > +     pm_runtime_set_active(sensor->dev);
> > +     pm_runtime_enable(sensor->dev);
> > +
> > +     ret = v4l2_async_register_subdev_sensor(&sensor->sd);
> > +     if (ret < 0)
> > +             goto error_v4l2_subdev_cleanup;
> > +
> > +     pm_runtime_idle(sensor->dev);
> > +
> > +     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);
> > +
> > +     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(&client->dev)) {
> > +             imx471_power_off(&client->dev);
> > +             pm_runtime_set_suspended(&client->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" },
> > +     { "TBE20A0" },
> > +     { /* 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