[PATCH v2 0/2] media: Add driver for ST VD56G3 camera sensor

[PATCH v2 0/2] media: Add driver for ST VD56G3 camera sensor

[Sylvain Petinot]

Hello,

This serie adds support for STMicroelectronics VD56G3 camera sensor.
This is a 1.5M pixel global shutter camera available in both Mono (VD56G3) and
colour (VD66GY) variants.

The following features are supported:
- Auto exposure with expo bias or
- Manual exposure with analog / digital gain
- H/V flip
- vblank/hblank/link freq
- Test pattern
- Supported resolutions in both raw8/raw10 :
   - 1124x1364
   - 1120x1360
   - 1024x1280
   - 1024x768
   - 768x1024
   - 720x1280
   - 640x480
   - 480x640
   - 320x240

This driver supports coldstart parameters for internal AE feature.
To make it work, the driver save gain/expo values in ctrl's cur.val during
poweroff phase. This implementation transgress V4L2 rules... Any advice to make
it proper would be greatly appreciated.

Driver tested on RB5 and RPI (with and without libcamera) for V1. V2 mainly
tested on RPI.

---

v1 -> v2:
- driver: Drop VD56G3_NUM_SUPPLIES
- driver: Rename 'ext_clock' to 'xclk_freq'
- driver: Make use of 'container_of_const' instead of 'container_of'
- driver: Drop usage of WARN()
- driver: Move a few variables to unsigned int
- driver: Add defines for the different Cut revisions
- driver: Replace dev_warn() by dev_err() in situation we're returning errors
- driver: Ensure sensor has dedicated 3.5ms to boot after reset
- driver: Take into account return value of __v4l2_ctrl_modify_range() and
  __v4l2_ctrl_s_ctrl() functions
- driver: Merge vd56g3_power_on() and vd56g3_boot()
- dt-bindings: Lowercase power supply names
- dt-bindings: Drop clock-lanes property
- dt-bindings: Drop unecessary 'items' contraint for lane-polarities
- dt-bindings: Drop unused labels


Sylvain Petinot (2):
  media: dt-bindings: Add ST VD56G3 camera sensor binding
  media: i2c: Add driver for ST VD56G3 camera sensor

 .../bindings/media/i2c/st,st-vd56g3.yaml      |  132 ++
 MAINTAINERS                                   |    9 +
 drivers/media/i2c/Kconfig                     |   11 +
 drivers/media/i2c/Makefile                    |    1 +
 drivers/media/i2c/st-vd56g3.c                 | 1608 +++++++++++++++++
 5 files changed, 1761 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/st,st-vd56g3.yaml
 create mode 100644 drivers/media/i2c/st-vd56g3.c

-- 
2.17.1

[PATCH v2 1/2] media: dt-bindings: Add ST VD56G3 camera sensor binding

[Sylvain Petinot]

Add devicetree bindings Documentation for ST VD56G3 & ST VD66GY camera
sensors. Update MAINTAINERS file.

Signed-off-by: Sylvain Petinot <sylvain.petinot@foss.st.com>
---
 .../bindings/media/i2c/st,st-vd56g3.yaml      | 132 ++++++++++++++++++
 MAINTAINERS                                   |   9 ++
 2 files changed, 141 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/st,st-vd56g3.yaml

diff --git a/Documentation/devicetree/bindings/media/i2c/st,st-vd56g3.yaml b/Documentation/devicetree/bindings/media/i2c/st,st-vd56g3.yaml
new file mode 100644
index 000000000000..22cb2557e311
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/st,st-vd56g3.yaml
@@ -0,0 +1,132 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+# Copyright (c) 2024 STMicroelectronics SA.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/i2c/st,st-vd56g3.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: STMicroelectronics VD56G3 Global Shutter Image Sensor
+
+maintainers:
+  - Benjamin Mugnier <benjamin.mugnier@foss.st.com>
+  - Sylvain Petinot <sylvain.petinot@foss.st.com>
+
+description: |-
+  The STMicroelectronics VD56G3 is a 1.5 M pixel global shutter image sensor
+  with an active array size of 1124 x 1364 (portrait orientation). It is
+  programmable through I2C, the address is fixed to 0x10. The sensor output is
+  available via CSI-2, which is configured as either 1 or 2 data lanes. The
+  sensor provides 8 GPIOS that can be used for external LED signal
+  (synchronized with sensor integration periods)
+
+properties:
+  compatible:
+    enum:
+      - st,st-vd56g3
+      - st,st-vd66gy
+    description:
+      Two variants are availables; VD56G3 is a monochrome sensor while VD66GY
+      is a colour variant.
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  vcore-supply:
+    description: Digital core power supply (1.15V)
+
+  vddio-supply:
+    description: Digital IO power supply (1.8V)
+
+  vana-supply:
+    description: Analog power supply (2.8V)
+
+  reset-gpios:
+    description: Sensor reset active low GPIO (XSHUTDOWN)
+    maxItems: 1
+
+  st,leds:
+    description:
+      Sensor's GPIOs used for external LED control. Signal being the enveloppe
+      of the integration time.
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    minItems: 1
+    maxItems: 8
+    items:
+      minimum: 0
+      maximum: 7
+
+  port:
+    $ref: /schemas/graph.yaml#/$defs/port-base
+
+    properties:
+      endpoint:
+        $ref: /schemas/media/video-interfaces.yaml#
+        unevaluatedProperties: false
+
+        properties:
+          data-lanes:
+            minItems: 1
+            maxItems: 2
+            items:
+              enum: [1, 2]
+
+          link-frequencies:
+            minItems: 1
+            maxItems: 1
+            items:
+              enum: [402000000, 750000000]
+
+          lane-polarities:
+            minItems: 1
+            maxItems: 3
+            description: Any lane can be inverted or not.
+
+        required:
+          - data-lanes
+          - link-frequencies
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - vcore-supply
+  - vddio-supply
+  - vana-supply
+  - reset-gpios
+  - port
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        camera-sensor@10 {
+            compatible = "st,st-vd56g3";
+            reg = <0x10>;
+
+            clocks = <&camera_clk_12M>;
+
+            vcore-supply = <&camera_vcore_v1v15>;
+            vddio-supply = <&camera_vddio_v1v8>;
+            vana-supply = <&camera_vana_v2v8>;
+
+            reset-gpios = <&gpio 5 GPIO_ACTIVE_LOW>;
+            st,leds = <6>;
+
+            port {
+                endpoint {
+                    data-lanes = <1 2>;
+                    link-frequencies = /bits/ 64 <402000000>;
+                    remote-endpoint = <&csiphy0_ep>;
+                };
+            };
+        };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index ef6be9d95143..554e6861425b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20885,6 +20885,15 @@ S:	Maintained
 F:	Documentation/hwmon/stpddc60.rst
 F:	drivers/hwmon/pmbus/stpddc60.c
 
+ST VD56G3 DRIVER
+M:	Benjamin Mugnier <benjamin.mugnier@foss.st.com>
+M:	Sylvain Petinot <sylvain.petinot@foss.st.com>
+L:	linux-media@vger.kernel.org
+S:	Maintained
+T:	git git://linuxtv.org/media_tree.git
+F:	Documentation/devicetree/bindings/media/i2c/st,st-vd56g3.yaml
+F:	drivers/media/i2c/st-vd56g3.c
+
 ST VGXY61 DRIVER
 M:	Benjamin Mugnier <benjamin.mugnier@foss.st.com>
 M:	Sylvain Petinot <sylvain.petinot@foss.st.com>
-- 
2.17.1

Re: [PATCH v2 1/2] media: dt-bindings: Add ST VD56G3 camera sensor binding

[Krzysztof Kozlowski]

On 21/05/2024 18:29, Sylvain Petinot wrote:
> Add devicetree bindings Documentation for ST VD56G3 & ST VD66GY camera
> sensors. Update MAINTAINERS file.
> 

A nit, subject: drop second/last, redundant "binding". The "dt-bindings"
prefix is already stating that these are bindings.
See also:
https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18


> Signed-off-by: Sylvain Petinot <sylvain.petinot@foss.st.com>
> ---
>  .../bindings/media/i2c/st,st-vd56g3.yaml      | 132 ++++++++++++++++++
>  MAINTAINERS                                   |   9 ++
>  2 files changed, 141 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/st,st-vd56g3.yaml
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/st,st-vd56g3.yaml b/Documentation/devicetree/bindings/media/i2c/st,st-vd56g3.yaml
> new file mode 100644
> index 000000000000..22cb2557e311
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/st,st-vd56g3.yaml

Why duplicated 'st'?

> @@ -0,0 +1,132 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright (c) 2024 STMicroelectronics SA.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/i2c/st,st-vd56g3.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: STMicroelectronics VD56G3 Global Shutter Image Sensor
> +
> +maintainers:
> +  - Benjamin Mugnier <benjamin.mugnier@foss.st.com>
> +  - Sylvain Petinot <sylvain.petinot@foss.st.com>
> +
> +description: |-
> +  The STMicroelectronics VD56G3 is a 1.5 M pixel global shutter image sensor

This claims device is VD56G3, not ST-VD56G3.

> +  with an active array size of 1124 x 1364 (portrait orientation). It is
> +  programmable through I2C, the address is fixed to 0x10. The sensor output is
> +  available via CSI-2, which is configured as either 1 or 2 data lanes. The
> +  sensor provides 8 GPIOS that can be used for external LED signal
> +  (synchronized with sensor integration periods)
> +
> +properties:
> +  compatible:
> +    enum:
> +      - st,st-vd56g3
> +      - st,st-vd66gy
> +    description:
> +      Two variants are availables; VD56G3 is a monochrome sensor while VD66GY
> +      is a colour variant.
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  vcore-supply:
> +    description: Digital core power supply (1.15V)
> +
> +  vddio-supply:
> +    description: Digital IO power supply (1.8V)
> +
> +  vana-supply:
> +    description: Analog power supply (2.8V)
> +
> +  reset-gpios:
> +    description: Sensor reset active low GPIO (XSHUTDOWN)
> +    maxItems: 1
> +
> +  st,leds:
> +    description:
> +      Sensor's GPIOs used for external LED control. Signal being the enveloppe
> +      of the integration time.

More information is needed. GPIOs coming from LED or SoC? What's the
meaning of values?

> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    minItems: 1
> +    maxItems: 8
> +    items:
> +      minimum: 0
> +      maximum: 7
> +
> +  port:
> +    $ref: /schemas/graph.yaml#/$defs/port-base

missing additionalProperties: false

> +
> +    properties:
> +      endpoint:
> +        $ref: /schemas/media/video-interfaces.yaml#
> +        unevaluatedProperties: false
> +
> +        properties:
> +          data-lanes:
> +            minItems: 1
> +            maxItems: 2
> +            items:
> +              enum: [1, 2]


> +
> +          link-frequencies:
> +            minItems: 1

maxItems is enough

> +            maxItems: 1
> +            items:
> +              enum: [402000000, 750000000]
> +
> +          lane-polarities:
> +            minItems: 1
> +            maxItems: 3
> +            description: Any lane can be inverted or not.
> +
> +        required:
> +          - data-lanes
> +          - link-frequencies
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - vcore-supply
> +  - vddio-supply
> +  - vana-supply
> +  - reset-gpios
> +  - port
> +


Not a video-interface-device.yaml type of device?

Best regards,
Krzysztof

Re: [PATCH v2 1/2] media: dt-bindings: Add ST VD56G3 camera sensor binding

[Sylvain Petinot]

Hi Krzysztof,

Thanks for the review.

On 5/21/2024 7:37 PM, Krzysztof Kozlowski wrote:
> On 21/05/2024 18:29, Sylvain Petinot wrote:
>> Add devicetree bindings Documentation for ST VD56G3 & ST VD66GY camera
>> sensors. Update MAINTAINERS file.
>>
> 
> A nit, subject: drop second/last, redundant "binding". The "dt-bindings"
> prefix is already stating that these are bindings.
> See also:
> https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
> 

Ok, fixed in V3.

> 
>> Signed-off-by: Sylvain Petinot <sylvain.petinot@foss.st.com>
>> ---
>>  .../bindings/media/i2c/st,st-vd56g3.yaml      | 132 ++++++++++++++++++
>>  MAINTAINERS                                   |   9 ++
>>  2 files changed, 141 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/media/i2c/st,st-vd56g3.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/media/i2c/st,st-vd56g3.yaml b/Documentation/devicetree/bindings/media/i2c/st,st-vd56g3.yaml
>> new file mode 100644
>> index 000000000000..22cb2557e311
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/i2c/st,st-vd56g3.yaml
> 
> Why duplicated 'st'?

Legacy : our first st-mipid02 driver was upstream this way few years back.

We have 3 options :

