[PATCH v2 0/2] watchdog: da9063: small improvements

[PATCH v2 0/2] watchdog: da9063: small improvements

[Marco Felsch]

The intention of this series is to improve the code quality and the
readability. These patches implementing the results of the discussion
https://www.spinics.net/lists/linux-watchdog/msg13940.html. Since the
"fixes" patche series is already applied on watchdog-next, this patch
series is based on the watchdog-next branch
(git://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git)
instead of the mainline kernel.

Based on Steve's statement, the da9061/2 and da9063 driver merge was
omitted.

Changelog:

v2:
  - fix compile error due to regval typo
  - fix "line over 80 characters" warning for
    da9063_wdt_update_timeout()

Marco Felsch (2):
  watchdog: da9063: rename helper function to avoid misunderstandings
  watchdog: da9063: remove duplicated timeout_to_sel calls

 drivers/watchdog/da9063_wdt.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

-- 
2.17.1

[PATCH v2 1/2] watchdog: da9063: rename helper function to avoid misunderstandings

[Marco Felsch]

_da9063_wdt_set_timeout() is called by da9063_wdg_set_timeout(),
da9063_wdg_start() and da9063_wdg_probe() but the name expect only to be
called by da9063_wdg_set_timeout(). Rename the function to avoid
misunderstandings.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/watchdog/da9063_wdt.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/watchdog/da9063_wdt.c b/drivers/watchdog/da9063_wdt.c
index a4380a887e85..760aa9bca81b 100644
--- a/drivers/watchdog/da9063_wdt.c
+++ b/drivers/watchdog/da9063_wdt.c
@@ -64,7 +64,7 @@ static int da9063_wdt_disable_timer(struct da9063 *da9063)
 				  DA9063_TWDSCALE_DISABLE);
 }
 
