Message-ID: <2025110622425227de2cac@mail.local>
Date: Thu, 6 Nov 2025 23:42:52 +0100
From: Alexandre Belloni <alexandre.belloni@...tlin.com>
To: dang.huynh@...nlining.org
Cc: Manivannan Sadhasivam <mani@...nel.org>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Linus Walleij <linus.walleij@...aro.org>,
	Bartosz Golaszewski <brgl@...ev.pl>,
	Michael Turquette <mturquette@...libre.com>,
	Stephen Boyd <sboyd@...nel.org>,
	Philipp Zabel <p.zabel@...gutronix.de>,
	Sebastian Reichel <sre@...nel.org>, Vinod Koul <vkoul@...nel.org>,
	Kees Cook <kees@...nel.org>,
	"Gustavo A. R. Silva" <gustavoars@...nel.org>,
	Ulf Hansson <ulf.hansson@...aro.org>,
	linux-arm-kernel@...ts.infradead.org,
	linux-unisoc@...ts.infradead.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-gpio@...r.kernel.org,
	linux-rtc@...r.kernel.org, linux-clk@...r.kernel.org,
	linux-pm@...r.kernel.org, dmaengine@...r.kernel.org,
	linux-hardening@...r.kernel.org, linux-mmc@...r.kernel.org
Subject: Re: [PATCH 06/25] rtc: Add driver for RDA Micro SoC

Hello,

There are checkpatch --strict issues, please fix them.


On 17/09/2025 03:25:03+0700, Dang Huynh via B4 Relay wrote:
>  MAINTAINERS           |   6 +
>  drivers/rtc/Kconfig   |  11 ++
>  drivers/rtc/Makefile  |   1 +
>  drivers/rtc/rtc-rda.c | 356 ++++++++++++++++++++++++++++++++++++++++++++++++++

Unless you can guarantee this driver will support all the future RDA
SoC RTCs, the filename needs to be SoC specific.

> +config RTC_DRV_RDA
> +	tristate "RDA Micro RTC"
> +	depends on ARCH_RDA || COMPILE_TEST
> +	select REGMAP_MMIO
> +	help
> +	  If you say yes here you get support for the built-in RTC on
> +	  RDA Micro SoC.

You probably also need to list which ones are supported.

> +static int rda_rtc_settime(struct device *dev, struct rtc_time *tm)
> +{
> +	struct rda_rtc *rtc = dev_get_drvdata(dev);
> +	u32 high, low;
> +	int ret;
> +
> +	ret = rtc_valid_tm(tm);
> +	if (ret < 0)
> +		return ret;

The RTC core will never pass an invalid rtc_tm, this check is useless.

> +
> +	/*
> +	 * The number of years since 1900 in kernel,
> +	 * but it is defined since 2000 by HW.
> +	 * The number of mons' range is from 0 to 11 in kernel,
> +	 * but it is defined from 1 to 12 by HW.

This comment is not super useful as this is super common in the RTC
drivers,. If you want to keep it, please fix it.

> +	 */
> +	low = FIELD_PREP(RDA_SEC_MASK, tm->tm_sec) |
> +		FIELD_PREP(RDA_MIN_MASK, tm->tm_min) |
> +		FIELD_PREP(RDA_HRS_MASK, tm->tm_hour);
> +
> +	high = FIELD_PREP(RDA_MDAY_MASK, tm->tm_mday) |
> +		FIELD_PREP(RDA_MON_MASK, tm->tm_mon + 1) |
> +		FIELD_PREP(RDA_YEAR_MASK, tm->tm_year - 100) |
> +		FIELD_PREP(RDA_WDAY_MASK, tm->tm_wday);
> +
> +	ret = regmap_write(rtc->regmap, RDA_RTC_CAL_LOAD_LOW_REG, low);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to update RTC low register: %d\n", ret);

This needs to be a dev_dbg or removed.

> +		return ret;
> +	}
> +
> +	ret = regmap_write(rtc->regmap, RDA_RTC_CAL_LOAD_HIGH_REG, high);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to update RTC low register: %d\n", ret);

