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