Hi Leela Krishna,
Looks mostly good, but see some comments inline.
On Monday 22 of July 2013 11:44:09 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>
> ---
> Note: This patch is rebased on kgene's for-next branch and tested on
> SMDK5420.
>
> Changes since V1:
> - changed the patch subject.
> - indentation correction in s3c2410_watchdog structure.
>
> drivers/watchdog/s3c2410_wdt.c | 204
> +++++++++++++++++++++++----------------- 1 file changed, 119
> insertions(+), 85 deletions(-)
>
> diff --git a/drivers/watchdog/s3c2410_wdt.c
> b/drivers/watchdog/s3c2410_wdt.c index 6a22cf5..de679d0 100644
> --- a/drivers/watchdog/s3c2410_wdt.c
> +++ b/drivers/watchdog/s3c2410_wdt.c
> @@ -84,13 +84,20 @@ 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_watchdog {
> + struct device *dev; /* platform device attached to */
> + struct clk *clock;
> + void __iomem *reg_base;
> + unsigned int count;
> + spinlock_t lock;
> + struct watchdog_device wdt_device;
> + struct notifier_block freq_transition;
> +};
> +
> +#define to_s3c2410_watchdog(wdd) \
> + container_of(wdd, struct s3c2410_watchdog, wdt_device)
> +#define freq_to_wdt(_n) \
> + container_of(_n, struct s3c2410_watchdog, freq_transition)
Please use static inlines here.
> /* watchdog control routines */
>
> @@ -104,27 +111,31 @@ do { \
>
> 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_watchdog *wdt = to_s3c2410_watchdog(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_watchdog *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_watchdog *wdt = to_s3c2410_watchdog(wdd);
> +
> + spin_lock(&wdt->lock);
> + __s3c2410wdt_stop(wdt);
> + spin_unlock(&wdt->lock);
>
> return 0;
> }
> @@ -133,11 +144,13 @@ static int s3c2410wdt_start(struct watchdog_device
> *wdd) {
> unsigned long wtcon;
>
Stray blank line.
> - spin_lock(&wdt_lock);
> + struct s3c2410_watchdog *wdt = to_s3c2410_watchdog(wdd);
> +
> + 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 +161,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_watchdog *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_watchdog *wdt = to_s3c2410_watchdog(wdd);
> + unsigned long freq = clk_get_rate(wdt->clock);
> unsigned int count;
> unsigned int divisor = 1;
> unsigned long wtcon;
> @@ -192,7 +206,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 +215,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 +256,22 @@ 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_watchdog *wdt = platform_get_drvdata(param);
Please separate variables from code with a blank line.
> + 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_watchdog *wdt = freq_to_wdt(nb);
>
> - if (!s3c2410wdt_is_running())
> + if (!s3c2410wdt_is_running(wdt))
> goto done;
>
> if (val == CPUFREQ_PRECHANGE) {
> @@ -265,14 +280,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,8 +297,8 @@ 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;
> }
>
> @@ -290,25 +306,30 @@ static struct notifier_block
> s3c2410wdt_cpufreq_transition_nb = { .notifier_call =
> s3c2410wdt_cpufreq_transition,
> };
I think the above notifier_block is no longer used after your changes, as
the compiler complains:
drivers/watchdog/s3c2410_wdt.c:305:30: warning:
‘s3c2410wdt_cpufreq_transition_nb’ defined but not used
>
> -static inline int s3c2410wdt_cpufreq_register(void)
> +static inline int s3c2410wdt_cpufreq_register(struct s3c2410_watchdog
> *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_watchdog
> *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_watchdog
> *wdt) {
> return 0;
> }
>
> -static inline void s3c2410wdt_cpufreq_deregister(void)
> +static inline void s3c2410wdt_cpufreq_deregister(struct s3c2410_watchdog
> *wdt) {
> }
> #endif
> @@ -316,6 +337,9 @@ static inline void
> s3c2410wdt_cpufreq_deregister(void) static int s3c2410wdt_probe(struct
> platform_device *pdev)
> {
> struct device *dev;
> + struct s3c2410_watchdog *wdt;
> + struct resource *wdt_mem;
> + struct resource *wdt_irq;
> unsigned int wtcon;
> int started = 0;
> int ret;
> @@ -323,8 +347,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 +369,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 +395,10 @@ 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);
> + if (s3c2410wdt_set_heartbeat(&wdt->wdt_device,
> + wdt->wdt_device.timeout)) {
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 +417,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 +427,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,35 +450,33 @@ 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_watchdog *wdt = platform_get_drvdata(dev);
+ Blank line.
> + watchdog_unregister_device(&wdt->wdt_device);
>
> - s3c2410wdt_cpufreq_deregister();
> + s3c2410wdt_cpufreq_deregister(wdt);
>
> - clk_disable_unprepare(wdt_clock);
> - wdt_clock = NULL;
> + 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_watchdog *wdt = platform_get_drvdata(dev);
+ Blank line.
> + s3c2410wdt_stop(&wdt->wdt_device);
> }
>
> #ifdef CONFIG_PM_SLEEP
Please also move the two save variables that are here to the structure...
> @@ -455,23 +486,26 @@ static unsigned long wtdat_save;
>
> static int s3c2410wdt_suspend(struct device *dev)
> {
> + struct s3c2410_watchdog *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);
> + wtcon_save = readl(wdt->reg_base + S3C2410_WTCON);
> + wtdat_save = readl(wdt->reg_base + S3C2410_WTDAT);
...and use them here instead those global ones.
> /* 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)
> {
> + struct s3c2410_watchdog *wdt = dev_get_drvdata(dev);
> +
> /* Restore watchdog state. */
>
> - writel(wtdat_save, wdt_base + S3C2410_WTDAT);
> - writel(wtdat_save, wdt_base + S3C2410_WTCNT); /* Reset count */
> - writel(wtcon_save, wdt_base + S3C2410_WTCON);
> + writel(wtdat_save, wdt->reg_base + S3C2410_WTDAT);
> + writel(wtdat_save, wdt->reg_base + S3C2410_WTCNT); /* Reset count */
> + writel(wtcon_save, wdt->reg_base + S3C2410_WTCON);
And here.
Best regards,
Tomasz
--
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
