Received-SPF: pass (google.com: domain of linux-kernel+bounces-191727-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <046f78a5-e235-4716-8738-7a529353481a@ti.com>
Date: Tue, 28 May 2024 09:32:54 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 1/3] arm64: boot: dts: ti: k3-am68-sk-base-board: Add
 LP8733 and TPS6287 nodes
To: Krzysztof Kozlowski <krzk@kernel.org>, <robh@kernel.org>,
        <conor+dt@kernel.org>, <krzk+dt@kernel.org>, <kristo@kernel.org>,
        <vigneshr@ti.com>, <nm@ti.com>, <broonie@kernel.org>,
        <lgirdwood@gmail.com>
CC: <marten.lindahl@axis.com>, <linux-arm-kernel@lists.infradead.org>,
        <devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <u-kumar1@ti.com>
References: <20240527124422.3553828-1-n-francis@ti.com>
 <20240527124422.3553828-2-n-francis@ti.com>
 <547196ae-af25-43bb-801c-9a9dbb6ec134@kernel.org>
Content-Language: en-US
From: Neha Malcom Francis <n-francis@ti.com>
In-Reply-To: <547196ae-af25-43bb-801c-9a9dbb6ec134@kernel.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit

Hi Krzysztof,

On 27/05/24 19:59, Krzysztof Kozlowski wrote:
> 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@ti.com>
>> 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
> 

Thanks for the review! I have posted v3.

-- 
Thanking You
Neha Malcom Francis