watchdog: s3c2410: Minor cleanups
Subsystems (2)
Krzysztof Kozlowski
maintainerv3 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
Krzysztof Kozlowski
maintainerPATCH 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
@@ -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
Re: [PATCH v3 1/3] watchdog: s3c2410: Constify local structures
Quoted 4 lines · Krzysztof Kozlowski
Structures
watchdog_device,watchdog_opsands3c2410_wdt_variantare not modified so they can be made const to increase code safeness.Signed-off-by Krzysztof Kozlowski <[email protected]>
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
Re: [PATCH v3 1/3] watchdog: s3c2410: Constify local structures
In reply to Krzysztof KozlowskiQuoted 4 lines · Krzysztof Kozlowski
Structures
watchdog_device,watchdog_opsands3c2410_wdt_variantare not modified so they can be made const to increase code safeness.Signed-off-by Krzysztof Kozlowski <[email protected]>
Quoted 36 lines
--- drivers/watchdog/s3c2410_wdt.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)drivers/watchdog/s3c2410_wdt.c +3
-3@@ -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) {
Krzysztof Kozlowski
maintainerPATCH v3 2/3 watchdog: s3c2410: Simplify getting driver data
In reply to Krzysztof KozlowskiSimplify 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
@@ -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
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 oneif()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
Krzysztof Kozlowski
maintainerRe: [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 oneif()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
Guenter Roeck
maintainerRe: [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 oneif()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
Re: [PATCH v3 2/3] watchdog: s3c2410: Simplify getting driver data
In reply to Krzysztof KozlowskiQuoted 6 lines · Krzysztof Kozlowski
Simplify the flow in helper function for getting the driver data by using
of_device_get_match_data()and only oneif()branch.The code should be equivalent.
Signed-off-by Krzysztof Kozlowski <[email protected]>
Quoted 41 lines
--- drivers/watchdog/s3c2410_wdt.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)drivers/watchdog/s3c2410_wdt.c +10
-7@@ -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)
Krzysztof Kozlowski
maintainerPATCH v3 3/3 watchdog: s3c2410: Minor code cleanup
In reply to Krzysztof KozlowskiCleanup 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
@@ -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
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.Signed-off-by Krzysztof Kozlowski <[email protected]>
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
Re: [PATCH v3 3/3] watchdog: s3c2410: Minor code cleanup
In reply to Krzysztof KozlowskiQuoted 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.Signed-off-by Krzysztof Kozlowski <[email protected]>
Quoted 115 lines
--- drivers/watchdog/s3c2410_wdt.c | 35 +++++++++++++---------------------- 1 file changed, 13 insertions(+), 22 deletions(-)drivers/watchdog/s3c2410_wdt.c +13
-22@@ -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" );