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

[Bjorn Andersson <bjorn.andersson@linaro.org>]

On Wed 08 Jun 11:17 CDT 2022, Johan Hovold wrote:

> 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?
> 

The pm8350c seems to include a number of PWM channels, the RGB current
sink (triled) and the LUT block - together making up the "Light Pulse
Generator".

So with that in mind, the compatible seems to have come from the fact
that the author only intended to use one of the PWM sub-blocks...


Thanks for the feedback on the series, will updated and resubmit
accordingly.

Regards,
Bjorn

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