[PATCH v2 1/2] mfd: mfd-core: Add mechanism for removal of a subset of children

[PATCH v2 1/2] mfd: mfd-core: Add mechanism for removal of a subset of children

[Charles Keepax]

Currently, the only way to remove MFD children is with a call to
mfd_remove_devices, which will remove all the children. Under
some circumstances it is useful to remove only a subset of the
child devices. For example if some additional clean up is required
between removal of certain child devices.

To accomplish this a tag field is added to mfd_cell, and a
corresponding mfd_remove_devices_by_tag function is added to
remove children with a specific tag. This allows a good amount of
flexibility in which child devices a driver wishes to remove, as a
driver could target specific drivers or a large group. Allowing other
use-cases such as removing drivers for functionality that is no longer
required.

Some investigation was done of using the mfd_cell name and id fields,
but it is hard to achieve a good level of flexibility there, at least
without significant complexity. Since the id gets modified by the core
and can even by automatically generated. Using the name alone would
work for my usecase but it is not hard to imagine a situation where it
wouldn't.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---

Following on from our discussions here:                                                                                                                                                                                                        │··················
                                                                                                                                                                                                                                               │··················
https://lore.kernel.org/lkml/20200122110842.10702-2-ckeepax@opensource.cirrus.com/#t                                                                                                                                                           │··················
                                                                                                                                                                                                                                               │··················
Happy to discuss other approaches as well, but this one was quite                                                                                                                                                                              │··················
appealing as it was very simple but affords quite a lot of flexibility.                                                                                                                                                                        │··················


Changes since v1:
 - Use a pointer to pass the tag to mfd_remove_devices_fn

Thanks,
Charles

 drivers/mfd/mfd-core.c   | 10 ++++++++++
 include/linux/mfd/core.h |  2 ++
 2 files changed, 12 insertions(+)

diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
index f5a73af60dd40..5cfff376051e1 100644
--- a/drivers/mfd/mfd-core.c
+++ b/drivers/mfd/mfd-core.c
@@ -287,6 +287,7 @@ static int mfd_remove_devices_fn(struct device *dev, void *data)
 {
 	struct platform_device *pdev;
 	const struct mfd_cell *cell;
+	int *tag = data;
 
 	if (dev->type != &mfd_dev_type)
 		return 0;
@@ -294,6 +295,9 @@ static int mfd_remove_devices_fn(struct device *dev, void *data)
 	pdev = to_platform_device(dev);
 	cell = mfd_get_cell(pdev);
 
+	if (tag && cell->tag != *tag)
+		return 0;
+
 	regulator_bulk_unregister_supply_alias(dev, cell->parent_supplies,
 					       cell->num_parent_supplies);
 
@@ -301,6 +305,12 @@ static int mfd_remove_devices_fn(struct device *dev, void *data)
 	return 0;
 }
 
