Messages in this thread | | | Date | Fri, 21 Jul 2023 15:53:10 +0800 | Subject | Re: [PATCH 3/3] rtc: Add driver for nuvoton ma35d1 rtc controller | From | Jacky Huang <> |
| |
Dear Krzysztof,
On 2023/7/20 下午 02:14, Krzysztof Kozlowski wrote: > On 20/07/2023 03:28, Jacky Huang wrote: >> From: Jacky Huang <ychuang3@nuvoton.com> >> >> The ma35d1 rtc controller provides real-time and calendar messaging >> capabilities. It supports programmable time tick and alarm match >> interrupts. The time and calendar messages are expressed in BCD format. >> This driver supports the built-in rtc controller of the ma35d1. It >> enables setting and reading the rtc time and configuring and reading >> the rtc alarm. >> > ... > >> +static int ma35d1_rtc_probe(struct platform_device *pdev) >> +{ >> + struct ma35_rtc *ma35_rtc; >> + struct clk *clk; >> + u32 regval; >> + int err; >> + >> + ma35_rtc = devm_kzalloc(&pdev->dev, sizeof(struct ma35_rtc), > sizeof(*)
I will modify this as ma35_rtc = devm_kzalloc(&pdev->dev, sizeof(*ma35_rtc),
>> + GFP_KERNEL); >> + if (!ma35_rtc) >> + return -ENOMEM; >> + >> + ma35_rtc->rtc_reg = devm_platform_ioremap_resource(pdev, 0); >> + if (IS_ERR(ma35_rtc->rtc_reg)) >> + return PTR_ERR(ma35_rtc->rtc_reg); >> + >> + clk = of_clk_get(pdev->dev.of_node, 0); >> + if (IS_ERR(clk)) { >> + err = PTR_ERR(clk); >> + dev_err(&pdev->dev, "failed to get core clk: %d\n", err); >> + return -ENOENT; > return dev_err_probe
I will replace these with
return dev_err_probe(&pdev->dev, PTR_ERR(clk), "failed to find rtc clock\n");
> >> + } >> + err = clk_prepare_enable(clk); >> + if (err) >> + return -ENOENT; >> + >> + platform_set_drvdata(pdev, ma35_rtc); >> + >> + ma35_rtc->rtcdev = devm_rtc_device_register(&pdev->dev, pdev->name, >> + &ma35d1_rtc_ops, THIS_MODULE); >> + if (IS_ERR(ma35_rtc->rtcdev)) { >> + dev_err(&pdev->dev, "rtc device register failed\n"); >> + return PTR_ERR(ma35_rtc->rtcdev); >> + } >> + >> + err = ma35d1_rtc_init(ma35_rtc, RTC_INIT_TIMEOUT); >> + if (err) >> + return err; >> + >> + regval = rtc_reg_read(ma35_rtc, MA35_REG_RTC_CLKFMT); >> + regval |= RTC_CLKFMT_24HEN; >> + rtc_reg_write(ma35_rtc, MA35_REG_RTC_CLKFMT, regval); >> + >> + ma35_rtc->irq_num = platform_get_irq(pdev, 0); >> + >> + if (devm_request_irq(&pdev->dev, ma35_rtc->irq_num, ma35d1_rtc_interrupt, >> + IRQF_NO_SUSPEND, "ma35d1rtc", ma35_rtc)) { >> + dev_err(&pdev->dev, "ma35d1 RTC request irq failed\n"); >> + return -EBUSY; > return dev_err_probe > >> + } >> + >> + regval = rtc_reg_read(ma35_rtc, MA35_REG_RTC_INTEN); >> + regval |= RTC_INTEN_TICKIEN; >> + rtc_reg_write(ma35_rtc, MA35_REG_RTC_INTEN, regval); >> + >> + device_init_wakeup(&pdev->dev, true); >> + >> + return 0; >> +} >> + >> +static int __exit ma35d1_rtc_remove(struct platform_device *pdev) > It's not an exit. > >> +{ >> + device_init_wakeup(&pdev->dev, false); >> + >> + platform_set_drvdata(pdev, NULL); > Just drop remove. You don't do anything useful here.
I will drop 'ma35d1_rtc_remove'. > > >> + >> + return 0; >> +} >> + >> +static int ma35d1_rtc_suspend(struct platform_device *pdev, pm_message_t state) >> +{ >> + struct ma35_rtc *ma35_rtc = platform_get_drvdata(pdev); >> + u32 regval; >> + >> + if (device_may_wakeup(&pdev->dev)) >> + enable_irq_wake(ma35_rtc->irq_num); >> + >> + regval = rtc_reg_read(ma35_rtc, MA35_REG_RTC_INTEN); >> + regval &= ~RTC_INTEN_TICKIEN; >> + rtc_reg_write(ma35_rtc, MA35_REG_RTC_INTEN, regval); >> + >> + return 0; >> +} >> + >> +static int ma35d1_rtc_resume(struct platform_device *pdev) >> +{ >> + struct ma35_rtc *ma35_rtc = platform_get_drvdata(pdev); >> + u32 regval; >> + >> + if (device_may_wakeup(&pdev->dev)) >> + disable_irq_wake(ma35_rtc->irq_num); >> + >> + regval = rtc_reg_read(ma35_rtc, MA35_REG_RTC_INTEN); >> + regval |= RTC_INTEN_TICKIEN; >> + rtc_reg_write(ma35_rtc, MA35_REG_RTC_INTEN, regval); >> + >> + return 0; >> +} >> + >> +static const struct of_device_id ma35d1_rtc_of_match[] = { >> + { .compatible = "nuvoton,ma35d1-rtc", }, >> + {}, >> +}; >> +MODULE_DEVICE_TABLE(of, ma35d1_rtc_of_match); >> + >> +static struct platform_driver ma35d1_rtc_driver = { >> + .remove = __exit_p(ma35d1_rtc_remove), >> + .suspend = ma35d1_rtc_suspend, >> + .resume = ma35d1_rtc_resume, >> + .probe = ma35d1_rtc_probe, >> + .driver = { >> + .name = "rtc-ma35d1", >> + .owner = THIS_MODULE, > ??? No.
I will drop this line.
>> + .of_match_table = of_match_ptr(ma35d1_rtc_of_match),
I will modify it as
.of_match_table = ma35d1_rtc_of_match,
> Drop of_match_ptr. Didn't you get such comment before? Your other > submission also had the same bug... > > Actually, most of these comments you already received for your other > drivers, so it would be great if we did not have to repeat it for every > new driver from you. > > Best regards, > Krzysztof >
I will be more careful in reviewing code to avoid making the same mistakes again. Thank you for your guidance.,
Best Regards, Jacky Huang
| |