diff mbox

[1/3] regulator: s2mps11: Refactor setting ramp delay

Message ID 1401110423-5647-2-git-send-email-k.kozlowski@samsung.com (mailing list archive)
State New, archived
Headers show

Commit Message

Krzysztof Kozlowski May 26, 2014, 1:20 p.m. UTC 
Prepare for merging the s2mpa01 regulator driver into s2mps11 by:
1. Adding common id for buck regulators.
2. Splitting shared ramp delay settings to match S2MPA01.
3. Adding a configuration of registers for setting ramp delay for each
   buck regulator.

The functionality of the driver should not change as this patch only
prepares for supporting S2MPA01 device.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
---
 drivers/regulator/s2mps11.c | 210 ++++++++++++++++++++++++++++++--------------
 1 file changed, 144 insertions(+), 66 deletions(-)

Comments

Yadwinder Singh Brar May 27, 2014, 6:26 a.m. UTC | #1 
Hi Krzysztof,


On Mon, May 26, 2014 at 6:50 PM, Krzysztof Kozlowski
<k.kozlowski@samsung.com> wrote:
> Prepare for merging the s2mpa01 regulator driver into s2mps11 by:
> 1. Adding common id for buck regulators.
> 2. Splitting shared ramp delay settings to match S2MPA01.
> 3. Adding a configuration of registers for setting ramp delay for each
>    buck regulator.
>
> The functionality of the driver should not change as this patch only
> prepares for supporting S2MPA01 device.
>
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> ---
>  drivers/regulator/s2mps11.c | 210 ++++++++++++++++++++++++++++++--------------
>  1 file changed, 144 insertions(+), 66 deletions(-)
>

[snip]

>
> -               if (ramp_delay > s2mps11->ramp_delay34)
> -                       s2mps11->ramp_delay34 = ramp_delay;
> +               if (ramp_delay > s2mps11->ramp_delay3)
> +                       s2mps11->ramp_delay3 = ramp_delay;
>                 else
> -                       ramp_delay = s2mps11->ramp_delay34;
> -
> -               ramp_shift = S2MPS11_BUCK34_RAMP_SHIFT;
> -               ramp_reg = S2MPS11_REG_RAMP;
> +                       ramp_delay = s2mps11->ramp_delay3;
>                 break;
>         case S2MPS11_BUCK4:
> -               enable_shift = S2MPS11_BUCK4_RAMP_EN_SHIFT;
>                 if (!ramp_delay) {
>                         ramp_enable = 0;
>                         break;
>                 }
>
> -               if (ramp_delay > s2mps11->ramp_delay34)
> -                       s2mps11->ramp_delay34 = ramp_delay;
> +               if (ramp_delay > s2mps11->ramp_delay4)
> +                       s2mps11->ramp_delay4 = ramp_delay;
>                 else
> -                       ramp_delay = s2mps11->ramp_delay34;
> -
> -               ramp_shift = S2MPS11_BUCK34_RAMP_SHIFT;
> -               ramp_reg = S2MPS11_REG_RAMP;
> +                       ramp_delay = s2mps11->ramp_delay4;

Main rationale behind shared value is completely omitted here, in
other cases also,
after just giving a NOTE in documentation asking user to make sure to
pass same value.
It doesn't seem safe, simply leaving a scope of stability issue (in
case ramp_delay3 > ramp_delay4).

