On Tue, Nov 28, 2017 at 08:39:27AM +0100, linux-kernel-dev-QonKdJ6Bx35Wk0Htik3J/w@public.gmane.org wrote:

The lock should be a member of your driver data struct.

+
+/*!
+ * This function does write synchronization for writes to the lp srtc block.
+ * To take care of the asynchronous CKIL clock, all writes from the IP domain
+ * will be synchronized to the CKIL domain.
+ */

Please drop this exclamation mark at the beginning of comments. If
anything, use kerneldoc style for the function descriptions.

+static inline void rtc_write_sync_lp(void __iomem *ioaddr)
+{
+	unsigned int i, count;
+	/* Wait for 3 CKIL cycles */
+	for (i = 0; i < 3; i++) {
+		count = readl(ioaddr + SRTC_LPSCLR);
+		while ((readl(ioaddr + SRTC_LPSCLR)) == count)
+			;

This becomes an infinite loop when the hardware is not working as
expected. You should avoid that.

+	}
+}
+
+/*!
+ * This function updates the RTC alarm registers and then clears all the
+ * interrupt status bits.
+ *
+ * @param  alrm         the new alarm value to be updated in the RTC
+ *
+ * @return  0 if successful; non-zero otherwise.
+ */
+static int rtc_update_alarm(struct device *dev, struct rtc_time *alrm)
+{

mxc_rtc_update_alarm please like the other device specific functions
please. Same for rtc_write_sync_lp().

+	struct rtc_drv_data *pdata = dev_get_drvdata(dev);
+	void __iomem *ioaddr = pdata->ioaddr;
+	struct rtc_time alarm_tm, now_tm;
+	unsigned long now, time;
+	int ret;
+
+	now = __raw_readl(ioaddr + SRTC_LPSCMR);
+	rtc_time_to_tm(now, &now_tm);
+
+	alarm_tm.tm_year = now_tm.tm_year;
+	alarm_tm.tm_mon = now_tm.tm_mon;
+	alarm_tm.tm_mday = now_tm.tm_mday;
+
+	alarm_tm.tm_hour = alrm->tm_hour;
+	alarm_tm.tm_min = alrm->tm_min;
+	alarm_tm.tm_sec = alrm->tm_sec;
+
+	rtc_tm_to_time(&now_tm, &now);
+	rtc_tm_to_time(&alarm_tm, &time);
+
+	if (time < now) {
+		time += 60 * 60 * 24;
+		rtc_time_to_tm(time, &alarm_tm);
+	}
+	ret = rtc_tm_to_time(&alarm_tm, &time);
+
+	__raw_writel(time, ioaddr + SRTC_LPSAR);
+
+	/* clear alarm interrupt status bit */
+	__raw_writel(SRTC_LPSR_ALP, ioaddr + SRTC_LPSR);
+
+	return ret;
+}
+
+/*!
+ * This function is the RTC interrupt service routine.
+ *
+ * @param  irq          RTC IRQ number
+ * @param  dev_id       device ID which is not used
+ *
+ * @return IRQ_HANDLED as defined in the include/linux/interrupt.h file.
+ */
+static irqreturn_t mxc_rtc_interrupt(int irq, void *dev_id)
+{
+	struct platform_device *pdev = dev_id;
+	struct rtc_drv_data *pdata = platform_get_drvdata(pdev);
+	void __iomem *ioaddr = pdata->ioaddr;
+	u32 lp_status, lp_cr;
+	u32 events = 0;
+
+	lp_status = __raw_readl(ioaddr + SRTC_LPSR);
+	lp_cr = __raw_readl(ioaddr + SRTC_LPCR);
+
+	/* update irq data & counter */
+	if (lp_status & SRTC_LPSR_ALP) {
+		if (lp_cr & SRTC_LPCR_ALP)
+			events |= (RTC_AF | RTC_IRQF);
+
+		/* disable further lp alarm interrupts */
+		lp_cr &= ~(SRTC_LPCR_ALP | SRTC_LPCR_WAE);
+	}
+
+	/* Update interrupt enables */
+	__raw_writel(lp_cr, ioaddr + SRTC_LPCR);
+
+	/* clear interrupt status */
+	__raw_writel(lp_status, ioaddr + SRTC_LPSR);
+
+	rtc_write_sync_lp(ioaddr);
+	rtc_update_irq(pdata->rtc, 1, events);
+	return IRQ_HANDLED;
+}

What about locking? You lock the read/modify/write operation of the LPCR
register below, and so you should do here.

