linux-watchdog.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/3] watchdog: iTCO_wdt: use module_platform_device() macro
@ 2020-11-17 15:22 Enrico Weigelt, metux IT consult
  2020-11-17 15:22 ` [PATCH 2/3] watchdog: iTCO_wdt: use dev_*() instead of pr_*() for logging Enrico Weigelt, metux IT consult
  2020-11-17 15:22 ` [PATCH 3/3] watchdog: iTCO_wdt: use pm_ptr() Enrico Weigelt, metux IT consult
  0 siblings, 2 replies; 8+ messages in thread
From: Enrico Weigelt, metux IT consult @ 2020-11-17 15:22 UTC (permalink / raw)
  To: linux-kernel; +Cc: wim, linux, linux-watchdog

Reducing init boilerplate by using the module_platform_device macro.

Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
---
 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 <wim@iguana.be>");
 MODULE_DESCRIPTION("Intel TCO WatchDog Timer Driver");
-- 
2.11.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 2/3] watchdog: iTCO_wdt: use dev_*() instead of pr_*() for logging
  2020-11-17 15:22 [PATCH 1/3] watchdog: iTCO_wdt: use module_platform_device() macro Enrico Weigelt, metux IT consult
@ 2020-11-17 15:22 ` Enrico Weigelt, metux IT consult
  2021-06-09 20:32   ` Bjorn Helgaas
  2020-11-17 15:22 ` [PATCH 3/3] watchdog: iTCO_wdt: use pm_ptr() Enrico Weigelt, metux IT consult
  1 sibling, 1 reply; 8+ messages in thread
From: Enrico Weigelt, metux IT consult @ 2020-11-17 15:22 UTC (permalink / raw)
  To: linux-kernel; +Cc: wim, linux, linux-watchdog

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 <info@metux.net>
---
 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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 3/3] watchdog: iTCO_wdt: use pm_ptr()
  2020-11-17 15:22 [PATCH 1/3] watchdog: iTCO_wdt: use module_platform_device() macro Enrico Weigelt, metux IT consult
  2020-11-17 15:22 ` [PATCH 2/3] watchdog: iTCO_wdt: use dev_*() instead of pr_*() for logging Enrico Weigelt, metux IT consult
@ 2020-11-17 15:22 ` Enrico Weigelt, metux IT consult
  2020-11-30 22:17   ` Guenter Roeck
  1 sibling, 1 reply; 8+ messages in thread
From: Enrico Weigelt, metux IT consult @ 2020-11-17 15:22 UTC (permalink / raw)
  To: linux-kernel; +Cc: wim, linux, linux-watchdog

Reduce a little bit of boilerplate by using pm_ptr().

Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
---
 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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 3/3] watchdog: iTCO_wdt: use pm_ptr()
  2020-11-17 15:22 ` [PATCH 3/3] watchdog: iTCO_wdt: use pm_ptr() Enrico Weigelt, metux IT consult
@ 2020-11-30 22:17   ` Guenter Roeck
  0 siblings, 0 replies; 8+ messages in thread
From: Guenter Roeck @ 2020-11-30 22:17 UTC (permalink / raw)
  To: Enrico Weigelt, metux IT consult; +Cc: linux-kernel, wim, linux-watchdog

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 <info@metux.net>
> ---
>  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

>  	},
>  };
>  

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/3] watchdog: iTCO_wdt: use dev_*() instead of pr_*() for logging
  2020-11-17 15:22 ` [PATCH 2/3] watchdog: iTCO_wdt: use dev_*() instead of pr_*() for logging Enrico Weigelt, metux IT consult
@ 2021-06-09 20:32   ` Bjorn Helgaas
  2021-06-09 23:54     ` Guenter Roeck
  0 siblings, 1 reply; 8+ messages in thread
From: Bjorn Helgaas @ 2021-06-09 20:32 UTC (permalink / raw)
  To: Enrico Weigelt, metux IT consult; +Cc: linux-kernel, wim, linux, linux-watchdog

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 <info@metux.net>

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
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/3] watchdog: iTCO_wdt: use dev_*() instead of pr_*() for logging
  2021-06-09 20:32   ` Bjorn Helgaas
@ 2021-06-09 23:54     ` Guenter Roeck
  2021-06-14 17:55       ` Enrico Weigelt, metux IT consult
  0 siblings, 1 reply; 8+ messages in thread
From: Guenter Roeck @ 2021-06-09 23:54 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Enrico Weigelt, metux IT consult, linux-kernel, wim, linux-watchdog

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 <info@metux.net>
> 
> 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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/3] watchdog: iTCO_wdt: use dev_*() instead of pr_*() for logging
  2021-06-09 23:54     ` Guenter Roeck
@ 2021-06-14 17:55       ` Enrico Weigelt, metux IT consult
  2021-06-15 14:51         ` Bjorn Helgaas
  0 siblings, 1 reply; 8+ messages in thread
From: Enrico Weigelt, metux IT consult @ 2021-06-14 17:55 UTC (permalink / raw)
  To: Guenter Roeck, Bjorn Helgaas
  Cc: Enrico Weigelt, metux IT consult, linux-kernel, wim, linux-watchdog

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
info@metux.net -- +49-151-27565287

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/3] watchdog: iTCO_wdt: use dev_*() instead of pr_*() for logging
  2021-06-14 17:55       ` Enrico Weigelt, metux IT consult
@ 2021-06-15 14:51         ` Bjorn Helgaas
  0 siblings, 0 replies; 8+ messages in thread
From: Bjorn Helgaas @ 2021-06-15 14:51 UTC (permalink / raw)
  To: Enrico Weigelt, metux IT consult
  Cc: Guenter Roeck, Enrico Weigelt, metux IT consult, linux-kernel,
	wim, linux-watchdog

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!

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-06-15 14:51 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-17 15:22 [PATCH 1/3] watchdog: iTCO_wdt: use module_platform_device() macro Enrico Weigelt, metux IT consult
2020-11-17 15:22 ` [PATCH 2/3] watchdog: iTCO_wdt: use dev_*() instead of pr_*() for logging Enrico Weigelt, metux IT consult
2021-06-09 20:32   ` Bjorn Helgaas
2021-06-09 23:54     ` Guenter Roeck
2021-06-14 17:55       ` Enrico Weigelt, metux IT consult
2021-06-15 14:51         ` Bjorn Helgaas
2020-11-17 15:22 ` [PATCH 3/3] watchdog: iTCO_wdt: use pm_ptr() Enrico Weigelt, metux IT consult
2020-11-30 22:17   ` Guenter Roeck

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).