[PATCH v2 0/2] Add IMX219 CMOS image sensor support

[PATCH v2 0/2] Add IMX219 CMOS image sensor support

[Andrey Konovalov]

This patchset adds support for IMX219 CMOS image sensor from Sony.
Sony IMX219 is an 8MPix, 1/4.0-inch CMOS active pixel digital image sensor
with an active array size of 3280H x 2464V. It is programmable through
I2C interface. Image data are sent through MIPI CSI-2, which can be configured
as either 2 or 4 data lanes, but this driver currently only supports 2 lanes.
The currently supported resolutions are 3280x2464 @ 15fps, 1920x1080 @ 30fps
(cropped FOV), and 1640x1232 (2x2 binned) @ 30fps.

The driver has been tested with Raspberry Pi Camera Module v2 connected to
Raspberry Pi Zero W.

Changes since v1 [1]:

dt-bindings:
  - license updated to (GPL-2.0-only OR BSD-2-Clause)
  - non-standard 'camera-clk' property has got type, description, properties,
    and required
  - 'clock-lanes', 'data-lanes', and 'clock-noncontinuous' moved to the correct
    node, the syntax has been fixed
  - 'port' added to required properties
  - minor corrections to some other property descriptions

imx219 sensor driver:
  - fixed width variables replaced with '[unsigned] int' where appropriate
  - excessive comments dropped
  - imx219_get_format_code() returns 'codes[...]', and doesn't use an extra
    internal variable any more
  - initializing 'ret' to 0 in the beginning of imx219_set_ctrl() dropped
  - uses IMX219_XCLK_FREQ macro instead of hardcoded number
  - IMX219_REG_ORIENTATION is dropped from the mode_*x*_regs[] tables as it is 
    written by the control handler
  - imx219_stop_streaming() return value changed to void 
  - calling imx219_power_on() moved from imx219_identify_module() to
    imx219_probe(), and calling imx219_power_off() in the probe() error path
    has been added. This simplifies imx219_identify_module(), and ensures
    that the sensor is powered off after probe() and until streaming is
    started
  - comment referring to "xclr" pin changed to call it "enable pin" vs "power
    down pin" to better match the "OUT_HIGH" pin configuration

Thanks,
Andrey

[1] https://patchwork.kernel.org/cover/11284783/


Andrey Konovalov (1):
  dt-bindings: media: i2c: Add IMX219 CMOS sensor binding

Dave Stevenson (1):
  media: i2c: Add driver for Sony IMX219 sensor

 .../devicetree/bindings/media/i2c/imx219.yaml |  134 ++
 MAINTAINERS                                   |    8 +
 drivers/media/i2c/Kconfig                     |   12 +
 drivers/media/i2c/Makefile                    |    1 +
 drivers/media/i2c/imx219.c                    | 1240 +++++++++++++++++
 5 files changed, 1395 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/imx219.yaml
 create mode 100644 drivers/media/i2c/imx219.c

-- 
2.17.1

[PATCH v2 1/2] dt-bindings: media: i2c: Add IMX219 CMOS sensor binding

[Andrey Konovalov]

Add YAML device tree binding for IMX219 CMOS image sensor, and
the relevant MAINTAINERS entries.

Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
---
 .../devicetree/bindings/media/i2c/imx219.yaml | 134 ++++++++++++++++++
 MAINTAINERS                                   |   8 ++
 2 files changed, 142 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/imx219.yaml

diff --git a/Documentation/devicetree/bindings/media/i2c/imx219.yaml b/Documentation/devicetree/bindings/media/i2c/imx219.yaml
new file mode 100644
index 000000000000..b58aa49a7c03
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/imx219.yaml
@@ -0,0 +1,134 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/i2c/imx219.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Sony 1/4.0-Inch 8Mpixel CMOS Digital Image Sensor
+
+maintainers:
+  - Dave Stevenson <dave.stevenson@raspberrypi.com>
+
+description: |-
+  The Sony imx219 is a 1/4.0-inch CMOS active pixel digital image sensor
+  with an active array size of 3280H x 2464V. It is programmable through
+  I2C interface. The I2C address is fixed to 0x10 as per sensor data sheet.
+  Image data is sent through MIPI CSI-2, which is configured as either 2 or
+  4 data lanes.
+
+properties:
+  compatible:
+    const: sony,imx219
+
+  reg:
+    description: I2C device address
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    items:
+      - const: xclk
+
+  VDIG-supply:
+    description:
+      Digital I/O voltage supply, 1.8 volts
+
+  VANA-supply:
+    description:
+      Analog voltage supply, 2.8 volts
+
+  VDDL-supply:
+    description:
+      Digital core voltage supply, 1.2 volts
+
+  xclr-gpios:
+    description: |-
+      Reference to the GPIO connected to the xclr pin, if any.
+      Must be released (set high) after all supplies are applied.
+
+  camera-clk:
+    type: object
+
+    description: Clock source for imx219
+
+    properties:
+      clock-frequency: true
+
+    required:
+      - clock-frequency
+
+  # See ../video-interfaces.txt for more details
+  port:
+    type: object
+    properties:
+      endpoint:
+        type: object
+        properties:
+          clock-lanes:
+            const: 0
+
+          data-lanes:
+            description: |-
+              Should be <1 2> for two-lane operation, or <1 2 3 4> for
+              four-lane operation.
+            oneOf:
+              - const: [[ 1, 2 ]]
+              - const: [[ 1, 2, 3, 4 ]]
+
+          clock-noncontinuous:
+            type: boolean
+            description: |-
+              Presence of this boolean property decides whether the MIPI CSI-2
+              clock is continuous or non-continuous.
+
+        required:
+          - clock-lanes
+          - data-lanes
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - VANA-supply
+  - VDIG-supply
+  - VDDL-supply
+  - port
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c0 {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        imx219: sensor@10 {
+            compatible = "sony,imx219";
+            reg = <0x10>;
+            clocks = <&imx219_clk>;
+            clock-names = "xclk";
+            VANA-supply = <&imx219_vana>;   /* 2.8v */
+            VDIG-supply = <&imx219_vdig>;   /* 1.8v */
+            VDDL-supply = <&imx219_vddl>;   /* 1.2v */
+
+            imx219_clk: camera-clk {
+                compatible = "fixed-clock";
+                #clock-cells = <0>;
+                clock-frequency = <24000000>;
+            };
+
+            port {
+                imx219_0: endpoint {
+                    remote-endpoint = <&csi1_ep>;
+                    clock-lanes = <0>;
+                    data-lanes = <1 2>;
+                    clock-noncontinuous;
+                };
+            };
+        };
+    };
+
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index ffa3371bc750..f7b6c24ec081 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15350,6 +15350,14 @@ S:	Maintained
 F:	drivers/media/i2c/imx214.c
 F:	Documentation/devicetree/bindings/media/i2c/sony,imx214.txt
 
+SONY IMX219 SENSOR DRIVER
+M:	Dave Stevenson <dave.stevenson@raspberrypi.com>
+L:	linux-media@vger.kernel.org
+T:	git git://linuxtv.org/media_tree.git
+S:	Maintained
+F:	drivers/media/i2c/imx219.c
+F:	Documentation/devicetree/bindings/media/i2c/imx219.yaml
+
 SONY IMX258 SENSOR DRIVER
 M:	Sakari Ailus <sakari.ailus@linux.intel.com>
 L:	linux-media@vger.kernel.org
-- 
2.17.1

Re: [PATCH v2 1/2] dt-bindings: media: i2c: Add IMX219 CMOS sensor binding

[Sakari Ailus]

Hi Andrey,

Thanks for the patchset.

On Fri, Dec 27, 2019 at 03:21:13PM +0300, Andrey Konovalov wrote:
> Add YAML device tree binding for IMX219 CMOS image sensor, and
> the relevant MAINTAINERS entries.
> 
> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
> ---
>  .../devicetree/bindings/media/i2c/imx219.yaml | 134 ++++++++++++++++++
>  MAINTAINERS                                   |   8 ++
>  2 files changed, 142 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/imx219.yaml
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/imx219.yaml b/Documentation/devicetree/bindings/media/i2c/imx219.yaml
> new file mode 100644
> index 000000000000..b58aa49a7c03
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/imx219.yaml
> @@ -0,0 +1,134 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/i2c/imx219.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Sony 1/4.0-Inch 8Mpixel CMOS Digital Image Sensor
> +
> +maintainers:
> +  - Dave Stevenson <dave.stevenson@raspberrypi.com>
> +
> +description: |-
> +  The Sony imx219 is a 1/4.0-inch CMOS active pixel digital image sensor
> +  with an active array size of 3280H x 2464V. It is programmable through
> +  I2C interface. The I2C address is fixed to 0x10 as per sensor data sheet.
> +  Image data is sent through MIPI CSI-2, which is configured as either 2 or
> +  4 data lanes.
> +
> +properties:
> +  compatible:
> +    const: sony,imx219
> +
> +  reg:
> +    description: I2C device address
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    items:
> +      - const: xclk

There's a single clock. Does it need a name? I'd just omit it.

> +
> +  VDIG-supply:
> +    description:
> +      Digital I/O voltage supply, 1.8 volts
> +
> +  VANA-supply:
> +    description:
> +      Analog voltage supply, 2.8 volts
> +
> +  VDDL-supply:
> +    description:
> +      Digital core voltage supply, 1.2 volts
> +
> +  xclr-gpios:
> +    description: |-
> +      Reference to the GPIO connected to the xclr pin, if any.
> +      Must be released (set high) after all supplies are applied.

A common name for this in camera sensors is xshutdown. I'd suggest to use
that.

> +
> +  camera-clk:
> +    type: object
> +
> +    description: Clock source for imx219
> +
> +    properties:
> +      clock-frequency: true
> +
> +    required:
> +      - clock-frequency

Hmm. The driver doesn't seem to use this for anything.

There are two approaches to this; either you can get and check the
frequency, or specify it in DT bindings, set and then check it.

See e.g. Documentation/devicetree/bindings/media/i2c/nokia,smia.txt (not in
YAML though).

> +
> +  # See ../video-interfaces.txt for more details
> +  port:
> +    type: object
> +    properties:
> +      endpoint:
> +        type: object
> +        properties:
> +          clock-lanes:
> +            const: 0

If the hardware does not support lane reordering, you can omit the
clock-lanes property as it provides no information.

> +
> +          data-lanes:
> +            description: |-
> +              Should be <1 2> for two-lane operation, or <1 2 3 4> for
> +              four-lane operation.
> +            oneOf:
> +              - const: [[ 1, 2 ]]
> +              - const: [[ 1, 2, 3, 4 ]]
> +
> +          clock-noncontinuous:
> +            type: boolean
> +            description: |-
> +              Presence of this boolean property decides whether the MIPI CSI-2
> +              clock is continuous or non-continuous.

How about: MIPI CSI-2 clock will be non-continuous if this property is
present, otherwise it's continuous.

> +
> +        required:
> +          - clock-lanes
> +          - data-lanes
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - VANA-supply
> +  - VDIG-supply
> +  - VDDL-supply
> +  - port
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c0 {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        imx219: sensor@10 {
> +            compatible = "sony,imx219";
> +            reg = <0x10>;
> +            clocks = <&imx219_clk>;
> +            clock-names = "xclk";
> +            VANA-supply = <&imx219_vana>;   /* 2.8v */
> +            VDIG-supply = <&imx219_vdig>;   /* 1.8v */
> +            VDDL-supply = <&imx219_vddl>;   /* 1.2v */
> +
> +            imx219_clk: camera-clk {
> +                compatible = "fixed-clock";
> +                #clock-cells = <0>;
> +                clock-frequency = <24000000>;
> +            };
> +
> +            port {
> +                imx219_0: endpoint {
> +                    remote-endpoint = <&csi1_ep>;
> +                    clock-lanes = <0>;
> +                    data-lanes = <1 2>;
> +                    clock-noncontinuous;
> +                };
> +            };
> +        };
> +    };
> +
> +...
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ffa3371bc750..f7b6c24ec081 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -15350,6 +15350,14 @@ S:	Maintained
>  F:	drivers/media/i2c/imx214.c
>  F:	Documentation/devicetree/bindings/media/i2c/sony,imx214.txt
>  
> +SONY IMX219 SENSOR DRIVER
> +M:	Dave Stevenson <dave.stevenson@raspberrypi.com>

Is Dave aware of this? :-)

> +L:	linux-media@vger.kernel.org
> +T:	git git://linuxtv.org/media_tree.git
> +S:	Maintained
> +F:	drivers/media/i2c/imx219.c
> +F:	Documentation/devicetree/bindings/media/i2c/imx219.yaml
> +
>  SONY IMX258 SENSOR DRIVER
>  M:	Sakari Ailus <sakari.ailus@linux.intel.com>
>  L:	linux-media@vger.kernel.org

-- 
Regards,

Sakari Ailus

Re: [PATCH v2 1/2] dt-bindings: media: i2c: Add IMX219 CMOS sensor binding

[Andrey Konovalov]

Hi Sakari,

Thank you for reviewing the patchset, and for the pointers on improving the driver (nokia,smia.txt etc)!
I'll write a separate email later, or just fix what you suggested in v3 (I agree with the proposed changes
I didn't comment on in this email).

Just few quick answers below.

Thanks,
Andrey

On 27.12.2019 17:17, Sakari Ailus wrote:
> Hi Andrey,
> 
> Thanks for the patchset.
> 
> On Fri, Dec 27, 2019 at 03:21:13PM +0300, Andrey Konovalov wrote:
>> Add YAML device tree binding for IMX219 CMOS image sensor, and
>> the relevant MAINTAINERS entries.
>>
>> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
>> ---
>>   .../devicetree/bindings/media/i2c/imx219.yaml | 134 ++++++++++++++++++
>>   MAINTAINERS                                   |   8 ++
>>   2 files changed, 142 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/media/i2c/imx219.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/media/i2c/imx219.yaml b/Documentation/devicetree/bindings/media/i2c/imx219.yaml
>> new file mode 100644
>> index 000000000000..b58aa49a7c03
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/i2c/imx219.yaml
>> @@ -0,0 +1,134 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/media/i2c/imx219.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Sony 1/4.0-Inch 8Mpixel CMOS Digital Image Sensor
>> +
>> +maintainers:
>> +  - Dave Stevenson <dave.stevenson@raspberrypi.com>
>> +
>> +description: |-
>> +  The Sony imx219 is a 1/4.0-inch CMOS active pixel digital image sensor
>> +  with an active array size of 3280H x 2464V. It is programmable through
>> +  I2C interface. The I2C address is fixed to 0x10 as per sensor data sheet.
>> +  Image data is sent through MIPI CSI-2, which is configured as either 2 or
>> +  4 data lanes.
>> +
>> +properties:
>> +  compatible:
>> +    const: sony,imx219
>> +
>> +  reg:
>> +    description: I2C device address
>> +    maxItems: 1
>> +
>> +  clocks:
>> +    maxItems: 1
>> +
>> +  clock-names:
>> +    items:
>> +      - const: xclk
> 
> There's a single clock. Does it need a name? I'd just omit it.
> 
>> +
>> +  VDIG-supply:
>> +    description:
>> +      Digital I/O voltage supply, 1.8 volts
>> +
>> +  VANA-supply:
>> +    description:
>> +      Analog voltage supply, 2.8 volts
>> +
>> +  VDDL-supply:
>> +    description:
>> +      Digital core voltage supply, 1.2 volts
>> +
>> +  xclr-gpios:
>> +    description: |-
>> +      Reference to the GPIO connected to the xclr pin, if any.
>> +      Must be released (set high) after all supplies are applied.
> 
> A common name for this in camera sensors is xshutdown. I'd suggest to use
> that.

Indeed, "xshutdown" is the pin name commonly used by OmniVision for their sensors.
(In older sensors they used "pwdn" which is similar, but the polarity is reversed.)

In their sensor datasheets Sony consistently use "xclr" for the pin and signal otherwise
very similar to OmniVision's "xshutdown".

Wouldn't using the signal name from the sensor by the different vendor just add more confusion
instead?

>> +
>> +  camera-clk:
>> +    type: object
>> +
>> +    description: Clock source for imx219
>> +
>> +    properties:
>> +      clock-frequency: true
>> +
>> +    required:
>> +      - clock-frequency
> 
> Hmm. The driver doesn't seem to use this for anything.
> 
> There are two approaches to this; either you can get and check the
> frequency, or specify it in DT bindings, set and then check it.
> 
> See e.g. Documentation/devicetree/bindings/media/i2c/nokia,smia.txt (not in
> YAML though).
> 
>> +
>> +  # See ../video-interfaces.txt for more details
>> +  port:
>> +    type: object
>> +    properties:
>> +      endpoint:
>> +        type: object
>> +        properties:
>> +          clock-lanes:
>> +            const: 0
> 
> If the hardware does not support lane reordering, you can omit the
> clock-lanes property as it provides no information.
> 
>> +
>> +          data-lanes:
>> +            description: |-
>> +              Should be <1 2> for two-lane operation, or <1 2 3 4> for
>> +              four-lane operation.
>> +            oneOf:
>> +              - const: [[ 1, 2 ]]
>> +              - const: [[ 1, 2, 3, 4 ]]
>> +
>> +          clock-noncontinuous:
>> +            type: boolean
>> +            description: |-
>> +              Presence of this boolean property decides whether the MIPI CSI-2
>> +              clock is continuous or non-continuous.
> 
> How about: MIPI CSI-2 clock will be non-continuous if this property is
> present, otherwise it's continuous.

This statement is more clear than the original. Thanks!

>> +
>> +        required:
>> +          - clock-lanes
>> +          - data-lanes
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - clocks
>> +  - clock-names
>> +  - VANA-supply
>> +  - VDIG-supply
>> +  - VDDL-supply
>> +  - port
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    i2c0 {
>> +        #address-cells = <1>;
>> +        #size-cells = <0>;
>> +
>> +        imx219: sensor@10 {
>> +            compatible = "sony,imx219";
>> +            reg = <0x10>;
>> +            clocks = <&imx219_clk>;
>> +            clock-names = "xclk";
>> +            VANA-supply = <&imx219_vana>;   /* 2.8v */
>> +            VDIG-supply = <&imx219_vdig>;   /* 1.8v */
>> +            VDDL-supply = <&imx219_vddl>;   /* 1.2v */
>> +
>> +            imx219_clk: camera-clk {
>> +                compatible = "fixed-clock";
>> +                #clock-cells = <0>;
>> +                clock-frequency = <24000000>;
>> +            };
>> +
>> +            port {
>> +                imx219_0: endpoint {
>> +                    remote-endpoint = <&csi1_ep>;
>> +                    clock-lanes = <0>;
>> +                    data-lanes = <1 2>;
>> +                    clock-noncontinuous;
>> +                };
>> +            };
>> +        };
>> +    };
>> +
>> +...
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index ffa3371bc750..f7b6c24ec081 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -15350,6 +15350,14 @@ S:	Maintained
>>   F:	drivers/media/i2c/imx214.c
>>   F:	Documentation/devicetree/bindings/media/i2c/sony,imx214.txt
>>   
>> +SONY IMX219 SENSOR DRIVER
>> +M:	Dave Stevenson <dave.stevenson@raspberrypi.com>
> 
> Is Dave aware of this? :-)

Yes, he is :)

But I forgot to add him to Cc this time. My bad..

Thanks,
Andrey

>> +L:	linux-media@vger.kernel.org
>> +T:	git git://linuxtv.org/media_tree.git
>> +S:	Maintained
>> +F:	drivers/media/i2c/imx219.c
>> +F:	Documentation/devicetree/bindings/media/i2c/imx219.yaml
>> +
>>   SONY IMX258 SENSOR DRIVER
>>   M:	Sakari Ailus <sakari.ailus@linux.intel.com>
>>   L:	linux-media@vger.kernel.org
> 

Re: [PATCH v2 1/2] dt-bindings: media: i2c: Add IMX219 CMOS sensor binding

[Sakari Ailus]

Hi Andrey,

On Fri, Dec 27, 2019 at 09:26:25PM +0300, Andrey Konovalov wrote:
> Hi Sakari,
> 
> Thank you for reviewing the patchset, and for the pointers on improving the driver (nokia,smia.txt etc)!
> I'll write a separate email later, or just fix what you suggested in v3 (I agree with the proposed changes
> I didn't comment on in this email).

Ack.

Please wrap the lines around 72 chars or so.

> 
> Just few quick answers below.
> 
> Thanks,
> Andrey
> 
> On 27.12.2019 17:17, Sakari Ailus wrote:
> > Hi Andrey,
> > 
> > Thanks for the patchset.
> > 
> > On Fri, Dec 27, 2019 at 03:21:13PM +0300, Andrey Konovalov wrote:
> > > Add YAML device tree binding for IMX219 CMOS image sensor, and
> > > the relevant MAINTAINERS entries.
> > > 
> > > Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
> > > ---
> > >   .../devicetree/bindings/media/i2c/imx219.yaml | 134 ++++++++++++++++++
> > >   MAINTAINERS                                   |   8 ++
> > >   2 files changed, 142 insertions(+)
> > >   create mode 100644 Documentation/devicetree/bindings/media/i2c/imx219.yaml
> > > 
> > > diff --git a/Documentation/devicetree/bindings/media/i2c/imx219.yaml b/Documentation/devicetree/bindings/media/i2c/imx219.yaml
> > > new file mode 100644
> > > index 000000000000..b58aa49a7c03
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/media/i2c/imx219.yaml
> > > @@ -0,0 +1,134 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/media/i2c/imx219.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Sony 1/4.0-Inch 8Mpixel CMOS Digital Image Sensor
> > > +
> > > +maintainers:
> > > +  - Dave Stevenson <dave.stevenson@raspberrypi.com>
> > > +
> > > +description: |-
> > > +  The Sony imx219 is a 1/4.0-inch CMOS active pixel digital image sensor
> > > +  with an active array size of 3280H x 2464V. It is programmable through
> > > +  I2C interface. The I2C address is fixed to 0x10 as per sensor data sheet.
> > > +  Image data is sent through MIPI CSI-2, which is configured as either 2 or
> > > +  4 data lanes.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    const: sony,imx219
> > > +
> > > +  reg:
> > > +    description: I2C device address
> > > +    maxItems: 1
> > > +
> > > +  clocks:
> > > +    maxItems: 1
> > > +
> > > +  clock-names:
> > > +    items:
> > > +      - const: xclk
> > 
> > There's a single clock. Does it need a name? I'd just omit it.
> > 
> > > +
> > > +  VDIG-supply:
> > > +    description:
> > > +      Digital I/O voltage supply, 1.8 volts
> > > +
> > > +  VANA-supply:
> > > +    description:
> > > +      Analog voltage supply, 2.8 volts
> > > +
> > > +  VDDL-supply:
> > > +    description:
> > > +      Digital core voltage supply, 1.2 volts
> > > +
> > > +  xclr-gpios:
> > > +    description: |-
> > > +      Reference to the GPIO connected to the xclr pin, if any.
> > > +      Must be released (set high) after all supplies are applied.
> > 
> > A common name for this in camera sensors is xshutdown. I'd suggest to use
> > that.
> 
> Indeed, "xshutdown" is the pin name commonly used by OmniVision for their sensors.

