Re: [PATCH v1] clocksource/drivers/timer-atmel-pit: Fix init failure cleanup

[Alexandre Belloni <alexandre.belloni@bootlin.com>]

Hello,

On 02/07/2026 17:12:48-0400, 최유호 wrote:
> Hi,
> 
> Just a gentle ping on this patch.
> 
> I would appreciate any feedback when you have a chance to review this.
> 
> Thanks
> 
> On Mon, 8 Jun 2026 at 14:20, Yuho Choi <dbgh9129@gmail.com> wrote:
> >
> > After clk_prepare_enable(data->mck) succeeds, at91sam926x_pit_dt_init()
> > can still fail while parsing the IRQ, registering the clocksource, or
> > requesting the IRQ. These paths only free the driver data, leaving the
> > master clock enabled.
> >
> > Unwind each initialized state on failure. Stop the PIT after it has been
> > started, dispose the IRQ mapping after it has been created, disable and
> > put the master clock, and unmap the registers before freeing the driver
> > data.
> >

This is a bit moot because if anything in the probe fails, then he
platform will probably not boot at all.

> > Fixes: 699e36e5b8e9 ("clocksource/drivers/timer-atmel-pit: Enable mck clock")
> > Signed-off-by: Yuho Choi <dbgh9129@gmail.com>
> > ---
> >  drivers/clocksource/timer-atmel-pit.c | 21 ++++++++++++++++-----
> >  1 file changed, 16 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/clocksource/timer-atmel-pit.c b/drivers/clocksource/timer-atmel-pit.c
> > index 888b06731e54..edd427ab93e6 100644
> > --- a/drivers/clocksource/timer-atmel-pit.c
> > +++ b/drivers/clocksource/timer-atmel-pit.c
> > @@ -12,7 +12,9 @@
> >  #include <linux/clk.h>
> >  #include <linux/clockchips.h>
> >  #include <linux/interrupt.h>
> > +#include <linux/io.h>
> >  #include <linux/irq.h>
> > +#include <linux/irqdomain.h>
> >  #include <linux/kernel.h>
> >  #include <linux/of.h>
> >  #include <linux/of_address.h>
> > @@ -185,13 +187,13 @@ static int __init at91sam926x_pit_dt_init(struct device_node *node)
> >         if (IS_ERR(data->mck)) {
> >                 pr_err("Unable to get mck clk\n");
> >                 ret = PTR_ERR(data->mck);
> > -               goto exit;
> > +               goto exit_iounmap;
> >         }
> >
> >         ret = clk_prepare_enable(data->mck);
> >         if (ret) {
> >                 pr_err("Unable to enable mck\n");
> > -               goto exit;
> > +               goto exit_clk_put;
> >         }


Switching to devm_clk_get_enabled would avoid most of the error
handling. Similarly, devm_ioremap could be used earlier.

> >
> >         /* Get the interrupts property */
> > @@ -199,7 +201,7 @@ static int __init at91sam926x_pit_dt_init(struct device_node *node)
> >         if (!data->irq) {
> >                 pr_err("Unable to get IRQ from DT\n");
> >                 ret = -EINVAL;
> > -               goto exit;
> > +               goto exit_clk_disable;
> >         }
> >
> >         /*
> > @@ -227,7 +229,7 @@ static int __init at91sam926x_pit_dt_init(struct device_node *node)
> >         ret = clocksource_register_hz(&data->clksrc, pit_rate);
> >         if (ret) {
> >                 pr_err("Failed to register clocksource\n");
> > -               goto exit;
> > +               goto exit_pit_disable;
> >         }
> >
> >         /* Set up irq handler */
> > @@ -237,7 +239,7 @@ static int __init at91sam926x_pit_dt_init(struct device_node *node)
> >         if (ret) {
> >                 pr_err("Unable to setup IRQ\n");
> >                 clocksource_unregister(&data->clksrc);
> > -               goto exit;
> > +               goto exit_pit_disable;
> >         }
> >
> >         /* Set up and register clockevents */
> > @@ -256,6 +258,15 @@ static int __init at91sam926x_pit_dt_init(struct device_node *node)
> >
> >         return 0;
> >
> > +exit_pit_disable:
> > +       pit_write(data->base, AT91_PIT_MR, 0);

I doubt this write is useful.

> > +       irq_dispose_mapping(data->irq);
> > +exit_clk_disable:
> > +       clk_disable_unprepare(data->mck);
> > +exit_clk_put:
> > +       clk_put(data->mck);
> > +exit_iounmap:
> > +       iounmap(data->base);
> >  exit:
> >         kfree(data);
> >         return ret;
> > --
> > 2.43.0
> >

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com