+void mfd_remove_devices_by_tag(struct device *parent, int tag)
+{
+	device_for_each_child_reverse(parent, &tag, mfd_remove_devices_fn);
+}
+EXPORT_SYMBOL(mfd_remove_devices_by_tag);
+
 void mfd_remove_devices(struct device *parent)
 {
 	device_for_each_child_reverse(parent, NULL, mfd_remove_devices_fn);
diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
index ab76cdd061993..47d1b257461ab 100644
--- a/include/linux/mfd/core.h
+++ b/include/linux/mfd/core.h
@@ -58,6 +58,7 @@ struct mfd_cell_acpi_match {
 struct mfd_cell {
 	const char		*name;
 	int			id;
+	int			tag;
 
 	int			(*enable)(struct platform_device *dev);
 	int			(*disable)(struct platform_device *dev);
@@ -135,6 +136,7 @@ static inline int mfd_add_hotplug_devices(struct device *parent,
 }
 
 extern void mfd_remove_devices(struct device *parent);
+extern void mfd_remove_devices_by_tag(struct device *parent, int tag);
 
 extern int devm_mfd_add_devices(struct device *dev, int id,
 				const struct mfd_cell *cells, int n_devs,
-- 
2.11.0

[PATCH v2 2/2] mfd: madera: Improve handling of regulator unbinding

[Charles Keepax]

The current unbinding process for Madera has some issues. The trouble
is runtime PM is disabled as the first step of the process, but
some of the drivers release IRQs causing regmap IRQ to issue a
runtime get which fails. To allow runtime PM to remain enabled during
mfd_remove_devices, the DCVDD regulator must remain available. In
the case of external DCVDD's this is simple, the regulator can simply
be disabled/put after the call to mfd_remove_devices. However, in
the case of an internally supplied DCVDD the regulator needs to be
released after the other MFD children depending on it.

Use the new MFD mfd_remove_devices_by_tag functionality to split
the child drivers into two groups. An "optional" group of drivers
and a core group that provide functionality to the others. Remove
the optional group first, clean up the regulators etc. then finally
remove the remaining children.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---

No changes since v1.

Thanks,
Charles

 drivers/mfd/madera-core.c | 73 +++++++++++++++++++++++++++++++++++++----------
 1 file changed, 58 insertions(+), 15 deletions(-)

diff --git a/drivers/mfd/madera-core.c b/drivers/mfd/madera-core.c
index 4724c1a01a39f..30a0c2bd32087 100644
--- a/drivers/mfd/madera-core.c
+++ b/drivers/mfd/madera-core.c
@@ -38,6 +38,8 @@
 #define MADERA_RESET_MIN_US	2000
 #define MADERA_RESET_MAX_US	3000
 
+#define MADERA_OPTIONAL_DRIVER	1
+
 static const char * const madera_core_supplies[] = {
 	"AVDD",
 	"DBVDD1",
@@ -56,14 +58,19 @@ static const char * const cs47l15_supplies[] = {
 static const struct mfd_cell cs47l15_devs[] = {
 	{ .name = "madera-pinctrl", },
 	{ .name = "madera-irq", },
-	{ .name = "madera-gpio", },
+	{
+		.name = "madera-gpio",
+		.tag = MADERA_OPTIONAL_DRIVER,
+	},
 	{
 		.name = "madera-extcon",
+		.tag = MADERA_OPTIONAL_DRIVER,
 		.parent_supplies = cs47l15_supplies,
 		.num_parent_supplies = 1, /* We only need MICVDD */
 	},
 	{
 		.name = "cs47l15-codec",
+		.tag = MADERA_OPTIONAL_DRIVER,
 		.parent_supplies = cs47l15_supplies,
 		.num_parent_supplies = ARRAY_SIZE(cs47l15_supplies),
 	},
@@ -80,15 +87,23 @@ static const char * const cs47l35_supplies[] = {
 static const struct mfd_cell cs47l35_devs[] = {
 	{ .name = "madera-pinctrl", },
 	{ .name = "madera-irq", },
-	{ .name = "madera-micsupp", },
-	{ .name = "madera-gpio", },
+	{
+		.name = "madera-micsupp",
+		.tag = MADERA_OPTIONAL_DRIVER,
+	},
+	{
+		.name = "madera-gpio",
+		.tag = MADERA_OPTIONAL_DRIVER,
+	},
 	{
 		.name = "madera-extcon",
+		.tag = MADERA_OPTIONAL_DRIVER,
 		.parent_supplies = cs47l35_supplies,
 		.num_parent_supplies = 1, /* We only need MICVDD */
 	},
 	{
 		.name = "cs47l35-codec",
+		.tag = MADERA_OPTIONAL_DRIVER,
 		.parent_supplies = cs47l35_supplies,
 		.num_parent_supplies = ARRAY_SIZE(cs47l35_supplies),
 	},
@@ -108,15 +123,23 @@ static const char * const cs47l85_supplies[] = {
 static const struct mfd_cell cs47l85_devs[] = {
 	{ .name = "madera-pinctrl", },
 	{ .name = "madera-irq", },
-	{ .name = "madera-micsupp", },
-	{ .name = "madera-gpio", },
+	{
+		.name = "madera-micsupp",
+		.tag = MADERA_OPTIONAL_DRIVER,
+	},
+	{
+		.name = "madera-gpio",
+		.tag = MADERA_OPTIONAL_DRIVER,
+	},
 	{
 		.name = "madera-extcon",
+		.tag = MADERA_OPTIONAL_DRIVER,
 		.parent_supplies = cs47l85_supplies,
 		.num_parent_supplies = 1, /* We only need MICVDD */
 	},
 	{
 		.name = "cs47l85-codec",
+		.tag = MADERA_OPTIONAL_DRIVER,
 		.parent_supplies = cs47l85_supplies,
 		.num_parent_supplies = ARRAY_SIZE(cs47l85_supplies),
 	},
@@ -134,15 +157,23 @@ static const char * const cs47l90_supplies[] = {
 static const struct mfd_cell cs47l90_devs[] = {
 	{ .name = "madera-pinctrl", },
 	{ .name = "madera-irq", },
-	{ .name = "madera-micsupp", },
-	{ .name = "madera-gpio", },
+	{
+		.name = "madera-micsupp",
+		.tag = MADERA_OPTIONAL_DRIVER,
+	},
+	{
+		.name = "madera-gpio",
+		.tag = MADERA_OPTIONAL_DRIVER,
+	},
 	{
 		.name = "madera-extcon",
+		.tag = MADERA_OPTIONAL_DRIVER,
 		.parent_supplies = cs47l90_supplies,
 		.num_parent_supplies = 1, /* We only need MICVDD */
 	},
 	{
 		.name = "cs47l90-codec",
+		.tag = MADERA_OPTIONAL_DRIVER,
 		.parent_supplies = cs47l90_supplies,
 		.num_parent_supplies = ARRAY_SIZE(cs47l90_supplies),
 	},
@@ -157,15 +188,23 @@ static const char * const cs47l92_supplies[] = {
 static const struct mfd_cell cs47l92_devs[] = {
 	{ .name = "madera-pinctrl", },
 	{ .name = "madera-irq", },
-	{ .name = "madera-micsupp", },
-	{ .name = "madera-gpio", },
+	{
+		.name = "madera-micsupp",
+		.tag = MADERA_OPTIONAL_DRIVER,
+	},
+	{
+		.name = "madera-gpio",
+		.tag = MADERA_OPTIONAL_DRIVER,
+	},
 	{
 		.name = "madera-extcon",
+		.tag = MADERA_OPTIONAL_DRIVER,
 		.parent_supplies = cs47l92_supplies,
 		.num_parent_supplies = 1, /* We only need MICVDD */
 	},
 	{
 		.name = "cs47l92-codec",
+		.tag = MADERA_OPTIONAL_DRIVER,
 		.parent_supplies = cs47l92_supplies,
 		.num_parent_supplies = ARRAY_SIZE(cs47l92_supplies),
 	},
@@ -743,18 +782,22 @@ int madera_dev_exit(struct madera *madera)
 	/* Prevent any IRQs being serviced while we clean up */
 	disable_irq(madera->irq);
 
-	/*
-	 * DCVDD could be supplied by a child node, we must disable it before
-	 * removing the children, and prevent PM runtime from turning it back on
-	 */
-	pm_runtime_disable(madera->dev);
+	pm_runtime_get_sync(madera->dev);
 
-	clk_disable_unprepare(madera->mclk[MADERA_MCLK2].clk);
+	mfd_remove_devices_by_tag(madera->dev, MADERA_OPTIONAL_DRIVER);
+
+	pm_runtime_disable(madera->dev);
 
 	regulator_disable(madera->dcvdd);
 	regulator_put(madera->dcvdd);
 
 	mfd_remove_devices(madera->dev);
+
+	pm_runtime_set_suspended(madera->dev);
+	pm_runtime_put_noidle(madera->dev);
+
+	clk_disable_unprepare(madera->mclk[MADERA_MCLK2].clk);
+
 	madera_enable_hard_reset(madera);
 
 	regulator_bulk_disable(madera->num_core_supplies,
-- 
2.11.0

Re: [PATCH v2 1/2] mfd: mfd-core: Add mechanism for removal of a subset of children

[Lee Jones]

On Mon, 15 Jun 2020, Charles Keepax wrote:

> Currently, the only way to remove MFD children is with a call to
> mfd_remove_devices, which will remove all the children. Under
> some circumstances it is useful to remove only a subset of the
> child devices. For example if some additional clean up is required
> between removal of certain child devices.
> 
> To accomplish this a tag field is added to mfd_cell, and a
> corresponding mfd_remove_devices_by_tag function is added to
> remove children with a specific tag. This allows a good amount of
> flexibility in which child devices a driver wishes to remove, as a
> driver could target specific drivers or a large group. Allowing other
> use-cases such as removing drivers for functionality that is no longer
> required.
> 
> Some investigation was done of using the mfd_cell name and id fields,
> but it is hard to achieve a good level of flexibility there, at least
> without significant complexity. Since the id gets modified by the core
> and can even by automatically generated. Using the name alone would
> work for my usecase but it is not hard to imagine a situation where it
> wouldn't.
> 
> Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> ---
> 
> Following on from our discussions here:                                                                                                                                                                                                        │··················
>                                                                                                                                                                                                                                                │··················
> https://lore.kernel.org/lkml/20200122110842.10702-2-ckeepax@opensource.cirrus.com/#t                                                                                                                                                           │··················
>                                                                                                                                                                                                                                                │··················
> Happy to discuss other approaches as well, but this one was quite                                                                                                                                                                              │··················
> appealing as it was very simple but affords quite a lot of flexibility.                                                                                                                                                                        │··················

What about this?

diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
index f5a73af60dd40..a06e0332e1e31 100644
--- a/drivers/mfd/mfd-core.c
+++ b/drivers/mfd/mfd-core.c
@@ -283,7 +283,7 @@ int mfd_add_devices(struct device *parent, int id,
 }
 EXPORT_SYMBOL(mfd_add_devices);
 
-static int mfd_remove_devices_fn(struct device *dev, void *data)
+static int mfd_remove_devices_fn(struct device *dev, void *level)
 {
        struct platform_device *pdev;
        const struct mfd_cell *cell;
@@ -294,6 +294,9 @@ static int mfd_remove_devices_fn(struct device *dev, void *data)
        pdev = to_platform_device(dev);
        cell = mfd_get_cell(pdev);
 
+       if (cell->level && (!level || cell->level != *level))
+               return 0;
+
        regulator_bulk_unregister_supply_alias(dev, cell->parent_supplies,
                                               cell->num_parent_supplies);
 
@@ -303,7 +306,11 @@ static int mfd_remove_devices_fn(struct device *dev, void *data)
 
 void mfd_remove_devices(struct device *parent)
 {
+       int level = MFD_DEP_LEVEL_HIGH;
+
        device_for_each_child_reverse(parent, NULL, mfd_remove_devices_fn);
+       device_for_each_child_reverse(parent, &level, mfd_remove_devices_fn);
 }
 EXPORT_SYMBOL(mfd_remove_devices);

No need for special calls from the parent driver in this case.

Just a requirement to set the cell's dependency level.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 1/2] mfd: mfd-core: Add mechanism for removal of a subset of children

[Charles Keepax]

On Tue, Jun 16, 2020 at 08:58:34AM +0100, Lee Jones wrote:
> On Mon, 15 Jun 2020, Charles Keepax wrote:
> > Happy to discuss other approaches as well, but this one was quite                                                                                                                                                                              │··················
> > appealing as it was very simple but affords quite a lot of flexibility.                                                                                                                                                                        │··················
> 
> What about this?
> 
> diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
> index f5a73af60dd40..a06e0332e1e31 100644
> --- a/drivers/mfd/mfd-core.c
> +++ b/drivers/mfd/mfd-core.c
> @@ -283,7 +283,7 @@ int mfd_add_devices(struct device *parent, int id,
>  }
>  EXPORT_SYMBOL(mfd_add_devices);
>  
> -static int mfd_remove_devices_fn(struct device *dev, void *data)
> +static int mfd_remove_devices_fn(struct device *dev, void *level)
>  {
>         struct platform_device *pdev;
>         const struct mfd_cell *cell;
> @@ -294,6 +294,9 @@ static int mfd_remove_devices_fn(struct device *dev, void *data)
>         pdev = to_platform_device(dev);
>         cell = mfd_get_cell(pdev);
>  
> +       if (cell->level && (!level || cell->level != *level))
> +               return 0;
> +
>         regulator_bulk_unregister_supply_alias(dev, cell->parent_supplies,
>                                                cell->num_parent_supplies);
>  
> @@ -303,7 +306,11 @@ static int mfd_remove_devices_fn(struct device *dev, void *data)
>  
>  void mfd_remove_devices(struct device *parent)
>  {
> +       int level = MFD_DEP_LEVEL_HIGH;
> +
>         device_for_each_child_reverse(parent, NULL, mfd_remove_devices_fn);
> +       device_for_each_child_reverse(parent, &level, mfd_remove_devices_fn);
>  }
>  EXPORT_SYMBOL(mfd_remove_devices);
> 
> No need for special calls from the parent driver in this case.
> 
> Just a requirement to set the cell's dependency level.
> 

Apologies if I am missing something here, but this looks like a
pretty challenging interface from the drivers side.  Rather than
just statically setting tag in the mfd_cells and separate calls
to mfd_remove_devices_by_tag, such as:

	mfd_remove_devices_by_tag(madera->dev, MADERA_OPTIONAL_DRIVER);

	pm_runtime_disable(madera->dev);
	regulator_disable(madera->dcvdd);
	regulator_put(madera->dcvdd);

	mfd_remove_devices(madera->dev);

You need to statically set the level but then also iterate through
the children and update the cell level on each subsequent remove,
in my case:

static int arizona_set_mfd_level(struct device *dev, void *data)
{
	struct platform_device *pdev = to_platform_device(dev);
	if (pdev->mfd_cell)
		pdev->mfd_cell->level = MFD_DEP_LEVEL_HIGH;
}
...
	mfd_remove_devices(madera->dev);

	device_for_each_child(madera->dev, NULL, arizona_set_mfd_level);

	pm_runtime_disable(madera->dev);
	regulator_disable(madera->dcvdd);
	regulator_put(madera->dcvdd);

	mfd_remove_devices(madera->dev);

Does this match how you would expect this to be used?

I do have some concerns. The code can't use mfd_get_cell since it
returns a const pointer, although the pointer in platform_device
isn't const so we access that directly, could update mfd_get_cell? We
also don't have access to mfd_dev_type outside of the mfd core so
its hard to check we are actually setting the mfd_cell of actual
MFD children, I guess just checking for mfd_cell being not NULL is
good enough?

Thanks,
Charles

Re: [PATCH v2 1/2] mfd: mfd-core: Add mechanism for removal of a subset of children

[Lee Jones]

On Tue, 16 Jun 2020, Charles Keepax wrote:

> On Tue, Jun 16, 2020 at 08:58:34AM +0100, Lee Jones wrote:
> > On Mon, 15 Jun 2020, Charles Keepax wrote:
> > > Happy to discuss other approaches as well, but this one was quite                                                                                                                                                                              │··················
> > > appealing as it was very simple but affords quite a lot of flexibility.                                                                                                                                                                        │··················
> > 
> > What about this?
> > 
> > diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
> > index f5a73af60dd40..a06e0332e1e31 100644
> > --- a/drivers/mfd/mfd-core.c
> > +++ b/drivers/mfd/mfd-core.c
> > @@ -283,7 +283,7 @@ int mfd_add_devices(struct device *parent, int id,
> >  }
> >  EXPORT_SYMBOL(mfd_add_devices);
> >  
> > -static int mfd_remove_devices_fn(struct device *dev, void *data)
> > +static int mfd_remove_devices_fn(struct device *dev, void *level)
> >  {
> >         struct platform_device *pdev;
> >         const struct mfd_cell *cell;
> > @@ -294,6 +294,9 @@ static int mfd_remove_devices_fn(struct device *dev, void *data)
> >         pdev = to_platform_device(dev);
> >         cell = mfd_get_cell(pdev);
> >  
> > +       if (cell->level && (!level || cell->level != *level))
> > +               return 0;
> > +
> >         regulator_bulk_unregister_supply_alias(dev, cell->parent_supplies,
> >                                                cell->num_parent_supplies);
> >  
> > @@ -303,7 +306,11 @@ static int mfd_remove_devices_fn(struct device *dev, void *data)
> >  
> >  void mfd_remove_devices(struct device *parent)
> >  {
> > +       int level = MFD_DEP_LEVEL_HIGH;
> > +
> >         device_for_each_child_reverse(parent, NULL, mfd_remove_devices_fn);
> > +       device_for_each_child_reverse(parent, &level, mfd_remove_devices_fn);
> >  }
> >  EXPORT_SYMBOL(mfd_remove_devices);
> > 
> > No need for special calls from the parent driver in this case.
> > 
> > Just a requirement to set the cell's dependency level.
> > 
> 
> Apologies if I am missing something here, but this looks like a
> pretty challenging interface from the drivers side.  Rather than
> just statically setting tag in the mfd_cells and separate calls
> to mfd_remove_devices_by_tag, such as:
> 
> 	mfd_remove_devices_by_tag(madera->dev, MADERA_OPTIONAL_DRIVER);
> 
> 	pm_runtime_disable(madera->dev);
> 	regulator_disable(madera->dcvdd);
> 	regulator_put(madera->dcvdd);
> 
> 	mfd_remove_devices(madera->dev);
> 
> You need to statically set the level but then also iterate through
> the children and update the cell level on each subsequent remove,
> in my case:
> 
> static int arizona_set_mfd_level(struct device *dev, void *data)
> {
> 	struct platform_device *pdev = to_platform_device(dev);
> 	if (pdev->mfd_cell)
> 		pdev->mfd_cell->level = MFD_DEP_LEVEL_HIGH;
> }
> ...
> 	mfd_remove_devices(madera->dev);
> 
> 	device_for_each_child(madera->dev, NULL, arizona_set_mfd_level);
> 
> 	pm_runtime_disable(madera->dev);
> 	regulator_disable(madera->dcvdd);
> 	regulator_put(madera->dcvdd);
> 
> 	mfd_remove_devices(madera->dev);
> 
> Does this match how you would expect this to be used?

No, not at all.

> I do have some concerns. The code can't use mfd_get_cell since it
> returns a const pointer, although the pointer in platform_device
> isn't const so we access that directly, could update mfd_get_cell? We
> also don't have access to mfd_dev_type outside of the mfd core so
> its hard to check we are actually setting the mfd_cell of actual
> MFD children, I guess just checking for mfd_cell being not NULL is
> good enough?

Hmmm... looks like I missed the fact that you needed additional
processing between the removal of each batch of devices.  My
implementation simply handles the order in which devices are removed
(a bit like initcall()s).

How about the inclusion of mfd_remove_devices_late(), whereby
mfd_remove_devices() will refuse to remove devices tagged with
MFD_DEP_LEVEL_HIGH.

Not sure why, but I really dislike the idea of device removal by
arbitrary value/tag, as I see it spawning all sorts of weird and
wonderful implications/hacks/abuse.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 1/2] mfd: mfd-core: Add mechanism for removal of a subset of children

[Charles Keepax]

On Tue, Jun 16, 2020 at 10:15:45AM +0100, Lee Jones wrote:
> On Tue, 16 Jun 2020, Charles Keepax wrote:
> > On Tue, Jun 16, 2020 at 08:58:34AM +0100, Lee Jones wrote:
> > > On Mon, 15 Jun 2020, Charles Keepax wrote:
> > Does this match how you would expect this to be used?
> 
> No, not at all.
> 
> > I do have some concerns. The code can't use mfd_get_cell since it
> > returns a const pointer, although the pointer in platform_device
> > isn't const so we access that directly, could update mfd_get_cell? We
> > also don't have access to mfd_dev_type outside of the mfd core so
> > its hard to check we are actually setting the mfd_cell of actual
> > MFD children, I guess just checking for mfd_cell being not NULL is
> > good enough?
> 
> Hmmm... looks like I missed the fact that you needed additional
> processing between the removal of each batch of devices.  My
> implementation simply handles the order in which devices are removed
> (a bit like initcall()s).
> 
> How about the inclusion of mfd_remove_devices_late(), whereby
> mfd_remove_devices() will refuse to remove devices tagged with
> MFD_DEP_LEVEL_HIGH.
> 

Yeah this should work fine for my use-case.

> Not sure why, but I really dislike the idea of device removal by
> arbitrary value/tag, as I see it spawning all sorts of weird and
> wonderful implications/hacks/abuse.
> 

Its definitely a spectrum with flexibility covering more
use-cases but also definitely opening things up to more abuse. If
you are more comfortable with this approach that is fine with me.

Would you like me to have a crack at coding it up this way? Or
did you want to do a patch?

Thanks,
Charles

Re: [PATCH v2 1/2] mfd: mfd-core: Add mechanism for removal of a subset of children

[Lee Jones]

On Tue, 16 Jun 2020, Charles Keepax wrote:

> On Tue, Jun 16, 2020 at 10:15:45AM +0100, Lee Jones wrote:
> > On Tue, 16 Jun 2020, Charles Keepax wrote:
> > > On Tue, Jun 16, 2020 at 08:58:34AM +0100, Lee Jones wrote:
> > > > On Mon, 15 Jun 2020, Charles Keepax wrote:
> > > Does this match how you would expect this to be used?
> > 
> > No, not at all.
> > 
> > > I do have some concerns. The code can't use mfd_get_cell since it
> > > returns a const pointer, although the pointer in platform_device
> > > isn't const so we access that directly, could update mfd_get_cell? We
> > > also don't have access to mfd_dev_type outside of the mfd core so
> > > its hard to check we are actually setting the mfd_cell of actual
> > > MFD children, I guess just checking for mfd_cell being not NULL is
> > > good enough?
> > 
> > Hmmm... looks like I missed the fact that you needed additional
> > processing between the removal of each batch of devices.  My
> > implementation simply handles the order in which devices are removed
> > (a bit like initcall()s).
> > 
> > How about the inclusion of mfd_remove_devices_late(), whereby
> > mfd_remove_devices() will refuse to remove devices tagged with
> > MFD_DEP_LEVEL_HIGH.
> > 
> 
> Yeah this should work fine for my use-case.
> 
> > Not sure why, but I really dislike the idea of device removal by
> > arbitrary value/tag, as I see it spawning all sorts of weird and
> > wonderful implications/hacks/abuse.
> > 
> 
> Its definitely a spectrum with flexibility covering more
> use-cases but also definitely opening things up to more abuse. If
> you are more comfortable with this approach that is fine with me.
> 
> Would you like me to have a crack at coding it up this way? Or
> did you want to do a patch?

Either/or.  I don't want to steal your thunder, but I'm happy to draft
if you are.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 1/2] mfd: mfd-core: Add mechanism for removal of a subset of children

[Charles Keepax]

On Tue, Jun 16, 2020 at 02:22:59PM +0100, Lee Jones wrote:
> On Tue, 16 Jun 2020, Charles Keepax wrote:
> > On Tue, Jun 16, 2020 at 10:15:45AM +0100, Lee Jones wrote:
> > > On Tue, 16 Jun 2020, Charles Keepax wrote:
> > > > On Tue, Jun 16, 2020 at 08:58:34AM +0100, Lee Jones wrote:
> > > > > On Mon, 15 Jun 2020, Charles Keepax wrote:
> > > > Does this match how you would expect this to be used?
> > > 
> > > No, not at all.
> > > 
> > > > I do have some concerns. The code can't use mfd_get_cell since it
> > > > returns a const pointer, although the pointer in platform_device
> > > > isn't const so we access that directly, could update mfd_get_cell? We
> > > > also don't have access to mfd_dev_type outside of the mfd core so
> > > > its hard to check we are actually setting the mfd_cell of actual
> > > > MFD children, I guess just checking for mfd_cell being not NULL is
> > > > good enough?
> > > 
> > > Hmmm... looks like I missed the fact that you needed additional
> > > processing between the removal of each batch of devices.  My
> > > implementation simply handles the order in which devices are removed
> > > (a bit like initcall()s).
> > > 
> > > How about the inclusion of mfd_remove_devices_late(), whereby
> > > mfd_remove_devices() will refuse to remove devices tagged with
> > > MFD_DEP_LEVEL_HIGH.
> > > 
> > 
> > Yeah this should work fine for my use-case.
> > 
> > > Not sure why, but I really dislike the idea of device removal by
> > > arbitrary value/tag, as I see it spawning all sorts of weird and
> > > wonderful implications/hacks/abuse.
> > > 
> > 
> > Its definitely a spectrum with flexibility covering more
> > use-cases but also definitely opening things up to more abuse. If
> > you are more comfortable with this approach that is fine with me.
> > 
> > Would you like me to have a crack at coding it up this way? Or
> > did you want to do a patch?
> 
> Either/or.  I don't want to steal your thunder, but I'm happy to draft
> if you are.
> 

Been having a poke this afternoon as I had some spare time, so
will wing that up and you can take over if I am too far off the
mark :-)

Thanks,
Charles

[PATCH v2 2/2] mfd: madera: Improve handling of regulator unbinding

[Charles Keepax]

The current unbinding process for Madera has some issues. The trouble
is runtime PM is disabled as the first step of the process, but
some of the drivers release IRQs causing regmap IRQ to issue a
runtime get which fails. To allow runtime PM to remain enabled during
mfd_remove_devices, the DCVDD regulator must remain available. In
the case of external DCVDD's this is simple, the regulator can simply
be disabled/put after the call to mfd_remove_devices. However, in
the case of an internally supplied DCVDD the regulator needs to be
released after the other MFD children depending on it.

Use the new MFD mfd_remove_devices_by_tag functionality to split
the child drivers into two groups. An "optional" group of drivers
and a core group that provide functionality to the others. Remove
the optional group first, clean up the regulators etc. then finally
remove the remaining children.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---

Changes since v1:
  - Added MFD by tag thing and removed the regulator parts

Thanks,
Charles

 drivers/mfd/madera-core.c | 73 +++++++++++++++++++++++++++++++++++++----------
 1 file changed, 58 insertions(+), 15 deletions(-)

diff --git a/drivers/mfd/madera-core.c b/drivers/mfd/madera-core.c
index 7e0835cb062b1..f89b8825ebc1f 100644
--- a/drivers/mfd/madera-core.c
+++ b/drivers/mfd/madera-core.c
@@ -38,6 +38,8 @@
 #define MADERA_RESET_MIN_US	2000
 #define MADERA_RESET_MAX_US	3000
 
+#define MADERA_OPTIONAL_DRIVER	1
+
 static const char * const madera_core_supplies[] = {
 	"AVDD",
 	"DBVDD1",
@@ -56,14 +58,19 @@ static const char * const cs47l15_supplies[] = {
 static const struct mfd_cell cs47l15_devs[] = {
 	{ .name = "madera-pinctrl", },
 	{ .name = "madera-irq" },
-	{ .name = "madera-gpio" },
+	{
+		.name = "madera-gpio",
+		.tag = MADERA_OPTIONAL_DRIVER,
+	},
 	{
 		.name = "madera-extcon",
+		.tag = MADERA_OPTIONAL_DRIVER,
 		.parent_supplies = cs47l15_supplies,
 		.num_parent_supplies = 1, /* We only need MICVDD */
 	},
 	{
 		.name = "cs47l15-codec",
+		.tag = MADERA_OPTIONAL_DRIVER,
 		.parent_supplies = cs47l15_supplies,
 		.num_parent_supplies = ARRAY_SIZE(cs47l15_supplies),
 	},
@@ -80,15 +87,23 @@ static const char * const cs47l35_supplies[] = {
 static const struct mfd_cell cs47l35_devs[] = {
 	{ .name = "madera-pinctrl", },
 	{ .name = "madera-irq", },
-	{ .name = "madera-micsupp", },
-	{ .name = "madera-gpio", },
+	{
+		.name = "madera-micsupp",
+		.tag = MADERA_OPTIONAL_DRIVER,
+	},
+	{
+		.name = "madera-gpio",
+		.tag = MADERA_OPTIONAL_DRIVER,
+	},
 	{
 		.name = "madera-extcon",
+		.tag = MADERA_OPTIONAL_DRIVER,
 		.parent_supplies = cs47l35_supplies,
 		.num_parent_supplies = 1, /* We only need MICVDD */
 	},
 	{
 		.name = "cs47l35-codec",
+		.tag = MADERA_OPTIONAL_DRIVER,
 		.parent_supplies = cs47l35_supplies,
 		.num_parent_supplies = ARRAY_SIZE(cs47l35_supplies),
 	},
@@ -108,15 +123,23 @@ static const char * const cs47l85_supplies[] = {
 static const struct mfd_cell cs47l85_devs[] = {
 	{ .name = "madera-pinctrl", },
 	{ .name = "madera-irq", },
-	{ .name = "madera-micsupp" },
-	{ .name = "madera-gpio", },
+	{
+		.name = "madera-micsupp",
+		.tag = MADERA_OPTIONAL_DRIVER,
+	},
+	{
+		.name = "madera-gpio",
+		.tag = MADERA_OPTIONAL_DRIVER,
+	},
 	{
 		.name = "madera-extcon",
+		.tag = MADERA_OPTIONAL_DRIVER,
 		.parent_supplies = cs47l85_supplies,
 		.num_parent_supplies = 1, /* We only need MICVDD */
 	},
 	{
 		.name = "cs47l85-codec",
+		.tag = MADERA_OPTIONAL_DRIVER,
 		.parent_supplies = cs47l85_supplies,
 		.num_parent_supplies = ARRAY_SIZE(cs47l85_supplies),
 	},
@@ -134,15 +157,23 @@ static const char * const cs47l90_supplies[] = {
 static const struct mfd_cell cs47l90_devs[] = {
 	{ .name = "madera-pinctrl", },
 	{ .name = "madera-irq", },
-	{ .name = "madera-micsupp", },
-	{ .name = "madera-gpio", },
+	{
+		.name = "madera-micsupp",
+		.tag = MADERA_OPTIONAL_DRIVER,
+	},
+	{
+		.name = "madera-gpio",
+		.tag = MADERA_OPTIONAL_DRIVER,
+	},
 	{
 		.name = "madera-extcon",
+		.tag = MADERA_OPTIONAL_DRIVER,
 		.parent_supplies = cs47l90_supplies,
 		.num_parent_supplies = 1, /* We only need MICVDD */
 	},
 	{
 		.name = "cs47l90-codec",
+		.tag = MADERA_OPTIONAL_DRIVER,
 		.parent_supplies = cs47l90_supplies,
 		.num_parent_supplies = ARRAY_SIZE(cs47l90_supplies),
 	},
@@ -157,15 +188,23 @@ static const char * const cs47l92_supplies[] = {
 static const struct mfd_cell cs47l92_devs[] = {
 	{ .name = "madera-pinctrl" },
 	{ .name = "madera-irq", },
-	{ .name = "madera-micsupp", },
-	{ .name = "madera-gpio" },
+	{
+		.name = "madera-micsupp",
+		.tag = MADERA_OPTIONAL_DRIVER,
+	},
+	{
+		.name = "madera-gpio",
+		.tag = MADERA_OPTIONAL_DRIVER,
+	},
 	{
 		.name = "madera-extcon",
+		.tag = MADERA_OPTIONAL_DRIVER,
 		.parent_supplies = cs47l92_supplies,
 		.num_parent_supplies = 1, /* We only need MICVDD */
 	},
 	{
 		.name = "cs47l92-codec",
+		.tag = MADERA_OPTIONAL_DRIVER,
 		.parent_supplies = cs47l92_supplies,
 		.num_parent_supplies = ARRAY_SIZE(cs47l92_supplies),
 	},
@@ -743,18 +782,22 @@ int madera_dev_exit(struct madera *madera)
 	/* Prevent any IRQs being serviced while we clean up */
 	disable_irq(madera->irq);
 
-	/*
-	 * DCVDD could be supplied by a child node, we must disable it before
-	 * removing the children, and prevent PM runtime from turning it back on
-	 */
-	pm_runtime_disable(madera->dev);
+	pm_runtime_get_sync(madera->dev);
 
-	clk_disable_unprepare(madera->mclk[MADERA_MCLK2].clk);
+	mfd_remove_devices_by_tag(madera->dev, MADERA_OPTIONAL_DRIVER);
+
+	pm_runtime_disable(madera->dev);
 
 	regulator_disable(madera->dcvdd);
 	regulator_put(madera->dcvdd);
 
 	mfd_remove_devices(madera->dev);
+
+	pm_runtime_set_suspended(madera->dev);
+	pm_runtime_put_noidle(madera->dev);
+
+	clk_disable_unprepare(madera->mclk[MADERA_MCLK2].clk);
+
 	madera_enable_hard_reset(madera);
 
 	regulator_bulk_disable(madera->num_core_supplies,
-- 
2.11.0