diff mbox series

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

Message ID 20191227122114.23075-2-andrey.konovalov@linaro.org
State Changes Requested, archived
Headers show
Series Add IMX219 CMOS image sensor support | expand

Checks

Context Check Description
robh/checkpatch success
robh/dt-meta-schema fail build log

Commit Message

Andrey Konovalov Dec. 27, 2019, 12:21 p.m. UTC 
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

Comments

Sakari Ailus Dec. 27, 2019, 2:17 p.m. UTC | #1 
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
Andrey Konovalov Dec. 27, 2019, 6:26 p.m. UTC | #2 
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
>
Sakari Ailus Dec. 30, 2019, 9:24 a.m. UTC | #3 
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.
Rob Herring Jan. 4, 2020, 9:53 p.m. UTC | #4 
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
>
Dave Stevenson Jan. 6, 2020, 2:56 p.m. UTC | #5 
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
Andrey Konovalov Jan. 10, 2020, 8:18 p.m. UTC | #6 
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
>>
diff mbox series

Patch

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