Ditto

> +		return ret;
> +	}
> +
> +	ret = regmap_update_bits(rtc->regmap, RDA_RTC_CMD_REG, RDA_RTC_CMD_CAL_LOAD, 1);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to update RTC cal load register: %d\n", ret);

Ditto

> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int rda_rtc_readtime(struct device *dev, struct rtc_time *tm)
> +{
> +	struct rda_rtc *rtc = dev_get_drvdata(dev);
> +	unsigned int high, low;
> +	int ret;
> +
> +	/*
> +	 * Check if RTC data is valid.
> +	 *
> +	 * When this bit is set, it means the data in the RTC is invalid
> +	 * or not configured.
> +	 */
> +	ret = regmap_test_bits(rtc->regmap, RDA_RTC_STA_REG, RDA_RTC_STA_NOT_PROG);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to read RTC status: %d\n", ret);

dev_dbg

> +		return ret;
> +	} else if (ret > 0)
> +		return -EINVAL;
> +
> +	ret = regmap_read(rtc->regmap, RDA_RTC_CUR_LOAD_HIGH_REG, &high);
> +	if (ret) {
> +		dev_err(dev, "Failed to read RTC high reg: %d\n", ret);

Ditto

> +		return ret;
> +	}
> +
> +	ret = regmap_read(rtc->regmap, RDA_RTC_CUR_LOAD_LOW_REG, &low);
> +	if (ret) {
> +		dev_err(dev, "Failed to read RTC low reg: %d\n", ret);

Ditto

> +		return ret;
> +	}
> +
> +	tm->tm_sec = FIELD_GET(RDA_SEC_MASK, low);
> +	tm->tm_min = FIELD_GET(RDA_MIN_MASK, low);
> +	tm->tm_hour = FIELD_GET(RDA_HRS_MASK, low);
> +	tm->tm_mday = FIELD_GET(RDA_MDAY_MASK, high);
> +	tm->tm_mon = FIELD_GET(RDA_MON_MASK, high);
> +	tm->tm_year = FIELD_GET(RDA_YEAR_MASK, high);
> +	tm->tm_wday = FIELD_GET(RDA_WDAY_MASK, high);
> +
> +	/*
> +	 * The number of years since 1900 in kernel,
> +	 * but it is defined since 2000 by HW.
> +	 */
> +	tm->tm_year += 100;
> +	/*
> +	 * The number of mons' range is from 0 to 11 in kernel,
> +	 * but it is defined from 1 to 12 by HW.
> +	 */

You can probably drop both comments.

