Re: [PATCH v2 8/8] arm64: dts: qcom: add OnePlus 8T (kebab)

On 30.06.2024 8:36 PM, Caleb Connolly wrote:

Initial support for USB, UFS, touchscreen, panel, wifi, and bluetooth.

Co-developed-by: Frieder Hannenheim <redacted>
Signed-off-by: Frieder Hannenheim <redacted>
Signed-off-by: Caleb Connolly <redacted>
---

[...]

+/delete-node/ &spss_mem;
+/delete-node/ &cdsp_secure_heap;
+

odd double newline

+
+/ {

[...]

+
+		framebuffer@9c000000 {
+			reg = <0 0x9c000000 0 0x2300000>;

0x0 for consistency

+			no-map;
+		};
+	};
+
+	panel_avdd_5p5: regulator-panel-avdd {
+		compatible = "regulator-fixed";
+		regulator-name = "panel_avdd_5p5";
+		regulator-min-microvolt = <5500000>;
+		regulator-max-microvolt = <5500000>;

Please unsqash these properties like in e.g. x1e80100-crd

+		regulator-enable-ramp-delay = <233>;
+		gpio = <&tlmm 61 GPIO_ACTIVE_HIGH>;
+		regulator-boot-on;
+		pinctrl-names = "default";
+		pinctrl-0 = <&panel_avdd_pins>;

property-n
property-names

(theres more occurences in this patch)

+		vreg_l11c_3p3: ldo11 {
+			regulator-name = "vreg_l11c_3p3";
+			regulator-min-microvolt = <2900000>;
+			regulator-max-microvolt = <3304000>;
+			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+			regulator-boot-on;
+			/* FIXME: we don't yet support power cycling the panel */
+			//regulator-always-on;

so should this be in or out?

[...]

+	/*
+	 * Pixelworks Iris 5 @ 26 (i3c) or 22 (i2c)
+	 * This is a co-processor for the display which needs to be
+	 * initialized along with the panel.
+	 */

yikes

+};
+
+&i2c15 {
+	status = "okay";
+
+	typec-mux@42 {
+		compatible = "fcs,fsa4480";
+		reg = <0x42>;
+
+		vcc-supply = <&vreg_s4a_1p8>;
+
+		orientation-switch;
+
+		/* Currently unsupported */
+		status = "disabled";

Any particular problems with it?

[...]

+		/*
+		 * FIXME: There is a bug somewhere in the display stack and it isn't
+		 * possible to get the panel to a working state after toggling reset.
+		 * At best it just shows one or more vertical red lines. So for now
+		 * let's skip the reset GPIO.
+		 */
+		// reset-gpios = <&tlmm 75 GPIO_ACTIVE_LOW>;

c++-style comments used not to be cool.. not sure what's the current policy

+
+		pinctrl-0 = <&panel_reset_pins &panel_vsync_pins &panel_vout_pins>;

should panel_vout_pins be modeled as a regulator?

[...]

+&pm8150_gpios {
+	/*
+	 * These are marked as reserved in downstream
+	 * with no description, without schematics we
+	 * don't know what the deal is here.
+	 */
+	gpio-reserved-ranges = <2 1>, <4 2>, <8 1>;

drivers/pinctrl/qcom/pinctrl-spmi-gpio.c

/* pm8150 has 10 GPIOs with holes on 2, 5, 7 and 8 */
{ .compatible = "qcom,pm8150-gpio", .data = (void *) 10 },

[...]

+&pon_resin {
+	status = "okay";
+
+	linux,code = <KEY_VOLUMEDOWN>;

status should go last

[...]

+&tlmm {
+	gpio-reserved-ranges = <28 4>, <40 4>;

Any chance you'd know what they're for?

[...]

+&usb_1_dwc3 {
+	dr_mode = "peripheral";

[...]

+
+	touchscreen@4b {
+		compatible = "syna,s3908";
+		reg = <0x4B>;

lowercase hex

+
+		pinctrl-0 = <&touch_irq_active &touch_rst_active>;
+		pinctrl-names = "default";
+
+		interrupts-extended = <&tlmm 39 0x2008>;

https://lore.kernel.org/linux-arm-msm/20240605160032.150587-1-krzysztof.kozlowski@linaro.org/ (local)

Konrad