[PATCH v3 1/5] rtc: mxc_rtc: Driver rework

[PATCH v3 1/5] rtc: mxc_rtc: Driver rework

[Alexander Shiyan]

This patch rework mxc_rtc driver.
Major changes have been made:
- Added second clock support (optional) which permit module functionality.
- Implemented support for periodic interrupts.
- Some code have been optimized.

Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
---
 drivers/rtc/rtc-mxc.c | 278 +++++++++++++++++++++-----------------------------
 1 file changed, 119 insertions(+), 159 deletions(-)

diff --git a/drivers/rtc/rtc-mxc.c b/drivers/rtc/rtc-mxc.c
index ab87bac..8ec47c8 100644
--- a/drivers/rtc/rtc-mxc.c
+++ b/drivers/rtc/rtc-mxc.c
@@ -12,7 +12,6 @@
 #include <linux/io.h>
 #include <linux/rtc.h>
 #include <linux/module.h>
-#include <linux/slab.h>
 #include <linux/interrupt.h>
 #include <linux/platform_device.h>
 #include <linux/clk.h>
@@ -39,20 +38,6 @@
 
 #define RTC_ENABLE_BIT  (1 << 7)
 
-#define MAX_PIE_NUM     9
-#define MAX_PIE_FREQ    512
-static const u32 PIE_BIT_DEF[MAX_PIE_NUM][2] = {
-	{ 2,		RTC_2HZ_BIT },
-	{ 4,		RTC_SAM0_BIT },
-	{ 8,		RTC_SAM1_BIT },
-	{ 16,		RTC_SAM2_BIT },
-	{ 32,		RTC_SAM3_BIT },
-	{ 64,		RTC_SAM4_BIT },
-	{ 128,		RTC_SAM5_BIT },
-	{ 256,		RTC_SAM6_BIT },
-	{ MAX_PIE_FREQ,	RTC_SAM7_BIT },
-};
-
 #define MXC_RTC_TIME	0
 #define MXC_RTC_ALARM	1
 