Some Aptina (OnSemi) devices also use xshutdown, and so does the CCS
standard.

> (In older sensors they used "pwdn" which is similar, but the polarity is reversed.)

Some devices have both xshutdown and pwdn. I don't see why; they're
controlled together in practice anyway.

> 
> In their sensor datasheets Sony consistently use "xclr" for the pin and signal otherwise
> very similar to OmniVision's "xshutdown".
> 
> Wouldn't using the signal name from the sensor by the different vendor just add more confusion
> instead?

What matters is the functionality really. I checked the existing bindings,
and it seems devices using "xshutdown" document reset-gpios property under
Documentation/devicetree/bindings/media/i2c . None refers to xclr.

So "reset-gpios" is the right name. The vast majority are active low (i.e.
lifting the sensor from reset requires high output) but there's at least
one that is active high. 

> 
> > > +
> > > +  camera-clk:
> > > +    type: object
> > > +
> > > +    description: Clock source for imx219
> > > +
> > > +    properties:
> > > +      clock-frequency: true
> > > +
> > > +    required:
> > > +      - clock-frequency
> > 
> > Hmm. The driver doesn't seem to use this for anything.
> > 
> > There are two approaches to this; either you can get and check the
> > frequency, or specify it in DT bindings, set and then check it.
> > 
> > See e.g. Documentation/devicetree/bindings/media/i2c/nokia,smia.txt (not in
> > YAML though).
> > 
> > > +
> > > +  # See ../video-interfaces.txt for more details
> > > +  port:
> > > +    type: object
> > > +    properties:
> > > +      endpoint:
> > > +        type: object
> > > +        properties:
> > > +          clock-lanes:
> > > +            const: 0
> > 
> > If the hardware does not support lane reordering, you can omit the
> > clock-lanes property as it provides no information.
> > 
> > > +
> > > +          data-lanes:
> > > +            description: |-
> > > +              Should be <1 2> for two-lane operation, or <1 2 3 4> for
> > > +              four-lane operation.
> > > +            oneOf:
> > > +              - const: [[ 1, 2 ]]
> > > +              - const: [[ 1, 2, 3, 4 ]]
> > > +
> > > +          clock-noncontinuous:
> > > +            type: boolean
> > > +            description: |-
> > > +              Presence of this boolean property decides whether the MIPI CSI-2
> > > +              clock is continuous or non-continuous.
> > 
> > How about: MIPI CSI-2 clock will be non-continuous if this property is
> > present, otherwise it's continuous.
> 
> This statement is more clear than the original. Thanks!

Perhaps:

s/will be/is/

In order to keep the same tense.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v2 1/2] dt-bindings: media: i2c: Add IMX219 CMOS sensor binding

[Dave Stevenson]

Hi Sakari

On Fri, 27 Dec 2019 at 14:18, Sakari Ailus <sakari.ailus@iki.fi> wrote:
>
> Hi Andrey,
>
> Thanks for the patchset.
>
> On Fri, Dec 27, 2019 at 03:21:13PM +0300, Andrey Konovalov wrote:
> > Add YAML device tree binding for IMX219 CMOS image sensor, and
> > the relevant MAINTAINERS entries.
> >
> > Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
> > ---
> >  .../devicetree/bindings/media/i2c/imx219.yaml | 134 ++++++++++++++++++
> >  MAINTAINERS                                   |   8 ++
> >  2 files changed, 142 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/media/i2c/imx219.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/media/i2c/imx219.yaml b/Documentation/devicetree/bindings/media/i2c/imx219.yaml
> > new file mode 100644
> > index 000000000000..b58aa49a7c03
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/i2c/imx219.yaml
> > @@ -0,0 +1,134 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/media/i2c/imx219.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Sony 1/4.0-Inch 8Mpixel CMOS Digital Image Sensor
> > +
> > +maintainers:
> > +  - Dave Stevenson <dave.stevenson@raspberrypi.com>
> > +
> > +description: |-
> > +  The Sony imx219 is a 1/4.0-inch CMOS active pixel digital image sensor
> > +  with an active array size of 3280H x 2464V. It is programmable through
> > +  I2C interface. The I2C address is fixed to 0x10 as per sensor data sheet.
> > +  Image data is sent through MIPI CSI-2, which is configured as either 2 or
> > +  4 data lanes.
> > +
> > +properties:
> > +  compatible:
> > +    const: sony,imx219
> > +
> > +  reg:
> > +    description: I2C device address
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    maxItems: 1
> > +
> > +  clock-names:
> > +    items:
> > +      - const: xclk
>
> There's a single clock. Does it need a name? I'd just omit it.
>
> > +
> > +  VDIG-supply:
> > +    description:
> > +      Digital I/O voltage supply, 1.8 volts
> > +
> > +  VANA-supply:
> > +    description:
> > +      Analog voltage supply, 2.8 volts
> > +
> > +  VDDL-supply:
> > +    description:
> > +      Digital core voltage supply, 1.2 volts
> > +
> > +  xclr-gpios:
> > +    description: |-
> > +      Reference to the GPIO connected to the xclr pin, if any.
> > +      Must be released (set high) after all supplies are applied.
>
> A common name for this in camera sensors is xshutdown. I'd suggest to use
> that.
>
> > +
> > +  camera-clk:
> > +    type: object
> > +
> > +    description: Clock source for imx219
> > +
> > +    properties:
> > +      clock-frequency: true
> > +
> > +    required:
> > +      - clock-frequency
>
> Hmm. The driver doesn't seem to use this for anything.
>
> There are two approaches to this; either you can get and check the
> frequency, or specify it in DT bindings, set and then check it.
>
> See e.g. Documentation/devicetree/bindings/media/i2c/nokia,smia.txt (not in
> YAML though).
>
> > +
> > +  # See ../video-interfaces.txt for more details
> > +  port:
> > +    type: object
> > +    properties:
> > +      endpoint:
> > +        type: object
> > +        properties:
> > +          clock-lanes:
> > +            const: 0
>
> If the hardware does not support lane reordering, you can omit the
> clock-lanes property as it provides no information.
>
> > +
> > +          data-lanes:
> > +            description: |-
> > +              Should be <1 2> for two-lane operation, or <1 2 3 4> for
> > +              four-lane operation.
> > +            oneOf:
> > +              - const: [[ 1, 2 ]]
> > +              - const: [[ 1, 2, 3, 4 ]]
> > +
> > +          clock-noncontinuous:
> > +            type: boolean
> > +            description: |-
> > +              Presence of this boolean property decides whether the MIPI CSI-2
> > +              clock is continuous or non-continuous.
>
> How about: MIPI CSI-2 clock will be non-continuous if this property is
> present, otherwise it's continuous.
>
> > +
> > +        required:
> > +          - clock-lanes
> > +          - data-lanes
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - clocks
> > +  - clock-names
> > +  - VANA-supply
> > +  - VDIG-supply
> > +  - VDDL-supply
> > +  - port
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    i2c0 {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        imx219: sensor@10 {
> > +            compatible = "sony,imx219";
> > +            reg = <0x10>;
> > +            clocks = <&imx219_clk>;
> > +            clock-names = "xclk";
> > +            VANA-supply = <&imx219_vana>;   /* 2.8v */
> > +            VDIG-supply = <&imx219_vdig>;   /* 1.8v */
> > +            VDDL-supply = <&imx219_vddl>;   /* 1.2v */
> > +
> > +            imx219_clk: camera-clk {
> > +                compatible = "fixed-clock";
> > +                #clock-cells = <0>;
> > +                clock-frequency = <24000000>;
> > +            };
> > +
> > +            port {
> > +                imx219_0: endpoint {
> > +                    remote-endpoint = <&csi1_ep>;
> > +                    clock-lanes = <0>;
> > +                    data-lanes = <1 2>;
> > +                    clock-noncontinuous;
> > +                };
> > +            };
> > +        };
> > +    };
> > +
> > +...
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index ffa3371bc750..f7b6c24ec081 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -15350,6 +15350,14 @@ S:   Maintained
> >  F:   drivers/media/i2c/imx214.c
> >  F:   Documentation/devicetree/bindings/media/i2c/sony,imx214.txt
> >
> > +SONY IMX219 SENSOR DRIVER
> > +M:   Dave Stevenson <dave.stevenson@raspberrypi.com>
>
> Is Dave aware of this? :-)

Yes, I'm aware, and happy to be listed as maintainer.
I'm very grateful to Andrey for his efforts in upstreaming this - it's
a task that I simply haven't the time for at present.

  Dave

> > +L:   linux-media@vger.kernel.org
> > +T:   git git://linuxtv.org/media_tree.git
> > +S:   Maintained
> > +F:   drivers/media/i2c/imx219.c
> > +F:   Documentation/devicetree/bindings/media/i2c/imx219.yaml
> > +
> >  SONY IMX258 SENSOR DRIVER
> >  M:   Sakari Ailus <sakari.ailus@linux.intel.com>
> >  L:   linux-media@vger.kernel.org
>
> --
> Regards,
>
> Sakari Ailus

Re: [PATCH v2 1/2] dt-bindings: media: i2c: Add IMX219 CMOS sensor binding

[Rob Herring]

On Fri, Dec 27, 2019 at 03:21:13PM +0300, Andrey Konovalov wrote:
> Add YAML device tree binding for IMX219 CMOS image sensor, and
> the relevant MAINTAINERS entries.
> 
> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
> ---
>  .../devicetree/bindings/media/i2c/imx219.yaml | 134 ++++++++++++++++++
>  MAINTAINERS                                   |   8 ++
>  2 files changed, 142 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/imx219.yaml
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/imx219.yaml b/Documentation/devicetree/bindings/media/i2c/imx219.yaml
> new file mode 100644
> index 000000000000..b58aa49a7c03
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/imx219.yaml
> @@ -0,0 +1,134 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/i2c/imx219.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Sony 1/4.0-Inch 8Mpixel CMOS Digital Image Sensor
> +
> +maintainers:
> +  - Dave Stevenson <dave.stevenson@raspberrypi.com>
> +
> +description: |-
> +  The Sony imx219 is a 1/4.0-inch CMOS active pixel digital image sensor
> +  with an active array size of 3280H x 2464V. It is programmable through
> +  I2C interface. The I2C address is fixed to 0x10 as per sensor data sheet.
> +  Image data is sent through MIPI CSI-2, which is configured as either 2 or
> +  4 data lanes.
> +
> +properties:
> +  compatible:
> +    const: sony,imx219
> +
> +  reg:
> +    description: I2C device address
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    items:
> +      - const: xclk
> +
> +  VDIG-supply:
> +    description:
> +      Digital I/O voltage supply, 1.8 volts
> +
> +  VANA-supply:
> +    description:
> +      Analog voltage supply, 2.8 volts
> +
> +  VDDL-supply:
> +    description:
> +      Digital core voltage supply, 1.2 volts
> +
> +  xclr-gpios:
> +    description: |-
> +      Reference to the GPIO connected to the xclr pin, if any.
> +      Must be released (set high) after all supplies are applied.
> +
> +  camera-clk:
> +    type: object
> +
> +    description: Clock source for imx219

This clock is external to the sensor, so a node for a fixed clock should 
be too.

> +
> +    properties:
> +      clock-frequency: true
> +
> +    required:
> +      - clock-frequency
> +
> +  # See ../video-interfaces.txt for more details
> +  port:
> +    type: object
> +    properties:
> +      endpoint:
> +        type: object
> +        properties:
> +          clock-lanes:
> +            const: 0
> +
> +          data-lanes:
> +            description: |-
> +              Should be <1 2> for two-lane operation, or <1 2 3 4> for
> +              four-lane operation.
> +            oneOf:
> +              - const: [[ 1, 2 ]]
> +              - const: [[ 1, 2, 3, 4 ]]

Not sure if this actually works. If it does, it exposes how we encode 
the DT yaml format which we don't want to do here.

It should be:

oneOf:
  - items:
      - const: 1
      - const: 2
  - items:
      - const: 1
      - const: 2
      - const: 3
      - const: 4

Really, I think you shouldn't need the 2nd case as that should be the 
default.

> +
> +          clock-noncontinuous:
> +            type: boolean
> +            description: |-
> +              Presence of this boolean property decides whether the MIPI CSI-2
> +              clock is continuous or non-continuous.
> +
> +        required:
> +          - clock-lanes
> +          - data-lanes
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - VANA-supply
> +  - VDIG-supply
> +  - VDDL-supply
> +  - port
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c0 {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        imx219: sensor@10 {
> +            compatible = "sony,imx219";
> +            reg = <0x10>;
> +            clocks = <&imx219_clk>;
> +            clock-names = "xclk";
> +            VANA-supply = <&imx219_vana>;   /* 2.8v */
> +            VDIG-supply = <&imx219_vdig>;   /* 1.8v */
> +            VDDL-supply = <&imx219_vddl>;   /* 1.2v */
> +
> +            imx219_clk: camera-clk {
> +                compatible = "fixed-clock";
> +                #clock-cells = <0>;
> +                clock-frequency = <24000000>;
> +            };
> +
> +            port {
> +                imx219_0: endpoint {
> +                    remote-endpoint = <&csi1_ep>;
> +                    clock-lanes = <0>;
> +                    data-lanes = <1 2>;
> +                    clock-noncontinuous;
> +                };
> +            };
> +        };
> +    };
> +
> +...
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ffa3371bc750..f7b6c24ec081 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -15350,6 +15350,14 @@ S:	Maintained
>  F:	drivers/media/i2c/imx214.c
>  F:	Documentation/devicetree/bindings/media/i2c/sony,imx214.txt
>  
> +SONY IMX219 SENSOR DRIVER
> +M:	Dave Stevenson <dave.stevenson@raspberrypi.com>
> +L:	linux-media@vger.kernel.org
> +T:	git git://linuxtv.org/media_tree.git
> +S:	Maintained
> +F:	drivers/media/i2c/imx219.c
> +F:	Documentation/devicetree/bindings/media/i2c/imx219.yaml
> +
>  SONY IMX258 SENSOR DRIVER
>  M:	Sakari Ailus <sakari.ailus@linux.intel.com>
>  L:	linux-media@vger.kernel.org
> -- 
> 2.17.1
> 

Re: [PATCH v2 1/2] dt-bindings: media: i2c: Add IMX219 CMOS sensor binding

[Andrey Konovalov]

Hi Rob,

Thanks for the review!

On 05.01.2020 00:53, Rob Herring wrote:
> On Fri, Dec 27, 2019 at 03:21:13PM +0300, Andrey Konovalov wrote:
>> Add YAML device tree binding for IMX219 CMOS image sensor, and
>> the relevant MAINTAINERS entries.
>>
>> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
>> ---
>>   .../devicetree/bindings/media/i2c/imx219.yaml | 134 ++++++++++++++++++
>>   MAINTAINERS                                   |   8 ++
>>   2 files changed, 142 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/media/i2c/imx219.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/media/i2c/imx219.yaml b/Documentation/devicetree/bindings/media/i2c/imx219.yaml
>> new file mode 100644
>> index 000000000000..b58aa49a7c03
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/i2c/imx219.yaml
>> @@ -0,0 +1,134 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/media/i2c/imx219.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Sony 1/4.0-Inch 8Mpixel CMOS Digital Image Sensor
>> +
>> +maintainers:
>> +  - Dave Stevenson <dave.stevenson@raspberrypi.com>
>> +
>> +description: |-
>> +  The Sony imx219 is a 1/4.0-inch CMOS active pixel digital image sensor
>> +  with an active array size of 3280H x 2464V. It is programmable through
>> +  I2C interface. The I2C address is fixed to 0x10 as per sensor data sheet.
>> +  Image data is sent through MIPI CSI-2, which is configured as either 2 or
>> +  4 data lanes.
>> +
>> +properties:
>> +  compatible:
>> +    const: sony,imx219
>> +
>> +  reg:
>> +    description: I2C device address
>> +    maxItems: 1
>> +
>> +  clocks:
>> +    maxItems: 1
>> +
>> +  clock-names:
>> +    items:
>> +      - const: xclk
>> +
>> +  VDIG-supply:
>> +    description:
>> +      Digital I/O voltage supply, 1.8 volts
>> +
>> +  VANA-supply:
>> +    description:
>> +      Analog voltage supply, 2.8 volts
>> +
>> +  VDDL-supply:
>> +    description:
>> +      Digital core voltage supply, 1.2 volts
>> +
>> +  xclr-gpios:
>> +    description: |-
>> +      Reference to the GPIO connected to the xclr pin, if any.
>> +      Must be released (set high) after all supplies are applied.
>> +
>> +  camera-clk:
>> +    type: object
>> +
>> +    description: Clock source for imx219
> 
> This clock is external to the sensor, so a node for a fixed clock should
> be too.

Done in v3.

>> +
>> +    properties:
>> +      clock-frequency: true
>> +
>> +    required:
>> +      - clock-frequency
>> +
>> +  # See ../video-interfaces.txt for more details
>> +  port:
>> +    type: object
>> +    properties:
>> +      endpoint:
>> +        type: object
>> +        properties:
>> +          clock-lanes:
>> +            const: 0
>> +
>> +          data-lanes:
>> +            description: |-
>> +              Should be <1 2> for two-lane operation, or <1 2 3 4> for
>> +              four-lane operation.
>> +            oneOf:
>> +              - const: [[ 1, 2 ]]
>> +              - const: [[ 1, 2, 3, 4 ]]
> 
> Not sure if this actually works. If it does, it exposes how we encode
> the DT yaml format which we don't want to do here.

It does work - I am still not quite comfortable with json schema, and got
to the above by looking at the files produced by 'make dt_binding_check'.


> It should be:
> 
> oneOf:
>    - items:
>        - const: 1
>        - const: 2
>    - items:
>        - const: 1
>        - const: 2
>        - const: 3
>        - const: 4

Thanks, got it.

> Really, I think you shouldn't need the 2nd case as that should be the
> default.

I've tried to implement that in v3 of the patch set.

Thanks,
Andrey

>> +
>> +          clock-noncontinuous:
>> +            type: boolean
>> +            description: |-
>> +              Presence of this boolean property decides whether the MIPI CSI-2
>> +              clock is continuous or non-continuous.
>> +
>> +        required:
>> +          - clock-lanes
>> +          - data-lanes
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - clocks
>> +  - clock-names
>> +  - VANA-supply
>> +  - VDIG-supply
>> +  - VDDL-supply
>> +  - port
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    i2c0 {
>> +        #address-cells = <1>;
>> +        #size-cells = <0>;
>> +
>> +        imx219: sensor@10 {
>> +            compatible = "sony,imx219";
>> +            reg = <0x10>;
>> +            clocks = <&imx219_clk>;
>> +            clock-names = "xclk";
>> +            VANA-supply = <&imx219_vana>;   /* 2.8v */
>> +            VDIG-supply = <&imx219_vdig>;   /* 1.8v */
>> +            VDDL-supply = <&imx219_vddl>;   /* 1.2v */
>> +
>> +            imx219_clk: camera-clk {
>> +                compatible = "fixed-clock";
>> +                #clock-cells = <0>;
>> +                clock-frequency = <24000000>;
>> +            };
>> +
>> +            port {
>> +                imx219_0: endpoint {
>> +                    remote-endpoint = <&csi1_ep>;
>> +                    clock-lanes = <0>;
>> +                    data-lanes = <1 2>;
>> +                    clock-noncontinuous;
>> +                };
>> +            };
>> +        };
>> +    };
>> +
>> +...
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index ffa3371bc750..f7b6c24ec081 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -15350,6 +15350,14 @@ S:	Maintained
>>   F:	drivers/media/i2c/imx214.c
>>   F:	Documentation/devicetree/bindings/media/i2c/sony,imx214.txt
>>   
>> +SONY IMX219 SENSOR DRIVER
>> +M:	Dave Stevenson <dave.stevenson@raspberrypi.com>
>> +L:	linux-media@vger.kernel.org
>> +T:	git git://linuxtv.org/media_tree.git
>> +S:	Maintained
>> +F:	drivers/media/i2c/imx219.c
>> +F:	Documentation/devicetree/bindings/media/i2c/imx219.yaml
>> +
>>   SONY IMX258 SENSOR DRIVER
>>   M:	Sakari Ailus <sakari.ailus@linux.intel.com>
>>   L:	linux-media@vger.kernel.org
>> -- 
>> 2.17.1
>>

[PATCH v2 2/2] media: i2c: Add driver for Sony IMX219 sensor

[Andrey Konovalov]

From: Dave Stevenson <dave.stevenson@raspberrypi.com>