> +	tm->tm_mon -= 1;
> +
> +	return 0;
> +}
> +
> +static int rda_rtc_readalarm(struct device *dev, struct rtc_wkalrm *alrm)
> +{
> +	struct rda_rtc *rtc = dev_get_drvdata(dev);
> +	struct rtc_time *tm = &alrm->time;
> +	unsigned int high, low;
> +	int ret;
> +
> +	ret = regmap_read(rtc->regmap, RDA_RTC_ALARM_HIGH_REG, &high);
> +	if (ret) {
> +		dev_err(dev, "Failed to read alarm low reg: %d\n", ret);

Just to be clear, the driver is super verbose with all those dev_err.
Strings are bloating the kernel and those string will probably never be
seen by any user and event if they are seen, the user doesn't have any
other action to do other than retrying. Please remove them of move them
to dev_dbg

> +		return ret;
> +	}
> +
> +	ret = regmap_read(rtc->regmap, RDA_RTC_ALARM_LOW_REG, &low);
> +	if (ret) {
> +		dev_err(dev, "Failed to read alarm low reg: %d\n", ret);
> +		return ret;
> +	}
> +
> +	tm->tm_sec = FIELD_GET(RDA_SEC_MASK, low);
> +	tm->tm_min = FIELD_GET(RDA_MIN_MASK, low);
> +	tm->tm_hour = FIELD_GET(RDA_HRS_MASK, low);
> +	tm->tm_mday = FIELD_GET(RDA_MDAY_MASK, high);
> +	tm->tm_mon = FIELD_GET(RDA_MON_MASK, high);
> +	tm->tm_year = FIELD_GET(RDA_YEAR_MASK, high);
> +	tm->tm_wday = FIELD_GET(RDA_WDAY_MASK, high);
> +
> +	/*
> +	 * The number of years since 1900 in kernel,
> +	 * but it is defined since 2000 by HW.
> +	 */
> +	tm->tm_year += 100;
> +	/*
> +	 * The number of mons' range is from 0 to 11 in kernel,
> +	 * but it is defined from 1 to 12 by HW.
> +	 */
> +	tm->tm_mon -= 1;
> +
> +	return 0;
> +}
> +
> +static int rda_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled)
> +{
> +	struct rda_rtc *rtc = dev_get_drvdata(dev);
> +
> +	if (enabled)
> +		return regmap_update_bits(rtc->regmap, RDA_RTC_CMD_REG,
> +				RDA_RTC_CMD_ALARM_ENABLE, 1);
> +
> +	return regmap_update_bits(rtc->regmap, RDA_RTC_CMD_REG,
> +			RDA_RTC_CMD_ALARM_DISABLE, 1);

Wow, this is super weird, so you have one bit to enable and one to
disable the alarm. Is RDA_RTC_CMD_REG write only?

> +}
> +
> +static int rda_rtc_setalarm(struct device *dev, struct rtc_wkalrm *alrm)
> +{
> +	struct rda_rtc *rtc = dev_get_drvdata(dev);
> +	struct rtc_time *tm = &alrm->time;
> +	u32 high, low;
> +	int ret;
> +
> +	ret = rtc_valid_tm(tm);
> +	if (ret < 0)
> +		return ret;
> +

tm will never be invalid

> +	/* TODO: Check if it's necessary to disable IRQ first */

I'd say probably not ;)

> +	rda_rtc_alarm_irq_enable(dev, 0);
> +
> +	/*
> +	 * The number of years since 1900 in kernel,
> +	 * but it is defined since 2000 by HW.
> +	 * The number of mons' range is from 0 to 11 in kernel,
> +	 * but it is defined from 1 to 12 by HW.
> +	 */

This is still the same comment...

> +	low = FIELD_PREP(RDA_SEC_MASK, tm->tm_sec) |
> +		FIELD_PREP(RDA_MIN_MASK, tm->tm_min) |
> +		FIELD_PREP(RDA_HRS_MASK, tm->tm_hour);
> +
> +	high = FIELD_PREP(RDA_MDAY_MASK, tm->tm_mday) |
> +		FIELD_PREP(RDA_MON_MASK, tm->tm_mon + 1) |
> +		FIELD_PREP(RDA_YEAR_MASK, tm->tm_year - 100) |
> +		FIELD_PREP(RDA_WDAY_MASK, tm->tm_wday);
> +
> +
> +	ret = regmap_write(rtc->regmap, RDA_RTC_ALARM_LOW_REG, low);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to set low alarm register: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = regmap_write(rtc->regmap, RDA_RTC_ALARM_HIGH_REG, high);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to set low alarm register: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = regmap_update_bits(rtc->regmap, RDA_RTC_CMD_REG, RDA_RTC_CMD_ALARM_LOAD, 1);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to set alarm register: %d\n", ret);
> +		return ret;
> +	}
> +
> +	dev_dbg(dev, "Alarm set: %4d-%02d-%02d %02d:%02d:%02d\n",
> +			2000 + (tm->tm_year - 100), tm->tm_mon + 1, tm->tm_mday,
> +			tm->tm_hour, tm->tm_min, tm->tm_sec);

You probably want to use %ptR or drop this as we have a tracepoint just
after.

