watchdog: iTCO_wdt: Use allocated data structures

PATCH Superseded Guenter Roeck 2 participants 14 messages Jan 3, 2017 – Jan 4, 2017 Watchdog device drivers

Share & tools

Guenter Roeck

author maintainer

2017-01-03 14:39 UTC #0 lore raw

PATCH 1/4 watchdog: iTCO_wdt: Use allocated data structures

Allocate private data and the watchdog device to to avoid having to clear it on remove and to enable subsequent simplifications.

Signed-off-by Guenter Roeck <[email protected]>

---
 drivers/watchdog/iTCO_wdt.c | 402 ++++++++++++++++++++++----------------------
 1 file changed, 205 insertions(+), 197 deletions(-)

drivers/watchdog/iTCO_wdt.c +205 ~~-197~~

index 06fcb6c8c917..a35a9164ccd0 100644
@@ -72,22 +72,24 @@
 
 /* Address definitions for the TCO */
 /* TCO base address */
-#define TCOBASE		(iTCO_wdt_private.tco_res->start)
+#define TCOBASE(p)	((p)->tco_res->start)
 /* SMI Control and Enable Register */
-#define SMI_EN		(iTCO_wdt_private.smi_res->start)
-
-#define TCO_RLD		(TCOBASE + 0x00) /* TCO Timer Reload and Curr. Value */
-#define TCOv1_TMR	(TCOBASE + 0x01) /* TCOv1 Timer Initial Value	*/
-#define TCO_DAT_IN	(TCOBASE + 0x02) /* TCO Data In Register	*/
-#define TCO_DAT_OUT	(TCOBASE + 0x03) /* TCO Data Out Register	*/
-#define TCO1_STS	(TCOBASE + 0x04) /* TCO1 Status Register	*/
-#define TCO2_STS	(TCOBASE + 0x06) /* TCO2 Status Register	*/
-#define TCO1_CNT	(TCOBASE + 0x08) /* TCO1 Control Register	*/
-#define TCO2_CNT	(TCOBASE + 0x0a) /* TCO2 Control Register	*/
-#define TCOv2_TMR	(TCOBASE + 0x12) /* TCOv2 Timer Initial Value	*/
+#define SMI_EN(p)	((p)->smi_res->start)
+
+#define TCO_RLD(p)	(TCOBASE(p) + 0x00) /* TCO Timer Reload/Curr. Value */
+#define TCOv1_TMR(p)	(TCOBASE(p) + 0x01) /* TCOv1 Timer Initial Value*/
+#define TCO_DAT_IN(p)	(TCOBASE(p) + 0x02) /* TCO Data In Register	*/
+#define TCO_DAT_OUT(p)	(TCOBASE(p) + 0x03) /* TCO Data Out Register	*/
+#define TCO1_STS(p)	(TCOBASE(p) + 0x04) /* TCO1 Status Register	*/
+#define TCO2_STS(p)	(TCOBASE(p) + 0x06) /* TCO2 Status Register	*/
+#define TCO1_CNT(p)	(TCOBASE(p) + 0x08) /* TCO1 Control Register	*/
+#define TCO2_CNT(p)	(TCOBASE(p) + 0x0a) /* TCO2 Control Register	*/
+#define TCOv2_TMR(p)	(TCOBASE(p) + 0x12) /* TCOv2 Timer Initial Value*/
 
 /* internal variables */