Regards,
Yadwinder
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Krzysztof Kozlowski May 27, 2014, 6:57 a.m. UTC | #2 
On wto, 2014-05-27 at 11:56 +0530, Yadwinder Singh Brar wrote:
> Hi Krzysztof,
> 
> 
> On Mon, May 26, 2014 at 6:50 PM, Krzysztof Kozlowski
> <k.kozlowski@samsung.com> wrote:
> > Prepare for merging the s2mpa01 regulator driver into s2mps11 by:
> > 1. Adding common id for buck regulators.
> > 2. Splitting shared ramp delay settings to match S2MPA01.
> > 3. Adding a configuration of registers for setting ramp delay for each
> >    buck regulator.
> >
> > The functionality of the driver should not change as this patch only
> > prepares for supporting S2MPA01 device.
> >
> > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> > ---
> >  drivers/regulator/s2mps11.c | 210 ++++++++++++++++++++++++++++++--------------
> >  1 file changed, 144 insertions(+), 66 deletions(-)
> >
> 
> [snip]
> 
> >
> > -               if (ramp_delay > s2mps11->ramp_delay34)
> > -                       s2mps11->ramp_delay34 = ramp_delay;
> > +               if (ramp_delay > s2mps11->ramp_delay3)
> > +                       s2mps11->ramp_delay3 = ramp_delay;
> >                 else
> > -                       ramp_delay = s2mps11->ramp_delay34;
> > -
> > -               ramp_shift = S2MPS11_BUCK34_RAMP_SHIFT;
> > -               ramp_reg = S2MPS11_REG_RAMP;
> > +                       ramp_delay = s2mps11->ramp_delay3;
> >                 break;
> >         case S2MPS11_BUCK4:
> > -               enable_shift = S2MPS11_BUCK4_RAMP_EN_SHIFT;
> >                 if (!ramp_delay) {
> >                         ramp_enable = 0;
> >                         break;
> >                 }
> >
> > -               if (ramp_delay > s2mps11->ramp_delay34)
> > -                       s2mps11->ramp_delay34 = ramp_delay;
> > +               if (ramp_delay > s2mps11->ramp_delay4)
> > +                       s2mps11->ramp_delay4 = ramp_delay;
> >                 else
> > -                       ramp_delay = s2mps11->ramp_delay34;
> > -
> > -               ramp_shift = S2MPS11_BUCK34_RAMP_SHIFT;
> > -               ramp_reg = S2MPS11_REG_RAMP;
> > +                       ramp_delay = s2mps11->ramp_delay4;
> 
> Main rationale behind shared value is completely omitted here, in
> other cases also,
> after just giving a NOTE in documentation asking user to make sure to
> pass same value.
> It doesn't seem safe, simply leaving a scope of stability issue (in
> case ramp_delay3 > ramp_delay4).

The documentation already states that these bucks (e.g. 3 and 4) share
the ramp delay setting and 'regulator-ramp-delay' should be the same.
However you're right that patch is not safe against changing shared ramp
delays to different values. Previously the code was safe so this is not
entirely "non-functional" change. I'll fix it to retain the same
behavior.

Best regards,
Krzysztof



--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/regulator/s2mps11.c b/drivers/regulator/s2mps11.c
index 02e2fb2fca66..fa12d75b784f 100644
--- a/drivers/regulator/s2mps11.c
+++ b/drivers/regulator/s2mps11.c
@@ -32,13 +32,32 @@ 
 #include <linux/mfd/samsung/s2mps11.h>
 #include <linux/mfd/samsung/s2mps14.h>
 
