[PATCH v3 0/3] iTCO_wdt: Add support for Intel Sunrisepoint

[PATCH v3 0/3] iTCO_wdt: Add support for Intel Sunrisepoint

[Matt Fleming]

From: Matt Fleming <matt.fleming@intel.com>

Starting with Intel Sunrisepoint (Skylake PCH) the TCO watchdog device
is now on the SMBUS, whereas for previous ICH/PCH it was on the LPC bus.

Because iTCO_wdt devices may now appear on either the LPC bus or the
SMBUS we need to abstract the bus information into an agnostic structure
instead of the existing 'lpc_ich_info' which tightly integrates both the
lpc_ich and iTCO_wdt drivers.

The first patch introduces a platform data structure to handle this and
shuffles the existing code around. The other patches add the
device-specific information to the i2c-i801 and iTCO_wdt drivers.

Patches based against v4.2-rc4, if there's some other tree I should base
this on, please let me know.

Comments welcome!

Changes in v3:

 - Added Ack/Review tags to patches

Changes in v2:

 - Use lowercase for all new files and data structures
 - Allocate platform data with devm_kzalloc()
 - Move Kconfig changes to final patch and use 'select', not 'depends'
 - Swap strcpy() for strlcpy()
 - Fixup use of lpc_ich_info in intel_pmc_ipc which was missed in v1
 - Don't use bitops in i2c-i801
 - Remove superfluous NULL checks in i2c-i801
 - Explicitly list reboot bit versions in no_reboot_bit()
 - Use switch/case construst instead of crazy if else
 - Fold original fixups from PATCH 4 and 5 into PATCH 1 and 2


Matt Fleming (2):
  iTCO_wdt: Expose watchdog properties using platform data
  iTCO_wdt: Add support for TCO on Intel Sunrisepoint

Mika Westerberg (1):
  i2c: i801: Create iTCO device on newer Intel PCHs

 drivers/i2c/busses/i2c-i801.c          | 120 ++++++++++++++++++++++++++++++++-
 drivers/mfd/lpc_ich.c                  |  32 ++++++++-
 drivers/platform/x86/intel_pmc_ipc.c   |   9 ++-
 drivers/watchdog/Kconfig               |   3 +-
 drivers/watchdog/iTCO_wdt.c            |  77 ++++++++++++---------
 include/linux/mfd/lpc_ich.h            |   6 --
 include/linux/platform_data/itco_wdt.h |  19 ++++++
 7 files changed, 219 insertions(+), 47 deletions(-)
 create mode 100644 include/linux/platform_data/itco_wdt.h

-- 
2.1.0

[PATCH v3 1/3] iTCO_wdt: Expose watchdog properties using platform data

[Matt Fleming]

From: Matt Fleming <matt.fleming@intel.com>

Intel Sunrisepoint (Skylake PCH) has the iTCO watchdog accessible across
the SMBus, unlike previous generations of PCH/ICH where it was on the
LPC bus. Because it's on the SMBus, it doesn't make sense to pass around
a 'struct lpc_ich_info', and leaking the type of bus into the iTCO
watchdog driver is kind of backwards anyway.

This change introduces a new 'struct itco_wdt_platform_data' for use
inside the iTCO watchdog driver and by the upcoming Intel Sunrisepoint
code, which neatly avoids having to include lpc_ich headers in the i801
i2c driver.

This change is overdue because lpc_ich_info has already found its way
into other TCO watchdog users, notably the intel_pmc_ipc driver where
the watchdog actually isn't on the LPC bus as far as I can see.

A simple translation layer is provided for converting from the existing
'struct lpc_ich_info' inside the lpc_ich mfd driver.

Cc: Peter Tyser <ptyser@xes-inc.com>
Cc: Samuel Ortiz <sameo@linux.intel.com>
Acked-by: Lee Jones <lee.jones@linaro.org>
Cc: Wim Van Sebroeck <wim@iguana.be>
Cc: Zha Qipeng <qipeng.zha@intel.com>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Jean Delvare <jdelvare@suse.com>
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---

v3:
 - Added Lee's ACK

v2:
 - Use lowercase "itco" in all new files and data structures.
 - Use devm_kzalloc() to allocate platform data, fixing a memory leak
   and saving us from having to free the allocation in error paths
 - Move stray Kconfig hunk to PATCH 3 that accidentally snuck into v1
 - Swap strcpy() for the safer strlcpy()
 - Fix lpc_ich_info user in the intel_pmc_ipc driver that was merged for
   v4.2.

 drivers/mfd/lpc_ich.c                  | 32 +++++++++++++++++++++++++++++---
 drivers/platform/x86/intel_pmc_ipc.c   |  9 ++++-----
 drivers/watchdog/iTCO_wdt.c            | 11 +++++------
 include/linux/mfd/lpc_ich.h            |  6 ------
 include/linux/platform_data/itco_wdt.h | 19 +++++++++++++++++++
 5 files changed, 57 insertions(+), 20 deletions(-)
 create mode 100644 include/linux/platform_data/itco_wdt.h

diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
index 8de34398abc0..c5a9a08b5dfb 100644
--- a/drivers/mfd/lpc_ich.c
+++ b/drivers/mfd/lpc_ich.c
@@ -66,6 +66,7 @@
 #include <linux/pci.h>
 #include <linux/mfd/core.h>
 #include <linux/mfd/lpc_ich.h>
+#include <linux/platform_data/itco_wdt.h>
 
 #define ACPIBASE		0x40
 #define ACPIBASE_GPE_OFF	0x28
@@ -835,9 +836,31 @@ static void lpc_ich_enable_pmc_space(struct pci_dev *dev)
 	priv->actrl_pbase_save = reg_save;
 }
 
