RE: [PATCH 3/4] rtc: bbnsm: Add the bbnsm rtc support

Subject: Re: [PATCH 3/4] rtc: bbnsm: Add the bbnsm rtc support

On 21/11/2022 14:51:43+0800, Jacky Bai wrote:

quoted

The BBNSM module includes a real time counter with alarm.
Add a RTC driver for this function.

Signed-off-by: Jacky Bai <ping.bai@nxp.com>
Reviewed-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/rtc/Kconfig     |  12 +++
 drivers/rtc/Makefile    |   1 +
 drivers/rtc/rtc-bbnsm.c | 223
++++++++++++++++++++++++++++++++++++++++

I'd prefer that filename to include imx or mxc if this is only available on those
SoCs.

For now, it is used on i.MX SoC, not sure if it will be used on NXP other SoC. I will
update the file name to include 'imx' in v2.

quoted

diff --git a/drivers/rtc/rtc-bbnsm.c b/drivers/rtc/rtc-bbnsm.c new

file mode 100644 index 000000000000..4157b238ed9a

--- /dev/null
+++ b/drivers/rtc/rtc-bbnsm.c

@@ -0,0 +1,223 @@
+// SPDX-License-Identifier: GPL-2.0+
+//
+// Copyright 2022 NXP.
+
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pm_wakeirq.h>
+#include <linux/rtc.h>
+#include <linux/mfd/syscon.h>
+#include <linux/regmap.h>

This list should be sorted alphabetically

Thx, will fix it in V2.

quoted

+
+#define BBNSM_CTRL	0x8
+#define BBNSM_INT_EN	0x10
+#define BBNSM_EVENTS	0x14
+#define BBNSM_RTC_LS	0x40
+#define BBNSM_RTC_MS	0x44
+#define BBNSM_TA	0x50
+
+#define RTC_EN		0x2
+#define RTC_EN_MSK	0x3
+#define TA_EN		(0x2 << 2)
+#define TA_DIS		(0x1 << 2)
+#define TA_EN_MSK	(0x3 << 2)
+#define RTC_INT_EN	0x2
+#define TA_INT_EN	(0x2 << 2)
+
+#define BBNSM_EVENT_TA	TA_EN
+

I'm not clear why this define is needed

This define is for RTC alarm event status check, as it use the same bitfield offset as TA_EN,
I just did the above define. It seems introduce some unnecessary confusion. 

quoted

+static irqreturn_t bbnsm_rtc_irq_handler(int irq, void *dev_id) {
+	struct device *dev = dev_id;
+	struct bbnsm_rtc  *bbnsm = dev_get_drvdata(dev);
+	u32 val;
+	u32 event = 0;

You can rework this function to remove this variable because it is either 0 or
RTC_AF | RTC_IRQF

Ok, will refine in V2.

quoted

+
+	regmap_read(bbnsm->regmap, BBNSM_EVENTS, &val);
+	if (val & BBNSM_EVENT_TA) {
+		event |= (RTC_AF | RTC_IRQF);
+		bbnsm_rtc_alarm_irq_enable(dev, false);
+		/* clear the alarm event */
+		regmap_write_bits(bbnsm->regmap, BBNSM_EVENTS, TA_EN_MSK,

BBNSM_EVENT_TA);

quoted

+		rtc_update_irq(bbnsm->rtc, 1, event);
+	}
+
+	return event ? IRQ_HANDLED : IRQ_NONE; }
+
+static int bbnsm_rtc_probe(struct platform_device *pdev) {
+	struct bbnsm_rtc *bbnsm;
+	int ret;
+
+	bbnsm = devm_kzalloc(&pdev->dev, sizeof(*bbnsm), GFP_KERNEL);
+	if (!bbnsm)
+		return -ENOMEM;
+
+	bbnsm->rtc = devm_rtc_allocate_device(&pdev->dev);
+
+	bbnsm->regmap =

syscon_regmap_lookup_by_phandle(pdev->dev.of_node, "regmap");

quoted

+	if (IS_ERR(bbnsm->regmap)) {
+		dev_err(&pdev->dev, "bbnsm get regmap failed\n");
+		return PTR_ERR(bbnsm->regmap);
+	}
+
+	bbnsm->irq = platform_get_irq(pdev, 0);
+	if (bbnsm->irq < 0)
+		return bbnsm->irq;
+
+	platform_set_drvdata(pdev, bbnsm);
+
+	/* clear all the pending events */
+	regmap_write(bbnsm->regmap, BBNSM_EVENTS, 0x7A);
+
+	/* Enable the Real-Time counter */
+	regmap_update_bits(bbnsm->regmap, BBNSM_CTRL, RTC_EN_MSK,

RTC_EN);

quoted

+

Please don't do that, this removes an important piece of information.
Instead, let .set_time enable it and check it in .read_time as if this is not set,
you now you are out of PoR and the time is invalid

Will remove it. set_time has the enable operation. I will add enable check in read_time callback.

quoted

+	device_init_wakeup(&pdev->dev, true);
+	ret = dev_pm_set_wake_irq(&pdev->dev, bbnsm->irq);
+	if (ret)
+		dev_err(&pdev->dev, "Failed to enable the irq wakeup\n");

dev_err but the function is not failing. Maybe just dev_warn? Also, is this
message really necessary?

Not too necessary, I can drop the above log print.

BR

quoted

+
+	ret = devm_request_irq(&pdev->dev, bbnsm->irq,

bbnsm_rtc_irq_handler,

quoted

+			IRQF_SHARED, "rtc alarm", &pdev->dev);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to request irq %d: %d\n",
+			bbnsm->irq, ret);
+		return ret;
+	}
+
+	bbnsm->rtc->ops = &bbnsm_rtc_ops;
+	bbnsm->rtc->range_max = U32_MAX;
+
+	return devm_rtc_register_device(bbnsm->rtc);
+}
+
+static const struct of_device_id bbnsm_dt_ids[] = {
+	{ .compatible = "nxp,bbnsm-rtc", },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, bbnsm_dt_ids);
+
+static struct platform_driver bbnsm_rtc_driver = {
+	.driver = {
+		.name = "bbnsm_rtc",
+		.of_match_table = bbnsm_dt_ids,
+	},
+	.probe = bbnsm_rtc_probe,
+};
+module_platform_driver(bbnsm_rtc_driver);
+
+MODULE_AUTHOR("Jacky Bai <ping.bai@nxp.com>");
+MODULE_DESCRIPTION("NXP BBNSM RTC Driver");

MODULE_LICENSE("GPL");

quoted

--
2.37.1

--