[PATCH 0/3] watchdog: omap: several cleanups

[PATCH 0/3] watchdog: omap: several cleanups

[Uwe Kleine-König]

Hello,

this is the current state of my cleanups for the omap watchdog driver.
As overtaking a running watchdog isn't that trivial as expected I'm
sending this out that it doesn't bitrot.

Best regards
Uwe Kleine-König

Uwe Kleine-König (3):
  watchdog: omap: use watchdog_init_timeout
  watchdog: omap: put struct watchdog_device into driver data
  watchdog: omap: simplify assignment of bootstatus

 .../devicetree/bindings/watchdog/omap-wdt.txt      |  9 +--
 drivers/watchdog/omap_wdt.c                        | 66 +++++++++-------------
 2 files changed, 32 insertions(+), 43 deletions(-)

-- 
2.1.4

[PATCH 1/3] watchdog: omap: use watchdog_init_timeout

[Uwe Kleine-König]

Instead of (partly) open coding it use the core function. As a side
effect the "timeout-sec" devicetree property is used now.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 Documentation/devicetree/bindings/watchdog/omap-wdt.txt | 9 +++++----
 drivers/watchdog/omap_wdt.c                             | 5 +----
 2 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/Documentation/devicetree/bindings/watchdog/omap-wdt.txt b/Documentation/devicetree/bindings/watchdog/omap-wdt.txt
index c227970671ea..4f18ec38be2f 100644
--- a/Documentation/devicetree/bindings/watchdog/omap-wdt.txt
+++ b/Documentation/devicetree/bindings/watchdog/omap-wdt.txt
@@ -1,10 +1,11 @@
 TI Watchdog Timer (WDT) Controller for OMAP
 
 Required properties:
-compatible:
-- "ti,omap3-wdt" for OMAP3
-- "ti,omap4-wdt" for OMAP4
-- ti,hwmods: Name of the hwmod associated to the WDT
+- compatible : "ti,omap3-wdt" (for OMAP3) or "ti,omap4-wdt" (for OMAP4)
+- ti,hwmods : Name of the hwmod associated to the WDT
+
+Optional properties:
+- timeout-sec : default watchdog timeout in seconds
 
 Examples:
 
diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c
index 39a6cfcba016..0eb9ca04e672 100644
--- a/drivers/watchdog/omap_wdt.c
+++ b/drivers/watchdog/omap_wdt.c
@@ -234,10 +234,7 @@ static int omap_wdt_probe(struct platform_device *pdev)
 	omap_wdt->min_timeout = TIMER_MARGIN_MIN;
 	omap_wdt->max_timeout = TIMER_MARGIN_MAX;
 
-	if (timer_margin >= TIMER_MARGIN_MIN &&
-	    timer_margin <= TIMER_MARGIN_MAX)
-		omap_wdt->timeout = timer_margin;
-	else
+	if (watchdog_init_timeout(omap_wdt, timer_margin, &pdev->dev) < 0)
 		omap_wdt->timeout = TIMER_MARGIN_DEFAULT;
 
 	watchdog_set_drvdata(omap_wdt, wdev);
-- 
2.1.4

[PATCH 2/3] watchdog: omap: put struct watchdog_device into driver data

[Uwe Kleine-König]

This way only a single allocation is needed (per device). Also this
stops making use of watchdog_{set,get}_drvdata.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/watchdog/omap_wdt.c | 55 ++++++++++++++++++++-------------------------
 1 file changed, 24 insertions(+), 31 deletions(-)

diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c
index 0eb9ca04e672..479e7c8e44f5 100644
--- a/drivers/watchdog/omap_wdt.c
+++ b/drivers/watchdog/omap_wdt.c
@@ -53,7 +53,10 @@ static unsigned timer_margin;
 module_param(timer_margin, uint, 0);
 MODULE_PARM_DESC(timer_margin, "initial watchdog timeout (in seconds)");
 