-static void lpc_ich_finalize_cell(struct pci_dev *dev, struct mfd_cell *cell)
+static int lpc_ich_finalize_wdt_cell(struct pci_dev *dev)
 {
+	struct itco_wdt_platform_data *pdata;
 	struct lpc_ich_priv *priv = pci_get_drvdata(dev);
+	struct lpc_ich_info *info;
+	struct mfd_cell *cell = &lpc_ich_cells[LPC_WDT];
+
+	pdata = devm_kzalloc(&dev->dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return -ENOMEM;
+
+	info = &lpc_chipset_info[priv->chipset];
+
+	pdata->version = info->iTCO_version;
+	strlcpy(pdata->name, info->name, sizeof(pdata->name));
+
+	cell->platform_data = pdata;
+	cell->pdata_size = sizeof(*pdata);
+	return 0;
+}
+
+static void lpc_ich_finalize_gpio_cell(struct pci_dev *dev)
+{
+	struct lpc_ich_priv *priv = pci_get_drvdata(dev);
+	struct mfd_cell *cell = &lpc_ich_cells[LPC_GPIO];
 
 	cell->platform_data = &lpc_chipset_info[priv->chipset];
 	cell->pdata_size = sizeof(struct lpc_ich_info);
@@ -933,7 +956,7 @@ gpe0_done:
 	lpc_chipset_info[priv->chipset].use_gpio = ret;
 	lpc_ich_enable_gpio_space(dev);
 
-	lpc_ich_finalize_cell(dev, &lpc_ich_cells[LPC_GPIO]);
+	lpc_ich_finalize_gpio_cell(dev);
 	ret = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
 			      &lpc_ich_cells[LPC_GPIO], 1, NULL, 0, NULL);
 
@@ -1007,7 +1030,10 @@ static int lpc_ich_init_wdt(struct pci_dev *dev)
 		res->end = base_addr + ACPIBASE_PMC_END;
 	}
 
-	lpc_ich_finalize_cell(dev, &lpc_ich_cells[LPC_WDT]);
+	ret = lpc_ich_finalize_wdt_cell(dev);
+	if (ret)
+		goto wdt_done;
+
 	ret = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
 			      &lpc_ich_cells[LPC_WDT], 1, NULL, 0, NULL);
 
diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c
index 105cfffe82c6..28b2a12bb26d 100644
--- a/drivers/platform/x86/intel_pmc_ipc.c
+++ b/drivers/platform/x86/intel_pmc_ipc.c
@@ -33,7 +33,7 @@
 #include <linux/suspend.h>
 #include <linux/acpi.h>
 #include <asm/intel_pmc_ipc.h>
-#include <linux/mfd/lpc_ich.h>
+#include <linux/platform_data/itco_wdt.h>
 
 /*
  * IPC registers
@@ -473,9 +473,9 @@ static struct resource tco_res[] = {
 	},
 };
 
-static struct lpc_ich_info tco_info = {
+static struct itco_wdt_platform_data tco_info = {
 	.name = "Apollo Lake SoC",
-	.iTCO_version = 3,
+	.version = 3,
 };
 
 static int ipc_create_punit_device(void)
@@ -552,8 +552,7 @@ static int ipc_create_tco_device(void)
 		goto err;
 	}
 
-	ret = platform_device_add_data(pdev, &tco_info,
-				       sizeof(struct lpc_ich_info));
+	ret = platform_device_add_data(pdev, &tco_info, sizeof(tco_info));
 	if (ret) {
 		dev_err(ipcdev.dev, "Failed to add tco platform data\n");
 		goto err;
diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
index 3c3fd417ddeb..a94401b2deca 100644
--- a/drivers/watchdog/iTCO_wdt.c
+++ b/drivers/watchdog/iTCO_wdt.c
@@ -66,8 +66,7 @@
 #include <linux/spinlock.h>		/* For spin_lock/spin_unlock/... */
 #include <linux/uaccess.h>		/* For copy_to_user/put_user/... */
 #include <linux/io.h>			/* For inb/outb/... */
-#include <linux/mfd/core.h>
-#include <linux/mfd/lpc_ich.h>
+#include <linux/platform_data/itco_wdt.h>
 
 #include "iTCO_vendor.h"
 
