Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 23 Jan 2020 09:26:39 +0000
From:   Charles Keepax <ckeepax@opensource.cirrus.com>
To:     Mark Brown <broonie@kernel.org>
CC:     <lee.jones@linaro.org>, <lgirdwood@gmail.com>,
        <linux-kernel@vger.kernel.org>, <patches@opensource.cirrus.com>
Subject: Re: [PATCH RESEND 1/2] regulator: arizona-ldo1: Improve handling of
 regulator unbinding
Message-ID: <20200123092639.GC4098@ediswmail.ad.cirrus.com>
References: <20200122110842.10702-1-ckeepax@opensource.cirrus.com>
 <20200122131149.GE3833@sirena.org.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20200122131149.GE3833@sirena.org.uk>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Jan 22, 2020 at 01:11:49PM +0000, Mark Brown wrote:
> On Wed, Jan 22, 2020 at 11:08:41AM +0000, Charles Keepax wrote:
> 
> > The current unbinding process for Madera has some issues. The trouble
> > is runtime PM is disabled as the first step of the process, but
> 
> Why not just leave runtime PM active until all the subdevices are gone?
> This is a really bad hack and it's going to be fragile.
> 

Admittedly I am not super fond of this solution either. But
leaving the PM runtime active is basically what this patch does
(well the mfd part). Leaving the PM runtime enabled means access
to the DCVDD regulator is required during the remove process,
which in turn means you need to put that regulator after the
other devices are removed but before DCVDD is removed. Currently
the only place we can do that is in the LDO remove, as per this
patch.

Other options that might be viable, pending input for yourself
and Lee:

1) We could look at adding a partial remove function to MFD.
Currently I can only call mfd_remove_devices which nobbles all
the devices. If I could make calls to remove specific devices I
could do one call to remove everything except DCVDD, do the put,
then remove the regulator.

2) We could look at adding some sort of pre-remove callback into
MFD, and the regulator put could go in there rather than the
regulator remove, as per this patch. Although this feels a little
like the same thing as this patch, just dressed up a little
differently.

3) We could look at doing something in regmap IRQ to change when
it does PM runtime calls, it is regmap doing runtime gets when
drivers remove IRQs that causes the issue. But my accessment was
that what regmap is doing makes perfect sense, so I don't think
this is a good approach.

> > +static int madera_ldo1_remove(struct platform_device *pdev)
> > +{
> > +	struct madera *madera = dev_get_drvdata(pdev->dev.parent);
> > +
> > +	if (madera->internal_dcvdd) {
> > +		regulator_disable(madera->dcvdd);
> > +		regulator_put(madera->dcvdd);
> > +	}
> 
> This is going to break bisection since it will result in double
> disables, it'd be fine to do the MFD change first since that'd just
> leak a reference to enable on a regulator which is about to be discarded
> entirely anyway but this reordering (and whatever other changes you've
> done since v1) means you add a double free.

Apologies yes that is a good point. If no one minds, and we end
up sticking with this approach, I feel it might best to squash
them both back into one patch?

Thanks,
Charles