diff mbox

[v3,2/5] mfd: sec: Add support for S2MPS15 PMIC

Message ID 1445863883-5187-3-git-send-email-alim.akhtar@samsung.com
State Not Applicable
Headers show

Commit Message

Alim Akhtar Oct. 26, 2015, 12:51 p.m. UTC
From: Thomas Abraham <thomas.ab@samsung.com>

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 <thomas.ab@samsung.com>
Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com>
[Alim: Added s2mps15_devs like rtc and clk and related changes]
Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
---
 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

Comments

Lee Jones Oct. 26, 2015, 2:34 p.m. UTC | #1
On Mon, 26 Oct 2015, Alim Akhtar wrote:

> From: Thomas Abraham <thomas.ab@samsung.com>
> 
> 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 <thomas.ab@samsung.com>
> Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com>
> [Alim: Added s2mps15_devs like rtc and clk and related changes]
> Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> ---
>  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.

> 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 <linux/mfd/samsung/s2mps11.h>
>  #include <linux/mfd/samsung/s2mps13.h>
>  #include <linux/mfd/samsung/s2mps14.h>
> +#include <linux/mfd/samsung/s2mps15.h>
>  #include <linux/mfd/samsung/s2mpu02.h>
>  #include <linux/mfd/samsung/s5m8763.h>
>  #include <linux/mfd/samsung/s5m8767.h>
> @@ -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 */
Krzysztof Kozlowski Oct. 28, 2015, 1:15 a.m. UTC | #2
On 26.10.2015 23:34, Lee Jones wrote:
> On Mon, 26 Oct 2015, Alim Akhtar wrote:
> 
>> From: Thomas Abraham <thomas.ab@samsung.com>
>>
>> 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 <thomas.ab@samsung.com>
>> Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com>
>> [Alim: Added s2mps15_devs like rtc and clk and related changes]
>> Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
>> ---
>>  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.

Best regards,
Krzysztof


> 
>> 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 <linux/mfd/samsung/s2mps11.h>
>>  #include <linux/mfd/samsung/s2mps13.h>
>>  #include <linux/mfd/samsung/s2mps14.h>
>> +#include <linux/mfd/samsung/s2mps15.h>
>>  #include <linux/mfd/samsung/s2mpu02.h>
>>  #include <linux/mfd/samsung/s5m8763.h>
>>  #include <linux/mfd/samsung/s5m8767.h>
>> @@ -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 */
>
Krzysztof Kozlowski Oct. 28, 2015, 1:28 a.m. UTC | #3
On 26.10.2015 21:51, Alim Akhtar wrote:
> From: Thomas Abraham <thomas.ab@samsung.com>
> 
> 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 <thomas.ab@samsung.com>
> Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com>
> [Alim: Added s2mps15_devs like rtc and clk and related changes]
> Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> ---
>  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
> 
> 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 <linux/mfd/samsung/s2mps11.h>
>  #include <linux/mfd/samsung/s2mps13.h>
>  #include <linux/mfd/samsung/s2mps14.h>
> +#include <linux/mfd/samsung/s2mps15.h>
>  #include <linux/mfd/samsung/s2mpu02.h>
>  #include <linux/mfd/samsung/s5m8763.h>
>  #include <linux/mfd/samsung/s5m8767.h>
> @@ -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",
> +	},

I have different questions here. RTC and CLK module look the same as
S2MPS14. Then you should existing compatibles and names.

I'll ask in details about this in separate emails for patch 4 and 5.
Until questions are resolved, these looks wrong.

Best regards,
Krzysztof
Lee Jones Oct. 28, 2015, 8:46 a.m. UTC | #4
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 <thomas.ab@samsung.com>
> >>
> >> 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 <thomas.ab@samsung.com>
> >> Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com>
> >> [Alim: Added s2mps15_devs like rtc and clk and related changes]
> >> Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> >> ---
> >>  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.

> >> 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 <linux/mfd/samsung/s2mps11.h>
> >>  #include <linux/mfd/samsung/s2mps13.h>
> >>  #include <linux/mfd/samsung/s2mps14.h>
> >> +#include <linux/mfd/samsung/s2mps15.h>
> >>  #include <linux/mfd/samsung/s2mpu02.h>
> >>  #include <linux/mfd/samsung/s5m8763.h>
> >>  #include <linux/mfd/samsung/s5m8767.h>
> >> @@ -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 */
> > 
>
Alim Akhtar Oct. 28, 2015, 11:21 a.m. UTC | #5
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 <thomas.ab@samsung.com>
>>>>
>>>> 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 <thomas.ab@samsung.com>
>>>> Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com>
>>>> [Alim: Added s2mps15_devs like rtc and clk and related changes]
>>>> Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
>>>> ---
>>>>   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 <linux/mfd/samsung/s2mps11.h>
>>>>   #include <linux/mfd/samsung/s2mps13.h>
>>>>   #include <linux/mfd/samsung/s2mps14.h>
>>>> +#include <linux/mfd/samsung/s2mps15.h>
>>>>   #include <linux/mfd/samsung/s2mpu02.h>
>>>>   #include <linux/mfd/samsung/s5m8763.h>
>>>>   #include <linux/mfd/samsung/s5m8767.h>
>>>> @@ -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 */
>>>
>>
>
Krzysztof Kozlowski Oct. 28, 2015, 1:44 p.m. UTC | #6
W dniu 28.10.2015 o 17:46, Lee Jones pisze:
> 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 <thomas.ab@samsung.com>
>>>>
>>>> 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 <thomas.ab@samsung.com>
>>>> Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com>
>>>> [Alim: Added s2mps15_devs like rtc and clk and related changes]
>>>> Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
>>>> ---
>>>>  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.

