Re: [PATCH v1] watchdog: ni903x_wdt: Convert to a platform driver

[Guenter Roeck]

On 3/14/26 04:53, Rafael J. Wysocki wrote:
> From: "Rafael J. Wysocki" <rafael.j.wysocki@xxxxxxxxx>
>
> In all cases in which a struct acpi_driver is used for binding a driver
> to an ACPI device object, a corresponding platform device is created by
> the ACPI core and that device is regarded as a proper representation of
> underlying hardware.  Accordingly, a struct platform_driver should be
> used by driver code to bind to that device.  There are multiple reasons
> why drivers should not bind directly to ACPI device objects [1].
>
> In particular, registering a watchdog device under a struct acpi_device
> is questionable because it causes the watchdog to be hidden in the ACPI
> bus sysfs hierarchy and it goes against the general rule that a struct
> acpi_device can only be a parent of another struct acpi_device.
>
> Overall, it is better to bind drivers to platform devices than to their
> ACPI companions, so convert the ni903x_wdt watchdog ACPI driver to a
> platform one.
>
> While this is not expected to alter functionality, it changes sysfs
> layout and so it will be visible to user space.
>
> Note that after this change it actually makes sense to look for
> the "timeout-sec" property via device_property_read_u32() under the
> device passed to watchdog_init_timeout() because it has an fwnode
> handle (unlike a struct acpi_device which is an fwnode itself).
>
> Link: https://lore.kernel.org/all/2396510.ElGaqSPkdT@rafael.j.wysocki/ [1]
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>

Reviewed-by: Guenter Roeck <linux@xxxxxxxxxxxx>

> ---
>   drivers/watchdog/ni903x_wdt.c | 27 ++++++++++++++-------------
>   1 file changed, 14 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/watchdog/ni903x_wdt.c b/drivers/watchdog/ni903x_wdt.c
> index 045bb72d9a43..8b1b9baa914e 100644
> --- a/drivers/watchdog/ni903x_wdt.c
> +++ b/drivers/watchdog/ni903x_wdt.c
> @@ -8,6 +8,7 @@
>   #include <linux/interrupt.h>
>   #include <linux/io.h>
>   #include <linux/module.h>
> +#include <linux/platform_device.h>
>   #include <linux/watchdog.h>
>   
>   #define NIWD_CONTROL	0x01
> @@ -177,9 +178,9 @@ static const struct watchdog_ops ni903x_wdd_ops = {
>   	.get_timeleft = ni903x_wdd_get_timeleft,
>   };
>   
> -static int ni903x_acpi_add(struct acpi_device *device)
> +static int ni903x_acpi_probe(struct platform_device *pdev)
>   {
> -	struct device *dev = &device->dev;
> +	struct device *dev = &pdev->dev;
>   	struct watchdog_device *wdd;
>   	struct ni903x_wdt *wdt;
>   	acpi_status status;
> @@ -189,10 +190,10 @@ static int ni903x_acpi_add(struct acpi_device *device)
>   	if (!wdt)
>   		return -ENOMEM;
>   
> -	device->driver_data = wdt;
> +	platform_set_drvdata(pdev, wdt);
>   	wdt->dev = dev;
>   
> -	status = acpi_walk_resources(device->handle, METHOD_NAME__CRS,
> +	status = acpi_walk_resources(ACPI_HANDLE(dev), METHOD_NAME__CRS,
>   				     ni903x_resources, wdt);
>   	if (ACPI_FAILURE(status) || wdt->io_base == 0) {
>   		dev_err(dev, "failed to get resources\n");
> @@ -224,9 +225,9 @@ static int ni903x_acpi_add(struct acpi_device *device)
>   	return 0;
>   }
>   
> -static void ni903x_acpi_remove(struct acpi_device *device)
> +static void ni903x_acpi_remove(struct platform_device *pdev)
>   {
> -	struct ni903x_wdt *wdt = acpi_driver_data(device);
> +	struct ni903x_wdt *wdt = platform_get_drvdata(pdev);
>   
>   	ni903x_wdd_stop(&wdt->wdd);
>   	watchdog_unregister_device(&wdt->wdd);
> @@ -238,16 +239,16 @@ static const struct acpi_device_id ni903x_device_ids[] = {
>   };
>   MODULE_DEVICE_TABLE(acpi, ni903x_device_ids);
>   
> -static struct acpi_driver ni903x_acpi_driver = {
> -	.name = NIWD_NAME,
> -	.ids = ni903x_device_ids,
> -	.ops = {
> -		.add = ni903x_acpi_add,
> -		.remove = ni903x_acpi_remove,
> +static struct platform_driver ni903x_acpi_driver = {
> +	.probe = ni903x_acpi_probe,
> +	.remove = ni903x_acpi_remove,
> +	.driver = {
> +		.name = NIWD_NAME,
> +		.acpi_match_table = ni903x_device_ids,
>   	},
>   };
>   
> -module_acpi_driver(ni903x_acpi_driver);
> +module_platform_driver(ni903x_acpi_driver);
>   
>   MODULE_DESCRIPTION("NI 903x Watchdog");
>   MODULE_AUTHOR("Jeff Westfahl <jeff.westfahl@xxxxxx>");