Adds a driver for the 8MPix Sony IMX219 CSI2 sensor.
Whilst the sensor supports 2 or 4 CSI2 data lanes, this driver
currently only supports 2 lanes.
8MPix @ 15fps, 1080P @ 30fps (cropped FOV), and 1640x1232 (2x2 binned)
@ 30fps are currently supported.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
---
 drivers/media/i2c/Kconfig  |   12 +
 drivers/media/i2c/Makefile |    1 +
 drivers/media/i2c/imx219.c | 1240 ++++++++++++++++++++++++++++++++++++
 3 files changed, 1253 insertions(+)
 create mode 100644 drivers/media/i2c/imx219.c

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index c68e002d26ea..6fa5af1f72b9 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -591,6 +591,18 @@ config VIDEO_IMX214
 	  To compile this driver as a module, choose M here: the
 	  module will be called imx214.
 
+config VIDEO_IMX219
+	tristate "Sony IMX219 sensor support"
+	depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
+	depends on MEDIA_CAMERA_SUPPORT
+	select V4L2_FWNODE
+	help
+	  This is a Video4Linux2 sensor driver for the Sony
+	  IMX219 camera.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called imx219.
+
 config VIDEO_IMX258
 	tristate "Sony IMX258 sensor support"
 	depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index c147bb9d28db..77bf7d0b691f 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -111,6 +111,7 @@ obj-$(CONFIG_VIDEO_OV2659)	+= ov2659.o
 obj-$(CONFIG_VIDEO_TC358743)	+= tc358743.o
 obj-$(CONFIG_VIDEO_HI556)	+= hi556.o
 obj-$(CONFIG_VIDEO_IMX214)	+= imx214.o
+obj-$(CONFIG_VIDEO_IMX219)	+= imx219.o
 obj-$(CONFIG_VIDEO_IMX258)	+= imx258.o
 obj-$(CONFIG_VIDEO_IMX274)	+= imx274.o
 obj-$(CONFIG_VIDEO_IMX290)	+= imx290.o
diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
new file mode 100644
index 000000000000..28b55c61cd77
--- /dev/null
+++ b/drivers/media/i2c/imx219.c
@@ -0,0 +1,1240 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * A V4L2 driver for Sony IMX219 cameras.
+ * Copyright (C) 2019, Raspberry Pi (Trading) Ltd
+ *
+ * Based on Sony imx258 camera driver
+ * Copyright (C) 2018 Intel Corporation
+ *
+ * DT / fwnode changes, and regulator / GPIO control taken from imx214 driver
+ * Copyright 2018 Qtechnology A/S
+ *
+ * Flip handling taken from the Sony IMX319 driver.
+ * Copyright (C) 2018 Intel Corporation
+ *
+ */
+
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/clkdev.h>
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/pm_runtime.h>
+#include <linux/regulator/consumer.h>
+#include <media/v4l2-ctrls.h>
+#include <media/v4l2-device.h>
+#include <media/v4l2-event.h>
+#include <media/v4l2-fwnode.h>
+#include <media/v4l2-mediabus.h>
+#include <asm/unaligned.h>
+
+#define IMX219_REG_VALUE_08BIT		1
+#define IMX219_REG_VALUE_16BIT		2
+
+#define IMX219_REG_MODE_SELECT		0x0100
+#define IMX219_MODE_STANDBY		0x00
+#define IMX219_MODE_STREAMING		0x01
+
+/* Chip ID */
+#define IMX219_REG_CHIP_ID		0x0000
+#define IMX219_CHIP_ID			0x0219
+
+/* External clock frequency is 24.0M */
+#define IMX219_XCLK_FREQ		24000000
+
+/* Pixel rate is fixed at 182.4M for all the modes */
+#define IMX219_PIXEL_RATE		182400000
+
+/* V_TIMING internal */
+#define IMX219_REG_VTS			0x0160
+#define IMX219_VTS_15FPS		0x0dc6
+#define IMX219_VTS_30FPS_1080P		0x06e3
+#define IMX219_VTS_30FPS_BINNED		0x06e3
+#define IMX219_VTS_MAX			0xffff
+
+#define IMX219_VBLANK_MIN		4
+
+/*Frame Length Line*/
+#define IMX219_FLL_MIN			0x08a6
+#define IMX219_FLL_MAX			0xffff
+#define IMX219_FLL_STEP			1
+#define IMX219_FLL_DEFAULT		0x0c98
+
+/* HBLANK control - read only */
+#define IMX219_PPL_DEFAULT		3448
+
+/* Exposure control */
+#define IMX219_REG_EXPOSURE		0x015a
+#define IMX219_EXPOSURE_MIN		4
+#define IMX219_EXPOSURE_STEP		1
+#define IMX219_EXPOSURE_DEFAULT		0x640
+#define IMX219_EXPOSURE_MAX		65535
+
+/* Analog gain control */
+#define IMX219_REG_ANALOG_GAIN		0x0157
+#define IMX219_ANA_GAIN_MIN		0
+#define IMX219_ANA_GAIN_MAX		232
+#define IMX219_ANA_GAIN_STEP		1
+#define IMX219_ANA_GAIN_DEFAULT		0x0
+
+/* Digital gain control */
+#define IMX219_REG_DIGITAL_GAIN		0x0158
+#define IMX219_DGTL_GAIN_MIN		0x0100
+#define IMX219_DGTL_GAIN_MAX		0x0fff
+#define IMX219_DGTL_GAIN_DEFAULT	0x0100
+#define IMX219_DGTL_GAIN_STEP		1
+
+#define IMX219_REG_ORIENTATION		0x0172
+
+/* Test Pattern Control */
+#define IMX219_REG_TEST_PATTERN		0x0600
+#define IMX219_TEST_PATTERN_DISABLE	0
+#define IMX219_TEST_PATTERN_SOLID_COLOR	1
+#define IMX219_TEST_PATTERN_COLOR_BARS	2
+#define IMX219_TEST_PATTERN_GREY_COLOR	3
+#define IMX219_TEST_PATTERN_PN9		4
+
+/* Test pattern colour components */
+#define IMX219_REG_TESTP_RED		0x0602
+#define IMX219_REG_TESTP_GREENR		0x0604
+#define IMX219_REG_TESTP_BLUE		0x0606
+#define IMX219_REG_TESTP_GREENB		0x0608
+#define IMX219_TESTP_COLOUR_MIN		0
+#define IMX219_TESTP_COLOUR_MAX		0x03ff
+#define IMX219_TESTP_COLOUR_STEP	1
+#define IMX219_TESTP_RED_DEFAULT	IMX219_TESTP_COLOUR_MAX
+#define IMX219_TESTP_GREENR_DEFAULT	0
+#define IMX219_TESTP_BLUE_DEFAULT	0
+#define IMX219_TESTP_GREENB_DEFAULT	0
+
+struct imx219_reg {
+	u16 address;
+	u8 val;
+};
+
+struct imx219_reg_list {
+	unsigned int num_of_regs;
+	const struct imx219_reg *regs;
+};
+
+/* Mode : resolution and related config&values */
+struct imx219_mode {
+	/* Frame width */
+	unsigned int width;
+	/* Frame height */
+	unsigned int height;
+
+	/* V-timing */
+	unsigned int vts_def;
+
+	/* Default register values */
+	struct imx219_reg_list reg_list;
+};
+
+/*
+ * Register sets lifted off the i2C interface from the Raspberry Pi firmware
+ * driver.
+ * 3280x2464 = mode 2, 1920x1080 = mode 1, and 1640x1232 = mode 4.
+ */
+static const struct imx219_reg mode_3280x2464_regs[] = {
+	{0x0100, 0x00},
+	{0x30eb, 0x0c},
+	{0x30eb, 0x05},
+	{0x300a, 0xff},
+	{0x300b, 0xff},
+	{0x30eb, 0x05},
+	{0x30eb, 0x09},
+	{0x0114, 0x01},
+	{0x0128, 0x00},
+	{0x012a, 0x18},
+	{0x012b, 0x00},
+	{0x0164, 0x00},
+	{0x0165, 0x00},
+	{0x0166, 0x0c},
+	{0x0167, 0xcf},
+	{0x0168, 0x00},
+	{0x0169, 0x00},
+	{0x016a, 0x09},
+	{0x016b, 0x9f},
+	{0x016c, 0x0c},
+	{0x016d, 0xd0},
+	{0x016e, 0x09},
+	{0x016f, 0xa0},
+	{0x0170, 0x01},
+	{0x0171, 0x01},
+	{0x0174, 0x00},
+	{0x0175, 0x00},
+	{0x018c, 0x0a},
+	{0x018d, 0x0a},
+	{0x0301, 0x05},
+	{0x0303, 0x01},
+	{0x0304, 0x03},
+	{0x0305, 0x03},
+	{0x0306, 0x00},
+	{0x0307, 0x39},
+	{0x0309, 0x0a},
+	{0x030b, 0x01},
+	{0x030c, 0x00},
+	{0x030d, 0x72},
+	{0x0624, 0x0c},
+	{0x0625, 0xd0},
+	{0x0626, 0x09},
+	{0x0627, 0xa0},
+	{0x455e, 0x00},
+	{0x471e, 0x4b},
+	{0x4767, 0x0f},
+	{0x4750, 0x14},
+	{0x4540, 0x00},
+	{0x47b4, 0x14},
+	{0x4713, 0x30},
+	{0x478b, 0x10},
+	{0x478f, 0x10},
+	{0x4793, 0x10},
+	{0x4797, 0x0e},
+	{0x479b, 0x0e},
+	{0x0162, 0x0d},
+	{0x0163, 0x78},
+};
+
+static const struct imx219_reg mode_1920_1080_regs[] = {
+	{0x0100, 0x00},
+	{0x30eb, 0x05},
+	{0x30eb, 0x0c},
+	{0x300a, 0xff},
+	{0x300b, 0xff},
+	{0x30eb, 0x05},
+	{0x30eb, 0x09},
+	{0x0114, 0x01},
+	{0x0128, 0x00},
+	{0x012a, 0x18},
+	{0x012b, 0x00},
+	{0x0162, 0x0d},
+	{0x0163, 0x78},
+	{0x0164, 0x02},
+	{0x0165, 0xa8},
+	{0x0166, 0x0a},
+	{0x0167, 0x27},
+	{0x0168, 0x02},
+	{0x0169, 0xb4},
+	{0x016a, 0x06},
+	{0x016b, 0xeb},
+	{0x016c, 0x07},
+	{0x016d, 0x80},
+	{0x016e, 0x04},
+	{0x016f, 0x38},
+	{0x0170, 0x01},
+	{0x0171, 0x01},
+	{0x0174, 0x00},
+	{0x0175, 0x00},
+	{0x018c, 0x0a},
+	{0x018d, 0x0a},
+	{0x0301, 0x05},
+	{0x0303, 0x01},
+	{0x0304, 0x03},
+	{0x0305, 0x03},
+	{0x0306, 0x00},
+	{0x0307, 0x39},
+	{0x0309, 0x0a},
+	{0x030b, 0x01},
+	{0x030c, 0x00},
+	{0x030d, 0x72},
+	{0x0624, 0x07},
+	{0x0625, 0x80},
+	{0x0626, 0x04},
+	{0x0627, 0x38},
+	{0x455e, 0x00},
+	{0x471e, 0x4b},
+	{0x4767, 0x0f},
+	{0x4750, 0x14},
+	{0x4540, 0x00},
+	{0x47b4, 0x14},
+	{0x4713, 0x30},
+	{0x478b, 0x10},
+	{0x478f, 0x10},
+	{0x4793, 0x10},
+	{0x4797, 0x0e},
+	{0x479b, 0x0e},
+	{0x0162, 0x0d},
+	{0x0163, 0x78},
+};
+
+static const struct imx219_reg mode_1640_1232_regs[] = {
+	{0x0100, 0x00},
+	{0x30eb, 0x0c},
+	{0x30eb, 0x05},
+	{0x300a, 0xff},
+	{0x300b, 0xff},
+	{0x30eb, 0x05},
+	{0x30eb, 0x09},
+	{0x0114, 0x01},
+	{0x0128, 0x00},
+	{0x012a, 0x18},
+	{0x012b, 0x00},
+	{0x0164, 0x00},
+	{0x0165, 0x00},
+	{0x0166, 0x0c},
+	{0x0167, 0xcf},
+	{0x0168, 0x00},
+	{0x0169, 0x00},
+	{0x016a, 0x09},
+	{0x016b, 0x9f},
+	{0x016c, 0x06},
+	{0x016d, 0x68},
+	{0x016e, 0x04},
+	{0x016f, 0xd0},
+	{0x0170, 0x01},
+	{0x0171, 0x01},
+	{0x0174, 0x01},
+	{0x0175, 0x01},
+	{0x018c, 0x0a},
+	{0x018d, 0x0a},
+	{0x0301, 0x05},
+	{0x0303, 0x01},
+	{0x0304, 0x03},
+	{0x0305, 0x03},
+	{0x0306, 0x00},
+	{0x0307, 0x39},
+	{0x0309, 0x0a},
+	{0x030b, 0x01},
+	{0x030c, 0x00},
+	{0x030d, 0x72},
+	{0x0624, 0x06},
+	{0x0625, 0x68},
+	{0x0626, 0x04},
+	{0x0627, 0xd0},
+	{0x455e, 0x00},
+	{0x471e, 0x4b},
+	{0x4767, 0x0f},
+	{0x4750, 0x14},
+	{0x4540, 0x00},
+	{0x47b4, 0x14},
+	{0x4713, 0x30},
+	{0x478b, 0x10},
+	{0x478f, 0x10},
+	{0x4793, 0x10},
+	{0x4797, 0x0e},
+	{0x479b, 0x0e},
+	{0x0162, 0x0d},
+	{0x0163, 0x78},
+};
+
+static const char * const imx219_test_pattern_menu[] = {
+	"Disabled",
+	"Color Bars",
+	"Solid Color",
+	"Grey Color Bars",
+	"PN9"
+};
+
+static const int imx219_test_pattern_val[] = {
+	IMX219_TEST_PATTERN_DISABLE,
+	IMX219_TEST_PATTERN_COLOR_BARS,
+	IMX219_TEST_PATTERN_SOLID_COLOR,
+	IMX219_TEST_PATTERN_GREY_COLOR,
+	IMX219_TEST_PATTERN_PN9,
+};
+
+/* regulator supplies */
+static const char * const imx219_supply_name[] = {
+	/* Supplies can be enabled in any order */
+	"VANA",  /* Analog (2.8V) supply */
+	"VDIG",  /* Digital Core (1.8V) supply */
+	"VDDL",  /* IF (1.2V) supply */
+};
+
+#define IMX219_NUM_SUPPLIES ARRAY_SIZE(imx219_supply_name)
+
+#define IMX219_XCLR_DELAY_MS 10	/* Initialisation delay after XCLR low->high */
+
+/* Mode configs */
+static const struct imx219_mode supported_modes[] = {
+	{
+		/* 8MPix 15fps mode */
+		.width = 3280,
+		.height = 2464,
+		.vts_def = IMX219_VTS_15FPS,
+		.reg_list = {
+			.num_of_regs = ARRAY_SIZE(mode_3280x2464_regs),
+			.regs = mode_3280x2464_regs,
+		},
+	},
+	{
+		/* 1080P 30fps cropped */
+		.width = 1920,
+		.height = 1080,
+		.vts_def = IMX219_VTS_30FPS_1080P,
+		.reg_list = {
+			.num_of_regs = ARRAY_SIZE(mode_1920_1080_regs),
+			.regs = mode_1920_1080_regs,
+		},
+	},
+	{
+		/* 2x2 binned 30fps mode */
+		.width = 1640,
+		.height = 1232,
+		.vts_def = IMX219_VTS_30FPS_BINNED,
+		.reg_list = {
+			.num_of_regs = ARRAY_SIZE(mode_1640_1232_regs),
+			.regs = mode_1640_1232_regs,
+		},
+	},
+};
+
+struct imx219 {
+	struct device *dev;
+
+	struct v4l2_subdev sd;
+	struct media_pad pad;
+
+	struct v4l2_fwnode_endpoint ep; /* the parsed DT endpoint info */
+	struct clk *xclk; /* system clock to IMX219 */
+	u32 xclk_freq;
+
+	struct gpio_desc *xclr_gpio;
+	struct regulator_bulk_data supplies[IMX219_NUM_SUPPLIES];
+
+	struct v4l2_ctrl_handler ctrl_handler;
+	/* V4L2 Controls */
+	struct v4l2_ctrl *pixel_rate;
+	struct v4l2_ctrl *exposure;
+	struct v4l2_ctrl *vflip;
+	struct v4l2_ctrl *hflip;
+	struct v4l2_ctrl *vblank;
+	struct v4l2_ctrl *hblank;
+
+	/* Current mode */
+	const struct imx219_mode *mode;
+
+	/*
+	 * Mutex for serialized access:
+	 * Protect sensor module set pad format and start/stop streaming safely.
+	 */
+	struct mutex mutex;
+
+	/* Streaming on/off */
+	bool streaming;
+};
+
+static inline struct imx219 *to_imx219(struct v4l2_subdev *_sd)
+{
+	return container_of(_sd, struct imx219, sd);
+}
+
+/* Read registers up to 2 at a time */
+static int imx219_read_reg(struct imx219 *imx219, u16 reg, u32 len, u32 *val)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
+	struct i2c_msg msgs[2];
+	u8 addr_buf[2] = { reg >> 8, reg & 0xff };
+	u8 data_buf[4] = { 0, };
+	int ret;
+
+	if (len > 4)
+		return -EINVAL;
+
+	/* Write register address */
+	msgs[0].addr = client->addr;
+	msgs[0].flags = 0;
+	msgs[0].len = ARRAY_SIZE(addr_buf);
+	msgs[0].buf = addr_buf;
+
+	/* Read data from register */
+	msgs[1].addr = client->addr;
+	msgs[1].flags = I2C_M_RD;
+	msgs[1].len = len;
+	msgs[1].buf = &data_buf[4 - len];
+
+	ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
+	if (ret != ARRAY_SIZE(msgs))
+		return -EIO;
+
+	*val = get_unaligned_be32(data_buf);
+
+	return 0;
+}
+
+/* Write registers up to 2 at a time */
+static int imx219_write_reg(struct imx219 *imx219, u16 reg, u32 len, u32 val)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
+	u8 buf[6];
+
+	if (len > 4)
+		return -EINVAL;
+
+	put_unaligned_be16(reg, buf);
+	put_unaligned_be32(val << (8 * (4 - len)), buf + 2);
+	if (i2c_master_send(client, buf, len + 2) != len + 2)
+		return -EIO;
+
+	return 0;
+}
+
+/* Write a list of registers */
+static int imx219_write_regs(struct imx219 *imx219,
+			     const struct imx219_reg *regs, u32 len)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
+	unsigned int i;
+	int ret;
+
+	for (i = 0; i < len; i++) {
+		ret = imx219_write_reg(imx219, regs[i].address, 1, regs[i].val);
+		if (ret) {
+			dev_err_ratelimited(&client->dev,
+					    "Failed to write reg 0x%4.4x. error = %d\n",
+					    regs[i].address, ret);
+
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+/* Get bayer order based on flip setting. */
+static u32 imx219_get_format_code(struct imx219 *imx219)
+{
+	/*
+	 * Only one bayer order is supported.
+	 * It depends on the flip settings.
+	 */
+	static const u32 codes[2][2] = {
+		{ MEDIA_BUS_FMT_SRGGB10_1X10, MEDIA_BUS_FMT_SGRBG10_1X10, },
+		{ MEDIA_BUS_FMT_SGBRG10_1X10, MEDIA_BUS_FMT_SBGGR10_1X10, },
+	};
+
+	lockdep_assert_held(&imx219->mutex);
+	return codes[imx219->vflip->val][imx219->hflip->val];
+}
+
+static int imx219_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
+{
+	struct imx219 *imx219 = to_imx219(sd);
+	struct v4l2_mbus_framefmt *try_fmt =
+		v4l2_subdev_get_try_format(sd, fh->pad, 0);
+
+	mutex_lock(&imx219->mutex);
+
+	/* Initialize try_fmt */
+	try_fmt->width = supported_modes[0].width;
+	try_fmt->height = supported_modes[0].height;
+	try_fmt->code = imx219_get_format_code(imx219);
+	try_fmt->field = V4L2_FIELD_NONE;
+
+	mutex_unlock(&imx219->mutex);
+
+	return 0;
+}
+
+static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
+{
+	struct imx219 *imx219 =
+		container_of(ctrl->handler, struct imx219, ctrl_handler);
+	struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
+	int ret;
+
+	if (ctrl->id == V4L2_CID_VBLANK) {
+		int exposure_max, exposure_def;
+
+		/* Update max exposure while meeting expected vblanking */
+		exposure_max = imx219->mode->height + ctrl->val - 4;
+		exposure_def = (exposure_max < IMX219_EXPOSURE_DEFAULT) ?
+			exposure_max : IMX219_EXPOSURE_DEFAULT;
+		__v4l2_ctrl_modify_range(imx219->exposure,
+					 imx219->exposure->minimum,
+					 exposure_max, imx219->exposure->step,
+					 exposure_def);
+	}
+
+	/*
+	 * Applying V4L2 control value only happens
+	 * when power is up for streaming
+	 */
+	if (pm_runtime_get_if_in_use(&client->dev) == 0)
+		return 0;
+
+	switch (ctrl->id) {
+	case V4L2_CID_ANALOGUE_GAIN:
+		ret = imx219_write_reg(imx219, IMX219_REG_ANALOG_GAIN,
+				       IMX219_REG_VALUE_08BIT, ctrl->val);
+		break;
+	case V4L2_CID_EXPOSURE:
+		ret = imx219_write_reg(imx219, IMX219_REG_EXPOSURE,
+				       IMX219_REG_VALUE_16BIT, ctrl->val);
+		break;
+	case V4L2_CID_DIGITAL_GAIN:
+		ret = imx219_write_reg(imx219, IMX219_REG_DIGITAL_GAIN,
+				       IMX219_REG_VALUE_16BIT, ctrl->val);
+		break;
+	case V4L2_CID_TEST_PATTERN:
+		ret = imx219_write_reg(imx219, IMX219_REG_TEST_PATTERN,
+				       IMX219_REG_VALUE_16BIT,
+				       imx219_test_pattern_val[ctrl->val]);
+		break;
+	case V4L2_CID_HFLIP:
+	case V4L2_CID_VFLIP:
+		ret = imx219_write_reg(imx219, IMX219_REG_ORIENTATION, 1,
+				       imx219->hflip->val |
+				       imx219->vflip->val << 1);
+		break;
+	case V4L2_CID_VBLANK:
+		ret = imx219_write_reg(imx219, IMX219_REG_VTS,
+				       IMX219_REG_VALUE_16BIT,
+				       imx219->mode->height + ctrl->val);
+		break;
+	case V4L2_CID_TEST_PATTERN_RED:
+		ret = imx219_write_reg(imx219, IMX219_REG_TESTP_RED,
+				       IMX219_REG_VALUE_16BIT, ctrl->val);
+		break;
+	case V4L2_CID_TEST_PATTERN_GREENR:
+		ret = imx219_write_reg(imx219, IMX219_REG_TESTP_GREENR,
+				       IMX219_REG_VALUE_16BIT, ctrl->val);
+		break;
+	case V4L2_CID_TEST_PATTERN_BLUE:
+		ret = imx219_write_reg(imx219, IMX219_REG_TESTP_BLUE,
+				       IMX219_REG_VALUE_16BIT, ctrl->val);
+		break;
+	case V4L2_CID_TEST_PATTERN_GREENB:
+		ret = imx219_write_reg(imx219, IMX219_REG_TESTP_GREENB,
+				       IMX219_REG_VALUE_16BIT, ctrl->val);
+		break;
+	default:
+		dev_info(&client->dev,
+			 "ctrl(id:0x%x,val:0x%x) is not handled\n",
+			 ctrl->id, ctrl->val);
+		ret = -EINVAL;
+		break;
+	}
+
+	pm_runtime_put(&client->dev);
+
+	return ret;
+}
+
+static const struct v4l2_ctrl_ops imx219_ctrl_ops = {
+	.s_ctrl = imx219_set_ctrl,
+};
+
+static int imx219_enum_mbus_code(struct v4l2_subdev *sd,
+				 struct v4l2_subdev_pad_config *cfg,
+				 struct v4l2_subdev_mbus_code_enum *code)
+{
+	struct imx219 *imx219 = to_imx219(sd);
+
+	/*
+	 * Only one bayer order is supported (though it depends on the flip
+	 * settings)
+	 */
+	if (code->index > 0)
+		return -EINVAL;
+
+	code->code = imx219_get_format_code(imx219);
+
+	return 0;
+}
+
+static int imx219_enum_frame_size(struct v4l2_subdev *sd,
+				  struct v4l2_subdev_pad_config *cfg,
+				  struct v4l2_subdev_frame_size_enum *fse)
+{
+	struct imx219 *imx219 = to_imx219(sd);
+
+	if (fse->index >= ARRAY_SIZE(supported_modes))
+		return -EINVAL;
+
+	if (fse->code != imx219_get_format_code(imx219))
+		return -EINVAL;
+
+	fse->min_width = supported_modes[fse->index].width;
+	fse->max_width = fse->min_width;
+	fse->min_height = supported_modes[fse->index].height;
+	fse->max_height = fse->min_height;
+
+	return 0;
+}
+
+static void imx219_reset_colorspace(struct v4l2_mbus_framefmt *fmt)
+{
+	fmt->colorspace = V4L2_COLORSPACE_SRGB;
+	fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace);
+	fmt->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
+							  fmt->colorspace,
+							  fmt->ycbcr_enc);
+	fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt->colorspace);
+}
+
+static void imx219_update_pad_format(struct imx219 *imx219,
+				     const struct imx219_mode *mode,
+				     struct v4l2_subdev_format *fmt)
+{
+	fmt->format.width = mode->width;
+	fmt->format.height = mode->height;
+	fmt->format.code = imx219_get_format_code(imx219);
+	fmt->format.field = V4L2_FIELD_NONE;
+
+	imx219_reset_colorspace(&fmt->format);
+}
+
+static int __imx219_get_pad_format(struct imx219 *imx219,
+				   struct v4l2_subdev_pad_config *cfg,
+				   struct v4l2_subdev_format *fmt)
+{
+	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY)
+		fmt->format = *v4l2_subdev_get_try_format(&imx219->sd, cfg,
+							  fmt->pad);
+	else
+		imx219_update_pad_format(imx219, imx219->mode, fmt);
+
+	return 0;
+}
+
+static int imx219_get_pad_format(struct v4l2_subdev *sd,
+				 struct v4l2_subdev_pad_config *cfg,
+				 struct v4l2_subdev_format *fmt)
+{
+	struct imx219 *imx219 = to_imx219(sd);
+	int ret;
+
+	mutex_lock(&imx219->mutex);
+	ret = __imx219_get_pad_format(imx219, cfg, fmt);
+	mutex_unlock(&imx219->mutex);
+
+	return ret;
+}
+
+static int imx219_set_pad_format(struct v4l2_subdev *sd,
+				 struct v4l2_subdev_pad_config *cfg,
+				 struct v4l2_subdev_format *fmt)
+{
+	struct imx219 *imx219 = to_imx219(sd);
+	const struct imx219_mode *mode;
+	struct v4l2_mbus_framefmt *framefmt;
+	int exposure_max, exposure_def, hblank;
+
+	mutex_lock(&imx219->mutex);
+
+	/* Bayer order varies with flips */
+	fmt->format.code = imx219_get_format_code(imx219);
+
+	mode = v4l2_find_nearest_size(supported_modes,
+				      ARRAY_SIZE(supported_modes),
+				      width, height,
+				      fmt->format.width, fmt->format.height);
+	imx219_update_pad_format(imx219, mode, fmt);
+	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
+		framefmt = v4l2_subdev_get_try_format(sd, cfg, fmt->pad);
+		*framefmt = fmt->format;
+	} else if (imx219->mode != mode) {
+		imx219->mode = mode;
+		/* Update limits and set FPS to default */
+		__v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN,
+					 IMX219_VTS_MAX - mode->height, 1,
+					 mode->vts_def - mode->height);
+		__v4l2_ctrl_s_ctrl(imx219->vblank,
+				   mode->vts_def - mode->height);
+		/* Update max exposure while meeting expected vblanking */
+		exposure_max = mode->vts_def - 4;
+		exposure_def = (exposure_max < IMX219_EXPOSURE_DEFAULT) ?
+			exposure_max : IMX219_EXPOSURE_DEFAULT;
+		__v4l2_ctrl_modify_range(imx219->exposure,
+					 imx219->exposure->minimum,
+					 exposure_max, imx219->exposure->step,
+					 exposure_def);
+		/*
+		 * Currently PPL is fixed to IMX219_PPL_DEFAULT, so hblank
+		 * depends on mode->width only, and is not changeble in any
+		 * way other than changing the mode.
+		 */
+		hblank = IMX219_PPL_DEFAULT - mode->width;
+		__v4l2_ctrl_modify_range(imx219->hblank, hblank, hblank, 1,
+					 hblank);
+	}
+
+	mutex_unlock(&imx219->mutex);
+
+	return 0;
+}
+
+static int imx219_start_streaming(struct imx219 *imx219)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
+	const struct imx219_reg_list *reg_list;
+	int ret;
+
+	/* Apply default values of current mode */
+	reg_list = &imx219->mode->reg_list;
+	ret = imx219_write_regs(imx219, reg_list->regs, reg_list->num_of_regs);
+	if (ret) {
+		dev_err(&client->dev, "%s failed to set mode\n", __func__);
+		return ret;
+	}
+
+	/* Apply customized values from user */
+	ret =  __v4l2_ctrl_handler_setup(imx219->sd.ctrl_handler);
+	if (ret)
+		return ret;
+
+	/* set stream on register */
+	return imx219_write_reg(imx219, IMX219_REG_MODE_SELECT,
+				IMX219_REG_VALUE_08BIT, IMX219_MODE_STREAMING);
+}
+
+static void imx219_stop_streaming(struct imx219 *imx219)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
+	int ret;
+
+	/* set stream off register */
+	ret = imx219_write_reg(imx219, IMX219_REG_MODE_SELECT,
+			       IMX219_REG_VALUE_08BIT, IMX219_MODE_STANDBY);
+	if (ret)
+		dev_err(&client->dev, "%s failed to set stream\n", __func__);
+}
+
+static int imx219_set_stream(struct v4l2_subdev *sd, int enable)
+{
+	struct imx219 *imx219 = to_imx219(sd);
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+	int ret = 0;
+
+	mutex_lock(&imx219->mutex);
+	if (imx219->streaming == enable) {
+		mutex_unlock(&imx219->mutex);
+		return 0;
+	}
+
+	if (enable) {
+		ret = pm_runtime_get_sync(&client->dev);
+		if (ret < 0) {
+			pm_runtime_put_noidle(&client->dev);
+			goto err_unlock;
+		}
+
+		/*
+		 * Apply default & customized values
+		 * and then start streaming.
+		 */
+		ret = imx219_start_streaming(imx219);
+		if (ret)
+			goto err_rpm_put;
+	} else {
+		imx219_stop_streaming(imx219);
+		pm_runtime_put(&client->dev);
+	}
+
+	imx219->streaming = enable;
+
+	/* vflip and hflip cannot change during streaming */
+	__v4l2_ctrl_grab(imx219->vflip, enable);
+	__v4l2_ctrl_grab(imx219->hflip, enable);
+
+	mutex_unlock(&imx219->mutex);
+
+	return ret;
+
+err_rpm_put:
+	pm_runtime_put(&client->dev);
+err_unlock:
+	mutex_unlock(&imx219->mutex);
+
+	return ret;
+}
+
+/* Power/clock management functions */
+static int imx219_power_on(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct imx219 *imx219 = to_imx219(sd);
+	int ret;
+
+	ret = regulator_bulk_enable(IMX219_NUM_SUPPLIES,
+				    imx219->supplies);
+	if (ret) {
+		dev_err(&client->dev, "%s: failed to enable regulators\n",
+			__func__);
+		return ret;
+	}
+
+	ret = clk_prepare_enable(imx219->xclk);
+	if (ret) {
+		dev_err(&client->dev, "%s: failed to enable clock\n",
+			__func__);
+		goto reg_off;
+	}
+
+	gpiod_set_value_cansleep(imx219->xclr_gpio, 1);
+	msleep(IMX219_XCLR_DELAY_MS);
+
+	return 0;
+reg_off:
+	regulator_bulk_disable(IMX219_NUM_SUPPLIES, imx219->supplies);
+	return ret;
+}
+
+static int imx219_power_off(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct imx219 *imx219 = to_imx219(sd);
+
+	gpiod_set_value_cansleep(imx219->xclr_gpio, 0);
+	regulator_bulk_disable(IMX219_NUM_SUPPLIES, imx219->supplies);
+	clk_disable_unprepare(imx219->xclk);
+
+	return 0;
+}
+
+static int __maybe_unused imx219_suspend(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct imx219 *imx219 = to_imx219(sd);
+
+	if (imx219->streaming)
+		imx219_stop_streaming(imx219);
+
+	return 0;
+}
+
+static int __maybe_unused imx219_resume(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct imx219 *imx219 = to_imx219(sd);
+	int ret;
+
+	if (imx219->streaming) {
+		ret = imx219_start_streaming(imx219);
+		if (ret)
+			goto error;
+	}
+
+	return 0;
+
+error:
+	imx219_stop_streaming(imx219);
+	imx219->streaming = 0;
+	return ret;
+}
+
+static int imx219_get_regulators(struct imx219 *imx219)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
+	int i;
+
+	for (i = 0; i < IMX219_NUM_SUPPLIES; i++)
+		imx219->supplies[i].supply = imx219_supply_name[i];
+
+	return devm_regulator_bulk_get(&client->dev,
+				       IMX219_NUM_SUPPLIES,
+				       imx219->supplies);
+}
+
+/* Verify chip ID */
+static int imx219_identify_module(struct imx219 *imx219)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
+	int ret;
+	u32 val;
+
+	ret = imx219_read_reg(imx219, IMX219_REG_CHIP_ID,
+			      IMX219_REG_VALUE_16BIT, &val);
+	if (ret) {
+		dev_err(&client->dev, "failed to read chip id %x\n",
+			IMX219_CHIP_ID);
+		return ret;
+	}
+
+	if (val != IMX219_CHIP_ID) {
+		dev_err(&client->dev, "chip id mismatch: %x!=%x\n",
+			IMX219_CHIP_ID, val);
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static const struct v4l2_subdev_core_ops imx219_core_ops = {
+	.subscribe_event = v4l2_ctrl_subdev_subscribe_event,
+	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
+};
+
+static const struct v4l2_subdev_video_ops imx219_video_ops = {
+	.s_stream = imx219_set_stream,
+};
+
+static const struct v4l2_subdev_pad_ops imx219_pad_ops = {
+	.enum_mbus_code = imx219_enum_mbus_code,
+	.get_fmt = imx219_get_pad_format,
+	.set_fmt = imx219_set_pad_format,
+	.enum_frame_size = imx219_enum_frame_size,
+};
+
+static const struct v4l2_subdev_ops imx219_subdev_ops = {
+	.core = &imx219_core_ops,
+	.video = &imx219_video_ops,
+	.pad = &imx219_pad_ops,
+};
+
+static const struct v4l2_subdev_internal_ops imx219_internal_ops = {
+	.open = imx219_open,
+};
+
+/* Initialize control handlers */
+static int imx219_init_controls(struct imx219 *imx219)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
+	struct v4l2_ctrl_handler *ctrl_hdlr;
+	unsigned int height = imx219->mode->height;
+	int exposure_max, exposure_def, hblank;
+	int i, ret;
+
+	ctrl_hdlr = &imx219->ctrl_handler;
+	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 9);
+	if (ret)
+		return ret;
+
+	mutex_init(&imx219->mutex);
+	ctrl_hdlr->lock = &imx219->mutex;
+
+	/* By default, PIXEL_RATE is read only */
+	imx219->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops,
+					       V4L2_CID_PIXEL_RATE,
+					       IMX219_PIXEL_RATE,
+					       IMX219_PIXEL_RATE, 1,
+					       IMX219_PIXEL_RATE);
+
+	/* Initial vblank/hblank/exposure parameters based on current mode */
+	imx219->vblank = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops,
+					   V4L2_CID_VBLANK, IMX219_VBLANK_MIN,
+					   IMX219_VTS_MAX - height, 1,
+					   imx219->mode->vts_def - height);
+	hblank = IMX219_PPL_DEFAULT - imx219->mode->width;
+	imx219->hblank = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops,
+					   V4L2_CID_HBLANK, hblank, hblank,
+					   1, hblank);
+	if (imx219->hblank)
+		imx219->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+	exposure_max = imx219->mode->vts_def - 4;
+	exposure_def = (exposure_max < IMX219_EXPOSURE_DEFAULT) ?
+		exposure_max : IMX219_EXPOSURE_DEFAULT;
+	imx219->exposure = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops,
+					     V4L2_CID_EXPOSURE,
+					     IMX219_EXPOSURE_MIN, exposure_max,
+					     IMX219_EXPOSURE_STEP,
+					     exposure_def);
+
+	v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops, V4L2_CID_ANALOGUE_GAIN,
+			  IMX219_ANA_GAIN_MIN, IMX219_ANA_GAIN_MAX,
+			  IMX219_ANA_GAIN_STEP, IMX219_ANA_GAIN_DEFAULT);
+
+	v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops, V4L2_CID_DIGITAL_GAIN,
+			  IMX219_DGTL_GAIN_MIN, IMX219_DGTL_GAIN_MAX,
+			  IMX219_DGTL_GAIN_STEP, IMX219_DGTL_GAIN_DEFAULT);
+
+	imx219->hflip = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops,
+					  V4L2_CID_HFLIP, 0, 1, 1, 0);
+	if (imx219->hflip)
+		imx219->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
+
+	imx219->vflip = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops,
+					  V4L2_CID_VFLIP, 0, 1, 1, 0);
+	if (imx219->vflip)
+		imx219->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
+
+	v4l2_ctrl_new_std_menu_items(ctrl_hdlr, &imx219_ctrl_ops,
+				     V4L2_CID_TEST_PATTERN,
+				     ARRAY_SIZE(imx219_test_pattern_menu) - 1,
+				     0, 0, imx219_test_pattern_menu);
+	for (i = 0; i < 4; i++) {
+		/*
+		 * The assumption is that
+		 * V4L2_CID_TEST_PATTERN_GREENR == V4L2_CID_TEST_PATTERN_RED + 1
+		 * V4L2_CID_TEST_PATTERN_BLUE   == V4L2_CID_TEST_PATTERN_RED + 2
+		 * V4L2_CID_TEST_PATTERN_GREENB == V4L2_CID_TEST_PATTERN_RED + 3
+		 */
+		v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops,
+				  V4L2_CID_TEST_PATTERN_RED + i,
+				  IMX219_TESTP_COLOUR_MIN,
+				  IMX219_TESTP_COLOUR_MAX,
+				  IMX219_TESTP_COLOUR_STEP,
+				  IMX219_TESTP_COLOUR_MAX);
+		/* The "Solid color" pattern is white by default */
+	}
+
+	if (ctrl_hdlr->error) {
+		ret = ctrl_hdlr->error;
+		dev_err(&client->dev, "%s control init failed (%d)\n",
+			__func__, ret);
+		goto error;
+	}
+
+	imx219->sd.ctrl_handler = ctrl_hdlr;
+
+	return 0;
+
+error:
+	v4l2_ctrl_handler_free(ctrl_hdlr);
+	mutex_destroy(&imx219->mutex);
+
+	return ret;
+}
+
+static void imx219_free_controls(struct imx219 *imx219)
+{
+	v4l2_ctrl_handler_free(imx219->sd.ctrl_handler);
+	mutex_destroy(&imx219->mutex);
+}
+
+static int imx219_probe(struct i2c_client *client,
+			const struct i2c_device_id *id)
+{
+	struct device *dev = &client->dev;
+	struct fwnode_handle *endpoint;
+	struct imx219 *imx219;
+	int ret;
+
+	imx219 = devm_kzalloc(&client->dev, sizeof(*imx219), GFP_KERNEL);
+	if (!imx219)
+		return -ENOMEM;
+
+	imx219->dev = dev;
+
+	v4l2_i2c_subdev_init(&imx219->sd, client, &imx219_subdev_ops);
+
+	/* Get CSI2 bus config */
+	endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(&client->dev),
+						  NULL);
+	if (!endpoint) {
+		dev_err(dev, "endpoint node not found\n");
+		return -EINVAL;
+	}
+
+	ret = v4l2_fwnode_endpoint_parse(endpoint, &imx219->ep);
+	fwnode_handle_put(endpoint);
+	if (ret) {
+		dev_err(dev, "Could not parse endpoint\n");
+		return ret;
+	}
+
+	/* Get system clock (xclk) */
+	imx219->xclk = devm_clk_get(dev, "xclk");
+	if (IS_ERR(imx219->xclk)) {
+		dev_err(dev, "failed to get xclk\n");
+		return PTR_ERR(imx219->xclk);
+	}
+
+	imx219->xclk_freq = clk_get_rate(imx219->xclk);
+	if (imx219->xclk_freq != IMX219_XCLK_FREQ) {
+		dev_err(dev, "xclk frequency not supported: %d Hz\n",
+			imx219->xclk_freq);
+		return -EINVAL;
+	}
+
+	ret = imx219_get_regulators(imx219);
+	if (ret)
+		return ret;
+
+	/* Request optional enable pin */
+	imx219->xclr_gpio = devm_gpiod_get_optional(dev, "xclr",
+						    GPIOD_OUT_HIGH);
+
+	/*
+	 * The sensor must be powered for imx219_identify_module()
+	 * to be able to read the CHIP_ID register
+	 */
+	ret = imx219_power_on(dev);
+	if (ret)
+		return ret;
+
+	ret = imx219_identify_module(imx219);
+	if (ret)
+		goto error_power_off;
+
+	/* Set default mode to max resolution */
+	imx219->mode = &supported_modes[0];
+
+	ret = imx219_init_controls(imx219);
+	if (ret)
+		goto error_power_off;
+
+	/* Initialize subdev */
+	imx219->sd.internal_ops = &imx219_internal_ops;
+	imx219->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+	imx219->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
+
+	/* Initialize source pad */
+	imx219->pad.flags = MEDIA_PAD_FL_SOURCE;
+
+	ret = media_entity_pads_init(&imx219->sd.entity, 1, &imx219->pad);
+	if (ret)
+		goto error_handler_free;
+
+	ret = v4l2_async_register_subdev_sensor_common(&imx219->sd);
+	if (ret < 0)
+		goto error_media_entity;
+
+	/* Enable runtime PM and turn off the device */
+	pm_runtime_set_active(dev);
+	pm_runtime_enable(dev);
+	pm_runtime_idle(dev);
+
+	return 0;
+
+error_media_entity:
+	media_entity_cleanup(&imx219->sd.entity);
+
+error_handler_free:
+	imx219_free_controls(imx219);
+
+error_power_off:
+	imx219_power_off(dev);
+
+	return ret;
+}
+
+static int imx219_remove(struct i2c_client *client)
+{
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct imx219 *imx219 = to_imx219(sd);
+
+	v4l2_async_unregister_subdev(sd);
+	media_entity_cleanup(&sd->entity);
+	imx219_free_controls(imx219);
+
+	pm_runtime_disable(&client->dev);
+	pm_runtime_set_suspended(&client->dev);
+
+	return 0;
+}
+
+static const struct of_device_id imx219_dt_ids[] = {
+	{ .compatible = "sony,imx219" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, imx219_dt_ids);
+
+static const struct dev_pm_ops imx219_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(imx219_suspend, imx219_resume)
+	SET_RUNTIME_PM_OPS(imx219_power_off, imx219_power_on, NULL)
+};
+
+static struct i2c_driver imx219_i2c_driver = {
+	.driver = {
+		.name = "imx219",
+		.of_match_table	= imx219_dt_ids,
+		.pm = &imx219_pm_ops,
+	},
+	.probe = imx219_probe,
+	.remove = imx219_remove,
+};
+
+module_i2c_driver(imx219_i2c_driver);
+
+MODULE_AUTHOR("Dave Stevenson <dave.stevenson@raspberrypi.com");
+MODULE_DESCRIPTION("Sony IMX219 sensor driver");
+MODULE_LICENSE("GPL v2");
-- 
2.17.1

