[PATCH v4 0/7] mfd: axp20x: add AXP221 PMIC support

[PATCH v4 0/7] mfd: axp20x: add AXP221 PMIC support

[Boris BREZILLON]

Hello,

This patch series adds basic support for X-Powers' AXP221 PMIC.

At the moment the MFD device only exposes AXP221 regulators but other
subdevices might be added later.

Best Regards,

Boris

Changes since v3:
 - rework mfd probe to avoid multiple AXP variant tests
 - add new compatible string to the DT bindings doc

Changes since v2:
 - add helper functions for regulator set registration
 - fix a copy/paste error in the axp20x mfd driver

Changes since v1:
 - merge code with the axp20x driver

Boris BREZILLON (7):
  mfd: axp20x: add AXP221 PMIC support
  regulator: axp20x: prepare support for multiple AXP chip families
  regulator: axp20x: add support for AXP221 regulators
  regulator: axp20x: reset probe data before each probe
  regulator: add support for regulator set registration
  regulator: axp20x: make use of devm_regulator_set_register
  mfd: AXP20x: add "x-powers,axp221" compatible string to DT bindings
    doc

 Documentation/devicetree/bindings/mfd/axp20x.txt |   2 +-
 drivers/mfd/axp20x.c                             |  64 ++++++-
 drivers/regulator/axp20x-regulator.c             | 202 ++++++++++++++++-------
 drivers/regulator/core.c                         | 106 ++++++++++++
 drivers/regulator/devres.c                       |  68 ++++++++
 include/linux/mfd/axp20x.h                       |  56 +++++++
 include/linux/regulator/driver.h                 |  51 ++++++
 7 files changed, 483 insertions(+), 66 deletions(-)

-- 
1.8.3.2

[PATCH v4 1/7] mfd: axp20x: add AXP221 PMIC support

[Boris BREZILLON]

Add support for the AXP221 PMIC device to the existing AXP20x driver.

The AXP221 defines a new set of registers, power supplies and regulators,
but most of the API is similar to the AXP20x ones.
The AXP20x irq chip definition is reused, though some interrupts are not
available in the AXP221.

Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
---
 drivers/mfd/axp20x.c       | 64 ++++++++++++++++++++++++++++++++++++++++++----
 include/linux/mfd/axp20x.h | 56 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 115 insertions(+), 5 deletions(-)

diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
index dee6539..ad4c177 100644
--- a/drivers/mfd/axp20x.c
+++ b/drivers/mfd/axp20x.c
@@ -33,6 +33,11 @@ static const struct regmap_range axp20x_writeable_ranges[] = {
 	regmap_reg_range(AXP20X_DCDC_MODE, AXP20X_FG_RES),
 };
 
+static const struct regmap_range axp22x_writeable_ranges[] = {
+	regmap_reg_range(AXP20X_DATACACHE(0), AXP20X_IRQ5_STATE),
+	regmap_reg_range(AXP20X_DCDC_MODE, AXP22X_BATLOW_THRES1),
+};
+
 static const struct regmap_range axp20x_volatile_ranges[] = {
 	regmap_reg_range(AXP20X_IRQ1_EN, AXP20X_IRQ5_STATE),
 };
@@ -42,6 +47,11 @@ static const struct regmap_access_table axp20x_writeable_table = {
 	.n_yes_ranges	= ARRAY_SIZE(axp20x_writeable_ranges),
 };
 
