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

Frank Wunderlich (linux)
2022-10-28T13:35:03Z

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@public-files.de>
>> +&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