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

[Rosen Penev]

On Tue, Jul 28, 2026 at 8:56 AM Chen-Yu Tsai <wens@xxxxxxxxxx> wrote:
>
> 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.
Not my experience. They can and do return -EPROBE_DEFER with some
drivers. No idea about this one in particular.
>
> 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
> >