-static int _da9063_wdt_set_timeout(struct da9063 *da9063, unsigned int regval)
+static int da9063_wdt_update_timeout(struct da9063 *da9063, unsigned int regval)
 {
 	int ret;
 
@@ -93,7 +93,7 @@ static int da9063_wdt_start(struct watchdog_device *wdd)
 	int ret;
 
 	selector = da9063_wdt_timeout_to_sel(wdd->timeout);
-	ret = _da9063_wdt_set_timeout(da9063, selector);
+	ret = da9063_wdt_update_timeout(da9063, selector);
 	if (ret)
 		dev_err(da9063->dev, "Watchdog failed to start (err = %d)\n",
 			ret);
@@ -148,7 +148,7 @@ static int da9063_wdt_set_timeout(struct watchdog_device *wdd,
 	 * enabling the watchdog, so the timeout must be buffered by the driver.
 	 */
 	if (watchdog_active(wdd))
-		ret = _da9063_wdt_set_timeout(da9063, selector);
+		ret = da9063_wdt_update_timeout(da9063, selector);
 
 	if (ret)
 		dev_err(da9063->dev, "Failed to set watchdog timeout (err = %d)\n",
@@ -223,7 +223,7 @@ static int da9063_wdt_probe(struct platform_device *pdev)
 		unsigned int timeout;
 
 		timeout = da9063_wdt_timeout_to_sel(DA9063_WDG_TIMEOUT);
-		_da9063_wdt_set_timeout(da9063, timeout);
+		da9063_wdt_update_timeout(da9063, timeout);
 		set_bit(WDOG_HW_RUNNING, &wdd->status);
 	}
 
-- 
2.17.1

Re: [PATCH v2 1/2] watchdog: da9063: rename helper function to avoid misunderstandings

[Guenter Roeck]

On Mon, Jun 04, 2018 at 05:00:58PM +0200, Marco Felsch wrote:
> _da9063_wdt_set_timeout() is called by da9063_wdg_set_timeout(),
> da9063_wdg_start() and da9063_wdg_probe() but the name expect only to be
> called by da9063_wdg_set_timeout(). Rename the function to avoid
> misunderstandings.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>

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

> ---
>  drivers/watchdog/da9063_wdt.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/watchdog/da9063_wdt.c b/drivers/watchdog/da9063_wdt.c
> index a4380a887e85..760aa9bca81b 100644
> --- a/drivers/watchdog/da9063_wdt.c
> +++ b/drivers/watchdog/da9063_wdt.c
> @@ -64,7 +64,7 @@ static int da9063_wdt_disable_timer(struct da9063 *da9063)
>  				  DA9063_TWDSCALE_DISABLE);
>  }
>  
> -static int _da9063_wdt_set_timeout(struct da9063 *da9063, unsigned int regval)
> +static int da9063_wdt_update_timeout(struct da9063 *da9063, unsigned int regval)
>  {
>  	int ret;
>  
> @@ -93,7 +93,7 @@ static int da9063_wdt_start(struct watchdog_device *wdd)
>  	int ret;
>  
>  	selector = da9063_wdt_timeout_to_sel(wdd->timeout);
> -	ret = _da9063_wdt_set_timeout(da9063, selector);
> +	ret = da9063_wdt_update_timeout(da9063, selector);
>  	if (ret)
>  		dev_err(da9063->dev, "Watchdog failed to start (err = %d)\n",
>  			ret);
> @@ -148,7 +148,7 @@ static int da9063_wdt_set_timeout(struct watchdog_device *wdd,
>  	 * enabling the watchdog, so the timeout must be buffered by the driver.
>  	 */
>  	if (watchdog_active(wdd))
> -		ret = _da9063_wdt_set_timeout(da9063, selector);
> +		ret = da9063_wdt_update_timeout(da9063, selector);
>  
>  	if (ret)
>  		dev_err(da9063->dev, "Failed to set watchdog timeout (err = %d)\n",
> @@ -223,7 +223,7 @@ static int da9063_wdt_probe(struct platform_device *pdev)
>  		unsigned int timeout;
>  
>  		timeout = da9063_wdt_timeout_to_sel(DA9063_WDG_TIMEOUT);
> -		_da9063_wdt_set_timeout(da9063, timeout);
> +		da9063_wdt_update_timeout(da9063, timeout);
>  		set_bit(WDOG_HW_RUNNING, &wdd->status);
>  	}
>  
> -- 
> 2.17.1
> 

RE: [PATCH v2 1/2] watchdog: da9063: rename helper function to avoid misunderstandings

[Steve Twiss]

On 04 June 2018 16:01 Marco Felsch wrote,

> Subject: [PATCH v2 1/2] watchdog: da9063: rename helper function to avoid
> misunderstandings
> 
> _da9063_wdt_set_timeout() is called by da9063_wdg_set_timeout(),
> da9063_wdg_start() and da9063_wdg_probe() but the name expect only to be
> called by da9063_wdg_set_timeout(). Rename the function to avoid
> misunderstandings.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
>  drivers/watchdog/da9063_wdt.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/watchdog/da9063_wdt.c b/drivers/watchdog/da9063_wdt.c
> index a4380a887e85..760aa9bca81b 100644
> --- a/drivers/watchdog/da9063_wdt.c
> +++ b/drivers/watchdog/da9063_wdt.c
> @@ -64,7 +64,7 @@ static int da9063_wdt_disable_timer(struct da9063 *da9063)
>  				  DA9063_TWDSCALE_DISABLE);
>  }
> 
> -static int _da9063_wdt_set_timeout(struct da9063 *da9063, unsigned int regval)
> +static int da9063_wdt_update_timeout(struct da9063 *da9063, unsigned int
> regval)
>  {
>  	int ret;
> 
> @@ -93,7 +93,7 @@ static int da9063_wdt_start(struct watchdog_device *wdd)
>  	int ret;
> 
>  	selector = da9063_wdt_timeout_to_sel(wdd->timeout);
> -	ret = _da9063_wdt_set_timeout(da9063, selector);
> +	ret = da9063_wdt_update_timeout(da9063, selector);
>  	if (ret)
>  		dev_err(da9063->dev, "Watchdog failed to start (err = %d)\n",
>  			ret);
> @@ -148,7 +148,7 @@ static int da9063_wdt_set_timeout(struct
> watchdog_device *wdd,
>  	 * enabling the watchdog, so the timeout must be buffered by the driver.
>  	 */
>  	if (watchdog_active(wdd))
> -		ret = _da9063_wdt_set_timeout(da9063, selector);
> +		ret = da9063_wdt_update_timeout(da9063, selector);
> 
>  	if (ret)
>  		dev_err(da9063->dev, "Failed to set watchdog timeout (err =
> %d)\n",
> @@ -223,7 +223,7 @@ static int da9063_wdt_probe(struct platform_device
> *pdev)
>  		unsigned int timeout;
> 
>  		timeout =
> da9063_wdt_timeout_to_sel(DA9063_WDG_TIMEOUT);
> -		_da9063_wdt_set_timeout(da9063, timeout);
> +		da9063_wdt_update_timeout(da9063, timeout);
>  		set_bit(WDOG_HW_RUNNING, &wdd->status);
>  	}
> 
> --
> 2.17.1

Hi Marco,

Based upon this patch applied to:
  git://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git
branch
  remotes/origin/watchdog-next
and git describe
  v4.17-rc1-344-gaeef812

Acked-by: Steve Twiss <stwiss.opensource@diasemi.com>

Regards,
Steve

[PATCH v2 2/2] watchdog: da9063: remove duplicated timeout_to_sel calls

[Marco Felsch]

Every time da9063_wdt_update_timeout() gets called a timeout_to_sel() is
made because the timeout argument of update_timeout() is the raw
register value. Moving the second<->raw-value translation into
da9063_wdt_update_timeout() removes duplicated code.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/watchdog/da9063_wdt.c | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/drivers/watchdog/da9063_wdt.c b/drivers/watchdog/da9063_wdt.c
index 760aa9bca81b..384dca16af8b 100644
--- a/drivers/watchdog/da9063_wdt.c
+++ b/drivers/watchdog/da9063_wdt.c
@@ -64,8 +64,10 @@ static int da9063_wdt_disable_timer(struct da9063 *da9063)
 				  DA9063_TWDSCALE_DISABLE);
 }
 