@@ -418,9 +417,9 @@ static int iTCO_wdt_probe(struct platform_device *dev)
 {
 	int ret = -ENODEV;
 	unsigned long val32;
-	struct lpc_ich_info *ich_info = dev_get_platdata(&dev->dev);
+	struct itco_wdt_platform_data *pdata = dev_get_platdata(&dev->dev);
 
-	if (!ich_info)
+	if (!pdata)
 		goto out;
 
 	spin_lock_init(&iTCO_wdt_private.io_lock);
@@ -435,7 +434,7 @@ static int iTCO_wdt_probe(struct platform_device *dev)
 	if (!iTCO_wdt_private.smi_res)
 		goto out;
 
-	iTCO_wdt_private.iTCO_version = ich_info->iTCO_version;
+	iTCO_wdt_private.iTCO_version = pdata->version;
 	iTCO_wdt_private.dev = dev;
 	iTCO_wdt_private.pdev = to_pci_dev(dev->dev.parent);
 
@@ -501,7 +500,7 @@ static int iTCO_wdt_probe(struct platform_device *dev)
 	}
 
 	pr_info("Found a %s TCO device (Version=%d, TCOBASE=0x%04llx)\n",
-		ich_info->name, ich_info->iTCO_version, (u64)TCOBASE);
+		pdata->name, pdata->version, (u64)TCOBASE);
 
 	/* Clear out the (probably old) status */
 	if (iTCO_wdt_private.iTCO_version == 3) {
diff --git a/include/linux/mfd/lpc_ich.h b/include/linux/mfd/lpc_ich.h
index 8feac782fa83..2b300b44f994 100644
--- a/include/linux/mfd/lpc_ich.h
+++ b/include/linux/mfd/lpc_ich.h
@@ -20,12 +20,6 @@
 #ifndef LPC_ICH_H
 #define LPC_ICH_H
 
-/* Watchdog resources */
-#define ICH_RES_IO_TCO		0
-#define ICH_RES_IO_SMI		1
-#define ICH_RES_MEM_OFF		2
-#define ICH_RES_MEM_GCS_PMC	0
-
 /* GPIO resources */
 #define ICH_RES_GPIO	0
 #define ICH_RES_GPE0	1
diff --git a/include/linux/platform_data/itco_wdt.h b/include/linux/platform_data/itco_wdt.h
new file mode 100644
index 000000000000..f16542c77ff7
--- /dev/null
+++ b/include/linux/platform_data/itco_wdt.h
@@ -0,0 +1,19 @@
+/*
+ * Platform data for the Intel TCO Watchdog
+ */
+
+#ifndef _ITCO_WDT_H_
+#define _ITCO_WDT_H_
+
+/* Watchdog resources */
+#define ICH_RES_IO_TCO		0
+#define ICH_RES_IO_SMI		1
+#define ICH_RES_MEM_OFF		2
+#define ICH_RES_MEM_GCS_PMC	0
+
+struct itco_wdt_platform_data {
+	char name[32];
+	unsigned int version;
+};
+
+#endif /* _ITCO_WDT_H_ */
-- 
2.1.0

[PATCH v3 2/3] i2c: i801: Create iTCO device on newer Intel PCHs

[Matt Fleming]

From: Mika Westerberg <mika.westerberg@linux.intel.com>

Starting from Intel Sunrisepoint (Skylake PCH) the iTCO watchdog resources
have been moved to reside under the i801 SMBus host controller whereas
previously they were under the LPC device.

In order to support the iTCO watchdog on newer PCHs we need to create the
platform device here in the SMBus driver and pass all known resources using
platform data.

Cc: Jean Delvare <jdelvare@suse.com>
Cc: Wolfram Sang <wsa@the-dreams.de>
Cc: <linux-i2c@vger.kernel.org>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---

v3:
 - Added Guenter's Review tag

v2:
 - Don't use bitops but same scheme used already
 - Delete superfluous NULL-checks for platform_device_unregister()

 drivers/i2c/busses/i2c-i801.c | 120 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 119 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 5ecbb3fdc27e..eaef9bc9d88c 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -88,12 +88,13 @@
 #include <linux/slab.h>
 #include <linux/wait.h>
 #include <linux/err.h>
+#include <linux/platform_device.h>
+#include <linux/platform_data/itco_wdt.h>
 
 #if (defined CONFIG_I2C_MUX_GPIO || defined CONFIG_I2C_MUX_GPIO_MODULE) && \
 		defined CONFIG_DMI
 #include <linux/gpio.h>
 #include <linux/i2c-mux-gpio.h>
-#include <linux/platform_device.h>
 #endif
 
 /* I801 SMBus address offsets */
@@ -113,6 +114,16 @@
 #define SMBPCICTL	0x004
 #define SMBPCISTS	0x006
 #define SMBHSTCFG	0x040
+#define TCOBASE		0x050
+#define TCOCTL		0x054
+
+#define ACPIBASE		0x040
+#define ACPIBASE_SMI_OFF	0x030
+#define ACPICTRL		0x044
+#define ACPICTRL_EN		0x080
+
+#define SBREG_BAR		0x10
+#define SBREG_SMBCTRL		0xc6000c
 
 /* Host status bits for SMBPCISTS */
 #define SMBPCISTS_INTS		0x08
@@ -125,6 +136,9 @@
 #define SMBHSTCFG_SMB_SMI_EN	2
 #define SMBHSTCFG_I2C_EN	4
 
+/* TCO configuration bits for TCOCTL */
+#define TCOCTL_EN		0x0100
+
 /* Auxiliary control register bits, ICH4+ only */
 #define SMBAUXCTL_CRC		1
 #define SMBAUXCTL_E32B		2
@@ -221,6 +235,7 @@ struct i801_priv {
 	const struct i801_mux_config *mux_drvdata;
 	struct platform_device *mux_pdev;
 #endif
+	struct platform_device *tco_pdev;
 };
 
 #define FEATURE_SMBUS_PEC	(1 << 0)
@@ -230,6 +245,7 @@ struct i801_priv {
 #define FEATURE_IRQ		(1 << 4)
 /* Not really a feature, but it's convenient to handle it as such */
 #define FEATURE_IDF		(1 << 15)
+#define FEATURE_TCO		(1 << 16)
 
 static const char *i801_feature_names[] = {
 	"SMBus PEC",
@@ -1132,6 +1148,95 @@ static inline unsigned int i801_get_adapter_class(struct i801_priv *priv)
 }
 #endif
 
+static const struct itco_wdt_platform_data tco_platform_data = {
+	.name = "Intel PCH",
+	.version = 4,
+};
+
+static DEFINE_SPINLOCK(p2sb_spinlock);
+
+static void i801_add_tco(struct i801_priv *priv)
+{
+	struct pci_dev *pci_dev = priv->pci_dev;
+	struct resource tco_res[3], *res;
+	struct platform_device *pdev;
+	unsigned int devfn;
+	u32 tco_base, tco_ctl;
+	u32 base_addr, ctrl_val;
+	u64 base64_addr;
+
+	if (!(priv->features & FEATURE_TCO))
+		return;
+
+	pci_read_config_dword(pci_dev, TCOBASE, &tco_base);
+	pci_read_config_dword(pci_dev, TCOCTL, &tco_ctl);
+	if (!(tco_ctl & TCOCTL_EN))
+		return;
+
+	memset(tco_res, 0, sizeof(tco_res));
+
+	res = &tco_res[ICH_RES_IO_TCO];
+	res->start = tco_base & ~1;
+	res->end = res->start + 32 - 1;
+	res->flags = IORESOURCE_IO;
+
+	/*
+	 * Power Management registers.
+	 */
+	devfn = PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 2);
+	pci_bus_read_config_dword(pci_dev->bus, devfn, ACPIBASE, &base_addr);
+
+	res = &tco_res[ICH_RES_IO_SMI];
+	res->start = (base_addr & ~1) + ACPIBASE_SMI_OFF;
+	res->end = res->start + 3;
+	res->flags = IORESOURCE_IO;
+
+	/*
+	 * Enable the ACPI I/O space.
+	 */
+	pci_bus_read_config_dword(pci_dev->bus, devfn, ACPICTRL, &ctrl_val);
+	ctrl_val |= ACPICTRL_EN;
+	pci_bus_write_config_dword(pci_dev->bus, devfn, ACPICTRL, ctrl_val);
+
+	/*
+	 * We must access the NO_REBOOT bit over the Primary to Sideband
+	 * bridge (P2SB). The BIOS prevents the P2SB device from being
+	 * enumerated by the PCI subsystem, so we need to unhide/hide it
+	 * to lookup the P2SB BAR.
+	 */
+	spin_lock(&p2sb_spinlock);
+
+	devfn = PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 1);
+
+	/* Unhide the P2SB device */
+	pci_bus_write_config_byte(pci_dev->bus, devfn, 0xe1, 0x0);
+
+	pci_bus_read_config_dword(pci_dev->bus, devfn, SBREG_BAR, &base_addr);
+	base64_addr = base_addr & 0xfffffff0;
+
+	pci_bus_read_config_dword(pci_dev->bus, devfn, SBREG_BAR + 0x4, &base_addr);
+	base64_addr |= (u64)base_addr << 32;
+
+	/* Hide the P2SB device */
+	pci_bus_write_config_byte(pci_dev->bus, devfn, 0xe1, 0x1);
+	spin_unlock(&p2sb_spinlock);
+
+	res = &tco_res[ICH_RES_MEM_OFF];
+	res->start = (resource_size_t)base64_addr + SBREG_SMBCTRL;
+	res->end = res->start + 3;
+	res->flags = IORESOURCE_MEM;
+
+	pdev = platform_device_register_resndata(&pci_dev->dev, "iTCO_wdt", -1,
+						 tco_res, 3, &tco_platform_data,
+						 sizeof(tco_platform_data));
+	if (IS_ERR(pdev)) {
+		dev_warn(&pci_dev->dev, "failed to create iTCO device\n");
+		return;
+	}
+
+	priv->tco_pdev = pdev;
+}
+
 static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 {
 	unsigned char temp;
@@ -1149,6 +1254,15 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 
 	priv->pci_dev = dev;
 	switch (dev->device) {
+	case PCI_DEVICE_ID_INTEL_SUNRISEPOINT_H_SMBUS:
+	case PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_SMBUS:
+		priv->features |= FEATURE_I2C_BLOCK_READ;
+		priv->features |= FEATURE_IRQ;
+		priv->features |= FEATURE_SMBUS_PEC;
+		priv->features |= FEATURE_BLOCK_BUFFER;
+		priv->features |= FEATURE_TCO;
+		break;
+
 	case PCI_DEVICE_ID_INTEL_PATSBURG_SMBUS_IDF0:
 	case PCI_DEVICE_ID_INTEL_PATSBURG_SMBUS_IDF1:
 	case PCI_DEVICE_ID_INTEL_PATSBURG_SMBUS_IDF2:
@@ -1265,6 +1379,8 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	dev_info(&dev->dev, "SMBus using %s\n",
 		 priv->features & FEATURE_IRQ ? "PCI interrupt" : "polling");
 
+	i801_add_tco(priv);
+
 	/* set up the sysfs linkage to our parent device */
 	priv->adapter.dev.parent = &dev->dev;
 
@@ -1296,6 +1412,8 @@ static void i801_remove(struct pci_dev *dev)
 	i2c_del_adapter(&priv->adapter);
 	pci_write_config_byte(dev, SMBHSTCFG, priv->original_hstcfg);
 
+	platform_device_unregister(priv->tco_pdev);
+
 	/*
 	 * do not call pci_disable_device(dev) since it can cause hard hangs on
 	 * some systems during power-off (eg. Fujitsu-Siemens Lifebook E8010)
-- 
2.1.0

[PATCH v3 3/3] iTCO_wdt: Add support for TCO on Intel Sunrisepoint

[Matt Fleming]

From: Matt Fleming <matt.fleming@intel.com>

The revision of the watchdog hardware in Sunrisepoint necessitates a new
"version" inside the TCO watchdog driver because some of the register
layouts have changed.

Also update the Kconfig entry to select both the LPC and SMBus drivers
since the TCO device is on the SMBus in Sunrisepoint.

Cc: Wim Van Sebroeck <wim@iguana.be>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Cc: Jean Delvare <jdelvare@suse.com>
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---

v3:
 - Added Guenter's Review tag

v2: 
 - Explicitly list TCO versions and their "no reboot" bits in the switch
   statement in no_reboot_bit()
 - Use a switch/case statement when clearing status bits instead of
   convoluted if/else branching
 - Select both of the bus drivers instead of using depends

 drivers/watchdog/Kconfig    |  3 ++-
 drivers/watchdog/iTCO_wdt.c | 66 ++++++++++++++++++++++++++++-----------------
 2 files changed, 43 insertions(+), 26 deletions(-)

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 241fafde42cb..55c4b5b0a317 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -797,7 +797,8 @@ config ITCO_WDT
 	tristate "Intel TCO Timer/Watchdog"
 	depends on (X86 || IA64) && PCI
 	select WATCHDOG_CORE
-	select LPC_ICH
+	select LPC_ICH if !EXPERT
+	select I2C_I801 if !EXPERT
 	---help---
 	  Hardware driver for the intel TCO timer based watchdog devices.
 	  These drivers are included in the Intel 82801 I/O Controller
diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
index a94401b2deca..7b8949d48029 100644
--- a/drivers/watchdog/iTCO_wdt.c
+++ b/drivers/watchdog/iTCO_wdt.c
@@ -145,58 +145,67 @@ static inline unsigned int ticks_to_seconds(int ticks)
 	return iTCO_wdt_private.iTCO_version == 3 ? ticks : (ticks * 6) / 10;
 }
 
+static inline u32 no_reboot_bit(void)
+{
+	u32 enable_bit;
+
+	switch (iTCO_wdt_private.iTCO_version) {
+	case 3:
+		enable_bit = 0x00000010;
+		break;
+	case 2:
+		enable_bit = 0x00000020;
+		break;
+	case 4:
+	case 1:
+	default:
+		enable_bit = 0x00000002;
+		break;
+	}
+
+	return enable_bit;
+}
+
 static void iTCO_wdt_set_NO_REBOOT_bit(void)
 {
 	u32 val32;
 
 	/* Set the NO_REBOOT bit: this disables reboots */
-	if (iTCO_wdt_private.iTCO_version == 3) {
-		val32 = readl(iTCO_wdt_private.gcs_pmc);
-		val32 |= 0x00000010;
-		writel(val32, iTCO_wdt_private.gcs_pmc);
-	} else if (iTCO_wdt_private.iTCO_version == 2) {
+	if (iTCO_wdt_private.iTCO_version >= 2) {
 		val32 = readl(iTCO_wdt_private.gcs_pmc);
-		val32 |= 0x00000020;
+		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 |= 0x00000002;
+		val32 |= no_reboot_bit();
 		pci_write_config_dword(iTCO_wdt_private.pdev, 0xd4, val32);
 	}
 }
 
 static int iTCO_wdt_unset_NO_REBOOT_bit(void)
 {
+	u32 enable_bit = no_reboot_bit();
 	int ret = 0;
-	u32 val32;
+	u32 val32 = 0;
 
 	/* Unset the NO_REBOOT bit: this enables reboots */
-	if (iTCO_wdt_private.iTCO_version == 3) {
-		val32 = readl(iTCO_wdt_private.gcs_pmc);
-		val32 &= 0xffffffef;
-		writel(val32, iTCO_wdt_private.gcs_pmc);
-
-		val32 = readl(iTCO_wdt_private.gcs_pmc);
-		if (val32 & 0x00000010)
-			ret = -EIO;
-	} else if (iTCO_wdt_private.iTCO_version == 2) {
+	if (iTCO_wdt_private.iTCO_version >= 2) {
 		val32 = readl(iTCO_wdt_private.gcs_pmc);
-		val32 &= 0xffffffdf;
+		val32 &= ~enable_bit;
 		writel(val32, iTCO_wdt_private.gcs_pmc);
 
 		val32 = readl(iTCO_wdt_private.gcs_pmc);
-		if (val32 & 0x00000020)
-			ret = -EIO;
 	} else if (iTCO_wdt_private.iTCO_version == 1) {
 		pci_read_config_dword(iTCO_wdt_private.pdev, 0xd4, &val32);
-		val32 &= 0xfffffffd;
+		val32 &= ~enable_bit;
 		pci_write_config_dword(iTCO_wdt_private.pdev, 0xd4, val32);
 
 		pci_read_config_dword(iTCO_wdt_private.pdev, 0xd4, &val32);
-		if (val32 & 0x00000002)
-			ret = -EIO;
 	}
 
+	if (val32 & enable_bit)
+		ret = -EIO;
+
 	return ret; /* returns: 0 = OK, -EIO = Error */
 }
 
@@ -503,12 +512,19 @@ static int iTCO_wdt_probe(struct platform_device *dev)
 		pdata->name, pdata->version, (u64)TCOBASE);
 
 	/* Clear out the (probably old) status */
-	if (iTCO_wdt_private.iTCO_version == 3) {
+	switch (iTCO_wdt_private.iTCO_version) {
+	case 4:
+		outw(0x0008, TCO1_STS);	/* Clear the Time Out Status bit */
+		outw(0x0002, TCO2_STS);	/* Clear SECOND_TO_STS bit */
+		break;
+	case 3:
 		outl(0x20008, TCO1_STS);
-	} else {
+		break;
+	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 */
+		break;
 	}
 
 	iTCO_wdt_watchdog_dev.bootstatus = 0;
-- 
2.1.0

Re: [PATCH v3 3/3] iTCO_wdt: Add support for TCO on Intel Sunrisepoint

[Andy Shevchenko]

On Thu, 2015-07-30 at 22:59 +0100, Matt Fleming wrote:
> From: Matt Fleming <matt.fleming@intel.com>
> 
> The revision of the watchdog hardware in Sunrisepoint necessitates a 
> new
> "version" inside the TCO watchdog driver because some of the register
> layouts have changed.
> 
> Also update the Kconfig entry to select both the LPC and SMBus 
> drivers
> since the TCO device is on the SMBus in Sunrisepoint.


Few minor comments below, though I'm okay with current version as well.

> 
> Cc: Wim Van Sebroeck <wim@iguana.be>
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> Cc: Jean Delvare <jdelvare@suse.com>
> Signed-off-by: Matt Fleming <matt.fleming@intel.com>
> ---
> 
> v3:
>  - Added Guenter's Review tag
> 
> v2: 
>  - Explicitly list TCO versions and their "no reboot" bits in the 
> switch
>    statement in no_reboot_bit()
>  - Use a switch/case statement when clearing status bits instead of
>    convoluted if/else branching
>  - Select both of the bus drivers instead of using depends
> 
>  drivers/watchdog/Kconfig    |  3 ++-
>  drivers/watchdog/iTCO_wdt.c | 66 ++++++++++++++++++++++++++++-------
> ----------
>  2 files changed, 43 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 241fafde42cb..55c4b5b0a317 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -797,7 +797,8 @@ config ITCO_WDT
>  	tristate "Intel TCO Timer/Watchdog"
>  	depends on (X86 || IA64) && PCI
>  	select WATCHDOG_CORE
> -	select LPC_ICH
> +	select LPC_ICH if !EXPERT
> +	select I2C_I801 if !EXPERT
>  	---help---
>  	  Hardware driver for the intel TCO timer based watchdog 
> devices.
>  	  These drivers are included in the Intel 82801 I/O 
> Controller
> diff --git a/drivers/watchdog/iTCO_wdt.c 
> b/drivers/watchdog/iTCO_wdt.c
> index a94401b2deca..7b8949d48029 100644
> --- a/drivers/watchdog/iTCO_wdt.c
> +++ b/drivers/watchdog/iTCO_wdt.c
> @@ -145,58 +145,67 @@ static inline unsigned int ticks_to_seconds(int 
> ticks)
>  	return iTCO_wdt_private.iTCO_version == 3 ? ticks : (ticks * 
> 6) / 10;
>  }
>  
> +static inline u32 no_reboot_bit(void)
> +{
> +	u32 enable_bit;
> +
> +	switch (iTCO_wdt_private.iTCO_version) {
> +	case 3:
> +		enable_bit = 0x00000010;
> +		break;
> +	case 2:
> +		enable_bit = 0x00000020;
> +		break;
> +	case 4:
> +	case 1:
> +	default:
> +		enable_bit = 0x00000002;
> +		break;
> +	}
> +
> +	return enable_bit;
> +}
> +
>  static void iTCO_wdt_set_NO_REBOOT_bit(void)
>  {
>  	u32 val32;
>  
>  	/* Set the NO_REBOOT bit: this disables reboots */
> -	if (iTCO_wdt_private.iTCO_version == 3) {
> -		val32 = readl(iTCO_wdt_private.gcs_pmc);
> -		val32 |= 0x00000010;
> -		writel(val32, iTCO_wdt_private.gcs_pmc);
> -	} else if (iTCO_wdt_private.iTCO_version == 2) {
> +	if (iTCO_wdt_private.iTCO_version >= 2) {
>  		val32 = readl(iTCO_wdt_private.gcs_pmc);
> -		val32 |= 0x00000020;
> +		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 |= 0x00000002;
> +		val32 |= no_reboot_bit();
>  		pci_write_config_dword(iTCO_wdt_private.pdev, 0xd4, 
> val32);
>  	}
>  }
>  
>  static int iTCO_wdt_unset_NO_REBOOT_bit(void)
>  {
> +	u32 enable_bit = no_reboot_bit();
>  	int ret = 0;
> -	u32 val32;
> +	u32 val32 = 0;
>  
>  	/* Unset the NO_REBOOT bit: this enables reboots */
> -	if (iTCO_wdt_private.iTCO_version == 3) {
> -		val32 = readl(iTCO_wdt_private.gcs_pmc);
> -		val32 &= 0xffffffef;
> -		writel(val32, iTCO_wdt_private.gcs_pmc);
> -
> -		val32 = readl(iTCO_wdt_private.gcs_pmc);
> -		if (val32 & 0x00000010)
> -			ret = -EIO;
> -	} else if (iTCO_wdt_private.iTCO_version == 2) {
> +	if (iTCO_wdt_private.iTCO_version >= 2) {
>  		val32 = readl(iTCO_wdt_private.gcs_pmc);
> -		val32 &= 0xffffffdf;
> +		val32 &= ~enable_bit;
>  		writel(val32, iTCO_wdt_private.gcs_pmc);
>  
>  		val32 = readl(iTCO_wdt_private.gcs_pmc);
> -		if (val32 & 0x00000020)
> -			ret = -EIO;
>  	} else if (iTCO_wdt_private.iTCO_version == 1) {
>  		pci_read_config_dword(iTCO_wdt_private.pdev, 0xd4, 
> &val32);
> -		val32 &= 0xfffffffd;
> +		val32 &= ~enable_bit;
>  		pci_write_config_dword(iTCO_wdt_private.pdev, 0xd4, 
> val32);
>  
>  		pci_read_config_dword(iTCO_wdt_private.pdev, 0xd4, 
> &val32);
> -		if (val32 & 0x00000002)
> -			ret = -EIO;
>  	}
>  
> +	if (val32 & enable_bit)
> +		ret = -EIO;
> +
>  	return ret; /* returns: 0 = OK, -EIO = Error */

What about removing ret at all?

if (val32 & enable_bit)
 return -EIO;

return 0;

>  }
>  
> @@ -503,12 +512,19 @@ static int iTCO_wdt_probe(struct 
> platform_device *dev)
>  		pdata->name, pdata->version, (u64)TCOBASE);
>  
>  	/* Clear out the (probably old) status */
> -	if (iTCO_wdt_private.iTCO_version == 3) {
> +	switch (iTCO_wdt_private.iTCO_version) {
> +	case 4:
> +		outw(0x0008, TCO1_STS);	/* Clear the Time 
> Out Status bit */
> +		outw(0x0002, TCO2_STS);	/* Clear 
> SECOND_TO_STS bit */
> +		break;
> +	case 3:
>  		outl(0x20008, TCO1_STS);
> -	} else {
> +		break;
> +	default:

Same idea here: put cases explicitly for all existing versions?

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 */
> +		break;
>  	}
>  
>  	iTCO_wdt_watchdog_dev.bootstatus = 0;

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v3 1/3] iTCO_wdt: Expose watchdog properties using platform data

[Darren Hart]

On Thu, Jul 30, 2015 at 10:58:59PM +0100, Matt Fleming wrote:
> From: Matt Fleming <matt.fleming@intel.com>
> 
> Intel Sunrisepoint (Skylake PCH) has the iTCO watchdog accessible across
> the SMBus, unlike previous generations of PCH/ICH where it was on the
> LPC bus. Because it's on the SMBus, it doesn't make sense to pass around
> a 'struct lpc_ich_info', and leaking the type of bus into the iTCO
> watchdog driver is kind of backwards anyway.
> 
> This change introduces a new 'struct itco_wdt_platform_data' for use
> inside the iTCO watchdog driver and by the upcoming Intel Sunrisepoint
> code, which neatly avoids having to include lpc_ich headers in the i801
> i2c driver.
> 
> This change is overdue because lpc_ich_info has already found its way
> into other TCO watchdog users, notably the intel_pmc_ipc driver where
> the watchdog actually isn't on the LPC bus as far as I can see.
> 
> A simple translation layer is provided for converting from the existing
> 'struct lpc_ich_info' inside the lpc_ich mfd driver.
> 
> Cc: Peter Tyser <ptyser@xes-inc.com>
> Cc: Samuel Ortiz <sameo@linux.intel.com>
> Acked-by: Lee Jones <lee.jones@linaro.org>
> Cc: Wim Van Sebroeck <wim@iguana.be>
> Cc: Zha Qipeng <qipeng.zha@intel.com>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Jean Delvare <jdelvare@suse.com>
> Signed-off-by: Matt Fleming <matt.fleming@intel.com>

For platform/drivers/x86:

Simple changes to accomodate refactoring.

Acked-by: Darren Hart <dvhart@linux.intel.com>

> ---
> 
> v3:
>  - Added Lee's ACK
> 
> v2:
>  - Use lowercase "itco" in all new files and data structures.
>  - Use devm_kzalloc() to allocate platform data, fixing a memory leak
>    and saving us from having to free the allocation in error paths
>  - Move stray Kconfig hunk to PATCH 3 that accidentally snuck into v1
>  - Swap strcpy() for the safer strlcpy()
>  - Fix lpc_ich_info user in the intel_pmc_ipc driver that was merged for
>    v4.2.
> 
>  drivers/mfd/lpc_ich.c                  | 32 +++++++++++++++++++++++++++++---
>  drivers/platform/x86/intel_pmc_ipc.c   |  9 ++++-----
>  drivers/watchdog/iTCO_wdt.c            | 11 +++++------
>  include/linux/mfd/lpc_ich.h            |  6 ------
>  include/linux/platform_data/itco_wdt.h | 19 +++++++++++++++++++
>  5 files changed, 57 insertions(+), 20 deletions(-)
>  create mode 100644 include/linux/platform_data/itco_wdt.h
> 
> diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
> index 8de34398abc0..c5a9a08b5dfb 100644
> --- a/drivers/mfd/lpc_ich.c
> +++ b/drivers/mfd/lpc_ich.c
> @@ -66,6 +66,7 @@
>  #include <linux/pci.h>
>  #include <linux/mfd/core.h>
>  #include <linux/mfd/lpc_ich.h>
> +#include <linux/platform_data/itco_wdt.h>
>  
>  #define ACPIBASE		0x40
>  #define ACPIBASE_GPE_OFF	0x28
> @@ -835,9 +836,31 @@ static void lpc_ich_enable_pmc_space(struct pci_dev *dev)
>  	priv->actrl_pbase_save = reg_save;
>  }
>  
> -static void lpc_ich_finalize_cell(struct pci_dev *dev, struct mfd_cell *cell)
> +static int lpc_ich_finalize_wdt_cell(struct pci_dev *dev)
>  {
> +	struct itco_wdt_platform_data *pdata;
>  	struct lpc_ich_priv *priv = pci_get_drvdata(dev);
> +	struct lpc_ich_info *info;
> +	struct mfd_cell *cell = &lpc_ich_cells[LPC_WDT];
> +
> +	pdata = devm_kzalloc(&dev->dev, sizeof(*pdata), GFP_KERNEL);
> +	if (!pdata)
> +		return -ENOMEM;
> +
> +	info = &lpc_chipset_info[priv->chipset];
> +
> +	pdata->version = info->iTCO_version;
> +	strlcpy(pdata->name, info->name, sizeof(pdata->name));
> +
> +	cell->platform_data = pdata;
> +	cell->pdata_size = sizeof(*pdata);
> +	return 0;
> +}
> +
> +static void lpc_ich_finalize_gpio_cell(struct pci_dev *dev)
> +{
> +	struct lpc_ich_priv *priv = pci_get_drvdata(dev);
> +	struct mfd_cell *cell = &lpc_ich_cells[LPC_GPIO];
>  
>  	cell->platform_data = &lpc_chipset_info[priv->chipset];
>  	cell->pdata_size = sizeof(struct lpc_ich_info);
> @@ -933,7 +956,7 @@ gpe0_done:
>  	lpc_chipset_info[priv->chipset].use_gpio = ret;
>  	lpc_ich_enable_gpio_space(dev);
>  
> -	lpc_ich_finalize_cell(dev, &lpc_ich_cells[LPC_GPIO]);
> +	lpc_ich_finalize_gpio_cell(dev);
>  	ret = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
>  			      &lpc_ich_cells[LPC_GPIO], 1, NULL, 0, NULL);
>  
> @@ -1007,7 +1030,10 @@ static int lpc_ich_init_wdt(struct pci_dev *dev)
>  		res->end = base_addr + ACPIBASE_PMC_END;
>  	}
>  
> -	lpc_ich_finalize_cell(dev, &lpc_ich_cells[LPC_WDT]);
> +	ret = lpc_ich_finalize_wdt_cell(dev);
> +	if (ret)
> +		goto wdt_done;
> +
>  	ret = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
>  			      &lpc_ich_cells[LPC_WDT], 1, NULL, 0, NULL);
>  
> diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c
> index 105cfffe82c6..28b2a12bb26d 100644
> --- a/drivers/platform/x86/intel_pmc_ipc.c
> +++ b/drivers/platform/x86/intel_pmc_ipc.c
> @@ -33,7 +33,7 @@
>  #include <linux/suspend.h>
>  #include <linux/acpi.h>
>  #include <asm/intel_pmc_ipc.h>
> -#include <linux/mfd/lpc_ich.h>
> +#include <linux/platform_data/itco_wdt.h>
>  
>  /*
>   * IPC registers
> @@ -473,9 +473,9 @@ static struct resource tco_res[] = {
>  	},
>  };
>  
> -static struct lpc_ich_info tco_info = {
> +static struct itco_wdt_platform_data tco_info = {
>  	.name = "Apollo Lake SoC",
> -	.iTCO_version = 3,
> +	.version = 3,
>  };
>  
>  static int ipc_create_punit_device(void)
> @@ -552,8 +552,7 @@ static int ipc_create_tco_device(void)
>  		goto err;
>  	}
>  
> -	ret = platform_device_add_data(pdev, &tco_info,
> -				       sizeof(struct lpc_ich_info));
> +	ret = platform_device_add_data(pdev, &tco_info, sizeof(tco_info));
>  	if (ret) {
>  		dev_err(ipcdev.dev, "Failed to add tco platform data\n");
>  		goto err;
> diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
> index 3c3fd417ddeb..a94401b2deca 100644
> --- a/drivers/watchdog/iTCO_wdt.c
> +++ b/drivers/watchdog/iTCO_wdt.c
> @@ -66,8 +66,7 @@
>  #include <linux/spinlock.h>		/* For spin_lock/spin_unlock/... */
>  #include <linux/uaccess.h>		/* For copy_to_user/put_user/... */
>  #include <linux/io.h>			/* For inb/outb/... */
> -#include <linux/mfd/core.h>
> -#include <linux/mfd/lpc_ich.h>
> +#include <linux/platform_data/itco_wdt.h>
>  
>  #include "iTCO_vendor.h"
>  
> @@ -418,9 +417,9 @@ static int iTCO_wdt_probe(struct platform_device *dev)
>  {
>  	int ret = -ENODEV;
>  	unsigned long val32;
> -	struct lpc_ich_info *ich_info = dev_get_platdata(&dev->dev);
> +	struct itco_wdt_platform_data *pdata = dev_get_platdata(&dev->dev);
>  
> -	if (!ich_info)
> +	if (!pdata)
>  		goto out;
>  
>  	spin_lock_init(&iTCO_wdt_private.io_lock);
> @@ -435,7 +434,7 @@ static int iTCO_wdt_probe(struct platform_device *dev)
>  	if (!iTCO_wdt_private.smi_res)
>  		goto out;
>  
> -	iTCO_wdt_private.iTCO_version = ich_info->iTCO_version;
> +	iTCO_wdt_private.iTCO_version = pdata->version;
>  	iTCO_wdt_private.dev = dev;
>  	iTCO_wdt_private.pdev = to_pci_dev(dev->dev.parent);
>  
> @@ -501,7 +500,7 @@ static int iTCO_wdt_probe(struct platform_device *dev)
>  	}
>  
>  	pr_info("Found a %s TCO device (Version=%d, TCOBASE=0x%04llx)\n",
> -		ich_info->name, ich_info->iTCO_version, (u64)TCOBASE);
> +		pdata->name, pdata->version, (u64)TCOBASE);
>  
>  	/* Clear out the (probably old) status */
>  	if (iTCO_wdt_private.iTCO_version == 3) {
> diff --git a/include/linux/mfd/lpc_ich.h b/include/linux/mfd/lpc_ich.h
> index 8feac782fa83..2b300b44f994 100644
> --- a/include/linux/mfd/lpc_ich.h
> +++ b/include/linux/mfd/lpc_ich.h
> @@ -20,12 +20,6 @@
>  #ifndef LPC_ICH_H
>  #define LPC_ICH_H
>  
> -/* Watchdog resources */
> -#define ICH_RES_IO_TCO		0
> -#define ICH_RES_IO_SMI		1
> -#define ICH_RES_MEM_OFF		2
> -#define ICH_RES_MEM_GCS_PMC	0
> -
>  /* GPIO resources */
>  #define ICH_RES_GPIO	0
>  #define ICH_RES_GPE0	1
> diff --git a/include/linux/platform_data/itco_wdt.h b/include/linux/platform_data/itco_wdt.h
> new file mode 100644
> index 000000000000..f16542c77ff7
> --- /dev/null
> +++ b/include/linux/platform_data/itco_wdt.h
> @@ -0,0 +1,19 @@
> +/*
> + * Platform data for the Intel TCO Watchdog
> + */
> +
> +#ifndef _ITCO_WDT_H_
> +#define _ITCO_WDT_H_
> +
> +/* Watchdog resources */
> +#define ICH_RES_IO_TCO		0
> +#define ICH_RES_IO_SMI		1
> +#define ICH_RES_MEM_OFF		2
> +#define ICH_RES_MEM_GCS_PMC	0
> +
> +struct itco_wdt_platform_data {
> +	char name[32];
> +	unsigned int version;
> +};
> +
> +#endif /* _ITCO_WDT_H_ */
> -- 
> 2.1.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH v3 3/3] iTCO_wdt: Add support for TCO on Intel Sunrisepoint