+
+/*!
+ * This function reads the current RTC time into tm in Gregorian date.
+ *
+ * @param  tm           contains the RTC time value upon return
+ *
+ * @return  0 if successful; non-zero otherwise.
+ */
+static int mxc_rtc_read_time(struct device *dev, struct rtc_time *tm)
+{
+	struct rtc_drv_data *pdata = dev_get_drvdata(dev);
+	time_t now = readl(pdata->ioaddr + SRTC_LPSCMR);
+
+	rtc_time_to_tm(now, tm);
+	return rtc_valid_tm(tm);
+}
+
+/*!
+ * This function sets the internal RTC time based on tm in Gregorian date.
+ *
+ * @param  tm           the time value to be set in the RTC
+ *
+ * @return  0 if successful; non-zero otherwise.
+ */
+static int mxc_rtc_set_time(struct device *dev, struct rtc_time *tm)
+{
+	struct rtc_drv_data *pdata = dev_get_drvdata(dev);
+	time64_t time = rtc_tm_to_time64(tm);
+
+	if (time > UINT_MAX) {
+		dev_warn(dev, "time_t has overflow\n");
+	}

SIGH. Who builds such a RTC in the 21st century?

Shouldn't this return an error?

+
+	writel(time, pdata->ioaddr + SRTC_LPSCMR);
+	rtc_write_sync_lp(pdata->ioaddr);
+	return 0;
+}
+
+/*!
+ * This function reads the current alarm value into the passed in \b alrm
+ * argument. It updates the \b alrm's pending field value based on the whether
+ * an alarm interrupt occurs or not.
+ *
+ * @param  alrm         contains the RTC alarm value upon return
+ *
+ * @return  0 if successful; non-zero otherwise.
+ */
+static int mxc_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
+{
+	struct rtc_drv_data *pdata = dev_get_drvdata(dev);
+	void __iomem *ioaddr = pdata->ioaddr;
+
+	rtc_time_to_tm(__raw_readl(ioaddr + SRTC_LPSAR), &alrm->time);
+	alrm->pending =
+	    ((__raw_readl(ioaddr + SRTC_LPSR) & SRTC_LPSR_ALP) != 0) ? 1 : 0;

if/else would be much easier to read here.

+
+	return 0;
+}
+
+static int mxc_rtc_alarm_irq_enable(struct device *dev, unsigned int enable)
+{
+	struct rtc_drv_data *pdata = dev_get_drvdata(dev);
+	unsigned long lock_flags = 0;
+	u32 lp_cr;
+
+	spin_lock_irqsave(&rtc_lock, lock_flags);
+	lp_cr = __raw_readl(pdata->ioaddr + SRTC_LPCR);
+
+	if (enable)
+		lp_cr |= (SRTC_LPCR_ALP | SRTC_LPCR_WAE);
+	else
+		lp_cr &= ~(SRTC_LPCR_ALP | SRTC_LPCR_WAE);
+
+	__raw_writel(lp_cr, pdata->ioaddr + SRTC_LPCR);
+	spin_unlock_irqrestore(&rtc_lock, lock_flags);
+	return 0;
+}
+
+/*!
+ * This function sets the RTC alarm based on passed in alrm.
+ *
+ * @param  alrm         the alarm value to be set in the RTC
+ *
+ * @return  0 if successful; non-zero otherwise.
+ */
+static int mxc_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
+{
+	struct rtc_drv_data *pdata = dev_get_drvdata(dev);
+	void __iomem *ioaddr = pdata->ioaddr;
+	int ret;
+
+	ret = rtc_update_alarm(dev, &alrm->time);
+	if (!ret)
+		mxc_rtc_alarm_irq_enable(dev, alrm->enabled);
+
+	rtc_write_sync_lp(ioaddr);
+	return ret;
+}
+
+/*!
+ * The RTC driver structure
+ */
+static const struct rtc_class_ops mxc_rtc_ops = {
+	.read_time = mxc_rtc_read_time,
+	.set_time = mxc_rtc_set_time,
+	.read_alarm = mxc_rtc_read_alarm,
+	.set_alarm = mxc_rtc_set_alarm,
+	.alarm_irq_enable = mxc_rtc_alarm_irq_enable,
+};
+
+/*! MXC RTC Power management control */
+static int mxc_rtc_probe(struct platform_device *pdev)
+{
+	struct timespec tv;
+	struct resource *res;
+	struct rtc_drv_data *pdata = NULL;

No need to initialize.

+	void __iomem *ioaddr;
+	int ret = 0;
+
+	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res)
+		return -ENODEV;
+
+	pdata->ioaddr = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(pdata->ioaddr))
+		return PTR_ERR(pdata->ioaddr);
+
+	ioaddr = pdata->ioaddr;
+
+	pdata->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(pdata->clk)) {
+		dev_err(&pdev->dev, "unable to get rtc clock!\n");
+		return PTR_ERR(pdata->clk);
+	}
+	ret = clk_prepare_enable(pdata->clk);
+	if (ret)
+		return ret;
+
+	/* Configure and enable the RTC */
+	pdata->irq = platform_get_irq(pdev, 0);
+	if (pdata->irq >= 0
+	    && devm_request_irq(&pdev->dev, pdata->irq, mxc_rtc_interrupt,
+				IRQF_SHARED, pdev->name, pdev) < 0) {
+		dev_warn(&pdev->dev, "interrupt not available.\n");
+		pdata->irq = -1;
+	}

