Re: [PATCH 1/3] arm64: allwinner: a64: add R_I2C controller

From: Maxime Ripard <maxime.ripard@bootlin.com>
To: Vasily Khoruzhick <anarsoul@gmail.com>
Cc: Mark Rutland <mark.rutland@arm.com>,
	devicetree@vger.kernel.org,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will.deacon@arm.com>, Chen-Yu Tsai <wens@csie.org>,
	Rob Herring <robh+dt@kernel.org>,
	arm-linux <linux-arm-kernel@lists.infradead.org>,
	Icenowy Zheng <icenowy@aosc.io>
Subject: Re: [PATCH 1/3] arm64: allwinner: a64: add R_I2C controller
Date: Mon, 4 Jun 2018 10:51:11 +0200	[thread overview]
Message-ID: <20180604085111.s23qamdethe6sr6d@flea> (raw)
In-Reply-To: <CA+E=qVfH3wWsSdzGn353q0Jf-PQ8cjU-i5osLOV1c4qWVGrR5A@mail.gmail.com>

[-- Attachment #1.1: Type: text/plain, Size: 2678 bytes --]

On Fri, Jun 01, 2018 at 10:30:00AM -0700, Vasily Khoruzhick wrote:
> >> ---
> >>  arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 17 +++++++++++++++++
> >>  1 file changed, 17 insertions(+)
> >>
> >> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> >> index 1b2ef28c42bd..b5e903ccf0ec 100644
> >> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> >> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> >> @@ -46,6 +46,7 @@
> >>  #include <dt-bindings/clock/sun8i-r-ccu.h>
> >>  #include <dt-bindings/interrupt-controller/arm-gic.h>
> >>  #include <dt-bindings/reset/sun50i-a64-ccu.h>
> >> +#include <dt-bindings/reset/sun8i-r-ccu.h>
> >>
> >>  / {
> >>       interrupt-parent = <&gic>;
> >> @@ -655,6 +656,17 @@
> >>                       #reset-cells = <1>;
> >>               };
> >>
> >> +             r_i2c: i2c@1f02400 {
> >> +                     compatible = "allwinner,sun6i-a31-i2c";
> >
> > You should add an a64 compatible here
> 
> We don't have it for regular i2c, why should I add it here?

We miss some of them. Adding for i2c would make sense too.

> >> +                     reg = <0x01f02400 0x400>;
> >> +                     interrupts = <GIC_SPI 44 IRQ_TYPE_LEVEL_HIGH>;
> >> +                     clocks = <&r_ccu CLK_APB0_I2C>;
> >> +                     resets = <&r_ccu RST_APB0_I2C>;
> >> +                     status = "disabled";
> >> +                     #address-cells = <1>;
> >> +                     #size-cells = <0>;
> >> +             };
> >> +
> >>               r_pio: pinctrl@1f02c00 {
> >>                       compatible = "allwinner,sun50i-a64-r-pinctrl";
> >>                       reg = <0x01f02c00 0x400>;
> >> @@ -670,6 +682,11 @@
> >>                               pins = "PL0", "PL1";
> >>                               function = "s_rsb";
> >>                       };
> >> +
> >> +                     r_i2c_pins_a: i2c-a {
> >> +                             pins = "PL8", "PL9";
> >> +                             function = "s_i2c";
> >> +                     };
> >
> > This should be ordered by alphabetical order
> 
> OK
> 
> > If this is the only muxing option, you can also add it to the i2c DT
> > node.
> 
> It's not the only option, but other option conflicts with rsb. Should
> I still add it to i2c DT node?

I guess you can put it there, the muxing will only be enforced if the
device is enabled, and there should be only one of RSB or I2C that
would be.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel