Re: [PATCH 3/4] arm64: dts: qcom: sc8280x: Add reference device

Johan Hovold
2022-06-08T16:17:41Z

On Tue, Jun 07, 2022 at 02:41:12PM -0700, Bjorn Andersson wrote:
> Add basic support for the SC8280XP reference device, which allows it to
> boot to a shell (using EFIFB) with functional storage (UFS), USB,
> keyboard, touchpad, touchscreen, backlight and remoteprocs.
>
> The PMICs are, per socinfo, reused from other platforms. But given that
> the address of the PMICs doesn't match other cases and that it's
> desirable to label things according to the schematics a new dtsi file is
> created to represent the reference combination of PMICs.

nit: missing p in "sc8280xp" in Subject.

> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
> arch/arm64/boot/dts/qcom/Makefile | 1 +
> arch/arm64/boot/dts/qcom/sc8280xp-crd.dts | 423 +++++++++++++++++++
> arch/arm64/boot/dts/qcom/sc8280xp-pmics.dtsi | 108 +++++
> 3 files changed, 532 insertions(+)
> create mode 100644 arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
> create mode 100644 arch/arm64/boot/dts/qcom/sc8280xp-pmics.dtsi

> + vreg_misc_3p3: misc-3p3-regulator {
> + compatible = "regulator-fixed";
> + regulator-name = "VREG_MISC_3P3";
> +
> + regulator-min-microvolt = <3300000>;
> + regulator-max-microvolt = <3300000>;
> +
> + gpio = <&pmc8280_1_gpios 0 GPIO_ACTIVE_HIGH>;

The PMIC gpios are 1-based, so this should be

gpio = <&pmc8280_1_gpios 1 GPIO_ACTIVE_HIGH>;

or the regulator fails to probe.

> + enable-active-high;
> + pinctrl-names = "default";
> + pinctrl-0 = <&misc_3p3_reg_en>;
> +
> + regulator-boot-on;
> + regulator-always-on;
> + };
> +
> + reserved-memory {
> + };
> +};

> +&qup0_i2c4 {
> + status = "okay";

Please move the status property last throughout here too.

> + clock-frequency = <400000>;
> +
> + pinctrl-names = "default";
> + pinctrl-0 = <&qup0_i2c4_default>, <&ts0_default>;
> +
> + hid@10 {

I've changed this to use the more descriptive name "touchscreen".

> + compatible = "hid-over-i2c";
> + reg = <0x10>;
> + hid-descr-addr = <0x1>;
> +
> + interrupts-extended = <&tlmm 175 IRQ_TYPE_LEVEL_LOW>;
> + };
> +};

> +&qup2_i2c5 {
> + status = "okay";
> + clock-frequency = <400000>;
> +
> + pinctrl-names = "default";
> + pinctrl-0 = <&qup2_i2c5_default>, <&kybd_default>, <&tpad_default>;
> +
> + hid@15 {

And this to "touchpad@15"

> + compatible = "hid-over-i2c";
> + reg = <0x15>;
> + hid-descr-addr = <0x1>;
> +
> + interrupts-extended = <&tlmm 182 IRQ_TYPE_LEVEL_LOW>;
> + };
> +
> + hid@68 {

And keyboard@68

Sure these are multifunction devices, but this is the primary function.

> + compatible = "hid-over-i2c";
> + reg = <0x68>;
> + hid-descr-addr = <0x1>;
> +
> + interrupts-extended = <&tlmm 104 IRQ_TYPE_LEVEL_LOW>;
> + };
> +};

> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-pmics.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp-pmics.dtsi
> new file mode 100644
> index 000000000000..36ed7d808ab8
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/sc8280xp-pmics.dtsi

> + pmc8280c: pmic@2 {
> + compatible = "qcom,pm8350c", "qcom,spmi-pmic";
> + reg = <0x2 SPMI_USID>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + pmc8280c_gpios: gpio@8800 {
> + compatible = "qcom,pm8350c-gpio", "qcom,spmi-gpio";
> + reg = <0x8800>;
> + gpio-controller;
> + gpio-ranges = <&pmc8280c_gpios 0 0 9>;
> + #gpio-cells = <2>;
> + interrupt-controller;
> + #interrupt-cells = <2>;
> + };
> +
> + pmc8280c_lpg: lpg@e800 {

I renamed the node (and label suffix) "pwm" when I noticed that the
binding had changed in mainline.

Since this device is used as a PWM provider I guess that's a better
name?

> + compatible = "qcom,pm8350c-pwm";
> + reg = <0xe800>;
> +
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + #pwm-cells = <2>;
> +
> + status = "disabled";
> + };
> + };

Johan