From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755490AbbJ1La7 (ORCPT ); Wed, 28 Oct 2015 07:30:59 -0400 Received: from mailout4.samsung.com ([203.254.224.34]:33028 "EHLO mailout4.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754169AbbJ1La5 (ORCPT ); Wed, 28 Oct 2015 07:30:57 -0400 X-AuditID: cbfee690-f794e6d0000014de-ce-5630b1ee62a9 Message-id: <5630AFC7.2030901@samsung.com> Date: Wed, 28 Oct 2015 16:51:43 +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: Lee Jones , Krzysztof Kozlowski Cc: broonie@kernel.org, mturquette@baylibre.com, linux-samsung-soc@vger.kernel.org, linux-clk@vger.kernel.org, rtc-linux@googlegroups.com, linux-kernel@vger.kernel.org, Thomas Abraham Subject: Re: [PATCH v3 2/5] mfd: sec: Add support for S2MPS15 PMIC References: <1445863883-5187-1-git-send-email-alim.akhtar@samsung.com> <1445863883-5187-3-git-send-email-alim.akhtar@samsung.com> <20151026143411.GL597@x1> <563021AF.6030100@samsung.com> <20151028084609.GG5828@x1> In-reply-to: <20151028084609.GG5828@x1> Content-type: text/plain; charset=UTF-8; format=flowed Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFlrLIsWRmVeSWpSXmKPExsWyRsSkVvfdRoMwg0dHrC2mPnzCZvH6haHF /a9HGS0+9txjtbi8aw6bxYzz+5gsLp5ytdjf2cFo0bGM0YHT4/2NVnaPPRNPsnlsWtXJ5nHn 2h42j74tqxg9Pm+SC2CL4rJJSc3JLEst0rdL4MroXDuFqeBYTMXzvqXsDYxXvLsYOTkkBEwk Hm98xg5hi0lcuLeeDcQWEljBKPGmRx6mZuuJH8xdjFxA8VmMEne77kE5DxglnrXdAerg4OAV 0JLY21UO0sAioCrx5MRssEFsAtoSd6dvYQIpERWIkHh8QQgkzCsgKPFj8j0WkLCIQKjEkoVq IBOZBa4ySqw/8g+sXFjASeL1MVmITbcZJTbPuwU2klNAXWJa33RWEJtZwEziUcs6ZghbXmLz mrdgp0kIvGWXWHlwFQvEPQIS3yYfAlsmISArsekAM8RfkhIHV9xgmcAoNgvJSbOQjJ2FZOwC RuZVjKKpBckFxUnpRSZ6xYm5xaV56XrJ+bmbGIGxePrfswk7GO8dsD7EKMDBqMTDa1ChFybE mlhWXJl7iNEU6IqJzFKiyfnAiM8riTc0NjOyMDUxNTYytzRTEud9LfUzWEggPbEkNTs1tSC1 KL6oNCe1+BAjEwenVANjx1mmbzO3xCvrM/6+W27wt9bFkId3h8imBZ9lJhwJtFni/4TtevKS jztajk05qld9LvF/oOinyn+rMy+93/3hbp9+9uG1RnI2vwS/HtrcUrB9XbXq7Vo+5fO5/A8K S4rlJsT9KBQ2zvFp9n8SGf1o31ZxLs/d1r5v0n+cfOvpk3ZA1ufM+0n2SizFGYmGWsxFxYkA gOePvMACAAA= X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFvrNIsWRmVeSWpSXmKPExsVy+t9jQd13Gw3CDFa9MraY+vAJm8XrF4YW 978eZbT42HOP1eLyrjlsFjPO72OyuHjK1WJ/ZwejRccyRgdOj/c3Wtk99kw8yeaxaVUnm8ed a3vYPPq2rGL0+LxJLoAtqoHRJiM1MSW1SCE1Lzk/JTMv3VbJOzjeOd7UzMBQ19DSwlxJIS8x N9VWycUnQNctMwfoJiWFssScUqBQQGJxsZK+HaYJoSFuuhYwjRG6viFBcD1GBmggYQ1jRufa KUwFx2IqnvctZW9gvOLdxcjJISFgIrH1xA9mCFtM4sK99WxdjFwcQgKzGCXudt1jhnAeMEo8 a7sDlOHg4BXQktjbVQ7SwCKgKvHkxGw2EJtNQFvi7vQtTCAlogIREo8vCIGEeQUEJX5MvscC EhYRCJVYslANZCKzwFVGifVH/oGVCws4Sbw+Jgux6TajxOZ5t8BGcgqoS0zrm84KYjMLmEk8 alnHDGHLS2xe85Z5AiPQkQgrZiEpm4WkbAEj8ypGidSC5ILipPRcw7zUcr3ixNzi0rx0veT8 3E2M4Ih/JrWD8eAu90OMAhyMSjy8P4v0w4RYE8uKK3MPMUpwMCuJ8H5YZRAmxJuSWFmVWpQf X1Sak1p8iNEUGAYTmaVEk/OBySivJN7Q2MTc1NjU0sTCxMxSSZz3QoZGmJBAemJJanZqakFq EUwfEwenVAPjpYSX02IyfndfPfgs4pWc0+ZtomuM7/pv0Hi1MGgV/9cJZVsyX+xUd/4+Mfv4 kie+ApoGXQvucjDyn9w46+C6u6vVQz+cXn5JXyPx2/0vy6+fDVy3tsApTbt6W9gmkUOvfBNN J9z+6Mb8J+co3+eeI/vzH7F8cJib7BWw45/m/xPRn5W81tZYJyixFGckGmoxFxUnAgC0+4EB DgMAAA== 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 Hello, On 10/28/2015 02:16 PM, Lee Jones wrote: > On Wed, 28 Oct 2015, Krzysztof Kozlowski wrote: > >> On 26.10.2015 23:34, Lee Jones wrote: >>> On Mon, 26 Oct 2015, Alim Akhtar wrote: >>> >>>> From: Thomas Abraham >>>> >>>> Add support for S2MPS15 PMIC which is similar to S2MPS11 PMIC. The S2MPS15 >>>> PMIC supports 27 LDO regulators, 10 buck regulators, RTC, three 32.768KHz >>>> clock outputs and battery charger. This patch adds initial support for >>>> LDO and buck regulators of S2MPS15 device. >>>> >>>> Signed-off-by: Thomas Abraham >>>> Signed-off-by: Alim Akhtar >>>> [Alim: Added s2mps15_devs like rtc and clk and related changes] >>>> Reviewed-by: Krzysztof Kozlowski >>>> --- >>>> drivers/mfd/sec-core.c | 31 +++++++ >>>> drivers/mfd/sec-irq.c | 8 ++ >>>> include/linux/mfd/samsung/core.h | 1 + >>>> include/linux/mfd/samsung/s2mps15.h | 158 +++++++++++++++++++++++++++++++++++ >>>> 4 files changed, 198 insertions(+) >>>> create mode 100644 include/linux/mfd/samsung/s2mps15.h >>> >>> I replied to the previous set and won't be reviewing this one until >>> all of the open points are solved. >> >> The naming and compatibles used by the driver are confusing but how it >> was at beginning. Beside the confusion, the names are correct: >> >> 1. Main mfd driver: >> - compatible: samsung,s2mps1*-pmic >> - driver name: sec_pmic >> >> 2. Regulator driver: >> - no compatible (because it always searches for "regulators" subnode of >> its parent... that is the convention/legacy behaviour) >> - driver name: s2mps1*-pmic >> >> I hope that explains your concerns. > > It explains *why*, but doesn't ease my concerns in any way. > > Unfortunately I've only just realised the disparity we have between > MFD and the Regulator subsystem, which is annoying because it's now > almost impossible to rectify. > > We should have taken one of two views. Either a) The MFD is the PMIC > device which encompasses regulator control. In which case the MFD > and it's corresponding compatible string would be named *-pmic and the > regulator driver would be called *-regulator. Or b) The MFD could be > considered a normal MFD and be named after the model number, then the > regulator 'could' be named *-pmic. > > However, with reference to b), how much other Power Management does > the regulator driver do besides control regulators? I suspect not > much. Therefore my preference would be for a). My second choice > would be a mixuture of the two where nothing gets named *-pmic. The > last option on my list would be the current situation where we seem to > be calling both the MFD (PMIC) itself and the Regulator driver > *-pmic, which is not good. > Well, I would have also preferred option a), but keeping existing DT bindings, looks like we need to go with the current situation. Krzysztof any thought on this? >>>> diff --git a/drivers/mfd/sec-core.c b/drivers/mfd/sec-core.c >>>> index 2626fc0b5b8c..db3d4d4ff805 100644 >>>> --- a/drivers/mfd/sec-core.c >>>> +++ b/drivers/mfd/sec-core.c >>>> @@ -29,6 +29,7 @@ >>>> #include >>>> #include >>>> #include >>>> +#include >>>> #include >>>> #include >>>> #include >>>> @@ -96,6 +97,17 @@ static const struct mfd_cell s2mps14_devs[] = { >>>> } >>>> }; >>>> >>>> +static const struct mfd_cell s2mps15_devs[] = { >>>> + { >>>> + .name = "s2mps15-pmic", >>>> + }, { >>>> + .name = "s2mps15-rtc", >>>> + }, { >>>> + .name = "s2mps15-clk", >>>> + .of_compatible = "samsung,s2mps15-clk", >>>> + }, >>>> +}; >>>> + >>>> static const struct mfd_cell s2mpa01_devs[] = { >>>> { >>>> .name = "s2mpa01-pmic", >>>> @@ -125,6 +137,9 @@ static const struct of_device_id sec_dt_match[] = { >>>> .compatible = "samsung,s2mps14-pmic", >>>> .data = (void *)S2MPS14X, >>>> }, { >>>> + .compatible = "samsung,s2mps15-pmic", >>>> + .data = (void *)S2MPS15X, >>>> + }, { >>>> .compatible = "samsung,s2mpa01-pmic", >>>> .data = (void *)S2MPA01, >>>> }, { >>>> @@ -226,6 +241,15 @@ static const struct regmap_config s2mps14_regmap_config = { >>>> .cache_type = REGCACHE_FLAT, >>>> }; >>>> >>>> +static const struct regmap_config s2mps15_regmap_config = { >>>> + .reg_bits = 8, >>>> + .val_bits = 8, >>>> + >>>> + .max_register = S2MPS15_REG_LDODSCH4, >>>> + .volatile_reg = s2mps11_volatile, >>>> + .cache_type = REGCACHE_FLAT, >>>> +}; >>>> + >>>> static const struct regmap_config s2mpu02_regmap_config = { >>>> .reg_bits = 8, >>>> .val_bits = 8, >>>> @@ -387,6 +411,9 @@ static int sec_pmic_probe(struct i2c_client *i2c, >>>> case S2MPS14X: >>>> regmap = &s2mps14_regmap_config; >>>> break; >>>> + case S2MPS15X: >>>> + regmap = &s2mps15_regmap_config; >>>> + break; >>>> case S5M8763X: >>>> regmap = &s5m8763_regmap_config; >>>> break; >>>> @@ -445,6 +472,10 @@ static int sec_pmic_probe(struct i2c_client *i2c, >>>> sec_devs = s2mps14_devs; >>>> num_sec_devs = ARRAY_SIZE(s2mps14_devs); >>>> break; >>>> + case S2MPS15X: >>>> + sec_devs = s2mps15_devs; >>>> + num_sec_devs = ARRAY_SIZE(s2mps15_devs); >>>> + break; >>>> case S2MPU02: >>>> sec_devs = s2mpu02_devs; >>>> num_sec_devs = ARRAY_SIZE(s2mpu02_devs); >>>> diff --git a/drivers/mfd/sec-irq.c b/drivers/mfd/sec-irq.c >>>> index 806fa8dbb22d..d77de431cc50 100644 >>>> --- a/drivers/mfd/sec-irq.c >>>> +++ b/drivers/mfd/sec-irq.c >>>> @@ -407,6 +407,11 @@ static const struct regmap_irq_chip s2mps14_irq_chip = { >>>> S2MPS1X_IRQ_CHIP_COMMON_DATA, >>>> }; >>>> >>>> +static const struct regmap_irq_chip s2mps15_irq_chip = { >>>> + .name = "s2mps15", >>>> + S2MPS1X_IRQ_CHIP_COMMON_DATA, >>>> +}; >>>> + >>>> static const struct regmap_irq_chip s2mpu02_irq_chip = { >>>> .name = "s2mpu02", >>>> .irqs = s2mpu02_irqs, >>>> @@ -466,6 +471,9 @@ int sec_irq_init(struct sec_pmic_dev *sec_pmic) >>>> case S2MPS14X: >>>> sec_irq_chip = &s2mps14_irq_chip; >>>> break; >>>> + case S2MPS15X: >>>> + sec_irq_chip = &s2mps15_irq_chip; >>>> + break; >>>> case S2MPU02: >>>> sec_irq_chip = &s2mpu02_irq_chip; >>>> break; >>>> diff --git a/include/linux/mfd/samsung/core.h b/include/linux/mfd/samsung/core.h >>>> index a06098639399..6bc4bcd488ac 100644 >>>> --- a/include/linux/mfd/samsung/core.h >>>> +++ b/include/linux/mfd/samsung/core.h >>>> @@ -44,6 +44,7 @@ enum sec_device_type { >>>> S2MPS11X, >>>> S2MPS13X, >>>> S2MPS14X, >>>> + S2MPS15X, >>>> S2MPU02, >>>> }; >>>> >>>> diff --git a/include/linux/mfd/samsung/s2mps15.h b/include/linux/mfd/samsung/s2mps15.h >>>> new file mode 100644 >>>> index 000000000000..36d35287c3c0 >>>> --- /dev/null >>>> +++ b/include/linux/mfd/samsung/s2mps15.h >>>> @@ -0,0 +1,158 @@ >>>> +/* >>>> + * Copyright (c) 2015 Samsung Electronics Co., Ltd >>>> + * http://www.samsung.com >>>> + * >>>> + * This program is free software; you can redistribute it and/or modify it >>>> + * under the terms of the GNU General Public License as published by the >>>> + * Free Software Foundation; either version 2 of the License, or (at your >>>> + * option) any later version. >>>> + * >>>> + * This program is distributed in the hope that it will be useful, >>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >>>> + * GNU General Public License for more details. >>>> + */ >>>> + >>>> +#ifndef __LINUX_MFD_S2MPS15_H >>>> +#define __LINUX_MFD_S2MPS15_H >>>> + >>>> +/* S2MPS15 registers */ >>>> +enum s2mps15_reg { >>>> + S2MPS15_REG_ID, >>>> + S2MPS15_REG_INT1, >>>> + S2MPS15_REG_INT2, >>>> + S2MPS15_REG_INT3, >>>> + S2MPS15_REG_INT1M, >>>> + S2MPS15_REG_INT2M, >>>> + S2MPS15_REG_INT3M, >>>> + S2MPS15_REG_ST1, >>>> + S2MPS15_REG_ST2, >>>> + S2MPS15_REG_PWRONSRC, >>>> + S2MPS15_REG_OFFSRC, >>>> + S2MPS15_REG_BU_CHG, >>>> + S2MPS15_REG_RTC_BUF, >>>> + S2MPS15_REG_CTRL1, >>>> + S2MPS15_REG_CTRL2, >>>> + S2MPS15_REG_RSVD1, >>>> + S2MPS15_REG_RSVD2, >>>> + S2MPS15_REG_RSVD3, >>>> + S2MPS15_REG_RSVD4, >>>> + S2MPS15_REG_RSVD5, >>>> + S2MPS15_REG_RSVD6, >>>> + S2MPS15_REG_CTRL3, >>>> + S2MPS15_REG_RSVD7, >>>> + S2MPS15_REG_RSVD8, >>>> + S2MPS15_REG_RSVD9, >>>> + S2MPS15_REG_B1CTRL1, >>>> + S2MPS15_REG_B1CTRL2, >>>> + S2MPS15_REG_B2CTRL1, >>>> + S2MPS15_REG_B2CTRL2, >>>> + S2MPS15_REG_B3CTRL1, >>>> + S2MPS15_REG_B3CTRL2, >>>> + S2MPS15_REG_B4CTRL1, >>>> + S2MPS15_REG_B4CTRL2, >>>> + S2MPS15_REG_B5CTRL1, >>>> + S2MPS15_REG_B5CTRL2, >>>> + S2MPS15_REG_B6CTRL1, >>>> + S2MPS15_REG_B6CTRL2, >>>> + S2MPS15_REG_B7CTRL1, >>>> + S2MPS15_REG_B7CTRL2, >>>> + S2MPS15_REG_B8CTRL1, >>>> + S2MPS15_REG_B8CTRL2, >>>> + S2MPS15_REG_B9CTRL1, >>>> + S2MPS15_REG_B9CTRL2, >>>> + S2MPS15_REG_B10CTRL1, >>>> + S2MPS15_REG_B10CTRL2, >>>> + S2MPS15_REG_BBCTRL1, >>>> + S2MPS15_REG_BBCTRL2, >>>> + S2MPS15_REG_BRAMP, >>>> + S2MPS15_REG_LDODVS1, >>>> + S2MPS15_REG_LDODVS2, >>>> + S2MPS15_REG_LDODVS3, >>>> + S2MPS15_REG_LDODVS4, >>>> + S2MPS15_REG_L1CTRL, >>>> + S2MPS15_REG_L2CTRL, >>>> + S2MPS15_REG_L3CTRL, >>>> + S2MPS15_REG_L4CTRL, >>>> + S2MPS15_REG_L5CTRL, >>>> + S2MPS15_REG_L6CTRL, >>>> + S2MPS15_REG_L7CTRL, >>>> + S2MPS15_REG_L8CTRL, >>>> + S2MPS15_REG_L9CTRL, >>>> + S2MPS15_REG_L10CTRL, >>>> + S2MPS15_REG_L11CTRL, >>>> + S2MPS15_REG_L12CTRL, >>>> + S2MPS15_REG_L13CTRL, >>>> + S2MPS15_REG_L14CTRL, >>>> + S2MPS15_REG_L15CTRL, >>>> + S2MPS15_REG_L16CTRL, >>>> + S2MPS15_REG_L17CTRL, >>>> + S2MPS15_REG_L18CTRL, >>>> + S2MPS15_REG_L19CTRL, >>>> + S2MPS15_REG_L20CTRL, >>>> + S2MPS15_REG_L21CTRL, >>>> + S2MPS15_REG_L22CTRL, >>>> + S2MPS15_REG_L23CTRL, >>>> + S2MPS15_REG_L24CTRL, >>>> + S2MPS15_REG_L25CTRL, >>>> + S2MPS15_REG_L26CTRL, >>>> + S2MPS15_REG_L27CTRL, >>>> + S2MPS15_REG_LDODSCH1, >>>> + S2MPS15_REG_LDODSCH2, >>>> + S2MPS15_REG_LDODSCH3, >>>> + S2MPS15_REG_LDODSCH4, >>>> +}; >>>> + >>>> +/* S2MPS15 regulator ids */ >>>> +enum s2mps15_regulators { >>>> + S2MPS15_LDO1, >>>> + S2MPS15_LDO2, >>>> + S2MPS15_LDO3, >>>> + S2MPS15_LDO4, >>>> + S2MPS15_LDO5, >>>> + S2MPS15_LDO6, >>>> + S2MPS15_LDO7, >>>> + S2MPS15_LDO8, >>>> + S2MPS15_LDO9, >>>> + S2MPS15_LDO10, >>>> + S2MPS15_LDO11, >>>> + S2MPS15_LDO12, >>>> + S2MPS15_LDO13, >>>> + S2MPS15_LDO14, >>>> + S2MPS15_LDO15, >>>> + S2MPS15_LDO16, >>>> + S2MPS15_LDO17, >>>> + S2MPS15_LDO18, >>>> + S2MPS15_LDO19, >>>> + S2MPS15_LDO20, >>>> + S2MPS15_LDO21, >>>> + S2MPS15_LDO22, >>>> + S2MPS15_LDO23, >>>> + S2MPS15_LDO24, >>>> + S2MPS15_LDO25, >>>> + S2MPS15_LDO26, >>>> + S2MPS15_LDO27, >>>> + S2MPS15_BUCK1, >>>> + S2MPS15_BUCK2, >>>> + S2MPS15_BUCK3, >>>> + S2MPS15_BUCK4, >>>> + S2MPS15_BUCK5, >>>> + S2MPS15_BUCK6, >>>> + S2MPS15_BUCK7, >>>> + S2MPS15_BUCK8, >>>> + S2MPS15_BUCK9, >>>> + S2MPS15_BUCK10, >>>> + S2MPS15_BUCK11, >>>> + S2MPS15_REGULATOR_MAX, >>>> +}; >>>> + >>>> +#define S2MPS15_LDO_VSEL_MASK (0x3F) >>>> +#define S2MPS15_BUCK_VSEL_MASK (0xFF) >>>> + >>>> +#define S2MPS15_ENABLE_SHIFT (0x06) >>>> +#define S2MPS15_ENABLE_MASK (0x03 << S2MPS15_ENABLE_SHIFT) >>>> + >>>> +#define S2MPS15_LDO_N_VOLTAGES (S2MPS15_LDO_VSEL_MASK + 1) >>>> +#define S2MPS15_BUCK_N_VOLTAGES (S2MPS15_BUCK_VSEL_MASK + 1) >>>> + >>>> +#endif /* __LINUX_MFD_S2MPS15_H */ >>> >> >