Re: [RFC 05/11] fixup! dt-bindings: media: i2c: Add bindings for IMI RDACM20

From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	Jacopo Mondi <jacopo+renesas@jmondi.org>
Cc: niklas.soderlund@ragnatech.se, linux-renesas-soc@vger.kernel.org
Subject: Re: [RFC 05/11] fixup! dt-bindings: media: i2c: Add bindings for IMI RDACM20
Date: Fri, 14 Feb 2020 09:07:28 +0000	[thread overview]
Message-ID: <dc03fd9f-a5e0-a317-6452-3cb57c470854@ideasonboard.com> (raw)
In-Reply-To: <20191216224204.GJ4856@pendragon.ideasonboard.com>

Hi Jacopo,

On 16/12/2019 22:42, Laurent Pinchart wrote:
> Hi Jacopo,
> 
> Thank you for the patch.
> 
> On Mon, Dec 16, 2019 at 06:16:14PM +0100, Jacopo Mondi wrote:
>> ---
>>  .../bindings/media/i2c/imi,rdacm20.txt        |  66 ----------
>>  .../bindings/media/i2c/imi,rdacm20.yaml       | 113 ++++++++++++++++++
>>  2 files changed, 113 insertions(+), 66 deletions(-)
>>  delete mode 100644 Documentation/devicetree/bindings/media/i2c/imi,rdacm20.txt
>>  create mode 100644 Documentation/devicetree/bindings/media/i2c/imi,rdacm20.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/media/i2c/imi,rdacm20.txt b/Documentation/devicetree/bindings/media/i2c/imi,rdacm20.txt
>> deleted file mode 100644
>> index 4731aafed63f..000000000000
>> --- a/Documentation/devicetree/bindings/media/i2c/imi,rdacm20.txt
>> +++ /dev/null
>> @@ -1,66 +0,0 @@
>> -IMI D&D RDACM20 Automotive Camera Platform
>> -------------------------------------------
>> -
>> -The IMI D&D RDACM20 is a GMSL-compatible camera designed for automotive
>> -applications. It encloses a Maxim Integrated MAX9271 GMSL serializer, an
>> -Omnivision OV10635 camera sensor and an embedded MCU, and connects to a remote
>> -GMSL endpoint through a coaxial cable.
>> -
>> -                                                     IMI RDACM20
>> - ---------------                               --------------------------------
>> -|      GMSL     |   <---  Video Stream        |       <- Video--------\        |
>> -|               |< ====== GMSL Link ======== >|MAX9271<- I2C bus-> <-->OV10635 |
>> -| de-serializer |   <---  I2C messages --->   |                   \<-->MCU     |
>> - ---------------                               --------------------------------
>> -
>> -The RDACM20 transmits video data generated by the embedded camera sensor on the
>> -GMSL serial channel to a remote GMSL de-serializer, as well as it receives and
>> -transmits I2C messages encapsulated in the GMSL bidirectional control channel.
>> -
>> -All I2C traffic received on the GMSL link not directed to the serializer is
>> -propagated on the local I2C bus to the embedded camera sensor and MCU. All
>> -I2C traffic generated on the local I2C bus not directed to the serializer is
>> -propagated to the remote de-serializer encapsulated in the GMSL control channel.
>> -
>> -The RDACM20 DT node should be a direct child of the GMSL Deserializer's I2C bus
>> -corresponding to the GMSL link that the camera is attached to.
>> -
>> -Required Properties:
>> -
>> -- compatible: Shall be "imi,rdacm20".
>> -- reg: I2C device addresses, the first to be assigned to the serializer
>> -  the second to be assigned to the camera sensor. An optional third address can
>> -  be provided to specify the MCU address if present.
>> -
>> -Connection to the remote GMSL endpoint are modelled using the OF graph bindings
>> -in accordance with the video interface bindings defined in
>> -Documentation/devicetree/bindings/media/video-interfaces.txt.
>> -
>> -The device node contains a single "port" child node with a single "endpoint"
>> -sub-device.
>> -
>> -Required endpoint properties:
>> -
>> -- remote-endpoint: phandle to the remote GMSL endpoint sub-node in the remote
>> -  node port.
>> -
>> -Example:
>> --------
>> -
>> -	i2c@0 {
>> -		#address-cells = <1>;
>> -		#size-cells = <0>;
>> -		reg = <0>;
>> -
>> -		camera@51 {
>> -			compatible = "imi,rdacm20";
>> -			reg = <0x31 0x41 0x51>;
>> -
>> -			port {
>> -				rdacm20_out0: endpoint {
>> -					remote-endpoint = <&max9286_in0>;
>> -				};
>> -			};
>> -
>> -		};
>> -	};
>> diff --git a/Documentation/devicetree/bindings/media/i2c/imi,rdacm20.yaml b/Documentation/devicetree/bindings/media/i2c/imi,rdacm20.yaml
>> new file mode 100644
>> index 000000000000..76740e285f44
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/i2c/imi,rdacm20.yaml
>> @@ -0,0 +1,113 @@
>> +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
>> +# Copyright (C) 2019 Renesas Electronics Corp.
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/media/i2c/imi,rdacm20.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title:  IMI D&D RDACM20 Automotive Camera Platform
>> +
>> +maintainers:
>> +  - Jacopo Mondi <jacopo+renesas@jmondi.org>
>> +  - Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>> +  - Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>> +  - Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>> +
>> +description: -|
>> +  The IMI D&D RDACM20 is a GMSL-compatible camera designed for automotive
>> +  applications. It encloses a Maxim Integrated MAX9271 GMSL serializer, an
>> +  Omnivision OV10635 camera sensor and an embedded MCU, and connects to a remote
>> +  GMSL endpoint through a coaxial cable.
>> +
>> +                                                   IMI RDACM20
>> +  +---------------+                        +--------------------------------+
>> +  |      GMSL     |   <- Video Stream      |       <- Video--------\        |
>> +  |               |< === GMSL Link ====== >|MAX9271<- I2C bus-> <-->OV10635 |
>> +  | de-serializer |   <- I2C messages ->   |                   \<-->MCU     |
>> +  +---------------+                        +--------------------------------+
>> +
>> +  The RDACM20 transmits video data generated by the embedded camera sensor on
>> +  the GMSL serial channel to a remote GMSL de-serializer, as well as it receives
>> +  and transmits I2C messages encapsulated in the GMSL bidirectional control
>> +  channel.
>> +
>> +  All I2C traffic received on the GMSL link not directed to the serializer is
>> +  propagated on the local I2C bus to the embedded camera sensor and MCU. All I2C
>> +  traffic generated on the local I2C bus not directed to the serializer is
>> +  propagated to the remote de-serializer encapsulated in the GMSL control
>> +  channel.
>> +
>> +  The RDACM20 DT node should be a direct child of the GMSL Deserializer's I2C
>> +  bus corresponding to the GMSL link that the camera is attached to.
>> +
>> +properties:
>> +  '#address-cells':
>> +    const: 1
>> +
>> +  '#size-cells':
>> +    const: 0
> 
> Are those two properties needed ?
> 

