Re: [PATCH 3/3] rtc: Add driver for nuvoton ma35d1 rtc controller

[Jacky Huang <ychuang570808@gmail.com>]

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