[PATCH 0/3] regulator: s2mps11: Minor fixes and prepare for S2MPS14

[PATCH 0/3] regulator: s2mps11: Minor fixes and prepare for S2MPS14

[Krzysztof Kozlowski]

Hi,

Few days ago I sent patches adding support for S2MPS14 device to the
sec-core/s2mps11 drivers. These patches were rather large as they covered
multiple subsystems so I decided to split everything into smaller, separate
patches.

The original patchset (version 3) can be found here:
http://thread.gmane.org/gmane.linux.kernel.samsung-soc/27194/focus=1649217

This patchset is a small subset of the patchset related to the s2mps11
regulator driver. The third patch here prepares for supporting the S2MPS14
device. I would be glad if it comes in however I understand that without
further patches it is a little premature.

Best regards,
Krzysztof

Krzysztof Kozlowski (3):
  regulator: s2mps11: Don't store registered regulators in state
    container
  regulator: s2mps11: Constify regulator_desc array
  regulator: s2mps11: Copy supported regulators from initconst

 drivers/regulator/s2mps11.c |   85 ++++++++++++++++++++++++++++++++++---------
 1 file changed, 67 insertions(+), 18 deletions(-)

-- 
1.7.9.5

[PATCH 1/3] regulator: s2mps11: Don't store registered regulators in state container

[Krzysztof Kozlowski]

Regulators registered by devm_regulator_register() do not have to be
stored in state container because they are never dereferenced later.
The array of regulator_dev can be safely removed from state container.

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

