Skip to contentSkip to conversation

watchdog: s3c2410: Minor cleanups

PATCH In review Krzysztof Kozlowski 3 participants 12 messages Mar 13, 2017 – Mar 14, 2017

Subsystems (2)
lore
Share & tools
lore

Krzysztof Kozlowski

author maintainer
#0 lore raw

v3 cover watchdog: s3c2410: Minor cleanups

Hi,

Continuation of cleanup of s3c2410-wdt driver.

Changes since v2:
=================
1. Address Guenter's comments about unneeded cast to const.

Best regards,
Krzysztof

Krzysztof Kozlowski (3):
watchdog: s3c2410: Constify local structures
watchdog: s3c2410: Simplify getting driver data
watchdog: s3c2410: Minor code cleanup

 drivers/watchdog/s3c2410_wdt.c | 58 +++++++++++++++++++-----------------------
 1 file changed, 26 insertions(+), 32 deletions(-)
Signature · 1 line
2.9.3
author maintainer
#1 lore raw

PATCH v3 1/3 watchdog: s3c2410: Constify local structures

Structures watchdog_device, watchdog_ops and s3c2410_wdt_variant are not modified so they can be made const to increase code safeness.

---
 drivers/watchdog/s3c2410_wdt.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/watchdog/s3c2410_wdt.c +3 -3
index 6ed97596ca80..52b66bcdd1ef 100644
@@ -131,7 +131,7 @@ struct s3c2410_wdt {
 	unsigned long		wtdat_save;
 	struct watchdog_device	wdt_device;
 	struct notifier_block	freq_transition;
-	struct s3c2410_wdt_variant *drv_data;
+	const struct s3c2410_wdt_variant *drv_data;
 	struct regmap *pmureg;
 };
 
@@ -401,7 +401,7 @@ static const struct watchdog_ops s3c2410wdt_ops = {
 	.restart = s3c2410wdt_restart,
 };
 