1- keep this unpleasant naming to keep consistency with st-mipid02 [1]
and st-vgxy61 [2]
2- rename this driver properly ('vd56g3') and keep the two others the
old way (I personally don't like this option)
3- rename this driver properly ('vd56g3') and in a second patch rename
the two others drivers.

I would be interested to get Sakari's opinion on this subject.

[1]:
https://elixir.bootlin.com/linux/v6.9.1/source/drivers/media/i2c/st-mipid02.c

[2]:
https://elixir.bootlin.com/linux/v6.9.1/source/drivers/media/i2c/st-vgxy61.c

> 
>> @@ -0,0 +1,132 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +# Copyright (c) 2024 STMicroelectronics SA.
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/media/i2c/st,st-vd56g3.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: STMicroelectronics VD56G3 Global Shutter Image Sensor
>> +
>> +maintainers:
>> +  - Benjamin Mugnier <benjamin.mugnier@foss.st.com>
>> +  - Sylvain Petinot <sylvain.petinot@foss.st.com>
>> +
>> +description: |-
>> +  The STMicroelectronics VD56G3 is a 1.5 M pixel global shutter image sensor
> 
> This claims device is VD56G3, not ST-VD56G3.

Sure, linked with previous point.

> 
>> +  with an active array size of 1124 x 1364 (portrait orientation). It is
>> +  programmable through I2C, the address is fixed to 0x10. The sensor output is
>> +  available via CSI-2, which is configured as either 1 or 2 data lanes. The
>> +  sensor provides 8 GPIOS that can be used for external LED signal
>> +  (synchronized with sensor integration periods)
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - st,st-vd56g3
>> +      - st,st-vd66gy
>> +    description:
>> +      Two variants are availables; VD56G3 is a monochrome sensor while VD66GY
>> +      is a colour variant.
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  clocks:
>> +    maxItems: 1
>> +
>> +  vcore-supply:
>> +    description: Digital core power supply (1.15V)
>> +
>> +  vddio-supply:
>> +    description: Digital IO power supply (1.8V)
>> +
>> +  vana-supply:
>> +    description: Analog power supply (2.8V)
>> +
>> +  reset-gpios:
>> +    description: Sensor reset active low GPIO (XSHUTDOWN)
>> +    maxItems: 1
>> +
>> +  st,leds:
>> +    description:
>> +      Sensor's GPIOs used for external LED control. Signal being the enveloppe
>> +      of the integration time.
> 
> More information is needed. GPIOs coming from LED or SoC? What's the
> meaning of values?

The vd56g3 image sensor provides 8 GPIOS that can be used for different
use cases (external led controls, synchronization between master/slave
sensors, external sensor trigger, etc.). This submission supports only
the first use case: the control of one(or multiple) external LED.

The vd56g3 sensor family are optimized for visible and near infrared
scenes. In NIR, external IR leds are generally used for illumination.

With such use case, a led (or a led driver) can be connected directly to
one of the 8 GPIOs of the sensor. On the driver side, when a led is
configured in the dt, the driver will configure the sensor accordingly.
It will also offer an optional "V4L2_FLASH_LED_MODE_FLASH" control to
start/stop the external control.

Different signal modes are supported by the HW, but the default
(implemented) one is a "strobe" mode where signal is the envelope of the
integration time (IR led is on while image sensor is integrating).

> 
>> +    $ref: /schemas/types.yaml#/definitions/uint32-array
>> +    minItems: 1
>> +    maxItems: 8
>> +    items:
>> +      minimum: 0
>> +      maximum: 7
>> +
>> +  port:
>> +    $ref: /schemas/graph.yaml#/$defs/port-base
> 
> missing additionalProperties: false

Ok, fixed in V3.

> 
>> +
>> +    properties:
>> +      endpoint:
>> +        $ref: /schemas/media/video-interfaces.yaml#
>> +        unevaluatedProperties: false
>> +
>> +        properties:
>> +          data-lanes:
>> +            minItems: 1
>> +            maxItems: 2
>> +            items:
>> +              enum: [1, 2]
> 
> 
>> +
>> +          link-frequencies:
>> +            minItems: 1
> 
> maxItems is enough

Ok, fixed in V3.

> 
>> +            maxItems: 1
>> +            items:
>> +              enum: [402000000, 750000000]
>> +
>> +          lane-polarities:
>> +            minItems: 1
>> +            maxItems: 3
>> +            description: Any lane can be inverted or not.
>> +
>> +        required:
>> +          - data-lanes
>> +          - link-frequencies
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - clocks
>> +  - vcore-supply
>> +  - vddio-supply
>> +  - vana-supply
>> +  - reset-gpios
>> +  - port
>> +
> 
> 
> Not a video-interface-device.yaml type of device?

Good point, something I'll consider in V3

> 
> Best regards,
> Krzysztof
> 

--
Sylvain

Re: [PATCH v2 1/2] media: dt-bindings: Add ST VD56G3 camera sensor binding

[Krzysztof Kozlowski]

On 27/05/2024 15:14, Sylvain Petinot wrote:
>>
>>> Signed-off-by: Sylvain Petinot <sylvain.petinot@foss.st.com>
>>> ---
>>>  .../bindings/media/i2c/st,st-vd56g3.yaml      | 132 ++++++++++++++++++
>>>  MAINTAINERS                                   |   9 ++
>>>  2 files changed, 141 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/media/i2c/st,st-vd56g3.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/media/i2c/st,st-vd56g3.yaml b/Documentation/devicetree/bindings/media/i2c/st,st-vd56g3.yaml
>>> new file mode 100644
>>> index 000000000000..22cb2557e311
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/media/i2c/st,st-vd56g3.yaml
>>
>> Why duplicated 'st'?
> 
> Legacy : our first st-mipid02 driver was upstream this way few years back.
> 
> We have 3 options :
> 
> 1- keep this unpleasant naming to keep consistency with st-mipid02 [1]
> and st-vgxy61 [2]

? Unpleasant?
Please follow generic rules. Filename must match compatible and
compatible must follow vendor,device format.

> 2- rename this driver properly ('vd56g3') and keep the two others the
> old way (I personally don't like this option)

We do not talk about driver here. Does not matter.

> 3- rename this driver properly ('vd56g3') and in a second patch rename
> the two others drivers.
> 
> I would be interested to get Sakari's opinion on this subject.

About what? Renaming drivers?

> 
> [1]:
> https://elixir.bootlin.com/linux/v6.9.1/source/drivers/media/i2c/st-mipid02.c
> 
> [2]:
> https://elixir.bootlin.com/linux/v6.9.1/source/drivers/media/i2c/st-vgxy61.c

Howe are these drivers anyhow related to the *binding*?


...

>>> +
>>> +  st,leds:
>>> +    description:
>>> +      Sensor's GPIOs used for external LED control. Signal being the enveloppe
>>> +      of the integration time.
>>
>> More information is needed. GPIOs coming from LED or SoC? What's the
>> meaning of values?
> 
> The vd56g3 image sensor provides 8 GPIOS that can be used for different
> use cases (external led controls, synchronization between master/slave
> sensors, external sensor trigger, etc.). This submission supports only
> the first use case: the control of one(or multiple) external LED.

What your driver supports is not really relevant. Describe hardware.

> 
> The vd56g3 sensor family are optimized for visible and near infrared
> scenes. In NIR, external IR leds are generally used for illumination.
> 
> With such use case, a led (or a led driver) can be connected directly to
> one of the 8 GPIOs of the sensor. On the driver side, when a led is
> configured in the dt, the driver will configure the sensor accordingly.
> It will also offer an optional "V4L2_FLASH_LED_MODE_FLASH" control to
> start/stop the external control.
> 
> Different signal modes are supported by the HW, but the default
> (implemented) one is a "strobe" mode where signal is the envelope of the
> integration time (IR led is on while image sensor is integrating).

You did not explain the meaning of the property. Please describe the
hardware and the meaning of values used in this property.



Best regards,
Krzysztof

Re: [PATCH v2 1/2] media: dt-bindings: Add ST VD56G3 camera sensor binding

[Sylvain Petinot]

Hello Krzysztof,


On 5/27/2024 9:00 PM, Krzysztof Kozlowski wrote:
> On 27/05/2024 15:14, Sylvain Petinot wrote:
>>>
>>>> Signed-off-by: Sylvain Petinot <sylvain.petinot@foss.st.com>
>>>> ---
>>>>  .../bindings/media/i2c/st,st-vd56g3.yaml      | 132 ++++++++++++++++++
>>>>  MAINTAINERS                                   |   9 ++
>>>>  2 files changed, 141 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/media/i2c/st,st-vd56g3.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/media/i2c/st,st-vd56g3.yaml b/Documentation/devicetree/bindings/media/i2c/st,st-vd56g3.yaml
>>>> new file mode 100644
>>>> index 000000000000..22cb2557e311
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/media/i2c/st,st-vd56g3.yaml
>>>
>>> Why duplicated 'st'?
>>
>> Legacy : our first st-mipid02 driver was upstream this way few years back.
>>
>> We have 3 options :
>>
>> 1- keep this unpleasant naming to keep consistency with st-mipid02 [1]
>> and st-vgxy61 [2]
> 
> ? Unpleasant?
> Please follow generic rules. Filename must match compatible and
> compatible must follow vendor,device format.
> 
>> 2- rename this driver properly ('vd56g3') and keep the two others the
>> old way (I personally don't like this option)
> 
> We do not talk about driver here. Does not matter.
> 
>> 3- rename this driver properly ('vd56g3') and in a second patch rename
>> the two others drivers.
>>
>> I would be interested to get Sakari's opinion on this subject.
> 
> About what? Renaming drivers?
> 
>>
>> [1]:
>> https://elixir.bootlin.com/linux/v6.9.1/source/drivers/media/i2c/st-mipid02.c
>>
>> [2]:
>> https://elixir.bootlin.com/linux/v6.9.1/source/drivers/media/i2c/st-vgxy61.c
> 
> Howe are these drivers anyhow related to the *binding*?

I got your point. bindings will be updated accordingly in V3 to follow
vendor,device format.

My point was more about consistency :
1- ensure driver name is aligned with the bindings name (without vendor
prefix)
2- ensure all ST image sensor drivers and bindings follow the same rules.
But, you're right, this is a out of bindings topic and I will handle it
separately.

> 
> 
> ...
> 
>>>> +
>>>> +  st,leds:
>>>> +    description:
>>>> +      Sensor's GPIOs used for external LED control. Signal being the enveloppe
>>>> +      of the integration time.
>>>
>>> More information is needed. GPIOs coming from LED or SoC? What's the
>>> meaning of values?
>>
>> The vd56g3 image sensor provides 8 GPIOS that can be used for different
>> use cases (external led controls, synchronization between master/slave
>> sensors, external sensor trigger, etc.). This submission supports only
>> the first use case: the control of one(or multiple) external LED.
> 
> What your driver supports is not really relevant. Describe hardware.
> 
>>
>> The vd56g3 sensor family are optimized for visible and near infrared
>> scenes. In NIR, external IR leds are generally used for illumination.
>>
>> With such use case, a led (or a led driver) can be connected directly to
>> one of the 8 GPIOs of the sensor. On the driver side, when a led is
>> configured in the dt, the driver will configure the sensor accordingly.
>> It will also offer an optional "V4L2_FLASH_LED_MODE_FLASH" control to
>> start/stop the external control.
>>
>> Different signal modes are supported by the HW, but the default
>> (implemented) one is a "strobe" mode where signal is the envelope of the
>> integration time (IR led is on while image sensor is integrating).
> 
> You did not explain the meaning of the property. Please describe the
> hardware and the meaning of values used in this property.
> 
> 

Sure, this was more contextual information. Please find below a proposal
for the "st,leds" property :

st,leds:
  description:
    List sensor's GPIOs used to control strobe light sources during exposure
    time. The numbers identify the sensor pin on which the illumination
system
    is connected. GPIOs are active-high.
  $ref: /schemas/types.yaml#/definitions/uint32-array
  minItems: 1
  maxItems: 8
  items:
    minimum: 0
    maximum: 7

> 
> Best regards,
> Krzysztof
> 

--
Sylvain

Re: [PATCH v2 1/2] media: dt-bindings: Add ST VD56G3 camera sensor binding

[Sakari Ailus]

Hi Sylvain,

On Mon, May 27, 2024 at 03:14:35PM +0200, Sylvain Petinot wrote:
> >> diff --git a/Documentation/devicetree/bindings/media/i2c/st,st-vd56g3.yaml b/Documentation/devicetree/bindings/media/i2c/st,st-vd56g3.yaml
> >> new file mode 100644
> >> index 000000000000..22cb2557e311
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/media/i2c/st,st-vd56g3.yaml
> > 
> > Why duplicated 'st'?
> 
> Legacy : our first st-mipid02 driver was upstream this way few years back.
> 
> We have 3 options :
> 
> 1- keep this unpleasant naming to keep consistency with st-mipid02 [1]
> and st-vgxy61 [2]
> 2- rename this driver properly ('vd56g3') and keep the two others the
> old way (I personally don't like this option)
> 3- rename this driver properly ('vd56g3') and in a second patch rename
> the two others drivers.
> 
> I would be interested to get Sakari's opinion on this subject.
> 
> [1]:
> https://elixir.bootlin.com/linux/v6.9.1/source/drivers/media/i2c/st-mipid02.c
> 
> [2]:
> https://elixir.bootlin.com/linux/v6.9.1/source/drivers/media/i2c/st-vgxy61.c

The driver could be renamed to align with a large majority that use the
same name as the bindings without the vendor prefix. You could add
MODULE_ALIAS() to help user space to cope with the change.

The DT compatible string indeed should reflect the name of the device, the
driver is indeed another matter.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v2 1/2] media: dt-bindings: Add ST VD56G3 camera sensor binding

[Krzysztof Kozlowski]

On 21/05/2024 18:29, Sylvain Petinot wrote:
> Add devicetree bindings Documentation for ST VD56G3 & ST VD66GY camera
> sensors. Update MAINTAINERS file.
> 
> Signed-off-by: Sylvain Petinot <sylvain.petinot@foss.st.com>


> diff --git a/MAINTAINERS b/MAINTAINERS
> index ef6be9d95143..554e6861425b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -20885,6 +20885,15 @@ S:	Maintained
>  F:	Documentation/hwmon/stpddc60.rst
>  F:	drivers/hwmon/pmbus/stpddc60.c
>  
> +ST VD56G3 DRIVER
> +M:	Benjamin Mugnier <benjamin.mugnier@foss.st.com>
> +M:	Sylvain Petinot <sylvain.petinot@foss.st.com>
> +L:	linux-media@vger.kernel.org
> +S:	Maintained
> +T:	git git://linuxtv.org/media_tree.git

This is a friendly reminder during the review process.

It seems my or other reviewer's previous comments were not fully
addressed. Maybe the feedback got lost between the quotes, maybe you
just forgot to apply it. Please go back to the previous discussion and
either implement all requested changes or keep discussing them.

Thank you.

Best regards,
Krzysztof

Re: [PATCH v2 1/2] media: dt-bindings: Add ST VD56G3 camera sensor binding

[Sakari Ailus]

Hi Krzysztof,

On Mon, May 27, 2024 at 09:04:38PM +0200, Krzysztof Kozlowski wrote:
> On 21/05/2024 18:29, Sylvain Petinot wrote:
> > Add devicetree bindings Documentation for ST VD56G3 & ST VD66GY camera
> > sensors. Update MAINTAINERS file.
> > 
> > Signed-off-by: Sylvain Petinot <sylvain.petinot@foss.st.com>
> 
> 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index ef6be9d95143..554e6861425b 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -20885,6 +20885,15 @@ S:	Maintained
> >  F:	Documentation/hwmon/stpddc60.rst
> >  F:	drivers/hwmon/pmbus/stpddc60.c
> >  
> > +ST VD56G3 DRIVER

I might add this is a sensor, i.e. "ST VD653G IMAGE SENSOR DRIVER".

> > +M:	Benjamin Mugnier <benjamin.mugnier@foss.st.com>
> > +M:	Sylvain Petinot <sylvain.petinot@foss.st.com>
> > +L:	linux-media@vger.kernel.org
> > +S:	Maintained
> > +T:	git git://linuxtv.org/media_tree.git
> 
> This is a friendly reminder during the review process.
> 
> It seems my or other reviewer's previous comments were not fully
> addressed. Maybe the feedback got lost between the quotes, maybe you
> just forgot to apply it. Please go back to the previous discussion and
> either implement all requested changes or keep discussing them.

The above MAINTAINERS entry is roughly in line with what else we have for
the Media tree. I'm in favour of listing the people who would look after
the driver, not just those who merge the patches (or even send PRs to
Linus).

In other words, I think the above entry is fine as-is.

Sylvain: could you also add the file under "V4L2 CAMERA SENSOR DRIVERS"
that lists myself as the maintainer?

Thanks.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v2 1/2] media: dt-bindings: Add ST VD56G3 camera sensor binding

[Krzysztof Kozlowski]

On 28/05/2024 11:22, Sakari Ailus wrote:
> Hi Krzysztof,
> 
> On Mon, May 27, 2024 at 09:04:38PM +0200, Krzysztof Kozlowski wrote:
>> On 21/05/2024 18:29, Sylvain Petinot wrote:
>>> Add devicetree bindings Documentation for ST VD56G3 & ST VD66GY camera
>>> sensors. Update MAINTAINERS file.
>>>
>>> Signed-off-by: Sylvain Petinot <sylvain.petinot@foss.st.com>
>>
>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index ef6be9d95143..554e6861425b 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -20885,6 +20885,15 @@ S:	Maintained
>>>  F:	Documentation/hwmon/stpddc60.rst
>>>  F:	drivers/hwmon/pmbus/stpddc60.c
>>>  
>>> +ST VD56G3 DRIVER
> 
> I might add this is a sensor, i.e. "ST VD653G IMAGE SENSOR DRIVER".
> 
>>> +M:	Benjamin Mugnier <benjamin.mugnier@foss.st.com>
>>> +M:	Sylvain Petinot <sylvain.petinot@foss.st.com>
>>> +L:	linux-media@vger.kernel.org
>>> +S:	Maintained
>>> +T:	git git://linuxtv.org/media_tree.git
>>
>> This is a friendly reminder during the review process.
>>
>> It seems my or other reviewer's previous comments were not fully
>> addressed. Maybe the feedback got lost between the quotes, maybe you
>> just forgot to apply it. Please go back to the previous discussion and
>> either implement all requested changes or keep discussing them.
> 
> The above MAINTAINERS entry is roughly in line with what else we have for
> the Media tree. I'm in favour of listing the people who would look after
> the driver, not just those who merge the patches (or even send PRs to
> Linus).

I did not propose to drop the entry.

> 
> In other words, I think the above entry is fine as-is.

I propose to drop duplicated, redundant git entry. Maintainer of this
driver does not have access to git tree and the git tree is already
explained in media subsystem entry. If you ever update the git tree, you
need to update 100 driver entries which is meaningless...


Best regards,
Krzysztof

Re: [PATCH v2 1/2] media: dt-bindings: Add ST VD56G3 camera sensor binding

[Sakari Ailus]

Hi Krzysztof,

On Tue, May 28, 2024 at 11:46:00AM +0200, Krzysztof Kozlowski wrote:
> On 28/05/2024 11:22, Sakari Ailus wrote:
> > Hi Krzysztof,
> > 
> > On Mon, May 27, 2024 at 09:04:38PM +0200, Krzysztof Kozlowski wrote:
> >> On 21/05/2024 18:29, Sylvain Petinot wrote:
> >>> Add devicetree bindings Documentation for ST VD56G3 & ST VD66GY camera
> >>> sensors. Update MAINTAINERS file.
> >>>
> >>> Signed-off-by: Sylvain Petinot <sylvain.petinot@foss.st.com>
> >>
> >>
> >>> diff --git a/MAINTAINERS b/MAINTAINERS
> >>> index ef6be9d95143..554e6861425b 100644
> >>> --- a/MAINTAINERS
> >>> +++ b/MAINTAINERS
> >>> @@ -20885,6 +20885,15 @@ S:	Maintained
> >>>  F:	Documentation/hwmon/stpddc60.rst
> >>>  F:	drivers/hwmon/pmbus/stpddc60.c
> >>>  
> >>> +ST VD56G3 DRIVER
> > 
> > I might add this is a sensor, i.e. "ST VD653G IMAGE SENSOR DRIVER".
> > 
> >>> +M:	Benjamin Mugnier <benjamin.mugnier@foss.st.com>
> >>> +M:	Sylvain Petinot <sylvain.petinot@foss.st.com>
> >>> +L:	linux-media@vger.kernel.org
> >>> +S:	Maintained
> >>> +T:	git git://linuxtv.org/media_tree.git
> >>
> >> This is a friendly reminder during the review process.
> >>
> >> It seems my or other reviewer's previous comments were not fully
> >> addressed. Maybe the feedback got lost between the quotes, maybe you
> >> just forgot to apply it. Please go back to the previous discussion and
> >> either implement all requested changes or keep discussing them.
> > 
> > The above MAINTAINERS entry is roughly in line with what else we have for
> > the Media tree. I'm in favour of listing the people who would look after
> > the driver, not just those who merge the patches (or even send PRs to
> > Linus).
> 
> I did not propose to drop the entry.
> 
> > 
> > In other words, I think the above entry is fine as-is.
> 
> I propose to drop duplicated, redundant git entry. Maintainer of this

Ah, I agree, that makes sense.

> driver does not have access to git tree and the git tree is already
> explained in media subsystem entry. If you ever update the git tree, you
> need to update 100 driver entries which is meaningless...

-- 
Regards,

Sakari Ailus

Re: [PATCH v2 1/2] media: dt-bindings: Add ST VD56G3 camera sensor binding

[Sylvain Petinot]

Hi Krzysztof, Sakari

On 5/28/2024 12:01 PM, Sakari Ailus wrote:
> Hi Krzysztof,
> 
> On Tue, May 28, 2024 at 11:46:00AM +0200, Krzysztof Kozlowski wrote:
>> On 28/05/2024 11:22, Sakari Ailus wrote:
>>> Hi Krzysztof,
>>>
>>> On Mon, May 27, 2024 at 09:04:38PM +0200, Krzysztof Kozlowski wrote:
>>>> On 21/05/2024 18:29, Sylvain Petinot wrote:
>>>>> Add devicetree bindings Documentation for ST VD56G3 & ST VD66GY camera
>>>>> sensors. Update MAINTAINERS file.
>>>>>
>>>>> Signed-off-by: Sylvain Petinot <sylvain.petinot@foss.st.com>
>>>>
>>>>
>>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>>> index ef6be9d95143..554e6861425b 100644
>>>>> --- a/MAINTAINERS
>>>>> +++ b/MAINTAINERS
>>>>> @@ -20885,6 +20885,15 @@ S:	Maintained
>>>>>  F:	Documentation/hwmon/stpddc60.rst
>>>>>  F:	drivers/hwmon/pmbus/stpddc60.c
>>>>>  
>>>>> +ST VD56G3 DRIVER
>>>
>>> I might add this is a sensor, i.e. "ST VD653G IMAGE SENSOR DRIVER".
>>>
>>>>> +M:	Benjamin Mugnier <benjamin.mugnier@foss.st.com>
>>>>> +M:	Sylvain Petinot <sylvain.petinot@foss.st.com>
>>>>> +L:	linux-media@vger.kernel.org
>>>>> +S:	Maintained
>>>>> +T:	git git://linuxtv.org/media_tree.git
>>>>
>>>> This is a friendly reminder during the review process.
>>>>
>>>> It seems my or other reviewer's previous comments were not fully
>>>> addressed. Maybe the feedback got lost between the quotes, maybe you
>>>> just forgot to apply it. Please go back to the previous discussion and
>>>> either implement all requested changes or keep discussing them.
>>>
>>> The above MAINTAINERS entry is roughly in line with what else we have for
>>> the Media tree. I'm in favour of listing the people who would look after
>>> the driver, not just those who merge the patches (or even send PRs to
>>> Linus).
>>
>> I did not propose to drop the entry.
>>
>>>
>>> In other words, I think the above entry is fine as-is.
>>
>> I propose to drop duplicated, redundant git entry. Maintainer of this
> 
> Ah, I agree, that makes sense.

Thanks for clarifying, git entry will be drop in V3.

> 
>> driver does not have access to git tree and the git tree is already
>> explained in media subsystem entry. If you ever update the git tree, you
>> need to update 100 driver entries which is meaningless...
> 

--
Sylvain

[PATCH v2 2/2] media: i2c: Add driver for ST VD56G3 camera sensor

[Sylvain Petinot]

Add V4L2 sub-device driver for STMicroelectronics VD56G3 camera sensor.
This is a 1.5 M pixel global shutter image sensor with an active array
size of 1124 x 1364 (portrait orientation).

The driver supports Mono (VD56G3) and Color (VD66GY) variants.

Signed-off-by: Sylvain Petinot <sylvain.petinot@foss.st.com>
---
 drivers/media/i2c/Kconfig     |   11 +
 drivers/media/i2c/Makefile    |    1 +
 drivers/media/i2c/st-vd56g3.c | 1608 +++++++++++++++++++++++++++++++++
 3 files changed, 1620 insertions(+)
 create mode 100644 drivers/media/i2c/st-vd56g3.c

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index c6d3ee472d81..c3d3cbdfcb3b 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -659,6 +659,17 @@ config VIDEO_S5K6A3
 	  This is a V4L2 sensor driver for Samsung S5K6A3 raw
 	  camera sensor.
 
+config VIDEO_ST_VD56G3
+	tristate "ST VD56G3 sensor support"
+	select V4L2_CCI_I2C
+	depends on OF && GPIOLIB
+	help
+	  This is a Video4Linux2 sensor driver for the ST VD56G3
+	  camera sensor.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called st-vd56g3.
+
 config VIDEO_ST_VGXY61
 	tristate "ST VGXY61 sensor support"
 	select V4L2_CCI_I2C
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index dfbe6448b549..b469cf0f8113 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -124,6 +124,7 @@ obj-$(CONFIG_VIDEO_SAA717X) += saa717x.o
 obj-$(CONFIG_VIDEO_SAA7185) += saa7185.o
 obj-$(CONFIG_VIDEO_SONY_BTF_MPX) += sony-btf-mpx.o
 obj-$(CONFIG_VIDEO_ST_MIPID02) += st-mipid02.o
+obj-$(CONFIG_VIDEO_ST_VD56G3) += st-vd56g3.o
 obj-$(CONFIG_VIDEO_ST_VGXY61) += st-vgxy61.o
 obj-$(CONFIG_VIDEO_TC358743) += tc358743.o
 obj-$(CONFIG_VIDEO_TC358746) += tc358746.o
diff --git a/drivers/media/i2c/st-vd56g3.c b/drivers/media/i2c/st-vd56g3.c
new file mode 100644
index 000000000000..1e4329502230
--- /dev/null
+++ b/drivers/media/i2c/st-vd56g3.c
@@ -0,0 +1,1608 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * A V4L2 driver for ST VD56G3 (Mono) and VD66GY (RGB) global shutter cameras.
+ * Copyright (C) 2024, STMicroelectronics SA
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/iopoll.h>
+#include <linux/module.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/units.h>
+
+#include <asm/unaligned.h>
+
+#include <media/mipi-csi2.h>
+#include <media/v4l2-async.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>
+#include <media/v4l2-subdev.h>
+
+/* Register Map */
+#define VD56G3_REG_MODEL_ID				CCI_REG16_LE(0x0000)
+#define VD56G3_MODEL_ID					0x5603
+#define VD56G3_REG_REVISION				CCI_REG16_LE(0x0002)
+#define VD56G3_REVISION_CUT3				0x31
+#define VD56G3_REG_OPTICAL_REVISION			CCI_REG8(0x001a)
+#define VD56G3_OPTICAL_REVISION_MONO			0
+#define VD56G3_OPTICAL_REVISION_BAYER			1
+#define VD56G3_REG_SYSTEM_FSM				CCI_REG8(0x0028)
+#define VD56G3_SYSTEM_FSM_READY_TO_BOOT			0x01
+#define VD56G3_SYSTEM_FSM_SW_STBY			0x02
+#define VD56G3_SYSTEM_FSM_STREAMING			0x03
+#define VD56G3_REG_APPLIED_COARSE_EXPOSURE		CCI_REG16_LE(0x0064)
+#define VD56G3_REG_APPLIED_ANALOG_GAIN			CCI_REG8(0x0068)
+#define VD56G3_REG_APPLIED_DIGITAL_GAIN			CCI_REG16_LE(0x006a)
+#define VD56G3_REG_BOOT					CCI_REG8(0x0200)
+#define VD56G3_CMD_ACK					0
+#define VD56G3_CMD_BOOT					1
+#define VD56G3_REG_STBY					CCI_REG8(0x0201)
+#define VD56G3_CMD_START_STREAM				1
+#define VD56G3_REG_STREAMING				CCI_REG8(0x0202)
+#define VD56G3_CMD_STOP_STREAM				1
+#define VD56G3_REG_EXT_CLOCK				CCI_REG32_LE(0x0220)
+#define VD56G3_REG_CLK_PLL_PREDIV			CCI_REG8(0x0224)
+#define VD56G3_REG_CLK_SYS_PLL_MULT			CCI_REG8(0x0226)
+#define VD56G3_REG_ORIENTATION				CCI_REG8(0x0302)
+#define VD56G3_REG_FORMAT_CTRL				CCI_REG8(0x030a)
+#define VD56G3_REG_OIF_CTRL				CCI_REG16_LE(0x030c)
+#define VD56G3_REG_OIF_IMG_CTRL				CCI_REG8(0x030f)
+#define VD56G3_REG_OIF_CSI_BITRATE			CCI_REG16_LE(0x0312)
+#define VD56G3_REG_DUSTER_CTRL				CCI_REG8(0x0318)
+#define VD56G3_DUSTER_DISABLE				0
+#define VD56G3_DUSTER_ENABLE_DEF_MODULES		0x13
+#define VD56G3_REG_ISL_ENABLE				CCI_REG8(0x0333)
+#define VD56G3_REG_DARKCAL_CTRL				CCI_REG8(0x0340)
+#define VD56G3_DARKCAL_ENABLE				1
+#define VD56G3_DARKCAL_DISABLE_DARKAVG			2
+#define VD56G3_REG_PATGEN_CTRL				CCI_REG16_LE(0x0400)
+#define VD56G3_PATGEN_ENABLE				1
+#define VD56G3_PATGEN_TYPE_SHIFT			4
+#define VD56G3_REG_AE_COLDSTART_COARSE_EXPOSURE		CCI_REG16_LE(0x042a)
+#define VD56G3_REG_AE_COLDSTART_ANALOG_GAIN		CCI_REG8(0x042c)
+#define VD56G3_REG_AE_COLDSTART_DIGITAL_GAIN		CCI_REG16_LE(0x042e)
+#define VD56G3_REG_AE_ROI_START_H			CCI_REG16_LE(0x0432)
+#define VD56G3_REG_AE_ROI_START_V			CCI_REG16_LE(0x0434)
+#define VD56G3_REG_AE_ROI_END_H				CCI_REG16_LE(0x0436)
+#define VD56G3_REG_AE_ROI_END_V				CCI_REG16_LE(0x0438)
+#define VD56G3_REG_AE_COMPENSATION			CCI_REG16_LE(0x043a)
+#define VD56G3_REG_EXP_MODE				CCI_REG8(0x044c)
+#define VD56G3_EXP_MODE_AUTO				0
+#define VD56G3_EXP_MODE_FREEZE				1
+#define VD56G3_EXP_MODE_MANUAL				2
+#define VD56G3_REG_MANUAL_ANALOG_GAIN			CCI_REG8(0x044d)
+#define VD56G3_REG_MANUAL_COARSE_EXPOSURE		CCI_REG16_LE(0x044e)
+#define VD56G3_REG_MANUAL_DIGITAL_GAIN_CH0		CCI_REG16_LE(0x0450)
+#define VD56G3_REG_MANUAL_DIGITAL_GAIN_CH1		CCI_REG16_LE(0x0452)
+#define VD56G3_REG_MANUAL_DIGITAL_GAIN_CH2		CCI_REG16_LE(0x0454)
+#define VD56G3_REG_MANUAL_DIGITAL_GAIN_CH3		CCI_REG16_LE(0x0456)
+#define VD56G3_REG_FRAME_LENGTH				CCI_REG16_LE(0x0458)
+#define VD56G3_REG_Y_START				CCI_REG16_LE(0x045a)
+#define VD56G3_REG_Y_END				CCI_REG16_LE(0x045c)
+#define VD56G3_REG_OUT_ROI_X_START			CCI_REG16_LE(0x045e)
+#define VD56G3_REG_OUT_ROI_X_END			CCI_REG16_LE(0x0460)
+#define VD56G3_REG_OUT_ROI_Y_START			CCI_REG16_LE(0x0462)
+#define VD56G3_REG_OUT_ROI_Y_END			CCI_REG16_LE(0x0464)
+#define VD56G3_REG_GPIO_0_CTRL				CCI_REG8(0x0467)
+#define VD56G3_GPIOX_GPIO_IN				0x01
+#define VD56G3_GPIOX_STROBE_MODE			0x02
+#define VD56G3_REG_READOUT_CTRL				CCI_REG8(0x047e)
+#define READOUT_NORMAL					0x00
+#define READOUT_DIGITAL_BINNING_X2			0x01
+
+/*
+ * The VD56G3 pixel array is organized as follows:
+ *
+ * +--------------------------------+
+ * |                                | \
+ * |   +------------------------+   |  |
+ * |   |                        |   |  |
+ * |   |                        |   |  |
+ * |   |                        |   |  |
+ * |   |                        |   |  |
+ * |   |                        |   |  |
+ * |   |   Default resolution   |   |  | Native height (1364)
+ * |   |      1120 x 1360       |   |  |
+ * |   |                        |   |  |
+ * |   |                        |   |  |
+ * |   |                        |   |  |
+ * |   |                        |   |  |
+ * |   +------------------------+   |  |
+ * |                                | /
+ * +--------------------------------+
+ *   <----------------------------->
+ *                     \-------------------  Native width (1124)
+ *
+ * The native resolution is 1124x1364.
+ * The recommended/default resolution is 1120x1360 (multiple of 16).
+ */
+#define VD56G3_NATIVE_WIDTH				1124
+#define VD56G3_NATIVE_HEIGHT				1364
+#define VD56G3_DEFAULT_WIDTH				1120
+#define VD56G3_DEFAULT_HEIGHT				1360
+#define VD56G3_DEFAULT_MODE				1
+
+/* PLL settings */
+#define VD56G3_TARGET_PLL				804000000UL
+#define VD56G3_VT_CLOCK_DIV				5
+
+/* Line length and Frame length (settings are for standard 10bits ADC mode) */
+#define VD56G3_LINE_LENGTH_MIN				1236
+#define VD56G3_VBLANK_MIN				110
+#define VD56G3_FRAME_LENGTH_DEF_60FPS			2168
+
+/* Exposure settings */
+#define VD56G3_EXPOSURE_MARGIN				75
+#define VD56G3_EXPOSURE_DEFAULT				1420
+
+/* Output Interface settings */
+#define VD56G3_MAX_CSI_DATA_LANES			2
+#define VD56G3_LINK_FREQ_DEF_1LANE			750000000UL
+#define VD56G3_LINK_FREQ_DEF_2LANES			402000000UL
+
+/* GPIOs */
+#define VD56G3_NB_GPIOS					8
+
+/* regulator supplies */
+static const char *const vd56g3_supply_names[] = {
+	"vcore",
+	"vddio",
+	"vana",
+};
+
+/* -----------------------------------------------------------------------------
+ * Models (VD56G3: Mono, VD66GY: Bayer RGB), Modes and formats
+ */
+
+enum vd56g3_models {
+	VD56G3_MODEL_VD56G3,
+	VD56G3_MODEL_VD66GY,
+};
+
+struct vd56g3_mode {
+	u32 width;
+	u32 height;
+};
+
+static const struct vd56g3_mode vd56g3_supported_modes[] = {
+	{
+		.width = VD56G3_NATIVE_WIDTH,
+		.height = VD56G3_NATIVE_HEIGHT,
+	},
+	{
+		.width = VD56G3_DEFAULT_WIDTH,
+		.height = VD56G3_DEFAULT_HEIGHT,
+	},
+	{
+		.width = 1024,
+		.height = 1280,
+	},
+	{
+		.width = 1024,
+		.height = 768,
+	},
+	{
+		.width = 768,
+		.height = 1024,
+	},
+	{
+		.width = 720,
+		.height = 1280,
+	},
+	{
+		.width = 640,
+		.height = 480,
+	},
+	{
+		.width = 480,
+		.height = 640,
+	},
+	{
+		.width = 320,
+		.height = 240,
+	},
+};
+
+/*
+ * Sensor support 8bits and 10bits output in both variants
+ *  - Monochrome
+ *  - RGB (with all H/V flip variations)
+ */
+static const unsigned int vd56g3_mbus_codes[2][5] = {
+	{
+		MEDIA_BUS_FMT_Y8_1X8,
+		MEDIA_BUS_FMT_SGRBG8_1X8,
+		MEDIA_BUS_FMT_SRGGB8_1X8,
+		MEDIA_BUS_FMT_SBGGR8_1X8,
+		MEDIA_BUS_FMT_SGBRG8_1X8,
+	},
+	{
+		MEDIA_BUS_FMT_Y10_1X10,
+		MEDIA_BUS_FMT_SGRBG10_1X10,
+		MEDIA_BUS_FMT_SRGGB10_1X10,
+		MEDIA_BUS_FMT_SBGGR10_1X10,
+		MEDIA_BUS_FMT_SGBRG10_1X10,
+	},
+};
+
+enum vd56g3_expo_state {
+	VD56G3_EXPO_AUTO,
+	VD56G3_EXPO_AUTO_FREEZE,
+	VD56G3_EXPO_MANUAL
+};
+
+struct vd56g3 {
+	struct i2c_client *i2c_client;
+	struct v4l2_subdev sd;
+	struct media_pad pad;
+	struct regulator_bulk_data supplies[ARRAY_SIZE(vd56g3_supply_names)];
+	struct gpio_desc *reset_gpio;
+	struct clk *xclk;
+	struct regmap *regmap;
+	u32 xclk_freq;
+	u32 pll_prediv;
+	u32 pll_mult;
+	u32 pixel_clock;
+	u16 oif_ctrl;
+	u8 nb_of_lane;
+	u32 gpios[VD56G3_NB_GPIOS];
+	unsigned long ext_leds_mask;
+	bool is_mono;
+	struct v4l2_ctrl_handler ctrl_handler;
+	struct v4l2_ctrl *hblank_ctrl;
+	struct v4l2_ctrl *vblank_ctrl;
+	struct {
+		struct v4l2_ctrl *hflip_ctrl;
+		struct v4l2_ctrl *vflip_ctrl;
+	};
+	struct v4l2_ctrl *patgen_ctrl;
+	struct {
+		struct v4l2_ctrl *ae_ctrl;
+		struct v4l2_ctrl *expo_ctrl;
+		struct v4l2_ctrl *again_ctrl;
+		struct v4l2_ctrl *dgain_ctrl;
+	};
+	struct v4l2_ctrl *ae_lock_ctrl;
+	struct v4l2_ctrl *ae_bias_ctrl;
+	struct v4l2_ctrl *led_ctrl;
+	bool streaming;
+};
+
+static inline struct vd56g3 *to_vd56g3(struct v4l2_subdev *sd)
+{
+	return container_of_const(sd, struct vd56g3, sd);
+}
+
+static inline struct v4l2_subdev *ctrl_to_sd(struct v4l2_ctrl *ctrl)
+{
+	return &container_of_const(ctrl->handler, struct vd56g3, ctrl_handler)
+			->sd;
+}
+
+/* -----------------------------------------------------------------------------
+ * Additional i2c register helpers
+ */
+
+static int vd56g3_poll_reg(struct vd56g3 *sensor, u32 reg, u8 poll_val,
+			   int *err)
+{
+	unsigned int val = 0;
+	int ret;
+
+	if (err && *err)
+		return *err;
+
+	ret = regmap_read_poll_timeout(sensor->regmap, CCI_REG_ADDR(reg), val,
+				       (val == poll_val), 2000,
+				       500 * USEC_PER_MSEC);
+
+	if (ret && err)
+		*err = ret;
+
+	return ret;
+}
+
+static int vd56g3_wait_state(struct vd56g3 *sensor, int state, int *err)
+{
+	return vd56g3_poll_reg(sensor, VD56G3_REG_SYSTEM_FSM, state, err);
+}
+
+/* -----------------------------------------------------------------------------
+ * Controls: definitions, helpers and handlers
+ */
+
+static const char *const vd56g3_tp_menu[] = { "Disabled", "Solid", "Colorbar",
+					      "Gradbar",  "Hgrey", "Vgrey",
+					      "Dgrey",	  "PN28" };
+
+static const s64 vd56g3_ev_bias_qmenu[] = { -4000, -3500, -3000, -2500, -2000,
+					    -1500, -1000, -500,	 0,	500,
+					    1000,  1500,  2000,	 2500,	3000,
+					    3500,  4000 };
+
+static const s64 vd56g3_link_freq_1lane[] = { VD56G3_LINK_FREQ_DEF_1LANE };
+
+static const s64 vd56g3_link_freq_2lanes[] = { VD56G3_LINK_FREQ_DEF_2LANES };
+
+static u8 vd56g3_get_bpp(__u32 code)
+{
+	switch (code) {
+	case MEDIA_BUS_FMT_Y8_1X8:
+	case MEDIA_BUS_FMT_SGRBG8_1X8:
+	case MEDIA_BUS_FMT_SRGGB8_1X8:
+	case MEDIA_BUS_FMT_SBGGR8_1X8:
+	case MEDIA_BUS_FMT_SGBRG8_1X8:
+	default:
+		return 8;
+	case MEDIA_BUS_FMT_Y10_1X10:
+	case MEDIA_BUS_FMT_SGRBG10_1X10:
+	case MEDIA_BUS_FMT_SRGGB10_1X10:
+	case MEDIA_BUS_FMT_SBGGR10_1X10:
+	case MEDIA_BUS_FMT_SGBRG10_1X10:
+		return 10;
+	}
+}
+
+static u8 vd56g3_get_datatype(__u32 code)
+{
+	switch (code) {
+	case MEDIA_BUS_FMT_Y8_1X8:
+	case MEDIA_BUS_FMT_SGRBG8_1X8:
+	case MEDIA_BUS_FMT_SRGGB8_1X8:
+	case MEDIA_BUS_FMT_SBGGR8_1X8:
+	case MEDIA_BUS_FMT_SGBRG8_1X8:
+	default:
+		return MIPI_CSI2_DT_RAW8;
+	case MEDIA_BUS_FMT_Y10_1X10:
+	case MEDIA_BUS_FMT_SGRBG10_1X10:
+	case MEDIA_BUS_FMT_SRGGB10_1X10:
+	case MEDIA_BUS_FMT_SBGGR10_1X10:
+	case MEDIA_BUS_FMT_SGBRG10_1X10:
+		return MIPI_CSI2_DT_RAW10;
+	}
+}
+
+static int vd56g3_read_expo_cluster(struct vd56g3 *sensor, bool force_cur_val)
+{
+	u64 exposure = 0;
+	u64 again = 0;
+	u64 dgain = 0;
+	int ret = 0;
+
+	/*
+	 * When 'force_cur_val' is enabled, save the ctrl value in 'cur.val'
+	 * instead of the normal 'val', this is used during poweroff to cache
+	 * volatile ctrls and enable coldstart.
+	 */
+	cci_read(sensor->regmap, VD56G3_REG_APPLIED_COARSE_EXPOSURE, &exposure,
+		 &ret);
+	cci_read(sensor->regmap, VD56G3_REG_APPLIED_ANALOG_GAIN, &again, &ret);
+	cci_read(sensor->regmap, VD56G3_REG_APPLIED_DIGITAL_GAIN, &dgain, &ret);
+	if (ret)
+		return ret;
+
+	if (force_cur_val) {
+		sensor->expo_ctrl->cur.val = exposure;
+		sensor->again_ctrl->cur.val = again;
+		sensor->dgain_ctrl->cur.val = dgain;
+	} else {
+		sensor->expo_ctrl->val = exposure;
+		sensor->again_ctrl->val = again;
+		sensor->dgain_ctrl->val = dgain;
+	}
+
+	return ret;
+}
+
+static int vd56g3_update_patgen(struct vd56g3 *sensor, u32 patgen_index)
+{
+	u32 pattern = patgen_index <= 3 ? patgen_index : patgen_index + 12;
+	u16 patgen = pattern << VD56G3_PATGEN_TYPE_SHIFT;
+	u8 duster = VD56G3_DUSTER_ENABLE_DEF_MODULES;
+	u8 darkcal = VD56G3_DARKCAL_ENABLE;
+	int ret = 0;
+
+	if (patgen_index) {
+		patgen |= VD56G3_PATGEN_ENABLE;
+		duster = VD56G3_DUSTER_DISABLE;
+		darkcal = VD56G3_DARKCAL_DISABLE_DARKAVG;
+	}
+
+	cci_write(sensor->regmap, VD56G3_REG_DUSTER_CTRL, duster, &ret);
+	cci_write(sensor->regmap, VD56G3_REG_DARKCAL_CTRL, darkcal, &ret);
+	cci_write(sensor->regmap, VD56G3_REG_PATGEN_CTRL, patgen, &ret);
+
+	return ret;
+}
+
+static int vd56g3_update_expo_cluster(struct vd56g3 *sensor, bool is_auto)
+{
+	enum vd56g3_expo_state expo_state = is_auto ? VD56G3_EXP_MODE_AUTO :
+						      VD56G3_EXP_MODE_MANUAL;
+	int ret = 0;
+
+	if (sensor->ae_ctrl->is_new)
+		cci_write(sensor->regmap, VD56G3_REG_EXP_MODE, expo_state,
+			  &ret);
+
+	/* In Auto expo, set coldstart parameters */
+	if (is_auto && sensor->ae_ctrl->is_new) {
+		cci_write(sensor->regmap,
+			  VD56G3_REG_AE_COLDSTART_COARSE_EXPOSURE,
+			  sensor->expo_ctrl->val, &ret);
+		cci_write(sensor->regmap, VD56G3_REG_AE_COLDSTART_ANALOG_GAIN,
+			  sensor->again_ctrl->val, &ret);
+		cci_write(sensor->regmap, VD56G3_REG_AE_COLDSTART_DIGITAL_GAIN,
+			  sensor->dgain_ctrl->val, &ret);
+	}
+
+	/* In Manual expo, set exposure, analog and digital gains */
+	if (!is_auto && sensor->expo_ctrl->is_new)
+		cci_write(sensor->regmap, VD56G3_REG_MANUAL_COARSE_EXPOSURE,
+			  sensor->expo_ctrl->val, &ret);
+
+	if (!is_auto && sensor->again_ctrl->is_new)
+		cci_write(sensor->regmap, VD56G3_REG_MANUAL_ANALOG_GAIN,
+			  sensor->again_ctrl->val, &ret);
+
+	if (!is_auto && sensor->dgain_ctrl->is_new) {
+		cci_write(sensor->regmap, VD56G3_REG_MANUAL_DIGITAL_GAIN_CH0,
+			  sensor->dgain_ctrl->val, &ret);
+		cci_write(sensor->regmap, VD56G3_REG_MANUAL_DIGITAL_GAIN_CH1,
+			  sensor->dgain_ctrl->val, &ret);
+		cci_write(sensor->regmap, VD56G3_REG_MANUAL_DIGITAL_GAIN_CH2,
+			  sensor->dgain_ctrl->val, &ret);
+		cci_write(sensor->regmap, VD56G3_REG_MANUAL_DIGITAL_GAIN_CH3,
+			  sensor->dgain_ctrl->val, &ret);
+	}
+
+	return ret;
+}
+
+static int vd56g3_lock_exposure(struct vd56g3 *sensor, u32 lock_val)
+{
+	bool ae_lock = lock_val & V4L2_LOCK_EXPOSURE;
+	enum vd56g3_expo_state expo_state = ae_lock ? VD56G3_EXP_MODE_FREEZE :
+						      VD56G3_EXP_MODE_AUTO;
+	int ret = 0;
+
+	if (sensor->ae_ctrl->val == V4L2_EXPOSURE_AUTO)
+		cci_write(sensor->regmap, VD56G3_REG_EXP_MODE, expo_state,
+			  &ret);
+
+	return ret;
+}
+
+static int vd56g3_write_gpiox(struct vd56g3 *sensor, unsigned long gpio_mask)
+{
+	unsigned long io;
+	u32 gpio_val;
+	int ret = 0;
+
+	for_each_set_bit(io, &gpio_mask, VD56G3_NB_GPIOS) {
+		gpio_val = sensor->gpios[io];
+
+		if (gpio_val == VD56G3_GPIOX_STROBE_MODE &&
+		    sensor->led_ctrl->val == V4L2_FLASH_LED_MODE_NONE)
+			gpio_val = VD56G3_GPIOX_GPIO_IN;
+
+		cci_write(sensor->regmap, VD56G3_REG_GPIO_0_CTRL + io, gpio_val,
+			  &ret);
+	}
+
+	return ret;
+}
+
+static int vd56g3_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
+{
+	struct v4l2_subdev *sd = ctrl_to_sd(ctrl);
+	struct vd56g3 *sensor = to_vd56g3(sd);
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+	int ret = 0;
+
+	/* Interact with HW only when it is powered ON */
+	if (!pm_runtime_get_if_in_use(&client->dev))
+		return 0;
+
+	switch (ctrl->id) {
+	case V4L2_CID_EXPOSURE_AUTO:
+		ret = vd56g3_read_expo_cluster(sensor, false);
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+	pm_runtime_mark_last_busy(&client->dev);
+	pm_runtime_put_autosuspend(&client->dev);
+
+	return ret;
+}
+
+static int vd56g3_s_ctrl(struct v4l2_ctrl *ctrl)
+{
+	struct v4l2_subdev *sd = ctrl_to_sd(ctrl);
+	struct vd56g3 *sensor = to_vd56g3(sd);
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+	struct v4l2_subdev_state *state;
+	const struct v4l2_rect *crop;
+	unsigned int frame_length = 0;
+	unsigned int expo_max;
+	unsigned int ae_compensation;
+	bool is_auto = false;
+	int ret;
+
+	state = v4l2_subdev_get_locked_active_state(sd);
+	crop = v4l2_subdev_state_get_crop(state, 0);
+
+	if (ctrl->flags & V4L2_CTRL_FLAG_READ_ONLY)
+		return 0;
+
+	/* Update controls state, range, etc. whatever the state of the HW */
+	switch (ctrl->id) {
+	case V4L2_CID_VBLANK:
+		frame_length = crop->height + ctrl->val;
+		expo_max = frame_length - VD56G3_EXPOSURE_MARGIN;
+		__v4l2_ctrl_modify_range(sensor->expo_ctrl, 0, expo_max, 1,
+					 VD56G3_EXPOSURE_DEFAULT);
+		break;
+	case V4L2_CID_EXPOSURE_AUTO:
+		is_auto = (ctrl->val == V4L2_EXPOSURE_AUTO);
+		__v4l2_ctrl_grab(sensor->ae_lock_ctrl, !is_auto);
+		__v4l2_ctrl_grab(sensor->ae_bias_ctrl, !is_auto);
+		break;
+	default:
+		break;
+	}
+
+	/* Interact with HW only when it is powered ON */
+	if (!pm_runtime_get_if_in_use(&client->dev))
+		return 0;
+
+	switch (ctrl->id) {
+	case V4L2_CID_HFLIP:
+		ret = cci_write(sensor->regmap, VD56G3_REG_ORIENTATION,
+				sensor->hflip_ctrl->val |
+					(sensor->vflip_ctrl->val << 1),
+				NULL);
+		break;
+	case V4L2_CID_TEST_PATTERN:
+		ret = vd56g3_update_patgen(sensor, ctrl->val);
+		break;
+	case V4L2_CID_EXPOSURE_AUTO:
+		ret = vd56g3_update_expo_cluster(sensor, is_auto);
+		break;
+	case V4L2_CID_3A_LOCK:
+		ret = vd56g3_lock_exposure(sensor, ctrl->val);
+		break;
+	case V4L2_CID_AUTO_EXPOSURE_BIAS:
+		ae_compensation =
+			DIV_ROUND_CLOSEST((int)vd56g3_ev_bias_qmenu[ctrl->val] *
+					  256, 1000);
+		ret = cci_write(sensor->regmap, VD56G3_REG_AE_COMPENSATION,
+				ae_compensation, NULL);
+		break;
+	case V4L2_CID_VBLANK:
+		ret = cci_write(sensor->regmap, VD56G3_REG_FRAME_LENGTH,
+				frame_length, NULL);
+		break;
+	case V4L2_CID_FLASH_LED_MODE:
+		ret = vd56g3_write_gpiox(sensor, sensor->ext_leds_mask);
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+	pm_runtime_mark_last_busy(&client->dev);
+	pm_runtime_put_autosuspend(&client->dev);
+
+	return ret;
+}
+
+static const struct v4l2_ctrl_ops vd56g3_ctrl_ops = {
+	.g_volatile_ctrl = vd56g3_g_volatile_ctrl,
+	.s_ctrl = vd56g3_s_ctrl,
+};
+
+static int vd56g3_update_controls(struct vd56g3 *sensor)
+{
+	struct v4l2_subdev_state *state;
+	const struct v4l2_rect *crop;
+	unsigned int hblank;
+	unsigned int vblank_min, vblank, vblank_max;
+	unsigned int frame_length;
+	unsigned int expo_max;
+	int ret;
+
+	state = v4l2_subdev_get_locked_active_state(&sensor->sd);
+	crop = v4l2_subdev_state_get_crop(state, 0);
+	hblank = VD56G3_LINE_LENGTH_MIN - crop->width;
+	vblank_min = VD56G3_VBLANK_MIN;
+	vblank = VD56G3_FRAME_LENGTH_DEF_60FPS - crop->height;
+	vblank_max = 0xffff - crop->height;
+	frame_length = crop->height + vblank;
+	expo_max = frame_length - VD56G3_EXPOSURE_MARGIN;
+
+	/* Update blanking and exposure (ranges + values) */
+	ret = __v4l2_ctrl_modify_range(sensor->hblank_ctrl, hblank, hblank, 1,
+				       hblank);
+	if (ret)
+		return ret;
+
+	ret = __v4l2_ctrl_modify_range(sensor->vblank_ctrl, vblank_min,
+				       vblank_max, 1, vblank);
+	if (ret)
+		return ret;
+
+	ret = __v4l2_ctrl_s_ctrl(sensor->vblank_ctrl, vblank);
+	if (ret)
+		return ret;
+
+	ret = __v4l2_ctrl_modify_range(sensor->expo_ctrl, 0, expo_max, 1,
+				       VD56G3_EXPOSURE_DEFAULT);
+	if (ret)
+		return ret;
+
+	return __v4l2_ctrl_s_ctrl(sensor->expo_ctrl, VD56G3_EXPOSURE_DEFAULT);
+}
+
+static int vd56g3_init_controls(struct vd56g3 *sensor)
+{
+	const struct v4l2_ctrl_ops *ops = &vd56g3_ctrl_ops;
+	struct v4l2_ctrl_handler *hdl = &sensor->ctrl_handler;
+	struct v4l2_ctrl *ctrl;
+	int ret;
+
+	v4l2_ctrl_handler_init(hdl, 25);
+
+	/* Horizontal & vertical flips modify bayer code on RGB variant */
+	sensor->hflip_ctrl =
+		v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HFLIP, 0, 1, 1, 0);
+	if (sensor->hflip_ctrl)
+		sensor->hflip_ctrl->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
+
+	sensor->vflip_ctrl =
+		v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VFLIP, 0, 1, 1, 0);
+	if (sensor->vflip_ctrl)
+		sensor->vflip_ctrl->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
+
+	sensor->patgen_ctrl =
+		v4l2_ctrl_new_std_menu_items(hdl, ops, V4L2_CID_TEST_PATTERN,
+					     ARRAY_SIZE(vd56g3_tp_menu) - 1, 0,
+					     0, vd56g3_tp_menu);
+
+	ctrl = v4l2_ctrl_new_int_menu(hdl, ops, V4L2_CID_LINK_FREQ,
+				      ARRAY_SIZE(vd56g3_link_freq_1lane) - 1, 0,
+				      (sensor->nb_of_lane == 2) ?
+					      vd56g3_link_freq_2lanes :
+					      vd56g3_link_freq_1lane);
+	if (ctrl)
+		ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+
+	ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_PIXEL_RATE,
+				 sensor->pixel_clock, sensor->pixel_clock, 1,
+				 sensor->pixel_clock);
+	if (ctrl)
+		ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+
+	sensor->ae_ctrl = v4l2_ctrl_new_std_menu(hdl, ops,
+						 V4L2_CID_EXPOSURE_AUTO,
+						 V4L2_EXPOSURE_MANUAL, 0,
+						 V4L2_EXPOSURE_AUTO);
+
+	sensor->ae_lock_ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_3A_LOCK, 0,
+						 GENMASK(2, 0), 0, 0);
+
+	sensor->ae_bias_ctrl =
+		v4l2_ctrl_new_int_menu(hdl, ops, V4L2_CID_AUTO_EXPOSURE_BIAS,
+				       ARRAY_SIZE(vd56g3_ev_bias_qmenu) - 1,
+				       ARRAY_SIZE(vd56g3_ev_bias_qmenu) / 2,
+				       vd56g3_ev_bias_qmenu);
+
+	/*
+	 * Analog gain [1, 8] is computed with the following logic :
+	 * 32/(32 - again_reg), with again_reg in the range [0:28]
+	 * Digital gain [1.00, 8.00] is coded as a Fixed Point 5.8
+	 */
+	sensor->again_ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_ANALOGUE_GAIN,
+					       0, 28, 1, 0);
+	sensor->dgain_ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_DIGITAL_GAIN,
+					       0x100, 0x800, 1, 0x100);
+
+	/*
+	 * Set the exposure, horizontal and vertical blanking ctrls
+	 * to hardcoded values, they will be updated in vd56g3_update_controls.
+	 * Exposure being in an auto-cluster, set a significant value here.
+	 */
+	sensor->expo_ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_EXPOSURE,
+					      VD56G3_EXPOSURE_DEFAULT,
+					      VD56G3_EXPOSURE_DEFAULT, 1,
+					      VD56G3_EXPOSURE_DEFAULT);
+	sensor->hblank_ctrl =
+		v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HBLANK, 1, 1, 1, 1);
+	if (sensor->hblank_ctrl)
+		sensor->hblank_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+	sensor->vblank_ctrl =
+		v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VBLANK, 1, 1, 1, 1);
+
+	/* Additional control based on device tree properties */
+	if (sensor->ext_leds_mask)
+		sensor->led_ctrl =
+			v4l2_ctrl_new_std_menu(hdl, ops,
+					       V4L2_CID_FLASH_LED_MODE,
+					       V4L2_FLASH_LED_MODE_FLASH, 0,
+					       V4L2_FLASH_LED_MODE_NONE);
+
+	if (hdl->error) {
+		ret = hdl->error;
+		goto free_ctrls;
+	}
+
+	v4l2_ctrl_cluster(2, &sensor->hflip_ctrl);
+	v4l2_ctrl_auto_cluster(4, &sensor->ae_ctrl, V4L2_EXPOSURE_MANUAL, true);
+
+	sensor->sd.ctrl_handler = hdl;
+
+	return 0;
+
+free_ctrls:
+	v4l2_ctrl_handler_free(hdl);
+
+	return ret;
+}
+
+/* -----------------------------------------------------------------------------
+ * Videos ops
+ */
+
+static int vd56g3_stream_on(struct vd56g3 *sensor,
+			    struct v4l2_subdev_state *state)
+{
+	const struct v4l2_mbus_framefmt *format;
+	const struct v4l2_rect *crop;
+	unsigned int csi_mbps = ((sensor->nb_of_lane == 2) ?
+					 VD56G3_LINK_FREQ_DEF_2LANES :
+					 VD56G3_LINK_FREQ_DEF_1LANE) * 2 / MEGA;
+	unsigned int binning;
+	int ret = 0;
+
+	format = v4l2_subdev_state_get_format(state, 0);
+	crop = v4l2_subdev_state_get_crop(state, 0);
+
+	/* configure clocks */
+	cci_write(sensor->regmap, VD56G3_REG_EXT_CLOCK, sensor->xclk_freq,
+		  &ret);
+	cci_write(sensor->regmap, VD56G3_REG_CLK_PLL_PREDIV, sensor->pll_prediv,
+		  &ret);
+	cci_write(sensor->regmap, VD56G3_REG_CLK_SYS_PLL_MULT, sensor->pll_mult,
+		  &ret);
+
+	/* configure output */
+	cci_write(sensor->regmap, VD56G3_REG_FORMAT_CTRL,
+		  vd56g3_get_bpp(format->code), &ret);
+	cci_write(sensor->regmap, VD56G3_REG_OIF_CTRL, sensor->oif_ctrl, &ret);
+	cci_write(sensor->regmap, VD56G3_REG_OIF_CSI_BITRATE, csi_mbps, &ret);
+	cci_write(sensor->regmap, VD56G3_REG_OIF_IMG_CTRL,
+		  vd56g3_get_datatype(format->code), &ret);
+	cci_write(sensor->regmap, VD56G3_REG_ISL_ENABLE, 0, &ret);
+
+	/* configure binning mode */
+	switch (crop->width / format->width) {
+	case 1:
+	default:
+		binning = READOUT_NORMAL;
+		break;
+	case 2:
+		binning = READOUT_DIGITAL_BINNING_X2;
+		break;
+	}
+	cci_write(sensor->regmap, VD56G3_REG_READOUT_CTRL, binning, &ret);
+
+	/* configure ROIs */
+	cci_write(sensor->regmap, VD56G3_REG_Y_START, crop->top, &ret);
+	cci_write(sensor->regmap, VD56G3_REG_Y_END,
+		  crop->top + crop->height - 1, &ret);
+	cci_write(sensor->regmap, VD56G3_REG_OUT_ROI_X_START, crop->left, &ret);
+	cci_write(sensor->regmap, VD56G3_REG_OUT_ROI_X_END,
+		  crop->left + crop->width - 1, &ret);
+	cci_write(sensor->regmap, VD56G3_REG_OUT_ROI_Y_START, 0, &ret);
+	cci_write(sensor->regmap, VD56G3_REG_OUT_ROI_Y_END, crop->height - 1,
+		  &ret);
+	cci_write(sensor->regmap, VD56G3_REG_AE_ROI_START_H, crop->left, &ret);
+	cci_write(sensor->regmap, VD56G3_REG_AE_ROI_END_H,
+		  crop->left + crop->width - 1, &ret);
+	cci_write(sensor->regmap, VD56G3_REG_AE_ROI_START_V, 0, &ret);
+	cci_write(sensor->regmap, VD56G3_REG_AE_ROI_END_V, crop->height - 1,
+		  &ret);
+	if (ret)
+		return ret;
+
+	/* Setup default GPIO values; could be overridden by V4L2 ctrl setup */
+	ret = vd56g3_write_gpiox(sensor, GENMASK(VD56G3_NB_GPIOS - 1, 0));
+	if (ret)
+		return ret;
+
+	/* Apply settings from V4L2 ctrls */
+	ret = __v4l2_ctrl_handler_setup(&sensor->ctrl_handler);
+	if (ret)
+		return ret;
+
+	/* start streaming */
+	cci_write(sensor->regmap, VD56G3_REG_STBY, VD56G3_CMD_START_STREAM,
+		  &ret);
+	vd56g3_poll_reg(sensor, VD56G3_REG_STBY, VD56G3_CMD_ACK, &ret);
+	vd56g3_wait_state(sensor, VD56G3_SYSTEM_FSM_STREAMING, &ret);
+
+	return ret;
+}
+
+static int vd56g3_stream_off(struct vd56g3 *sensor)
+{
+	int ret;
+
+	/* Retrieve Expo cluster to enable coldstart of AE */
+	ret = vd56g3_read_expo_cluster(sensor, true);
+
+	cci_write(sensor->regmap, VD56G3_REG_STREAMING, VD56G3_CMD_STOP_STREAM,
+		  &ret);
+	vd56g3_poll_reg(sensor, VD56G3_REG_STREAMING, VD56G3_CMD_ACK, &ret);
+	vd56g3_wait_state(sensor, VD56G3_SYSTEM_FSM_SW_STBY, &ret);
+
+	return ret;
+}
+
+static int vd56g3_s_stream(struct v4l2_subdev *sd, int enable)
+{
+	struct vd56g3 *sensor = to_vd56g3(sd);
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+	struct v4l2_subdev_state *state;
+	int ret = 0;
+
+	state = v4l2_subdev_lock_and_get_active_state(sd);
+
+	if (enable) {
+		ret = pm_runtime_resume_and_get(&client->dev);
+		if (ret < 0)
+			goto unlock;
+		ret = vd56g3_stream_on(sensor, state);
+		if (ret) {
+			dev_err(&client->dev, "Failed to start streaming\n");
+			pm_runtime_put_sync(&client->dev);
+		}
+	} else {
+		vd56g3_stream_off(sensor);
+		pm_runtime_mark_last_busy(&client->dev);
+		pm_runtime_put_autosuspend(&client->dev);
+	}
+
+	if (!ret) {
+		sensor->streaming = enable;
+
+		/* some controls are locked during streaming */
+		__v4l2_ctrl_grab(sensor->hflip_ctrl, enable);
+		__v4l2_ctrl_grab(sensor->vflip_ctrl, enable);
+		__v4l2_ctrl_grab(sensor->patgen_ctrl, enable);
+	}
+
+unlock:
+	v4l2_subdev_unlock_state(state);
+
+	return ret;
+}
+
+static const struct v4l2_subdev_video_ops vd56g3_video_ops = {
+	.s_stream = vd56g3_s_stream,
+};
+
+/* -----------------------------------------------------------------------------
+ * Pad ops
+ */
+
+/* Media bus code is dependent of :
+ *      - 8bits or 10bits output
+ *      - variant : Mono or RGB
+ *      - H/V flips parameters in case of RGB
+ */
+static u32 vd56g3_get_mbus_code(struct vd56g3 *sensor, u32 code)
+{
+	unsigned int i_bpp;
+	unsigned int j;
+
+	for (i_bpp = 0; i_bpp < ARRAY_SIZE(vd56g3_mbus_codes); i_bpp++) {
+		for (j = 0; j < ARRAY_SIZE(vd56g3_mbus_codes[i_bpp]); j++) {
+			if (vd56g3_mbus_codes[i_bpp][j] == code)
+				goto endloops;
+		}
+	}
+
+endloops:
+	if (i_bpp >= ARRAY_SIZE(vd56g3_mbus_codes))
+		i_bpp = 0;
+
+	if (sensor->is_mono)
+		j = 0;
+	else
+		j = 1 + (sensor->hflip_ctrl->val ? 1 : 0) +
+		    (sensor->vflip_ctrl->val ? 2 : 0);
+
+	return vd56g3_mbus_codes[i_bpp][j];
+}
+
+static int vd56g3_enum_mbus_code(struct v4l2_subdev *sd,
+				 struct v4l2_subdev_state *sd_state,
+				 struct v4l2_subdev_mbus_code_enum *code)
+{
+	struct vd56g3 *sensor = to_vd56g3(sd);
+
+	if (code->index >= ARRAY_SIZE(vd56g3_mbus_codes))
+		return -EINVAL;
+
+	code->code =
+		vd56g3_get_mbus_code(sensor, vd56g3_mbus_codes[code->index][0]);
+
+	return 0;
+}
+
+static int vd56g3_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(vd56g3_supported_modes))
+		return -EINVAL;
+
+	fse->min_width = vd56g3_supported_modes[fse->index].width;
+	fse->max_width = fse->min_width;
+	fse->min_height = vd56g3_supported_modes[fse->index].height;
+	fse->max_height = fse->min_height;
+
+	return 0;
+}
+
+static void vd56g3_update_img_pad_format(struct vd56g3 *sensor,
+					 const struct vd56g3_mode *mode,
+					 u32 mbus_code,
+					 struct v4l2_mbus_framefmt *mbus_fmt)
+{
+	mbus_fmt->width = mode->width;
+	mbus_fmt->height = mode->height;
+	mbus_fmt->code = vd56g3_get_mbus_code(sensor, mbus_code);
+	mbus_fmt->colorspace = V4L2_COLORSPACE_RAW;
+	mbus_fmt->field = V4L2_FIELD_NONE;
+	mbus_fmt->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
+	mbus_fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
+	mbus_fmt->xfer_func = V4L2_XFER_FUNC_NONE;
+}
+
+static int vd56g3_set_pad_fmt(struct v4l2_subdev *sd,
+			      struct v4l2_subdev_state *sd_state,
+			      struct v4l2_subdev_format *sd_fmt)
+{
+	struct vd56g3 *sensor = to_vd56g3(sd);
+	const struct vd56g3_mode *new_mode;
+	struct v4l2_rect pad_crop;
+	unsigned int binning;
+	int ret = 0;
+
+	if (sensor->streaming)
+		return -EBUSY;
+
+	new_mode = v4l2_find_nearest_size(vd56g3_supported_modes,
+					  ARRAY_SIZE(vd56g3_supported_modes),
+					  width, height, sd_fmt->format.width,
+					  sd_fmt->format.height);
+
+	vd56g3_update_img_pad_format(sensor, new_mode, sd_fmt->format.code,
+				     &sd_fmt->format);
+
+	/* Compute crop rectangle (maximized via binning) */
+	binning = min(VD56G3_NATIVE_WIDTH / sd_fmt->format.width,
+		      VD56G3_NATIVE_HEIGHT / sd_fmt->format.height);
+	binning = min(binning, 2U);
+	pad_crop.width = sd_fmt->format.width * binning;
+	pad_crop.height = sd_fmt->format.height * binning;
+	pad_crop.left = (VD56G3_NATIVE_WIDTH - pad_crop.width) / 2;
+	pad_crop.top = (VD56G3_NATIVE_HEIGHT - pad_crop.height) / 2;
+
+	/* Update active state's format and crop */
+	if (sd_fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE)
+		ret = vd56g3_update_controls(sensor);
+
+	if (!ret) {
+		*v4l2_subdev_state_get_format(sd_state, sd_fmt->pad) =
+			sd_fmt->format;
+		*v4l2_subdev_state_get_crop(sd_state, sd_fmt->pad) = pad_crop;
+	}
+
+	return ret;
+}
+
+static int vd56g3_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, 0);
+		break;
+	case V4L2_SEL_TGT_NATIVE_SIZE:
+	case V4L2_SEL_TGT_CROP_DEFAULT:
+	case V4L2_SEL_TGT_CROP_BOUNDS:
+		sel->r.top = 0;
+		sel->r.left = 0;
+		sel->r.width = VD56G3_NATIVE_WIDTH;
+		sel->r.height = VD56G3_NATIVE_HEIGHT;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int vd56g3_init_state(struct v4l2_subdev *sd,
+			     struct v4l2_subdev_state *sd_state)
+{
+	unsigned int def_mode = VD56G3_DEFAULT_MODE;
+	struct v4l2_subdev_format fmt = {
+		.which = V4L2_SUBDEV_FORMAT_TRY,
+		.pad = 0,
+		.format = {
+			.code = vd56g3_mbus_codes[0][0],
+			.width = vd56g3_supported_modes[def_mode].width,
+			.height = vd56g3_supported_modes[def_mode].height,
+		},
+	};
+
+	vd56g3_set_pad_fmt(sd, sd_state, &fmt);
+	return 0;
+}
+
+static const struct v4l2_subdev_core_ops vd56g3_core_ops = {
+	.subscribe_event = v4l2_ctrl_subdev_subscribe_event,
+	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
+};
+
+static const struct v4l2_subdev_pad_ops vd56g3_pad_ops = {
+	.enum_mbus_code = vd56g3_enum_mbus_code,
+	.enum_frame_size = vd56g3_enum_frame_size,
+	.get_fmt = v4l2_subdev_get_fmt,
+	.set_fmt = vd56g3_set_pad_fmt,
+	.get_selection = vd56g3_get_selection,
+};
+
+static const struct v4l2_subdev_ops vd56g3_subdev_ops = {
+	.core = &vd56g3_core_ops,
+	.video = &vd56g3_video_ops,
+	.pad = &vd56g3_pad_ops,
+};
+
+static const struct media_entity_operations vd56g3_subdev_entity_ops = {
+	.link_validate = v4l2_subdev_link_validate,
+};
+
+static const struct v4l2_subdev_internal_ops vd56g3_internal_ops = {
+	.init_state = vd56g3_init_state,
+};
+
+/* -----------------------------------------------------------------------------
+ * Power management
+ */
+
+static int vd56g3_power_on(struct vd56g3 *sensor)
+{
+	struct i2c_client *client = sensor->i2c_client;
+	int ret;
+
+	/* power on */
+	ret = regulator_bulk_enable(ARRAY_SIZE(sensor->supplies),
+				    sensor->supplies);
+	if (ret) {
+		dev_err(&client->dev, "Failed to enable regulators %d", ret);
+		return ret;
+	}
+
+	ret = clk_prepare_enable(sensor->xclk);
+	if (ret) {
+		dev_err(&client->dev, "Failed to enable clock %d", ret);
+		goto disable_reg;
+	}
+
+	gpiod_set_value_cansleep(sensor->reset_gpio, 0);
+	usleep_range(3500, 4000);
+	ret = vd56g3_wait_state(sensor, VD56G3_SYSTEM_FSM_READY_TO_BOOT, NULL);
+	if (ret) {
+		dev_err(&client->dev, "Sensor reset failed %d\n", ret);
+		goto disable_clock;
+	}
+
+	/* boot sensor */
+	cci_write(sensor->regmap, VD56G3_REG_BOOT, VD56G3_CMD_BOOT, &ret);
+	vd56g3_poll_reg(sensor, VD56G3_REG_BOOT, VD56G3_CMD_ACK, &ret);
+	vd56g3_wait_state(sensor, VD56G3_SYSTEM_FSM_SW_STBY, &ret);
+	if (ret) {
+		dev_err(&client->dev, "sensor boot failed %d", ret);
+		goto disable_clock;
+	}
+
+	return 0;
+
+disable_clock:
+	clk_disable_unprepare(sensor->xclk);
+	gpiod_set_value_cansleep(sensor->reset_gpio, 1);
+disable_reg:
+	regulator_bulk_disable(ARRAY_SIZE(sensor->supplies), sensor->supplies);
+
+	return ret;
+}
+
+static int vd56g3_power_off(struct vd56g3 *sensor)
+{
+	clk_disable_unprepare(sensor->xclk);
+	gpiod_set_value_cansleep(sensor->reset_gpio, 1);
+	regulator_bulk_disable(ARRAY_SIZE(sensor->supplies), sensor->supplies);
+	return 0;
+}
+
+static int vd56g3_runtime_resume(struct device *dev)
+{
+	struct v4l2_subdev *sd = dev_get_drvdata(dev);
+	struct vd56g3 *sensor = to_vd56g3(sd);
+	struct i2c_client *client = sensor->i2c_client;
+	int ret;
+
+	ret = vd56g3_power_on(sensor);
+	if (ret)
+		dev_err(&client->dev, "Failed to power on %d", ret);
+
+	return ret;
+}
+
+static int vd56g3_runtime_suspend(struct device *dev)
+{
+	struct v4l2_subdev *sd = dev_get_drvdata(dev);
+	struct vd56g3 *sensor = to_vd56g3(sd);
+
+	return vd56g3_power_off(sensor);
+}
+
+static const struct dev_pm_ops vd56g3_pm_ops = {
+	SET_RUNTIME_PM_OPS(vd56g3_runtime_suspend, vd56g3_runtime_resume, NULL)
+};
+
+/* -----------------------------------------------------------------------------
+ * Probe and initialization
+ */
+
+static int vd56g3_check_csi_conf(struct vd56g3 *sensor,
+				 struct fwnode_handle *endpoint)
+{
+	struct i2c_client *client = sensor->i2c_client;
+	struct v4l2_fwnode_endpoint ep = { .bus_type = V4L2_MBUS_CSI2_DPHY };
+	u32 phy_data_lanes[VD56G3_MAX_CSI_DATA_LANES] = { ~0, ~0 };
+	u8 n_lanes;
+	u64 frequency;
+	int p, l;
+	int ret = 0;
+
+	ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &ep);
+	if (ret)
+		return -EINVAL;
+
+	/* Check lanes number */
+	n_lanes = ep.bus.mipi_csi2.num_data_lanes;
+	if (n_lanes != 1 && n_lanes != 2) {
+		dev_err(&client->dev, "Invalid data lane number %d\n", n_lanes);
+		ret = -EINVAL;
+		goto done;
+	}
+	sensor->nb_of_lane = n_lanes;
+
+	/* Clock lane must be first */
+	if (ep.bus.mipi_csi2.clock_lane != 0) {
+		dev_err(&client->dev, "Clk lane must be mapped to lane 0\n");
+		ret = -EINVAL;
+		goto done;
+	}
+
+	/* Prepare Output Interface conf based on lane settings
+	 * logical to physical lane conversion (+ pad remaining slots)
+	 */
+	for (l = 0; l < n_lanes; l++)
+		phy_data_lanes[ep.bus.mipi_csi2.data_lanes[l] - 1] = l;
+	for (p = 0; p < VD56G3_MAX_CSI_DATA_LANES; p++) {
+		if (phy_data_lanes[p] != ~0)
+			continue;
+		phy_data_lanes[p] = l;
+		l++;
+	}
+	sensor->oif_ctrl = n_lanes |
+			   (ep.bus.mipi_csi2.lane_polarities[0] << 3) |
+			   ((phy_data_lanes[0]) << 4) |
+			   (ep.bus.mipi_csi2.lane_polarities[1] << 6) |
+			   ((phy_data_lanes[1]) << 7) |
+			   (ep.bus.mipi_csi2.lane_polarities[2] << 9);
+
+	/* Check link frequency */
+	if (!ep.nr_of_link_frequencies) {
+		dev_err(&client->dev, "link-frequency not found in DT\n");
+		ret = -EINVAL;
+		goto done;
+	}
+	frequency = (n_lanes == 2) ? VD56G3_LINK_FREQ_DEF_2LANES :
+				     VD56G3_LINK_FREQ_DEF_1LANE;
+	if (ep.nr_of_link_frequencies != 1 ||
+	    ep.link_frequencies[0] != frequency) {
+		dev_err(&client->dev, "Link frequency not supported: %lld\n",
+			ep.link_frequencies[0]);
+		ret = -EINVAL;
+		goto done;
+	}
+
+done:
+	v4l2_fwnode_endpoint_free(&ep);
+	return ret;
+}
+
+static int vd56g3_parse_dt_gpios_array(struct vd56g3 *sensor, char *prop_name,
+				       u32 *array, int *nb)
+{
+	struct i2c_client *client = sensor->i2c_client;
+	struct device_node *np = client->dev.of_node;
+	unsigned int i;
+
+	*nb = of_property_read_variable_u32_array(np, prop_name, array, 0,
+						  VD56G3_NB_GPIOS);
+
+	if (*nb == -EINVAL) {
+		*nb = 0;
+		return *nb;
+	} else if (*nb < 0) {
+		dev_err(&client->dev, "Failed to read %s prop\n", prop_name);
+		return *nb;
+	}
+
+	for (i = 0; i < *nb; i++) {
+		if (array[i] >= VD56G3_NB_GPIOS) {
+			dev_err(&client->dev, "Invalid GPIO : %d\n", array[i]);
+			return -EINVAL;
+		}
+	}
+
+	return 0;
+}
+
+static int vd56g3_parse_dt_gpios(struct vd56g3 *sensor)
+{
+	u32 led_gpios[VD56G3_NB_GPIOS];
+	int nb_gpios_leds;
+	unsigned int i;
+	int ret;
+
+	/* Initialize GPIOs to default */
+	for (i = 0; i < VD56G3_NB_GPIOS; i++)
+		sensor->gpios[i] = VD56G3_GPIOX_GPIO_IN;
+	sensor->ext_leds_mask = 0;
+
+	/* Take into account optional 'st,leds' output for GPIOs */
+	ret = vd56g3_parse_dt_gpios_array(sensor, "st,leds", led_gpios,
+					  &nb_gpios_leds);
+	if (ret)
+		return ret;
+	for (i = 0; i < nb_gpios_leds; i++) {
+		sensor->gpios[led_gpios[i]] = VD56G3_GPIOX_STROBE_MODE;
+		set_bit(led_gpios[i], &sensor->ext_leds_mask);
+	}
+
+	return 0;
+}
+
+static int vd56g3_parse_dt(struct vd56g3 *sensor)
+{
+	struct i2c_client *client = sensor->i2c_client;
+	struct device *dev = &client->dev;
+	struct fwnode_handle *endpoint;
+	int ret;
+
+	endpoint = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), 0, 0, 0);
+	if (!endpoint) {
+		dev_err(dev, "endpoint node not found\n");
+		return -EINVAL;
+	}
+
+	ret = vd56g3_check_csi_conf(sensor, endpoint);
+	fwnode_handle_put(endpoint);
+	if (ret)
+		return ret;
+
+	return vd56g3_parse_dt_gpios(sensor);
+}
+
+static int vd56g3_get_regulators(struct vd56g3 *sensor)
+{
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(sensor->supplies); i++)
+		sensor->supplies[i].supply = vd56g3_supply_names[i];
+
+	return devm_regulator_bulk_get(&sensor->i2c_client->dev,
+				       ARRAY_SIZE(sensor->supplies),
+				       sensor->supplies);
+}
+
+static int vd56g3_prepare_clock_tree(struct vd56g3 *sensor)
+{
+	struct i2c_client *client = sensor->i2c_client;
+	const unsigned int predivs[] = { 1, 2, 4 };
+	u32 pll_out;
+	int i;
+
+	/* External clock must be in [6Mhz-27Mhz] */
+	if (sensor->xclk_freq < 6 * HZ_PER_MHZ ||
+	    sensor->xclk_freq > 27 * HZ_PER_MHZ) {
+		dev_err(&client->dev,
+			"Only 6Mhz-27Mhz clock range supported. Provided %lu MHz\n",
+			sensor->xclk_freq / HZ_PER_MHZ);
+		return -EINVAL;
+	}
+
+	/* PLL input should be in [6Mhz-12Mhz[ */
+	for (i = 0; i < ARRAY_SIZE(predivs); i++) {
+		sensor->pll_prediv = predivs[i];
+		if (sensor->xclk_freq / sensor->pll_prediv < 12 * HZ_PER_MHZ)
+			break;
+	}
+
+	/* PLL output clock must be as close as possible to 804Mhz */
+	sensor->pll_mult = (VD56G3_TARGET_PLL * sensor->pll_prediv +
+			    sensor->xclk_freq / 2) /
+			   sensor->xclk_freq;
+	pll_out = sensor->xclk_freq * sensor->pll_mult / sensor->pll_prediv;
+
+	/* Target Pixel Clock for standard 10bit ADC mode : 160.8Mhz */
+	sensor->pixel_clock = pll_out / VD56G3_VT_CLOCK_DIV;
+
+	return 0;
+}
+
+static int vd56g3_detect(struct vd56g3 *sensor)
+{
+	struct i2c_client *client = sensor->i2c_client;
+	struct device *dev = &client->dev;
+	unsigned int model;
+	u64 model_id = 0;
+	u64 device_revision = 0;
+	u64 optical_revision = 0;
+	int ret = 0;
+
+	model = (uintptr_t)device_get_match_data(dev);
+
+	cci_read(sensor->regmap, VD56G3_REG_MODEL_ID, &model_id, &ret);
+	if (ret)
+		return ret;
+
+	if (model_id != VD56G3_MODEL_ID) {
+		dev_err(&client->dev, "Unsupported sensor id %x",
+			(u16)model_id);
+		return -ENODEV;
+	}
+
+	cci_read(sensor->regmap, VD56G3_REG_REVISION, &device_revision, &ret);
+	if (ret)
+		return ret;
+
+	if ((device_revision >> 8) != VD56G3_REVISION_CUT3) {
+		dev_err(&client->dev, "Unsupported Cut version %x",
+			(u16)device_revision);
+		return -ENODEV;
+	}
+
+	cci_read(sensor->regmap, VD56G3_REG_OPTICAL_REVISION, &optical_revision,
+		 &ret);
+	if (ret)
+		return ret;
+
+	sensor->is_mono =
+		((optical_revision & 1) == VD56G3_OPTICAL_REVISION_MONO);
+	if ((sensor->is_mono && model == VD56G3_MODEL_VD66GY) ||
+	    (!sensor->is_mono && model == VD56G3_MODEL_VD56G3)) {
+		dev_err(&client->dev,
+			"Found %s sensor, while %s model is defined in DT",
+			(sensor->is_mono) ? "Mono" : "Bayer",
+			(model == VD56G3_MODEL_VD56G3) ? "vd56g3" : "vd66gy");
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
+static int vd56g3_subdev_init(struct vd56g3 *sensor)
+{
+	struct i2c_client *client = sensor->i2c_client;
+	int ret;
+
+	/* Init sub device */
+	v4l2_i2c_subdev_init(&sensor->sd, client, &vd56g3_subdev_ops);
+	sensor->sd.internal_ops = &vd56g3_internal_ops;
+	sensor->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
+			    V4L2_SUBDEV_FL_HAS_EVENTS;
+	sensor->sd.entity.ops = &vd56g3_subdev_entity_ops;
+
+	/* Init source pad */
+	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(&client->dev, "Failed to init media entity : %d", ret);
+		return ret;
+	}
+
+	/* Init controls */
+	ret = vd56g3_init_controls(sensor);
+	if (ret) {
+		dev_err(&client->dev, "Controls initialization failed %d", ret);
+		goto err_media;
+	}
+
+	/* Init vd56g3 struct : default resolution + raw8 */
+	sensor->sd.state_lock = sensor->ctrl_handler.lock;
+	ret = v4l2_subdev_init_finalize(&sensor->sd);
+	if (ret) {
+		dev_err(&client->dev, "subdev init error: %d", ret);
+		goto err_ctrls;
+	}
+
+	return vd56g3_update_controls(sensor);
+
+err_ctrls:
+	v4l2_ctrl_handler_free(sensor->sd.ctrl_handler);
+
+err_media:
+	media_entity_cleanup(&sensor->sd.entity);
+	return ret;
+}
+
+static void vd56g3_subdev_cleanup(struct vd56g3 *sensor)
+{
+	v4l2_async_unregister_subdev(&sensor->sd);
+	v4l2_subdev_cleanup(&sensor->sd);
+	media_entity_cleanup(&sensor->sd.entity);
+	v4l2_ctrl_handler_free(sensor->sd.ctrl_handler);
+}
+
+static int vd56g3_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct vd56g3 *sensor;
+	int ret;
+
+	sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL);
+	if (!sensor)
+		return -ENOMEM;
+
+	sensor->i2c_client = client;
+
+	ret = vd56g3_parse_dt(sensor);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to parse Device Tree.");
+
+	/* Get (and check) resources : power regs, ext clock, reset gpio */
+	ret = vd56g3_get_regulators(sensor);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to get regulators.");
+
+	sensor->xclk = devm_clk_get(dev, NULL);
+	if (IS_ERR(sensor->xclk))
+		return dev_err_probe(dev, PTR_ERR(sensor->xclk),
+				     "Failed to get xclk.");
+	sensor->xclk_freq = clk_get_rate(sensor->xclk);
+	ret = vd56g3_prepare_clock_tree(sensor);
+	if (ret)
+		return ret;
+
+	sensor->reset_gpio =
+		devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(sensor->reset_gpio))
+		return dev_err_probe(dev, PTR_ERR(sensor->reset_gpio),
+				     "Failed to get reset gpio.");
+
+	sensor->regmap = devm_cci_regmap_init_i2c(client, 16);
+	if (IS_ERR(sensor->regmap))
+		return dev_err_probe(dev, PTR_ERR(sensor->regmap),
+				     "Failed to init regmap.");
+
+	/* Power ON */
+	ret = vd56g3_power_on(sensor);
+	if (ret)
+		return dev_err_probe(dev, ret, "Sensor power on failed.");
+
+	/* Enable PM runtime with autosuspend (sensor being ON, set active) */
+	pm_runtime_set_active(dev);
+	pm_runtime_get_noresume(dev);
+	pm_runtime_enable(dev);
+	pm_runtime_set_autosuspend_delay(dev, 1000);
+	pm_runtime_use_autosuspend(dev);
+
+	/* Check HW model/version */
+	ret = vd56g3_detect(sensor);
+	if (ret) {
+		dev_err(&client->dev, "Sensor detect failed : %d", ret);
+		goto err_power_off;
+	}
+
+	/* Initialize, then register V4L2 subdev */
+	ret = vd56g3_subdev_init(sensor);
+	if (ret) {
+		dev_err(&client->dev, "V4l2 init failed : %d", ret);
+		goto err_power_off;
+	}
+
+	ret = v4l2_async_register_subdev(&sensor->sd);
+	if (ret) {
+		dev_err(&client->dev, "async subdev register failed %d", ret);
+		goto err_subdev;
+	}
+
+	/* Sensor could now be powered off (after the autosuspend delay) */
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
+
+	dev_dbg(&client->dev, "Successfully probe %s sensor",
+		(sensor->is_mono) ? "vd56g3" : "vd66gy");
+
+	return 0;
+
+err_subdev:
+	vd56g3_subdev_cleanup(sensor);
+err_power_off:
+	pm_runtime_disable(dev);
+	pm_runtime_put_noidle(dev);
+	vd56g3_power_off(sensor);
+	return ret;
+}
+
+static void vd56g3_remove(struct i2c_client *client)
+{
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct vd56g3 *sensor = to_vd56g3(sd);
+
+	vd56g3_subdev_cleanup(sensor);
+
+	pm_runtime_disable(&client->dev);
+	if (!pm_runtime_status_suspended(&client->dev))
+		vd56g3_power_off(sensor);
+	pm_runtime_set_suspended(&client->dev);
+}
+
+static const struct of_device_id vd56g3_dt_ids[] = {
+	{ .compatible = "st,st-vd56g3", .data = (void *)VD56G3_MODEL_VD56G3 },
+	{ .compatible = "st,st-vd66gy", .data = (void *)VD56G3_MODEL_VD66GY },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, vd56g3_dt_ids);
+
+static struct i2c_driver vd56g3_i2c_driver = {
+	.driver = {
+		.name  = "st-vd56g3",
+		.of_match_table = vd56g3_dt_ids,
+		.pm = &vd56g3_pm_ops,
+	},
+	.probe = vd56g3_probe,
+	.remove = vd56g3_remove,
+};
+
+module_i2c_driver(vd56g3_i2c_driver);
+
+MODULE_AUTHOR("Benjamin Mugnier <benjamin.mugnier@foss.st.com>");
+MODULE_AUTHOR("Mickael Guene <mickael.guene@st.com>");
+MODULE_AUTHOR("Sylvain Petinot <sylvain.petinot@foss.st.com>");
+MODULE_DESCRIPTION("ST VD56G3 sensor driver");
+MODULE_LICENSE("GPL");
-- 
2.17.1