I would just make the interrupt mandatory. I see no valid reason why it
shouldn't be available and it makes your driver logic simpler when you
can just assume it's available.

+
+	if (pdata->irq >= 0)
+		device_init_wakeup(&pdev->dev, 1);
+
+	/* initialize glitch detect */
+	__raw_writel(SRTC_LPPDR_INIT, ioaddr + SRTC_LPPDR);
+	udelay(100);
+
+	/* clear lp interrupt status */
+	__raw_writel(0xFFFFFFFF, ioaddr + SRTC_LPSR);
+	udelay(100);
+
+	/* move out of init state */
+	__raw_writel((SRTC_LPCR_IE | SRTC_LPCR_NSA), ioaddr + SRTC_LPCR);
+
+	udelay(100);
+
+	while ((__raw_readl(ioaddr + SRTC_LPSR) & SRTC_LPSR_IES) == 0)
+		;
+
+	/* move out of non-valid state */
+	__raw_writel((SRTC_LPCR_IE | SRTC_LPCR_NVE | SRTC_LPCR_NSA |
+		      SRTC_LPCR_EN_LP), ioaddr + SRTC_LPCR);
+
+	udelay(100);

Where do all these udelay() come from? Are they really needed?

+
+	while ((__raw_readl(ioaddr + SRTC_LPSR) & SRTC_LPSR_NVES) == 0)
+		;
+
+	__raw_writel(0xFFFFFFFF, ioaddr + SRTC_LPSR);
+	udelay(100);
+
+	platform_set_drvdata(pdev, pdata);
+	pdata->rtc =
+	    devm_rtc_device_register(&pdev->dev, pdev->name, &mxc_rtc_ops,
+				     THIS_MODULE);
+	if (IS_ERR(pdata->rtc)) {
+		ret = PTR_ERR(pdata->rtc);
+		goto exit_put_clk;
+	}
+
+	tv.tv_nsec = 0;
+	tv.tv_sec = __raw_readl(ioaddr + SRTC_LPSCMR);

Why this? tv is set but unused.

+
+	/* By default, devices should wakeup if they can */
+	/* So srtc is set as "should wakeup" as it can */
+	device_init_wakeup(&pdev->dev, 1);

Why is this done twice, one time only when we have a valid irq? This
looks fishy.

+
+	return ret;
+
+exit_put_clk:
+	clk_disable_unprepare(pdata->clk);
+	return ret;
+}
+
+static int __exit mxc_rtc_remove(struct platform_device *pdev)
+{
+	struct rtc_drv_data *pdata = platform_get_drvdata(pdev);
+
+	clk_disable_unprepare(pdata->clk);
+	return 0;
+}
+
+/*!
+ * This function is called to save the system time delta relative to
+ * the MXC RTC when enterring a low power state. This time delta is
+ * then used on resume to adjust the system time to account for time
+ * loss while suspended.
+ *
+ * @param   pdev  not used

Not true.

+ * @param   state Power state to enter.
+ *
+ * @return  The function always returns 0.
+ */
+static int mxc_rtc_suspend(struct platform_device *pdev, pm_message_t state)
+{
+	struct rtc_drv_data *pdata = platform_get_drvdata(pdev);
+
+	if (device_may_wakeup(&pdev->dev))
+		enable_irq_wake(pdata->irq);
+
+	return 0;
+}
+
+/*!
+ * This function is called to correct the system time based on the
+ * current MXC RTC time relative to the time delta saved during
+ * suspend.
+ *
+ * @param   pdev  not used

Not true.

+ *
+ * @return  The function always returns 0.
+ */
+static int mxc_rtc_resume(struct platform_device *pdev)
+{
+	struct rtc_drv_data *pdata = platform_get_drvdata(pdev);
+
+	if (device_may_wakeup(&pdev->dev))
+		disable_irq_wake(pdata->irq);
+
+	return 0;
+}
+
+static const struct of_device_id mxc_ids[] = {
+	{.compatible = "fsl,imx53-rtc",},
+	{}
+};
+
+/*!
+ * Contains pointers to the power management callback functions.

True, but doesn't contain any useful information.

+ */
+static struct platform_driver mxc_rtc_driver = {
+	.driver = {
+		   .name = "mxc_rtc_v8",
+		   .of_match_table = mxc_ids,
+		   },

Indentation is broken here.

+	.probe = mxc_rtc_probe,
+	.remove = mxc_rtc_remove,
+	.suspend = mxc_rtc_suspend,
+	.resume = mxc_rtc_resume,
+};
+
+module_platform_driver(mxc_rtc_driver);
+
+MODULE_AUTHOR("Freescale Semiconductor, Inc.");
+MODULE_DESCRIPTION("Realtime Clock Driver (RTC)");
+MODULE_LICENSE("GPL");

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html