From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1031043AbbKEFge (ORCPT ); Thu, 5 Nov 2015 00:36:34 -0500 Received: from mailout4.samsung.com ([203.254.224.34]:45059 "EHLO mailout4.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1030300AbbKEFgb (ORCPT ); Thu, 5 Nov 2015 00:36:31 -0500 X-AuditID: cbfee68e-f791c6d000001498-d8-563aeadc432b Message-id: <563AEB2D.5000408@samsung.com> Date: Thu, 05 Nov 2015 11:07:49 +0530 From: Alim Akhtar User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.5.0 MIME-version: 1.0 To: Krzysztof Kozlowski Cc: linux-samsung-soc@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, kgene@kernel.org Subject: Re: [PATCH 1/2] arm64: dts: exynos7: Add pmic s2mps15 device tree node References: <1446458641-4447-1-git-send-email-alim.akhtar@samsung.com> <56375EB1.2090504@samsung.com> In-reply-to: Content-type: text/plain; charset=UTF-8; format=flowed Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFprBIsWRmVeSWpSXmKPExsWyRsSkRvfuK6swg7uvmS3mHznHavH6haFF /2Mgb9Pja6wWl3fNYbOYcX4fkwObx6ZVnWwem5fUe/RtWcXo8XmTXABLFJdNSmpOZllqkb5d AlfG9tsHWQse6VasnL6OvYFxlUIXIweHhICJxNW5tl2MnECmmMSFe+vZuhi5OIQEVjBKzL45 jxUiYSLR3bmNCSIxi1FizbLJUM4DRonmrYuZQap4BbQkJi15wwhiswioSrz9s5cNxGYT0Ja4 O30LE8g2UYEIiccXhCDKBSV+TL7HAmKLCBhKHNy9HWwms8BsRokJO5qYQBLCAv4Sax8fYYRY 9pVR4sCFC2ALOAWCJe5seQu2mFnATOJRyzooW15i8xqQOBfQ2fvYJRYc+s4OcZGAxLfJh1gg fpaV2HSAGeI1SYmDK26wTGAUm4XkqFlIxs5CMnYBI/MqRtHUguSC4qT0IiO94sTc4tK8dL3k /NxNjMAIO/3vWd8OxpsHrA8xCnAwKvHwGlRbhQmxJpYVV+YeYjQFumIis5Rocj4wjvNK4g2N zYwsTE1MjY3MLc2UxHkTpH4GCwmkJ5akZqemFqQWxReV5qQWH2Jk4uCUamAMq5uVv2/Z13fy QXnOSz8VreiLXHKmgP2nisuiy/smsRi3VrZ9XNSwdZeGkbB/z6S1S7eLVvp3Tw+ZJNDh6WDA deHJXZm6B0W2vM1OUlmnLHp+V8v6ZpakvMrNPyNXdEnmzg6t1j1N6XffeUaaXJp0ZM75eVzO L1YeCjnJL5ffOOPset5ku41KLMUZiYZazEXFiQAvIISjqwIAAA== X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFnrGIsWRmVeSWpSXmKPExsVy+t9jQd07r6zCDOb8MrWYf+Qcq8XrF4YW /Y9fM1tsenyN1eLyrjlsFjPO72NyYPPYtKqTzWPzknqPvi2rGD0+b5ILYIlqYLTJSE1MSS1S SM1Lzk/JzEu3VfIOjneONzUzMNQ1tLQwV1LIS8xNtVVy8QnQdcvMAdqtpFCWmFMKFApILC5W 0rfDNCE0xE3XAqYxQtc3JAiux8gADSSsYczYfvsga8Ej3YqV09exNzCuUuhi5OSQEDCR6O7c xgRhi0lcuLeerYuRi0NIYBajxJplk5kgnAeMEs1bFzODVPEKaElMWvKGEcRmEVCVePtnLxuI zSagLXF3+hagBg4OUYEIiccXhCDKBSV+TL7HAmKLCBhKHNy9HWwms8BsRokJO5rANgsL+Eus fXyEEWLZV0aJAxcugC3gFAiWuLPlLdhiZgEziUct66BseYnNa94yT2AEuhNhySwkZbOQlC1g ZF7FKJFakFxQnJSea5iXWq5XnJhbXJqXrpecn7uJERzHz6R2MB7c5X6IUYCDUYmH16DaKkyI NbGsuDL3EKMEB7OSCG/BTKAQb0piZVVqUX58UWlOavEhRlNgKExklhJNzgemmLySeENjE3NT Y1NLEwsTM0slcV59T6MwIYH0xJLU7NTUgtQimD4mDk6pBkYft9XeOlPWbtdiYdxeXbrjdFWu U+CdumOvHTYYPOAWjlA2mnrqS9/VDx5KTjW8nbLdyZY2+T8mugSfiYxd8fVPZ0PjtjmhO+Zk uXCmux2O7z+86fhqbtd1m3Sr7jcHCPM5LnKyOvzuPN+00qL9J0OijhmGP9QS8kkPUHtnx3Y7 5ndl1YFHukosxRmJhlrMRcWJAAQtKoz5AgAA DLP-Filter: Pass X-MTR: 20000000000000000@CPGS X-CFilter-Loop: Reflected Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Krzysztof On 11/02/2015 07:22 PM, Krzysztof Kozlowski wrote: > 2015-11-02 22:01 GMT+09:00 Alim Akhtar : >>>> >>>> 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>; >>>> + 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"; >>>> + }; >>> >>> >>> Don't you want to use one of these clocks for s3c-rtc (&rtc node)? >>> >> yes, you are right, rtc on this board is currently broken, mainly because of >> the introduction of rtc_src clock in the s3c-rtc driver. >> That is on my do list next. will take a look. >> >> Are you suggesting to remove this -clk node now and add along with rtc >> changes? I feel this should go in along with this patch. > > Just add it in consecutive patch in this series. You added here some > providers (clock and regulators) without consumers. This of course > looks good as a way of providing full description of the board but: > 1. For regulators always on: may be meaningless for kernel. Kernel > does not use it. Existence of regulator subnode will fulfill driver's > needs for probe. > 2. For clocks: actually will disable these clocks because of lack of > consumers... which is fine but probably not what you wanted. > > The standard approach is to add such providers when they are needed - > there are some consumers using them. > OK. for now will keep the pmic clock added as clock will be in disabled state, so it wont harm. - will keep system related regulator like supply to arm,mif,int etc .. will remove supplies to other peripherals IPs. Hope thats fine. >>>> + >>>> + regulators { >>>> + ldo1_reg: LDO1 { >>>> + regulator-name = "vdd_ldo1"; >>>> + regulator-min-microvolt = <500000>; >>>> + regulator-max-microvolt = <900000>; >>>> + regulator-always-on; >>>> + regulator-enable-ramp-delay = <125>; >>>> + }; > > (...) > >>>> + >>>> + buck10_reg: BUCK10 { >>>> + regulator-name = "vdd_buck10"; >>>> + regulator-min-microvolt = <1000000>; >>>> + regulator-max-microvolt = <3000000>; >>>> + regulator-boot-on; >>>> + regulator-always-on; >>>> + regulator-ramp-delay = <25000>; >>>> + regulator-enable-ramp-delay = <250>; >>>> + }; >>> >>> >>> All of these ldo3 and bucks in vendor tree for Espresso board have >>> ramp delay of 12000. Also they don't have enable-ramp-delay set and >>> voltages sometimes differ. I don't have S2MPS15 datasheet so I don't >>> know what is the true value... I'll leave it up to you but it looks >>> suspicious. >>> >> These values generally comes from our board design team, so I cann't really >> comment on that, it may vary from board revision etc. >> I will check if we have any updated version of recommended value and update >> accordingly. > > Okay, just pointing the difference. I cannot verify them. > >> >>>> + }; >>>> + }; >>>> +}; >>> >>> >>> What will be the benefit of defining all of these regulators if they >>> are always on and without consumers? No one will disable them, no one >>> will change the voltage. Please provide some consumers. >>> >> As many drivers are not yet enabled in arm64 defconfig, that is one of the >> reason why we are not seeing many consumer for these nodes. > > That is not a problem. Please send a patch changing the defconfig. > Usually defconfig (for armv7 this would be exynos and multi_v7) should > provide bootable and working environment for all of our supported > boards. > >> This is the ground work being done for enabling those. If you insist will >> try to reduce what is being used now. Moreover this was used to verify >> functionality of pmic driver as well. > > Actually as a verification I think even adding simple node > "regulators" is sufficient - driver will add all regulators anyway. > However seeing all regulators described for particular board is > good... but lack of consumers is disturbing because this may mean that > it was not really fully modeled. > >>>From my point of view - all of regulators in DT are welcomed but at > least some of them should have a consumer. This means that someone > took care and looked at the relationships between them. > > Best regards, > Krzysztof >