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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <eeea4f10-16d7-d218-da55-c15ce6525b43@gmail.com>
Date:   Fri, 21 Jul 2023 15:53:10 +0800
From:   Jacky Huang <ychuang570808@...il.com>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        a.zummo@...ertech.it, alexandre.belloni@...tlin.com,
        robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org,
        conor+dt@...nel.org
Cc:     linux-rtc@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        soc@...nel.org, mjchen@...oton.com, schung@...oton.com,
        Jacky Huang <ychuang3@...oton.com>
Subject: Re: [PATCH 3/3] rtc: Add driver for nuvoton ma35d1 rtc controller

Dear Krzysztof,


On 2023/7/20 下午 02:14, Krzysztof Kozlowski wrote:
> On 20/07/2023 03:28, Jacky Huang wrote:
>> From: Jacky Huang <ychuang3@...oton.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