Re: [PATCH] mfd: wcd934x: Unroll regmap and irqchip on removal

[Bjorn Andersson <andersson@kernel.org>]

On Tue, Mar 10, 2026 at 09:13:16AM +0000, Lee Jones wrote:
> On Mon, 09 Mar 2026, Bjorn Andersson wrote:
> 
> > When the slimbus-up event is handled a new regmap is created, an IRQ
> > chip is registered on this regmap and then the MFD devices are added.
> > 
> > But the regmap is left dangling if either any of those operations are
> > failing or if the slimbus-down event ever comes. Which manifest itself
> > as an error print from debugfs once the next slimbus-up event happens.
> > 
> > Likewise, if for some reason a slimbus-down event would be followed by
> > a slimbus-up event without the MFD being torn down by the slimbus
> > controller inbetween, we're going to have a dangling irq_chip.
> > 
> > Add cleanup of the registered resources on failure and on removal.
> 
> I don't understand.  Why isn't devm_* working here?
> 

devm_ "works", but there are two different ends to the life cycle; the
primary is a call to wcd934x_slim_status() with SLIM_DEVICE_STATUS_DOWN,
which might be followed by another SLIM_DEVICE_STATUS_UP. The other is
the removal of the device while it's still in "up state" (although it
seems this shouldn't happen).

For the latter devres makes some sense - although I prefer to keep
devres to codepaths rooted in probe().

> And if you want to force-remove, why not devm_regmap_del_irq_chip() in
> the error path instead?

Because the primary end of the life cycle for these allocations aren't
the removal of the device, it's the SLIM_DEVICE_STATUS_DOWN event.

The whole purpose with devres is to simplify the code, if we need to
manually handle the resources anyway they are just a cognitive burden.

> If you end up using this, then you could go the
> other way (the preferred method) and use devm_mfd_add_devices().
> 

mfd_add_devices() and mfd_remove_devices() does not follow the device's
life cycle, so that wouldn't be helpful.

Perhaps the slimbus stack can be redesigned so that these resources
follow the device's life cycle? I don't understand it well enough to
say at this point.
I would argue that it would still be better to fix the memory and
debugfs leaks before making such changes anyways.

Regards,
Bjorn

> > Fixes: 6ac7e4d7ad70 ("mfd: wcd934x: Add support to wcd9340/wcd9341 codec")
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
> > ---
> >  drivers/mfd/wcd934x.c | 49 ++++++++++++++++++++++++++++++++++++++-----------
> >  1 file changed, 38 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/mfd/wcd934x.c b/drivers/mfd/wcd934x.c
> > index 3c3080e8c8cf7ecaaa62e255c7e01a850e65e9ad..b03cc91cc3a6a114a34efdb278420ae3dfa016eb 100644
> > --- a/drivers/mfd/wcd934x.c
> > +++ b/drivers/mfd/wcd934x.c
> > @@ -170,29 +170,56 @@ static int wcd934x_slim_status_up(struct slim_device *sdev)
> >  	ret = wcd934x_bring_up(ddata);
> >  	if (ret) {
> >  		dev_err(dev, "Failed to bring up WCD934X: err = %d\n", ret);
> > -		return ret;
> > +		goto err_regmap_exit;
> >  	}
> >  
> > -	ret = devm_regmap_add_irq_chip(dev, ddata->regmap, ddata->irq,
> > -				       IRQF_TRIGGER_HIGH, 0,
> > -				       &wcd934x_regmap_irq_chip,
> > -				       &ddata->irq_data);
> > +	ret = regmap_add_irq_chip(ddata->regmap, ddata->irq,
> > +				  IRQF_TRIGGER_HIGH, 0,
> > +				  &wcd934x_regmap_irq_chip,
> > +				  &ddata->irq_data);
> >  	if (ret) {
> >  		dev_err(dev, "Failed to add IRQ chip: err = %d\n", ret);
> > -		return ret;
> > +		goto err_regmap_exit;
> >  	}
> >  
> >  	ret = mfd_add_devices(dev, PLATFORM_DEVID_AUTO, wcd934x_devices,
> >  			      ARRAY_SIZE(wcd934x_devices), NULL, 0, NULL);
> >  	if (ret) {
> > -		dev_err(dev, "Failed to add child devices: err = %d\n",
> > -			ret);
> > -		return ret;
> > +		dev_err(dev, "Failed to add child devices: err = %d\n", ret);
> > +		goto err_del_irq_chip;
> >  	}
> >  
> > +	return 0;
> > +
> > +err_del_irq_chip:
> > +	regmap_del_irq_chip(ddata->irq, ddata->irq_data);
> > +	ddata->irq_data = NULL;
> > +err_regmap_exit:
> > +	regmap_exit(ddata->regmap);
> > +	ddata->regmap = NULL;
> >  	return ret;
> >  }
> >  
> > +static void wcd934x_slim_status_down(struct slim_device *sdev)
> > +{
> > +	struct device *dev = &sdev->dev;
> > +	struct wcd934x_ddata *ddata;
> > +
> > +	ddata = dev_get_drvdata(dev);
> > +
> > +	mfd_remove_devices(&sdev->dev);
> > +
> > +	if (ddata->irq_data) {
> > +		regmap_del_irq_chip(ddata->irq, ddata->irq_data);
> > +		ddata->irq_data = NULL;
> > +	}
> > +
> > +	if (ddata->regmap) {
> > +		regmap_exit(ddata->regmap);
> > +		ddata->regmap = NULL;
> > +	}
> > +}
> > +
> >  static int wcd934x_slim_status(struct slim_device *sdev,
> >  			       enum slim_device_status status)
> >  {
> > @@ -200,7 +227,7 @@ static int wcd934x_slim_status(struct slim_device *sdev,
> >  	case SLIM_DEVICE_STATUS_UP:
> >  		return wcd934x_slim_status_up(sdev);
> >  	case SLIM_DEVICE_STATUS_DOWN:
> > -		mfd_remove_devices(&sdev->dev);
> > +		wcd934x_slim_status_down(sdev);
> >  		break;
> >  	default:
> >  		return -EINVAL;
> > @@ -276,7 +303,7 @@ static void wcd934x_slim_remove(struct slim_device *sdev)
> >  	struct wcd934x_ddata *ddata = dev_get_drvdata(&sdev->dev);
> >  
> >  	regulator_bulk_disable(WCD934X_MAX_SUPPLY, ddata->supplies);
> > -	mfd_remove_devices(&sdev->dev);
> > +	wcd934x_slim_status_down(sdev);
> >  }
> >  
> >  static const struct slim_device_id wcd934x_slim_id[] = {
> > 
> > ---
> > base-commit: a0ae2a256046c0c5d3778d1a194ff2e171f16e5f
> > change-id: 20260309-wcd934x-unroll-regmap-f9e08834f9d7
> > 
> > Best regards,
> > -- 
> > Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
> > 
> 
> -- 
> Lee Jones [李琼斯]