RE: [RFC 3/5] media: platform: rcar_drif: Add DRIF support

[Ramesh Shanmugasundaram <ramesh.shanmugasundaram-kTT6dE0pTRh9uiUsa/gSgQ@public.gmane.org>]

Hi Geert,

Thank you for the review comments.

> On Wed, Oct 12, 2016 at 4:10 PM, Ramesh Shanmugasundaram
> <ramesh.shanmugasundaram@bp.renesas.com> wrote:
> > This patch adds Digital Radio Interface (DRIF) support to R-Car Gen3
> SoCs.
> > The driver exposes each instance of DRIF as a V4L2 SDR device. A DRIF
> > device represents a channel and each channel can have one or two
> > sub-channels respectively depending on the target board.
> >
> > DRIF supports only Rx functionality. It receives samples from a RF
> > frontend tuner chip it is interfaced with. The combination of DRIF and
> > the tuner device, which is registered as a sub-device, determines the
> > receive sample rate and format.
> >
> > In order to be compliant as a V4L2 SDR device, DRIF needs to bind with
> > the tuner device, which can be provided by a third party vendor. DRIF
> > acts as a slave device and the tuner device acts as a master
> > transmitting the samples. The driver allows asynchronous binding of a
> > tuner device that is registered as a v4l2 sub-device. The driver can
> > learn about the tuner it is interfaced with based on port endpoint
> > properties of the device in device tree. The V4L2 SDR device inherits
> > the controls exposed by the tuner device.
> >
> > The device can also be configured to use either one or both of the
> > data pins at runtime based on the master (tuner) configuration.
> 
> Thanks for your patch!
> 
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/renesas,drif.txt
> > @@ -0,0 +1,109 @@
> > +Renesas R-Car Gen3 DRIF controller (DRIF)
> > +-----------------------------------------
> > +
> > +Required properties:
> > +--------------------
> > +- compatible: "renesas,drif-r8a7795" if DRIF controller is a part of
> R8A7795 SoC.
> 
> "renesas,r8a7795-drif", as Rob already pointed out.

Agreed.

> 
> > +             "renesas,rcar-gen3-drif" for a generic R-Car Gen3
> compatible device.
> > +             When compatible with the generic version, nodes must list
> the
> > +             SoC-specific version corresponding to the platform first
> > +             followed by the generic version.
> > +
> > +- reg: offset and length of each sub-channel.
> > +- interrupts: associated with each sub-channel.
> > +- clocks: phandles and clock specifiers for each sub-channel.
> > +- clock-names: clock input name strings: "fck0", "fck1".
> > +- pinctrl-0: pin control group to be used for this controller.
> > +- pinctrl-names: must be "default".
> > +- dmas: phandles to the DMA channels for each sub-channel.
> > +- dma-names: names for the DMA channels: "rx0", "rx1".
> > +
> > +Required child nodes:
> > +---------------------
> > +- Each DRIF channel can have one or both of the sub-channels enabled
> > +in a
> > +  setup. The sub-channels are represented as a child node. The name
> > +of the
> > +  child nodes are "sub-channel0" and "sub-channel1" respectively.
> > +Each child
> > +  node supports the "status" property only, which is used to
> > +enable/disable
> > +  the respective sub-channel.
> 
> > +Example
> > +--------
> > +
> > +SoC common dtsi file
> > +
> > +drif0: rif@e6f40000 {
> > +       compatible = "renesas,drif-r8a7795",
> > +                  "renesas,rcar-gen3-drif";
> > +       reg = <0 0xe6f40000 0 0x64>, <0 0xe6f50000 0 0x64>;
> > +       interrupts = <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>,
> > +                  <GIC_SPI 13 IRQ_TYPE_LEVEL_HIGH>;
> > +       clocks = <&cpg CPG_MOD 515>, <&cpg CPG_MOD 514>;
> > +       clock-names = "fck0", "fck1";
> > +       dmas = <&dmac1 0x20>, <&dmac1 0x22>;
> > +       dma-names = "rx0", "rx1";
> 
> I could not find the DMAC channels in the datasheet?

It is mentioned only in v0.5 h/w manual. v0.52 manual introduced DRIF chapter but then some of the old references were missing :-(. There are few more doc anomalies, which I shall document in the next version of the patch.

> Most modules are either tied to dmac0, or two both dmac1 and dmac2.
> In the latter case, you want to list two sets of dmas, one for each DMAC.

You are right. I have added both dmac1 & 2 now.

> 
> > +       power-domains = <&sysc R8A7795_PD_ALWAYS_ON>;
> > +       status = "disabled";
> > +
> > +       sub-channel0 {
> > +               status = "disabled";
> > +       };
> > +
> > +       sub-channel1 {
> > +               status = "disabled";
> > +       };
> > +
> > +};
> 
> As you're modelling this in DT under a single device node, this means you
> cannot use runtime PM to manage the module clocks of the individual
> channels.
> 
> An alternative could be to have two separate nodes for each channel, and
> tie them together using a phandle.

I agree & thanks for the suggestion. Is the below model looks anything closer? Appreciate your inputs.

dtsi
-------

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

-------
board dts file would have something like this

&drif00 {
        status = "okay";
};      
        
&drif01 {
        status = "okay";
};      
        
&drif0 {
        pinctrl-0 = <&drif0_pins>;
        pinctrl-names = "default";
        status = "okay";

        port {
                drif0_ep: endpoint {
                     remote-endpoint = <&max2175_0_ep>;
                };
        };
};
-------
The drif0X driver instance will help only in registering with genpd.
The parent drif0 instance will parse "sub-channels" phandles and use the resources of respective enabled sub-channels using it's pdev. 

Thanks,
Ramesh