-static struct {		/* this is private data for the iTCO_wdt device */
+struct iTCO_wdt_private {
+	struct watchdog_device wddev;
+
 	/* TCO version/generation */
 	unsigned int iTCO_version;
 	struct resource *tco_res;

@@ -105,7 +107,7 @@ static struct {		/* this is private data for the iTCO_wdt device */
 	struct pci_dev *pdev;
 	/* whether or not the watchdog has been suspended */
 	bool suspended;
-} iTCO_wdt_private;
+};
 
 /* module parameters */
 #define WATCHDOG_TIMEOUT 30	/* 30 sec default heartbeat */

@@ -135,21 +137,23 @@ MODULE_PARM_DESC(turn_SMI_watchdog_clear_off,
  * every 0.6 seconds.  v3's internal timer is stored as seconds (some
  * datasheets incorrectly state 0.6 seconds).
  */
-static inline unsigned int seconds_to_ticks(int secs)
+static inline unsigned int seconds_to_ticks(struct iTCO_wdt_private *p,
+					    int secs)
 {
-	return iTCO_wdt_private.iTCO_version == 3 ? secs : (secs * 10) / 6;
+	return p->iTCO_version == 3 ? secs : (secs * 10) / 6;
 }
 
-static inline unsigned int ticks_to_seconds(int ticks)
+static inline unsigned int ticks_to_seconds(struct iTCO_wdt_private *p,
+					    int ticks)
 {
-	return iTCO_wdt_private.iTCO_version == 3 ? ticks : (ticks * 6) / 10;
+	return p->iTCO_version == 3 ? ticks : (ticks * 6) / 10;
 }
 
-static inline u32 no_reboot_bit(void)
+static inline u32 no_reboot_bit(struct iTCO_wdt_private *p)
 {
 	u32 enable_bit;
 
-	switch (iTCO_wdt_private.iTCO_version) {
+	switch (p->iTCO_version) {
 	case 5:
 	case 3:
 		enable_bit = 0x00000010;

@@ -167,40 +171,40 @@ static inline u32 no_reboot_bit(void)
 	return enable_bit;
 }
 
-static void iTCO_wdt_set_NO_REBOOT_bit(void)
+static void iTCO_wdt_set_NO_REBOOT_bit(struct iTCO_wdt_private *p)
 {
 	u32 val32;
 
 	/* Set the NO_REBOOT bit: this disables reboots */
-	if (iTCO_wdt_private.iTCO_version >= 2) {
-		val32 = readl(iTCO_wdt_private.gcs_pmc);
-		val32 |= no_reboot_bit();
-		writel(val32, iTCO_wdt_private.gcs_pmc);
-	} else if (iTCO_wdt_private.iTCO_version == 1) {
-		pci_read_config_dword(iTCO_wdt_private.pdev, 0xd4, &val32);
-		val32 |= no_reboot_bit();
-		pci_write_config_dword(iTCO_wdt_private.pdev, 0xd4, val32);
+	if (p->iTCO_version >= 2) {
+		val32 = readl(p->gcs_pmc);
+		val32 |= no_reboot_bit(p);
+		writel(val32, p->gcs_pmc);
+	} else if (p->iTCO_version == 1) {
+		pci_read_config_dword(p->pdev, 0xd4, &val32);
+		val32 |= no_reboot_bit(p);
+		pci_write_config_dword(p->pdev, 0xd4, val32);
 	}
 }
 
-static int iTCO_wdt_unset_NO_REBOOT_bit(void)
+static int iTCO_wdt_unset_NO_REBOOT_bit(struct iTCO_wdt_private *p)
 {
-	u32 enable_bit = no_reboot_bit();
+	u32 enable_bit = no_reboot_bit(p);
 	u32 val32 = 0;
 
 	/* Unset the NO_REBOOT bit: this enables reboots */
-	if (iTCO_wdt_private.iTCO_version >= 2) {
-		val32 = readl(iTCO_wdt_private.gcs_pmc);
+	if (p->iTCO_version >= 2) {
+		val32 = readl(p->gcs_pmc);
 		val32 &= ~enable_bit;
-		writel(val32, iTCO_wdt_private.gcs_pmc);
+		writel(val32, p->gcs_pmc);
 
-		val32 = readl(iTCO_wdt_private.gcs_pmc);
-	} else if (iTCO_wdt_private.iTCO_version == 1) {
-		pci_read_config_dword(iTCO_wdt_private.pdev, 0xd4, &val32);
+		val32 = readl(p->gcs_pmc);
+	} else if (p->iTCO_version == 1) {
+		pci_read_config_dword(p->pdev, 0xd4, &val32);
 		val32 &= ~enable_bit;
-		pci_write_config_dword(iTCO_wdt_private.pdev, 0xd4, val32);
+		pci_write_config_dword(p->pdev, 0xd4, val32);
 
-		pci_read_config_dword(iTCO_wdt_private.pdev, 0xd4, &val32);
+		pci_read_config_dword(p->pdev, 0xd4, &val32);
 	}
 
 	if (val32 & enable_bit)

@@ -211,32 +215,33 @@ static int iTCO_wdt_unset_NO_REBOOT_bit(void)
 
 static int iTCO_wdt_start(struct watchdog_device *wd_dev)
 {
+	struct iTCO_wdt_private *p = watchdog_get_drvdata(wd_dev);
 	unsigned int val;
 
-	spin_lock(&iTCO_wdt_private.io_lock);
+	spin_lock(&p->io_lock);
 
-	iTCO_vendor_pre_start(iTCO_wdt_private.smi_res, wd_dev->timeout);
+	iTCO_vendor_pre_start(p->smi_res, wd_dev->timeout);
 
 	/* disable chipset's NO_REBOOT bit */
-	if (iTCO_wdt_unset_NO_REBOOT_bit()) {
-		spin_unlock(&iTCO_wdt_private.io_lock);
+	if (iTCO_wdt_unset_NO_REBOOT_bit(p)) {
+		spin_unlock(&p->io_lock);
 		pr_err("failed to reset NO_REBOOT flag, reboot disabled by hardware/BIOS\n");
 		return -EIO;
 	}
 
 	/* Force the timer to its reload value by writing to the TCO_RLD
 	   register */
-	if (iTCO_wdt_private.iTCO_version >= 2)
-		outw(0x01, TCO_RLD);
-	else if (iTCO_wdt_private.iTCO_version == 1)
-		outb(0x01, TCO_RLD);
+	if (p->iTCO_version >= 2)
+		outw(0x01, TCO_RLD(p));
+	else if (p->iTCO_version == 1)
+		outb(0x01, TCO_RLD(p));
 
 	/* Bit 11: TCO Timer Halt -> 0 = The TCO timer is enabled to count */
-	val = inw(TCO1_CNT);
+	val = inw(TCO1_CNT(p));
 	val &= 0xf7ff;
-	outw(val, TCO1_CNT);
-	val = inw(TCO1_CNT);
-	spin_unlock(&iTCO_wdt_private.io_lock);
+	outw(val, TCO1_CNT(p));
+	val = inw(TCO1_CNT(p));
+	spin_unlock(&p->io_lock);
 
 	if (val & 0x0800)
 		return -1;

@@ -245,22 +250,23 @@ static int iTCO_wdt_start(struct watchdog_device *wd_dev)
 
 static int iTCO_wdt_stop(struct watchdog_device *wd_dev)
 {
+	struct iTCO_wdt_private *p = watchdog_get_drvdata(wd_dev);
 	unsigned int val;
 
-	spin_lock(&iTCO_wdt_private.io_lock);
+	spin_lock(&p->io_lock);
 
-	iTCO_vendor_pre_stop(iTCO_wdt_private.smi_res);
+	iTCO_vendor_pre_stop(p->smi_res);
 
 	/* Bit 11: TCO Timer Halt -> 1 = The TCO timer is disabled */
-	val = inw(TCO1_CNT);
+	val = inw(TCO1_CNT(p));
 	val |= 0x0800;
-	outw(val, TCO1_CNT);
-	val = inw(TCO1_CNT);
+	outw(val, TCO1_CNT(p));
+	val = inw(TCO1_CNT(p));
 
 	/* Set the NO_REBOOT bit to prevent later reboots, just for sure */
-	iTCO_wdt_set_NO_REBOOT_bit();
+	iTCO_wdt_set_NO_REBOOT_bit(p);
 
-	spin_unlock(&iTCO_wdt_private.io_lock);
+	spin_unlock(&p->io_lock);
 
 	if ((val & 0x0800) == 0)
 		return -1;

@@ -269,67 +275,70 @@ static int iTCO_wdt_stop(struct watchdog_device *wd_dev)
 
 static int iTCO_wdt_ping(struct watchdog_device *wd_dev)
 {
-	spin_lock(&iTCO_wdt_private.io_lock);
+	struct iTCO_wdt_private *p = watchdog_get_drvdata(wd_dev);
 
-	iTCO_vendor_pre_keepalive(iTCO_wdt_private.smi_res, wd_dev->timeout);
+	spin_lock(&p->io_lock);
+
+	iTCO_vendor_pre_keepalive(p->smi_res, wd_dev->timeout);
 
 	/* Reload the timer by writing to the TCO Timer Counter register */
-	if (iTCO_wdt_private.iTCO_version >= 2) {
-		outw(0x01, TCO_RLD);
-	} else if (iTCO_wdt_private.iTCO_version == 1) {
+	if (p->iTCO_version >= 2) {
+		outw(0x01, TCO_RLD(p));
+	} else if (p->iTCO_version == 1) {
 		/* Reset the timeout status bit so that the timer
 		 * needs to count down twice again before rebooting */
-		outw(0x0008, TCO1_STS);	/* write 1 to clear bit */
+		outw(0x0008, TCO1_STS(p));	/* write 1 to clear bit */
 
-		outb(0x01, TCO_RLD);
+		outb(0x01, TCO_RLD(p));
 	}
 
-	spin_unlock(&iTCO_wdt_private.io_lock);
+	spin_unlock(&p->io_lock);
 	return 0;
 }
 
 static int iTCO_wdt_set_timeout(struct watchdog_device *wd_dev, unsigned int t)
 {
+	struct iTCO_wdt_private *p = watchdog_get_drvdata(wd_dev);
 	unsigned int val16;
 	unsigned char val8;
 	unsigned int tmrval;
 
-	tmrval = seconds_to_ticks(t);
+	tmrval = seconds_to_ticks(p, t);
 
 	/* For TCO v1 the timer counts down twice before rebooting */
-	if (iTCO_wdt_private.iTCO_version == 1)
+	if (p->iTCO_version == 1)
 		tmrval /= 2;
 
 	/* from the specs: */
 	/* "Values of 0h-3h are ignored and should not be attempted" */
 	if (tmrval < 0x04)
 		return -EINVAL;
-	if (((iTCO_wdt_private.iTCO_version >= 2) && (tmrval > 0x3ff)) ||
-	    ((iTCO_wdt_private.iTCO_version == 1) && (tmrval > 0x03f)))
+	if ((p->iTCO_version >= 2 && tmrval > 0x3ff) ||
+	    (p->iTCO_version == 1 && tmrval > 0x03f))
 		return -EINVAL;
 
 	iTCO_vendor_pre_set_heartbeat(tmrval);
 
 	/* Write new heartbeat to watchdog */
-	if (iTCO_wdt_private.iTCO_version >= 2) {
-		spin_lock(&iTCO_wdt_private.io_lock);
-		val16 = inw(TCOv2_TMR);
+	if (p->iTCO_version >= 2) {
+		spin_lock(&p->io_lock);
+		val16 = inw(TCOv2_TMR(p));
 		val16 &= 0xfc00;
 		val16 |= tmrval;
-		outw(val16, TCOv2_TMR);
-		val16 = inw(TCOv2_TMR);
-		spin_unlock(&iTCO_wdt_private.io_lock);
+		outw(val16, TCOv2_TMR(p));
+		val16 = inw(TCOv2_TMR(p));
+		spin_unlock(&p->io_lock);
 
 		if ((val16 & 0x3ff) != tmrval)
 			return -EINVAL;
-	} else if (iTCO_wdt_private.iTCO_version == 1) {
-		spin_lock(&iTCO_wdt_private.io_lock);
-		val8 = inb(TCOv1_TMR);
+	} else if (p->iTCO_version == 1) {
+		spin_lock(&p->io_lock);
+		val8 = inb(TCOv1_TMR(p));
 		val8 &= 0xc0;
 		val8 |= (tmrval & 0xff);
-		outb(val8, TCOv1_TMR);
-		val8 = inb(TCOv1_TMR);
-		spin_unlock(&iTCO_wdt_private.io_lock);
+		outb(val8, TCOv1_TMR(p));
+		val8 = inb(TCOv1_TMR(p));
+		spin_unlock(&p->io_lock);
 
 		if ((val8 & 0x3f) != tmrval)
 			return -EINVAL;

@@ -341,27 +350,28 @@ static int iTCO_wdt_set_timeout(struct watchdog_device *wd_dev, unsigned int t)
 
 static unsigned int iTCO_wdt_get_timeleft(struct watchdog_device *wd_dev)
 {
+	struct iTCO_wdt_private *p = watchdog_get_drvdata(wd_dev);
 	unsigned int val16;
 	unsigned char val8;
 	unsigned int time_left = 0;
 
 	/* read the TCO Timer */
-	if (iTCO_wdt_private.iTCO_version >= 2) {
-		spin_lock(&iTCO_wdt_private.io_lock);
-		val16 = inw(TCO_RLD);
+	if (p->iTCO_version >= 2) {
+		spin_lock(&p->io_lock);
+		val16 = inw(TCO_RLD(p));
 		val16 &= 0x3ff;
-		spin_unlock(&iTCO_wdt_private.io_lock);
+		spin_unlock(&p->io_lock);
 
-		time_left = ticks_to_seconds(val16);
-	} else if (iTCO_wdt_private.iTCO_version == 1) {
-		spin_lock(&iTCO_wdt_private.io_lock);
-		val8 = inb(TCO_RLD);
+		time_left = ticks_to_seconds(p, val16);
+	} else if (p->iTCO_version == 1) {
+		spin_lock(&p->io_lock);
+		val8 = inb(TCO_RLD(p));
 		val8 &= 0x3f;
-		if (!(inw(TCO1_STS) & 0x0008))
-			val8 += (inb(TCOv1_TMR) & 0x3f);
-		spin_unlock(&iTCO_wdt_private.io_lock);
+		if (!(inw(TCO1_STS(p)) & 0x0008))
+			val8 += (inb(TCOv1_TMR(p)) & 0x3f);
+		spin_unlock(&p->io_lock);
 
-		time_left = ticks_to_seconds(val8);
+		time_left = ticks_to_seconds(p, val8);
 	}
 	return time_left;
 }

@@ -387,166 +397,165 @@ static const struct watchdog_ops iTCO_wdt_ops = {
 	.get_timeleft =		iTCO_wdt_get_timeleft,
 };
 
-static struct watchdog_device iTCO_wdt_watchdog_dev = {
-	.info =		&ident,
-	.ops =		&iTCO_wdt_ops,
-};
-
 /*
  *	Init & exit routines
  */
 
-static void iTCO_wdt_cleanup(void)
+static void iTCO_wdt_cleanup(struct iTCO_wdt_private *p)
 {
 	/* Stop the timer before we leave */
 	if (!nowayout)
-		iTCO_wdt_stop(&iTCO_wdt_watchdog_dev);
+		iTCO_wdt_stop(&p->wddev);
 
 	/* Deregister */
-	watchdog_unregister_device(&iTCO_wdt_watchdog_dev);
+	watchdog_unregister_device(&p->wddev);
 
 	/* release resources */
-	release_region(iTCO_wdt_private.tco_res->start,
-			resource_size(iTCO_wdt_private.tco_res));
-	release_region(iTCO_wdt_private.smi_res->start,
-			resource_size(iTCO_wdt_private.smi_res));
-	if (iTCO_wdt_private.iTCO_version >= 2) {
-		iounmap(iTCO_wdt_private.gcs_pmc);
-		release_mem_region(iTCO_wdt_private.gcs_pmc_res->start,
-				resource_size(iTCO_wdt_private.gcs_pmc_res));
+	release_region(p->tco_res->start,
+			resource_size(p->tco_res));
+	release_region(p->smi_res->start,
+			resource_size(p->smi_res));
+	if (p->iTCO_version >= 2) {
+		iounmap(p->gcs_pmc);
+		release_mem_region(p->gcs_pmc_res->start,
+				resource_size(p->gcs_pmc_res));
 	}
-
-	iTCO_wdt_private.tco_res = NULL;
-	iTCO_wdt_private.smi_res = NULL;
-	iTCO_wdt_private.gcs_pmc_res = NULL;
-	iTCO_wdt_private.gcs_pmc = NULL;
 }
 
 static int iTCO_wdt_probe(struct platform_device *dev)
 {
-	int ret = -ENODEV;
-	unsigned long val32;
 	struct itco_wdt_platform_data *pdata = dev_get_platdata(&dev->dev);
+	struct iTCO_wdt_private *p;
+	unsigned long val32;
+	int ret;
 
 	if (!pdata)
-		goto out;
+		return -ENODEV;
 
-	spin_lock_init(&iTCO_wdt_private.io_lock);
+	p = devm_kzalloc(&dev->dev, sizeof(*p), GFP_KERNEL);
+	if (!p)
+		return -ENOMEM;
 
-	iTCO_wdt_private.tco_res =
-		platform_get_resource(dev, IORESOURCE_IO, ICH_RES_IO_TCO);
-	if (!iTCO_wdt_private.tco_res)
-		goto out;
+	spin_lock_init(&p->io_lock);
 
-	iTCO_wdt_private.smi_res =
-		platform_get_resource(dev, IORESOURCE_IO, ICH_RES_IO_SMI);
-	if (!iTCO_wdt_private.smi_res)
-		goto out;
+	p->tco_res = platform_get_resource(dev, IORESOURCE_IO, ICH_RES_IO_TCO);
+	if (!p->tco_res)
+		return -ENODEV;
 
-	iTCO_wdt_private.iTCO_version = pdata->version;
-	iTCO_wdt_private.dev = dev;
-	iTCO_wdt_private.pdev = to_pci_dev(dev->dev.parent);
+	p->smi_res = platform_get_resource(dev, IORESOURCE_IO, ICH_RES_IO_SMI);
+	if (!p->smi_res)
+		return -ENODEV;
+
+	p->iTCO_version = pdata->version;
+	p->dev = dev;
+	p->pdev = to_pci_dev(dev->dev.parent);
 
 	/*
 	 * Get the Memory-Mapped GCS or PMC register, we need it for the
 	 * NO_REBOOT flag (TCO v2 and v3).
 	 */
-	if (iTCO_wdt_private.iTCO_version >= 2) {
-		iTCO_wdt_private.gcs_pmc_res = platform_get_resource(dev,
-							IORESOURCE_MEM,
-							ICH_RES_MEM_GCS_PMC);
-
-		if (!iTCO_wdt_private.gcs_pmc_res)
-			goto out;
-
-		if (!request_mem_region(iTCO_wdt_private.gcs_pmc_res->start,
-			resource_size(iTCO_wdt_private.gcs_pmc_res), dev->name)) {
-			ret = -EBUSY;
-			goto out;
-		}
-		iTCO_wdt_private.gcs_pmc = ioremap(iTCO_wdt_private.gcs_pmc_res->start,
-			resource_size(iTCO_wdt_private.gcs_pmc_res));
-		if (!iTCO_wdt_private.gcs_pmc) {
+	if (p->iTCO_version >= 2) {
+		p->gcs_pmc_res = platform_get_resource(dev,
+						       IORESOURCE_MEM,
+						       ICH_RES_MEM_GCS_PMC);
+
+		if (!p->gcs_pmc_res)
+			return -ENODEV;
+
+		if (!request_mem_region(p->gcs_pmc_res->start,
+					resource_size(p->gcs_pmc_res),
+					dev->name))
+			return -EBUSY;
+
+		p->gcs_pmc = ioremap(p->gcs_pmc_res->start,
+				     resource_size(p->gcs_pmc_res));
+		if (!p->gcs_pmc) {
 			ret = -EIO;
 			goto unreg_gcs_pmc;
 		}
 	}
 
 	/* Check chipset's NO_REBOOT bit */
-	if (iTCO_wdt_unset_NO_REBOOT_bit() && iTCO_vendor_check_noreboot_on()) {
+	if (iTCO_wdt_unset_NO_REBOOT_bit(p) &&
+	    iTCO_vendor_check_noreboot_on()) {
 		pr_info("unable to reset NO_REBOOT flag, device disabled by hardware/BIOS\n");
 		ret = -ENODEV;	/* Cannot reset NO_REBOOT bit */
 		goto unmap_gcs_pmc;
 	}
 
 	/* Set the NO_REBOOT bit to prevent later reboots, just for sure */
-	iTCO_wdt_set_NO_REBOOT_bit();
+	iTCO_wdt_set_NO_REBOOT_bit(p);
 
 	/* The TCO logic uses the TCO_EN bit in the SMI_EN register */
-	if (!request_region(iTCO_wdt_private.smi_res->start,
-			resource_size(iTCO_wdt_private.smi_res), dev->name)) {
+	if (!request_region(p->smi_res->start,
+			    resource_size(p->smi_res), dev->name)) {
 		pr_err("I/O address 0x%04llx already in use, device disabled\n",
-		       (u64)SMI_EN);
+		       (u64)SMI_EN(p));
 		ret = -EBUSY;
 		goto unmap_gcs_pmc;
 	}
-	if (turn_SMI_watchdog_clear_off >= iTCO_wdt_private.iTCO_version) {
+	if (turn_SMI_watchdog_clear_off >= p->iTCO_version) {
 		/*
 		 * Bit 13: TCO_EN -> 0
 		 * Disables TCO logic generating an SMI#
 		 */
-		val32 = inl(SMI_EN);
+		val32 = inl(SMI_EN(p));
 		val32 &= 0xffffdfff;	/* Turn off SMI clearing watchdog */
-		outl(val32, SMI_EN);
+		outl(val32, SMI_EN(p));
 	}
 
-	if (!request_region(iTCO_wdt_private.tco_res->start,
-			resource_size(iTCO_wdt_private.tco_res), dev->name)) {
+	if (!request_region(p->tco_res->start,
+			    resource_size(p->tco_res), dev->name)) {
 		pr_err("I/O address 0x%04llx already in use, device disabled\n",
-		       (u64)TCOBASE);
+		       (u64)TCOBASE(p));
 		ret = -EBUSY;
 		goto unreg_smi;
 	}
 
 	pr_info("Found a %s TCO device (Version=%d, TCOBASE=0x%04llx)\n",
-		pdata->name, pdata->version, (u64)TCOBASE);
+		pdata->name, pdata->version, (u64)TCOBASE(p));
 
 	/* Clear out the (probably old) status */
-	switch (iTCO_wdt_private.iTCO_version) {
+	switch (p->iTCO_version) {
 	case 5:
 	case 4:
-		outw(0x0008, TCO1_STS);	/* Clear the Time Out Status bit */
-		outw(0x0002, TCO2_STS);	/* Clear SECOND_TO_STS bit */
+		outw(0x0008, TCO1_STS(p)); /* Clear the Time Out Status bit */
+		outw(0x0002, TCO2_STS(p)); /* Clear SECOND_TO_STS bit */
 		break;
 	case 3:
-		outl(0x20008, TCO1_STS);
+		outl(0x20008, TCO1_STS(p));
 		break;
 	case 2:
 	case 1:
 	default:
-		outw(0x0008, TCO1_STS);	/* Clear the Time Out Status bit */
-		outw(0x0002, TCO2_STS);	/* Clear SECOND_TO_STS bit */
-		outw(0x0004, TCO2_STS);	/* Clear BOOT_STS bit */
+		outw(0x0008, TCO1_STS(p)); /* Clear the Time Out Status bit */
+		outw(0x0002, TCO2_STS(p)); /* Clear SECOND_TO_STS bit */
+		outw(0x0004, TCO2_STS(p)); /* Clear BOOT_STS bit */
 		break;
 	}
 
-	iTCO_wdt_watchdog_dev.bootstatus = 0;
-	iTCO_wdt_watchdog_dev.timeout = WATCHDOG_TIMEOUT;
-	watchdog_set_nowayout(&iTCO_wdt_watchdog_dev, nowayout);
-	iTCO_wdt_watchdog_dev.parent = &dev->dev;
+	p->wddev.info =	&ident,
+	p->wddev.ops = &iTCO_wdt_ops,
+	p->wddev.bootstatus = 0;
+	p->wddev.timeout = WATCHDOG_TIMEOUT;
+	watchdog_set_nowayout(&p->wddev, nowayout);
+	p->wddev.parent = &dev->dev;
+
+	watchdog_set_drvdata(&p->wddev, p);
+	platform_set_drvdata(dev, p);
 
 	/* Make sure the watchdog is not running */
-	iTCO_wdt_stop(&iTCO_wdt_watchdog_dev);
+	iTCO_wdt_stop(&p->wddev);
 
 	/* Check that the heartbeat value is within it's range;
 	   if not reset to the default */
-	if (iTCO_wdt_set_timeout(&iTCO_wdt_watchdog_dev, heartbeat)) {
-		iTCO_wdt_set_timeout(&iTCO_wdt_watchdog_dev, WATCHDOG_TIMEOUT);
+	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",
 			WATCHDOG_TIMEOUT);
 	}
 
-	ret = watchdog_register_device(&iTCO_wdt_watchdog_dev);
+	ret = watchdog_register_device(&p->wddev);
 	if (ret != 0) {
 		pr_err("cannot register watchdog device (err=%d)\n", ret);
 		goto unreg_tco;

@@ -558,38 +567,34 @@ static int iTCO_wdt_probe(struct platform_device *dev)
 	return 0;
 
 unreg_tco:
-	release_region(iTCO_wdt_private.tco_res->start,
-			resource_size(iTCO_wdt_private.tco_res));
+	release_region(p->tco_res->start, resource_size(p->tco_res));
 unreg_smi:
-	release_region(iTCO_wdt_private.smi_res->start,
-			resource_size(iTCO_wdt_private.smi_res));
+	release_region(p->smi_res->start, resource_size(p->smi_res));
 unmap_gcs_pmc:
-	if (iTCO_wdt_private.iTCO_version >= 2)
-		iounmap(iTCO_wdt_private.gcs_pmc);
+	if (p->iTCO_version >= 2)
+		iounmap(p->gcs_pmc);
 unreg_gcs_pmc:
-	if (iTCO_wdt_private.iTCO_version >= 2)
-		release_mem_region(iTCO_wdt_private.gcs_pmc_res->start,
-				resource_size(iTCO_wdt_private.gcs_pmc_res));
-out:
-	iTCO_wdt_private.tco_res = NULL;
-	iTCO_wdt_private.smi_res = NULL;
-	iTCO_wdt_private.gcs_pmc_res = NULL;
-	iTCO_wdt_private.gcs_pmc = NULL;
-
+	if (p->iTCO_version >= 2)
+		release_mem_region(p->gcs_pmc_res->start,
+				   resource_size(p->gcs_pmc_res));
 	return ret;
 }
 
 static int iTCO_wdt_remove(struct platform_device *dev)
 {
-	if (iTCO_wdt_private.tco_res || iTCO_wdt_private.smi_res)
-		iTCO_wdt_cleanup();
+	struct iTCO_wdt_private *p = platform_get_drvdata(dev);
+
+	if (p->tco_res || p->smi_res)
+		iTCO_wdt_cleanup(p);
 
 	return 0;
 }
 
 static void iTCO_wdt_shutdown(struct platform_device *dev)
 {
-	iTCO_wdt_stop(NULL);
+	struct iTCO_wdt_private *p = platform_get_drvdata(dev);
+
+	iTCO_wdt_stop(&p->wddev);
 }
 
 #ifdef CONFIG_PM_SLEEP

@@ -610,21 +615,24 @@ static inline bool need_suspend(void) { return true; }
 
 static int iTCO_wdt_suspend_noirq(struct device *dev)
 {
+	struct iTCO_wdt_private *p = dev_get_drvdata(dev);
 	int ret = 0;
 
-	iTCO_wdt_private.suspended = false;
-	if (watchdog_active(&iTCO_wdt_watchdog_dev) && need_suspend()) {
-		ret = iTCO_wdt_stop(&iTCO_wdt_watchdog_dev);
+	p->suspended = false;
+	if (watchdog_active(&p->wddev) && need_suspend()) {
+		ret = iTCO_wdt_stop(&p->wddev);
 		if (!ret)
-			iTCO_wdt_private.suspended = true;
+			p->suspended = true;
 	}
 	return ret;
 }
 
 static int iTCO_wdt_resume_noirq(struct device *dev)
 {
-	if (iTCO_wdt_private.suspended)
-		iTCO_wdt_start(&iTCO_wdt_watchdog_dev);
+	struct iTCO_wdt_private *p = dev_get_drvdata(dev);
+
+	if (p->suspended)
+		iTCO_wdt_start(&p->wddev);
 
 	return 0;
 }

Signature · 1 line

2.7.4

Guenter Roeck

author maintainer

2017-01-03 14:39 UTC #1 lore raw

PATCH 2/4 watchdog: iTCO_wdt: Use device managed resources

Using device managed resources simplifies error handling and cleanup, and to reduce the likelyhood of errors.

Signed-off-by Guenter Roeck <[email protected]>

---
 drivers/watchdog/iTCO_wdt.c | 80 ++++++++++-----------------------------------
 1 file changed, 17 insertions(+), 63 deletions(-)

drivers/watchdog/iTCO_wdt.c +17 ~~-63~~

index a35a9164ccd0..eed1dee6de19 100644
@@ -401,27 +401,6 @@ static const struct watchdog_ops iTCO_wdt_ops = {
  *	Init & exit routines
  */
 
-static void iTCO_wdt_cleanup(struct iTCO_wdt_private *p)
-{
-	/* Stop the timer before we leave */
-	if (!nowayout)
-		iTCO_wdt_stop(&p->wddev);
-
-	/* Deregister */
-	watchdog_unregister_device(&p->wddev);
-
-	/* release resources */
-	release_region(p->tco_res->start,
-			resource_size(p->tco_res));
-	release_region(p->smi_res->start,
-			resource_size(p->smi_res));
-	if (p->iTCO_version >= 2) {
-		iounmap(p->gcs_pmc);
-		release_mem_region(p->gcs_pmc_res->start,
-				resource_size(p->gcs_pmc_res));
-	}
-}
-
 static int iTCO_wdt_probe(struct platform_device *dev)
 {
 	struct itco_wdt_platform_data *pdata = dev_get_platdata(&dev->dev);

@@ -458,41 +437,28 @@ static int iTCO_wdt_probe(struct platform_device *dev)
 		p->gcs_pmc_res = platform_get_resource(dev,
 						       IORESOURCE_MEM,
 						       ICH_RES_MEM_GCS_PMC);
-
-		if (!p->gcs_pmc_res)
-			return -ENODEV;
-
-		if (!request_mem_region(p->gcs_pmc_res->start,
-					resource_size(p->gcs_pmc_res),
-					dev->name))
-			return -EBUSY;
-
-		p->gcs_pmc = ioremap(p->gcs_pmc_res->start,
-				     resource_size(p->gcs_pmc_res));
-		if (!p->gcs_pmc) {
-			ret = -EIO;
-			goto unreg_gcs_pmc;
-		}
+		p->gcs_pmc = devm_ioremap_resource(&dev->dev, p->gcs_pmc_res);
+		if (IS_ERR(p->gcs_pmc))
+			return PTR_ERR(p->gcs_pmc);
 	}
 
 	/* Check chipset's NO_REBOOT bit */
 	if (iTCO_wdt_unset_NO_REBOOT_bit(p) &&
 	    iTCO_vendor_check_noreboot_on()) {
 		pr_info("unable to reset NO_REBOOT flag, device disabled by hardware/BIOS\n");
-		ret = -ENODEV;	/* Cannot reset NO_REBOOT bit */
-		goto unmap_gcs_pmc;
+		return -ENODEV;	/* Cannot reset NO_REBOOT bit */
 	}
 
 	/* Set the NO_REBOOT bit to prevent later reboots, just for sure */
 	iTCO_wdt_set_NO_REBOOT_bit(p);
 
 	/* The TCO logic uses the TCO_EN bit in the SMI_EN register */
-	if (!request_region(p->smi_res->start,
-			    resource_size(p->smi_res), dev->name)) {
+	if (!devm_request_region(&dev->dev, p->smi_res->start,
+				 resource_size(p->smi_res),
+				 dev->name)) {
 		pr_err("I/O address 0x%04llx already in use, device disabled\n",
 		       (u64)SMI_EN(p));
-		ret = -EBUSY;
-		goto unmap_gcs_pmc;
+		return -EBUSY;
 	}
 	if (turn_SMI_watchdog_clear_off >= p->iTCO_version) {
 		/*

@@ -504,12 +470,12 @@ static int iTCO_wdt_probe(struct platform_device *dev)
 		outl(val32, SMI_EN(p));
 	}
 
-	if (!request_region(p->tco_res->start,
-			    resource_size(p->tco_res), dev->name)) {
+	if (!devm_request_region(&dev->dev, p->tco_res->start,
+				 resource_size(p->tco_res),
+				 dev->name)) {
 		pr_err("I/O address 0x%04llx already in use, device disabled\n",
 		       (u64)TCOBASE(p));
-		ret = -EBUSY;
-		goto unreg_smi;
+		return -EBUSY;
 	}
 
 	pr_info("Found a %s TCO device (Version=%d, TCOBASE=0x%04llx)\n",

@@ -555,37 +521,25 @@ static int iTCO_wdt_probe(struct platform_device *dev)
 			WATCHDOG_TIMEOUT);
 	}
 
-	ret = watchdog_register_device(&p->wddev);
+	ret = devm_watchdog_register_device(&dev->dev, &p->wddev);
 	if (ret != 0) {
 		pr_err("cannot register watchdog device (err=%d)\n", ret);
-		goto unreg_tco;
+		return ret;
 	}
 
 	pr_info("initialized. heartbeat=%d sec (nowayout=%d)\n",
 		heartbeat, nowayout);
 
 	return 0;
-
-unreg_tco:
-	release_region(p->tco_res->start, resource_size(p->tco_res));
-unreg_smi:
-	release_region(p->smi_res->start, resource_size(p->smi_res));
-unmap_gcs_pmc:
-	if (p->iTCO_version >= 2)
-		iounmap(p->gcs_pmc);
-unreg_gcs_pmc:
-	if (p->iTCO_version >= 2)
-		release_mem_region(p->gcs_pmc_res->start,
-				   resource_size(p->gcs_pmc_res));
-	return ret;
 }
 
 static int iTCO_wdt_remove(struct platform_device *dev)
 {
 	struct iTCO_wdt_private *p = platform_get_drvdata(dev);
 
-	if (p->tco_res || p->smi_res)
-		iTCO_wdt_cleanup(p);
+	/* Stop the timer before we leave */
+	if (!nowayout)
+		iTCO_wdt_stop(&p->wddev);
 
 	return 0;
 }

Signature · 1 line

2.7.4

Andy Shevchenko

Reviewed-by on v1 2/4

2017-01-03 22:41 UTC #2 lore raw

Re: [PATCH 2/4] watchdog: iTCO_wdt: Use device managed resources

Quoted 4 lines · Guenter Roeck

Using device managed resources simplifies error handling and cleanup, and to reduce the likelyhood of errors.
Signed-off-by Guenter Roeck <[email protected]>

Reviewed-by Andy Shevchenko <[email protected]>

Does it make sense to convert to dev_err() at some point?

Quoted 149 lines

---
 drivers/watchdog/iTCO_wdt.c | 80 ++++++++++-----------------------------------
 1 file changed, 17 insertions(+), 63 deletions(-)

drivers/watchdog/iTCO_wdt.c +17 ~~-64~~

index a35a9164ccd0..eed1dee6de19 100644
@@ -401,27 +401,6 @@ static const struct watchdog_ops iTCO_wdt_ops = {
  *     Init & exit routines
  */
 
-static void iTCO_wdt_cleanup(struct iTCO_wdt_private *p)
-{
-       /* Stop the timer before we leave */
-       if (!nowayout)
-               iTCO_wdt_stop(&p->wddev);
-
-       /* Deregister */
-       watchdog_unregister_device(&p->wddev);
-
-       /* release resources */
-       release_region(p->tco_res->start,
-                       resource_size(p->tco_res));
-       release_region(p->smi_res->start,
-                       resource_size(p->smi_res));
-       if (p->iTCO_version >= 2) {
-               iounmap(p->gcs_pmc);
-               release_mem_region(p->gcs_pmc_res->start,
-                               resource_size(p->gcs_pmc_res));
-       }
-}
-
 static int iTCO_wdt_probe(struct platform_device *dev)
 {
        struct itco_wdt_platform_data *pdata = dev_get_platdata(&dev->dev);

@@ -458,41 +437,28 @@ static int iTCO_wdt_probe(struct platform_device *dev)
                p->gcs_pmc_res = platform_get_resource(dev,
                                                       IORESOURCE_MEM,
                                                       ICH_RES_MEM_GCS_PMC);
-
-               if (!p->gcs_pmc_res)
-                       return -ENODEV;
-
-               if (!request_mem_region(p->gcs_pmc_res->start,
-                                       resource_size(p->gcs_pmc_res),
-                                       dev->name))
-                       return -EBUSY;
-
-               p->gcs_pmc = ioremap(p->gcs_pmc_res->start,
-                                    resource_size(p->gcs_pmc_res));
-               if (!p->gcs_pmc) {
-                       ret = -EIO;
-                       goto unreg_gcs_pmc;
-               }
+               p->gcs_pmc = devm_ioremap_resource(&dev->dev, p->gcs_pmc_res);
+               if (IS_ERR(p->gcs_pmc))
+                       return PTR_ERR(p->gcs_pmc);
        }
 
        /* Check chipset's NO_REBOOT bit */
        if (iTCO_wdt_unset_NO_REBOOT_bit(p) &&
            iTCO_vendor_check_noreboot_on()) {
                pr_info("unable to reset NO_REBOOT flag, device disabled by hardware/BIOS\n");
-               ret = -ENODEV;  /* Cannot reset NO_REBOOT bit */
-               goto unmap_gcs_pmc;
+               return -ENODEV; /* Cannot reset NO_REBOOT bit */
        }
 
        /* Set the NO_REBOOT bit to prevent later reboots, just for sure */
        iTCO_wdt_set_NO_REBOOT_bit(p);
 
        /* The TCO logic uses the TCO_EN bit in the SMI_EN register */
-       if (!request_region(p->smi_res->start,
-                           resource_size(p->smi_res), dev->name)) {
+       if (!devm_request_region(&dev->dev, p->smi_res->start,
+                                resource_size(p->smi_res),
+                                dev->name)) {
                pr_err("I/O address 0x%04llx already in use, device disabled\n",
                       (u64)SMI_EN(p));
-               ret = -EBUSY;
-               goto unmap_gcs_pmc;
+               return -EBUSY;
        }
        if (turn_SMI_watchdog_clear_off >= p->iTCO_version) {
                /*

@@ -504,12 +470,12 @@ static int iTCO_wdt_probe(struct platform_device *dev)
                outl(val32, SMI_EN(p));
        }
 
-       if (!request_region(p->tco_res->start,
-                           resource_size(p->tco_res), dev->name)) {
+       if (!devm_request_region(&dev->dev, p->tco_res->start,
+                                resource_size(p->tco_res),
+                                dev->name)) {
                pr_err("I/O address 0x%04llx already in use, device disabled\n",
                       (u64)TCOBASE(p));
-               ret = -EBUSY;
-               goto unreg_smi;
+               return -EBUSY;
        }
 
        pr_info("Found a %s TCO device (Version=%d, TCOBASE=0x%04llx)\n",

@@ -555,37 +521,25 @@ static int iTCO_wdt_probe(struct platform_device *dev)
                        WATCHDOG_TIMEOUT);
        }
 
-       ret = watchdog_register_device(&p->wddev);
+       ret = devm_watchdog_register_device(&dev->dev, &p->wddev);
        if (ret != 0) {
                pr_err("cannot register watchdog device (err=%d)\n", ret);
-               goto unreg_tco;
+               return ret;
        }
 
        pr_info("initialized. heartbeat=%d sec (nowayout=%d)\n",
                heartbeat, nowayout);
 
        return 0;
-
-unreg_tco:
-       release_region(p->tco_res->start, resource_size(p->tco_res));
-unreg_smi:
-       release_region(p->smi_res->start, resource_size(p->smi_res));
-unmap_gcs_pmc:
-       if (p->iTCO_version >= 2)
-               iounmap(p->gcs_pmc);
-unreg_gcs_pmc:
-       if (p->iTCO_version >= 2)
-               release_mem_region(p->gcs_pmc_res->start,
-                                  resource_size(p->gcs_pmc_res));
-       return ret;
 }
 
 static int iTCO_wdt_remove(struct platform_device *dev)
 {
        struct iTCO_wdt_private *p = platform_get_drvdata(dev);
 
-       if (p->tco_res || p->smi_res)
-               iTCO_wdt_cleanup(p);
+       /* Stop the timer before we leave */
+       if (!nowayout)
+               iTCO_wdt_stop(&p->wddev);
 
        return 0;
 }
--

2.7.4

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Guenter Roeck

author maintainer

2017-01-03 23:39 UTC #3 lore raw

Re: [PATCH 2/4] watchdog: iTCO_wdt: Use device managed resources

Quoted 10 lines · Andy Shevchenko

Quoted 4 lines · Guenter Roeck
Using device managed resources simplifies error handling and cleanup, and to reduce the likelyhood of errors.
Signed-off-by Guenter Roeck <[email protected]>
Reviewed-by Andy Shevchenko <[email protected]>
Does it make sense to convert to dev_err() at some point?

Sounds like an idea. Let me play with it.

Thanks,
Guenter

Quoted 159 lines

Quoted 149 lines

---
 drivers/watchdog/iTCO_wdt.c | 80 ++++++++++-----------------------------------
 1 file changed, 17 insertions(+), 63 deletions(-)

drivers/watchdog/iTCO_wdt.c +17 ~~-64~~

index a35a9164ccd0..eed1dee6de19 100644
@@ -401,27 +401,6 @@ static const struct watchdog_ops iTCO_wdt_ops = {
  *     Init & exit routines
  */
 
-static void iTCO_wdt_cleanup(struct iTCO_wdt_private *p)
-{
-       /* Stop the timer before we leave */
-       if (!nowayout)
-               iTCO_wdt_stop(&p->wddev);
-
-       /* Deregister */
-       watchdog_unregister_device(&p->wddev);
-
-       /* release resources */
-       release_region(p->tco_res->start,
-                       resource_size(p->tco_res));
-       release_region(p->smi_res->start,
-                       resource_size(p->smi_res));
-       if (p->iTCO_version >= 2) {
-               iounmap(p->gcs_pmc);
-               release_mem_region(p->gcs_pmc_res->start,
-                               resource_size(p->gcs_pmc_res));
-       }
-}
-
 static int iTCO_wdt_probe(struct platform_device *dev)
 {
        struct itco_wdt_platform_data *pdata = dev_get_platdata(&dev->dev);

@@ -458,41 +437,28 @@ static int iTCO_wdt_probe(struct platform_device *dev)
                p->gcs_pmc_res = platform_get_resource(dev,
                                                       IORESOURCE_MEM,
                                                       ICH_RES_MEM_GCS_PMC);
-
-               if (!p->gcs_pmc_res)
-                       return -ENODEV;
-
-               if (!request_mem_region(p->gcs_pmc_res->start,
-                                       resource_size(p->gcs_pmc_res),
-                                       dev->name))
-                       return -EBUSY;
-
-               p->gcs_pmc = ioremap(p->gcs_pmc_res->start,
-                                    resource_size(p->gcs_pmc_res));
-               if (!p->gcs_pmc) {
-                       ret = -EIO;
-                       goto unreg_gcs_pmc;
-               }
+               p->gcs_pmc = devm_ioremap_resource(&dev->dev, p->gcs_pmc_res);
+               if (IS_ERR(p->gcs_pmc))
+                       return PTR_ERR(p->gcs_pmc);
        }
 
        /* Check chipset's NO_REBOOT bit */
        if (iTCO_wdt_unset_NO_REBOOT_bit(p) &&
            iTCO_vendor_check_noreboot_on()) {
                pr_info("unable to reset NO_REBOOT flag, device disabled by hardware/BIOS\n");
-               ret = -ENODEV;  /* Cannot reset NO_REBOOT bit */
-               goto unmap_gcs_pmc;
+               return -ENODEV; /* Cannot reset NO_REBOOT bit */
        }
 
        /* Set the NO_REBOOT bit to prevent later reboots, just for sure */
        iTCO_wdt_set_NO_REBOOT_bit(p);
 
        /* The TCO logic uses the TCO_EN bit in the SMI_EN register */
-       if (!request_region(p->smi_res->start,
-                           resource_size(p->smi_res), dev->name)) {
+       if (!devm_request_region(&dev->dev, p->smi_res->start,
+                                resource_size(p->smi_res),
+                                dev->name)) {
                pr_err("I/O address 0x%04llx already in use, device disabled\n",
                       (u64)SMI_EN(p));
-               ret = -EBUSY;
-               goto unmap_gcs_pmc;
+               return -EBUSY;
        }
        if (turn_SMI_watchdog_clear_off >= p->iTCO_version) {
                /*

@@ -504,12 +470,12 @@ static int iTCO_wdt_probe(struct platform_device *dev)
                outl(val32, SMI_EN(p));
        }
 
-       if (!request_region(p->tco_res->start,
-                           resource_size(p->tco_res), dev->name)) {
+       if (!devm_request_region(&dev->dev, p->tco_res->start,
+                                resource_size(p->tco_res),
+                                dev->name)) {
                pr_err("I/O address 0x%04llx already in use, device disabled\n",
                       (u64)TCOBASE(p));
-               ret = -EBUSY;
-               goto unreg_smi;
+               return -EBUSY;
        }
 
        pr_info("Found a %s TCO device (Version=%d, TCOBASE=0x%04llx)\n",

@@ -555,37 +521,25 @@ static int iTCO_wdt_probe(struct platform_device *dev)
                        WATCHDOG_TIMEOUT);
        }
 
-       ret = watchdog_register_device(&p->wddev);
+       ret = devm_watchdog_register_device(&dev->dev, &p->wddev);
        if (ret != 0) {
                pr_err("cannot register watchdog device (err=%d)\n", ret);
-               goto unreg_tco;
+               return ret;
        }
 
        pr_info("initialized. heartbeat=%d sec (nowayout=%d)\n",
                heartbeat, nowayout);
 
        return 0;
-
-unreg_tco:
-       release_region(p->tco_res->start, resource_size(p->tco_res));
-unreg_smi:
-       release_region(p->smi_res->start, resource_size(p->smi_res));
-unmap_gcs_pmc:
-       if (p->iTCO_version >= 2)
-               iounmap(p->gcs_pmc);
-unreg_gcs_pmc:
-       if (p->iTCO_version >= 2)
-               release_mem_region(p->gcs_pmc_res->start,
-                                  resource_size(p->gcs_pmc_res));
-       return ret;
 }
 
 static int iTCO_wdt_remove(struct platform_device *dev)
 {
        struct iTCO_wdt_private *p = platform_get_drvdata(dev);
 
-       if (p->tco_res || p->smi_res)
-               iTCO_wdt_cleanup(p);
+       /* Stop the timer before we leave */
+       if (!nowayout)
+               iTCO_wdt_stop(&p->wddev);
 
        return 0;
 }
--

2.7.4

Signature · 6 lines

With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Guenter Roeck

author maintainer

2017-01-04 02:46 UTC #4 lore raw

Re: [PATCH 2/4] watchdog: iTCO_wdt: Use device managed resources

In reply to Andy Shevchenko

Quoted 10 lines · Andy Shevchenko

Quoted 4 lines · Guenter Roeck
Using device managed resources simplifies error handling and cleanup, and to reduce the likelyhood of errors.
Signed-off-by Guenter Roeck <[email protected]>
Reviewed-by Andy Shevchenko <[email protected]>
Does it make sense to convert to dev_err() at some point?

I tried. It gives me

[59886.759774] iTCO_vendor_support: vendor-support=0
[59886.760452] iTCO_wdt: Intel TCO WatchDog Timer Driver v1.11
[59886.760540] iTCO_wdt iTCO_wdt.0.auto: Found a 9 Series TCO device (Version=2, TCOBASE=0x1860)
[59886.761849] iTCO_wdt iTCO_wdt.0.auto: initialized. heartbeat=30 sec (nowayout=0)

which I don't really like too much. Let's stick with pr_{info,err}.

Thanks,
Guenter

Guenter Roeck

author maintainer

2017-01-03 14:39 UTC #5 lore raw

PATCH 3/4 watchdog: iTCO_wdt: Use pdev for platform device and pci_dev for pci device

In reply to Guenter Roeck

Use pdev for struct platform_device, pcidev for struct pci_dev, and dev for struct device variables to improve consistency.

Remove 'struct platform_device *dev;' from struct iTCO_wdt_private since it was unused.

Signed-off-by Guenter Roeck <[email protected]>

---
 drivers/watchdog/iTCO_wdt.c | 53 ++++++++++++++++++++++-----------------------
 1 file changed, 26 insertions(+), 27 deletions(-)

drivers/watchdog/iTCO_wdt.c +26 ~~-27~~

index eed1dee6de19..ad29ae03a30b 100644
@@ -102,9 +102,8 @@ struct iTCO_wdt_private {
 	unsigned long __iomem *gcs_pmc;
 	/* the lock for io operations */
 	spinlock_t io_lock;
-	struct platform_device *dev;
 	/* the PCI-device */
-	struct pci_dev *pdev;
+	struct pci_dev *pcidev;
 	/* whether or not the watchdog has been suspended */
 	bool suspended;
 };

@@ -181,9 +180,9 @@ static void iTCO_wdt_set_NO_REBOOT_bit(struct iTCO_wdt_private *p)
 		val32 |= no_reboot_bit(p);
 		writel(val32, p->gcs_pmc);
 	} else if (p->iTCO_version == 1) {
-		pci_read_config_dword(p->pdev, 0xd4, &val32);
+		pci_read_config_dword(p->pcidev, 0xd4, &val32);
 		val32 |= no_reboot_bit(p);
-		pci_write_config_dword(p->pdev, 0xd4, val32);
+		pci_write_config_dword(p->pcidev, 0xd4, val32);
 	}
 }

@@ -200,11 +199,11 @@ static int iTCO_wdt_unset_NO_REBOOT_bit(struct iTCO_wdt_private *p)
 
 		val32 = readl(p->gcs_pmc);
 	} else if (p->iTCO_version == 1) {
-		pci_read_config_dword(p->pdev, 0xd4, &val32);
+		pci_read_config_dword(p->pcidev, 0xd4, &val32);
 		val32 &= ~enable_bit;
-		pci_write_config_dword(p->pdev, 0xd4, val32);
+		pci_write_config_dword(p->pcidev, 0xd4, val32);
 
-		pci_read_config_dword(p->pdev, 0xd4, &val32);
+		pci_read_config_dword(p->pcidev, 0xd4, &val32);
 	}
 
 	if (val32 & enable_bit)

@@ -401,9 +400,10 @@ static const struct watchdog_ops iTCO_wdt_ops = {
  *	Init & exit routines
  */
 
-static int iTCO_wdt_probe(struct platform_device *dev)
+static int iTCO_wdt_probe(struct platform_device *pdev)
 {
-	struct itco_wdt_platform_data *pdata = dev_get_platdata(&dev->dev);
+	struct device *dev = &pdev->dev;
+	struct itco_wdt_platform_data *pdata = dev_get_platdata(dev);
 	struct iTCO_wdt_private *p;
 	unsigned long val32;
 	int ret;

@@ -411,33 +411,32 @@ static int iTCO_wdt_probe(struct platform_device *dev)
 	if (!pdata)
 		return -ENODEV;
 
-	p = devm_kzalloc(&dev->dev, sizeof(*p), GFP_KERNEL);
+	p = devm_kzalloc(dev, sizeof(*p), GFP_KERNEL);
 	if (!p)
 		return -ENOMEM;
 
 	spin_lock_init(&p->io_lock);
 
-	p->tco_res = platform_get_resource(dev, IORESOURCE_IO, ICH_RES_IO_TCO);
+	p->tco_res = platform_get_resource(pdev, IORESOURCE_IO, ICH_RES_IO_TCO);
 	if (!p->tco_res)
 		return -ENODEV;
 
-	p->smi_res = platform_get_resource(dev, IORESOURCE_IO, ICH_RES_IO_SMI);
+	p->smi_res = platform_get_resource(pdev, IORESOURCE_IO, ICH_RES_IO_SMI);
 	if (!p->smi_res)
 		return -ENODEV;
 
 	p->iTCO_version = pdata->version;
-	p->dev = dev;
-	p->pdev = to_pci_dev(dev->dev.parent);
+	p->pcidev = to_pci_dev(dev->parent);
 
 	/*
 	 * Get the Memory-Mapped GCS or PMC register, we need it for the
 	 * NO_REBOOT flag (TCO v2 and v3).
 	 */
 	if (p->iTCO_version >= 2) {
-		p->gcs_pmc_res = platform_get_resource(dev,
+		p->gcs_pmc_res = platform_get_resource(pdev,
 						       IORESOURCE_MEM,
 						       ICH_RES_MEM_GCS_PMC);
-		p->gcs_pmc = devm_ioremap_resource(&dev->dev, p->gcs_pmc_res);
+		p->gcs_pmc = devm_ioremap_resource(dev, p->gcs_pmc_res);
 		if (IS_ERR(p->gcs_pmc))
 			return PTR_ERR(p->gcs_pmc);
 	}

@@ -453,9 +452,9 @@ static int iTCO_wdt_probe(struct platform_device *dev)
 	iTCO_wdt_set_NO_REBOOT_bit(p);
 
 	/* The TCO logic uses the TCO_EN bit in the SMI_EN register */
-	if (!devm_request_region(&dev->dev, p->smi_res->start,
+	if (!devm_request_region(dev, p->smi_res->start,
 				 resource_size(p->smi_res),
-				 dev->name)) {
+				 pdev->name)) {
 		pr_err("I/O address 0x%04llx already in use, device disabled\n",
 		       (u64)SMI_EN(p));
 		return -EBUSY;

@@ -470,9 +469,9 @@ static int iTCO_wdt_probe(struct platform_device *dev)
 		outl(val32, SMI_EN(p));
 	}
 
-	if (!devm_request_region(&dev->dev, p->tco_res->start,
+	if (!devm_request_region(dev, p->tco_res->start,
 				 resource_size(p->tco_res),
-				 dev->name)) {
+				 pdev->name)) {
 		pr_err("I/O address 0x%04llx already in use, device disabled\n",
 		       (u64)TCOBASE(p));
 		return -EBUSY;

@@ -505,10 +504,10 @@ static int iTCO_wdt_probe(struct platform_device *dev)
 	p->wddev.bootstatus = 0;
 	p->wddev.timeout = WATCHDOG_TIMEOUT;
 	watchdog_set_nowayout(&p->wddev, nowayout);
-	p->wddev.parent = &dev->dev;
+	p->wddev.parent = dev;
 
 	watchdog_set_drvdata(&p->wddev, p);
-	platform_set_drvdata(dev, p);
+	platform_set_drvdata(pdev, p);
 
 	/* Make sure the watchdog is not running */
 	iTCO_wdt_stop(&p->wddev);

@@ -521,7 +520,7 @@ static int iTCO_wdt_probe(struct platform_device *dev)
 			WATCHDOG_TIMEOUT);
 	}
 
-	ret = devm_watchdog_register_device(&dev->dev, &p->wddev);
+	ret = devm_watchdog_register_device(dev, &p->wddev);
 	if (ret != 0) {
 		pr_err("cannot register watchdog device (err=%d)\n", ret);
 		return ret;

@@ -533,9 +532,9 @@ static int iTCO_wdt_probe(struct platform_device *dev)
 	return 0;
 }
 
-static int iTCO_wdt_remove(struct platform_device *dev)
+static int iTCO_wdt_remove(struct platform_device *pdev)
 {
-	struct iTCO_wdt_private *p = platform_get_drvdata(dev);
+	struct iTCO_wdt_private *p = platform_get_drvdata(pdev);
 
 	/* Stop the timer before we leave */
 	if (!nowayout)

@@ -544,9 +543,9 @@ static int iTCO_wdt_remove(struct platform_device *dev)
 	return 0;
 }
 
-static void iTCO_wdt_shutdown(struct platform_device *dev)
+static void iTCO_wdt_shutdown(struct platform_device *pdev)
 {
-	struct iTCO_wdt_private *p = platform_get_drvdata(dev);
+	struct iTCO_wdt_private *p = platform_get_drvdata(pdev);
 
 	iTCO_wdt_stop(&p->wddev);
 }

Signature · 1 line

2.7.4

Andy Shevchenko

Reviewed-by on v1 3/4

2017-01-03 22:39 UTC #6 lore raw

Re: [PATCH 3/4] watchdog: iTCO_wdt: Use pdev for platform device and pci_dev for pci device

Quoted 5 lines · Guenter Roeck

Use pdev for struct platform_device, pcidev for struct pci_dev, and dev for struct device variables to improve consistency.
Remove 'struct platform_device *dev;' from struct iTCO_wdt_private since it was unused.

Would pci_dev work?

In any case

Reviewed-by Andy Shevchenko <[email protected]>

Quoted 174 lines

Signed-off-by Guenter Roeck <[email protected]>

---
 drivers/watchdog/iTCO_wdt.c | 53 ++++++++++++++++++++++-----------------------
 1 file changed, 26 insertions(+), 27 deletions(-)

drivers/watchdog/iTCO_wdt.c +26 ~~-28~~

index eed1dee6de19..ad29ae03a30b 100644
@@ -102,9 +102,8 @@ struct iTCO_wdt_private {
        unsigned long __iomem *gcs_pmc;
        /* the lock for io operations */
        spinlock_t io_lock;
-       struct platform_device *dev;
        /* the PCI-device */
-       struct pci_dev *pdev;
+       struct pci_dev *pcidev;
        /* whether or not the watchdog has been suspended */
        bool suspended;
 };

@@ -181,9 +180,9 @@ static void iTCO_wdt_set_NO_REBOOT_bit(struct iTCO_wdt_private *p)
                val32 |= no_reboot_bit(p);
                writel(val32, p->gcs_pmc);
        } else if (p->iTCO_version == 1) {
-               pci_read_config_dword(p->pdev, 0xd4, &val32);
+               pci_read_config_dword(p->pcidev, 0xd4, &val32);
                val32 |= no_reboot_bit(p);
-               pci_write_config_dword(p->pdev, 0xd4, val32);
+               pci_write_config_dword(p->pcidev, 0xd4, val32);
        }
 }

@@ -200,11 +199,11 @@ static int iTCO_wdt_unset_NO_REBOOT_bit(struct iTCO_wdt_private *p)
 
                val32 = readl(p->gcs_pmc);
        } else if (p->iTCO_version == 1) {
-               pci_read_config_dword(p->pdev, 0xd4, &val32);
+               pci_read_config_dword(p->pcidev, 0xd4, &val32);
                val32 &= ~enable_bit;
-               pci_write_config_dword(p->pdev, 0xd4, val32);
+               pci_write_config_dword(p->pcidev, 0xd4, val32);
 
-               pci_read_config_dword(p->pdev, 0xd4, &val32);
+               pci_read_config_dword(p->pcidev, 0xd4, &val32);
        }
 
        if (val32 & enable_bit)

@@ -401,9 +400,10 @@ static const struct watchdog_ops iTCO_wdt_ops = {
  *     Init & exit routines
  */
 
-static int iTCO_wdt_probe(struct platform_device *dev)
+static int iTCO_wdt_probe(struct platform_device *pdev)
 {
-       struct itco_wdt_platform_data *pdata = dev_get_platdata(&dev->dev);
+       struct device *dev = &pdev->dev;
+       struct itco_wdt_platform_data *pdata = dev_get_platdata(dev);
        struct iTCO_wdt_private *p;
        unsigned long val32;
        int ret;

@@ -411,33 +411,32 @@ static int iTCO_wdt_probe(struct platform_device *dev)
        if (!pdata)
                return -ENODEV;
 
-       p = devm_kzalloc(&dev->dev, sizeof(*p), GFP_KERNEL);
+       p = devm_kzalloc(dev, sizeof(*p), GFP_KERNEL);
        if (!p)
                return -ENOMEM;
 
        spin_lock_init(&p->io_lock);
 
-       p->tco_res = platform_get_resource(dev, IORESOURCE_IO, ICH_RES_IO_TCO);
+       p->tco_res = platform_get_resource(pdev, IORESOURCE_IO, ICH_RES_IO_TCO);
        if (!p->tco_res)
                return -ENODEV;
 
-       p->smi_res = platform_get_resource(dev, IORESOURCE_IO, ICH_RES_IO_SMI);
+       p->smi_res = platform_get_resource(pdev, IORESOURCE_IO, ICH_RES_IO_SMI);
        if (!p->smi_res)
                return -ENODEV;
 
        p->iTCO_version = pdata->version;
-       p->dev = dev;
-       p->pdev = to_pci_dev(dev->dev.parent);
+       p->pcidev = to_pci_dev(dev->parent);
 
        /*
         * Get the Memory-Mapped GCS or PMC register, we need it for the
         * NO_REBOOT flag (TCO v2 and v3).
         */
        if (p->iTCO_version >= 2) {
-               p->gcs_pmc_res = platform_get_resource(dev,
+               p->gcs_pmc_res = platform_get_resource(pdev,
                                                       IORESOURCE_MEM,
                                                       ICH_RES_MEM_GCS_PMC);
-               p->gcs_pmc = devm_ioremap_resource(&dev->dev, p->gcs_pmc_res);
+               p->gcs_pmc = devm_ioremap_resource(dev, p->gcs_pmc_res);
                if (IS_ERR(p->gcs_pmc))
                        return PTR_ERR(p->gcs_pmc);
        }

@@ -453,9 +452,9 @@ static int iTCO_wdt_probe(struct platform_device *dev)
        iTCO_wdt_set_NO_REBOOT_bit(p);
 
        /* The TCO logic uses the TCO_EN bit in the SMI_EN register */
-       if (!devm_request_region(&dev->dev, p->smi_res->start,
+       if (!devm_request_region(dev, p->smi_res->start,
                                 resource_size(p->smi_res),
-                                dev->name)) {
+                                pdev->name)) {
                pr_err("I/O address 0x%04llx already in use, device disabled\n",
                       (u64)SMI_EN(p));
                return -EBUSY;

@@ -470,9 +469,9 @@ static int iTCO_wdt_probe(struct platform_device *dev)
                outl(val32, SMI_EN(p));
        }
 
-       if (!devm_request_region(&dev->dev, p->tco_res->start,
+       if (!devm_request_region(dev, p->tco_res->start,
                                 resource_size(p->tco_res),
-                                dev->name)) {
+                                pdev->name)) {
                pr_err("I/O address 0x%04llx already in use, device disabled\n",
                       (u64)TCOBASE(p));
                return -EBUSY;

@@ -505,10 +504,10 @@ static int iTCO_wdt_probe(struct platform_device *dev)
        p->wddev.bootstatus = 0;
        p->wddev.timeout = WATCHDOG_TIMEOUT;
        watchdog_set_nowayout(&p->wddev, nowayout);
-       p->wddev.parent = &dev->dev;
+       p->wddev.parent = dev;
 
        watchdog_set_drvdata(&p->wddev, p);
-       platform_set_drvdata(dev, p);
+       platform_set_drvdata(pdev, p);
 
        /* Make sure the watchdog is not running */
        iTCO_wdt_stop(&p->wddev);

@@ -521,7 +520,7 @@ static int iTCO_wdt_probe(struct platform_device *dev)
                        WATCHDOG_TIMEOUT);
        }
 
-       ret = devm_watchdog_register_device(&dev->dev, &p->wddev);
+       ret = devm_watchdog_register_device(dev, &p->wddev);
        if (ret != 0) {
                pr_err("cannot register watchdog device (err=%d)\n", ret);
                return ret;

@@ -533,9 +532,9 @@ static int iTCO_wdt_probe(struct platform_device *dev)
        return 0;
 }
 
-static int iTCO_wdt_remove(struct platform_device *dev)
+static int iTCO_wdt_remove(struct platform_device *pdev)
 {
-       struct iTCO_wdt_private *p = platform_get_drvdata(dev);
+       struct iTCO_wdt_private *p = platform_get_drvdata(pdev);
 
        /* Stop the timer before we leave */
        if (!nowayout)

@@ -544,9 +543,9 @@ static int iTCO_wdt_remove(struct platform_device *dev)
        return 0;
 }
 
-static void iTCO_wdt_shutdown(struct platform_device *dev)
+static void iTCO_wdt_shutdown(struct platform_device *pdev)
 {
-       struct iTCO_wdt_private *p = platform_get_drvdata(dev);
+       struct iTCO_wdt_private *p = platform_get_drvdata(pdev);
 
        iTCO_wdt_stop(&p->wddev);
 }
--

2.7.4

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Guenter Roeck

author maintainer

2017-01-03 23:40 UTC #7 lore raw

Re: [PATCH 3/4] watchdog: iTCO_wdt: Use pdev for platform device and pci_dev for pci device

Quoted 9 lines · Andy Shevchenko

Quoted 5 lines · Guenter Roeck
Use pdev for struct platform_device, pcidev for struct pci_dev, and dev for struct device variables to improve consistency.
Remove 'struct platform_device *dev;' from struct iTCO_wdt_private since it was unused.
Would pci_dev work?

Sure, or maybe just 'pci'. Any preference ?

Thanks,
Guenter

Quoted 187 lines

In any case

Reviewed-by Andy Shevchenko <[email protected]>

Quoted 174 lines

Signed-off-by Guenter Roeck <[email protected]>

---
 drivers/watchdog/iTCO_wdt.c | 53 ++++++++++++++++++++++-----------------------
 1 file changed, 26 insertions(+), 27 deletions(-)

drivers/watchdog/iTCO_wdt.c +26 ~~-28~~

index eed1dee6de19..ad29ae03a30b 100644
@@ -102,9 +102,8 @@ struct iTCO_wdt_private {
        unsigned long __iomem *gcs_pmc;
        /* the lock for io operations */
        spinlock_t io_lock;
-       struct platform_device *dev;
        /* the PCI-device */
-       struct pci_dev *pdev;
+       struct pci_dev *pcidev;
        /* whether or not the watchdog has been suspended */
        bool suspended;
 };

@@ -181,9 +180,9 @@ static void iTCO_wdt_set_NO_REBOOT_bit(struct iTCO_wdt_private *p)
                val32 |= no_reboot_bit(p);
                writel(val32, p->gcs_pmc);
        } else if (p->iTCO_version == 1) {
-               pci_read_config_dword(p->pdev, 0xd4, &val32);
+               pci_read_config_dword(p->pcidev, 0xd4, &val32);
                val32 |= no_reboot_bit(p);
-               pci_write_config_dword(p->pdev, 0xd4, val32);
+               pci_write_config_dword(p->pcidev, 0xd4, val32);
        }
 }

@@ -200,11 +199,11 @@ static int iTCO_wdt_unset_NO_REBOOT_bit(struct iTCO_wdt_private *p)
 
                val32 = readl(p->gcs_pmc);
        } else if (p->iTCO_version == 1) {
-               pci_read_config_dword(p->pdev, 0xd4, &val32);
+               pci_read_config_dword(p->pcidev, 0xd4, &val32);
                val32 &= ~enable_bit;
-               pci_write_config_dword(p->pdev, 0xd4, val32);
+               pci_write_config_dword(p->pcidev, 0xd4, val32);
 
-               pci_read_config_dword(p->pdev, 0xd4, &val32);
+               pci_read_config_dword(p->pcidev, 0xd4, &val32);
        }
 
        if (val32 & enable_bit)

@@ -401,9 +400,10 @@ static const struct watchdog_ops iTCO_wdt_ops = {
  *     Init & exit routines
  */
 
-static int iTCO_wdt_probe(struct platform_device *dev)
+static int iTCO_wdt_probe(struct platform_device *pdev)
 {
-       struct itco_wdt_platform_data *pdata = dev_get_platdata(&dev->dev);
+       struct device *dev = &pdev->dev;
+       struct itco_wdt_platform_data *pdata = dev_get_platdata(dev);
        struct iTCO_wdt_private *p;
        unsigned long val32;
        int ret;

@@ -411,33 +411,32 @@ static int iTCO_wdt_probe(struct platform_device *dev)
        if (!pdata)
                return -ENODEV;
 
-       p = devm_kzalloc(&dev->dev, sizeof(*p), GFP_KERNEL);
+       p = devm_kzalloc(dev, sizeof(*p), GFP_KERNEL);
        if (!p)
                return -ENOMEM;
 
        spin_lock_init(&p->io_lock);
 
-       p->tco_res = platform_get_resource(dev, IORESOURCE_IO, ICH_RES_IO_TCO);
+       p->tco_res = platform_get_resource(pdev, IORESOURCE_IO, ICH_RES_IO_TCO);
        if (!p->tco_res)
                return -ENODEV;
 
-       p->smi_res = platform_get_resource(dev, IORESOURCE_IO, ICH_RES_IO_SMI);
+       p->smi_res = platform_get_resource(pdev, IORESOURCE_IO, ICH_RES_IO_SMI);
        if (!p->smi_res)
                return -ENODEV;
 
        p->iTCO_version = pdata->version;
-       p->dev = dev;
-       p->pdev = to_pci_dev(dev->dev.parent);
+       p->pcidev = to_pci_dev(dev->parent);
 
        /*
         * Get the Memory-Mapped GCS or PMC register, we need it for the
         * NO_REBOOT flag (TCO v2 and v3).
         */
        if (p->iTCO_version >= 2) {
-               p->gcs_pmc_res = platform_get_resource(dev,
+               p->gcs_pmc_res = platform_get_resource(pdev,
                                                       IORESOURCE_MEM,
                                                       ICH_RES_MEM_GCS_PMC);
-               p->gcs_pmc = devm_ioremap_resource(&dev->dev, p->gcs_pmc_res);
+               p->gcs_pmc = devm_ioremap_resource(dev, p->gcs_pmc_res);
                if (IS_ERR(p->gcs_pmc))
                        return PTR_ERR(p->gcs_pmc);
        }

@@ -453,9 +452,9 @@ static int iTCO_wdt_probe(struct platform_device *dev)
        iTCO_wdt_set_NO_REBOOT_bit(p);
 
        /* The TCO logic uses the TCO_EN bit in the SMI_EN register */
-       if (!devm_request_region(&dev->dev, p->smi_res->start,
+       if (!devm_request_region(dev, p->smi_res->start,
                                 resource_size(p->smi_res),
-                                dev->name)) {
+                                pdev->name)) {
                pr_err("I/O address 0x%04llx already in use, device disabled\n",
                       (u64)SMI_EN(p));
                return -EBUSY;

@@ -470,9 +469,9 @@ static int iTCO_wdt_probe(struct platform_device *dev)
                outl(val32, SMI_EN(p));
        }
 
-       if (!devm_request_region(&dev->dev, p->tco_res->start,
+       if (!devm_request_region(dev, p->tco_res->start,
                                 resource_size(p->tco_res),
-                                dev->name)) {
+                                pdev->name)) {
                pr_err("I/O address 0x%04llx already in use, device disabled\n",
                       (u64)TCOBASE(p));
                return -EBUSY;

@@ -505,10 +504,10 @@ static int iTCO_wdt_probe(struct platform_device *dev)
        p->wddev.bootstatus = 0;
        p->wddev.timeout = WATCHDOG_TIMEOUT;
        watchdog_set_nowayout(&p->wddev, nowayout);
-       p->wddev.parent = &dev->dev;
+       p->wddev.parent = dev;
 
        watchdog_set_drvdata(&p->wddev, p);
-       platform_set_drvdata(dev, p);
+       platform_set_drvdata(pdev, p);
 
        /* Make sure the watchdog is not running */
        iTCO_wdt_stop(&p->wddev);

@@ -521,7 +520,7 @@ static int iTCO_wdt_probe(struct platform_device *dev)
                        WATCHDOG_TIMEOUT);
        }
 
-       ret = devm_watchdog_register_device(&dev->dev, &p->wddev);
+       ret = devm_watchdog_register_device(dev, &p->wddev);
        if (ret != 0) {
                pr_err("cannot register watchdog device (err=%d)\n", ret);
                return ret;

@@ -533,9 +532,9 @@ static int iTCO_wdt_probe(struct platform_device *dev)
        return 0;
 }
 
-static int iTCO_wdt_remove(struct platform_device *dev)
+static int iTCO_wdt_remove(struct platform_device *pdev)
 {
-       struct iTCO_wdt_private *p = platform_get_drvdata(dev);
+       struct iTCO_wdt_private *p = platform_get_drvdata(pdev);
 
        /* Stop the timer before we leave */
        if (!nowayout)

@@ -544,9 +543,9 @@ static int iTCO_wdt_remove(struct platform_device *dev)
        return 0;
 }
 
-static void iTCO_wdt_shutdown(struct platform_device *dev)
+static void iTCO_wdt_shutdown(struct platform_device *pdev)
 {
-       struct iTCO_wdt_private *p = platform_get_drvdata(dev);
+       struct iTCO_wdt_private *p = platform_get_drvdata(pdev);
 
        iTCO_wdt_stop(&p->wddev);
 }
--

2.7.4

Signature · 6 lines

With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Andy Shevchenko

2017-01-03 23:50 UTC #9 lore raw

Re: [PATCH 3/4] watchdog: iTCO_wdt: Use pdev for platform device and pci_dev for pci device

Quoted 9 lines · Andy Shevchenko

Quoted 8 lines · Guenter Roeck
Quoted 7 lines · Andy Shevchenko
Quoted 5 lines · Guenter Roeck
Use pdev for struct platform_device, pcidev for struct pci_dev, and dev for struct device variables to improve consistency.
Remove 'struct platform_device *dev;' from struct iTCO_wdt_private since it was unused.

Quoted 6 lines

Quoted 3 lines
Quoted 2 lines
Would pci_dev work?
Sure, or maybe just 'pci'. Any preference ?
Just slightly prefer pci_dev over others (pcidev, pci), but do not insist. Up to you.

Or even

struct device *dev;
...
struct pci_dev *pci_dev = to_pci_dev(dev);

in places where it's guaranteed to be PCI.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Guenter Roeck

author maintainer

2017-01-04 01:39 UTC #10 lore raw

Re: [PATCH 3/4] watchdog: iTCO_wdt: Use pdev for platform device and pci_dev for pci device

Quoted 15 lines · Andy Shevchenko

Quoted 13 lines · Andy Shevchenko
Quoted 11 lines · Guenter Roeck
Quoted 9 lines · Andy Shevchenko
Quoted 5 lines · Guenter Roeck
Use pdev for struct platform_device, pcidev for struct pci_dev, and dev for struct device variables to improve consistency.
Remove 'struct platform_device *dev;' from struct iTCO_wdt_private since it was unused.
Would pci_dev work?
Sure, or maybe just 'pci'. Any preference ?

Quoted 9 lines

Quoted 2 lines
Just slightly prefer pci_dev over others (pcidev, pci), but do not insist. Up to you.
Or even
struct device *dev;
...
struct pci_dev *pci_dev = to_pci_dev(dev);

'dev' would have to be a pointer to the parent device. If I consider using dev_{info,err}, I'll need 'dev' in struct iTCO_wdt_private. I think I'll stick with pci_dev.

Thanks,
Guenter

Guenter Roeck

author maintainer

2017-01-03 14:39 UTC #11 lore raw

PATCH 4/4 watchdog: iTCO_wdt: Simplify module init function

In reply to Guenter Roeck

The 'ret' variable in iTCO_wdt_init_module() does not add any value; drop it.

Signed-off-by Guenter Roeck <[email protected]>

---
 drivers/watchdog/iTCO_wdt.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

drivers/watchdog/iTCO_wdt.c +1 -7

index ad29ae03a30b..fc7712112412 100644
@@ -612,15 +612,9 @@ static struct platform_driver iTCO_wdt_driver = {
 
 static int __init iTCO_wdt_init_module(void)
 {
-	int err;
-
 	pr_info("Intel TCO WatchDog Timer Driver v%s\n", DRV_VERSION);
 
-	err = platform_driver_register(&iTCO_wdt_driver);
-	if (err)
-		return err;
-
-	return 0;
+	return platform_driver_register(&iTCO_wdt_driver);
 }
 
 static void __exit iTCO_wdt_cleanup_module(void)

Signature · 1 line

2.7.4

Andy Shevchenko

Reviewed-by on v1 4/4

2017-01-03 22:38 UTC #12 lore raw

Re: [PATCH 4/4] watchdog: iTCO_wdt: Simplify module init function

Quoted 2 lines · Guenter Roeck

The 'ret' variable in iTCO_wdt_init_module() does not add any value; drop it.

Perhaps 'err', otherwise:

Reviewed-by Andy Shevchenko <[email protected]>

Quoted 30 lines

Signed-off-by Guenter Roeck <[email protected]>

---
 drivers/watchdog/iTCO_wdt.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

drivers/watchdog/iTCO_wdt.c +1 -8

index ad29ae03a30b..fc7712112412 100644
@@ -612,15 +612,9 @@ static struct platform_driver iTCO_wdt_driver = {
 
 static int __init iTCO_wdt_init_module(void)
 {
-       int err;
-
        pr_info("Intel TCO WatchDog Timer Driver v%s\n", DRV_VERSION);
 
-       err = platform_driver_register(&iTCO_wdt_driver);
-       if (err)
-               return err;
-
-       return 0;
+       return platform_driver_register(&iTCO_wdt_driver);
 }
 
 static void __exit iTCO_wdt_cleanup_module(void)
--

2.7.4

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Andy Shevchenko

Reviewed-by on v1 1/4

2017-01-03 22:44 UTC #13 lore raw

In reply to Guenter Roeck

Quoted 1 line · Guenter Roeck

Allocate private data and the watchdog device to to avoid having

'too to' ?

Quoted 1 line

to clear it on remove and to enable subsequent simplifications.

I doubt it will be more than one device per platform, but change is good by itself to reduce amount of global module variables and a such.

Reviewed-by Andy Shevchenko <[email protected]>

Quoted 692 lines

Signed-off-by Guenter Roeck <[email protected]>

---
 drivers/watchdog/iTCO_wdt.c | 402 ++++++++++++++++++++++----------------------
 1 file changed, 205 insertions(+), 197 deletions(-)

drivers/watchdog/iTCO_wdt.c +205 ~~-198~~

index 06fcb6c8c917..a35a9164ccd0 100644
@@ -72,22 +72,24 @@
 
 /* Address definitions for the TCO */
 /* TCO base address */
-#define TCOBASE                (iTCO_wdt_private.tco_res->start)
+#define TCOBASE(p)     ((p)->tco_res->start)
 /* SMI Control and Enable Register */
-#define SMI_EN         (iTCO_wdt_private.smi_res->start)
-
-#define TCO_RLD                (TCOBASE + 0x00) /* TCO Timer Reload and Curr. Value */
-#define TCOv1_TMR      (TCOBASE + 0x01) /* TCOv1 Timer Initial Value   */
-#define TCO_DAT_IN     (TCOBASE + 0x02) /* TCO Data In Register        */
-#define TCO_DAT_OUT    (TCOBASE + 0x03) /* TCO Data Out Register       */
-#define TCO1_STS       (TCOBASE + 0x04) /* TCO1 Status Register        */
-#define TCO2_STS       (TCOBASE + 0x06) /* TCO2 Status Register        */
-#define TCO1_CNT       (TCOBASE + 0x08) /* TCO1 Control Register       */
-#define TCO2_CNT       (TCOBASE + 0x0a) /* TCO2 Control Register       */
-#define TCOv2_TMR      (TCOBASE + 0x12) /* TCOv2 Timer Initial Value   */
+#define SMI_EN(p)      ((p)->smi_res->start)
+
+#define TCO_RLD(p)     (TCOBASE(p) + 0x00) /* TCO Timer Reload/Curr. Value */
+#define TCOv1_TMR(p)   (TCOBASE(p) + 0x01) /* TCOv1 Timer Initial Value*/
+#define TCO_DAT_IN(p)  (TCOBASE(p) + 0x02) /* TCO Data In Register     */
+#define TCO_DAT_OUT(p) (TCOBASE(p) + 0x03) /* TCO Data Out Register    */
+#define TCO1_STS(p)    (TCOBASE(p) + 0x04) /* TCO1 Status Register     */
+#define TCO2_STS(p)    (TCOBASE(p) + 0x06) /* TCO2 Status Register     */
+#define TCO1_CNT(p)    (TCOBASE(p) + 0x08) /* TCO1 Control Register    */
+#define TCO2_CNT(p)    (TCOBASE(p) + 0x0a) /* TCO2 Control Register    */
+#define TCOv2_TMR(p)   (TCOBASE(p) + 0x12) /* TCOv2 Timer Initial Value*/
 
 /* internal variables */
-static struct {                /* this is private data for the iTCO_wdt device */
+struct iTCO_wdt_private {
+       struct watchdog_device wddev;
+
        /* TCO version/generation */
        unsigned int iTCO_version;
        struct resource *tco_res;

@@ -105,7 +107,7 @@ static struct {             /* this is private data for the iTCO_wdt device */
        struct pci_dev *pdev;
        /* whether or not the watchdog has been suspended */
        bool suspended;
-} iTCO_wdt_private;
+};
 
 /* module parameters */
 #define WATCHDOG_TIMEOUT 30    /* 30 sec default heartbeat */

@@ -135,21 +137,23 @@ MODULE_PARM_DESC(turn_SMI_watchdog_clear_off,
  * every 0.6 seconds.  v3's internal timer is stored as seconds (some
  * datasheets incorrectly state 0.6 seconds).
  */
-static inline unsigned int seconds_to_ticks(int secs)
+static inline unsigned int seconds_to_ticks(struct iTCO_wdt_private *p,
+                                           int secs)
 {
-       return iTCO_wdt_private.iTCO_version == 3 ? secs : (secs * 10) / 6;
+       return p->iTCO_version == 3 ? secs : (secs * 10) / 6;
 }
 
-static inline unsigned int ticks_to_seconds(int ticks)
+static inline unsigned int ticks_to_seconds(struct iTCO_wdt_private *p,
+                                           int ticks)
 {
-       return iTCO_wdt_private.iTCO_version == 3 ? ticks : (ticks * 6) / 10;
+       return p->iTCO_version == 3 ? ticks : (ticks * 6) / 10;
 }
 
-static inline u32 no_reboot_bit(void)
+static inline u32 no_reboot_bit(struct iTCO_wdt_private *p)
 {
        u32 enable_bit;
 
-       switch (iTCO_wdt_private.iTCO_version) {
+       switch (p->iTCO_version) {
        case 5:
        case 3:
                enable_bit = 0x00000010;

@@ -167,40 +171,40 @@ static inline u32 no_reboot_bit(void)
        return enable_bit;
 }
 
-static void iTCO_wdt_set_NO_REBOOT_bit(void)
+static void iTCO_wdt_set_NO_REBOOT_bit(struct iTCO_wdt_private *p)
 {
        u32 val32;
 
        /* Set the NO_REBOOT bit: this disables reboots */
-       if (iTCO_wdt_private.iTCO_version >= 2) {
-               val32 = readl(iTCO_wdt_private.gcs_pmc);
-               val32 |= no_reboot_bit();
-               writel(val32, iTCO_wdt_private.gcs_pmc);
-       } else if (iTCO_wdt_private.iTCO_version == 1) {
-               pci_read_config_dword(iTCO_wdt_private.pdev, 0xd4, &val32);
-               val32 |= no_reboot_bit();
-               pci_write_config_dword(iTCO_wdt_private.pdev, 0xd4, val32);
+       if (p->iTCO_version >= 2) {
+               val32 = readl(p->gcs_pmc);
+               val32 |= no_reboot_bit(p);
+               writel(val32, p->gcs_pmc);
+       } else if (p->iTCO_version == 1) {
+               pci_read_config_dword(p->pdev, 0xd4, &val32);
+               val32 |= no_reboot_bit(p);
+               pci_write_config_dword(p->pdev, 0xd4, val32);
        }
 }
 
-static int iTCO_wdt_unset_NO_REBOOT_bit(void)
+static int iTCO_wdt_unset_NO_REBOOT_bit(struct iTCO_wdt_private *p)
 {
-       u32 enable_bit = no_reboot_bit();
+       u32 enable_bit = no_reboot_bit(p);
        u32 val32 = 0;
 
        /* Unset the NO_REBOOT bit: this enables reboots */
-       if (iTCO_wdt_private.iTCO_version >= 2) {
-               val32 = readl(iTCO_wdt_private.gcs_pmc);
+       if (p->iTCO_version >= 2) {
+               val32 = readl(p->gcs_pmc);
                val32 &= ~enable_bit;
-               writel(val32, iTCO_wdt_private.gcs_pmc);
+               writel(val32, p->gcs_pmc);
 
-               val32 = readl(iTCO_wdt_private.gcs_pmc);
-       } else if (iTCO_wdt_private.iTCO_version == 1) {
-               pci_read_config_dword(iTCO_wdt_private.pdev, 0xd4, &val32);
+               val32 = readl(p->gcs_pmc);
+       } else if (p->iTCO_version == 1) {
+               pci_read_config_dword(p->pdev, 0xd4, &val32);
                val32 &= ~enable_bit;
-               pci_write_config_dword(iTCO_wdt_private.pdev, 0xd4, val32);
+               pci_write_config_dword(p->pdev, 0xd4, val32);
 
-               pci_read_config_dword(iTCO_wdt_private.pdev, 0xd4, &val32);
+               pci_read_config_dword(p->pdev, 0xd4, &val32);
        }
 
        if (val32 & enable_bit)

@@ -211,32 +215,33 @@ static int iTCO_wdt_unset_NO_REBOOT_bit(void)
 
 static int iTCO_wdt_start(struct watchdog_device *wd_dev)
 {
+       struct iTCO_wdt_private *p = watchdog_get_drvdata(wd_dev);
        unsigned int val;
 
-       spin_lock(&iTCO_wdt_private.io_lock);
+       spin_lock(&p->io_lock);
 
-       iTCO_vendor_pre_start(iTCO_wdt_private.smi_res, wd_dev->timeout);
+       iTCO_vendor_pre_start(p->smi_res, wd_dev->timeout);
 
        /* disable chipset's NO_REBOOT bit */
-       if (iTCO_wdt_unset_NO_REBOOT_bit()) {
-               spin_unlock(&iTCO_wdt_private.io_lock);
+       if (iTCO_wdt_unset_NO_REBOOT_bit(p)) {
+               spin_unlock(&p->io_lock);
                pr_err("failed to reset NO_REBOOT flag, reboot disabled by hardware/BIOS\n");
                return -EIO;
        }
 
        /* Force the timer to its reload value by writing to the TCO_RLD
           register */
-       if (iTCO_wdt_private.iTCO_version >= 2)
-               outw(0x01, TCO_RLD);
-       else if (iTCO_wdt_private.iTCO_version == 1)
-               outb(0x01, TCO_RLD);
+       if (p->iTCO_version >= 2)
+               outw(0x01, TCO_RLD(p));
+       else if (p->iTCO_version == 1)
+               outb(0x01, TCO_RLD(p));
 
        /* Bit 11: TCO Timer Halt -> 0 = The TCO timer is enabled to count */
-       val = inw(TCO1_CNT);
+       val = inw(TCO1_CNT(p));
        val &= 0xf7ff;
-       outw(val, TCO1_CNT);
-       val = inw(TCO1_CNT);
-       spin_unlock(&iTCO_wdt_private.io_lock);
+       outw(val, TCO1_CNT(p));
+       val = inw(TCO1_CNT(p));
+       spin_unlock(&p->io_lock);
 
        if (val & 0x0800)
                return -1;

@@ -245,22 +250,23 @@ static int iTCO_wdt_start(struct watchdog_device *wd_dev)
 
 static int iTCO_wdt_stop(struct watchdog_device *wd_dev)
 {
+       struct iTCO_wdt_private *p = watchdog_get_drvdata(wd_dev);
        unsigned int val;
 
-       spin_lock(&iTCO_wdt_private.io_lock);
+       spin_lock(&p->io_lock);
 
-       iTCO_vendor_pre_stop(iTCO_wdt_private.smi_res);
+       iTCO_vendor_pre_stop(p->smi_res);
 
        /* Bit 11: TCO Timer Halt -> 1 = The TCO timer is disabled */
-       val = inw(TCO1_CNT);
+       val = inw(TCO1_CNT(p));
        val |= 0x0800;
-       outw(val, TCO1_CNT);
-       val = inw(TCO1_CNT);
+       outw(val, TCO1_CNT(p));
+       val = inw(TCO1_CNT(p));
 
        /* Set the NO_REBOOT bit to prevent later reboots, just for sure */
-       iTCO_wdt_set_NO_REBOOT_bit();
+       iTCO_wdt_set_NO_REBOOT_bit(p);
 
-       spin_unlock(&iTCO_wdt_private.io_lock);
+       spin_unlock(&p->io_lock);
 
        if ((val & 0x0800) == 0)
                return -1;

@@ -269,67 +275,70 @@ static int iTCO_wdt_stop(struct watchdog_device *wd_dev)
 
 static int iTCO_wdt_ping(struct watchdog_device *wd_dev)
 {
-       spin_lock(&iTCO_wdt_private.io_lock);
+       struct iTCO_wdt_private *p = watchdog_get_drvdata(wd_dev);
 
-       iTCO_vendor_pre_keepalive(iTCO_wdt_private.smi_res, wd_dev->timeout);
+       spin_lock(&p->io_lock);
+
+       iTCO_vendor_pre_keepalive(p->smi_res, wd_dev->timeout);
 
        /* Reload the timer by writing to the TCO Timer Counter register */
-       if (iTCO_wdt_private.iTCO_version >= 2) {
-               outw(0x01, TCO_RLD);
-       } else if (iTCO_wdt_private.iTCO_version == 1) {
+       if (p->iTCO_version >= 2) {
+               outw(0x01, TCO_RLD(p));
+       } else if (p->iTCO_version == 1) {
                /* Reset the timeout status bit so that the timer
                 * needs to count down twice again before rebooting */
-               outw(0x0008, TCO1_STS); /* write 1 to clear bit */
+               outw(0x0008, TCO1_STS(p));      /* write 1 to clear bit */
 
-               outb(0x01, TCO_RLD);
+               outb(0x01, TCO_RLD(p));
        }
 
-       spin_unlock(&iTCO_wdt_private.io_lock);
+       spin_unlock(&p->io_lock);
        return 0;
 }
 
 static int iTCO_wdt_set_timeout(struct watchdog_device *wd_dev, unsigned int t)
 {
+       struct iTCO_wdt_private *p = watchdog_get_drvdata(wd_dev);
        unsigned int val16;
        unsigned char val8;
        unsigned int tmrval;
 
-       tmrval = seconds_to_ticks(t);
+       tmrval = seconds_to_ticks(p, t);
 
        /* For TCO v1 the timer counts down twice before rebooting */
-       if (iTCO_wdt_private.iTCO_version == 1)
+       if (p->iTCO_version == 1)
                tmrval /= 2;
 
        /* from the specs: */
        /* "Values of 0h-3h are ignored and should not be attempted" */
        if (tmrval < 0x04)
                return -EINVAL;
-       if (((iTCO_wdt_private.iTCO_version >= 2) && (tmrval > 0x3ff)) ||
-           ((iTCO_wdt_private.iTCO_version == 1) && (tmrval > 0x03f)))
+       if ((p->iTCO_version >= 2 && tmrval > 0x3ff) ||
+           (p->iTCO_version == 1 && tmrval > 0x03f))
                return -EINVAL;
 
        iTCO_vendor_pre_set_heartbeat(tmrval);
 
        /* Write new heartbeat to watchdog */
-       if (iTCO_wdt_private.iTCO_version >= 2) {
-               spin_lock(&iTCO_wdt_private.io_lock);
-               val16 = inw(TCOv2_TMR);
+       if (p->iTCO_version >= 2) {
+               spin_lock(&p->io_lock);
+               val16 = inw(TCOv2_TMR(p));
                val16 &= 0xfc00;
                val16 |= tmrval;
-               outw(val16, TCOv2_TMR);
-               val16 = inw(TCOv2_TMR);
-               spin_unlock(&iTCO_wdt_private.io_lock);
+               outw(val16, TCOv2_TMR(p));
+               val16 = inw(TCOv2_TMR(p));
+               spin_unlock(&p->io_lock);
 
                if ((val16 & 0x3ff) != tmrval)
                        return -EINVAL;
-       } else if (iTCO_wdt_private.iTCO_version == 1) {
-               spin_lock(&iTCO_wdt_private.io_lock);
-               val8 = inb(TCOv1_TMR);
+       } else if (p->iTCO_version == 1) {
+               spin_lock(&p->io_lock);
+               val8 = inb(TCOv1_TMR(p));
                val8 &= 0xc0;
                val8 |= (tmrval & 0xff);
-               outb(val8, TCOv1_TMR);
-               val8 = inb(TCOv1_TMR);
-               spin_unlock(&iTCO_wdt_private.io_lock);
+               outb(val8, TCOv1_TMR(p));
+               val8 = inb(TCOv1_TMR(p));
+               spin_unlock(&p->io_lock);
 
                if ((val8 & 0x3f) != tmrval)
                        return -EINVAL;

@@ -341,27 +350,28 @@ static int iTCO_wdt_set_timeout(struct watchdog_device *wd_dev, unsigned int t)
 
 static unsigned int iTCO_wdt_get_timeleft(struct watchdog_device *wd_dev)
 {
+       struct iTCO_wdt_private *p = watchdog_get_drvdata(wd_dev);
        unsigned int val16;
        unsigned char val8;
        unsigned int time_left = 0;
 
        /* read the TCO Timer */
-       if (iTCO_wdt_private.iTCO_version >= 2) {
-               spin_lock(&iTCO_wdt_private.io_lock);
-               val16 = inw(TCO_RLD);
+       if (p->iTCO_version >= 2) {
+               spin_lock(&p->io_lock);
+               val16 = inw(TCO_RLD(p));
                val16 &= 0x3ff;
-               spin_unlock(&iTCO_wdt_private.io_lock);
+               spin_unlock(&p->io_lock);
 
-               time_left = ticks_to_seconds(val16);
-       } else if (iTCO_wdt_private.iTCO_version == 1) {
-               spin_lock(&iTCO_wdt_private.io_lock);
-               val8 = inb(TCO_RLD);
+               time_left = ticks_to_seconds(p, val16);
+       } else if (p->iTCO_version == 1) {
+               spin_lock(&p->io_lock);
+               val8 = inb(TCO_RLD(p));
                val8 &= 0x3f;
-               if (!(inw(TCO1_STS) & 0x0008))
-                       val8 += (inb(TCOv1_TMR) & 0x3f);
-               spin_unlock(&iTCO_wdt_private.io_lock);
+               if (!(inw(TCO1_STS(p)) & 0x0008))
+                       val8 += (inb(TCOv1_TMR(p)) & 0x3f);
+               spin_unlock(&p->io_lock);
 
-               time_left = ticks_to_seconds(val8);
+               time_left = ticks_to_seconds(p, val8);
        }
        return time_left;
 }

@@ -387,166 +397,165 @@ static const struct watchdog_ops iTCO_wdt_ops = {
        .get_timeleft =         iTCO_wdt_get_timeleft,
 };
 
-static struct watchdog_device iTCO_wdt_watchdog_dev = {
-       .info =         &ident,
-       .ops =          &iTCO_wdt_ops,
-};
-
 /*
  *     Init & exit routines
  */
 
-static void iTCO_wdt_cleanup(void)
+static void iTCO_wdt_cleanup(struct iTCO_wdt_private *p)
 {
        /* Stop the timer before we leave */
        if (!nowayout)
-               iTCO_wdt_stop(&iTCO_wdt_watchdog_dev);
+               iTCO_wdt_stop(&p->wddev);
 
        /* Deregister */
-       watchdog_unregister_device(&iTCO_wdt_watchdog_dev);
+       watchdog_unregister_device(&p->wddev);
 
        /* release resources */
-       release_region(iTCO_wdt_private.tco_res->start,
-                       resource_size(iTCO_wdt_private.tco_res));
-       release_region(iTCO_wdt_private.smi_res->start,
-                       resource_size(iTCO_wdt_private.smi_res));
-       if (iTCO_wdt_private.iTCO_version >= 2) {
-               iounmap(iTCO_wdt_private.gcs_pmc);
-               release_mem_region(iTCO_wdt_private.gcs_pmc_res->start,
-                               resource_size(iTCO_wdt_private.gcs_pmc_res));
+       release_region(p->tco_res->start,
+                       resource_size(p->tco_res));
+       release_region(p->smi_res->start,
+                       resource_size(p->smi_res));
+       if (p->iTCO_version >= 2) {
+               iounmap(p->gcs_pmc);
+               release_mem_region(p->gcs_pmc_res->start,
+                               resource_size(p->gcs_pmc_res));
        }
-
-       iTCO_wdt_private.tco_res = NULL;
-       iTCO_wdt_private.smi_res = NULL;
-       iTCO_wdt_private.gcs_pmc_res = NULL;
-       iTCO_wdt_private.gcs_pmc = NULL;
 }
 
 static int iTCO_wdt_probe(struct platform_device *dev)
 {
-       int ret = -ENODEV;
-       unsigned long val32;
        struct itco_wdt_platform_data *pdata = dev_get_platdata(&dev->dev);
+       struct iTCO_wdt_private *p;
+       unsigned long val32;
+       int ret;
 
        if (!pdata)
-               goto out;
+               return -ENODEV;
 
-       spin_lock_init(&iTCO_wdt_private.io_lock);
+       p = devm_kzalloc(&dev->dev, sizeof(*p), GFP_KERNEL);
+       if (!p)
+               return -ENOMEM;
 
-       iTCO_wdt_private.tco_res =
-               platform_get_resource(dev, IORESOURCE_IO, ICH_RES_IO_TCO);
-       if (!iTCO_wdt_private.tco_res)
-               goto out;
+       spin_lock_init(&p->io_lock);
 
-       iTCO_wdt_private.smi_res =
-               platform_get_resource(dev, IORESOURCE_IO, ICH_RES_IO_SMI);
-       if (!iTCO_wdt_private.smi_res)
-               goto out;
+       p->tco_res = platform_get_resource(dev, IORESOURCE_IO, ICH_RES_IO_TCO);
+       if (!p->tco_res)
+               return -ENODEV;
 
-       iTCO_wdt_private.iTCO_version = pdata->version;
-       iTCO_wdt_private.dev = dev;
-       iTCO_wdt_private.pdev = to_pci_dev(dev->dev.parent);
+       p->smi_res = platform_get_resource(dev, IORESOURCE_IO, ICH_RES_IO_SMI);
+       if (!p->smi_res)
+               return -ENODEV;
+
+       p->iTCO_version = pdata->version;
+       p->dev = dev;
+       p->pdev = to_pci_dev(dev->dev.parent);
 
        /*
         * Get the Memory-Mapped GCS or PMC register, we need it for the
         * NO_REBOOT flag (TCO v2 and v3).
         */
-       if (iTCO_wdt_private.iTCO_version >= 2) {
-               iTCO_wdt_private.gcs_pmc_res = platform_get_resource(dev,
-                                                       IORESOURCE_MEM,
-                                                       ICH_RES_MEM_GCS_PMC);
-
-               if (!iTCO_wdt_private.gcs_pmc_res)
-                       goto out;
-
-               if (!request_mem_region(iTCO_wdt_private.gcs_pmc_res->start,
-                       resource_size(iTCO_wdt_private.gcs_pmc_res), dev->name)) {
-                       ret = -EBUSY;
-                       goto out;
-               }
-               iTCO_wdt_private.gcs_pmc = ioremap(iTCO_wdt_private.gcs_pmc_res->start,
-                       resource_size(iTCO_wdt_private.gcs_pmc_res));
-               if (!iTCO_wdt_private.gcs_pmc) {
+       if (p->iTCO_version >= 2) {
+               p->gcs_pmc_res = platform_get_resource(dev,
+                                                      IORESOURCE_MEM,
+                                                      ICH_RES_MEM_GCS_PMC);
+
+               if (!p->gcs_pmc_res)
+                       return -ENODEV;
+
+               if (!request_mem_region(p->gcs_pmc_res->start,
+                                       resource_size(p->gcs_pmc_res),
+                                       dev->name))
+                       return -EBUSY;
+
+               p->gcs_pmc = ioremap(p->gcs_pmc_res->start,
+                                    resource_size(p->gcs_pmc_res));
+               if (!p->gcs_pmc) {
                        ret = -EIO;
                        goto unreg_gcs_pmc;
                }
        }
 
        /* Check chipset's NO_REBOOT bit */
-       if (iTCO_wdt_unset_NO_REBOOT_bit() && iTCO_vendor_check_noreboot_on()) {
+       if (iTCO_wdt_unset_NO_REBOOT_bit(p) &&
+           iTCO_vendor_check_noreboot_on()) {
                pr_info("unable to reset NO_REBOOT flag, device disabled by hardware/BIOS\n");
                ret = -ENODEV;  /* Cannot reset NO_REBOOT bit */
                goto unmap_gcs_pmc;
        }
 
        /* Set the NO_REBOOT bit to prevent later reboots, just for sure */
-       iTCO_wdt_set_NO_REBOOT_bit();
+       iTCO_wdt_set_NO_REBOOT_bit(p);
 
        /* The TCO logic uses the TCO_EN bit in the SMI_EN register */
-       if (!request_region(iTCO_wdt_private.smi_res->start,
-                       resource_size(iTCO_wdt_private.smi_res), dev->name)) {
+       if (!request_region(p->smi_res->start,
+                           resource_size(p->smi_res), dev->name)) {
                pr_err("I/O address 0x%04llx already in use, device disabled\n",
-                      (u64)SMI_EN);
+                      (u64)SMI_EN(p));
                ret = -EBUSY;
                goto unmap_gcs_pmc;
        }
-       if (turn_SMI_watchdog_clear_off >= iTCO_wdt_private.iTCO_version) {
+       if (turn_SMI_watchdog_clear_off >= p->iTCO_version) {
                /*
                 * Bit 13: TCO_EN -> 0
                 * Disables TCO logic generating an SMI#
                 */
-               val32 = inl(SMI_EN);
+               val32 = inl(SMI_EN(p));
                val32 &= 0xffffdfff;    /* Turn off SMI clearing watchdog */
-               outl(val32, SMI_EN);
+               outl(val32, SMI_EN(p));
        }
 
-       if (!request_region(iTCO_wdt_private.tco_res->start,
-                       resource_size(iTCO_wdt_private.tco_res), dev->name)) {
+       if (!request_region(p->tco_res->start,
+                           resource_size(p->tco_res), dev->name)) {
                pr_err("I/O address 0x%04llx already in use, device disabled\n",
-                      (u64)TCOBASE);
+                      (u64)TCOBASE(p));
                ret = -EBUSY;
                goto unreg_smi;
        }
 
        pr_info("Found a %s TCO device (Version=%d, TCOBASE=0x%04llx)\n",
-               pdata->name, pdata->version, (u64)TCOBASE);
+               pdata->name, pdata->version, (u64)TCOBASE(p));
 
        /* Clear out the (probably old) status */
-       switch (iTCO_wdt_private.iTCO_version) {
+       switch (p->iTCO_version) {
        case 5:
        case 4:
-               outw(0x0008, TCO1_STS); /* Clear the Time Out Status bit */
-               outw(0x0002, TCO2_STS); /* Clear SECOND_TO_STS bit */
+               outw(0x0008, TCO1_STS(p)); /* Clear the Time Out Status bit */
+               outw(0x0002, TCO2_STS(p)); /* Clear SECOND_TO_STS bit */
                break;
        case 3:
-               outl(0x20008, TCO1_STS);
+               outl(0x20008, TCO1_STS(p));
                break;
        case 2:
        case 1:
        default:
-               outw(0x0008, TCO1_STS); /* Clear the Time Out Status bit */
-               outw(0x0002, TCO2_STS); /* Clear SECOND_TO_STS bit */
-               outw(0x0004, TCO2_STS); /* Clear BOOT_STS bit */
+               outw(0x0008, TCO1_STS(p)); /* Clear the Time Out Status bit */
+               outw(0x0002, TCO2_STS(p)); /* Clear SECOND_TO_STS bit */
+               outw(0x0004, TCO2_STS(p)); /* Clear BOOT_STS bit */
                break;
        }
 
-       iTCO_wdt_watchdog_dev.bootstatus = 0;
-       iTCO_wdt_watchdog_dev.timeout = WATCHDOG_TIMEOUT;
-       watchdog_set_nowayout(&iTCO_wdt_watchdog_dev, nowayout);
-       iTCO_wdt_watchdog_dev.parent = &dev->dev;
+       p->wddev.info = &ident,
+       p->wddev.ops = &iTCO_wdt_ops,
+       p->wddev.bootstatus = 0;
+       p->wddev.timeout = WATCHDOG_TIMEOUT;
+       watchdog_set_nowayout(&p->wddev, nowayout);
+       p->wddev.parent = &dev->dev;
+
+       watchdog_set_drvdata(&p->wddev, p);
+       platform_set_drvdata(dev, p);
 
        /* Make sure the watchdog is not running */
-       iTCO_wdt_stop(&iTCO_wdt_watchdog_dev);
+       iTCO_wdt_stop(&p->wddev);
 
        /* Check that the heartbeat value is within it's range;
           if not reset to the default */
-       if (iTCO_wdt_set_timeout(&iTCO_wdt_watchdog_dev, heartbeat)) {
-               iTCO_wdt_set_timeout(&iTCO_wdt_watchdog_dev, WATCHDOG_TIMEOUT);
+       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",
                        WATCHDOG_TIMEOUT);
        }
 
-       ret = watchdog_register_device(&iTCO_wdt_watchdog_dev);
+       ret = watchdog_register_device(&p->wddev);
        if (ret != 0) {
                pr_err("cannot register watchdog device (err=%d)\n", ret);
                goto unreg_tco;

@@ -558,38 +567,34 @@ static int iTCO_wdt_probe(struct platform_device *dev)
        return 0;
 
 unreg_tco:
-       release_region(iTCO_wdt_private.tco_res->start,
-                       resource_size(iTCO_wdt_private.tco_res));
+       release_region(p->tco_res->start, resource_size(p->tco_res));
 unreg_smi:
-       release_region(iTCO_wdt_private.smi_res->start,
-                       resource_size(iTCO_wdt_private.smi_res));
+       release_region(p->smi_res->start, resource_size(p->smi_res));
 unmap_gcs_pmc:
-       if (iTCO_wdt_private.iTCO_version >= 2)
-               iounmap(iTCO_wdt_private.gcs_pmc);
+       if (p->iTCO_version >= 2)
+               iounmap(p->gcs_pmc);
 unreg_gcs_pmc:
-       if (iTCO_wdt_private.iTCO_version >= 2)
-               release_mem_region(iTCO_wdt_private.gcs_pmc_res->start,
-                               resource_size(iTCO_wdt_private.gcs_pmc_res));
-out:
-       iTCO_wdt_private.tco_res = NULL;
-       iTCO_wdt_private.smi_res = NULL;
-       iTCO_wdt_private.gcs_pmc_res = NULL;
-       iTCO_wdt_private.gcs_pmc = NULL;
-
+       if (p->iTCO_version >= 2)
+               release_mem_region(p->gcs_pmc_res->start,
+                                  resource_size(p->gcs_pmc_res));
        return ret;
 }
 
 static int iTCO_wdt_remove(struct platform_device *dev)
 {
-       if (iTCO_wdt_private.tco_res || iTCO_wdt_private.smi_res)
-               iTCO_wdt_cleanup();
+       struct iTCO_wdt_private *p = platform_get_drvdata(dev);
+
+       if (p->tco_res || p->smi_res)
+               iTCO_wdt_cleanup(p);
 
        return 0;
 }
 
 static void iTCO_wdt_shutdown(struct platform_device *dev)
 {
-       iTCO_wdt_stop(NULL);
+       struct iTCO_wdt_private *p = platform_get_drvdata(dev);
+
+       iTCO_wdt_stop(&p->wddev);
 }
 
 #ifdef CONFIG_PM_SLEEP

@@ -610,21 +615,24 @@ static inline bool need_suspend(void) { return true; }
 
 static int iTCO_wdt_suspend_noirq(struct device *dev)
 {
+       struct iTCO_wdt_private *p = dev_get_drvdata(dev);
        int ret = 0;
 
-       iTCO_wdt_private.suspended = false;
-       if (watchdog_active(&iTCO_wdt_watchdog_dev) && need_suspend()) {
-               ret = iTCO_wdt_stop(&iTCO_wdt_watchdog_dev);
+       p->suspended = false;
+       if (watchdog_active(&p->wddev) && need_suspend()) {
+               ret = iTCO_wdt_stop(&p->wddev);
                if (!ret)
-                       iTCO_wdt_private.suspended = true;
+                       p->suspended = true;
        }
        return ret;
 }
 
 static int iTCO_wdt_resume_noirq(struct device *dev)
 {
-       if (iTCO_wdt_private.suspended)
-               iTCO_wdt_start(&iTCO_wdt_watchdog_dev);
+       struct iTCO_wdt_private *p = dev_get_drvdata(dev);
+
+       if (p->suspended)
+               iTCO_wdt_start(&p->wddev);
 
        return 0;
 }
--

2.7.4

Signature · 2 lines

With Best Regards,
Andy Shevchenko