Hello Wim Van Sebroeck, Can you please review this patch and take necessary action. Best Wishes, Leela Krishna Amudala. On Wed, Jul 24, 2013 at 1:23 PM, Kukjin Kim <kgene@xxxxxxxxxx> wrote: > Leela Krishna Amudala wrote: >> >> This patch removes the global variables in the driver file and >> group them into a structure. >> >> Signed-off-by: Leela Krishna Amudala <l.krishna@xxxxxxxxxxx> > > (+ Wim Van Sebroeck) > > Looks good to me, > > Acked-by: Kukjin Kim <kgene.kim@xxxxxxxxxxx> > > Thanks, > Kukjin > >> --- >> Note: This patch is rebased on kgene's for-next branch and tested on >> SMDK5420. >> >> Changes since V2: >> - Addressed comments given by Tomasz Figa <t.figa@xxxxxxxxxxx> >> https://patchwork.kernel.org/patch/2831032/ >> - Renamed structure name from s3c2410_watchdog to s3c2410_wdt. >> >> Changes since V1: >> - changed the patch subject. >> - indentation correction in s3c2410_watchdog structure. >> >> drivers/watchdog/s3c2410_wdt.c | 225 +++++++++++++++++++++++------------ >> ----- >> 1 file changed, 131 insertions(+), 94 deletions(-) >> >> diff --git a/drivers/watchdog/s3c2410_wdt.c >> b/drivers/watchdog/s3c2410_wdt.c >> index 6a22cf5..739dbd3 100644 >> --- a/drivers/watchdog/s3c2410_wdt.c >> +++ b/drivers/watchdog/s3c2410_wdt.c >> @@ -84,13 +84,17 @@ MODULE_PARM_DESC(soft_noboot, "Watchdog action, set to >> 1 to ignore reboots, " >> "0 to reboot (default 0)"); >> MODULE_PARM_DESC(debug, "Watchdog debug, set to >1 for debug (default >> 0)"); >> >> -static struct device *wdt_dev; /* platform device attached to */ >> -static struct resource *wdt_mem; >> -static struct resource *wdt_irq; >> -static struct clk *wdt_clock; >> -static void __iomem *wdt_base; >> -static unsigned int wdt_count; >> -static DEFINE_SPINLOCK(wdt_lock); >> +struct s3c2410_wdt { >> + struct device *dev; >> + struct clk *clock; >> + void __iomem *reg_base; >> + unsigned int count; >> + spinlock_t lock; >> + unsigned long wtcon_save; >> + unsigned long wtdat_save; >> + struct watchdog_device wdt_device; >> + struct notifier_block freq_transition; >> +}; >> >> /* watchdog control routines */ >> >> @@ -102,29 +106,43 @@ do { > \ >> >> /* functions */ >> >> +static inline struct s3c2410_wdt *to_s3c2410_wdt(struct watchdog_device >> *wdd) >> +{ >> + return container_of(wdd, struct s3c2410_wdt, wdt_device); >> +} >> + >> +static inline struct s3c2410_wdt *freq_to_wdt(struct notifier_block *nb) >> +{ >> + return container_of(nb, struct s3c2410_wdt, freq_transition); >> +} >> + >> static int s3c2410wdt_keepalive(struct watchdog_device *wdd) >> { >> - spin_lock(&wdt_lock); >> - writel(wdt_count, wdt_base + S3C2410_WTCNT); >> - spin_unlock(&wdt_lock); >> + struct s3c2410_wdt *wdt = to_s3c2410_wdt(wdd); >> + >> + spin_lock(&wdt->lock); >> + writel(wdt->count, wdt->reg_base + S3C2410_WTCNT); >> + spin_unlock(&wdt->lock); >> >> return 0; >> } >> >> -static void __s3c2410wdt_stop(void) >> +static void __s3c2410wdt_stop(struct s3c2410_wdt *wdt) >> { >> unsigned long wtcon; >> >> - wtcon = readl(wdt_base + S3C2410_WTCON); >> + wtcon = readl(wdt->reg_base + S3C2410_WTCON); >> wtcon &= ~(S3C2410_WTCON_ENABLE | S3C2410_WTCON_RSTEN); >> - writel(wtcon, wdt_base + S3C2410_WTCON); >> + writel(wtcon, wdt->reg_base + S3C2410_WTCON); >> } >> >> static int s3c2410wdt_stop(struct watchdog_device *wdd) >> { >> - spin_lock(&wdt_lock); >> - __s3c2410wdt_stop(); >> - spin_unlock(&wdt_lock); >> + struct s3c2410_wdt *wdt = to_s3c2410_wdt(wdd); >> + >> + spin_lock(&wdt->lock); >> + __s3c2410wdt_stop(wdt); >> + spin_unlock(&wdt->lock); >> >> return 0; >> } >> @@ -132,12 +150,13 @@ static int s3c2410wdt_stop(struct watchdog_device >> *wdd) >> static int s3c2410wdt_start(struct watchdog_device *wdd) >> { >> unsigned long wtcon; >> + struct s3c2410_wdt *wdt = to_s3c2410_wdt(wdd); >> >> - spin_lock(&wdt_lock); >> + spin_lock(&wdt->lock); >> >> - __s3c2410wdt_stop(); >> + __s3c2410wdt_stop(wdt); >> >> - wtcon = readl(wdt_base + S3C2410_WTCON); >> + wtcon = readl(wdt->reg_base + S3C2410_WTCON); >> wtcon |= S3C2410_WTCON_ENABLE | S3C2410_WTCON_DIV128; >> >> if (soft_noboot) { >> @@ -148,25 +167,26 @@ static int s3c2410wdt_start(struct watchdog_device >> *wdd) >> wtcon |= S3C2410_WTCON_RSTEN; >> } >> >> - DBG("%s: wdt_count=0x%08x, wtcon=%08lx\n", >> - __func__, wdt_count, wtcon); >> + DBG("%s: count=0x%08x, wtcon=%08lx\n", >> + __func__, wdt->count, wtcon); >> >> - writel(wdt_count, wdt_base + S3C2410_WTDAT); >> - writel(wdt_count, wdt_base + S3C2410_WTCNT); >> - writel(wtcon, wdt_base + S3C2410_WTCON); >> - spin_unlock(&wdt_lock); >> + writel(wdt->count, wdt->reg_base + S3C2410_WTDAT); >> + writel(wdt->count, wdt->reg_base + S3C2410_WTCNT); >> + writel(wtcon, wdt->reg_base + S3C2410_WTCON); >> + spin_unlock(&wdt->lock); >> >> return 0; >> } >> >> -static inline int s3c2410wdt_is_running(void) >> +static inline int s3c2410wdt_is_running(struct s3c2410_wdt *wdt) >> { >> - return readl(wdt_base + S3C2410_WTCON) & S3C2410_WTCON_ENABLE; >> + return readl(wdt->reg_base + S3C2410_WTCON) & S3C2410_WTCON_ENABLE; >> } >> >> static int s3c2410wdt_set_heartbeat(struct watchdog_device *wdd, unsigned >> timeout) >> { >> - unsigned long freq = clk_get_rate(wdt_clock); >> + struct s3c2410_wdt *wdt = to_s3c2410_wdt(wdd); >> + unsigned long freq = clk_get_rate(wdt->clock); >> unsigned int count; >> unsigned int divisor = 1; >> unsigned long wtcon; >> @@ -192,7 +212,7 @@ static int s3c2410wdt_set_heartbeat(struct >> watchdog_device *wdd, unsigned timeou >> } >> >> if ((count / divisor) >= 0x10000) { >> - dev_err(wdt_dev, "timeout %d too big\n", timeout); >> + dev_err(wdt->dev, "timeout %d too big\n", timeout); >> return -EINVAL; >> } >> } >> @@ -201,15 +221,15 @@ static int s3c2410wdt_set_heartbeat(struct >> watchdog_device *wdd, unsigned timeou >> __func__, timeout, divisor, count, count/divisor); >> >> count /= divisor; >> - wdt_count = count; >> + wdt->count = count; >> >> /* update the pre-scaler */ >> - wtcon = readl(wdt_base + S3C2410_WTCON); >> + wtcon = readl(wdt->reg_base + S3C2410_WTCON); >> wtcon &= ~S3C2410_WTCON_PRESCALE_MASK; >> wtcon |= S3C2410_WTCON_PRESCALE(divisor-1); >> >> - writel(count, wdt_base + S3C2410_WTDAT); >> - writel(wtcon, wdt_base + S3C2410_WTCON); >> + writel(count, wdt->reg_base + S3C2410_WTDAT); >> + writel(wtcon, wdt->reg_base + S3C2410_WTCON); >> >> wdd->timeout = (count * divisor) / freq; >> >> @@ -242,21 +262,23 @@ static struct watchdog_device s3c2410_wdd = { >> >> static irqreturn_t s3c2410wdt_irq(int irqno, void *param) >> { >> - dev_info(wdt_dev, "watchdog timer expired (irq)\n"); >> + struct s3c2410_wdt *wdt = platform_get_drvdata(param); >> + >> + dev_info(wdt->dev, "watchdog timer expired (irq)\n"); >> >> - s3c2410wdt_keepalive(&s3c2410_wdd); >> + s3c2410wdt_keepalive(&wdt->wdt_device); >> return IRQ_HANDLED; >> } >> >> - >> #ifdef CONFIG_CPU_FREQ >> >> static int s3c2410wdt_cpufreq_transition(struct notifier_block *nb, >> unsigned long val, void *data) >> { >> int ret; >> + struct s3c2410_wdt *wdt = freq_to_wdt(nb); >> >> - if (!s3c2410wdt_is_running()) >> + if (!s3c2410wdt_is_running(wdt)) >> goto done; >> >> if (val == CPUFREQ_PRECHANGE) { >> @@ -265,14 +287,15 @@ static int s3c2410wdt_cpufreq_transition(struct >> notifier_block *nb, >> * the watchdog is running. >> */ >> >> - s3c2410wdt_keepalive(&s3c2410_wdd); >> + s3c2410wdt_keepalive(&wdt->wdt_device); >> } else if (val == CPUFREQ_POSTCHANGE) { >> - s3c2410wdt_stop(&s3c2410_wdd); >> + s3c2410wdt_stop(&wdt->wdt_device); >> >> - ret = s3c2410wdt_set_heartbeat(&s3c2410_wdd, >> s3c2410_wdd.timeout); >> + ret = s3c2410wdt_set_heartbeat(&wdt->wdt_device, >> + wdt->wdt_device.timeout); >> >> if (ret >= 0) >> - s3c2410wdt_start(&s3c2410_wdd); >> + s3c2410wdt_start(&wdt->wdt_device); >> else >> goto err; >> } >> @@ -281,34 +304,35 @@ done: >> return 0; >> >> err: >> - dev_err(wdt_dev, "cannot set new value for timeout %d\n", >> - s3c2410_wdd.timeout); >> + dev_err(wdt->dev, "cannot set new value for timeout %d\n", >> + wdt->wdt_device.timeout); >> return ret; >> } >> >> -static struct notifier_block s3c2410wdt_cpufreq_transition_nb = { >> - .notifier_call = s3c2410wdt_cpufreq_transition, >> -}; >> - >> -static inline int s3c2410wdt_cpufreq_register(void) >> +static inline int s3c2410wdt_cpufreq_register(struct s3c2410_wdt *wdt) >> { >> - return cpufreq_register_notifier(&s3c2410wdt_cpufreq_transition_nb, >> + wdt->freq_transition.notifier_call = s3c2410wdt_cpufreq_transition; >> + >> + return cpufreq_register_notifier(&wdt->freq_transition, >> CPUFREQ_TRANSITION_NOTIFIER); >> } >> >> -static inline void s3c2410wdt_cpufreq_deregister(void) >> +static inline void s3c2410wdt_cpufreq_deregister(struct s3c2410_wdt *wdt) >> { >> - cpufreq_unregister_notifier(&s3c2410wdt_cpufreq_transition_nb, >> + wdt->freq_transition.notifier_call = s3c2410wdt_cpufreq_transition; >> + >> + cpufreq_unregister_notifier(&wdt->freq_transition, >> CPUFREQ_TRANSITION_NOTIFIER); >> } >> >> #else >> -static inline int s3c2410wdt_cpufreq_register(void) >> + >> +static inline int s3c2410wdt_cpufreq_register(struct s3c2410_wdt *wdt) >> { >> return 0; >> } >> >> -static inline void s3c2410wdt_cpufreq_deregister(void) >> +static inline void s3c2410wdt_cpufreq_deregister(struct s3c2410_wdt *wdt) >> { >> } >> #endif >> @@ -316,6 +340,9 @@ static inline void s3c2410wdt_cpufreq_deregister(void) >> static int s3c2410wdt_probe(struct platform_device *pdev) >> { >> struct device *dev; >> + struct s3c2410_wdt *wdt; >> + struct resource *wdt_mem; >> + struct resource *wdt_irq; >> unsigned int wtcon; >> int started = 0; >> int ret; >> @@ -323,8 +350,14 @@ static int s3c2410wdt_probe(struct platform_device >> *pdev) >> DBG("%s: probe=%p\n", __func__, pdev); >> >> dev = &pdev->dev; >> - wdt_dev = &pdev->dev; >> >> + wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL); >> + if (!wdt) >> + return -ENOMEM; >> + >> + wdt->dev = &pdev->dev; >> + spin_lock_init(&wdt->lock); >> + wdt->wdt_device = s3c2410_wdd; >> wdt_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> if (wdt_mem == NULL) { >> dev_err(dev, "no memory resource specified\n"); >> @@ -339,24 +372,24 @@ static int s3c2410wdt_probe(struct platform_device >> *pdev) >> } >> >> /* get the memory region for the watchdog timer */ >> - wdt_base = devm_ioremap_resource(dev, wdt_mem); >> - if (IS_ERR(wdt_base)) { >> - ret = PTR_ERR(wdt_base); >> + wdt->reg_base = devm_ioremap_resource(dev, wdt_mem); >> + if (IS_ERR(wdt->reg_base)) { >> + ret = PTR_ERR(wdt->reg_base); >> goto err; >> } >> >> - DBG("probe: mapped wdt_base=%p\n", wdt_base); >> + DBG("probe: mapped reg_base=%p\n", wdt->reg_base); >> >> - wdt_clock = devm_clk_get(dev, "watchdog"); >> - if (IS_ERR(wdt_clock)) { >> + wdt->clock = devm_clk_get(dev, "watchdog"); >> + if (IS_ERR(wdt->clock)) { >> dev_err(dev, "failed to find watchdog clock source\n"); >> - ret = PTR_ERR(wdt_clock); >> + ret = PTR_ERR(wdt->clock); >> goto err; >> } >> >> - clk_prepare_enable(wdt_clock); >> + clk_prepare_enable(wdt->clock); >> >> - ret = s3c2410wdt_cpufreq_register(); >> + ret = s3c2410wdt_cpufreq_register(wdt); >> if (ret < 0) { >> dev_err(dev, "failed to register cpufreq\n"); >> goto err_clk; >> @@ -365,9 +398,11 @@ static int s3c2410wdt_probe(struct platform_device >> *pdev) >> /* see if we can actually set the requested timer margin, and if >> * not, try the default value */ >> >> - watchdog_init_timeout(&s3c2410_wdd, tmr_margin, &pdev->dev); >> - if (s3c2410wdt_set_heartbeat(&s3c2410_wdd, s3c2410_wdd.timeout)) { >> - started = s3c2410wdt_set_heartbeat(&s3c2410_wdd, >> + watchdog_init_timeout(&wdt->wdt_device, tmr_margin, &pdev->dev); >> + ret = s3c2410wdt_set_heartbeat(&wdt->wdt_device, >> + wdt->wdt_device.timeout); >> + if (ret) { >> + started = s3c2410wdt_set_heartbeat(&wdt->wdt_device, >> > CONFIG_S3C2410_WATCHDOG_DEFAULT_TIME); >> >> if (started == 0) >> @@ -386,9 +421,9 @@ static int s3c2410wdt_probe(struct platform_device >> *pdev) >> goto err_cpufreq; >> } >> >> - watchdog_set_nowayout(&s3c2410_wdd, nowayout); >> + watchdog_set_nowayout(&wdt->wdt_device, nowayout); >> >> - ret = watchdog_register_device(&s3c2410_wdd); >> + ret = watchdog_register_device(&wdt->wdt_device); >> if (ret) { >> dev_err(dev, "cannot register watchdog (%d)\n", ret); >> goto err_cpufreq; >> @@ -396,18 +431,20 @@ static int s3c2410wdt_probe(struct platform_device >> *pdev) >> >> if (tmr_atboot && started == 0) { >> dev_info(dev, "starting watchdog timer\n"); >> - s3c2410wdt_start(&s3c2410_wdd); >> + s3c2410wdt_start(&wdt->wdt_device); >> } else if (!tmr_atboot) { >> /* if we're not enabling the watchdog, then ensure it is >> * disabled if it has been left running from the bootloader >> * or other source */ >> >> - s3c2410wdt_stop(&s3c2410_wdd); >> + s3c2410wdt_stop(&wdt->wdt_device); >> } >> >> + platform_set_drvdata(pdev, wdt); >> + >> /* print out a statement of readiness */ >> >> - wtcon = readl(wdt_base + S3C2410_WTCON); >> + wtcon = readl(wdt->reg_base + S3C2410_WTCON); >> >> dev_info(dev, "watchdog %sactive, reset %sabled, irq %sabled\n", >> (wtcon & S3C2410_WTCON_ENABLE) ? "" : "in", >> @@ -417,64 +454,64 @@ static int s3c2410wdt_probe(struct platform_device >> *pdev) >> return 0; >> >> err_cpufreq: >> - s3c2410wdt_cpufreq_deregister(); >> + s3c2410wdt_cpufreq_deregister(wdt); >> >> err_clk: >> - clk_disable_unprepare(wdt_clock); >> - wdt_clock = NULL; >> + clk_disable_unprepare(wdt->clock); >> + wdt->clock = NULL; >> >> err: >> - wdt_irq = NULL; >> - wdt_mem = NULL; >> return ret; >> } >> >> static int s3c2410wdt_remove(struct platform_device *dev) >> { >> - watchdog_unregister_device(&s3c2410_wdd); >> + struct s3c2410_wdt *wdt = platform_get_drvdata(dev); >> >> - s3c2410wdt_cpufreq_deregister(); >> + watchdog_unregister_device(&wdt->wdt_device); >> >> - clk_disable_unprepare(wdt_clock); >> - wdt_clock = NULL; >> + s3c2410wdt_cpufreq_deregister(wdt); >> + >> + clk_disable_unprepare(wdt->clock); >> + wdt->clock = NULL; >> >> - wdt_irq = NULL; >> - wdt_mem = NULL; >> return 0; >> } >> >> static void s3c2410wdt_shutdown(struct platform_device *dev) >> { >> - s3c2410wdt_stop(&s3c2410_wdd); >> + struct s3c2410_wdt *wdt = platform_get_drvdata(dev); >> + >> + s3c2410wdt_stop(&wdt->wdt_device); >> } >> >> #ifdef CONFIG_PM_SLEEP >> >> -static unsigned long wtcon_save; >> -static unsigned long wtdat_save; >> - >> static int s3c2410wdt_suspend(struct device *dev) >> { >> + struct s3c2410_wdt *wdt = dev_get_drvdata(dev); >> + >> /* Save watchdog state, and turn it off. */ >> - wtcon_save = readl(wdt_base + S3C2410_WTCON); >> - wtdat_save = readl(wdt_base + S3C2410_WTDAT); >> + wdt->wtcon_save = readl(wdt->reg_base + S3C2410_WTCON); >> + wdt->wtdat_save = readl(wdt->reg_base + S3C2410_WTDAT); >> >> /* Note that WTCNT doesn't need to be saved. */ >> - s3c2410wdt_stop(&s3c2410_wdd); >> + s3c2410wdt_stop(&wdt->wdt_device); >> >> return 0; >> } >> >> static int s3c2410wdt_resume(struct device *dev) >> { >> - /* Restore watchdog state. */ >> + struct s3c2410_wdt *wdt = dev_get_drvdata(dev); >> >> - writel(wtdat_save, wdt_base + S3C2410_WTDAT); >> - writel(wtdat_save, wdt_base + S3C2410_WTCNT); /* Reset count */ >> - writel(wtcon_save, wdt_base + S3C2410_WTCON); >> + /* Restore watchdog state. */ >> + writel(wdt->wtdat_save, wdt->reg_base + S3C2410_WTDAT); >> + writel(wdt->wtdat_save, wdt->reg_base + S3C2410_WTCNT);/* Reset >> count */ >> + writel(wdt->wtcon_save, wdt->reg_base + S3C2410_WTCON); >> >> dev_info(dev, "watchdog %sabled\n", >> - (wtcon_save & S3C2410_WTCON_ENABLE) ? "en" : "dis"); >> + (wdt->wtcon_save & S3C2410_WTCON_ENABLE) ? "en" : "dis"); >> >> return 0; >> } >> -- >> 1.7.10.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
 |