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.hannenh...@proton.me>
> Signed-off-by: Frieder Hannenheim <frieder.hannenh...@proton.me>
> Signed-off-by: Caleb Connolly <ca...@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.kozlow...@linaro.org/

Konrad

Reply via email to