diff --git a/drivers/regulator/s2mps11.c b/drivers/regulator/s2mps11.c
index cd0b9e35a56d..3647d590b962 100644
--- a/drivers/regulator/s2mps11.c
+++ b/drivers/regulator/s2mps11.c
@@ -28,8 +28,6 @@
 #define S2MPS11_REGULATOR_CNT ARRAY_SIZE(regulators)
 
 struct s2mps11_info {
-	struct regulator_dev *rdev[S2MPS11_REGULATOR_MAX];
-
 	int ramp_delay2;
 	int ramp_delay34;
 	int ramp_delay5;
@@ -439,6 +437,8 @@ common_reg:
 	config.regmap = iodev->regmap_pmic;
 	config.driver_data = s2mps11;
 	for (i = 0; i < S2MPS11_REGULATOR_MAX; i++) {
+		struct regulator_dev *regulator;
+
 		if (!reg_np) {
 			config.init_data = pdata->regulators[i].initdata;
 			config.of_node = pdata->regulators[i].reg_node;
@@ -447,10 +447,10 @@ common_reg:
 			config.of_node = rdata[i].of_node;
 		}
 
-		s2mps11->rdev[i] = devm_regulator_register(&pdev->dev,
+		regulator = devm_regulator_register(&pdev->dev,
 						&regulators[i], &config);
-		if (IS_ERR(s2mps11->rdev[i])) {
-			ret = PTR_ERR(s2mps11->rdev[i]);
+		if (IS_ERR(regulator)) {
+			ret = PTR_ERR(regulator);
 			dev_err(&pdev->dev, "regulator init failed for %d\n",
 				i);
 			return ret;
-- 
1.7.9.5

[PATCH 2/3] regulator: s2mps11: Constify regulator_desc array

[Krzysztof Kozlowski]

Constify the regulator_desc 'regulators' array.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
---
 drivers/regulator/s2mps11.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/regulator/s2mps11.c b/drivers/regulator/s2mps11.c
index 3647d590b962..189038aecbf5 100644
--- a/drivers/regulator/s2mps11.c
+++ b/drivers/regulator/s2mps11.c
@@ -343,7 +343,7 @@ static struct regulator_ops s2mps11_buck_ops = {
 	.enable_mask	= S2MPS11_ENABLE_MASK			\
 }
 
-static struct regulator_desc regulators[] = {
+static const struct regulator_desc regulators[] = {
 	regulator_desc_ldo2(1),
 	regulator_desc_ldo1(2),
 	regulator_desc_ldo1(3),
-- 
1.7.9.5

[PATCH 3/3] regulator: s2mps11: Copy supported regulators from initconst

[Krzysztof Kozlowski]

Add __initconst to 'regulator_desc' array with supported regulators.
During probe choose how many and which regulators will be supported
according to device ID. Then copy the 'regulator_desc' array to
allocated memory so the regulator core can use it.

Additionally allocate array of of_regulator_match() dynamically (based
on number of regulators) instead of allocation on the stack.

This is needed for supporting different devices in s2mps11
driver and actually prepares the regulator driver for supporting the
S2MPS14 device.

Code for supporting the S2MPS14 device will add its own array of
'regulator_desc' (also marked as __initconst). This way memory footprint
of the driver will be reduced (approximately 'regulators_desc' array for
S2MPS11 occupies 5 kB on 32-bit ARM, for S2MPS14 will occupy 3 kB).

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
Cc: Yadwinder Singh Brar <yadi.brar01@gmail.com>
Reviewed-by: Yadwinder Singh Brar <yadi.brar@samsung.com>
---
 drivers/regulator/s2mps11.c |   75 +++++++++++++++++++++++++++++++++++--------
 1 file changed, 62 insertions(+), 13 deletions(-)

diff --git a/drivers/regulator/s2mps11.c b/drivers/regulator/s2mps11.c
index 189038aecbf5..ca66fc56fa58 100644
--- a/drivers/regulator/s2mps11.c
+++ b/drivers/regulator/s2mps11.c
@@ -25,9 +25,8 @@
 #include <linux/mfd/samsung/core.h>
 #include <linux/mfd/samsung/s2mps11.h>
 
-#define S2MPS11_REGULATOR_CNT ARRAY_SIZE(regulators)
-
 struct s2mps11_info {
+	unsigned int rdev_num;
 	int ramp_delay2;
 	int ramp_delay34;
 	int ramp_delay5;
@@ -343,7 +342,7 @@ static struct regulator_ops s2mps11_buck_ops = {
 	.enable_mask	= S2MPS11_ENABLE_MASK			\
 }
 
-static const struct regulator_desc regulators[] = {
+static const struct regulator_desc s2mps11_regulators[] __initconst = {
 	regulator_desc_ldo2(1),
 	regulator_desc_ldo1(2),
 	regulator_desc_ldo1(3),
@@ -394,21 +393,62 @@ static const struct regulator_desc regulators[] = {
 	regulator_desc_buck10,
 };
 
+/*
+ * Allocates memory under 'regulators' pointer and copies there array
+ * of regulator_desc for given device.
+ *
+ * Returns number of regulators or negative ERRNO on error.
+ */
+static int __init
+s2mps11_pmic_init_regulators_desc(struct platform_device *pdev,
+		struct regulator_desc **regulators)
+{
+	const struct regulator_desc *regulators_init;
+	enum sec_device_type dev_type;
+	int rdev_num;
+
+	dev_type = platform_get_device_id(pdev)->driver_data;
+	switch (dev_type) {
+	case S2MPS11X:
+		rdev_num = ARRAY_SIZE(s2mps11_regulators);
+		regulators_init = s2mps11_regulators;
+		break;
+	default:
+		dev_err(&pdev->dev, "Invalid device type: %u\n", dev_type);
+		return -EINVAL;
+	};
+
+	*regulators = devm_kzalloc(&pdev->dev,
+			sizeof(**regulators) * rdev_num, GFP_KERNEL);
+	if (!*regulators)
+		return -ENOMEM;
+
+	memcpy(*regulators, regulators_init, sizeof(**regulators) * rdev_num);
+
+	return rdev_num;
+}
+
 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 sec_platform_data *pdata = iodev->pdata;
+	struct of_regulator_match *rdata = NULL;
 	struct device_node *reg_np = NULL;
 	struct regulator_config config = { };
 	struct s2mps11_info *s2mps11;
-	int i, ret;
+	int i, ret = 0;
+	struct regulator_desc *regulators = NULL;
+	int rdev_num;
 
 	s2mps11 = devm_kzalloc(&pdev->dev, sizeof(struct s2mps11_info),
 				GFP_KERNEL);
 	if (!s2mps11)
 		return -ENOMEM;
 
+	rdev_num = s2mps11_pmic_init_regulators_desc(pdev, &regulators);
+	if (rdev_num < 0)
+		return rdev_num;
+
 	if (!iodev->dev->of_node) {
 		if (pdata) {
 			goto common_reg;
@@ -419,24 +459,30 @@ static int s2mps11_pmic_probe(struct platform_device *pdev)
 		}
 	}
 
-	for (i = 0; i < S2MPS11_REGULATOR_CNT; i++)
+	rdata = kzalloc(sizeof(*rdata) * rdev_num, GFP_KERNEL);
+	if (!rdata)
+		return -ENOMEM;
+
+	for (i = 0; i < rdev_num; i++)
 		rdata[i].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;
+		ret = -EINVAL;
+		goto out;
 	}
 
-	of_regulator_match(&pdev->dev, reg_np, rdata, S2MPS11_REGULATOR_MAX);
+	of_regulator_match(&pdev->dev, reg_np, rdata, rdev_num);
 
 common_reg:
 	platform_set_drvdata(pdev, s2mps11);
+	s2mps11->rdev_num = rdev_num;
 
 	config.dev = &pdev->dev;
 	config.regmap = iodev->regmap_pmic;
 	config.driver_data = s2mps11;
-	for (i = 0; i < S2MPS11_REGULATOR_MAX; i++) {
+	for (i = 0; i < rdev_num; i++) {
 		struct regulator_dev *regulator;
 
 		if (!reg_np) {
@@ -453,15 +499,18 @@ common_reg:
 			ret = PTR_ERR(regulator);
 			dev_err(&pdev->dev, "regulator init failed for %d\n",
 				i);
-			return ret;
+			goto out;
 		}
 	}
 
-	return 0;
+out:
+	kfree(rdata);
+
+	return ret;
 }
 
 static const struct platform_device_id s2mps11_pmic_id[] = {
-	{ "s2mps11-pmic", 0},
+	{ "s2mps11-pmic", S2MPS11X},
 	{ },
 };
 MODULE_DEVICE_TABLE(platform, s2mps11_pmic_id);
-- 
1.7.9.5

Re: [PATCH 1/3] regulator: s2mps11: Don't store registered regulators in state container

[Mark Brown]

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

On Fri, Feb 28, 2014 at 11:01:48AM +0100, Krzysztof Kozlowski wrote:
> Regulators registered by devm_regulator_register() do not have to be
> stored in state container because they are never dereferenced later.
> The array of regulator_dev can be safely removed from state container.

Applied, thanks.

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

Re: [PATCH 2/3] regulator: s2mps11: Constify regulator_desc array

[Mark Brown]

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

On Fri, Feb 28, 2014 at 11:01:49AM +0100, Krzysztof Kozlowski wrote:
> Constify the regulator_desc 'regulators' array.

Applied, thanks.

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

Re: [PATCH 3/3] regulator: s2mps11: Copy supported regulators from initconst

[Mark Brown]

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

On Fri, Feb 28, 2014 at 11:01:50AM +0100, Krzysztof Kozlowski wrote:
> Add __initconst to 'regulator_desc' array with supported regulators.
> During probe choose how many and which regulators will be supported
> according to device ID. Then copy the 'regulator_desc' array to
> allocated memory so the regulator core can use it.

Applied, thanks.

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

Re: [PATCH 3/3] regulator: s2mps11: Copy supported regulators from initconst

[Krzysztof Kozlowski]

Hi,

On Mon, 2014-03-03 at 10:09 +0800, Mark Brown wrote:
> On Fri, Feb 28, 2014 at 11:01:50AM +0100, Krzysztof Kozlowski wrote:
> > Add __initconst to 'regulator_desc' array with supported regulators.
> > During probe choose how many and which regulators will be supported
> > according to device ID. Then copy the 'regulator_desc' array to
> > allocated memory so the regulator core can use it.
> 
> Applied, thanks.

Thanks! Unfortunately I wonder now whether it was a good idea to mark
the regulator_desc array as __initconst. I've seen the warning from
kbuild test robot:
--------
>> WARNING: vmlinux.o(.text+0xf0faab): Section mismatch in reference
from the function s2mps11_pmic_probe() to the
variable .init.rodata:s2mps11_regulators
   The function s2mps11_pmic_probe() references
   the variable __initconst s2mps11_regulators.
   This is often because s2mps11_pmic_probe lacks a __initconst
   annotation or the annotation of s2mps11_regulators is wrong.
--------

I have two ideas for fixing this:
1. The s2mps11_pmic_probe() could be marked with __init and 
platform_driver_probe() should be used. Unfortunately this does not work
because the driver is registered and probed a little later after
s2mps11_pmic_init() when I2C bus driver is probed. During that time the
drv->probe() is actually NULL.

2. The s2mps11_pmic_probe() won't be marked as __init and could copy the
regulator_desc (__initconst) array to local static variable. This way if
it would be called twice the mentioned array __initconst won't be
dereferenced. Unfortunately this won't remove the warning.

Any ideas for solving this?

Best regards,
Krzysztof

Re: [PATCH 3/3] regulator: s2mps11: Copy supported regulators from initconst

[Krzysztof Kozlowski]

> 
> Hi,
> 
> On Mon, 2014-03-03 at 10:09 +0800, Mark Brown wrote:
> > On Fri, Feb 28, 2014 at 11:01:50AM +0100, Krzysztof Kozlowski wrote:
> > > Add __initconst to 'regulator_desc' array with supported regulators.
> > > During probe choose how many and which regulators will be supported
> > > according to device ID. Then copy the 'regulator_desc' array to
> > > allocated memory so the regulator core can use it.
> > 
> > Applied, thanks.
> 
> Thanks! Unfortunately I wonder now whether it was a good idea to mark
> the regulator_desc array as __initconst. I've seen the warning from
> kbuild test robot:
> --------
> >> WARNING: vmlinux.o(.text+0xf0faab): Section mismatch in reference
> from the function s2mps11_pmic_probe() to the
> variable .init.rodata:s2mps11_regulators
>    The function s2mps11_pmic_probe() references
>    the variable __initconst s2mps11_regulators.
>    This is often because s2mps11_pmic_probe lacks a __initconst
>    annotation or the annotation of s2mps11_regulators is wrong.
> --------
> 
> I have two ideas for fixing this:
> 1. The s2mps11_pmic_probe() could be marked with __init and 
> platform_driver_probe() should be used. Unfortunately this does not work
> because the driver is registered and probed a little later after
> s2mps11_pmic_init() when I2C bus driver is probed. During that time the
> drv->probe() is actually NULL.
> 
> 2. The s2mps11_pmic_probe() won't be marked as __init and could copy the
> regulator_desc (__initconst) array to local static variable. This way if
> it would be called twice the mentioned array __initconst won't be
> dereferenced. Unfortunately this won't remove the warning.
> 
> Any ideas for solving this?

I sent a patch removing the __initconst. From my point of view these two
patches can be squashed, so effectively only choosing number of
supported regulators is introduced (as it was in my original patch from
11th of February).


Best regards,
Krzysztof