Messages in this thread | | | Date | Fri, 13 Oct 2023 20:02:39 +0200 | Subject | Re: [PATCH v6 2/3] clocksource: Add JH7110 timer driver | From | Christophe JAILLET <> |
| |
Le 13/10/2023 à 11:34, Xingyu Wu a écrit : > On 2023/10/13 1:53, Christophe JAILLET wrote: >> Le 12/10/2023 à 10:10, Xingyu Wu a écrit : >>> Add timer driver for the StarFive JH7110 SoC. >>> >>> Signed-off-by: Xingyu Wu <xingyu.wu-bONrM45KWFOXmMXjJBpWqg-XMD5yJDbdMStu3cLTcvVIw@public.gmane.orge.org> >> >> ... > > It looks normal in my email and the web. Is this due to some settings?
Hi,
I use gmane.org and a news reader (Thunderbird). Gmane sometimes (not always!) obfuscate e-mail addresses.
Do not pay attantion to these strange rewritten addresses.
> >> >>> +static int jh7110_timer_probe(struct platform_device *pdev) >>> +{ >>> + struct jh7110_clkevt *clkevt[JH7110_TIMER_CH_MAX]; >>> + char name[4]; >>> + struct clk *pclk; >>> + struct reset_control *rst; >>> + int ch; >>> + int ret; >>> + void __iomem *base; >>> + >>> + base = devm_platform_ioremap_resource(pdev, 0); >>> + if (IS_ERR(base)) >>> + return dev_err_probe(&pdev->dev, PTR_ERR(base), >>> + "failed to map registers\n"); >>> + >>> + rst = devm_reset_control_get_exclusive(&pdev->dev, "apb"); >>> + if (IS_ERR(rst)) >>> + return dev_err_probe(&pdev->dev, PTR_ERR(rst), "failed to get apb reset\n"); >>> + >>> + pclk = devm_clk_get_enabled(&pdev->dev, "apb"); >>> + if (IS_ERR(pclk)) >>> + return dev_err_probe(&pdev->dev, PTR_ERR(pclk), >>> + "failed to get & enable apb clock\n"); >>> + >>> + ret = reset_control_deassert(rst); >>> + if (ret) >>> + return dev_err_probe(&pdev->dev, ret, "failed to deassert apb reset\n"); >> >> Hi, >> >> I'm not very familiar with the reset_control_[de]assert() functions, but shouldn't this be undone by a reset_control_assert() call if an error occurs later? > > In this case, the reset controller is set from 'assert' state to 'deassert' state. If it is failed and still 'assert' state, I don't think it need to call reset_control_assert().
Emil already explained what I meaned (sorry for not being clear enough). I do agree with his proposed approach.
> >> >>> + >>> + for (ch = 0; ch < JH7110_TIMER_CH_MAX; ch++) { >>> + clkevt[ch] = devm_kzalloc(&pdev->dev, sizeof(*clkevt[ch]), GFP_KERNEL); >>> + if (!clkevt[ch]) >>> + return -ENOMEM; >>> + >>> + snprintf(name, sizeof(name), "ch%d", ch); >>> + >>> + clkevt[ch]->base = base + JH7110_TIMER_CH_BASE(ch); >>> + /* Ensure timer is disabled */ >>> + jh7110_timer_disable(clkevt[ch]); >>> + >>> + rst = devm_reset_control_get_exclusive(&pdev->dev, name); >>> + if (IS_ERR(rst)) >>> + return PTR_ERR(rst); >>> + >>> + clkevt[ch]->clk = devm_clk_get_enabled(&pdev->dev, name); >>> + if (IS_ERR(clkevt[ch]->clk)) >>> + return PTR_ERR(clkevt[ch]->clk); >>> + >>> + ret = reset_control_deassert(rst); >>> + if (ret) >>> + return ret; >> >> Same here. >> >>> + >>> + clkevt[ch]->evt.irq = platform_get_irq(pdev, ch); >>> + if (clkevt[ch]->evt.irq < 0) >>> + return clkevt[ch]->evt.irq; >>> + >>> + snprintf(clkevt[ch]->name, sizeof(clkevt[ch]->name), "%s.ch%d", pdev->name, ch); >>> + jh7110_clockevents_register(clkevt[ch]); >>> + >>> + ret = devm_request_irq(&pdev->dev, clkevt[ch]->evt.irq, jh7110_timer_interrupt, >>> + IRQF_TIMER | IRQF_IRQPOLL, >>> + clkevt[ch]->name, &clkevt[ch]->evt); >>> + if (ret) >>> + return ret; >>> + >>> + ret = jh7110_clocksource_init(clkevt[ch]); >> >> Does something should be done if this fails? >> >> CJ > > Yes, it should be call reset_control_assert() here and I will add it in next version.
My point was for the above reset_control_assert() but also for the resources allocated within this for loop.
I have not checked all paths, but in case of error in the probe: - There is another reset_control_deassert()
- jh7110_clocksource_init() --> jh7110_timer_int_init_enable() --> jh7110_timer_enable() Should jh7110_timer_disable() be called?
- jh7110_clocksource_init() --> clocksource_register_hz(). Should clocksource_unregister() be called?
If I'm correct and depending on how you update the code, a .remove function may be needed as well.
CJ
> >> >>> + if (ret) >>> + return ret; >>> + } >>> + >>> + return 0; >>> +} >>> + >>> +static const struct of_device_id jh7110_timer_match[] = { >>> + { .compatible = "starfive,jh7110-timer", }, >>> + { /* sentinel */ } >>> +}; >>> +MODULE_DEVICE_TABLE(of, jh7110_timer_match); >>> + >>> +static struct platform_driver jh7110_timer_driver = { >>> + .probe = jh7110_timer_probe, >>> + .driver = { >>> + .name = "jh7110-timer", >>> + .of_match_table = jh7110_timer_match, >>> + }, >>> +}; >>> +module_platform_driver(jh7110_timer_driver); >>> + >>> +MODULE_AUTHOR("Xingyu Wu <xingyu.wu-bONrM45KWFOXmMXjJBpWqg-XMD5yJDbdMRS5n6/RkiaJA@public.gmane.orgne.org>"); >>> +MODULE_DESCRIPTION("StarFive JH7110 timer driver"); >>> +MODULE_LICENSE("GPL"); >> > > Thanks, > Xingyu Wu > > >
| |