-static int da9063_wdt_update_timeout(struct da9063 *da9063, unsigned int regval)
+static int
+da9063_wdt_update_timeout(struct da9063 *da9063, unsigned int timeout)
 {
+	unsigned int regval;
 	int ret;
 
 	/*
@@ -81,6 +83,7 @@ static int da9063_wdt_update_timeout(struct da9063 *da9063, unsigned int regval)
 		return ret;
 
 	usleep_range(150, 300);
+	regval = da9063_wdt_timeout_to_sel(timeout);
 
 	return regmap_update_bits(da9063->regmap, DA9063_REG_CONTROL_D,
 				  DA9063_TWDSCALE_MASK, regval);
@@ -89,11 +92,9 @@ static int da9063_wdt_update_timeout(struct da9063 *da9063, unsigned int regval)
 static int da9063_wdt_start(struct watchdog_device *wdd)
 {
 	struct da9063 *da9063 = watchdog_get_drvdata(wdd);
-	unsigned int selector;
 	int ret;
 
-	selector = da9063_wdt_timeout_to_sel(wdd->timeout);
-	ret = da9063_wdt_update_timeout(da9063, selector);
+	ret = da9063_wdt_update_timeout(da9063, wdd->timeout);
 	if (ret)
 		dev_err(da9063->dev, "Watchdog failed to start (err = %d)\n",
 			ret);
@@ -132,11 +133,8 @@ static int da9063_wdt_set_timeout(struct watchdog_device *wdd,
 				  unsigned int timeout)
 {
 	struct da9063 *da9063 = watchdog_get_drvdata(wdd);
-	unsigned int selector;
 	int ret = 0;
 
-	selector = da9063_wdt_timeout_to_sel(timeout);
-
 	/*
 	 * There are two cases when a set_timeout() will be called:
 	 * 1. The watchdog is off and someone wants to set the timeout for the
@@ -148,13 +146,13 @@ static int da9063_wdt_set_timeout(struct watchdog_device *wdd,
 	 * enabling the watchdog, so the timeout must be buffered by the driver.
 	 */
 	if (watchdog_active(wdd))
-		ret = da9063_wdt_update_timeout(da9063, selector);
+		ret = da9063_wdt_update_timeout(da9063, timeout);
 
 	if (ret)
 		dev_err(da9063->dev, "Failed to set watchdog timeout (err = %d)\n",
 			ret);
 	else
-		wdd->timeout = wdt_timeout[selector];
+		wdd->timeout = wdt_timeout[da9063_wdt_timeout_to_sel(timeout)];
 
 	return ret;
 }
@@ -220,10 +218,7 @@ static int da9063_wdt_probe(struct platform_device *pdev)
 
 	/* Change the timeout to the default value if the watchdog is running */
 	if (da9063_wdt_is_running(da9063)) {
-		unsigned int timeout;
-
-		timeout = da9063_wdt_timeout_to_sel(DA9063_WDG_TIMEOUT);
-		da9063_wdt_update_timeout(da9063, timeout);
+		da9063_wdt_update_timeout(da9063, DA9063_WDG_TIMEOUT);
 		set_bit(WDOG_HW_RUNNING, &wdd->status);
 	}
 
-- 
2.17.1

Re: [PATCH v2 2/2] watchdog: da9063: remove duplicated timeout_to_sel calls

[Guenter Roeck]