Re: [PATCH v2 2/2] media: i2c: Add driver for Sony IMX219 sensor

[Sakari Ailus]

Hi Andrey,

On Fri, Dec 27, 2019 at 03:21:14PM +0300, Andrey Konovalov wrote:
> From: Dave Stevenson <dave.stevenson@raspberrypi.com>
> 
> Adds a driver for the 8MPix Sony IMX219 CSI2 sensor.
> Whilst the sensor supports 2 or 4 CSI2 data lanes, this driver
> currently only supports 2 lanes.
> 8MPix @ 15fps, 1080P @ 30fps (cropped FOV), and 1640x1232 (2x2 binned)
> @ 30fps are currently supported.
> 
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
> ---
>  drivers/media/i2c/Kconfig  |   12 +
>  drivers/media/i2c/Makefile |    1 +
>  drivers/media/i2c/imx219.c | 1240 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 1253 insertions(+)
>  create mode 100644 drivers/media/i2c/imx219.c
> 
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index c68e002d26ea..6fa5af1f72b9 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -591,6 +591,18 @@ config VIDEO_IMX214
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called imx214.
>  
> +config VIDEO_IMX219
> +	tristate "Sony IMX219 sensor support"
> +	depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
> +	depends on MEDIA_CAMERA_SUPPORT

You can safely drop this line.