Re: [PATCH v2 2/2] media: i2c: Add driver for ST VD56G3 camera sensor

[Sakari Ailus]

Hi Sylvain,

Thanks for the update. A few more comments below...

On Tue, May 21, 2024 at 06:29:50PM +0200, Sylvain Petinot wrote:
> Add V4L2 sub-device driver for STMicroelectronics VD56G3 camera sensor.
> This is a 1.5 M pixel global shutter image sensor with an active array
> size of 1124 x 1364 (portrait orientation).
> 
> The driver supports Mono (VD56G3) and Color (VD66GY) variants.
> 
> Signed-off-by: Sylvain Petinot <sylvain.petinot@foss.st.com>
> ---
>  drivers/media/i2c/Kconfig     |   11 +
>  drivers/media/i2c/Makefile    |    1 +
>  drivers/media/i2c/st-vd56g3.c | 1608 +++++++++++++++++++++++++++++++++
>  3 files changed, 1620 insertions(+)
>  create mode 100644 drivers/media/i2c/st-vd56g3.c
> 
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index c6d3ee472d81..c3d3cbdfcb3b 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -659,6 +659,17 @@ config VIDEO_S5K6A3
>  	  This is a V4L2 sensor driver for Samsung S5K6A3 raw
>  	  camera sensor.
>  
> +config VIDEO_ST_VD56G3
> +	tristate "ST VD56G3 sensor support"
> +	select V4L2_CCI_I2C
> +	depends on OF && GPIOLIB
> +	help
> +	  This is a Video4Linux2 sensor driver for the ST VD56G3
> +	  camera sensor.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called st-vd56g3.
> +
>  config VIDEO_ST_VGXY61
>  	tristate "ST VGXY61 sensor support"
>  	select V4L2_CCI_I2C
> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> index dfbe6448b549..b469cf0f8113 100644
> --- a/drivers/media/i2c/Makefile
> +++ b/drivers/media/i2c/Makefile
> @@ -124,6 +124,7 @@ obj-$(CONFIG_VIDEO_SAA717X) += saa717x.o
>  obj-$(CONFIG_VIDEO_SAA7185) += saa7185.o
>  obj-$(CONFIG_VIDEO_SONY_BTF_MPX) += sony-btf-mpx.o
>  obj-$(CONFIG_VIDEO_ST_MIPID02) += st-mipid02.o
> +obj-$(CONFIG_VIDEO_ST_VD56G3) += st-vd56g3.o
>  obj-$(CONFIG_VIDEO_ST_VGXY61) += st-vgxy61.o
>  obj-$(CONFIG_VIDEO_TC358743) += tc358743.o
>  obj-$(CONFIG_VIDEO_TC358746) += tc358746.o
> diff --git a/drivers/media/i2c/st-vd56g3.c b/drivers/media/i2c/st-vd56g3.c
> new file mode 100644
> index 000000000000..1e4329502230
> --- /dev/null
> +++ b/drivers/media/i2c/st-vd56g3.c
> @@ -0,0 +1,1608 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * A V4L2 driver for ST VD56G3 (Mono) and VD66GY (RGB) global shutter cameras.
> + * Copyright (C) 2024, STMicroelectronics SA
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/iopoll.h>
> +#include <linux/module.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/units.h>
> +
> +#include <asm/unaligned.h>
> +
> +#include <media/mipi-csi2.h>
> +#include <media/v4l2-async.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>
> +#include <media/v4l2-subdev.h>
> +
> +/* Register Map */
> +#define VD56G3_REG_MODEL_ID				CCI_REG16_LE(0x0000)
> +#define VD56G3_MODEL_ID					0x5603
> +#define VD56G3_REG_REVISION				CCI_REG16_LE(0x0002)
> +#define VD56G3_REVISION_CUT3				0x31
> +#define VD56G3_REG_OPTICAL_REVISION			CCI_REG8(0x001a)
> +#define VD56G3_OPTICAL_REVISION_MONO			0
> +#define VD56G3_OPTICAL_REVISION_BAYER			1
> +#define VD56G3_REG_SYSTEM_FSM				CCI_REG8(0x0028)
> +#define VD56G3_SYSTEM_FSM_READY_TO_BOOT			0x01
> +#define VD56G3_SYSTEM_FSM_SW_STBY			0x02
> +#define VD56G3_SYSTEM_FSM_STREAMING			0x03
> +#define VD56G3_REG_APPLIED_COARSE_EXPOSURE		CCI_REG16_LE(0x0064)
> +#define VD56G3_REG_APPLIED_ANALOG_GAIN			CCI_REG8(0x0068)
> +#define VD56G3_REG_APPLIED_DIGITAL_GAIN			CCI_REG16_LE(0x006a)
> +#define VD56G3_REG_BOOT					CCI_REG8(0x0200)
> +#define VD56G3_CMD_ACK					0
> +#define VD56G3_CMD_BOOT					1
> +#define VD56G3_REG_STBY					CCI_REG8(0x0201)
> +#define VD56G3_CMD_START_STREAM				1
> +#define VD56G3_REG_STREAMING				CCI_REG8(0x0202)
> +#define VD56G3_CMD_STOP_STREAM				1
> +#define VD56G3_REG_EXT_CLOCK				CCI_REG32_LE(0x0220)
> +#define VD56G3_REG_CLK_PLL_PREDIV			CCI_REG8(0x0224)
> +#define VD56G3_REG_CLK_SYS_PLL_MULT			CCI_REG8(0x0226)
> +#define VD56G3_REG_ORIENTATION				CCI_REG8(0x0302)
> +#define VD56G3_REG_FORMAT_CTRL				CCI_REG8(0x030a)
> +#define VD56G3_REG_OIF_CTRL				CCI_REG16_LE(0x030c)
> +#define VD56G3_REG_OIF_IMG_CTRL				CCI_REG8(0x030f)
> +#define VD56G3_REG_OIF_CSI_BITRATE			CCI_REG16_LE(0x0312)
> +#define VD56G3_REG_DUSTER_CTRL				CCI_REG8(0x0318)
> +#define VD56G3_DUSTER_DISABLE				0
> +#define VD56G3_DUSTER_ENABLE_DEF_MODULES		0x13
> +#define VD56G3_REG_ISL_ENABLE				CCI_REG8(0x0333)
> +#define VD56G3_REG_DARKCAL_CTRL				CCI_REG8(0x0340)
> +#define VD56G3_DARKCAL_ENABLE				1
> +#define VD56G3_DARKCAL_DISABLE_DARKAVG			2
> +#define VD56G3_REG_PATGEN_CTRL				CCI_REG16_LE(0x0400)
> +#define VD56G3_PATGEN_ENABLE				1
> +#define VD56G3_PATGEN_TYPE_SHIFT			4
> +#define VD56G3_REG_AE_COLDSTART_COARSE_EXPOSURE		CCI_REG16_LE(0x042a)
> +#define VD56G3_REG_AE_COLDSTART_ANALOG_GAIN		CCI_REG8(0x042c)
> +#define VD56G3_REG_AE_COLDSTART_DIGITAL_GAIN		CCI_REG16_LE(0x042e)
> +#define VD56G3_REG_AE_ROI_START_H			CCI_REG16_LE(0x0432)
> +#define VD56G3_REG_AE_ROI_START_V			CCI_REG16_LE(0x0434)
> +#define VD56G3_REG_AE_ROI_END_H				CCI_REG16_LE(0x0436)
> +#define VD56G3_REG_AE_ROI_END_V				CCI_REG16_LE(0x0438)
> +#define VD56G3_REG_AE_COMPENSATION			CCI_REG16_LE(0x043a)
> +#define VD56G3_REG_EXP_MODE				CCI_REG8(0x044c)
> +#define VD56G3_EXP_MODE_AUTO				0
> +#define VD56G3_EXP_MODE_FREEZE				1
> +#define VD56G3_EXP_MODE_MANUAL				2
> +#define VD56G3_REG_MANUAL_ANALOG_GAIN			CCI_REG8(0x044d)
> +#define VD56G3_REG_MANUAL_COARSE_EXPOSURE		CCI_REG16_LE(0x044e)
> +#define VD56G3_REG_MANUAL_DIGITAL_GAIN_CH0		CCI_REG16_LE(0x0450)
> +#define VD56G3_REG_MANUAL_DIGITAL_GAIN_CH1		CCI_REG16_LE(0x0452)
> +#define VD56G3_REG_MANUAL_DIGITAL_GAIN_CH2		CCI_REG16_LE(0x0454)
> +#define VD56G3_REG_MANUAL_DIGITAL_GAIN_CH3		CCI_REG16_LE(0x0456)
> +#define VD56G3_REG_FRAME_LENGTH				CCI_REG16_LE(0x0458)
> +#define VD56G3_REG_Y_START				CCI_REG16_LE(0x045a)
> +#define VD56G3_REG_Y_END				CCI_REG16_LE(0x045c)
> +#define VD56G3_REG_OUT_ROI_X_START			CCI_REG16_LE(0x045e)
> +#define VD56G3_REG_OUT_ROI_X_END			CCI_REG16_LE(0x0460)
> +#define VD56G3_REG_OUT_ROI_Y_START			CCI_REG16_LE(0x0462)
> +#define VD56G3_REG_OUT_ROI_Y_END			CCI_REG16_LE(0x0464)
> +#define VD56G3_REG_GPIO_0_CTRL				CCI_REG8(0x0467)
> +#define VD56G3_GPIOX_GPIO_IN				0x01
> +#define VD56G3_GPIOX_STROBE_MODE			0x02
> +#define VD56G3_REG_READOUT_CTRL				CCI_REG8(0x047e)
> +#define READOUT_NORMAL					0x00
> +#define READOUT_DIGITAL_BINNING_X2			0x01
> +
> +/*
> + * The VD56G3 pixel array is organized as follows:
> + *
> + * +--------------------------------+
> + * |                                | \
> + * |   +------------------------+   |  |
> + * |   |                        |   |  |
> + * |   |                        |   |  |
> + * |   |                        |   |  |
> + * |   |                        |   |  |
> + * |   |                        |   |  |
> + * |   |   Default resolution   |   |  | Native height (1364)

What's outside the default resolution? It doesn't appear the driver would
allow capturing pixels out side this area.

> + * |   |      1120 x 1360       |   |  |
> + * |   |                        |   |  |
> + * |   |                        |   |  |
> + * |   |                        |   |  |
> + * |   |                        |   |  |
> + * |   +------------------------+   |  |
> + * |                                | /
> + * +--------------------------------+
> + *   <----------------------------->
> + *                     \-------------------  Native width (1124)
> + *
> + * The native resolution is 1124x1364.
> + * The recommended/default resolution is 1120x1360 (multiple of 16).
> + */
> +#define VD56G3_NATIVE_WIDTH				1124
> +#define VD56G3_NATIVE_HEIGHT				1364
> +#define VD56G3_DEFAULT_WIDTH				1120
> +#define VD56G3_DEFAULT_HEIGHT				1360
> +#define VD56G3_DEFAULT_MODE				1
> +
> +/* PLL settings */
> +#define VD56G3_TARGET_PLL				804000000UL
> +#define VD56G3_VT_CLOCK_DIV				5
> +
> +/* Line length and Frame length (settings are for standard 10bits ADC mode) */
> +#define VD56G3_LINE_LENGTH_MIN				1236
> +#define VD56G3_VBLANK_MIN				110
> +#define VD56G3_FRAME_LENGTH_DEF_60FPS			2168
> +
> +/* Exposure settings */
> +#define VD56G3_EXPOSURE_MARGIN				75
> +#define VD56G3_EXPOSURE_DEFAULT				1420
> +
> +/* Output Interface settings */
> +#define VD56G3_MAX_CSI_DATA_LANES			2
> +#define VD56G3_LINK_FREQ_DEF_1LANE			750000000UL
> +#define VD56G3_LINK_FREQ_DEF_2LANES			402000000UL
> +
> +/* GPIOs */
> +#define VD56G3_NB_GPIOS					8
> +
> +/* regulator supplies */
> +static const char *const vd56g3_supply_names[] = {
> +	"vcore",
> +	"vddio",
> +	"vana",
> +};
> +
> +/* -----------------------------------------------------------------------------
> + * Models (VD56G3: Mono, VD66GY: Bayer RGB), Modes and formats
> + */
> +
> +enum vd56g3_models {
> +	VD56G3_MODEL_VD56G3,
> +	VD56G3_MODEL_VD66GY,
> +};
> +
> +struct vd56g3_mode {
> +	u32 width;
> +	u32 height;
> +};
> +
> +static const struct vd56g3_mode vd56g3_supported_modes[] = {
> +	{
> +		.width = VD56G3_NATIVE_WIDTH,
> +		.height = VD56G3_NATIVE_HEIGHT,
> +	},
> +	{
> +		.width = VD56G3_DEFAULT_WIDTH,
> +		.height = VD56G3_DEFAULT_HEIGHT,
> +	},
> +	{
> +		.width = 1024,
> +		.height = 1280,
> +	},
> +	{
> +		.width = 1024,
> +		.height = 768,
> +	},
> +	{
> +		.width = 768,
> +		.height = 1024,
> +	},
> +	{
> +		.width = 720,
> +		.height = 1280,
> +	},
> +	{
> +		.width = 640,
> +		.height = 480,
> +	},
> +	{
> +		.width = 480,
> +		.height = 640,
> +	},
> +	{
> +		.width = 320,
> +		.height = 240,
> +	},
> +};
> +
> +/*
> + * Sensor support 8bits and 10bits output in both variants
> + *  - Monochrome
> + *  - RGB (with all H/V flip variations)
> + */
> +static const unsigned int vd56g3_mbus_codes[2][5] = {
> +	{
> +		MEDIA_BUS_FMT_Y8_1X8,
> +		MEDIA_BUS_FMT_SGRBG8_1X8,
> +		MEDIA_BUS_FMT_SRGGB8_1X8,
> +		MEDIA_BUS_FMT_SBGGR8_1X8,
> +		MEDIA_BUS_FMT_SGBRG8_1X8,
> +	},
> +	{
> +		MEDIA_BUS_FMT_Y10_1X10,
> +		MEDIA_BUS_FMT_SGRBG10_1X10,
> +		MEDIA_BUS_FMT_SRGGB10_1X10,
> +		MEDIA_BUS_FMT_SBGGR10_1X10,
> +		MEDIA_BUS_FMT_SGBRG10_1X10,
> +	},
> +};
> +
> +enum vd56g3_expo_state {
> +	VD56G3_EXPO_AUTO,
> +	VD56G3_EXPO_AUTO_FREEZE,
> +	VD56G3_EXPO_MANUAL
> +};
> +
> +struct vd56g3 {
> +	struct i2c_client *i2c_client;
> +	struct v4l2_subdev sd;
> +	struct media_pad pad;
> +	struct regulator_bulk_data supplies[ARRAY_SIZE(vd56g3_supply_names)];
> +	struct gpio_desc *reset_gpio;
> +	struct clk *xclk;
> +	struct regmap *regmap;
> +	u32 xclk_freq;
> +	u32 pll_prediv;
> +	u32 pll_mult;
> +	u32 pixel_clock;
> +	u16 oif_ctrl;
> +	u8 nb_of_lane;
> +	u32 gpios[VD56G3_NB_GPIOS];
> +	unsigned long ext_leds_mask;
> +	bool is_mono;
> +	struct v4l2_ctrl_handler ctrl_handler;
> +	struct v4l2_ctrl *hblank_ctrl;
> +	struct v4l2_ctrl *vblank_ctrl;
> +	struct {
> +		struct v4l2_ctrl *hflip_ctrl;
> +		struct v4l2_ctrl *vflip_ctrl;
> +	};
> +	struct v4l2_ctrl *patgen_ctrl;
> +	struct {
> +		struct v4l2_ctrl *ae_ctrl;
> +		struct v4l2_ctrl *expo_ctrl;
> +		struct v4l2_ctrl *again_ctrl;
> +		struct v4l2_ctrl *dgain_ctrl;
> +	};
> +	struct v4l2_ctrl *ae_lock_ctrl;
> +	struct v4l2_ctrl *ae_bias_ctrl;
> +	struct v4l2_ctrl *led_ctrl;
> +	bool streaming;
> +};
> +
> +static inline struct vd56g3 *to_vd56g3(struct v4l2_subdev *sd)
> +{
> +	return container_of_const(sd, struct vd56g3, sd);
> +}
> +
> +static inline struct v4l2_subdev *ctrl_to_sd(struct v4l2_ctrl *ctrl)
> +{
> +	return &container_of_const(ctrl->handler, struct vd56g3, ctrl_handler)
> +			->sd;
> +}
> +
> +/* -----------------------------------------------------------------------------
> + * Additional i2c register helpers
> + */
> +
> +static int vd56g3_poll_reg(struct vd56g3 *sensor, u32 reg, u8 poll_val,
> +			   int *err)
> +{
> +	unsigned int val = 0;
> +	int ret;
> +
> +	if (err && *err)
> +		return *err;
> +
> +	ret = regmap_read_poll_timeout(sensor->regmap, CCI_REG_ADDR(reg), val,
> +				       (val == poll_val), 2000,
> +				       500 * USEC_PER_MSEC);
> +
> +	if (ret && err)
> +		*err = ret;
> +
> +	return ret;
> +}
> +
> +static int vd56g3_wait_state(struct vd56g3 *sensor, int state, int *err)
> +{
> +	return vd56g3_poll_reg(sensor, VD56G3_REG_SYSTEM_FSM, state, err);
> +}
> +
> +/* -----------------------------------------------------------------------------
> + * Controls: definitions, helpers and handlers
> + */
> +
> +static const char *const vd56g3_tp_menu[] = { "Disabled", "Solid", "Colorbar",
> +					      "Gradbar",  "Hgrey", "Vgrey",
> +					      "Dgrey",	  "PN28" };
> +
> +static const s64 vd56g3_ev_bias_qmenu[] = { -4000, -3500, -3000, -2500, -2000,
> +					    -1500, -1000, -500,	 0,	500,
> +					    1000,  1500,  2000,	 2500,	3000,
> +					    3500,  4000 };
> +
> +static const s64 vd56g3_link_freq_1lane[] = { VD56G3_LINK_FREQ_DEF_1LANE };
> +
> +static const s64 vd56g3_link_freq_2lanes[] = { VD56G3_LINK_FREQ_DEF_2LANES };
> +
> +static u8 vd56g3_get_bpp(__u32 code)
> +{
> +	switch (code) {
> +	case MEDIA_BUS_FMT_Y8_1X8:
> +	case MEDIA_BUS_FMT_SGRBG8_1X8:
> +	case MEDIA_BUS_FMT_SRGGB8_1X8:
> +	case MEDIA_BUS_FMT_SBGGR8_1X8:
> +	case MEDIA_BUS_FMT_SGBRG8_1X8:
> +	default:
> +		return 8;
> +	case MEDIA_BUS_FMT_Y10_1X10:
> +	case MEDIA_BUS_FMT_SGRBG10_1X10:
> +	case MEDIA_BUS_FMT_SRGGB10_1X10:
> +	case MEDIA_BUS_FMT_SBGGR10_1X10:
> +	case MEDIA_BUS_FMT_SGBRG10_1X10:
> +		return 10;
> +	}
> +}
> +
> +static u8 vd56g3_get_datatype(__u32 code)
> +{
> +	switch (code) {
> +	case MEDIA_BUS_FMT_Y8_1X8:
> +	case MEDIA_BUS_FMT_SGRBG8_1X8:
> +	case MEDIA_BUS_FMT_SRGGB8_1X8:
> +	case MEDIA_BUS_FMT_SBGGR8_1X8:
> +	case MEDIA_BUS_FMT_SGBRG8_1X8:
> +	default:
> +		return MIPI_CSI2_DT_RAW8;
> +	case MEDIA_BUS_FMT_Y10_1X10:
> +	case MEDIA_BUS_FMT_SGRBG10_1X10:
> +	case MEDIA_BUS_FMT_SRGGB10_1X10:
> +	case MEDIA_BUS_FMT_SBGGR10_1X10:
> +	case MEDIA_BUS_FMT_SGBRG10_1X10:
> +		return MIPI_CSI2_DT_RAW10;
> +	}
> +}
> +
> +static int vd56g3_read_expo_cluster(struct vd56g3 *sensor, bool force_cur_val)
> +{
> +	u64 exposure = 0;
> +	u64 again = 0;
> +	u64 dgain = 0;
> +	int ret = 0;
> +
> +	/*
> +	 * When 'force_cur_val' is enabled, save the ctrl value in 'cur.val'
> +	 * instead of the normal 'val', this is used during poweroff to cache
> +	 * volatile ctrls and enable coldstart.
> +	 */
> +	cci_read(sensor->regmap, VD56G3_REG_APPLIED_COARSE_EXPOSURE, &exposure,
> +		 &ret);
> +	cci_read(sensor->regmap, VD56G3_REG_APPLIED_ANALOG_GAIN, &again, &ret);
> +	cci_read(sensor->regmap, VD56G3_REG_APPLIED_DIGITAL_GAIN, &dgain, &ret);
> +	if (ret)
> +		return ret;
> +
> +	if (force_cur_val) {
> +		sensor->expo_ctrl->cur.val = exposure;
> +		sensor->again_ctrl->cur.val = again;
> +		sensor->dgain_ctrl->cur.val = dgain;
> +	} else {
> +		sensor->expo_ctrl->val = exposure;
> +		sensor->again_ctrl->val = again;
> +		sensor->dgain_ctrl->val = dgain;
> +	}
> +
> +	return ret;
> +}
> +
> +static int vd56g3_update_patgen(struct vd56g3 *sensor, u32 patgen_index)
> +{
> +	u32 pattern = patgen_index <= 3 ? patgen_index : patgen_index + 12;
> +	u16 patgen = pattern << VD56G3_PATGEN_TYPE_SHIFT;
> +	u8 duster = VD56G3_DUSTER_ENABLE_DEF_MODULES;
> +	u8 darkcal = VD56G3_DARKCAL_ENABLE;
> +	int ret = 0;
> +
> +	if (patgen_index) {
> +		patgen |= VD56G3_PATGEN_ENABLE;
> +		duster = VD56G3_DUSTER_DISABLE;
> +		darkcal = VD56G3_DARKCAL_DISABLE_DARKAVG;
> +	}
> +
> +	cci_write(sensor->regmap, VD56G3_REG_DUSTER_CTRL, duster, &ret);
> +	cci_write(sensor->regmap, VD56G3_REG_DARKCAL_CTRL, darkcal, &ret);
> +	cci_write(sensor->regmap, VD56G3_REG_PATGEN_CTRL, patgen, &ret);
> +
> +	return ret;
> +}
> +
> +static int vd56g3_update_expo_cluster(struct vd56g3 *sensor, bool is_auto)
> +{
> +	enum vd56g3_expo_state expo_state = is_auto ? VD56G3_EXP_MODE_AUTO :
> +						      VD56G3_EXP_MODE_MANUAL;
> +	int ret = 0;
> +
> +	if (sensor->ae_ctrl->is_new)
> +		cci_write(sensor->regmap, VD56G3_REG_EXP_MODE, expo_state,
> +			  &ret);
> +
> +	/* In Auto expo, set coldstart parameters */
> +	if (is_auto && sensor->ae_ctrl->is_new) {
> +		cci_write(sensor->regmap,
> +			  VD56G3_REG_AE_COLDSTART_COARSE_EXPOSURE,
> +			  sensor->expo_ctrl->val, &ret);
> +		cci_write(sensor->regmap, VD56G3_REG_AE_COLDSTART_ANALOG_GAIN,
> +			  sensor->again_ctrl->val, &ret);
> +		cci_write(sensor->regmap, VD56G3_REG_AE_COLDSTART_DIGITAL_GAIN,
> +			  sensor->dgain_ctrl->val, &ret);
> +	}
> +
> +	/* In Manual expo, set exposure, analog and digital gains */
> +	if (!is_auto && sensor->expo_ctrl->is_new)
> +		cci_write(sensor->regmap, VD56G3_REG_MANUAL_COARSE_EXPOSURE,
> +			  sensor->expo_ctrl->val, &ret);
> +
> +	if (!is_auto && sensor->again_ctrl->is_new)
> +		cci_write(sensor->regmap, VD56G3_REG_MANUAL_ANALOG_GAIN,
> +			  sensor->again_ctrl->val, &ret);
> +
> +	if (!is_auto && sensor->dgain_ctrl->is_new) {
> +		cci_write(sensor->regmap, VD56G3_REG_MANUAL_DIGITAL_GAIN_CH0,
> +			  sensor->dgain_ctrl->val, &ret);
> +		cci_write(sensor->regmap, VD56G3_REG_MANUAL_DIGITAL_GAIN_CH1,
> +			  sensor->dgain_ctrl->val, &ret);
> +		cci_write(sensor->regmap, VD56G3_REG_MANUAL_DIGITAL_GAIN_CH2,
> +			  sensor->dgain_ctrl->val, &ret);
> +		cci_write(sensor->regmap, VD56G3_REG_MANUAL_DIGITAL_GAIN_CH3,
> +			  sensor->dgain_ctrl->val, &ret);
> +	}
> +
> +	return ret;
> +}
> +
> +static int vd56g3_lock_exposure(struct vd56g3 *sensor, u32 lock_val)
> +{
> +	bool ae_lock = lock_val & V4L2_LOCK_EXPOSURE;
> +	enum vd56g3_expo_state expo_state = ae_lock ? VD56G3_EXP_MODE_FREEZE :
> +						      VD56G3_EXP_MODE_AUTO;
> +	int ret = 0;
> +
> +	if (sensor->ae_ctrl->val == V4L2_EXPOSURE_AUTO)
> +		cci_write(sensor->regmap, VD56G3_REG_EXP_MODE, expo_state,

You can return cci_write's return value here.

> +			  &ret);
> +
> +	return ret;

And return 0 here.

> +}
> +
> +static int vd56g3_write_gpiox(struct vd56g3 *sensor, unsigned long gpio_mask)
> +{
> +	unsigned long io;
> +	u32 gpio_val;
> +	int ret = 0;
> +
> +	for_each_set_bit(io, &gpio_mask, VD56G3_NB_GPIOS) {
> +		gpio_val = sensor->gpios[io];
> +
> +		if (gpio_val == VD56G3_GPIOX_STROBE_MODE &&
> +		    sensor->led_ctrl->val == V4L2_FLASH_LED_MODE_NONE)
> +			gpio_val = VD56G3_GPIOX_GPIO_IN;
> +
> +		cci_write(sensor->regmap, VD56G3_REG_GPIO_0_CTRL + io, gpio_val,
> +			  &ret);
> +	}
> +
> +	return ret;
> +}
> +
> +static int vd56g3_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct v4l2_subdev *sd = ctrl_to_sd(ctrl);
> +	struct vd56g3 *sensor = to_vd56g3(sd);
> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
> +	int ret = 0;
> +
> +	/* Interact with HW only when it is powered ON */
> +	if (!pm_runtime_get_if_in_use(&client->dev))
> +		return 0;
> +
> +	switch (ctrl->id) {
> +	case V4L2_CID_EXPOSURE_AUTO:
> +		ret = vd56g3_read_expo_cluster(sensor, false);
> +		break;
> +	default:
> +		ret = -EINVAL;
> +		break;
> +	}
> +
> +	pm_runtime_mark_last_busy(&client->dev);
> +	pm_runtime_put_autosuspend(&client->dev);

