Re: [PATCH v2 1/2] arm64: dts: qcom: Add sm8250-oneplus-common.dtsi

[Frieder Hannenheim]

On 16.10.26 10:25, Konrad Dybcio wrote:
> On 10/15/26 2:32 PM, Frieder Hannenheim wrote:
> > This file contains common properties shared by all OnePlus devices using
> > the Qualcomm sm8250 chipset.
> >
> > Signed-off-by: Frieder Hannenheim <git@xxxxxxxxxxxxxxx>
> > Co-developed-by: Robert Oscilowski <drgitx@xxxxxxxxx>
> > Signed-off-by: Robert Oscilowski <drgitx@xxxxxxxxx>
> > ---
>
> [...]
>
> > +/*
> > + * Delete following upstream (sm8250.dtsi) reserved
> > + * memory mappings which are different on this device.
> > + */
> > +/delete-node/ &removed_mem;
> > +/delete-node/ &camera_mem;
> > +/delete-node/ &wlan_mem;
> > +/delete-node/ &ipa_fw_mem;
> > +/delete-node/ &ipa_gsi_mem;
> > +/delete-node/ &gpu_mem;
> > +/delete-node/ &npu_mem;
> > +/delete-node/ &video_mem;
> > +/delete-node/ &cvp_mem;
> > +/delete-node/ &cdsp_mem;
> > +/delete-node/ &slpi_mem;
> > +/delete-node/ &adsp_mem;
> > +/delete-node/ &spss_mem;
> > +/delete-node/ &cdsp_secure_heap;
>
> Let's sort this list

Will do

> > +
> > +/ {
> > +	chassis-type = "handset";
> > +	qcom,msm-id = <QCOM_ID_SM8250 0x10000>,
>
> You can drop this first entry, it's for SM8250v1, which is a prototype revision

Will do

> > +		      <QCOM_ID_SM8250 0x20001>;
> > +	qcom,board-id = <QCOM_BOARD_ID_MTP 0x0>,
> > +			<0x0 0x0>;
>
> Likewise you should be able to drop the second 0/0 entry as it shouldn't be
> meaningful

Will do

> [...]
>
> > +	gpio_keys: gpio-keys {
> > +		compatible = "gpio-keys";
> > +		label = "Volume keys";
> > +
> > +		pinctrl-names = "default";
> > +		pinctrl-0 = <&vol_up_n &vol_down_n>;
>
> property-n
> property-names, please
>
> also <&vol_up_n &vol_down_n> -> <&vol_up_n>, <&vol_down_l>;

Will do

> > +
> > +		key-vol-up {
> > +			label = "Volume Up";
> > +			gpios = <&pm8150_gpios 6 GPIO_ACTIVE_LOW>;
> > +			linux,code = <KEY_VOLUMEUP>;
> > +			debounce-interval = <15>;
> > +			linux,can-disable;
> > +			wakeup-source;
> > +		};
> > +
> > +		key-vol-down {
>
> Let's keep the nodes sorted by their node names

Will do

> [...]
>
>
> > +		ramoops: ramoops@b0000000 {
> > +			compatible = "ramoops";
> > +			reg = <0x0 0xb0000000 0x0 0x400000>;
> > +			record-size = <0x40000>;
> > +			console-size = <0x40000>;
> > +			ftrace-size =  <0x40000>;
> > +			pmsg-size = <0x200000>;
> > +			ecc-size = <0x0>;
>
> The zero is the default, you can drop it

Will do

> [...]
>
> > +&tlmm {
> > +	gpio-reserved-ranges = <28 4>, <40 4>;
>
> Do we know what these are connected to? Usually it'd be the fingerprint
> scanner and NFC eSE

Yes that's correct. I will add a comment in the next rev.

> Konrad