+#define to_omap_wdt_dev(_wdog)	container_of(_wdog, struct omap_wdt_dev, wdog)
+
 struct omap_wdt_dev {
+	struct watchdog_device wdog;
 	void __iomem    *base;          /* physical */
 	struct device   *dev;
 	bool		omap_wdt_users;
@@ -123,7 +126,7 @@ static void omap_wdt_set_timer(struct omap_wdt_dev *wdev,
 
 static int omap_wdt_start(struct watchdog_device *wdog)
 {
-	struct omap_wdt_dev *wdev = watchdog_get_drvdata(wdog);
+	struct omap_wdt_dev *wdev = to_omap_wdt_dev(wdog);
 	void __iomem *base = wdev->base;
 
 	mutex_lock(&wdev->lock);
@@ -151,7 +154,7 @@ static int omap_wdt_start(struct watchdog_device *wdog)
 
 static int omap_wdt_stop(struct watchdog_device *wdog)
 {
-	struct omap_wdt_dev *wdev = watchdog_get_drvdata(wdog);
+	struct omap_wdt_dev *wdev = to_omap_wdt_dev(wdog);
 
 	mutex_lock(&wdev->lock);
 	omap_wdt_disable(wdev);
@@ -163,7 +166,7 @@ static int omap_wdt_stop(struct watchdog_device *wdog)
 
 static int omap_wdt_ping(struct watchdog_device *wdog)
 {
-	struct omap_wdt_dev *wdev = watchdog_get_drvdata(wdog);
+	struct omap_wdt_dev *wdev = to_omap_wdt_dev(wdog);
 
 	mutex_lock(&wdev->lock);
 	omap_wdt_reload(wdev);
@@ -175,7 +178,7 @@ static int omap_wdt_ping(struct watchdog_device *wdog)
 static int omap_wdt_set_timeout(struct watchdog_device *wdog,
 				unsigned int timeout)
 {
-	struct omap_wdt_dev *wdev = watchdog_get_drvdata(wdog);
+	struct omap_wdt_dev *wdev = to_omap_wdt_dev(wdog);
 
 	mutex_lock(&wdev->lock);
 	omap_wdt_disable(wdev);
@@ -204,16 +207,11 @@ static const struct watchdog_ops omap_wdt_ops = {
 static int omap_wdt_probe(struct platform_device *pdev)
 {
 	struct omap_wd_timer_platform_data *pdata = dev_get_platdata(&pdev->dev);
-	struct watchdog_device *omap_wdt;
 	struct resource *res;
 	struct omap_wdt_dev *wdev;
 	u32 rs;
 	int ret;
 
-	omap_wdt = devm_kzalloc(&pdev->dev, sizeof(*omap_wdt), GFP_KERNEL);
-	if (!omap_wdt)
-		return -ENOMEM;
-
 	wdev = devm_kzalloc(&pdev->dev, sizeof(*wdev), GFP_KERNEL);
 	if (!wdev)
 		return -ENOMEM;
@@ -229,18 +227,17 @@ static int omap_wdt_probe(struct platform_device *pdev)
 	if (IS_ERR(wdev->base))
 		return PTR_ERR(wdev->base);
 
-	omap_wdt->info	      = &omap_wdt_info;
-	omap_wdt->ops	      = &omap_wdt_ops;
-	omap_wdt->min_timeout = TIMER_MARGIN_MIN;
-	omap_wdt->max_timeout = TIMER_MARGIN_MAX;
+	wdev->wdog.info = &omap_wdt_info;
+	wdev->wdog.ops = &omap_wdt_ops;
+	wdev->wdog.min_timeout = TIMER_MARGIN_MIN;
+	wdev->wdog.max_timeout = TIMER_MARGIN_MAX;
 
-	if (watchdog_init_timeout(omap_wdt, timer_margin, &pdev->dev) < 0)
-		omap_wdt->timeout = TIMER_MARGIN_DEFAULT;
+	if (watchdog_init_timeout(&wdev->wdog, timer_margin, &pdev->dev) < 0)
+		wdev->wdog.timeout = TIMER_MARGIN_DEFAULT;
 
-	watchdog_set_drvdata(omap_wdt, wdev);
-	watchdog_set_nowayout(omap_wdt, nowayout);
+	watchdog_set_nowayout(&wdev->wdog, nowayout);
 
-	platform_set_drvdata(pdev, omap_wdt);
+	platform_set_drvdata(pdev, wdev);
 
 	pm_runtime_enable(wdev->dev);
 	pm_runtime_get_sync(wdev->dev);
@@ -249,12 +246,12 @@ static int omap_wdt_probe(struct platform_device *pdev)
 		rs = pdata->read_reset_sources();
 	else
 		rs = 0;
-	omap_wdt->bootstatus = (rs & (1 << OMAP_MPU_WD_RST_SRC_ID_SHIFT)) ?
-				WDIOF_CARDRESET : 0;
+	wdev->wdog.bootstatus = (rs & (1 << OMAP_MPU_WD_RST_SRC_ID_SHIFT)) ?
+		WDIOF_CARDRESET : 0;
 
 	omap_wdt_disable(wdev);
 
-	ret = watchdog_register_device(omap_wdt);
+	ret = watchdog_register_device(&wdev->wdog);
 	if (ret) {
 		pm_runtime_disable(wdev->dev);
 		return ret;
@@ -262,7 +259,7 @@ static int omap_wdt_probe(struct platform_device *pdev)
 
 	pr_info("OMAP Watchdog Timer Rev 0x%02x: initial timeout %d sec\n",
 		readl_relaxed(wdev->base + OMAP_WATCHDOG_REV) & 0xFF,
-		omap_wdt->timeout);
+		wdev->wdog.timeout);
 
 	pm_runtime_put_sync(wdev->dev);
 
@@ -271,8 +268,7 @@ static int omap_wdt_probe(struct platform_device *pdev)
 
 static void omap_wdt_shutdown(struct platform_device *pdev)
 {
-	struct watchdog_device *wdog = platform_get_drvdata(pdev);
-	struct omap_wdt_dev *wdev = watchdog_get_drvdata(wdog);
+	struct omap_wdt_dev *wdev = platform_get_drvdata(pdev);
 
 	mutex_lock(&wdev->lock);
 	if (wdev->omap_wdt_users) {
@@ -284,11 +280,10 @@ static void omap_wdt_shutdown(struct platform_device *pdev)
 
 static int omap_wdt_remove(struct platform_device *pdev)
 {
-	struct watchdog_device *wdog = platform_get_drvdata(pdev);
-	struct omap_wdt_dev *wdev = watchdog_get_drvdata(wdog);
+	struct omap_wdt_dev *wdev = platform_get_drvdata(pdev);
 
 	pm_runtime_disable(wdev->dev);
-	watchdog_unregister_device(wdog);
+	watchdog_unregister_device(&wdev->wdog);
 
 	return 0;
 }
@@ -303,8 +298,7 @@ static int omap_wdt_remove(struct platform_device *pdev)
 
 static int omap_wdt_suspend(struct platform_device *pdev, pm_message_t state)
 {
-	struct watchdog_device *wdog = platform_get_drvdata(pdev);
-	struct omap_wdt_dev *wdev = watchdog_get_drvdata(wdog);
+	struct omap_wdt_dev *wdev = platform_get_drvdata(pdev);
 
 	mutex_lock(&wdev->lock);
 	if (wdev->omap_wdt_users) {
@@ -318,8 +312,7 @@ static int omap_wdt_suspend(struct platform_device *pdev, pm_message_t state)
 
 static int omap_wdt_resume(struct platform_device *pdev)
 {
-	struct watchdog_device *wdog = platform_get_drvdata(pdev);
-	struct omap_wdt_dev *wdev = watchdog_get_drvdata(wdog);
+	struct omap_wdt_dev *wdev = platform_get_drvdata(pdev);
 
 	mutex_lock(&wdev->lock);
 	if (wdev->omap_wdt_users) {
-- 
2.1.4

[PATCH 3/3] watchdog: omap: simplify assignment of bootstatus

[Uwe Kleine-König]

Instead of using an over-long expression involving the ?: operator use
an if and intead of an else branch rely on the fact that the data
structure was allocated using devm_kzalloc. This also allows to put the
used helper variable into a more local scope.

There is no functional change.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/watchdog/omap_wdt.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c
index 479e7c8e44f5..0421c06a6cf0 100644
--- a/drivers/watchdog/omap_wdt.c
+++ b/drivers/watchdog/omap_wdt.c
@@ -209,7 +209,6 @@ static int omap_wdt_probe(struct platform_device *pdev)
 	struct omap_wd_timer_platform_data *pdata = dev_get_platdata(&pdev->dev);
 	struct resource *res;
 	struct omap_wdt_dev *wdev;
-	u32 rs;
 	int ret;
 
 	wdev = devm_kzalloc(&pdev->dev, sizeof(*wdev), GFP_KERNEL);
@@ -242,12 +241,11 @@ static int omap_wdt_probe(struct platform_device *pdev)
 	pm_runtime_enable(wdev->dev);
 	pm_runtime_get_sync(wdev->dev);
 
-	if (pdata && pdata->read_reset_sources)
-		rs = pdata->read_reset_sources();
-	else
-		rs = 0;
-	wdev->wdog.bootstatus = (rs & (1 << OMAP_MPU_WD_RST_SRC_ID_SHIFT)) ?
-		WDIOF_CARDRESET : 0;
+	if (pdata && pdata->read_reset_sources) {
+		u32 rs = pdata->read_reset_sources();
+		if (rs & (1 << OMAP_MPU_WD_RST_SRC_ID_SHIFT))
+			wdev->wdog.bootstatus = WDIOF_CARDRESET;
+	}
 
 	omap_wdt_disable(wdev);
 
-- 
2.1.4

Re: [PATCH 1/3] watchdog: omap: use watchdog_init_timeout

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2139 bytes --]

Hi,

On Fri, Apr 24, 2015 at 11:48:31AM +0200, Uwe Kleine-König wrote:
> Instead of (partly) open coding it use the core function. As a side
> effect the "timeout-sec" devicetree property is used now.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  Documentation/devicetree/bindings/watchdog/omap-wdt.txt | 9 +++++----
>  drivers/watchdog/omap_wdt.c                             | 5 +----
>  2 files changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/omap-wdt.txt b/Documentation/devicetree/bindings/watchdog/omap-wdt.txt
> index c227970671ea..4f18ec38be2f 100644
> --- a/Documentation/devicetree/bindings/watchdog/omap-wdt.txt
> +++ b/Documentation/devicetree/bindings/watchdog/omap-wdt.txt
> @@ -1,10 +1,11 @@
>  TI Watchdog Timer (WDT) Controller for OMAP
>  
>  Required properties:
> -compatible:
> -- "ti,omap3-wdt" for OMAP3
> -- "ti,omap4-wdt" for OMAP4
> -- ti,hwmods: Name of the hwmod associated to the WDT
> +- compatible : "ti,omap3-wdt" (for OMAP3) or "ti,omap4-wdt" (for OMAP4)
> +- ti,hwmods : Name of the hwmod associated to the WDT
> +
> +Optional properties:
> +- timeout-sec : default watchdog timeout in seconds

adding a new property here ? Why ? In fact, none of these DT binding doc
changes fit in $subject. Sure, watchdog_init_timeout() needs
timeout-sec, but no OMAP DT today passes it.

> diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c
> index 39a6cfcba016..0eb9ca04e672 100644
> --- a/drivers/watchdog/omap_wdt.c
> +++ b/drivers/watchdog/omap_wdt.c
> @@ -234,10 +234,7 @@ static int omap_wdt_probe(struct platform_device *pdev)
>  	omap_wdt->min_timeout = TIMER_MARGIN_MIN;
>  	omap_wdt->max_timeout = TIMER_MARGIN_MAX;
>  
> -	if (timer_margin >= TIMER_MARGIN_MIN &&
> -	    timer_margin <= TIMER_MARGIN_MAX)
> -		omap_wdt->timeout = timer_margin;
> -	else
> +	if (watchdog_init_timeout(omap_wdt, timer_margin, &pdev->dev) < 0)
>  		omap_wdt->timeout = TIMER_MARGIN_DEFAULT;
>  
>  	watchdog_set_drvdata(omap_wdt, wdev);
> -- 
> 2.1.4
> 

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/3] watchdog: omap: use watchdog_init_timeout

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2164 bytes --]

Hi,

On Fri, Apr 24, 2015 at 11:48:31AM +0200, Uwe Kleine-König wrote:
> Instead of (partly) open coding it use the core function. As a side
> effect the "timeout-sec" devicetree property is used now.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/watchdog/omap-wdt.txt | 9 +++++----
>  drivers/watchdog/omap_wdt.c                             | 5 +----
>  2 files changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/omap-wdt.txt b/Documentation/devicetree/bindings/watchdog/omap-wdt.txt
> index c227970671ea..4f18ec38be2f 100644
> --- a/Documentation/devicetree/bindings/watchdog/omap-wdt.txt
> +++ b/Documentation/devicetree/bindings/watchdog/omap-wdt.txt
> @@ -1,10 +1,11 @@
>  TI Watchdog Timer (WDT) Controller for OMAP
>  
>  Required properties:
> -compatible:
> -- "ti,omap3-wdt" for OMAP3
> -- "ti,omap4-wdt" for OMAP4
> -- ti,hwmods: Name of the hwmod associated to the WDT
> +- compatible : "ti,omap3-wdt" (for OMAP3) or "ti,omap4-wdt" (for OMAP4)
> +- ti,hwmods : Name of the hwmod associated to the WDT
> +
> +Optional properties:
> +- timeout-sec : default watchdog timeout in seconds

adding a new property here ? Why ? In fact, none of these DT binding doc
changes fit in $subject. Sure, watchdog_init_timeout() needs
timeout-sec, but no OMAP DT today passes it.

> diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c
> index 39a6cfcba016..0eb9ca04e672 100644
> --- a/drivers/watchdog/omap_wdt.c
> +++ b/drivers/watchdog/omap_wdt.c
> @@ -234,10 +234,7 @@ static int omap_wdt_probe(struct platform_device *pdev)
>  	omap_wdt->min_timeout = TIMER_MARGIN_MIN;
>  	omap_wdt->max_timeout = TIMER_MARGIN_MAX;
>  
> -	if (timer_margin >= TIMER_MARGIN_MIN &&
> -	    timer_margin <= TIMER_MARGIN_MAX)
> -		omap_wdt->timeout = timer_margin;
> -	else
> +	if (watchdog_init_timeout(omap_wdt, timer_margin, &pdev->dev) < 0)
>  		omap_wdt->timeout = TIMER_MARGIN_DEFAULT;
>  
>  	watchdog_set_drvdata(omap_wdt, wdev);
> -- 
> 2.1.4
> 

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/3] watchdog: omap: put struct watchdog_device into driver data

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 6806 bytes --]

On Fri, Apr 24, 2015 at 11:48:32AM +0200, Uwe Kleine-König wrote:
> This way only a single allocation is needed (per device). Also this
> stops making use of watchdog_{set,get}_drvdata.

And this is better because ... ?

Nothing against it, just feels like this commit log needs a little more
verbosity

> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/watchdog/omap_wdt.c | 55 ++++++++++++++++++++-------------------------
>  1 file changed, 24 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c
> index 0eb9ca04e672..479e7c8e44f5 100644
> --- a/drivers/watchdog/omap_wdt.c
> +++ b/drivers/watchdog/omap_wdt.c
> @@ -53,7 +53,10 @@ static unsigned timer_margin;
>  module_param(timer_margin, uint, 0);
>  MODULE_PARM_DESC(timer_margin, "initial watchdog timeout (in seconds)");
>  
> +#define to_omap_wdt_dev(_wdog)	container_of(_wdog, struct omap_wdt_dev, wdog)
> +
>  struct omap_wdt_dev {
> +	struct watchdog_device wdog;
>  	void __iomem    *base;          /* physical */
>  	struct device   *dev;
>  	bool		omap_wdt_users;
> @@ -123,7 +126,7 @@ static void omap_wdt_set_timer(struct omap_wdt_dev *wdev,
>  
>  static int omap_wdt_start(struct watchdog_device *wdog)
>  {
> -	struct omap_wdt_dev *wdev = watchdog_get_drvdata(wdog);
> +	struct omap_wdt_dev *wdev = to_omap_wdt_dev(wdog);
>  	void __iomem *base = wdev->base;
>  
>  	mutex_lock(&wdev->lock);
> @@ -151,7 +154,7 @@ static int omap_wdt_start(struct watchdog_device *wdog)
>  
>  static int omap_wdt_stop(struct watchdog_device *wdog)
>  {
> -	struct omap_wdt_dev *wdev = watchdog_get_drvdata(wdog);
> +	struct omap_wdt_dev *wdev = to_omap_wdt_dev(wdog);
>  
>  	mutex_lock(&wdev->lock);
>  	omap_wdt_disable(wdev);
> @@ -163,7 +166,7 @@ static int omap_wdt_stop(struct watchdog_device *wdog)
>  
>  static int omap_wdt_ping(struct watchdog_device *wdog)
>  {
> -	struct omap_wdt_dev *wdev = watchdog_get_drvdata(wdog);
> +	struct omap_wdt_dev *wdev = to_omap_wdt_dev(wdog);
>  
>  	mutex_lock(&wdev->lock);
>  	omap_wdt_reload(wdev);
> @@ -175,7 +178,7 @@ static int omap_wdt_ping(struct watchdog_device *wdog)
>  static int omap_wdt_set_timeout(struct watchdog_device *wdog,
>  				unsigned int timeout)
>  {
> -	struct omap_wdt_dev *wdev = watchdog_get_drvdata(wdog);
> +	struct omap_wdt_dev *wdev = to_omap_wdt_dev(wdog);
>  
>  	mutex_lock(&wdev->lock);
>  	omap_wdt_disable(wdev);
> @@ -204,16 +207,11 @@ static const struct watchdog_ops omap_wdt_ops = {
>  static int omap_wdt_probe(struct platform_device *pdev)
>  {
>  	struct omap_wd_timer_platform_data *pdata = dev_get_platdata(&pdev->dev);
> -	struct watchdog_device *omap_wdt;
>  	struct resource *res;
>  	struct omap_wdt_dev *wdev;
>  	u32 rs;
>  	int ret;
>  
> -	omap_wdt = devm_kzalloc(&pdev->dev, sizeof(*omap_wdt), GFP_KERNEL);
> -	if (!omap_wdt)
> -		return -ENOMEM;
> -
>  	wdev = devm_kzalloc(&pdev->dev, sizeof(*wdev), GFP_KERNEL);
>  	if (!wdev)
>  		return -ENOMEM;
> @@ -229,18 +227,17 @@ static int omap_wdt_probe(struct platform_device *pdev)
>  	if (IS_ERR(wdev->base))
>  		return PTR_ERR(wdev->base);
>  
> -	omap_wdt->info	      = &omap_wdt_info;
> -	omap_wdt->ops	      = &omap_wdt_ops;
> -	omap_wdt->min_timeout = TIMER_MARGIN_MIN;
> -	omap_wdt->max_timeout = TIMER_MARGIN_MAX;
> +	wdev->wdog.info = &omap_wdt_info;
> +	wdev->wdog.ops = &omap_wdt_ops;
> +	wdev->wdog.min_timeout = TIMER_MARGIN_MIN;
> +	wdev->wdog.max_timeout = TIMER_MARGIN_MAX;
>  
> -	if (watchdog_init_timeout(omap_wdt, timer_margin, &pdev->dev) < 0)
> -		omap_wdt->timeout = TIMER_MARGIN_DEFAULT;
> +	if (watchdog_init_timeout(&wdev->wdog, timer_margin, &pdev->dev) < 0)
> +		wdev->wdog.timeout = TIMER_MARGIN_DEFAULT;
>  
> -	watchdog_set_drvdata(omap_wdt, wdev);
> -	watchdog_set_nowayout(omap_wdt, nowayout);
> +	watchdog_set_nowayout(&wdev->wdog, nowayout);
>  
> -	platform_set_drvdata(pdev, omap_wdt);
> +	platform_set_drvdata(pdev, wdev);
>  
>  	pm_runtime_enable(wdev->dev);
>  	pm_runtime_get_sync(wdev->dev);
> @@ -249,12 +246,12 @@ static int omap_wdt_probe(struct platform_device *pdev)
>  		rs = pdata->read_reset_sources();
>  	else
>  		rs = 0;
> -	omap_wdt->bootstatus = (rs & (1 << OMAP_MPU_WD_RST_SRC_ID_SHIFT)) ?
> -				WDIOF_CARDRESET : 0;
> +	wdev->wdog.bootstatus = (rs & (1 << OMAP_MPU_WD_RST_SRC_ID_SHIFT)) ?
> +		WDIOF_CARDRESET : 0;
>  
>  	omap_wdt_disable(wdev);
>  
> -	ret = watchdog_register_device(omap_wdt);
> +	ret = watchdog_register_device(&wdev->wdog);
>  	if (ret) {
>  		pm_runtime_disable(wdev->dev);
>  		return ret;
> @@ -262,7 +259,7 @@ static int omap_wdt_probe(struct platform_device *pdev)
>  
>  	pr_info("OMAP Watchdog Timer Rev 0x%02x: initial timeout %d sec\n",
>  		readl_relaxed(wdev->base + OMAP_WATCHDOG_REV) & 0xFF,
> -		omap_wdt->timeout);
> +		wdev->wdog.timeout);
>  
>  	pm_runtime_put_sync(wdev->dev);
>  
> @@ -271,8 +268,7 @@ static int omap_wdt_probe(struct platform_device *pdev)
>  
>  static void omap_wdt_shutdown(struct platform_device *pdev)
>  {
> -	struct watchdog_device *wdog = platform_get_drvdata(pdev);
> -	struct omap_wdt_dev *wdev = watchdog_get_drvdata(wdog);
> +	struct omap_wdt_dev *wdev = platform_get_drvdata(pdev);
>  
>  	mutex_lock(&wdev->lock);
>  	if (wdev->omap_wdt_users) {
> @@ -284,11 +280,10 @@ static void omap_wdt_shutdown(struct platform_device *pdev)
>  
>  static int omap_wdt_remove(struct platform_device *pdev)
>  {
> -	struct watchdog_device *wdog = platform_get_drvdata(pdev);
> -	struct omap_wdt_dev *wdev = watchdog_get_drvdata(wdog);
> +	struct omap_wdt_dev *wdev = platform_get_drvdata(pdev);
>  
>  	pm_runtime_disable(wdev->dev);
> -	watchdog_unregister_device(wdog);
> +	watchdog_unregister_device(&wdev->wdog);
>  
>  	return 0;
>  }
> @@ -303,8 +298,7 @@ static int omap_wdt_remove(struct platform_device *pdev)
>  
>  static int omap_wdt_suspend(struct platform_device *pdev, pm_message_t state)
>  {
> -	struct watchdog_device *wdog = platform_get_drvdata(pdev);
> -	struct omap_wdt_dev *wdev = watchdog_get_drvdata(wdog);
> +	struct omap_wdt_dev *wdev = platform_get_drvdata(pdev);
>  
>  	mutex_lock(&wdev->lock);
>  	if (wdev->omap_wdt_users) {
> @@ -318,8 +312,7 @@ static int omap_wdt_suspend(struct platform_device *pdev, pm_message_t state)
>  
>  static int omap_wdt_resume(struct platform_device *pdev)
>  {
> -	struct watchdog_device *wdog = platform_get_drvdata(pdev);
> -	struct omap_wdt_dev *wdev = watchdog_get_drvdata(wdog);
> +	struct omap_wdt_dev *wdev = platform_get_drvdata(pdev);
>  
>  	mutex_lock(&wdev->lock);
>  	if (wdev->omap_wdt_users) {
> -- 
> 2.1.4
> 

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/3] watchdog: omap: put struct watchdog_device into driver data

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 6831 bytes --]

On Fri, Apr 24, 2015 at 11:48:32AM +0200, Uwe Kleine-König wrote:
> This way only a single allocation is needed (per device). Also this
> stops making use of watchdog_{set,get}_drvdata.

And this is better because ... ?

Nothing against it, just feels like this commit log needs a little more
verbosity

> Signed-off-by: Uwe Kleine-König <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> ---
>  drivers/watchdog/omap_wdt.c | 55 ++++++++++++++++++++-------------------------
>  1 file changed, 24 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c
> index 0eb9ca04e672..479e7c8e44f5 100644
> --- a/drivers/watchdog/omap_wdt.c
> +++ b/drivers/watchdog/omap_wdt.c
> @@ -53,7 +53,10 @@ static unsigned timer_margin;
>  module_param(timer_margin, uint, 0);
>  MODULE_PARM_DESC(timer_margin, "initial watchdog timeout (in seconds)");
>  
> +#define to_omap_wdt_dev(_wdog)	container_of(_wdog, struct omap_wdt_dev, wdog)
> +
>  struct omap_wdt_dev {
> +	struct watchdog_device wdog;
>  	void __iomem    *base;          /* physical */
>  	struct device   *dev;
>  	bool		omap_wdt_users;
> @@ -123,7 +126,7 @@ static void omap_wdt_set_timer(struct omap_wdt_dev *wdev,
>  
>  static int omap_wdt_start(struct watchdog_device *wdog)
>  {
> -	struct omap_wdt_dev *wdev = watchdog_get_drvdata(wdog);
> +	struct omap_wdt_dev *wdev = to_omap_wdt_dev(wdog);
>  	void __iomem *base = wdev->base;
>  
>  	mutex_lock(&wdev->lock);
> @@ -151,7 +154,7 @@ static int omap_wdt_start(struct watchdog_device *wdog)
>  
>  static int omap_wdt_stop(struct watchdog_device *wdog)
>  {
> -	struct omap_wdt_dev *wdev = watchdog_get_drvdata(wdog);
> +	struct omap_wdt_dev *wdev = to_omap_wdt_dev(wdog);
>  
>  	mutex_lock(&wdev->lock);
>  	omap_wdt_disable(wdev);
> @@ -163,7 +166,7 @@ static int omap_wdt_stop(struct watchdog_device *wdog)
>  
>  static int omap_wdt_ping(struct watchdog_device *wdog)
>  {
> -	struct omap_wdt_dev *wdev = watchdog_get_drvdata(wdog);
> +	struct omap_wdt_dev *wdev = to_omap_wdt_dev(wdog);
>  
>  	mutex_lock(&wdev->lock);
>  	omap_wdt_reload(wdev);
> @@ -175,7 +178,7 @@ static int omap_wdt_ping(struct watchdog_device *wdog)
>  static int omap_wdt_set_timeout(struct watchdog_device *wdog,
>  				unsigned int timeout)
>  {
> -	struct omap_wdt_dev *wdev = watchdog_get_drvdata(wdog);
> +	struct omap_wdt_dev *wdev = to_omap_wdt_dev(wdog);
>  
>  	mutex_lock(&wdev->lock);
>  	omap_wdt_disable(wdev);
> @@ -204,16 +207,11 @@ static const struct watchdog_ops omap_wdt_ops = {
>  static int omap_wdt_probe(struct platform_device *pdev)
>  {
>  	struct omap_wd_timer_platform_data *pdata = dev_get_platdata(&pdev->dev);
> -	struct watchdog_device *omap_wdt;
>  	struct resource *res;
>  	struct omap_wdt_dev *wdev;
>  	u32 rs;
>  	int ret;
>  
> -	omap_wdt = devm_kzalloc(&pdev->dev, sizeof(*omap_wdt), GFP_KERNEL);
> -	if (!omap_wdt)
> -		return -ENOMEM;
> -
>  	wdev = devm_kzalloc(&pdev->dev, sizeof(*wdev), GFP_KERNEL);
>  	if (!wdev)
>  		return -ENOMEM;
> @@ -229,18 +227,17 @@ static int omap_wdt_probe(struct platform_device *pdev)
>  	if (IS_ERR(wdev->base))
>  		return PTR_ERR(wdev->base);
>  
> -	omap_wdt->info	      = &omap_wdt_info;
> -	omap_wdt->ops	      = &omap_wdt_ops;
> -	omap_wdt->min_timeout = TIMER_MARGIN_MIN;
> -	omap_wdt->max_timeout = TIMER_MARGIN_MAX;
> +	wdev->wdog.info = &omap_wdt_info;
> +	wdev->wdog.ops = &omap_wdt_ops;
> +	wdev->wdog.min_timeout = TIMER_MARGIN_MIN;
> +	wdev->wdog.max_timeout = TIMER_MARGIN_MAX;
>  
> -	if (watchdog_init_timeout(omap_wdt, timer_margin, &pdev->dev) < 0)
> -		omap_wdt->timeout = TIMER_MARGIN_DEFAULT;
> +	if (watchdog_init_timeout(&wdev->wdog, timer_margin, &pdev->dev) < 0)
> +		wdev->wdog.timeout = TIMER_MARGIN_DEFAULT;
>  
> -	watchdog_set_drvdata(omap_wdt, wdev);
> -	watchdog_set_nowayout(omap_wdt, nowayout);
> +	watchdog_set_nowayout(&wdev->wdog, nowayout);
>  
> -	platform_set_drvdata(pdev, omap_wdt);
> +	platform_set_drvdata(pdev, wdev);
>  
>  	pm_runtime_enable(wdev->dev);
>  	pm_runtime_get_sync(wdev->dev);
> @@ -249,12 +246,12 @@ static int omap_wdt_probe(struct platform_device *pdev)
>  		rs = pdata->read_reset_sources();
>  	else
>  		rs = 0;
> -	omap_wdt->bootstatus = (rs & (1 << OMAP_MPU_WD_RST_SRC_ID_SHIFT)) ?
> -				WDIOF_CARDRESET : 0;
> +	wdev->wdog.bootstatus = (rs & (1 << OMAP_MPU_WD_RST_SRC_ID_SHIFT)) ?
> +		WDIOF_CARDRESET : 0;
>  
>  	omap_wdt_disable(wdev);
>  
> -	ret = watchdog_register_device(omap_wdt);
> +	ret = watchdog_register_device(&wdev->wdog);
>  	if (ret) {
>  		pm_runtime_disable(wdev->dev);
>  		return ret;
> @@ -262,7 +259,7 @@ static int omap_wdt_probe(struct platform_device *pdev)
>  
>  	pr_info("OMAP Watchdog Timer Rev 0x%02x: initial timeout %d sec\n",
>  		readl_relaxed(wdev->base + OMAP_WATCHDOG_REV) & 0xFF,
> -		omap_wdt->timeout);
> +		wdev->wdog.timeout);
>  
>  	pm_runtime_put_sync(wdev->dev);
>  
> @@ -271,8 +268,7 @@ static int omap_wdt_probe(struct platform_device *pdev)
>  
>  static void omap_wdt_shutdown(struct platform_device *pdev)
>  {
> -	struct watchdog_device *wdog = platform_get_drvdata(pdev);
> -	struct omap_wdt_dev *wdev = watchdog_get_drvdata(wdog);
> +	struct omap_wdt_dev *wdev = platform_get_drvdata(pdev);
>  
>  	mutex_lock(&wdev->lock);
>  	if (wdev->omap_wdt_users) {
> @@ -284,11 +280,10 @@ static void omap_wdt_shutdown(struct platform_device *pdev)
>  
>  static int omap_wdt_remove(struct platform_device *pdev)
>  {
> -	struct watchdog_device *wdog = platform_get_drvdata(pdev);
> -	struct omap_wdt_dev *wdev = watchdog_get_drvdata(wdog);
> +	struct omap_wdt_dev *wdev = platform_get_drvdata(pdev);
>  
>  	pm_runtime_disable(wdev->dev);
> -	watchdog_unregister_device(wdog);
> +	watchdog_unregister_device(&wdev->wdog);
>  
>  	return 0;
>  }
> @@ -303,8 +298,7 @@ static int omap_wdt_remove(struct platform_device *pdev)
>  
>  static int omap_wdt_suspend(struct platform_device *pdev, pm_message_t state)
>  {
> -	struct watchdog_device *wdog = platform_get_drvdata(pdev);
> -	struct omap_wdt_dev *wdev = watchdog_get_drvdata(wdog);
> +	struct omap_wdt_dev *wdev = platform_get_drvdata(pdev);
>  
>  	mutex_lock(&wdev->lock);
>  	if (wdev->omap_wdt_users) {
> @@ -318,8 +312,7 @@ static int omap_wdt_suspend(struct platform_device *pdev, pm_message_t state)
>  
>  static int omap_wdt_resume(struct platform_device *pdev)
>  {
> -	struct watchdog_device *wdog = platform_get_drvdata(pdev);
> -	struct omap_wdt_dev *wdev = watchdog_get_drvdata(wdog);
> +	struct omap_wdt_dev *wdev = platform_get_drvdata(pdev);
>  
>  	mutex_lock(&wdev->lock);
>  	if (wdev->omap_wdt_users) {
> -- 
> 2.1.4
> 

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 3/3] watchdog: omap: simplify assignment of bootstatus

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1767 bytes --]

On Fri, Apr 24, 2015 at 11:48:33AM +0200, Uwe Kleine-König wrote:
> Instead of using an over-long expression involving the ?: operator use
> an if and intead of an else branch rely on the fact that the data
> structure was allocated using devm_kzalloc. This also allows to put the
> used helper variable into a more local scope.
> 
> There is no functional change.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

looks good to me

Reviewed-by: Felipe Balbi <balbi@ti.com>

> ---
>  drivers/watchdog/omap_wdt.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c
> index 479e7c8e44f5..0421c06a6cf0 100644
> --- a/drivers/watchdog/omap_wdt.c
> +++ b/drivers/watchdog/omap_wdt.c
> @@ -209,7 +209,6 @@ static int omap_wdt_probe(struct platform_device *pdev)
>  	struct omap_wd_timer_platform_data *pdata = dev_get_platdata(&pdev->dev);
>  	struct resource *res;
>  	struct omap_wdt_dev *wdev;
> -	u32 rs;
>  	int ret;
>  
>  	wdev = devm_kzalloc(&pdev->dev, sizeof(*wdev), GFP_KERNEL);
> @@ -242,12 +241,11 @@ static int omap_wdt_probe(struct platform_device *pdev)
>  	pm_runtime_enable(wdev->dev);
>  	pm_runtime_get_sync(wdev->dev);
>  
> -	if (pdata && pdata->read_reset_sources)
> -		rs = pdata->read_reset_sources();
> -	else
> -		rs = 0;
> -	wdev->wdog.bootstatus = (rs & (1 << OMAP_MPU_WD_RST_SRC_ID_SHIFT)) ?
> -		WDIOF_CARDRESET : 0;
> +	if (pdata && pdata->read_reset_sources) {
> +		u32 rs = pdata->read_reset_sources();
> +		if (rs & (1 << OMAP_MPU_WD_RST_SRC_ID_SHIFT))
> +			wdev->wdog.bootstatus = WDIOF_CARDRESET;
> +	}
>  
>  	omap_wdt_disable(wdev);
>  
> -- 
> 2.1.4
> 

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 3/3] watchdog: omap: simplify assignment of bootstatus

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1814 bytes --]

On Fri, Apr 24, 2015 at 11:48:33AM +0200, Uwe Kleine-König wrote:
> Instead of using an over-long expression involving the ?: operator use
> an if and intead of an else branch rely on the fact that the data
> structure was allocated using devm_kzalloc. This also allows to put the
> used helper variable into a more local scope.
> 
> There is no functional change.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>

looks good to me

Reviewed-by: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>

> ---
>  drivers/watchdog/omap_wdt.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c
> index 479e7c8e44f5..0421c06a6cf0 100644
> --- a/drivers/watchdog/omap_wdt.c
> +++ b/drivers/watchdog/omap_wdt.c
> @@ -209,7 +209,6 @@ static int omap_wdt_probe(struct platform_device *pdev)
>  	struct omap_wd_timer_platform_data *pdata = dev_get_platdata(&pdev->dev);
>  	struct resource *res;
>  	struct omap_wdt_dev *wdev;
> -	u32 rs;
>  	int ret;
>  
>  	wdev = devm_kzalloc(&pdev->dev, sizeof(*wdev), GFP_KERNEL);
> @@ -242,12 +241,11 @@ static int omap_wdt_probe(struct platform_device *pdev)
>  	pm_runtime_enable(wdev->dev);
>  	pm_runtime_get_sync(wdev->dev);
>  
> -	if (pdata && pdata->read_reset_sources)
> -		rs = pdata->read_reset_sources();
> -	else
> -		rs = 0;
> -	wdev->wdog.bootstatus = (rs & (1 << OMAP_MPU_WD_RST_SRC_ID_SHIFT)) ?
> -		WDIOF_CARDRESET : 0;
> +	if (pdata && pdata->read_reset_sources) {
> +		u32 rs = pdata->read_reset_sources();
> +		if (rs & (1 << OMAP_MPU_WD_RST_SRC_ID_SHIFT))
> +			wdev->wdog.bootstatus = WDIOF_CARDRESET;
> +	}
>  
>  	omap_wdt_disable(wdev);
>  
> -- 
> 2.1.4
> 

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/3] watchdog: omap: use watchdog_init_timeout

[Uwe Kleine-König]

On Fri, Apr 24, 2015 at 09:42:59AM -0500, Felipe Balbi wrote:
> Hi,
> 
> On Fri, Apr 24, 2015 at 11:48:31AM +0200, Uwe Kleine-König wrote:
> > Instead of (partly) open coding it use the core function. As a side
> > effect the "timeout-sec" devicetree property is used now.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> >  Documentation/devicetree/bindings/watchdog/omap-wdt.txt | 9 +++++----
> >  drivers/watchdog/omap_wdt.c                             | 5 +----
> >  2 files changed, 6 insertions(+), 8 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/watchdog/omap-wdt.txt b/Documentation/devicetree/bindings/watchdog/omap-wdt.txt
> > index c227970671ea..4f18ec38be2f 100644
> > --- a/Documentation/devicetree/bindings/watchdog/omap-wdt.txt
> > +++ b/Documentation/devicetree/bindings/watchdog/omap-wdt.txt
> > @@ -1,10 +1,11 @@
> >  TI Watchdog Timer (WDT) Controller for OMAP
> >  
> >  Required properties:
> > -compatible:
> > -- "ti,omap3-wdt" for OMAP3
> > -- "ti,omap4-wdt" for OMAP4
> > -- ti,hwmods: Name of the hwmod associated to the WDT
> > +- compatible : "ti,omap3-wdt" (for OMAP3) or "ti,omap4-wdt" (for OMAP4)
> > +- ti,hwmods : Name of the hwmod associated to the WDT
> > +
> > +Optional properties:
> > +- timeout-sec : default watchdog timeout in seconds
> 
> adding a new property here ? Why ? In fact, none of these DT binding doc
> changes fit in $subject. Sure, watchdog_init_timeout() needs
> timeout-sec, but no OMAP DT today passes it.
watchdog_init_timeout doesn't *need* the new property. After all it's
optional. The other changes are just reformatting to make it match the
usual style and make it understandable. Could have mentioned it in the
commitlog, that's true.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 1/3] watchdog: omap: use watchdog_init_timeout

[Uwe Kleine-König]

On Fri, Apr 24, 2015 at 09:42:59AM -0500, Felipe Balbi wrote:
> Hi,
> 
> On Fri, Apr 24, 2015 at 11:48:31AM +0200, Uwe Kleine-König wrote:
> > Instead of (partly) open coding it use the core function. As a side
> > effect the "timeout-sec" devicetree property is used now.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> >  Documentation/devicetree/bindings/watchdog/omap-wdt.txt | 9 +++++----
> >  drivers/watchdog/omap_wdt.c                             | 5 +----
> >  2 files changed, 6 insertions(+), 8 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/watchdog/omap-wdt.txt b/Documentation/devicetree/bindings/watchdog/omap-wdt.txt
> > index c227970671ea..4f18ec38be2f 100644
> > --- a/Documentation/devicetree/bindings/watchdog/omap-wdt.txt
> > +++ b/Documentation/devicetree/bindings/watchdog/omap-wdt.txt
> > @@ -1,10 +1,11 @@
> >  TI Watchdog Timer (WDT) Controller for OMAP
> >  
> >  Required properties:
> > -compatible:
> > -- "ti,omap3-wdt" for OMAP3
> > -- "ti,omap4-wdt" for OMAP4
> > -- ti,hwmods: Name of the hwmod associated to the WDT
> > +- compatible : "ti,omap3-wdt" (for OMAP3) or "ti,omap4-wdt" (for OMAP4)
> > +- ti,hwmods : Name of the hwmod associated to the WDT
> > +
> > +Optional properties:
> > +- timeout-sec : default watchdog timeout in seconds
> 
> adding a new property here ? Why ? In fact, none of these DT binding doc
> changes fit in $subject. Sure, watchdog_init_timeout() needs
> timeout-sec, but no OMAP DT today passes it.
watchdog_init_timeout doesn't *need* the new property. After all it's
optional. The other changes are just reformatting to make it match the
usual style and make it understandable. Could have mentioned it in the
commitlog, that's true.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] watchdog: omap: simplify assignment of bootstatus

[Uwe Kleine-König]

On Fri, Apr 24, 2015 at 11:48:33AM +0200, Uwe Kleine-König wrote:
> Instead of using an over-long expression involving the ?: operator use
> an if and intead of an else branch rely on the fact that the data
just noticed ^^ there is an s missing ...

Wim, if you apply this version, can you fix that up?

Best regards
Uwe

> structure was allocated using devm_kzalloc. This also allows to put the
> used helper variable into a more local scope.
> 
> There is no functional change.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/watchdog/omap_wdt.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c
> index 479e7c8e44f5..0421c06a6cf0 100644
> --- a/drivers/watchdog/omap_wdt.c
> +++ b/drivers/watchdog/omap_wdt.c
> @@ -209,7 +209,6 @@ static int omap_wdt_probe(struct platform_device *pdev)
>  	struct omap_wd_timer_platform_data *pdata = dev_get_platdata(&pdev->dev);
>  	struct resource *res;
>  	struct omap_wdt_dev *wdev;
> -	u32 rs;
>  	int ret;
>  
>  	wdev = devm_kzalloc(&pdev->dev, sizeof(*wdev), GFP_KERNEL);
> @@ -242,12 +241,11 @@ static int omap_wdt_probe(struct platform_device *pdev)
>  	pm_runtime_enable(wdev->dev);
>  	pm_runtime_get_sync(wdev->dev);
>  
> -	if (pdata && pdata->read_reset_sources)
> -		rs = pdata->read_reset_sources();
> -	else
> -		rs = 0;
> -	wdev->wdog.bootstatus = (rs & (1 << OMAP_MPU_WD_RST_SRC_ID_SHIFT)) ?
> -		WDIOF_CARDRESET : 0;
> +	if (pdata && pdata->read_reset_sources) {
> +		u32 rs = pdata->read_reset_sources();
> +		if (rs & (1 << OMAP_MPU_WD_RST_SRC_ID_SHIFT))
> +			wdev->wdog.bootstatus = WDIOF_CARDRESET;
> +	}
>  
>  	omap_wdt_disable(wdev);
>  
> -- 
> 2.1.4
> 
> 

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 2/3] watchdog: omap: put struct watchdog_device into driver data

[Uwe Kleine-König]

Hello Felipe,

On Fri, Apr 24, 2015 at 09:44:48AM -0500, Felipe Balbi wrote:
> On Fri, Apr 24, 2015 at 11:48:32AM +0200, Uwe Kleine-König wrote:
> > This way only a single allocation is needed (per device). Also this
> > stops making use of watchdog_{set,get}_drvdata.
> 
> And this is better because ... ?
a single allocation is better because AFAIK it takes less memory to
allocate a + b bytes in a single piece than in two.

watchdog_{set,get}_drvdata seems ugly to me. Judging from the other
frameworks I know better than wdog I'd say there is no need for these
functions. I didn't know if I should be more explicit about my view in
the commitlog and if so how.

Also I didn't check other wdog drivers, but using this style of
embedding the "framework struct" into driver data is a common idiom for
uart and gpio drivers for example.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 2/3] watchdog: omap: put struct watchdog_device into driver data

[Uwe Kleine-König]

Hello Felipe,

On Fri, Apr 24, 2015 at 09:44:48AM -0500, Felipe Balbi wrote:
> On Fri, Apr 24, 2015 at 11:48:32AM +0200, Uwe Kleine-König wrote:
> > This way only a single allocation is needed (per device). Also this
> > stops making use of watchdog_{set,get}_drvdata.
> 
> And this is better because ... ?
a single allocation is better because AFAIK it takes less memory to
allocate a + b bytes in a single piece than in two.

watchdog_{set,get}_drvdata seems ugly to me. Judging from the other
frameworks I know better than wdog I'd say there is no need for these
functions. I didn't know if I should be more explicit about my view in
the commitlog and if so how.

Also I didn't check other wdog drivers, but using this style of
embedding the "framework struct" into driver data is a common idiom for
uart and gpio drivers for example.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1a/3] watchdog: omap: clearify device tree documentation

[Uwe Kleine-König]

ti,hwmods doesn't belong into the compatible section but is a property
on it's own. Also reformat the section of required properties to match the
usual style of dt binding documents.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Hello Felipe,

what about this patch before the previously sent patch 1?

Best regards
Uwe

 Documentation/devicetree/bindings/watchdog/omap-wdt.txt | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/watchdog/omap-wdt.txt b/Documentation/devicetree/bindings/watchdog/omap-wdt.txt
index c227970671ea..597e19d18dca 100644
--- a/Documentation/devicetree/bindings/watchdog/omap-wdt.txt
+++ b/Documentation/devicetree/bindings/watchdog/omap-wdt.txt
@@ -1,10 +1,8 @@
 TI Watchdog Timer (WDT) Controller for OMAP
 
 Required properties:
-compatible:
-- "ti,omap3-wdt" for OMAP3
-- "ti,omap4-wdt" for OMAP4
-- ti,hwmods: Name of the hwmod associated to the WDT
+- compatible : "ti,omap3-wdt" for OMAP3 or "ti,omap4-wdt" for OMAP4
+- ti,hwmods : Name of the hwmod associated to the WDT
 
 Examples:
 
-- 
2.1.4

[PATCH 1a/3] watchdog: omap: clearify device tree documentation

[Uwe Kleine-König]

ti,hwmods doesn't belong into the compatible section but is a property
on it's own. Also reformat the section of required properties to match the
usual style of dt binding documents.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
---
Hello Felipe,

what about this patch before the previously sent patch 1?

Best regards
Uwe

 Documentation/devicetree/bindings/watchdog/omap-wdt.txt | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/watchdog/omap-wdt.txt b/Documentation/devicetree/bindings/watchdog/omap-wdt.txt
index c227970671ea..597e19d18dca 100644
--- a/Documentation/devicetree/bindings/watchdog/omap-wdt.txt
+++ b/Documentation/devicetree/bindings/watchdog/omap-wdt.txt
@@ -1,10 +1,8 @@
 TI Watchdog Timer (WDT) Controller for OMAP
 
 Required properties:
-compatible:
-- "ti,omap3-wdt" for OMAP3
-- "ti,omap4-wdt" for OMAP4
-- ti,hwmods: Name of the hwmod associated to the WDT
+- compatible : "ti,omap3-wdt" for OMAP3 or "ti,omap4-wdt" for OMAP4
+- ti,hwmods : Name of the hwmod associated to the WDT
 
 Examples:
 
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1a/3] watchdog: omap: clearify device tree documentation

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1416 bytes --]

On Fri, Apr 24, 2015 at 10:20:50PM +0200, Uwe Kleine-König wrote:
> ti,hwmods doesn't belong into the compatible section but is a property
> on it's own. Also reformat the section of required properties to match the
> usual style of dt binding documents.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

looks great to me, thanks

Reviewed-by: Felipe Balbi <balbi@ti.com>
Acked-by: Felipe Balbi <balbi@ti.com>

> ---
> Hello Felipe,
> 
> what about this patch before the previously sent patch 1?
> 
> Best regards
> Uwe
> 
>  Documentation/devicetree/bindings/watchdog/omap-wdt.txt | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/omap-wdt.txt b/Documentation/devicetree/bindings/watchdog/omap-wdt.txt
> index c227970671ea..597e19d18dca 100644
> --- a/Documentation/devicetree/bindings/watchdog/omap-wdt.txt
> +++ b/Documentation/devicetree/bindings/watchdog/omap-wdt.txt
> @@ -1,10 +1,8 @@
>  TI Watchdog Timer (WDT) Controller for OMAP
>  
>  Required properties:
> -compatible:
> -- "ti,omap3-wdt" for OMAP3
> -- "ti,omap4-wdt" for OMAP4
> -- ti,hwmods: Name of the hwmod associated to the WDT
> +- compatible : "ti,omap3-wdt" for OMAP3 or "ti,omap4-wdt" for OMAP4
> +- ti,hwmods : Name of the hwmod associated to the WDT
>  
>  Examples:
>  
> -- 
> 2.1.4
> 

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1a/3] watchdog: omap: clearify device tree documentation

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1485 bytes --]

On Fri, Apr 24, 2015 at 10:20:50PM +0200, Uwe Kleine-König wrote:
> ti,hwmods doesn't belong into the compatible section but is a property
> on it's own. Also reformat the section of required properties to match the
> usual style of dt binding documents.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>

looks great to me, thanks

Reviewed-by: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>
Acked-by: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>

> ---
> Hello Felipe,
> 
> what about this patch before the previously sent patch 1?
> 
> Best regards
> Uwe
> 
>  Documentation/devicetree/bindings/watchdog/omap-wdt.txt | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/omap-wdt.txt b/Documentation/devicetree/bindings/watchdog/omap-wdt.txt
> index c227970671ea..597e19d18dca 100644
> --- a/Documentation/devicetree/bindings/watchdog/omap-wdt.txt
> +++ b/Documentation/devicetree/bindings/watchdog/omap-wdt.txt
> @@ -1,10 +1,8 @@
>  TI Watchdog Timer (WDT) Controller for OMAP
>  
>  Required properties:
> -compatible:
> -- "ti,omap3-wdt" for OMAP3
> -- "ti,omap4-wdt" for OMAP4
> -- ti,hwmods: Name of the hwmod associated to the WDT
> +- compatible : "ti,omap3-wdt" for OMAP3 or "ti,omap4-wdt" for OMAP4
> +- ti,hwmods : Name of the hwmod associated to the WDT
>  
>  Examples:
>  
> -- 
> 2.1.4
> 

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 3/3] watchdog: omap: simplify assignment of bootstatus

[Guenter Roeck]

On 04/24/2015 12:03 PM, Uwe Kleine-König wrote:
> On Fri, Apr 24, 2015 at 11:48:33AM +0200, Uwe Kleine-König wrote:
>> Instead of using an over-long expression involving the ?: operator use
>> an if and intead of an else branch rely on the fact that the data
> just noticed ^^ there is an s missing ...
>
> Wim, if you apply this version, can you fix that up?
>

Hi Uwe,

if you don't mind, can you sesend the entire series ?
I don't think patch 1 applies after 1a, and it might make sense
to add some more explanation to patch 2. FWIW, I would say that using
to_omap_wdt_dev() instead of watchdog_get_drvdata() simplifies the code,
or simply that the functions are no longer needed in the driver (ugliness
may be seen as personal opinion).

When you do so, please feel free to add

Reviewed-by: Guenter Roeck <linux@roeck-us.net> to all four patches.

Thanks,
Guenter

Re: [PATCH 1a/3] watchdog: omap: clearify device tree documentation

[Guenter Roeck]

On 04/24/2015 07:10 PM, Felipe Balbi wrote:
> On Fri, Apr 24, 2015 at 10:20:50PM +0200, Uwe Kleine-König wrote:
>> ti,hwmods doesn't belong into the compatible section but is a property
>> on it's own. Also reformat the section of required properties to match the
>> usual style of dt binding documents.
>>
>> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>
> looks great to me, thanks
>
> Reviewed-by: Felipe Balbi <balbi@ti.com>
> Acked-by: Felipe Balbi <balbi@ti.com>
>

Reviewed-by: and Acked-by: are mutually exclusive.

Guenter

Re: [PATCH 1a/3] watchdog: omap: clearify device tree documentation

[Guenter Roeck]

On 04/24/2015 07:10 PM, Felipe Balbi wrote:
> On Fri, Apr 24, 2015 at 10:20:50PM +0200, Uwe Kleine-König wrote:
>> ti,hwmods doesn't belong into the compatible section but is a property
>> on it's own. Also reformat the section of required properties to match the
>> usual style of dt binding documents.
>>
>> Signed-off-by: Uwe Kleine-König <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>
> looks great to me, thanks
>
> Reviewed-by: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>
> Acked-by: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>
>

Reviewed-by: and Acked-by: are mutually exclusive.

Guenter

--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1a/3] watchdog: omap: clearify device tree documentation

[Uwe Kleine-König]

On Sun, Apr 26, 2015 at 08:29:20AM -0700, Guenter Roeck wrote:
> On 04/24/2015 07:10 PM, Felipe Balbi wrote:
> >On Fri, Apr 24, 2015 at 10:20:50PM +0200, Uwe Kleine-König wrote:
> >>ti,hwmods doesn't belong into the compatible section but is a property
> >>on it's own. Also reformat the section of required properties to match the
> >>usual style of dt binding documents.
> >>
> >>Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> >
> >looks great to me, thanks
> >
> >Reviewed-by: Felipe Balbi <balbi@ti.com>
> >Acked-by: Felipe Balbi <balbi@ti.com>
> >
> 
> Reviewed-by: and Acked-by: are mutually exclusive.
Really? Acked means you like it and Reviewed by means you looked deeper
into it. Maybe reviewing without liking isn't likely, *shrug*.

The command

	$ git rev-list linus/master | while read cmmt; do git cat-file commit $cmmt | awk -v cmmt=$cmmt '$1 ~ /(Acked|Reviewed)-by:/ { $1 = ""; print cmmt " " $0 }' | sort; done | uniq -d

should catch all commits that have duplicate tags. There are a few,
*shrugagain*.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 1a/3] watchdog: omap: clearify device tree documentation

[Uwe Kleine-König]

On Sun, Apr 26, 2015 at 08:29:20AM -0700, Guenter Roeck wrote:
> On 04/24/2015 07:10 PM, Felipe Balbi wrote:
> >On Fri, Apr 24, 2015 at 10:20:50PM +0200, Uwe Kleine-König wrote:
> >>ti,hwmods doesn't belong into the compatible section but is a property
> >>on it's own. Also reformat the section of required properties to match the
> >>usual style of dt binding documents.
> >>
> >>Signed-off-by: Uwe Kleine-König <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> >
> >looks great to me, thanks
> >
> >Reviewed-by: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>
> >Acked-by: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>
> >
> 
> Reviewed-by: and Acked-by: are mutually exclusive.
Really? Acked means you like it and Reviewed by means you looked deeper
into it. Maybe reviewing without liking isn't likely, *shrug*.

The command

	$ git rev-list linus/master | while read cmmt; do git cat-file commit $cmmt | awk -v cmmt=$cmmt '$1 ~ /(Acked|Reviewed)-by:/ { $1 = ""; print cmmt " " $0 }' | sort; done | uniq -d

should catch all commits that have duplicate tags. There are a few,
*shrugagain*.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] watchdog: omap: simplify assignment of bootstatus

[Uwe Kleine-König]

Hello Guenter,

On Sun, Apr 26, 2015 at 08:28:16AM -0700, Guenter Roeck wrote:
> if you don't mind, can you sesend the entire series ?
yeah, that's my plan. I already fixed it for Felipe's comments and
intend to resend tomorrow.

> I don't think patch 1 applies after 1a, and it might make sense
> to add some more explanation to patch 2. FWIW, I would say that using
> to_omap_wdt_dev() instead of watchdog_get_drvdata() simplifies the code,
> or simply that the functions are no longer needed in the driver (ugliness
> may be seen as personal opinion).
> 
> When you do so, please feel free to add
> 
> Reviewed-by: Guenter Roeck <linux@roeck-us.net> to all four patches.
Thanks
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 1a/3] watchdog: omap: clearify device tree documentation

[Guenter Roeck]

On 04/26/2015 12:12 PM, Uwe Kleine-König wrote:
> On Sun, Apr 26, 2015 at 08:29:20AM -0700, Guenter Roeck wrote:
>> On 04/24/2015 07:10 PM, Felipe Balbi wrote:
>>> On Fri, Apr 24, 2015 at 10:20:50PM +0200, Uwe Kleine-König wrote:
>>>> ti,hwmods doesn't belong into the compatible section but is a property
>>>> on it's own. Also reformat the section of required properties to match the
>>>> usual style of dt binding documents.
>>>>
>>>> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>>>
>>> looks great to me, thanks
>>>
>>> Reviewed-by: Felipe Balbi <balbi@ti.com>
>>> Acked-by: Felipe Balbi <balbi@ti.com>
>>>
>>
>> Reviewed-by: and Acked-by: are mutually exclusive.
> Really? Acked means you like it and Reviewed by means you looked deeper
> into it. Maybe reviewing without liking isn't likely, *shrug*.
>

SubmittingPatches says about Acked-by:

" ... to signify and record their approval ... has at least reviewed the patch
and has indicated acceptance ...".

and about Reviewed-by:

" ... indicates that the patch has been reviewed and found acceptable ..."

Maybe I should have used the term "redundant". *shrugtoo*.

Guenter

Re: [PATCH 1a/3] watchdog: omap: clearify device tree documentation

[Guenter Roeck]

On 04/26/2015 12:12 PM, Uwe Kleine-König wrote:
> On Sun, Apr 26, 2015 at 08:29:20AM -0700, Guenter Roeck wrote:
>> On 04/24/2015 07:10 PM, Felipe Balbi wrote:
>>> On Fri, Apr 24, 2015 at 10:20:50PM +0200, Uwe Kleine-König wrote:
>>>> ti,hwmods doesn't belong into the compatible section but is a property
>>>> on it's own. Also reformat the section of required properties to match the
>>>> usual style of dt binding documents.
>>>>
>>>> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>>>
>>> looks great to me, thanks
>>>
>>> Reviewed-by: Felipe Balbi <balbi@ti.com>
>>> Acked-by: Felipe Balbi <balbi@ti.com>
>>>
>>
>> Reviewed-by: and Acked-by: are mutually exclusive.
> Really? Acked means you like it and Reviewed by means you looked deeper
> into it. Maybe reviewing without liking isn't likely, *shrug*.
>

SubmittingPatches says about Acked-by:

" ... to signify and record their approval ... has at least reviewed the patch
and has indicated acceptance ...".

and about Reviewed-by:

" ... indicates that the patch has been reviewed and found acceptable ..."

Maybe I should have used the term "redundant". *shrugtoo*.

Guenter

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html