This should be __pm_runtime_put_autosuspend(). pm_runtime_put_autosuspend()
will be re-purposed soon to include marking the last busy timestamp.

Same elsewhere in the driver.

> +
> +	return ret;
> +}
> +
> +static int vd56g3_s_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct v4l2_subdev *sd = ctrl_to_sd(ctrl);
> +	struct vd56g3 *sensor = to_vd56g3(sd);
> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
> +	struct v4l2_subdev_state *state;
> +	const struct v4l2_rect *crop;
> +	unsigned int frame_length = 0;
> +	unsigned int expo_max;
> +	unsigned int ae_compensation;
> +	bool is_auto = false;
> +	int ret;
> +
> +	state = v4l2_subdev_get_locked_active_state(sd);
> +	crop = v4l2_subdev_state_get_crop(state, 0);
> +
> +	if (ctrl->flags & V4L2_CTRL_FLAG_READ_ONLY)
> +		return 0;
> +
> +	/* Update controls state, range, etc. whatever the state of the HW */
> +	switch (ctrl->id) {
> +	case V4L2_CID_VBLANK:
> +		frame_length = crop->height + ctrl->val;
> +		expo_max = frame_length - VD56G3_EXPOSURE_MARGIN;
> +		__v4l2_ctrl_modify_range(sensor->expo_ctrl, 0, expo_max, 1,
> +					 VD56G3_EXPOSURE_DEFAULT);

__v4l2_ctrl_modify_range() may fail. Please handle the possible error.

> +		break;
> +	case V4L2_CID_EXPOSURE_AUTO:
> +		is_auto = (ctrl->val == V4L2_EXPOSURE_AUTO);
> +		__v4l2_ctrl_grab(sensor->ae_lock_ctrl, !is_auto);
> +		__v4l2_ctrl_grab(sensor->ae_bias_ctrl, !is_auto);
> +		break;
> +	default:
> +		break;

You could omit default here.

> +	}
> +
> +	/* Interact with HW only when it is powered ON */
> +	if (!pm_runtime_get_if_in_use(&client->dev))
> +		return 0;
> +
> +	switch (ctrl->id) {
> +	case V4L2_CID_HFLIP:
> +		ret = cci_write(sensor->regmap, VD56G3_REG_ORIENTATION,
> +				sensor->hflip_ctrl->val |
> +					(sensor->vflip_ctrl->val << 1),
> +				NULL);
> +		break;
> +	case V4L2_CID_TEST_PATTERN:
> +		ret = vd56g3_update_patgen(sensor, ctrl->val);
> +		break;
> +	case V4L2_CID_EXPOSURE_AUTO:
> +		ret = vd56g3_update_expo_cluster(sensor, is_auto);
> +		break;
> +	case V4L2_CID_3A_LOCK:
> +		ret = vd56g3_lock_exposure(sensor, ctrl->val);
> +		break;
> +	case V4L2_CID_AUTO_EXPOSURE_BIAS:
> +		ae_compensation =
> +			DIV_ROUND_CLOSEST((int)vd56g3_ev_bias_qmenu[ctrl->val] *
> +					  256, 1000);
> +		ret = cci_write(sensor->regmap, VD56G3_REG_AE_COMPENSATION,
> +				ae_compensation, NULL);
> +		break;
> +	case V4L2_CID_VBLANK:
> +		ret = cci_write(sensor->regmap, VD56G3_REG_FRAME_LENGTH,
> +				frame_length, NULL);
> +		break;
> +	case V4L2_CID_FLASH_LED_MODE:
> +		ret = vd56g3_write_gpiox(sensor, sensor->ext_leds_mask);
> +		break;
> +	default:
> +		ret = -EINVAL;
> +		break;
> +	}
> +
> +	pm_runtime_mark_last_busy(&client->dev);
> +	pm_runtime_put_autosuspend(&client->dev);
> +
> +	return ret;
> +}
> +
> +static const struct v4l2_ctrl_ops vd56g3_ctrl_ops = {
> +	.g_volatile_ctrl = vd56g3_g_volatile_ctrl,
> +	.s_ctrl = vd56g3_s_ctrl,
> +};
> +
> +static int vd56g3_update_controls(struct vd56g3 *sensor)
> +{
> +	struct v4l2_subdev_state *state;
> +	const struct v4l2_rect *crop;
> +	unsigned int hblank;
> +	unsigned int vblank_min, vblank, vblank_max;
> +	unsigned int frame_length;
> +	unsigned int expo_max;
> +	int ret;
> +
> +	state = v4l2_subdev_get_locked_active_state(&sensor->sd);
> +	crop = v4l2_subdev_state_get_crop(state, 0);
> +	hblank = VD56G3_LINE_LENGTH_MIN - crop->width;
> +	vblank_min = VD56G3_VBLANK_MIN;
> +	vblank = VD56G3_FRAME_LENGTH_DEF_60FPS - crop->height;
> +	vblank_max = 0xffff - crop->height;

How about adding a corresponding FRAME_LENGTH_MAX macro for 0xffff?

> +	frame_length = crop->height + vblank;
> +	expo_max = frame_length - VD56G3_EXPOSURE_MARGIN;
> +
> +	/* Update blanking and exposure (ranges + values) */
> +	ret = __v4l2_ctrl_modify_range(sensor->hblank_ctrl, hblank, hblank, 1,
> +				       hblank);
> +	if (ret)
> +		return ret;
> +
> +	ret = __v4l2_ctrl_modify_range(sensor->vblank_ctrl, vblank_min,
> +				       vblank_max, 1, vblank);
> +	if (ret)
> +		return ret;
> +
> +	ret = __v4l2_ctrl_s_ctrl(sensor->vblank_ctrl, vblank);
> +	if (ret)
> +		return ret;
> +
> +	ret = __v4l2_ctrl_modify_range(sensor->expo_ctrl, 0, expo_max, 1,
> +				       VD56G3_EXPOSURE_DEFAULT);
> +	if (ret)
> +		return ret;
> +
> +	return __v4l2_ctrl_s_ctrl(sensor->expo_ctrl, VD56G3_EXPOSURE_DEFAULT);
> +}
> +
> +static int vd56g3_init_controls(struct vd56g3 *sensor)
> +{
> +	const struct v4l2_ctrl_ops *ops = &vd56g3_ctrl_ops;
> +	struct v4l2_ctrl_handler *hdl = &sensor->ctrl_handler;
> +	struct v4l2_ctrl *ctrl;
> +	int ret;
> +
> +	v4l2_ctrl_handler_init(hdl, 25);
> +
> +	/* Horizontal & vertical flips modify bayer code on RGB variant */
> +	sensor->hflip_ctrl =
> +		v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HFLIP, 0, 1, 1, 0);
> +	if (sensor->hflip_ctrl)
> +		sensor->hflip_ctrl->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> +
> +	sensor->vflip_ctrl =
> +		v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VFLIP, 0, 1, 1, 0);
> +	if (sensor->vflip_ctrl)
> +		sensor->vflip_ctrl->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> +
> +	sensor->patgen_ctrl =
> +		v4l2_ctrl_new_std_menu_items(hdl, ops, V4L2_CID_TEST_PATTERN,
> +					     ARRAY_SIZE(vd56g3_tp_menu) - 1, 0,
> +					     0, vd56g3_tp_menu);
> +
> +	ctrl = v4l2_ctrl_new_int_menu(hdl, ops, V4L2_CID_LINK_FREQ,
> +				      ARRAY_SIZE(vd56g3_link_freq_1lane) - 1, 0,
> +				      (sensor->nb_of_lane == 2) ?
> +					      vd56g3_link_freq_2lanes :
> +					      vd56g3_link_freq_1lane);
> +	if (ctrl)
> +		ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +
> +	ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_PIXEL_RATE,
> +				 sensor->pixel_clock, sensor->pixel_clock, 1,
> +				 sensor->pixel_clock);
> +	if (ctrl)
> +		ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +
> +	sensor->ae_ctrl = v4l2_ctrl_new_std_menu(hdl, ops,
> +						 V4L2_CID_EXPOSURE_AUTO,
> +						 V4L2_EXPOSURE_MANUAL, 0,
> +						 V4L2_EXPOSURE_AUTO);
> +
> +	sensor->ae_lock_ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_3A_LOCK, 0,
> +						 GENMASK(2, 0), 0, 0);
> +
> +	sensor->ae_bias_ctrl =
> +		v4l2_ctrl_new_int_menu(hdl, ops, V4L2_CID_AUTO_EXPOSURE_BIAS,
> +				       ARRAY_SIZE(vd56g3_ev_bias_qmenu) - 1,
> +				       ARRAY_SIZE(vd56g3_ev_bias_qmenu) / 2,
> +				       vd56g3_ev_bias_qmenu);
> +
> +	/*
> +	 * Analog gain [1, 8] is computed with the following logic :
> +	 * 32/(32 - again_reg), with again_reg in the range [0:28]
> +	 * Digital gain [1.00, 8.00] is coded as a Fixed Point 5.8
> +	 */
> +	sensor->again_ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_ANALOGUE_GAIN,
> +					       0, 28, 1, 0);
> +	sensor->dgain_ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_DIGITAL_GAIN,
> +					       0x100, 0x800, 1, 0x100);
> +
> +	/*
> +	 * Set the exposure, horizontal and vertical blanking ctrls
> +	 * to hardcoded values, they will be updated in vd56g3_update_controls.
> +	 * Exposure being in an auto-cluster, set a significant value here.
> +	 */
> +	sensor->expo_ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_EXPOSURE,
> +					      VD56G3_EXPOSURE_DEFAULT,
> +					      VD56G3_EXPOSURE_DEFAULT, 1,
> +					      VD56G3_EXPOSURE_DEFAULT);
> +	sensor->hblank_ctrl =
> +		v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HBLANK, 1, 1, 1, 1);
> +	if (sensor->hblank_ctrl)
> +		sensor->hblank_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +	sensor->vblank_ctrl =
> +		v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VBLANK, 1, 1, 1, 1);
> +
> +	/* Additional control based on device tree properties */
> +	if (sensor->ext_leds_mask)
> +		sensor->led_ctrl =
> +			v4l2_ctrl_new_std_menu(hdl, ops,
> +					       V4L2_CID_FLASH_LED_MODE,
> +					       V4L2_FLASH_LED_MODE_FLASH, 0,
> +					       V4L2_FLASH_LED_MODE_NONE);
> +
> +	if (hdl->error) {
> +		ret = hdl->error;
> +		goto free_ctrls;
> +	}
> +
> +	v4l2_ctrl_cluster(2, &sensor->hflip_ctrl);
> +	v4l2_ctrl_auto_cluster(4, &sensor->ae_ctrl, V4L2_EXPOSURE_MANUAL, true);
> +
> +	sensor->sd.ctrl_handler = hdl;
> +
> +	return 0;
> +
> +free_ctrls:
> +	v4l2_ctrl_handler_free(hdl);
> +
> +	return ret;
> +}
> +
> +/* -----------------------------------------------------------------------------
> + * Videos ops
> + */
> +
> +static int vd56g3_stream_on(struct vd56g3 *sensor,
> +			    struct v4l2_subdev_state *state)
> +{
> +	const struct v4l2_mbus_framefmt *format;
> +	const struct v4l2_rect *crop;
> +	unsigned int csi_mbps = ((sensor->nb_of_lane == 2) ?
> +					 VD56G3_LINK_FREQ_DEF_2LANES :
> +					 VD56G3_LINK_FREQ_DEF_1LANE) * 2 / MEGA;
> +	unsigned int binning;
> +	int ret = 0;
> +
> +	format = v4l2_subdev_state_get_format(state, 0);
> +	crop = v4l2_subdev_state_get_crop(state, 0);

You could do the assignments in variable declaration.

> +
> +	/* configure clocks */
> +	cci_write(sensor->regmap, VD56G3_REG_EXT_CLOCK, sensor->xclk_freq,
> +		  &ret);
> +	cci_write(sensor->regmap, VD56G3_REG_CLK_PLL_PREDIV, sensor->pll_prediv,
> +		  &ret);
> +	cci_write(sensor->regmap, VD56G3_REG_CLK_SYS_PLL_MULT, sensor->pll_mult,
> +		  &ret);
> +
> +	/* configure output */
> +	cci_write(sensor->regmap, VD56G3_REG_FORMAT_CTRL,
> +		  vd56g3_get_bpp(format->code), &ret);
> +	cci_write(sensor->regmap, VD56G3_REG_OIF_CTRL, sensor->oif_ctrl, &ret);
> +	cci_write(sensor->regmap, VD56G3_REG_OIF_CSI_BITRATE, csi_mbps, &ret);
> +	cci_write(sensor->regmap, VD56G3_REG_OIF_IMG_CTRL,
> +		  vd56g3_get_datatype(format->code), &ret);
> +	cci_write(sensor->regmap, VD56G3_REG_ISL_ENABLE, 0, &ret);
> +
> +	/* configure binning mode */
> +	switch (crop->width / format->width) {
> +	case 1:
> +	default:
> +		binning = READOUT_NORMAL;
> +		break;
> +	case 2:
> +		binning = READOUT_DIGITAL_BINNING_X2;
> +		break;
> +	}
> +	cci_write(sensor->regmap, VD56G3_REG_READOUT_CTRL, binning, &ret);
> +
> +	/* configure ROIs */
> +	cci_write(sensor->regmap, VD56G3_REG_Y_START, crop->top, &ret);
> +	cci_write(sensor->regmap, VD56G3_REG_Y_END,
> +		  crop->top + crop->height - 1, &ret);
> +	cci_write(sensor->regmap, VD56G3_REG_OUT_ROI_X_START, crop->left, &ret);
> +	cci_write(sensor->regmap, VD56G3_REG_OUT_ROI_X_END,
> +		  crop->left + crop->width - 1, &ret);
> +	cci_write(sensor->regmap, VD56G3_REG_OUT_ROI_Y_START, 0, &ret);
> +	cci_write(sensor->regmap, VD56G3_REG_OUT_ROI_Y_END, crop->height - 1,
> +		  &ret);
> +	cci_write(sensor->regmap, VD56G3_REG_AE_ROI_START_H, crop->left, &ret);
> +	cci_write(sensor->regmap, VD56G3_REG_AE_ROI_END_H,
> +		  crop->left + crop->width - 1, &ret);
> +	cci_write(sensor->regmap, VD56G3_REG_AE_ROI_START_V, 0, &ret);
> +	cci_write(sensor->regmap, VD56G3_REG_AE_ROI_END_V, crop->height - 1,
> +		  &ret);
> +	if (ret)
> +		return ret;
> +
> +	/* Setup default GPIO values; could be overridden by V4L2 ctrl setup */
> +	ret = vd56g3_write_gpiox(sensor, GENMASK(VD56G3_NB_GPIOS - 1, 0));
> +	if (ret)
> +		return ret;
> +
> +	/* Apply settings from V4L2 ctrls */
> +	ret = __v4l2_ctrl_handler_setup(&sensor->ctrl_handler);
> +	if (ret)
> +		return ret;
> +
> +	/* start streaming */
> +	cci_write(sensor->regmap, VD56G3_REG_STBY, VD56G3_CMD_START_STREAM,
> +		  &ret);
> +	vd56g3_poll_reg(sensor, VD56G3_REG_STBY, VD56G3_CMD_ACK, &ret);
> +	vd56g3_wait_state(sensor, VD56G3_SYSTEM_FSM_STREAMING, &ret);
> +
> +	return ret;
> +}
> +
> +static int vd56g3_stream_off(struct vd56g3 *sensor)
> +{
> +	int ret;
> +
> +	/* Retrieve Expo cluster to enable coldstart of AE */
> +	ret = vd56g3_read_expo_cluster(sensor, true);
> +
> +	cci_write(sensor->regmap, VD56G3_REG_STREAMING, VD56G3_CMD_STOP_STREAM,
> +		  &ret);
> +	vd56g3_poll_reg(sensor, VD56G3_REG_STREAMING, VD56G3_CMD_ACK, &ret);
> +	vd56g3_wait_state(sensor, VD56G3_SYSTEM_FSM_SW_STBY, &ret);
> +
> +	return ret;
> +}
> +
> +static int vd56g3_s_stream(struct v4l2_subdev *sd, int enable)
> +{
> +	struct vd56g3 *sensor = to_vd56g3(sd);
> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
> +	struct v4l2_subdev_state *state;
> +	int ret = 0;
> +
> +	state = v4l2_subdev_lock_and_get_active_state(sd);
> +
> +	if (enable) {
> +		ret = pm_runtime_resume_and_get(&client->dev);
> +		if (ret < 0)
> +			goto unlock;
> +		ret = vd56g3_stream_on(sensor, state);
> +		if (ret) {
> +			dev_err(&client->dev, "Failed to start streaming\n");
> +			pm_runtime_put_sync(&client->dev);
> +		}
> +	} else {
> +		vd56g3_stream_off(sensor);
> +		pm_runtime_mark_last_busy(&client->dev);
> +		pm_runtime_put_autosuspend(&client->dev);
> +	}
> +
> +	if (!ret) {
> +		sensor->streaming = enable;
> +
> +		/* some controls are locked during streaming */
> +		__v4l2_ctrl_grab(sensor->hflip_ctrl, enable);
> +		__v4l2_ctrl_grab(sensor->vflip_ctrl, enable);
> +		__v4l2_ctrl_grab(sensor->patgen_ctrl, enable);
> +	}
> +
> +unlock:
> +	v4l2_subdev_unlock_state(state);
> +
> +	return ret;
> +}
> +
> +static const struct v4l2_subdev_video_ops vd56g3_video_ops = {
> +	.s_stream = vd56g3_s_stream,
> +};
> +
> +/* -----------------------------------------------------------------------------
> + * Pad ops
> + */
> +
> +/* Media bus code is dependent of :
> + *      - 8bits or 10bits output
> + *      - variant : Mono or RGB
> + *      - H/V flips parameters in case of RGB
> + */
> +static u32 vd56g3_get_mbus_code(struct vd56g3 *sensor, u32 code)
> +{
> +	unsigned int i_bpp;
> +	unsigned int j;
> +
> +	for (i_bpp = 0; i_bpp < ARRAY_SIZE(vd56g3_mbus_codes); i_bpp++) {
> +		for (j = 0; j < ARRAY_SIZE(vd56g3_mbus_codes[i_bpp]); j++) {
> +			if (vd56g3_mbus_codes[i_bpp][j] == code)
> +				goto endloops;
> +		}
> +	}
> +
> +endloops:
> +	if (i_bpp >= ARRAY_SIZE(vd56g3_mbus_codes))
> +		i_bpp = 0;
> +
> +	if (sensor->is_mono)
> +		j = 0;
> +	else
> +		j = 1 + (sensor->hflip_ctrl->val ? 1 : 0) +
> +		    (sensor->vflip_ctrl->val ? 2 : 0);
> +
> +	return vd56g3_mbus_codes[i_bpp][j];
> +}
> +
> +static int vd56g3_enum_mbus_code(struct v4l2_subdev *sd,
> +				 struct v4l2_subdev_state *sd_state,
> +				 struct v4l2_subdev_mbus_code_enum *code)
> +{
> +	struct vd56g3 *sensor = to_vd56g3(sd);
> +
> +	if (code->index >= ARRAY_SIZE(vd56g3_mbus_codes))
> +		return -EINVAL;
> +
> +	code->code =
> +		vd56g3_get_mbus_code(sensor, vd56g3_mbus_codes[code->index][0]);
> +
> +	return 0;
> +}
> +
> +static int vd56g3_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(vd56g3_supported_modes))
> +		return -EINVAL;
> +
> +	fse->min_width = vd56g3_supported_modes[fse->index].width;
> +	fse->max_width = fse->min_width;
> +	fse->min_height = vd56g3_supported_modes[fse->index].height;
> +	fse->max_height = fse->min_height;
> +
> +	return 0;
> +}
> +
> +static void vd56g3_update_img_pad_format(struct vd56g3 *sensor,
> +					 const struct vd56g3_mode *mode,
> +					 u32 mbus_code,
> +					 struct v4l2_mbus_framefmt *mbus_fmt)
> +{
> +	mbus_fmt->width = mode->width;
> +	mbus_fmt->height = mode->height;
> +	mbus_fmt->code = vd56g3_get_mbus_code(sensor, mbus_code);
> +	mbus_fmt->colorspace = V4L2_COLORSPACE_RAW;
> +	mbus_fmt->field = V4L2_FIELD_NONE;
> +	mbus_fmt->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> +	mbus_fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> +	mbus_fmt->xfer_func = V4L2_XFER_FUNC_NONE;
> +}
> +
> +static int vd56g3_set_pad_fmt(struct v4l2_subdev *sd,
> +			      struct v4l2_subdev_state *sd_state,
> +			      struct v4l2_subdev_format *sd_fmt)
> +{
> +	struct vd56g3 *sensor = to_vd56g3(sd);
> +	const struct vd56g3_mode *new_mode;
> +	struct v4l2_rect pad_crop;
> +	unsigned int binning;
> +	int ret = 0;
> +
> +	if (sensor->streaming)
> +		return -EBUSY;

Setting the try format should be possible during streaming.

> +
> +	new_mode = v4l2_find_nearest_size(vd56g3_supported_modes,
> +					  ARRAY_SIZE(vd56g3_supported_modes),
> +					  width, height, sd_fmt->format.width,
> +					  sd_fmt->format.height);
> +
> +	vd56g3_update_img_pad_format(sensor, new_mode, sd_fmt->format.code,
> +				     &sd_fmt->format);
> +
> +	/* Compute crop rectangle (maximized via binning) */
> +	binning = min(VD56G3_NATIVE_WIDTH / sd_fmt->format.width,
> +		      VD56G3_NATIVE_HEIGHT / sd_fmt->format.height);
> +	binning = min(binning, 2U);
> +	pad_crop.width = sd_fmt->format.width * binning;
> +	pad_crop.height = sd_fmt->format.height * binning;
> +	pad_crop.left = (VD56G3_NATIVE_WIDTH - pad_crop.width) / 2;
> +	pad_crop.top = (VD56G3_NATIVE_HEIGHT - pad_crop.height) / 2;
> +
> +	/* Update active state's format and crop */
> +	if (sd_fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> +		ret = vd56g3_update_controls(sensor);
> +
> +	if (!ret) {
> +		*v4l2_subdev_state_get_format(sd_state, sd_fmt->pad) =
> +			sd_fmt->format;
> +		*v4l2_subdev_state_get_crop(sd_state, sd_fmt->pad) = pad_crop;
> +	}
> +
> +	return ret;
> +}
> +
> +static int vd56g3_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, 0);
> +		break;
> +	case V4L2_SEL_TGT_NATIVE_SIZE:
> +	case V4L2_SEL_TGT_CROP_DEFAULT:
> +	case V4L2_SEL_TGT_CROP_BOUNDS:
> +		sel->r.top = 0;
> +		sel->r.left = 0;
> +		sel->r.width = VD56G3_NATIVE_WIDTH;
> +		sel->r.height = VD56G3_NATIVE_HEIGHT;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int vd56g3_init_state(struct v4l2_subdev *sd,
> +			     struct v4l2_subdev_state *sd_state)
> +{
> +	unsigned int def_mode = VD56G3_DEFAULT_MODE;
> +	struct v4l2_subdev_format fmt = {
> +		.which = V4L2_SUBDEV_FORMAT_TRY,
> +		.pad = 0,
> +		.format = {
> +			.code = vd56g3_mbus_codes[0][0],
> +			.width = vd56g3_supported_modes[def_mode].width,
> +			.height = vd56g3_supported_modes[def_mode].height,
> +		},
> +	};
> +
> +	vd56g3_set_pad_fmt(sd, sd_state, &fmt);

I think you could just return what vd56g3_set_pad_fmt did -- it's zero
(after fixing the streaming check in it).

> +	return 0;
> +}
> +
> +static const struct v4l2_subdev_core_ops vd56g3_core_ops = {
> +	.subscribe_event = v4l2_ctrl_subdev_subscribe_event,
> +	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
> +};
> +
> +static const struct v4l2_subdev_pad_ops vd56g3_pad_ops = {
> +	.enum_mbus_code = vd56g3_enum_mbus_code,
> +	.enum_frame_size = vd56g3_enum_frame_size,
> +	.get_fmt = v4l2_subdev_get_fmt,
> +	.set_fmt = vd56g3_set_pad_fmt,
> +	.get_selection = vd56g3_get_selection,
> +};
> +
> +static const struct v4l2_subdev_ops vd56g3_subdev_ops = {
> +	.core = &vd56g3_core_ops,
> +	.video = &vd56g3_video_ops,
> +	.pad = &vd56g3_pad_ops,
> +};
> +
> +static const struct media_entity_operations vd56g3_subdev_entity_ops = {
> +	.link_validate = v4l2_subdev_link_validate,
> +};
> +
> +static const struct v4l2_subdev_internal_ops vd56g3_internal_ops = {
> +	.init_state = vd56g3_init_state,
> +};
> +
> +/* -----------------------------------------------------------------------------
> + * Power management
> + */
> +
> +static int vd56g3_power_on(struct vd56g3 *sensor)
> +{
> +	struct i2c_client *client = sensor->i2c_client;
> +	int ret;
> +
> +	/* power on */
> +	ret = regulator_bulk_enable(ARRAY_SIZE(sensor->supplies),
> +				    sensor->supplies);
> +	if (ret) {
> +		dev_err(&client->dev, "Failed to enable regulators %d", ret);
> +		return ret;
> +	}
> +
> +	ret = clk_prepare_enable(sensor->xclk);
> +	if (ret) {
> +		dev_err(&client->dev, "Failed to enable clock %d", ret);
> +		goto disable_reg;
> +	}
> +
> +	gpiod_set_value_cansleep(sensor->reset_gpio, 0);
> +	usleep_range(3500, 4000);
> +	ret = vd56g3_wait_state(sensor, VD56G3_SYSTEM_FSM_READY_TO_BOOT, NULL);
> +	if (ret) {
> +		dev_err(&client->dev, "Sensor reset failed %d\n", ret);
> +		goto disable_clock;
> +	}
> +
> +	/* boot sensor */
> +	cci_write(sensor->regmap, VD56G3_REG_BOOT, VD56G3_CMD_BOOT, &ret);
> +	vd56g3_poll_reg(sensor, VD56G3_REG_BOOT, VD56G3_CMD_ACK, &ret);
> +	vd56g3_wait_state(sensor, VD56G3_SYSTEM_FSM_SW_STBY, &ret);
> +	if (ret) {
> +		dev_err(&client->dev, "sensor boot failed %d", ret);
> +		goto disable_clock;
> +	}
> +
> +	return 0;
> +
> +disable_clock:
> +	clk_disable_unprepare(sensor->xclk);
> +	gpiod_set_value_cansleep(sensor->reset_gpio, 1);
> +disable_reg:
> +	regulator_bulk_disable(ARRAY_SIZE(sensor->supplies), sensor->supplies);
> +
> +	return ret;
> +}
> +
> +static int vd56g3_power_off(struct vd56g3 *sensor)
> +{
> +	clk_disable_unprepare(sensor->xclk);
> +	gpiod_set_value_cansleep(sensor->reset_gpio, 1);
> +	regulator_bulk_disable(ARRAY_SIZE(sensor->supplies), sensor->supplies);
> +	return 0;

You can make the return type void.

Do you need two pairs of functions doing the same, or could you call
vd56g3_runtime_resume and vd56g3_runtime_suspend from driver's probe and
remove functions, too?

> +}
> +
> +static int vd56g3_runtime_resume(struct device *dev)
> +{
> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> +	struct vd56g3 *sensor = to_vd56g3(sd);
> +	struct i2c_client *client = sensor->i2c_client;
> +	int ret;
> +
> +	ret = vd56g3_power_on(sensor);
> +	if (ret)
> +		dev_err(&client->dev, "Failed to power on %d", ret);

You seem to be printing more precise messages in vd56g3_power_on() already.

> +
> +	return ret;
> +}
> +
> +static int vd56g3_runtime_suspend(struct device *dev)
> +{
> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> +	struct vd56g3 *sensor = to_vd56g3(sd);
> +
> +	return vd56g3_power_off(sensor);
> +}
> +
> +static const struct dev_pm_ops vd56g3_pm_ops = {
> +	SET_RUNTIME_PM_OPS(vd56g3_runtime_suspend, vd56g3_runtime_resume, NULL)
> +};
> +
> +/* -----------------------------------------------------------------------------
> + * Probe and initialization
> + */
> +
> +static int vd56g3_check_csi_conf(struct vd56g3 *sensor,
> +				 struct fwnode_handle *endpoint)
> +{
> +	struct i2c_client *client = sensor->i2c_client;
> +	struct v4l2_fwnode_endpoint ep = { .bus_type = V4L2_MBUS_CSI2_DPHY };
> +	u32 phy_data_lanes[VD56G3_MAX_CSI_DATA_LANES] = { ~0, ~0 };
> +	u8 n_lanes;
> +	u64 frequency;
> +	int p, l;
> +	int ret = 0;
> +
> +	ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &ep);
> +	if (ret)
> +		return -EINVAL;
> +
> +	/* Check lanes number */
> +	n_lanes = ep.bus.mipi_csi2.num_data_lanes;
> +	if (n_lanes != 1 && n_lanes != 2) {
> +		dev_err(&client->dev, "Invalid data lane number %d\n", n_lanes);
> +		ret = -EINVAL;
> +		goto done;
> +	}
> +	sensor->nb_of_lane = n_lanes;
> +
> +	/* Clock lane must be first */
> +	if (ep.bus.mipi_csi2.clock_lane != 0) {
> +		dev_err(&client->dev, "Clk lane must be mapped to lane 0\n");
> +		ret = -EINVAL;
> +		goto done;
> +	}
> +
> +	/* Prepare Output Interface conf based on lane settings

/*
 * Multi-line
 * comment.
 */

