Re: [PATCH 2/5] ARM: dts: sun4i: Add support for mmc

[Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 2834 bytes --]

Hi Hans,

On Sat, Dec 14, 2013 at 10:58:12PM +0100, Hans de Goede wrote:
> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Commit-log :)

> ---
>  arch/arm/boot/dts/sun4i-a10-cubieboard.dts | 16 ++++++++++++++++
>  arch/arm/boot/dts/sun4i-a10.dtsi           | 16 ++++++++++++++++
>  2 files changed, 32 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/sun4i-a10-cubieboard.dts b/arch/arm/boot/dts/sun4i-a10-cubieboard.dts
> index 425a7db..d193937 100644
> --- a/arch/arm/boot/dts/sun4i-a10-cubieboard.dts
> +++ b/arch/arm/boot/dts/sun4i-a10-cubieboard.dts
> @@ -42,7 +42,23 @@
>  			};
>  		};
>  
> +		sdc0: sdc@01c0f000 {
> +			pinctrl-names = "default";
> +			pinctrl-0 = <&sdc0_pins_a>;
> +			pinctrl-1 = <&mmc0_cd_pin_cubieboard>;
> +			cd-gpios = <&pio 7 1 0>; /* PH1 */
> +			cd-mode = <1>;
> +			status = "okay";
> +		};
> +
>  		pinctrl@01c20800 {
> +			mmc0_cd_pin_cubieboard: mmc0_cd_pin@0 {
> +				allwinner,pins = "PH1";
> +				allwinner,function = "gpio_in";
> +				allwinner,drive = <0>;
> +				allwinner,pull = <0>;
> +			};
> +

You seem to be using mmc and sdc without any real distinction. Please
try to be consistent and use only one of the two (and I have a slight
preference for mmc :))

>  			led_pins_cubieboard: led_pins@0 {
>  				allwinner,pins = "PH20", "PH21";
>  				allwinner,function = "gpio_out";
> diff --git a/arch/arm/boot/dts/sun4i-a10.dtsi b/arch/arm/boot/dts/sun4i-a10.dtsi
> index 4dccdb0..13bccd5 100644
> --- a/arch/arm/boot/dts/sun4i-a10.dtsi
> +++ b/arch/arm/boot/dts/sun4i-a10.dtsi
> @@ -306,6 +306,15 @@
>  			#size-cells = <0>;
>  		};
>  
> +		sdc0: sdc@01c0f000 {
> +			compatible = "allwinner,sun4i-mmc";
> +			reg = <0x01c0f000 0x1000>;
> +			clocks = <&ahb_gates 8>, <&mmc0>;
> +			interrupts = <32>;
> +			bus-width = <4>;
> +			status = "disabled";
> +		};
> +

The A10 has much more mmc controller doesn't it?

Could you add all of them in the DT ?

>  		intc: interrupt-controller@01c20400 {
>  			compatible = "allwinner,sun4i-ic";
>  			reg = <0x01c20400 0x400>;
> @@ -376,6 +385,13 @@
>  				allwinner,drive = <0>;
>  				allwinner,pull = <0>;
>  			};
> +
> +			sdc0_pins_a: sdc0@0 {
> +				allwinner,pins = "PF0","PF1","PF2","PF3","PF4","PF5";
> +				allwinner,function = "mmc0";
> +				allwinner,drive = <3>;
> +				allwinner,pull = <1>;

Wow, you need both the pullups and a 40mA output?

Ideally, I'd like this patch to be splitted into three:
  - One that adds the MMC controller nodes to the DTSI
  - One that adds the muxing options you need to the pinctrl node
  - One that enables the controller on the boards

Thanks,

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]