Message-ID: <YYHZdXqpNqzusQ5n@piout.net>
Date:   Wed, 3 Nov 2021 01:36:05 +0100
From:   Alexandre Belloni <alexandre.belloni@...tlin.com>
To:     Vincent Shih <vincent.sunplus@...il.com>
Cc:     a.zummo@...ertech.it, p.zabel@...gutronix.de,
        linux-kernel@...r.kernel.org, linux-rtc@...r.kernel.org,
        robh+dt@...nel.org, devicetree@...r.kernel.org,
        Vincent Shih <vincent.shih@...plus.com>
Subject: Re: [PATCH 1/2] rtc: Add driver for Sunplus SP7021

Hello,

On 02/11/2021 14:22:02+0800, Vincent Shih wrote:
> +/**************************************************************************************************/
> +/* How to test RTC:										  */
> +/*												  */
> +/* 1. use kernel commands									  */
> +/* hwclock - query and set the hardware clock (RTC)						  */
> +/*												  */
> +/* (for i in `seq 5`; do (echo ------ && echo -n 'date      : ' && date && echo -n 'hwclock -r: ' */
> +/*								&& hwclock -r; sleep 1); done)	  */
> +/* date 121209002014 # Set system to 2014/Dec/12 09:00						  */
> +/* (for i in `seq 5`; do (echo ------ && echo -n 'date      : ' && date && echo -n 'hwclock -r: ' */
> +/*								&& hwclock -r; sleep 1); done)	  */
> +/* hwclock -s # Set the System Time from the Hardware Clock					  */
> +/* (for i in `seq 5`; do (echo ------ && echo -n 'date      : ' && date && echo -n 'hwclock -r: ' */
> +/*								&& hwclock -r; sleep 1); done)	  */
> +/* date 121213002014 # Set system to 2014/Dec/12 13:00						  */
> +/* (for i in `seq 5`; do (echo ------ && echo -n 'date      : ' && date && echo -n 'hwclock -r: ' */
> +/*								&& hwclock -r; sleep 1); done)	  */
> +/* hwclock -w # Set the Hardware Clock to the current System Time				  */
> +/* (for i in `seq 10000`; do (echo ------ && echo -n 'date  : ' && date && echo -n 'hwclock -r: ' */
> +/*								&& hwclock -r; sleep 1); done)	  */
> +/*												  */
> +/* How to setup alarm (e.g., 10 sec later):							  */
> +/*     echo 0 > /sys/class/rtc/rtc0/wakealarm && nnn=`date '+%s'` && echo $nnn && \		  */
> +/*     nnn=`expr $nnn + 10` && echo $nnn > /sys/class/rtc/rtc0/wakealarm			  */
> +/*												  */
> +/* 2. use RTC Driver Test Program (\linux\application\module_test\rtc\rtc-test.c)		  */
> +/*												  */
> +/**************************************************************************************************/

I don't feel this is a super useful comment, especially since it is
buried in a driver and this basically says to use rtctest.c

> +#include <linux/module.h>
> +#include <linux/err.h>
> +#include <linux/rtc.h>
> +#include <linux/platform_device.h>
> +#include <linux/clk.h>
> +#include <linux/reset.h>
> +#include <linux/of.h>
> +#include <linux/ktime.h>
> +#include <linux/io.h>

This list has to be sorted

> +
> +/* ---------------------------------------------------------------------------------------------- */
> +#define FUNC_DEBUG() pr_debug("[RTC] Debug: %s(%d)\n", __func__, __LINE__)
> +
> +#define RTC_DEBUG(fmt, args ...) pr_debug("[RTC] Debug: " fmt, ## args)
> +#define RTC_INFO(fmt, args ...) pr_info("[RTC] Info: " fmt, ## args)
> +#define RTC_WARN(fmt, args ...) pr_warn("[RTC] Warning: " fmt, ## args)
> +#define RTC_ERR(fmt, args ...) pr_err("[RTC] Error: " fmt, ## args)
> +/* ---------------------------------------------------------------------------------------------- */
> +
> +struct sunplus_rtc {
> +	struct clk *rtcclk;
> +	struct reset_control *rstc;
> +	unsigned long set_alarm_again;
> +	u32 charging_mode;
> +};
> +
> +struct sunplus_rtc sp_rtc;
> +
> +#define RTC_REG_NAME		"rtc_reg"
> +
> +struct sp_rtc_reg {
> +	unsigned int rsv00;
> +	unsigned int rsv01;
> +	unsigned int rsv02;
> +	unsigned int rsv03;
> +	unsigned int rsv04;
> +	unsigned int rsv05;
> +	unsigned int rsv06;
> +	unsigned int rsv07;
> +	unsigned int rsv08;
> +	unsigned int rsv09;
> +	unsigned int rsv10;
> +	unsigned int rsv11;
> +	unsigned int rsv12;
> +	unsigned int rsv13;
> +	unsigned int rsv14;
> +	unsigned int rsv15;
> +	unsigned int rtc_ctrl;
> +	unsigned int rtc_timer_out;
> +	unsigned int rtc_divider;
> +	unsigned int rtc_timer_set;
> +	unsigned int rtc_alarm_set;
> +	unsigned int rtc_user_data;
> +	unsigned int rtc_reset_record;
> +	unsigned int rtc_battery_ctrl;
> +	unsigned int rtc_trim_ctrl;
> +	unsigned int rsv25;
> +	unsigned int rsv26;
> +	unsigned int rsv27;
> +	unsigned int rsv28;
> +	unsigned int rsv29;
> +	unsigned int rsv30;
> +	unsigned int rsv31;
> +};

