Re: [PATCHv2] mailbox: sun6i: modernize probe and convert to fully managed

[Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>]

On Fri, 14 Aug 2026 13:55:53 -0700
Rosen Penev <rosenp@gmail.com> wrote:

> Replace irq_of_parse_and_map() with platform_get_irq() and move both
> IRQ and MMIO resource acquisition to the top of probe, before any
> allocations, for early error exit.
> 
> Switch from devm_clk_get() + clk_prepare_enable() to
> devm_clk_get_enabled(), which combines both operations and registers
> devres callbacks for automatic disable/unprepare on unbind. This
> eliminates the manual err_disable_unprepare error path and the
> remove callback entirely.
> 
> Use devm_mbox_controller_register() for devres-managed controller
> registration, and drop the remove callback and platform_set_drvdata()
> which are no longer needed.
> 
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
Drive by review whilst having coffee...

Looks good  - possible follow up suggestion inline + suggestion
to make a minor reorganization to avoid setting
mbox->regs = regs; way later than where it can be set.

> ---
>  v2: fix compilation
>  drivers/mailbox/sun6i-msgbox.c | 62 ++++++++++------------------------
>  1 file changed, 18 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/mailbox/sun6i-msgbox.c b/drivers/mailbox/sun6i-msgbox.c
> index 6ba6920f4645..3b838f6c837a 100644
> --- a/drivers/mailbox/sun6i-msgbox.c
> +++ b/drivers/mailbox/sun6i-msgbox.c
> @@ -198,7 +198,17 @@ static int sun6i_msgbox_probe(struct platform_device *pdev)
>  	struct mbox_chan *chans;
>  	struct reset_control *reset;
>  	struct sun6i_msgbox *mbox;
> +	void __iomem *regs;
>  	int i, ret;
> +	int irq;
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0)
> +		return irq;
> +
> +	regs = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(regs))
> +		return PTR_ERR(regs);

Maybe do this just after mbox is allocated so you can set
mbox->regs if this succeeds.  Where it is now is a long way
from either mbox or regs being acquired.
>  
>  	mbox = devm_kzalloc(dev, sizeof(*mbox), GFP_KERNEL);
>  	if (!mbox)
> @@ -211,24 +221,18 @@ static int sun6i_msgbox_probe(struct platform_device *pdev)
>  	for (i = 0; i < NUM_CHANS; ++i)
>  		chans[i].con_priv = mbox;
>  
> -	mbox->clk = devm_clk_get(dev, NULL);
> +	mbox->clk = devm_clk_get_enabled(dev, NULL);
>  	if (IS_ERR(mbox->clk)) {
>  		ret = PTR_ERR(mbox->clk);
>  		dev_err(dev, "Failed to get clock: %d\n", ret);
>  		return ret;

		return dev_err_probe(dev, PTR_ERR(mbox->clk), "Failed to get clock\n");
>  	}
>  
> -	ret = clk_prepare_enable(mbox->clk);
> -	if (ret) {
> -		dev_err(dev, "Failed to enable clock: %d\n", ret);
> -		return ret;
> -	}
> -
>  	reset = devm_reset_control_get_exclusive(dev, NULL);
>  	if (IS_ERR(reset)) {
>  		ret = PTR_ERR(reset);
>  		dev_err(dev, "Failed to get reset control: %d\n", ret);
> -		goto err_disable_unprepare;
> +		return ret;

		return dev_err_probe(dev, PTR_ERR(reset), "Failed to get reset control\n");

and other similar cases - both more compact and correctly handles deferred probe and
skipping reporting of things like memory allocations failing (as those are very
noisy anyway).

>  	}
>  
>  	/*
> @@ -242,25 +246,17 @@ static int sun6i_msgbox_probe(struct platform_device *pdev)
>  	ret = reset_control_deassert(reset);
>  	if (ret) {
>  		dev_err(dev, "Failed to deassert reset: %d\n", ret);
> -		goto err_disable_unprepare;
> +		return ret;
>  	}
>  
> -	mbox->regs = devm_platform_ioremap_resource(pdev, 0);
> -	if (IS_ERR(mbox->regs)) {
> -		ret = PTR_ERR(mbox->regs);
> -		dev_err(dev, "Failed to map MMIO resource: %d\n", ret);
> -		goto err_disable_unprepare;
> -	}
> +	mbox->regs = regs;
>  
>  	/* Disable all IRQs for this end of the msgbox. */
>  	writel(0, mbox->regs + LOCAL_IRQ_EN_REG);
>  
> -	ret = devm_request_irq(dev, irq_of_parse_and_map(dev->of_node, 0),
> -			       sun6i_msgbox_irq, 0, dev_name(dev), mbox);
> -	if (ret) {
> -		dev_err(dev, "Failed to register IRQ handler: %d\n", ret);
> -		goto err_disable_unprepare;
> -	}
> +	ret = devm_request_irq(dev, irq, sun6i_msgbox_irq, 0, dev_name(dev), mbox);
> +	if (ret)
> +		return ret;
>  
>  	mbox->controller.dev           = dev;
>  	mbox->controller.ops           = &sun6i_msgbox_chan_ops;
> @@ -271,29 +267,8 @@ static int sun6i_msgbox_probe(struct platform_device *pdev)
>  	mbox->controller.txpoll_period = 5;
>  
>  	spin_lock_init(&mbox->lock);
> -	platform_set_drvdata(pdev, mbox);
> -
> -	ret = mbox_controller_register(&mbox->controller);
> -	if (ret) {
> -		dev_err(dev, "Failed to register controller: %d\n", ret);
> -		goto err_disable_unprepare;
> -	}
> -
> -	return 0;
> -
> -err_disable_unprepare:
> -	clk_disable_unprepare(mbox->clk);
> -
> -	return ret;
> -}
> -
> -static void sun6i_msgbox_remove(struct platform_device *pdev)
> -{
> -	struct sun6i_msgbox *mbox = platform_get_drvdata(pdev);
>  
> -	mbox_controller_unregister(&mbox->controller);
> -	/* See the comment in sun6i_msgbox_probe about the reset line. */
> -	clk_disable_unprepare(mbox->clk);
> +	return devm_mbox_controller_register(dev, &mbox->controller);
>  }
>  
>  static const struct of_device_id sun6i_msgbox_of_match[] = {
> @@ -308,7 +283,6 @@ static struct platform_driver sun6i_msgbox_driver = {
>  		.of_match_table = sun6i_msgbox_of_match,
>  	},
>  	.probe = sun6i_msgbox_probe,
> -	.remove = sun6i_msgbox_remove,
>  };
>  module_platform_driver(sun6i_msgbox_driver);
>