Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 16 Jun 2020 10:15:45 +0100
From:   Lee Jones <lee.jones@linaro.org>
To:     Charles Keepax <ckeepax@opensource.cirrus.com>
Cc:     linux-kernel@vger.kernel.org, patches@opensource.cirrus.com
Subject: Re: [PATCH v2 1/2] mfd: mfd-core: Add mechanism for removal of a
 subset of children
Message-ID: <20200616091545.GP2608702@dell>
References: <20200615150722.5249-1-ckeepax@opensource.cirrus.com>
 <20200616075834.GF2608702@dell>
 <20200616084748.GS71940@ediswmail.ad.cirrus.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20200616084748.GS71940@ediswmail.ad.cirrus.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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