Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 25 Jan 2022 22:13:00 -0600
From:   Bjorn Andersson <bjorn.andersson@linaro.org>
To:     Ben Wolsieffer <benwolsieffer@gmail.com>, linus.walleij@linaro.org
Cc:     Andy Gross <agross@kernel.org>, Rob Herring <robh+dt@kernel.org>,
        Arnd Bergmann <arnd@arndb.de>, Olof Johansson <olof@lixom.net>,
        soc@kernel.org, Stephen Boyd <sboyd@codeaurora.org>,
        linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 3/3] ARM: dts: qcom: basic HP TouchPad support
Message-ID: <YfDKTGQDh3tDMECz@builder.lan>
References: <cover.1643075547.git.benwolsieffer@gmail.com>
 <9f19df2a0017b71547445ac34df221e827c45bd0.1643075547.git.benwolsieffer@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <9f19df2a0017b71547445ac34df221e827c45bd0.1643075547.git.benwolsieffer@gmail.com>
Precedence: bulk

On Mon 24 Jan 20:07 CST 2022, Ben Wolsieffer wrote:

> Modify the Dragonboard device tree to support the most basic hardware on
> the HP TouchPad. The headphone UART port and eMMC are supported.
> 

We typically don't have one commit for the cloning and then one to
update the content, in particular since your diffstat became rather
weird.

That said, got some comments below, things that I wouldn't have spotted
if you sent this as just a new file.

> Signed-off-by: Ben Wolsieffer <benwolsieffer@gmail.com>
> ---

@Linus, please take a look at the regulator question below.

>  arch/arm/boot/dts/qcom-apq8060-tenderloin.dts | 549 ++----------------
>  1 file changed, 45 insertions(+), 504 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/qcom-apq8060-tenderloin.dts b/arch/arm/boot/dts/qcom-apq8060-tenderloin.dts
> index 996e73aa0b0b..e294f3920b9f 100644
> --- a/arch/arm/boot/dts/qcom-apq8060-tenderloin.dts
> +++ b/arch/arm/boot/dts/qcom-apq8060-tenderloin.dts
> @@ -14,6 +14,8 @@ aliases {
>  	};
>  
>  	chosen {
> +		/* Bootloader passes console=tty1, which overrides stdout-path */
> +		bootargs = "console=ttyMSM0,115200 earlycon";
>  		stdout-path = "serial0:115200n8";
>  	};
[..]
>  
>  	soc {
>  		pinctrl@800000 {
> -			/* eMMMC pins, all 8 data lines connected */

It would be nice if you could throw a separate patch on the list that
fixes this spelling mistake in the original as well.

> -			dragon_sdcc1_pins: sdcc1 {
> +			/* eMMC pins, all 8 data lines connected */
> +			emmc_pins: sdcc1 {
>  				mux {
>  					pins = "gpio159", "gpio160", "gpio161",
>  					     "gpio162", "gpio163", "gpio164",
>  					     "gpio165", "gpio166", "gpio167",
>  					     "gpio168";
> -					     function = "sdc1";
> +					function = "sdc1";
>  				};
>  				clk {
>  					pins = "gpio167"; /* SDC1 CLK */
[..]
> @@ -171,205 +77,33 @@ pinconf {
>  				};
>  			};
>  
> -			dragon_gsbi12_i2c_pins: gsbi12_i2c {
> -				mux {
> -					pins = "gpio115", "gpio116";
> -					function = "gsbi12";
> -				};
> -				pinconf {
> -					pins = "gpio115", "gpio116";
> -					drive-strength = <16>;
> -					/* These have external pull-up 4.7kOhm to 1.8V */
> -					bias-disable;
> -				};
> -			};
> -
> -			/* Primary serial port uart 0 pins */
> -			dragon_gsbi12_serial_pins: gsbi12_serial {
> +			/* Headphone UART pins */
> +			headphone_uart_pins: gsbi12_serial {
>  				mux {
>  					pins = "gpio117", "gpio118";
>  					function = "gsbi12";
>  				};
> -				tx {
> -					pins = "gpio117";
> -					drive-strength = <8>;
> -					bias-disable;
> -				};
>  				rx {
> -					pins = "gpio118";
> +					pins = "gpio117";
>  					drive-strength = <2>;
>  					bias-pull-up;
>  				};
> -			};

I find it hard to conclude what the resulting snippet is from this
chunk, did rx swap place from gpio118 to gpio117?

[..]
> @@ -814,14 +378,16 @@ l20 {
>  					bias-pull-down;
>  				};
>  				l21 {
> -					// 1.1 V according to schematic
>  					regulator-min-microvolt = <1200000>;
>  					regulator-max-microvolt = <1200000>;
>  					bias-pull-down;
> -					regulator-always-on;
> +					/*
> +					 * RPM driver can't handle always-on regulators that are
> +					 * supplied by regulators initialized after them.
> +					 */

That looks like an oversight that should be corrected, perhaps it needs
similar attention that was given to the smd-rpm driver recently?

But this makes me wonder, how can this work on the other board? Linus?

> +					// regulator-always-on;
>  				};
>  				l22 {
> -					// 1.2 V according to schematic
>  					regulator-min-microvolt = <1150000>;
>  					regulator-max-microvolt = <1150000>;
>  					bias-pull-down;
> @@ -845,7 +411,7 @@ l25 {
>  				};
>  
>  				s0 {
> -					// regulator-min-microvolt = <500000>;
> +					// regulator-min-microvolt = <800000>;
>  					// regulator-max-microvolt = <1325000>;

This looks like the full range the regulator could do, do you see a
reason for documenting that here? Unless there's a good reason I think
you should leave the commented min/max out.

>  					regulator-min-microvolt = <1100000>;
>  					regulator-max-microvolt = <1100000>;

Regards,
Bjorn