On 18/07/2022 23:30, Caleb Connolly wrote: > From: Amit Pundir <amit.pundir@xxxxxxxxxx> > > This adds an initial dts for the Blueline (Pixel 3). Supported > functionality includes display, Debug UART, UFS, USB-C (peripheral), WiFi, > Bluetooth and modem. > Thank you for your patch. There is something to discuss/improve. (...) > + volume-keys { > + compatible = "gpio-keys"; > + label = "Volume keys"; > + autorepeat; > + > + pinctrl-names = "default"; > + pinctrl-0 = <&volume_up_gpio>; > + > + vol-up { key-vol-up (DT schema requires it now) > + label = "Volume Up"; > + linux,code = <KEY_VOLUMEUP>; > + gpios = <&pm8998_gpio 6 GPIO_ACTIVE_LOW>; > + debounce-interval = <15>; > + }; > + }; > + > + reserved-memory { > + #address-cells = <2>; > + #size-cells = <2>; > + ranges; > + > + mpss_region: memory@8e000000 { > + reg = <0 0x8e000000 0 0x9800000>; > + no-map; > + }; > + > + venus_mem: venus@97800000 { > + reg = <0 0x97800000 0 0x500000>; > + no-map; > + }; > + > + cdsp_mem: cdsp-mem@97D00000 { > + reg = <0 0x97D00000 0 0x800000>; > + no-map; > + }; > + > + mba_region: mba@98500000 { > + reg = <0 0x98500000 0 0x200000>; > + no-map; > + }; > + > + slpi_mem: slpi@98700000 { > + reg = <0 0x98700000 0 0x1400000>; > + no-map; > + }; > + > + spss_mem: spss@99B00000 { > + reg = <0 0x99B00000 0 0x100000>; > + no-map; > + }; > + > + /* rmtfs lower guard */ > + memory@f2700000 { > + reg = <0 0xf2700000 0 0x1000>; > + no-map; > + }; > + > + rmtfs_mem: memory@f2701000 { > + compatible = "qcom,rmtfs-mem"; > + reg = <0 0xf2701000 0 0x200000>; > + no-map; > + > + qcom,client-id = <1>; > + qcom,vmid = <15>; > + }; > + > + /* rmtfs upper guard */ > + memory@f2901000 { > + reg = <0 0xf2901000 0 0x1000>; > + no-map; > + }; > + }; > + > + vph_pwr: vph-pwr-regulator { > + compatible = "regulator-fixed"; > + regulator-name = "vph_pwr"; > + regulator-min-microvolt = <3700000>; > + regulator-max-microvolt = <3700000>; > + }; > + > + vreg_s4a_1p8: vreg-s4a-1p8 { Please use consistent naming, so if previous was "xxx-regulator", keep similar pattern here. > + compatible = "regulator-fixed"; > + regulator-name = "vreg_s4a_1p8"; > + > + regulator-min-microvolt = <1800000>; > + regulator-max-microvolt = <1800000>; > + regulator-always-on; > + regulator-boot-on; > + > + vin-supply = <&vph_pwr>; > + }; > +}; > + > +&adsp_pas { (...) > + > +&pm8998_gpio { > + volume_up_gpio: vol-up-active { The bindings require node name to finish with "-state" > + pins = "gpio6"; > + function = "normal"; > + input-enable; > + bias-pull-up; > + qcom,drive-strength = <0>; > + }; > + > + panel_pmgpio_pins: panel-pmgpio-active { Ditto. > + pins = "gpio2", "gpio5"; > + function = "normal"; > + input-enable; > + bias-disable; > + power-source = <0>; > + }; > +}; > + > +&pm8998_pon { > + resin { > + compatible = "qcom,pm8941-resin"; > + interrupts = <0x0 0x8 1 IRQ_TYPE_EDGE_BOTH>; > + debounce = <15625>; > + bias-pull-up; > + linux,code = <KEY_VOLUMEDOWN>; > + }; > +}; > + Best regards, Krzysztof