lkml.org 
[lkml]   [2023]   [Jul]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH 3/3] rtc: Add driver for nuvoton ma35d1 rtc controller
From
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

\
 
 \ /
  Last update: 2023-07-21 09:54    [W:0.032 / U:0.484 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site