[PATCH v2 1/3] rtc: mxc_rtc: Driver rework
Sascha Hauer
s.hauer at pengutronix.de
Sun Jun 23 15:18:48 EDT 2013
On Sun, Jun 23, 2013 at 12:32:27PM +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.
> - Code have been optimized and cleaned.
>
> Signed-off-by: Alexander Shiyan <shc_work at mail.ru>
> ---
> drivers/rtc/rtc-mxc.c | 382 +++++++++++++++++++++-----------------------------
> 1 file changed, 159 insertions(+), 223 deletions(-)
>
> diff --git a/drivers/rtc/rtc-mxc.c b/drivers/rtc/rtc-mxc.c
> index ab87bac..90b0222 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>
> @@ -21,54 +20,37 @@
> #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 BIT(0)
> +#define RTC_ALM_BIT BIT(2)
> +#define RTC_1HZ_BIT BIT(4)
> +#define RTC_2HZ_BIT BIT(7)
> +#define RTC_SAM0_BIT BIT(8)
> +#define RTC_SAM1_BIT BIT(9)
> +#define RTC_SAM2_BIT BIT(10)
> +#define RTC_SAM3_BIT BIT(11)
> +#define RTC_SAM4_BIT BIT(12)
> +#define RTC_SAM5_BIT BIT(13)
> +#define RTC_SAM6_BIT BIT(14)
> +#define RTC_SAM7_BIT BIT(15)
This is only unnecessary churn. Personally I wouldn't change from
(1 << x) to BIT(x) or the other way round. If you really want to do it
this at least should be a separate patch.
> -#define RTC_HOURMIN 0x00 /* 32bit rtc hour/min counter reg */
> -#define RTC_SECOND 0x04 /* 32bit rtc seconds counter reg */
> -#define RTC_ALRM_HM 0x08 /* 32bit rtc alarm hour/min reg */
> -#define RTC_ALRM_SEC 0x0C /* 32bit rtc alarm seconds reg */
> -#define RTC_RTCCTL 0x10 /* 32bit rtc control reg */
> -#define RTC_RTCISR 0x14 /* 32bit rtc interrupt status reg */
> -#define RTC_RTCIENR 0x18 /* 32bit rtc interrupt enable reg */
> -#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 */
> +#define RTC_HOURMIN 0x00 /* rtc hour/min counter */
> +#define RTC_SECOND 0x04 /* rtc seconds counter */
> +#define RTC_ALRM_HM 0x08 /* rtc alarm hour/min */
> +#define RTC_ALRM_SEC 0x0c /* rtc alarm seconds */
> +#define RTC_RTCCTL 0x10 /* rtc control */
> +#define RTC_RTCISR 0x14 /* rtc interrupt status */
> +#define RTC_RTCIENR 0x18 /* rtc interrupt enable */
> +#define RTC_STPWCH 0x1c /* rtc stopwatch min */
> +#define RTC_DAYR 0x20 /* rtc days counter */
> +#define RTC_DAYALARM 0x24 /* rtc day alarm */
Also only churn. This only hides what the patch really does.
> 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);
Why? If there's a reason for this change let us know it in a commit
message to a separate patch.
> - 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);
ditto.
> 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);
> - 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);
Another hunk that might make sense but we do not know what's wrong with
the original code.
> 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);
>
> /* update irq data & counter */
> if (status & RTC_ALM_BIT) {
> - events |= (RTC_AF | RTC_IRQF);
> + events |= RTC_AF | RTC_IRQF;
> /* RTC alarm should be one-shot */
> mxc_rtc_irq_enable(&pdev->dev, RTC_ALM_BIT, 0);
> }
>
> if (status & RTC_1HZ_BIT)
> - events |= (RTC_UF | RTC_IRQF);
> + events |= RTC_UF | RTC_IRQF;
>
> if (status & PIT_ALL_ON)
> - events |= (RTC_PF | RTC_IRQF);
> + events |= RTC_PF | RTC_IRQF;
>
> rtc_update_irq(pdata->rtc, 1, events);
> +
> spin_unlock_irqrestore(&pdata->rtc->irq_lock, flags);
Only cosmetic above. Should be ok, but please in a patch with 'only
cosmetic, no functional change' description.
I'm stopping here. This really needs a serious rework.
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 |
More information about the linux-arm-kernel
mailing list