Hi Leela, > >> --- > >> 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); > >> +} > >> + Why do you use a container_of function instead of the watchdog_get_drvdata function? > >> +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); If you would have used watchdog_get_drvdata then this would then become: struct s3c2410_wdt *wdt = watchdog_get_drvdata(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); same here > >> + > >> + 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); same here > >> > >> - 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); same here > >> + 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); and you would also need to add something like: watchdog_set_drvdata(&wdt->wdt_device, wdt); > >> > >> - 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 For the rest code is indeed OK. (the container_of is not incorrect but it looks so much more complex and we added the watchdog_set_drvdata and watchdog_get_drvdata to add the watchdog data structures...). Kind regards, wim. -- 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
 |