Reducing init boilerplate by using the module_platform_device macro.
Signed-off-by: Enrico Weigelt, metux IT consult <[email protected]>
---
drivers/watchdog/iTCO_wdt.c | 16 +---------------
1 file changed, 1 insertion(+), 15 deletions(-)
diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
index a370a185a41c..f2ddc8fc71cd 100644
--- a/drivers/watchdog/iTCO_wdt.c
+++ b/drivers/watchdog/iTCO_wdt.c
@@ -651,21 +651,7 @@ static struct platform_driver iTCO_wdt_driver = {
},
};
-static int __init iTCO_wdt_init_module(void)
-{
- pr_info("Intel TCO WatchDog Timer Driver v%s\n", DRV_VERSION);
-
- return platform_driver_register(&iTCO_wdt_driver);
-}
-
-static void __exit iTCO_wdt_cleanup_module(void)
-{
- platform_driver_unregister(&iTCO_wdt_driver);
- pr_info("Watchdog Module Unloaded\n");
-}
-
-module_init(iTCO_wdt_init_module);
-module_exit(iTCO_wdt_cleanup_module);
+module_platform_driver(iTCO_wdt_driver);
MODULE_AUTHOR("Wim Van Sebroeck <[email protected]>");
MODULE_DESCRIPTION("Intel TCO WatchDog Timer Driver");
--
2.11.0
Reduce a little bit of boilerplate by using pm_ptr().
Signed-off-by: Enrico Weigelt, metux IT consult <[email protected]>
---
drivers/watchdog/iTCO_wdt.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
index edc588a06ae6..660fc262fbc0 100644
--- a/drivers/watchdog/iTCO_wdt.c
+++ b/drivers/watchdog/iTCO_wdt.c
@@ -636,16 +636,13 @@ static const struct dev_pm_ops iTCO_wdt_pm = {
.resume_noirq = iTCO_wdt_resume_noirq,
};
-#define ITCO_WDT_PM_OPS (&iTCO_wdt_pm)
-#else
-#define ITCO_WDT_PM_OPS NULL
#endif /* CONFIG_PM_SLEEP */
static struct platform_driver iTCO_wdt_driver = {
.probe = iTCO_wdt_probe,
.driver = {
.name = DRV_NAME,
- .pm = ITCO_WDT_PM_OPS,
+ .pm = pm_ptr(&iTCO_wdt_pm),
},
};
--
2.11.0
For device log outputs, it's better to have device name / ID
prefixed in all messages, so use the proper dev_*() functions here.
Explicit message on module load/unload don't seem to be really helpful
(we have other means to check which modules have been loaded), instead
just add noise to the kernel log. So, removing them.
Signed-off-by: Enrico Weigelt, metux IT consult <[email protected]>
---
drivers/watchdog/iTCO_wdt.c | 18 ++++++++----------
1 file changed, 8 insertions(+), 10 deletions(-)
diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
index f2ddc8fc71cd..edc588a06ae6 100644
--- a/drivers/watchdog/iTCO_wdt.c
+++ b/drivers/watchdog/iTCO_wdt.c
@@ -40,8 +40,6 @@
* Includes, defines, variables, module parameters, ...
*/
-#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
-
/* Module and version information */
#define DRV_NAME "iTCO_wdt"
#define DRV_VERSION "1.11"
@@ -279,7 +277,7 @@ static int iTCO_wdt_start(struct watchdog_device *wd_dev)
/* disable chipset's NO_REBOOT bit */
if (p->update_no_reboot_bit(p->no_reboot_priv, false)) {
spin_unlock(&p->io_lock);
- pr_err("failed to reset NO_REBOOT flag, reboot disabled by hardware/BIOS\n");
+ dev_err(wd_dev->dev, "failed to reset NO_REBOOT flag, reboot disabled by hardware/BIOS\n");
return -EIO;
}
@@ -510,7 +508,7 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
/* Check chipset's NO_REBOOT bit */
if (p->update_no_reboot_bit(p->no_reboot_priv, false) &&
iTCO_vendor_check_noreboot_on()) {
- pr_info("unable to reset NO_REBOOT flag, device disabled by hardware/BIOS\n");
+ dev_info(dev, "unable to reset NO_REBOOT flag, device disabled by hardware/BIOS\n");
return -ENODEV; /* Cannot reset NO_REBOOT bit */
}
@@ -530,12 +528,12 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
if (!devm_request_region(dev, p->tco_res->start,
resource_size(p->tco_res),
pdev->name)) {
- pr_err("I/O address 0x%04llx already in use, device disabled\n",
+ dev_err(dev, "I/O address 0x%04llx already in use, device disabled\n",
(u64)TCOBASE(p));
return -EBUSY;
}
- pr_info("Found a %s TCO device (Version=%d, TCOBASE=0x%04llx)\n",
+ dev_info(dev, "Found a %s TCO device (Version=%d, TCOBASE=0x%04llx)\n",
pdata->name, pdata->version, (u64)TCOBASE(p));
/* Clear out the (probably old) status */
@@ -558,7 +556,7 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
break;
}
- p->wddev.info = &ident,
+ p->wddev.info = &ident,
p->wddev.ops = &iTCO_wdt_ops,
p->wddev.bootstatus = 0;
p->wddev.timeout = WATCHDOG_TIMEOUT;
@@ -575,7 +573,7 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
if not reset to the default */
if (iTCO_wdt_set_timeout(&p->wddev, heartbeat)) {
iTCO_wdt_set_timeout(&p->wddev, WATCHDOG_TIMEOUT);
- pr_info("timeout value out of range, using %d\n",
+ dev_info(dev, "timeout value out of range, using %d\n",
WATCHDOG_TIMEOUT);
}
@@ -583,11 +581,11 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
watchdog_stop_on_unregister(&p->wddev);
ret = devm_watchdog_register_device(dev, &p->wddev);
if (ret != 0) {
- pr_err("cannot register watchdog device (err=%d)\n", ret);
+ dev_err(dev, "cannot register watchdog device (err=%d)\n", ret);
return ret;
}
- pr_info("initialized. heartbeat=%d sec (nowayout=%d)\n",
+ dev_info(dev, "initialized. heartbeat=%d sec (nowayout=%d)\n",
heartbeat, nowayout);
return 0;
--
2.11.0
On Tue, Nov 17, 2020 at 04:22:12PM +0100, Enrico Weigelt, metux IT consult wrote:
> Reducing init boilerplate by using the module_platform_device macro.
>
> Signed-off-by: Enrico Weigelt, metux IT consult <[email protected]>
Reviewed-by: Guenter Roeck <[email protected]>
> ---
> drivers/watchdog/iTCO_wdt.c | 16 +---------------
> 1 file changed, 1 insertion(+), 15 deletions(-)
>
> diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
> index a370a185a41c..f2ddc8fc71cd 100644
> --- a/drivers/watchdog/iTCO_wdt.c
> +++ b/drivers/watchdog/iTCO_wdt.c
> @@ -651,21 +651,7 @@ static struct platform_driver iTCO_wdt_driver = {
> },
> };
>
> -static int __init iTCO_wdt_init_module(void)
> -{
> - pr_info("Intel TCO WatchDog Timer Driver v%s\n", DRV_VERSION);
> -
> - return platform_driver_register(&iTCO_wdt_driver);
> -}
> -
> -static void __exit iTCO_wdt_cleanup_module(void)
> -{
> - platform_driver_unregister(&iTCO_wdt_driver);
> - pr_info("Watchdog Module Unloaded\n");
> -}
> -
> -module_init(iTCO_wdt_init_module);
> -module_exit(iTCO_wdt_cleanup_module);
> +module_platform_driver(iTCO_wdt_driver);
>
> MODULE_AUTHOR("Wim Van Sebroeck <[email protected]>");
> MODULE_DESCRIPTION("Intel TCO WatchDog Timer Driver");
On Tue, Nov 17, 2020 at 04:22:13PM +0100, Enrico Weigelt, metux IT consult wrote:
> For device log outputs, it's better to have device name / ID
> prefixed in all messages, so use the proper dev_*() functions here.
>
> Explicit message on module load/unload don't seem to be really helpful
> (we have other means to check which modules have been loaded), instead
> just add noise to the kernel log. So, removing them.
>
> Signed-off-by: Enrico Weigelt, metux IT consult <[email protected]>
Reviewed-by: Guenter Roeck <[email protected]>
> ---
> drivers/watchdog/iTCO_wdt.c | 18 ++++++++----------
> 1 file changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
> index f2ddc8fc71cd..edc588a06ae6 100644
> --- a/drivers/watchdog/iTCO_wdt.c
> +++ b/drivers/watchdog/iTCO_wdt.c
> @@ -40,8 +40,6 @@
> * Includes, defines, variables, module parameters, ...
> */
>
> -#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> -
> /* Module and version information */
> #define DRV_NAME "iTCO_wdt"
> #define DRV_VERSION "1.11"
> @@ -279,7 +277,7 @@ static int iTCO_wdt_start(struct watchdog_device *wd_dev)
> /* disable chipset's NO_REBOOT bit */
> if (p->update_no_reboot_bit(p->no_reboot_priv, false)) {
> spin_unlock(&p->io_lock);
> - pr_err("failed to reset NO_REBOOT flag, reboot disabled by hardware/BIOS\n");
> + dev_err(wd_dev->dev, "failed to reset NO_REBOOT flag, reboot disabled by hardware/BIOS\n");
> return -EIO;
> }
>
> @@ -510,7 +508,7 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
> /* Check chipset's NO_REBOOT bit */
> if (p->update_no_reboot_bit(p->no_reboot_priv, false) &&
> iTCO_vendor_check_noreboot_on()) {
> - pr_info("unable to reset NO_REBOOT flag, device disabled by hardware/BIOS\n");
> + dev_info(dev, "unable to reset NO_REBOOT flag, device disabled by hardware/BIOS\n");
> return -ENODEV; /* Cannot reset NO_REBOOT bit */
> }
>
> @@ -530,12 +528,12 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
> if (!devm_request_region(dev, p->tco_res->start,
> resource_size(p->tco_res),
> pdev->name)) {
> - pr_err("I/O address 0x%04llx already in use, device disabled\n",
> + dev_err(dev, "I/O address 0x%04llx already in use, device disabled\n",
> (u64)TCOBASE(p));
> return -EBUSY;
> }
>
> - pr_info("Found a %s TCO device (Version=%d, TCOBASE=0x%04llx)\n",
> + dev_info(dev, "Found a %s TCO device (Version=%d, TCOBASE=0x%04llx)\n",
> pdata->name, pdata->version, (u64)TCOBASE(p));
>
> /* Clear out the (probably old) status */
> @@ -558,7 +556,7 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
> break;
> }
>
> - p->wddev.info = &ident,
> + p->wddev.info = &ident,
> p->wddev.ops = &iTCO_wdt_ops,
> p->wddev.bootstatus = 0;
> p->wddev.timeout = WATCHDOG_TIMEOUT;
> @@ -575,7 +573,7 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
> if not reset to the default */
> if (iTCO_wdt_set_timeout(&p->wddev, heartbeat)) {
> iTCO_wdt_set_timeout(&p->wddev, WATCHDOG_TIMEOUT);
> - pr_info("timeout value out of range, using %d\n",
> + dev_info(dev, "timeout value out of range, using %d\n",
> WATCHDOG_TIMEOUT);
> }
>
> @@ -583,11 +581,11 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
> watchdog_stop_on_unregister(&p->wddev);
> ret = devm_watchdog_register_device(dev, &p->wddev);
> if (ret != 0) {
> - pr_err("cannot register watchdog device (err=%d)\n", ret);
> + dev_err(dev, "cannot register watchdog device (err=%d)\n", ret);
> return ret;
> }
>
> - pr_info("initialized. heartbeat=%d sec (nowayout=%d)\n",
> + dev_info(dev, "initialized. heartbeat=%d sec (nowayout=%d)\n",
> heartbeat, nowayout);
>
> return 0;
On Tue, Nov 17, 2020 at 04:22:14PM +0100, Enrico Weigelt, metux IT consult wrote:
> Reduce a little bit of boilerplate by using pm_ptr().
>
> Signed-off-by: Enrico Weigelt, metux IT consult <[email protected]>
> ---
> drivers/watchdog/iTCO_wdt.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
> index edc588a06ae6..660fc262fbc0 100644
> --- a/drivers/watchdog/iTCO_wdt.c
> +++ b/drivers/watchdog/iTCO_wdt.c
> @@ -636,16 +636,13 @@ static const struct dev_pm_ops iTCO_wdt_pm = {
> .resume_noirq = iTCO_wdt_resume_noirq,
> };
>
> -#define ITCO_WDT_PM_OPS (&iTCO_wdt_pm)
> -#else
> -#define ITCO_WDT_PM_OPS NULL
> #endif /* CONFIG_PM_SLEEP */
>
> static struct platform_driver iTCO_wdt_driver = {
> .probe = iTCO_wdt_probe,
> .driver = {
> .name = DRV_NAME,
> - .pm = ITCO_WDT_PM_OPS,
> + .pm = pm_ptr(&iTCO_wdt_pm),
Not sure if this works. The ifdef is for CONFIG_PM_SLEEP,
but pm_ptr depends on CONFIG_PM. While PM_SLEEP selects PM,
PM is also selected by other means. Other drivers using pm_ptr
also use SIMPLE_DEV_PM_OPS() and __maybe_unused for the function
declarations. This is not the case here. With this in mind,
I'd rather not make this change without changing the rest
of the code as well to ensure consistency.
Thanks,
Guenter
> },
> };
>
On Tue, Nov 17, 2020 at 04:22:13PM +0100, Enrico Weigelt, metux IT consult wrote:
> For device log outputs, it's better to have device name / ID
> prefixed in all messages, so use the proper dev_*() functions here.
>
> Explicit message on module load/unload don't seem to be really helpful
> (we have other means to check which modules have been loaded), instead
> just add noise to the kernel log. So, removing them.
>
> Signed-off-by: Enrico Weigelt, metux IT consult <[email protected]>
I like this patch a lot; thanks for doing it! It's merged upstream as
c21172b3a73e ("watchdog: iTCO_wdt: use dev_*() instead of pr_*() for
logging").
It looks like there are a couple more pr_err() uses, so I wondered if
they were missed or skipped intentionally:
if (p->smi_res) {
/* The TCO logic uses the TCO_EN bit in the SMI_EN register */
if (!devm_request_region(dev, p->smi_res->start,
resource_size(p->smi_res),
pdev->name)) {
pr_err("I/O address 0x%04llx already in use, device disabled\n",
(u64)SMI_EN(p));
return -EBUSY;
}
} else if (iTCO_vendorsupport ||
turn_SMI_watchdog_clear_off >= p->iTCO_version) {
pr_err("SMI I/O resource is missing\n");
return -ENODEV;
}
> ---
> drivers/watchdog/iTCO_wdt.c | 18 ++++++++----------
> 1 file changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
> index f2ddc8fc71cd..edc588a06ae6 100644
> --- a/drivers/watchdog/iTCO_wdt.c
> +++ b/drivers/watchdog/iTCO_wdt.c
> @@ -40,8 +40,6 @@
> * Includes, defines, variables, module parameters, ...
> */
>
> -#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> -
> /* Module and version information */
> #define DRV_NAME "iTCO_wdt"
> #define DRV_VERSION "1.11"
> @@ -279,7 +277,7 @@ static int iTCO_wdt_start(struct watchdog_device *wd_dev)
> /* disable chipset's NO_REBOOT bit */
> if (p->update_no_reboot_bit(p->no_reboot_priv, false)) {
> spin_unlock(&p->io_lock);
> - pr_err("failed to reset NO_REBOOT flag, reboot disabled by hardware/BIOS\n");
> + dev_err(wd_dev->dev, "failed to reset NO_REBOOT flag, reboot disabled by hardware/BIOS\n");
> return -EIO;
> }
>
> @@ -510,7 +508,7 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
> /* Check chipset's NO_REBOOT bit */
> if (p->update_no_reboot_bit(p->no_reboot_priv, false) &&
> iTCO_vendor_check_noreboot_on()) {
> - pr_info("unable to reset NO_REBOOT flag, device disabled by hardware/BIOS\n");
> + dev_info(dev, "unable to reset NO_REBOOT flag, device disabled by hardware/BIOS\n");
> return -ENODEV; /* Cannot reset NO_REBOOT bit */
> }
>
> @@ -530,12 +528,12 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
> if (!devm_request_region(dev, p->tco_res->start,
> resource_size(p->tco_res),
> pdev->name)) {
> - pr_err("I/O address 0x%04llx already in use, device disabled\n",
> + dev_err(dev, "I/O address 0x%04llx already in use, device disabled\n",
> (u64)TCOBASE(p));
> return -EBUSY;
> }
>
> - pr_info("Found a %s TCO device (Version=%d, TCOBASE=0x%04llx)\n",
> + dev_info(dev, "Found a %s TCO device (Version=%d, TCOBASE=0x%04llx)\n",
> pdata->name, pdata->version, (u64)TCOBASE(p));
>
> /* Clear out the (probably old) status */
> @@ -558,7 +556,7 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
> break;
> }
>
> - p->wddev.info = &ident,
> + p->wddev.info = &ident,
> p->wddev.ops = &iTCO_wdt_ops,
> p->wddev.bootstatus = 0;
> p->wddev.timeout = WATCHDOG_TIMEOUT;
> @@ -575,7 +573,7 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
> if not reset to the default */
> if (iTCO_wdt_set_timeout(&p->wddev, heartbeat)) {
> iTCO_wdt_set_timeout(&p->wddev, WATCHDOG_TIMEOUT);
> - pr_info("timeout value out of range, using %d\n",
> + dev_info(dev, "timeout value out of range, using %d\n",
> WATCHDOG_TIMEOUT);
> }
>
> @@ -583,11 +581,11 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
> watchdog_stop_on_unregister(&p->wddev);
> ret = devm_watchdog_register_device(dev, &p->wddev);
> if (ret != 0) {
> - pr_err("cannot register watchdog device (err=%d)\n", ret);
> + dev_err(dev, "cannot register watchdog device (err=%d)\n", ret);
> return ret;
> }
>
> - pr_info("initialized. heartbeat=%d sec (nowayout=%d)\n",
> + dev_info(dev, "initialized. heartbeat=%d sec (nowayout=%d)\n",
> heartbeat, nowayout);
>
> return 0;
> --
> 2.11.0
>
On Wed, Jun 09, 2021 at 03:32:29PM -0500, Bjorn Helgaas wrote:
> On Tue, Nov 17, 2020 at 04:22:13PM +0100, Enrico Weigelt, metux IT consult wrote:
> > For device log outputs, it's better to have device name / ID
> > prefixed in all messages, so use the proper dev_*() functions here.
> >
> > Explicit message on module load/unload don't seem to be really helpful
> > (we have other means to check which modules have been loaded), instead
> > just add noise to the kernel log. So, removing them.
> >
> > Signed-off-by: Enrico Weigelt, metux IT consult <[email protected]>
>
> I like this patch a lot; thanks for doing it! It's merged upstream as
> c21172b3a73e ("watchdog: iTCO_wdt: use dev_*() instead of pr_*() for
> logging").
>
> It looks like there are a couple more pr_err() uses, so I wondered if
> they were missed or skipped intentionally:
>
> if (p->smi_res) {
> /* The TCO logic uses the TCO_EN bit in the SMI_EN register */
> if (!devm_request_region(dev, p->smi_res->start,
> resource_size(p->smi_res),
> pdev->name)) {
> pr_err("I/O address 0x%04llx already in use, device disabled\n",
> (u64)SMI_EN(p));
> return -EBUSY;
> }
> } else if (iTCO_vendorsupport ||
> turn_SMI_watchdog_clear_off >= p->iTCO_version) {
> pr_err("SMI I/O resource is missing\n");
> return -ENODEV;
> }
>
The above came in with a recent commit. I suspect they simply got lost,
and they should be converted as well.
Guenter
On 10.06.21 01:54, Guenter Roeck wrote:
> The above came in with a recent commit. I suspect they simply got lost,
> and they should be converted as well.
Since I might need to touch it again in near future, I'll do it in
one of the next rounds.
--mtx
--
---
Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
GPG/PGP-Schlüssel zu.
---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
[email protected] -- +49-151-27565287
On Mon, Jun 14, 2021 at 07:55:24PM +0200, Enrico Weigelt, metux IT consult wrote:
> On 10.06.21 01:54, Guenter Roeck wrote:
>
> > The above came in with a recent commit. I suspect they simply got lost,
> > and they should be converted as well.
>
> Since I might need to touch it again in near future, I'll do it in
> one of the next rounds.
Thanks!