[v2,1/2] watchdog: f71808e_wdt: refactor to platform device/driver pair
diff mbox series

Message ID 20201020062112.6762-2-a.fatoum@pengutronix.de
State New, archived
Headers show
Series
  • watchdog: f71808e_wdt: migrate to new kernel API
Related show

Commit Message

Ahmad Fatoum Oct. 20, 2020, 6:21 a.m. UTC
Driver so far wasn't ported to the driver model and set up its
miscdevice out of the init after probing the I/O ports for a watchdog
with correct vendor and device revision.

Keep the device detection part at init time, but move watchdog setup
to a platform driver probe function.

While at it, refactor some of the driver code we have to now touch
anyway:

 - platform_device_id is used instead of the two big switches mapping
   hardware ID to an enum and then mapping it to a pinconf function
 - we rename f71808e_ and watchdog_data to fintek_wdt, to avoid mix up
   of the generic parts up with the device specific parts

Suggested-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/watchdog/f71808e_wdt.c | 377 +++++++++++++++++++--------------
 1 file changed, 215 insertions(+), 162 deletions(-)

Comments

Guenter Roeck Nov. 8, 2020, 5:26 p.m. UTC | #1
On Tue, Oct 20, 2020 at 08:21:11AM +0200, Ahmad Fatoum wrote:
> Driver so far wasn't ported to the driver model and set up its
> miscdevice out of the init after probing the I/O ports for a watchdog
> with correct vendor and device revision.
> 
> Keep the device detection part at init time, but move watchdog setup
> to a platform driver probe function.
> 
> While at it, refactor some of the driver code we have to now touch
> anyway:
> 
>  - platform_device_id is used instead of the two big switches mapping
>    hardware ID to an enum and then mapping it to a pinconf function
>  - we rename f71808e_ and watchdog_data to fintek_wdt, to avoid mix up
>    of the generic parts up with the device specific parts
> 
> Suggested-by: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

Sorry, I just found this. Sorry, it got lost.
The changes you are making here go way beyond a conversion to a platform
driver. I am not necessarily opposed to the changes, but they should be
split into multiple patches to simlify (and to a large degree enable)
a meaningful review. More comments inline.