We don't use that kind of structs, please use defines and offsets.

> +
> +static struct sp_rtc_reg *rtc_reg_ptr;
> +
> +static void sp_get_seconds(unsigned long *secs)
> +{
> +	*secs = (unsigned long)readl(&rtc_reg_ptr->rtc_timer_out);
> +}
> +
> +static void sp_set_seconds(unsigned long secs)
> +{
> +	writel((u32)secs, &rtc_reg_ptr->rtc_timer_set);
> +}
> +
> +static int sp_rtc_read_time(struct device *dev, struct rtc_time *tm)
> +{
> +	unsigned long secs;
> +
> +	sp_get_seconds(&secs);
> +	rtc_time64_to_tm(secs, tm);
> +	RTC_DEBUG("%s:  RTC date/time to %d-%d-%d, %02d:%02d:%02d.\r\n",
> +		__func__, tm->tm_mday, tm->tm_mon + 1, tm->tm_year,
> +					tm->tm_hour, tm->tm_min, tm->tm_sec);
> +
> +	return rtc_valid_tm(tm);
> +}
> +
> +int sp_rtc_get_time(struct rtc_time *tm)
> +{
> +	unsigned long secs;
> +
> +	sp_get_seconds(&secs);
> +	rtc_time64_to_tm(secs, tm);
> +	return 0;
> +}
> +EXPORT_SYMBOL(sp_rtc_get_time);
> +

Why is this exported?

