[PATCH 1/7] regulator: act8865: Expose act8600 registers via debugfs

[PATCH 1/7] regulator: act8865: Expose act8600 registers via debugfs

[Maarten ter Huurne]

The read/write/volatile configuration is valid also when debugfs is
not enabled, but it doesn't add any value then.

Signed-off-by: Maarten ter Huurne <maarten@treewalker.org>
---
 drivers/regulator/act8865-regulator.c | 67 ++++++++++++++++++++++++++++++++---
 1 file changed, 63 insertions(+), 4 deletions(-)

diff --git a/drivers/regulator/act8865-regulator.c b/drivers/regulator/act8865-regulator.c
index f8d4cd3..7bdb844 100644
--- a/drivers/regulator/act8865-regulator.c
+++ b/drivers/regulator/act8865-regulator.c
@@ -56,6 +56,7 @@
 #define ACT8600_APCH_STAT	0xAA
 #define ACT8600_OTG0		0xB0
 #define ACT8600_OTG1		0xB2
+#define ACT8600_INT		0xC1
 
 /*
  * ACT8846 Global Register Map.
@@ -139,11 +140,59 @@ struct act8865 {
 	int off_mask;
 };
 
-static const struct regmap_config act8865_regmap_config = {
-	.reg_bits = 8,
-	.val_bits = 8,
+#ifdef CONFIG_DEBUG_FS
+
+static const struct regmap_range act8600_reg_ranges[] = {
+	{ 0x00, 0x01 },
+	{ 0x10, 0x10 }, { 0x12, 0x12 },
+	{ 0x20, 0x20 }, { 0x22, 0x22 },
+	{ 0x30, 0x30 }, { 0x32, 0x32 },
+	{ 0x40, 0x41 },
+	{ 0x50, 0x51 },
+	{ 0x60, 0x61 },
+	{ 0x70, 0x71 },
+	{ 0x80, 0x81 },
+	{ 0x91, 0x91 },
+	{ 0xA1, 0xA1 }, { 0xA8, 0xAA },
+	{ 0xB0, 0xB0 }, { 0xB2, 0xB2 },
+	{ 0xC1, 0xC1 },
+};
+static const struct regmap_range act8600_reg_ro_ranges[] = {
+	{ 0xAA, 0xAA },
+	{ 0xC1, 0xC1 },
+};
+static const struct regmap_range act8600_reg_volatile_ranges[] = {
+	{ 0x00, 0x01 },
+	{ 0x12, 0x12 },
+	{ 0x22, 0x22 },
+	{ 0x32, 0x32 },
+	{ 0x41, 0x41 },
+	{ 0x51, 0x51 },
+	{ 0x61, 0x61 },
+	{ 0x71, 0x71 },
+	{ 0x81, 0x81 },
+	{ 0xA8, 0xA8 }, { 0xAA, 0xAA },
+	{ 0xB0, 0xB0 },
+	{ 0xC1, 0xC1 },
 };
 
+static const struct regmap_access_table act8600_write_ranges_table = {
+	.yes_ranges = act8600_reg_ranges,
+	.n_yes_ranges = ARRAY_SIZE(act8600_reg_ranges),
+	.no_ranges = act8600_reg_ro_ranges,
+	.n_no_ranges = ARRAY_SIZE(act8600_reg_ro_ranges),
+};
+static const struct regmap_access_table act8600_read_ranges_table = {
+	.yes_ranges = act8600_reg_ranges,
+	.n_yes_ranges = ARRAY_SIZE(act8600_reg_ranges),
+};
+static const struct regmap_access_table act8600_volatile_ranges_table = {
+	.yes_ranges = act8600_reg_volatile_ranges,
+	.n_yes_ranges = ARRAY_SIZE(act8600_reg_volatile_ranges),
+};
+
+#endif
+
 static const struct regulator_linear_range act8865_voltage_ranges[] = {
 	REGULATOR_LINEAR_RANGE(600000, 0, 23, 25000),
 	REGULATOR_LINEAR_RANGE(1200000, 24, 47, 50000),
@@ -421,6 +470,11 @@ static int act8865_pmic_probe(struct i2c_client *client,
 	struct device_node **of_node;
 	int i, ret, num_regulators;
 	struct act8865 *act8865;
+	struct regmap_config regmap_config = {
+		.reg_bits = 8,
+		.val_bits = 8,
+		.max_register = 0xFF,
+	};
 	unsigned long type;
 	int off_reg, off_mask;
 	int voltage_select = 0;
@@ -449,6 +503,11 @@ static int act8865_pmic_probe(struct i2c_client *client,
 		num_regulators = ARRAY_SIZE(act8600_regulators);
 		off_reg = -1;
 		off_mask = -1;
+#ifdef CONFIG_DEBUG_FS
+		regmap_config.wr_table = &act8600_write_ranges_table;
+		regmap_config.rd_table = &act8600_read_ranges_table;
+		regmap_config.volatile_table = &act8600_volatile_ranges_table;
+#endif
 		break;
 	case ACT8846:
 		regulators = act8846_regulators;
@@ -495,7 +554,7 @@ static int act8865_pmic_probe(struct i2c_client *client,
 	if (!act8865)
 		return -ENOMEM;
 
-	act8865->regmap = devm_regmap_init_i2c(client, &act8865_regmap_config);
+	act8865->regmap = devm_regmap_init_i2c(client, &regmap_config);
 	if (IS_ERR(act8865->regmap)) {
 		ret = PTR_ERR(act8865->regmap);
 		dev_err(&client->dev, "Failed to allocate register map: %d\n",
-- 
2.6.2

[PATCH 2/7] regulator: act8865: Remove redundant dev lookups

[Maarten ter Huurne]

The local variable "dev" already contains a pointer to the device,
so there is no need to take the address of "client->dev" again.

Signed-off-by: Maarten ter Huurne <maarten@treewalker.org>
---
 drivers/regulator/act8865-regulator.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/regulator/act8865-regulator.c b/drivers/regulator/act8865-regulator.c
index 7bdb844..0e2d3f5 100644
--- a/drivers/regulator/act8865-regulator.c
+++ b/drivers/regulator/act8865-regulator.c
@@ -557,8 +557,7 @@ static int act8865_pmic_probe(struct i2c_client *client,
 	act8865->regmap = devm_regmap_init_i2c(client, &regmap_config);
 	if (IS_ERR(act8865->regmap)) {
 		ret = PTR_ERR(act8865->regmap);
-		dev_err(&client->dev, "Failed to allocate register map: %d\n",
-			ret);
+		dev_err(dev, "Failed to allocate register map: %d\n", ret);
 		return ret;
 	}
 
@@ -585,7 +584,7 @@ static int act8865_pmic_probe(struct i2c_client *client,
 		config.driver_data = act8865;
 		config.regmap = act8865->regmap;
 
-		rdev = devm_regulator_register(&client->dev, desc, &config);
+		rdev = devm_regulator_register(dev, desc, &config);
 		if (IS_ERR(rdev)) {
 			dev_err(dev, "failed to register %s\n", desc->name);
 			return PTR_ERR(rdev);
-- 
2.6.2

[PATCH 3/7] regulator: act8865: Remove "static" from local variable

[Maarten ter Huurne]

There is no need to preserve its value between calls. I guess this
was a copy-paste slip-up.

Signed-off-by: Maarten ter Huurne <maarten@treewalker.org>
---
 drivers/regulator/act8865-regulator.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/regulator/act8865-regulator.c b/drivers/regulator/act8865-regulator.c
index 0e2d3f5..477d60c 100644
--- a/drivers/regulator/act8865-regulator.c
+++ b/drivers/regulator/act8865-regulator.c
@@ -464,7 +464,7 @@ static void act8865_power_off(void)
 static int act8865_pmic_probe(struct i2c_client *client,
 			      const struct i2c_device_id *i2c_id)
 {
-	static const struct regulator_desc *regulators;
+	const struct regulator_desc *regulators;
 	struct act8865_platform_data pdata_of, *pdata;
 	struct device *dev = &client->dev;
 	struct device_node **of_node;
-- 
2.6.2

[PATCH 4/7] regulator: act8865: Rename platform_data field to init_data

[Maarten ter Huurne]

Make the field name match its type.

Signed-off-by: Maarten ter Huurne <maarten@treewalker.org>
---
 drivers/regulator/act8865-regulator.c | 4 ++--
 include/linux/regulator/act8865.h     | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/regulator/act8865-regulator.c b/drivers/regulator/act8865-regulator.c
index 477d60c..0d1b235 100644
--- a/drivers/regulator/act8865-regulator.c
+++ b/drivers/regulator/act8865-regulator.c
@@ -418,7 +418,7 @@ static int act8865_pdata_from_dt(struct device *dev,
 	for (i = 0; i < num_matches; i++) {
 		regulator->id = i;
 		regulator->name = matches[i].name;
-		regulator->platform_data = matches[i].init_data;
+		regulator->init_data = matches[i].init_data;
 		of_node[i] = matches[i].of_node;
 		regulator++;
 	}
@@ -445,7 +445,7 @@ static struct regulator_init_data
 
 	for (i = 0; i < pdata->num_regulators; i++) {
 		if (pdata->regulators[i].id == id)
-			return pdata->regulators[i].platform_data;
+			return pdata->regulators[i].init_data;
 	}
 
 	return NULL;
diff --git a/include/linux/regulator/act8865.h b/include/linux/regulator/act8865.h
index 15fa8f2..2eb3860 100644
--- a/include/linux/regulator/act8865.h
+++ b/include/linux/regulator/act8865.h
@@ -68,12 +68,12 @@ enum {
  * act8865_regulator_data - regulator data
  * @id: regulator id
  * @name: regulator name
- * @platform_data: regulator init data
+ * @init_data: regulator init data
  */
 struct act8865_regulator_data {
 	int id;
 	const char *name;
-	struct regulator_init_data *platform_data;
+	struct regulator_init_data *init_data;
 };
 
 /**
-- 
2.6.2

[PATCH 5/7] regulator: act8865: Pass of_node via act8865_regulator_data

[Maarten ter Huurne]

This makes the code easier to read and it avoids a dynamic memory
allocation.

Note that the "too many regulators" error handler was broken prior
to its removal in this commit, since it dereferenced pdata, which
can be NULL in the non-DT case.

Signed-off-by: Maarten ter Huurne <maarten@treewalker.org>
---
 drivers/regulator/act8865-regulator.c | 42 +++++++++++++----------------------
 include/linux/regulator/act8865.h     |  2 ++
 2 files changed, 18 insertions(+), 26 deletions(-)

diff --git a/drivers/regulator/act8865-regulator.c b/drivers/regulator/act8865-regulator.c
index 0d1b235..cdfe082 100644
--- a/drivers/regulator/act8865-regulator.c
+++ b/drivers/regulator/act8865-regulator.c
@@ -368,7 +368,6 @@ static struct of_regulator_match act8600_matches[] = {
 };
 
 static int act8865_pdata_from_dt(struct device *dev,
-				 struct device_node **of_node,
 				 struct act8865_platform_data *pdata,
 				 unsigned long type)
 {
@@ -419,7 +418,7 @@ static int act8865_pdata_from_dt(struct device *dev,
 		regulator->id = i;
 		regulator->name = matches[i].name;
 		regulator->init_data = matches[i].init_data;
-		of_node[i] = matches[i].of_node;
+		regulator->of_node = matches[i].of_node;
 		regulator++;
 	}
 
@@ -427,7 +426,6 @@ static int act8865_pdata_from_dt(struct device *dev,
 }
 #else
 static inline int act8865_pdata_from_dt(struct device *dev,
-					struct device_node **of_node,
 					struct act8865_platform_data *pdata,
 					unsigned long type)
 {
@@ -435,8 +433,8 @@ static inline int act8865_pdata_from_dt(struct device *dev,
 }
 #endif
 
-static struct regulator_init_data
-*act8865_get_init_data(int id, struct act8865_platform_data *pdata)
+static struct act8865_regulator_data *act8865_get_regulator_data(
+		int id, struct act8865_platform_data *pdata)
 {
 	int i;
 
@@ -445,7 +443,7 @@ static struct regulator_init_data
 
 	for (i = 0; i < pdata->num_regulators; i++) {
 		if (pdata->regulators[i].id == id)
-			return pdata->regulators[i].init_data;
+			return &pdata->regulators[i];
 	}
 
 	return NULL;
@@ -467,7 +465,6 @@ static int act8865_pmic_probe(struct i2c_client *client,
 	const struct regulator_desc *regulators;
 	struct act8865_platform_data pdata_of, *pdata;
 	struct device *dev = &client->dev;
-	struct device_node **of_node;
 	int i, ret, num_regulators;
 	struct act8865 *act8865;
 	struct regmap_config regmap_config = {
@@ -531,25 +528,14 @@ static int act8865_pmic_probe(struct i2c_client *client,
 		return -EINVAL;
 	}
 
-	of_node = devm_kzalloc(dev, sizeof(struct device_node *) *
-			       num_regulators, GFP_KERNEL);
-	if (!of_node)
-		return -ENOMEM;
-
 	if (dev->of_node && !pdata) {
-		ret = act8865_pdata_from_dt(dev, of_node, &pdata_of, type);
+		ret = act8865_pdata_from_dt(dev, &pdata_of, type);
 		if (ret < 0)
 			return ret;
 
 		pdata = &pdata_of;
 	}
 
-	if (pdata->num_regulators > num_regulators) {
-		dev_err(dev, "too many regulators: %d\n",
-			pdata->num_regulators);
-		return -EINVAL;
-	}
-
 	act8865 = devm_kzalloc(dev, sizeof(struct act8865), GFP_KERNEL);
 	if (!act8865)
 		return -ENOMEM;
@@ -575,14 +561,19 @@ static int act8865_pmic_probe(struct i2c_client *client,
 	/* Finally register devices */
 	for (i = 0; i < num_regulators; i++) {
 		const struct regulator_desc *desc = &regulators[i];
-		struct regulator_config config = { };
+		struct regulator_config config = {
+			.dev = dev,
+			.regmap = act8865->regmap,
+			.driver_data = act8865,
+		};
+		struct act8865_regulator_data *rdata;
 		struct regulator_dev *rdev;
 
-		config.dev = dev;
-		config.init_data = act8865_get_init_data(desc->id, pdata);
-		config.of_node = of_node[i];
-		config.driver_data = act8865;
-		config.regmap = act8865->regmap;
+		rdata = act8865_get_regulator_data(desc->id, pdata);
+		if (rdata) {
+			config.init_data = rdata->init_data;
+			config.of_node = rdata->of_node;
+		}
 
 		rdev = devm_regulator_register(dev, desc, &config);
 		if (IS_ERR(rdev)) {
@@ -592,7 +583,6 @@ static int act8865_pmic_probe(struct i2c_client *client,
 	}
 
 	i2c_set_clientdata(client, act8865);
-	devm_kfree(dev, of_node);
 
 	return 0;
 }
diff --git a/include/linux/regulator/act8865.h b/include/linux/regulator/act8865.h
index 2eb3860..113d861 100644
--- a/include/linux/regulator/act8865.h
+++ b/include/linux/regulator/act8865.h
@@ -69,11 +69,13 @@ enum {
  * @id: regulator id
  * @name: regulator name
  * @init_data: regulator init data
+ * @of_node: device tree node (optional)
  */
 struct act8865_regulator_data {
 	int id;
 	const char *name;
 	struct regulator_init_data *init_data;
+	struct device_node *of_node;
 };
 
 /**
-- 
2.6.2

[PATCH 6/7] regulator: act8865: Specify fixed voltage of 3.3V for ACT8600's REG9

[Maarten ter Huurne]

The documentation lists both 1.8V and 3.3V for this regulator output,
but 3.3V is mentioned more often and also matches what JZ4770 and
JZ4780 expect as an input.

Note that the voltage of REG9 is not programmable, so this commit only
changes the voltage reported in sysfs and debugfs, not the actual
output voltage of the hardware.

Signed-off-by: Maarten ter Huurne <maarten@treewalker.org>
---
 drivers/regulator/act8865-regulator.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/regulator/act8865-regulator.c b/drivers/regulator/act8865-regulator.c
index cdfe082..ac6956d 100644
--- a/drivers/regulator/act8865-regulator.c
+++ b/drivers/regulator/act8865-regulator.c
@@ -267,7 +267,7 @@ static const struct regulator_desc act8600_regulators[] = {
 		.ops = &act8865_ldo_ops,
 		.type = REGULATOR_VOLTAGE,
 		.n_voltages = 1,
-		.fixed_uV = 1800000,
+		.fixed_uV = 3300000,
 		.enable_reg = ACT8600_LDO910_CTRL,
 		.enable_mask = ACT8865_ENA,
 		.owner = THIS_MODULE,
-- 
2.6.2

[PATCH 7/7] regulator: act8865: Init at subsys level

[Maarten ter Huurne]

Since the defined regulators are used in other drivers, we can avoid
deferred probing by registering this driver sooner.

Signed-off-by: Maarten ter Huurne <maarten@treewalker.org>
---
 drivers/regulator/act8865-regulator.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/regulator/act8865-regulator.c b/drivers/regulator/act8865-regulator.c
index 48608e2..6a0d34f 100644
--- a/drivers/regulator/act8865-regulator.c
+++ b/drivers/regulator/act8865-regulator.c
@@ -686,7 +686,17 @@ static struct i2c_driver act8865_pmic_driver = {
 	.id_table	= act8865_ids,
 };
 
-module_i2c_driver(act8865_pmic_driver);
+static int __init act8865_pmic_driver_init(void)
+{
+	return i2c_add_driver(&act8865_pmic_driver);
+}
+subsys_initcall(act8865_pmic_driver_init);
+
+static void __exit act8865_pmic_driver_exit(void)
+{
+	i2c_del_driver(&act8865_pmic_driver);
+}
+module_exit(act8865_pmic_driver_exit);
 
 MODULE_DESCRIPTION("active-semi act88xx voltage regulator driver");
 MODULE_AUTHOR("Wenyou Yang <wenyou.yang@atmel.com>");
-- 
2.6.2

Re: [PATCH 1/7] regulator: act8865: Expose act8600 registers via debugfs

[Mark Brown]

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

On Sun, Feb 28, 2016 at 04:53:23PM +0100, Maarten ter Huurne wrote:

> The read/write/volatile configuration is valid also when debugfs is
> not enabled, but it doesn't add any value then.

Please write changelogs that accurately describe what your changes do.
Any debugfs changes are at best a second order side effect of changes
here, what this appears to do is some combination of defining one or
more new registers (which don't seem to ever be referred to...) and
providing some access maps.

> +#ifdef CONFIG_DEBUG_FS
> +

No, this is broken.  The access information for a device is not affected
by debugfs and does have an effect on how we work with the device.
Having access maps that depend on random build settings like this is a
recipie for hard to diagnose bugs.

> +static const struct regmap_range act8600_reg_ranges[] = {
> +	{ 0x00, 0x01 },
> +	{ 0x10, 0x10 }, { 0x12, 0x12 },
> +	{ 0x20, 0x20 }, { 0x22, 0x22 },
> +	{ 0x30, 0x30 }, { 0x32, 0x32 },

Your formatting here is just weird and confusing, randomly grouping
things on lines with no obvious .

> +};
> +static const struct regmap_range act8600_reg_ro_ranges[] = {

Missing blank lines between variables.

> +static const struct regmap_range act8600_reg_volatile_ranges[] = {
> +	{ 0x00, 0x01 },

For ranges you should explicitly use .start and .end otherwise these
look like they're intended to be register default tables.

> @@ -421,6 +470,11 @@ static int act8865_pmic_probe(struct i2c_client *client,
>  	struct device_node **of_node;
>  	int i, ret, num_regulators;
>  	struct act8865 *act8865;
> +	struct regmap_config regmap_config = {
> +		.reg_bits = 8,
> +		.val_bits = 8,
> +		.max_register = 0xFF,
> +	};

Why have you moved this from a global static variable where it normally
is to a local variable?  This is unusual and confusing...

> +#ifdef CONFIG_DEBUG_FS
> +		regmap_config.wr_table = &act8600_write_ranges_table;
> +		regmap_config.rd_table = &act8600_read_ranges_table;
> +		regmap_config.volatile_table = &act8600_volatile_ranges_table;
> +#endif

...and the fact that you're doing things like this ought to be a warning
that there's a problem with the way you're handling the access maps.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 7/7] regulator: act8865: Init at subsys level

[Mark Brown]

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

On Sun, Feb 28, 2016 at 04:53:29PM +0100, Maarten ter Huurne wrote:
> Since the defined regulators are used in other drivers, we can avoid
> deferred probing by registering this driver sooner.

No, don't play silly link order games.  We are never going to get a link
order that avoids deferred probes, faffing about changing things to try
to make one just leads to lots of noise.  If you want to avoid noise
from deferred probe then work on something that communicates
dependencies to the driver core so we don't have to brute force things,
Raphael has some ideas there he's been looking at.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 5/7] regulator: act8865: Pass of_node via act8865_regulator_data

[Mark Brown]

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

On Sun, Feb 28, 2016 at 04:53:27PM +0100, Maarten ter Huurne wrote:

> +		struct regulator_config config = {
> +			.dev = dev,
> +			.regmap = act8865->regmap,
> +			.driver_data = act8865,
> +		};
> +		struct act8865_regulator_data *rdata;
>  		struct regulator_dev *rdev;
>  
> -		config.dev = dev;
> -		config.init_data = act8865_get_init_data(desc->id, pdata);
> -		config.of_node = of_node[i];
> -		config.driver_data = act8865;
> -		config.regmap = act8865->regmap;

This appears to be doing some unrelated refectoring of the code which
should be in a separate commit to make review clearer.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 7/7] regulator: act8865: Init at subsys level

[Maarten ter Huurne]

On Monday 29 February 2016 20:35:37 Mark Brown wrote:
> On Sun, Feb 28, 2016 at 04:53:29PM +0100, Maarten ter Huurne wrote:
> > Since the defined regulators are used in other drivers, we can avoid
> > deferred probing by registering this driver sooner.
> 
> No, don't play silly link order games.  We are never going to get a
> link order that avoids deferred probes, faffing about changing things
> to try to make one just leads to lots of noise.  If you want to avoid
> noise from deferred probe then work on something that communicates
> dependencies to the driver core so we don't have to brute force
> things, Raphael has some ideas there he's been looking at.

On our device the LCD panel is powered by one of the ACT8600 outputs. 
Avoiding the deferred probe means the screen turns on a couple of 
seconds earlier.

I agree that the current hardcoded priority system is not ideal and a 
dependency based system would be the right solution in the long term, 
but I was looking for a short term solution with this patch.

In any case, I excluded this patch from the v2 series. I cleaned up the 
register configuration (debugfs) patch, split the of_node patch into two 
patches and resubmitted the redundant dev lookups patch. Thanks for 
reviewing and please let me know if there is anything more that needs 
changing.

Bye,
		Maarten