[PATCH 0/4] Add DT support for s2mps11

[PATCH 0/4] Add DT support for s2mps11

[Yadwinder Singh Brar]

Mainly this series adds dt support for s2mps11 in mfd and regulator driver.
It also implements the set_voltage_time_sel() for bucks as we can not use
generic implementation as we need to take care of ramp_disable and ramp_delay
sharing by some bucks.

Yadwinder Singh Brar (4):
  regulator: s2mps11: Convert ramp rate to uV/us and set default ramp
    rate
  regulator: s2mps11: Implement set_voltage_time_sel() ops for bucks
  mfd: s2mps11: Add device tree support
  regulator: s2mps11: Add device tree support

 Documentation/devicetree/bindings/mfd/s2mps11.txt |   98 ++++++++++++
 drivers/mfd/sec-core.c                            |    3 +
 drivers/regulator/s2mps11.c                       |  176 +++++++++++++++++++--
 include/linux/mfd/samsung/s2mps11.h               |    1 +
 4 files changed, 264 insertions(+), 14 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mfd/s2mps11.txt

[PATCH 1/4] regulator: s2mps11: Convert ramp rate to uV/us and set default ramp rate

[Yadwinder Singh Brar]

This patch makes driver to use uV/us as units of ramp_delay. It makes driver
in compliance with regulator framework and make ramp rate precise.

This patch also sets default ramp rate in regulator descriptor which can be
used in case if case ramp rate is not set in regulator constraints.

Signed-off-by: Yadwinder Singh Brar <yadi.brar@samsung.com>
---
 drivers/regulator/s2mps11.c         |    7 ++++++-
 include/linux/mfd/samsung/s2mps11.h |    1 +
 2 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/drivers/regulator/s2mps11.c b/drivers/regulator/s2mps11.c
index cd9ea2e..0ec9aea 100644
--- a/drivers/regulator/s2mps11.c
+++ b/drivers/regulator/s2mps11.c
@@ -43,7 +43,7 @@ static int get_ramp_delay(int ramp_delay)
 {
 	unsigned char cnt = 0;
 
-	ramp_delay /= 6;
+	ramp_delay /= 6250;
 
 	while (true) {
 		ramp_delay = ramp_delay >> 1;
@@ -114,6 +114,7 @@ static struct regulator_ops s2mps11_buck_ops = {
 	.min_uV		= S2MPS11_BUCK_MIN1,			\
 	.uV_step	= S2MPS11_BUCK_STEP1,			\
 	.n_voltages	= S2MPS11_BUCK_N_VOLTAGES,		\
+	.ramp_delay	= S2MPS11_RAMP_DELAY,			\
 	.vsel_reg	= S2MPS11_REG_B1CTRL2 + (num - 1) * 2,	\
 	.vsel_mask	= S2MPS11_BUCK_VSEL_MASK,		\
 	.enable_reg	= S2MPS11_REG_B1CTRL1 + (num - 1) * 2,	\
@@ -129,6 +130,7 @@ static struct regulator_ops s2mps11_buck_ops = {
 	.min_uV		= S2MPS11_BUCK_MIN1,			\
 	.uV_step	= S2MPS11_BUCK_STEP1,			\
 	.n_voltages	= S2MPS11_BUCK_N_VOLTAGES,		\
+	.ramp_delay	= S2MPS11_RAMP_DELAY,			\
 	.vsel_reg	= S2MPS11_REG_B5CTRL2,			\
 	.vsel_mask	= S2MPS11_BUCK_VSEL_MASK,		\
 	.enable_reg	= S2MPS11_REG_B5CTRL1,			\
@@ -144,6 +146,7 @@ static struct regulator_ops s2mps11_buck_ops = {
 	.min_uV		= S2MPS11_BUCK_MIN1,			\
 	.uV_step	= S2MPS11_BUCK_STEP1,			\
 	.n_voltages	= S2MPS11_BUCK_N_VOLTAGES,		\
+	.ramp_delay	= S2MPS11_RAMP_DELAY,			\
 	.vsel_reg	= S2MPS11_REG_B6CTRL2 + (num - 6) * 2,	\
 	.vsel_mask	= S2MPS11_BUCK_VSEL_MASK,		\
 	.enable_reg	= S2MPS11_REG_B6CTRL1 + (num - 6) * 2,	\
@@ -159,6 +162,7 @@ static struct regulator_ops s2mps11_buck_ops = {
 	.min_uV		= S2MPS11_BUCK_MIN3,			\
 	.uV_step	= S2MPS11_BUCK_STEP3,			\
 	.n_voltages	= S2MPS11_BUCK_N_VOLTAGES,		\
+	.ramp_delay	= S2MPS11_RAMP_DELAY,			\
 	.vsel_reg	= S2MPS11_REG_B9CTRL2,			\
 	.vsel_mask	= S2MPS11_BUCK_VSEL_MASK,		\
 	.enable_reg	= S2MPS11_REG_B9CTRL1,			\
@@ -174,6 +178,7 @@ static struct regulator_ops s2mps11_buck_ops = {
 	.min_uV		= S2MPS11_BUCK_MIN2,			\
 	.uV_step	= S2MPS11_BUCK_STEP2,			\
 	.n_voltages	= S2MPS11_BUCK_N_VOLTAGES,		\
+	.ramp_delay	= S2MPS11_RAMP_DELAY,			\
 	.vsel_reg	= S2MPS11_REG_B10CTRL2,			\
 	.vsel_mask	= S2MPS11_BUCK_VSEL_MASK,		\
 	.enable_reg	= S2MPS11_REG_B10CTRL1,			\
diff --git a/include/linux/mfd/samsung/s2mps11.h b/include/linux/mfd/samsung/s2mps11.h
index ad2252f..4e94dc6 100644
--- a/include/linux/mfd/samsung/s2mps11.h
+++ b/include/linux/mfd/samsung/s2mps11.h
@@ -189,6 +189,7 @@ enum s2mps11_regulators {
 #define S2MPS11_ENABLE_SHIFT	0x06
 #define S2MPS11_LDO_N_VOLTAGES	(S2MPS11_LDO_VSEL_MASK + 1)
 #define S2MPS11_BUCK_N_VOLTAGES (S2MPS11_BUCK_VSEL_MASK + 1)
+#define S2MPS11_RAMP_DELAY	25000		/* uV/us */
 
 #define S2MPS11_PMIC_EN_SHIFT	6
 #define S2MPS11_REGULATOR_MAX (S2MPS11_REG_MAX - 3)
-- 
1.7.0.4

[PATCH 2/4] regulator: s2mps11: Implement set_voltage_time_sel() ops for bucks

[Yadwinder Singh Brar]

Currently driver uses local struct s2mps11_info to store ramp rate for bucks
whic its getting through platform data, so instead of using regulator
constraints it should use s2mps11_info to calculate ramp delay.

Signed-off-by: Yadwinder Singh Brar <yadi.brar@samsung.com>
---
 drivers/regulator/s2mps11.c |   53 ++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 52 insertions(+), 1 deletions(-)

diff --git a/drivers/regulator/s2mps11.c b/drivers/regulator/s2mps11.c
index 0ec9aea..d42ad2a 100644
--- a/drivers/regulator/s2mps11.c
+++ b/drivers/regulator/s2mps11.c
@@ -54,6 +54,57 @@ static int get_ramp_delay(int ramp_delay)
 	return cnt;
 }
 
+static int s2mps11_regulator_set_voltage_time_sel(struct regulator_dev *rdev,
+				   unsigned int old_selector,
+				   unsigned int new_selector)
+{
+	struct s2mps11_info *s2mps11 = rdev_get_drvdata(rdev);
+	unsigned int ramp_delay = 0;
+	int old_volt, new_volt;
+
+	switch (rdev->desc->id) {
+	case S2MPS11_BUCK2:
+		if (!s2mps11->buck2_ramp)
+			return 0;
+		ramp_delay = s2mps11->ramp_delay2;
+		break;
+	case S2MPS11_BUCK3:
+		if (!s2mps11->buck3_ramp)
+			return 0;
+		ramp_delay = s2mps11->ramp_delay34;
+		break;
+	case S2MPS11_BUCK4:
+		if (!s2mps11->buck4_ramp)
+			return 0;
+		ramp_delay = s2mps11->ramp_delay34;
+		break;
+	case S2MPS11_BUCK5:
+		ramp_delay = s2mps11->ramp_delay5;
+		break;
+	case S2MPS11_BUCK6:
+		if (!s2mps11->buck6_ramp)
+			return 0;
+	case S2MPS11_BUCK1:
+		ramp_delay = s2mps11->ramp_delay16;
+		break;
+	case S2MPS11_BUCK7:
+	case S2MPS11_BUCK8:
+	case S2MPS11_BUCK10:
+		ramp_delay = s2mps11->ramp_delay7810;
+		break;
+	case S2MPS11_BUCK9:
+		ramp_delay = s2mps11->ramp_delay9;
+	}
+
+	if (ramp_delay == 0)
+		ramp_delay = rdev->desc->ramp_delay;
+
+	old_volt = rdev->desc->min_uV + (rdev->desc->uV_step * old_selector);
+	new_volt = rdev->desc->min_uV + (rdev->desc->uV_step * new_selector);
+
+	return DIV_ROUND_UP(abs(new_volt - old_volt), ramp_delay);
+}
+
 static struct regulator_ops s2mps11_ldo_ops = {
 	.list_voltage		= regulator_list_voltage_linear,
 	.map_voltage		= regulator_map_voltage_linear,
@@ -73,7 +124,7 @@ static struct regulator_ops s2mps11_buck_ops = {
 	.disable		= regulator_disable_regmap,
 	.get_voltage_sel	= regulator_get_voltage_sel_regmap,
 	.set_voltage_sel	= regulator_set_voltage_sel_regmap,
-	.set_voltage_time_sel	= regulator_set_voltage_time_sel,
+	.set_voltage_time_sel	= s2mps11_regulator_set_voltage_time_sel,
 };
 
 #define regulator_desc_ldo1(num)	{		\
-- 
1.7.0.4

[PATCH 3/4] mfd: s2mps11: Add device tree support

[Yadwinder Singh Brar]

This patch adds DT compatible string for s2mps11 and binding documentation.

Signed-off-by: Yadwinder Singh Brar <yadi.brar@samsung.com>
---
 Documentation/devicetree/bindings/mfd/s2mps11.txt |   98 +++++++++++++++++++++
 drivers/mfd/sec-core.c                            |    3 +
 2 files changed, 101 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mfd/s2mps11.txt

diff --git a/Documentation/devicetree/bindings/mfd/s2mps11.txt b/Documentation/devicetree/bindings/mfd/s2mps11.txt
new file mode 100644
index 0000000..7984625
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/s2mps11.txt
@@ -0,0 +1,98 @@
+
+* Samsung S2MPS11 Voltage and Current Regulator
+
+The Samsung S2MP211 is a multi-function device which includes volatage and
+current regulators, rtc, charger controller and other sub-blocks. It is
+interfaced to the host controller using a i2c interface. Each sub-block is
+addressed by the host system using different i2c slave address.
+
+Required properties:
+- compatible: Should be "samsung,s2mps11-pmic".
+- reg: Specifies the i2c slave address of the pmic block. It should be 0x66.
+
+Optional properties:
+- interrupt-parent: Specifies the phandle of the interrupt controller to which
+  the interrupts from s2mps11 are delivered to.
+- interrupts: Interrupt specifiers for interrupt sources.
+
+Optional nodes:
+- regulators: The regulators of s2mps11 that have to be instantiated should be
+included in a sub-node named 'regulators'. Regulator nodes included in this
+sub-node should be of the format as listed below.
+
+	regulator_name {
+		[standard regulator constraints....];
+		[regulator-ramp-disable];
+	};
+
+ regulator-ramp-delay for BUCKs = [6250/12500/25000(default)/50000] uV/us
+
+ Optional property for BUCK[2/3/4/6] only:
+ regulator-ramp-disable: boolean, disables ramp delay on hardware.
+
+NOTE: Some BUCKs shares the ramp rate setting i.e. same ramp value will be set
+for a particular group of BUCKs. So provide same regulator-ramp-delay<value>.
+Grouping of BUCKs sharing ramp rate setting is as follow : BUCK[1, 6],
+BUCK[3, 4], and BUCK[7, 8, 10]
+
+The regulator constraints inside the regulator nodes use the standard regulator
+bindings which are documented elsewhere.
+
+The following are the names of the regulators that the s2mps11 pmic block
+supports. Note: The 'n' in LDOn and BUCKn represents the LDO or BUCK number
+as per the datasheet of s2mps11.
+
+	- LDOn
+		  - valid values for n are 1 to 28
+		  - Example: LDO0, LD01, LDO28
+	- BUCKn
+		  - valid values for n are 1 to 9.
+		  - Example: BUCK1, BUCK2, BUCK9
+
+Example:
+
+	s2mps11_pmic@66 {
+		compatible = "samsung,s2mps11-pmic";
+		reg = <0x66>;
+
+		regulators {
+			ldo1_reg: LDO1 {
+				regulator-name = "VDD_ABB_3.3V";
+				regulator-min-microvolt = <3300000>;
+				regulator-max-microvolt = <3300000>;
+			};
+
+			ldo2_reg: LDO2 {
+				regulator-name = "VDD_ALIVE_1.1V";
+				regulator-min-microvolt = <1100000>;
+				regulator-max-microvolt = <1100000>;
+				regulator-always-on;
+			};
+
+			buck1_reg: BUCK1 {
+				regulator-name = "vdd_mif";
+				regulator-min-microvolt = <950000>;
+				regulator-max-microvolt = <1350000>;
+				regulator-always-on;
+				regulator-boot-on;
+			};
+
+			buck2_reg: BUCK2 {
+				regulator-name = "vdd_arm";
+				regulator-min-microvolt = <950000>;
+				regulator-max-microvolt = <1350000>;
+				regulator-always-on;
+				regulator-boot-on;
+				regulator-ramp-delay = <50000>;
+			};
+
+			buck3_reg: BUCK3 {
+				regulator-name = "vdd_xxx";
+				regulator-min-microvolt = <950000>;
+				regulator-max-microvolt = <1350000>;
+				regulator-always-on;
+				regulator-boot-on;
+				regulator-ramp-disable;
+			};
+		};
+	};
diff --git a/drivers/mfd/sec-core.c b/drivers/mfd/sec-core.c
index 77ee26e..760da8a 100644
--- a/drivers/mfd/sec-core.c
+++ b/drivers/mfd/sec-core.c
@@ -66,6 +66,9 @@ static struct of_device_id sec_dt_match[] = {
 	{	.compatible = "samsung,s5m8767-pmic",
 		.data = (void *)S5M8767X,
 	},
+	{	.compatible = "samsung,s2mps11-pmic",
+		.data = (void *)S2MPS11X,
+	},
 	{},
 };
 #endif
-- 
1.7.0.4

[PATCH 4/4] regulator: s2mps11: Add device tree support

[Yadwinder Singh Brar]

This patch adds device tree support. It parses parent's node to get regulator
constraints. This patch also rearrange code little bit.

Signed-off-by: Yadwinder Singh Brar <yadi.brar@samsung.com>
---
 drivers/regulator/s2mps11.c |  116 ++++++++++++++++++++++++++++++++++++++-----
 1 files changed, 104 insertions(+), 12 deletions(-)

diff --git a/drivers/regulator/s2mps11.c b/drivers/regulator/s2mps11.c
index d42ad2a..559ad7b 100644
--- a/drivers/regulator/s2mps11.c
+++ b/drivers/regulator/s2mps11.c
@@ -20,9 +20,12 @@
 #include <linux/platform_device.h>
 #include <linux/regulator/driver.h>
 #include <linux/regulator/machine.h>
+#include <linux/regulator/of_regulator.h>
 #include <linux/mfd/samsung/core.h>
 #include <linux/mfd/samsung/s2mps11.h>
 
+#define S2MPS11_REGULATOR_CNT ARRAY_SIZE(regulators)
+
 struct s2mps11_info {
 	struct regulator_dev *rdev[S2MPS11_REGULATOR_MAX];
 
@@ -287,26 +290,107 @@ static struct regulator_desc regulators[] = {
 	regulator_desc_buck10,
 };
 
+static void s2mps11_pmic_set_buck_ramp(struct regulator_init_data *init_data,
+					struct s2mps11_info *s2mps11, int id)
+{
+	unsigned int ramp_rate;
+
+	if (!init_data)
+		return;
+
+	ramp_rate = init_data->constraints.ramp_delay;
+
+	switch (id) {
+	case S2MPS11_BUCK2:
+		s2mps11->ramp_delay2 = ramp_rate;
+		break;
+	case S2MPS11_BUCK3:
+	case S2MPS11_BUCK4:
+		if (ramp_rate > s2mps11->ramp_delay34)
+			s2mps11->ramp_delay34 = ramp_rate;
+		break;
+	case S2MPS11_BUCK5:
+		s2mps11->ramp_delay5 = ramp_rate;
+		break;
+	case S2MPS11_BUCK1:
+	case S2MPS11_BUCK6:
+		if (ramp_rate > s2mps11->ramp_delay16)
+			s2mps11->ramp_delay16 = ramp_rate;
+		break;
+	case S2MPS11_BUCK7:
+	case S2MPS11_BUCK8:
+	case S2MPS11_BUCK10:
+		if (ramp_rate > s2mps11->ramp_delay7810)
+			s2mps11->ramp_delay7810 = ramp_rate;
+		break;
+	case S2MPS11_BUCK9:
+		s2mps11->ramp_delay9 = ramp_rate;
+	}
+}
+
+static void s2mps11_pmic_parse_dt(struct of_regulator_match *rdata,
+					struct s2mps11_info *s2mps11)
+{
+	if (!of_find_property(rdata[S2MPS11_BUCK2].of_node,
+				"regulator-ramp-disable", NULL))
+		s2mps11->buck2_ramp = true;
+
+	if (!of_find_property(rdata[S2MPS11_BUCK3].of_node,
+				"regulator-ramp-disable", NULL))
+		s2mps11->buck3_ramp = true;
+
+	if (!of_find_property(rdata[S2MPS11_BUCK4].of_node,
+				"regulator-ramp-disable", NULL))
+		s2mps11->buck4_ramp = true;
+
+	if (!of_find_property(rdata[S2MPS11_BUCK6].of_node,
+				"regulator-ramp-disable", NULL))
+		s2mps11->buck6_ramp = true;
+}
+
 static int s2mps11_pmic_probe(struct platform_device *pdev)
 {
 	struct sec_pmic_dev *iodev = dev_get_drvdata(pdev->dev.parent);
 	struct sec_platform_data *pdata = dev_get_platdata(iodev->dev);
+	struct of_regulator_match rdata[S2MPS11_REGULATOR_MAX];
+	struct device_node *reg_np = NULL;
 	struct regulator_config config = { };
 	struct s2mps11_info *s2mps11;
-	int i, ret;
+	int i, id, ret;
 	unsigned char ramp_enable, ramp_reg = 0;
 
-	if (!pdata) {
-		dev_err(pdev->dev.parent, "Platform data not supplied\n");
-		return -ENODEV;
-	}
-
 	s2mps11 = devm_kzalloc(&pdev->dev, sizeof(struct s2mps11_info),
 				GFP_KERNEL);
 	if (!s2mps11)
 		return -ENOMEM;
 
-	platform_set_drvdata(pdev, s2mps11);
+	if (iodev->dev->of_node)
+		goto p_data;
+
+	for (i = 0; i < S2MPS11_REGULATOR_CNT; i++) {
+		id = regulators[i].id;
+		rdata[id].name = regulators[i].name;
+	}
+
+	reg_np = of_find_node_by_name(iodev->dev->of_node, "regulators");
+	if (!reg_np) {
+		dev_err(&pdev->dev, "could not find regulators sub-node\n");
+		return -EINVAL;
+	}
+
+	of_regulator_match(&pdev->dev, reg_np, rdata, S2MPS11_REGULATOR_MAX);
+
+	s2mps11_pmic_parse_dt(rdata, s2mps11);
+
+	for (id = S2MPS11_BUCK1; id <= S2MPS11_BUCK9; id++)
+		s2mps11_pmic_set_buck_ramp(rdata[id].init_data, s2mps11, id);
+
+	goto set_ramp;
+p_data:
+	if (!pdata) {
+		dev_err(pdev->dev.parent, "Platform data not supplied\n");
+		return -ENODEV;
+	}
 
 	s2mps11->ramp_delay2 = pdata->buck2_ramp_delay;
 	s2mps11->ramp_delay34 = pdata->buck34_ramp_delay;
@@ -320,6 +404,7 @@ static int s2mps11_pmic_probe(struct platform_device *pdev)
 	s2mps11->buck3_ramp = pdata->buck3_ramp_enable;
 	s2mps11->buck4_ramp = pdata->buck4_ramp_enable;
 
+set_ramp:
 	ramp_enable = (s2mps11->buck2_ramp << 3) | (s2mps11->buck3_ramp << 2) |
 		(s2mps11->buck4_ramp << 1) | s2mps11->buck6_ramp ;
 
@@ -338,12 +423,19 @@ static int s2mps11_pmic_probe(struct platform_device *pdev)
 	ramp_reg |= get_ramp_delay(s2mps11->ramp_delay9);
 	sec_reg_write(iodev, S2MPS11_REG_RAMP_BUCK, ramp_reg);
 
-	for (i = 0; i < S2MPS11_REGULATOR_MAX; i++) {
+	platform_set_drvdata(pdev, s2mps11);
 
-		config.dev = &pdev->dev;
-		config.regmap = iodev->regmap;
-		config.init_data = pdata->regulators[i].initdata;
-		config.driver_data = s2mps11;
+	config.dev = &pdev->dev;
+	config.regmap = iodev->regmap;
+	config.driver_data = s2mps11;
+	for (i = 0; i < S2MPS11_REGULATOR_MAX; i++) {
+		if (reg_np) {
+			config.init_data = pdata->regulators[i].initdata;
+		} else {
+			id = regulators[i].id;
+			config.init_data = rdata[id].init_data;
+			config.of_node = rdata[id].of_node;
+		}
 
 		s2mps11->rdev[i] = regulator_register(&regulators[i], &config);
 		if (IS_ERR(s2mps11->rdev[i])) {
-- 
1.7.0.4

Re: [PATCH 3/4] mfd: s2mps11: Add device tree support

[Sachin Kamat]

On 24 June 2013 16:50, Yadwinder Singh Brar <yadi.brar@samsung.com> wrote:
> This patch adds DT compatible string for s2mps11 and binding documentation.
>
> Signed-off-by: Yadwinder Singh Brar <yadi.brar@samsung.com>
> ---
>  Documentation/devicetree/bindings/mfd/s2mps11.txt |   98 +++++++++++++++++++++
>  drivers/mfd/sec-core.c                            |    3 +
>  2 files changed, 101 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/mfd/s2mps11.txt
>
> diff --git a/Documentation/devicetree/bindings/mfd/s2mps11.txt b/Documentation/devicetree/bindings/mfd/s2mps11.txt
> new file mode 100644
> index 0000000..7984625
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/s2mps11.txt
> @@ -0,0 +1,98 @@
> +
> +* Samsung S2MPS11 Voltage and Current Regulator
> +
> +The Samsung S2MP211 is a multi-function device which includes volatage and

s/volatage/voltage

> +current regulators, rtc, charger controller and other sub-blocks. It is

s/rtc/RTC

> +interfaced to the host controller using a i2c interface. Each sub-block is
> +addressed by the host system using different i2c slave address.

s/i2c/I2C -> also elsewhere

> +
> +Required properties:
> +- compatible: Should be "samsung,s2mps11-pmic".
> +- reg: Specifies the i2c slave address of the pmic block. It should be 0x66.
> +
> +Optional properties:
> +- interrupt-parent: Specifies the phandle of the interrupt controller to which
> +  the interrupts from s2mps11 are delivered to.
> +- interrupts: Interrupt specifiers for interrupt sources.
> +
> +Optional nodes:
> +- regulators: The regulators of s2mps11 that have to be instantiated should be
> +included in a sub-node named 'regulators'. Regulator nodes included in this
> +sub-node should be of the format as listed below.
> +
> +       regulator_name {
> +               [standard regulator constraints....];
> +               [regulator-ramp-disable];
> +       };
> +
> + regulator-ramp-delay for BUCKs = [6250/12500/25000(default)/50000] uV/us
> +
> + Optional property for BUCK[2/3/4/6] only:
> + regulator-ramp-disable: boolean, disables ramp delay on hardware.
> +
> +NOTE: Some BUCKs shares the ramp rate setting i.e. same ramp value will be set

s/shares/share

-- 
With warm regards,
Sachin

Re: [PATCH 4/4] regulator: s2mps11: Add device tree support

[Sachin Kamat]

> +static void s2mps11_pmic_set_buck_ramp(struct regulator_init_data *init_data,
> +                                       struct s2mps11_info *s2mps11, int id)
> +{
> +       unsigned int ramp_rate;
> +
> +       if (!init_data)
> +               return;
> +
> +       ramp_rate = init_data->constraints.ramp_delay;
> +
> +       switch (id) {
> +       case S2MPS11_BUCK2:
> +               s2mps11->ramp_delay2 = ramp_rate;
> +               break;
> +       case S2MPS11_BUCK3:
> +       case S2MPS11_BUCK4:
> +               if (ramp_rate > s2mps11->ramp_delay34)
> +                       s2mps11->ramp_delay34 = ramp_rate;
> +               break;
> +       case S2MPS11_BUCK5:
> +               s2mps11->ramp_delay5 = ramp_rate;
> +               break;
> +       case S2MPS11_BUCK1:
> +       case S2MPS11_BUCK6:
> +               if (ramp_rate > s2mps11->ramp_delay16)
> +                       s2mps11->ramp_delay16 = ramp_rate;
> +               break;
> +       case S2MPS11_BUCK7:
> +       case S2MPS11_BUCK8:
> +       case S2MPS11_BUCK10:
> +               if (ramp_rate > s2mps11->ramp_delay7810)
> +                       s2mps11->ramp_delay7810 = ramp_rate;
> +               break;
> +       case S2MPS11_BUCK9:
> +               s2mps11->ramp_delay9 = ramp_rate;

How about adding a 'break' here for completeness.

[snip]

>  static int s2mps11_pmic_probe(struct platform_device *pdev)
>  {
>         struct sec_pmic_dev *iodev = dev_get_drvdata(pdev->dev.parent);
>         struct sec_platform_data *pdata = dev_get_platdata(iodev->dev);
> +       struct of_regulator_match rdata[S2MPS11_REGULATOR_MAX];
> +       struct device_node *reg_np = NULL;
>         struct regulator_config config = { };
>         struct s2mps11_info *s2mps11;
> -       int i, ret;
> +       int i, id, ret;
>         unsigned char ramp_enable, ramp_reg = 0;
>
> -       if (!pdata) {
> -               dev_err(pdev->dev.parent, "Platform data not supplied\n");
> -               return -ENODEV;
> -       }
> -
>         s2mps11 = devm_kzalloc(&pdev->dev, sizeof(struct s2mps11_info),

sizeof(*s2mps11) is preferred.

-- 
With warm regards,
Sachin

Re: [PATCH 2/4] regulator: s2mps11: Implement set_voltage_time_sel() ops for bucks

[Sachin Kamat]

On 24 June 2013 16:50, Yadwinder Singh Brar <yadi.brar@samsung.com> wrote:
> Currently driver uses local struct s2mps11_info to store ramp rate for bucks
> whic its getting through platform data, so instead of using regulator
> constraints it should use s2mps11_info to calculate ramp delay.
>
> Signed-off-by: Yadwinder Singh Brar <yadi.brar@samsung.com>
[snip]
> +
> +       switch (rdev->desc->id) {
> +       case S2MPS11_BUCK2:
> +               if (!s2mps11->buck2_ramp)
> +                       return 0;
> +               ramp_delay = s2mps11->ramp_delay2;
> +               break;
> +       case S2MPS11_BUCK3:
> +               if (!s2mps11->buck3_ramp)
> +                       return 0;
> +               ramp_delay = s2mps11->ramp_delay34;
> +               break;
> +       case S2MPS11_BUCK4:
> +               if (!s2mps11->buck4_ramp)
> +                       return 0;
> +               ramp_delay = s2mps11->ramp_delay34;
> +               break;
> +       case S2MPS11_BUCK5:
> +               ramp_delay = s2mps11->ramp_delay5;
> +               break;
> +       case S2MPS11_BUCK6:
> +               if (!s2mps11->buck6_ramp)
> +                       return 0;
> +       case S2MPS11_BUCK1:

nit: Why not have this at the beginning?

> +               ramp_delay = s2mps11->ramp_delay16;
> +               break;
> +       case S2MPS11_BUCK7:
> +       case S2MPS11_BUCK8:
> +       case S2MPS11_BUCK10:
> +               ramp_delay = s2mps11->ramp_delay7810;
> +               break;
> +       case S2MPS11_BUCK9:
> +               ramp_delay = s2mps11->ramp_delay9;
> +       }

How about adding a break statement above?

-- 
With warm regards,
Sachin

Re: [PATCH 2/4] regulator: s2mps11: Implement set_voltage_time_sel() ops for bucks

[Yadwinder Singh Brar]

On Wed, Jul 3, 2013 at 5:25 PM, Sachin Kamat <sachin.kamat@linaro.org> wrote:
> On 24 June 2013 16:50, Yadwinder Singh Brar <yadi.brar@samsung.com> wrote:
>> Currently driver uses local struct s2mps11_info to store ramp rate for bucks
>> whic its getting through platform data, so instead of using regulator
>> constraints it should use s2mps11_info to calculate ramp delay.
>>
>> Signed-off-by: Yadwinder Singh Brar <yadi.brar@samsung.com>
> [snip]
>> +
>> +       switch (rdev->desc->id) {
>> +       case S2MPS11_BUCK2:
>> +               if (!s2mps11->buck2_ramp)
>> +                       return 0;
>> +               ramp_delay = s2mps11->ramp_delay2;
>> +               break;
>> +       case S2MPS11_BUCK3:
>> +               if (!s2mps11->buck3_ramp)
>> +                       return 0;
>> +               ramp_delay = s2mps11->ramp_delay34;
>> +               break;
>> +       case S2MPS11_BUCK4:
>> +               if (!s2mps11->buck4_ramp)
>> +                       return 0;
>> +               ramp_delay = s2mps11->ramp_delay34;
>> +               break;
>> +       case S2MPS11_BUCK5:
>> +               ramp_delay = s2mps11->ramp_delay5;
>> +               break;
>> +       case S2MPS11_BUCK6:
>> +               if (!s2mps11->buck6_ramp)
>> +                       return 0;
>> +       case S2MPS11_BUCK1:
>
> nit: Why not have this at the beginning?
>

Nothing special. Instead of putting "case S2MPS11_BUCK6" at beginning,
I preferred to put S2MPS11_BUCK1 here.

>> +               ramp_delay = s2mps11->ramp_delay16;
>> +               break;
>> +       case S2MPS11_BUCK7:
>> +       case S2MPS11_BUCK8:
>> +       case S2MPS11_BUCK10:
>> +               ramp_delay = s2mps11->ramp_delay7810;
>> +               break;
>> +       case S2MPS11_BUCK9:
>> +               ramp_delay = s2mps11->ramp_delay9;
>> +       }
>
> How about adding a break statement above?
>

hmm .. I can't see any worth of it.
Is it required according to linux coding convention ?


Regards,
Yadwinder

--
> With warm regards,
> Sachin

Re: [PATCH 4/4] regulator: s2mps11: Add device tree support

[Yadwinder Singh Brar]

On Wed, Jul 3, 2013 at 5:23 PM, Sachin Kamat <sachin.kamat@linaro.org> wrote:
>> +static void s2mps11_pmic_set_buck_ramp(struct regulator_init_data *init_data,
>> +                                       struct s2mps11_info *s2mps11, int id)
>> +{
>> +       unsigned int ramp_rate;
>> +
>> +       if (!init_data)
>> +               return;
>> +
>> +       ramp_rate = init_data->constraints.ramp_delay;
>> +
>> +       switch (id) {
>> +       case S2MPS11_BUCK2:
>> +               s2mps11->ramp_delay2 = ramp_rate;
>> +               break;
>> +       case S2MPS11_BUCK3:
>> +       case S2MPS11_BUCK4:
>> +               if (ramp_rate > s2mps11->ramp_delay34)
>> +                       s2mps11->ramp_delay34 = ramp_rate;
>> +               break;
>> +       case S2MPS11_BUCK5:
>> +               s2mps11->ramp_delay5 = ramp_rate;
>> +               break;
>> +       case S2MPS11_BUCK1:
>> +       case S2MPS11_BUCK6:
>> +               if (ramp_rate > s2mps11->ramp_delay16)
>> +                       s2mps11->ramp_delay16 = ramp_rate;
>> +               break;
>> +       case S2MPS11_BUCK7:
>> +       case S2MPS11_BUCK8:
>> +       case S2MPS11_BUCK10:
>> +               if (ramp_rate > s2mps11->ramp_delay7810)
>> +                       s2mps11->ramp_delay7810 = ramp_rate;
>> +               break;
>> +       case S2MPS11_BUCK9:
>> +               s2mps11->ramp_delay9 = ramp_rate;
>
> How about adding a 'break' here for completeness.
>
> [snip]
>
>>  static int s2mps11_pmic_probe(struct platform_device *pdev)
>>  {
>>         struct sec_pmic_dev *iodev = dev_get_drvdata(pdev->dev.parent);
>>         struct sec_platform_data *pdata = dev_get_platdata(iodev->dev);
>> +       struct of_regulator_match rdata[S2MPS11_REGULATOR_MAX];
>> +       struct device_node *reg_np = NULL;
>>         struct regulator_config config = { };
>>         struct s2mps11_info *s2mps11;
>> -       int i, ret;
>> +       int i, id, ret;
>>         unsigned char ramp_enable, ramp_reg = 0;
>>
>> -       if (!pdata) {
>> -               dev_err(pdev->dev.parent, "Platform data not supplied\n");
>> -               return -ENODEV;
>> -       }
>> -
>>         s2mps11 = devm_kzalloc(&pdev->dev, sizeof(struct s2mps11_info),
>
> sizeof(*s2mps11) is preferred.
>

Will fix this and spellings in documentation in next version after
waiting some time for review comments from others also.


Regards,
Yadwinder

> --
> With warm regards,
> Sachin

Re: [PATCH 1/4] regulator: s2mps11: Convert ramp rate to uV/us and set default ramp rate

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 236 bytes --]

On Mon, Jun 24, 2013 at 04:50:55PM +0530, Yadwinder Singh Brar wrote:
> This patch makes driver to use uV/us as units of ramp_delay. It makes driver
> in compliance with regulator framework and make ramp rate precise.

Applied, thanks.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/4] regulator: s2mps11: Implement set_voltage_time_sel() ops for bucks

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 305 bytes --]

On Mon, Jun 24, 2013 at 04:50:56PM +0530, Yadwinder Singh Brar wrote:
> Currently driver uses local struct s2mps11_info to store ramp rate for bucks
> whic its getting through platform data, so instead of using regulator
> constraints it should use s2mps11_info to calculate ramp delay.

Applied, thanks.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 4/4] regulator: s2mps11: Add device tree support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 660 bytes --]

On Mon, Jun 24, 2013 at 04:50:58PM +0530, Yadwinder Singh Brar wrote:

> +static void s2mps11_pmic_parse_dt(struct of_regulator_match *rdata,
> +					struct s2mps11_info *s2mps11)
> +{
> +	if (!of_find_property(rdata[S2MPS11_BUCK2].of_node,
> +				"regulator-ramp-disable", NULL))
> +		s2mps11->buck2_ramp = true;

This property should be specific to this binding rather than named as a
generic regulator property.  It's also a bit odd that setting a property
called "regulator-ramp-disable" causes the flag buckN_ramp to be set to
true - if the ramp is being disabled I'd expect a flag with that name to
be false not true.  Seems something isn't named right.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 3/4] mfd: s2mps11: Add device tree support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 830 bytes --]

On Mon, Jun 24, 2013 at 04:50:57PM +0530, Yadwinder Singh Brar wrote:

> + regulator-ramp-delay for BUCKs = [6250/12500/25000(default)/50000] uV/us

> + Optional property for BUCK[2/3/4/6] only:
> + regulator-ramp-disable: boolean, disables ramp delay on hardware.

If these were device specific bindings they ought to be namespaced as
samsung, or similar.  However I think they are generic enough that they
could just be added to the standard regulator bindings, ramp rates are
common enough and units of time are a simple way to express them.  I'd
also suggest just using a ramp delay of zero for no ramp, it's simpler
and will probably fall naturally out of the bindings.

Please just send a separate patch adding these to the generic regulator
binding document instead of this one, we can add generic code/ops for
this later.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 3/4] mfd: s2mps11: Add device tree support

[Yadwinder Singh Brar]

On Wed, Jul 3, 2013 at 11:35 PM, Mark Brown <broonie@kernel.org> wrote:
> On Mon, Jun 24, 2013 at 04:50:57PM +0530, Yadwinder Singh Brar wrote:
>
>> + regulator-ramp-delay for BUCKs = [6250/12500/25000(default)/50000] uV/us
>
>> + Optional property for BUCK[2/3/4/6] only:
>> + regulator-ramp-disable: boolean, disables ramp delay on hardware.
>
> If these were device specific bindings they ought to be namespaced as
> samsung, or similar.  However I think they are generic enough that they
> could just be added to the standard regulator bindings, ramp rates are
> common enough and units of time are a simple way to express them.  I'd
> also suggest just using a ramp delay of zero for no ramp, it's simpler
> and will probably fall naturally out of the bindings.
>

Yes, I was also expecting to see regulator-ramp-disable as generic binding.
Before preparing this patch I thought of using regulator-ramp-delay = 0,
but I found it inappropriate because :
-  Currently ramp-delay (= 0) if not defined in DT, leaves the
hardware with default
ramp settings, and uses default ramp-rate defined in rdev->desc . So we need
an extra mechanism(either special(-ve) value of ramp-delay or an extra flag
(ramp_disable) in regulator constraints) to figure out whether ramp-rate is
actually set to zero or its left (by default) zero.
- As ramp time is inversely propositional to ramp-rate(i.e. ramp-delay ,
its wrongly named, my mistake :( ) so it may look weired to use ramp-rate =0
as no ramp(ramp_time = 0).

> Please just send a separate patch adding these to the generic regulator
> binding document instead of this one, we can add generic code/ops for
> this later.

I had below idea in my mind, I dropped it because I thought it may be
to early to put common code as no other driver in mainline appeared
with this requirement yet but probably may come.

8<-----------------------------------------------------------------------

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 815d6df..cf73e7c 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2424,7 +2424,8 @@ static int _regulator_do_set_voltage(struct regulator_dev

        /* Call set_voltage_time_sel if successfully obtained old_selector */
        if (ret == 0 && _regulator_is_enabled(rdev) && old_selector >= 0 &&
-           old_selector != selector && rdev->desc->ops->set_voltage_time_sel)
+           old_selector != selector && rdev->desc->ops->set_voltage_time_sel
+           && !rdev->constraints->ramp_disable) {

                delay = rdev->desc->ops->set_voltage_time_sel(rdev,
                                                old_selector, selector);
diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.
index 66ca769..c579aca 100644
--- a/drivers/regulator/of_regulator.c
+++ b/drivers/regulator/of_regulator.c
@@ -64,6 +64,9 @@ static void of_get_regulation_constraints(struct device_node
        ramp_delay = of_get_property(np, "regulator-ramp-delay", NULL);
        if (ramp_delay)
                constraints->ramp_delay = be32_to_cpu(*ramp_delay);
+
+       if (of_find_property(np, "regulator-ramp-disable", NULL))
+               constraints->ramp_disable = true;
 }

 /**
diff --git a/include/linux/regulator/machine.h b/include/linux/regulator/machin
index 36adbc8..02592da 100644
--- a/include/linux/regulator/machine.h
+++ b/include/linux/regulator/machine.h
@@ -134,6 +134,7 @@ struct regulation_constraints {
        unsigned always_on:1;   /* regulator never off when system is on */
        unsigned boot_on:1;     /* bootloader/firmware enabled regulator */
        unsigned apply_uV:1;    /* apply uV constraint if min == max */
+       unsigned int ramp_disable:1;
 };

 /**


Regards,
Yadwinder

Re: [PATCH 4/4] regulator: s2mps11: Add device tree support

[Yadwinder Singh Brar]

On Wed, Jul 3, 2013 at 11:30 PM, Mark Brown <broonie@kernel.org> wrote:
> On Mon, Jun 24, 2013 at 04:50:58PM +0530, Yadwinder Singh Brar wrote:
>
>> +static void s2mps11_pmic_parse_dt(struct of_regulator_match *rdata,
>> +                                     struct s2mps11_info *s2mps11)
>> +{
>> +     if (!of_find_property(rdata[S2MPS11_BUCK2].of_node,
>> +                             "regulator-ramp-disable", NULL))
>> +             s2mps11->buck2_ramp = true;
>
> This property should be specific to this binding rather than named as a
> generic regulator property.  It's also a bit odd that setting a property
> called "regulator-ramp-disable" causes the flag buckN_ramp to be set to
> true - if the ramp is being disabled I'd expect a flag with that name to
> be false not true.  Seems something isn't named right.

Yes, it seems incorrect but in existing code based on pdata, its name like that.
If you insist, I can rename it ? as It seems sensible to put
"regulator-ramp-disable" as our intension is to do that(by default its
always enable).


Thanks,
Yadwinder

Re: [PATCH 3/4] mfd: s2mps11: Add device tree support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 946 bytes --]

On Thu, Jul 04, 2013 at 10:37:30AM +0530, Yadwinder Singh Brar wrote:

> -  Currently ramp-delay (= 0) if not defined in DT, leaves the
> hardware with default

That's just an issue in the code if that is the case, you can test for
the presence of a property independently of getting its value.

> - As ramp time is inversely propositional to ramp-rate(i.e. ramp-delay ,
> its wrongly named, my mistake :( ) so it may look weired to use ramp-rate =0
> as no ramp(ramp_time = 0).

I think it's fairly obvious what's going on there, it fits in with the
general pattern that a lower number is faster too.

> I had below idea in my mind, I dropped it because I thought it may be
> to early to put common code as no other driver in mainline appeared
> with this requirement yet but probably may come.

There are plenty of drivers that could configure this, it's just that
it's normally done either in hardware or the bootloader rather than by
the OS.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 4/4] regulator: s2mps11: Add device tree support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 388 bytes --]

On Thu, Jul 04, 2013 at 10:44:36AM +0530, Yadwinder Singh Brar wrote:

> Yes, it seems incorrect but in existing code based on pdata, its name like that.
> If you insist, I can rename it ? as It seems sensible to put
> "regulator-ramp-disable" as our intension is to do that(by default its
> always enable).

It seems sensible to rename yes, otherwise this code just looks totally
wrong.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 3/4] mfd: s2mps11: Add device tree support

[Yadwinder Singh Brar]

On Thu, Jul 4, 2013 at 2:52 PM, Mark Brown <broonie@kernel.org> wrote:
> On Thu, Jul 04, 2013 at 10:37:30AM +0530, Yadwinder Singh Brar wrote:
>
>> -  Currently ramp-delay (= 0) if not defined in DT, leaves the
>> hardware with default
>
> That's just an issue in the code if that is the case, you can test for
> the presence of a property independently of getting its value.
>

Yes, for that we will need an extra flag (ramp_disable) in
constraints, to figure out whether
ramp-rate is actually set to zero or its left (by default) zero if we
do it in common code as
now we have locally in driver.

>> - As ramp time is inversely propositional to ramp-rate(i.e. ramp-delay ,
>> its wrongly named, my mistake :( ) so it may look weired to use ramp-rate =0
>> as no ramp(ramp_time = 0).
>
> I think it's fairly obvious what's going on there, it fits in with the
> general pattern that a lower number is faster too.
>

yes, lower number(ramp_time) is faster, but I meant to say that lower
ramp-rate means higher ramp_time.

I think its matter of assumption, so to conclude our discussion,
please let me know that which approach we should use:
- assume "regulator-ramp-delay = <0>" as ramp_disable.
or
- parsing "regulator_ramp_disable" from DT.



Regards,
Yadwinder

Re: [PATCH 3/4] mfd: s2mps11: Add device tree support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 828 bytes --]

On Thu, Jul 04, 2013 at 05:13:46PM +0530, Yadwinder Singh Brar wrote:
> On Thu, Jul 4, 2013 at 2:52 PM, Mark Brown <broonie@kernel.org> wrote:

> > That's just an issue in the code if that is the case, you can test for
> > the presence of a property independently of getting its value.

> Yes, for that we will need an extra flag (ramp_disable) in
> constraints, to figure out whether
> ramp-rate is actually set to zero or its left (by default) zero if we
> do it in common code as
> now we have locally in driver.

Yes, I agree that makes sense for the constraints.

> I think its matter of assumption, so to conclude our discussion,
> please let me know that which approach we should use:
> - assume "regulator-ramp-delay = <0>" as ramp_disable.
> or
> - parsing "regulator_ramp_disable" from DT.

I prefer the first option.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]