Re: [PATCH v2 1/3] arm64: boot: dts: ti: k3-am68-sk-base-board: Add LP8733 and TPS6287 nodes

From: Krzysztof Kozlowski
Date: Mon May 27 2024 - 11:38:43 EST


On 27/05/2024 14:44, Neha Malcom Francis wrote:
> Add DTS node for LP87334E PMIC and two TPS6287x high current buck
> converters.
>
> LP87334E is responsible for supplying power to the MCU and MAIN domains
> as well as to LPDDR4. The two TPS6287x supply power to the MAIN
> domain for AVS and other core supplies.

Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching. For bindings, the preferred subjects are
explained here:
https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters

>
> Signed-off-by: Neha Malcom Francis <n-francis@xxxxxx>
> Link: https://www.ti.com/lit/pdf/slda060
> ---
> .../boot/dts/ti/k3-am68-sk-base-board.dts | 77 +++++++++++++++++++
> 1 file changed, 77 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/ti/k3-am68-sk-base-board.dts b/arch/arm64/boot/dts/ti/k3-am68-sk-base-board.dts
> index d743f023cdd9..74e59035013c 100644
> --- a/arch/arm64/boot/dts/ti/k3-am68-sk-base-board.dts
> +++ b/arch/arm64/boot/dts/ti/k3-am68-sk-base-board.dts
> @@ -414,6 +414,83 @@ &wkup_uart0 {
> pinctrl-0 = <&wkup_uart0_pins_default>;
> };
>
> +&wkup_i2c0 {
> + bootph-all;
> + status = "okay";

Please order the properties according to DTS coding style:
https://www.kernel.org/doc/html/latest/devicetree/bindings/dts-coding-style.html#order-of-properties-in-device-node

> + pinctrl-names = "default";
> + pinctrl-0 = <&wkup_i2c0_pins_default>;
> + clock-frequency = <400000>;
> +
> + lp8733: pmic@60 {
> + compatible = "ti,lp8733";
> + reg = <0x60>;
> +
> + buck0-in-supply = <&vsys_3v3>;
> + buck1-in-supply = <&vsys_3v3>;
> + ldo0-in-supply = <&vsys_3v3>;
> + ldo1-in-supply = <&vsys_3v3>;
> +
> + lp8733_regulators: regulators {
> + lp8733_buck0_reg: buck0 {
> + /* FB_B0 -> LP8733-BUCK1 - VDD_MCU_0V85 */
> + regulator-name = "lp8733-buck0";
> + regulator-min-microvolt = <850000>;
> + regulator-max-microvolt = <850000>;
> + regulator-always-on;
> + regulator-boot-on;
> + };
> +
> + lp8733_buck1_reg: buck1 {
> + /* FB_B1 -> LP8733-BUCK2 - VDD_DDR_1V1 */
> + regulator-name = "lp8733-buck1";
> + regulator-min-microvolt = <1100000>;
> + regulator-max-microvolt = <1100000>;
> + regulator-always-on;
> + regulator-boot-on;
> + };
> +
> + lp8733_ldo0_reg: ldo0 {
> + /* LDO0 -> LP8733-LDO1 - VDA_DLL_0V8 */
> + regulator-name = "lp8733-ldo0";
> + regulator-min-microvolt = <800000>;
> + regulator-max-microvolt = <800000>;
> + regulator-boot-on;
> + regulator-always-on;
> + };
> +
> + lp8733_ldo1_reg: ldo1 {
> + /* LDO1 -> LP8733-LDO2 - VDA_LN_1V8 */
> + regulator-name = "lp8733-ldo1";
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <1800000>;
> + regulator-always-on;
> + regulator-boot-on;
> + };
> + };
> + };
> +
> + tps62873a: tps62873@40 {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation


> + compatible = "ti,tps62873";
> + bootph-pre-ram;
> + reg = <0x40>;
> + regulator-name = "VDD_CPU_AVS";
> + regulator-min-microvolt = <600000>;
> + regulator-max-microvolt = <900000>;
> + regulator-boot-on;
> + regulator-always-on;
> + };
> +
> + tps62873b: tps62873@43 {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation




Best regards,
Krzysztof