[PATCH resend v2 0/3] mfd-core: Don't register supplies from add_device, add register_supply_aliases()

[PATCH resend v2 0/3] mfd-core: Don't register supplies from add_device, add register_supply_aliases()

[Hans de Goede]

Hi all,

I already send this series before (in time for 3.16) and AFAIK we agreed
on going with this series instead of the incomplete fix for the related oops
which now has been merged as commit d137be00ee017bc40e6027cb66d667a2e0b450fd

I still believe that this series is the more correct fix, as platform
devices must simply not be used at all before they have been added.

This series changes the probe messages for the axp209 pmic regulators from:

[    1.386371] Adding alias for supply acin,(null) -> acin,0-0034
[    1.392205] Adding alias for supply vin2,(null) -> vin2,0-0034
[    1.398112] Adding alias for supply vin3,(null) -> vin3,0-0034
[    1.403995] Adding alias for supply ldo24in,(null) -> ldo24in,0-0034
[    1.410344] Adding alias for supply ldo3in,(null) -> ldo3in,0-0034
[    1.416551] Adding alias for supply ldo5in,(null) -> ldo5in,0-0034

To:

[    1.410545] Adding alias for supply acin,axp20x-regulator -> acin,0-0034
[    1.417288] Adding alias for supply vin2,axp20x-regulator -> vin2,0-0034
[    1.424002] Adding alias for supply vin3,axp20x-regulator -> vin3,0-0034
[    1.430695] Adding alias for supply ldo24in,axp20x-regulator -> ldo24in,0-003
4
[    1.437922] Adding alias for supply ldo3in,axp20x-regulator -> ldo3in,0-0034
[    1.444973] Adding alias for supply ldo5in,axp20x-regulator -> ldo5in,0-0034

Notice how all the "(null)" bits are gone.

No need to rush this into 3.16, but I would still like to see this merged
into 3.17 .

Changes since v1:
-Rebased on top of 3.16-rc1 / commit d137be00ee017bc40e6027cb66d667a2e0b450fd

Thanks & Regards,

Hans

[PATCH resend v2 1/3] mfd-core: Don't register supplies from add_device, add register_supply_aliases()

[Hans de Goede]

We cannot register supply alias in mfd_add_device before calling
platform_add_device because platform-dev's name must be set before registering
the aliases which happens from platform_add_device.

So stop registering supply aliases from mfd_add_device, and add a
mfd_register_supply_aliases helper functions for the cell's plaform driver
probe method to use.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/mfd/mfd-core.c   | 37 +++++++++++++++++++++----------------
 include/linux/mfd/core.h |  6 +++++-
 2 files changed, 26 insertions(+), 17 deletions(-)

diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
index 892d343..03f6fee 100644
--- a/drivers/mfd/mfd-core.c
+++ b/drivers/mfd/mfd-core.c
@@ -102,13 +102,6 @@ static int mfd_add_device(struct device *parent, int id,
 	pdev->dev.dma_mask = parent->dma_mask;
 	pdev->dev.dma_parms = parent->dma_parms;
 
-	ret = regulator_bulk_register_supply_alias(
-			&pdev->dev, cell->parent_supplies,
-			parent, cell->parent_supplies,
-			cell->num_parent_supplies);
-	if (ret < 0)
-		goto fail_res;
-
 	if (parent->of_node && cell->of_compatible) {
 		for_each_child_of_node(parent->of_node, np) {
 			if (of_device_is_compatible(np, cell->of_compatible)) {
@@ -122,12 +115,12 @@ static int mfd_add_device(struct device *parent, int id,
 		ret = platform_device_add_data(pdev,
 					cell->platform_data, cell->pdata_size);
 		if (ret)
-			goto fail_alias;
+			goto fail_res;
 	}
 
 	ret = mfd_platform_add_cell(pdev, cell, usage_count);
 	if (ret)
-		goto fail_alias;
+		goto fail_res;
 
 	for (r = 0; r < cell->num_resources; r++) {
 		res[r].name = cell->resources[r].name;
@@ -162,17 +155,17 @@ static int mfd_add_device(struct device *parent, int id,
 		if (!cell->ignore_resource_conflicts) {
 			ret = acpi_check_resource_conflict(&res[r]);
 			if (ret)
-				goto fail_alias;
+				goto fail_res;
 		}
 	}
 
 	ret = platform_device_add_resources(pdev, res, cell->num_resources);
 	if (ret)
-		goto fail_alias;
+		goto fail_res;
 
 	ret = platform_device_add(pdev);
 	if (ret)
-		goto fail_alias;
+		goto fail_res;
 
 	if (cell->pm_runtime_no_callbacks)
 		pm_runtime_no_callbacks(&pdev->dev);
@@ -181,10 +174,6 @@ static int mfd_add_device(struct device *parent, int id,
 
 	return 0;
 
-fail_alias:
-	regulator_bulk_unregister_supply_alias(&pdev->dev,
-					       cell->parent_supplies,
-					       cell->num_parent_supplies);
 fail_res:
 	kfree(res);
 fail_device:
@@ -289,5 +278,21 @@ int mfd_clone_cell(const char *cell, const char **clones, size_t n_clones)
 }
 EXPORT_SYMBOL(mfd_clone_cell);
 
+int mfd_register_supply_aliases(struct platform_device *pdev)
+{
+	const struct mfd_cell *cell;
+
+	if (pdev->dev.type != &mfd_dev_type)
+		return -EINVAL;
+
+	cell = mfd_get_cell(pdev);
+
+	return devm_regulator_bulk_register_supply_alias(
+			&pdev->dev, cell->parent_supplies,
+			pdev->dev.parent, cell->parent_supplies,
+			cell->num_parent_supplies);
+}
+EXPORT_SYMBOL(mfd_register_supply_aliases);
+
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Ian Molton, Dmitry Baryshkov");
diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
index f543de9..f05488e 100644
--- a/include/linux/mfd/core.h
+++ b/include/linux/mfd/core.h
@@ -61,7 +61,9 @@ struct mfd_cell {
 	bool			pm_runtime_no_callbacks;
 
 	/* A list of regulator supplies that should be mapped to the MFD
-	 * device rather than the child device when requested
+	 * device rather than the child device when requested.
+	 * Drivers using this must call mfd_register_supply_aliases()
+	 * from their probe method.
 	 */
 	const char * const	*parent_supplies;
 	int			num_parent_supplies;
@@ -110,4 +112,6 @@ extern int mfd_add_devices(struct device *parent, int id,
 
 extern void mfd_remove_devices(struct device *parent);
 
+extern int mfd_register_supply_aliases(struct platform_device *pdev);
+
 #endif
-- 
2.0.0

[PATCH resend v2 2/3] regulator/axp20x: Call mfd_register_supply_aliases

[Hans de Goede]

The mfd-core no longer registers the supply aliases, instead the platform
driver probe method must now call mfd_register_supply_aliases().

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/regulator/axp20x-regulator.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/regulator/axp20x-regulator.c b/drivers/regulator/axp20x-regulator.c
index 004aadb..0e5ff8a 100644
--- a/drivers/regulator/axp20x-regulator.c
+++ b/drivers/regulator/axp20x-regulator.c
@@ -21,6 +21,7 @@
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
 #include <linux/mfd/axp20x.h>
+#include <linux/mfd/core.h>
 #include <linux/regulator/driver.h>
 #include <linux/regulator/of_regulator.h>
 
@@ -238,6 +239,10 @@ static int axp20x_regulator_probe(struct platform_device *pdev)
 	int ret, i;
 	u32 workmode;
 
+	ret = mfd_register_supply_aliases(pdev);
+	if (ret)
+		return ret;
+
 	ret = axp20x_regulator_parse_dt(pdev);
 	if (ret)
 		return ret;
-- 
2.0.0

[PATCH resend v2 3/3] arizona-mfd-codecs: Add mfd_register_supply_aliases() calls

[Hans de Goede]

The mfd-core no longer registers the supply aliases, instead the platform
driver probe method must now call mfd_register_supply_aliases().

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 sound/soc/codecs/wm5102.c | 5 +++++
 sound/soc/codecs/wm5110.c | 5 +++++
 sound/soc/codecs/wm8997.c | 7 ++++++-
 3 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/sound/soc/codecs/wm5102.c b/sound/soc/codecs/wm5102.c
index 289b64d..82249c4 100644
--- a/sound/soc/codecs/wm5102.c
+++ b/sound/soc/codecs/wm5102.c
@@ -26,6 +26,7 @@
 #include <sound/initval.h>
 #include <sound/tlv.h>
 
+#include <linux/mfd/core.h>
 #include <linux/mfd/arizona/core.h>
 #include <linux/mfd/arizona/registers.h>
 
@@ -1829,6 +1830,10 @@ static int wm5102_probe(struct platform_device *pdev)
 	struct wm5102_priv *wm5102;
 	int i, ret;
 
+	ret = mfd_register_supply_aliases(pdev);
+	if (ret)
+		return ret;
+
 	wm5102 = devm_kzalloc(&pdev->dev, sizeof(struct wm5102_priv),
 			      GFP_KERNEL);
 	if (wm5102 == NULL)
diff --git a/sound/soc/codecs/wm5110.c b/sound/soc/codecs/wm5110.c
index 2e5fcb5..786a64a 100644
--- a/sound/soc/codecs/wm5110.c
+++ b/sound/soc/codecs/wm5110.c
@@ -26,6 +26,7 @@
 #include <sound/initval.h>
 #include <sound/tlv.h>
 
+#include <linux/mfd/core.h>
 #include <linux/mfd/arizona/core.h>
 #include <linux/mfd/arizona/registers.h>
 
@@ -1665,6 +1666,10 @@ static int wm5110_probe(struct platform_device *pdev)
 	struct wm5110_priv *wm5110;
 	int i, ret;
 
+	ret = mfd_register_supply_aliases(pdev);
+	if (ret)
+		return ret;
+
 	wm5110 = devm_kzalloc(&pdev->dev, sizeof(struct wm5110_priv),
 			      GFP_KERNEL);
 	if (wm5110 == NULL)
diff --git a/sound/soc/codecs/wm8997.c b/sound/soc/codecs/wm8997.c
index bb9b47b..e7c9c4e 100644
--- a/sound/soc/codecs/wm8997.c
+++ b/sound/soc/codecs/wm8997.c
@@ -26,6 +26,7 @@
 #include <sound/initval.h>
 #include <sound/tlv.h>
 
+#include <linux/mfd/core.h>
 #include <linux/mfd/arizona/core.h>
 #include <linux/mfd/arizona/registers.h>
 
@@ -1110,7 +1111,11 @@ static int wm8997_probe(struct platform_device *pdev)
 {
 	struct arizona *arizona = dev_get_drvdata(pdev->dev.parent);
 	struct wm8997_priv *wm8997;
-	int i;
+	int i, ret;
+
+	ret = mfd_register_supply_aliases(pdev);
+	if (ret)
+		return ret;
 
 	wm8997 = devm_kzalloc(&pdev->dev, sizeof(struct wm8997_priv),
 			      GFP_KERNEL);
-- 
2.0.0

Re: [PATCH resend v2 0/3] mfd-core: Don't register supplies from add_device, add register_supply_aliases()

[Lee Jones]

> I already send this series before (in time for 3.16) and AFAIK we agreed
> on going with this series instead of the incomplete fix for the related oops
> which now has been merged as commit d137be00ee017bc40e6027cb66d667a2e0b450fd
> 
> I still believe that this series is the more correct fix, as platform
> devices must simply not be used at all before they have been added.

We were waiting for Mark's comments.  Which we still have to do.

As they never appeared the temporary fix went in instead.

Moving forward, should that fix be reverted, or it is okay just to
apply this set on top?

> This series changes the probe messages for the axp209 pmic regulators from:
> 
> [    1.386371] Adding alias for supply acin,(null) -> acin,0-0034
> [    1.392205] Adding alias for supply vin2,(null) -> vin2,0-0034
> [    1.398112] Adding alias for supply vin3,(null) -> vin3,0-0034
> [    1.403995] Adding alias for supply ldo24in,(null) -> ldo24in,0-0034
> [    1.410344] Adding alias for supply ldo3in,(null) -> ldo3in,0-0034
> [    1.416551] Adding alias for supply ldo5in,(null) -> ldo5in,0-0034
> 
> To:
> 
> [    1.410545] Adding alias for supply acin,axp20x-regulator -> acin,0-0034
> [    1.417288] Adding alias for supply vin2,axp20x-regulator -> vin2,0-0034
> [    1.424002] Adding alias for supply vin3,axp20x-regulator -> vin3,0-0034
> [    1.430695] Adding alias for supply ldo24in,axp20x-regulator -> ldo24in,0-003
> 4
> [    1.437922] Adding alias for supply ldo3in,axp20x-regulator -> ldo3in,0-0034
> [    1.444973] Adding alias for supply ldo5in,axp20x-regulator -> ldo5in,0-0034
> 
> Notice how all the "(null)" bits are gone.
> 
> No need to rush this into 3.16, but I would still like to see this merged
> into 3.17 .
> 
> Changes since v1:
> -Rebased on top of 3.16-rc1 / commit d137be00ee017bc40e6027cb66d667a2e0b450fd
> 
> Thanks & Regards,
> 
> Hans

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

Re: [PATCH resend v2 0/3] mfd-core: Don't register supplies from add_device, add register_supply_aliases()

[Hans de Goede]

Hi,

On 07/01/2014 11:05 AM, Lee Jones wrote:
>> I already send this series before (in time for 3.16) and AFAIK we agreed
>> on going with this series instead of the incomplete fix for the related oops
>> which now has been merged as commit d137be00ee017bc40e6027cb66d667a2e0b450fd
>>
>> I still believe that this series is the more correct fix, as platform
>> devices must simply not be used at all before they have been added.
> 
> We were waiting for Mark's comments.  Which we still have to do.
> 
> As they never appeared the temporary fix went in instead.
> 
> Moving forward, should that fix be reverted, or it is okay just to
> apply this set on top?

The resend has been rebased on top of the fix, so it should be applied
on top.

Regards,

Hans

Re: [PATCH resend v2 1/3] mfd-core: Don't register supplies from add_device, add register_supply_aliases()

[Lee Jones]

On Mon, 30 Jun 2014, Hans de Goede wrote:

> We cannot register supply alias in mfd_add_device before calling
> platform_add_device because platform-dev's name must be set before registering
> the aliases which happens from platform_add_device.
> 
> So stop registering supply aliases from mfd_add_device, and add a
> mfd_register_supply_aliases helper functions for the cell's plaform driver
> probe method to use.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Didn't I already Ack this?

> ---
>  drivers/mfd/mfd-core.c   | 37 +++++++++++++++++++++----------------
>  include/linux/mfd/core.h |  6 +++++-
>  2 files changed, 26 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
> index 892d343..03f6fee 100644
> --- a/drivers/mfd/mfd-core.c
> +++ b/drivers/mfd/mfd-core.c
> @@ -102,13 +102,6 @@ static int mfd_add_device(struct device *parent, int id,
>  	pdev->dev.dma_mask = parent->dma_mask;
>  	pdev->dev.dma_parms = parent->dma_parms;
>  
> -	ret = regulator_bulk_register_supply_alias(
> -			&pdev->dev, cell->parent_supplies,
> -			parent, cell->parent_supplies,
> -			cell->num_parent_supplies);
> -	if (ret < 0)
> -		goto fail_res;
> -
>  	if (parent->of_node && cell->of_compatible) {
>  		for_each_child_of_node(parent->of_node, np) {
>  			if (of_device_is_compatible(np, cell->of_compatible)) {
> @@ -122,12 +115,12 @@ static int mfd_add_device(struct device *parent, int id,
>  		ret = platform_device_add_data(pdev,
>  					cell->platform_data, cell->pdata_size);
>  		if (ret)
> -			goto fail_alias;
> +			goto fail_res;
>  	}
>  
>  	ret = mfd_platform_add_cell(pdev, cell, usage_count);
>  	if (ret)
> -		goto fail_alias;
> +		goto fail_res;
>  
>  	for (r = 0; r < cell->num_resources; r++) {
>  		res[r].name = cell->resources[r].name;
> @@ -162,17 +155,17 @@ static int mfd_add_device(struct device *parent, int id,
>  		if (!cell->ignore_resource_conflicts) {
>  			ret = acpi_check_resource_conflict(&res[r]);
>  			if (ret)
> -				goto fail_alias;
> +				goto fail_res;
>  		}
>  	}
>  
>  	ret = platform_device_add_resources(pdev, res, cell->num_resources);
>  	if (ret)
> -		goto fail_alias;
> +		goto fail_res;
>  
>  	ret = platform_device_add(pdev);
>  	if (ret)
> -		goto fail_alias;
> +		goto fail_res;
>  
>  	if (cell->pm_runtime_no_callbacks)
>  		pm_runtime_no_callbacks(&pdev->dev);
> @@ -181,10 +174,6 @@ static int mfd_add_device(struct device *parent, int id,
>  
>  	return 0;
>  
> -fail_alias:
> -	regulator_bulk_unregister_supply_alias(&pdev->dev,
> -					       cell->parent_supplies,
> -					       cell->num_parent_supplies);
>  fail_res:
>  	kfree(res);
>  fail_device:
> @@ -289,5 +278,21 @@ int mfd_clone_cell(const char *cell, const char **clones, size_t n_clones)
>  }
>  EXPORT_SYMBOL(mfd_clone_cell);
>  
> +int mfd_register_supply_aliases(struct platform_device *pdev)
> +{
> +	const struct mfd_cell *cell;
> +
> +	if (pdev->dev.type != &mfd_dev_type)
> +		return -EINVAL;
> +
> +	cell = mfd_get_cell(pdev);
> +
> +	return devm_regulator_bulk_register_supply_alias(
> +			&pdev->dev, cell->parent_supplies,
> +			pdev->dev.parent, cell->parent_supplies,
> +			cell->num_parent_supplies);
> +}
> +EXPORT_SYMBOL(mfd_register_supply_aliases);
> +
>  MODULE_LICENSE("GPL");
>  MODULE_AUTHOR("Ian Molton, Dmitry Baryshkov");
> diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
> index f543de9..f05488e 100644
> --- a/include/linux/mfd/core.h
> +++ b/include/linux/mfd/core.h
> @@ -61,7 +61,9 @@ struct mfd_cell {
>  	bool			pm_runtime_no_callbacks;
>  
>  	/* A list of regulator supplies that should be mapped to the MFD
> -	 * device rather than the child device when requested
> +	 * device rather than the child device when requested.
> +	 * Drivers using this must call mfd_register_supply_aliases()
> +	 * from their probe method.
>  	 */
>  	const char * const	*parent_supplies;
>  	int			num_parent_supplies;
> @@ -110,4 +112,6 @@ extern int mfd_add_devices(struct device *parent, int id,
>  
>  extern void mfd_remove_devices(struct device *parent);
>  
> +extern int mfd_register_supply_aliases(struct platform_device *pdev);
> +
>  #endif

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

Re: [PATCH resend v2 1/3] mfd-core: Don't register supplies from add_device, add register_supply_aliases()

[Hans de Goede]

Hi,

On 07/01/2014 11:22 AM, Lee Jones wrote:
> On Mon, 30 Jun 2014, Hans de Goede wrote:
> 
>> We cannot register supply alias in mfd_add_device before calling
>> platform_add_device because platform-dev's name must be set before registering
>> the aliases which happens from platform_add_device.
>>
>> So stop registering supply aliases from mfd_add_device, and add a
>> mfd_register_supply_aliases helper functions for the cell's plaform driver
>> probe method to use.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> 
> Didn't I already Ack this?

Ah yes you did, but back then I expected the series to get picked up
immediately, so I did not add the ack to my local copy of it.

Regards,

Hans

Re: [PATCH resend v2 1/3] mfd-core: Don't register supplies from add_device, add register_supply_aliases()

[Lee Jones]

On Tue, 01 Jul 2014, Hans de Goede wrote:
> On 07/01/2014 11:22 AM, Lee Jones wrote:
> > On Mon, 30 Jun 2014, Hans de Goede wrote:
> > 
> >> We cannot register supply alias in mfd_add_device before calling
> >> platform_add_device because platform-dev's name must be set before registering
> >> the aliases which happens from platform_add_device.
> >>
> >> So stop registering supply aliases from mfd_add_device, and add a
> >> mfd_register_supply_aliases helper functions for the cell's plaform driver
> >> probe method to use.
> >>
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > 
> > Didn't I already Ack this?
> 
> Ah yes you did, but back then I expected the series to get picked up
> immediately, so I did not add the ack to my local copy of it.

When resubmitting, it's always a good idea to look at previous review
in order to search for review comments to fix, or Acks to apply.
Unfortunately the set couldn't go in without Mark's review - which is
still the case.

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