On Mon, Jun 04, 2018 at 05:00:59PM +0200, Marco Felsch wrote:
> Every time da9063_wdt_update_timeout() gets called a timeout_to_sel() is
> made because the timeout argument of update_timeout() is the raw
> register value. Moving the second<->raw-value translation into
> da9063_wdt_update_timeout() removes duplicated code.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>

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

> ---
>  drivers/watchdog/da9063_wdt.c | 21 ++++++++-------------
>  1 file changed, 8 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/watchdog/da9063_wdt.c b/drivers/watchdog/da9063_wdt.c
> index 760aa9bca81b..384dca16af8b 100644
> --- a/drivers/watchdog/da9063_wdt.c
> +++ b/drivers/watchdog/da9063_wdt.c
> @@ -64,8 +64,10 @@ static int da9063_wdt_disable_timer(struct da9063 *da9063)
>  				  DA9063_TWDSCALE_DISABLE);
>  }
>  
> -static int da9063_wdt_update_timeout(struct da9063 *da9063, unsigned int regval)
> +static int
> +da9063_wdt_update_timeout(struct da9063 *da9063, unsigned int timeout)
>  {
> +	unsigned int regval;
>  	int ret;
>  
>  	/*
> @@ -81,6 +83,7 @@ static int da9063_wdt_update_timeout(struct da9063 *da9063, unsigned int regval)
>  		return ret;
>  
>  	usleep_range(150, 300);
> +	regval = da9063_wdt_timeout_to_sel(timeout);
>  
>  	return regmap_update_bits(da9063->regmap, DA9063_REG_CONTROL_D,
>  				  DA9063_TWDSCALE_MASK, regval);
> @@ -89,11 +92,9 @@ static int da9063_wdt_update_timeout(struct da9063 *da9063, unsigned int regval)
>  static int da9063_wdt_start(struct watchdog_device *wdd)
>  {
>  	struct da9063 *da9063 = watchdog_get_drvdata(wdd);
> -	unsigned int selector;
>  	int ret;
>  
> -	selector = da9063_wdt_timeout_to_sel(wdd->timeout);
> -	ret = da9063_wdt_update_timeout(da9063, selector);
> +	ret = da9063_wdt_update_timeout(da9063, wdd->timeout);
>  	if (ret)
>  		dev_err(da9063->dev, "Watchdog failed to start (err = %d)\n",
>  			ret);
> @@ -132,11 +133,8 @@ static int da9063_wdt_set_timeout(struct watchdog_device *wdd,
>  				  unsigned int timeout)
>  {
>  	struct da9063 *da9063 = watchdog_get_drvdata(wdd);
> -	unsigned int selector;
>  	int ret = 0;
>  
> -	selector = da9063_wdt_timeout_to_sel(timeout);
> -
>  	/*
>  	 * There are two cases when a set_timeout() will be called:
>  	 * 1. The watchdog is off and someone wants to set the timeout for the
> @@ -148,13 +146,13 @@ static int da9063_wdt_set_timeout(struct watchdog_device *wdd,
>  	 * enabling the watchdog, so the timeout must be buffered by the driver.
>  	 */
>  	if (watchdog_active(wdd))
> -		ret = da9063_wdt_update_timeout(da9063, selector);
> +		ret = da9063_wdt_update_timeout(da9063, timeout);
>  
>  	if (ret)
>  		dev_err(da9063->dev, "Failed to set watchdog timeout (err = %d)\n",
>  			ret);
>  	else
> -		wdd->timeout = wdt_timeout[selector];
> +		wdd->timeout = wdt_timeout[da9063_wdt_timeout_to_sel(timeout)];
>  
>  	return ret;
>  }
> @@ -220,10 +218,7 @@ static int da9063_wdt_probe(struct platform_device *pdev)
>  
>  	/* Change the timeout to the default value if the watchdog is running */
>  	if (da9063_wdt_is_running(da9063)) {
> -		unsigned int timeout;
> -
> -		timeout = da9063_wdt_timeout_to_sel(DA9063_WDG_TIMEOUT);
> -		da9063_wdt_update_timeout(da9063, timeout);
> +		da9063_wdt_update_timeout(da9063, DA9063_WDG_TIMEOUT);
>  		set_bit(WDOG_HW_RUNNING, &wdd->status);
>  	}
>  
> -- 
> 2.17.1
> 

RE: [PATCH v2 2/2] watchdog: da9063: remove duplicated timeout_to_sel calls

[Steve Twiss]

On 04 June 2018 16:01 Marco Felsch wrote,

