[PATCH v2 0/4] hwmon: (sch56xx) Use devres for watchdog and platform_device_register_simple()

[PATCH v2 0/4] hwmon: (sch56xx) Use devres for watchdog and platform_device_register_simple()

[W_Armin]

From: Armin Wolf <W_Armin@gmx.de>

Since it was requested to use devres for the watchdog device,
use devm_watchdog_register() for watchdog registration and do
some small cleanups.

Also use platform_device_register_simple() in sch56xx_device_add().

Tested on a Fujitsu Esprimo P720.

v2:
- drop sch5627_remove()
- make sch56xx_watchdog_register() return void

Armin Wolf (4):
  hwmon: (sch56xx) Use devres functions for watchdog
  hwmon: (sch56xx-common) Use strscpy
  hwmon: (sch56xx-common) Use helper function
  hwmon: (sch56xx-common) Simplify sch56xx_device_add

 drivers/hwmon/sch5627.c        | 18 ++--------
 drivers/hwmon/sch5636.c        |  9 ++---
 drivers/hwmon/sch56xx-common.c | 65 ++++++++++------------------------
 drivers/hwmon/sch56xx-common.h |  4 +--
 4 files changed, 25 insertions(+), 71 deletions(-)

--
2.20.1

[PATCH v2 1/4] hwmon: (sch56xx) Use devres functions for watchdog

[W_Armin]

From: Armin Wolf <W_Armin@gmx.de>

Use devm_kzalloc()/devm_watchdog_register() for
watchdog registration since it allows us to remove
the sch56xx_watchdog_data struct from the drivers
own data structs.
Remove sch56xx_watchdog_unregister since devres
takes care of that now.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/hwmon/sch5627.c        | 18 +++---------------
 drivers/hwmon/sch5636.c        |  9 ++-------
 drivers/hwmon/sch56xx-common.c | 28 ++++++++++------------------
 drivers/hwmon/sch56xx-common.h |  4 ++--
 4 files changed, 17 insertions(+), 42 deletions(-)

