Re: [PATCH 5/5] media: platform: rcar_drif: Add DRIF support

Hi Rob,

On Mon, Nov 14, 2016 at 8:52 PM, Rob Herring [off-list ref] wrote:

On Thu, Nov 10, 2016 at 11:22:20AM +0200, Laurent Pinchart wrote:

quoted

On Wednesday 09 Nov 2016 15:44:44 Ramesh Shanmugasundaram wrote:

quoted

--- /dev/null
+++ b/Documentation/devicetree/bindings/media/renesas,drif.txt

@@ -0,0 +1,136 @@

quoted

quoted

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

quoted

quoted

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

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html