> +	 * logical to physical lane conversion (+ pad remaining slots)
> +	 */
> +	for (l = 0; l < n_lanes; l++)
> +		phy_data_lanes[ep.bus.mipi_csi2.data_lanes[l] - 1] = l;
> +	for (p = 0; p < VD56G3_MAX_CSI_DATA_LANES; p++) {
> +		if (phy_data_lanes[p] != ~0)
> +			continue;
> +		phy_data_lanes[p] = l;
> +		l++;
> +	}
> +	sensor->oif_ctrl = n_lanes |
> +			   (ep.bus.mipi_csi2.lane_polarities[0] << 3) |
> +			   ((phy_data_lanes[0]) << 4) |
> +			   (ep.bus.mipi_csi2.lane_polarities[1] << 6) |
> +			   ((phy_data_lanes[1]) << 7) |
> +			   (ep.bus.mipi_csi2.lane_polarities[2] << 9);
> +
> +	/* Check link frequency */
> +	if (!ep.nr_of_link_frequencies) {
> +		dev_err(&client->dev, "link-frequency not found in DT\n");
> +		ret = -EINVAL;
> +		goto done;
> +	}
> +	frequency = (n_lanes == 2) ? VD56G3_LINK_FREQ_DEF_2LANES :
> +				     VD56G3_LINK_FREQ_DEF_1LANE;
> +	if (ep.nr_of_link_frequencies != 1 ||
> +	    ep.link_frequencies[0] != frequency) {
> +		dev_err(&client->dev, "Link frequency not supported: %lld\n",
> +			ep.link_frequencies[0]);
> +		ret = -EINVAL;
> +		goto done;
> +	}
> +
> +done:
> +	v4l2_fwnode_endpoint_free(&ep);
> +	return ret;
> +}
> +
> +static int vd56g3_parse_dt_gpios_array(struct vd56g3 *sensor, char *prop_name,
> +				       u32 *array, int *nb)
> +{
> +	struct i2c_client *client = sensor->i2c_client;
> +	struct device_node *np = client->dev.of_node;
> +	unsigned int i;
> +
> +	*nb = of_property_read_variable_u32_array(np, prop_name, array, 0,
> +						  VD56G3_NB_GPIOS);
> +
> +	if (*nb == -EINVAL) {
> +		*nb = 0;
> +		return *nb;
> +	} else if (*nb < 0) {
> +		dev_err(&client->dev, "Failed to read %s prop\n", prop_name);
> +		return *nb;
> +	}
> +
> +	for (i = 0; i < *nb; i++) {
> +		if (array[i] >= VD56G3_NB_GPIOS) {
> +			dev_err(&client->dev, "Invalid GPIO : %d\n", array[i]);
> +			return -EINVAL;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int vd56g3_parse_dt_gpios(struct vd56g3 *sensor)
> +{
> +	u32 led_gpios[VD56G3_NB_GPIOS];
> +	int nb_gpios_leds;
> +	unsigned int i;
> +	int ret;
> +
> +	/* Initialize GPIOs to default */
> +	for (i = 0; i < VD56G3_NB_GPIOS; i++)
> +		sensor->gpios[i] = VD56G3_GPIOX_GPIO_IN;
> +	sensor->ext_leds_mask = 0;
> +
> +	/* Take into account optional 'st,leds' output for GPIOs */
> +	ret = vd56g3_parse_dt_gpios_array(sensor, "st,leds", led_gpios,
> +					  &nb_gpios_leds);
> +	if (ret)
> +		return ret;
> +	for (i = 0; i < nb_gpios_leds; i++) {
> +		sensor->gpios[led_gpios[i]] = VD56G3_GPIOX_STROBE_MODE;
> +		set_bit(led_gpios[i], &sensor->ext_leds_mask);
> +	}
> +
> +	return 0;
> +}
> +
> +static int vd56g3_parse_dt(struct vd56g3 *sensor)
> +{
> +	struct i2c_client *client = sensor->i2c_client;
> +	struct device *dev = &client->dev;
> +	struct fwnode_handle *endpoint;
> +	int ret;
> +
> +	endpoint = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), 0, 0, 0);
> +	if (!endpoint) {
> +		dev_err(dev, "endpoint node not found\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = vd56g3_check_csi_conf(sensor, endpoint);
> +	fwnode_handle_put(endpoint);
> +	if (ret)
> +		return ret;
> +
> +	return vd56g3_parse_dt_gpios(sensor);
> +}
> +
> +static int vd56g3_get_regulators(struct vd56g3 *sensor)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(sensor->supplies); i++)
> +		sensor->supplies[i].supply = vd56g3_supply_names[i];
> +
> +	return devm_regulator_bulk_get(&sensor->i2c_client->dev,
> +				       ARRAY_SIZE(sensor->supplies),
> +				       sensor->supplies);
> +}
> +
> +static int vd56g3_prepare_clock_tree(struct vd56g3 *sensor)
> +{
> +	struct i2c_client *client = sensor->i2c_client;
> +	const unsigned int predivs[] = { 1, 2, 4 };
> +	u32 pll_out;
> +	int i;
> +
> +	/* External clock must be in [6Mhz-27Mhz] */
> +	if (sensor->xclk_freq < 6 * HZ_PER_MHZ ||
> +	    sensor->xclk_freq > 27 * HZ_PER_MHZ) {
> +		dev_err(&client->dev,
> +			"Only 6Mhz-27Mhz clock range supported. Provided %lu MHz\n",
> +			sensor->xclk_freq / HZ_PER_MHZ);
> +		return -EINVAL;
> +	}
> +
> +	/* PLL input should be in [6Mhz-12Mhz[ */
> +	for (i = 0; i < ARRAY_SIZE(predivs); i++) {
> +		sensor->pll_prediv = predivs[i];
> +		if (sensor->xclk_freq / sensor->pll_prediv < 12 * HZ_PER_MHZ)
> +			break;
> +	}
> +
> +	/* PLL output clock must be as close as possible to 804Mhz */
> +	sensor->pll_mult = (VD56G3_TARGET_PLL * sensor->pll_prediv +
> +			    sensor->xclk_freq / 2) /
> +			   sensor->xclk_freq;
> +	pll_out = sensor->xclk_freq * sensor->pll_mult / sensor->pll_prediv;
> +
> +	/* Target Pixel Clock for standard 10bit ADC mode : 160.8Mhz */
> +	sensor->pixel_clock = pll_out / VD56G3_VT_CLOCK_DIV;
> +
> +	return 0;
> +}
> +
> +static int vd56g3_detect(struct vd56g3 *sensor)
> +{
> +	struct i2c_client *client = sensor->i2c_client;
> +	struct device *dev = &client->dev;
> +	unsigned int model;
> +	u64 model_id = 0;
> +	u64 device_revision = 0;
> +	u64 optical_revision = 0;
> +	int ret = 0;
> +
> +	model = (uintptr_t)device_get_match_data(dev);
> +
> +	cci_read(sensor->regmap, VD56G3_REG_MODEL_ID, &model_id, &ret);
> +	if (ret)
> +		return ret;

As you're calling cci_read() and then checking ret, I'd assing ret here the
usual way (and below, too).

> +
> +	if (model_id != VD56G3_MODEL_ID) {
> +		dev_err(&client->dev, "Unsupported sensor id %x",
> +			(u16)model_id);
> +		return -ENODEV;
> +	}
> +
> +	cci_read(sensor->regmap, VD56G3_REG_REVISION, &device_revision, &ret);
> +	if (ret)
> +		return ret;
> +
> +	if ((device_revision >> 8) != VD56G3_REVISION_CUT3) {
> +		dev_err(&client->dev, "Unsupported Cut version %x",
> +			(u16)device_revision);
> +		return -ENODEV;
> +	}
> +
> +	cci_read(sensor->regmap, VD56G3_REG_OPTICAL_REVISION, &optical_revision,
> +		 &ret);
> +	if (ret)
> +		return ret;
> +
> +	sensor->is_mono =
> +		((optical_revision & 1) == VD56G3_OPTICAL_REVISION_MONO);
> +	if ((sensor->is_mono && model == VD56G3_MODEL_VD66GY) ||
> +	    (!sensor->is_mono && model == VD56G3_MODEL_VD56G3)) {
> +		dev_err(&client->dev,
> +			"Found %s sensor, while %s model is defined in DT",
> +			(sensor->is_mono) ? "Mono" : "Bayer",
> +			(model == VD56G3_MODEL_VD56G3) ? "vd56g3" : "vd66gy");
> +		return -ENODEV;
> +	}
> +
> +	return 0;
> +}
> +
> +static int vd56g3_subdev_init(struct vd56g3 *sensor)
> +{
> +	struct i2c_client *client = sensor->i2c_client;
> +	int ret;
> +
> +	/* Init sub device */
> +	v4l2_i2c_subdev_init(&sensor->sd, client, &vd56g3_subdev_ops);
> +	sensor->sd.internal_ops = &vd56g3_internal_ops;
> +	sensor->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
> +			    V4L2_SUBDEV_FL_HAS_EVENTS;
> +	sensor->sd.entity.ops = &vd56g3_subdev_entity_ops;
> +
> +	/* Init source pad */
> +	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(&client->dev, "Failed to init media entity : %d", ret);
> +		return ret;
> +	}
> +
> +	/* Init controls */
> +	ret = vd56g3_init_controls(sensor);
> +	if (ret) {
> +		dev_err(&client->dev, "Controls initialization failed %d", ret);
> +		goto err_media;
> +	}
> +
> +	/* Init vd56g3 struct : default resolution + raw8 */
> +	sensor->sd.state_lock = sensor->ctrl_handler.lock;
> +	ret = v4l2_subdev_init_finalize(&sensor->sd);
> +	if (ret) {
> +		dev_err(&client->dev, "subdev init error: %d", ret);
> +		goto err_ctrls;
> +	}
> +
> +	return vd56g3_update_controls(sensor);
> +
> +err_ctrls:
> +	v4l2_ctrl_handler_free(sensor->sd.ctrl_handler);
> +
> +err_media:
> +	media_entity_cleanup(&sensor->sd.entity);

Newline here?

> +	return ret;
> +}
> +
> +static void vd56g3_subdev_cleanup(struct vd56g3 *sensor)
> +{
> +	v4l2_async_unregister_subdev(&sensor->sd);
> +	v4l2_subdev_cleanup(&sensor->sd);
> +	media_entity_cleanup(&sensor->sd.entity);
> +	v4l2_ctrl_handler_free(sensor->sd.ctrl_handler);
> +}
> +
> +static int vd56g3_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct vd56g3 *sensor;
> +	int ret;
> +
> +	sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL);
> +	if (!sensor)
> +		return -ENOMEM;
> +
> +	sensor->i2c_client = client;
> +
> +	ret = vd56g3_parse_dt(sensor);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to parse Device Tree.");
> +
> +	/* Get (and check) resources : power regs, ext clock, reset gpio */
> +	ret = vd56g3_get_regulators(sensor);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to get regulators.");
> +
> +	sensor->xclk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(sensor->xclk))
> +		return dev_err_probe(dev, PTR_ERR(sensor->xclk),
> +				     "Failed to get xclk.");
> +	sensor->xclk_freq = clk_get_rate(sensor->xclk);
> +	ret = vd56g3_prepare_clock_tree(sensor);
> +	if (ret)
> +		return ret;
> +
> +	sensor->reset_gpio =
> +		devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(sensor->reset_gpio))
> +		return dev_err_probe(dev, PTR_ERR(sensor->reset_gpio),
> +				     "Failed to get reset gpio.");
> +
> +	sensor->regmap = devm_cci_regmap_init_i2c(client, 16);
> +	if (IS_ERR(sensor->regmap))
> +		return dev_err_probe(dev, PTR_ERR(sensor->regmap),
> +				     "Failed to init regmap.");
> +
> +	/* Power ON */
> +	ret = vd56g3_power_on(sensor);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Sensor power on failed.");
> +
> +	/* Enable PM runtime with autosuspend (sensor being ON, set active) */
> +	pm_runtime_set_active(dev);
> +	pm_runtime_get_noresume(dev);
> +	pm_runtime_enable(dev);
> +	pm_runtime_set_autosuspend_delay(dev, 1000);
> +	pm_runtime_use_autosuspend(dev);
> +
> +	/* Check HW model/version */
> +	ret = vd56g3_detect(sensor);
> +	if (ret) {
> +		dev_err(&client->dev, "Sensor detect failed : %d", ret);
> +		goto err_power_off;
> +	}
> +
> +	/* Initialize, then register V4L2 subdev */
> +	ret = vd56g3_subdev_init(sensor);
> +	if (ret) {
> +		dev_err(&client->dev, "V4l2 init failed : %d", ret);
> +		goto err_power_off;
> +	}
> +
> +	ret = v4l2_async_register_subdev(&sensor->sd);
> +	if (ret) {
> +		dev_err(&client->dev, "async subdev register failed %d", ret);
> +		goto err_subdev;
> +	}
> +
> +	/* Sensor could now be powered off (after the autosuspend delay) */
> +	pm_runtime_mark_last_busy(dev);
> +	pm_runtime_put_autosuspend(dev);
> +
> +	dev_dbg(&client->dev, "Successfully probe %s sensor",
> +		(sensor->is_mono) ? "vd56g3" : "vd66gy");
> +
> +	return 0;
> +
> +err_subdev:
> +	vd56g3_subdev_cleanup(sensor);
> +err_power_off:
> +	pm_runtime_disable(dev);
> +	pm_runtime_put_noidle(dev);
> +	vd56g3_power_off(sensor);

