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

[Chen-Yu Tsai]

On Tue, Jul 28, 2026 at 3:42 AM Rosen Penev <rosenp@xxxxxxxxx> 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.

There's no real reason for the code movement. It's going to fail once
or maybe twice at the most. And the ones you moved are the least likely
to fail.

Now I could see replacing irq_of_parse_and_map() with the more standard
platform_get_irq(), but you haven't given a reasonable reason. And it
should be a separate patch since it has nothing to do with streamlining
the error handling and dropping the remove function.

> 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.

These two are what are important.

> Assisted-by: Opencode:Big-Pickle
> Signed-off-by: Rosen Penev <rosenp@xxxxxxxxx>
> ---
>  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..28d8636afad2 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);
>
>         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;
>         }
>
> -       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;
>         }
>
>         /*
> @@ -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(&mbox->controller);

Like Sashiko said, this doesn't compile. All devm_*() functions require
a |struct device *| parameter.


Build testing your patch before sending it is a must.


ChenYu

>  }
>
>  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);
>
> --
> 2.55.0
>