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

[Johan Hovold <johan@kernel.org>]

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