Hi Vincent,

On Tue, 2021-11-09 at 14:38 +0800, Vincent Shih wrote:
[...]

+struct sunplus_rtc {
+	struct clk *rtcclk;
+	struct reset_control *rstc;
+	void __iomem *base;
+	u32 ohms;
+};
+
+struct sunplus_rtc sp_rtc;

Why is this global?


[...]

+static int sp_rtc_probe(struct platform_device *plat_dev)
+{
+	struct rtc_device *rtc;
+	struct resource *res;
+	const struct rtc_class_ops *rtc_ops;
+	void __iomem *reg_base;
+	int ret, irq;
+
+	memset(&sp_rtc, 0, sizeof(sp_rtc));

I'd allocate device private data here with devm_kzalloc() instead.

+	// find and map our resources
+	res = platform_get_resource_byname(plat_dev, IORESOURCE_MEM, RTC_REG_NAME);
+
+	if (res) {
+		dev_dbg(&plat_dev->dev, "res = 0x%x\n", res->start);
+
+		reg_base = devm_ioremap_resource(&plat_dev->dev, res);

There is no need to check res before feeding it into
devm_ioremap_resource(). You can simplify this even further with
devm_platform_ioremap_resource_by_name().

+		if (IS_ERR(reg_base)) {
+			dev_err(&plat_dev->dev, "%s devm_ioremap_resource fail\n", RTC_REG_NAME);
+			return PTR_ERR(reg_base);
+		}
+
+		dev_dbg(&plat_dev->dev, "reg_base = 0x%lx\n", (unsigned long)reg_base);
+	}
+
+	sp_rtc.base = reg_base;
+	rtc_ops = &sp_rtc_ops;
+
+	// Keep RTC from system reset
+	writel(DIS_SYS_RST_RTC_MASK | DIS_SYS_RST_RTC, sp_rtc.base + RTC_CTRL);

Are you allowed to write to sp_rtc.base registers before releasing the
reset?

+	// request irq
+	irq = platform_get_irq(plat_dev, 0);
+	if (irq < 0) {
+		dev_err(&plat_dev->dev, "platform_get_irq failed\n");
+		irq = IRQ_NOTCONNECTED;

By doing this you are making devm_request_irq() below return -ENOTCONN.
Why not return the real error code right here?

		return dev_err_probe(&plat_dev->dev, irq, "platform_get_irq failed\n");

+	}
+
+	ret = devm_request_irq(&plat_dev->dev, irq, rtc_irq_handler,
+					IRQF_TRIGGER_RISING, "rtc irq", plat_dev);
+	if (ret) {
+		dev_err(&plat_dev->dev, "devm_request_irq failed: %d\n", ret);
+		return ret;

This could be shortened to:

		return dev_err_probe(&plat_dev->dev, ret, "devm_request_irq failed\n");

+	}
+
+	// Setup trickle charger
+	if (plat_dev->dev.of_node)
+		sp_rtc_set_trickle_charger(plat_dev->dev);
+
+	// reset
+	sp_rtc.rstc = devm_reset_control_get_exclusive(&plat_dev->dev, NULL);
+	if (IS_ERR(sp_rtc.rstc)) {
+		ret = dev_err_probe(&plat_dev->dev, PTR_ERR(sp_rtc.rstc),
+					    "failed to retrieve reset controller\n");
+		return PTR_ERR(sp_rtc.rstc);

This could be shortened to:

		return dev_err_probe(&plat_dev->dev, PTR_ERR(sp_rtc.rstc),
				     "failed to retrieve reset controller\n");

+	}
+
+	// clk
+	sp_rtc.rtcclk = devm_clk_get(&plat_dev->dev, NULL);
+	if (IS_ERR(sp_rtc.rtcclk)) {
+		dev_err(&plat_dev->dev, "devm_clk_get fail\n");
+		return PTR_ERR(sp_rtc.rtcclk);

+	}
+
+	ret = reset_control_deassert(sp_rtc.rstc);
+	if (ret)
+		return ret;
+
+	ret = clk_prepare_enable(sp_rtc.rtcclk);
+	if (ret)

Assert reset on failure?

+		return ret;
+
+	device_init_wakeup(&plat_dev->dev, 1);
+
+	rtc = devm_rtc_allocate_device(&plat_dev->dev);
+	if (IS_ERR(rtc))

Disable clock and assert reset on failure?

+		return PTR_ERR(rtc);
+
+	rtc->range_max = U32_MAX;
+	rtc->range_min = 0;
+	rtc->ops = rtc_ops;
+
+	ret = devm_rtc_register_device(rtc);
+	if (ret)
+		goto free_reset_assert_clk;
+
+	platform_set_drvdata(plat_dev, rtc);
+	dev_info(&plat_dev->dev, "sp7021-rtc loaded\n");
+
+	return 0;
+
+free_reset_assert_clk:
+	reset_control_assert(sp_rtc.rstc);
+	clk_disable_unprepare(sp_rtc.rtcclk);

Should this be the other way around? I'd expect this to be in the
opposite order of reset deassert and clock enable.

+
+	return ret;
+}
+
+static int sp_rtc_remove(struct platform_device *plat_dev)
+{
+	reset_control_assert(sp_rtc.rstc);
+	clk_disable_unprepare(sp_rtc.rtcclk);

Same as above.

regards
Philipp