From mboxrd@z Thu Jan 1 00:00:00 1970 From: Javier Martinez Canillas Subject: Re: [PATCH 1/2] arm64: dts: exynos7: Add pmic s2mps15 device tree node Date: Mon, 2 Nov 2015 12:30:37 +0100 Message-ID: <5637495D.1080701@osg.samsung.com> References: <1446458641-4447-1-git-send-email-alim.akhtar@samsung.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1446458641-4447-1-git-send-email-alim.akhtar-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> Sender: devicetree-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Alim Akhtar , linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org Cc: k.kozlowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, kgene-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org List-Id: linux-samsung-soc@vger.kernel.org Hello Alim, This patch looks mostly good to me, I just have two comments below: On 11/02/2015 11:04 AM, Alim Akhtar wrote: > This patch add pmic (s2mps15) node of espresso board, > which includes addition of regulators and pmic-clk sub-nodes. > > Signed-off-by: Abhilash Kesavan > Signed-off-by: Alim Akhtar > --- > This patch should go in after driver side changes [1] lands. > [1]-> https://www.mail-archive.com/linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg47736.html > > arch/arm64/boot/dts/exynos/exynos7-espresso.dts | 349 +++++++++++++++++++++++ > 1 file changed, 349 insertions(+) > > diff --git a/arch/arm64/boot/dts/exynos/exynos7-espresso.dts b/arch/arm64/boot/dts/exynos/exynos7-espresso.dts > index 838a3626dac1..8ce04a0ec928 100644 > --- a/arch/arm64/boot/dts/exynos/exynos7-espresso.dts > +++ b/arch/arm64/boot/dts/exynos/exynos7-espresso.dts > @@ -53,6 +53,355 @@ > status = "okay"; > }; > > +&hsi2c_4 { > + samsung,i2c-sda-delay = <100>; > + samsung,i2c-max-bus-freq = <200000>; > + status = "okay"; > + > + s2mps15_pmic@66 { > + compatible = "samsung,s2mps15-pmic"; > + reg = <0x66>; > + interrupts = <2 0>; Maybe using IRQ_TYPE_NONE instead of 0? > + interrupt-parent = <&gpa0>; > + pinctrl-names = "default"; > + pinctrl-0 = <&pmic_irq>; > + wakeup-source; > + > + s2mps15_osc: clocks { > + compatible = "samsung,s2mps13-clk"; > + #clock-cells = <1>; > + clock-output-names = "s2mps13_ap", "s2mps13_cp", > + "s2mps13_bt"; > + }; > + > + regulators { > + ldo1_reg: LDO1 { > + regulator-name = "vdd_ldo1"; > + regulator-min-microvolt = <500000>; > + regulator-max-microvolt = <900000>; > + regulator-always-on; I see that all regulators are marked as regulator-always-on but that will prevent the regulator subsystem to disable unused regulators. Can you please double check which regulators should really be always on and which ones can be disabled if are not used? After those two changes: Reviewed-by: Javier Martinez Canillas Best regards, -- Javier Martinez Canillas Open Source Group Samsung Research America -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html From mboxrd@z Thu Jan 1 00:00:00 1970 From: javier@osg.samsung.com (Javier Martinez Canillas) Date: Mon, 2 Nov 2015 12:30:37 +0100 Subject: [PATCH 1/2] arm64: dts: exynos7: Add pmic s2mps15 device tree node In-Reply-To: <1446458641-4447-1-git-send-email-alim.akhtar@samsung.com> References: <1446458641-4447-1-git-send-email-alim.akhtar@samsung.com> Message-ID: <5637495D.1080701@osg.samsung.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hello Alim, This patch looks mostly good to me, I just have two comments below: On 11/02/2015 11:04 AM, Alim Akhtar wrote: > This patch add pmic (s2mps15) node of espresso board, > which includes addition of regulators and pmic-clk sub-nodes. > > Signed-off-by: Abhilash Kesavan > Signed-off-by: Alim Akhtar > --- > This patch should go in after driver side changes [1] lands. > [1]-> https://www.mail-archive.com/linux-samsung-soc at vger.kernel.org/msg47736.html > > arch/arm64/boot/dts/exynos/exynos7-espresso.dts | 349 +++++++++++++++++++++++ > 1 file changed, 349 insertions(+) > > diff --git a/arch/arm64/boot/dts/exynos/exynos7-espresso.dts b/arch/arm64/boot/dts/exynos/exynos7-espresso.dts > index 838a3626dac1..8ce04a0ec928 100644 > --- a/arch/arm64/boot/dts/exynos/exynos7-espresso.dts > +++ b/arch/arm64/boot/dts/exynos/exynos7-espresso.dts > @@ -53,6 +53,355 @@ > status = "okay"; > }; > > +&hsi2c_4 { > + samsung,i2c-sda-delay = <100>; > + samsung,i2c-max-bus-freq = <200000>; > + status = "okay"; > + > + s2mps15_pmic at 66 { > + compatible = "samsung,s2mps15-pmic"; > + reg = <0x66>; > + interrupts = <2 0>; Maybe using IRQ_TYPE_NONE instead of 0? > + interrupt-parent = <&gpa0>; > + pinctrl-names = "default"; > + pinctrl-0 = <&pmic_irq>; > + wakeup-source; > + > + s2mps15_osc: clocks { > + compatible = "samsung,s2mps13-clk"; > + #clock-cells = <1>; > + clock-output-names = "s2mps13_ap", "s2mps13_cp", > + "s2mps13_bt"; > + }; > + > + regulators { > + ldo1_reg: LDO1 { > + regulator-name = "vdd_ldo1"; > + regulator-min-microvolt = <500000>; > + regulator-max-microvolt = <900000>; > + regulator-always-on; I see that all regulators are marked as regulator-always-on but that will prevent the regulator subsystem to disable unused regulators. Can you please double check which regulators should really be always on and which ones can be disabled if are not used? After those two changes: Reviewed-by: Javier Martinez Canillas Best regards, -- Javier Martinez Canillas Open Source Group Samsung Research America From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753638AbbKBLap (ORCPT ); Mon, 2 Nov 2015 06:30:45 -0500 Received: from lists.s-osg.org ([54.187.51.154]:54092 "EHLO lists.s-osg.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752062AbbKBLao (ORCPT ); Mon, 2 Nov 2015 06:30:44 -0500 Subject: Re: [PATCH 1/2] arm64: dts: exynos7: Add pmic s2mps15 device tree node To: Alim Akhtar , linux-samsung-soc@vger.kernel.org References: <1446458641-4447-1-git-send-email-alim.akhtar@samsung.com> Cc: k.kozlowski@samsung.com, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, kgene@kernel.org From: Javier Martinez Canillas X-Enigmail-Draft-Status: N1110 Message-ID: <5637495D.1080701@osg.samsung.com> Date: Mon, 2 Nov 2015 12:30:37 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: <1446458641-4447-1-git-send-email-alim.akhtar@samsung.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hello Alim, This patch looks mostly good to me, I just have two comments below: On 11/02/2015 11:04 AM, Alim Akhtar wrote: > This patch add pmic (s2mps15) node of espresso board, > which includes addition of regulators and pmic-clk sub-nodes. > > Signed-off-by: Abhilash Kesavan > Signed-off-by: Alim Akhtar > --- > This patch should go in after driver side changes [1] lands. > [1]-> https://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg47736.html > > arch/arm64/boot/dts/exynos/exynos7-espresso.dts | 349 +++++++++++++++++++++++ > 1 file changed, 349 insertions(+) > > diff --git a/arch/arm64/boot/dts/exynos/exynos7-espresso.dts b/arch/arm64/boot/dts/exynos/exynos7-espresso.dts > index 838a3626dac1..8ce04a0ec928 100644 > --- a/arch/arm64/boot/dts/exynos/exynos7-espresso.dts > +++ b/arch/arm64/boot/dts/exynos/exynos7-espresso.dts > @@ -53,6 +53,355 @@ > status = "okay"; > }; > > +&hsi2c_4 { > + samsung,i2c-sda-delay = <100>; > + samsung,i2c-max-bus-freq = <200000>; > + status = "okay"; > + > + s2mps15_pmic@66 { > + compatible = "samsung,s2mps15-pmic"; > + reg = <0x66>; > + interrupts = <2 0>; Maybe using IRQ_TYPE_NONE instead of 0? > + interrupt-parent = <&gpa0>; > + pinctrl-names = "default"; > + pinctrl-0 = <&pmic_irq>; > + wakeup-source; > + > + s2mps15_osc: clocks { > + compatible = "samsung,s2mps13-clk"; > + #clock-cells = <1>; > + clock-output-names = "s2mps13_ap", "s2mps13_cp", > + "s2mps13_bt"; > + }; > + > + regulators { > + ldo1_reg: LDO1 { > + regulator-name = "vdd_ldo1"; > + regulator-min-microvolt = <500000>; > + regulator-max-microvolt = <900000>; > + regulator-always-on; I see that all regulators are marked as regulator-always-on but that will prevent the regulator subsystem to disable unused regulators. Can you please double check which regulators should really be always on and which ones can be disabled if are not used? After those two changes: Reviewed-by: Javier Martinez Canillas Best regards, -- Javier Martinez Canillas Open Source Group Samsung Research America