> Subject: [PATCH v2 2/2] watchdog: da9063: remove duplicated timeout_to_sel calls
> 
> Every time da9063_wdt_update_timeout() gets called a timeout_to_sel() is
> made because the timeout argument of update_timeout() is the raw
> register value. Moving the second<->raw-value translation into
> da9063_wdt_update_timeout() removes duplicated code.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
>  drivers/watchdog/da9063_wdt.c | 21 ++++++++-------------
>  1 file changed, 8 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/watchdog/da9063_wdt.c b/drivers/watchdog/da9063_wdt.c
> index 760aa9bca81b..384dca16af8b 100644
> --- a/drivers/watchdog/da9063_wdt.c
> +++ b/drivers/watchdog/da9063_wdt.c
> @@ -64,8 +64,10 @@ static int da9063_wdt_disable_timer(struct da9063
> *da9063)
>  				  DA9063_TWDSCALE_DISABLE);
>  }
> 
> -static int da9063_wdt_update_timeout(struct da9063 *da9063, unsigned int
> regval)
> +static int
> +da9063_wdt_update_timeout(struct da9063 *da9063, unsigned int timeout)
>  {
> +	unsigned int regval;
>  	int ret;
> 
>  	/*
> @@ -81,6 +83,7 @@ static int da9063_wdt_update_timeout(struct da9063
> *da9063, unsigned int regval)
>  		return ret;
> 
>  	usleep_range(150, 300);
> +	regval = da9063_wdt_timeout_to_sel(timeout);
> 
>  	return regmap_update_bits(da9063->regmap,
> DA9063_REG_CONTROL_D,
>  				  DA9063_TWDSCALE_MASK, regval);
> @@ -89,11 +92,9 @@ static int da9063_wdt_update_timeout(struct da9063
> *da9063, unsigned int regval)
>  static int da9063_wdt_start(struct watchdog_device *wdd)
>  {
>  	struct da9063 *da9063 = watchdog_get_drvdata(wdd);
> -	unsigned int selector;
>  	int ret;
> 
> -	selector = da9063_wdt_timeout_to_sel(wdd->timeout);
> -	ret = da9063_wdt_update_timeout(da9063, selector);
> +	ret = da9063_wdt_update_timeout(da9063, wdd->timeout);
>  	if (ret)
>  		dev_err(da9063->dev, "Watchdog failed to start (err = %d)\n",
>  			ret);
> @@ -132,11 +133,8 @@ static int da9063_wdt_set_timeout(struct
> watchdog_device *wdd,
>  				  unsigned int timeout)
>  {
>  	struct da9063 *da9063 = watchdog_get_drvdata(wdd);
> -	unsigned int selector;
>  	int ret = 0;
> 
> -	selector = da9063_wdt_timeout_to_sel(timeout);
> -
>  	/*
>  	 * There are two cases when a set_timeout() will be called:
>  	 * 1. The watchdog is off and someone wants to set the timeout for the
> @@ -148,13 +146,13 @@ static int da9063_wdt_set_timeout(struct
> watchdog_device *wdd,
>  	 * enabling the watchdog, so the timeout must be buffered by the driver.
>  	 */
>  	if (watchdog_active(wdd))
> -		ret = da9063_wdt_update_timeout(da9063, selector);
> +		ret = da9063_wdt_update_timeout(da9063, timeout);
> 
>  	if (ret)
>  		dev_err(da9063->dev, "Failed to set watchdog timeout (err =
> %d)\n",
>  			ret);
>  	else
> -		wdd->timeout = wdt_timeout[selector];
> +		wdd->timeout =
> wdt_timeout[da9063_wdt_timeout_to_sel(timeout)];
> 
>  	return ret;
>  }
> @@ -220,10 +218,7 @@ static int da9063_wdt_probe(struct platform_device
> *pdev)
> 
>  	/* Change the timeout to the default value if the watchdog is running */
>  	if (da9063_wdt_is_running(da9063)) {
> -		unsigned int timeout;
> -
> -		timeout =
> da9063_wdt_timeout_to_sel(DA9063_WDG_TIMEOUT);
> -		da9063_wdt_update_timeout(da9063, timeout);
> +		da9063_wdt_update_timeout(da9063, DA9063_WDG_TIMEOUT);
>  		set_bit(WDOG_HW_RUNNING, &wdd->status);
>  	}
> 
> --
> 2.17.1

Hi Marco,

Thanks.

Based upon this patch applied to:
  git://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git
branch
  remotes/origin/watchdog-next
and git describe
  v4.17-rc1-344-gaeef812 + patch [PATCH v2 1/2] watchdog: da9063: rename helper function to avoid misunderstandings

Acked-by: Steve Twiss <stwiss.opensource@diasemi.com>

Regards,
Steve