A newline here?

> +	return ret;
> +}
> +
> +static void vd56g3_remove(struct i2c_client *client)
> +{
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct vd56g3 *sensor = to_vd56g3(sd);
> +
> +	vd56g3_subdev_cleanup(sensor);
> +
> +	pm_runtime_disable(&client->dev);
> +	if (!pm_runtime_status_suspended(&client->dev))
> +		vd56g3_power_off(sensor);
> +	pm_runtime_set_suspended(&client->dev);
> +}
> +
> +static const struct of_device_id vd56g3_dt_ids[] = {
> +	{ .compatible = "st,st-vd56g3", .data = (void *)VD56G3_MODEL_VD56G3 },
> +	{ .compatible = "st,st-vd66gy", .data = (void *)VD56G3_MODEL_VD66GY },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, vd56g3_dt_ids);
> +
> +static struct i2c_driver vd56g3_i2c_driver = {
> +	.driver = {
> +		.name  = "st-vd56g3",
> +		.of_match_table = vd56g3_dt_ids,
> +		.pm = &vd56g3_pm_ops,
> +	},
> +	.probe = vd56g3_probe,
> +	.remove = vd56g3_remove,
> +};
> +
> +module_i2c_driver(vd56g3_i2c_driver);
> +
> +MODULE_AUTHOR("Benjamin Mugnier <benjamin.mugnier@foss.st.com>");
> +MODULE_AUTHOR("Mickael Guene <mickael.guene@st.com>");
> +MODULE_AUTHOR("Sylvain Petinot <sylvain.petinot@foss.st.com>");
> +MODULE_DESCRIPTION("ST VD56G3 sensor driver");
> +MODULE_LICENSE("GPL");

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v2 2/2] media: i2c: Add driver for ST VD56G3 camera sensor

[Sylvain Petinot]

Hi Sakari,

Thanks for the review,


