diff mbox series

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

Message ID 20181120100318.367987-6-lkundrak@v3.sk (mailing list archive)
State New, archived
Headers show
Series media: make Marvell camera work on DT-based OLPC XO-1.75 | expand

Commit Message

Lubomir Rintel Nov. 20, 2018, 10:03 a.m. UTC 
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

Comments

Jacopo Mondi Nov. 22, 2018, 8:08 p.m. UTC | #1 
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. UTC | #2 
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>;
> +			};
> +		};
> +	};
Lubomir Rintel April 25, 2019, 2:28 p.m. UTC | #3 
On Thu, 2018-11-22 at 21:08 +0100, jacopo mondi wrote:
> 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.

Yes. Will fix.

> > +
> > +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"

Sounds good; I'll use that.

> > + - 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?

I did this somewhat intentionally, because the hardware documentation
is secret and the only clue about the clocks comes from what is
actually implemented in the driver.

I think I can still improve it somehow though, without too much risk of
stating things incorrectly.

> > + - 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".

Thanks, will fix in next patch version.

> 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?

Hard to tell, given Marvell won't publish the documentation.

The driver seems to have some rudimentary support for bus-type of
<V4L2_MBUS_CSI2_DPHY>, but I have no idea whether it's on MMP2
hardware. The 16x documentation, that may or may not be relevant, only
lists parallel formats.

I'd prefer not to take risks of making wrong guesses and don't list I
can not verify to be correct.

> Thanks
>   j

There are points above I have not responded to. I'll address them in
the next version of the patch.

Thank you
Lubo
diff mbox series

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>;
+			};
+		};
+	};