-static struct watchdog_device s3c2410_wdd = {
+static const struct watchdog_device s3c2410_wdd = {
 	.info = &s3c2410_wdt_ident,
 	.ops = &s3c2410wdt_ops,
 	.timeout = S3C2410_WATCHDOG_DEFAULT_TIME,
@@ -507,7 +507,7 @@ static inline unsigned int s3c2410wdt_get_bootstatus(struct s3c2410_wdt *wdt)
 	return 0;
 }
 
-static inline struct s3c2410_wdt_variant *
+static inline const struct s3c2410_wdt_variant *
 s3c2410_get_wdt_drv_data(struct platform_device *pdev)
 {
 	if (pdev->dev.of_node) {
Signature · 1 line
2.9.3
Reviewed-by on v3 1/3
#2 lore raw

Re: [PATCH v3 1/3] watchdog: s3c2410: Constify local structures

Quoted 4 lines · Krzysztof Kozlowski

Structures watchdog_device, watchdog_ops and s3c2410_wdt_variant are not modified so they can be made const to increase code safeness.

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

maintainer Reviewed-by on v3 1/3
#3 lore raw

Re: [PATCH v3 1/3] watchdog: s3c2410: Constify local structures

In reply to Krzysztof Kozlowski
Quoted 4 lines · Krzysztof Kozlowski

Structures watchdog_device, watchdog_ops and s3c2410_wdt_variant are not modified so they can be made const to increase code safeness.

Quoted 36 lines
---
 drivers/watchdog/s3c2410_wdt.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/watchdog/s3c2410_wdt.c +3 -3
index 6ed97596ca80..52b66bcdd1ef 100644
@@ -131,7 +131,7 @@ struct s3c2410_wdt {
 	unsigned long		wtdat_save;
 	struct watchdog_device	wdt_device;
 	struct notifier_block	freq_transition;
-	struct s3c2410_wdt_variant *drv_data;
+	const struct s3c2410_wdt_variant *drv_data;
 	struct regmap *pmureg;
 };
 
@@ -401,7 +401,7 @@ static const struct watchdog_ops s3c2410wdt_ops = {
 	.restart = s3c2410wdt_restart,
 };
 
-static struct watchdog_device s3c2410_wdd = {
+static const struct watchdog_device s3c2410_wdd = {
 	.info = &s3c2410_wdt_ident,
 	.ops = &s3c2410wdt_ops,
 	.timeout = S3C2410_WATCHDOG_DEFAULT_TIME,
@@ -507,7 +507,7 @@ static inline unsigned int s3c2410wdt_get_bootstatus(struct s3c2410_wdt *wdt)
 	return 0;
 }
 
-static inline struct s3c2410_wdt_variant *
+static inline const struct s3c2410_wdt_variant *
 s3c2410_get_wdt_drv_data(struct platform_device *pdev)
 {
 	if (pdev->dev.of_node) {
author maintainer
#4 lore raw

PATCH v3 2/3 watchdog: s3c2410: Simplify getting driver data

In reply to Krzysztof Kozlowski

Simplify the flow in helper function for getting the driver data by using of_device_get_match_data() and only one if() branch.

The code should be equivalent.

---
 drivers/watchdog/s3c2410_wdt.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)
drivers/watchdog/s3c2410_wdt.c +10 -7
index 52b66bcdd1ef..0532dc93e600 100644
@@ -37,6 +37,7 @@
 #include <linux/slab.h>
 #include <linux/err.h>
 #include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/mfd/syscon.h>
 #include <linux/regmap.h>
 #include <linux/delay.h>
@@ -510,14 +511,16 @@ static inline unsigned int s3c2410wdt_get_bootstatus(struct s3c2410_wdt *wdt)
 static inline const struct s3c2410_wdt_variant *
 s3c2410_get_wdt_drv_data(struct platform_device *pdev)
 {
-	if (pdev->dev.of_node) {
-		const struct of_device_id *match;
-		match = of_match_node(s3c2410_wdt_match, pdev->dev.of_node);
-		return (struct s3c2410_wdt_variant *)match->data;
-	} else {
-		return (struct s3c2410_wdt_variant *)
-			platform_get_device_id(pdev)->driver_data;
+	const struct s3c2410_wdt_variant *variant;
+
+	variant = of_device_get_match_data(&pdev->dev);
+	if (!variant) {
+		/* Device matched by platform_device_id */
+		variant = (struct s3c2410_wdt_variant *)
+			   platform_get_device_id(pdev)->driver_data;
 	}
+
+	return variant;
 }
 
 static int s3c2410wdt_probe(struct platform_device *pdev)
Signature · 1 line
2.9.3
#5 lore raw

Re: [PATCH v3 2/3] watchdog: s3c2410: Simplify getting driver data

Hi,

Quoted 4 lines · Krzysztof Kozlowski

Simplify the flow in helper function for getting the driver data by using of_device_get_match_data() and only one if() branch.

The code should be equivalent.

While you are at it could you remove s3c2410_get_wdt_drv_data() helper? It is used only once during probe and is marked inline anyway..

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

author maintainer
#6 lore raw

Re: [PATCH v3 2/3] watchdog: s3c2410: Simplify getting driver data

Quoted 4 lines · Bartlomiej Zolnierkiewicz

Hi,

On Monday, March 13, 2017 09:07:25 PM Krzysztof Kozlowski wrote:

Quoted 8 lines
Quoted 4 lines

Simplify the flow in helper function for getting the driver data by using of_device_get_match_data() and only one if() branch.

The code should be equivalent.

While you are at it could you remove s3c2410_get_wdt_drv_data() helper? It is used only once during probe and is marked inline anyway..

Thanks for feedback!
The existence of this helper is purely from code readability (thus inline does not matter). The probe is big so splitting some small self-contained part helps. Not much but a little...

Best regards,
Krzysztof

maintainer
#7 lore raw

Re: [PATCH v3 2/3] watchdog: s3c2410: Simplify getting driver data

Quoted 11 lines · Krzysztof Kozlowski
Quoted 9 lines · Bartlomiej Zolnierkiewicz

Hi,

Quoted 4 lines · Krzysztof Kozlowski

Simplify the flow in helper function for getting the driver data by using of_device_get_match_data() and only one if() branch.

The code should be equivalent.

Quoted 9 lines
Quoted 3 lines

While you are at it could you remove s3c2410_get_wdt_drv_data() helper? It is used only once during probe and is marked inline anyway..

Thanks for feedback!
The existence of this helper is purely from code readability (thus inline does not matter). The probe is big so splitting some small self-contained part helps. Not much but a little...

Agreed. I don't see value in removing this helper.

Guenter

maintainer Reviewed-by on v3 2/3
#8 lore raw

Re: [PATCH v3 2/3] watchdog: s3c2410: Simplify getting driver data

In reply to Krzysztof Kozlowski
Quoted 6 lines · Krzysztof Kozlowski

Simplify the flow in helper function for getting the driver data by using of_device_get_match_data() and only one if() branch.

The code should be equivalent.

Quoted 41 lines
---
 drivers/watchdog/s3c2410_wdt.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)
drivers/watchdog/s3c2410_wdt.c +10 -7
index 52b66bcdd1ef..0532dc93e600 100644
@@ -37,6 +37,7 @@
 #include <linux/slab.h>
 #include <linux/err.h>
 #include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/mfd/syscon.h>
 #include <linux/regmap.h>
 #include <linux/delay.h>
@@ -510,14 +511,16 @@ static inline unsigned int s3c2410wdt_get_bootstatus(struct s3c2410_wdt *wdt)
 static inline const struct s3c2410_wdt_variant *
 s3c2410_get_wdt_drv_data(struct platform_device *pdev)
 {
-	if (pdev->dev.of_node) {
-		const struct of_device_id *match;
-		match = of_match_node(s3c2410_wdt_match, pdev->dev.of_node);
-		return (struct s3c2410_wdt_variant *)match->data;
-	} else {
-		return (struct s3c2410_wdt_variant *)
-			platform_get_device_id(pdev)->driver_data;
+	const struct s3c2410_wdt_variant *variant;
+
+	variant = of_device_get_match_data(&pdev->dev);
+	if (!variant) {
+		/* Device matched by platform_device_id */
+		variant = (struct s3c2410_wdt_variant *)
+			   platform_get_device_id(pdev)->driver_data;
 	}
+
+	return variant;
 }
 
 static int s3c2410wdt_probe(struct platform_device *pdev)
author maintainer
#9 lore raw

PATCH v3 3/3 watchdog: s3c2410: Minor code cleanup

In reply to Krzysztof Kozlowski

Cleanup the code from minor readability issues (no functional changes):
1. Fix checkpatch: ERROR: Do not include the paragraph about writing to the Free Software Foundation's mailing address.
2. Fix checkpatch: WARNING: quoted string split across lines
3. Fix chechpatch: WARNING: Prefer 'unsigned int' to bare use of 'unsigned'
4. Use 'dev' consistently in probe function instead of mixing dev with &pdev->dev.

---
 drivers/watchdog/s3c2410_wdt.c | 35 +++++++++++++----------------------
 1 file changed, 13 insertions(+), 22 deletions(-)
drivers/watchdog/s3c2410_wdt.c +13 -22
index 0532dc93e600..adaa43543f0a 100644
@@ -1,5 +1,4 @@
-/* linux/drivers/char/watchdog/s3c2410_wdt.c
- *
+/*
  * Copyright (c) 2004 Simtec Electronics
  *	Ben Dooks <[email protected]>
  *
@@ -17,11 +16,7 @@
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software
- * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
-*/
+ */
 
 #include <linux/module.h>
 #include <linux/moduleparam.h>
@@ -95,8 +90,7 @@ MODULE_PARM_DESC(tmr_atboot,
 			__MODULE_STRING(S3C2410_WATCHDOG_ATBOOT));
 MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
 			__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
-MODULE_PARM_DESC(soft_noboot, "Watchdog action, set to 1 to ignore reboots, "
-			"0 to reboot (default 0)");
+MODULE_PARM_DESC(soft_noboot, "Watchdog action, set to 1 to ignore reboots, 0 to reboot (default 0)");
 
 /**
  * struct s3c2410_wdt_variant - Per-variant config data
@@ -311,7 +305,8 @@ static inline int s3c2410wdt_is_running(struct s3c2410_wdt *wdt)
 	return readl(wdt->reg_base + S3C2410_WTCON) & S3C2410_WTCON_ENABLE;
 }
 
-static int s3c2410wdt_set_heartbeat(struct watchdog_device *wdd, unsigned timeout)
+static int s3c2410wdt_set_heartbeat(struct watchdog_device *wdd,
+				    unsigned int timeout)
 {
 	struct s3c2410_wdt *wdt = watchdog_get_drvdata(wdd);
 	unsigned long freq = clk_get_rate(wdt->clock);
@@ -525,7 +520,7 @@ s3c2410_get_wdt_drv_data(struct platform_device *pdev)
 
 static int s3c2410wdt_probe(struct platform_device *pdev)
 {
-	struct device *dev;
+	struct device *dev = &pdev->dev;
 	struct s3c2410_wdt *wdt;
 	struct resource *wdt_mem;
 	struct resource *wdt_irq;
@@ -533,13 +528,11 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
 	int started = 0;
 	int ret;
 
-	dev = &pdev->dev;
-
 	wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
 	if (!wdt)
 		return -ENOMEM;
 
-	wdt->dev = &pdev->dev;
+	wdt->dev = dev;
 	spin_lock_init(&wdt->lock);
 	wdt->wdt_device = s3c2410_wdd;
 
@@ -595,7 +588,7 @@ 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(&wdt->wdt_device, tmr_margin, &pdev->dev);
+	watchdog_init_timeout(&wdt->wdt_device, tmr_margin, dev);
 	ret = s3c2410wdt_set_heartbeat(&wdt->wdt_device,
 					wdt->wdt_device.timeout);
 	if (ret) {
@@ -604,11 +597,10 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
 
 		if (started == 0)
 			dev_info(dev,
-			   "tmr_margin value out of range, default %d used\n",
-			       S3C2410_WATCHDOG_DEFAULT_TIME);
+				 "tmr_margin value out of range, default %d used\n",
+				 S3C2410_WATCHDOG_DEFAULT_TIME);
 		else
-			dev_info(dev, "default timer value is out of range, "
-							"cannot start\n");
+			dev_info(dev, "default timer value is out of range, cannot start\n");
 	}
 
 	ret = devm_request_irq(dev, wdt_irq->start, s3c2410wdt_irq, 0,
@@ -622,7 +614,7 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
 	watchdog_set_restart_priority(&wdt->wdt_device, 128);
 
 	wdt->wdt_device.bootstatus = s3c2410wdt_get_bootstatus(wdt);
-	wdt->wdt_device.parent = &pdev->dev;
+	wdt->wdt_device.parent = dev;
 
 	ret = watchdog_register_device(&wdt->wdt_device);
 	if (ret) {
@@ -757,7 +749,6 @@ static struct platform_driver s3c2410wdt_driver = {
 
 module_platform_driver(s3c2410wdt_driver);
 
-MODULE_AUTHOR("Ben Dooks <[email protected]>, "
-	      "Dimitry Andric <[email protected]>");
+MODULE_AUTHOR("Ben Dooks <[email protected]>, Dimitry Andric <[email protected]>");
 MODULE_DESCRIPTION("S3C2410 Watchdog Device Driver");
 MODULE_LICENSE("GPL");
Signature · 1 line
2.9.3
Reviewed-by on v3 3/3
#10 lore raw

Re: [PATCH v3 3/3] watchdog: s3c2410: Minor code cleanup

Quoted 10 lines · Krzysztof Kozlowski

Cleanup the code from minor readability issues (no functional changes):
1. Fix checkpatch: ERROR: Do not include the paragraph about writing to the Free Software Foundation's mailing address.
2. Fix checkpatch: WARNING: quoted string split across lines
3. Fix chechpatch: WARNING: Prefer 'unsigned int' to bare use of 'unsigned'
4. Use 'dev' consistently in probe function instead of mixing dev with &pdev->dev.

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

maintainer Reviewed-by on v3 3/3
#11 lore raw

Re: [PATCH v3 3/3] watchdog: s3c2410: Minor code cleanup

In reply to Krzysztof Kozlowski
Quoted 10 lines · Krzysztof Kozlowski

Cleanup the code from minor readability issues (no functional changes):
1. Fix checkpatch: ERROR: Do not include the paragraph about writing to the Free Software Foundation's mailing address.
2. Fix checkpatch: WARNING: quoted string split across lines
3. Fix chechpatch: WARNING: Prefer 'unsigned int' to bare use of 'unsigned'
4. Use 'dev' consistently in probe function instead of mixing dev with &pdev->dev.

Quoted 115 lines
---
 drivers/watchdog/s3c2410_wdt.c | 35 +++++++++++++----------------------
 1 file changed, 13 insertions(+), 22 deletions(-)
drivers/watchdog/s3c2410_wdt.c +13 -22
index 0532dc93e600..adaa43543f0a 100644
@@ -1,5 +1,4 @@
-/* linux/drivers/char/watchdog/s3c2410_wdt.c
- *
+/*
  * Copyright (c) 2004 Simtec Electronics
  *	Ben Dooks <[email protected]>
  *
@@ -17,11 +16,7 @@
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software
- * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
-*/
+ */
 
 #include <linux/module.h>
 #include <linux/moduleparam.h>
@@ -95,8 +90,7 @@ MODULE_PARM_DESC(tmr_atboot,
 			__MODULE_STRING(S3C2410_WATCHDOG_ATBOOT));
 MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
 			__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
-MODULE_PARM_DESC(soft_noboot, "Watchdog action, set to 1 to ignore reboots, "
-			"0 to reboot (default 0)");
+MODULE_PARM_DESC(soft_noboot, "Watchdog action, set to 1 to ignore reboots, 0 to reboot (default 0)");
 
 /**
  * struct s3c2410_wdt_variant - Per-variant config data
@@ -311,7 +305,8 @@ static inline int s3c2410wdt_is_running(struct s3c2410_wdt *wdt)
 	return readl(wdt->reg_base + S3C2410_WTCON) & S3C2410_WTCON_ENABLE;
 }
 
-static int s3c2410wdt_set_heartbeat(struct watchdog_device *wdd, unsigned timeout)
+static int s3c2410wdt_set_heartbeat(struct watchdog_device *wdd,
+				    unsigned int timeout)
 {
 	struct s3c2410_wdt *wdt = watchdog_get_drvdata(wdd);
 	unsigned long freq = clk_get_rate(wdt->clock);
@@ -525,7 +520,7 @@ s3c2410_get_wdt_drv_data(struct platform_device *pdev)
 
 static int s3c2410wdt_probe(struct platform_device *pdev)
 {
-	struct device *dev;
+	struct device *dev = &pdev->dev;
 	struct s3c2410_wdt *wdt;
 	struct resource *wdt_mem;
 	struct resource *wdt_irq;
@@ -533,13 +528,11 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
 	int started = 0;
 	int ret;
 
-	dev = &pdev->dev;
-
 	wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
 	if (!wdt)
 		return -ENOMEM;
 
-	wdt->dev = &pdev->dev;
+	wdt->dev = dev;
 	spin_lock_init(&wdt->lock);
 	wdt->wdt_device = s3c2410_wdd;
 
@@ -595,7 +588,7 @@ 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(&wdt->wdt_device, tmr_margin, &pdev->dev);
+	watchdog_init_timeout(&wdt->wdt_device, tmr_margin, dev);
 	ret = s3c2410wdt_set_heartbeat(&wdt->wdt_device,
 					wdt->wdt_device.timeout);
 	if (ret) {
@@ -604,11 +597,10 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
 
 		if (started == 0)
 			dev_info(dev,
-			   "tmr_margin value out of range, default %d used\n",
-			       S3C2410_WATCHDOG_DEFAULT_TIME);
+				 "tmr_margin value out of range, default %d used\n",
+				 S3C2410_WATCHDOG_DEFAULT_TIME);
 		else
-			dev_info(dev, "default timer value is out of range, "
-							"cannot start\n");
+			dev_info(dev, "default timer value is out of range, cannot start\n");
 	}
 
 	ret = devm_request_irq(dev, wdt_irq->start, s3c2410wdt_irq, 0,
@@ -622,7 +614,7 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
 	watchdog_set_restart_priority(&wdt->wdt_device, 128);
 
 	wdt->wdt_device.bootstatus = s3c2410wdt_get_bootstatus(wdt);
-	wdt->wdt_device.parent = &pdev->dev;
+	wdt->wdt_device.parent = dev;
 
 	ret = watchdog_register_device(&wdt->wdt_device);
 	if (ret) {
@@ -757,7 +749,6 @@ static struct platform_driver s3c2410wdt_driver = {
 
 module_platform_driver(s3c2410wdt_driver);
 
-MODULE_AUTHOR("Ben Dooks <[email protected]>, "
-	      "Dimitry Andric <[email protected]>");
+MODULE_AUTHOR("Ben Dooks <[email protected]>, Dimitry Andric <[email protected]>");
 MODULE_DESCRIPTION("S3C2410 Watchdog Device Driver");
 MODULE_LICENSE("GPL");