> +static int sp_rtc_suspend(struct platform_device *pdev, pm_message_t state)
> +{
> +	FUNC_DEBUG();
> +
> +	// Keep RTC from system reset
> +	writel((1 << (16+4)) | (1 << 4), &rtc_reg_ptr->rtc_ctrl);
> +

Plenty of magic values here, please explain.

> +	return 0;
> +}
> +
> +static int sp_rtc_resume(struct platform_device *pdev)
> +{
> +	/*						*/
> +	/* Because RTC is still powered during suspend,	*/
> +	/* there is nothing to do here.			*/
> +	/*						*/
> +	FUNC_DEBUG();
> +
> +	// Keep RTC from system reset
> +	writel((1 << (16+4)) | (1 << 4), &rtc_reg_ptr->rtc_ctrl);
> +
> +	return 0;
> +}
> +
> +static int sp_rtc_set_time(struct device *dev, struct rtc_time *tm)
> +{
> +	unsigned long secs;
> +
> +	secs = rtc_tm_to_time64(tm);
> +	RTC_DEBUG("%s, secs = %lu\n", __func__, secs);
> +	sp_set_seconds(secs);
> +
> +	return 0;
> +}
> +
> +static int sp_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> +{
> +	struct rtc_device *rtc = dev_get_drvdata(dev);
> +	unsigned long alarm_time;
> +
> +	alarm_time = rtc_tm_to_time64(&alrm->time);
> +	RTC_DEBUG("%s, alarm_time: %u\n", __func__, (u32)(alarm_time));
> +
> +	if (alarm_time > 0xFFFFFFFF)
> +		return -EINVAL;

Please set the range of the rtc properly and the core will do this check
for you.

> +
> +	if ((rtc->aie_timer.enabled) && (rtc->aie_timer.node.expires == ktime_set(alarm_time, 0))) {
> +		if (rtc->uie_rtctimer.enabled)
> +			sp_rtc.set_alarm_again = 1;
> +	}

You have to explain that.

> +
> +	writel((u32)alarm_time, &rtc_reg_ptr->rtc_alarm_set);
> +	wmb();			// make sure settings are effective.

doesn't writel come with a barrier?

> +
> +	// enable alarm for update irq
> +	if (rtc->uie_rtctimer.enabled)
> +		writel((0x003F << 16) | 0x17, &rtc_reg_ptr->rtc_ctrl);
> +	else if (!rtc->aie_timer.enabled)
> +		writel((0x0007 << 16) | 0x0, &rtc_reg_ptr->rtc_ctrl);

Magic values, please explain also, I'm not sure why you need to look at
uie_rtctimer and aie_timer as your RTC seems capable of having an alarm
every seconds.

> +
> +	return 0;
> +}
> +
> +static int sp_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> +{
> +	unsigned int alarm_time;
> +
> +	alarm_time = readl(&rtc_reg_ptr->rtc_alarm_set);
> +	RTC_DEBUG("%s, alarm_time: %u\n", __func__, alarm_time);
> +	rtc_time64_to_tm((unsigned long)(alarm_time), &alrm->time);
> +

You have to also set whether the alarm is enabled or not, else, simply
don't bother returning anything.

> +	return 0;
> +}
> +
> +static int sp_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled)
> +{
> +	struct rtc_device *rtc = dev_get_drvdata(dev);
> +
> +	if (enabled)
> +		writel((0x003F << 16) | 0x17, &rtc_reg_ptr->rtc_ctrl);
> +	else if (!rtc->uie_rtctimer.enabled)
> +		writel((0x0007 << 16) | 0x0, &rtc_reg_ptr->rtc_ctrl);
> +
> +	return 0;
> +}
> +
> +static const struct rtc_class_ops sp_rtc_ops = {
> +	.read_time =		sp_rtc_read_time,
> +	.set_time =		sp_rtc_set_time,
> +	.set_alarm =		sp_rtc_set_alarm,
> +	.read_alarm =		sp_rtc_read_alarm,
> +	.alarm_irq_enable =	sp_rtc_alarm_irq_enable,
> +};
> +
> +static irqreturn_t rtc_irq_handler(int irq, void *dev_id)
> +{
> +	struct platform_device *plat_dev = dev_id;
> +	struct rtc_device *rtc = platform_get_drvdata(plat_dev);
> +
> +	if (rtc->uie_rtctimer.enabled) {
> +		rtc_update_irq(rtc, 1, RTC_IRQF | RTC_UF);
> +		RTC_DEBUG("[RTC] update irq\n");
> +
> +		if (sp_rtc.set_alarm_again == 1) {
> +			sp_rtc.set_alarm_again = 0;
> +			rtc_update_irq(rtc, 1, RTC_IRQF | RTC_AF);
> +			RTC_DEBUG("[RTC] alarm irq\n");
> +		}
> +	} else {
> +		rtc_update_irq(rtc, 1, RTC_IRQF | RTC_AF);
> +		RTC_DEBUG("[RTC] alarm irq\n");
> +	}

I'm pretty sure you can get rid of most of that and stop looking at
uie_rtctimer.

> +
> +	return IRQ_HANDLED;
> +}
> +
> +/* ---------------------------------------------------------------------------------------------- */
> +/* mode   bat_charge_rsel   bat_charge_dsel   bat_charge_en     Remarks				  */
> +/* 0xE            x              x                 0            Disable				  */
> +/* 0x1            0              0                 1            0.86mA (2K Ohm with diode)	  */
> +/* 0x5            1              0                 1            1.81mA (250 Ohm with diode)	  */
> +/* 0x9            2              0                 1            2.07mA (50 Ohm with diode)	  */
> +/* 0xD            3              0                 1            16.0mA (0 Ohm with diode)	  */
> +/* 0x3            0              1                 1            1.36mA (2K Ohm without diode)	  */
> +/* 0x7            1              1                 1            3.99mA (250 Ohm without diode)	  */
> +/* 0xB            2              1                 1            4.41mA (50 Ohm without diode)	  */
> +/* 0xF            3              1                 1            16.0mA (0 Ohm without diode)	  */
> +/* ---------------------------------------------------------------------------------------------- */
> +static void sp_rtc_set_batt_charge_ctrl(u32 _mode)
> +{
> +	u8 m = _mode & 0x000F;
> +
> +	RTC_DEBUG("battery charge mode: 0x%X\n", m);
> +	writel((0x000F << 16) | m, &rtc_reg_ptr->rtc_battery_ctrl);
> +}
> +
> +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();
> +
> +	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);
> +
> +	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);
> +
> +	// 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);
> +
> +	// reset
> +	sp_rtc.rstc = devm_reset_control_get(&plat_dev->dev, NULL);
> +	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;
> +	}
> +
> +	ret = reset_control_deassert(sp_rtc.rstc);
> +	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);
> +	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);
> +	if (ret) {
> +		RTC_ERR("Failed to retrieve \'charging-mode\'!\n");
> +		goto free_reset_assert;
> +	}
> +	sp_rtc_set_batt_charge_ctrl(sp_rtc.charging_mode);

There are generic trickle charger property, please use those.

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

Use devm_rtc_allocate_device/devm_rtc_register_device instead.

> +
> +	platform_set_drvdata(plat_dev, rtc);
> +
> +	RTC_INFO("sp7021-rtc loaded\n");
> +
> +	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);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id sp_rtc_of_match[] = {
> +	{ .compatible = "sunplus,sp7021-rtc" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, sp_rtc_of_match);
> +
> +static struct platform_driver sp_rtc_driver = {
> +	.probe   = sp_rtc_probe,
> +	.remove  = sp_rtc_remove,
> +	.suspend = sp_rtc_suspend,
> +	.resume  = sp_rtc_resume,
> +	.driver  = {
> +		.name = "sp7021-rtc",
> +		.owner = THIS_MODULE,
> +		.of_match_table = sp_rtc_of_match,
> +	},
> +};
> +module_platform_driver(sp_rtc_driver);
> +
> +MODULE_AUTHOR("Vincent Shih <vincent.shih@...plus.com>");
> +MODULE_DESCRIPTION("Sunplus RTC driver");
> +MODULE_LICENSE("GPL v2");
> +
> -- 
> 2.7.4
> 

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Powered by blists - more mailing lists