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

[Konrad Dybcio <konrad.dybcio@linaro.org>]

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 <frieder.hannenheim@proton.me>
> Signed-off-by: Frieder Hannenheim <frieder.hannenheim@proton.me>
> Signed-off-by: Caleb Connolly <caleb@postmarketos.org>
> ---

[...]

> +/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/

Konrad