@@ -66,9 +51,6 @@ static const u32 PIE_BIT_DEF[MAX_PIE_NUM][2] = {
 #define RTC_STPWCH	0x1C	/*  32bit rtc stopwatch min reg */
 #define RTC_DAYR	0x20	/*  32bit rtc days counter reg */
 #define RTC_DAYALARM	0x24	/*  32bit rtc day alarm reg */
-#define RTC_TEST1	0x28	/*  32bit rtc test reg 1 */
-#define RTC_TEST2	0x2C	/*  32bit rtc test reg 2 */
-#define RTC_TEST3	0x30	/*  32bit rtc test reg 3 */
 
 enum imx_rtc_type {
 	IMX1_RTC,
@@ -79,29 +61,12 @@ struct rtc_plat_data {
 	struct rtc_device *rtc;
 	void __iomem *ioaddr;
 	int irq;
-	struct clk *clk;
-	struct rtc_time g_rtc_alarm;
+	struct rtc_class_ops	rtc_ops;
+	struct clk		*clk_rtc;
+	struct clk		*clk_ipg;
 	enum imx_rtc_type devtype;
 };
 
-static struct platform_device_id imx_rtc_devtype[] = {
-	{
-		.name = "imx1-rtc",
-		.driver_data = IMX1_RTC,
-	}, {
-		.name = "imx21-rtc",
-		.driver_data = IMX21_RTC,
-	}, {
-		/* sentinel */
-	}
-};
-MODULE_DEVICE_TABLE(platform, imx_rtc_devtype);
-
-static inline int is_imx1_rtc(struct rtc_plat_data *data)
-{
-	return data->devtype == IMX1_RTC;
-}
-
 /*
  * This function is used to obtain the RTC time or the alarm value in
  * second.
@@ -110,20 +75,16 @@ static u32 get_alarm_or_time(struct device *dev, int time_alarm)
 {
 	struct platform_device *pdev = to_platform_device(dev);
 	struct rtc_plat_data *pdata = platform_get_drvdata(pdev);
-	void __iomem *ioaddr = pdata->ioaddr;
-	u32 day = 0, hr = 0, min = 0, sec = 0, hr_min = 0;
-
-	switch (time_alarm) {
-	case MXC_RTC_TIME:
-		day = readw(ioaddr + RTC_DAYR);
-		hr_min = readw(ioaddr + RTC_HOURMIN);
-		sec = readw(ioaddr + RTC_SECOND);
-		break;
-	case MXC_RTC_ALARM:
-		day = readw(ioaddr + RTC_DAYALARM);
-		hr_min = readw(ioaddr + RTC_ALRM_HM) & 0xffff;
-		sec = readw(ioaddr + RTC_ALRM_SEC);
-		break;
+	u32 day, hr, min, sec, hr_min;
+
+	if (time_alarm == MXC_RTC_TIME) {
+		day = readw(pdata->ioaddr + RTC_DAYR);
+		hr_min = readw(pdata->ioaddr + RTC_HOURMIN);
+		sec = readw(pdata->ioaddr + RTC_SECOND);
+	} else {
+		day = readw(pdata->ioaddr + RTC_DAYALARM);
+		hr_min = readw(pdata->ioaddr + RTC_ALRM_HM);
+		sec = readw(pdata->ioaddr + RTC_ALRM_SEC);
 	}
 
 	hr = hr_min >> 8;
@@ -140,7 +101,6 @@ static void set_alarm_or_time(struct device *dev, int time_alarm, u32 time)
 	u32 day, hr, min, sec, temp;
 	struct platform_device *pdev = to_platform_device(dev);
 	struct rtc_plat_data *pdata = platform_get_drvdata(pdev);
-	void __iomem *ioaddr = pdata->ioaddr;
 
 	day = time / 86400;
 	time -= day * 86400;
@@ -155,17 +115,14 @@ static void set_alarm_or_time(struct device *dev, int time_alarm, u32 time)
 
 	temp = (hr << 8) + min;
 
-	switch (time_alarm) {
-	case MXC_RTC_TIME:
-		writew(day, ioaddr + RTC_DAYR);
-		writew(sec, ioaddr + RTC_SECOND);
-		writew(temp, ioaddr + RTC_HOURMIN);
-		break;
-	case MXC_RTC_ALARM:
-		writew(day, ioaddr + RTC_DAYALARM);
-		writew(sec, ioaddr + RTC_ALRM_SEC);
-		writew(temp, ioaddr + RTC_ALRM_HM);
-		break;
+	if (time_alarm == MXC_RTC_TIME) {
+		writew(day, pdata->ioaddr + RTC_DAYR);
+		writew(sec, pdata->ioaddr + RTC_SECOND);
+		writew(temp, pdata->ioaddr + RTC_HOURMIN);
+	} else {
+		writew(day, pdata->ioaddr + RTC_DAYALARM);
+		writew(sec, pdata->ioaddr + RTC_ALRM_SEC);
+		writew(temp, pdata->ioaddr + RTC_ALRM_HM);
 	}
 }
 
@@ -179,7 +136,6 @@ static int rtc_update_alarm(struct device *dev, struct rtc_time *alrm)
 	unsigned long now, time;
 	struct platform_device *pdev = to_platform_device(dev);
 	struct rtc_plat_data *pdata = platform_get_drvdata(pdev);
-	void __iomem *ioaddr = pdata->ioaddr;
 
 	now = get_alarm_or_time(dev, MXC_RTC_TIME);
 	rtc_time_to_tm(now, &now_tm);
@@ -191,8 +147,9 @@ static int rtc_update_alarm(struct device *dev, struct rtc_time *alrm)
 	alarm_tm.tm_sec = alrm->tm_sec;
 	rtc_tm_to_time(&alarm_tm, &time);
 
-	/* clear all the interrupt status bits */
-	writew(readw(ioaddr + RTC_RTCISR), ioaddr + RTC_RTCISR);
+	/* clear interrupt status bit */
+	writew(RTC_ALM_BIT, pdata->ioaddr + RTC_RTCISR);
+
 	set_alarm_or_time(dev, MXC_RTC_ALARM, time);
 
 	return 0;
@@ -203,18 +160,19 @@ static void mxc_rtc_irq_enable(struct device *dev, unsigned int bit,
 {
 	struct platform_device *pdev = to_platform_device(dev);
 	struct rtc_plat_data *pdata = platform_get_drvdata(pdev);
-	void __iomem *ioaddr = pdata->ioaddr;
 	u32 reg;
 
 	spin_lock_irq(&pdata->rtc->irq_lock);
-	reg = readw(ioaddr + RTC_RTCIENR);
 
-	if (enabled)
+	reg = readw(pdata->ioaddr + RTC_RTCIENR);
+	if (enabled) {
 		reg |= bit;
-	else
+		/* Clear interrupt status */
+		writew(reg, pdata->ioaddr + RTC_RTCISR);
+	} else
 		reg &= ~bit;
+	writew(reg, pdata->ioaddr + RTC_RTCIENR);
 
-	writew(reg, ioaddr + RTC_RTCIENR);
 	spin_unlock_irq(&pdata->rtc->irq_lock);
 }
 
@@ -252,30 +210,42 @@ static irqreturn_t mxc_rtc_interrupt(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
-/*
- * Clear all interrupts and release the IRQ
- */
-static void mxc_rtc_release(struct device *dev)
+static int mxc_rtc_open(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
 	struct rtc_plat_data *pdata = platform_get_drvdata(pdev);
-	void __iomem *ioaddr = pdata->ioaddr;
 
-	spin_lock_irq(&pdata->rtc->irq_lock);
+	if (pdata->irq >= 0) {
+		unsigned long rate = clk_get_rate(pdata->clk_rtc);
 
-	/* Disable all rtc interrupts */
-	writew(0, ioaddr + RTC_RTCIENR);
+		pdata->rtc->max_user_freq = rate / 64;
+		rtc_irq_set_freq(pdata->rtc, NULL, rate / 64);
+		mxc_rtc_irq_enable(&pdev->dev, RTC_1HZ_BIT | RTC_SAM7_BIT, 1);
+	}
 
-	/* Clear all interrupt status */
-	writew(0xffffffff, ioaddr + RTC_RTCISR);
+	return 0;
+}
 
-	spin_unlock_irq(&pdata->rtc->irq_lock);
+static void mxc_rtc_release(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct rtc_plat_data *pdata = platform_get_drvdata(pdev);
+
+	if (pdata->irq >= 0)
+		mxc_rtc_irq_enable(&pdev->dev, RTC_1HZ_BIT | RTC_SAM7_BIT, 0);
 }
 
 static int mxc_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled)
 {
-	mxc_rtc_irq_enable(dev, RTC_ALM_BIT, enabled);
-	return 0;
+	struct platform_device *pdev = to_platform_device(dev);
+	struct rtc_plat_data *pdata = platform_get_drvdata(pdev);
+
+	if (pdata->irq >= 0) {
+		mxc_rtc_irq_enable(dev, RTC_ALM_BIT, enabled);
+		return 0;
+	}
+
+	return -EINVAL;
 }
 
 /*
@@ -306,7 +276,7 @@ static int mxc_rtc_set_mmss(struct device *dev, unsigned long time)
 	/*
 	 * TTC_DAYR register is 9-bit in MX1 SoC, save time and day of year only
 	 */
-	if (is_imx1_rtc(pdata)) {
+	if (pdata->devtype == IMX1_RTC) {
 		struct rtc_time tm;
 
 		rtc_time_to_tm(time, &tm);
@@ -331,10 +301,9 @@ static int mxc_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
 {
 	struct platform_device *pdev = to_platform_device(dev);
 	struct rtc_plat_data *pdata = platform_get_drvdata(pdev);
-	void __iomem *ioaddr = pdata->ioaddr;
 
 	rtc_time_to_tm(get_alarm_or_time(dev, MXC_RTC_ALARM), &alrm->time);
-	alrm->pending = ((readw(ioaddr + RTC_RTCISR) & RTC_ALM_BIT)) ? 1 : 0;
+	alrm->pending = !!(readw(pdata->ioaddr + RTC_RTCISR) & RTC_ALM_BIT);
 
 	return 0;
 }
@@ -349,61 +318,41 @@ static int mxc_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
 	int ret;
 
 	ret = rtc_update_alarm(dev, &alrm->time);
-	if (ret)
-		return ret;
+	if ((pdata->irq >= 0) && !ret)
+		mxc_rtc_irq_enable(dev, RTC_ALM_BIT, alrm->enabled);
 
-	memcpy(&pdata->g_rtc_alarm, &alrm->time, sizeof(struct rtc_time));
-	mxc_rtc_irq_enable(dev, RTC_ALM_BIT, alrm->enabled);
-
-	return 0;
+	return ret;
 }
 
-/* RTC layer */
-static struct rtc_class_ops mxc_rtc_ops = {
-	.release		= mxc_rtc_release,
-	.read_time		= mxc_rtc_read_time,
-	.set_mmss		= mxc_rtc_set_mmss,
-	.read_alarm		= mxc_rtc_read_alarm,
-	.set_alarm		= mxc_rtc_set_alarm,
-	.alarm_irq_enable	= mxc_rtc_alarm_irq_enable,
-};
-
 static int mxc_rtc_probe(struct platform_device *pdev)
 {
+	struct rtc_plat_data *pdata;
 	struct resource *res;
-	struct rtc_device *rtc;
-	struct rtc_plat_data *pdata = NULL;
 	u32 reg;
 	unsigned long rate;
 	int ret;
 
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!res)
-		return -ENODEV;
-
 	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
 	if (!pdata)
 		return -ENOMEM;
 
-	pdata->devtype = pdev->id_entry->driver_data;
-
-	if (!devm_request_mem_region(&pdev->dev, res->start,
-				     resource_size(res), pdev->name))
-		return -EBUSY;
-
-	pdata->ioaddr = devm_ioremap(&pdev->dev, res->start,
-				     resource_size(res));
-
-	pdata->clk = devm_clk_get(&pdev->dev, NULL);
-	if (IS_ERR(pdata->clk)) {
-		dev_err(&pdev->dev, "unable to get clock!\n");
-		ret = PTR_ERR(pdata->clk);
-		goto exit_free_pdata;
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	pdata->ioaddr = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(pdata->ioaddr))
+		return PTR_ERR(pdata->ioaddr);
+
+	pdata->clk_rtc = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(pdata->clk_rtc)) {
+		dev_err(&pdev->dev, "Unable to get clock!\n");
+		return PTR_ERR(pdata->clk_rtc);
 	}
 
-	clk_prepare_enable(pdata->clk);
-	rate = clk_get_rate(pdata->clk);
+	pdata->clk_ipg = devm_clk_get(&pdev->dev, "ipg");
+	if (!IS_ERR(pdata->clk_ipg))
+		clk_prepare_enable(pdata->clk_ipg);
+	clk_prepare_enable(pdata->clk_rtc);
 
+	rate = clk_get_rate(pdata->clk_rtc);
 	if (rate == 32768)
 		reg = RTC_INPUT_CLK_32768HZ;
 	else if (rate == 32000)
@@ -411,49 +360,55 @@ static int mxc_rtc_probe(struct platform_device *pdev)
 	else if (rate == 38400)
 		reg = RTC_INPUT_CLK_38400HZ;
 	else {
-		dev_err(&pdev->dev, "rtc clock is not valid (%lu)\n", rate);
+		dev_err(&pdev->dev, "RTC clock is not valid (%lu)\n", rate);
 		ret = -EINVAL;
 		goto exit_put_clk;
 	}
 
-	reg |= RTC_ENABLE_BIT;
-	writew(reg, (pdata->ioaddr + RTC_RTCCTL));
-	if (((readw(pdata->ioaddr + RTC_RTCCTL)) & RTC_ENABLE_BIT) == 0) {
-		dev_err(&pdev->dev, "hardware module can't be enabled!\n");
+	writew(reg | RTC_ENABLE_BIT, pdata->ioaddr + RTC_RTCCTL);
+	if (!(readw(pdata->ioaddr + RTC_RTCCTL) & RTC_ENABLE_BIT)) {
+		dev_err(&pdev->dev, "Hardware module can't be enabled!\n");
 		ret = -EIO;
 		goto exit_put_clk;
 	}
 
-	platform_set_drvdata(pdev, pdata);
+	/* Disable all interrupts */
+	writew(0, pdata->ioaddr + RTC_RTCIENR);
 
-	/* Configure and enable the RTC */
-	pdata->irq = platform_get_irq(pdev, 0);
+	pdata->devtype = pdev->id_entry->driver_data;
+	platform_set_drvdata(pdev, pdata);
 
-	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;
+	pdata->rtc_ops.open		= mxc_rtc_open;
+	pdata->rtc_ops.release		= mxc_rtc_release;
+	pdata->rtc_ops.read_time	= mxc_rtc_read_time;
+	pdata->rtc_ops.set_mmss		= mxc_rtc_set_mmss;
+	pdata->rtc_ops.read_alarm	= mxc_rtc_read_alarm;
+	pdata->rtc_ops.set_alarm	= mxc_rtc_set_alarm;
+	pdata->rtc_ops.alarm_irq_enable	= mxc_rtc_alarm_irq_enable;
+
+	pdata->rtc = devm_rtc_device_register(&pdev->dev, pdev->name,
+					      &pdata->rtc_ops, THIS_MODULE);
+	if (IS_ERR(pdata->rtc)) {
+		ret = PTR_ERR(pdata->rtc);
+		goto exit_put_clk;
 	}
 
+	pdata->irq = platform_get_irq(pdev, 0);
 	if (pdata->irq >= 0)
-		device_init_wakeup(&pdev->dev, 1);
+		if (devm_request_irq(&pdev->dev, pdata->irq, mxc_rtc_interrupt,
+				     IRQF_SHARED, pdev->name, pdev) < 0) {
+			dev_warn(&pdev->dev, "Not using interrupt\n");
+			pdata->irq = -1;
+		}
 
-	rtc = devm_rtc_device_register(&pdev->dev, pdev->name, &mxc_rtc_ops,
-				  THIS_MODULE);
-	if (IS_ERR(rtc)) {
-		ret = PTR_ERR(rtc);
-		goto exit_put_clk;
-	}
-
-	pdata->rtc = rtc;
+	device_init_wakeup(&pdev->dev, pdata->irq >= 0);
 
 	return 0;
 
 exit_put_clk:
-	clk_disable_unprepare(pdata->clk);
-
-exit_free_pdata:
+	clk_disable_unprepare(pdata->clk_rtc);
+	if (!IS_ERR(pdata->clk_ipg))
+		clk_disable_unprepare(pdata->clk_ipg);
 
 	return ret;
 }
@@ -462,13 +417,14 @@ static int mxc_rtc_remove(struct platform_device *pdev)
 {
 	struct rtc_plat_data *pdata = platform_get_drvdata(pdev);
 
-	clk_disable_unprepare(pdata->clk);
+	clk_disable_unprepare(pdata->clk_rtc);
+	if (!IS_ERR(pdata->clk_ipg))
+		clk_disable_unprepare(pdata->clk_ipg);
 
 	return 0;
 }
 
-#ifdef CONFIG_PM_SLEEP
-static int mxc_rtc_suspend(struct device *dev)
+static int __maybe_unused mxc_rtc_suspend(struct device *dev)
 {
 	struct rtc_plat_data *pdata = dev_get_drvdata(dev);
 
@@ -478,7 +434,7 @@ static int mxc_rtc_suspend(struct device *dev)
 	return 0;
 }
 
-static int mxc_rtc_resume(struct device *dev)
+static int __maybe_unused mxc_rtc_resume(struct device *dev)
 {
 	struct rtc_plat_data *pdata = dev_get_drvdata(dev);
 
@@ -487,24 +443,28 @@ static int mxc_rtc_resume(struct device *dev)
 
 	return 0;
 }
-#endif
 
 static SIMPLE_DEV_PM_OPS(mxc_rtc_pm_ops, mxc_rtc_suspend, mxc_rtc_resume);
 
+static const struct platform_device_id mxc_rtc_id_table[] = {
+	{ .name = "imx1-rtc", .driver_data = IMX1_RTC, },
+	{ .name = "imx21-rtc", .driver_data = IMX21_RTC, },
+	{ }
+};
+MODULE_DEVICE_TABLE(platform, mxc_rtc_id_table);
+
 static struct platform_driver mxc_rtc_driver = {
 	.driver = {
 		   .name	= "mxc_rtc",
 		   .pm		= &mxc_rtc_pm_ops,
 		   .owner	= THIS_MODULE,
 	},
-	.id_table = imx_rtc_devtype,
+	.id_table = mxc_rtc_id_table,
 	.probe = mxc_rtc_probe,
 	.remove = mxc_rtc_remove,
 };
-
 module_platform_driver(mxc_rtc_driver)
 
 MODULE_AUTHOR("Daniel Mack <daniel@caiaq.de>");
 MODULE_DESCRIPTION("RTC driver for Freescale MXC");
 MODULE_LICENSE("GPL");
-
-- 
1.8.1.5

[PATCH v3 2/5] rtc: mxc_rtc: Cleanup code

[Alexander Shiyan]

This patch provide cleanup mxc_rtc driver code (spaces to tabs).
No functional changes.

Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
---
 drivers/rtc/rtc-mxc.c | 52 ++++++++++++++++++++++++++-------------------------
 1 file changed, 27 insertions(+), 25 deletions(-)

diff --git a/drivers/rtc/rtc-mxc.c b/drivers/rtc/rtc-mxc.c
index 8ec47c8..f333581 100644
--- a/drivers/rtc/rtc-mxc.c
+++ b/drivers/rtc/rtc-mxc.c
@@ -20,23 +20,23 @@
 #define RTC_INPUT_CLK_32000HZ	(0x01 << 5)
 #define RTC_INPUT_CLK_38400HZ	(0x02 << 5)
 
-#define RTC_SW_BIT      (1 << 0)
-#define RTC_ALM_BIT     (1 << 2)
-#define RTC_1HZ_BIT     (1 << 4)
-#define RTC_2HZ_BIT     (1 << 7)
-#define RTC_SAM0_BIT    (1 << 8)
-#define RTC_SAM1_BIT    (1 << 9)
-#define RTC_SAM2_BIT    (1 << 10)
-#define RTC_SAM3_BIT    (1 << 11)
-#define RTC_SAM4_BIT    (1 << 12)
-#define RTC_SAM5_BIT    (1 << 13)
-#define RTC_SAM6_BIT    (1 << 14)
-#define RTC_SAM7_BIT    (1 << 15)
-#define PIT_ALL_ON      (RTC_2HZ_BIT | RTC_SAM0_BIT | RTC_SAM1_BIT | \
+#define RTC_SW_BIT	(1 << 0)
+#define RTC_ALM_BIT	(1 << 2)
+#define RTC_1HZ_BIT	(1 << 4)
+#define RTC_2HZ_BIT	(1 << 7)
+#define RTC_SAM0_BIT	(1 << 8)
+#define RTC_SAM1_BIT	(1 << 9)
+#define RTC_SAM2_BIT	(1 << 10)
+#define RTC_SAM3_BIT	(1 << 11)
+#define RTC_SAM4_BIT	(1 << 12)
+#define RTC_SAM5_BIT	(1 << 13)
+#define RTC_SAM6_BIT	(1 << 14)
+#define RTC_SAM7_BIT	(1 << 15)
+#define PIT_ALL_ON	(RTC_2HZ_BIT | RTC_SAM0_BIT | RTC_SAM1_BIT | \
 			 RTC_SAM2_BIT | RTC_SAM3_BIT | RTC_SAM4_BIT | \
 			 RTC_SAM5_BIT | RTC_SAM6_BIT | RTC_SAM7_BIT)
 
-#define RTC_ENABLE_BIT  (1 << 7)
+#define RTC_ENABLE_BIT	(1 << 7)
 
 #define MXC_RTC_TIME	0
 #define MXC_RTC_ALARM	1
@@ -58,13 +58,13 @@ enum imx_rtc_type {
 };
 
 struct rtc_plat_data {
-	struct rtc_device *rtc;
-	void __iomem *ioaddr;
-	int irq;
+	struct rtc_device	*rtc;
+	void __iomem		*ioaddr;
+	int			irq;
 	struct rtc_class_ops	rtc_ops;
 	struct clk		*clk_rtc;
 	struct clk		*clk_ipg;
-	enum imx_rtc_type devtype;
+	enum imx_rtc_type	devtype;
 };
 
 /*
@@ -156,7 +156,7 @@ static int rtc_update_alarm(struct device *dev, struct rtc_time *alrm)
 }
 
 static void mxc_rtc_irq_enable(struct device *dev, unsigned int bit,
-				unsigned int enabled)
+			       unsigned int enabled)
 {
 	struct platform_device *pdev = to_platform_device(dev);
 	struct rtc_plat_data *pdata = platform_get_drvdata(pdev);
@@ -187,6 +187,7 @@ static irqreturn_t mxc_rtc_interrupt(int irq, void *dev_id)
 	u32 events = 0;
 
 	spin_lock_irqsave(&pdata->rtc->irq_lock, flags);
+
 	status = readw(ioaddr + RTC_RTCISR) & readw(ioaddr + RTC_RTCIENR);
 	/* clear interrupt sources */
 	writew(status, ioaddr + RTC_RTCISR);
@@ -205,6 +206,7 @@ static irqreturn_t mxc_rtc_interrupt(int irq, void *dev_id)
 		events |= (RTC_PF | RTC_IRQF);
 
 	rtc_update_irq(pdata->rtc, 1, events);
+
 	spin_unlock_irqrestore(&pdata->rtc->irq_lock, flags);
 
 	return IRQ_HANDLED;
@@ -454,14 +456,14 @@ static const struct platform_device_id mxc_rtc_id_table[] = {
 MODULE_DEVICE_TABLE(platform, mxc_rtc_id_table);
 
 static struct platform_driver mxc_rtc_driver = {
-	.driver = {
-		   .name	= "mxc_rtc",
-		   .pm		= &mxc_rtc_pm_ops,
-		   .owner	= THIS_MODULE,
+	.driver	= {
+		.name	= "mxc_rtc",
+		.owner	= THIS_MODULE,
+		.pm	= &mxc_rtc_pm_ops,
 	},
 	.id_table = mxc_rtc_id_table,
-	.probe = mxc_rtc_probe,
-	.remove = mxc_rtc_remove,
+	.probe	= mxc_rtc_probe,
+	.remove	= mxc_rtc_remove,
 };
 module_platform_driver(mxc_rtc_driver)
 
-- 
1.8.1.5

[PATCH v3 4/5] rtc: mxc_rtc: Add DT support

[Alexander Shiyan]

Add DT bindings for the mxc_rtc driver and read the device
configuration from the DT node at probe time if available.

Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
---
 Documentation/devicetree/bindings/rtc/mxc-rtc.txt | 21 ++++++++++++++++++
 drivers/rtc/rtc-mxc.c                             | 27 +++++++++++++++++++----
 2 files changed, 44 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/rtc/mxc-rtc.txt

diff --git a/Documentation/devicetree/bindings/rtc/mxc-rtc.txt b/Documentation/devicetree/bindings/rtc/mxc-rtc.txt
new file mode 100644
index 0000000..ad8b0b3
--- /dev/null
+++ b/Documentation/devicetree/bindings/rtc/mxc-rtc.txt
@@ -0,0 +1,21 @@
+* i.MX Real Time Clock controller
+
+This binding supports the following chips: i.MX1, i.MX27, i.MX31, i.MX35
+
+Required properties:
+- compatible: should be: "fsl,imx1-rtc" or "fsl,imx21-rtc"
+- reg: physical base address of the controller and length of memory mapped
+  region.
+- interrupts: rtc alarm interrupt
+- clocks : Should contain the rtc and ipg clocks, in the order
+  determined by the clock-names property.
+
+Example:
+
+rtc at 10007000 {
+	compatible = "fsl,imx27-rtc", "fsl,imx21-rtc";
+	reg = <0x10007000 0x1000>;
+	interrupts = <22>;
+	clocks = <&clks 2>, <&clks 33>;
+	clock-names = "rtc", "ipg";
+};
diff --git a/drivers/rtc/rtc-mxc.c b/drivers/rtc/rtc-mxc.c
index 80c6f1f..5079c44 100644
--- a/drivers/rtc/rtc-mxc.c
+++ b/drivers/rtc/rtc-mxc.c
@@ -13,6 +13,8 @@
 #include <linux/rtc.h>
 #include <linux/module.h>
 #include <linux/interrupt.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/clk.h>
 
@@ -377,7 +379,16 @@ static int mxc_rtc_probe(struct platform_device *pdev)
 	/* Disable all interrupts */
 	writew(0, pdata->ioaddr + RTC_RTCIENR);
 
-	pdata->devtype = pdev->id_entry->driver_data;
+	if (pdev->dev.of_node) {
+		struct platform_driver *pdrv = container_of(pdev->dev.driver,
+						struct platform_driver, driver);
+		const struct of_device_id *of_id =
+			of_match_device(pdrv->driver.of_match_table, &pdev->dev);
+
+		pdata->devtype = (enum imx_rtc_type)of_id->data;
+	} else
+		pdata->devtype = pdev->id_entry->driver_data;
+
 	platform_set_drvdata(pdev, pdata);
 
 	pdata->rtc_ops.open		= mxc_rtc_open;
@@ -448,6 +459,13 @@ static int __maybe_unused mxc_rtc_resume(struct device *dev)
 
 static SIMPLE_DEV_PM_OPS(mxc_rtc_pm_ops, mxc_rtc_suspend, mxc_rtc_resume);
 
+static const struct of_device_id mxc_rtc_dt_ids[] = {
+	{ .compatible = "fsl,imx1-rtc", .data = (void *)IMX1_RTC, },
+	{ .compatible = "fsl,imx21-rtc", .data = (void *)IMX21_RTC, },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, mxc_rtc_dt_ids);
+
 static const struct platform_device_id mxc_rtc_id_table[] = {
 	{ .name = "imx1-rtc", .driver_data = IMX1_RTC, },
 	{ .name = "imx21-rtc", .driver_data = IMX21_RTC, },
@@ -457,9 +475,10 @@ MODULE_DEVICE_TABLE(platform, mxc_rtc_id_table);
 
 static struct platform_driver mxc_rtc_driver = {
 	.driver	= {
-		.name	= "mxc_rtc",
-		.owner	= THIS_MODULE,
-		.pm	= &mxc_rtc_pm_ops,
+		.name		= "mxc_rtc",
+		.owner		= THIS_MODULE,
+		.of_match_table	= mxc_rtc_dt_ids,
+		.pm		= &mxc_rtc_pm_ops,
 	},
 	.id_table = mxc_rtc_id_table,
 	.probe	= mxc_rtc_probe,
-- 
1.8.1.5

[PATCH v3 3/5] ARM: imx: Using proper clocks for RTC driver

[Alexander Shiyan]

i.MX RTC driver requires 32k clock for time function and optional
clock for module itself. This patch fixes these clock names for the
driver and adds missing definitions.

Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
---
 arch/arm/mach-imx/clk-imx31.c | 3 ++-
 arch/arm/mach-imx/clk-imx35.c | 2 ++
 drivers/rtc/rtc-mxc.c         | 2 +-
 3 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-imx/clk-imx31.c b/arch/arm/mach-imx/clk-imx31.c
index b5b65f3..3f2676d 100644
--- a/arch/arm/mach-imx/clk-imx31.c
+++ b/arch/arm/mach-imx/clk-imx31.c
@@ -134,7 +134,8 @@ int __init mx31_clocks_init(unsigned long fref)
 	clk_register_clkdev(clk[cspi3_gate], NULL, "imx31-cspi.2");
 	clk_register_clkdev(clk[pwm_gate], "pwm", NULL);
 	clk_register_clkdev(clk[wdog_gate], NULL, "imx2-wdt.0");
-	clk_register_clkdev(clk[rtc_gate], NULL, "imx21-rtc");
+	clk_register_clkdev(clk[ckil], "rtc", "imx21-rtc");
+	clk_register_clkdev(clk[rtc_gate], "ipg", "imx21-rtc");
 	clk_register_clkdev(clk[epit1_gate], "epit", NULL);
 	clk_register_clkdev(clk[epit2_gate], "epit", NULL);
 	clk_register_clkdev(clk[nfc], NULL, "imx27-nand.0");
diff --git a/arch/arm/mach-imx/clk-imx35.c b/arch/arm/mach-imx/clk-imx35.c
index 2193c83..2e5af91 100644
--- a/arch/arm/mach-imx/clk-imx35.c
+++ b/arch/arm/mach-imx/clk-imx35.c
@@ -258,6 +258,8 @@ int __init mx35_clocks_init(void)
 	clk_register_clkdev(clk[nfc_div], NULL, "imx25-nand.0");
 	clk_register_clkdev(clk[csi_gate], NULL, "mx3-camera.0");
 	clk_register_clkdev(clk[admux_gate], "audmux", NULL);
+	clk_register_clkdev(clk[ckil], "rtc", "imx21-rtc");
+	clk_register_clkdev(clk[rtc_gate], "ipg", "imx21-rtc");
 
 	clk_prepare_enable(clk[spba_gate]);
 	clk_prepare_enable(clk[gpio1_gate]);
diff --git a/drivers/rtc/rtc-mxc.c b/drivers/rtc/rtc-mxc.c
index f333581..80c6f1f 100644
--- a/drivers/rtc/rtc-mxc.c
+++ b/drivers/rtc/rtc-mxc.c
@@ -343,7 +343,7 @@ static int mxc_rtc_probe(struct platform_device *pdev)
 	if (IS_ERR(pdata->ioaddr))
 		return PTR_ERR(pdata->ioaddr);
 
-	pdata->clk_rtc = devm_clk_get(&pdev->dev, NULL);
+	pdata->clk_rtc = devm_clk_get(&pdev->dev, "rtc");
 	if (IS_ERR(pdata->clk_rtc)) {
 		dev_err(&pdev->dev, "Unable to get clock!\n");
 		return PTR_ERR(pdata->clk_rtc);
-- 
1.8.1.5

[PATCH v3 5/5] ARM: imx: move i.MX1/21/27 RTC clk lookup into DT

[Alexander Shiyan]

Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
---
 arch/arm/mach-imx/clk-imx1.c  | 1 -
 arch/arm/mach-imx/clk-imx21.c | 1 -
 arch/arm/mach-imx/clk-imx27.c | 1 -
 3 files changed, 3 deletions(-)

diff --git a/arch/arm/mach-imx/clk-imx1.c b/arch/arm/mach-imx/clk-imx1.c
index 15f9d22..210f33e 100644
--- a/arch/arm/mach-imx/clk-imx1.c
+++ b/arch/arm/mach-imx/clk-imx1.c
@@ -106,7 +106,6 @@ int __init mx1_clocks_init(unsigned long fref)
 	clk_register_clkdev(clk[dummy], "ahb", "imx1-fb.0");
 	clk_register_clkdev(clk[hclk], "mshc", NULL);
 	clk_register_clkdev(clk[per3], "ssi", NULL);
-	clk_register_clkdev(clk[clk32], NULL, "imx1-rtc.0");
 	clk_register_clkdev(clk[clko], "clko", NULL);
 
 	mxc_timer_init(MX1_IO_ADDRESS(MX1_TIM1_BASE_ADDR), MX1_TIM1_INT);
diff --git a/arch/arm/mach-imx/clk-imx21.c b/arch/arm/mach-imx/clk-imx21.c
index d7ed660..a30537f 100644
--- a/arch/arm/mach-imx/clk-imx21.c
+++ b/arch/arm/mach-imx/clk-imx21.c
@@ -172,7 +172,6 @@ int __init mx21_clocks_init(unsigned long lref, unsigned long href)
 	clk_register_clkdev(clk[emma_gate], "emma", NULL);
 	clk_register_clkdev(clk[slcdc_gate], "slcdc", NULL);
 	clk_register_clkdev(clk[gpio_gate], "gpio", NULL);
-	clk_register_clkdev(clk[rtc_gate], "rtc", NULL);
 	clk_register_clkdev(clk[csi_hclk_gate], "csi", NULL);
 	clk_register_clkdev(clk[ssi1_gate], "ssi1", NULL);
 	clk_register_clkdev(clk[ssi2_gate], "ssi2", NULL);
diff --git a/arch/arm/mach-imx/clk-imx27.c b/arch/arm/mach-imx/clk-imx27.c
index c3cfa41..0aca839 100644
--- a/arch/arm/mach-imx/clk-imx27.c
+++ b/arch/arm/mach-imx/clk-imx27.c
@@ -283,7 +283,6 @@ int __init mx27_clocks_init(unsigned long fref)
 	clk_register_clkdev(clk[gpio_ipg_gate], "gpio", NULL);
 	clk_register_clkdev(clk[brom_ahb_gate], "brom", NULL);
 	clk_register_clkdev(clk[ata_ahb_gate], "ata", NULL);
-	clk_register_clkdev(clk[rtc_ipg_gate], NULL, "imx21-rtc");
 	clk_register_clkdev(clk[scc_ipg_gate], "scc", NULL);
 	clk_register_clkdev(clk[cpu_div], NULL, "cpufreq-cpu0.0");
 	clk_register_clkdev(clk[emi_ahb_gate], "emi_ahb" , NULL);
-- 
1.8.1.5

[PATCH v3 1/5] rtc: mxc_rtc: Driver rework

[Sascha Hauer]

Alexander,

On Sat, Jun 29, 2013 at 12:40:40PM +0400, Alexander Shiyan wrote:
> This patch rework mxc_rtc driver.
> Major changes have been made:
> - Added second clock support (optional) which permit module functionality.
> - Implemented support for periodic interrupts.
> - Some code have been optimized.

You won't get any further with this if you don't listen to comments.

We're at v3 and Still this patch combines many totally unrelated changes
in a single patch. This was noted by Shawn and more detailed by myself.

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 |

Re[2]: [PATCH v3 1/5] rtc: mxc_rtc: Driver rework

[Alexander Shiyan]

> Alexander,
> 
> On Sat, Jun 29, 2013 at 12:40:40PM +0400, Alexander Shiyan wrote:
> > This patch rework mxc_rtc driver.
> > Major changes have been made:
> > - Added second clock support (optional) which permit module functionality.
> > - Implemented support for periodic interrupts.
> > - Some code have been optimized.
> 
> You won't get any further with this if you don't listen to comments.
> 
> We're at v3 and Still this patch combines many totally unrelated changes
> in a single patch. This was noted by Shawn and more detailed by myself.

Where Sascha?
v3 is so different than v2. Can you inline your comments in v3?

---

Re[3]: [PATCH v3 1/5] rtc: mxc_rtc: Driver rework

[Alexander Shiyan]

> > On Sat, Jun 29, 2013 at 12:40:40PM +0400, Alexander Shiyan wrote:
> > > This patch rework mxc_rtc driver.
> > > Major changes have been made:
> > > - Added second clock support (optional) which permit module functionality.
> > > - Implemented support for periodic interrupts.
> > > - Some code have been optimized.
> > 
> > You won't get any further with this if you don't listen to comments.
> > 
> > We're at v3 and Still this patch combines many totally unrelated changes
> > in a single patch. This was noted by Shawn and more detailed by myself.
> 
> Where Sascha?
> v3 is so different than v2. Can you inline your comments in v3?

At the moment, the driver does not work at all, even for a boards which declared.
I do not understand the issue of making changes if we can correct this situation 
Please fix me.
Once again, now driver not work at all .....

---

[PATCH v3 1/5] rtc: mxc_rtc: Driver rework

[Sascha Hauer]

On Sun, Jun 30, 2013 at 01:33:35AM +0400, Alexander Shiyan wrote:
> > > On Sat, Jun 29, 2013 at 12:40:40PM +0400, Alexander Shiyan wrote:
> > > > This patch rework mxc_rtc driver.
> > > > Major changes have been made:
> > > > - Added second clock support (optional) which permit module functionality.
> > > > - Implemented support for periodic interrupts.
> > > > - Some code have been optimized.
> > > 
> > > You won't get any further with this if you don't listen to comments.
> > > 
> > > We're at v3 and Still this patch combines many totally unrelated changes
> > > in a single patch. This was noted by Shawn and more detailed by myself.
> > 
> > Where Sascha?
> > v3 is so different than v2. Can you inline your comments in v3?

I just did that.

> 
> At the moment, the driver does not work at all, even for a boards which declared.
> I do not understand the issue of making changes if we can correct this situation 
> Please fix me.
> Once again, now driver not work at all .....

If the driver is really so broken and ugly that it can't be fixed or
only with a huge amount of work, then your option would be to replace it
completely and clearly say why you think it's broken and why we need a
new driver.

I don't think that's the case. The driver has it's deficiencies, but
they can be fixed. Many of your changes are fine, but it's really 5-10
patches you have thrown into a single patch.
This is simply not nice to people reviewing it.

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 |

Re[2]: [PATCH v3 1/5] rtc: mxc_rtc: Driver rework

[Alexander Shiyan]

> > > > On Sat, Jun 29, 2013 at 12:40:40PM +0400, Alexander Shiyan wrote:
> > > > > This patch rework mxc_rtc driver.
> > > > > Major changes have been made:
> > > > > - Added second clock support (optional) which permit module functionality.
> > > > > - Implemented support for periodic interrupts.
> > > > > - Some code have been optimized.
> > > > 
> > > > You won't get any further with this if you don't listen to comments.
> > > > 
> > > > We're at v3 and Still this patch combines many totally unrelated changes
> > > > in a single patch. This was noted by Shawn and more detailed by myself.
> > > 
> > > Where Sascha?
> > > v3 is so different than v2. Can you inline your comments in v3?
> 
> I just did that.
> 
> > 
> > At the moment, the driver does not work at all, even for a boards which declared.
> > I do not understand the issue of making changes if we can correct this situation 
> > Please fix me.
> > Once again, now driver not work at all .....
> 
> If the driver is really so broken and ugly that it can't be fixed or
> only with a huge amount of work, then your option would be to replace it
> completely and clearly say why you think it's broken and why we need a
> new driver.
> 
> I don't think that's the case. The driver has it's deficiencies, but
> they can be fixed. Many of your changes are fine, but it's really 5-10
> patches you have thrown into a single patch.
> This is simply not nice to people reviewing it.

The situation is this: I do not use this device in their work, I just wanted to
add the driver to the DT-tree, respectively I had to check if it works.
Well, I got around to it, and the result was a series of patches to achieve the
ultimate goal - RTC DT-node.
The driver has not been updated for a long time and is very bad that my efforts
were in vain verification. On my opinion, any changes it is better than inaction.
Well, leave it to the discretion of the community.
Thanks.

---

[PATCH v3 1/5] rtc: mxc_rtc: Driver rework

[Sascha Hauer]

On Sun, Jun 30, 2013 at 01:10:33PM +0400, Alexander Shiyan wrote:
> > > > > On Sat, Jun 29, 2013 at 12:40:40PM +0400, Alexander Shiyan wrote:
> > > > > > This patch rework mxc_rtc driver.
> > > > > > Major changes have been made:
> > > > > > - Added second clock support (optional) which permit module functionality.
> > > > > > - Implemented support for periodic interrupts.
> > > > > > - Some code have been optimized.
> > > > > 
> > > > > You won't get any further with this if you don't listen to comments.
> > > > > 
> > > > > We're at v3 and Still this patch combines many totally unrelated changes
> > > > > in a single patch. This was noted by Shawn and more detailed by myself.
> > > > 
> > > > Where Sascha?
> > > > v3 is so different than v2. Can you inline your comments in v3?
> > 
> > I just did that.
> > 
> > > 
> > > At the moment, the driver does not work at all, even for a boards which declared.
> > > I do not understand the issue of making changes if we can correct this situation 
> > > Please fix me.
> > > Once again, now driver not work at all .....
> > 
> > If the driver is really so broken and ugly that it can't be fixed or
> > only with a huge amount of work, then your option would be to replace it
> > completely and clearly say why you think it's broken and why we need a
> > new driver.
> > 
> > I don't think that's the case. The driver has it's deficiencies, but
> > they can be fixed. Many of your changes are fine, but it's really 5-10
> > patches you have thrown into a single patch.
> > This is simply not nice to people reviewing it.
> 
> The situation is this: I do not use this device in their work, I just wanted to
> add the driver to the DT-tree, respectively I had to check if it works.
> Well, I got around to it, and the result was a series of patches to achieve the
> ultimate goal - RTC DT-node.
> The driver has not been updated for a long time and is very bad that my efforts
> were in vain verification. On my opinion, any changes it is better than inaction.
> Well, leave it to the discretion of the community.

Most of your patches are fine, it's just that the first one does too
much in a single patch and it would be easy to split that up.

Anyway, I'm not the one applying it. If anyone thinks I'm too picky here
just rule me out.

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 |

[PATCH v3 1/5] rtc: mxc_rtc: Driver rework

[Sascha Hauer]

On Sat, Jun 29, 2013 at 12:40:40PM +0400, Alexander Shiyan wrote:
> This patch rework mxc_rtc driver.
> Major changes have been made:
> - Added second clock support (optional) which permit module functionality.
> - Implemented support for periodic interrupts.
> - Some code have been optimized.
> 
> Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
> ---
>  drivers/rtc/rtc-mxc.c | 278 +++++++++++++++++++++-----------------------------
>  1 file changed, 119 insertions(+), 159 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-mxc.c b/drivers/rtc/rtc-mxc.c
> index ab87bac..8ec47c8 100644
> --- a/drivers/rtc/rtc-mxc.c
> +++ b/drivers/rtc/rtc-mxc.c
> @@ -12,7 +12,6 @@
>  #include <linux/io.h>
>  #include <linux/rtc.h>
>  #include <linux/module.h>
> -#include <linux/slab.h>
>  #include <linux/interrupt.h>
>  #include <linux/platform_device.h>
>  #include <linux/clk.h>
> @@ -39,20 +38,6 @@
>  
>  #define RTC_ENABLE_BIT  (1 << 7)
>  
> -#define MAX_PIE_NUM     9
> -#define MAX_PIE_FREQ    512
> -static const u32 PIE_BIT_DEF[MAX_PIE_NUM][2] = {
> -	{ 2,		RTC_2HZ_BIT },
> -	{ 4,		RTC_SAM0_BIT },
> -	{ 8,		RTC_SAM1_BIT },
> -	{ 16,		RTC_SAM2_BIT },
> -	{ 32,		RTC_SAM3_BIT },
> -	{ 64,		RTC_SAM4_BIT },
> -	{ 128,		RTC_SAM5_BIT },
> -	{ 256,		RTC_SAM6_BIT },
> -	{ MAX_PIE_FREQ,	RTC_SAM7_BIT },
> -};
> -
>  #define MXC_RTC_TIME	0
>  #define MXC_RTC_ALARM	1
>  
> @@ -66,9 +51,6 @@ static const u32 PIE_BIT_DEF[MAX_PIE_NUM][2] = {
>  #define RTC_STPWCH	0x1C	/*  32bit rtc stopwatch min reg */
>  #define RTC_DAYR	0x20	/*  32bit rtc days counter reg */
>  #define RTC_DAYALARM	0x24	/*  32bit rtc day alarm reg */
> -#define RTC_TEST1	0x28	/*  32bit rtc test reg 1 */
> -#define RTC_TEST2	0x2C	/*  32bit rtc test reg 2 */
> -#define RTC_TEST3	0x30	/*  32bit rtc test reg 3 */
>  
>  enum imx_rtc_type {
>  	IMX1_RTC,
> @@ -79,29 +61,12 @@ struct rtc_plat_data {
>  	struct rtc_device *rtc;
>  	void __iomem *ioaddr;
>  	int irq;
> -	struct clk *clk;
> -	struct rtc_time g_rtc_alarm;
> +	struct rtc_class_ops	rtc_ops;
> +	struct clk		*clk_rtc;
> +	struct clk		*clk_ipg;
>  	enum imx_rtc_type devtype;
>  };
>  
> -static struct platform_device_id imx_rtc_devtype[] = {
> -	{
> -		.name = "imx1-rtc",
> -		.driver_data = IMX1_RTC,
> -	}, {
> -		.name = "imx21-rtc",
> -		.driver_data = IMX21_RTC,
> -	}, {
> -		/* sentinel */
> -	}
> -};
> -MODULE_DEVICE_TABLE(platform, imx_rtc_devtype);
> -
> -static inline int is_imx1_rtc(struct rtc_plat_data *data)
> -{
> -	return data->devtype == IMX1_RTC;
> -}

What is wrong with this function?

> -
>  /*
>   * This function is used to obtain the RTC time or the alarm value in
>   * second.
> @@ -110,20 +75,16 @@ static u32 get_alarm_or_time(struct device *dev, int time_alarm)
>  {
>  	struct platform_device *pdev = to_platform_device(dev);
>  	struct rtc_plat_data *pdata = platform_get_drvdata(pdev);
> -	void __iomem *ioaddr = pdata->ioaddr;
> -	u32 day = 0, hr = 0, min = 0, sec = 0, hr_min = 0;
> -
> -	switch (time_alarm) {
> -	case MXC_RTC_TIME:
> -		day = readw(ioaddr + RTC_DAYR);
> -		hr_min = readw(ioaddr + RTC_HOURMIN);
> -		sec = readw(ioaddr + RTC_SECOND);
> -		break;
> -	case MXC_RTC_ALARM:
> -		day = readw(ioaddr + RTC_DAYALARM);
> -		hr_min = readw(ioaddr + RTC_ALRM_HM) & 0xffff;
> -		sec = readw(ioaddr + RTC_ALRM_SEC);
> -		break;
> +	u32 day, hr, min, sec, hr_min;
> +
> +	if (time_alarm == MXC_RTC_TIME) {
> +		day = readw(pdata->ioaddr + RTC_DAYR);
> +		hr_min = readw(pdata->ioaddr + RTC_HOURMIN);
> +		sec = readw(pdata->ioaddr + RTC_SECOND);
> +	} else {
> +		day = readw(pdata->ioaddr + RTC_DAYALARM);
> +		hr_min = readw(pdata->ioaddr + RTC_ALRM_HM);
> +		sec = readw(pdata->ioaddr + RTC_ALRM_SEC);
>  	}

It is debatable whether this change makes sense or not. It is cleanup
though and should not be mixed with functionality change also in this
patch.

>  
>  	hr = hr_min >> 8;
> @@ -140,7 +101,6 @@ static void set_alarm_or_time(struct device *dev, int time_alarm, u32 time)
>  	u32 day, hr, min, sec, temp;
>  	struct platform_device *pdev = to_platform_device(dev);
>  	struct rtc_plat_data *pdata = platform_get_drvdata(pdev);
> -	void __iomem *ioaddr = pdata->ioaddr;
>  
>  	day = time / 86400;
>  	time -= day * 86400;
> @@ -155,17 +115,14 @@ static void set_alarm_or_time(struct device *dev, int time_alarm, u32 time)
>  
>  	temp = (hr << 8) + min;
>  
> -	switch (time_alarm) {
> -	case MXC_RTC_TIME:
> -		writew(day, ioaddr + RTC_DAYR);
> -		writew(sec, ioaddr + RTC_SECOND);
> -		writew(temp, ioaddr + RTC_HOURMIN);
> -		break;
> -	case MXC_RTC_ALARM:
> -		writew(day, ioaddr + RTC_DAYALARM);
> -		writew(sec, ioaddr + RTC_ALRM_SEC);
> -		writew(temp, ioaddr + RTC_ALRM_HM);
> -		break;
> +	if (time_alarm == MXC_RTC_TIME) {
> +		writew(day, pdata->ioaddr + RTC_DAYR);
> +		writew(sec, pdata->ioaddr + RTC_SECOND);
> +		writew(temp, pdata->ioaddr + RTC_HOURMIN);
> +	} else {
> +		writew(day, pdata->ioaddr + RTC_DAYALARM);
> +		writew(sec, pdata->ioaddr + RTC_ALRM_SEC);
> +		writew(temp, pdata->ioaddr + RTC_ALRM_HM);
>  	}
>  }
>  
> @@ -179,7 +136,6 @@ static int rtc_update_alarm(struct device *dev, struct rtc_time *alrm)
>  	unsigned long now, time;
>  	struct platform_device *pdev = to_platform_device(dev);
>  	struct rtc_plat_data *pdata = platform_get_drvdata(pdev);
> -	void __iomem *ioaddr = pdata->ioaddr;
>  
>  	now = get_alarm_or_time(dev, MXC_RTC_TIME);
>  	rtc_time_to_tm(now, &now_tm);
> @@ -191,8 +147,9 @@ static int rtc_update_alarm(struct device *dev, struct rtc_time *alrm)
>  	alarm_tm.tm_sec = alrm->tm_sec;
>  	rtc_tm_to_time(&alarm_tm, &time);
>  
> -	/* clear all the interrupt status bits */
> -	writew(readw(ioaddr + RTC_RTCISR), ioaddr + RTC_RTCISR);
> +	/* clear interrupt status bit */
> +	writew(RTC_ALM_BIT, pdata->ioaddr + RTC_RTCISR);
> +

This change is not explained. Are there problems with the old code?
This hunk raises a question which should be answered in a commit message
for a separate patch.

>  	set_alarm_or_time(dev, MXC_RTC_ALARM, time);
>  
>  	return 0;
> @@ -203,18 +160,19 @@ static void mxc_rtc_irq_enable(struct device *dev, unsigned int bit,
>  {
>  	struct platform_device *pdev = to_platform_device(dev);
>  	struct rtc_plat_data *pdata = platform_get_drvdata(pdev);
> -	void __iomem *ioaddr = pdata->ioaddr;
>  	u32 reg;
>  
>  	spin_lock_irq(&pdata->rtc->irq_lock);
> -	reg = readw(ioaddr + RTC_RTCIENR);
>  
> -	if (enabled)
> +	reg = readw(pdata->ioaddr + RTC_RTCIENR);
> +	if (enabled) {
>  		reg |= bit;
> -	else
> +		/* Clear interrupt status */
> +		writew(reg, pdata->ioaddr + RTC_RTCISR);
> +	} else
>  		reg &= ~bit;
> +	writew(reg, pdata->ioaddr + RTC_RTCIENR);
>  
> -	writew(reg, ioaddr + RTC_RTCIENR);

What's the change introduced here and why is it introduced? reading this
is made unnecessarily complicated due to replacing ioaddr with
pdata->ioaddr.

>  	spin_unlock_irq(&pdata->rtc->irq_lock);
>  }
>  
> @@ -252,30 +210,42 @@ static irqreturn_t mxc_rtc_interrupt(int irq, void *dev_id)
>  	return IRQ_HANDLED;
>  }
>  
> -/*
> - * Clear all interrupts and release the IRQ
> - */
> -static void mxc_rtc_release(struct device *dev)
> +static int mxc_rtc_open(struct device *dev)
>  {
>  	struct platform_device *pdev = to_platform_device(dev);
>  	struct rtc_plat_data *pdata = platform_get_drvdata(pdev);
> -	void __iomem *ioaddr = pdata->ioaddr;
>  
> -	spin_lock_irq(&pdata->rtc->irq_lock);
> +	if (pdata->irq >= 0) {
> +		unsigned long rate = clk_get_rate(pdata->clk_rtc);
>  
> -	/* Disable all rtc interrupts */
> -	writew(0, ioaddr + RTC_RTCIENR);
> +		pdata->rtc->max_user_freq = rate / 64;
> +		rtc_irq_set_freq(pdata->rtc, NULL, rate / 64);
> +		mxc_rtc_irq_enable(&pdev->dev, RTC_1HZ_BIT | RTC_SAM7_BIT, 1);
> +	}

The irq seems to be made optional here. Do we need this? Do we want
this? Again, this is something hidden in a big patch.

>  
> -	/* Clear all interrupt status */
> -	writew(0xffffffff, ioaddr + RTC_RTCISR);
> +	return 0;
> +}
>  
> -	spin_unlock_irq(&pdata->rtc->irq_lock);
> +static void mxc_rtc_release(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct rtc_plat_data *pdata = platform_get_drvdata(pdev);
> +
> +	if (pdata->irq >= 0)
> +		mxc_rtc_irq_enable(&pdev->dev, RTC_1HZ_BIT | RTC_SAM7_BIT, 0);
>  }
>  
>  static int mxc_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled)
>  {
> -	mxc_rtc_irq_enable(dev, RTC_ALM_BIT, enabled);
> -	return 0;
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct rtc_plat_data *pdata = platform_get_drvdata(pdev);
> +
> +	if (pdata->irq >= 0) {
> +		mxc_rtc_irq_enable(dev, RTC_ALM_BIT, enabled);
> +		return 0;
> +	}
> +
> +	return -EINVAL;
>  }
>  
>  /*
> @@ -306,7 +276,7 @@ static int mxc_rtc_set_mmss(struct device *dev, unsigned long time)
>  	/*
>  	 * TTC_DAYR register is 9-bit in MX1 SoC, save time and day of year only
>  	 */
> -	if (is_imx1_rtc(pdata)) {
> +	if (pdata->devtype == IMX1_RTC) {
>  		struct rtc_time tm;
>  
>  		rtc_time_to_tm(time, &tm);
> @@ -331,10 +301,9 @@ static int mxc_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
>  {
>  	struct platform_device *pdev = to_platform_device(dev);
>  	struct rtc_plat_data *pdata = platform_get_drvdata(pdev);
> -	void __iomem *ioaddr = pdata->ioaddr;
>  
>  	rtc_time_to_tm(get_alarm_or_time(dev, MXC_RTC_ALARM), &alrm->time);
> -	alrm->pending = ((readw(ioaddr + RTC_RTCISR) & RTC_ALM_BIT)) ? 1 : 0;
> +	alrm->pending = !!(readw(pdata->ioaddr + RTC_RTCISR) & RTC_ALM_BIT);
>  
>  	return 0;
>  }
> @@ -349,61 +318,41 @@ static int mxc_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
>  	int ret;
>  
>  	ret = rtc_update_alarm(dev, &alrm->time);
> -	if (ret)
> -		return ret;
> +	if ((pdata->irq >= 0) && !ret)
> +		mxc_rtc_irq_enable(dev, RTC_ALM_BIT, alrm->enabled);
>  
> -	memcpy(&pdata->g_rtc_alarm, &alrm->time, sizeof(struct rtc_time));
> -	mxc_rtc_irq_enable(dev, RTC_ALM_BIT, alrm->enabled);
> -
> -	return 0;
> +	return ret;
>  }
>  
> -/* RTC layer */
> -static struct rtc_class_ops mxc_rtc_ops = {
> -	.release		= mxc_rtc_release,
> -	.read_time		= mxc_rtc_read_time,
> -	.set_mmss		= mxc_rtc_set_mmss,
> -	.read_alarm		= mxc_rtc_read_alarm,
> -	.set_alarm		= mxc_rtc_set_alarm,
> -	.alarm_irq_enable	= mxc_rtc_alarm_irq_enable,
> -};
> -
>  static int mxc_rtc_probe(struct platform_device *pdev)
>  {
> +	struct rtc_plat_data *pdata;
>  	struct resource *res;
> -	struct rtc_device *rtc;
> -	struct rtc_plat_data *pdata = NULL;
>  	u32 reg;
>  	unsigned long rate;
>  	int ret;
>  
> -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	if (!res)
> -		return -ENODEV;
> -
>  	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
>  	if (!pdata)
>  		return -ENOMEM;
>  
> -	pdata->devtype = pdev->id_entry->driver_data;
> -
> -	if (!devm_request_mem_region(&pdev->dev, res->start,
> -				     resource_size(res), pdev->name))
> -		return -EBUSY;
> -
> -	pdata->ioaddr = devm_ioremap(&pdev->dev, res->start,
> -				     resource_size(res));
> -
> -	pdata->clk = devm_clk_get(&pdev->dev, NULL);
> -	if (IS_ERR(pdata->clk)) {
> -		dev_err(&pdev->dev, "unable to get clock!\n");
> -		ret = PTR_ERR(pdata->clk);
> -		goto exit_free_pdata;
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	pdata->ioaddr = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(pdata->ioaddr))
> +		return PTR_ERR(pdata->ioaddr);
> +
> +	pdata->clk_rtc = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(pdata->clk_rtc)) {
> +		dev_err(&pdev->dev, "Unable to get clock!\n");
> +		return PTR_ERR(pdata->clk_rtc);
>  	}
>  
> -	clk_prepare_enable(pdata->clk);
> -	rate = clk_get_rate(pdata->clk);
> +	pdata->clk_ipg = devm_clk_get(&pdev->dev, "ipg");
> +	if (!IS_ERR(pdata->clk_ipg))
> +		clk_prepare_enable(pdata->clk_ipg);
> +	clk_prepare_enable(pdata->clk_rtc);
>  
> +	rate = clk_get_rate(pdata->clk_rtc);
>  	if (rate == 32768)
>  		reg = RTC_INPUT_CLK_32768HZ;
>  	else if (rate == 32000)
> @@ -411,49 +360,55 @@ static int mxc_rtc_probe(struct platform_device *pdev)
>  	else if (rate == 38400)
>  		reg = RTC_INPUT_CLK_38400HZ;
>  	else {
> -		dev_err(&pdev->dev, "rtc clock is not valid (%lu)\n", rate);
> +		dev_err(&pdev->dev, "RTC clock is not valid (%lu)\n", rate);

churn

>  		ret = -EINVAL;
>  		goto exit_put_clk;
>  	}
>  
> -	reg |= RTC_ENABLE_BIT;
> -	writew(reg, (pdata->ioaddr + RTC_RTCCTL));
> -	if (((readw(pdata->ioaddr + RTC_RTCCTL)) & RTC_ENABLE_BIT) == 0) {
> -		dev_err(&pdev->dev, "hardware module can't be enabled!\n");
> +	writew(reg | RTC_ENABLE_BIT, pdata->ioaddr + RTC_RTCCTL);
> +	if (!(readw(pdata->ioaddr + RTC_RTCCTL) & RTC_ENABLE_BIT)) {
> +		dev_err(&pdev->dev, "Hardware module can't be enabled!\n");

churn

>  		ret = -EIO;
>  		goto exit_put_clk;
>  	}
>  
> -	platform_set_drvdata(pdev, pdata);
> +	/* Disable all interrupts */
> +	writew(0, pdata->ioaddr + RTC_RTCIENR);
>  
> -	/* Configure and enable the RTC */
> -	pdata->irq = platform_get_irq(pdev, 0);
> +	pdata->devtype = pdev->id_entry->driver_data;
> +	platform_set_drvdata(pdev, pdata);
>  
> -	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;
> +	pdata->rtc_ops.open		= mxc_rtc_open;
> +	pdata->rtc_ops.release		= mxc_rtc_release;
> +	pdata->rtc_ops.read_time	= mxc_rtc_read_time;
> +	pdata->rtc_ops.set_mmss		= mxc_rtc_set_mmss;
> +	pdata->rtc_ops.read_alarm	= mxc_rtc_read_alarm;
> +	pdata->rtc_ops.set_alarm	= mxc_rtc_set_alarm;
> +	pdata->rtc_ops.alarm_irq_enable	= mxc_rtc_alarm_irq_enable;

So struct rtc_class_ops is embedded into struct rtc_plat_data now. Why
is this necessary?

> +
> +	pdata->rtc = devm_rtc_device_register(&pdev->dev, pdev->name,
> +					      &pdata->rtc_ops, THIS_MODULE);
> +	if (IS_ERR(pdata->rtc)) {
> +		ret = PTR_ERR(pdata->rtc);
> +		goto exit_put_clk;
>  	}
>  
> +	pdata->irq = platform_get_irq(pdev, 0);
>  	if (pdata->irq >= 0)
> -		device_init_wakeup(&pdev->dev, 1);
> +		if (devm_request_irq(&pdev->dev, pdata->irq, mxc_rtc_interrupt,
> +				     IRQF_SHARED, pdev->name, pdev) < 0) {
> +			dev_warn(&pdev->dev, "Not using interrupt\n");
> +			pdata->irq = -1;
> +		}
>  
> -	rtc = devm_rtc_device_register(&pdev->dev, pdev->name, &mxc_rtc_ops,
> -				  THIS_MODULE);
> -	if (IS_ERR(rtc)) {
> -		ret = PTR_ERR(rtc);
> -		goto exit_put_clk;
> -	}
> -
> -	pdata->rtc = rtc;
> +	device_init_wakeup(&pdev->dev, pdata->irq >= 0);
>  
>  	return 0;
>  
>  exit_put_clk:
> -	clk_disable_unprepare(pdata->clk);
> -
> -exit_free_pdata:
> +	clk_disable_unprepare(pdata->clk_rtc);
> +	if (!IS_ERR(pdata->clk_ipg))
> +		clk_disable_unprepare(pdata->clk_ipg);
>  
>  	return ret;
>  }
> @@ -462,13 +417,14 @@ static int mxc_rtc_remove(struct platform_device *pdev)
>  {
>  	struct rtc_plat_data *pdata = platform_get_drvdata(pdev);
>  
> -	clk_disable_unprepare(pdata->clk);
> +	clk_disable_unprepare(pdata->clk_rtc);
> +	if (!IS_ERR(pdata->clk_ipg))
> +		clk_disable_unprepare(pdata->clk_ipg);
>  
>  	return 0;
>  }
>  
> -#ifdef CONFIG_PM_SLEEP
> -static int mxc_rtc_suspend(struct device *dev)
> +static int __maybe_unused mxc_rtc_suspend(struct device *dev)
>  {
>  	struct rtc_plat_data *pdata = dev_get_drvdata(dev);
>  
> @@ -478,7 +434,7 @@ static int mxc_rtc_suspend(struct device *dev)
>  	return 0;
>  }
>  
> -static int mxc_rtc_resume(struct device *dev)
> +static int __maybe_unused mxc_rtc_resume(struct device *dev)
>  {
>  	struct rtc_plat_data *pdata = dev_get_drvdata(dev);
>  
> @@ -487,24 +443,28 @@ static int mxc_rtc_resume(struct device *dev)
>  
>  	return 0;
>  }
> -#endif
>  
>  static SIMPLE_DEV_PM_OPS(mxc_rtc_pm_ops, mxc_rtc_suspend, mxc_rtc_resume);
>  
> +static const struct platform_device_id mxc_rtc_id_table[] = {
> +	{ .name = "imx1-rtc", .driver_data = IMX1_RTC, },
> +	{ .name = "imx21-rtc", .driver_data = IMX21_RTC, },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(platform, mxc_rtc_id_table);
> +
>  static struct platform_driver mxc_rtc_driver = {
>  	.driver = {
>  		   .name	= "mxc_rtc",
>  		   .pm		= &mxc_rtc_pm_ops,
>  		   .owner	= THIS_MODULE,
>  	},
> -	.id_table = imx_rtc_devtype,
> +	.id_table = mxc_rtc_id_table,
>  	.probe = mxc_rtc_probe,
>  	.remove = mxc_rtc_remove,
>  };
> -
>  module_platform_driver(mxc_rtc_driver)
>  
>  MODULE_AUTHOR("Daniel Mack <daniel@caiaq.de>");
>  MODULE_DESCRIPTION("RTC driver for Freescale MXC");
>  MODULE_LICENSE("GPL");
> -
> -- 
> 1.8.1.5
> 
> 

-- 
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 |

Re[2]: [PATCH v3 1/5] rtc: mxc_rtc: Driver rework

[Alexander Shiyan]

> On Sat, Jun 29, 2013 at 12:40:40PM +0400, Alexander Shiyan wrote:
> > This patch rework mxc_rtc driver.
> > Major changes have been made:
> > - Added second clock support (optional) which permit module functionality.
> > - Implemented support for periodic interrupts.
> > - Some code have been optimized.
> > 
> > Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
> > ---
...
> > -static inline int is_imx1_rtc(struct rtc_plat_data *data)
> > -{
> > -	return data->devtype == IMX1_RTC;
> > -}
> 
> What is wrong with this function?

All good here. This call is used only once in set_mmss, so no reason
to have separate function.
I agree that we can make it in a separate patch, but the optimization of the
code is specified in the changelog.

> >   * This function is used to obtain the RTC time or the alarm value in
> >   * second.
> > @@ -110,20 +75,16 @@ static u32 get_alarm_or_time(struct device *dev, int time_alarm)
> >  {
> >  	struct platform_device *pdev = to_platform_device(dev);
> >  	struct rtc_plat_data *pdata = platform_get_drvdata(pdev);
> > -	void __iomem *ioaddr = pdata->ioaddr;
> > -	u32 day = 0, hr = 0, min = 0, sec = 0, hr_min = 0;
> > -
> > -	switch (time_alarm) {
> > -	case MXC_RTC_TIME:
> > -		day = readw(ioaddr + RTC_DAYR);
> > -		hr_min = readw(ioaddr + RTC_HOURMIN);
> > -		sec = readw(ioaddr + RTC_SECOND);
> > -		break;
> > -	case MXC_RTC_ALARM:
> > -		day = readw(ioaddr + RTC_DAYALARM);
> > -		hr_min = readw(ioaddr + RTC_ALRM_HM) & 0xffff;
> > -		sec = readw(ioaddr + RTC_ALRM_SEC);
> > -		break;
> > +	u32 day, hr, min, sec, hr_min;
> > +
> > +	if (time_alarm == MXC_RTC_TIME) {
> > +		day = readw(pdata->ioaddr + RTC_DAYR);
> > +		hr_min = readw(pdata->ioaddr + RTC_HOURMIN);
> > +		sec = readw(pdata->ioaddr + RTC_SECOND);
> > +	} else {
> > +		day = readw(pdata->ioaddr + RTC_DAYALARM);
> > +		hr_min = readw(pdata->ioaddr + RTC_ALRM_HM);
> > +		sec = readw(pdata->ioaddr + RTC_ALRM_SEC);
> >  	}
> 
> It is debatable whether this change makes sense or not. It is cleanup
> though and should not be mixed with functionality change also in this
> patch.

As I wrote earlier, this is just a way to remove the initial variables setup.
"u32 day = 0, hr = 0, min = 0, sec = 0, hr_min = 0;" ==> "u32 day, hr, min, sec, hr_min;"

...
> > -	/* clear all the interrupt status bits */
> > -	writew(readw(ioaddr + RTC_RTCISR), ioaddr + RTC_RTCISR);
> > +	/* clear interrupt status bit */
> > +	writew(RTC_ALM_BIT, pdata->ioaddr + RTC_RTCISR);
> > +
> 
> This change is not explained. Are there problems with the old code?
> This hunk raises a question which should be answered in a commit message
> for a separate patch.

Not correct to clear all interrupt status bits. We can lost update and/or periodic
status.

...
> >  	struct rtc_plat_data *pdata = platform_get_drvdata(pdev);
> > -	void __iomem *ioaddr = pdata->ioaddr;
> >  
> > -	spin_lock_irq(&pdata->rtc->irq_lock);
> > +	if (pdata->irq >= 0) {
> > +		unsigned long rate = clk_get_rate(pdata->clk_rtc);
> >  
> > -	/* Disable all rtc interrupts */
> > -	writew(0, ioaddr + RTC_RTCIENR);
> > +		pdata->rtc->max_user_freq = rate / 64;
> > +		rtc_irq_set_freq(pdata->rtc, NULL, rate / 64);
> > +		mxc_rtc_irq_enable(&pdev->dev, RTC_1HZ_BIT | RTC_SAM7_BIT, 1);
> > +	}
> 
> The irq seems to be made optional here. Do we need this? Do we want
> this? Again, this is something hidden in a big patch.

IRQ is checked in mxc_rtc_irq_enable function.
I agree that freq should be set if IRQ is used.

...
> > +	pdata->rtc_ops.open		= mxc_rtc_open;
> > +	pdata->rtc_ops.release		= mxc_rtc_release;
> > +	pdata->rtc_ops.read_time	= mxc_rtc_read_time;
> > +	pdata->rtc_ops.set_mmss		= mxc_rtc_set_mmss;
> > +	pdata->rtc_ops.read_alarm	= mxc_rtc_read_alarm;
> > +	pdata->rtc_ops.set_alarm	= mxc_rtc_set_alarm;
> > +	pdata->rtc_ops.alarm_irq_enable	= mxc_rtc_alarm_irq_enable;
> 
> So struct rtc_class_ops is embedded into struct rtc_plat_data now. Why
> is this necessary?

Just save BSS. Can be moved into cleanup part.

...

To summarize, I do not know what to do with this patch. Most likely I shall
not continue to work on this driver. Just keep in mind that using driver in its
current state is impossible (clk).
Thanks.

---

[PATCH v3 1/5] rtc: mxc_rtc: Driver re work

[Lothar Waßmann]

Hi,

Alexander Shiyan writes:
> > On Sat, Jun 29, 2013 at 12:40:40PM +0400, Alexander Shiyan wrote:
> > > This patch rework mxc_rtc driver.
> > > Major changes have been made:
> > > - Added second clock support (optional) which permit module functionality.
> > > - Implemented support for periodic interrupts.
> > > - Some code have been optimized.
> > > 
> > > Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
> > > ---
> ...
> > > -static inline int is_imx1_rtc(struct rtc_plat_data *data)
> > > -{
> > > -	return data->devtype == IMX1_RTC;
> > > -}
> > 
> > What is wrong with this function?
> 
> All good here. This call is used only once in set_mmss, so no reason
> to have separate function.
> I agree that we can make it in a separate patch, but the optimization of the
> code is specified in the changelog.
> 
That's an inline function anyway. There should be no difference in
code with or without this function. Only difference in source code
readability.

> > > +	pdata->rtc_ops.open		= mxc_rtc_open;
> > > +	pdata->rtc_ops.release		= mxc_rtc_release;
> > > +	pdata->rtc_ops.read_time	= mxc_rtc_read_time;
> > > +	pdata->rtc_ops.set_mmss		= mxc_rtc_set_mmss;
> > > +	pdata->rtc_ops.read_alarm	= mxc_rtc_read_alarm;
> > > +	pdata->rtc_ops.set_alarm	= mxc_rtc_set_alarm;
> > > +	pdata->rtc_ops.alarm_irq_enable	= mxc_rtc_alarm_irq_enable;
> > 
> > So struct rtc_class_ops is embedded into struct rtc_plat_data now. Why
> > is this necessary?
> 
> Just save BSS. Can be moved into cleanup part.
> 
The purpose of platform_data is to convey platform specific
information to drivers, not a general driver local storage.
Thus platform_data should be treated read-only by drivers.


Lothar Wa?mann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstra?e 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Gesch?ftsf?hrer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________

Re[4]: [PATCH v3 1/5] rtc: mxc_rtc: Driver re work

[Alexander Shiyan]

> Alexander Shiyan writes:
> > > On Sat, Jun 29, 2013 at 12:40:40PM +0400, Alexander Shiyan wrote:
> > > > This patch rework mxc_rtc driver.
> > > > Major changes have been made:
> > > > - Added second clock support (optional) which permit module functionality.
> > > > - Implemented support for periodic interrupts.
> > > > - Some code have been optimized.
> > > > 
> > > > Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
...
> > > > +	pdata->rtc_ops.open		= mxc_rtc_open;
> > > > +	pdata->rtc_ops.release		= mxc_rtc_release;
> > > > +	pdata->rtc_ops.read_time	= mxc_rtc_read_time;
> > > > +	pdata->rtc_ops.set_mmss		= mxc_rtc_set_mmss;
> > > > +	pdata->rtc_ops.read_alarm	= mxc_rtc_read_alarm;
> > > > +	pdata->rtc_ops.set_alarm	= mxc_rtc_set_alarm;
> > > > +	pdata->rtc_ops.alarm_irq_enable	= mxc_rtc_alarm_irq_enable;
> > > 
> > > So struct rtc_class_ops is embedded into struct rtc_plat_data now. Why
> > > is this necessary?
> > 
> > Just save BSS. Can be moved into cleanup part.
> > 
> The purpose of platform_data is to convey platform specific
> information to drivers, not a general driver local storage.
> Thus platform_data should be treated read-only by drivers.

"pdata" here is not a platform_data.
This is a private driver struct. this was be renamed by me in v1, but I revert
these changes.

---