| Message ID | 20230916134147.163764-2-lukas.walter@aceart.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] dt-bindings: arm: qcom: Add Huawei Honor 5X / GR5 (2016) | expand |
On 16/09/2023 15:41, Lukas Walter wrote: > This dts adds support for Huawei Honor 5X / GR5 (2016) smartphone > released in 2015. > > Add device tree with initial support for: > > - GPIO keys > - Hall sensor > - SDHCI (internal and external storage) > - WCNSS (BT/WIFI) > - Sensors (accelerometer, proximity and gyroscope) > - Vibrator > - Touchscreen > > Signed-off-by: Lukas Walter <lukas.walter@aceart.de> > Signed-off-by: Raymond Hackley <raymondhackley@protonmail.com> Order of SoB is unusual. Who did what here? Best regards, Krzysztof
On 16/09/2023 14:41, Lukas Walter wrote:
> + +&wcnss_iris { + compatible = "qcom,wcn3620"; +}; +
Are you sure this is 3620, have you tried wcn3660 and/or wcn3680 ?
---
bod
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
>Order of SoB is unusual. Who did what here?
I created the dts and they update the model name. So it should be the
other way around
-----BEGIN PGP SIGNATURE-----
iQIzBAEBCAAdFiEEi1ngOOsyNO1iyMXiY16HCsLx2zUFAmUG/FQACgkQY16HCsLx
2zXbug//cFZNScSBr6k1I4tHuthR2+CFhTqOO0x0EqMa6W+l96s7ZimJA8UGq2jJ
F+4BZHujtBlpd1p4bE/GMDERy6MMHmAW7/N8kl3dEfwErwvjwML4nvDLRSC2Movd
93MNJ+4tncTndMLUEyARUUg5sP6Idgc/83pMRzKn4toFxWMHfWPFHy5XlNLDjJT5
/BuvYlbAFpme9ZavTLMGtalOp6ovsS1qNRrNk/CZecV+I5tCVFs4ZbzMHf8vKi3/
B7yObHRsZEdy80KE77kedgyBlGWu0/Tvu7xrK2qZxawkbraLBn+qE76wCmgk91qg
7EheFFr30v1LuudZbA+5fOE3qC13u3LbmCaRwwKAC8S7C6OgMM8Vcg08rPdaaV10
m1MDSPgxddA/TitQNz3epvFahpm1WWuGhR2xihDP/r345kogUYcRiLw4eqIkYKjb
64Wgalclq8XByymGLndYuoBaR31wauOjCtShiHByRTp86XTWh9C+KJ2wVGdE2UWL
ok+qs1hrao328+FtgtZn7FxnmO2yLA9I6xLeEPgTSt5VSZ9NWpRNQ5h61AYDEaZV
YPy3cTJV5VN5Mrp6dul7bT/Ti5rp8tCH5zk6D62WTSq2xEzkDtjnE1DKzr2+0KNt
c2VZQGmquTiTotnQF2c4E2M9ONd+TxN9sn0dYqwwZa0BLEI1JBk=
=F2cY
-----END PGP SIGNATURE-----
>Are you sure this is 3620, have you tried wcn3660 and/or wcn3680 ?
I am sure. Downstream source [1] and downstream dmesg (wcnss: IRIS Reg:
51120004 which should equal [2]) indicate 3620 (3620A does not exist)
[1]:
https://github.com/CyanogenMod/android_kernel_huawei_kiwi/blob/cm-14.1/arch/arm/boot/dts/qcom/huawei_msm8939_kiw_al20_vb/huawei-bt.dtsi#L5
[2]:
https://github.com/msm8916-mainline/linux-downstream/blob/b20608408caff817ec874f325127b07609fbaeb8/drivers/net/wireless/wcnss/wcnss_vreg.c#L51
On 9/16/23 15:41, Lukas Walter wrote: > This dts adds support for Huawei Honor 5X / GR5 (2016) smartphone > released in 2015. > > Add device tree with initial support for: > > - GPIO keys > - Hall sensor > - SDHCI (internal and external storage) > - WCNSS (BT/WIFI) > - Sensors (accelerometer, proximity and gyroscope) > - Vibrator > - Touchscreen > > Signed-off-by: Lukas Walter <lukas.walter@aceart.de> > Signed-off-by: Raymond Hackley <raymondhackley@protonmail.com> > --- Beyond the signoff question from Krzysztof, this looks really good. Some comments below. [...] > + > + reserved-memory { > + reserved@84a00000 { > + reg = <0x0 0x84a00000 0x0 0x1600000>; > + no-map; > + }; Do we know what this is for? > + }; > + > + gpio-hall-sensor { > + compatible = "gpio-keys"; > + > + pinctrl-0 = <&gpio_hall_sensor_default>; > + pinctrl-names = "default"; > + > + label = "GPIO Hall Effect Sensor"; I think we can have both hall sensor and V+ under gpio-keys And then I am not sure how useful the label is for the container node, maybe you or somebody else can tell me whether it's used anywhere > + > + event-hall-sensor { > + label = "Hall Effect Sensor"; > + gpios = <&tlmm 69 GPIO_ACTIVE_LOW>; > + linux,input-type = <EV_SW>; > + linux,code = <SW_LID>; > + linux,can-disable; Should this not be a wakeup-source btw? > + }; > + }; > + [...] > + /* > + * NOTE: vdd is not directly supplied by pm8916_l16, it seems to be a > + * fixed regulator that is automatically enabled by pm8916_l16. That sounds reasonable, many boards have such circuits Konrad
Date: Wed, 20 Sep 2023 16:47:30 +0200 >> + >> + reserved-memory { >> + reserved@84a00000 { >> + reg = <0x0 0x84a00000 0x0 0x1600000>; >> + no-map; >> + }; >Do we know what this is for? This seems to be some QSEE/TrustZone memory required to boot. I would name it `qseecom_mem: qseecom@84a00000` like other phones currently have it. `[ 1.162115] QSEECOM: qseecom_probe: secure app region addr=0x84a00000 size=0x1900000` >> + }; >> + >> + gpio-hall-sensor { >> + compatible = "gpio-keys"; >> + >> + pinctrl-0 = <&gpio_hall_sensor_default>; >> + pinctrl-names = "default"; >> + >> + label = "GPIO Hall Effect Sensor"; >I think we can have both hall sensor and V+ under gpio-keys > >And then I am not sure how useful the label is for the container >node, maybe you or somebody else can tell me whether it's used >anywhere >> + >> + event-hall-sensor { >> + label = "Hall Effect Sensor"; >> + gpios = <&tlmm 69 GPIO_ACTIVE_LOW>; >> + linux,input-type = <EV_SW>; >> + linux,code = <SW_LID>; >> + linux,can-disable; >Should this not be a wakeup-source btw? I am not sure how to change this. I would like to leave this as many other hall sensors seem to be configured identically. Is this fine? Should I send a V2 with the signoff and reserved-memory changes?
On 25.09.2023 16:28, lukas walter wrote: > Date: Wed, 20 Sep 2023 16:47:30 +0200 > >>> + >>> + reserved-memory { >>> + reserved@84a00000 { >>> + reg = <0x0 0x84a00000 0x0 0x1600000>; >>> + no-map; >>> + }; >> Do we know what this is for? > > This seems to be some QSEE/TrustZone memory required to boot. > I would name it `qseecom_mem: qseecom@84a00000` like other phones > currently have it. > > `[ 1.162115] QSEECOM: qseecom_probe: secure app region > addr=0x84a00000 size=0x1900000` Sounds good! > >>> + }; >>> + >>> + gpio-hall-sensor { >>> + compatible = "gpio-keys"; >>> + >>> + pinctrl-0 = <&gpio_hall_sensor_default>; >>> + pinctrl-names = "default"; >>> + >>> + label = "GPIO Hall Effect Sensor"; >> I think we can have both hall sensor and V+ under gpio-keys >> >> And then I am not sure how useful the label is for the container >> node, maybe you or somebody else can tell me whether it's used >> anywhere >>> + >>> + event-hall-sensor { >>> + label = "Hall Effect Sensor"; >>> + gpios = <&tlmm 69 GPIO_ACTIVE_LOW>; >>> + linux,input-type = <EV_SW>; >>> + linux,code = <SW_LID>; >>> + linux,can-disable; >> Should this not be a wakeup-source btw? > > I am not sure how to change this. I would like to leave this as many > other hall sensors seem to be configured identically. Krzysztof, opinions? > > Is this fine? > Should I send a V2 with the signoff and reserved-memory changes? I don't quite get it, what signoff? Konrad
>> Is this fine? >> Should I send a V2 with the signoff and reserved-memory changes? >I don't quite get it, what signoff? Krzysztof noticed that the Signed-off-by are in the wrong order. But I was told this alone is not worth a V2.
diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile index 2cca20563a1d..be9781c8e693 100644 --- a/arch/arm64/boot/dts/qcom/Makefile +++ b/arch/arm64/boot/dts/qcom/Makefile @@ -41,6 +41,7 @@ dtb-$(CONFIG_ARCH_QCOM) += msm8916-thwc-uf896.dtb dtb-$(CONFIG_ARCH_QCOM) += msm8916-thwc-ufi001c.dtb dtb-$(CONFIG_ARCH_QCOM) += msm8916-wingtech-wt88047.dtb dtb-$(CONFIG_ARCH_QCOM) += msm8916-yiming-uz801v3.dtb +dtb-$(CONFIG_ARCH_QCOM) += msm8939-huawei-kiwi.dtb dtb-$(CONFIG_ARCH_QCOM) += msm8939-samsung-a7.dtb dtb-$(CONFIG_ARCH_QCOM) += msm8939-sony-xperia-kanuti-tulip.dtb dtb-$(CONFIG_ARCH_QCOM) += msm8953-motorola-potter.dtb diff --git a/arch/arm64/boot/dts/qcom/msm8939-huawei-kiwi.dts b/arch/arm64/boot/dts/qcom/msm8939-huawei-kiwi.dts new file mode 100644 index 000000000000..19bd95233774 --- /dev/null +++ b/arch/arm64/boot/dts/qcom/msm8939-huawei-kiwi.dts @@ -0,0 +1,238 @@ +// SPDX-License-Identifier: GPL-2.0-only + +/dts-v1/; + +#include "msm8939-pm8916.dtsi" +#include <dt-bindings/gpio/gpio.h> + +/ { + model = "Huawei Honor 5X / GR5 (2016)"; + compatible = "huawei,kiwi", "qcom,msm8939"; + chassis-type = "handset"; + + aliases { + mmc0 = &sdhc_1; /* SDC1 eMMC slot */ + mmc1 = &sdhc_2; /* SDC2 SD card slot */ + serial0 = &blsp_uart2; + }; + + chosen { + stdout-path = "serial0"; + }; + + reserved-memory { + reserved@84a00000 { + reg = <0x0 0x84a00000 0x0 0x1600000>; + no-map; + }; + }; + + gpio-hall-sensor { + compatible = "gpio-keys"; + + pinctrl-0 = <&gpio_hall_sensor_default>; + pinctrl-names = "default"; + + label = "GPIO Hall Effect Sensor"; + + event-hall-sensor { + label = "Hall Effect Sensor"; + gpios = <&tlmm 69 GPIO_ACTIVE_LOW>; + linux,input-type = <EV_SW>; + linux,code = <SW_LID>; + linux,can-disable; + }; + }; + + gpio-keys { + compatible = "gpio-keys"; + + pinctrl-0 = <&gpio_keys_default>; + pinctrl-names = "default"; + + label = "GPIO Buttons"; + + button-volume-up { + label = "Volume Up"; + gpios = <&tlmm 107 GPIO_ACTIVE_LOW>; + linux,code = <KEY_VOLUMEUP>; + }; + }; + + usb_id: usb-id { + compatible = "linux,extcon-usb-gpio"; + id-gpios = <&tlmm 110 GPIO_ACTIVE_HIGH>; + pinctrl-0 = <&usb_id_default>; + pinctrl-names = "default"; + }; +}; + +&blsp_i2c2 { + status = "okay"; + + accelerometer@1e { + compatible = "kionix,kx023-1025"; + reg = <0x1e>; + + vdd-supply = <&pm8916_l17>; + vddio-supply = <&pm8916_l6>; + pinctrl-0 = <&accel_int_default>; + pinctrl-names = "default"; + mount-matrix = "-1", "0", "0", + "0", "1", "0", + "0", "0", "1"; + }; + + proximity@39 { + compatible = "avago,apds9930"; + reg = <0x39>; + + interrupt-parent = <&tlmm>; + interrupts = <113 IRQ_TYPE_EDGE_FALLING>; + + vdd-supply = <&pm8916_l17>; + vddio-supply = <&pm8916_l6>; + + led-max-microamp = <25000>; + amstaos,proximity-diodes = <0>; + + pinctrl-0 = <&prox_irq_default>; + pinctrl-names = "default"; + }; +}; + +&blsp_i2c5 { + status = "okay"; + + touchscreen@1c { + compatible = "cypress,tt21000"; + + reg = <0x1c>; + interrupt-parent = <&tlmm>; + interrupts = <13 IRQ_TYPE_EDGE_FALLING>; + + reset-gpios = <&tlmm 12 GPIO_ACTIVE_LOW>; + + /* + * NOTE: vdd is not directly supplied by pm8916_l16, it seems to be a + * fixed regulator that is automatically enabled by pm8916_l16. + */ + vdd-supply = <&pm8916_l16>; + vddio-supply = <&pm8916_l16>; + + pinctrl-0 = <&touchscreen_default>; + pinctrl-names = "default"; + }; +}; + +&blsp_uart2 { + status = "okay"; +}; + +&pm8916_l8 { + regulator-min-microvolt = <2950000>; + regulator-max-microvolt = <2950000>; +}; + +&pm8916_resin { + linux,code = <KEY_VOLUMEDOWN>; + status = "okay"; +}; + +&pm8916_rpm_regulators { + pm8916_l16: l16 { + regulator-min-microvolt = <1800000>; + regulator-max-microvolt = <1800000>; + }; + + pm8916_l17: l17 { + regulator-min-microvolt = <2850000>; + regulator-max-microvolt = <2850000>; + }; +}; + +&pm8916_vib { + status = "okay"; +}; + +&sdhc_1 { + status = "okay"; +}; + +&sdhc_2 { + pinctrl-0 = <&sdc2_default &sdc2_cd_default>; + pinctrl-1 = <&sdc2_sleep &sdc2_cd_default>; + pinctrl-names = "default", "sleep"; + + cd-gpios = <&tlmm 38 GPIO_ACTIVE_HIGH>; + + status = "okay"; +}; + +&usb { + extcon = <&usb_id>, <&usb_id>; + status = "okay"; +}; + +&usb_hs_phy { + extcon = <&usb_id>; +}; + +&wcnss { + status = "okay"; +}; + +&wcnss_iris { + compatible = "qcom,wcn3620"; +}; + +&tlmm { + accel_int_default: accel-int-default-state { + pins = "gpio115"; + function = "gpio"; + drive-strength = <2>; + bias-disable; + }; + + gpio_hall_sensor_default: gpio-hall-sensor-default-state { + pins = "gpio69"; + function = "gpio"; + drive-strength = <2>; + bias-disable; + }; + + gpio_keys_default: gpio-keys-default-state { + pins = "gpio107"; + function = "gpio"; + drive-strength = <2>; + bias-pull-up; + }; + + prox_irq_default: prox-irq-default-state { + pins = "gpio113"; + function = "gpio"; + drive-strength = <2>; + bias-disable; + }; + + sdc2_cd_default: sdc2-cd-default-state { + pins = "gpio38"; + function = "gpio"; + drive-strength = <2>; + bias-disable; + }; + + touchscreen_default: touchscreen-default-state { + pins = "gpio12", "gpio13"; + function = "gpio"; + drive-strength = <2>; + bias-disable; + }; + + usb_id_default: usb-id-default-state { + pins = "gpio110"; + function = "gpio"; + drive-strength = <8>; + bias-pull-up; + }; +};