Re: [PATCH 2/2] arm64: dts: qcom: Add Motorola Moto G10

[Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>]

On 10/22/26 10:40 PM, Michael McAllister wrote:
> Add an initial board description for the SM4250-based Motorola Moto G10
> (codenamed capri), including eMMC, USB-C, buttons, GPU, Wi-Fi and
> Bluetooth. Use the firmware framebuffer until native panel support is
> available.

[...]

> +// SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)
> +/*
> + * Copyright (c) 2026 Michael McAllister <me@michaelmcallister.org>
> + *
> + * PM6125 regulator definitions derived from sm4250-oneplus-billie2.dts:
> + * Copyright (c) 2021 Iskren Chernev <iskren.chernev@gmail.com>

Did you adjust them to match your specific device? Otherwise you risk
some magic smoke..

> + */
> +
> +/dts-v1/;
> +
> +#include <dt-bindings/gpio/gpio.h>
> +#include <dt-bindings/input/input.h>
> +#include "sm4250.dtsi"
> +#include "pm6125.dtsi"
> +#include "pmi632.dtsi"
> +
> +/ {
> +	model = "Motorola Moto G10";
> +	compatible = "motorola,capri", "qcom,sm4250";
> +
> +	/* Used by the bootloader to select the board. */
> +	qcom,msm-id = <0x1a1 0x10000>;

Please add a line to socinfo (this is SM4250 as you said), include that
header and use the define here

[...]

> +/* The stock firmware uses CPU mode 3 and affinity level 1 for these states. */
> +&cluster_0_sleep_0 {
> +	arm,psci-suspend-param = <0x41000023>;
> +};
> +
> +&cluster_0_sleep_1 {
> +	arm,psci-suspend-param = <0x41000043>;
> +};
> +
> +&cluster_1_sleep_0 {
> +	arm,psci-suspend-param = <0x41000023>;
> +};
> +
> +&cluster_1_sleep_1 {
> +	arm,psci-suspend-param = <0x41000043>;
> +};

This isn't customized per-device. I've confirmed these values are
right, please update them in the SoC DTSI in a preceding patch.

We also have this state:

system_sleep_0: system-sleep-0 {
	compatible = "domain-idle-state";
	arm,psci-suspend-param = <0x42000343>;
	entry-latency-us = <10831>;
	exit-latency-us = <4506>;
	min-residency-us = <15338>;
};

but this is definitely going to stay pending on this series:

https://lore.kernel.org/linux-arm-msm/20261015-b4-shikra_lpm_addition-v2-0-ac7f1a0d5638@oss.qualcomm.com/

[...]

> +&reserved_memory {
> +	/delete-node/ memory@53800000;
> +	/delete-node/ memory@56100000;
> +	/delete-node/ memory@56110000;
> +	/delete-node/ memory@56115000;

Please remove these nodes via a &label_reference at the top of the
file instead, so the compiler will do some more checking

[...]

> +	/* Bootloader debug data. */

Ultra nit: usually comments don't end in a period

[...]

> +		vreg_l8a: l8 {
> +			/* Handset ICNSS DT overrides CX/MX to 640 mV. */

This is presumably CX/MX for the bluetooth chip?
I find this comment a bit more on the confusing side rather than helping

> +			regulator-min-microvolt = <640000>;
> +			regulator-max-microvolt = <640000>;
> +		};
> +
> +		vreg_l9a: l9 {
> +			regulator-min-microvolt = <1800000>;
> +			regulator-max-microvolt = <1800000>;
> +			/* Display and touch share this rail with Bluetooth. */

This will become self-evident as you add support for these pieces

[...]

> +		vreg_l16a: l16 {
> +			/* ICNSS WCN3990 XO supply setting. */

This comment feels likewise a little strange

[...]

> +&tlmm {
> +	gpio-reserved-ranges = <14 4>;

Do we know what these map to? Usually that's NFC eSE or a fingerprint
scanner. If you do, please add a comment, like in glymur-crd.dtsi

[...]

> +&usb_dwc3 {
> +	dr_mode = "otg";

OTG is the default, you can drop this line


> +	maximum-speed = "high-speed";
> +	phy-names = "usb2-phy";
> +	phys = <&usb_hsphy>;
> +	role-switch-default-mode = "peripheral";

And this is the default too

Konrad