diff --git a/drivers/hwmon/sch5627.c b/drivers/hwmon/sch5627.c
index 4324a5dbc968..8f1b569c69e7 100644
--- a/drivers/hwmon/sch5627.c
+++ b/drivers/hwmon/sch5627.c
@@ -64,7 +64,6 @@ static const char * const SCH5627_IN_LABELS[SCH5627_NO_IN] = {

 struct sch5627_data {
 	unsigned short addr;
-	struct sch56xx_watchdog_data *watchdog;
 	u8 control;
 	u8 temp_max[SCH5627_NO_TEMPS];
 	u8 temp_crit[SCH5627_NO_TEMPS];
@@ -357,16 +356,6 @@ static const struct hwmon_chip_info sch5627_chip_info = {
 	.info = sch5627_info,
 };

-static int sch5627_remove(struct platform_device *pdev)
-{
-	struct sch5627_data *data = platform_get_drvdata(pdev);
-
-	if (data->watchdog)
-		sch56xx_watchdog_unregister(data->watchdog);
-
-	return 0;
-}
-
 static int sch5627_probe(struct platform_device *pdev)
 {
 	struct sch5627_data *data;
@@ -460,9 +449,9 @@ static int sch5627_probe(struct platform_device *pdev)
 		return PTR_ERR(hwmon_dev);

 	/* Note failing to register the watchdog is not a fatal error */
-	data->watchdog = sch56xx_watchdog_register(&pdev->dev, data->addr,
-			(build_code << 24) | (build_id << 8) | hwmon_rev,
-			&data->update_lock, 1);
+	sch56xx_watchdog_register(&pdev->dev, data->addr,
+				  (build_code << 24) | (build_id << 8) | hwmon_rev,
+				  &data->update_lock, 1);

 	return 0;
 }
@@ -472,7 +461,6 @@ static struct platform_driver sch5627_driver = {
 		.name	= DRVNAME,
 	},
 	.probe		= sch5627_probe,
-	.remove		= sch5627_remove,
 };

 module_platform_driver(sch5627_driver);
diff --git a/drivers/hwmon/sch5636.c b/drivers/hwmon/sch5636.c
index 5683a38740f6..a5cd4de36575 100644
--- a/drivers/hwmon/sch5636.c
+++ b/drivers/hwmon/sch5636.c
@@ -54,7 +54,6 @@ static const u16 SCH5636_REG_FAN_VAL[SCH5636_NO_FANS] = {
 struct sch5636_data {
 	unsigned short addr;
 	struct device *hwmon_dev;
-	struct sch56xx_watchdog_data *watchdog;

 	struct mutex update_lock;
 	char valid;			/* !=0 if following fields are valid */
@@ -372,9 +371,6 @@ static int sch5636_remove(struct platform_device *pdev)
 	struct sch5636_data *data = platform_get_drvdata(pdev);
 	int i;

-	if (data->watchdog)
-		sch56xx_watchdog_unregister(data->watchdog);
-
 	if (data->hwmon_dev)
 		hwmon_device_unregister(data->hwmon_dev);

@@ -495,9 +491,8 @@ static int sch5636_probe(struct platform_device *pdev)
 	}

 	/* Note failing to register the watchdog is not a fatal error */
-	data->watchdog = sch56xx_watchdog_register(&pdev->dev, data->addr,
-					(revision[0] << 8) | revision[1],
-					&data->update_lock, 0);
+	sch56xx_watchdog_register(&pdev->dev, data->addr, (revision[0] << 8) | revision[1],
+				  &data->update_lock, 0);

 	return 0;

diff --git a/drivers/hwmon/sch56xx-common.c b/drivers/hwmon/sch56xx-common.c
index 6c84780e358e..2db3456ba6f1 100644
--- a/drivers/hwmon/sch56xx-common.c
+++ b/drivers/hwmon/sch56xx-common.c
@@ -378,8 +378,8 @@ static const struct watchdog_ops watchdog_ops = {
 	.set_timeout	= watchdog_set_timeout,
 };

-struct sch56xx_watchdog_data *sch56xx_watchdog_register(struct device *parent,
-	u16 addr, u32 revision, struct mutex *io_lock, int check_enabled)
+void sch56xx_watchdog_register(struct device *parent, u16 addr, u32 revision,
+			       struct mutex *io_lock, int check_enabled)
 {
 	struct sch56xx_watchdog_data *data;
 	int err, control, output_enable;
@@ -393,17 +393,17 @@ struct sch56xx_watchdog_data *sch56xx_watchdog_register(struct device *parent,
 	mutex_unlock(io_lock);

 	if (control < 0)
-		return NULL;
+		return;
 	if (output_enable < 0)
-		return NULL;
+		return;
 	if (check_enabled && !(output_enable & SCH56XX_WDOG_OUTPUT_ENABLE)) {
 		pr_warn("Watchdog not enabled by BIOS, not registering\n");
-		return NULL;
+		return;
 	}

-	data = kzalloc(sizeof(struct sch56xx_watchdog_data), GFP_KERNEL);
+	data = devm_kzalloc(parent, sizeof(struct sch56xx_watchdog_data), GFP_KERNEL);
 	if (!data)
-		return NULL;
+		return;

 	data->addr = addr;
 	data->io_lock = io_lock;
@@ -438,24 +438,16 @@ struct sch56xx_watchdog_data *sch56xx_watchdog_register(struct device *parent,
 	data->watchdog_output_enable = output_enable;

 	watchdog_set_drvdata(&data->wddev, data);
-	err = watchdog_register_device(&data->wddev);
+	err = devm_watchdog_register_device(parent, &data->wddev);
 	if (err) {
 		pr_err("Registering watchdog chardev: %d\n", err);
-		kfree(data);
-		return NULL;
+		devm_kfree(parent, data);
 	}

-	return data;
+	return;
 }
 EXPORT_SYMBOL(sch56xx_watchdog_register);

-void sch56xx_watchdog_unregister(struct sch56xx_watchdog_data *data)
-{
-	watchdog_unregister_device(&data->wddev);
-	kfree(data);
-}
-EXPORT_SYMBOL(sch56xx_watchdog_unregister);
-
 /*
  * platform dev find, add and remove functions
  */
diff --git a/drivers/hwmon/sch56xx-common.h b/drivers/hwmon/sch56xx-common.h
index 75eb73617cf2..e907d9da0dd5 100644
--- a/drivers/hwmon/sch56xx-common.h
+++ b/drivers/hwmon/sch56xx-common.h
@@ -14,6 +14,6 @@ int sch56xx_read_virtual_reg16(u16 addr, u16 reg);
 int sch56xx_read_virtual_reg12(u16 addr, u16 msb_reg, u16 lsn_reg,
 			       int high_nibble);

-struct sch56xx_watchdog_data *sch56xx_watchdog_register(struct device *parent,
-	u16 addr, u32 revision, struct mutex *io_lock, int check_enabled);
+void sch56xx_watchdog_register(struct device *parent, u16 addr, u32 revision,
+			       struct mutex *io_lock, int check_enabled);
 void sch56xx_watchdog_unregister(struct sch56xx_watchdog_data *data);
--
2.20.1

Re: [PATCH v2 1/4] hwmon: (sch56xx) Use devres functions for watchdog

[Guenter Roeck]

On Sat, May 08, 2021 at 03:14:54PM +0200, W_Armin@gmx.de wrote:
> From: Armin Wolf <W_Armin@gmx.de>
> 
> Use devm_kzalloc()/devm_watchdog_register() for
> watchdog registration since it allows us to remove
> the sch56xx_watchdog_data struct from the drivers
> own data structs.
> Remove sch56xx_watchdog_unregister since devres
> takes care of that now.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>

Applied, with cosmetic change.

[ ... ]
> -	return data;
> +	return;
>  }

Dropped unnecessary return statement.

Guenter

[PATCH v2 2/4] hwmon: (sch56xx-common) Use strscpy

[W_Armin]

From: Armin Wolf <W_Armin@gmx.de>

strlcpy is considered deprecated.
Replace it with strscpy.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/hwmon/sch56xx-common.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/hwmon/sch56xx-common.c b/drivers/hwmon/sch56xx-common.c
index 2db3456ba6f1..0b3250fc57b8 100644
--- a/drivers/hwmon/sch56xx-common.c
+++ b/drivers/hwmon/sch56xx-common.c
@@ -408,8 +408,7 @@ void sch56xx_watchdog_register(struct device *parent, u16 addr, u32 revision,
 	data->addr = addr;
 	data->io_lock = io_lock;

-	strlcpy(data->wdinfo.identity, "sch56xx watchdog",
-		sizeof(data->wdinfo.identity));
+	strscpy(data->wdinfo.identity, "sch56xx watchdog", sizeof(data->wdinfo.identity));
 	data->wdinfo.firmware_version = revision;
 	data->wdinfo.options = WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT;
 	if (!nowayout)
--
2.20.1

Re: [PATCH v2 2/4] hwmon: (sch56xx-common) Use strscpy

[Guenter Roeck]

On Sat, May 08, 2021 at 03:14:55PM +0200, W_Armin@gmx.de wrote:
> From: Armin Wolf <W_Armin@gmx.de>
> 
> strlcpy is considered deprecated.
> Replace it with strscpy.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>

Applied.

Thanks,
Guenter

[PATCH v2 3/4] hwmon: (sch56xx-common) Use helper function

[W_Armin]

From: Armin Wolf <W_Armin@gmx.de>

Use watchdog_set_nowayout() to process param
setting and change param type to bool.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/hwmon/sch56xx-common.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/hwmon/sch56xx-common.c b/drivers/hwmon/sch56xx-common.c
index 0b3250fc57b8..180801f80118 100644
--- a/drivers/hwmon/sch56xx-common.c
+++ b/drivers/hwmon/sch56xx-common.c
@@ -20,8 +20,8 @@
 #include "sch56xx-common.h"

 /* Insmod parameters */
-static int nowayout = WATCHDOG_NOWAYOUT;
-module_param(nowayout, int, 0);
+static bool nowayout = WATCHDOG_NOWAYOUT;
+module_param(nowayout, bool, 0);
 MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
 	__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");

@@ -420,8 +420,7 @@ void sch56xx_watchdog_register(struct device *parent, u16 addr, u32 revision,
 	data->wddev.timeout = 60;
 	data->wddev.min_timeout = 1;
 	data->wddev.max_timeout = 255 * 60;
-	if (nowayout)
-		set_bit(WDOG_NO_WAY_OUT, &data->wddev.status);
+	watchdog_set_nowayout(&data->wddev, nowayout);
 	if (output_enable & SCH56XX_WDOG_OUTPUT_ENABLE)
 		set_bit(WDOG_ACTIVE, &data->wddev.status);

--
2.20.1

Re: [PATCH v2 3/4] hwmon: (sch56xx-common) Use helper function

[Guenter Roeck]

On Sat, May 08, 2021 at 03:14:56PM +0200, W_Armin@gmx.de wrote:
> From: Armin Wolf <W_Armin@gmx.de>
> 
> Use watchdog_set_nowayout() to process param
> setting and change param type to bool.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>

Applied. Unrelated comment below.

> ---
>  drivers/hwmon/sch56xx-common.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> --
> 2.20.1
> 
> diff --git a/drivers/hwmon/sch56xx-common.c b/drivers/hwmon/sch56xx-common.c
> index 0b3250fc57b8..180801f80118 100644
> --- a/drivers/hwmon/sch56xx-common.c
> +++ b/drivers/hwmon/sch56xx-common.c
> @@ -20,8 +20,8 @@
>  #include "sch56xx-common.h"
> 
>  /* Insmod parameters */
> -static int nowayout = WATCHDOG_NOWAYOUT;
> -module_param(nowayout, int, 0);
> +static bool nowayout = WATCHDOG_NOWAYOUT;
> +module_param(nowayout, bool, 0);
>  MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
>  	__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> 
> @@ -420,8 +420,7 @@ void sch56xx_watchdog_register(struct device *parent, u16 addr, u32 revision,
>  	data->wddev.timeout = 60;
>  	data->wddev.min_timeout = 1;
>  	data->wddev.max_timeout = 255 * 60;
> -	if (nowayout)
> -		set_bit(WDOG_NO_WAY_OUT, &data->wddev.status);
> +	watchdog_set_nowayout(&data->wddev, nowayout);
>  	if (output_enable & SCH56XX_WDOG_OUTPUT_ENABLE)
>  		set_bit(WDOG_ACTIVE, &data->wddev.status);
> 
This should really set WDOG_HW_RUNNING.

Guenter

[PATCH v2 4/4] hwmon: (sch56xx-common) Simplify sch56xx_device_add

[W_Armin]

From: Armin Wolf <W_Armin@gmx.de>

Use platform_device_register_simple() instead of
manually calling platform_device_alloc()/platform_device_add().

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/hwmon/sch56xx-common.c | 27 ++++-----------------------
 1 file changed, 4 insertions(+), 23 deletions(-)

diff --git a/drivers/hwmon/sch56xx-common.c b/drivers/hwmon/sch56xx-common.c
index 180801f80118..5307fda73e70 100644
--- a/drivers/hwmon/sch56xx-common.c
+++ b/drivers/hwmon/sch56xx-common.c
@@ -506,37 +506,18 @@ static int __init sch56xx_device_add(int address, const char *name)
 	struct resource res = {
 		.start	= address,
 		.end	= address + REGION_LENGTH - 1,
+		.name	= name,
 		.flags	= IORESOURCE_IO,
 	};
 	int err;

-	sch56xx_pdev = platform_device_alloc(name, address);
-	if (!sch56xx_pdev)
-		return -ENOMEM;
-
-	res.name = sch56xx_pdev->name;
 	err = acpi_check_resource_conflict(&res);
 	if (err)
-		goto exit_device_put;
-
-	err = platform_device_add_resources(sch56xx_pdev, &res, 1);
-	if (err) {
-		pr_err("Device resource addition failed\n");
-		goto exit_device_put;
-	}
-
-	err = platform_device_add(sch56xx_pdev);
-	if (err) {
-		pr_err("Device addition failed\n");
-		goto exit_device_put;
-	}
-
-	return 0;
+		return err;

-exit_device_put:
-	platform_device_put(sch56xx_pdev);
+	sch56xx_pdev = platform_device_register_simple(name, -1, &res, 1);

-	return err;
+	return PTR_ERR_OR_ZERO(sch56xx_pdev);
 }

 static int __init sch56xx_init(void)
--
2.20.1

Re: [PATCH v2 4/4] hwmon: (sch56xx-common) Simplify sch56xx_device_add

[Guenter Roeck]

On Sat, May 08, 2021 at 03:14:57PM +0200, W_Armin@gmx.de wrote:
> From: Armin Wolf <W_Armin@gmx.de>
> 
> Use platform_device_register_simple() instead of
> manually calling platform_device_alloc()/platform_device_add().
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>

Applied.

Thanks,
Guenter

> ---
>  drivers/hwmon/sch56xx-common.c | 27 ++++-----------------------
>  1 file changed, 4 insertions(+), 23 deletions(-)
> 
> --
> 2.20.1
> 
> diff --git a/drivers/hwmon/sch56xx-common.c b/drivers/hwmon/sch56xx-common.c
> index 180801f80118..5307fda73e70 100644
> --- a/drivers/hwmon/sch56xx-common.c
> +++ b/drivers/hwmon/sch56xx-common.c
> @@ -506,37 +506,18 @@ static int __init sch56xx_device_add(int address, const char *name)
>  	struct resource res = {
>  		.start	= address,
>  		.end	= address + REGION_LENGTH - 1,
> +		.name	= name,
>  		.flags	= IORESOURCE_IO,
>  	};
>  	int err;
> 
> -	sch56xx_pdev = platform_device_alloc(name, address);
> -	if (!sch56xx_pdev)
> -		return -ENOMEM;
> -
> -	res.name = sch56xx_pdev->name;
>  	err = acpi_check_resource_conflict(&res);
>  	if (err)
> -		goto exit_device_put;
> -
> -	err = platform_device_add_resources(sch56xx_pdev, &res, 1);
> -	if (err) {
> -		pr_err("Device resource addition failed\n");
> -		goto exit_device_put;
> -	}
> -
> -	err = platform_device_add(sch56xx_pdev);
> -	if (err) {
> -		pr_err("Device addition failed\n");
> -		goto exit_device_put;
> -	}
> -
> -	return 0;
> +		return err;
> 
> -exit_device_put:
> -	platform_device_put(sch56xx_pdev);
> +	sch56xx_pdev = platform_device_register_simple(name, -1, &res, 1);
> 
> -	return err;
> +	return PTR_ERR_OR_ZERO(sch56xx_pdev);
>  }
> 
>  static int __init sch56xx_init(void)

Re: [PATCH v2 0/4] hwmon: (sch56xx) Use devres for watchdog and platform_device_register_simple()

[Hans de Goede]

Hi,

On 5/8/21 3:14 PM, W_Armin@gmx.de wrote:
> From: Armin Wolf <W_Armin@gmx.de>
> 
> Since it was requested to use devres for the watchdog device,
> use devm_watchdog_register() for watchdog registration and do
> some small cleanups.
> 
> Also use platform_device_register_simple() in sch56xx_device_add().
> 
> Tested on a Fujitsu Esprimo P720.
> 
> v2:
> - drop sch5627_remove()
> - make sch56xx_watchdog_register() return void

Thanks, series (still) looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

For the series.

Regards,

Hans


p.s.

For your next series when I give my Reviewed-by for v1 and
there are some small changes which require a v2, feel free to
add the Reviewed-by to the v2 series.




> 
> Armin Wolf (4):
>   hwmon: (sch56xx) Use devres functions for watchdog
>   hwmon: (sch56xx-common) Use strscpy
>   hwmon: (sch56xx-common) Use helper function
>   hwmon: (sch56xx-common) Simplify sch56xx_device_add
> 
>  drivers/hwmon/sch5627.c        | 18 ++--------
>  drivers/hwmon/sch5636.c        |  9 ++---
>  drivers/hwmon/sch56xx-common.c | 65 ++++++++++------------------------
>  drivers/hwmon/sch56xx-common.h |  4 +--
>  4 files changed, 25 insertions(+), 71 deletions(-)
> 
> --
> 2.20.1
>