Messages in this thread | | | Date | Tue, 16 Jun 2020 08:47:48 +0000 | From | Charles Keepax <> | Subject | Re: [PATCH v2 1/2] mfd: mfd-core: Add mechanism for removal of a subset of children |
| |
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
| |