Re: [PATCH v2 1/1] arm64: dts: qcom: Add Lenovo Yoga C630

Bjorn Andersson
2019-09-04T20:43:20Z

On Wed 04 Sep 04:52 PDT 2019, Vinod Koul wrote:

> On 04-09-19, 12:39, Lee Jones wrote:
> > --- a/arch/arm64/boot/dts/qcom/Makefile
> > +++ b/arch/arm64/boot/dts/qcom/Makefile
> > @@ -12,5 +12,6 @@ dtb-$(CONFIG_ARCH_QCOM) += sdm845-cheza-r2.dtb
> > dtb-$(CONFIG_ARCH_QCOM) += sdm845-cheza-r3.dtb
> > dtb-$(CONFIG_ARCH_QCOM) += sdm845-db845c.dtb
> > dtb-$(CONFIG_ARCH_QCOM) += sdm845-mtp.dtb
> > +dtb-$(CONFIG_ARCH_QCOM) += sdm850-lenovo-yoga-c630.dtb
>
> Can we keep this sorted, so before mtp.
>
> > dtb-$(CONFIG_ARCH_QCOM) += qcs404-evb-1000.dtb
> > dtb-$(CONFIG_ARCH_QCOM) += qcs404-evb-4000.dtb
> > diff --git a/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts b/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts
> > new file mode 100644
> > index 000000000000..ad160c718b33
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts
> > @@ -0,0 +1,454 @@
> > +// SPDX-License-Identifier: GPL-2.0
>
> Are we going to make this dual? or BSD..
>

Sounds good, Rob wants dual license so lets flag it as such.

> > +&apps_rsc {
> > + pm8998-rpmh-regulators {
> > + compatible = "qcom,pm8998-rpmh-regulators";
> > + qcom,pmic-id = "a";
> > +
> > + vdd-l2-l8-l17-supply = <&vreg_s3a_1p35>;
> > + vdd-l7-l12-l14-l15-supply = <&vreg_s5a_2p04>;
> > +
> > + vreg_s2a_1p125: smps2 {
> > + };
> > +
> > + vreg_s3a_1p35: smps3 {
> > + regulator-min-microvolt = <1352000>;
> > + regulator-max-microvolt = <1352000>;
> > + regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
> > + };
> > +
> > + vreg_s4a_1p8: smps4 {
> > + regulator-min-microvolt = <1800000>;
> > + regulator-max-microvolt = <1800000>;
> > + regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
> > + };
> > +
> > + vreg_s5a_2p04: smps5 {
> > + regulator-min-microvolt = <2040000>;
> > + regulator-max-microvolt = <2040000>;
> > + regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
> > + };
> > +
> > + vreg_s7a_1p025: smps7 {
>
> Any reason why we dont specify the mode and min/max voltage for this
> and few others below..?
>

Iirc these values are not known from the tables provided by the
firmware. Label names are just "borrowed" from the MTP, so we can't
derive anything from there either.

I intended to review and clean this up before it was posted...

Regards,
Bjorn

> > +&i2c1 {
> > + status = "okay";
> > + clock-frequency = <400000>;
> > + qcom,geni-se-fifo;
> > +
> > + battery@70 {
> > + compatible = "some,battery";
>
> some,battery ..?
>
> > +&qup_i2c12_default {
>
> Please move the qup nodes up so that nodes are sorted alphabetically
>
> --
> ~Vinod