> +	select V4L2_FWNODE
> +	help
> +	  This is a Video4Linux2 sensor driver for the Sony
> +	  IMX219 camera.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called imx219.
> +
>  config VIDEO_IMX258
>  	tristate "Sony IMX258 sensor support"
>  	depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> index c147bb9d28db..77bf7d0b691f 100644
> --- a/drivers/media/i2c/Makefile
> +++ b/drivers/media/i2c/Makefile
> @@ -111,6 +111,7 @@ obj-$(CONFIG_VIDEO_OV2659)	+= ov2659.o
>  obj-$(CONFIG_VIDEO_TC358743)	+= tc358743.o
>  obj-$(CONFIG_VIDEO_HI556)	+= hi556.o
>  obj-$(CONFIG_VIDEO_IMX214)	+= imx214.o
> +obj-$(CONFIG_VIDEO_IMX219)	+= imx219.o
>  obj-$(CONFIG_VIDEO_IMX258)	+= imx258.o
>  obj-$(CONFIG_VIDEO_IMX274)	+= imx274.o
>  obj-$(CONFIG_VIDEO_IMX290)	+= imx290.o
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> new file mode 100644
> index 000000000000..28b55c61cd77
> --- /dev/null
> +++ b/drivers/media/i2c/imx219.c
> @@ -0,0 +1,1240 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * A V4L2 driver for Sony IMX219 cameras.
> + * Copyright (C) 2019, Raspberry Pi (Trading) Ltd
> + *
> + * Based on Sony imx258 camera driver
> + * Copyright (C) 2018 Intel Corporation
> + *
> + * DT / fwnode changes, and regulator / GPIO control taken from imx214 driver
> + * Copyright 2018 Qtechnology A/S
> + *
> + * Flip handling taken from the Sony IMX319 driver.
> + * Copyright (C) 2018 Intel Corporation
> + *
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/clkdev.h>
> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regulator/consumer.h>
> +#include <media/v4l2-ctrls.h>
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-event.h>
> +#include <media/v4l2-fwnode.h>
> +#include <media/v4l2-mediabus.h>
> +#include <asm/unaligned.h>
> +
> +#define IMX219_REG_VALUE_08BIT		1
> +#define IMX219_REG_VALUE_16BIT		2
> +
> +#define IMX219_REG_MODE_SELECT		0x0100
> +#define IMX219_MODE_STANDBY		0x00
> +#define IMX219_MODE_STREAMING		0x01
> +
> +/* Chip ID */
> +#define IMX219_REG_CHIP_ID		0x0000
> +#define IMX219_CHIP_ID			0x0219
> +
> +/* External clock frequency is 24.0M */
> +#define IMX219_XCLK_FREQ		24000000
> +
> +/* Pixel rate is fixed at 182.4M for all the modes */
> +#define IMX219_PIXEL_RATE		182400000
> +
> +/* V_TIMING internal */
> +#define IMX219_REG_VTS			0x0160
> +#define IMX219_VTS_15FPS		0x0dc6
> +#define IMX219_VTS_30FPS_1080P		0x06e3
> +#define IMX219_VTS_30FPS_BINNED		0x06e3
> +#define IMX219_VTS_MAX			0xffff
> +
> +#define IMX219_VBLANK_MIN		4
> +
> +/*Frame Length Line*/
> +#define IMX219_FLL_MIN			0x08a6
> +#define IMX219_FLL_MAX			0xffff
> +#define IMX219_FLL_STEP			1
> +#define IMX219_FLL_DEFAULT		0x0c98
> +
> +/* HBLANK control - read only */
> +#define IMX219_PPL_DEFAULT		3448
> +
> +/* Exposure control */
> +#define IMX219_REG_EXPOSURE		0x015a
> +#define IMX219_EXPOSURE_MIN		4
> +#define IMX219_EXPOSURE_STEP		1
> +#define IMX219_EXPOSURE_DEFAULT		0x640
> +#define IMX219_EXPOSURE_MAX		65535
> +
> +/* Analog gain control */
> +#define IMX219_REG_ANALOG_GAIN		0x0157
> +#define IMX219_ANA_GAIN_MIN		0
> +#define IMX219_ANA_GAIN_MAX		232
> +#define IMX219_ANA_GAIN_STEP		1
> +#define IMX219_ANA_GAIN_DEFAULT		0x0
> +
> +/* Digital gain control */
> +#define IMX219_REG_DIGITAL_GAIN		0x0158
> +#define IMX219_DGTL_GAIN_MIN		0x0100
> +#define IMX219_DGTL_GAIN_MAX		0x0fff
> +#define IMX219_DGTL_GAIN_DEFAULT	0x0100
> +#define IMX219_DGTL_GAIN_STEP		1
> +
> +#define IMX219_REG_ORIENTATION		0x0172
> +
> +/* Test Pattern Control */
> +#define IMX219_REG_TEST_PATTERN		0x0600
> +#define IMX219_TEST_PATTERN_DISABLE	0
> +#define IMX219_TEST_PATTERN_SOLID_COLOR	1
> +#define IMX219_TEST_PATTERN_COLOR_BARS	2
> +#define IMX219_TEST_PATTERN_GREY_COLOR	3
> +#define IMX219_TEST_PATTERN_PN9		4
> +
> +/* Test pattern colour components */
> +#define IMX219_REG_TESTP_RED		0x0602
> +#define IMX219_REG_TESTP_GREENR		0x0604
> +#define IMX219_REG_TESTP_BLUE		0x0606
> +#define IMX219_REG_TESTP_GREENB		0x0608
> +#define IMX219_TESTP_COLOUR_MIN		0
> +#define IMX219_TESTP_COLOUR_MAX		0x03ff
> +#define IMX219_TESTP_COLOUR_STEP	1
> +#define IMX219_TESTP_RED_DEFAULT	IMX219_TESTP_COLOUR_MAX
> +#define IMX219_TESTP_GREENR_DEFAULT	0
> +#define IMX219_TESTP_BLUE_DEFAULT	0
> +#define IMX219_TESTP_GREENB_DEFAULT	0
> +
> +struct imx219_reg {
> +	u16 address;
> +	u8 val;
> +};
> +
> +struct imx219_reg_list {
> +	unsigned int num_of_regs;
> +	const struct imx219_reg *regs;
> +};
> +
> +/* Mode : resolution and related config&values */
> +struct imx219_mode {
> +	/* Frame width */
> +	unsigned int width;
> +	/* Frame height */
> +	unsigned int height;
> +
> +	/* V-timing */
> +	unsigned int vts_def;
> +
> +	/* Default register values */
> +	struct imx219_reg_list reg_list;
> +};
> +
> +/*
> + * Register sets lifted off the i2C interface from the Raspberry Pi firmware
> + * driver.
> + * 3280x2464 = mode 2, 1920x1080 = mode 1, and 1640x1232 = mode 4.
> + */
> +static const struct imx219_reg mode_3280x2464_regs[] = {
> +	{0x0100, 0x00},
> +	{0x30eb, 0x0c},
> +	{0x30eb, 0x05},
> +	{0x300a, 0xff},
> +	{0x300b, 0xff},
> +	{0x30eb, 0x05},
> +	{0x30eb, 0x09},
> +	{0x0114, 0x01},
> +	{0x0128, 0x00},
> +	{0x012a, 0x18},
> +	{0x012b, 0x00},
> +	{0x0164, 0x00},
> +	{0x0165, 0x00},
> +	{0x0166, 0x0c},
> +	{0x0167, 0xcf},
> +	{0x0168, 0x00},
> +	{0x0169, 0x00},
> +	{0x016a, 0x09},
> +	{0x016b, 0x9f},
> +	{0x016c, 0x0c},
> +	{0x016d, 0xd0},
> +	{0x016e, 0x09},
> +	{0x016f, 0xa0},
> +	{0x0170, 0x01},
> +	{0x0171, 0x01},
> +	{0x0174, 0x00},
> +	{0x0175, 0x00},
> +	{0x018c, 0x0a},
> +	{0x018d, 0x0a},
> +	{0x0301, 0x05},
> +	{0x0303, 0x01},
> +	{0x0304, 0x03},
> +	{0x0305, 0x03},
> +	{0x0306, 0x00},
> +	{0x0307, 0x39},
> +	{0x0309, 0x0a},
> +	{0x030b, 0x01},
> +	{0x030c, 0x00},
> +	{0x030d, 0x72},
> +	{0x0624, 0x0c},
> +	{0x0625, 0xd0},
> +	{0x0626, 0x09},
> +	{0x0627, 0xa0},
> +	{0x455e, 0x00},
> +	{0x471e, 0x4b},
> +	{0x4767, 0x0f},
> +	{0x4750, 0x14},
> +	{0x4540, 0x00},
> +	{0x47b4, 0x14},
> +	{0x4713, 0x30},
> +	{0x478b, 0x10},
> +	{0x478f, 0x10},
> +	{0x4793, 0x10},
> +	{0x4797, 0x0e},
> +	{0x479b, 0x0e},
> +	{0x0162, 0x0d},
> +	{0x0163, 0x78},
> +};
> +
> +static const struct imx219_reg mode_1920_1080_regs[] = {
> +	{0x0100, 0x00},
> +	{0x30eb, 0x05},
> +	{0x30eb, 0x0c},
> +	{0x300a, 0xff},
> +	{0x300b, 0xff},
> +	{0x30eb, 0x05},
> +	{0x30eb, 0x09},
> +	{0x0114, 0x01},
> +	{0x0128, 0x00},
> +	{0x012a, 0x18},
> +	{0x012b, 0x00},
> +	{0x0162, 0x0d},
> +	{0x0163, 0x78},
> +	{0x0164, 0x02},
> +	{0x0165, 0xa8},
> +	{0x0166, 0x0a},
> +	{0x0167, 0x27},
> +	{0x0168, 0x02},
> +	{0x0169, 0xb4},
> +	{0x016a, 0x06},
> +	{0x016b, 0xeb},
> +	{0x016c, 0x07},
> +	{0x016d, 0x80},
> +	{0x016e, 0x04},
> +	{0x016f, 0x38},
> +	{0x0170, 0x01},
> +	{0x0171, 0x01},
> +	{0x0174, 0x00},
> +	{0x0175, 0x00},
> +	{0x018c, 0x0a},
> +	{0x018d, 0x0a},
> +	{0x0301, 0x05},
> +	{0x0303, 0x01},
> +	{0x0304, 0x03},
> +	{0x0305, 0x03},
> +	{0x0306, 0x00},
> +	{0x0307, 0x39},
> +	{0x0309, 0x0a},
> +	{0x030b, 0x01},
> +	{0x030c, 0x00},
> +	{0x030d, 0x72},
> +	{0x0624, 0x07},
> +	{0x0625, 0x80},
> +	{0x0626, 0x04},
> +	{0x0627, 0x38},
> +	{0x455e, 0x00},
> +	{0x471e, 0x4b},
> +	{0x4767, 0x0f},
> +	{0x4750, 0x14},
> +	{0x4540, 0x00},
> +	{0x47b4, 0x14},
> +	{0x4713, 0x30},
> +	{0x478b, 0x10},
> +	{0x478f, 0x10},
> +	{0x4793, 0x10},
> +	{0x4797, 0x0e},
> +	{0x479b, 0x0e},
> +	{0x0162, 0x0d},
> +	{0x0163, 0x78},
> +};
> +
> +static const struct imx219_reg mode_1640_1232_regs[] = {
> +	{0x0100, 0x00},
> +	{0x30eb, 0x0c},
> +	{0x30eb, 0x05},
> +	{0x300a, 0xff},
> +	{0x300b, 0xff},
> +	{0x30eb, 0x05},
> +	{0x30eb, 0x09},
> +	{0x0114, 0x01},
> +	{0x0128, 0x00},
> +	{0x012a, 0x18},
> +	{0x012b, 0x00},
> +	{0x0164, 0x00},
> +	{0x0165, 0x00},
> +	{0x0166, 0x0c},
> +	{0x0167, 0xcf},
> +	{0x0168, 0x00},
> +	{0x0169, 0x00},
> +	{0x016a, 0x09},
> +	{0x016b, 0x9f},
> +	{0x016c, 0x06},
> +	{0x016d, 0x68},
> +	{0x016e, 0x04},
> +	{0x016f, 0xd0},
> +	{0x0170, 0x01},
> +	{0x0171, 0x01},
> +	{0x0174, 0x01},
> +	{0x0175, 0x01},
> +	{0x018c, 0x0a},
> +	{0x018d, 0x0a},
> +	{0x0301, 0x05},
> +	{0x0303, 0x01},
> +	{0x0304, 0x03},
> +	{0x0305, 0x03},
> +	{0x0306, 0x00},
> +	{0x0307, 0x39},
> +	{0x0309, 0x0a},
> +	{0x030b, 0x01},
> +	{0x030c, 0x00},
> +	{0x030d, 0x72},
> +	{0x0624, 0x06},
> +	{0x0625, 0x68},
> +	{0x0626, 0x04},
> +	{0x0627, 0xd0},
> +	{0x455e, 0x00},
> +	{0x471e, 0x4b},
> +	{0x4767, 0x0f},
> +	{0x4750, 0x14},
> +	{0x4540, 0x00},
> +	{0x47b4, 0x14},
> +	{0x4713, 0x30},
> +	{0x478b, 0x10},
> +	{0x478f, 0x10},
> +	{0x4793, 0x10},
> +	{0x4797, 0x0e},
> +	{0x479b, 0x0e},
> +	{0x0162, 0x0d},
> +	{0x0163, 0x78},
> +};
> +
> +static const char * const imx219_test_pattern_menu[] = {
> +	"Disabled",
> +	"Color Bars",
> +	"Solid Color",
> +	"Grey Color Bars",
> +	"PN9"
> +};
> +
> +static const int imx219_test_pattern_val[] = {
> +	IMX219_TEST_PATTERN_DISABLE,
> +	IMX219_TEST_PATTERN_COLOR_BARS,
> +	IMX219_TEST_PATTERN_SOLID_COLOR,
> +	IMX219_TEST_PATTERN_GREY_COLOR,
> +	IMX219_TEST_PATTERN_PN9,
> +};
> +
> +/* regulator supplies */
> +static const char * const imx219_supply_name[] = {
> +	/* Supplies can be enabled in any order */
> +	"VANA",  /* Analog (2.8V) supply */
> +	"VDIG",  /* Digital Core (1.8V) supply */
> +	"VDDL",  /* IF (1.2V) supply */
> +};
> +
> +#define IMX219_NUM_SUPPLIES ARRAY_SIZE(imx219_supply_name)
> +
> +#define IMX219_XCLR_DELAY_MS 10	/* Initialisation delay after XCLR low->high */
> +
> +/* Mode configs */
> +static const struct imx219_mode supported_modes[] = {
> +	{
> +		/* 8MPix 15fps mode */
> +		.width = 3280,
> +		.height = 2464,
> +		.vts_def = IMX219_VTS_15FPS,
> +		.reg_list = {
> +			.num_of_regs = ARRAY_SIZE(mode_3280x2464_regs),
> +			.regs = mode_3280x2464_regs,
> +		},
> +	},
> +	{
> +		/* 1080P 30fps cropped */
> +		.width = 1920,
> +		.height = 1080,
> +		.vts_def = IMX219_VTS_30FPS_1080P,
> +		.reg_list = {
> +			.num_of_regs = ARRAY_SIZE(mode_1920_1080_regs),
> +			.regs = mode_1920_1080_regs,
> +		},
> +	},
> +	{
> +		/* 2x2 binned 30fps mode */
> +		.width = 1640,
> +		.height = 1232,
> +		.vts_def = IMX219_VTS_30FPS_BINNED,
> +		.reg_list = {
> +			.num_of_regs = ARRAY_SIZE(mode_1640_1232_regs),
> +			.regs = mode_1640_1232_regs,
> +		},
> +	},
> +};
> +
> +struct imx219 {
> +	struct device *dev;
> +
> +	struct v4l2_subdev sd;
> +	struct media_pad pad;
> +
> +	struct v4l2_fwnode_endpoint ep; /* the parsed DT endpoint info */
> +	struct clk *xclk; /* system clock to IMX219 */
> +	u32 xclk_freq;
> +
> +	struct gpio_desc *xclr_gpio;
> +	struct regulator_bulk_data supplies[IMX219_NUM_SUPPLIES];
> +
> +	struct v4l2_ctrl_handler ctrl_handler;
> +	/* V4L2 Controls */
> +	struct v4l2_ctrl *pixel_rate;
> +	struct v4l2_ctrl *exposure;
> +	struct v4l2_ctrl *vflip;
> +	struct v4l2_ctrl *hflip;
> +	struct v4l2_ctrl *vblank;
> +	struct v4l2_ctrl *hblank;
> +
> +	/* Current mode */
> +	const struct imx219_mode *mode;
> +
> +	/*
> +	 * Mutex for serialized access:
> +	 * Protect sensor module set pad format and start/stop streaming safely.
> +	 */
> +	struct mutex mutex;
> +
> +	/* Streaming on/off */
> +	bool streaming;
> +};
> +
> +static inline struct imx219 *to_imx219(struct v4l2_subdev *_sd)
> +{
> +	return container_of(_sd, struct imx219, sd);
> +}
> +
> +/* Read registers up to 2 at a time */
> +static int imx219_read_reg(struct imx219 *imx219, u16 reg, u32 len, u32 *val)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
> +	struct i2c_msg msgs[2];
> +	u8 addr_buf[2] = { reg >> 8, reg & 0xff };
> +	u8 data_buf[4] = { 0, };
> +	int ret;
> +
> +	if (len > 4)
> +		return -EINVAL;
> +
> +	/* Write register address */
> +	msgs[0].addr = client->addr;
> +	msgs[0].flags = 0;
> +	msgs[0].len = ARRAY_SIZE(addr_buf);
> +	msgs[0].buf = addr_buf;
> +
> +	/* Read data from register */
> +	msgs[1].addr = client->addr;
> +	msgs[1].flags = I2C_M_RD;
> +	msgs[1].len = len;
> +	msgs[1].buf = &data_buf[4 - len];
> +
> +	ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
> +	if (ret != ARRAY_SIZE(msgs))
> +		return -EIO;
> +
> +	*val = get_unaligned_be32(data_buf);
> +
> +	return 0;
> +}
> +
> +/* Write registers up to 2 at a time */
> +static int imx219_write_reg(struct imx219 *imx219, u16 reg, u32 len, u32 val)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
> +	u8 buf[6];
> +
> +	if (len > 4)
> +		return -EINVAL;
> +
> +	put_unaligned_be16(reg, buf);
> +	put_unaligned_be32(val << (8 * (4 - len)), buf + 2);
> +	if (i2c_master_send(client, buf, len + 2) != len + 2)
> +		return -EIO;
> +
> +	return 0;
> +}
> +
> +/* Write a list of registers */
> +static int imx219_write_regs(struct imx219 *imx219,
> +			     const struct imx219_reg *regs, u32 len)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
> +	unsigned int i;
> +	int ret;
> +
> +	for (i = 0; i < len; i++) {
> +		ret = imx219_write_reg(imx219, regs[i].address, 1, regs[i].val);
> +		if (ret) {
> +			dev_err_ratelimited(&client->dev,
> +					    "Failed to write reg 0x%4.4x. error = %d\n",
> +					    regs[i].address, ret);
> +
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +/* Get bayer order based on flip setting. */
> +static u32 imx219_get_format_code(struct imx219 *imx219)
> +{
> +	/*
> +	 * Only one bayer order is supported.
> +	 * It depends on the flip settings.
> +	 */
> +	static const u32 codes[2][2] = {
> +		{ MEDIA_BUS_FMT_SRGGB10_1X10, MEDIA_BUS_FMT_SGRBG10_1X10, },
> +		{ MEDIA_BUS_FMT_SGBRG10_1X10, MEDIA_BUS_FMT_SBGGR10_1X10, },
> +	};
> +
> +	lockdep_assert_held(&imx219->mutex);
> +	return codes[imx219->vflip->val][imx219->hflip->val];
> +}
> +
> +static int imx219_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> +{
> +	struct imx219 *imx219 = to_imx219(sd);
> +	struct v4l2_mbus_framefmt *try_fmt =
> +		v4l2_subdev_get_try_format(sd, fh->pad, 0);
> +
> +	mutex_lock(&imx219->mutex);
> +
> +	/* Initialize try_fmt */
> +	try_fmt->width = supported_modes[0].width;
> +	try_fmt->height = supported_modes[0].height;
> +	try_fmt->code = imx219_get_format_code(imx219);

Please assign this when getting the format (subdev pad get_fmt callback).

> +	try_fmt->field = V4L2_FIELD_NONE;
> +
> +	mutex_unlock(&imx219->mutex);
> +
> +	return 0;
> +}
> +
> +static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct imx219 *imx219 =
> +		container_of(ctrl->handler, struct imx219, ctrl_handler);
> +	struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
> +	int ret;
> +
> +	if (ctrl->id == V4L2_CID_VBLANK) {
> +		int exposure_max, exposure_def;
> +
> +		/* Update max exposure while meeting expected vblanking */
> +		exposure_max = imx219->mode->height + ctrl->val - 4;
> +		exposure_def = (exposure_max < IMX219_EXPOSURE_DEFAULT) ?
> +			exposure_max : IMX219_EXPOSURE_DEFAULT;
> +		__v4l2_ctrl_modify_range(imx219->exposure,
> +					 imx219->exposure->minimum,
> +					 exposure_max, imx219->exposure->step,
> +					 exposure_def);
> +	}
> +
> +	/*
> +	 * Applying V4L2 control value only happens
> +	 * when power is up for streaming
> +	 */
> +	if (pm_runtime_get_if_in_use(&client->dev) == 0)
> +		return 0;
> +
> +	switch (ctrl->id) {
> +	case V4L2_CID_ANALOGUE_GAIN:
> +		ret = imx219_write_reg(imx219, IMX219_REG_ANALOG_GAIN,
> +				       IMX219_REG_VALUE_08BIT, ctrl->val);
> +		break;
> +	case V4L2_CID_EXPOSURE:
> +		ret = imx219_write_reg(imx219, IMX219_REG_EXPOSURE,
> +				       IMX219_REG_VALUE_16BIT, ctrl->val);
> +		break;
> +	case V4L2_CID_DIGITAL_GAIN:
> +		ret = imx219_write_reg(imx219, IMX219_REG_DIGITAL_GAIN,
> +				       IMX219_REG_VALUE_16BIT, ctrl->val);
> +		break;
> +	case V4L2_CID_TEST_PATTERN:
> +		ret = imx219_write_reg(imx219, IMX219_REG_TEST_PATTERN,
> +				       IMX219_REG_VALUE_16BIT,
> +				       imx219_test_pattern_val[ctrl->val]);
> +		break;
> +	case V4L2_CID_HFLIP:
> +	case V4L2_CID_VFLIP:
> +		ret = imx219_write_reg(imx219, IMX219_REG_ORIENTATION, 1,
> +				       imx219->hflip->val |
> +				       imx219->vflip->val << 1);
> +		break;
> +	case V4L2_CID_VBLANK:
> +		ret = imx219_write_reg(imx219, IMX219_REG_VTS,
> +				       IMX219_REG_VALUE_16BIT,
> +				       imx219->mode->height + ctrl->val);
> +		break;
> +	case V4L2_CID_TEST_PATTERN_RED:
> +		ret = imx219_write_reg(imx219, IMX219_REG_TESTP_RED,
> +				       IMX219_REG_VALUE_16BIT, ctrl->val);
> +		break;
> +	case V4L2_CID_TEST_PATTERN_GREENR:
> +		ret = imx219_write_reg(imx219, IMX219_REG_TESTP_GREENR,
> +				       IMX219_REG_VALUE_16BIT, ctrl->val);
> +		break;
> +	case V4L2_CID_TEST_PATTERN_BLUE:
> +		ret = imx219_write_reg(imx219, IMX219_REG_TESTP_BLUE,
> +				       IMX219_REG_VALUE_16BIT, ctrl->val);
> +		break;
> +	case V4L2_CID_TEST_PATTERN_GREENB:
> +		ret = imx219_write_reg(imx219, IMX219_REG_TESTP_GREENB,
> +				       IMX219_REG_VALUE_16BIT, ctrl->val);
> +		break;
> +	default:
> +		dev_info(&client->dev,
> +			 "ctrl(id:0x%x,val:0x%x) is not handled\n",
> +			 ctrl->id, ctrl->val);
> +		ret = -EINVAL;
> +		break;
> +	}
> +
> +	pm_runtime_put(&client->dev);
> +
> +	return ret;
> +}
> +
> +static const struct v4l2_ctrl_ops imx219_ctrl_ops = {
> +	.s_ctrl = imx219_set_ctrl,
> +};
> +
> +static int imx219_enum_mbus_code(struct v4l2_subdev *sd,
> +				 struct v4l2_subdev_pad_config *cfg,
> +				 struct v4l2_subdev_mbus_code_enum *code)
> +{
> +	struct imx219 *imx219 = to_imx219(sd);
> +
> +	/*
> +	 * Only one bayer order is supported (though it depends on the flip
> +	 * settings)
> +	 */
> +	if (code->index > 0)
> +		return -EINVAL;
> +
> +	code->code = imx219_get_format_code(imx219);
> +
> +	return 0;
> +}
> +
> +static int imx219_enum_frame_size(struct v4l2_subdev *sd,
> +				  struct v4l2_subdev_pad_config *cfg,
> +				  struct v4l2_subdev_frame_size_enum *fse)
> +{
> +	struct imx219 *imx219 = to_imx219(sd);
> +
> +	if (fse->index >= ARRAY_SIZE(supported_modes))
> +		return -EINVAL;
> +
> +	if (fse->code != imx219_get_format_code(imx219))
> +		return -EINVAL;
> +
> +	fse->min_width = supported_modes[fse->index].width;
> +	fse->max_width = fse->min_width;
> +	fse->min_height = supported_modes[fse->index].height;
> +	fse->max_height = fse->min_height;
> +
> +	return 0;
> +}
> +
> +static void imx219_reset_colorspace(struct v4l2_mbus_framefmt *fmt)
> +{
> +	fmt->colorspace = V4L2_COLORSPACE_SRGB;
> +	fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace);
> +	fmt->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
> +							  fmt->colorspace,
> +							  fmt->ycbcr_enc);
> +	fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt->colorspace);
> +}
> +
> +static void imx219_update_pad_format(struct imx219 *imx219,
> +				     const struct imx219_mode *mode,
> +				     struct v4l2_subdev_format *fmt)
> +{
> +	fmt->format.width = mode->width;
> +	fmt->format.height = mode->height;
> +	fmt->format.code = imx219_get_format_code(imx219);
> +	fmt->format.field = V4L2_FIELD_NONE;
> +
> +	imx219_reset_colorspace(&fmt->format);
> +}
> +
> +static int __imx219_get_pad_format(struct imx219 *imx219,
> +				   struct v4l2_subdev_pad_config *cfg,
> +				   struct v4l2_subdev_format *fmt)
> +{
> +	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY)
> +		fmt->format = *v4l2_subdev_get_try_format(&imx219->sd, cfg,
> +							  fmt->pad);
> +	else
> +		imx219_update_pad_format(imx219, imx219->mode, fmt);
> +
> +	return 0;
> +}
> +
> +static int imx219_get_pad_format(struct v4l2_subdev *sd,
> +				 struct v4l2_subdev_pad_config *cfg,
> +				 struct v4l2_subdev_format *fmt)
> +{
> +	struct imx219 *imx219 = to_imx219(sd);
> +	int ret;
> +
> +	mutex_lock(&imx219->mutex);
> +	ret = __imx219_get_pad_format(imx219, cfg, fmt);
> +	mutex_unlock(&imx219->mutex);
> +
> +	return ret;
> +}
> +
> +static int imx219_set_pad_format(struct v4l2_subdev *sd,
> +				 struct v4l2_subdev_pad_config *cfg,
> +				 struct v4l2_subdev_format *fmt)
> +{
> +	struct imx219 *imx219 = to_imx219(sd);
> +	const struct imx219_mode *mode;
> +	struct v4l2_mbus_framefmt *framefmt;
> +	int exposure_max, exposure_def, hblank;
> +
> +	mutex_lock(&imx219->mutex);
> +
> +	/* Bayer order varies with flips */
> +	fmt->format.code = imx219_get_format_code(imx219);
> +
> +	mode = v4l2_find_nearest_size(supported_modes,
> +				      ARRAY_SIZE(supported_modes),
> +				      width, height,
> +				      fmt->format.width, fmt->format.height);
> +	imx219_update_pad_format(imx219, mode, fmt);
> +	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> +		framefmt = v4l2_subdev_get_try_format(sd, cfg, fmt->pad);
> +		*framefmt = fmt->format;
> +	} else if (imx219->mode != mode) {
> +		imx219->mode = mode;
> +		/* Update limits and set FPS to default */
> +		__v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN,
> +					 IMX219_VTS_MAX - mode->height, 1,
> +					 mode->vts_def - mode->height);
> +		__v4l2_ctrl_s_ctrl(imx219->vblank,
> +				   mode->vts_def - mode->height);
> +		/* Update max exposure while meeting expected vblanking */
> +		exposure_max = mode->vts_def - 4;
> +		exposure_def = (exposure_max < IMX219_EXPOSURE_DEFAULT) ?
> +			exposure_max : IMX219_EXPOSURE_DEFAULT;
> +		__v4l2_ctrl_modify_range(imx219->exposure,
> +					 imx219->exposure->minimum,
> +					 exposure_max, imx219->exposure->step,
> +					 exposure_def);
> +		/*
> +		 * Currently PPL is fixed to IMX219_PPL_DEFAULT, so hblank
> +		 * depends on mode->width only, and is not changeble in any
> +		 * way other than changing the mode.
> +		 */
> +		hblank = IMX219_PPL_DEFAULT - mode->width;
> +		__v4l2_ctrl_modify_range(imx219->hblank, hblank, hblank, 1,
> +					 hblank);
> +	}
> +
> +	mutex_unlock(&imx219->mutex);
> +
> +	return 0;
> +}
> +
> +static int imx219_start_streaming(struct imx219 *imx219)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
> +	const struct imx219_reg_list *reg_list;
> +	int ret;
> +
> +	/* Apply default values of current mode */
> +	reg_list = &imx219->mode->reg_list;
> +	ret = imx219_write_regs(imx219, reg_list->regs, reg_list->num_of_regs);
> +	if (ret) {
> +		dev_err(&client->dev, "%s failed to set mode\n", __func__);
> +		return ret;
> +	}
> +
> +	/* Apply customized values from user */
> +	ret =  __v4l2_ctrl_handler_setup(imx219->sd.ctrl_handler);
> +	if (ret)
> +		return ret;
> +
> +	/* set stream on register */
> +	return imx219_write_reg(imx219, IMX219_REG_MODE_SELECT,
> +				IMX219_REG_VALUE_08BIT, IMX219_MODE_STREAMING);
> +}
> +
> +static void imx219_stop_streaming(struct imx219 *imx219)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
> +	int ret;
> +
> +	/* set stream off register */
> +	ret = imx219_write_reg(imx219, IMX219_REG_MODE_SELECT,
> +			       IMX219_REG_VALUE_08BIT, IMX219_MODE_STANDBY);
> +	if (ret)
> +		dev_err(&client->dev, "%s failed to set stream\n", __func__);
> +}
> +
> +static int imx219_set_stream(struct v4l2_subdev *sd, int enable)
> +{
> +	struct imx219 *imx219 = to_imx219(sd);
> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
> +	int ret = 0;
> +
> +	mutex_lock(&imx219->mutex);
> +	if (imx219->streaming == enable) {
> +		mutex_unlock(&imx219->mutex);
> +		return 0;
> +	}
> +
> +	if (enable) {
> +		ret = pm_runtime_get_sync(&client->dev);
> +		if (ret < 0) {
> +			pm_runtime_put_noidle(&client->dev);
> +			goto err_unlock;
> +		}
> +
> +		/*
> +		 * Apply default & customized values
> +		 * and then start streaming.
> +		 */
> +		ret = imx219_start_streaming(imx219);
> +		if (ret)
> +			goto err_rpm_put;
> +	} else {
> +		imx219_stop_streaming(imx219);
> +		pm_runtime_put(&client->dev);
> +	}
> +
> +	imx219->streaming = enable;
> +
> +	/* vflip and hflip cannot change during streaming */
> +	__v4l2_ctrl_grab(imx219->vflip, enable);
> +	__v4l2_ctrl_grab(imx219->hflip, enable);
> +
> +	mutex_unlock(&imx219->mutex);
> +
> +	return ret;
> +
> +err_rpm_put:
> +	pm_runtime_put(&client->dev);
> +err_unlock:
> +	mutex_unlock(&imx219->mutex);
> +
> +	return ret;
> +}
> +
> +/* Power/clock management functions */
> +static int imx219_power_on(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct imx219 *imx219 = to_imx219(sd);
> +	int ret;
> +
> +	ret = regulator_bulk_enable(IMX219_NUM_SUPPLIES,
> +				    imx219->supplies);
> +	if (ret) {
> +		dev_err(&client->dev, "%s: failed to enable regulators\n",
> +			__func__);
> +		return ret;
> +	}
> +
> +	ret = clk_prepare_enable(imx219->xclk);
> +	if (ret) {
> +		dev_err(&client->dev, "%s: failed to enable clock\n",
> +			__func__);
> +		goto reg_off;
> +	}
> +
> +	gpiod_set_value_cansleep(imx219->xclr_gpio, 1);
> +	msleep(IMX219_XCLR_DELAY_MS);

I guess 10 ms is ok albeit it'd be nicer if you calculated the required
delay instead.

> +
> +	return 0;

Empty line here, please.

> +reg_off:
> +	regulator_bulk_disable(IMX219_NUM_SUPPLIES, imx219->supplies);

And here.

> +	return ret;
> +}
> +
> +static int imx219_power_off(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct imx219 *imx219 = to_imx219(sd);
> +
> +	gpiod_set_value_cansleep(imx219->xclr_gpio, 0);
> +	regulator_bulk_disable(IMX219_NUM_SUPPLIES, imx219->supplies);
> +	clk_disable_unprepare(imx219->xclk);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused imx219_suspend(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct imx219 *imx219 = to_imx219(sd);
> +
> +	if (imx219->streaming)
> +		imx219_stop_streaming(imx219);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused imx219_resume(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct imx219 *imx219 = to_imx219(sd);
> +	int ret;
> +
> +	if (imx219->streaming) {
> +		ret = imx219_start_streaming(imx219);
> +		if (ret)
> +			goto error;
> +	}
> +
> +	return 0;
> +
> +error:
> +	imx219_stop_streaming(imx219);
> +	imx219->streaming = 0;

Please add an empty line here.

> +	return ret;
> +}
> +
> +static int imx219_get_regulators(struct imx219 *imx219)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
> +	int i;

unsigned int

> +
> +	for (i = 0; i < IMX219_NUM_SUPPLIES; i++)
> +		imx219->supplies[i].supply = imx219_supply_name[i];
> +
> +	return devm_regulator_bulk_get(&client->dev,
> +				       IMX219_NUM_SUPPLIES,
> +				       imx219->supplies);
> +}
> +
> +/* Verify chip ID */
> +static int imx219_identify_module(struct imx219 *imx219)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
> +	int ret;
> +	u32 val;
> +
> +	ret = imx219_read_reg(imx219, IMX219_REG_CHIP_ID,
> +			      IMX219_REG_VALUE_16BIT, &val);
> +	if (ret) {
> +		dev_err(&client->dev, "failed to read chip id %x\n",
> +			IMX219_CHIP_ID);
> +		return ret;
> +	}
> +
> +	if (val != IMX219_CHIP_ID) {
> +		dev_err(&client->dev, "chip id mismatch: %x!=%x\n",
> +			IMX219_CHIP_ID, val);
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct v4l2_subdev_core_ops imx219_core_ops = {
> +	.subscribe_event = v4l2_ctrl_subdev_subscribe_event,
> +	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
> +};
> +
> +static const struct v4l2_subdev_video_ops imx219_video_ops = {
> +	.s_stream = imx219_set_stream,
> +};
> +
> +static const struct v4l2_subdev_pad_ops imx219_pad_ops = {
> +	.enum_mbus_code = imx219_enum_mbus_code,
> +	.get_fmt = imx219_get_pad_format,
> +	.set_fmt = imx219_set_pad_format,
> +	.enum_frame_size = imx219_enum_frame_size,
> +};
> +
> +static const struct v4l2_subdev_ops imx219_subdev_ops = {
> +	.core = &imx219_core_ops,
> +	.video = &imx219_video_ops,
> +	.pad = &imx219_pad_ops,
> +};
> +
> +static const struct v4l2_subdev_internal_ops imx219_internal_ops = {
> +	.open = imx219_open,
> +};
> +
> +/* Initialize control handlers */
> +static int imx219_init_controls(struct imx219 *imx219)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
> +	struct v4l2_ctrl_handler *ctrl_hdlr;
> +	unsigned int height = imx219->mode->height;
> +	int exposure_max, exposure_def, hblank;
> +	int i, ret;
> +
> +	ctrl_hdlr = &imx219->ctrl_handler;
> +	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 9);
> +	if (ret)
> +		return ret;
> +
> +	mutex_init(&imx219->mutex);
> +	ctrl_hdlr->lock = &imx219->mutex;
> +
> +	/* By default, PIXEL_RATE is read only */
> +	imx219->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops,
> +					       V4L2_CID_PIXEL_RATE,
> +					       IMX219_PIXEL_RATE,
> +					       IMX219_PIXEL_RATE, 1,
> +					       IMX219_PIXEL_RATE);
> +
> +	/* Initial vblank/hblank/exposure parameters based on current mode */
> +	imx219->vblank = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops,
> +					   V4L2_CID_VBLANK, IMX219_VBLANK_MIN,
> +					   IMX219_VTS_MAX - height, 1,
> +					   imx219->mode->vts_def - height);
> +	hblank = IMX219_PPL_DEFAULT - imx219->mode->width;
> +	imx219->hblank = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops,
> +					   V4L2_CID_HBLANK, hblank, hblank,
> +					   1, hblank);
> +	if (imx219->hblank)
> +		imx219->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +	exposure_max = imx219->mode->vts_def - 4;
> +	exposure_def = (exposure_max < IMX219_EXPOSURE_DEFAULT) ?
> +		exposure_max : IMX219_EXPOSURE_DEFAULT;
> +	imx219->exposure = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops,
> +					     V4L2_CID_EXPOSURE,
> +					     IMX219_EXPOSURE_MIN, exposure_max,
> +					     IMX219_EXPOSURE_STEP,
> +					     exposure_def);
> +
> +	v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops, V4L2_CID_ANALOGUE_GAIN,
> +			  IMX219_ANA_GAIN_MIN, IMX219_ANA_GAIN_MAX,
> +			  IMX219_ANA_GAIN_STEP, IMX219_ANA_GAIN_DEFAULT);
> +
> +	v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops, V4L2_CID_DIGITAL_GAIN,
> +			  IMX219_DGTL_GAIN_MIN, IMX219_DGTL_GAIN_MAX,
> +			  IMX219_DGTL_GAIN_STEP, IMX219_DGTL_GAIN_DEFAULT);
> +
> +	imx219->hflip = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops,
> +					  V4L2_CID_HFLIP, 0, 1, 1, 0);
> +	if (imx219->hflip)
> +		imx219->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> +
> +	imx219->vflip = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops,
> +					  V4L2_CID_VFLIP, 0, 1, 1, 0);
> +	if (imx219->vflip)
> +		imx219->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> +
> +	v4l2_ctrl_new_std_menu_items(ctrl_hdlr, &imx219_ctrl_ops,
> +				     V4L2_CID_TEST_PATTERN,
> +				     ARRAY_SIZE(imx219_test_pattern_menu) - 1,
> +				     0, 0, imx219_test_pattern_menu);
> +	for (i = 0; i < 4; i++) {
> +		/*
> +		 * The assumption is that
> +		 * V4L2_CID_TEST_PATTERN_GREENR == V4L2_CID_TEST_PATTERN_RED + 1
> +		 * V4L2_CID_TEST_PATTERN_BLUE   == V4L2_CID_TEST_PATTERN_RED + 2
> +		 * V4L2_CID_TEST_PATTERN_GREENB == V4L2_CID_TEST_PATTERN_RED + 3
> +		 */
> +		v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops,
> +				  V4L2_CID_TEST_PATTERN_RED + i,
> +				  IMX219_TESTP_COLOUR_MIN,
> +				  IMX219_TESTP_COLOUR_MAX,
> +				  IMX219_TESTP_COLOUR_STEP,
> +				  IMX219_TESTP_COLOUR_MAX);
> +		/* The "Solid color" pattern is white by default */
> +	}
> +
> +	if (ctrl_hdlr->error) {
> +		ret = ctrl_hdlr->error;
> +		dev_err(&client->dev, "%s control init failed (%d)\n",
> +			__func__, ret);
> +		goto error;
> +	}
> +
> +	imx219->sd.ctrl_handler = ctrl_hdlr;
> +
> +	return 0;
> +
> +error:
> +	v4l2_ctrl_handler_free(ctrl_hdlr);
> +	mutex_destroy(&imx219->mutex);
> +
> +	return ret;
> +}
> +
> +static void imx219_free_controls(struct imx219 *imx219)
> +{
> +	v4l2_ctrl_handler_free(imx219->sd.ctrl_handler);
> +	mutex_destroy(&imx219->mutex);
> +}
> +
> +static int imx219_probe(struct i2c_client *client,
> +			const struct i2c_device_id *id)
> +{
> +	struct device *dev = &client->dev;
> +	struct fwnode_handle *endpoint;
> +	struct imx219 *imx219;
> +	int ret;
> +
> +	imx219 = devm_kzalloc(&client->dev, sizeof(*imx219), GFP_KERNEL);
> +	if (!imx219)
> +		return -ENOMEM;
> +
> +	imx219->dev = dev;

Instead of putting a dev field to struct imx219, you can find the device in
struct i2c_client.dev, which you can get by:

	v4l2_get_subdevdata(imx219->sd)

> +
> +	v4l2_i2c_subdev_init(&imx219->sd, client, &imx219_subdev_ops);
> +
> +	/* Get CSI2 bus config */
> +	endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(&client->dev),
> +						  NULL);
> +	if (!endpoint) {
> +		dev_err(dev, "endpoint node not found\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = v4l2_fwnode_endpoint_parse(endpoint, &imx219->ep);
> +	fwnode_handle_put(endpoint);
> +	if (ret) {
> +		dev_err(dev, "Could not parse endpoint\n");
> +		return ret;
> +	}
> +
> +	/* Get system clock (xclk) */
> +	imx219->xclk = devm_clk_get(dev, "xclk");
> +	if (IS_ERR(imx219->xclk)) {
> +		dev_err(dev, "failed to get xclk\n");
> +		return PTR_ERR(imx219->xclk);
> +	}
> +
> +	imx219->xclk_freq = clk_get_rate(imx219->xclk);
> +	if (imx219->xclk_freq != IMX219_XCLK_FREQ) {
> +		dev_err(dev, "xclk frequency not supported: %d Hz\n",
> +			imx219->xclk_freq);
> +		return -EINVAL;
> +	}

Could you also check the link frequencies (the link-frequencies property
that also should be added to DT bindings) matches with what is possible
with the given xclk frequency? Please see e.g. imx319 driver for an
example.

> +
> +	ret = imx219_get_regulators(imx219);
> +	if (ret)
> +		return ret;
> +
> +	/* Request optional enable pin */
> +	imx219->xclr_gpio = devm_gpiod_get_optional(dev, "xclr",
> +						    GPIOD_OUT_HIGH);
> +
> +	/*
> +	 * The sensor must be powered for imx219_identify_module()
> +	 * to be able to read the CHIP_ID register
> +	 */
> +	ret = imx219_power_on(dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = imx219_identify_module(imx219);
> +	if (ret)
> +		goto error_power_off;
> +
> +	/* Set default mode to max resolution */
> +	imx219->mode = &supported_modes[0];
> +
> +	ret = imx219_init_controls(imx219);
> +	if (ret)
> +		goto error_power_off;
> +
> +	/* Initialize subdev */
> +	imx219->sd.internal_ops = &imx219_internal_ops;
> +	imx219->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> +	imx219->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> +
> +	/* Initialize source pad */
> +	imx219->pad.flags = MEDIA_PAD_FL_SOURCE;
> +
> +	ret = media_entity_pads_init(&imx219->sd.entity, 1, &imx219->pad);
> +	if (ret)
> +		goto error_handler_free;
> +
> +	ret = v4l2_async_register_subdev_sensor_common(&imx219->sd);
> +	if (ret < 0)
> +		goto error_media_entity;
> +
> +	/* Enable runtime PM and turn off the device */
> +	pm_runtime_set_active(dev);
> +	pm_runtime_enable(dev);
> +	pm_runtime_idle(dev);
> +
> +	return 0;
> +
> +error_media_entity:
> +	media_entity_cleanup(&imx219->sd.entity);
> +
> +error_handler_free:
> +	imx219_free_controls(imx219);
> +
> +error_power_off:
> +	imx219_power_off(dev);
> +
> +	return ret;
> +}
> +
> +static int imx219_remove(struct i2c_client *client)
> +{
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct imx219 *imx219 = to_imx219(sd);
> +
> +	v4l2_async_unregister_subdev(sd);
> +	media_entity_cleanup(&sd->entity);
> +	imx219_free_controls(imx219);
> +
> +	pm_runtime_disable(&client->dev);
> +	pm_runtime_set_suspended(&client->dev);

imx219_power_off(), if the sensor is powered on here. Please see e.g. the
smiapp driver regarding this.

> +
> +	return 0;
> +}
> +
> +static const struct of_device_id imx219_dt_ids[] = {
> +	{ .compatible = "sony,imx219" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, imx219_dt_ids);
> +
> +static const struct dev_pm_ops imx219_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(imx219_suspend, imx219_resume)
> +	SET_RUNTIME_PM_OPS(imx219_power_off, imx219_power_on, NULL)
> +};
> +
> +static struct i2c_driver imx219_i2c_driver = {
> +	.driver = {
> +		.name = "imx219",
> +		.of_match_table	= imx219_dt_ids,
> +		.pm = &imx219_pm_ops,
> +	},
> +	.probe = imx219_probe,

Could you use .probe_new, and remove the i2c_device_id argument?

> +	.remove = imx219_remove,
> +};
> +
> +module_i2c_driver(imx219_i2c_driver);
> +
> +MODULE_AUTHOR("Dave Stevenson <dave.stevenson@raspberrypi.com");
> +MODULE_DESCRIPTION("Sony IMX219 sensor driver");
> +MODULE_LICENSE("GPL v2");

-- 
Regards,

Sakari Ailus

Re: [PATCH v2 2/2] media: i2c: Add driver for Sony IMX219 sensor

[Andrey Konovalov]

Hi Sakari,

Sorry for delayed reply..
(your email got into a wrong folder, and I might not find it there if Ezequiel
did not warn me that I didn't address the comments from your review)

On 27.12.2019 17:55, Sakari Ailus wrote:
> Hi Andrey,
> 
> On Fri, Dec 27, 2019 at 03:21:14PM +0300, Andrey Konovalov wrote:
>> From: Dave Stevenson <dave.stevenson@raspberrypi.com>
>>
>> Adds a driver for the 8MPix Sony IMX219 CSI2 sensor.
>> Whilst the sensor supports 2 or 4 CSI2 data lanes, this driver
>> currently only supports 2 lanes.
>> 8MPix @ 15fps, 1080P @ 30fps (cropped FOV), and 1640x1232 (2x2 binned)
>> @ 30fps are currently supported.
>>
>> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
>> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
>> ---
>>   drivers/media/i2c/Kconfig  |   12 +
>>   drivers/media/i2c/Makefile |    1 +
>>   drivers/media/i2c/imx219.c | 1240 ++++++++++++++++++++++++++++++++++++
>>   3 files changed, 1253 insertions(+)
>>   create mode 100644 drivers/media/i2c/imx219.c

<snip>

>> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
>> new file mode 100644
>> index 000000000000..28b55c61cd77
>> --- /dev/null
>> +++ b/drivers/media/i2c/imx219.c

<snip>

>> +/* Power/clock management functions */
>> +static int imx219_power_on(struct device *dev)
>> +{
>> +	struct i2c_client *client = to_i2c_client(dev);
>> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
>> +	struct imx219 *imx219 = to_imx219(sd);
>> +	int ret;
>> +
>> +	ret = regulator_bulk_enable(IMX219_NUM_SUPPLIES,
>> +				    imx219->supplies);
>> +	if (ret) {
>> +		dev_err(&client->dev, "%s: failed to enable regulators\n",
>> +			__func__);
>> +		return ret;
>> +	}
>> +
>> +	ret = clk_prepare_enable(imx219->xclk);
>> +	if (ret) {
>> +		dev_err(&client->dev, "%s: failed to enable clock\n",
>> +			__func__);
>> +		goto reg_off;
>> +	}
>> +
>> +	gpiod_set_value_cansleep(imx219->xclr_gpio, 1);
>> +	msleep(IMX219_XCLR_DELAY_MS);
> 
> I guess 10 ms is ok albeit it'd be nicer if you calculated the required
> delay instead.

I think this 10 ms delay actually serves two purposes here. It is
not only the delay after XCLR pin is set high (reset de-asserted),
but it also lets the camera power supplies voltages to settle after
regulator_bulk_enable() called few lines above.

So I would make the delay a sum of 1) the value which depends on
input clock frequency (the driver currently supports 24MHz only)
and 2) a fixed value of 8 ms or so to account for the power supplies
settle time. So that the sum would be the same 10 ms for 24MHz input
clock.
Does it makes sense?

<snip>

>> +static int imx219_probe(struct i2c_client *client,
>> +			const struct i2c_device_id *id)
>> +{
>> +	struct device *dev = &client->dev;
>> +	struct fwnode_handle *endpoint;
>> +	struct imx219 *imx219;
>> +	int ret;
>> +
>> +	imx219 = devm_kzalloc(&client->dev, sizeof(*imx219), GFP_KERNEL);
>> +	if (!imx219)
>> +		return -ENOMEM;
>> +
>> +	imx219->dev = dev;
> 
> Instead of putting a dev field to struct imx219, you can find the device in
> struct i2c_client.dev, which you can get by:
> 
> 	v4l2_get_subdevdata(imx219->sd)
> 
>> +
>> +	v4l2_i2c_subdev_init(&imx219->sd, client, &imx219_subdev_ops);
>> +
>> +	/* Get CSI2 bus config */
>> +	endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(&client->dev),
>> +						  NULL);
>> +	if (!endpoint) {
>> +		dev_err(dev, "endpoint node not found\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	ret = v4l2_fwnode_endpoint_parse(endpoint, &imx219->ep);
>> +	fwnode_handle_put(endpoint);
>> +	if (ret) {
>> +		dev_err(dev, "Could not parse endpoint\n");
>> +		return ret;
>> +	}
>> +
>> +	/* Get system clock (xclk) */
>> +	imx219->xclk = devm_clk_get(dev, "xclk");
>> +	if (IS_ERR(imx219->xclk)) {
>> +		dev_err(dev, "failed to get xclk\n");
>> +		return PTR_ERR(imx219->xclk);
>> +	}
>> +
>> +	imx219->xclk_freq = clk_get_rate(imx219->xclk);
>> +	if (imx219->xclk_freq != IMX219_XCLK_FREQ) {
>> +		dev_err(dev, "xclk frequency not supported: %d Hz\n",
>> +			imx219->xclk_freq);
>> +		return -EINVAL;
>> +	}
> 
> Could you also check the link frequencies (the link-frequencies property
> that also should be added to DT bindings) matches with what is possible
> with the given xclk frequency? Please see e.g. imx319 driver for an
> example.

The driver only supports single xclk frequency and single link-frequency
(hardcoded in the registers value tables). So the check will be more like
the one in imx290 driver (error out if the link-frequency property isn't
equal to the pre#define-d default value).

>> +
>> +	ret = imx219_get_regulators(imx219);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Request optional enable pin */
>> +	imx219->xclr_gpio = devm_gpiod_get_optional(dev, "xclr",
>> +						    GPIOD_OUT_HIGH);
>> +
>> +	/*
>> +	 * The sensor must be powered for imx219_identify_module()
>> +	 * to be able to read the CHIP_ID register
>> +	 */
>> +	ret = imx219_power_on(dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = imx219_identify_module(imx219);
>> +	if (ret)
>> +		goto error_power_off;
>> +
>> +	/* Set default mode to max resolution */
>> +	imx219->mode = &supported_modes[0];
>> +
>> +	ret = imx219_init_controls(imx219);
>> +	if (ret)
>> +		goto error_power_off;
>> +
>> +	/* Initialize subdev */
>> +	imx219->sd.internal_ops = &imx219_internal_ops;
>> +	imx219->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>> +	imx219->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
>> +
>> +	/* Initialize source pad */
>> +	imx219->pad.flags = MEDIA_PAD_FL_SOURCE;
>> +
>> +	ret = media_entity_pads_init(&imx219->sd.entity, 1, &imx219->pad);
>> +	if (ret)
>> +		goto error_handler_free;
>> +
>> +	ret = v4l2_async_register_subdev_sensor_common(&imx219->sd);
>> +	if (ret < 0)
>> +		goto error_media_entity;
>> +
>> +	/* Enable runtime PM and turn off the device */
>> +	pm_runtime_set_active(dev);
>> +	pm_runtime_enable(dev);
>> +	pm_runtime_idle(dev);
>> +
>> +	return 0;
>> +
>> +error_media_entity:
>> +	media_entity_cleanup(&imx219->sd.entity);
>> +
>> +error_handler_free:
>> +	imx219_free_controls(imx219);
>> +
>> +error_power_off:
>> +	imx219_power_off(dev);
>> +
>> +	return ret;
>> +}
>> +
>> +static int imx219_remove(struct i2c_client *client)
>> +{
>> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
>> +	struct imx219 *imx219 = to_imx219(sd);
>> +
>> +	v4l2_async_unregister_subdev(sd);
>> +	media_entity_cleanup(&sd->entity);
>> +	imx219_free_controls(imx219);
>> +
>> +	pm_runtime_disable(&client->dev);
>> +	pm_runtime_set_suspended(&client->dev);
> 
> imx219_power_off(), if the sensor is powered on here. Please see e.g. the
> smiapp driver regarding this.

It should be powered off here.

The sensor is powered on when streaming is started, and is powered off when
it is stopped: if the enable argument is false, imx219_set_stream() calls
pm_runtime_put().
IOW, it follows the imx319 driver as the example of power management.

>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id imx219_dt_ids[] = {
>> +	{ .compatible = "sony,imx219" },
>> +	{ /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, imx219_dt_ids);
>> +
>> +static const struct dev_pm_ops imx219_pm_ops = {
>> +	SET_SYSTEM_SLEEP_PM_OPS(imx219_suspend, imx219_resume)
>> +	SET_RUNTIME_PM_OPS(imx219_power_off, imx219_power_on, NULL)
>> +};
>> +
>> +static struct i2c_driver imx219_i2c_driver = {
>> +	.driver = {
>> +		.name = "imx219",
>> +		.of_match_table	= imx219_dt_ids,
>> +		.pm = &imx219_pm_ops,
>> +	},
>> +	.probe = imx219_probe,
> 
> Could you use .probe_new, and remove the i2c_device_id argument?

I'll fix this and all the other issues I didn't comment on in this email
in the v4 of the patch set.

Thanks,
Andrey

>> +	.remove = imx219_remove,
>> +};
>> +
>> +module_i2c_driver(imx219_i2c_driver);
>> +
>> +MODULE_AUTHOR("Dave Stevenson <dave.stevenson@raspberrypi.com");
>> +MODULE_DESCRIPTION("Sony IMX219 sensor driver");
>> +MODULE_LICENSE("GPL v2");
> 

Re: [PATCH v2 2/2] media: i2c: Add driver for Sony IMX219 sensor

[Dave Stevenson]

Hi Andrey & Sakari

On Mon, 13 Jan 2020 at 19:16, Andrey Konovalov
<andrey.konovalov@linaro.org> wrote:
>
> Hi Sakari,
>
> Sorry for delayed reply..
> (your email got into a wrong folder, and I might not find it there if Ezequiel
> did not warn me that I didn't address the comments from your review)

For some reason I missed getting Sakari's email too - apologies.

> On 27.12.2019 17:55, Sakari Ailus wrote:
> > Hi Andrey,
> >
> > On Fri, Dec 27, 2019 at 03:21:14PM +0300, Andrey Konovalov wrote:
> >> From: Dave Stevenson <dave.stevenson@raspberrypi.com>
> >>
> >> Adds a driver for the 8MPix Sony IMX219 CSI2 sensor.
> >> Whilst the sensor supports 2 or 4 CSI2 data lanes, this driver
> >> currently only supports 2 lanes.
> >> 8MPix @ 15fps, 1080P @ 30fps (cropped FOV), and 1640x1232 (2x2 binned)
> >> @ 30fps are currently supported.
> >>
> >> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> >> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
> >> ---
> >>   drivers/media/i2c/Kconfig  |   12 +
> >>   drivers/media/i2c/Makefile |    1 +
> >>   drivers/media/i2c/imx219.c | 1240 ++++++++++++++++++++++++++++++++++++
> >>   3 files changed, 1253 insertions(+)
> >>   create mode 100644 drivers/media/i2c/imx219.c
>
> <snip>
>
> >> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> >> new file mode 100644
> >> index 000000000000..28b55c61cd77
> >> --- /dev/null
> >> +++ b/drivers/media/i2c/imx219.c
>
> <snip>
>
> >> +/* Power/clock management functions */
> >> +static int imx219_power_on(struct device *dev)
> >> +{
> >> +    struct i2c_client *client = to_i2c_client(dev);
> >> +    struct v4l2_subdev *sd = i2c_get_clientdata(client);
> >> +    struct imx219 *imx219 = to_imx219(sd);
> >> +    int ret;
> >> +
> >> +    ret = regulator_bulk_enable(IMX219_NUM_SUPPLIES,
> >> +                                imx219->supplies);
> >> +    if (ret) {
> >> +            dev_err(&client->dev, "%s: failed to enable regulators\n",
> >> +                    __func__);
> >> +            return ret;
> >> +    }
> >> +
> >> +    ret = clk_prepare_enable(imx219->xclk);
> >> +    if (ret) {
> >> +            dev_err(&client->dev, "%s: failed to enable clock\n",
> >> +                    __func__);
> >> +            goto reg_off;
> >> +    }
> >> +
> >> +    gpiod_set_value_cansleep(imx219->xclr_gpio, 1);
> >> +    msleep(IMX219_XCLR_DELAY_MS);
> >
> > I guess 10 ms is ok albeit it'd be nicer if you calculated the required
> > delay instead.
>
> I think this 10 ms delay actually serves two purposes here. It is
> not only the delay after XCLR pin is set high (reset de-asserted),
> but it also lets the camera power supplies voltages to settle after
> regulator_bulk_enable() called few lines above.
>
> So I would make the delay a sum of 1) the value which depends on
> input clock frequency (the driver currently supports 24MHz only)
> and 2) a fixed value of 8 ms or so to account for the power supplies
> settle time. So that the sum would be the same 10 ms for 24MHz input
> clock.
> Does it makes sense?

Regulator settling times shouldn't really be included here - that
should be taken care of via the regulator framework (in the case of DT
for regulator-fixed you define the startup-delay-us property).

What level do you end up computing it to?

This delay covers t4, t5 and t6 from figure 38 on page 77 of the datasheet[1]
t4 is max 200usecs.
t5 is 6ms.
t6 is 32000 cycles of INCK. As you say INCK=24MHz is the only
supported clock frequency at present, so 1.3ms. Minimum clock is 6MHz
which will make this 5.3ms.

t6 is in parallel with t5, but it is smaller than t5 even at the
minimum clock frequency.
Yes we can be programming the sensor over I2C after t5 but before t6,
but you'd now want to be computing the number of I2C writes required,
the speed of the I2C bus, and probably a few other parameters to
ensure you don't violate t5. The sensor supports 1MHz CCI if INCK is
>11.4MHz. A quick check says we have around 60 register writes to
initialise the sensor. 38 clocks (4 * (8 data bits + ACK) + start +
end) to write each register, which I make 2.4ms. It is therefore
possible to violate t5.

Is it worth that level of computation, or do you just take t4+t5 at 6.2ms?

I have been a bit naughty up until now in not setting startup-delay-us
on the regulator definition and relying on this delay instead. The
driver ought to do the right thing though and I'll fix my
configuration.

  Dave

[1] https://github.com/rellimmot/Sony-IMX219-Raspberry-Pi-V2-CMOS

> <snip>
>
> >> +static int imx219_probe(struct i2c_client *client,
> >> +                    const struct i2c_device_id *id)
> >> +{
> >> +    struct device *dev = &client->dev;
> >> +    struct fwnode_handle *endpoint;
> >> +    struct imx219 *imx219;
> >> +    int ret;
> >> +
> >> +    imx219 = devm_kzalloc(&client->dev, sizeof(*imx219), GFP_KERNEL);
> >> +    if (!imx219)
> >> +            return -ENOMEM;
> >> +
> >> +    imx219->dev = dev;
> >
> > Instead of putting a dev field to struct imx219, you can find the device in
> > struct i2c_client.dev, which you can get by:
> >
> >       v4l2_get_subdevdata(imx219->sd)
> >
> >> +
> >> +    v4l2_i2c_subdev_init(&imx219->sd, client, &imx219_subdev_ops);
> >> +
> >> +    /* Get CSI2 bus config */
> >> +    endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(&client->dev),
> >> +                                              NULL);
> >> +    if (!endpoint) {
> >> +            dev_err(dev, "endpoint node not found\n");
> >> +            return -EINVAL;
> >> +    }
> >> +
> >> +    ret = v4l2_fwnode_endpoint_parse(endpoint, &imx219->ep);
> >> +    fwnode_handle_put(endpoint);
> >> +    if (ret) {
> >> +            dev_err(dev, "Could not parse endpoint\n");
> >> +            return ret;
> >> +    }
> >> +
> >> +    /* Get system clock (xclk) */
> >> +    imx219->xclk = devm_clk_get(dev, "xclk");
> >> +    if (IS_ERR(imx219->xclk)) {
> >> +            dev_err(dev, "failed to get xclk\n");
> >> +            return PTR_ERR(imx219->xclk);
> >> +    }
> >> +
> >> +    imx219->xclk_freq = clk_get_rate(imx219->xclk);
> >> +    if (imx219->xclk_freq != IMX219_XCLK_FREQ) {
> >> +            dev_err(dev, "xclk frequency not supported: %d Hz\n",
> >> +                    imx219->xclk_freq);
> >> +            return -EINVAL;
> >> +    }
> >
> > Could you also check the link frequencies (the link-frequencies property
> > that also should be added to DT bindings) matches with what is possible
> > with the given xclk frequency? Please see e.g. imx319 driver for an
> > example.
>
> The driver only supports single xclk frequency and single link-frequency
> (hardcoded in the registers value tables). So the check will be more like
> the one in imx290 driver (error out if the link-frequency property isn't
> equal to the pre#define-d default value).
>
> >> +
> >> +    ret = imx219_get_regulators(imx219);
> >> +    if (ret)
> >> +            return ret;
> >> +
> >> +    /* Request optional enable pin */
> >> +    imx219->xclr_gpio = devm_gpiod_get_optional(dev, "xclr",
> >> +                                                GPIOD_OUT_HIGH);
> >> +
> >> +    /*
> >> +     * The sensor must be powered for imx219_identify_module()
> >> +     * to be able to read the CHIP_ID register
> >> +     */
> >> +    ret = imx219_power_on(dev);
> >> +    if (ret)
> >> +            return ret;
> >> +
> >> +    ret = imx219_identify_module(imx219);
> >> +    if (ret)
> >> +            goto error_power_off;
> >> +
> >> +    /* Set default mode to max resolution */
> >> +    imx219->mode = &supported_modes[0];
> >> +
> >> +    ret = imx219_init_controls(imx219);
> >> +    if (ret)
> >> +            goto error_power_off;
> >> +
> >> +    /* Initialize subdev */
> >> +    imx219->sd.internal_ops = &imx219_internal_ops;
> >> +    imx219->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> >> +    imx219->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> >> +
> >> +    /* Initialize source pad */
> >> +    imx219->pad.flags = MEDIA_PAD_FL_SOURCE;
> >> +
> >> +    ret = media_entity_pads_init(&imx219->sd.entity, 1, &imx219->pad);
> >> +    if (ret)
> >> +            goto error_handler_free;
> >> +
> >> +    ret = v4l2_async_register_subdev_sensor_common(&imx219->sd);
> >> +    if (ret < 0)
> >> +            goto error_media_entity;
> >> +
> >> +    /* Enable runtime PM and turn off the device */
> >> +    pm_runtime_set_active(dev);
> >> +    pm_runtime_enable(dev);
> >> +    pm_runtime_idle(dev);
> >> +
> >> +    return 0;
> >> +
> >> +error_media_entity:
> >> +    media_entity_cleanup(&imx219->sd.entity);
> >> +
> >> +error_handler_free:
> >> +    imx219_free_controls(imx219);
> >> +
> >> +error_power_off:
> >> +    imx219_power_off(dev);
> >> +
> >> +    return ret;
> >> +}
> >> +
> >> +static int imx219_remove(struct i2c_client *client)
> >> +{
> >> +    struct v4l2_subdev *sd = i2c_get_clientdata(client);
> >> +    struct imx219 *imx219 = to_imx219(sd);
> >> +
> >> +    v4l2_async_unregister_subdev(sd);
> >> +    media_entity_cleanup(&sd->entity);
> >> +    imx219_free_controls(imx219);
> >> +
> >> +    pm_runtime_disable(&client->dev);
> >> +    pm_runtime_set_suspended(&client->dev);
> >
> > imx219_power_off(), if the sensor is powered on here. Please see e.g. the
> > smiapp driver regarding this.
>
> It should be powered off here.
>
> The sensor is powered on when streaming is started, and is powered off when
> it is stopped: if the enable argument is false, imx219_set_stream() calls
> pm_runtime_put().
> IOW, it follows the imx319 driver as the example of power management.
>
> >> +
> >> +    return 0;
> >> +}
> >> +
> >> +static const struct of_device_id imx219_dt_ids[] = {
> >> +    { .compatible = "sony,imx219" },
> >> +    { /* sentinel */ }
> >> +};
> >> +MODULE_DEVICE_TABLE(of, imx219_dt_ids);
> >> +
> >> +static const struct dev_pm_ops imx219_pm_ops = {
> >> +    SET_SYSTEM_SLEEP_PM_OPS(imx219_suspend, imx219_resume)
> >> +    SET_RUNTIME_PM_OPS(imx219_power_off, imx219_power_on, NULL)
> >> +};
> >> +
> >> +static struct i2c_driver imx219_i2c_driver = {
> >> +    .driver = {
> >> +            .name = "imx219",
> >> +            .of_match_table = imx219_dt_ids,
> >> +            .pm = &imx219_pm_ops,
> >> +    },
> >> +    .probe = imx219_probe,
> >
> > Could you use .probe_new, and remove the i2c_device_id argument?
>
> I'll fix this and all the other issues I didn't comment on in this email
> in the v4 of the patch set.
>
> Thanks,
> Andrey
>
> >> +    .remove = imx219_remove,
> >> +};
> >> +
> >> +module_i2c_driver(imx219_i2c_driver);
> >> +
> >> +MODULE_AUTHOR("Dave Stevenson <dave.stevenson@raspberrypi.com");
> >> +MODULE_DESCRIPTION("Sony IMX219 sensor driver");
> >> +MODULE_LICENSE("GPL v2");
> >

Re: [PATCH v2 2/2] media: i2c: Add driver for Sony IMX219 sensor

[Sakari Ailus]

Hi Dave,

On Tue, Jan 14, 2020 at 11:34:46AM +0000, Dave Stevenson wrote:

...

> > >> +    msleep(IMX219_XCLR_DELAY_MS);
> > >
> > > I guess 10 ms is ok albeit it'd be nicer if you calculated the required
> > > delay instead.
> >
> > I think this 10 ms delay actually serves two purposes here. It is
> > not only the delay after XCLR pin is set high (reset de-asserted),
> > but it also lets the camera power supplies voltages to settle after
> > regulator_bulk_enable() called few lines above.
> >
> > So I would make the delay a sum of 1) the value which depends on
> > input clock frequency (the driver currently supports 24MHz only)
> > and 2) a fixed value of 8 ms or so to account for the power supplies
> > settle time. So that the sum would be the same 10 ms for 24MHz input
> > clock.
> > Does it makes sense?
> 
> Regulator settling times shouldn't really be included here - that
> should be taken care of via the regulator framework (in the case of DT
> for regulator-fixed you define the startup-delay-us property).
> 
> What level do you end up computing it to?
> 
> This delay covers t4, t5 and t6 from figure 38 on page 77 of the datasheet[1]
> t4 is max 200usecs.
> t5 is 6ms.
> t6 is 32000 cycles of INCK. As you say INCK=24MHz is the only
> supported clock frequency at present, so 1.3ms. Minimum clock is 6MHz
> which will make this 5.3ms.

It's nicer to calculate that still. Someone may well add support for other
frequencies and it's easy to miss changing this.

> 
> t6 is in parallel with t5, but it is smaller than t5 even at the
> minimum clock frequency.
> Yes we can be programming the sensor over I2C after t5 but before t6,
> but you'd now want to be computing the number of I2C writes required,
> the speed of the I2C bus, and probably a few other parameters to
> ensure you don't violate t5. The sensor supports 1MHz CCI if INCK is
> >11.4MHz. A quick check says we have around 60 register writes to
> initialise the sensor. 38 clocks (4 * (8 data bits + ACK) + start +
> end) to write each register, which I make 2.4ms. It is therefore
> possible to violate t5.
> 
> Is it worth that level of computation, or do you just take t4+t5 at 6.2ms?

I'd just ignore the I²C part and wait for long enough. This is not *that*
much time after all. Of course, if someone feels like optimising this, why
not, but it looks like something that doesn't really pay off.

> 
> I have been a bit naughty up until now in not setting startup-delay-us
> on the regulator definition and relying on this delay instead. The
> driver ought to do the right thing though and I'll fix my
> configuration.

Agreed.

-- 
Sakari Ailus

Re: [PATCH v2 2/2] media: i2c: Add driver for Sony IMX219 sensor

[Sakari Ailus]

Hi Andrey,

On Mon, Jan 13, 2020 at 10:16:21PM +0300, Andrey Konovalov wrote:
> Hi Sakari,
> 
> Sorry for delayed reply..
> (your email got into a wrong folder, and I might not find it there if Ezequiel
> did not warn me that I didn't address the comments from your review)

No worries.

> 
> On 27.12.2019 17:55, Sakari Ailus wrote:
> > Hi Andrey,
> > 
> > On Fri, Dec 27, 2019 at 03:21:14PM +0300, Andrey Konovalov wrote:
> > > From: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > > 
> > > Adds a driver for the 8MPix Sony IMX219 CSI2 sensor.
> > > Whilst the sensor supports 2 or 4 CSI2 data lanes, this driver
> > > currently only supports 2 lanes.
> > > 8MPix @ 15fps, 1080P @ 30fps (cropped FOV), and 1640x1232 (2x2 binned)
> > > @ 30fps are currently supported.
> > > 
> > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > > Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
> > > ---
> > >   drivers/media/i2c/Kconfig  |   12 +
> > >   drivers/media/i2c/Makefile |    1 +
> > >   drivers/media/i2c/imx219.c | 1240 ++++++++++++++++++++++++++++++++++++
> > >   3 files changed, 1253 insertions(+)
> > >   create mode 100644 drivers/media/i2c/imx219.c
> 
> <snip>
> 
> > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > > new file mode 100644
> > > index 000000000000..28b55c61cd77
> > > --- /dev/null
> > > +++ b/drivers/media/i2c/imx219.c
> 
> <snip>
> 
> > > +/* Power/clock management functions */
> > > +static int imx219_power_on(struct device *dev)
> > > +{
> > > +	struct i2c_client *client = to_i2c_client(dev);
> > > +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > > +	struct imx219 *imx219 = to_imx219(sd);
> > > +	int ret;
> > > +
> > > +	ret = regulator_bulk_enable(IMX219_NUM_SUPPLIES,
> > > +				    imx219->supplies);
> > > +	if (ret) {
> > > +		dev_err(&client->dev, "%s: failed to enable regulators\n",
> > > +			__func__);
> > > +		return ret;
> > > +	}
> > > +
> > > +	ret = clk_prepare_enable(imx219->xclk);
> > > +	if (ret) {
> > > +		dev_err(&client->dev, "%s: failed to enable clock\n",
> > > +			__func__);
> > > +		goto reg_off;
> > > +	}
> > > +
> > > +	gpiod_set_value_cansleep(imx219->xclr_gpio, 1);
> > > +	msleep(IMX219_XCLR_DELAY_MS);
> > 
> > I guess 10 ms is ok albeit it'd be nicer if you calculated the required
> > delay instead.
> 
> I think this 10 ms delay actually serves two purposes here. It is
> not only the delay after XCLR pin is set high (reset de-asserted),
> but it also lets the camera power supplies voltages to settle after
> regulator_bulk_enable() called few lines above.
> 
> So I would make the delay a sum of 1) the value which depends on
> input clock frequency (the driver currently supports 24MHz only)
> and 2) a fixed value of 8 ms or so to account for the power supplies
> settle time. So that the sum would be the same 10 ms for 24MHz input
> clock.
> Does it makes sense?

It does, agreed.

> 
> <snip>
> 
> > > +static int imx219_probe(struct i2c_client *client,
> > > +			const struct i2c_device_id *id)
> > > +{
> > > +	struct device *dev = &client->dev;
> > > +	struct fwnode_handle *endpoint;
> > > +	struct imx219 *imx219;
> > > +	int ret;
> > > +
> > > +	imx219 = devm_kzalloc(&client->dev, sizeof(*imx219), GFP_KERNEL);
> > > +	if (!imx219)
> > > +		return -ENOMEM;
> > > +
> > > +	imx219->dev = dev;
> > 
> > Instead of putting a dev field to struct imx219, you can find the device in
> > struct i2c_client.dev, which you can get by:
> > 
> > 	v4l2_get_subdevdata(imx219->sd)
> > 
> > > +
> > > +	v4l2_i2c_subdev_init(&imx219->sd, client, &imx219_subdev_ops);
> > > +
> > > +	/* Get CSI2 bus config */
> > > +	endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(&client->dev),
> > > +						  NULL);
> > > +	if (!endpoint) {
> > > +		dev_err(dev, "endpoint node not found\n");
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	ret = v4l2_fwnode_endpoint_parse(endpoint, &imx219->ep);
> > > +	fwnode_handle_put(endpoint);
> > > +	if (ret) {
> > > +		dev_err(dev, "Could not parse endpoint\n");
> > > +		return ret;
> > > +	}
> > > +
> > > +	/* Get system clock (xclk) */
> > > +	imx219->xclk = devm_clk_get(dev, "xclk");
> > > +	if (IS_ERR(imx219->xclk)) {
> > > +		dev_err(dev, "failed to get xclk\n");
> > > +		return PTR_ERR(imx219->xclk);
> > > +	}
> > > +
> > > +	imx219->xclk_freq = clk_get_rate(imx219->xclk);
> > > +	if (imx219->xclk_freq != IMX219_XCLK_FREQ) {
> > > +		dev_err(dev, "xclk frequency not supported: %d Hz\n",
> > > +			imx219->xclk_freq);
> > > +		return -EINVAL;
> > > +	}
> > 
> > Could you also check the link frequencies (the link-frequencies property
> > that also should be added to DT bindings) matches with what is possible
> > with the given xclk frequency? Please see e.g. imx319 driver for an
> > example.
> 
> The driver only supports single xclk frequency and single link-frequency
> (hardcoded in the registers value tables). So the check will be more like
> the one in imx290 driver (error out if the link-frequency property isn't
> equal to the pre#define-d default value).

Right. The frequencies don't seem to be in DT bindings either; they should
be added there. The sensor supports multiple frequencies as in practice
every other sensor does.

The driver does not necessarily need to check for them if it supports a
single one only.

> 
> > > +
> > > +	ret = imx219_get_regulators(imx219);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	/* Request optional enable pin */
> > > +	imx219->xclr_gpio = devm_gpiod_get_optional(dev, "xclr",
> > > +						    GPIOD_OUT_HIGH);
> > > +
> > > +	/*
> > > +	 * The sensor must be powered for imx219_identify_module()
> > > +	 * to be able to read the CHIP_ID register
> > > +	 */
> > > +	ret = imx219_power_on(dev);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = imx219_identify_module(imx219);
> > > +	if (ret)
> > > +		goto error_power_off;
> > > +
> > > +	/* Set default mode to max resolution */
> > > +	imx219->mode = &supported_modes[0];
> > > +
> > > +	ret = imx219_init_controls(imx219);
> > > +	if (ret)
> > > +		goto error_power_off;
> > > +
> > > +	/* Initialize subdev */
> > > +	imx219->sd.internal_ops = &imx219_internal_ops;
> > > +	imx219->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> > > +	imx219->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> > > +
> > > +	/* Initialize source pad */
> > > +	imx219->pad.flags = MEDIA_PAD_FL_SOURCE;
> > > +
> > > +	ret = media_entity_pads_init(&imx219->sd.entity, 1, &imx219->pad);
> > > +	if (ret)
> > > +		goto error_handler_free;
> > > +
> > > +	ret = v4l2_async_register_subdev_sensor_common(&imx219->sd);
> > > +	if (ret < 0)
> > > +		goto error_media_entity;
> > > +
> > > +	/* Enable runtime PM and turn off the device */
> > > +	pm_runtime_set_active(dev);
> > > +	pm_runtime_enable(dev);
> > > +	pm_runtime_idle(dev);
> > > +
> > > +	return 0;
> > > +
> > > +error_media_entity:
> > > +	media_entity_cleanup(&imx219->sd.entity);
> > > +
> > > +error_handler_free:
> > > +	imx219_free_controls(imx219);
> > > +
> > > +error_power_off:
> > > +	imx219_power_off(dev);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static int imx219_remove(struct i2c_client *client)
> > > +{
> > > +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > > +	struct imx219 *imx219 = to_imx219(sd);
> > > +
> > > +	v4l2_async_unregister_subdev(sd);
> > > +	media_entity_cleanup(&sd->entity);
> > > +	imx219_free_controls(imx219);
> > > +
> > > +	pm_runtime_disable(&client->dev);
> > > +	pm_runtime_set_suspended(&client->dev);
> > 
> > imx219_power_off(), if the sensor is powered on here. Please see e.g. the
> > smiapp driver regarding this.
> 
> It should be powered off here.
> 
> The sensor is powered on when streaming is started, and is powered off when
> it is stopped: if the enable argument is false, imx219_set_stream() calls
> pm_runtime_put().
> IOW, it follows the imx319 driver as the example of power management.

Well, yes, or rather maybe.

If you have a non-ACPI system and runtime PM is disabled, then the sensor
is powered on here.

The imx319 driver only works on ACPI based systems so it's not something
you can compare to directly. Check e.g. the smiapp driver which works on
both DT and ACPI systems.

> 
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static const struct of_device_id imx219_dt_ids[] = {
> > > +	{ .compatible = "sony,imx219" },
> > > +	{ /* sentinel */ }
> > > +};
> > > +MODULE_DEVICE_TABLE(of, imx219_dt_ids);
> > > +
> > > +static const struct dev_pm_ops imx219_pm_ops = {
> > > +	SET_SYSTEM_SLEEP_PM_OPS(imx219_suspend, imx219_resume)
> > > +	SET_RUNTIME_PM_OPS(imx219_power_off, imx219_power_on, NULL)
> > > +};
> > > +
> > > +static struct i2c_driver imx219_i2c_driver = {
> > > +	.driver = {
> > > +		.name = "imx219",
> > > +		.of_match_table	= imx219_dt_ids,
> > > +		.pm = &imx219_pm_ops,
> > > +	},
> > > +	.probe = imx219_probe,
> > 
> > Could you use .probe_new, and remove the i2c_device_id argument?
> 
> I'll fix this and all the other issues I didn't comment on in this email
> in the v4 of the patch set.

Thanks!

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v2 2/2] media: i2c: Add driver for Sony IMX219 sensor

[Ezequiel Garcia]

On Fri, 2019-12-27 at 15:21 +0300, Andrey Konovalov wrote:
> From: Dave Stevenson <dave.stevenson@raspberrypi.com>
> 
> Adds a driver for the 8MPix Sony IMX219 CSI2 sensor.
> Whilst the sensor supports 2 or 4 CSI2 data lanes, this driver
> currently only supports 2 lanes.
> 8MPix @ 15fps, 1080P @ 30fps (cropped FOV), and 1640x1232 (2x2 binned)
> @ 30fps are currently supported.
> 
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
> ---
>  drivers/media/i2c/Kconfig  |   12 +
>  drivers/media/i2c/Makefile |    1 +
>  drivers/media/i2c/imx219.c | 1240 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 1253 insertions(+)
>  create mode 100644 drivers/media/i2c/imx219.c
> 
> 
[..]
> +
> +static int imx219_probe(struct i2c_client *client,
> +			const struct i2c_device_id *id)
> +{
> +	struct device *dev = &client->dev;
> +	struct fwnode_handle *endpoint;
> +	struct imx219 *imx219;
> +	int ret;
> +
> +	imx219 = devm_kzalloc(&client->dev, sizeof(*imx219), GFP_KERNEL);
> +	if (!imx219)
> +		return -ENOMEM;
> +
> +	imx219->dev = dev;
> +
> +	v4l2_i2c_subdev_init(&imx219->sd, client, &imx219_subdev_ops);
> +
> +	/* Get CSI2 bus config */
> +	endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(&client->dev),
> +						  NULL);
> +	if (!endpoint) {
> +		dev_err(dev, "endpoint node not found\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = v4l2_fwnode_endpoint_parse(endpoint, &imx219->ep);
> +	fwnode_handle_put(endpoint);
> +	if (ret) {
> +		dev_err(dev, "Could not parse endpoint\n");
> +		return ret;
> +	}
> +
> +	/* Get system clock (xclk) */
> +	imx219->xclk = devm_clk_get(dev, "xclk");
> +	if (IS_ERR(imx219->xclk)) {
> +		dev_err(dev, "failed to get xclk\n");
> +		return PTR_ERR(imx219->xclk);
> +	}
> +
> +	imx219->xclk_freq = clk_get_rate(imx219->xclk);
> +	if (imx219->xclk_freq != IMX219_XCLK_FREQ) {
> +		dev_err(dev, "xclk frequency not supported: %d Hz\n",
> +			imx219->xclk_freq);
> +		return -EINVAL;
> +	}
> +
> +	ret = imx219_get_regulators(imx219);
> +	if (ret)
> +		return ret;
> +

I think printing an error if you can't regulators
(as the core will stay silent), is a good idea.

Just got bitten by this while trying the driver with
a RPI camera module v2, and the RKISP1 patches that
we recently posted on this mailing list.

(did _very_ limited testing, but working nicely so far).

> +	/* Request optional enable pin */
> +	imx219->xclr_gpio = devm_gpiod_get_optional(dev, "xclr",
> +						    GPIOD_OUT_HIGH);
> +
> +	/*
> +	 * The sensor must be powered for imx219_identify_module()
> +	 * to be able to read the CHIP_ID register
> +	 */
> +	ret = imx219_power_on(dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = imx219_identify_module(imx219);
> +	if (ret)
> +		goto error_power_off;
> +
> +	/* Set default mode to max resolution */
> +	imx219->mode = &supported_modes[0];
> +
> +	ret = imx219_init_controls(imx219);
> +	if (ret)
> +		goto error_power_off;
> +
> +	/* Initialize subdev */
> +	imx219->sd.internal_ops = &imx219_internal_ops;
> +	imx219->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> +	imx219->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> +
> +	/* Initialize source pad */
> +	imx219->pad.flags = MEDIA_PAD_FL_SOURCE;
> +
> +	ret = media_entity_pads_init(&imx219->sd.entity, 1, &imx219->pad);
> +	if (ret)
> +		goto error_handler_free;
> +
> +	ret = v4l2_async_register_subdev_sensor_common(&imx219->sd);
> +	if (ret < 0)
> +		goto error_media_entity;
> +

Ditto on these last two, it's possible to get silent
failures here, which are a bit annoying to debug.

Thanks,
Ezequiel