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

[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@xxxxxxxxxxx>
> >
> > 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