Starting from the description of device-family. This is called "Power
Management IC" but it is rather a "Power Deliver/Distribute IC". There
isn't any logic inside except enable/disable/configure/set low power
mode for regulators.

However in the same time the IC comes (always) with:
 - 32kHz clocks,
 - RTC,
 - backup battery charger (no driver for it),
 - reset for SoC,
 - shutdown on thermal alert (also no driver for this control).

The solution a) seems fine to me. Make sense and it looks entirely
backward compatible - only driver names will be modified.

Best regards,
Krzysztof
Krzysztof Kozlowski Oct. 28, 2015, 1:47 p.m. UTC | #7
W dniu 28.10.2015 o 20:21, Alim Akhtar pisze:
> 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 <thomas.ab@samsung.com>
>>>>>
>>>>> 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 <thomas.ab@samsung.com>
>>>>> Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com>
>>>>> [Alim: Added s2mps15_devs like rtc and clk and related changes]
>>>>> Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
>>>>> ---
>>>>>   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?
> 

Looks good to me. Please, go ahead.

Best regards,
Krzysztof
Lee Jones Oct. 28, 2015, 1:51 p.m. UTC | #8
On Wed, 28 Oct 2015, Krzysztof Kozlowski wrote:

> W dniu 28.10.2015 o 17:46, Lee Jones pisze:
> > 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 <thomas.ab@samsung.com>
> >>>>
> >>>> 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 <thomas.ab@samsung.com>
> >>>> Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com>
> >>>> [Alim: Added s2mps15_devs like rtc and clk and related changes]
> >>>> Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> >>>> ---
> >>>>  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.
> 
> Starting from the description of device-family. This is called "Power
> Management IC" but it is rather a "Power Deliver/Distribute IC". There
> isn't any logic inside except enable/disable/configure/set low power
> mode for regulators.
> 
> However in the same time the IC comes (always) with:
>  - 32kHz clocks,
>  - RTC,
>  - backup battery charger (no driver for it),
>  - reset for SoC,
>  - shutdown on thermal alert (also no driver for this control).
> 
> The solution a) seems fine to me. Make sense and it looks entirely
> backward compatible - only driver names will be modified.

Perfect solution from my PoV.

Thanks Krzysztof.
Alim Akhtar Oct. 29, 2015, 4:19 a.m. UTC | #9
On 10/28/2015 07:21 PM, Lee Jones wrote:
> On Wed, 28 Oct 2015, Krzysztof Kozlowski wrote:
>
>> W dniu 28.10.2015 o 17:46, Lee Jones pisze:
>>> 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 <thomas.ab@samsung.com>
>>>>>>
>>>>>> 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 <thomas.ab@samsung.com>
>>>>>> Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com>
>>>>>> [Alim: Added s2mps15_devs like rtc and clk and related changes]
>>>>>> Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
>>>>>> ---
>>>>>>   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.
>>
>> Starting from the description of device-family. This is called "Power
>> Management IC" but it is rather a "Power Deliver/Distribute IC". There
>> isn't any logic inside except enable/disable/configure/set low power
>> mode for regulators.
>>
>> However in the same time the IC comes (always) with:
>>   - 32kHz clocks,
>>   - RTC,
>>   - backup battery charger (no driver for it),
>>   - reset for SoC,
>>   - shutdown on thermal alert (also no driver for this control).
>>
>> The solution a) seems fine to me. Make sense and it looks entirely
>> backward compatible - only driver names will be modified.
>
> Perfect solution from my PoV.
>
> Thanks Krzysztof.
>
Thanks Jones/Krzysztof as always.
Will update the patches.
diff mbox

Patch

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 <linux/mfd/samsung/s2mps11.h>
 #include <linux/mfd/samsung/s2mps13.h>
 #include <linux/mfd/samsung/s2mps14.h>
+#include <linux/mfd/samsung/s2mps15.h>
 #include <linux/mfd/samsung/s2mpu02.h>
 #include <linux/mfd/samsung/s5m8763.h>
 #include <linux/mfd/samsung/s5m8767.h>
@@ -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 */