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 <[email protected]>
---
Following on from our discussions here: │··················
│··················
https://lore.kernel.org/lkml/[email protected]/#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
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 <[email protected]>
> ---
>
> Following on from our discussions here: │··················
> │··················
> https://lore.kernel.org/lkml/[email protected]/#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
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
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
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
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
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