On 5/28/2024 1:49 PM, Sakari Ailus wrote:
> Hi Sylvain,
> 
> Thanks for the update. A few more comments below...
> 
> On Tue, May 21, 2024 at 06:29:50PM +0200, Sylvain Petinot wrote:
>> Add V4L2 sub-device driver for STMicroelectronics VD56G3 camera sensor.
>> This is a 1.5 M pixel global shutter image sensor with an active array
>> size of 1124 x 1364 (portrait orientation).
>>
>> The driver supports Mono (VD56G3) and Color (VD66GY) variants.
>>
>> Signed-off-by: Sylvain Petinot <sylvain.petinot@foss.st.com>
>> ---
>>  drivers/media/i2c/Kconfig     |   11 +
>>  drivers/media/i2c/Makefile    |    1 +
>>  drivers/media/i2c/st-vd56g3.c | 1608 +++++++++++++++++++++++++++++++++
>>  3 files changed, 1620 insertions(+)
>>  create mode 100644 drivers/media/i2c/st-vd56g3.c
>>
>> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
>> index c6d3ee472d81..c3d3cbdfcb3b 100644
>> --- a/drivers/media/i2c/Kconfig
>> +++ b/drivers/media/i2c/Kconfig
>> @@ -659,6 +659,17 @@ config VIDEO_S5K6A3
>>  	  This is a V4L2 sensor driver for Samsung S5K6A3 raw
>>  	  camera sensor.
>>  
>> +config VIDEO_ST_VD56G3
>> +	tristate "ST VD56G3 sensor support"
>> +	select V4L2_CCI_I2C
>> +	depends on OF && GPIOLIB
>> +	help
>> +	  This is a Video4Linux2 sensor driver for the ST VD56G3
>> +	  camera sensor.
>> +
>> +	  To compile this driver as a module, choose M here: the
>> +	  module will be called st-vd56g3.
>> +
>>  config VIDEO_ST_VGXY61
>>  	tristate "ST VGXY61 sensor support"
>>  	select V4L2_CCI_I2C
>> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
>> index dfbe6448b549..b469cf0f8113 100644
>> --- a/drivers/media/i2c/Makefile
>> +++ b/drivers/media/i2c/Makefile
>> @@ -124,6 +124,7 @@ obj-$(CONFIG_VIDEO_SAA717X) += saa717x.o
>>  obj-$(CONFIG_VIDEO_SAA7185) += saa7185.o
>>  obj-$(CONFIG_VIDEO_SONY_BTF_MPX) += sony-btf-mpx.o
>>  obj-$(CONFIG_VIDEO_ST_MIPID02) += st-mipid02.o
>> +obj-$(CONFIG_VIDEO_ST_VD56G3) += st-vd56g3.o
>>  obj-$(CONFIG_VIDEO_ST_VGXY61) += st-vgxy61.o
>>  obj-$(CONFIG_VIDEO_TC358743) += tc358743.o
>>  obj-$(CONFIG_VIDEO_TC358746) += tc358746.o
>> diff --git a/drivers/media/i2c/st-vd56g3.c b/drivers/media/i2c/st-vd56g3.c
>> new file mode 100644
>> index 000000000000..1e4329502230
>> --- /dev/null
>> +++ b/drivers/media/i2c/st-vd56g3.c
>> @@ -0,0 +1,1608 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * A V4L2 driver for ST VD56G3 (Mono) and VD66GY (RGB) global shutter cameras.
>> + * Copyright (C) 2024, STMicroelectronics SA
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/delay.h>
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/i2c.h>
>> +#include <linux/iopoll.h>
>> +#include <linux/module.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/regmap.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/units.h>
>> +
>> +#include <asm/unaligned.h>
>> +
>> +#include <media/mipi-csi2.h>
>> +#include <media/v4l2-async.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>
>> +#include <media/v4l2-subdev.h>
>> +
>> +/* Register Map */
>> +#define VD56G3_REG_MODEL_ID				CCI_REG16_LE(0x0000)
>> +#define VD56G3_MODEL_ID					0x5603
>> +#define VD56G3_REG_REVISION				CCI_REG16_LE(0x0002)
>> +#define VD56G3_REVISION_CUT3				0x31
>> +#define VD56G3_REG_OPTICAL_REVISION			CCI_REG8(0x001a)
>> +#define VD56G3_OPTICAL_REVISION_MONO			0
>> +#define VD56G3_OPTICAL_REVISION_BAYER			1
>> +#define VD56G3_REG_SYSTEM_FSM				CCI_REG8(0x0028)
>> +#define VD56G3_SYSTEM_FSM_READY_TO_BOOT			0x01
>> +#define VD56G3_SYSTEM_FSM_SW_STBY			0x02
>> +#define VD56G3_SYSTEM_FSM_STREAMING			0x03
>> +#define VD56G3_REG_APPLIED_COARSE_EXPOSURE		CCI_REG16_LE(0x0064)
>> +#define VD56G3_REG_APPLIED_ANALOG_GAIN			CCI_REG8(0x0068)
>> +#define VD56G3_REG_APPLIED_DIGITAL_GAIN			CCI_REG16_LE(0x006a)
>> +#define VD56G3_REG_BOOT					CCI_REG8(0x0200)
>> +#define VD56G3_CMD_ACK					0
>> +#define VD56G3_CMD_BOOT					1
>> +#define VD56G3_REG_STBY					CCI_REG8(0x0201)
>> +#define VD56G3_CMD_START_STREAM				1
>> +#define VD56G3_REG_STREAMING				CCI_REG8(0x0202)
>> +#define VD56G3_CMD_STOP_STREAM				1
>> +#define VD56G3_REG_EXT_CLOCK				CCI_REG32_LE(0x0220)
>> +#define VD56G3_REG_CLK_PLL_PREDIV			CCI_REG8(0x0224)
>> +#define VD56G3_REG_CLK_SYS_PLL_MULT			CCI_REG8(0x0226)
>> +#define VD56G3_REG_ORIENTATION				CCI_REG8(0x0302)
>> +#define VD56G3_REG_FORMAT_CTRL				CCI_REG8(0x030a)
>> +#define VD56G3_REG_OIF_CTRL				CCI_REG16_LE(0x030c)
>> +#define VD56G3_REG_OIF_IMG_CTRL				CCI_REG8(0x030f)
>> +#define VD56G3_REG_OIF_CSI_BITRATE			CCI_REG16_LE(0x0312)
>> +#define VD56G3_REG_DUSTER_CTRL				CCI_REG8(0x0318)
>> +#define VD56G3_DUSTER_DISABLE				0
>> +#define VD56G3_DUSTER_ENABLE_DEF_MODULES		0x13
>> +#define VD56G3_REG_ISL_ENABLE				CCI_REG8(0x0333)
>> +#define VD56G3_REG_DARKCAL_CTRL				CCI_REG8(0x0340)
>> +#define VD56G3_DARKCAL_ENABLE				1
>> +#define VD56G3_DARKCAL_DISABLE_DARKAVG			2
>> +#define VD56G3_REG_PATGEN_CTRL				CCI_REG16_LE(0x0400)
>> +#define VD56G3_PATGEN_ENABLE				1
>> +#define VD56G3_PATGEN_TYPE_SHIFT			4
>> +#define VD56G3_REG_AE_COLDSTART_COARSE_EXPOSURE		CCI_REG16_LE(0x042a)
>> +#define VD56G3_REG_AE_COLDSTART_ANALOG_GAIN		CCI_REG8(0x042c)
>> +#define VD56G3_REG_AE_COLDSTART_DIGITAL_GAIN		CCI_REG16_LE(0x042e)
>> +#define VD56G3_REG_AE_ROI_START_H			CCI_REG16_LE(0x0432)
>> +#define VD56G3_REG_AE_ROI_START_V			CCI_REG16_LE(0x0434)
>> +#define VD56G3_REG_AE_ROI_END_H				CCI_REG16_LE(0x0436)
>> +#define VD56G3_REG_AE_ROI_END_V				CCI_REG16_LE(0x0438)
>> +#define VD56G3_REG_AE_COMPENSATION			CCI_REG16_LE(0x043a)
>> +#define VD56G3_REG_EXP_MODE				CCI_REG8(0x044c)
>> +#define VD56G3_EXP_MODE_AUTO				0
>> +#define VD56G3_EXP_MODE_FREEZE				1
>> +#define VD56G3_EXP_MODE_MANUAL				2
>> +#define VD56G3_REG_MANUAL_ANALOG_GAIN			CCI_REG8(0x044d)
>> +#define VD56G3_REG_MANUAL_COARSE_EXPOSURE		CCI_REG16_LE(0x044e)
>> +#define VD56G3_REG_MANUAL_DIGITAL_GAIN_CH0		CCI_REG16_LE(0x0450)
>> +#define VD56G3_REG_MANUAL_DIGITAL_GAIN_CH1		CCI_REG16_LE(0x0452)
>> +#define VD56G3_REG_MANUAL_DIGITAL_GAIN_CH2		CCI_REG16_LE(0x0454)
>> +#define VD56G3_REG_MANUAL_DIGITAL_GAIN_CH3		CCI_REG16_LE(0x0456)
>> +#define VD56G3_REG_FRAME_LENGTH				CCI_REG16_LE(0x0458)
>> +#define VD56G3_REG_Y_START				CCI_REG16_LE(0x045a)
>> +#define VD56G3_REG_Y_END				CCI_REG16_LE(0x045c)
>> +#define VD56G3_REG_OUT_ROI_X_START			CCI_REG16_LE(0x045e)
>> +#define VD56G3_REG_OUT_ROI_X_END			CCI_REG16_LE(0x0460)
>> +#define VD56G3_REG_OUT_ROI_Y_START			CCI_REG16_LE(0x0462)
>> +#define VD56G3_REG_OUT_ROI_Y_END			CCI_REG16_LE(0x0464)
>> +#define VD56G3_REG_GPIO_0_CTRL				CCI_REG8(0x0467)
>> +#define VD56G3_GPIOX_GPIO_IN				0x01
>> +#define VD56G3_GPIOX_STROBE_MODE			0x02
>> +#define VD56G3_REG_READOUT_CTRL				CCI_REG8(0x047e)
>> +#define READOUT_NORMAL					0x00
>> +#define READOUT_DIGITAL_BINNING_X2			0x01
>> +
>> +/*
>> + * The VD56G3 pixel array is organized as follows:
>> + *
>> + * +--------------------------------+
>> + * |                                | \
>> + * |   +------------------------+   |  |
>> + * |   |                        |   |  |
>> + * |   |                        |   |  |
>> + * |   |                        |   |  |
>> + * |   |                        |   |  |
>> + * |   |                        |   |  |
>> + * |   |   Default resolution   |   |  | Native height (1364)
> 
> What's outside the default resolution? It doesn't appear the driver would
> allow capturing pixels out side this area.

Well both native and default resolutions are supported in the
'vd56g3_supported_modes' below.
However this quite exotic resolution (1364 x 1124) isn't well supported
by csi receivers, ISPs. That's why the default resolution of the driver
is 1120 x 1360 (multiple of 16).

> 
>> + * |   |      1120 x 1360       |   |  |
>> + * |   |                        |   |  |
>> + * |   |                        |   |  |
>> + * |   |                        |   |  |
>> + * |   |                        |   |  |
>> + * |   +------------------------+   |  |
>> + * |                                | /
>> + * +--------------------------------+
>> + *   <----------------------------->
>> + *                     \-------------------  Native width (1124)
>> + *
>> + * The native resolution is 1124x1364.
>> + * The recommended/default resolution is 1120x1360 (multiple of 16).
>> + */
>> +#define VD56G3_NATIVE_WIDTH				1124
>> +#define VD56G3_NATIVE_HEIGHT				1364
>> +#define VD56G3_DEFAULT_WIDTH				1120
>> +#define VD56G3_DEFAULT_HEIGHT				1360
>> +#define VD56G3_DEFAULT_MODE				1
>> +
>> +/* PLL settings */
>> +#define VD56G3_TARGET_PLL				804000000UL
>> +#define VD56G3_VT_CLOCK_DIV				5
>> +
>> +/* Line length and Frame length (settings are for standard 10bits ADC mode) */
>> +#define VD56G3_LINE_LENGTH_MIN				1236
>> +#define VD56G3_VBLANK_MIN				110
>> +#define VD56G3_FRAME_LENGTH_DEF_60FPS			2168
>> +
>> +/* Exposure settings */
>> +#define VD56G3_EXPOSURE_MARGIN				75
>> +#define VD56G3_EXPOSURE_DEFAULT				1420
>> +
>> +/* Output Interface settings */
>> +#define VD56G3_MAX_CSI_DATA_LANES			2
>> +#define VD56G3_LINK_FREQ_DEF_1LANE			750000000UL
>> +#define VD56G3_LINK_FREQ_DEF_2LANES			402000000UL
>> +
>> +/* GPIOs */
>> +#define VD56G3_NB_GPIOS					8
>> +
>> +/* regulator supplies */
>> +static const char *const vd56g3_supply_names[] = {
>> +	"vcore",
>> +	"vddio",
>> +	"vana",
>> +};
>> +
>> +/* -----------------------------------------------------------------------------
>> + * Models (VD56G3: Mono, VD66GY: Bayer RGB), Modes and formats
>> + */
>> +
>> +enum vd56g3_models {
>> +	VD56G3_MODEL_VD56G3,
>> +	VD56G3_MODEL_VD66GY,
>> +};
>> +
>> +struct vd56g3_mode {
>> +	u32 width;
>> +	u32 height;
>> +};
>> +
>> +static const struct vd56g3_mode vd56g3_supported_modes[] = {
>> +	{
>> +		.width = VD56G3_NATIVE_WIDTH,
>> +		.height = VD56G3_NATIVE_HEIGHT,
>> +	},
>> +	{
>> +		.width = VD56G3_DEFAULT_WIDTH,
>> +		.height = VD56G3_DEFAULT_HEIGHT,
>> +	},
>> +	{
>> +		.width = 1024,
>> +		.height = 1280,
>> +	},
>> +	{
>> +		.width = 1024,
>> +		.height = 768,
>> +	},
>> +	{
>> +		.width = 768,
>> +		.height = 1024,
>> +	},
>> +	{
>> +		.width = 720,
>> +		.height = 1280,
>> +	},
>> +	{
>> +		.width = 640,
>> +		.height = 480,
>> +	},
>> +	{
>> +		.width = 480,
>> +		.height = 640,
>> +	},
>> +	{
>> +		.width = 320,
>> +		.height = 240,
>> +	},
>> +};
>> +
>> +/*
>> + * Sensor support 8bits and 10bits output in both variants
>> + *  - Monochrome
>> + *  - RGB (with all H/V flip variations)
>> + */
>> +static const unsigned int vd56g3_mbus_codes[2][5] = {
>> +	{
>> +		MEDIA_BUS_FMT_Y8_1X8,
>> +		MEDIA_BUS_FMT_SGRBG8_1X8,
>> +		MEDIA_BUS_FMT_SRGGB8_1X8,
>> +		MEDIA_BUS_FMT_SBGGR8_1X8,
>> +		MEDIA_BUS_FMT_SGBRG8_1X8,
>> +	},
>> +	{
>> +		MEDIA_BUS_FMT_Y10_1X10,
>> +		MEDIA_BUS_FMT_SGRBG10_1X10,
>> +		MEDIA_BUS_FMT_SRGGB10_1X10,
>> +		MEDIA_BUS_FMT_SBGGR10_1X10,
>> +		MEDIA_BUS_FMT_SGBRG10_1X10,
>> +	},
>> +};
>> +
>> +enum vd56g3_expo_state {
>> +	VD56G3_EXPO_AUTO,
>> +	VD56G3_EXPO_AUTO_FREEZE,
>> +	VD56G3_EXPO_MANUAL
>> +};
>> +
>> +struct vd56g3 {
>> +	struct i2c_client *i2c_client;
>> +	struct v4l2_subdev sd;
>> +	struct media_pad pad;
>> +	struct regulator_bulk_data supplies[ARRAY_SIZE(vd56g3_supply_names)];
>> +	struct gpio_desc *reset_gpio;
>> +	struct clk *xclk;
>> +	struct regmap *regmap;
>> +	u32 xclk_freq;
>> +	u32 pll_prediv;
>> +	u32 pll_mult;
>> +	u32 pixel_clock;
>> +	u16 oif_ctrl;
>> +	u8 nb_of_lane;
>> +	u32 gpios[VD56G3_NB_GPIOS];
>> +	unsigned long ext_leds_mask;
>> +	bool is_mono;
>> +	struct v4l2_ctrl_handler ctrl_handler;
>> +	struct v4l2_ctrl *hblank_ctrl;
>> +	struct v4l2_ctrl *vblank_ctrl;
>> +	struct {
>> +		struct v4l2_ctrl *hflip_ctrl;
>> +		struct v4l2_ctrl *vflip_ctrl;
>> +	};
>> +	struct v4l2_ctrl *patgen_ctrl;
>> +	struct {
>> +		struct v4l2_ctrl *ae_ctrl;
>> +		struct v4l2_ctrl *expo_ctrl;
>> +		struct v4l2_ctrl *again_ctrl;
>> +		struct v4l2_ctrl *dgain_ctrl;
>> +	};
>> +	struct v4l2_ctrl *ae_lock_ctrl;
>> +	struct v4l2_ctrl *ae_bias_ctrl;
>> +	struct v4l2_ctrl *led_ctrl;
>> +	bool streaming;
>> +};
>> +
>> +static inline struct vd56g3 *to_vd56g3(struct v4l2_subdev *sd)
>> +{
>> +	return container_of_const(sd, struct vd56g3, sd);
>> +}
>> +
>> +static inline struct v4l2_subdev *ctrl_to_sd(struct v4l2_ctrl *ctrl)
>> +{
>> +	return &container_of_const(ctrl->handler, struct vd56g3, ctrl_handler)
>> +			->sd;
>> +}
>> +
>> +/* -----------------------------------------------------------------------------
>> + * Additional i2c register helpers
>> + */
>> +
>> +static int vd56g3_poll_reg(struct vd56g3 *sensor, u32 reg, u8 poll_val,
>> +			   int *err)
>> +{
>> +	unsigned int val = 0;
>> +	int ret;
>> +
>> +	if (err && *err)
>> +		return *err;
>> +
>> +	ret = regmap_read_poll_timeout(sensor->regmap, CCI_REG_ADDR(reg), val,
>> +				       (val == poll_val), 2000,
>> +				       500 * USEC_PER_MSEC);
>> +
>> +	if (ret && err)
>> +		*err = ret;
>> +
>> +	return ret;
>> +}
>> +
>> +static int vd56g3_wait_state(struct vd56g3 *sensor, int state, int *err)
>> +{
>> +	return vd56g3_poll_reg(sensor, VD56G3_REG_SYSTEM_FSM, state, err);
>> +}
>> +
>> +/* -----------------------------------------------------------------------------
>> + * Controls: definitions, helpers and handlers
>> + */
>> +
>> +static const char *const vd56g3_tp_menu[] = { "Disabled", "Solid", "Colorbar",
>> +					      "Gradbar",  "Hgrey", "Vgrey",
>> +					      "Dgrey",	  "PN28" };
>> +
>> +static const s64 vd56g3_ev_bias_qmenu[] = { -4000, -3500, -3000, -2500, -2000,
>> +					    -1500, -1000, -500,	 0,	500,
>> +					    1000,  1500,  2000,	 2500,	3000,
>> +					    3500,  4000 };
>> +
>> +static const s64 vd56g3_link_freq_1lane[] = { VD56G3_LINK_FREQ_DEF_1LANE };
>> +
>> +static const s64 vd56g3_link_freq_2lanes[] = { VD56G3_LINK_FREQ_DEF_2LANES };
>> +
>> +static u8 vd56g3_get_bpp(__u32 code)
>> +{
>> +	switch (code) {
>> +	case MEDIA_BUS_FMT_Y8_1X8:
>> +	case MEDIA_BUS_FMT_SGRBG8_1X8:
>> +	case MEDIA_BUS_FMT_SRGGB8_1X8:
>> +	case MEDIA_BUS_FMT_SBGGR8_1X8:
>> +	case MEDIA_BUS_FMT_SGBRG8_1X8:
>> +	default:
>> +		return 8;
>> +	case MEDIA_BUS_FMT_Y10_1X10:
>> +	case MEDIA_BUS_FMT_SGRBG10_1X10:
>> +	case MEDIA_BUS_FMT_SRGGB10_1X10:
>> +	case MEDIA_BUS_FMT_SBGGR10_1X10:
>> +	case MEDIA_BUS_FMT_SGBRG10_1X10:
>> +		return 10;
>> +	}
>> +}
>> +
>> +static u8 vd56g3_get_datatype(__u32 code)
>> +{
>> +	switch (code) {
>> +	case MEDIA_BUS_FMT_Y8_1X8:
>> +	case MEDIA_BUS_FMT_SGRBG8_1X8:
>> +	case MEDIA_BUS_FMT_SRGGB8_1X8:
>> +	case MEDIA_BUS_FMT_SBGGR8_1X8:
>> +	case MEDIA_BUS_FMT_SGBRG8_1X8:
>> +	default:
>> +		return MIPI_CSI2_DT_RAW8;
>> +	case MEDIA_BUS_FMT_Y10_1X10:
>> +	case MEDIA_BUS_FMT_SGRBG10_1X10:
>> +	case MEDIA_BUS_FMT_SRGGB10_1X10:
>> +	case MEDIA_BUS_FMT_SBGGR10_1X10:
>> +	case MEDIA_BUS_FMT_SGBRG10_1X10:
>> +		return MIPI_CSI2_DT_RAW10;
>> +	}
>> +}
>> +
>> +static int vd56g3_read_expo_cluster(struct vd56g3 *sensor, bool force_cur_val)
>> +{
>> +	u64 exposure = 0;
>> +	u64 again = 0;
>> +	u64 dgain = 0;
>> +	int ret = 0;
>> +
>> +	/*
>> +	 * When 'force_cur_val' is enabled, save the ctrl value in 'cur.val'
>> +	 * instead of the normal 'val', this is used during poweroff to cache
>> +	 * volatile ctrls and enable coldstart.
>> +	 */
>> +	cci_read(sensor->regmap, VD56G3_REG_APPLIED_COARSE_EXPOSURE, &exposure,
>> +		 &ret);
>> +	cci_read(sensor->regmap, VD56G3_REG_APPLIED_ANALOG_GAIN, &again, &ret);
>> +	cci_read(sensor->regmap, VD56G3_REG_APPLIED_DIGITAL_GAIN, &dgain, &ret);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (force_cur_val) {
>> +		sensor->expo_ctrl->cur.val = exposure;
>> +		sensor->again_ctrl->cur.val = again;
>> +		sensor->dgain_ctrl->cur.val = dgain;
>> +	} else {
>> +		sensor->expo_ctrl->val = exposure;
>> +		sensor->again_ctrl->val = again;
>> +		sensor->dgain_ctrl->val = dgain;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static int vd56g3_update_patgen(struct vd56g3 *sensor, u32 patgen_index)
>> +{
>> +	u32 pattern = patgen_index <= 3 ? patgen_index : patgen_index + 12;
>> +	u16 patgen = pattern << VD56G3_PATGEN_TYPE_SHIFT;
>> +	u8 duster = VD56G3_DUSTER_ENABLE_DEF_MODULES;
>> +	u8 darkcal = VD56G3_DARKCAL_ENABLE;
>> +	int ret = 0;
>> +
>> +	if (patgen_index) {
>> +		patgen |= VD56G3_PATGEN_ENABLE;
>> +		duster = VD56G3_DUSTER_DISABLE;
>> +		darkcal = VD56G3_DARKCAL_DISABLE_DARKAVG;
>> +	}
>> +
>> +	cci_write(sensor->regmap, VD56G3_REG_DUSTER_CTRL, duster, &ret);
>> +	cci_write(sensor->regmap, VD56G3_REG_DARKCAL_CTRL, darkcal, &ret);
>> +	cci_write(sensor->regmap, VD56G3_REG_PATGEN_CTRL, patgen, &ret);
>> +
>> +	return ret;
>> +}
>> +
>> +static int vd56g3_update_expo_cluster(struct vd56g3 *sensor, bool is_auto)
>> +{
>> +	enum vd56g3_expo_state expo_state = is_auto ? VD56G3_EXP_MODE_AUTO :
>> +						      VD56G3_EXP_MODE_MANUAL;
>> +	int ret = 0;
>> +
>> +	if (sensor->ae_ctrl->is_new)
>> +		cci_write(sensor->regmap, VD56G3_REG_EXP_MODE, expo_state,
>> +			  &ret);
>> +
>> +	/* In Auto expo, set coldstart parameters */
>> +	if (is_auto && sensor->ae_ctrl->is_new) {
>> +		cci_write(sensor->regmap,
>> +			  VD56G3_REG_AE_COLDSTART_COARSE_EXPOSURE,
>> +			  sensor->expo_ctrl->val, &ret);
>> +		cci_write(sensor->regmap, VD56G3_REG_AE_COLDSTART_ANALOG_GAIN,
>> +			  sensor->again_ctrl->val, &ret);
>> +		cci_write(sensor->regmap, VD56G3_REG_AE_COLDSTART_DIGITAL_GAIN,
>> +			  sensor->dgain_ctrl->val, &ret);
>> +	}
>> +
>> +	/* In Manual expo, set exposure, analog and digital gains */
>> +	if (!is_auto && sensor->expo_ctrl->is_new)
>> +		cci_write(sensor->regmap, VD56G3_REG_MANUAL_COARSE_EXPOSURE,
>> +			  sensor->expo_ctrl->val, &ret);
>> +
>> +	if (!is_auto && sensor->again_ctrl->is_new)
>> +		cci_write(sensor->regmap, VD56G3_REG_MANUAL_ANALOG_GAIN,
>> +			  sensor->again_ctrl->val, &ret);
>> +
>> +	if (!is_auto && sensor->dgain_ctrl->is_new) {
>> +		cci_write(sensor->regmap, VD56G3_REG_MANUAL_DIGITAL_GAIN_CH0,
>> +			  sensor->dgain_ctrl->val, &ret);
>> +		cci_write(sensor->regmap, VD56G3_REG_MANUAL_DIGITAL_GAIN_CH1,
>> +			  sensor->dgain_ctrl->val, &ret);
>> +		cci_write(sensor->regmap, VD56G3_REG_MANUAL_DIGITAL_GAIN_CH2,
>> +			  sensor->dgain_ctrl->val, &ret);
>> +		cci_write(sensor->regmap, VD56G3_REG_MANUAL_DIGITAL_GAIN_CH3,
>> +			  sensor->dgain_ctrl->val, &ret);
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static int vd56g3_lock_exposure(struct vd56g3 *sensor, u32 lock_val)
>> +{
>> +	bool ae_lock = lock_val & V4L2_LOCK_EXPOSURE;
>> +	enum vd56g3_expo_state expo_state = ae_lock ? VD56G3_EXP_MODE_FREEZE :
>> +						      VD56G3_EXP_MODE_AUTO;
>> +	int ret = 0;
>> +
>> +	if (sensor->ae_ctrl->val == V4L2_EXPOSURE_AUTO)
>> +		cci_write(sensor->regmap, VD56G3_REG_EXP_MODE, expo_state,
> 
> You can return cci_write's return value here.
> 
>> +			  &ret);
>> +
>> +	return ret;
> 
> And return 0 here.
> 

Sure, Applied for V3.

>> +}
>> +
>> +static int vd56g3_write_gpiox(struct vd56g3 *sensor, unsigned long gpio_mask)
>> +{
>> +	unsigned long io;
>> +	u32 gpio_val;
>> +	int ret = 0;
>> +
>> +	for_each_set_bit(io, &gpio_mask, VD56G3_NB_GPIOS) {
>> +		gpio_val = sensor->gpios[io];
>> +
>> +		if (gpio_val == VD56G3_GPIOX_STROBE_MODE &&
>> +		    sensor->led_ctrl->val == V4L2_FLASH_LED_MODE_NONE)
>> +			gpio_val = VD56G3_GPIOX_GPIO_IN;
>> +
>> +		cci_write(sensor->regmap, VD56G3_REG_GPIO_0_CTRL + io, gpio_val,
>> +			  &ret);
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static int vd56g3_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
>> +{
>> +	struct v4l2_subdev *sd = ctrl_to_sd(ctrl);
>> +	struct vd56g3 *sensor = to_vd56g3(sd);
>> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
>> +	int ret = 0;
>> +
>> +	/* Interact with HW only when it is powered ON */
>> +	if (!pm_runtime_get_if_in_use(&client->dev))
>> +		return 0;
>> +
>> +	switch (ctrl->id) {
>> +	case V4L2_CID_EXPOSURE_AUTO:
>> +		ret = vd56g3_read_expo_cluster(sensor, false);
>> +		break;
>> +	default:
>> +		ret = -EINVAL;
>> +		break;
>> +	}
>> +
>> +	pm_runtime_mark_last_busy(&client->dev);
>> +	pm_runtime_put_autosuspend(&client->dev);
> 
> This should be __pm_runtime_put_autosuspend(). pm_runtime_put_autosuspend()
> will be re-purposed soon to include marking the last busy timestamp.

I missed that, thanks for pointing me.

> 
> Same elsewhere in the driver.

Applied for V3.

> 
>> +
>> +	return ret;
>> +}
>> +
>> +static int vd56g3_s_ctrl(struct v4l2_ctrl *ctrl)
>> +{
>> +	struct v4l2_subdev *sd = ctrl_to_sd(ctrl);
>> +	struct vd56g3 *sensor = to_vd56g3(sd);
>> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
>> +	struct v4l2_subdev_state *state;
>> +	const struct v4l2_rect *crop;
>> +	unsigned int frame_length = 0;
>> +	unsigned int expo_max;
>> +	unsigned int ae_compensation;
>> +	bool is_auto = false;
>> +	int ret;
>> +
>> +	state = v4l2_subdev_get_locked_active_state(sd);
>> +	crop = v4l2_subdev_state_get_crop(state, 0);
>> +
>> +	if (ctrl->flags & V4L2_CTRL_FLAG_READ_ONLY)
>> +		return 0;
>> +
>> +	/* Update controls state, range, etc. whatever the state of the HW */
>> +	switch (ctrl->id) {
>> +	case V4L2_CID_VBLANK:
>> +		frame_length = crop->height + ctrl->val;
>> +		expo_max = frame_length - VD56G3_EXPOSURE_MARGIN;
>> +		__v4l2_ctrl_modify_range(sensor->expo_ctrl, 0, expo_max, 1,
>> +					 VD56G3_EXPOSURE_DEFAULT);
> 
> __v4l2_ctrl_modify_range() may fail. Please handle the possible error.

I will handle it for V3.

> 
>> +		break;
>> +	case V4L2_CID_EXPOSURE_AUTO:
>> +		is_auto = (ctrl->val == V4L2_EXPOSURE_AUTO);
>> +		__v4l2_ctrl_grab(sensor->ae_lock_ctrl, !is_auto);
>> +		__v4l2_ctrl_grab(sensor->ae_bias_ctrl, !is_auto);
>> +		break;
>> +	default:
>> +		break;
> 
> You could omit default here.

I don't really like switch case without default. For sure I can omit,
but I prefer making it explicit.

> 
>> +	}
>> +
>> +	/* Interact with HW only when it is powered ON */
>> +	if (!pm_runtime_get_if_in_use(&client->dev))
>> +		return 0;
>> +
>> +	switch (ctrl->id) {
>> +	case V4L2_CID_HFLIP:
>> +		ret = cci_write(sensor->regmap, VD56G3_REG_ORIENTATION,
>> +				sensor->hflip_ctrl->val |
>> +					(sensor->vflip_ctrl->val << 1),
>> +				NULL);
>> +		break;
>> +	case V4L2_CID_TEST_PATTERN:
>> +		ret = vd56g3_update_patgen(sensor, ctrl->val);
>> +		break;
>> +	case V4L2_CID_EXPOSURE_AUTO:
>> +		ret = vd56g3_update_expo_cluster(sensor, is_auto);
>> +		break;
>> +	case V4L2_CID_3A_LOCK:
>> +		ret = vd56g3_lock_exposure(sensor, ctrl->val);
>> +		break;
>> +	case V4L2_CID_AUTO_EXPOSURE_BIAS:
>> +		ae_compensation =
>> +			DIV_ROUND_CLOSEST((int)vd56g3_ev_bias_qmenu[ctrl->val] *
>> +					  256, 1000);
>> +		ret = cci_write(sensor->regmap, VD56G3_REG_AE_COMPENSATION,
>> +				ae_compensation, NULL);
>> +		break;
>> +	case V4L2_CID_VBLANK:
>> +		ret = cci_write(sensor->regmap, VD56G3_REG_FRAME_LENGTH,
>> +				frame_length, NULL);
>> +		break;
>> +	case V4L2_CID_FLASH_LED_MODE:
>> +		ret = vd56g3_write_gpiox(sensor, sensor->ext_leds_mask);
>> +		break;
>> +	default:
>> +		ret = -EINVAL;
>> +		break;
>> +	}
>> +
>> +	pm_runtime_mark_last_busy(&client->dev);
>> +	pm_runtime_put_autosuspend(&client->dev);
>> +
>> +	return ret;
>> +}
>> +
>> +static const struct v4l2_ctrl_ops vd56g3_ctrl_ops = {
>> +	.g_volatile_ctrl = vd56g3_g_volatile_ctrl,
>> +	.s_ctrl = vd56g3_s_ctrl,
>> +};
>> +
>> +static int vd56g3_update_controls(struct vd56g3 *sensor)
>> +{
>> +	struct v4l2_subdev_state *state;
>> +	const struct v4l2_rect *crop;
>> +	unsigned int hblank;
>> +	unsigned int vblank_min, vblank, vblank_max;
>> +	unsigned int frame_length;
>> +	unsigned int expo_max;
>> +	int ret;
>> +
>> +	state = v4l2_subdev_get_locked_active_state(&sensor->sd);
>> +	crop = v4l2_subdev_state_get_crop(state, 0);
>> +	hblank = VD56G3_LINE_LENGTH_MIN - crop->width;
>> +	vblank_min = VD56G3_VBLANK_MIN;
>> +	vblank = VD56G3_FRAME_LENGTH_DEF_60FPS - crop->height;
>> +	vblank_max = 0xffff - crop->height;
> 
> How about adding a corresponding FRAME_LENGTH_MAX macro for 0xffff?

Ok, better.

> 
>> +	frame_length = crop->height + vblank;
>> +	expo_max = frame_length - VD56G3_EXPOSURE_MARGIN;
>> +
>> +	/* Update blanking and exposure (ranges + values) */
>> +	ret = __v4l2_ctrl_modify_range(sensor->hblank_ctrl, hblank, hblank, 1,
>> +				       hblank);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = __v4l2_ctrl_modify_range(sensor->vblank_ctrl, vblank_min,
>> +				       vblank_max, 1, vblank);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = __v4l2_ctrl_s_ctrl(sensor->vblank_ctrl, vblank);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = __v4l2_ctrl_modify_range(sensor->expo_ctrl, 0, expo_max, 1,
>> +				       VD56G3_EXPOSURE_DEFAULT);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return __v4l2_ctrl_s_ctrl(sensor->expo_ctrl, VD56G3_EXPOSURE_DEFAULT);
>> +}
>> +
>> +static int vd56g3_init_controls(struct vd56g3 *sensor)
>> +{
>> +	const struct v4l2_ctrl_ops *ops = &vd56g3_ctrl_ops;
>> +	struct v4l2_ctrl_handler *hdl = &sensor->ctrl_handler;
>> +	struct v4l2_ctrl *ctrl;
>> +	int ret;
>> +
>> +	v4l2_ctrl_handler_init(hdl, 25);
>> +
>> +	/* Horizontal & vertical flips modify bayer code on RGB variant */
>> +	sensor->hflip_ctrl =
>> +		v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HFLIP, 0, 1, 1, 0);
>> +	if (sensor->hflip_ctrl)
>> +		sensor->hflip_ctrl->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
>> +
>> +	sensor->vflip_ctrl =
>> +		v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VFLIP, 0, 1, 1, 0);
>> +	if (sensor->vflip_ctrl)
>> +		sensor->vflip_ctrl->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
>> +
>> +	sensor->patgen_ctrl =
>> +		v4l2_ctrl_new_std_menu_items(hdl, ops, V4L2_CID_TEST_PATTERN,
>> +					     ARRAY_SIZE(vd56g3_tp_menu) - 1, 0,
>> +					     0, vd56g3_tp_menu);
>> +
>> +	ctrl = v4l2_ctrl_new_int_menu(hdl, ops, V4L2_CID_LINK_FREQ,
>> +				      ARRAY_SIZE(vd56g3_link_freq_1lane) - 1, 0,
>> +				      (sensor->nb_of_lane == 2) ?
>> +					      vd56g3_link_freq_2lanes :
>> +					      vd56g3_link_freq_1lane);
>> +	if (ctrl)
>> +		ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>> +
>> +	ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_PIXEL_RATE,
>> +				 sensor->pixel_clock, sensor->pixel_clock, 1,
>> +				 sensor->pixel_clock);
>> +	if (ctrl)
>> +		ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>> +
>> +	sensor->ae_ctrl = v4l2_ctrl_new_std_menu(hdl, ops,
>> +						 V4L2_CID_EXPOSURE_AUTO,
>> +						 V4L2_EXPOSURE_MANUAL, 0,
>> +						 V4L2_EXPOSURE_AUTO);
>> +
>> +	sensor->ae_lock_ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_3A_LOCK, 0,
>> +						 GENMASK(2, 0), 0, 0);
>> +
>> +	sensor->ae_bias_ctrl =
>> +		v4l2_ctrl_new_int_menu(hdl, ops, V4L2_CID_AUTO_EXPOSURE_BIAS,
>> +				       ARRAY_SIZE(vd56g3_ev_bias_qmenu) - 1,
>> +				       ARRAY_SIZE(vd56g3_ev_bias_qmenu) / 2,
>> +				       vd56g3_ev_bias_qmenu);
>> +
>> +	/*
>> +	 * Analog gain [1, 8] is computed with the following logic :
>> +	 * 32/(32 - again_reg), with again_reg in the range [0:28]
>> +	 * Digital gain [1.00, 8.00] is coded as a Fixed Point 5.8
>> +	 */
>> +	sensor->again_ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_ANALOGUE_GAIN,
>> +					       0, 28, 1, 0);
>> +	sensor->dgain_ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_DIGITAL_GAIN,
>> +					       0x100, 0x800, 1, 0x100);
>> +
>> +	/*
>> +	 * Set the exposure, horizontal and vertical blanking ctrls
>> +	 * to hardcoded values, they will be updated in vd56g3_update_controls.
>> +	 * Exposure being in an auto-cluster, set a significant value here.
>> +	 */
>> +	sensor->expo_ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_EXPOSURE,
>> +					      VD56G3_EXPOSURE_DEFAULT,
>> +					      VD56G3_EXPOSURE_DEFAULT, 1,
>> +					      VD56G3_EXPOSURE_DEFAULT);
>> +	sensor->hblank_ctrl =
>> +		v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HBLANK, 1, 1, 1, 1);
>> +	if (sensor->hblank_ctrl)
>> +		sensor->hblank_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>> +	sensor->vblank_ctrl =
>> +		v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VBLANK, 1, 1, 1, 1);
>> +
>> +	/* Additional control based on device tree properties */
>> +	if (sensor->ext_leds_mask)
>> +		sensor->led_ctrl =
>> +			v4l2_ctrl_new_std_menu(hdl, ops,
>> +					       V4L2_CID_FLASH_LED_MODE,
>> +					       V4L2_FLASH_LED_MODE_FLASH, 0,
>> +					       V4L2_FLASH_LED_MODE_NONE);
>> +
>> +	if (hdl->error) {
>> +		ret = hdl->error;
>> +		goto free_ctrls;
>> +	}
>> +
>> +	v4l2_ctrl_cluster(2, &sensor->hflip_ctrl);
>> +	v4l2_ctrl_auto_cluster(4, &sensor->ae_ctrl, V4L2_EXPOSURE_MANUAL, true);
>> +
>> +	sensor->sd.ctrl_handler = hdl;
>> +
>> +	return 0;
>> +
>> +free_ctrls:
>> +	v4l2_ctrl_handler_free(hdl);
>> +
>> +	return ret;
>> +}
>> +
>> +/* -----------------------------------------------------------------------------
>> + * Videos ops
>> + */
>> +
>> +static int vd56g3_stream_on(struct vd56g3 *sensor,
>> +			    struct v4l2_subdev_state *state)
>> +{
>> +	const struct v4l2_mbus_framefmt *format;
>> +	const struct v4l2_rect *crop;
>> +	unsigned int csi_mbps = ((sensor->nb_of_lane == 2) ?
>> +					 VD56G3_LINK_FREQ_DEF_2LANES :
>> +					 VD56G3_LINK_FREQ_DEF_1LANE) * 2 / MEGA;
>> +	unsigned int binning;
>> +	int ret = 0;
>> +
>> +	format = v4l2_subdev_state_get_format(state, 0);
>> +	crop = v4l2_subdev_state_get_crop(state, 0);
> 
> You could do the assignments in variable declaration.

Ok, applied for V3

> 
>> +
>> +	/* configure clocks */
>> +	cci_write(sensor->regmap, VD56G3_REG_EXT_CLOCK, sensor->xclk_freq,
>> +		  &ret);
>> +	cci_write(sensor->regmap, VD56G3_REG_CLK_PLL_PREDIV, sensor->pll_prediv,
>> +		  &ret);
>> +	cci_write(sensor->regmap, VD56G3_REG_CLK_SYS_PLL_MULT, sensor->pll_mult,
>> +		  &ret);
>> +
>> +	/* configure output */
>> +	cci_write(sensor->regmap, VD56G3_REG_FORMAT_CTRL,
>> +		  vd56g3_get_bpp(format->code), &ret);
>> +	cci_write(sensor->regmap, VD56G3_REG_OIF_CTRL, sensor->oif_ctrl, &ret);
>> +	cci_write(sensor->regmap, VD56G3_REG_OIF_CSI_BITRATE, csi_mbps, &ret);
>> +	cci_write(sensor->regmap, VD56G3_REG_OIF_IMG_CTRL,
>> +		  vd56g3_get_datatype(format->code), &ret);
>> +	cci_write(sensor->regmap, VD56G3_REG_ISL_ENABLE, 0, &ret);
>> +
>> +	/* configure binning mode */
>> +	switch (crop->width / format->width) {
>> +	case 1:
>> +	default:
>> +		binning = READOUT_NORMAL;
>> +		break;
>> +	case 2:
>> +		binning = READOUT_DIGITAL_BINNING_X2;
>> +		break;
>> +	}
>> +	cci_write(sensor->regmap, VD56G3_REG_READOUT_CTRL, binning, &ret);
>> +
>> +	/* configure ROIs */
>> +	cci_write(sensor->regmap, VD56G3_REG_Y_START, crop->top, &ret);
>> +	cci_write(sensor->regmap, VD56G3_REG_Y_END,
>> +		  crop->top + crop->height - 1, &ret);
>> +	cci_write(sensor->regmap, VD56G3_REG_OUT_ROI_X_START, crop->left, &ret);
>> +	cci_write(sensor->regmap, VD56G3_REG_OUT_ROI_X_END,
>> +		  crop->left + crop->width - 1, &ret);
>> +	cci_write(sensor->regmap, VD56G3_REG_OUT_ROI_Y_START, 0, &ret);
>> +	cci_write(sensor->regmap, VD56G3_REG_OUT_ROI_Y_END, crop->height - 1,
>> +		  &ret);
>> +	cci_write(sensor->regmap, VD56G3_REG_AE_ROI_START_H, crop->left, &ret);
>> +	cci_write(sensor->regmap, VD56G3_REG_AE_ROI_END_H,
>> +		  crop->left + crop->width - 1, &ret);
>> +	cci_write(sensor->regmap, VD56G3_REG_AE_ROI_START_V, 0, &ret);
>> +	cci_write(sensor->regmap, VD56G3_REG_AE_ROI_END_V, crop->height - 1,
>> +		  &ret);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Setup default GPIO values; could be overridden by V4L2 ctrl setup */
>> +	ret = vd56g3_write_gpiox(sensor, GENMASK(VD56G3_NB_GPIOS - 1, 0));
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Apply settings from V4L2 ctrls */
>> +	ret = __v4l2_ctrl_handler_setup(&sensor->ctrl_handler);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* start streaming */
>> +	cci_write(sensor->regmap, VD56G3_REG_STBY, VD56G3_CMD_START_STREAM,
>> +		  &ret);
>> +	vd56g3_poll_reg(sensor, VD56G3_REG_STBY, VD56G3_CMD_ACK, &ret);
>> +	vd56g3_wait_state(sensor, VD56G3_SYSTEM_FSM_STREAMING, &ret);
>> +
>> +	return ret;
>> +}
>> +
>> +static int vd56g3_stream_off(struct vd56g3 *sensor)
>> +{
>> +	int ret;
>> +
>> +	/* Retrieve Expo cluster to enable coldstart of AE */
>> +	ret = vd56g3_read_expo_cluster(sensor, true);
>> +
>> +	cci_write(sensor->regmap, VD56G3_REG_STREAMING, VD56G3_CMD_STOP_STREAM,
>> +		  &ret);
>> +	vd56g3_poll_reg(sensor, VD56G3_REG_STREAMING, VD56G3_CMD_ACK, &ret);
>> +	vd56g3_wait_state(sensor, VD56G3_SYSTEM_FSM_SW_STBY, &ret);
>> +
>> +	return ret;
>> +}
>> +
>> +static int vd56g3_s_stream(struct v4l2_subdev *sd, int enable)
>> +{
>> +	struct vd56g3 *sensor = to_vd56g3(sd);
>> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
>> +	struct v4l2_subdev_state *state;
>> +	int ret = 0;
>> +
>> +	state = v4l2_subdev_lock_and_get_active_state(sd);
>> +
>> +	if (enable) {
>> +		ret = pm_runtime_resume_and_get(&client->dev);
>> +		if (ret < 0)
>> +			goto unlock;
>> +		ret = vd56g3_stream_on(sensor, state);
>> +		if (ret) {
>> +			dev_err(&client->dev, "Failed to start streaming\n");
>> +			pm_runtime_put_sync(&client->dev);
>> +		}
>> +	} else {
>> +		vd56g3_stream_off(sensor);
>> +		pm_runtime_mark_last_busy(&client->dev);
>> +		pm_runtime_put_autosuspend(&client->dev);
>> +	}
>> +
>> +	if (!ret) {
>> +		sensor->streaming = enable;
>> +
>> +		/* some controls are locked during streaming */
>> +		__v4l2_ctrl_grab(sensor->hflip_ctrl, enable);
>> +		__v4l2_ctrl_grab(sensor->vflip_ctrl, enable);
>> +		__v4l2_ctrl_grab(sensor->patgen_ctrl, enable);
>> +	}
>> +
>> +unlock:
>> +	v4l2_subdev_unlock_state(state);
>> +
>> +	return ret;
>> +}
>> +
>> +static const struct v4l2_subdev_video_ops vd56g3_video_ops = {
>> +	.s_stream = vd56g3_s_stream,
>> +};
>> +
>> +/* -----------------------------------------------------------------------------
>> + * Pad ops
>> + */
>> +
>> +/* Media bus code is dependent of :
>> + *      - 8bits or 10bits output
>> + *      - variant : Mono or RGB
>> + *      - H/V flips parameters in case of RGB
>> + */
>> +static u32 vd56g3_get_mbus_code(struct vd56g3 *sensor, u32 code)
>> +{
>> +	unsigned int i_bpp;
>> +	unsigned int j;
>> +
>> +	for (i_bpp = 0; i_bpp < ARRAY_SIZE(vd56g3_mbus_codes); i_bpp++) {
>> +		for (j = 0; j < ARRAY_SIZE(vd56g3_mbus_codes[i_bpp]); j++) {
>> +			if (vd56g3_mbus_codes[i_bpp][j] == code)
>> +				goto endloops;
>> +		}
>> +	}
>> +
>> +endloops:
>> +	if (i_bpp >= ARRAY_SIZE(vd56g3_mbus_codes))
>> +		i_bpp = 0;
>> +
>> +	if (sensor->is_mono)
>> +		j = 0;
>> +	else
>> +		j = 1 + (sensor->hflip_ctrl->val ? 1 : 0) +
>> +		    (sensor->vflip_ctrl->val ? 2 : 0);
>> +
>> +	return vd56g3_mbus_codes[i_bpp][j];
>> +}
>> +
>> +static int vd56g3_enum_mbus_code(struct v4l2_subdev *sd,
>> +				 struct v4l2_subdev_state *sd_state,
>> +				 struct v4l2_subdev_mbus_code_enum *code)
>> +{
>> +	struct vd56g3 *sensor = to_vd56g3(sd);
>> +
>> +	if (code->index >= ARRAY_SIZE(vd56g3_mbus_codes))
>> +		return -EINVAL;
>> +
>> +	code->code =
>> +		vd56g3_get_mbus_code(sensor, vd56g3_mbus_codes[code->index][0]);
>> +
>> +	return 0;
>> +}
>> +
>> +static int vd56g3_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(vd56g3_supported_modes))
>> +		return -EINVAL;
>> +
>> +	fse->min_width = vd56g3_supported_modes[fse->index].width;
>> +	fse->max_width = fse->min_width;
>> +	fse->min_height = vd56g3_supported_modes[fse->index].height;
>> +	fse->max_height = fse->min_height;
>> +
>> +	return 0;
>> +}
>> +
>> +static void vd56g3_update_img_pad_format(struct vd56g3 *sensor,
>> +					 const struct vd56g3_mode *mode,
>> +					 u32 mbus_code,
>> +					 struct v4l2_mbus_framefmt *mbus_fmt)
>> +{
>> +	mbus_fmt->width = mode->width;
>> +	mbus_fmt->height = mode->height;
>> +	mbus_fmt->code = vd56g3_get_mbus_code(sensor, mbus_code);
>> +	mbus_fmt->colorspace = V4L2_COLORSPACE_RAW;
>> +	mbus_fmt->field = V4L2_FIELD_NONE;
>> +	mbus_fmt->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
>> +	mbus_fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
>> +	mbus_fmt->xfer_func = V4L2_XFER_FUNC_NONE;
>> +}
>> +
>> +static int vd56g3_set_pad_fmt(struct v4l2_subdev *sd,
>> +			      struct v4l2_subdev_state *sd_state,
>> +			      struct v4l2_subdev_format *sd_fmt)
>> +{
>> +	struct vd56g3 *sensor = to_vd56g3(sd);
>> +	const struct vd56g3_mode *new_mode;
>> +	struct v4l2_rect pad_crop;
>> +	unsigned int binning;
>> +	int ret = 0;
>> +
>> +	if (sensor->streaming)
>> +		return -EBUSY;
> 
> Setting the try format should be possible during streaming.

Good point, this sensor->streaming check results from an old copy/paste.
I had a quick look at imx219 driver and it's implemented differently, I
need to see if I can align, this could allow to drop the 'streaming'
boolean (which is never used elesewhere).

> 
>> +
>> +	new_mode = v4l2_find_nearest_size(vd56g3_supported_modes,
>> +					  ARRAY_SIZE(vd56g3_supported_modes),
>> +					  width, height, sd_fmt->format.width,
>> +					  sd_fmt->format.height);
>> +
>> +	vd56g3_update_img_pad_format(sensor, new_mode, sd_fmt->format.code,
>> +				     &sd_fmt->format);
>> +
>> +	/* Compute crop rectangle (maximized via binning) */
>> +	binning = min(VD56G3_NATIVE_WIDTH / sd_fmt->format.width,
>> +		      VD56G3_NATIVE_HEIGHT / sd_fmt->format.height);
>> +	binning = min(binning, 2U);
>> +	pad_crop.width = sd_fmt->format.width * binning;
>> +	pad_crop.height = sd_fmt->format.height * binning;
>> +	pad_crop.left = (VD56G3_NATIVE_WIDTH - pad_crop.width) / 2;
>> +	pad_crop.top = (VD56G3_NATIVE_HEIGHT - pad_crop.height) / 2;
>> +
>> +	/* Update active state's format and crop */
>> +	if (sd_fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE)
>> +		ret = vd56g3_update_controls(sensor);
>> +
>> +	if (!ret) {
>> +		*v4l2_subdev_state_get_format(sd_state, sd_fmt->pad) =
>> +			sd_fmt->format;
>> +		*v4l2_subdev_state_get_crop(sd_state, sd_fmt->pad) = pad_crop;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static int vd56g3_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, 0);
>> +		break;
>> +	case V4L2_SEL_TGT_NATIVE_SIZE:
>> +	case V4L2_SEL_TGT_CROP_DEFAULT:
>> +	case V4L2_SEL_TGT_CROP_BOUNDS:
>> +		sel->r.top = 0;
>> +		sel->r.left = 0;
>> +		sel->r.width = VD56G3_NATIVE_WIDTH;
>> +		sel->r.height = VD56G3_NATIVE_HEIGHT;
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int vd56g3_init_state(struct v4l2_subdev *sd,
>> +			     struct v4l2_subdev_state *sd_state)
>> +{
>> +	unsigned int def_mode = VD56G3_DEFAULT_MODE;
>> +	struct v4l2_subdev_format fmt = {
>> +		.which = V4L2_SUBDEV_FORMAT_TRY,
>> +		.pad = 0,
>> +		.format = {
>> +			.code = vd56g3_mbus_codes[0][0],
>> +			.width = vd56g3_supported_modes[def_mode].width,
>> +			.height = vd56g3_supported_modes[def_mode].height,
>> +		},
>> +	};
>> +
>> +	vd56g3_set_pad_fmt(sd, sd_state, &fmt);
> 
> I think you could just return what vd56g3_set_pad_fmt did -- it's zero
> (after fixing the streaming check in it).

Ok, make sense.

> 
>> +	return 0;
>> +}
>> +
>> +static const struct v4l2_subdev_core_ops vd56g3_core_ops = {
>> +	.subscribe_event = v4l2_ctrl_subdev_subscribe_event,
>> +	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
>> +};
>> +
>> +static const struct v4l2_subdev_pad_ops vd56g3_pad_ops = {
>> +	.enum_mbus_code = vd56g3_enum_mbus_code,
>> +	.enum_frame_size = vd56g3_enum_frame_size,
>> +	.get_fmt = v4l2_subdev_get_fmt,
>> +	.set_fmt = vd56g3_set_pad_fmt,
>> +	.get_selection = vd56g3_get_selection,
>> +};
>> +
>> +static const struct v4l2_subdev_ops vd56g3_subdev_ops = {
>> +	.core = &vd56g3_core_ops,
>> +	.video = &vd56g3_video_ops,
>> +	.pad = &vd56g3_pad_ops,
>> +};
>> +
>> +static const struct media_entity_operations vd56g3_subdev_entity_ops = {
>> +	.link_validate = v4l2_subdev_link_validate,
>> +};
>> +
>> +static const struct v4l2_subdev_internal_ops vd56g3_internal_ops = {
>> +	.init_state = vd56g3_init_state,
>> +};
>> +
>> +/* -----------------------------------------------------------------------------
>> + * Power management
>> + */
>> +
>> +static int vd56g3_power_on(struct vd56g3 *sensor)
>> +{
>> +	struct i2c_client *client = sensor->i2c_client;
>> +	int ret;
>> +
>> +	/* power on */
>> +	ret = regulator_bulk_enable(ARRAY_SIZE(sensor->supplies),
>> +				    sensor->supplies);
>> +	if (ret) {
>> +		dev_err(&client->dev, "Failed to enable regulators %d", ret);
>> +		return ret;
>> +	}
>> +
>> +	ret = clk_prepare_enable(sensor->xclk);
>> +	if (ret) {
>> +		dev_err(&client->dev, "Failed to enable clock %d", ret);
>> +		goto disable_reg;
>> +	}
>> +
>> +	gpiod_set_value_cansleep(sensor->reset_gpio, 0);
>> +	usleep_range(3500, 4000);
>> +	ret = vd56g3_wait_state(sensor, VD56G3_SYSTEM_FSM_READY_TO_BOOT, NULL);
>> +	if (ret) {
>> +		dev_err(&client->dev, "Sensor reset failed %d\n", ret);
>> +		goto disable_clock;
>> +	}
>> +
>> +	/* boot sensor */
>> +	cci_write(sensor->regmap, VD56G3_REG_BOOT, VD56G3_CMD_BOOT, &ret);
>> +	vd56g3_poll_reg(sensor, VD56G3_REG_BOOT, VD56G3_CMD_ACK, &ret);
>> +	vd56g3_wait_state(sensor, VD56G3_SYSTEM_FSM_SW_STBY, &ret);
>> +	if (ret) {
>> +		dev_err(&client->dev, "sensor boot failed %d", ret);
>> +		goto disable_clock;
>> +	}
>> +
>> +	return 0;
>> +
>> +disable_clock:
>> +	clk_disable_unprepare(sensor->xclk);
>> +	gpiod_set_value_cansleep(sensor->reset_gpio, 1);
>> +disable_reg:
>> +	regulator_bulk_disable(ARRAY_SIZE(sensor->supplies), sensor->supplies);
>> +
>> +	return ret;
>> +}
>> +
>> +static int vd56g3_power_off(struct vd56g3 *sensor)
>> +{
>> +	clk_disable_unprepare(sensor->xclk);
>> +	gpiod_set_value_cansleep(sensor->reset_gpio, 1);
>> +	regulator_bulk_disable(ARRAY_SIZE(sensor->supplies), sensor->supplies);
>> +	return 0;
> 
> You can make the return type void.
> 
> Do you need two pairs of functions doing the same, or could you call
> vd56g3_runtime_resume and vd56g3_runtime_suspend from driver's probe and
> remove functions, too?

"Well, in fact, I tested both options before submitting V2 (I mean the
unification of vd56g3_runtime_resume/suspend functions with
vd56g3_power_on/off).

The unification option has the advantage of simplifying the code and
removing two "useless" functions. The only drawback is that I had to
call v4l2_i2c_subdev_init() earlier in the probe() function, whereas
it's currently called in vd56g3_subdev_init() (currently at the end of
the probe()). OK, it's not a big deal, but I find that the resulting
code is not as well structured/divided (thus readable).