+static const struct regmap_access_table axp22x_writeable_table = {
+	.yes_ranges	= axp22x_writeable_ranges,
+	.n_yes_ranges	= ARRAY_SIZE(axp22x_writeable_ranges),
+};
+
 static const struct regmap_access_table axp20x_volatile_table = {
 	.yes_ranges	= axp20x_volatile_ranges,
 	.n_yes_ranges	= ARRAY_SIZE(axp20x_volatile_ranges),
@@ -70,6 +80,15 @@ static const struct regmap_config axp20x_regmap_config = {
 	.cache_type	= REGCACHE_RBTREE,
 };
 
+static const struct regmap_config axp22x_regmap_config = {
+	.reg_bits	= 8,
+	.val_bits	= 8,
+	.wr_table	= &axp22x_writeable_table,
+	.volatile_table	= &axp20x_volatile_table,
+	.max_register	= AXP22X_BATLOW_THRES1,
+	.cache_type	= REGCACHE_RBTREE,
+};
+
 #define AXP20X_IRQ(_irq, _off, _mask) \
 	[AXP20X_IRQ_##_irq] = { .reg_offset = (_off), .mask = BIT(_mask) }
 
@@ -116,6 +135,7 @@ static const struct regmap_irq axp20x_regmap_irqs[] = {
 static const struct of_device_id axp20x_of_match[] = {
 	{ .compatible = "x-powers,axp202", .data = (void *) AXP202_ID },
 	{ .compatible = "x-powers,axp209", .data = (void *) AXP209_ID },
+	{ .compatible = "x-powers,axp221", .data = (void *) AXP221_ID },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, axp20x_of_match);
@@ -149,6 +169,21 @@ static const char * const axp20x_supplies[] = {
 	"ldo5in",
 };
 
+static const char *axp22x_supplies[] = {
+	"vbus",
+	"acin",
+	"vin1",
+	"vin2",
+	"vin3",
+	"vin4",
+	"vin5",
+	"aldoin",
+	"dldoin",
+	"eldoin",
+	"ldoioin",
+	"rtcldoin",
+};
+
 static struct mfd_cell axp20x_cells[] = {
 	{
 		.name			= "axp20x-pek",
@@ -161,6 +196,14 @@ static struct mfd_cell axp20x_cells[] = {
 	},
 };
 
+static struct mfd_cell axp22x_cells[] = {
+	{
+		.name			= "axp20x-regulator",
+		.parent_supplies	= axp22x_supplies,
+		.num_parent_supplies	= ARRAY_SIZE(axp22x_supplies),
+	},
+};
+
 static struct axp20x_dev *axp20x_pm_power_off;
 static void axp20x_power_off(void)
 {
@@ -171,8 +214,11 @@ static void axp20x_power_off(void)
 static int axp20x_i2c_probe(struct i2c_client *i2c,
 			 const struct i2c_device_id *id)
 {
-	struct axp20x_dev *axp20x;
+	const struct regmap_config *regmap_cfg;
 	const struct of_device_id *of_id;
+	struct axp20x_dev *axp20x;
+	struct mfd_cell *cells;
+	int ncells;
 	int ret;
 
 	axp20x = devm_kzalloc(&i2c->dev, sizeof(*axp20x), GFP_KERNEL);
@@ -190,7 +236,17 @@ static int axp20x_i2c_probe(struct i2c_client *i2c,
 	axp20x->dev = &i2c->dev;
 	dev_set_drvdata(axp20x->dev, axp20x);
 
-	axp20x->regmap = devm_regmap_init_i2c(i2c, &axp20x_regmap_config);
+	if (axp20x->variant == AXP221_ID) {
+		regmap_cfg = &axp22x_regmap_config;
+		cells = axp22x_cells;
+		ncells = ARRAY_SIZE(axp22x_cells);
+	} else {
+		regmap_cfg = &axp20x_regmap_config;
+		cells = axp20x_cells;
+		ncells = ARRAY_SIZE(axp20x_cells);
+	}
+
+	axp20x->regmap = devm_regmap_init_i2c(i2c, regmap_cfg);
 	if (IS_ERR(axp20x->regmap)) {
 		ret = PTR_ERR(axp20x->regmap);
 		dev_err(&i2c->dev, "regmap init failed: %d\n", ret);
@@ -206,9 +262,7 @@ static int axp20x_i2c_probe(struct i2c_client *i2c,
 		return ret;
 	}
 
-	ret = mfd_add_devices(axp20x->dev, -1, axp20x_cells,
-			      ARRAY_SIZE(axp20x_cells), NULL, 0, NULL);
-
+	ret = mfd_add_devices(axp20x->dev, -1, cells, ncells, NULL, 0, NULL);
 	if (ret) {
 		dev_err(&i2c->dev, "failed to add MFD devices: %d\n", ret);
 		regmap_del_irq_chip(i2c->irq, axp20x->regmap_irqc);
diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
index d0e31a2..8a8ce02 100644
--- a/include/linux/mfd/axp20x.h
+++ b/include/linux/mfd/axp20x.h
@@ -14,6 +14,7 @@
 enum {
 	AXP202_ID = 0,
 	AXP209_ID,
+	AXP221_ID,
 };
 
 #define AXP20X_DATACACHE(m)		(0x04 + (m))
@@ -43,6 +44,28 @@ enum {
 #define AXP20X_V_LTF_DISCHRG		0x3c
 #define AXP20X_V_HTF_DISCHRG		0x3d
 
+#define AXP22X_PWR_OUT_CTRL1		0x10
+#define AXP22X_PWR_OUT_CTRL2		0x12
+#define AXP22X_PWR_OUT_CTRL3		0x13
+#define AXP22X_DLDO1_V_OUT		0x15
+#define AXP22X_DLDO2_V_OUT		0x16
+#define AXP22X_DLDO3_V_OUT		0x17
+#define AXP22X_DLDO4_V_OUT		0x18
+#define AXP22X_ELDO1_V_OUT		0x19
+#define AXP22X_ELDO2_V_OUT		0x1a
+#define AXP22X_ELDO3_V_OUT		0x1b
+#define AXP22X_DC5LDO_V_OUT		0x1c
+#define AXP22X_DCDC1_V_OUT		0x21
+#define AXP22X_DCDC2_V_OUT		0x22
+#define AXP22X_DCDC3_V_OUT		0x23
+#define AXP22X_DCDC4_V_OUT		0x24
+#define AXP22X_DCDC5_V_OUT		0x25
+#define AXP22X_DCDC23_V_RAMP_CTRL	0x27
+#define AXP22X_ALDO1_V_OUT		0x28
+#define AXP22X_ALDO2_V_OUT		0x29
+#define AXP22X_ALDO3_V_OUT		0x2a
+#define AXP22X_CHRG_CTRL3		0x35
+
 /* Interrupt */
 #define AXP20X_IRQ1_EN			0x40
 #define AXP20X_IRQ2_EN			0x41
@@ -96,6 +119,9 @@ enum {
 #define AXP20X_VBUS_MON			0x8b
 #define AXP20X_OVER_TMP			0x8f
 
+#define AXP22X_PWREN_CTRL1		0x8c
+#define AXP22X_PWREN_CTRL2		0x8d
+
 /* GPIO */
 #define AXP20X_GPIO0_CTRL		0x90
 #define AXP20X_LDO5_V_OUT		0x91
@@ -104,6 +130,11 @@ enum {
 #define AXP20X_GPIO20_SS		0x94
 #define AXP20X_GPIO3_CTRL		0x95
 
+#define AXP22X_LDO_IO0_V_OUT		0x91
+#define AXP22X_LDO_IO1_V_OUT		0x93
+#define AXP22X_GPIO_STATE		0x94
+#define AXP22X_GPIO_PULL_DOWN		0x95
+
 /* Battery */
 #define AXP20X_CHRG_CC_31_24		0xb0
 #define AXP20X_CHRG_CC_23_16		0xb1
@@ -116,6 +147,8 @@ enum {
 #define AXP20X_CC_CTRL			0xb8
 #define AXP20X_FG_RES			0xb9
 
+#define AXP22X_BATLOW_THRES1		0xe6
+
 /* Regulators IDs */
 enum {
 	AXP20X_LDO1 = 0,
@@ -128,6 +161,29 @@ enum {
 	AXP20X_REG_ID_MAX,
 };
 
+enum {
+	AXP22X_DCDC1 = 0,
+	AXP22X_DCDC2,
+	AXP22X_DCDC3,
+	AXP22X_DCDC4,
+	AXP22X_DCDC5,
+	AXP22X_DC5LDO,
+	AXP22X_ALDO1,
+	AXP22X_ALDO2,
+	AXP22X_ALDO3,
+	AXP22X_ELDO1,
+	AXP22X_ELDO2,
+	AXP22X_ELDO3,
+	AXP22X_DLDO1,
+	AXP22X_DLDO2,
+	AXP22X_DLDO3,
+	AXP22X_DLDO4,
+	AXP22X_RTC_LDO,
+	AXP22X_LDO_IO0,
+	AXP22X_LDO_IO1,
+	AXP22X_REG_ID_MAX,
+};
+
 /* IRQs */
 enum {
 	AXP20X_IRQ_ACIN_OVER_V = 1,
-- 
1.8.3.2

Re: [PATCH v4 1/7] mfd: axp20x: add AXP221 PMIC support

[Lee Jones]

On Tue, 17 Jun 2014, Boris BREZILLON wrote:

> Add support for the AXP221 PMIC device to the existing AXP20x driver.
> 
> The AXP221 defines a new set of registers, power supplies and regulators,
> but most of the API is similar to the AXP20x ones.
> The AXP20x irq chip definition is reused, though some interrupts are not
> available in the AXP221.
> 
> Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
> ---
>  drivers/mfd/axp20x.c       | 64 ++++++++++++++++++++++++++++++++++++++++++----
>  include/linux/mfd/axp20x.h | 56 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 115 insertions(+), 5 deletions(-)

Looks good to me now.
  Acked-by: Lee Jones <lee.jones@linaro.org>

Can this go in independently to the other patches in the set?

> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
> index dee6539..ad4c177 100644
> --- a/drivers/mfd/axp20x.c
> +++ b/drivers/mfd/axp20x.c
> @@ -33,6 +33,11 @@ static const struct regmap_range axp20x_writeable_ranges[] = {
>  	regmap_reg_range(AXP20X_DCDC_MODE, AXP20X_FG_RES),
>  };
>  
> +static const struct regmap_range axp22x_writeable_ranges[] = {
> +	regmap_reg_range(AXP20X_DATACACHE(0), AXP20X_IRQ5_STATE),
> +	regmap_reg_range(AXP20X_DCDC_MODE, AXP22X_BATLOW_THRES1),
> +};
> +
>  static const struct regmap_range axp20x_volatile_ranges[] = {
>  	regmap_reg_range(AXP20X_IRQ1_EN, AXP20X_IRQ5_STATE),
>  };
> @@ -42,6 +47,11 @@ static const struct regmap_access_table axp20x_writeable_table = {
>  	.n_yes_ranges	= ARRAY_SIZE(axp20x_writeable_ranges),
>  };
>  
> +static const struct regmap_access_table axp22x_writeable_table = {
> +	.yes_ranges	= axp22x_writeable_ranges,
> +	.n_yes_ranges	= ARRAY_SIZE(axp22x_writeable_ranges),
> +};
> +
>  static const struct regmap_access_table axp20x_volatile_table = {
>  	.yes_ranges	= axp20x_volatile_ranges,
>  	.n_yes_ranges	= ARRAY_SIZE(axp20x_volatile_ranges),
> @@ -70,6 +80,15 @@ static const struct regmap_config axp20x_regmap_config = {
>  	.cache_type	= REGCACHE_RBTREE,
>  };
>  
> +static const struct regmap_config axp22x_regmap_config = {
> +	.reg_bits	= 8,
> +	.val_bits	= 8,
> +	.wr_table	= &axp22x_writeable_table,
> +	.volatile_table	= &axp20x_volatile_table,
> +	.max_register	= AXP22X_BATLOW_THRES1,
> +	.cache_type	= REGCACHE_RBTREE,
> +};
> +
>  #define AXP20X_IRQ(_irq, _off, _mask) \
>  	[AXP20X_IRQ_##_irq] = { .reg_offset = (_off), .mask = BIT(_mask) }
>  
> @@ -116,6 +135,7 @@ static const struct regmap_irq axp20x_regmap_irqs[] = {
>  static const struct of_device_id axp20x_of_match[] = {
>  	{ .compatible = "x-powers,axp202", .data = (void *) AXP202_ID },
>  	{ .compatible = "x-powers,axp209", .data = (void *) AXP209_ID },
> +	{ .compatible = "x-powers,axp221", .data = (void *) AXP221_ID },
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(of, axp20x_of_match);
> @@ -149,6 +169,21 @@ static const char * const axp20x_supplies[] = {
>  	"ldo5in",
>  };
>  
> +static const char *axp22x_supplies[] = {
> +	"vbus",
> +	"acin",
> +	"vin1",
> +	"vin2",
> +	"vin3",
> +	"vin4",
> +	"vin5",
> +	"aldoin",
> +	"dldoin",
> +	"eldoin",
> +	"ldoioin",
> +	"rtcldoin",
> +};
> +
>  static struct mfd_cell axp20x_cells[] = {
>  	{
>  		.name			= "axp20x-pek",
> @@ -161,6 +196,14 @@ static struct mfd_cell axp20x_cells[] = {
>  	},
>  };
>  
> +static struct mfd_cell axp22x_cells[] = {
> +	{
> +		.name			= "axp20x-regulator",
> +		.parent_supplies	= axp22x_supplies,
> +		.num_parent_supplies	= ARRAY_SIZE(axp22x_supplies),
> +	},
> +};
> +
>  static struct axp20x_dev *axp20x_pm_power_off;
>  static void axp20x_power_off(void)
>  {
> @@ -171,8 +214,11 @@ static void axp20x_power_off(void)
>  static int axp20x_i2c_probe(struct i2c_client *i2c,
>  			 const struct i2c_device_id *id)
>  {
> -	struct axp20x_dev *axp20x;
> +	const struct regmap_config *regmap_cfg;
>  	const struct of_device_id *of_id;
> +	struct axp20x_dev *axp20x;
> +	struct mfd_cell *cells;
> +	int ncells;
>  	int ret;
>  
>  	axp20x = devm_kzalloc(&i2c->dev, sizeof(*axp20x), GFP_KERNEL);
> @@ -190,7 +236,17 @@ static int axp20x_i2c_probe(struct i2c_client *i2c,
>  	axp20x->dev = &i2c->dev;
>  	dev_set_drvdata(axp20x->dev, axp20x);
>  
> -	axp20x->regmap = devm_regmap_init_i2c(i2c, &axp20x_regmap_config);
> +	if (axp20x->variant == AXP221_ID) {
> +		regmap_cfg = &axp22x_regmap_config;
> +		cells = axp22x_cells;
> +		ncells = ARRAY_SIZE(axp22x_cells);
> +	} else {
> +		regmap_cfg = &axp20x_regmap_config;
> +		cells = axp20x_cells;
> +		ncells = ARRAY_SIZE(axp20x_cells);
> +	}
> +
> +	axp20x->regmap = devm_regmap_init_i2c(i2c, regmap_cfg);
>  	if (IS_ERR(axp20x->regmap)) {
>  		ret = PTR_ERR(axp20x->regmap);
>  		dev_err(&i2c->dev, "regmap init failed: %d\n", ret);
> @@ -206,9 +262,7 @@ static int axp20x_i2c_probe(struct i2c_client *i2c,
>  		return ret;
>  	}
>  
> -	ret = mfd_add_devices(axp20x->dev, -1, axp20x_cells,
> -			      ARRAY_SIZE(axp20x_cells), NULL, 0, NULL);
> -
> +	ret = mfd_add_devices(axp20x->dev, -1, cells, ncells, NULL, 0, NULL);
>  	if (ret) {
>  		dev_err(&i2c->dev, "failed to add MFD devices: %d\n", ret);
>  		regmap_del_irq_chip(i2c->irq, axp20x->regmap_irqc);
> diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
> index d0e31a2..8a8ce02 100644
> --- a/include/linux/mfd/axp20x.h
> +++ b/include/linux/mfd/axp20x.h
> @@ -14,6 +14,7 @@
>  enum {
>  	AXP202_ID = 0,
>  	AXP209_ID,
> +	AXP221_ID,
>  };
>  
>  #define AXP20X_DATACACHE(m)		(0x04 + (m))
> @@ -43,6 +44,28 @@ enum {
>  #define AXP20X_V_LTF_DISCHRG		0x3c
>  #define AXP20X_V_HTF_DISCHRG		0x3d
>  
> +#define AXP22X_PWR_OUT_CTRL1		0x10
> +#define AXP22X_PWR_OUT_CTRL2		0x12
> +#define AXP22X_PWR_OUT_CTRL3		0x13
> +#define AXP22X_DLDO1_V_OUT		0x15
> +#define AXP22X_DLDO2_V_OUT		0x16
> +#define AXP22X_DLDO3_V_OUT		0x17
> +#define AXP22X_DLDO4_V_OUT		0x18
> +#define AXP22X_ELDO1_V_OUT		0x19
> +#define AXP22X_ELDO2_V_OUT		0x1a
> +#define AXP22X_ELDO3_V_OUT		0x1b
> +#define AXP22X_DC5LDO_V_OUT		0x1c
> +#define AXP22X_DCDC1_V_OUT		0x21
> +#define AXP22X_DCDC2_V_OUT		0x22
> +#define AXP22X_DCDC3_V_OUT		0x23
> +#define AXP22X_DCDC4_V_OUT		0x24
> +#define AXP22X_DCDC5_V_OUT		0x25
> +#define AXP22X_DCDC23_V_RAMP_CTRL	0x27
> +#define AXP22X_ALDO1_V_OUT		0x28
> +#define AXP22X_ALDO2_V_OUT		0x29
> +#define AXP22X_ALDO3_V_OUT		0x2a
> +#define AXP22X_CHRG_CTRL3		0x35
> +
>  /* Interrupt */
>  #define AXP20X_IRQ1_EN			0x40
>  #define AXP20X_IRQ2_EN			0x41
> @@ -96,6 +119,9 @@ enum {
>  #define AXP20X_VBUS_MON			0x8b
>  #define AXP20X_OVER_TMP			0x8f
>  
> +#define AXP22X_PWREN_CTRL1		0x8c
> +#define AXP22X_PWREN_CTRL2		0x8d
> +
>  /* GPIO */
>  #define AXP20X_GPIO0_CTRL		0x90
>  #define AXP20X_LDO5_V_OUT		0x91
> @@ -104,6 +130,11 @@ enum {
>  #define AXP20X_GPIO20_SS		0x94
>  #define AXP20X_GPIO3_CTRL		0x95
>  
> +#define AXP22X_LDO_IO0_V_OUT		0x91
> +#define AXP22X_LDO_IO1_V_OUT		0x93
> +#define AXP22X_GPIO_STATE		0x94
> +#define AXP22X_GPIO_PULL_DOWN		0x95
> +
>  /* Battery */
>  #define AXP20X_CHRG_CC_31_24		0xb0
>  #define AXP20X_CHRG_CC_23_16		0xb1
> @@ -116,6 +147,8 @@ enum {
>  #define AXP20X_CC_CTRL			0xb8
>  #define AXP20X_FG_RES			0xb9
>  
> +#define AXP22X_BATLOW_THRES1		0xe6
> +
>  /* Regulators IDs */
>  enum {
>  	AXP20X_LDO1 = 0,
> @@ -128,6 +161,29 @@ enum {
>  	AXP20X_REG_ID_MAX,
>  };
>  
> +enum {
> +	AXP22X_DCDC1 = 0,
> +	AXP22X_DCDC2,
> +	AXP22X_DCDC3,
> +	AXP22X_DCDC4,
> +	AXP22X_DCDC5,
> +	AXP22X_DC5LDO,
> +	AXP22X_ALDO1,
> +	AXP22X_ALDO2,
> +	AXP22X_ALDO3,
> +	AXP22X_ELDO1,
> +	AXP22X_ELDO2,
> +	AXP22X_ELDO3,
> +	AXP22X_DLDO1,
> +	AXP22X_DLDO2,
> +	AXP22X_DLDO3,
> +	AXP22X_DLDO4,
> +	AXP22X_RTC_LDO,
> +	AXP22X_LDO_IO0,
> +	AXP22X_LDO_IO1,
> +	AXP22X_REG_ID_MAX,
> +};
> +
>  /* IRQs */
>  enum {
>  	AXP20X_IRQ_ACIN_OVER_V = 1,

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v4 1/7] mfd: axp20x: add AXP221 PMIC support

[Boris BREZILLON]

On 18/06/2014 10:36, Lee Jones wrote:
> On Tue, 17 Jun 2014, Boris BREZILLON wrote:
>
>> Add support for the AXP221 PMIC device to the existing AXP20x driver.
>>
>> The AXP221 defines a new set of registers, power supplies and regulators,
>> but most of the API is similar to the AXP20x ones.
>> The AXP20x irq chip definition is reused, though some interrupts are not
>> available in the AXP221.
>>
>> Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
>> ---
>>  drivers/mfd/axp20x.c       | 64 ++++++++++++++++++++++++++++++++++++++++++----
>>  include/linux/mfd/axp20x.h | 56 ++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 115 insertions(+), 5 deletions(-)
> Looks good to me now.
>   Acked-by: Lee Jones <lee.jones@linaro.org>
>
> Can this go in independently to the other patches in the set?

Yes, as long as no one define an axp221 node in his DT.
In this case axp221 regulators would be considered as axp20x ones (which
might be a bit problematic), because axp20x-regulator driver does not
test the axp variant before probing its regulators (this test is
introduced in patch 3).

Best Regards,

Boris


-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH v4 1/7] mfd: axp20x: add AXP221 PMIC support

[Lee Jones]

On Wed, 18 Jun 2014, Boris BREZILLON wrote:
> On 18/06/2014 10:36, Lee Jones wrote:
> > On Tue, 17 Jun 2014, Boris BREZILLON wrote:
> >
> >> Add support for the AXP221 PMIC device to the existing AXP20x driver.
> >>
> >> The AXP221 defines a new set of registers, power supplies and regulators,
> >> but most of the API is similar to the AXP20x ones.
> >> The AXP20x irq chip definition is reused, though some interrupts are not
> >> available in the AXP221.
> >>
> >> Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
> >> ---
> >>  drivers/mfd/axp20x.c       | 64 ++++++++++++++++++++++++++++++++++++++++++----
> >>  include/linux/mfd/axp20x.h | 56 ++++++++++++++++++++++++++++++++++++++++
> >>  2 files changed, 115 insertions(+), 5 deletions(-)
> > Looks good to me now.
> >   Acked-by: Lee Jones <lee.jones@linaro.org>
> >
> > Can this go in independently to the other patches in the set?
> 
> Yes, as long as no one define an axp221 node in his DT.
> In this case axp221 regulators would be considered as axp20x ones (which
> might be a bit problematic), because axp20x-regulator driver does not
> test the axp variant before probing its regulators (this test is
> introduced in patch 3).

Then I'd prefer to wait and have the dependencies go in at the same
time.

Mark,
  As normal, I'm happy to create a shared branch between us.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH v4 2/7] regulator: axp20x: prepare support for multiple AXP chip families

[Boris BREZILLON]

Rework the AXP20X_ macros to support the several chip families, so that
each family can define it's own set of regulators, and regulator matches.

Pass a match table to the axp20x_regulator_parse_dt function instead of
statically using the axp20x match table.

Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
---
 drivers/regulator/axp20x-regulator.c | 74 +++++++++++++++++++-----------------
 1 file changed, 39 insertions(+), 35 deletions(-)

diff --git a/drivers/regulator/axp20x-regulator.c b/drivers/regulator/axp20x-regulator.c
index 004aadb..9716f8e 100644
--- a/drivers/regulator/axp20x-regulator.c
+++ b/drivers/regulator/axp20x-regulator.c
@@ -32,13 +32,13 @@
 
 #define AXP20X_FREQ_DCDC_MASK		0x0f
 
-#define AXP20X_DESC_IO(_id, _supply, _min, _max, _step, _vreg, _vmask, _ereg,   \
-		       _emask, _enable_val, _disable_val)			\
-	[AXP20X_##_id] = {							\
+#define AXP_DESC_IO(_family, _id, _supply, _min, _max, _step, _vreg, _vmask,	\
+		    _ereg, _emask, _enable_val, _disable_val)			\
+	[_family##_##_id] = {							\
 		.name		= #_id,						\
 		.supply_name	= (_supply),					\
 		.type		= REGULATOR_VOLTAGE,				\
-		.id		= AXP20X_##_id,					\
+		.id		= _family##_##_id,				\
 		.n_voltages	= (((_max) - (_min)) / (_step) + 1),		\
 		.owner		= THIS_MODULE,					\
 		.min_uV		= (_min) * 1000,				\
@@ -52,13 +52,13 @@
 		.ops		= &axp20x_ops,					\
 	}
 
-#define AXP20X_DESC(_id, _supply, _min, _max, _step, _vreg, _vmask, _ereg,	\
-		    _emask) 							\
-	[AXP20X_##_id] = {							\
+#define AXP_DESC(_family, _id, _supply, _min, _max, _step, _vreg, _vmask, 	\
+		 _ereg, _emask) 						\
+	[_family##_##_id] = {							\
 		.name		= #_id,						\
 		.supply_name	= (_supply),					\
 		.type		= REGULATOR_VOLTAGE,				\
-		.id		= AXP20X_##_id,					\
+		.id		= _family##_##_id,				\
 		.n_voltages	= (((_max) - (_min)) / (_step) + 1),		\
 		.owner		= THIS_MODULE,					\
 		.min_uV		= (_min) * 1000,				\
@@ -70,24 +70,25 @@
 		.ops		= &axp20x_ops,					\
 	}
 
-#define AXP20X_DESC_FIXED(_id, _supply, _volt)					\
-	[AXP20X_##_id] = {							\
+#define AXP_DESC_FIXED(_family, _id, _supply, _volt)				\
+	[_family##_##_id] = {							\
 		.name		= #_id,						\
 		.supply_name	= (_supply),					\
 		.type		= REGULATOR_VOLTAGE,				\
-		.id		= AXP20X_##_id,					\
+		.id		= _family##_##_id,				\
 		.n_voltages	= 1,						\
 		.owner		= THIS_MODULE,					\
 		.min_uV		= (_volt) * 1000,				\
 		.ops		= &axp20x_ops_fixed				\
 	}
 
-#define AXP20X_DESC_TABLE(_id, _supply, _table, _vreg, _vmask, _ereg, _emask)	\
-	[AXP20X_##_id] = {							\
+#define AXP_DESC_TABLE(_family, _id, _supply, _table, _vreg, _vmask, _ereg,	\
+		       _emask)							\
+	[_family##_##_id] = {							\
 		.name		= #_id,						\
 		.supply_name	= (_supply),					\
 		.type		= REGULATOR_VOLTAGE,				\
-		.id		= AXP20X_##_id,					\
+		.id		= _family##_##_id,				\
 		.n_voltages	= ARRAY_SIZE(_table),				\
 		.owner		= THIS_MODULE,					\
 		.vsel_reg	= (_vreg),					\
@@ -127,36 +128,36 @@ static struct regulator_ops axp20x_ops = {
 };
 
 static const struct regulator_desc axp20x_regulators[] = {
-	AXP20X_DESC(DCDC2, "vin2", 700, 2275, 25, AXP20X_DCDC2_V_OUT, 0x3f,
+	AXP_DESC(AXP20X, DCDC2, "vin2", 700, 2275, 25, AXP20X_DCDC2_V_OUT, 0x3f,
 		    AXP20X_PWR_OUT_CTRL, 0x10),
-	AXP20X_DESC(DCDC3, "vin3", 700, 3500, 25, AXP20X_DCDC3_V_OUT, 0x7f,
+	AXP_DESC(AXP20X, DCDC3, "vin3", 700, 3500, 25, AXP20X_DCDC3_V_OUT, 0x7f,
 		    AXP20X_PWR_OUT_CTRL, 0x02),
-	AXP20X_DESC_FIXED(LDO1, "acin", 1300),
-	AXP20X_DESC(LDO2, "ldo24in", 1800, 3300, 100, AXP20X_LDO24_V_OUT, 0xf0,
+	AXP_DESC_FIXED(AXP20X, LDO1, "acin", 1300),
+	AXP_DESC(AXP20X, LDO2, "ldo24in", 1800, 3300, 100, AXP20X_LDO24_V_OUT, 0xf0,
 		    AXP20X_PWR_OUT_CTRL, 0x04),
-	AXP20X_DESC(LDO3, "ldo3in", 700, 3500, 25, AXP20X_LDO3_V_OUT, 0x7f,
+	AXP_DESC(AXP20X, LDO3, "ldo3in", 700, 3500, 25, AXP20X_LDO3_V_OUT, 0x7f,
 		    AXP20X_PWR_OUT_CTRL, 0x40),
-	AXP20X_DESC_TABLE(LDO4, "ldo24in", axp20x_ldo4_data, AXP20X_LDO24_V_OUT, 0x0f,
+	AXP_DESC_TABLE(AXP20X, LDO4, "ldo24in", axp20x_ldo4_data, AXP20X_LDO24_V_OUT, 0x0f,
 			  AXP20X_PWR_OUT_CTRL, 0x08),
-	AXP20X_DESC_IO(LDO5, "ldo5in", 1800, 3300, 100, AXP20X_LDO5_V_OUT, 0xf0,
+	AXP_DESC_IO(AXP20X, LDO5, "ldo5in", 1800, 3300, 100, AXP20X_LDO5_V_OUT, 0xf0,
 		       AXP20X_GPIO0_CTRL, 0x07, AXP20X_IO_ENABLED,
 		       AXP20X_IO_DISABLED),
 };
 
-#define AXP_MATCH(_name, _id) \
-	[AXP20X_##_id] = { \
+#define AXP_MATCH(_family, _name, _id) \
+	[_family##_##_id] = { \
 		.name		= #_name, \
-		.driver_data	= (void *) &axp20x_regulators[AXP20X_##_id], \
+		.driver_data	= (void *) &axp20x_regulators[_family##_##_id], \
 	}
 
 static struct of_regulator_match axp20x_matches[] = {
-	AXP_MATCH(dcdc2, DCDC2),
-	AXP_MATCH(dcdc3, DCDC3),
-	AXP_MATCH(ldo1, LDO1),
-	AXP_MATCH(ldo2, LDO2),
-	AXP_MATCH(ldo3, LDO3),
-	AXP_MATCH(ldo4, LDO4),
-	AXP_MATCH(ldo5, LDO5),
+	AXP_MATCH(AXP20X, dcdc2, DCDC2),
+	AXP_MATCH(AXP20X, dcdc3, DCDC3),
+	AXP_MATCH(AXP20X, ldo1, LDO1),
+	AXP_MATCH(AXP20X, ldo2, LDO2),
+	AXP_MATCH(AXP20X, ldo3, LDO3),
+	AXP_MATCH(AXP20X, ldo4, LDO4),
+	AXP_MATCH(AXP20X, ldo5, LDO5),
 };
 
 static int axp20x_set_dcdc_freq(struct platform_device *pdev, u32 dcdcfreq)
@@ -179,7 +180,9 @@ static int axp20x_set_dcdc_freq(struct platform_device *pdev, u32 dcdcfreq)
 				  AXP20X_FREQ_DCDC_MASK, dcdcfreq);
 }
 
-static int axp20x_regulator_parse_dt(struct platform_device *pdev)
+static int axp20x_regulator_parse_dt(struct platform_device *pdev,
+				     struct of_regulator_match *matches,
+				     int nmatches)
 {
 	struct device_node *np, *regulators;
 	int ret;
@@ -193,8 +196,8 @@ static int axp20x_regulator_parse_dt(struct platform_device *pdev)
 	if (!regulators) {
 		dev_warn(&pdev->dev, "regulators node not found\n");
 	} else {
-		ret = of_regulator_match(&pdev->dev, regulators, axp20x_matches,
-					 ARRAY_SIZE(axp20x_matches));
+		ret = of_regulator_match(&pdev->dev, regulators, matches,
+					 nmatches);
 		if (ret < 0) {
 			dev_err(&pdev->dev, "Error parsing regulator init data: %d\n", ret);
 			return ret;
@@ -238,7 +241,8 @@ static int axp20x_regulator_probe(struct platform_device *pdev)
 	int ret, i;
 	u32 workmode;
 
-	ret = axp20x_regulator_parse_dt(pdev);
+	ret = axp20x_regulator_parse_dt(pdev, axp20x_matches,
+					ARRAY_SIZE(axp20x_matches));
 	if (ret)
 		return ret;
 
-- 
1.8.3.2

[PATCH v4 3/7] regulator: axp20x: add support for AXP221 regulators

[Boris BREZILLON]

Add AXP221 regulator definitions and choose the appropriate definitions
according to the variant id passed by the MFD device.

Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
---
 drivers/regulator/axp20x-regulator.c | 98 +++++++++++++++++++++++++++++++++---
 1 file changed, 90 insertions(+), 8 deletions(-)

diff --git a/drivers/regulator/axp20x-regulator.c b/drivers/regulator/axp20x-regulator.c
index 9716f8e..7a30f49 100644
--- a/drivers/regulator/axp20x-regulator.c
+++ b/drivers/regulator/axp20x-regulator.c
@@ -27,6 +27,9 @@
 #define AXP20X_IO_ENABLED		0x03
 #define AXP20X_IO_DISABLED		0x07
 
+#define AXP22X_IO_ENABLED		0x04
+#define AXP22X_IO_DISABLED		0x03
+
 #define AXP20X_WORKMODE_DCDC2_MASK	BIT(2)
 #define AXP20X_WORKMODE_DCDC3_MASK	BIT(1)
 
@@ -144,6 +147,48 @@ static const struct regulator_desc axp20x_regulators[] = {
 		       AXP20X_IO_DISABLED),
 };
 
+static const struct regulator_desc axp22x_regulators[] = {
+	AXP_DESC(AXP22X, DCDC1, "vin1", 1600, 3400, 100, AXP22X_DCDC1_V_OUT, 0x1f,
+		    AXP22X_PWR_OUT_CTRL1, BIT(1)),
+	AXP_DESC(AXP22X, DCDC2, "vin2", 600, 1540, 20, AXP22X_DCDC2_V_OUT, 0x3f,
+		    AXP22X_PWR_OUT_CTRL1, BIT(2)),
+	AXP_DESC(AXP22X, DCDC3, "vin3", 600, 1860, 20, AXP22X_DCDC3_V_OUT, 0x3f,
+		    AXP22X_PWR_OUT_CTRL1, BIT(3)),
+	AXP_DESC(AXP22X, DCDC4, "vin4", 600, 1540, 20, AXP22X_DCDC4_V_OUT, 0x3f,
+		    AXP22X_PWR_OUT_CTRL1, BIT(3)),
+	AXP_DESC(AXP22X, DCDC5, "vin5", 1000, 2550, 50, AXP22X_DCDC5_V_OUT, 0x1f,
+		    AXP22X_PWR_OUT_CTRL1, BIT(4)),
+	AXP_DESC(AXP22X, DC5LDO, "vin5", 700, 1400, 100, AXP22X_DC5LDO_V_OUT, 0x7,
+		    AXP22X_PWR_OUT_CTRL1, BIT(0)),
+	AXP_DESC(AXP22X, ALDO1, "aldoin", 700, 3300, 100, AXP22X_ALDO1_V_OUT, 0x1f,
+		    AXP22X_PWR_OUT_CTRL1, BIT(6)),
+	AXP_DESC(AXP22X, ALDO2, "aldoin", 700, 3300, 100, AXP22X_ALDO2_V_OUT, 0x1f,
+		    AXP22X_PWR_OUT_CTRL1, BIT(7)),
+	AXP_DESC(AXP22X, ALDO3, "aldoin", 700, 3300, 100, AXP22X_ALDO3_V_OUT, 0x1f,
+		    AXP22X_PWR_OUT_CTRL3, BIT(7)),
+	AXP_DESC(AXP22X, DLDO1, "dldoin", 700, 3300, 100, AXP22X_DLDO1_V_OUT, 0x1f,
+		    AXP22X_PWR_OUT_CTRL2, BIT(3)),
+	AXP_DESC(AXP22X, DLDO2, "dldoin", 700, 3300, 100, AXP22X_DLDO2_V_OUT, 0x1f,
+		    AXP22X_PWR_OUT_CTRL2, BIT(4)),
+	AXP_DESC(AXP22X, DLDO3, "dldoin", 700, 3300, 100, AXP22X_DLDO3_V_OUT, 0x1f,
+		    AXP22X_PWR_OUT_CTRL2, BIT(5)),
+	AXP_DESC(AXP22X, DLDO4, "dldoin", 700, 3300, 100, AXP22X_DLDO4_V_OUT, 0x1f,
+		    AXP22X_PWR_OUT_CTRL2, BIT(6)),
+	AXP_DESC(AXP22X, ELDO1, "eldoin", 700, 3300, 100, AXP22X_ELDO1_V_OUT, 0x1f,
+		    AXP22X_PWR_OUT_CTRL2, BIT(0)),
+	AXP_DESC(AXP22X, ELDO2, "eldoin", 700, 3300, 100, AXP22X_ELDO2_V_OUT, 0x1f,
+		    AXP22X_PWR_OUT_CTRL2, BIT(1)),
+	AXP_DESC(AXP22X, ELDO3, "eldoin", 700, 3300, 100, AXP22X_ELDO3_V_OUT, 0x1f,
+		    AXP22X_PWR_OUT_CTRL2, BIT(2)),
+	AXP_DESC_IO(AXP22X, LDO_IO0, "ldoioin", 1800, 3300, 100, AXP22X_LDO_IO0_V_OUT,
+		       0x1f, AXP20X_GPIO0_CTRL, 0x07, AXP22X_IO_ENABLED,
+		       AXP22X_IO_DISABLED),
+	AXP_DESC_IO(AXP22X, LDO_IO1, "ldoioin", 1800, 3300, 100, AXP22X_LDO_IO1_V_OUT,
+		       0x1f, AXP20X_GPIO1_CTRL, 0x07, AXP22X_IO_ENABLED,
+		       AXP22X_IO_DISABLED),
+	AXP_DESC_FIXED(AXP22X, RTC_LDO, "rtcldoin", 3000),
+};
+
 #define AXP_MATCH(_family, _name, _id) \
 	[_family##_##_id] = { \
 		.name		= #_name, \
@@ -160,6 +205,28 @@ static struct of_regulator_match axp20x_matches[] = {
 	AXP_MATCH(AXP20X, ldo5, LDO5),
 };
 
+static struct of_regulator_match axp22x_matches[] = {
+	AXP_MATCH(AXP22X, dcdc1, DCDC1),
+	AXP_MATCH(AXP22X, dcdc2, DCDC2),
+	AXP_MATCH(AXP22X, dcdc3, DCDC3),
+	AXP_MATCH(AXP22X, dcdc4, DCDC4),
+	AXP_MATCH(AXP22X, dcdc5, DCDC5),
+	AXP_MATCH(AXP22X, dc5ldo, DC5LDO),
+	AXP_MATCH(AXP22X, aldo1, ALDO1),
+	AXP_MATCH(AXP22X, aldo2, ALDO2),
+	AXP_MATCH(AXP22X, aldo3, ALDO3),
+	AXP_MATCH(AXP22X, dldo1, DLDO1),
+	AXP_MATCH(AXP22X, dldo2, DLDO2),
+	AXP_MATCH(AXP22X, dldo3, DLDO3),
+	AXP_MATCH(AXP22X, dldo4, DLDO4),
+	AXP_MATCH(AXP22X, eldo1, ELDO1),
+	AXP_MATCH(AXP22X, eldo2, ELDO2),
+	AXP_MATCH(AXP22X, eldo3, ELDO3),
+	AXP_MATCH(AXP22X, ldo_io0, LDO_IO0),
+	AXP_MATCH(AXP22X, ldo_io1, LDO_IO1),
+	AXP_MATCH(AXP22X, rtc_ldo, RTC_LDO),
+};
+
 static int axp20x_set_dcdc_freq(struct platform_device *pdev, u32 dcdcfreq)
 {
 	struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
@@ -238,37 +305,52 @@ static int axp20x_regulator_probe(struct platform_device *pdev)
 	struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
 	struct regulator_config config = { };
 	struct regulator_init_data *init_data;
+	struct of_regulator_match *matches;
+	int nmatches;
+	const struct regulator_desc *regulators;
+	int nregulators;
 	int ret, i;
 	u32 workmode;
 
-	ret = axp20x_regulator_parse_dt(pdev, axp20x_matches,
-					ARRAY_SIZE(axp20x_matches));
+	if (axp20x->variant == AXP221_ID) {
+		matches = axp22x_matches;
+		nmatches = ARRAY_SIZE(axp22x_matches);
+		regulators = axp22x_regulators;
+		nregulators = AXP22X_REG_ID_MAX;
+	} else {
+		matches = axp20x_matches;
+		nmatches = ARRAY_SIZE(axp20x_matches);
+		regulators = axp20x_regulators;
+		nregulators = AXP20X_REG_ID_MAX;
+	}
+
+	ret = axp20x_regulator_parse_dt(pdev, matches, nmatches);
 	if (ret)
 		return ret;
 
 	for (i = 0; i < AXP20X_REG_ID_MAX; i++) {
-		init_data = axp20x_matches[i].init_data;
+		init_data = matches[i].init_data;
 
 		config.dev = &pdev->dev;
 		config.init_data = init_data;
 		config.regmap = axp20x->regmap;
-		config.of_node = axp20x_matches[i].of_node;
+		config.of_node = matches[i].of_node;
 
-		rdev = devm_regulator_register(&pdev->dev, &axp20x_regulators[i],
+		rdev = devm_regulator_register(&pdev->dev, &regulators[i],
 					       &config);
 		if (IS_ERR(rdev)) {
 			dev_err(&pdev->dev, "Failed to register %s\n",
-				axp20x_regulators[i].name);
+				regulators[i].name);
 
 			return PTR_ERR(rdev);
 		}
 
-		ret = of_property_read_u32(axp20x_matches[i].of_node, "x-powers,dcdc-workmode",
+		ret = of_property_read_u32(matches[i].of_node, "x-powers,dcdc-workmode",
 					   &workmode);
 		if (!ret) {
 			if (axp20x_set_dcdc_workmode(rdev, i, workmode))
 				dev_err(&pdev->dev, "Failed to set workmode on %s\n",
-					axp20x_regulators[i].name);
+					regulators[i].name);
 		}
 	}
 
-- 
1.8.3.2

[PATCH v4 4/7] regulator: axp20x: reset probe data before each probe

[Boris BREZILLON]

The init_data and of_node fields of the axp2xx_matches tables are filled
at each device probe by the axp20x_regulator_parse_dt function (which then
calls the of_regulator_match function).
This means we can probe a new device and consider data initialized during
the probe of another device as valid.

Reset init_data and of_node field to NULL before each probe in order to
avoid this kind of issue.

Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
---
 drivers/regulator/axp20x-regulator.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/regulator/axp20x-regulator.c b/drivers/regulator/axp20x-regulator.c
index 7a30f49..d42bf6d 100644
--- a/drivers/regulator/axp20x-regulator.c
+++ b/drivers/regulator/axp20x-regulator.c
@@ -324,6 +324,15 @@ static int axp20x_regulator_probe(struct platform_device *pdev)
 		nregulators = AXP20X_REG_ID_MAX;
 	}
 
+	/*
+	 * Reset matches table (this table might have been modified by a
+	 * previous AXP2xx device probe).
+	 */
+	for (i = 0; i < nmatches; i++) {
+		matches[i].init_data = NULL;
+		matches[i].of_node = NULL;
+	}
+
 	ret = axp20x_regulator_parse_dt(pdev, matches, nmatches);
 	if (ret)
 		return ret;
-- 
1.8.3.2

Re: [PATCH v4 4/7] regulator: axp20x: reset probe data before each probe

[Maxime Ripard]

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

On Tue, Jun 17, 2014 at 09:38:40AM +0200, Boris BREZILLON wrote:
> The init_data and of_node fields of the axp2xx_matches tables are filled
> at each device probe by the axp20x_regulator_parse_dt function (which then
> calls the of_regulator_match function).
> This means we can probe a new device and consider data initialized during
> the probe of another device as valid.
> 
> Reset init_data and of_node field to NULL before each probe in order to
> avoid this kind of issue.
> 
> Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
> ---
>  drivers/regulator/axp20x-regulator.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/regulator/axp20x-regulator.c b/drivers/regulator/axp20x-regulator.c
> index 7a30f49..d42bf6d 100644
> --- a/drivers/regulator/axp20x-regulator.c
> +++ b/drivers/regulator/axp20x-regulator.c
> @@ -324,6 +324,15 @@ static int axp20x_regulator_probe(struct platform_device *pdev)
>  		nregulators = AXP20X_REG_ID_MAX;
>  	}
>  
> +	/*
> +	 * Reset matches table (this table might have been modified by a
> +	 * previous AXP2xx device probe).
> +	 */
> +	for (i = 0; i < nmatches; i++) {
> +		matches[i].init_data = NULL;
> +		matches[i].of_node = NULL;
> +	}
> +

That looks rather hackish, especially since we've never been in such a
case yet, since we have a single PMIC in our system.

Can't you just use memzero here?

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

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

Re: [PATCH v4 4/7] regulator: axp20x: reset probe data before each probe

[Boris BREZILLON]

On 17/06/2014 22:44, Maxime Ripard wrote:
> On Tue, Jun 17, 2014 at 09:38:40AM +0200, Boris BREZILLON wrote:
>> The init_data and of_node fields of the axp2xx_matches tables are filled
>> at each device probe by the axp20x_regulator_parse_dt function (which then
>> calls the of_regulator_match function).
>> This means we can probe a new device and consider data initialized during
>> the probe of another device as valid.
>>
>> Reset init_data and of_node field to NULL before each probe in order to
>> avoid this kind of issue.
>>
>> Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
>> ---
>>  drivers/regulator/axp20x-regulator.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/regulator/axp20x-regulator.c b/drivers/regulator/axp20x-regulator.c
>> index 7a30f49..d42bf6d 100644
>> --- a/drivers/regulator/axp20x-regulator.c
>> +++ b/drivers/regulator/axp20x-regulator.c
>> @@ -324,6 +324,15 @@ static int axp20x_regulator_probe(struct platform_device *pdev)
>>  		nregulators = AXP20X_REG_ID_MAX;
>>  	}
>>  
>> +	/*
>> +	 * Reset matches table (this table might have been modified by a
>> +	 * previous AXP2xx device probe).
>> +	 */
>> +	for (i = 0; i < nmatches; i++) {
>> +		matches[i].init_data = NULL;
>> +		matches[i].of_node = NULL;
>> +	}
>> +
> That looks rather hackish, especially since we've never been in such a
> case yet, since we have a single PMIC in our system.

Even if something is unlikely to happen, it doesn't mean it's impossible.
I'm pretty sure there are (or will be) some systems containing several
identical PMICs in the wild, and fixing this possible bug now prevents
us (or other developers) from having a big headache debugging this in
the future.

BTW, what is hackish in this code ?

>
> Can't you just use memzero here?

We can't just memset the whole matches table to zero, because some
fields (name and driver_data) are statically assigned.

>
> Maxime
>

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [linux-sunxi] Re: [PATCH v4 4/7] regulator: axp20x: reset probe data before each probe

[Hans de Goede]

Hi,

On 06/18/2014 09:11 AM, Boris BREZILLON wrote:
> 
> On 17/06/2014 22:44, Maxime Ripard wrote:
>> On Tue, Jun 17, 2014 at 09:38:40AM +0200, Boris BREZILLON wrote:
>>> The init_data and of_node fields of the axp2xx_matches tables are filled
>>> at each device probe by the axp20x_regulator_parse_dt function (which then
>>> calls the of_regulator_match function).
>>> This means we can probe a new device and consider data initialized during
>>> the probe of another device as valid.
>>>
>>> Reset init_data and of_node field to NULL before each probe in order to
>>> avoid this kind of issue.
>>>
>>> Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
>>> ---
>>>  drivers/regulator/axp20x-regulator.c | 9 +++++++++
>>>  1 file changed, 9 insertions(+)
>>>
>>> diff --git a/drivers/regulator/axp20x-regulator.c b/drivers/regulator/axp20x-regulator.c
>>> index 7a30f49..d42bf6d 100644
>>> --- a/drivers/regulator/axp20x-regulator.c
>>> +++ b/drivers/regulator/axp20x-regulator.c
>>> @@ -324,6 +324,15 @@ static int axp20x_regulator_probe(struct platform_device *pdev)
>>>  		nregulators = AXP20X_REG_ID_MAX;
>>>  	}
>>>  
>>> +	/*
>>> +	 * Reset matches table (this table might have been modified by a
>>> +	 * previous AXP2xx device probe).
>>> +	 */
>>> +	for (i = 0; i < nmatches; i++) {
>>> +		matches[i].init_data = NULL;
>>> +		matches[i].of_node = NULL;
>>> +	}
>>> +
>> That looks rather hackish, especially since we've never been in such a
>> case yet, since we have a single PMIC in our system.
> 
> Even if something is unlikely to happen, it doesn't mean it's impossible.
> I'm pretty sure there are (or will be) some systems containing several
> identical PMICs in the wild, and fixing this possible bug now prevents
> us (or other developers) from having a big headache debugging this in
> the future.

If you're really worried about this, you should also be worried
about 2 probes running at the same time racing against each other
(I know the bus level code will not do that now, but what about the
 future).

If you cannot treat / use the global struct as const, then you really should
have a local copy, and const-ify the global version and use it as a template
to initialize the local copy.

> BTW, what is hackish in this code ?

See above, changing a global struct, and then re-initializing it on the
next probe just is not pretty. TBH this raised my eyebrows the first time
you posted it already, but I decided to let it be. But since we're discussing
this now anyways I have to agree with Maxime.

Regards,

Hans

Re: [PATCH v4 4/7] regulator: axp20x: reset probe data before each probe

[Maxime Ripard]

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

On Wed, Jun 18, 2014 at 09:11:59AM +0200, Boris BREZILLON wrote:
> 
> On 17/06/2014 22:44, Maxime Ripard wrote:
> > On Tue, Jun 17, 2014 at 09:38:40AM +0200, Boris BREZILLON wrote:
> >> The init_data and of_node fields of the axp2xx_matches tables are filled
> >> at each device probe by the axp20x_regulator_parse_dt function (which then
> >> calls the of_regulator_match function).
> >> This means we can probe a new device and consider data initialized during
> >> the probe of another device as valid.
> >>
> >> Reset init_data and of_node field to NULL before each probe in order to
> >> avoid this kind of issue.
> >>
> >> Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
> >> ---
> >>  drivers/regulator/axp20x-regulator.c | 9 +++++++++
> >>  1 file changed, 9 insertions(+)
> >>
> >> diff --git a/drivers/regulator/axp20x-regulator.c b/drivers/regulator/axp20x-regulator.c
> >> index 7a30f49..d42bf6d 100644
> >> --- a/drivers/regulator/axp20x-regulator.c
> >> +++ b/drivers/regulator/axp20x-regulator.c
> >> @@ -324,6 +324,15 @@ static int axp20x_regulator_probe(struct platform_device *pdev)
> >>  		nregulators = AXP20X_REG_ID_MAX;
> >>  	}
> >>  
> >> +	/*
> >> +	 * Reset matches table (this table might have been modified by a
> >> +	 * previous AXP2xx device probe).
> >> +	 */
> >> +	for (i = 0; i < nmatches; i++) {
> >> +		matches[i].init_data = NULL;
> >> +		matches[i].of_node = NULL;
> >> +	}
> >> +
> > That looks rather hackish, especially since we've never been in such a
> > case yet, since we have a single PMIC in our system.
> 
> Even if something is unlikely to happen, it doesn't mean it's impossible.
> I'm pretty sure there are (or will be) some systems containing several
> identical PMICs in the wild, and fixing this possible bug now prevents
> us (or other developers) from having a big headache debugging this in
> the future.
> 
> BTW, what is hackish in this code ?

Pretty what Hans was saying, either you think that there will only be
one single instance of the driver, and using a global definition is
fine, or you can have several instances of the driver, and in this
case you'll use a dynamic allocation, but you seem to be stuck in
between.

I understand that you might not want to redeclare by hand the whole
match content, so maybe you can just use memcpy from the global
definition then.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

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

[PATCH v4 5/7] regulator: add support for regulator set registration

[Boris BREZILLON]

PMIC devices often provide several regulators, and these regulators are
all using the same regmap and are all attached to the same device (the
PMIC device).

Add helper functions (both simple and resource managed versions) to
register and unregister such kind of regulator set.

This implementation handles self dependency issues where one regulator
provided the PMIC depends on another one provided by the same PMIC.

Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
---
 drivers/regulator/core.c         | 106 +++++++++++++++++++++++++++++++++++++++
 drivers/regulator/devres.c       |  68 +++++++++++++++++++++++++
 include/linux/regulator/driver.h |  51 +++++++++++++++++++
 3 files changed, 225 insertions(+)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 4c1f999..f37a604 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -3607,6 +3607,112 @@ void regulator_unregister(struct regulator_dev *rdev)
 EXPORT_SYMBOL_GPL(regulator_unregister);
 
 /**
+ * regulator_set_register - register a set of regulators
+ * @config: runtime configuration for regulator set
+ *
+ * Called by regulator drivers to register a set of regulators.
+ * Returns a valid pointer to struct regulator_set on success
+ * or an ERR_PTR() on error.
+ */
+struct regulator_set *
+regulator_set_register(const struct regulator_set_config *config)
+{
+	struct regulator_set *rset;
+	int prev_nregistered = -1;
+	int nregistered = 0;
+	int ret;
+	int i;
+
+	if (!config->descs || !config->nregulators)
+		return ERR_PTR(-EINVAL);
+
+	rset = kzalloc(sizeof(*rset) +
+		       (config->nregulators * sizeof(struct regulator_dev *)),
+		       GFP_KERNEL);
+	if (!rset)
+		return ERR_PTR(-ENOMEM);
+
+	rset->nregulators = config->nregulators;
+
+	while (nregistered < config->nregulators &&
+	       prev_nregistered < nregistered) {
+
+		prev_nregistered = nregistered;
+
+		for (i = 0; i < config->nregulators; i++) {
+			struct regulator_config conf;
+			struct regulator_dev *regulator;
+
+			if (rset->regulators[i])
+				continue;
+
+			memset(&conf, 0, sizeof(conf));
+
+			conf.dev = config->dev;
+			conf.regmap = config->regmap;
+			conf.driver_data = config->driver_data;
+			if (config->matches && config->matches[i].init_data)
+				conf.init_data = config->matches[i].init_data;
+			else if (config->init_data)
+				conf.init_data = config->init_data[i];
+
+			if (config->matches)
+				conf.of_node = config->matches[i].of_node;
+
+			regulator = regulator_register(&config->descs[i],
+						       &conf);
+			if (IS_ERR(regulator)) {
+				ret = PTR_ERR(regulator);
+				if (ret == -EPROBE_DEFER)
+					continue;
+
+				goto err;
+			}
+
+			rset->regulators[i] = regulator;
+			nregistered++;
+		}
+	}
+
+	if (nregistered < config->nregulators) {
+		ret = -EPROBE_DEFER;
+		goto err;
+	}
+
+	return rset;
+
+err:
+	for (i = 0; i < config->nregulators; i++) {
+		if (rset->regulators[i])
+			regulator_unregister(rset->regulators[i]);
+	}
+
+	kfree(rset);
+
+	return ERR_PTR(ret);
+}
+EXPORT_SYMBOL_GPL(regulator_set_register);
+
+/**
+ * regulator_set_unregister - unregister regulator set
+ * @rset: regulator set to unregister
+ *
+ * Called by regulator drivers to unregister a regulator set.
+ */
+void regulator_set_unregister(struct regulator_set *rset)
+{
+	int i;
+
+	for (i = 0; i < rset->nregulators; i++) {
+		if (rset->regulators[i])
+			regulator_unregister(rset->regulators[i]);
+	}
+
+	kfree(rset);
+}
+EXPORT_SYMBOL_GPL(regulator_set_unregister);
+
+/**
  * regulator_suspend_prepare - prepare regulators for system wide suspend
  * @state: system suspend state
  *
diff --git a/drivers/regulator/devres.c b/drivers/regulator/devres.c
index 8f785bc..36ac40a 100644
--- a/drivers/regulator/devres.c
+++ b/drivers/regulator/devres.c
@@ -251,6 +251,74 @@ void devm_regulator_unregister(struct device *dev, struct regulator_dev *rdev)
 }
 EXPORT_SYMBOL_GPL(devm_regulator_unregister);
 
+static void devm_rset_release(struct device *dev, void *res)
+{
+	regulator_set_unregister(*(struct regulator_set **)res);
+}
+
+/**
+ * devm_regulator_set_register - Resource managed regulator_set_register()
+ * @dev: device registering the regulator set
+ * @config: runtime configuration for regulator set
+ *
+ * Called by regulator drivers to register a set of regulators.  Returns a
+ * valid pointer to struct regulator_set on success or an ERR_PTR() on
+ * error.  The regulator will automatically be released when the device
+ * is unbound.
+ */
+struct regulator_set *
+devm_regulator_set_register(struct device *dev,
+			    const struct regulator_set_config *config)
+{
+	struct regulator_set **ptr, *rset;
+
+	ptr = devres_alloc(devm_rset_release, sizeof(*ptr),
+			   GFP_KERNEL);
+	if (!ptr)
+		return ERR_PTR(-ENOMEM);
+
+	rset = regulator_set_register(config);
+	if (!IS_ERR(rset)) {
+		*ptr = rset;
+		devres_add(dev, ptr);
+	} else {
+		devres_free(ptr);
+	}
+
+	return rset;
+}
+EXPORT_SYMBOL_GPL(devm_regulator_set_register);
+
+static int devm_rset_match(struct device *dev, void *res, void *data)
+{
+	struct regulator_set **r = res;
+	if (!r || !*r) {
+		WARN_ON(!r || !*r);
+		return 0;
+	}
+	return *r == data;
+}
+
+/**
+ * devm_regulator_set_unregister - Resource managed regulator_set_unregister()
+ * @dev: device unregistering the regulator set
+ * @rset: regulator set to release
+ *
+ * Unregister a regulator set registered with devm_regulator_set_register().
+ * Normally this function will not need to be called and the resource
+ * management code will ensure that the resource is freed.
+ */
+void devm_regulator_set_unregister(struct device *dev,
+				   struct regulator_set *rset)
+{
+	int rc;
+
+	rc = devres_release(dev, devm_rdev_release, devm_rset_match, rset);
+	if (rc != 0)
+		WARN_ON(rc);
+}
+EXPORT_SYMBOL_GPL(devm_regulator_unregister);
+
 struct regulator_supply_alias_match {
 	struct device *dev;
 	const char *id;
diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
index bbe03a1..b683f68 100644
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -350,6 +350,48 @@ struct regulator_dev {
 	unsigned int ena_gpio_state:1;
 };
 
+/**
+ * struct regulator_set_config - Dynamic regulator set descriptor
+ *
+ * This structure describe a regulator and each regulator in this set
+ * will be registered using informations passed through regulator_desc
+ * and of_regulator_match tables.
+ *
+ * @dev: struct device providing the regulator set.
+ * @driver_data: private driver data for this regulator set.
+ * @regmap: regmap to use for core regmap helpers if dev_get_regulator() is
+ *          insufficient.
+ * @descs: table of regulator descriptors.
+ * @matches: table of DT regulator descriptors. This table should have
+ *           been filled by the of_regulator_match function.
+ * @init_data: a table of init_data in case matches is NULL or the init_data
+ *             of the match entry is NULL.
+ * @nregulators: number of regulators in the regulator set.
+ */
+struct regulator_set_config {
+	struct device *dev;
+	void *driver_data;
+	struct regmap *regmap;
+	const struct regulator_desc *descs;
+	const struct of_regulator_match *matches;
+	const struct regulator_init_data **init_data;
+	int nregulators;
+};
+
+/*
+ * struct regulator_set - Regulator set
+ *
+ * This structure stores a set of regulator devices provided by a single
+ * device (e.g. a PMIC device).
+ *
+ * @nregulators: number of regulators in the set
+ * @regulators: regulators table
+ */
+struct regulator_set {
+	int nregulators;
+	struct regulator_dev *regulators[0];
+};
+
 struct regulator_dev *
 regulator_register(const struct regulator_desc *regulator_desc,
 		   const struct regulator_config *config);
@@ -360,6 +402,15 @@ devm_regulator_register(struct device *dev,
 void regulator_unregister(struct regulator_dev *rdev);
 void devm_regulator_unregister(struct device *dev, struct regulator_dev *rdev);
 
+struct regulator_set *
+regulator_set_register(const struct regulator_set_config *config);
+struct regulator_set *
+devm_regulator_set_register(struct device *dev,
+			    const struct regulator_set_config *config);
+void regulator_set_unregister(struct regulator_set *rset);
+void devm_regulator_set_unregister(struct device *dev,
+				   struct regulator_set *rset);
+
 int regulator_notifier_call_chain(struct regulator_dev *rdev,
 				  unsigned long event, void *data);
 
-- 
1.8.3.2

[PATCH v4 6/7] regulator: axp20x: make use of devm_regulator_set_register

[Boris BREZILLON]

Make use of the devm_regulator_set_register instead of registering each
regulator provided by the PMIC.

This also solves a self dependency issue where one regulator of the PMIC
is used as a supply for anoher regulator provided by the same PMIC.

Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
---
 drivers/regulator/axp20x-regulator.c | 59 ++++++++++++++----------------------
 1 file changed, 23 insertions(+), 36 deletions(-)

diff --git a/drivers/regulator/axp20x-regulator.c b/drivers/regulator/axp20x-regulator.c
index d42bf6d..e8f0df7 100644
--- a/drivers/regulator/axp20x-regulator.c
+++ b/drivers/regulator/axp20x-regulator.c
@@ -301,66 +301,53 @@ static int axp20x_set_dcdc_workmode(struct regulator_dev *rdev, int id, u32 work
 
 static int axp20x_regulator_probe(struct platform_device *pdev)
 {
-	struct regulator_dev *rdev;
+	struct regulator_set *rset;
 	struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
-	struct regulator_config config = { };
-	struct regulator_init_data *init_data;
+	struct regulator_set_config config;
 	struct of_regulator_match *matches;
-	int nmatches;
-	const struct regulator_desc *regulators;
-	int nregulators;
 	int ret, i;
 	u32 workmode;
 
+	memset(&config, 0, sizeof(config));
+	config.dev = &pdev->dev;
+	config.regmap = axp20x->regmap;
 	if (axp20x->variant == AXP221_ID) {
 		matches = axp22x_matches;
-		nmatches = ARRAY_SIZE(axp22x_matches);
-		regulators = axp22x_regulators;
-		nregulators = AXP22X_REG_ID_MAX;
+		config.descs = axp22x_regulators;
+		config.nregulators = AXP22X_REG_ID_MAX;
 	} else {
 		matches = axp20x_matches;
-		nmatches = ARRAY_SIZE(axp20x_matches);
-		regulators = axp20x_regulators;
-		nregulators = AXP20X_REG_ID_MAX;
+		config.descs = axp20x_regulators;
+		config.nregulators = AXP20X_REG_ID_MAX;
 	}
+	config.matches = matches;
 
 	/*
 	 * Reset matches table (this table might have been modified by a
 	 * previous AXP2xx device probe).
 	 */
-	for (i = 0; i < nmatches; i++) {
+	for (i = 0; i < config.nregulators; i++) {
 		matches[i].init_data = NULL;
 		matches[i].of_node = NULL;
 	}
 
-	ret = axp20x_regulator_parse_dt(pdev, matches, nmatches);
+	ret = axp20x_regulator_parse_dt(pdev, matches,
+					config.nregulators);
 	if (ret)
 		return ret;
 
-	for (i = 0; i < AXP20X_REG_ID_MAX; i++) {
-		init_data = matches[i].init_data;
+	rset = devm_regulator_set_register(&pdev->dev, &config);
+	if (IS_ERR(rset))
+		return PTR_ERR(rset);
 
-		config.dev = &pdev->dev;
-		config.init_data = init_data;
-		config.regmap = axp20x->regmap;
-		config.of_node = matches[i].of_node;
-
-		rdev = devm_regulator_register(&pdev->dev, &regulators[i],
-					       &config);
-		if (IS_ERR(rdev)) {
-			dev_err(&pdev->dev, "Failed to register %s\n",
-				regulators[i].name);
-
-			return PTR_ERR(rdev);
-		}
-
-		ret = of_property_read_u32(matches[i].of_node, "x-powers,dcdc-workmode",
+	for (i = 0; i < rset->nregulators; i++) {
+		ret = of_property_read_u32(config.matches[i].of_node,
+					   "x-powers,dcdc-workmode",
 					   &workmode);
-		if (!ret) {
-			if (axp20x_set_dcdc_workmode(rdev, i, workmode))
-				dev_err(&pdev->dev, "Failed to set workmode on %s\n",
-					regulators[i].name);
-		}
+		if (!ret &&
+		    axp20x_set_dcdc_workmode(rset->regulators[i], i, workmode))
+			dev_err(&pdev->dev, "Failed to set workmode on %s\n",
+				config.descs[i].name);
 	}
 
 	return 0;
-- 
1.8.3.2

Re: [PATCH v4 6/7] regulator: axp20x: make use of devm_regulator_set_register

[Maxime Ripard]

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

On Tue, Jun 17, 2014 at 09:38:42AM +0200, Boris BREZILLON wrote:
> Make use of the devm_regulator_set_register instead of registering each
> regulator provided by the PMIC.
> 
> This also solves a self dependency issue where one regulator of the PMIC
> is used as a supply for anoher regulator provided by the same PMIC.
> 
> Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
> ---
>  drivers/regulator/axp20x-regulator.c | 59 ++++++++++++++----------------------
>  1 file changed, 23 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/regulator/axp20x-regulator.c b/drivers/regulator/axp20x-regulator.c
> index d42bf6d..e8f0df7 100644
> --- a/drivers/regulator/axp20x-regulator.c
> +++ b/drivers/regulator/axp20x-regulator.c
> @@ -301,66 +301,53 @@ static int axp20x_set_dcdc_workmode(struct regulator_dev *rdev, int id, u32 work
>  
>  static int axp20x_regulator_probe(struct platform_device *pdev)
>  {
> -	struct regulator_dev *rdev;
> +	struct regulator_set *rset;
>  	struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
> -	struct regulator_config config = { };
> -	struct regulator_init_data *init_data;
> +	struct regulator_set_config config;
>  	struct of_regulator_match *matches;
> -	int nmatches;
> -	const struct regulator_desc *regulators;
> -	int nregulators;
>  	int ret, i;
>  	u32 workmode;
>  
> +	memset(&config, 0, sizeof(config));
> +	config.dev = &pdev->dev;
> +	config.regmap = axp20x->regmap;
>  	if (axp20x->variant == AXP221_ID) {
>  		matches = axp22x_matches;
> -		nmatches = ARRAY_SIZE(axp22x_matches);
> -		regulators = axp22x_regulators;
> -		nregulators = AXP22X_REG_ID_MAX;
> +		config.descs = axp22x_regulators;
> +		config.nregulators = AXP22X_REG_ID_MAX;
>  	} else {
>  		matches = axp20x_matches;
> -		nmatches = ARRAY_SIZE(axp20x_matches);
> -		regulators = axp20x_regulators;
> -		nregulators = AXP20X_REG_ID_MAX;
> +		config.descs = axp20x_regulators;
> +		config.nregulators = AXP20X_REG_ID_MAX;
>  	}
> +	config.matches = matches;
>  
>  	/*
>  	 * Reset matches table (this table might have been modified by a
>  	 * previous AXP2xx device probe).
>  	 */
> -	for (i = 0; i < nmatches; i++) {
> +	for (i = 0; i < config.nregulators; i++) {
>  		matches[i].init_data = NULL;
>  		matches[i].of_node = NULL;
>  	}
>  
> -	ret = axp20x_regulator_parse_dt(pdev, matches, nmatches);
> +	ret = axp20x_regulator_parse_dt(pdev, matches,
> +					config.nregulators);
>  	if (ret)
>  		return ret;
>  
> -	for (i = 0; i < AXP20X_REG_ID_MAX; i++) {
> -		init_data = matches[i].init_data;
> +	rset = devm_regulator_set_register(&pdev->dev, &config);
> +	if (IS_ERR(rset))
> +		return PTR_ERR(rset);
>  
> -		config.dev = &pdev->dev;
> -		config.init_data = init_data;
> -		config.regmap = axp20x->regmap;
> -		config.of_node = matches[i].of_node;
> -
> -		rdev = devm_regulator_register(&pdev->dev, &regulators[i],
> -					       &config);
> -		if (IS_ERR(rdev)) {
> -			dev_err(&pdev->dev, "Failed to register %s\n",
> -				regulators[i].name);
> -
> -			return PTR_ERR(rdev);
> -		}
> -
> -		ret = of_property_read_u32(matches[i].of_node, "x-powers,dcdc-workmode",
> +	for (i = 0; i < rset->nregulators; i++) {
> +		ret = of_property_read_u32(config.matches[i].of_node,
> +					   "x-powers,dcdc-workmode",
>  					   &workmode);
> -		if (!ret) {
> -			if (axp20x_set_dcdc_workmode(rdev, i, workmode))
> -				dev_err(&pdev->dev, "Failed to set workmode on %s\n",
> -					regulators[i].name);
> -		}
> +		if (!ret &&
> +		    axp20x_set_dcdc_workmode(rset->regulators[i], i, workmode))
> +			dev_err(&pdev->dev, "Failed to set workmode on %s\n",
> +				config.descs[i].name);
>  	}
>  
>  	return 0;
> -- 
> 1.8.3.2
> 

It looks like you're modifying code you just introduced. You should
probably move this patch earlier on, so that you don't have to patch
your patch.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

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

Re: [PATCH v4 6/7] regulator: axp20x: make use of devm_regulator_set_register

[Boris BREZILLON]

On 17/06/2014 22:46, Maxime Ripard wrote:
> On Tue, Jun 17, 2014 at 09:38:42AM +0200, Boris BREZILLON wrote:
>> Make use of the devm_regulator_set_register instead of registering each
>> regulator provided by the PMIC.
>>
>> This also solves a self dependency issue where one regulator of the PMIC
>> is used as a supply for anoher regulator provided by the same PMIC.
>>
>> Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
>> ---
>>  drivers/regulator/axp20x-regulator.c | 59 ++++++++++++++----------------------
>>  1 file changed, 23 insertions(+), 36 deletions(-)
>>
>> diff --git a/drivers/regulator/axp20x-regulator.c b/drivers/regulator/axp20x-regulator.c
>> index d42bf6d..e8f0df7 100644
>> --- a/drivers/regulator/axp20x-regulator.c
>> +++ b/drivers/regulator/axp20x-regulator.c
>> @@ -301,66 +301,53 @@ static int axp20x_set_dcdc_workmode(struct regulator_dev *rdev, int id, u32 work
>>  
>>  static int axp20x_regulator_probe(struct platform_device *pdev)
>>  {
>> -	struct regulator_dev *rdev;
>> +	struct regulator_set *rset;
>>  	struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
>> -	struct regulator_config config = { };
>> -	struct regulator_init_data *init_data;
>> +	struct regulator_set_config config;
>>  	struct of_regulator_match *matches;
>> -	int nmatches;
>> -	const struct regulator_desc *regulators;
>> -	int nregulators;
>>  	int ret, i;
>>  	u32 workmode;
>>  
>> +	memset(&config, 0, sizeof(config));
>> +	config.dev = &pdev->dev;
>> +	config.regmap = axp20x->regmap;
>>  	if (axp20x->variant == AXP221_ID) {
>>  		matches = axp22x_matches;
>> -		nmatches = ARRAY_SIZE(axp22x_matches);
>> -		regulators = axp22x_regulators;
>> -		nregulators = AXP22X_REG_ID_MAX;
>> +		config.descs = axp22x_regulators;
>> +		config.nregulators = AXP22X_REG_ID_MAX;
>>  	} else {
>>  		matches = axp20x_matches;
>> -		nmatches = ARRAY_SIZE(axp20x_matches);
>> -		regulators = axp20x_regulators;
>> -		nregulators = AXP20X_REG_ID_MAX;
>> +		config.descs = axp20x_regulators;
>> +		config.nregulators = AXP20X_REG_ID_MAX;
>>  	}
>> +	config.matches = matches;
>>  
>>  	/*
>>  	 * Reset matches table (this table might have been modified by a
>>  	 * previous AXP2xx device probe).
>>  	 */
>> -	for (i = 0; i < nmatches; i++) {
>> +	for (i = 0; i < config.nregulators; i++) {
>>  		matches[i].init_data = NULL;
>>  		matches[i].of_node = NULL;
>>  	}
>>  
>> -	ret = axp20x_regulator_parse_dt(pdev, matches, nmatches);
>> +	ret = axp20x_regulator_parse_dt(pdev, matches,
>> +					config.nregulators);
>>  	if (ret)
>>  		return ret;
>>  
>> -	for (i = 0; i < AXP20X_REG_ID_MAX; i++) {
>> -		init_data = matches[i].init_data;
>> +	rset = devm_regulator_set_register(&pdev->dev, &config);
>> +	if (IS_ERR(rset))
>> +		return PTR_ERR(rset);
>>  
>> -		config.dev = &pdev->dev;
>> -		config.init_data = init_data;
>> -		config.regmap = axp20x->regmap;
>> -		config.of_node = matches[i].of_node;
>> -
>> -		rdev = devm_regulator_register(&pdev->dev, &regulators[i],
>> -					       &config);
>> -		if (IS_ERR(rdev)) {
>> -			dev_err(&pdev->dev, "Failed to register %s\n",
>> -				regulators[i].name);
>> -
>> -			return PTR_ERR(rdev);
>> -		}
>> -
>> -		ret = of_property_read_u32(matches[i].of_node, "x-powers,dcdc-workmode",
>> +	for (i = 0; i < rset->nregulators; i++) {
>> +		ret = of_property_read_u32(config.matches[i].of_node,
>> +					   "x-powers,dcdc-workmode",
>>  					   &workmode);
>> -		if (!ret) {
>> -			if (axp20x_set_dcdc_workmode(rdev, i, workmode))
>> -				dev_err(&pdev->dev, "Failed to set workmode on %s\n",
>> -					regulators[i].name);
>> -		}
>> +		if (!ret &&
>> +		    axp20x_set_dcdc_workmode(rset->regulators[i], i, workmode))
>> +			dev_err(&pdev->dev, "Failed to set workmode on %s\n",
>> +				config.descs[i].name);
>>  	}
>>  
>>  	return 0;
>> -- 
>> 1.8.3.2
>>
> It looks like you're modifying code you just introduced. You should
> probably move this patch earlier on, so that you don't have to patch
> your patch.

Okay, I'll switch patch 5 and 6.

> Maxime
>

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCH v4 7/7] mfd: AXP20x: add "x-powers,axp221" compatible string to DT bindings doc

[Boris BREZILLON]

Add "x-powers,axp221" compatible string to the AXP20x DT bindings
documentation.

Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
---
 Documentation/devicetree/bindings/mfd/axp20x.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/mfd/axp20x.txt b/Documentation/devicetree/bindings/mfd/axp20x.txt
index cc9e01b..f22a169 100644
--- a/Documentation/devicetree/bindings/mfd/axp20x.txt
+++ b/Documentation/devicetree/bindings/mfd/axp20x.txt
@@ -5,7 +5,7 @@ axp202 (X-Powers)
 axp209 (X-Powers)
 
 Required properties:
-- compatible: "x-powers,axp202" or "x-powers,axp209"
+- compatible: "x-powers,axp202", "x-powers,axp209" or "x-powers,axp221"
 - reg: The I2C slave address for the AXP chip
 - interrupt-parent: The parent interrupt controller
 - interrupts: Interrupt specifiers for interrupt sources
-- 
1.8.3.2