Hi Jacopo,

This question is left unanswered, and the properties are still in the
code base...

Can/should we drop these two properties?

I can see that the i2c-node will specify the properties, but I don't
think the rdacm20 node will..

i.e.:

>> +    i2c@e66d8000 {
>> +      #address-cells = <1>;
>> +      #size-cells = <0>;

	They are utilised here (in the i2c node)...

>> +
>> +      reg = <0 0xe66d8000 0 0x40>;
>> +
>> +      camera@31 {
>> +        compatible = "imi,rdacm20";
>> +        reg = <0x31>, <0x41>, <0x51>;

but not here in the camera node....

>> +
>> +        port {
>> +          rdacm20_out0: endpoint {
>> +            remote-endpoint = <&max9286_in0>;
>> +          };
>> +        };
>> +      };
>> +    };

--
Kieran

>> +
>> +  compatible:
>> +    const: imi,rdacm20
>> +
>> +  reg:
>> +    description: -|
>> +      I2C device addresses, the first to be assigned to the serializer the
>> +      second to be assigned to the camera sensor. An optional third address can
>> +      be provided to specify the MCU address if present.
>> +    minItems: 2
>> +    maxItems: 3
>> +
>> +  port:
>> +    type: object
>> +    additionalProperties: false
>> +    description: -|
>> +      Connection to the remote GMSL endpoint are modelled using the OF graph
>> +      bindings in accordance with the video interface bindings defined in
>> +      Documentation/devicetree/bindings/media/video-interfaces.txt.
>> +
>> +      The device node contains a single "port" child node with a single
>> +      "endpoint" sub-device.
>> +
>> +    properties:
>> +      endpoint:
>> +        type: object
>> +        additionalProperties: false
>> +
>> +        properties:
>> +          remote-endpoint:
>> +            description: -|
>> +              phandle to the remote GMSL endpoint sub-node in the remote node
>> +              port.
>> +            maxItems: 1
>> +
>> +        required:
>> +          - remote-endpoint
>> +
>> +    required:
>> +      - endpoint
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - port
>> +
>> +examples:
>> +  - |
>> +    i2c@e66d8000 {
>> +      #address-cells = <1>;
>> +      #size-cells = <0>;
>> +
>> +      reg = <0 0xe66d8000 0 0x40>;
>> +
>> +      camera@31 {
>> +        compatible = "imi,rdacm20";
>> +        reg = <0x31>, <0x41>, <0x51>;
>> +
>> +        port {
>> +          rdacm20_out0: endpoint {
>> +            remote-endpoint = <&max9286_in0>;
>> +          };
>> +        };
>> +      };
>> +    };
> 