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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] dts: qcom: duplicate KEY_VOLUMEDOWN mapping in pon_resin and gpio-keys
- [Low] dts: qcom: status='disabled' has no effect on pinctrl state nodes
--

commit fdf402f94fd160b62e640439999822bae70298f5
Author: Frieder Hannenheim <git@fhannenheim.net>

arm64: dts: qcom: Add sm8250-oneplus-common.dtsi

This commit adds common properties shared by all OnePlus devices using
the Qualcomm sm8250 chipset.

> diff --git a/arch/arm64/boot/dts/qcom/sm8250-oneplus-common.dtsi b/arch/arm64/boot/dts/qcom/sm8250-oneplus-common.dtsi
> index 0000000000000..59f4a3c119c8f
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/sm8250-oneplus-common.dtsi
[ ... ]
> +	gpio_keys: gpio-keys {
> +		compatible = "gpio-keys";
> +		label = "Volume keys";
> +		autorepeat;
> +
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&volume_up_gpio &volume_down_gpio>;
> +
> +		vol-up {
> +			label = "Volume Up";
> +			gpios = <&pm8150_gpios 6 GPIO_ACTIVE_LOW>;
> +			linux,code = <KEY_VOLUMEUP>;
> +			debounce-interval = <15>;
> +			linux,can-disable;
> +			wakeup-source;
> +		};
> +
> +		vol-down {
> +			label = "Volume Down";
> +			linux,code = <KEY_VOLUMEDOWN>;
> +			gpios = <&pm8150_gpios 7 GPIO_ACTIVE_LOW>;
> +			debounce-interval = <15>;
> +			linux,can-disable;
> +			wakeup-source;
> +		};
> +	};
[ ... ]
> +&pon_resin {
> +	linux,code = <KEY_VOLUMEDOWN>;
> +
> +	status = "okay";
> +};

[Severity: Medium]
Will mapping KEY_VOLUMEDOWN here in the pon_resin node in addition to the
gpio-keys node above cause double key presses?

Because the physical volume down button on Qualcomm devices is typically
wired to both the pmic RESIN_N pin and a pmic gpio, mapping both paths
can cause the kernel to register two input devices. This can emit duplicate
input events for a single physical button press, resulting in double volume
steps in userspace.

[ ... ]
> +&qup_i2c7_default {
> +	status = "disabled";
> +};
> +
> +&qup_spi7_data_clk {
> +	status = "disabled";
> +};

[Severity: Low]
Does setting the status property on pinctrl configuration state nodes like
qup_i2c7_default and qup_spi7_data_clk have any effect?

The pinctrl subsystem generally does not process the status property on
configuration nodes, making these assignments ineffective. If the intention
was to disable the i2c and spi interfaces, should the property be applied
to the device controller nodes themselves (for instance, i2c7) instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015-kebab_dts-v1-0-1fee1204a18b@fhannenheim.net?part=1