Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1345332 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/7] regulator: act8865: Expose act8600 registers via debugfs Maarten ter Huurne <maarten@treewalker.org> - 2016-02-28 17:00 +0100
    [PATCH 4/7] regulator: act8865: Rename platform_data field to init_data Maarten ter Huurne <maarten@treewalker.org> - 2016-02-28 17:00 +0100
    [PATCH 2/7] regulator: act8865: Remove redundant dev lookups Maarten ter Huurne <maarten@treewalker.org> - 2016-02-28 17:00 +0100
    Re: [PATCH 1/7] regulator: act8865: Expose act8600 registers via  debugfs Mark Brown <broonie@kernel.org> - 2016-02-29 12:30 +0100

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

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

[toc] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web