Re: [RFC v2 7/7] arm64: dts: mt7986: add Bananapi R3

[Frank Wunderlich (linux)]

Hi

looked now on the keys and regulators comments...

Am 2022-10-28 11:19, schrieb AngeloGioacchino Del Regno:
> Il 26/10/22 11:36, Frank Wunderlich ha scritto:
> > From: Frank Wunderlich <frank-w@xxxxxxxxxxxxxxx>
> > +&mmc0 {
> > +	//sdcard
>
> C-style comments please

i drop it completely if still using the way of an separate sd dts.

> > +	gpio-keys {
> > +		compatible = "gpio-keys";
> > +
> > +		factory-key {
>
> I'd say that this is not "factory-key" but "reset-key"?
>
> > +			label = "reset";
> > +			linux,code = <KEY_RESTART>;
> > +			gpios = <&pio 9 GPIO_ACTIVE_LOW>;
> > +		};

i kept definition similar to mt7622-bpi-r64,in shematic it is defined as 
"reset/factory default" and openwrt wants to use it for "factory" 
function (afair booting some kind of rescue-system for reflashing 
nand/nor). basicly it is a gpio connected to different reset-lines 
(including m.2 slot where it has some side-effects in my board-version).

> > +	};
> > +
> > +	reg_1p8v: regulator-1p8v {
> > +		compatible = "regulator-fixed";
> > +		regulator-name = "fixed-1.8V";
>
> This is "avdd18", isn't it?

no it is the 1.8VD used for emmc.

> > +		regulator-min-microvolt = <1800000>;
> > +		regulator-max-microvolt = <1800000>;
> > +		regulator-boot-on;
> > +		regulator-always-on;
>
> All these regulators have a vin-supply: please fill it in.
> Moreover, in the schematics, I can also see other LDOs: 0.9VD (input 
> +12VD),
> AVDD12 (input 1.8VD), DDRV_VPP (input 3.3VD)...

i have not looked for which the others are defined in shematic only 
added the ones i need to get the board running.

> Of course, this means that you have one more 1.8V regulator, called 
> 1.8vd.

this is the right one

> > +	};
> > +
> > +	reg_3p3v: regulator-3p3v {
> > +		compatible = "regulator-fixed";
> > +		regulator-name = "fixed-3.3V";
>
> regulator-name = "3.3vd";
>
> > +		regulator-min-microvolt = <3300000>;
> > +		regulator-max-microvolt = <3300000>;
> > +		regulator-boot-on;
> > +		regulator-always-on;
>
> vin-supply = <&dcin>; (dcin: regulator-12vd { ... })
>
> > +	};
> > +
> > +	reg_5v: regulator-5v {
> > +		compatible = "regulator-fixed";
> > +		regulator-name = "fixed-5V";
>
> regulator-name  = "fixed-5p1";

why this regulator name and not regulator-name = "5.1vd"; here (or 5vd)?

> > +		regulator-min-microvolt = <5000000>;
> > +		regulator-max-microvolt = <5000000>;
>
> Schematics say "+5V: 5.1V/3A", so this is not 5000000.
>
> > +		regulator-boot-on;
> > +		regulator-always-on;
>
>
> vin-supply = <&dcin>;

basicly i will change the regulator to this:
/* dcin above gpio-keys to preserve alphabetic order - or should i name 
it reg_dcin to have all regulators together? but dcin should be always 
above the others which breaks ordering */

	dcin: regulator-12vd {
		compatible = "regulator-fixed";
		regulator-name = "12vd";
		regulator-min-microvolt = <12000000>;
		regulator-max-microvolt = <12000000>;
		regulator-boot-on;
		regulator-always-on;
	};

/* as far as i see in my shematic all regulators here are powered from 
+12v */
	reg_1p8v: regulator-1p8v {
		compatible = "regulator-fixed";
		regulator-name = "1.8vd";
		regulator-min-microvolt = <1800000>;
		regulator-max-microvolt = <1800000>;
		regulator-boot-on;
		regulator-always-on;
		vin-supply = <&dcin>;
	};

	reg_3p3v: regulator-3p3v {
		compatible = "regulator-fixed";
		regulator-name = "3.3vd";
		regulator-min-microvolt = <3300000>;
		regulator-max-microvolt = <3300000>;
		regulator-boot-on;
		regulator-always-on;
		vin-supply = <&dcin>;
	};

	reg_5v: regulator-5v {
		compatible = "regulator-fixed";
		regulator-name  = "fixed-5p1";
		regulator-min-microvolt = <5100000>;
		regulator-max-microvolt = <5100000>;
		regulator-boot-on;
		regulator-always-on;
		vin-supply = <&dcin>;
	};

using a different naming sheme for reg_5v (regulator-name) does not 
makes sense to me.

regards Frank