+/*
+ * Enum used as a common id of buck regulators on S2MPA01 and S2MPS11.
+ */
+enum s2mpx_bucks {
+	S2MPX_BUCK1,
+	S2MPX_BUCK2,
+	S2MPX_BUCK3,
+	S2MPX_BUCK4,
+	S2MPX_BUCK5,
+	S2MPX_BUCK6,
+	S2MPX_BUCK7,
+	S2MPX_BUCK8,
+	S2MPX_BUCK9,
+	S2MPX_BUCK10,
+};
+
 struct s2mps11_info {
+	enum sec_device_type dev_type;
 	unsigned int rdev_num;
 	int ramp_delay2;
-	int ramp_delay34;
+	int ramp_delay3;
+	int ramp_delay4;
 	int ramp_delay5;
 	int ramp_delay16;
-	int ramp_delay7810;
+	int ramp_delay7;
+	int ramp_delay810;
 	int ramp_delay9;
 	/*
 	 * One bit for each S2MPS14 regulator whether the suspend mode
@@ -49,6 +68,47 @@  struct s2mps11_info {
 	int *ext_control_gpio;
 };
 
+/*
+ * Description of ramp delay register for one buck regulator
+ * on S2MPA01 and S2MPS11.
+ */
+struct s2mpx_ramp_reg {
+	unsigned int ramp_shift;
+	unsigned int ramp_reg;
+	unsigned int enable_shift;
+	bool enable_supported;
+};
+
+#define s2mps11_ramp_reg(r_shift) {					\
+	.ramp_shift		= S2MPS11_ ## r_shift ## _RAMP_SHIFT,	\
+	.ramp_reg		= S2MPS11_REG_RAMP_BUCK,		\
+	.enable_shift		= 0,					\
+	.enable_supported	= false,				\
+}
+#define s2mps11_buck2346_ramp_reg(r_shift, r_reg, e_shift) {		\
+	.ramp_shift		= S2MPS11_ ## r_shift ## _RAMP_SHIFT,	\
+	.ramp_reg		= S2MPS11_REG_ ## r_reg,		\
+	.enable_shift		= S2MPS11_ ## e_shift ## _RAMP_EN_SHIFT,\
+	.enable_supported	= true,					\
+}
+
+static const struct s2mpx_ramp_reg s2mps11_ramp_regs[] = {
+	[S2MPX_BUCK1]	= s2mps11_ramp_reg(BUCK16),
+	[S2MPX_BUCK2]	= s2mps11_buck2346_ramp_reg(BUCK2, RAMP, BUCK2),
+	[S2MPX_BUCK3]	= s2mps11_buck2346_ramp_reg(BUCK34, RAMP, BUCK3),
+	[S2MPX_BUCK4]	= s2mps11_buck2346_ramp_reg(BUCK34, RAMP, BUCK4),
+	[S2MPX_BUCK5]	= s2mps11_ramp_reg(BUCK5),
+	[S2MPX_BUCK6]	= s2mps11_buck2346_ramp_reg(BUCK16, RAMP_BUCK, BUCK6),
+	[S2MPX_BUCK7]	= s2mps11_ramp_reg(BUCK7810),
+	[S2MPX_BUCK8]	= s2mps11_ramp_reg(BUCK7810),
+	[S2MPX_BUCK9]	= s2mps11_ramp_reg(BUCK9),
+	[S2MPX_BUCK10]	= s2mps11_ramp_reg(BUCK7810),
+};
+
+static const struct s2mpx_ramp_reg * const s2mpx_ramp_regs[] = {
+	[S2MPS11X] = s2mps11_ramp_regs,
+};
+
 static int get_ramp_delay(int ramp_delay)
 {
 	unsigned char cnt = 0;
@@ -68,6 +128,23 @@  static int get_ramp_delay(int ramp_delay)
 	return cnt;
 }
 
+/*
+ * Maps a buck regulator id to enum s2mpx_bucks.
+ * Valid only for buck regulators on on S2MPS11.
+ *
+ * Returns a value of enum s2mpx_bucks or -EINVAL.
+ */
+static int get_s2mpx_buck_id(enum sec_device_type dev_type,
+		int rdev_id)
+{
+	switch (dev_type) {
+	case S2MPS11X:
+		return rdev_id - S2MPS11_BUCK1;
+	default:
+		return -EINVAL;
+	}
+}
+
 static int s2mps11_regulator_set_voltage_time_sel(struct regulator_dev *rdev,
 				   unsigned int old_selector,
 				   unsigned int new_selector)
