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

[Lubomir Rintel <lkundrak@v3.sk>]

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