| Message ID | 20190411205144.43557-1-jeffrey.l.hugo@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] arm64: dts: qcom: Add Lenovo Miix 630 | expand |
On Thu, Apr 11, 2019 at 01:51:44PM -0700, Jeffrey Hugo wrote: > This adds the initial DT for the Lenovo Miix 630 laptop. Supported > functionality includes USB (host), microSD-card, keyboard, and trackpad. > > Signed-off-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com> > --- > > v2: > -Changed "cls" to "clam" since feedback indicated "cls" is too opaque, but > "clamshell" is a mouthfull. "clam" seems to be a happy medium. Please use "clamshell". The extra 5 characters aren't going to hurt anyone, and it avoids having yet another short name just confuses matters further. [...] > + keyboard@3a { > + /* QTEC0001 is the ACPI HID, which could be used for quirks */ > + compatible = "QTEC0001", "hid-over-i2c"; As mentioned last time, please drop the ACPI HID, and allocate a real compatible string. Thanks, Mark.
I'm sorry, I had completely missed your reply to v1. To answer your previous question, why do we need a DT? DT is the quick path to support for these systems. There is already a fair amount of support in mainline to support the core SoC (msm8998) which is/was heavily used in top tier phone products. All of that support is DT based, which is expected to also be leveraged to other products beyond these laptops, and the existing phones on the market. As far as I am aware, the ACPI for the laptops was developed only with Windows in mind, and it is expected, from initial investigations, that supporting the ACPI is going to be painful. Its on the road map, but getting full functionality, quickly, is what what we'd like to do to meet the demand for these systems. On Fri, Apr 12, 2019 at 8:47 AM Mark Rutland <mark.rutland@arm.com> wrote: > > On Thu, Apr 11, 2019 at 01:51:44PM -0700, Jeffrey Hugo wrote: > > This adds the initial DT for the Lenovo Miix 630 laptop. Supported > > functionality includes USB (host), microSD-card, keyboard, and trackpad. > > > > Signed-off-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com> > > --- > > > > v2: > > -Changed "cls" to "clam" since feedback indicated "cls" is too opaque, but > > "clamshell" is a mouthfull. "clam" seems to be a happy medium. > > Please use "clamshell". The extra 5 characters aren't going to hurt > anyone, and it avoids having yet another short name just confuses > matters further. Sure. Will queue this for the next rev. > > [...] > > > + keyboard@3a { > > + /* QTEC0001 is the ACPI HID, which could be used for quirks */ > > + compatible = "QTEC0001", "hid-over-i2c"; > > As mentioned last time, please drop the ACPI HID, and allocate a real > compatible string. So, I'm in a quandary with this device. As far as I can tell, its an off the shelf component, the device adheres to the "PNP0C50" spec (HID over I2C), and can be driven by the full "hid-over-i2c" driver (which is just a DT shim over the PNP0C50 ACPI driver). However, the device itself identifies itself as an ELAN 400 device, which is an ID that is also used for standalone touchpad devices. Per my understanding of the Linux drivers, there is a separate ELAN driver for the standalone touchpad devices as its been discovered though trial and error that the Linux PNP0C50 driver cannot drive those devices. To handle this, there is a quirk in hid-quirks which rejects ELAN 400 devices, except those which are "QTEC0001". We need that quirk bypass for this device because the ELAN driver cannot handle this device. I'd much rather have a single identifier to quirk on, rather than having one for DT and one for ACPI, and its not looking feasible to get the vendor to update the ACPI, so it seems like using the ACPI identifier is just simpler. So, if you want a different compatible string, I'll need to go put DT in a driver that is primarily ACPI. I'm not sure what the HID folks will think of that. I'll propose it, but what do you view is a "proper" compatible string? "ELAN0400-msm8998-clamshell"?
On Fri, Apr 12, 2019 at 09:19:18AM -0600, Jeffrey Hugo wrote: > > > + keyboard@3a { > > > + /* QTEC0001 is the ACPI HID, which could be used for quirks */ > > > + compatible = "QTEC0001", "hid-over-i2c"; > > > > As mentioned last time, please drop the ACPI HID, and allocate a real > > compatible string. > > So, I'm in a quandary with this device. As far as I can tell, its an > off the shelf component, the device adheres to the "PNP0C50" spec (HID > over I2C), and can be driven by the full "hid-over-i2c" driver (which > is just a DT shim over the PNP0C50 ACPI driver). However, the device > itself identifies itself as an ELAN 400 device, which is an ID that is > also used for standalone touchpad devices. Per my understanding of > the Linux drivers, there is a separate ELAN driver for the standalone > touchpad devices as its been discovered though trial and error that > the Linux PNP0C50 driver cannot drive those devices. To handle this, > there is a quirk in hid-quirks which rejects ELAN 400 devices, except > those which are "QTEC0001". > > We need that quirk bypass for this device because the ELAN driver > cannot handle this device. This is useful context; thanks for writing this up! > I'd much rather have a single identifier to quirk on, rather than > having one for DT and one for ACPI, and its not looking feasible to > get the vendor to update the ACPI, so it seems like using the ACPI > identifier is just simpler. > > So, if you want a different compatible string, I'll need to go put DT > in a driver that is primarily ACPI. I'm not sure what the HID folks > will think of that. My objection is that an ACPI HID is _not_ a DT compatible string, and the two should be treated separately. Munging the two together opens the door for other pain. The driver in question has a DT probe function, i2c_hid_of_probe, so there's certainly a place to wire up that quirk. > I'll propose it, but what do you view is a > "proper" compatible string? "ELAN0400-msm8998-clamshell"? A proper compatible string has a vendor-prefix, and is documented somewhere in Documentation/devicetree/bindings. e.g. you could allocate something like: "qcom,msm8998-clamshell-hid-over-i2c" Thanks, Mark.
On Fri, Apr 12, 2019 at 9:30 AM Mark Rutland <mark.rutland@arm.com> wrote: > > On Fri, Apr 12, 2019 at 09:19:18AM -0600, Jeffrey Hugo wrote: > > > > + keyboard@3a { > > > > + /* QTEC0001 is the ACPI HID, which could be used for quirks */ > > > > + compatible = "QTEC0001", "hid-over-i2c"; > > > > > > As mentioned last time, please drop the ACPI HID, and allocate a real > > > compatible string. > > > > So, I'm in a quandary with this device. As far as I can tell, its an > > off the shelf component, the device adheres to the "PNP0C50" spec (HID > > over I2C), and can be driven by the full "hid-over-i2c" driver (which > > is just a DT shim over the PNP0C50 ACPI driver). However, the device > > itself identifies itself as an ELAN 400 device, which is an ID that is > > also used for standalone touchpad devices. Per my understanding of > > the Linux drivers, there is a separate ELAN driver for the standalone > > touchpad devices as its been discovered though trial and error that > > the Linux PNP0C50 driver cannot drive those devices. To handle this, > > there is a quirk in hid-quirks which rejects ELAN 400 devices, except > > those which are "QTEC0001". > > > > We need that quirk bypass for this device because the ELAN driver > > cannot handle this device. > > This is useful context; thanks for writing this up! > > > I'd much rather have a single identifier to quirk on, rather than > > having one for DT and one for ACPI, and its not looking feasible to > > get the vendor to update the ACPI, so it seems like using the ACPI > > identifier is just simpler. > > > > So, if you want a different compatible string, I'll need to go put DT > > in a driver that is primarily ACPI. I'm not sure what the HID folks > > will think of that. > > My objection is that an ACPI HID is _not_ a DT compatible string, and > the two should be treated separately. Munging the two together opens the > door for other pain. > > The driver in question has a DT probe function, i2c_hid_of_probe, so > there's certainly a place to wire up that quirk. Except that isn't where HID quirks are handled. They are handled in drivers/hid/hid-quirks.c There is a table of vendor/device IDs of HIDs which need quirking. In hid_ignore(), there is provision to detect based on vendor/device and the device name, whether the device can be driven by the HID framework, or needs to be handled by a standalone driver. All the I2C HID DT driver is doing is filling in the vendor/device IDs (read from the hardware) into the HID framework struct. The device name seems to come from standard mechanisms. > > > I'll propose it, but what do you view is a > > "proper" compatible string? "ELAN0400-msm8998-clamshell"? > > A proper compatible string has a vendor-prefix, and is documented > somewhere in Documentation/devicetree/bindings. > > e.g. you could allocate something like: > > "qcom,msm8998-clamshell-hid-over-i2c" Ok, let me create a simple bindings file, propose to amend the hid-quirks to accept the DT string, and then have a v3 of the miix 630 dts all as one series to hopefully get everyone on the same page and happy with things.
On Thu 11 Apr 13:51 PDT 2019, Jeffrey Hugo wrote: > diff --git a/arch/arm64/boot/dts/qcom/msm8998-clam.dtsi b/arch/arm64/boot/dts/qcom/msm8998-clam.dtsi [..] > +&tlmm { > + gpio-reserved-ranges = <0 4>, <81 4>; Did you double check that you actually need these? (Iirc boot testing is enough) > + > + touchpad: touchpad { > + config { > + pins = "gpio123"; > + bias-pull-up; /* pull up */ > + }; > + }; > +}; [..] > diff --git a/arch/arm64/boot/dts/qcom/msm8998-lenovo-miix-630.dts b/arch/arm64/boot/dts/qcom/msm8998-lenovo-miix-630.dts [..] > +/ { > + model = "Lenovo Miix 630"; > + compatible = "qcom,msm8998-clam"; compatible = "lenovo,miix-630", "qcom,msm8998"; > + > + /* bootloader doesn't understand DT, so no qcom,board-id */ You can omit this comment, as I don't think people will miss these non-standard properties. Regards, Bjorn
On Fri, Apr 12, 2019 at 12:01 PM Bjorn Andersson <bjorn.andersson@linaro.org> wrote: > > On Thu 11 Apr 13:51 PDT 2019, Jeffrey Hugo wrote: > > diff --git a/arch/arm64/boot/dts/qcom/msm8998-clam.dtsi b/arch/arm64/boot/dts/qcom/msm8998-clam.dtsi > [..] > > +&tlmm { > > + gpio-reserved-ranges = <0 4>, <81 4>; > > Did you double check that you actually need these? (Iirc boot testing is > enough) Yes, still needed. > > > + > > + touchpad: touchpad { > > + config { > > + pins = "gpio123"; > > + bias-pull-up; /* pull up */ > > + }; > > + }; > > +}; > [..] > > diff --git a/arch/arm64/boot/dts/qcom/msm8998-lenovo-miix-630.dts b/arch/arm64/boot/dts/qcom/msm8998-lenovo-miix-630.dts > [..] > > +/ { > > + model = "Lenovo Miix 630"; > > + compatible = "qcom,msm8998-clam"; > > compatible = "lenovo,miix-630", "qcom,msm8998"; Thanks for the suggestion. Will do. > > > + > > + /* bootloader doesn't understand DT, so no qcom,board-id */ > > You can omit this comment, as I don't think people will miss these > non-standard properties. Makes sense. Will do.
diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile index 21d548f02d39..c3e4307bcbd4 100644 --- a/arch/arm64/boot/dts/qcom/Makefile +++ b/arch/arm64/boot/dts/qcom/Makefile @@ -6,6 +6,7 @@ dtb-$(CONFIG_ARCH_QCOM) += msm8916-mtp.dtb dtb-$(CONFIG_ARCH_QCOM) += msm8992-bullhead-rev-101.dtb dtb-$(CONFIG_ARCH_QCOM) += msm8994-angler-rev-101.dtb dtb-$(CONFIG_ARCH_QCOM) += msm8996-mtp.dtb +dtb-$(CONFIG_ARCH_QCOM) += msm8998-lenovo-miix-630.dtb dtb-$(CONFIG_ARCH_QCOM) += msm8998-mtp.dtb dtb-$(CONFIG_ARCH_QCOM) += sdm845-mtp.dtb dtb-$(CONFIG_ARCH_QCOM) += qcs404-evb-1000.dtb diff --git a/arch/arm64/boot/dts/qcom/msm8998-clam.dtsi b/arch/arm64/boot/dts/qcom/msm8998-clam.dtsi new file mode 100644 index 000000000000..db2d75fff897 --- /dev/null +++ b/arch/arm64/boot/dts/qcom/msm8998-clam.dtsi @@ -0,0 +1,278 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright (c) 2019, Jeffrey Hugo. All rights reserved. */ + +/* + * Common include for MSM8998 clamshell (clam) devices, ie the Lenovo Miix 630, + * Asus NovaGo TP370QL, and HP Envy x2. All three devices are basically the + * same, with differences in peripherals. + */ + +#include "msm8998.dtsi" +#include "pm8998.dtsi" +#include "pm8005.dtsi" + +/ { + chosen { + }; + + thermal-zones { + battery-thermal { + polling-delay-passive = <250>; + polling-delay = <1000>; + + thermal-sensors = <&tsens0 0>; + + trips { + battery_crit: trip0 { + temperature = <60000>; + hysteresis = <2000>; + type = "critical"; + }; + }; + }; + + skin-thermal { + polling-delay-passive = <250>; + polling-delay = <1000>; + + thermal-sensors = <&tsens1 5>; + + trips { + skin_alert: trip0 { + temperature = <44000>; + hysteresis = <2000>; + type = "passive"; + }; + + skip_crit: trip1 { + temperature = <70000>; + hysteresis = <2000>; + type = "critical"; + }; + }; + }; + }; + + vph_pwr: vph-pwr-regulator { + compatible = "regulator-fixed"; + regulator-name = "vph_pwr"; + regulator-always-on; + regulator-boot-on; + }; +}; + +&qusb2phy { + status = "okay"; + + vdda-pll-supply = <&vreg_l12a_1p8>; + vdda-phy-dpdm-supply = <&vreg_l24a_3p075>; +}; + +&rpm_requests { + pm8998-regulators { + compatible = "qcom,rpm-pm8998-regulators"; + + vdd_s1-supply = <&vph_pwr>; + vdd_s2-supply = <&vph_pwr>; + vdd_s3-supply = <&vph_pwr>; + vdd_s4-supply = <&vph_pwr>; + vdd_s5-supply = <&vph_pwr>; + vdd_s6-supply = <&vph_pwr>; + vdd_s7-supply = <&vph_pwr>; + vdd_s8-supply = <&vph_pwr>; + vdd_s9-supply = <&vph_pwr>; + vdd_s10-supply = <&vph_pwr>; + vdd_s11-supply = <&vph_pwr>; + vdd_s12-supply = <&vph_pwr>; + vdd_s13-supply = <&vph_pwr>; + vdd_l1_l27-supply = <&vreg_s7a_1p025>; + vdd_l2_l8_l17-supply = <&vreg_s3a_1p35>; + vdd_l3_l11-supply = <&vreg_s7a_1p025>; + vdd_l4_l5-supply = <&vreg_s7a_1p025>; + vdd_l6-supply = <&vreg_s5a_2p04>; + vdd_l7_l12_l14_l15-supply = <&vreg_s5a_2p04>; + vdd_l9-supply = <&vph_pwr>; + vdd_l10_l23_l25-supply = <&vph_pwr>; + vdd_l13_l19_l21-supply = <&vph_pwr>; + vdd_l16_l28-supply = <&vph_pwr>; + vdd_l18_l22-supply = <&vph_pwr>; + vdd_l20_l24-supply = <&vph_pwr>; + vdd_l26-supply = <&vreg_s3a_1p35>; + vdd_lvs1_lvs2-supply = <&vreg_s4a_1p8>; + + vreg_s3a_1p35: s3 { + regulator-min-microvolt = <1352000>; + regulator-max-microvolt = <1352000>; + }; + vreg_s4a_1p8: s4 { + regulator-min-microvolt = <1800000>; + regulator-max-microvolt = <1800000>; + regulator-allow-set-load; + }; + vreg_s5a_2p04: s5 { + regulator-min-microvolt = <1904000>; + regulator-max-microvolt = <2040000>; + }; + vreg_s7a_1p025: s7 { + regulator-min-microvolt = <900000>; + regulator-max-microvolt = <1028000>; + }; + vreg_l1a_0p875: l1 { + regulator-min-microvolt = <880000>; + regulator-max-microvolt = <880000>; + regulator-allow-set-load; + }; + vreg_l2a_1p2: l2 { + regulator-min-microvolt = <1200000>; + regulator-max-microvolt = <1200000>; + regulator-allow-set-load; + }; + vreg_l3a_1p0: l3 { + regulator-min-microvolt = <1000000>; + regulator-max-microvolt = <1000000>; + }; + vreg_l5a_0p8: l5 { + regulator-min-microvolt = <800000>; + regulator-max-microvolt = <800000>; + }; + vreg_l6a_1p8: l6 { + regulator-min-microvolt = <1808000>; + regulator-max-microvolt = <1808000>; + }; + vreg_l7a_1p8: l7 { + regulator-min-microvolt = <1800000>; + regulator-max-microvolt = <1800000>; + }; + vreg_l8a_1p2: l8 { + regulator-min-microvolt = <1200000>; + regulator-max-microvolt = <1200000>; + }; + vreg_l9a_1p8: l9 { + regulator-min-microvolt = <1808000>; + regulator-max-microvolt = <2960000>; + }; + vreg_l10a_1p8: l10 { + regulator-min-microvolt = <1808000>; + regulator-max-microvolt = <2960000>; + }; + vreg_l11a_1p0: l11 { + regulator-min-microvolt = <1000000>; + regulator-max-microvolt = <1000000>; + }; + vreg_l12a_1p8: l12 { + regulator-min-microvolt = <1800000>; + regulator-max-microvolt = <1800000>; + }; + vreg_l13a_2p95: l13 { + regulator-min-microvolt = <1808000>; + regulator-max-microvolt = <2960000>; + }; + vreg_l14a_1p88: l14 { + regulator-min-microvolt = <1880000>; + regulator-max-microvolt = <1880000>; + }; + vreg_15a_1p8: l15 { + regulator-min-microvolt = <1800000>; + regulator-max-microvolt = <1800000>; + }; + vreg_l16a_2p7: l16 { + regulator-min-microvolt = <2704000>; + regulator-max-microvolt = <2704000>; + }; + vreg_l17a_1p3: l17 { + regulator-min-microvolt = <1304000>; + regulator-max-microvolt = <1304000>; + }; + vreg_l18a_2p7: l18 { + regulator-min-microvolt = <2704000>; + regulator-max-microvolt = <2704000>; + }; + vreg_l19a_3p0: l19 { + regulator-min-microvolt = <3008000>; + regulator-max-microvolt = <3008000>; + }; + vreg_l20a_2p95: l20 { + regulator-min-microvolt = <2960000>; + regulator-max-microvolt = <2960000>; + regulator-allow-set-load; + }; + vreg_l21a_2p95: l21 { + regulator-min-microvolt = <2960000>; + regulator-max-microvolt = <2960000>; + regulator-allow-set-load; + regulator-system-load = <800000>; + }; + vreg_l22a_2p85: l22 { + regulator-min-microvolt = <2864000>; + regulator-max-microvolt = <2864000>; + }; + vreg_l23a_3p3: l23 { + regulator-min-microvolt = <3312000>; + regulator-max-microvolt = <3312000>; + }; + vreg_l24a_3p075: l24 { + regulator-min-microvolt = <3088000>; + regulator-max-microvolt = <3088000>; + }; + vreg_l25a_3p3: l25 { + regulator-min-microvolt = <3104000>; + regulator-max-microvolt = <3312000>; + }; + vreg_l26a_1p2: l26 { + regulator-min-microvolt = <1200000>; + regulator-max-microvolt = <1200000>; + }; + vreg_l28_3p0: l28 { + regulator-min-microvolt = <3008000>; + regulator-max-microvolt = <3008000>; + }; + + vreg_lvs1a_1p8: lvs1 { + regulator-min-microvolt = <1800000>; + regulator-max-microvolt = <1800000>; + }; + + vreg_lvs2a_1p8: lvs2 { + regulator-min-microvolt = <1800000>; + regulator-max-microvolt = <1800000>; + }; + + }; +}; + +&tlmm { + gpio-reserved-ranges = <0 4>, <81 4>; + + touchpad: touchpad { + config { + pins = "gpio123"; + bias-pull-up; /* pull up */ + }; + }; +}; + +&sdhc2 { + status = "okay"; + + vmmc-supply = <&vreg_l21a_2p95>; + vqmmc-supply = <&vreg_l13a_2p95>; + + pinctrl-names = "default", "sleep"; + pinctrl-0 = <&sdc2_clk_on &sdc2_cmd_on &sdc2_data_on &sdc2_cd_on>; + pinctrl-1 = <&sdc2_clk_off &sdc2_cmd_off &sdc2_data_off &sdc2_cd_off>; +}; + +&usb3 { + status = "okay"; +}; + +&usb3_dwc3 { + dr_mode = "host"; /* Force to host until we have Type-C hooked up */ +}; + +&usb3phy { + status = "okay"; + + vdda-phy-supply = <&vreg_l1a_0p875>; + vdda-pll-supply = <&vreg_l2a_1p2>; +}; diff --git a/arch/arm64/boot/dts/qcom/msm8998-lenovo-miix-630.dts b/arch/arm64/boot/dts/qcom/msm8998-lenovo-miix-630.dts new file mode 100644 index 000000000000..eaca07fe2574 --- /dev/null +++ b/arch/arm64/boot/dts/qcom/msm8998-lenovo-miix-630.dts @@ -0,0 +1,33 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright (c) 2019, Jeffrey Hugo. All rights reserved. */ + +/dts-v1/; + +#include "msm8998-clam.dtsi" + +/ { + model = "Lenovo Miix 630"; + compatible = "qcom,msm8998-clam"; + + /* bootloader doesn't understand DT, so no qcom,board-id */ +}; + +&blsp1_i2c6 { + status = "okay"; + + keyboard@3a { + /* QTEC0001 is the ACPI HID, which could be used for quirks */ + compatible = "QTEC0001", "hid-over-i2c"; + interrupt-parent = <&tlmm>; + interrupts = <0x79 IRQ_TYPE_LEVEL_LOW>; + reg = <0x3a>; + hid-descr-addr = <0x0001>; + + pinctrl-names = "default"; + pinctrl-0 = <&touchpad>; + }; +}; + +&sdhc2 { + cd-gpios = <&tlmm 95 GPIO_ACTIVE_HIGH>; +};
This adds the initial DT for the Lenovo Miix 630 laptop. Supported functionality includes USB (host), microSD-card, keyboard, and trackpad. Signed-off-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com> --- v2: -Changed "cls" to "clam" since feedback indicated "cls" is too opaque, but "clamshell" is a mouthfull. "clam" seems to be a happy medium. arch/arm64/boot/dts/qcom/Makefile | 1 + arch/arm64/boot/dts/qcom/msm8998-clam.dtsi | 278 ++++++++++++++++++ .../boot/dts/qcom/msm8998-lenovo-miix-630.dts | 33 +++ 3 files changed, 312 insertions(+) create mode 100644 arch/arm64/boot/dts/qcom/msm8998-clam.dtsi create mode 100644 arch/arm64/boot/dts/qcom/msm8998-lenovo-miix-630.dts