Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
Date:   Fri, 28 Oct 2022 15:34:38 +0200
From:   "Frank Wunderlich (linux)" <linux@fw-web.de>
To:     AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@collabora.com>
Cc:     linux-mediatek@lists.infradead.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Matthias Brugger <matthias.bgg@gmail.com>,
        linux-arm-kernel@lists.infradead.org
Subject: Re: [RFC v2 7/7] arm64: dts: mt7986: add Bananapi R3
In-Reply-To: <64daf96b-b2b5-6f02-91aa-58d19083ee01@collabora.com>
References: <20221026093650.110290-1-linux@fw-web.de>
 <20221026093650.110290-8-linux@fw-web.de>
 <64daf96b-b2b5-6f02-91aa-58d19083ee01@collabora.com>
User-Agent: Roundcube Webmail/1.4.13
Message-ID: <022c1168b9ca2f6a3ca8a60a1379dee6@fw-web.de>
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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