LinuxTV Patchwork [v3,05/14] media: dt-bindings: marvell,mmp2-ccic: Add Marvell MMP2 camera

login
register
mail settings
Submitter Lubomir Rintel
Date Nov. 20, 2018, 10:03 a.m.
Message ID <20181120100318.367987-6-lkundrak@v3.sk>
Download mbox | patch
Permalink /patch/53040/
State Changes Requested
Delegated to: Hans Verkuil
Headers show

Comments

Lubomir Rintel - Nov. 20, 2018, 10:03 a.m.
Add Marvell MMP2 camera host interface.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>

---
Changes since v2:
- Added #clock-cells, clock-names, port

 .../bindings/media/marvell,mmp2-ccic.txt      | 37 +++++++++++++++++++
 1 file changed, 37 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/marvell,mmp2-ccic.txt
jacopo@jmondi.org - Nov. 22, 2018, 8:08 p.m.
Hi Lubomir,

On Tue, Nov 20, 2018 at 11:03:10AM +0100, Lubomir Rintel wrote:
> Add Marvell MMP2 camera host interface.
>
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
>
> ---
> Changes since v2:
> - Added #clock-cells, clock-names, port
>
>  .../bindings/media/marvell,mmp2-ccic.txt      | 37 +++++++++++++++++++
>  1 file changed, 37 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/marvell,mmp2-ccic.txt
>
> diff --git a/Documentation/devicetree/bindings/media/marvell,mmp2-ccic.txt b/Documentation/devicetree/bindings/media/marvell,mmp2-ccic.txt
> new file mode 100644
> index 000000000000..e5e8ca90e7f7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/marvell,mmp2-ccic.txt
> @@ -0,0 +1,37 @@
> +Marvell MMP2 camera host interface
> +
> +Required properties:
> + - compatible: Should be "marvell,mmp2-ccic"
> + - reg: register base and size
> + - interrupts: the interrupt number
> + - #clock-cells: must be 0
> + - any required generic properties defined in video-interfaces.txt

I don't think video-interfaces applies here. It described bindings to
be used for endpoint and port nodes.

> +
> +Optional properties:
> + - clocks: input clock (see clock-bindings.txt)

What do you think of:
"reference to the input clock as specified by clock-bindings.txt"

> + - clock-names: names of the clocks used, may include "axi", "func" and
> +                "phy"

"may include" is abit vague. Which clock should the interface be
powered from, and in which case?

> + - clock-output-names: should contain the name of the clock driving the
> +                       sensor master clock MCLK

This is a property for the clock provider part, and I will just list
the only clock this interfaces provides here:

    - clock-output-names: Optional clock source for sensors. Shall be "mclk".

See a comment on patch 14 on the use of the clock provider part.

> +
> +Required subnodes:
> + - port: the parallel bus interface port with a single endpoint linked to
> +         the sensor's endpoint as described in video-interfaces.txt
> +
> +Example:
> +
> +	camera0: camera@d420a000 {
> +		compatible = "marvell,mmp2-ccic";
> +		reg = <0xd420a000 0x800>;
> +		interrupts = <42>;
> +		clocks = <&soc_clocks MMP2_CLK_CCIC0>;
> +		clock-names = "axi";
> +		#clock-cells = <0>;
> +		clock-output-names = "mclk";
> +
> +		port {
> +			camera0_0: endpoint {
> +				remote-endpoint = <&ov7670_0>;

I'm debated, your sensor does not support configuring the parallel bus,
that's fine, but as "bindings describe hardware" shouldn't you list
here the bus configurations the HW interface supports and list their default
values? Or there are none for real in this platform?

Thanks
  j


> +			};
> +		};
> +	};
> --
> 2.19.1
>
Sakari Ailus - Nov. 27, 2018, 10:08 a.m.
Hi Lubomir,

On Tue, Nov 20, 2018 at 11:03:10AM +0100, Lubomir Rintel wrote:
> Add Marvell MMP2 camera host interface.
> 
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> 
> ---
> Changes since v2:
> - Added #clock-cells, clock-names, port
> 
>  .../bindings/media/marvell,mmp2-ccic.txt      | 37 +++++++++++++++++++
>  1 file changed, 37 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/marvell,mmp2-ccic.txt
> 
> diff --git a/Documentation/devicetree/bindings/media/marvell,mmp2-ccic.txt b/Documentation/devicetree/bindings/media/marvell,mmp2-ccic.txt
> new file mode 100644
> index 000000000000..e5e8ca90e7f7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/marvell,mmp2-ccic.txt
> @@ -0,0 +1,37 @@
> +Marvell MMP2 camera host interface
> +
> +Required properties:
> + - compatible: Should be "marvell,mmp2-ccic"
> + - reg: register base and size
> + - interrupts: the interrupt number
> + - #clock-cells: must be 0
> + - any required generic properties defined in video-interfaces.txt

Could you document what is relevant for the hardware? There are quite a few
properties documented there...

> +
> +Optional properties:
> + - clocks: input clock (see clock-bindings.txt)
> + - clock-names: names of the clocks used, may include "axi", "func" and
> +                "phy"
> + - clock-output-names: should contain the name of the clock driving the
> +                       sensor master clock MCLK
> +
> +Required subnodes:
> + - port: the parallel bus interface port with a single endpoint linked to
> +         the sensor's endpoint as described in video-interfaces.txt

Please use the full path to video-interfaces.txt. Same above, as well as
for clock-bindings.txt.

Are there endpoint properties that are applicable for the hardware?

> +
> +Example:
> +
> +	camera0: camera@d420a000 {
> +		compatible = "marvell,mmp2-ccic";
> +		reg = <0xd420a000 0x800>;
> +		interrupts = <42>;
> +		clocks = <&soc_clocks MMP2_CLK_CCIC0>;
> +		clock-names = "axi";
> +		#clock-cells = <0>;
> +		clock-output-names = "mclk";
> +
> +		port {
> +			camera0_0: endpoint {
> +				remote-endpoint = <&ov7670_0>;
> +			};
> +		};
> +	};

Patch

diff --git a/Documentation/devicetree/bindings/media/marvell,mmp2-ccic.txt b/Documentation/devicetree/bindings/media/marvell,mmp2-ccic.txt
new file mode 100644
index 000000000000..e5e8ca90e7f7
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/marvell,mmp2-ccic.txt
@@ -0,0 +1,37 @@ 
+Marvell MMP2 camera host interface
+
+Required properties:
+ - compatible: Should be "marvell,mmp2-ccic"
+ - reg: register base and size
+ - interrupts: the interrupt number
+ - #clock-cells: must be 0
+ - any required generic properties defined in video-interfaces.txt
+
+Optional properties:
+ - clocks: input clock (see clock-bindings.txt)
+ - clock-names: names of the clocks used, may include "axi", "func" and
+                "phy"
+ - clock-output-names: should contain the name of the clock driving the
+                       sensor master clock MCLK
+
+Required subnodes:
+ - port: the parallel bus interface port with a single endpoint linked to
+         the sensor's endpoint as described in video-interfaces.txt
+
+Example:
+
+	camera0: camera@d420a000 {
+		compatible = "marvell,mmp2-ccic";
+		reg = <0xd420a000 0x800>;
+		interrupts = <42>;
+		clocks = <&soc_clocks MMP2_CLK_CCIC0>;
+		clock-names = "axi";
+		#clock-cells = <0>;
+		clock-output-names = "mclk";
+
+		port {
+			camera0_0: endpoint {
+				remote-endpoint = <&ov7670_0>;
+			};
+		};
+	};

Privacy Policy