I'm interested to get your feedback to decide wich option to push for V3.

> 
>> +}
>> +
>> +static int vd56g3_runtime_resume(struct device *dev)
>> +{
>> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
>> +	struct vd56g3 *sensor = to_vd56g3(sd);
>> +	struct i2c_client *client = sensor->i2c_client;
>> +	int ret;
>> +
>> +	ret = vd56g3_power_on(sensor);
>> +	if (ret)
>> +		dev_err(&client->dev, "Failed to power on %d", ret);
> 
> You seem to be printing more precise messages in vd56g3_power_on() already.

Sure, linked with previous comment.

> 
>> +
>> +	return ret;
>> +}
>> +
>> +static int vd56g3_runtime_suspend(struct device *dev)
>> +{
>> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
>> +	struct vd56g3 *sensor = to_vd56g3(sd);
>> +
>> +	return vd56g3_power_off(sensor);
>> +}
>> +
>> +static const struct dev_pm_ops vd56g3_pm_ops = {
>> +	SET_RUNTIME_PM_OPS(vd56g3_runtime_suspend, vd56g3_runtime_resume, NULL)
>> +};
>> +
>> +/* -----------------------------------------------------------------------------
>> + * Probe and initialization
>> + */
>> +
>> +static int vd56g3_check_csi_conf(struct vd56g3 *sensor,
>> +				 struct fwnode_handle *endpoint)
>> +{
>> +	struct i2c_client *client = sensor->i2c_client;
>> +	struct v4l2_fwnode_endpoint ep = { .bus_type = V4L2_MBUS_CSI2_DPHY };
>> +	u32 phy_data_lanes[VD56G3_MAX_CSI_DATA_LANES] = { ~0, ~0 };
>> +	u8 n_lanes;
>> +	u64 frequency;
>> +	int p, l;
>> +	int ret = 0;
>> +
>> +	ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &ep);
>> +	if (ret)
>> +		return -EINVAL;
>> +
>> +	/* Check lanes number */
>> +	n_lanes = ep.bus.mipi_csi2.num_data_lanes;
>> +	if (n_lanes != 1 && n_lanes != 2) {
>> +		dev_err(&client->dev, "Invalid data lane number %d\n", n_lanes);
>> +		ret = -EINVAL;
>> +		goto done;
>> +	}
>> +	sensor->nb_of_lane = n_lanes;
>> +
>> +	/* Clock lane must be first */
>> +	if (ep.bus.mipi_csi2.clock_lane != 0) {
>> +		dev_err(&client->dev, "Clk lane must be mapped to lane 0\n");
>> +		ret = -EINVAL;
>> +		goto done;
>> +	}
>> +
>> +	/* Prepare Output Interface conf based on lane settings
> 
> /*
>  * Multi-line
>  * comment.
>  */

Ok, will be fixed in V3.

> 
>> +	 * logical to physical lane conversion (+ pad remaining slots)
>> +	 */
>> +	for (l = 0; l < n_lanes; l++)
>> +		phy_data_lanes[ep.bus.mipi_csi2.data_lanes[l] - 1] = l;
>> +	for (p = 0; p < VD56G3_MAX_CSI_DATA_LANES; p++) {
>> +		if (phy_data_lanes[p] != ~0)
>> +			continue;
>> +		phy_data_lanes[p] = l;
>> +		l++;
>> +	}
>> +	sensor->oif_ctrl = n_lanes |
>> +			   (ep.bus.mipi_csi2.lane_polarities[0] << 3) |
>> +			   ((phy_data_lanes[0]) << 4) |
>> +			   (ep.bus.mipi_csi2.lane_polarities[1] << 6) |
>> +			   ((phy_data_lanes[1]) << 7) |
>> +			   (ep.bus.mipi_csi2.lane_polarities[2] << 9);
>> +
>> +	/* Check link frequency */
>> +	if (!ep.nr_of_link_frequencies) {
>> +		dev_err(&client->dev, "link-frequency not found in DT\n");
>> +		ret = -EINVAL;
>> +		goto done;
>> +	}
>> +	frequency = (n_lanes == 2) ? VD56G3_LINK_FREQ_DEF_2LANES :
>> +				     VD56G3_LINK_FREQ_DEF_1LANE;
>> +	if (ep.nr_of_link_frequencies != 1 ||
>> +	    ep.link_frequencies[0] != frequency) {
>> +		dev_err(&client->dev, "Link frequency not supported: %lld\n",
>> +			ep.link_frequencies[0]);
>> +		ret = -EINVAL;
>> +		goto done;
>> +	}
>> +
>> +done:
>> +	v4l2_fwnode_endpoint_free(&ep);
>> +	return ret;
>> +}
>> +
>> +static int vd56g3_parse_dt_gpios_array(struct vd56g3 *sensor, char *prop_name,
>> +				       u32 *array, int *nb)
>> +{
>> +	struct i2c_client *client = sensor->i2c_client;
>> +	struct device_node *np = client->dev.of_node;
>> +	unsigned int i;
>> +
>> +	*nb = of_property_read_variable_u32_array(np, prop_name, array, 0,
>> +						  VD56G3_NB_GPIOS);
>> +
>> +	if (*nb == -EINVAL) {
>> +		*nb = 0;
>> +		return *nb;
>> +	} else if (*nb < 0) {
>> +		dev_err(&client->dev, "Failed to read %s prop\n", prop_name);
>> +		return *nb;
>> +	}
>> +
>> +	for (i = 0; i < *nb; i++) {
>> +		if (array[i] >= VD56G3_NB_GPIOS) {
>> +			dev_err(&client->dev, "Invalid GPIO : %d\n", array[i]);
>> +			return -EINVAL;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int vd56g3_parse_dt_gpios(struct vd56g3 *sensor)
>> +{
>> +	u32 led_gpios[VD56G3_NB_GPIOS];
>> +	int nb_gpios_leds;
>> +	unsigned int i;
>> +	int ret;
>> +
>> +	/* Initialize GPIOs to default */
>> +	for (i = 0; i < VD56G3_NB_GPIOS; i++)
>> +		sensor->gpios[i] = VD56G3_GPIOX_GPIO_IN;
>> +	sensor->ext_leds_mask = 0;
>> +
>> +	/* Take into account optional 'st,leds' output for GPIOs */
>> +	ret = vd56g3_parse_dt_gpios_array(sensor, "st,leds", led_gpios,
>> +					  &nb_gpios_leds);
>> +	if (ret)
>> +		return ret;
>> +	for (i = 0; i < nb_gpios_leds; i++) {
>> +		sensor->gpios[led_gpios[i]] = VD56G3_GPIOX_STROBE_MODE;
>> +		set_bit(led_gpios[i], &sensor->ext_leds_mask);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int vd56g3_parse_dt(struct vd56g3 *sensor)
>> +{
>> +	struct i2c_client *client = sensor->i2c_client;
>> +	struct device *dev = &client->dev;
>> +	struct fwnode_handle *endpoint;
>> +	int ret;
>> +
>> +	endpoint = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), 0, 0, 0);
>> +	if (!endpoint) {
>> +		dev_err(dev, "endpoint node not found\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	ret = vd56g3_check_csi_conf(sensor, endpoint);
>> +	fwnode_handle_put(endpoint);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return vd56g3_parse_dt_gpios(sensor);
>> +}
>> +
>> +static int vd56g3_get_regulators(struct vd56g3 *sensor)
>> +{
>> +	unsigned int i;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(sensor->supplies); i++)
>> +		sensor->supplies[i].supply = vd56g3_supply_names[i];
>> +
>> +	return devm_regulator_bulk_get(&sensor->i2c_client->dev,
>> +				       ARRAY_SIZE(sensor->supplies),
>> +				       sensor->supplies);
>> +}
>> +
>> +static int vd56g3_prepare_clock_tree(struct vd56g3 *sensor)
>> +{
>> +	struct i2c_client *client = sensor->i2c_client;
>> +	const unsigned int predivs[] = { 1, 2, 4 };
>> +	u32 pll_out;
>> +	int i;
>> +
>> +	/* External clock must be in [6Mhz-27Mhz] */
>> +	if (sensor->xclk_freq < 6 * HZ_PER_MHZ ||
>> +	    sensor->xclk_freq > 27 * HZ_PER_MHZ) {
>> +		dev_err(&client->dev,
>> +			"Only 6Mhz-27Mhz clock range supported. Provided %lu MHz\n",
>> +			sensor->xclk_freq / HZ_PER_MHZ);
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* PLL input should be in [6Mhz-12Mhz[ */
>> +	for (i = 0; i < ARRAY_SIZE(predivs); i++) {
>> +		sensor->pll_prediv = predivs[i];
>> +		if (sensor->xclk_freq / sensor->pll_prediv < 12 * HZ_PER_MHZ)
>> +			break;
>> +	}
>> +
>> +	/* PLL output clock must be as close as possible to 804Mhz */
>> +	sensor->pll_mult = (VD56G3_TARGET_PLL * sensor->pll_prediv +
>> +			    sensor->xclk_freq / 2) /
>> +			   sensor->xclk_freq;
>> +	pll_out = sensor->xclk_freq * sensor->pll_mult / sensor->pll_prediv;
>> +
>> +	/* Target Pixel Clock for standard 10bit ADC mode : 160.8Mhz */
>> +	sensor->pixel_clock = pll_out / VD56G3_VT_CLOCK_DIV;
>> +
>> +	return 0;
>> +}
>> +
>> +static int vd56g3_detect(struct vd56g3 *sensor)
>> +{
>> +	struct i2c_client *client = sensor->i2c_client;
>> +	struct device *dev = &client->dev;
>> +	unsigned int model;
>> +	u64 model_id = 0;
>> +	u64 device_revision = 0;
>> +	u64 optical_revision = 0;
>> +	int ret = 0;
>> +
>> +	model = (uintptr_t)device_get_match_data(dev);
>> +
>> +	cci_read(sensor->regmap, VD56G3_REG_MODEL_ID, &model_id, &ret);
>> +	if (ret)
>> +		return ret;
> 
> As you're calling cci_read() and then checking ret, I'd assing ret here the
> usual way (and below, too).
> 

Yes, better !

>> +
>> +	if (model_id != VD56G3_MODEL_ID) {
>> +		dev_err(&client->dev, "Unsupported sensor id %x",
>> +			(u16)model_id);
>> +		return -ENODEV;
>> +	}
>> +
>> +	cci_read(sensor->regmap, VD56G3_REG_REVISION, &device_revision, &ret);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if ((device_revision >> 8) != VD56G3_REVISION_CUT3) {
>> +		dev_err(&client->dev, "Unsupported Cut version %x",
>> +			(u16)device_revision);
>> +		return -ENODEV;
>> +	}
>> +
>> +	cci_read(sensor->regmap, VD56G3_REG_OPTICAL_REVISION, &optical_revision,
>> +		 &ret);
>> +	if (ret)
>> +		return ret;
>> +
>> +	sensor->is_mono =
>> +		((optical_revision & 1) == VD56G3_OPTICAL_REVISION_MONO);
>> +	if ((sensor->is_mono && model == VD56G3_MODEL_VD66GY) ||
>> +	    (!sensor->is_mono && model == VD56G3_MODEL_VD56G3)) {
>> +		dev_err(&client->dev,
>> +			"Found %s sensor, while %s model is defined in DT",
>> +			(sensor->is_mono) ? "Mono" : "Bayer",
>> +			(model == VD56G3_MODEL_VD56G3) ? "vd56g3" : "vd66gy");
>> +		return -ENODEV;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int vd56g3_subdev_init(struct vd56g3 *sensor)
>> +{
>> +	struct i2c_client *client = sensor->i2c_client;
>> +	int ret;
>> +
>> +	/* Init sub device */
>> +	v4l2_i2c_subdev_init(&sensor->sd, client, &vd56g3_subdev_ops);
>> +	sensor->sd.internal_ops = &vd56g3_internal_ops;
>> +	sensor->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
>> +			    V4L2_SUBDEV_FL_HAS_EVENTS;
>> +	sensor->sd.entity.ops = &vd56g3_subdev_entity_ops;
>> +
>> +	/* Init source pad */
>> +	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(&client->dev, "Failed to init media entity : %d", ret);
>> +		return ret;
>> +	}
>> +
>> +	/* Init controls */
>> +	ret = vd56g3_init_controls(sensor);
>> +	if (ret) {
>> +		dev_err(&client->dev, "Controls initialization failed %d", ret);
>> +		goto err_media;
>> +	}
>> +
>> +	/* Init vd56g3 struct : default resolution + raw8 */
>> +	sensor->sd.state_lock = sensor->ctrl_handler.lock;
>> +	ret = v4l2_subdev_init_finalize(&sensor->sd);
>> +	if (ret) {
>> +		dev_err(&client->dev, "subdev init error: %d", ret);
>> +		goto err_ctrls;
>> +	}
>> +
>> +	return vd56g3_update_controls(sensor);
>> +
>> +err_ctrls:
>> +	v4l2_ctrl_handler_free(sensor->sd.ctrl_handler);
>> +
>> +err_media:
>> +	media_entity_cleanup(&sensor->sd.entity);
> 
> Newline here?

Yes, there's a few others newline missing, will be fixed in V3.

> 
>> +	return ret;
>> +}
>> +
>> +static void vd56g3_subdev_cleanup(struct vd56g3 *sensor)
>> +{
>> +	v4l2_async_unregister_subdev(&sensor->sd);
>> +	v4l2_subdev_cleanup(&sensor->sd);
>> +	media_entity_cleanup(&sensor->sd.entity);
>> +	v4l2_ctrl_handler_free(sensor->sd.ctrl_handler);
>> +}
>> +
>> +static int vd56g3_probe(struct i2c_client *client)
>> +{
>> +	struct device *dev = &client->dev;
>> +	struct vd56g3 *sensor;
>> +	int ret;
>> +
>> +	sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL);
>> +	if (!sensor)
>> +		return -ENOMEM;
>> +
>> +	sensor->i2c_client = client;
>> +
>> +	ret = vd56g3_parse_dt(sensor);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "Failed to parse Device Tree.");
>> +
>> +	/* Get (and check) resources : power regs, ext clock, reset gpio */
>> +	ret = vd56g3_get_regulators(sensor);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "Failed to get regulators.");
>> +
>> +	sensor->xclk = devm_clk_get(dev, NULL);
>> +	if (IS_ERR(sensor->xclk))
>> +		return dev_err_probe(dev, PTR_ERR(sensor->xclk),
>> +				     "Failed to get xclk.");
>> +	sensor->xclk_freq = clk_get_rate(sensor->xclk);
>> +	ret = vd56g3_prepare_clock_tree(sensor);
>> +	if (ret)
>> +		return ret;
>> +
>> +	sensor->reset_gpio =
>> +		devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
>> +	if (IS_ERR(sensor->reset_gpio))
>> +		return dev_err_probe(dev, PTR_ERR(sensor->reset_gpio),
>> +				     "Failed to get reset gpio.");
>> +
>> +	sensor->regmap = devm_cci_regmap_init_i2c(client, 16);
>> +	if (IS_ERR(sensor->regmap))
>> +		return dev_err_probe(dev, PTR_ERR(sensor->regmap),
>> +				     "Failed to init regmap.");
>> +
>> +	/* Power ON */
>> +	ret = vd56g3_power_on(sensor);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "Sensor power on failed.");
>> +
>> +	/* Enable PM runtime with autosuspend (sensor being ON, set active) */
>> +	pm_runtime_set_active(dev);
>> +	pm_runtime_get_noresume(dev);
>> +	pm_runtime_enable(dev);
>> +	pm_runtime_set_autosuspend_delay(dev, 1000);
>> +	pm_runtime_use_autosuspend(dev);
>> +
>> +	/* Check HW model/version */
>> +	ret = vd56g3_detect(sensor);
>> +	if (ret) {
>> +		dev_err(&client->dev, "Sensor detect failed : %d", ret);
>> +		goto err_power_off;
>> +	}
>> +
>> +	/* Initialize, then register V4L2 subdev */
>> +	ret = vd56g3_subdev_init(sensor);
>> +	if (ret) {
>> +		dev_err(&client->dev, "V4l2 init failed : %d", ret);
>> +		goto err_power_off;
>> +	}
>> +
>> +	ret = v4l2_async_register_subdev(&sensor->sd);
>> +	if (ret) {
>> +		dev_err(&client->dev, "async subdev register failed %d", ret);
>> +		goto err_subdev;
>> +	}
>> +
>> +	/* Sensor could now be powered off (after the autosuspend delay) */
>> +	pm_runtime_mark_last_busy(dev);
>> +	pm_runtime_put_autosuspend(dev);
>> +
>> +	dev_dbg(&client->dev, "Successfully probe %s sensor",
>> +		(sensor->is_mono) ? "vd56g3" : "vd66gy");
>> +
>> +	return 0;
>> +
>> +err_subdev:
>> +	vd56g3_subdev_cleanup(sensor);
>> +err_power_off:
>> +	pm_runtime_disable(dev);
>> +	pm_runtime_put_noidle(dev);
>> +	vd56g3_power_off(sensor);
> 
> A newline here?

Ok

> 
>> +	return ret;
>> +}
>> +
>> +static void vd56g3_remove(struct i2c_client *client)
>> +{
>> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
>> +	struct vd56g3 *sensor = to_vd56g3(sd);
>> +
>> +	vd56g3_subdev_cleanup(sensor);
>> +
>> +	pm_runtime_disable(&client->dev);
>> +	if (!pm_runtime_status_suspended(&client->dev))
>> +		vd56g3_power_off(sensor);
>> +	pm_runtime_set_suspended(&client->dev);
>> +}
>> +
>> +static const struct of_device_id vd56g3_dt_ids[] = {
>> +	{ .compatible = "st,st-vd56g3", .data = (void *)VD56G3_MODEL_VD56G3 },
>> +	{ .compatible = "st,st-vd66gy", .data = (void *)VD56G3_MODEL_VD66GY },
>> +	{ /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, vd56g3_dt_ids);
>> +
>> +static struct i2c_driver vd56g3_i2c_driver = {
>> +	.driver = {
>> +		.name  = "st-vd56g3",
>> +		.of_match_table = vd56g3_dt_ids,
>> +		.pm = &vd56g3_pm_ops,
>> +	},
>> +	.probe = vd56g3_probe,
>> +	.remove = vd56g3_remove,
>> +};
>> +
>> +module_i2c_driver(vd56g3_i2c_driver);
>> +
>> +MODULE_AUTHOR("Benjamin Mugnier <benjamin.mugnier@foss.st.com>");
>> +MODULE_AUTHOR("Mickael Guene <mickael.guene@st.com>");
>> +MODULE_AUTHOR("Sylvain Petinot <sylvain.petinot@foss.st.com>");
>> +MODULE_DESCRIPTION("ST VD56G3 sensor driver");
>> +MODULE_LICENSE("GPL");
> 

--
Sylvain

Re: [PATCH v2 2/2] media: i2c: Add driver for ST VD56G3 camera sensor

[Sakari Ailus]

Hi Sylwain,

Apologies for the delay...

On Mon, Jun 03, 2024 at 11:59:29AM +0200, Sylvain Petinot wrote:
> >> +/*
> >> + * The VD56G3 pixel array is organized as follows:
> >> + *
> >> + * +--------------------------------+
> >> + * |                                | \
> >> + * |   +------------------------+   |  |
> >> + * |   |                        |   |  |
> >> + * |   |                        |   |  |
> >> + * |   |                        |   |  |
> >> + * |   |                        |   |  |
> >> + * |   |                        |   |  |
> >> + * |   |   Default resolution   |   |  | Native height (1364)
> > 
> > What's outside the default resolution? It doesn't appear the driver would
> > allow capturing pixels out side this area.
> 
> Well both native and default resolutions are supported in the
> 'vd56g3_supported_modes' below.
> However this quite exotic resolution (1364 x 1124) isn't well supported
> by csi receivers, ISPs. That's why the default resolution of the driver
> is 1120 x 1360 (multiple of 16).

Ack.

I'd still keep the native resolution as the default and allow configuring
something else if the user space wants to.

The desired resolution really depends on the use case (as well as the ISP).

> >> +		break;
> >> +	case V4L2_CID_EXPOSURE_AUTO:
> >> +		is_auto = (ctrl->val == V4L2_EXPOSURE_AUTO);
> >> +		__v4l2_ctrl_grab(sensor->ae_lock_ctrl, !is_auto);
> >> +		__v4l2_ctrl_grab(sensor->ae_bias_ctrl, !is_auto);
> >> +		break;
> >> +	default:
> >> +		break;
> > 
> > You could omit default here.
> 
> I don't really like switch case without default. For sure I can omit,
> but I prefer making it explicit.

I'm ok with that.

...

> >> +static int vd56g3_power_off(struct vd56g3 *sensor)
> >> +{
> >> +	clk_disable_unprepare(sensor->xclk);
> >> +	gpiod_set_value_cansleep(sensor->reset_gpio, 1);
> >> +	regulator_bulk_disable(ARRAY_SIZE(sensor->supplies), sensor->supplies);
> >> +	return 0;
> > 
> > You can make the return type void.
> > 
> > Do you need two pairs of functions doing the same, or could you call
> > vd56g3_runtime_resume and vd56g3_runtime_suspend from driver's probe and
> > remove functions, too?
> 
> "Well, in fact, I tested both options before submitting V2 (I mean the
> unification of vd56g3_runtime_resume/suspend functions with
> vd56g3_power_on/off).
> 
> The unification option has the advantage of simplifying the code and
> removing two "useless" functions. The only drawback is that I had to
> call v4l2_i2c_subdev_init() earlier in the probe() function, whereas
> it's currently called in vd56g3_subdev_init() (currently at the end of
> the probe()). OK, it's not a big deal, but I find that the resulting
> code is not as well structured/divided (thus readable).
> 
> I'm interested to get your feedback to decide wich option to push for V3.

I'd prefer calling v4l2_i2c_subdev_init() earlier, in order to set the
device context. These are usually among those things that should be done as
early as possible, in order to avoid invalid pointers where much of the
driver code expects something else.

Btw. if you're not in too much hurry (I guess so?), we're just about to
rework the sensor API, to include internal pads and embedded data, for
better sensor configurability. It'll take a while before we're there
though, but when this driver is merged, the existing API must continue to
work.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v2 2/2] media: i2c: Add driver for ST VD56G3 camera sensor

[Sylvain Petinot]

Hello Sakari,

Thanks for your feedback, no worries about the delay ...

On 9/9/2024 9:06 AM, Sakari Ailus wrote:
> Hi Sylwain,
> 
> Apologies for the delay...
> 
> On Mon, Jun 03, 2024 at 11:59:29AM +0200, Sylvain Petinot wrote:
>>>> +/*
>>>> + * The VD56G3 pixel array is organized as follows:
>>>> + *
>>>> + * +--------------------------------+
>>>> + * |                                | \
>>>> + * |   +------------------------+   |  |
>>>> + * |   |                        |   |  |
>>>> + * |   |                        |   |  |
>>>> + * |   |                        |   |  |
>>>> + * |   |                        |   |  |
>>>> + * |   |                        |   |  |
>>>> + * |   |   Default resolution   |   |  | Native height (1364)
>>>
>>> What's outside the default resolution? It doesn't appear the driver would
>>> allow capturing pixels out side this area.
>>
>> Well both native and default resolutions are supported in the
>> 'vd56g3_supported_modes' below.
>> However this quite exotic resolution (1364 x 1124) isn't well supported
>> by csi receivers, ISPs. That's why the default resolution of the driver
>> is 1120 x 1360 (multiple of 16).
> 
> Ack.
> 
> I'd still keep the native resolution as the default and allow configuring
> something else if the user space wants to.
> 
> The desired resolution really depends on the use case (as well as the ISP).

Sure, I can change the default to the native... The choice I made was
more a way to simplify (my) life (several times we went into debugging
before realizing that it was the exotic resolution causing the unwanted
behavior).
But, that's probably better, I'll change it for V4.

> 
>>>> +		break;
>>>> +	case V4L2_CID_EXPOSURE_AUTO:
>>>> +		is_auto = (ctrl->val == V4L2_EXPOSURE_AUTO);
>>>> +		__v4l2_ctrl_grab(sensor->ae_lock_ctrl, !is_auto);
>>>> +		__v4l2_ctrl_grab(sensor->ae_bias_ctrl, !is_auto);
>>>> +		break;
>>>> +	default:
>>>> +		break;
>>>
>>> You could omit default here.
>>
>> I don't really like switch case without default. For sure I can omit,
>> but I prefer making it explicit.
> 
> I'm ok with that.
> 
> ...
> 
>>>> +static int vd56g3_power_off(struct vd56g3 *sensor)
>>>> +{
>>>> +	clk_disable_unprepare(sensor->xclk);
>>>> +	gpiod_set_value_cansleep(sensor->reset_gpio, 1);
>>>> +	regulator_bulk_disable(ARRAY_SIZE(sensor->supplies), sensor->supplies);
>>>> +	return 0;
>>>
>>> You can make the return type void.
>>>
>>> Do you need two pairs of functions doing the same, or could you call
>>> vd56g3_runtime_resume and vd56g3_runtime_suspend from driver's probe and
>>> remove functions, too?
>>
>> "Well, in fact, I tested both options before submitting V2 (I mean the
>> unification of vd56g3_runtime_resume/suspend functions with
>> vd56g3_power_on/off).
>>
>> The unification option has the advantage of simplifying the code and
>> removing two "useless" functions. The only drawback is that I had to
>> call v4l2_i2c_subdev_init() earlier in the probe() function, whereas
>> it's currently called in vd56g3_subdev_init() (currently at the end of
>> the probe()). OK, it's not a big deal, but I find that the resulting
>> code is not as well structured/divided (thus readable).
>>
>> I'm interested to get your feedback to decide wich option to push for V3.
> 
> I'd prefer calling v4l2_i2c_subdev_init() earlier, in order to set the
> device context. These are usually among those things that should be done as
> early as possible, in order to avoid invalid pointers where much of the
> driver code expects something else.

Yes, the V3 I pushed include this modification.

> 
> Btw. if you're not in too much hurry (I guess so?), we're just about to
> rework the sensor API, to include internal pads and embedded data, for
> better sensor configurability. It'll take a while before we're there
> though, but when this driver is merged, the existing API must continue to
> work.
> 

I have a few branches with stream API (notably for the support of status
lines), but I need a to test a bit more before pushing.


--
Sylvain