> ---
>  drivers/watchdog/f71808e_wdt.c | 377 +++++++++++++++++++--------------
>  1 file changed, 215 insertions(+), 162 deletions(-)
> 
> diff --git a/drivers/watchdog/f71808e_wdt.c b/drivers/watchdog/f71808e_wdt.c
> index f60beec1bbae..4ff7a2509125 100644
> --- a/drivers/watchdog/f71808e_wdt.c
> +++ b/drivers/watchdog/f71808e_wdt.c
> @@ -9,12 +9,15 @@
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>  
>  #include <linux/err.h>
> +#include <linux/slab.h>
>  #include <linux/fs.h>
>  #include <linux/init.h>
>  #include <linux/io.h>
>  #include <linux/ioport.h>
>  #include <linux/miscdevice.h>
>  #include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/mod_devicetable.h>
>  #include <linux/mutex.h>
>  #include <linux/notifier.h>
>  #include <linux/reboot.h>
> @@ -110,22 +113,6 @@ module_param(start_withtimeout, uint, 0);
>  MODULE_PARM_DESC(start_withtimeout, "Start watchdog timer on module load with"
>  	" given initial timeout. Zero (default) disables this feature.");
>  
> -enum chips { f71808fg, f71858fg, f71862fg, f71868, f71869, f71882fg, f71889fg,
> -	     f81803, f81865, f81866};
> -
> -static const char *f71808e_names[] = {
> -	"f71808fg",
> -	"f71858fg",
> -	"f71862fg",
> -	"f71868",
> -	"f71869",
> -	"f71882fg",
> -	"f71889fg",
> -	"f81803",
> -	"f81865",
> -	"f81866",
> -};
> -
>  /* Super-I/O Function prototypes */
>  static inline int superio_inb(int base, int reg);
>  static inline int superio_inw(int base, int reg);
> @@ -136,9 +123,17 @@ static inline int superio_enter(int base);
>  static inline void superio_select(int base, int ld);
>  static inline void superio_exit(int base);
>  
> -struct watchdog_data {
> +struct fintek_wdt;
> +
> +struct fintek_wdt_variant {
> +	u16 id;
> +	void (*pinconf)(struct fintek_wdt *wd);
> +	const char *identity_override;
> +};
> +
> +struct fintek_wdt {
>  	unsigned short	sioaddr;
> -	enum chips	type;
> +	const struct fintek_wdt_variant *variant;

I'd rather have this all in here. Just add 
	void (*pinconf)(struct fintek_wdt *wd);
and assign it in the probe function, or call it
from an array indexed with type.

This change is also independent of the conversion to a platform
driver and should be done in a separate patch.

>  	unsigned long	opened;
>  	struct mutex	lock;
>  	char		expect_close;
> @@ -152,10 +147,15 @@ struct watchdog_data {
>  	char		caused_reboot;	/* last reboot was by the watchdog */
>  };
>  
> -static struct watchdog_data watchdog = {
> +static struct fintek_wdt watchdog = {
>  	.lock = __MUTEX_INITIALIZER(watchdog.lock),
>  };
>  
> +static inline bool has_f81865_wdo_conf(struct fintek_wdt *wd)
> +{
> +	return wd->variant->id == SIO_F81865_ID || wd->variant->id == SIO_F81866_ID;
> +}
> +
This change is independent of the conversion to a platform driver
and should be a separate patch.

>  /* Super I/O functions */
>  static inline int superio_inb(int base, int reg)
>  {
> @@ -247,7 +247,7 @@ static int watchdog_set_pulse_width(unsigned int pw)
>  	int err = 0;
>  	unsigned int t1 = 25, t2 = 125, t3 = 5000;
>  
> -	if (watchdog.type == f71868) {
> +	if (watchdog.variant->id == SIO_F71868_ID) {

I am not sure if dropping watchdog.type and replacing it with SIO_F71868_ID
is a good idea. In the current code, there is no 1:1 match of those,
specifically for SIO_F71869_ID/SIO_F71869A_ID. Deviating from that only
complicates the code.

>  		t1 = 30;
>  		t2 = 150;
>  		t3 = 6000;
> @@ -309,7 +309,6 @@ static int watchdog_keepalive(void)
>  static int watchdog_start(void)
>  {
>  	int err;
> -	u8 tmp;
>  
>  	/* Make sure we don't die as soon as the watchdog is enabled below */
>  	err = watchdog_keepalive();
> @@ -323,81 +322,12 @@ static int watchdog_start(void)
>  	superio_select(watchdog.sioaddr, SIO_F71808FG_LD_WDT);
>  
>  	/* Watchdog pin configuration */
> -	switch (watchdog.type) {
> -	case f71808fg:
> -		/* Set pin 21 to GPIO23/WDTRST#, then to WDTRST# */
> -		superio_clear_bit(watchdog.sioaddr, SIO_REG_MFUNCT2, 3);
> -		superio_clear_bit(watchdog.sioaddr, SIO_REG_MFUNCT3, 3);
> -		break;
> -
> -	case f71862fg:
> -		if (f71862fg_pin == 63) {
> -			/* SPI must be disabled first to use this pin! */
> -			superio_clear_bit(watchdog.sioaddr, SIO_REG_ROM_ADDR_SEL, 6);
> -			superio_set_bit(watchdog.sioaddr, SIO_REG_MFUNCT3, 4);
> -		} else if (f71862fg_pin == 56) {
> -			superio_set_bit(watchdog.sioaddr, SIO_REG_MFUNCT1, 1);
> -		}
> -		break;
> -
> -	case f71868:
> -	case f71869:
> -		/* GPIO14 --> WDTRST# */
> -		superio_clear_bit(watchdog.sioaddr, SIO_REG_MFUNCT1, 4);
> -		break;
> -
> -	case f71882fg:
> -		/* Set pin 56 to WDTRST# */
> -		superio_set_bit(watchdog.sioaddr, SIO_REG_MFUNCT1, 1);
> -		break;
> -
> -	case f71889fg:
> -		/* set pin 40 to WDTRST# */
> -		superio_outb(watchdog.sioaddr, SIO_REG_MFUNCT3,
> -			superio_inb(watchdog.sioaddr, SIO_REG_MFUNCT3) & 0xcf);
> -		break;
> -
> -	case f81803:
> -		/* Enable TSI Level register bank */
> -		superio_clear_bit(watchdog.sioaddr, SIO_REG_CLOCK_SEL, 3);
> -		/* Set pin 27 to WDTRST# */
> -		superio_outb(watchdog.sioaddr, SIO_REG_TSI_LEVEL_SEL, 0x5f &
> -			superio_inb(watchdog.sioaddr, SIO_REG_TSI_LEVEL_SEL));
> -		break;
> -
> -	case f81865:
> -		/* Set pin 70 to WDTRST# */
> -		superio_clear_bit(watchdog.sioaddr, SIO_REG_MFUNCT3, 5);
> -		break;
> -
> -	case f81866:
> -		/*
> -		 * GPIO1 Control Register when 27h BIT3:2 = 01 & BIT0 = 0.
> -		 * The PIN 70(GPIO15/WDTRST) is controlled by 2Ch:
> -		 *     BIT5: 0 -> WDTRST#
> -		 *           1 -> GPIO15
> -		 */
> -		tmp = superio_inb(watchdog.sioaddr, SIO_F81866_REG_PORT_SEL);
> -		tmp &= ~(BIT(3) | BIT(0));
> -		tmp |= BIT(2);
> -		superio_outb(watchdog.sioaddr, SIO_F81866_REG_PORT_SEL, tmp);
> -
> -		superio_clear_bit(watchdog.sioaddr, SIO_F81866_REG_GPIO1, 5);
> -		break;
> -
> -	default:
> -		/*
> -		 * 'default' label to shut up the compiler and catch
> -		 * programmer errors
> -		 */
> -		err = -ENODEV;
> -		goto exit_superio;
> -	}
> +	watchdog.variant->pinconf(&watchdog);
>  
>  	superio_select(watchdog.sioaddr, SIO_F71808FG_LD_WDT);
>  	superio_set_bit(watchdog.sioaddr, SIO_REG_ENABLE, 0);
>  
> -	if (watchdog.type == f81865 || watchdog.type == f81866)
> +	if (has_f81865_wdo_conf(&watchdog))
>  		superio_set_bit(watchdog.sioaddr, F81865_REG_WDO_CONF,
>  				F81865_FLAG_WDOUT_EN);
>  	else
> @@ -425,7 +355,6 @@ static int watchdog_start(void)
>  				F71808FG_FLAG_WD_PULSE);
>  	}
>  
> -exit_superio:
>  	superio_exit(watchdog.sioaddr);
>  exit_unlock:
>  	mutex_unlock(&watchdog.lock);
> @@ -665,21 +594,39 @@ static struct notifier_block watchdog_notifier = {
>  	.notifier_call = watchdog_notify_sys,
>  };
>  
> -static int __init watchdog_init(int sioaddr)
> +static int fintek_wdt_probe(struct platform_device *pdev)
>  {
> +	const struct platform_device_id *id;
> +	unsigned short sioaddr;
> +	struct resource *res;
>  	int wdt_conf, err = 0;
> +	const char *identity;
> +
> +	if (watchdog_miscdev.parent)
> +		return -EBUSY;
> +
> +	watchdog_miscdev.parent = &pdev->dev;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_IO, 0);
> +	if (!res)
> +		return -ENXIO;
> +
> +	id = platform_get_device_id(pdev);
> +	watchdog.variant = (void *)id->driver_data;
> +	if (!watchdog.variant)
> +		return -ENODEV;
>  
>  	/* No need to lock watchdog.lock here because no entry points
>  	 * into the module have been registered yet.
>  	 */
> -	watchdog.sioaddr = sioaddr;
> +	watchdog.sioaddr = res->start;
>  	watchdog.ident.options = WDIOF_MAGICCLOSE
>  				| WDIOF_KEEPALIVEPING
>  				| WDIOF_CARDRESET;
>  
> -	snprintf(watchdog.ident.identity,
> -		sizeof(watchdog.ident.identity), "%s watchdog",
> -		f71808e_names[watchdog.type]);
> +	identity = watchdog.variant->identity_override ?: id->name;
> +
> +	memcpy(watchdog.ident.identity, identity, sizeof(watchdog.ident.identity));
>  
>  	err = superio_enter(sioaddr);
>  	if (err)
> @@ -772,73 +719,176 @@ static int __init watchdog_init(int sioaddr)
>  	return err;
>  }
>  
> -static int __init f71808e_find(int sioaddr)
> +static void f71808fg_pinconf(struct fintek_wdt *wd)
>  {
> +	/* Set pin 21 to GPIO23/WDTRST#, then to WDTRST# */
> +	superio_clear_bit(wd->sioaddr, SIO_REG_MFUNCT2, 3);
> +	superio_clear_bit(wd->sioaddr, SIO_REG_MFUNCT3, 3);
> +}
> +
> +static void f71862fg_pinconf(struct fintek_wdt *wd)
> +{
> +	if (f71862fg_pin == 63) {
> +		/* SPI must be disabled first to use this pin! */
> +		superio_clear_bit(wd->sioaddr, SIO_REG_ROM_ADDR_SEL, 6);
> +		superio_set_bit(wd->sioaddr, SIO_REG_MFUNCT3, 4);
> +	} else if (f71862fg_pin == 56) {
> +		superio_set_bit(wd->sioaddr, SIO_REG_MFUNCT1, 1);
> +	}
> +}
> +
> +static void f71868_pinconf(struct fintek_wdt *wd)
> +{
> +	/* GPIO14 --> WDTRST# */
> +	superio_clear_bit(wd->sioaddr, SIO_REG_MFUNCT1, 4);
> +}
> +
> +static void f71882fg_pinconf(struct fintek_wdt *wd)
> +{
> +	/* Set pin 56 to WDTRST# */
> +	superio_set_bit(wd->sioaddr, SIO_REG_MFUNCT1, 1);
> +}
> +
> +static void f71889fg_pinconf(struct fintek_wdt *wd)
> +{
> +	/* set pin 40 to WDTRST# */
> +	superio_outb(wd->sioaddr, SIO_REG_MFUNCT3,
> +		     superio_inb(wd->sioaddr, SIO_REG_MFUNCT3) & 0xcf);
> +}
> +
> +static void f81803_pinconf(struct fintek_wdt *wd)
> +{
> +	/* Enable TSI Level register bank */
> +	superio_clear_bit(wd->sioaddr, SIO_REG_CLOCK_SEL, 3);
> +	/* Set pin 27 to WDTRST# */
> +	superio_outb(wd->sioaddr, SIO_REG_TSI_LEVEL_SEL, 0x5f &
> +		     superio_inb(wd->sioaddr, SIO_REG_TSI_LEVEL_SEL));
> +}
> +
> +static void f81865_pinconf(struct fintek_wdt *wd)
> +{
> +	/* Set pin 70 to WDTRST# */
> +	superio_clear_bit(wd->sioaddr, SIO_REG_MFUNCT3, 5);
> +}
> +
> +static void f81866_pinconf(struct fintek_wdt *wd)
> +{
> +	u8 tmp;
> +	/*
> +	 * GPIO1 Control Register when 27h BIT3:2 = 01 & BIT0 = 0.
> +	 * The PIN 70(GPIO15/WDTRST) is controlled by 2Ch:
> +	 *     BIT5: 0 -> WDTRST#
> +	 *           1 -> GPIO15
> +	 */
> +	tmp = superio_inb(wd->sioaddr, SIO_F81866_REG_PORT_SEL);
> +	tmp &= ~(BIT(3) | BIT(0));
> +	tmp |= BIT(2);
> +	superio_outb(wd->sioaddr, SIO_F81866_REG_PORT_SEL, tmp);
> +
> +	superio_clear_bit(wd->sioaddr, SIO_F81866_REG_GPIO1, 5);
> +}
> +
> +#define FINTEK_DRV_DATA(...) \
> +	(kernel_ulong_t)(&(struct fintek_wdt_variant) { __VA_ARGS__ })
> +
> +/*
> + * Array is used by both platform bus to map name to driver data and by
> + * init below to map id in driver_data to name for device creation.
> + */
> +static const struct platform_device_id fintek_wdt_id_table[] = {
> +	{ "f71808fg watchdog", FINTEK_DRV_DATA(SIO_F71808_ID,  f71808fg_pinconf) },
> +	{ "f71862fg watchdog", FINTEK_DRV_DATA(SIO_F71862_ID,  f71862fg_pinconf) },
> +	{ "f71868 watchdog",   FINTEK_DRV_DATA(SIO_F71868_ID,  f71868_pinconf) },
> +	{ "f71869 watchdog",   FINTEK_DRV_DATA(SIO_F71869_ID,  f71868_pinconf) },
> +	/* For backwards-compatibility, we define a different watchdog identity name */
> +	{ "f71869a watchdog",  FINTEK_DRV_DATA(SIO_F71869A_ID, f71868_pinconf,
> +					       .identity_override = "f71869 watchdog") },

I don't think .identity_override adds real value.

> +	{ "f71882fg watchdog", FINTEK_DRV_DATA(SIO_F71882_ID,  f71882fg_pinconf) },
> +	{ "f71889fg watchdog", FINTEK_DRV_DATA(SIO_F71889_ID,  f71889fg_pinconf) },
> +	{ "f81803 watchdog",   FINTEK_DRV_DATA(SIO_F81803_ID,  f81803_pinconf) },
> +	{ "f81865 watchdog",   FINTEK_DRV_DATA(SIO_F81865_ID,  f81865_pinconf) },
> +	{ "f81866 watchdog",   FINTEK_DRV_DATA(SIO_F81866_ID,  f81866_pinconf) },

I am not entirely sure I understand exactly what the macro does, and checkpatch
doesn't like it either.

Anyway, I don't think this complexity is necessary. Other similar drivers add
device information as platform driver data (using platform_device_add_data)
and get it using dev_get_platdata(). That would be much simpler and more
straightforward.

> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(platform, fintek_wdt_id_table);
> +
> +/* Confirmed (by datasheet) not to have a watchdog.
> + * We list this here anyway to maintain backwards-compatibility in not warning about them
> + */
> +static const u16 fintek_no_wdt_table[] = { SIO_F71858_ID, 0 };
> +
> +static int fintek_wdt_remove(struct platform_device *pdev)
> +{
> +	if (watchdog_is_running()) {
> +		pr_warn("Watchdog timer still running, stopping it\n");
> +		watchdog_stop();
> +	}
> +	misc_deregister(&watchdog_miscdev);
> +	unregister_reboot_notifier(&watchdog_notifier);
> +	return 0;
> +}
> +
> +static struct platform_driver fintek_wdt_driver = {
> +	.probe		= fintek_wdt_probe,
> +	.remove		= fintek_wdt_remove,
> +	.id_table	= fintek_wdt_id_table,
> +	.driver		= {
> +		.name	= DRVNAME,
> +	},
> +};
> +
> +static struct platform_device *fintek_wdt_pdev;
> +
> +static __init const char *fintek_wdt_find(int sioaddr)
> +{
> +	const char *name = ERR_PTR(-ENODEV);
> +	const struct platform_device_id *id;
> +	struct fintek_wdt_variant *variant;
>  	u16 devid;
> -	int err = superio_enter(sioaddr);
> +	const u16 *pdevid;
> +	int err;
> +
> +	err = superio_enter(sioaddr);
>  	if (err)
> -		return err;
> +		return ERR_PTR(err);
>  
>  	devid = superio_inw(sioaddr, SIO_REG_MANID);
>  	if (devid != SIO_FINTEK_ID) {
>  		pr_debug("Not a Fintek device\n");
> -		err = -ENODEV;
> -		goto exit;
> +		goto out;
>  	}
>  
>  	devid = force_id ? force_id : superio_inw(sioaddr, SIO_REG_DEVID);
> -	switch (devid) {
> -	case SIO_F71808_ID:
> -		watchdog.type = f71808fg;
> -		break;
> -	case SIO_F71862_ID:
> -		watchdog.type = f71862fg;
> -		break;
> -	case SIO_F71868_ID:
> -		watchdog.type = f71868;
> -		break;
> -	case SIO_F71869_ID:
> -	case SIO_F71869A_ID:
> -		watchdog.type = f71869;
> -		break;
> -	case SIO_F71882_ID:
> -		watchdog.type = f71882fg;
> -		break;
> -	case SIO_F71889_ID:
> -		watchdog.type = f71889fg;
> -		break;
> -	case SIO_F71858_ID:
> -		/* Confirmed (by datasheet) not to have a watchdog. */
> -		err = -ENODEV;
> -		goto exit;
> -	case SIO_F81803_ID:
> -		watchdog.type = f81803;
> -		break;
> -	case SIO_F81865_ID:
> -		watchdog.type = f81865;
> -		break;
> -	case SIO_F81866_ID:
> -		watchdog.type = f81866;
> -		break;
> -	default:
> -		pr_info("Unrecognized Fintek device: %04x\n",
> -			(unsigned int)devid);
> -		err = -ENODEV;
> -		goto exit;
> +	for (id = fintek_wdt_id_table; id->driver_data; id++) {
> +		variant = (void *)id->driver_data;
> +		if (variant->id == devid) {
> +			name = id->name;
> +			pr_info("Found %s chip, revision %d\n",
> +				name, superio_inb(sioaddr, SIO_REG_DEVREV));
> +			goto out;
> +		}
> +	}
> +
> +	for (pdevid = fintek_no_wdt_table; *pdevid; pdevid++) {
> +		if (*pdevid == devid)
> +			goto out;

I'd rather keep the old code here. I don't see a substantial benefit in
its replacement.

>  	}
>  
> -	pr_info("Found %s watchdog chip, revision %d\n",
> -		f71808e_names[watchdog.type],
> -		(int)superio_inb(sioaddr, SIO_REG_DEVREV));
> -exit:
> +	pr_info("Unrecognized Fintek device: %04x\n", devid);
> +out:
>  	superio_exit(sioaddr);
> -	return err;
> +	return name;
>  }
>  
> -static int __init f71808e_init(void)
> +static struct resource wdt_res = {
> +	.name = "superio port",
> +	.flags = IORESOURCE_IO,
> +};
> +
> +static int __init fintek_wdt_init(void)
>  {
> -	static const unsigned short addrs[] = { 0x2e, 0x4e };
> -	int err = -ENODEV;
> +	unsigned short addrs[] = { 0x2e, 0x4e };

Why drop the static here ? It won't change.

> +	const char *wdt_name;
>  	int i;
>  
>  	if (f71862fg_pin != 63 && f71862fg_pin != 56) {
> @@ -846,30 +896,33 @@ static int __init f71808e_init(void)
>  		return -EINVAL;
>  	}
>  
> +	platform_driver_register(&fintek_wdt_driver);
> +

The driver should only be registered if a watchdog device is found.

>  	for (i = 0; i < ARRAY_SIZE(addrs); i++) {
> -		err = f71808e_find(addrs[i]);
> -		if (err == 0)
> +		wdt_name = fintek_wdt_find(addrs[i]);
> +		if (!IS_ERR(wdt_name))
>  			break;
>  	}
> +
>  	if (i == ARRAY_SIZE(addrs))
> -		return err;
> +		return PTR_ERR(wdt_name);
> +
> +	wdt_res.start = addrs[i];
> +	wdt_res.end = wdt_res.start + 1;
>  
> -	return watchdog_init(addrs[i]);
> +	fintek_wdt_pdev = platform_device_register_simple(wdt_name, -1, &wdt_res, 1);
> +	return PTR_ERR_OR_ZERO(fintek_wdt_pdev);
>  }
>  
> -static void __exit f71808e_exit(void)
> +static void __exit fintek_wdt_exit(void)
>  {
> -	if (watchdog_is_running()) {
> -		pr_warn("Watchdog timer still running, stopping it\n");
> -		watchdog_stop();
> -	}
> -	misc_deregister(&watchdog_miscdev);
> -	unregister_reboot_notifier(&watchdog_notifier);
> +	platform_device_unregister(fintek_wdt_pdev);
> +	platform_driver_unregister(&fintek_wdt_driver);
>  }
>  
>  MODULE_DESCRIPTION("F71808E Watchdog Driver");
>  MODULE_AUTHOR("Giel van Schijndel <me@mortis.eu>");
>  MODULE_LICENSE("GPL");
>  
> -module_init(f71808e_init);
> -module_exit(f71808e_exit);
> +module_init(fintek_wdt_init);
> +module_exit(fintek_wdt_exit);
Ahmad Fatoum Nov. 17, 2020, 11:01 a.m. UTC | #2
Hello Guenter,

Thanks for the review. Comments inline.

On 11/8/20 6:26 PM, Guenter Roeck wrote:
> On Tue, Oct 20, 2020 at 08:21:11AM +0200, Ahmad Fatoum wrote:
>> Driver so far wasn't ported to the driver model and set up its
>> miscdevice out of the init after probing the I/O ports for a watchdog
>> with correct vendor and device revision.
>>
>> Keep the device detection part at init time, but move watchdog setup
>> to a platform driver probe function.
>>
>> While at it, refactor some of the driver code we have to now touch
>> anyway:
>>
>>  - platform_device_id is used instead of the two big switches mapping
>>    hardware ID to an enum and then mapping it to a pinconf function
>>  - we rename f71808e_ and watchdog_data to fintek_wdt, to avoid mix up
>>    of the generic parts up with the device specific parts
>>
>> Suggested-by: Guenter Roeck <linux@roeck-us.net>
>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> 
> Sorry, I just found this. Sorry, it got lost.
> The changes you are making here go way beyond a conversion to a platform
> driver. I am not necessarily opposed to the changes, but they should be
> split into multiple patches to simlify (and to a large degree enable)
> a meaningful review. More comments inline.

Most code shuffling around was due to the use of platform_device_id.
I can look into splitting this up though.

> 
>> ---
>>  drivers/watchdog/f71808e_wdt.c | 377 +++++++++++++++++++--------------
>>  1 file changed, 215 insertions(+), 162 deletions(-)
>>
>> diff --git a/drivers/watchdog/f71808e_wdt.c b/drivers/watchdog/f71808e_wdt.c
>> index f60beec1bbae..4ff7a2509125 100644
>> --- a/drivers/watchdog/f71808e_wdt.c
>> +++ b/drivers/watchdog/f71808e_wdt.c
>> @@ -9,12 +9,15 @@
>>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>>  
>>  #include <linux/err.h>
>> +#include <linux/slab.h>
>>  #include <linux/fs.h>
>>  #include <linux/init.h>
>>  #include <linux/io.h>
>>  #include <linux/ioport.h>
>>  #include <linux/miscdevice.h>
>>  #include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/mod_devicetable.h>
>>  #include <linux/mutex.h>
>>  #include <linux/notifier.h>
>>  #include <linux/reboot.h>
>> @@ -110,22 +113,6 @@ module_param(start_withtimeout, uint, 0);
>>  MODULE_PARM_DESC(start_withtimeout, "Start watchdog timer on module load with"
>>  	" given initial timeout. Zero (default) disables this feature.");
>>  
>> -enum chips { f71808fg, f71858fg, f71862fg, f71868, f71869, f71882fg, f71889fg,
>> -	     f81803, f81865, f81866};
>> -
>> -static const char *f71808e_names[] = {
>> -	"f71808fg",
>> -	"f71858fg",
>> -	"f71862fg",
>> -	"f71868",
>> -	"f71869",
>> -	"f71882fg",
>> -	"f71889fg",
>> -	"f81803",
>> -	"f81865",
>> -	"f81866",
>> -};
>> -
>>  /* Super-I/O Function prototypes */
>>  static inline int superio_inb(int base, int reg);
>>  static inline int superio_inw(int base, int reg);
>> @@ -136,9 +123,17 @@ static inline int superio_enter(int base);
>>  static inline void superio_select(int base, int ld);
>>  static inline void superio_exit(int base);
>>  
>> -struct watchdog_data {
>> +struct fintek_wdt;
>> +
>> +struct fintek_wdt_variant {
>> +	u16 id;
>> +	void (*pinconf)(struct fintek_wdt *wd);
>> +	const char *identity_override;
>> +};
>> +
>> +struct fintek_wdt {
>>  	unsigned short	sioaddr;
>> -	enum chips	type;
>> +	const struct fintek_wdt_variant *variant;
> 
> I'd rather have this all in here. Just add 
> 	void (*pinconf)(struct fintek_wdt *wd);
> and assign it in the probe function, or call it
> from an array indexed with type.
> 
> This change is also independent of the conversion to a platform
> driver and should be done in a separate patch.

Ok.

>>  	unsigned long	opened;
>>  	struct mutex	lock;
>>  	char		expect_close;
>> @@ -152,10 +147,15 @@ struct watchdog_data {
>>  	char		caused_reboot;	/* last reboot was by the watchdog */
>>  };
>>  
>> -static struct watchdog_data watchdog = {
>> +static struct fintek_wdt watchdog = {
>>  	.lock = __MUTEX_INITIALIZER(watchdog.lock),
>>  };
>>  
>> +static inline bool has_f81865_wdo_conf(struct fintek_wdt *wd)
>> +{
>> +	return wd->variant->id == SIO_F81865_ID || wd->variant->id == SIO_F81866_ID;
>> +}
>> +
> This change is independent of the conversion to a platform driver
> and should be a separate patch.

Ok.

>>  /* Super I/O functions */
>>  static inline int superio_inb(int base, int reg)
>>  {
>> @@ -247,7 +247,7 @@ static int watchdog_set_pulse_width(unsigned int pw)
>>  	int err = 0;
>>  	unsigned int t1 = 25, t2 = 125, t3 = 5000;
>>  
>> -	if (watchdog.type == f71868) {
>> +	if (watchdog.variant->id == SIO_F71868_ID) {
> 
> I am not sure if dropping watchdog.type and replacing it with SIO_F71868_ID
> is a good idea. In the current code, there is no 1:1 match of those,
> specifically for SIO_F71869_ID/SIO_F71869A_ID. Deviating from that only
> complicates the code.

We can have a 1:1 match if we drop some quirks. See below.

> 
>>  		t1 = 30;
>>  		t2 = 150;
>>  		t3 = 6000;
>> @@ -309,7 +309,6 @@ static int watchdog_keepalive(void)
>>  static int watchdog_start(void)
>>  {
>>  	int err;
>> -	u8 tmp;
>>  
>>  	/* Make sure we don't die as soon as the watchdog is enabled below */
>>  	err = watchdog_keepalive();
>> @@ -323,81 +322,12 @@ static int watchdog_start(void)
>>  	superio_select(watchdog.sioaddr, SIO_F71808FG_LD_WDT);
>>  
>>  	/* Watchdog pin configuration */
>> -	switch (watchdog.type) {
>> -	case f71808fg:
>> -		/* Set pin 21 to GPIO23/WDTRST#, then to WDTRST# */
>> -		superio_clear_bit(watchdog.sioaddr, SIO_REG_MFUNCT2, 3);
>> -		superio_clear_bit(watchdog.sioaddr, SIO_REG_MFUNCT3, 3);
>> -		break;
>> -
>> -	case f71862fg:
>> -		if (f71862fg_pin == 63) {
>> -			/* SPI must be disabled first to use this pin! */
>> -			superio_clear_bit(watchdog.sioaddr, SIO_REG_ROM_ADDR_SEL, 6);
>> -			superio_set_bit(watchdog.sioaddr, SIO_REG_MFUNCT3, 4);
>> -		} else if (f71862fg_pin == 56) {
>> -			superio_set_bit(watchdog.sioaddr, SIO_REG_MFUNCT1, 1);
>> -		}
>> -		break;
>> -
>> -	case f71868:
>> -	case f71869:
>> -		/* GPIO14 --> WDTRST# */
>> -		superio_clear_bit(watchdog.sioaddr, SIO_REG_MFUNCT1, 4);
>> -		break;
>> -
>> -	case f71882fg:
>> -		/* Set pin 56 to WDTRST# */
>> -		superio_set_bit(watchdog.sioaddr, SIO_REG_MFUNCT1, 1);
>> -		break;
>> -
>> -	case f71889fg:
>> -		/* set pin 40 to WDTRST# */
>> -		superio_outb(watchdog.sioaddr, SIO_REG_MFUNCT3,
>> -			superio_inb(watchdog.sioaddr, SIO_REG_MFUNCT3) & 0xcf);
>> -		break;
>> -
>> -	case f81803:
>> -		/* Enable TSI Level register bank */
>> -		superio_clear_bit(watchdog.sioaddr, SIO_REG_CLOCK_SEL, 3);
>> -		/* Set pin 27 to WDTRST# */
>> -		superio_outb(watchdog.sioaddr, SIO_REG_TSI_LEVEL_SEL, 0x5f &
>> -			superio_inb(watchdog.sioaddr, SIO_REG_TSI_LEVEL_SEL));
>> -		break;
>> -
>> -	case f81865:
>> -		/* Set pin 70 to WDTRST# */
>> -		superio_clear_bit(watchdog.sioaddr, SIO_REG_MFUNCT3, 5);
>> -		break;
>> -
>> -	case f81866:
>> -		/*
>> -		 * GPIO1 Control Register when 27h BIT3:2 = 01 & BIT0 = 0.
>> -		 * The PIN 70(GPIO15/WDTRST) is controlled by 2Ch:
>> -		 *     BIT5: 0 -> WDTRST#
>> -		 *           1 -> GPIO15
>> -		 */
>> -		tmp = superio_inb(watchdog.sioaddr, SIO_F81866_REG_PORT_SEL);
>> -		tmp &= ~(BIT(3) | BIT(0));
>> -		tmp |= BIT(2);
>> -		superio_outb(watchdog.sioaddr, SIO_F81866_REG_PORT_SEL, tmp);
>> -
>> -		superio_clear_bit(watchdog.sioaddr, SIO_F81866_REG_GPIO1, 5);
>> -		break;
>> -
>> -	default:
>> -		/*
>> -		 * 'default' label to shut up the compiler and catch
>> -		 * programmer errors
>> -		 */
>> -		err = -ENODEV;
>> -		goto exit_superio;
>> -	}
>> +	watchdog.variant->pinconf(&watchdog);
>>  
>>  	superio_select(watchdog.sioaddr, SIO_F71808FG_LD_WDT);
>>  	superio_set_bit(watchdog.sioaddr, SIO_REG_ENABLE, 0);
>>  
>> -	if (watchdog.type == f81865 || watchdog.type == f81866)
>> +	if (has_f81865_wdo_conf(&watchdog))
>>  		superio_set_bit(watchdog.sioaddr, F81865_REG_WDO_CONF,
>>  				F81865_FLAG_WDOUT_EN);
>>  	else
>> @@ -425,7 +355,6 @@ static int watchdog_start(void)
>>  				F71808FG_FLAG_WD_PULSE);
>>  	}
>>  
>> -exit_superio:
>>  	superio_exit(watchdog.sioaddr);
>>  exit_unlock:
>>  	mutex_unlock(&watchdog.lock);
>> @@ -665,21 +594,39 @@ static struct notifier_block watchdog_notifier = {
>>  	.notifier_call = watchdog_notify_sys,
>>  };
>>  
>> -static int __init watchdog_init(int sioaddr)
>> +static int fintek_wdt_probe(struct platform_device *pdev)
>>  {
>> +	const struct platform_device_id *id;
>> +	unsigned short sioaddr;
>> +	struct resource *res;
>>  	int wdt_conf, err = 0;
>> +	const char *identity;
>> +
>> +	if (watchdog_miscdev.parent)
>> +		return -EBUSY;
>> +
>> +	watchdog_miscdev.parent = &pdev->dev;
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_IO, 0);
>> +	if (!res)
>> +		return -ENXIO;
>> +
>> +	id = platform_get_device_id(pdev);
>> +	watchdog.variant = (void *)id->driver_data;
>> +	if (!watchdog.variant)
>> +		return -ENODEV;
>>  
>>  	/* No need to lock watchdog.lock here because no entry points
>>  	 * into the module have been registered yet.
>>  	 */
>> -	watchdog.sioaddr = sioaddr;
>> +	watchdog.sioaddr = res->start;
>>  	watchdog.ident.options = WDIOF_MAGICCLOSE
>>  				| WDIOF_KEEPALIVEPING
>>  				| WDIOF_CARDRESET;
>>  
>> -	snprintf(watchdog.ident.identity,
>> -		sizeof(watchdog.ident.identity), "%s watchdog",
>> -		f71808e_names[watchdog.type]);
>> +	identity = watchdog.variant->identity_override ?: id->name;
>> +
>> +	memcpy(watchdog.ident.identity, identity, sizeof(watchdog.ident.identity));
>>  
>>  	err = superio_enter(sioaddr);
>>  	if (err)
>> @@ -772,73 +719,176 @@ static int __init watchdog_init(int sioaddr)
>>  	return err;
>>  }
>>  
>> -static int __init f71808e_find(int sioaddr)
>> +static void f71808fg_pinconf(struct fintek_wdt *wd)
>>  {
>> +	/* Set pin 21 to GPIO23/WDTRST#, then to WDTRST# */
>> +	superio_clear_bit(wd->sioaddr, SIO_REG_MFUNCT2, 3);
>> +	superio_clear_bit(wd->sioaddr, SIO_REG_MFUNCT3, 3);
>> +}
>> +
>> +static void f71862fg_pinconf(struct fintek_wdt *wd)
>> +{
>> +	if (f71862fg_pin == 63) {
>> +		/* SPI must be disabled first to use this pin! */
>> +		superio_clear_bit(wd->sioaddr, SIO_REG_ROM_ADDR_SEL, 6);
>> +		superio_set_bit(wd->sioaddr, SIO_REG_MFUNCT3, 4);
>> +	} else if (f71862fg_pin == 56) {
>> +		superio_set_bit(wd->sioaddr, SIO_REG_MFUNCT1, 1);
>> +	}
>> +}
>> +
>> +static void f71868_pinconf(struct fintek_wdt *wd)
>> +{
>> +	/* GPIO14 --> WDTRST# */
>> +	superio_clear_bit(wd->sioaddr, SIO_REG_MFUNCT1, 4);
>> +}
>> +
>> +static void f71882fg_pinconf(struct fintek_wdt *wd)
>> +{
>> +	/* Set pin 56 to WDTRST# */
>> +	superio_set_bit(wd->sioaddr, SIO_REG_MFUNCT1, 1);
>> +}
>> +
>> +static void f71889fg_pinconf(struct fintek_wdt *wd)
>> +{
>> +	/* set pin 40 to WDTRST# */
>> +	superio_outb(wd->sioaddr, SIO_REG_MFUNCT3,
>> +		     superio_inb(wd->sioaddr, SIO_REG_MFUNCT3) & 0xcf);
>> +}
>> +
>> +static void f81803_pinconf(struct fintek_wdt *wd)
>> +{
>> +	/* Enable TSI Level register bank */
>> +	superio_clear_bit(wd->sioaddr, SIO_REG_CLOCK_SEL, 3);
>> +	/* Set pin 27 to WDTRST# */
>> +	superio_outb(wd->sioaddr, SIO_REG_TSI_LEVEL_SEL, 0x5f &
>> +		     superio_inb(wd->sioaddr, SIO_REG_TSI_LEVEL_SEL));
>> +}
>> +
>> +static void f81865_pinconf(struct fintek_wdt *wd)
>> +{
>> +	/* Set pin 70 to WDTRST# */
>> +	superio_clear_bit(wd->sioaddr, SIO_REG_MFUNCT3, 5);
>> +}
>> +
>> +static void f81866_pinconf(struct fintek_wdt *wd)
>> +{
>> +	u8 tmp;
>> +	/*
>> +	 * GPIO1 Control Register when 27h BIT3:2 = 01 & BIT0 = 0.
>> +	 * The PIN 70(GPIO15/WDTRST) is controlled by 2Ch:
>> +	 *     BIT5: 0 -> WDTRST#
>> +	 *           1 -> GPIO15
>> +	 */
>> +	tmp = superio_inb(wd->sioaddr, SIO_F81866_REG_PORT_SEL);
>> +	tmp &= ~(BIT(3) | BIT(0));
>> +	tmp |= BIT(2);
>> +	superio_outb(wd->sioaddr, SIO_F81866_REG_PORT_SEL, tmp);
>> +
>> +	superio_clear_bit(wd->sioaddr, SIO_F81866_REG_GPIO1, 5);
>> +}
>> +
>> +#define FINTEK_DRV_DATA(...) \
>> +	(kernel_ulong_t)(&(struct fintek_wdt_variant) { __VA_ARGS__ })
>> +
>> +/*
>> + * Array is used by both platform bus to map name to driver data and by
>> + * init below to map id in driver_data to name for device creation.
>> + */
>> +static const struct platform_device_id fintek_wdt_id_table[] = {
>> +	{ "f71808fg watchdog", FINTEK_DRV_DATA(SIO_F71808_ID,  f71808fg_pinconf) },
>> +	{ "f71862fg watchdog", FINTEK_DRV_DATA(SIO_F71862_ID,  f71862fg_pinconf) },
>> +	{ "f71868 watchdog",   FINTEK_DRV_DATA(SIO_F71868_ID,  f71868_pinconf) },
>> +	{ "f71869 watchdog",   FINTEK_DRV_DATA(SIO_F71869_ID,  f71868_pinconf) },
>> +	/* For backwards-compatibility, we define a different watchdog identity name */
>> +	{ "f71869a watchdog",  FINTEK_DRV_DATA(SIO_F71869A_ID, f71868_pinconf,
>> +					       .identity_override = "f71869 watchdog") },
> 
> I don't think .identity_override adds real value.

Userspace that relied on the identity name for whatever would be broken
otherwise. Are you ok with this? (And if yes, could I just rename
all of them to drop the space?)

> 
>> +	{ "f71882fg watchdog", FINTEK_DRV_DATA(SIO_F71882_ID,  f71882fg_pinconf) },
>> +	{ "f71889fg watchdog", FINTEK_DRV_DATA(SIO_F71889_ID,  f71889fg_pinconf) },
>> +	{ "f81803 watchdog",   FINTEK_DRV_DATA(SIO_F81803_ID,  f81803_pinconf) },
>> +	{ "f81865 watchdog",   FINTEK_DRV_DATA(SIO_F81865_ID,  f81865_pinconf) },
>> +	{ "f81866 watchdog",   FINTEK_DRV_DATA(SIO_F81866_ID,  f81866_pinconf) },
> 
> I am not entirely sure I understand exactly what the macro does, and checkpatch
> doesn't like it either.

I had to rebase onto v5.10-rc3 to get checkpatch to complain about
"ERROR: Macros with complex values should be enclosed in parentheses"

> Anyway, I don't think this complexity is necessary. Other similar drivers add
> device information as platform driver data (using platform_device_add_data)
> and get it using dev_get_platdata(). That would be much simpler and more
> straightforward.

Passing function pointers via platdata would be unusual, no?

>> +	{ /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(platform, fintek_wdt_id_table);
>> +
>> +/* Confirmed (by datasheet) not to have a watchdog.
>> + * We list this here anyway to maintain backwards-compatibility in not warning about them
>> + */
>> +static const u16 fintek_no_wdt_table[] = { SIO_F71858_ID, 0 };
>> +
>> +static int fintek_wdt_remove(struct platform_device *pdev)
>> +{
>> +	if (watchdog_is_running()) {
>> +		pr_warn("Watchdog timer still running, stopping it\n");
>> +		watchdog_stop();
>> +	}
>> +	misc_deregister(&watchdog_miscdev);
>> +	unregister_reboot_notifier(&watchdog_notifier);
>> +	return 0;
>> +}
>> +
>> +static struct platform_driver fintek_wdt_driver = {
>> +	.probe		= fintek_wdt_probe,
>> +	.remove		= fintek_wdt_remove,
>> +	.id_table	= fintek_wdt_id_table,
>> +	.driver		= {
>> +		.name	= DRVNAME,
>> +	},
>> +};
>> +
>> +static struct platform_device *fintek_wdt_pdev;
>> +
>> +static __init const char *fintek_wdt_find(int sioaddr)
>> +{
>> +	const char *name = ERR_PTR(-ENODEV);
>> +	const struct platform_device_id *id;
>> +	struct fintek_wdt_variant *variant;
>>  	u16 devid;
>> -	int err = superio_enter(sioaddr);
>> +	const u16 *pdevid;
>> +	int err;
>> +
>> +	err = superio_enter(sioaddr);
>>  	if (err)
>> -		return err;
>> +		return ERR_PTR(err);
>>  
>>  	devid = superio_inw(sioaddr, SIO_REG_MANID);
>>  	if (devid != SIO_FINTEK_ID) {
>>  		pr_debug("Not a Fintek device\n");
>> -		err = -ENODEV;
>> -		goto exit;
>> +		goto out;
>>  	}
>>  
>>  	devid = force_id ? force_id : superio_inw(sioaddr, SIO_REG_DEVID);
>> -	switch (devid) {
>> -	case SIO_F71808_ID:
>> -		watchdog.type = f71808fg;
>> -		break;
>> -	case SIO_F71862_ID:
>> -		watchdog.type = f71862fg;
>> -		break;
>> -	case SIO_F71868_ID:
>> -		watchdog.type = f71868;
>> -		break;
>> -	case SIO_F71869_ID:
>> -	case SIO_F71869A_ID:
>> -		watchdog.type = f71869;
>> -		break;
>> -	case SIO_F71882_ID:
>> -		watchdog.type = f71882fg;
>> -		break;
>> -	case SIO_F71889_ID:
>> -		watchdog.type = f71889fg;
>> -		break;
>> -	case SIO_F71858_ID:
>> -		/* Confirmed (by datasheet) not to have a watchdog. */
>> -		err = -ENODEV;
>> -		goto exit;
>> -	case SIO_F81803_ID:
>> -		watchdog.type = f81803;
>> -		break;
>> -	case SIO_F81865_ID:
>> -		watchdog.type = f81865;
>> -		break;
>> -	case SIO_F81866_ID:
>> -		watchdog.type = f81866;
>> -		break;
>> -	default:
>> -		pr_info("Unrecognized Fintek device: %04x\n",
>> -			(unsigned int)devid);
>> -		err = -ENODEV;
>> -		goto exit;
>> +	for (id = fintek_wdt_id_table; id->driver_data; id++) {
>> +		variant = (void *)id->driver_data;
>> +		if (variant->id == devid) {
>> +			name = id->name;
>> +			pr_info("Found %s chip, revision %d\n",
>> +				name, superio_inb(sioaddr, SIO_REG_DEVREV));
>> +			goto out;
>> +		}
>> +	}
>> +
>> +	for (pdevid = fintek_no_wdt_table; *pdevid; pdevid++) {
>> +		if (*pdevid == devid)
>> +			goto out;
> 
> I'd rather keep the old code here. I don't see a substantial benefit in
> its replacement.

I didn't see any benefit in the existance of such a big switch, when
the id_table has the same function.

>>  	}
>>  
>> -	pr_info("Found %s watchdog chip, revision %d\n",
>> -		f71808e_names[watchdog.type],
>> -		(int)superio_inb(sioaddr, SIO_REG_DEVREV));
>> -exit:
>> +	pr_info("Unrecognized Fintek device: %04x\n", devid);
>> +out:
>>  	superio_exit(sioaddr);
>> -	return err;
>> +	return name;
>>  }
>>  
>> -static int __init f71808e_init(void)
>> +static struct resource wdt_res = {
>> +	.name = "superio port",
>> +	.flags = IORESOURCE_IO,
>> +};
>> +
>> +static int __init fintek_wdt_init(void)
>>  {
>> -	static const unsigned short addrs[] = { 0x2e, 0x4e };
>> -	int err = -ENODEV;
>> +	unsigned short addrs[] = { 0x2e, 0x4e };
> 
> Why drop the static here ? It won't change.

I'd have expected the compiler to generate exactly the same
code in this case, so the static const would be unnecessary.
I'll reinstate the static const in v3.

>> +	const char *wdt_name;
>>  	int i;
>>  
>>  	if (f71862fg_pin != 63 && f71862fg_pin != 56) {
>> @@ -846,30 +896,33 @@ static int __init f71808e_init(void)
>>  		return -EINVAL;
>>  	}
>>  
>> +	platform_driver_register(&fintek_wdt_driver);
>> +
> 
> The driver should only be registered if a watchdog device is found.

Right, I am missing a platform_driver_unregister. Will fix.
 
>>  	for (i = 0; i < ARRAY_SIZE(addrs); i++) {
>> -		err = f71808e_find(addrs[i]);
>> -		if (err == 0)
>> +		wdt_name = fintek_wdt_find(addrs[i]);
>> +		if (!IS_ERR(wdt_name))
>>  			break;
>>  	}
>> +
>>  	if (i == ARRAY_SIZE(addrs))
>> -		return err;
>> +		return PTR_ERR(wdt_name);
>> +
>> +	wdt_res.start = addrs[i];
>> +	wdt_res.end = wdt_res.start + 1;
>>  
>> -	return watchdog_init(addrs[i]);
>> +	fintek_wdt_pdev = platform_device_register_simple(wdt_name, -1, &wdt_res, 1);
>> +	return PTR_ERR_OR_ZERO(fintek_wdt_pdev);
>>  }
>>  
>> -static void __exit f71808e_exit(void)
>> +static void __exit fintek_wdt_exit(void)
>>  {
>> -	if (watchdog_is_running()) {
>> -		pr_warn("Watchdog timer still running, stopping it\n");
>> -		watchdog_stop();
>> -	}
>> -	misc_deregister(&watchdog_miscdev);
>> -	unregister_reboot_notifier(&watchdog_notifier);
>> +	platform_device_unregister(fintek_wdt_pdev);
>> +	platform_driver_unregister(&fintek_wdt_driver);
>>  }
>>  
>>  MODULE_DESCRIPTION("F71808E Watchdog Driver");
>>  MODULE_AUTHOR("Giel van Schijndel <me@mortis.eu>");
>>  MODULE_LICENSE("GPL");
>>  
>> -module_init(f71808e_init);
>> -module_exit(f71808e_exit);
>> +module_init(fintek_wdt_init);
>> +module_exit(fintek_wdt_exit);
>

Patch
diff mbox series

diff --git a/drivers/watchdog/f71808e_wdt.c b/drivers/watchdog/f71808e_wdt.c
index f60beec1bbae..4ff7a2509125 100644
--- a/drivers/watchdog/f71808e_wdt.c
+++ b/drivers/watchdog/f71808e_wdt.c
@@ -9,12 +9,15 @@ 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
 #include <linux/err.h>
+#include <linux/slab.h>
 #include <linux/fs.h>
 #include <linux/init.h>
 #include <linux/io.h>
 #include <linux/ioport.h>
 #include <linux/miscdevice.h>
 #include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/mod_devicetable.h>
 #include <linux/mutex.h>
 #include <linux/notifier.h>
 #include <linux/reboot.h>
@@ -110,22 +113,6 @@  module_param(start_withtimeout, uint, 0);
 MODULE_PARM_DESC(start_withtimeout, "Start watchdog timer on module load with"
 	" given initial timeout. Zero (default) disables this feature.");
 
-enum chips { f71808fg, f71858fg, f71862fg, f71868, f71869, f71882fg, f71889fg,
-	     f81803, f81865, f81866};
-
-static const char *f71808e_names[] = {
-	"f71808fg",
-	"f71858fg",
-	"f71862fg",
-	"f71868",
-	"f71869",
-	"f71882fg",
-	"f71889fg",
-	"f81803",
-	"f81865",
-	"f81866",
-};
-
 /* Super-I/O Function prototypes */
 static inline int superio_inb(int base, int reg);
 static inline int superio_inw(int base, int reg);
@@ -136,9 +123,17 @@  static inline int superio_enter(int base);
 static inline void superio_select(int base, int ld);
 static inline void superio_exit(int base);
 
-struct watchdog_data {
+struct fintek_wdt;
+
+struct fintek_wdt_variant {
+	u16 id;
+	void (*pinconf)(struct fintek_wdt *wd);
+	const char *identity_override;
+};
+
+struct fintek_wdt {
 	unsigned short	sioaddr;
-	enum chips	type;
+	const struct fintek_wdt_variant *variant;
 	unsigned long	opened;
 	struct mutex	lock;
 	char		expect_close;
@@ -152,10 +147,15 @@  struct watchdog_data {
 	char		caused_reboot;	/* last reboot was by the watchdog */
 };
 
-static struct watchdog_data watchdog = {
+static struct fintek_wdt watchdog = {
 	.lock = __MUTEX_INITIALIZER(watchdog.lock),
 };
 
+static inline bool has_f81865_wdo_conf(struct fintek_wdt *wd)
+{
+	return wd->variant->id == SIO_F81865_ID || wd->variant->id == SIO_F81866_ID;
+}
+
 /* Super I/O functions */
 static inline int superio_inb(int base, int reg)
 {
@@ -247,7 +247,7 @@  static int watchdog_set_pulse_width(unsigned int pw)
 	int err = 0;
 	unsigned int t1 = 25, t2 = 125, t3 = 5000;
 
-	if (watchdog.type == f71868) {
+	if (watchdog.variant->id == SIO_F71868_ID) {
 		t1 = 30;
 		t2 = 150;
 		t3 = 6000;
@@ -309,7 +309,6 @@  static int watchdog_keepalive(void)
 static int watchdog_start(void)
 {
 	int err;
-	u8 tmp;
 
 	/* Make sure we don't die as soon as the watchdog is enabled below */
 	err = watchdog_keepalive();
@@ -323,81 +322,12 @@  static int watchdog_start(void)
 	superio_select(watchdog.sioaddr, SIO_F71808FG_LD_WDT);
 
 	/* Watchdog pin configuration */
-	switch (watchdog.type) {
-	case f71808fg:
-		/* Set pin 21 to GPIO23/WDTRST#, then to WDTRST# */
-		superio_clear_bit(watchdog.sioaddr, SIO_REG_MFUNCT2, 3);
-		superio_clear_bit(watchdog.sioaddr, SIO_REG_MFUNCT3, 3);
-		break;
-
-	case f71862fg:
-		if (f71862fg_pin == 63) {
-			/* SPI must be disabled first to use this pin! */
-			superio_clear_bit(watchdog.sioaddr, SIO_REG_ROM_ADDR_SEL, 6);
-			superio_set_bit(watchdog.sioaddr, SIO_REG_MFUNCT3, 4);
-		} else if (f71862fg_pin == 56) {
-			superio_set_bit(watchdog.sioaddr, SIO_REG_MFUNCT1, 1);
-		}
-		break;
-
-	case f71868:
-	case f71869:
-		/* GPIO14 --> WDTRST# */
-		superio_clear_bit(watchdog.sioaddr, SIO_REG_MFUNCT1, 4);
-		break;
-
-	case f71882fg:
-		/* Set pin 56 to WDTRST# */
-		superio_set_bit(watchdog.sioaddr, SIO_REG_MFUNCT1, 1);
-		break;
-
-	case f71889fg:
-		/* set pin 40 to WDTRST# */
-		superio_outb(watchdog.sioaddr, SIO_REG_MFUNCT3,
-			superio_inb(watchdog.sioaddr, SIO_REG_MFUNCT3) & 0xcf);
-		break;
-
-	case f81803:
-		/* Enable TSI Level register bank */
-		superio_clear_bit(watchdog.sioaddr, SIO_REG_CLOCK_SEL, 3);
-		/* Set pin 27 to WDTRST# */
-		superio_outb(watchdog.sioaddr, SIO_REG_TSI_LEVEL_SEL, 0x5f &
-			superio_inb(watchdog.sioaddr, SIO_REG_TSI_LEVEL_SEL));
-		break;
-
-	case f81865:
-		/* Set pin 70 to WDTRST# */
-		superio_clear_bit(watchdog.sioaddr, SIO_REG_MFUNCT3, 5);
-		break;
-
-	case f81866:
-		/*
-		 * GPIO1 Control Register when 27h BIT3:2 = 01 & BIT0 = 0.
-		 * The PIN 70(GPIO15/WDTRST) is controlled by 2Ch:
-		 *     BIT5: 0 -> WDTRST#
-		 *           1 -> GPIO15
-		 */
-		tmp = superio_inb(watchdog.sioaddr, SIO_F81866_REG_PORT_SEL);
-		tmp &= ~(BIT(3) | BIT(0));
-		tmp |= BIT(2);
-		superio_outb(watchdog.sioaddr, SIO_F81866_REG_PORT_SEL, tmp);
-
-		superio_clear_bit(watchdog.sioaddr, SIO_F81866_REG_GPIO1, 5);
-		break;
-
-	default:
-		/*
-		 * 'default' label to shut up the compiler and catch
-		 * programmer errors
-		 */
-		err = -ENODEV;
-		goto exit_superio;
-	}
+	watchdog.variant->pinconf(&watchdog);
 
 	superio_select(watchdog.sioaddr, SIO_F71808FG_LD_WDT);
 	superio_set_bit(watchdog.sioaddr, SIO_REG_ENABLE, 0);
 
-	if (watchdog.type == f81865 || watchdog.type == f81866)
+	if (has_f81865_wdo_conf(&watchdog))
 		superio_set_bit(watchdog.sioaddr, F81865_REG_WDO_CONF,
 				F81865_FLAG_WDOUT_EN);
 	else
@@ -425,7 +355,6 @@  static int watchdog_start(void)
 				F71808FG_FLAG_WD_PULSE);
 	}
 
-exit_superio:
 	superio_exit(watchdog.sioaddr);
 exit_unlock:
 	mutex_unlock(&watchdog.lock);
@@ -665,21 +594,39 @@  static struct notifier_block watchdog_notifier = {
 	.notifier_call = watchdog_notify_sys,
 };
 
-static int __init watchdog_init(int sioaddr)
+static int fintek_wdt_probe(struct platform_device *pdev)
 {
+	const struct platform_device_id *id;
+	unsigned short sioaddr;
+	struct resource *res;
 	int wdt_conf, err = 0;
+	const char *identity;
+
+	if (watchdog_miscdev.parent)
+		return -EBUSY;
+
+	watchdog_miscdev.parent = &pdev->dev;
+
+	res = platform_get_resource(pdev, IORESOURCE_IO, 0);
+	if (!res)
+		return -ENXIO;
+
+	id = platform_get_device_id(pdev);
+	watchdog.variant = (void *)id->driver_data;
+	if (!watchdog.variant)
+		return -ENODEV;
 
 	/* No need to lock watchdog.lock here because no entry points
 	 * into the module have been registered yet.
 	 */
-	watchdog.sioaddr = sioaddr;
+	watchdog.sioaddr = res->start;
 	watchdog.ident.options = WDIOF_MAGICCLOSE
 				| WDIOF_KEEPALIVEPING
 				| WDIOF_CARDRESET;
 
-	snprintf(watchdog.ident.identity,
-		sizeof(watchdog.ident.identity), "%s watchdog",
-		f71808e_names[watchdog.type]);
+	identity = watchdog.variant->identity_override ?: id->name;
+
+	memcpy(watchdog.ident.identity, identity, sizeof(watchdog.ident.identity));
 
 	err = superio_enter(sioaddr);
 	if (err)
@@ -772,73 +719,176 @@  static int __init watchdog_init(int sioaddr)
 	return err;
 }
 
-static int __init f71808e_find(int sioaddr)
+static void f71808fg_pinconf(struct fintek_wdt *wd)
 {
+	/* Set pin 21 to GPIO23/WDTRST#, then to WDTRST# */
+	superio_clear_bit(wd->sioaddr, SIO_REG_MFUNCT2, 3);
+	superio_clear_bit(wd->sioaddr, SIO_REG_MFUNCT3, 3);
+}
+
+static void f71862fg_pinconf(struct fintek_wdt *wd)
+{
+	if (f71862fg_pin == 63) {
+		/* SPI must be disabled first to use this pin! */
+		superio_clear_bit(wd->sioaddr, SIO_REG_ROM_ADDR_SEL, 6);
+		superio_set_bit(wd->sioaddr, SIO_REG_MFUNCT3, 4);
+	} else if (f71862fg_pin == 56) {
+		superio_set_bit(wd->sioaddr, SIO_REG_MFUNCT1, 1);
+	}
+}
+
+static void f71868_pinconf(struct fintek_wdt *wd)
+{
+	/* GPIO14 --> WDTRST# */
+	superio_clear_bit(wd->sioaddr, SIO_REG_MFUNCT1, 4);
+}
+
+static void f71882fg_pinconf(struct fintek_wdt *wd)
+{
+	/* Set pin 56 to WDTRST# */
+	superio_set_bit(wd->sioaddr, SIO_REG_MFUNCT1, 1);
+}
+
+static void f71889fg_pinconf(struct fintek_wdt *wd)
+{
+	/* set pin 40 to WDTRST# */
+	superio_outb(wd->sioaddr, SIO_REG_MFUNCT3,
+		     superio_inb(wd->sioaddr, SIO_REG_MFUNCT3) & 0xcf);
+}
+
+static void f81803_pinconf(struct fintek_wdt *wd)
+{
+	/* Enable TSI Level register bank */
+	superio_clear_bit(wd->sioaddr, SIO_REG_CLOCK_SEL, 3);
+	/* Set pin 27 to WDTRST# */
+	superio_outb(wd->sioaddr, SIO_REG_TSI_LEVEL_SEL, 0x5f &
+		     superio_inb(wd->sioaddr, SIO_REG_TSI_LEVEL_SEL));
+}
+
+static void f81865_pinconf(struct fintek_wdt *wd)
+{
+	/* Set pin 70 to WDTRST# */
+	superio_clear_bit(wd->sioaddr, SIO_REG_MFUNCT3, 5);
+}
+
+static void f81866_pinconf(struct fintek_wdt *wd)
+{
+	u8 tmp;
+	/*
+	 * GPIO1 Control Register when 27h BIT3:2 = 01 & BIT0 = 0.
+	 * The PIN 70(GPIO15/WDTRST) is controlled by 2Ch:
+	 *     BIT5: 0 -> WDTRST#
+	 *           1 -> GPIO15
+	 */
+	tmp = superio_inb(wd->sioaddr, SIO_F81866_REG_PORT_SEL);
+	tmp &= ~(BIT(3) | BIT(0));
+	tmp |= BIT(2);
+	superio_outb(wd->sioaddr, SIO_F81866_REG_PORT_SEL, tmp);
+
+	superio_clear_bit(wd->sioaddr, SIO_F81866_REG_GPIO1, 5);
+}
+
+#define FINTEK_DRV_DATA(...) \
+	(kernel_ulong_t)(&(struct fintek_wdt_variant) { __VA_ARGS__ })
+
+/*
+ * Array is used by both platform bus to map name to driver data and by
+ * init below to map id in driver_data to name for device creation.
+ */
+static const struct platform_device_id fintek_wdt_id_table[] = {
+	{ "f71808fg watchdog", FINTEK_DRV_DATA(SIO_F71808_ID,  f71808fg_pinconf) },
+	{ "f71862fg watchdog", FINTEK_DRV_DATA(SIO_F71862_ID,  f71862fg_pinconf) },
+	{ "f71868 watchdog",   FINTEK_DRV_DATA(SIO_F71868_ID,  f71868_pinconf) },
+	{ "f71869 watchdog",   FINTEK_DRV_DATA(SIO_F71869_ID,  f71868_pinconf) },
+	/* For backwards-compatibility, we define a different watchdog identity name */
+	{ "f71869a watchdog",  FINTEK_DRV_DATA(SIO_F71869A_ID, f71868_pinconf,
+					       .identity_override = "f71869 watchdog") },
+	{ "f71882fg watchdog", FINTEK_DRV_DATA(SIO_F71882_ID,  f71882fg_pinconf) },
+	{ "f71889fg watchdog", FINTEK_DRV_DATA(SIO_F71889_ID,  f71889fg_pinconf) },
+	{ "f81803 watchdog",   FINTEK_DRV_DATA(SIO_F81803_ID,  f81803_pinconf) },
+	{ "f81865 watchdog",   FINTEK_DRV_DATA(SIO_F81865_ID,  f81865_pinconf) },
+	{ "f81866 watchdog",   FINTEK_DRV_DATA(SIO_F81866_ID,  f81866_pinconf) },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(platform, fintek_wdt_id_table);
+
+/* Confirmed (by datasheet) not to have a watchdog.
+ * We list this here anyway to maintain backwards-compatibility in not warning about them
+ */
+static const u16 fintek_no_wdt_table[] = { SIO_F71858_ID, 0 };
+
+static int fintek_wdt_remove(struct platform_device *pdev)
+{
+	if (watchdog_is_running()) {
+		pr_warn("Watchdog timer still running, stopping it\n");
+		watchdog_stop();
+	}
+	misc_deregister(&watchdog_miscdev);
+	unregister_reboot_notifier(&watchdog_notifier);
+	return 0;
+}
+
+static struct platform_driver fintek_wdt_driver = {
+	.probe		= fintek_wdt_probe,
+	.remove		= fintek_wdt_remove,
+	.id_table	= fintek_wdt_id_table,
+	.driver		= {
+		.name	= DRVNAME,
+	},
+};
+
+static struct platform_device *fintek_wdt_pdev;
+
+static __init const char *fintek_wdt_find(int sioaddr)
+{
+	const char *name = ERR_PTR(-ENODEV);
+	const struct platform_device_id *id;
+	struct fintek_wdt_variant *variant;
 	u16 devid;
-	int err = superio_enter(sioaddr);
+	const u16 *pdevid;
+	int err;
+
+	err = superio_enter(sioaddr);
 	if (err)
-		return err;
+		return ERR_PTR(err);
 
 	devid = superio_inw(sioaddr, SIO_REG_MANID);
 	if (devid != SIO_FINTEK_ID) {
 		pr_debug("Not a Fintek device\n");
-		err = -ENODEV;
-		goto exit;
+		goto out;
 	}
 
 	devid = force_id ? force_id : superio_inw(sioaddr, SIO_REG_DEVID);
-	switch (devid) {
-	case SIO_F71808_ID:
-		watchdog.type = f71808fg;
-		break;
-	case SIO_F71862_ID:
-		watchdog.type = f71862fg;
-		break;
-	case SIO_F71868_ID:
-		watchdog.type = f71868;
-		break;
-	case SIO_F71869_ID:
-	case SIO_F71869A_ID:
-		watchdog.type = f71869;
-		break;
-	case SIO_F71882_ID:
-		watchdog.type = f71882fg;
-		break;
-	case SIO_F71889_ID:
-		watchdog.type = f71889fg;
-		break;
-	case SIO_F71858_ID:
-		/* Confirmed (by datasheet) not to have a watchdog. */
-		err = -ENODEV;
-		goto exit;
-	case SIO_F81803_ID:
-		watchdog.type = f81803;
-		break;
-	case SIO_F81865_ID:
-		watchdog.type = f81865;
-		break;
-	case SIO_F81866_ID:
-		watchdog.type = f81866;
-		break;
-	default:
-		pr_info("Unrecognized Fintek device: %04x\n",
-			(unsigned int)devid);
-		err = -ENODEV;
-		goto exit;
+	for (id = fintek_wdt_id_table; id->driver_data; id++) {
+		variant = (void *)id->driver_data;
+		if (variant->id == devid) {
+			name = id->name;
+			pr_info("Found %s chip, revision %d\n",
+				name, superio_inb(sioaddr, SIO_REG_DEVREV));
+			goto out;
+		}
+	}
+
+	for (pdevid = fintek_no_wdt_table; *pdevid; pdevid++) {
+		if (*pdevid == devid)
+			goto out;
 	}
 
-	pr_info("Found %s watchdog chip, revision %d\n",
-		f71808e_names[watchdog.type],
-		(int)superio_inb(sioaddr, SIO_REG_DEVREV));
-exit:
+	pr_info("Unrecognized Fintek device: %04x\n", devid);
+out:
 	superio_exit(sioaddr);
-	return err;
+	return name;
 }
 
-static int __init f71808e_init(void)
+static struct resource wdt_res = {
+	.name = "superio port",
+	.flags = IORESOURCE_IO,
+};
+
+static int __init fintek_wdt_init(void)
 {
-	static const unsigned short addrs[] = { 0x2e, 0x4e };
-	int err = -ENODEV;
+	unsigned short addrs[] = { 0x2e, 0x4e };
+	const char *wdt_name;
 	int i;
 
 	if (f71862fg_pin != 63 && f71862fg_pin != 56) {
@@ -846,30 +896,33 @@  static int __init f71808e_init(void)
 		return -EINVAL;
 	}
 
+	platform_driver_register(&fintek_wdt_driver);
+
 	for (i = 0; i < ARRAY_SIZE(addrs); i++) {
-		err = f71808e_find(addrs[i]);
-		if (err == 0)
+		wdt_name = fintek_wdt_find(addrs[i]);
+		if (!IS_ERR(wdt_name))
 			break;
 	}
+
 	if (i == ARRAY_SIZE(addrs))
-		return err;
+		return PTR_ERR(wdt_name);
+
+	wdt_res.start = addrs[i];
+	wdt_res.end = wdt_res.start + 1;
 
-	return watchdog_init(addrs[i]);
+	fintek_wdt_pdev = platform_device_register_simple(wdt_name, -1, &wdt_res, 1);
+	return PTR_ERR_OR_ZERO(fintek_wdt_pdev);
 }
 
-static void __exit f71808e_exit(void)
+static void __exit fintek_wdt_exit(void)
 {
-	if (watchdog_is_running()) {
-		pr_warn("Watchdog timer still running, stopping it\n");
-		watchdog_stop();
-	}
-	misc_deregister(&watchdog_miscdev);
-	unregister_reboot_notifier(&watchdog_notifier);
+	platform_device_unregister(fintek_wdt_pdev);
+	platform_driver_unregister(&fintek_wdt_driver);
 }
 
 MODULE_DESCRIPTION("F71808E Watchdog Driver");
 MODULE_AUTHOR("Giel van Schijndel <me@mortis.eu>");
 MODULE_LICENSE("GPL");
 
-module_init(f71808e_init);
-module_exit(f71808e_exit);
+module_init(fintek_wdt_init);
+module_exit(fintek_wdt_exit);