Hi Rob, Geert, Laurent, Thank you for the review comments. > On Mon, Nov 14, 2016 at 8:52 PM, Rob Herring <robh@xxxxxxxxxx> wrote: > > On Thu, Nov 10, 2016 at 11:22:20AM +0200, Laurent Pinchart wrote: > >> On Wednesday 09 Nov 2016 15:44:44 Ramesh Shanmugasundaram wrote: > >> > --- /dev/null > >> > +++ b/Documentation/devicetree/bindings/media/renesas,drif.txt > >> > @@ -0,0 +1,136 @@ > > >> > +Example > >> > +-------- > >> > + > >> > +SoC common dtsi file > >> > + > >> > + drif00: rif@e6f40000 { > >> > + compatible = "renesas,r8a7795-drif", > >> > + "renesas,rcar-gen3-drif"; > >> > + reg = <0 0xe6f40000 0 0x64>; > >> > + interrupts = <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>; > >> > + clocks = <&cpg CPG_MOD 515>; > >> > + clock-names = "fck"; > >> > + dmas = <&dmac1 0x20>, <&dmac2 0x20>; > >> > + dma-names = "rx", "rx"; > > > > rx, rx? That doesn't make sense. While we don't explicitly disallow > > this, I'm thinking we should. I wonder if there's any case this is > > valid. If not, then a dtc check for this could be added. > > The device can be used with either dmac1 or dmac2. > Which one is used is decided at run time, based on the availability of DMA > channels per DMAC, which is a limited resource. > Yep. > >> > + power-domains = <&sysc R8A7795_PD_ALWAYS_ON>; > >> > + status = "disabled"; > >> > + }; > >> > + > >> > + drif01: rif@e6f50000 { > >> > + compatible = "renesas,r8a7795-drif", > >> > + "renesas,rcar-gen3-drif"; > >> > + reg = <0 0xe6f50000 0 0x64>; > >> > + interrupts = <GIC_SPI 13 IRQ_TYPE_LEVEL_HIGH>; > >> > + clocks = <&cpg CPG_MOD 514>; > >> > + clock-names = "fck"; > >> > + dmas = <&dmac1 0x22>, <&dmac2 0x22>; > >> > + dma-names = "rx", "rx"; > >> > + power-domains = <&sysc R8A7795_PD_ALWAYS_ON>; > >> > + status = "disabled"; > >> > + }; > >> > + > >> > + drif0: rif@0 { > >> > + compatible = "renesas,r8a7795-drif", > >> > + "renesas,rcar-gen3-drif"; > >> > + sub-channels = <&drif00>, <&drif01>; > >> > + status = "disabled"; > >> > + }; > >> > >> I'm afraid this really hurts my eyes, especially using the same > >> compatible string for both the channel and sub-channel nodes. > >> > >> We need to decide how to model the hardware in DT. Given that the two > >> channels are mostly independent having separate DT nodes makes sense > >> to me. However, as they share the clock and sync signals, somehow > >> grouping them makes sense. I see three ways to do so, and there might > be more. > >> > >> 1. Adding an extra DT node for the channels group, with phandles to > >> the two channels. This is what you're proposing here. > >> > >> 2. Adding an extra DT node for the channels group, as a parent of the > >> two channels. > >> > >> 3. Adding phandles to the channels, pointing to each other, or > >> possibly a phandle from channel 0 pointing to channel 1. > >> > >> Neither of these options seem perfect to me. I don't like option 1 as > >> the group DT node really doesn't describe a hardware block. If we > >> want to use a DT node to convey group information, option 2 seems > >> better to me. However, it somehow abuses the DT parent-child model > >> that is supposed to describe relationships from a control bus point > >> of view. Option 3 has the drawback of not scaling properly, at least > >> with phandles in both channels pointing to the other one. > >> > >> Rob, Geert, tell me you have a fourth idea I haven't thought of that > >> would solve all those problems :-) > > > > What's the purpose/need for grouping them? > > > > I'm fine with Option 2, though I want to make sure it is really needed. > > Each half of a DRIF pair is basically an SPI slave controller without TX > capability, sharing clock and chip-select between the two halves. > Hence you can use either one half to receive 1 bit per clock pulse, or > both halves to receive 2 bits per clock pulse. > You cannot use both halves for independent operation due to the signal > sharing. Is the below model looks OK? I assume this is Option 2. Any preferences on the "parent" compatible string please? --------------------------------------------------------- drif0: rif@0 { compatible = "renesas,rcar-gen3-drif", "simple-bus"; #address-cells = <2>; #size-cells = <2>; status = "disabled"; drif00: rif@e6f40000 { compatible = "renesas,r8a7795-drif", "renesas,rcar-gen3-drif"; reg = <0 0xe6f40000 0 0x64>; interrupts = <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>; clocks = <&cpg CPG_MOD 515>; clock-names = "fck"; dmas = <&dmac1 0x20>, <&dmac2 0x20>; dma-names = "rx", "rx"; power-domains = <&sysc R8A7795_PD_ALWAYS_ON>; status = "disabled"; }; drif01: rif@e6f50000 { compatible = "renesas,r8a7795-drif", "renesas,rcar-gen3-drif"; reg = <0 0xe6f50000 0 0x64>; interrupts = <GIC_SPI 13 IRQ_TYPE_LEVEL_HIGH>; clocks = <&cpg CPG_MOD 514>; clock-names = "fck"; dmas = <&dmac1 0x22>, <&dmac2 0x22>; dma-names = "rx", "rx"; power-domains = <&sysc R8A7795_PD_ALWAYS_ON>; status = "disabled"; }; }; --------------------------------------------------------- Thanks in advance, Ramesh ��.n��������+%������w��{.n����z�{��ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f