On Thu, Feb 23, 2023 at 03:36:54PM +0000, Bryan O'Donoghue wrote:
> The apq8039-t2 is an apq8039 based board paired with a wcn3680b WiFi
> chipset.
>
> Co-developed-by: Shawn Guo <shawn.guo@xxxxxxxxxx>
> Signed-off-by: Shawn Guo <shawn.guo@xxxxxxxxxx>
> Co-developed-by: Jun Nie <jun.nie@xxxxxxxxxx>
> Signed-off-by: Jun Nie <jun.nie@xxxxxxxxxx>
> Co-developed-by: Benjamin Li <benl@xxxxxxxxxxxx>
> Signed-off-by: Benjamin Li <benl@xxxxxxxxxxxx>
> Co-developed-by: James Willcox <jwillcox@xxxxxxxxxxxx>
> Signed-off-by: James Willcox <jwillcox@xxxxxxxxxxxx>
> Co-developed-by: Leo Yan <leo.yan@xxxxxxxxxx>
> Signed-off-by: Leo Yan <leo.yan@xxxxxxxxxx>
> Co-developed-by: Joseph Gates <jgates@xxxxxxxxxxxx>
> Signed-off-by: Joseph Gates <jgates@xxxxxxxxxxxx>
> Co-developed-by: Max Chen <mchen@xxxxxxxxxxxx>
> Signed-off-by: Max Chen <mchen@xxxxxxxxxxxx>
> Co-developed-by: Zac Crosby <zac@xxxxxxxxxxxx>
> Signed-off-by: Zac Crosby <zac@xxxxxxxxxxxx>
> Reviewed-by: Konrad Dybcio <konrad.dybcio@xxxxxxxxxx>
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@xxxxxxxxxx>
> ---
> arch/arm64/boot/dts/qcom/Makefile | 1 +
> arch/arm64/boot/dts/qcom/apq8039-t2.dts | 550 ++++++++++++++++++++++++
> 2 files changed, 551 insertions(+)
> create mode 100644 arch/arm64/boot/dts/qcom/apq8039-t2.dts
>
> diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile
> index 31aa54f0428c3..2983e83a19061 100644
> --- a/arch/arm64/boot/dts/qcom/Makefile
> +++ b/arch/arm64/boot/dts/qcom/Makefile
> @@ -1,5 +1,6 @@
> # SPDX-License-Identifier: GPL-2.0
> dtb-$(CONFIG_ARCH_QCOM) += apq8016-sbc.dtb
> +dtb-$(CONFIG_ARCH_QCOM) += apq8039-t2.dtb
> dtb-$(CONFIG_ARCH_QCOM) += apq8094-sony-xperia-kitakami-karin_windy.dtb
> dtb-$(CONFIG_ARCH_QCOM) += apq8096-db820c.dtb
> dtb-$(CONFIG_ARCH_QCOM) += apq8096-ifc6640.dtb
> diff --git a/arch/arm64/boot/dts/qcom/apq8039-t2.dts b/arch/arm64/boot/dts/qcom/apq8039-t2.dts
> new file mode 100644
> index 0000000000000..6ff044bfe2ebc
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/apq8039-t2.dts
> @@ -0,0 +1,550 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2015, The Linux Foundation. All rights reserved.
> + * Copyright (c) 2020-2023, Linaro Ltd.
> + *
> + */
> +
> +/dts-v1/;
> +
> +#include "msm8939.dtsi"
> +#include "msm8939-pm8916.dtsi"
> +#include <dt-bindings/arm/qcom,ids.h>
> +#include <dt-bindings/gpio/gpio.h>
> +#include <dt-bindings/pinctrl/qcom,pmic-gpio.h>
> +#include <dt-bindings/sound/apq8016-lpass.h>
> +
> +/ {
> + model = "Square, Inc. T2 Devkit";
> + compatible = "square,apq8039-t2", "qcom,msm8939";
> +
> + qcom,board-id = <0x53 0x54>;
> + qcom,msm-id = <QCOM_ID_MSM8939 0>, <QCOM_ID_MSM8939 0x30000>, <QCOM_ID_APQ8039 0x30000>;
Does the board have variants with MSM8939 v2, MSM8939 v3.0 and APQ8039
v3.0? If not it should be enough to list only a single item (the actual
SoC in use).
> [...]
> +&blsp1_uart1 {
> + pinctrl-names = "default", "sleep";
> + pinctrl-0 = <&blsp1_uart1_default>;
> + pinctrl-1 = <&blsp1_uart1_sleep>;
> + status = "okay";
> +};
> +
> +&blsp1_uart2 {
> + pinctrl-names = "default", "sleep";
> + pinctrl-0 = <&blsp1_uart2_default>;
> + pinctrl-1 = <&blsp1_uart2_sleep>;
This is in msm8939.dtsi already.
> [...]
> +&pronto {
> + pinctrl-names = "default";
> + pinctrl-0 = <&wcnss_pin_a>;
This is also in msm8939.dtsi already.
> + status = "okay";
> +
> + iris {
> + compatible = "qcom,wcn3680";
> + };
> +};
> +
> [...]
> + blsp1_uart1_default: blsp1-uart1-default-state {
> + pins = "gpio0", "gpio1";
> + function = "blsp_uart1";
> + drive-strength = <16>;
> + bias-disable;
> + };
> +
> + blsp1_uart1_sleep: blsp1-uart1-sleep-state {
> + pins = "gpio0", "gpio1";
> + function = "gpio";
> + drive-strength = <2>;
> + bias-pull-down;
> + };
Personally I'd override just the pins of the definition in msm8939.dtsi
since everything else is exactly the same. Not a strict change request
though, I guess this is subjective:
&blsp1_uart1_default {
pins = "gpio0", "gpio1";
};
&blsp1_uart1_sleep {
pins = "gpio0", "gpio1";
};
> +
> + ext_buck_vsel_reg: ext-buck-vsel-reg-state {
> + function = "gpio";
> + pins = "gpio111";
> + drive-strength = <2>;
> + };
This seems unused.
> [...]
> + sq_spe_enable: sq-spe-enable-state {
> + pins = "gpio35";
> + function = "gpio";
> + output-low;
> + };
This seems unused as well.
> +
> + tlmm_crq_reg: tlmm-crq-reg-state {
> + function = "gpio";
> + pins = "gpio12";
> + output-high;
> + };
> +
> + tlmm_spe_reg: tlmm-spe-reg-state {
> + pins = "gpio108";
> + function = "gpio";
> + output-high;
> + };
> +
Does output-high really make sense here? These are assigned to a
regulator-fixed. The driver for fixed regulators initializes the
GPIO state based on the "regulator-boot-on" property. If the property
exists it does gpiod_get(... GPIOD_OUT_HIGH), otherwise GPIOD_OUT_LOW.
You don't have regulator-boot-on. So I would expect the GPIO/regulator
state to toggle unnecessarily during initialization:
- First pinctrl is applied so the GPIO is set to output-high.
The regulator gets turned on for a few ms/ns.
- Then the driver probes which initializes it to GPIOD_OUT_LOW.
The regulator turns back off.
I'd just drop the output-high here and leave it up to the regulator
driver to initialize the GPIO state. If you want the regulator on during
boot, use regulator-boot-on.
Thanks,
Stephan
