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
