RE: [PATCH v2 3/3] ARM: dts: rskrza1: add mmc DT support

From: Chris Brandt <Chris.Brandt@renesas.com>
To: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Simon Horman <horms+renesas@verge.net.au>,
	Magnus Damm <magnus.damm@gmail.com>,
	Rob Herring <robh+dt@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	Linux-Renesas <linux-renesas-soc@vger.kernel.org>
Subject: RE: [PATCH v2 3/3] ARM: dts: rskrza1: add mmc DT support
Date: Tue, 27 Sep 2016 08:11:19 +0000	[thread overview]
Message-ID: <SG2PR06MB1165C87F37A43CEF998EA78B8ACC0@SG2PR06MB1165.apcprd06.prod.outlook.com> (raw)
In-Reply-To: <CAMuHMdXQrO_brqrzZtp4iV8CnxER-yfTYT03YdDxud3Fp0bHAw@mail.gmail.com>

Hi Geert,

> > Since the MMC and SDHI1 on the RSK share the same socket connector
> > (CN1), you cannot enable MMC and SDHI1 at the same time. Therefore the
> > status has been set to disabled because SDHI is more popular with this
> board.
> > However, keeping this code in here serves as a good way to document
> > how the MMC on the RZ/A1 has been known to work for someone that does
> > want to use MMC instead of SDHI1.
> >
> > A fixed 3.3 regulator is included because it is required by the mmc
> > driver.
> 
> I have mixed feelings about adding these "disabled" nodes, purely for
> documentation.
> 
> What do other people think?

From what I found, users just want to 'copy/paste' working code and delete then parts they don't need. Deleting code is easier than adding code.

However...
If this kind of 'dead sample code' in the DT is considered ugly for upstream, I can take it out and only include it in the Renesas BSP packages.
My main focus is for the drivers and the .dtsi files to be upstream. I think of the board dts files as more like 'sample configurations' anyway, so providing them separately is not an issue.

> > --- a/arch/arm/boot/dts/r7s72100-rskrza1.dts
> > +++ b/arch/arm/boot/dts/r7s72100-rskrza1.dts
> > @@ -33,6 +33,15 @@
> >                 #address-cells = <1>;
> >                 #size-cells = <1>;
> >         };
> > +
> > +       d3_3v: regulator-d3-3v {
> > +               compatible = "regulator-fixed";
> > +               regulator-name = "D3.3V";
> > +               regulator-min-microvolt = <3300000>;
> > +               regulator-max-microvolt = <3300000>;
> > +               regulator-boot-on;
> > +               regulator-always-on;
> 
> 'status = "disabled";', too?

Good point. That would be "disabled" too.
But, as I mentioned above, if you decide to have me remove the disabled mmcif node, I'll remove this node too.

Chris