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

+static int sp_rtc_probe(struct platform_device *plat_dev)
+{
+	int ret;
+	int err, irq;
+	struct rtc_device *rtc = NULL;
+	struct resource *res;
+	void __iomem *reg_base = NULL;
+
+	FUNC_DEBUG();

Drop these.

+	memset(&sp_rtc, 0, sizeof(sp_rtc));
+
+	// find and map our resources
+	res = platform_get_resource_byname(plat_dev, IORESOURCE_MEM, RTC_REG_NAME);
+	RTC_DEBUG("res = 0x%x\n", res->start);

Drop, this will crash if res == NULL.

+
+	if (res) {
+		reg_base = devm_ioremap_resource(&plat_dev->dev, res);
+		if (IS_ERR(reg_base))
+			RTC_ERR("%s devm_ioremap_resource fail\n", RTC_REG_NAME);
+	}
+	RTC_DEBUG("reg_base = 0x%lx\n", (unsigned long)reg_base);

Drop or use dev_dbg() instead.

+
+	// clk
+	sp_rtc.rtcclk = devm_clk_get(&plat_dev->dev, NULL);
+	RTC_DEBUG("sp_rtc->clk = 0x%lx\n", (unsigned long)sp_rtc.rtcclk);
+	if (IS_ERR(sp_rtc.rtcclk))
+		RTC_DEBUG("devm_clk_get fail\n");
+
+	ret = clk_prepare_enable(sp_rtc.rtcclk);

Only enable the clock after all resources are requested. That will
simplify the error path.

+
+	// reset
+	sp_rtc.rstc = devm_reset_control_get(&plat_dev->dev, NULL);

Use devm_reset_control_get_exclusive() instead.
This should be done before clk_prepare_enable().

+	RTC_DEBUG("sp_rtc->rstc = 0x%lx\n", (unsigned long)sp_rtc.rstc);
+	if (IS_ERR(sp_rtc.rstc)) {
+		ret = PTR_ERR(sp_rtc.rstc);
+		RTC_ERR("SPI failed to retrieve reset controller: %d\n", ret);
+		goto free_clk;

Then you could use return dev_err_probe() here.

+	}
+
+	ret = reset_control_deassert(sp_rtc.rstc);

Same as for the clock, only deassert the reset after all resources are
requested.

+	if (ret)
+		goto free_clk;
+
+	rtc_reg_ptr = (struct sp_rtc_reg *)(reg_base);
+
+	// Keep RTC from system reset
+	writel((1 << (16+4)) | (1 << 4), &rtc_reg_ptr->rtc_ctrl);
+
+	// request irq
+	irq = platform_get_irq(plat_dev, 0);

This should be done before clk_prepare_enable().

+	if (irq < 0) {
+		RTC_ERR("platform_get_irq failed\n");
+		goto free_reset_assert;
+	}
+
+	err = devm_request_irq(&plat_dev->dev, irq, rtc_irq_handler,
+					IRQF_TRIGGER_RISING, "rtc irq", plat_dev);
+	if (err) {
+		RTC_ERR("devm_request_irq failed: %d\n", err);
+		goto free_reset_assert;
+	}
+
+	// Get charging-mode.
+	ret = of_property_read_u32(plat_dev->dev.of_node, "charging-mode", &sp_rtc.charging_mode);

This could be done before clk_prepare_enable().

+	if (ret) {
+		RTC_ERR("Failed to retrieve \'charging-mode\'!\n");
+		goto free_reset_assert;
+	}
+	sp_rtc_set_batt_charge_ctrl(sp_rtc.charging_mode);
+
+	device_init_wakeup(&plat_dev->dev, 1);
+
+	rtc = devm_rtc_device_register(&plat_dev->dev, "sp7021-rtc", &sp_rtc_ops, THIS_MODULE);
+	if (IS_ERR(rtc)) {
+		ret = PTR_ERR(rtc);
+		goto free_reset_assert;
+	}
+
+	platform_set_drvdata(plat_dev, rtc);
+
+	RTC_INFO("sp7021-rtc loaded\n");

Use dev_info() instead.

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

	clk_disable_unprepare(sp_rtc.rtcclk);


regards
Philipp