@@ -75,29 +152,40 @@  static int s2mps11_regulator_set_voltage_time_sel(struct regulator_dev *rdev,
 	struct s2mps11_info *s2mps11 = rdev_get_drvdata(rdev);
 	unsigned int ramp_delay = 0;
 	int old_volt, new_volt;
+	int buck_id = get_s2mpx_buck_id(s2mps11->dev_type, rdev_get_id(rdev));
 
-	switch (rdev_get_id(rdev)) {
-	case S2MPS11_BUCK2:
+	switch (buck_id) {
+	case S2MPX_BUCK2:
 		ramp_delay = s2mps11->ramp_delay2;
 		break;
-	case S2MPS11_BUCK3:
-	case S2MPS11_BUCK4:
-		ramp_delay = s2mps11->ramp_delay34;
+	case S2MPX_BUCK3:
+		ramp_delay = s2mps11->ramp_delay3;
 		break;
-	case S2MPS11_BUCK5:
+	case S2MPX_BUCK4:
+		ramp_delay = s2mps11->ramp_delay4;
+		break;
+	case S2MPX_BUCK5:
 		ramp_delay = s2mps11->ramp_delay5;
 		break;
-	case S2MPS11_BUCK6:
-	case S2MPS11_BUCK1:
+	case S2MPX_BUCK6:
+	case S2MPX_BUCK1:
 		ramp_delay = s2mps11->ramp_delay16;
 		break;
-	case S2MPS11_BUCK7:
-	case S2MPS11_BUCK8:
-	case S2MPS11_BUCK10:
-		ramp_delay = s2mps11->ramp_delay7810;
+	case S2MPX_BUCK7:
+		ramp_delay = s2mps11->ramp_delay7;
 		break;
-	case S2MPS11_BUCK9:
+	case S2MPX_BUCK8:
+	case S2MPX_BUCK10:
+		ramp_delay = s2mps11->ramp_delay810;
+		break;
+	case S2MPX_BUCK9:
 		ramp_delay = s2mps11->ramp_delay9;
+		break;
+	default:
+		dev_err(rdev_get_dev(rdev),
+				"Wrong regulator id %d for set_voltage_time_sel\n",
+				rdev_get_id(rdev));
+		return -EINVAL;
 	}
 
 	if (ramp_delay == 0)
@@ -112,66 +200,55 @@  static int s2mps11_regulator_set_voltage_time_sel(struct regulator_dev *rdev,
 static int s2mps11_set_ramp_delay(struct regulator_dev *rdev, int ramp_delay)
 {
 	struct s2mps11_info *s2mps11 = rdev_get_drvdata(rdev);
-	unsigned int ramp_val, ramp_shift, ramp_reg = S2MPS11_REG_RAMP_BUCK;
-	unsigned int ramp_enable = 1, enable_shift = 0;
+	const struct s2mpx_ramp_reg *ramp_reg;
+	unsigned int ramp_val;
+	unsigned int ramp_enable = 1;
+	int buck_id;
 	int ret;
 
-	switch (rdev_get_id(rdev)) {
-	case S2MPS11_BUCK1:
+	buck_id = get_s2mpx_buck_id(s2mps11->dev_type, rdev_get_id(rdev));
+
+	switch (buck_id) {
+	case S2MPX_BUCK1:
 		if (ramp_delay > s2mps11->ramp_delay16)
 			s2mps11->ramp_delay16 = ramp_delay;
 		else
 			ramp_delay = s2mps11->ramp_delay16;
-
-		ramp_shift = S2MPS11_BUCK16_RAMP_SHIFT;
 		break;
-	case S2MPS11_BUCK2:
-		enable_shift = S2MPS11_BUCK2_RAMP_EN_SHIFT;
+	case S2MPX_BUCK2:
 		if (!ramp_delay) {
 			ramp_enable = 0;
 			break;
 		}
 
 		s2mps11->ramp_delay2 = ramp_delay;
-		ramp_shift = S2MPS11_BUCK2_RAMP_SHIFT;
-		ramp_reg = S2MPS11_REG_RAMP;
 		break;
-	case S2MPS11_BUCK3:
-		enable_shift = S2MPS11_BUCK3_RAMP_EN_SHIFT;
+	case S2MPX_BUCK3:
 		if (!ramp_delay) {
 			ramp_enable = 0;
 			break;
 		}
 
-		if (ramp_delay > s2mps11->ramp_delay34)
-			s2mps11->ramp_delay34 = ramp_delay;
+		if (ramp_delay > s2mps11->ramp_delay3)
+			s2mps11->ramp_delay3 = ramp_delay;
 		else
-			ramp_delay = s2mps11->ramp_delay34;
-
-		ramp_shift = S2MPS11_BUCK34_RAMP_SHIFT;
-		ramp_reg = S2MPS11_REG_RAMP;
+			ramp_delay = s2mps11->ramp_delay3;
 		break;
 	case S2MPS11_BUCK4:
-		enable_shift = S2MPS11_BUCK4_RAMP_EN_SHIFT;
 		if (!ramp_delay) {
 			ramp_enable = 0;
 			break;
 		}
 
-		if (ramp_delay > s2mps11->ramp_delay34)
-			s2mps11->ramp_delay34 = ramp_delay;
+		if (ramp_delay > s2mps11->ramp_delay4)
+			s2mps11->ramp_delay4 = ramp_delay;
 		else
-			ramp_delay = s2mps11->ramp_delay34;
-
-		ramp_shift = S2MPS11_BUCK34_RAMP_SHIFT;
-		ramp_reg = S2MPS11_REG_RAMP;
+			ramp_delay = s2mps11->ramp_delay4;
 		break;
 	case S2MPS11_BUCK5:
 		s2mps11->ramp_delay5 = ramp_delay;
-		ramp_shift = S2MPS11_BUCK5_RAMP_SHIFT;
 		break;
 	case S2MPS11_BUCK6:
-		enable_shift = S2MPS11_BUCK6_RAMP_EN_SHIFT;
 		if (!ramp_delay) {
 			ramp_enable = 0;
 			break;
@@ -181,36 +258,37 @@  static int s2mps11_set_ramp_delay(struct regulator_dev *rdev, int ramp_delay)
 			s2mps11->ramp_delay16 = ramp_delay;
 		else
 			ramp_delay = s2mps11->ramp_delay16;
-
-		ramp_shift = S2MPS11_BUCK16_RAMP_SHIFT;
 		break;
 	case S2MPS11_BUCK7:
+		if (ramp_delay > s2mps11->ramp_delay7)
+			s2mps11->ramp_delay7 = ramp_delay;
+		else
+			ramp_delay = s2mps11->ramp_delay7;
+		break;
 	case S2MPS11_BUCK8:
 	case S2MPS11_BUCK10:
-		if (ramp_delay > s2mps11->ramp_delay7810)
-			s2mps11->ramp_delay7810 = ramp_delay;
+		if (ramp_delay > s2mps11->ramp_delay810)
+			s2mps11->ramp_delay810 = ramp_delay;
 		else
-			ramp_delay = s2mps11->ramp_delay7810;
-
-		ramp_shift = S2MPS11_BUCK7810_RAMP_SHIFT;
+			ramp_delay = s2mps11->ramp_delay810;
 		break;
 	case S2MPS11_BUCK9:
 		s2mps11->ramp_delay9 = ramp_delay;
-		ramp_shift = S2MPS11_BUCK9_RAMP_SHIFT;
 		break;
 	default:
 		return 0;
 	}
 
+	ramp_reg = &s2mpx_ramp_regs[s2mps11->dev_type][buck_id];
+
 	if (!ramp_enable)
 		goto ramp_disable;
 
 	/* Ramp delay can be enabled/disabled only for buck[2346] */
-	if ((rdev_get_id(rdev) >= S2MPS11_BUCK2 &&
-			rdev_get_id(rdev) <= S2MPS11_BUCK4) ||
-			rdev_get_id(rdev) == S2MPS11_BUCK6)  {
+	if (ramp_reg->enable_supported) {
 		ret = regmap_update_bits(rdev->regmap, S2MPS11_REG_RAMP,
-					 1 << enable_shift, 1 << enable_shift);
+					 1 << ramp_reg->enable_shift,
+					 1 << ramp_reg->enable_shift);
 		if (ret) {
 			dev_err(&rdev->dev, "failed to enable ramp rate\n");
 			return ret;
@@ -219,12 +297,13 @@  static int s2mps11_set_ramp_delay(struct regulator_dev *rdev, int ramp_delay)
 
 	ramp_val = get_ramp_delay(ramp_delay);
 
-	return regmap_update_bits(rdev->regmap, ramp_reg, 0x3 << ramp_shift,
-				  ramp_val << ramp_shift);
+	return regmap_update_bits(rdev->regmap, ramp_reg->ramp_reg,
+				0x3 << ramp_reg->ramp_shift,
+				ramp_val << ramp_reg->ramp_shift);
 
 ramp_disable:
 	return regmap_update_bits(rdev->regmap, S2MPS11_REG_RAMP,
-				  1 << enable_shift, 0);
+				  1 << ramp_reg->enable_shift, 0);
 }
 
 static struct regulator_ops s2mps11_ldo_ops = {
@@ -250,7 +329,7 @@  static struct regulator_ops s2mps11_buck_ops = {
 	.set_ramp_delay		= s2mps11_set_ramp_delay,
 };
 
-#define regulator_desc_s2mps11_ldo1(num)	{		\
+#define regulator_desc_s2mps11_ldo1(num) {		\
 	.name		= "LDO"#num,			\
 	.id		= S2MPS11_LDO##num,		\
 	.ops		= &s2mps11_ldo_ops,		\
@@ -605,8 +684,7 @@  static void s2mps14_pmic_dt_parse_ext_control_gpio(struct platform_device *pdev,
 }
 
 static int s2mps11_pmic_dt_parse(struct platform_device *pdev,
-		struct of_regulator_match *rdata, struct s2mps11_info *s2mps11,
-		enum sec_device_type dev_type)
+		struct of_regulator_match *rdata, struct s2mps11_info *s2mps11)
 {
 	struct device_node *reg_np;
 
@@ -617,7 +695,7 @@  static int s2mps11_pmic_dt_parse(struct platform_device *pdev,
 	}
 
 	of_regulator_match(&pdev->dev, reg_np, rdata, s2mps11->rdev_num);
-	if (dev_type == S2MPS14X)
+	if (s2mps11->dev_type == S2MPS14X)
 		s2mps14_pmic_dt_parse_ext_control_gpio(pdev, rdata, s2mps11);
 
 	of_node_put(reg_np);
@@ -634,15 +712,14 @@  static int s2mps11_pmic_probe(struct platform_device *pdev)
 	struct s2mps11_info *s2mps11;
 	int i, ret = 0;
 	const struct regulator_desc *regulators;
-	enum sec_device_type dev_type;
 
 	s2mps11 = devm_kzalloc(&pdev->dev, sizeof(struct s2mps11_info),
 				GFP_KERNEL);
 	if (!s2mps11)
 		return -ENOMEM;
 
-	dev_type = platform_get_device_id(pdev)->driver_data;
-	switch (dev_type) {
+	s2mps11->dev_type = platform_get_device_id(pdev)->driver_data;
+	switch (s2mps11->dev_type) {
 	case S2MPS11X:
 		s2mps11->rdev_num = ARRAY_SIZE(s2mps11_regulators);
 		regulators = s2mps11_regulators;
@@ -652,7 +729,8 @@  static int s2mps11_pmic_probe(struct platform_device *pdev)
 		regulators = s2mps14_regulators;
 		break;
 	default:
-		dev_err(&pdev->dev, "Invalid device type: %u\n", dev_type);
+		dev_err(&pdev->dev, "Invalid device type: %u\n",
+				s2mps11->dev_type);
 		return -EINVAL;
 	};
 
@@ -686,7 +764,7 @@  static int s2mps11_pmic_probe(struct platform_device *pdev)
 	for (i = 0; i < s2mps11->rdev_num; i++)
 		rdata[i].name = regulators[i].name;
 
-	ret = s2mps11_pmic_dt_parse(pdev, rdata, s2mps11, dev_type);
+	ret = s2mps11_pmic_dt_parse(pdev, rdata, s2mps11);
 	if (ret)
 		goto out;