Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <5f867f68-4ad6-11c7-5c1f-f568889b0ddf@linaro.org>
Date:   Thu, 20 Jul 2023 08:14:09 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: [PATCH 3/3] rtc: Add driver for nuvoton ma35d1 rtc controller
Content-Language: en-US
To:     Jacky Huang <ychuang570808@gmail.com>, a.zummo@towertech.it,
        alexandre.belloni@bootlin.com, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org
Cc:     linux-rtc@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        soc@kernel.org, mjchen@nuvoton.com, schung@nuvoton.com,
        Jacky Huang <ychuang3@nuvoton.com>
References: <20230720012826.430026-1-ychuang570808@gmail.com>
 <20230720012826.430026-4-ychuang570808@gmail.com>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <20230720012826.430026-4-ychuang570808@gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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(*)

> +								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

> +	}
> +	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.


> +
> +	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.

> +		.of_match_table = of_match_ptr(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