Sender: Guenter Roeck <groeck7@gmail.com>
Date:   Tue, 7 Mar 2023 09:46:05 -0800
From:   Guenter Roeck <linux@roeck-us.net>
To:     Sergio Paracuellos <sergio.paracuellos@gmail.com>
Cc:     linux-watchdog@vger.kernel.org, wim@linux-watchdog.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/2] watchdog: rt2880-wdt: avoid static global
 declarations
Message-ID: <9c8de316-404d-40e4-85df-910b5d1b09bf@roeck-us.net>
References: <20230301065510.2818425-1-sergio.paracuellos@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230301065510.2818425-1-sergio.paracuellos@gmail.com>
Precedence: bulk

On Wed, Mar 01, 2023 at 07:55:09AM +0100, Sergio Paracuellos wrote:
> Instead of using static global definitions in driver code, refactor code
> introducing a new watchdog driver data structure and use it along the code.
> 
> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>  drivers/watchdog/rt2880_wdt.c | 89 ++++++++++++++++++++---------------
>  1 file changed, 50 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/watchdog/rt2880_wdt.c b/drivers/watchdog/rt2880_wdt.c
> index 49aff800824d..e54737bcf939 100644
> --- a/drivers/watchdog/rt2880_wdt.c
> +++ b/drivers/watchdog/rt2880_wdt.c
> @@ -40,10 +40,13 @@
>  #define TMR1CTL_PRESCALE_MASK		0xf
>  #define TMR1CTL_PRESCALE_65536		0xf
>  
> -static struct clk *rt288x_wdt_clk;
> -static unsigned long rt288x_wdt_freq;
> -static void __iomem *rt288x_wdt_base;
> -static struct reset_control *rt288x_wdt_reset;
> +struct rt2880_wdt_data {
> +	void __iomem *base;
> +	unsigned long freq;
> +	struct clk *clk;
> +	struct reset_control *rst;
> +	struct watchdog_device wdt;
> +};
>  
>  static bool nowayout = WATCHDOG_NOWAYOUT;
>  module_param(nowayout, bool, 0);
> @@ -51,52 +54,56 @@ MODULE_PARM_DESC(nowayout,
>  		"Watchdog cannot be stopped once started (default="
>  		__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
>  
> -static inline void rt_wdt_w32(unsigned reg, u32 val)
> +static inline void rt_wdt_w32(void __iomem *base, unsigned reg, u32 val)
>  {
> -	iowrite32(val, rt288x_wdt_base + reg);
> +	iowrite32(val, base + reg);
>  }
>  
> -static inline u32 rt_wdt_r32(unsigned reg)
> +static inline u32 rt_wdt_r32(void __iomem *base, unsigned reg)
>  {
> -	return ioread32(rt288x_wdt_base + reg);
> +	return ioread32(base + reg);
>  }
>  
>  static int rt288x_wdt_ping(struct watchdog_device *w)
>  {
> -	rt_wdt_w32(TIMER_REG_TMR1LOAD, w->timeout * rt288x_wdt_freq);
> +	struct rt2880_wdt_data *drvdata = watchdog_get_drvdata(w);
> +
> +	rt_wdt_w32(drvdata->base, TIMER_REG_TMR1LOAD, w->timeout * drvdata->freq);
>  
>  	return 0;
>  }
>  
>  static int rt288x_wdt_start(struct watchdog_device *w)
>  {
> +	struct rt2880_wdt_data *drvdata = watchdog_get_drvdata(w);
>  	u32 t;
>  
> -	t = rt_wdt_r32(TIMER_REG_TMR1CTL);
> +	t = rt_wdt_r32(drvdata->base, TIMER_REG_TMR1CTL);
>  	t &= ~(TMR1CTL_MODE_MASK << TMR1CTL_MODE_SHIFT |
>  		TMR1CTL_PRESCALE_MASK);
>  	t |= (TMR1CTL_MODE_WDT << TMR1CTL_MODE_SHIFT |
>  		TMR1CTL_PRESCALE_65536);
> -	rt_wdt_w32(TIMER_REG_TMR1CTL, t);
> +	rt_wdt_w32(drvdata->base, TIMER_REG_TMR1CTL, t);
>  
>  	rt288x_wdt_ping(w);
>  
> -	t = rt_wdt_r32(TIMER_REG_TMR1CTL);
> +	t = rt_wdt_r32(drvdata->base, TIMER_REG_TMR1CTL);
>  	t |= TMR1CTL_ENABLE;
> -	rt_wdt_w32(TIMER_REG_TMR1CTL, t);
> +	rt_wdt_w32(drvdata->base, TIMER_REG_TMR1CTL, t);
>  
>  	return 0;
>  }
>  
>  static int rt288x_wdt_stop(struct watchdog_device *w)
>  {
> +	struct rt2880_wdt_data *drvdata = watchdog_get_drvdata(w);
>  	u32 t;
>  
>  	rt288x_wdt_ping(w);
>  
> -	t = rt_wdt_r32(TIMER_REG_TMR1CTL);
> +	t = rt_wdt_r32(drvdata->base, TIMER_REG_TMR1CTL);
>  	t &= ~TMR1CTL_ENABLE;
> -	rt_wdt_w32(TIMER_REG_TMR1CTL, t);
> +	rt_wdt_w32(drvdata->base, TIMER_REG_TMR1CTL, t);
>  
>  	return 0;
>  }
> @@ -130,41 +137,45 @@ static const struct watchdog_ops rt288x_wdt_ops = {
>  	.set_timeout = rt288x_wdt_set_timeout,
>  };
>  
> -static struct watchdog_device rt288x_wdt_dev = {
> -	.info = &rt288x_wdt_info,
> -	.ops = &rt288x_wdt_ops,
> -	.min_timeout = 1,
> -};
> -
>  static int rt288x_wdt_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> +	struct watchdog_device *wdt;
> +	struct rt2880_wdt_data *drvdata;
>  	int ret;
>  
> -	rt288x_wdt_base = devm_platform_ioremap_resource(pdev, 0);
> -	if (IS_ERR(rt288x_wdt_base))
> -		return PTR_ERR(rt288x_wdt_base);
> +	drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> +	if (!drvdata)
> +		return -ENOMEM;
> +
> +	drvdata->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(drvdata->base))
> +		return PTR_ERR(drvdata->base);
>  
> -	rt288x_wdt_clk = devm_clk_get(dev, NULL);
> -	if (IS_ERR(rt288x_wdt_clk))
> -		return PTR_ERR(rt288x_wdt_clk);
> +	drvdata->clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(drvdata->clk))
> +		return PTR_ERR(drvdata->clk);
>  
> -	rt288x_wdt_reset = devm_reset_control_get_exclusive(dev, NULL);
> -	if (!IS_ERR(rt288x_wdt_reset))
> -		reset_control_deassert(rt288x_wdt_reset);
> +	drvdata->rst = devm_reset_control_get_exclusive(dev, NULL);
> +	if (!IS_ERR(drvdata->rst))
> +		reset_control_deassert(drvdata->rst);
>  
> -	rt288x_wdt_freq = clk_get_rate(rt288x_wdt_clk) / RALINK_WDT_PRESCALE;
> +	drvdata->freq = clk_get_rate(drvdata->clk) / RALINK_WDT_PRESCALE;
>  
> -	rt288x_wdt_dev.bootstatus = rt288x_wdt_bootcause();
> -	rt288x_wdt_dev.max_timeout = (0xfffful / rt288x_wdt_freq);
> -	rt288x_wdt_dev.parent = dev;
> +	wdt = &drvdata->wdt;
> +	wdt->info = &rt288x_wdt_info;
> +	wdt->ops = &rt288x_wdt_ops;
> +	wdt->min_timeout = 1;
> +	wdt->max_timeout = (0xfffful / drvdata->freq);
> +	wdt->parent = dev;
> +	wdt->bootstatus = rt288x_wdt_bootcause();
>  
> -	watchdog_init_timeout(&rt288x_wdt_dev, rt288x_wdt_dev.max_timeout,
> -			      dev);
> -	watchdog_set_nowayout(&rt288x_wdt_dev, nowayout);
> +	watchdog_init_timeout(wdt, wdt->max_timeout, dev);
> +	watchdog_set_nowayout(wdt, nowayout);
> +	watchdog_set_drvdata(wdt, drvdata);
>  
> -	watchdog_stop_on_reboot(&rt288x_wdt_dev);
> -	ret = devm_watchdog_register_device(dev, &rt288x_wdt_dev);
> +	watchdog_stop_on_reboot(wdt);
> +	ret = devm_watchdog_register_device(dev, &drvdata->wdt);
>  	if (!ret)
>  		dev_info(dev, "Initialized\n");
>