[Matt Fleming]

(Sorry for the delay in replying)

On Fri, 31 Jul, at 11:41:04AM, Andy Shevchenko wrote:
> >  
> > +	if (val32 & enable_bit)
> > +		ret = -EIO;
> > +
> >  	return ret; /* returns: 0 = OK, -EIO = Error */
> 
> What about removing ret at all?
> 
> if (val32 & enable_bit)
>  return -EIO;
> 
> return 0;
 
Yeah, good catch. I'll make this change.

> >  }
> >  
> > @@ -503,12 +512,19 @@ static int iTCO_wdt_probe(struct 
> > platform_device *dev)
> >  		pdata->name, pdata->version, (u64)TCOBASE);
> >  
> >  	/* Clear out the (probably old) status */
> > -	if (iTCO_wdt_private.iTCO_version == 3) {
> > +	switch (iTCO_wdt_private.iTCO_version) {
> > +	case 4:
> > +		outw(0x0008, TCO1_STS);	/* Clear the Time 
> > Out Status bit */
> > +		outw(0x0002, TCO2_STS);	/* Clear 
> > SECOND_TO_STS bit */
> > +		break;
> > +	case 3:
> >  		outl(0x20008, TCO1_STS);
> > -	} else {
> > +		break;
> > +	default:
> 
> Same idea here: put cases explicitly for all existing versions?
> 
> case 2:
> case 1:
> default:
 
I intentionally didn't do that because I figured it was implied that
versions 2 and 1 are included in the default case. But I've no problem
with making this change. I'll send a v4.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH v3 3/3] iTCO_wdt: Add support for TCO on Intel Sunrisepoint

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 501 bytes --]

On Thu, Aug 06, 2015 at 01:00:24PM +0100, Matt Fleming wrote:
> (Sorry for the delay in replying)
> 
> On Fri, 31 Jul, at 11:41:04AM, Andy Shevchenko wrote:
> > >  
> > > +	if (val32 & enable_bit)
> > > +		ret = -EIO;
> > > +
> > >  	return ret; /* returns: 0 = OK, -EIO = Error */
> > 
> > What about removing ret at all?
> > 
> > if (val32 & enable_bit)
> >  return -EIO;
> > 
> > return 0;
>  
> Yeah, good catch. I'll make this change.

I'd go for the ternary operator here.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]