> +
> +	return 0;
> +}
> +
> +static int rda_rtc_proc(struct device *dev, struct seq_file *seq)
> +{
> +	struct rda_rtc *rtc = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = regmap_test_bits(rtc->regmap, RDA_RTC_STA_REG, RDA_RTC_STA_ALARM_ENABLE);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to read alarm status: %d\n", ret);
> +		return ret;
> +	}
> +
> +	seq_printf(seq, "alarm enable\t: %s\n", (ret > 0) ? "yes" : "no");
> +
> +	return 0;
> +}

Drop this function, this interface is obsolete

> +
> +static const struct rtc_class_ops rda_rtc_ops = {
> +	.read_time = rda_rtc_readtime,
> +	.set_time = rda_rtc_settime,
> +	.read_alarm = rda_rtc_readalarm,
> +	.set_alarm = rda_rtc_setalarm,
> +	.proc = rda_rtc_proc,
> +	.alarm_irq_enable = rda_rtc_alarm_irq_enable,
> +};
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int rda_rtc_suspend(struct platform_device *pdev, pm_message_t state)
> +{
> +	/* TODO: Check if it's okay to turn on alarm IRQ when it's not set */
> +	return rda_rtc_alarm_irq_enable(&pdev->dev, 1);
> +}
> +
> +static int rda_rtc_resume(struct platform_device *pdev)
> +{
> +	/* If alarms were left, we turn them off. */
> +	return rda_rtc_alarm_irq_enable(&pdev->dev, 0);
> +}

Let userspace enabling/disabling alarm, the kernel must not decide to
enable or disable them which fixes your TODO

> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(rda_rtc_pm_ops, rda_rtc_suspend, rda_rtc_resume);
> +
> +static const struct regmap_config regmap_config = {
> +	.reg_bits = 32,
> +	.val_bits = 32,
> +	.reg_stride = 4,
> +};
> +
> +static int rda_rtc_probe(struct platform_device *pdev)
> +{
> +	struct rda_rtc *rda_rtc;
> +	void __iomem *base;
> +
> +	rda_rtc = devm_kzalloc(&pdev->dev, sizeof(*rda_rtc), GFP_KERNEL);
> +	if (!rda_rtc)
> +		return -ENOMEM;
> +
> +	base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(base))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(base),
> +				"failed to remap resource\n");
> +
> +	rda_rtc->regmap = devm_regmap_init_mmio(&pdev->dev, base, &regmap_config);
> +	if (!rda_rtc->regmap)
> +		return dev_err_probe(&pdev->dev, PTR_ERR(rda_rtc->regmap),
> +				"can't find regmap\n");
> +
> +	rda_rtc->rtc_dev = devm_rtc_allocate_device(&pdev->dev);
> +	if (IS_ERR(rda_rtc->rtc_dev))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(rda_rtc->rtc_dev),
> +				"failed to allocate rtc device\n");
> +
> +	rda_rtc->rtc_dev->ops = &rda_rtc_ops;
> +	rda_rtc->rtc_dev->range_min = RTC_TIMESTAMP_BEGIN_2000;
> +	rda_rtc->rtc_dev->range_max = RTC_TIMESTAMP_END_2127;
> +
> +	platform_set_drvdata(pdev, rda_rtc);
> +
> +	return devm_rtc_register_device(rda_rtc->rtc_dev);
> +}
> +
> +static const struct of_device_id rda_rtc_id_table[] = {
> +	{ .compatible = "rda,8810pl-rtc", },
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, rda_rtc_id_table);
> +
> +static struct platform_driver rda_rtc_driver = {
> +	.probe = rda_rtc_probe,
> +	.driver = {
> +		.name = "rtc-rda",
> +		.pm = &rda_rtc_pm_ops,
> +		.of_match_table = rda_rtc_id_table,
> +	},
> +};
> +module_platform_driver(rda_rtc_driver);
> +
> +MODULE_AUTHOR("Dang Huynh <dang.huynh@...nlining.org>");
> +MODULE_DESCRIPTION("RDA Micro RTC driver");
> +MODULE_LICENSE("GPL");
> 
> -- 
> 2.51.0
> 
> 

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

Powered by blists - more mailing lists