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

[PATCH 0/5] 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!

Andy Shevchenko (2):
  iTCO_wdt: fixup for the header
  i2c-i801: fixup regarding watchdog timer

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          | 128 ++++++++++++++++++++++++++++++++-
 drivers/mfd/lpc_ich.c                  |  32 ++++++++-
 drivers/watchdog/Kconfig               |   2 +-
 drivers/watchdog/iTCO_wdt.c            |  69 ++++++++++--------
 include/linux/mfd/lpc_ich.h            |   6 --
 include/linux/platform_data/iTCO_wdt.h |  19 +++++
 6 files changed, 215 insertions(+), 41 deletions(-)
 create mode 100644 include/linux/platform_data/iTCO_wdt.h

-- 
2.1.0

[PATCH 1/5] 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.

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>
Cc: Lee Jones <lee.jones@linaro.org>
Cc: Wim Van Sebroeck <wim@iguana.be>
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---
 drivers/mfd/lpc_ich.c                  | 32 +++++++++++++++++++++++++++++---
 drivers/watchdog/Kconfig               |  2 +-
 drivers/watchdog/iTCO_wdt.c            | 11 +++++------
 include/linux/mfd/lpc_ich.h            |  6 ------
 include/linux/platform_data/iTCO_wdt.h | 18 ++++++++++++++++++
 5 files changed, 53 insertions(+), 16 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..d190b74a6321 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 = kzalloc(sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return -ENOMEM;
+
+	info = &lpc_chipset_info[priv->chipset];
+
+	pdata->iTCO_version = info->iTCO_version;
+	strcpy(pdata->name, info->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/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 241fafde42cb..5336fe2ff689 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -797,7 +797,7 @@ config ITCO_WDT
 	tristate "Intel TCO Timer/Watchdog"
 	depends on (X86 || IA64) && PCI
 	select WATCHDOG_CORE
-	select LPC_ICH
+	depends on LPC_ICH || I2C_I801
 	---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 3c3fd417ddeb..9a6e70976f64 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->iTCO_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->iTCO_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..ce53c2b01f6d
--- /dev/null
+++ b/include/linux/platform_data/iTCO_wdt.h
@@ -0,0 +1,18 @@
+/*
+ * Platform data for the Intel TCO Watchdog
+ */
+
+#ifndef _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 iTCO_version;
+};
+
+#endif /* _ITCO_WDT_H_ */
-- 
2.1.0

[PATCH 2/5] 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>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---
 drivers/i2c/busses/i2c-i801.c | 127 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 126 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 5ecbb3fdc27e..c79dbe116ccc 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		BIT(7)
+
+#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		BIT(8)
+
 /* 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,102 @@ 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",
+	.iTCO_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], 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[0], 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 void i801_del_tco(struct i801_priv *priv)
+{
+	if (priv->tco_pdev) {
+		platform_device_unregister(priv->tco_pdev);
+		priv->tco_pdev = NULL;
+	}
+}
+
 static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 {
 	unsigned char temp;
@@ -1149,6 +1261,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 +1386,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 +1419,8 @@ static void i801_remove(struct pci_dev *dev)
 	i2c_del_adapter(&priv->adapter);
 	pci_write_config_byte(dev, SMBHSTCFG, priv->original_hstcfg);
 
+	i801_del_tco(priv);
+
 	/*
 	 * 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 3/5] 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.

Cc: Wim Van Sebroeck <wim@iguana.be>
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---
 drivers/watchdog/iTCO_wdt.c | 58 ++++++++++++++++++++++++++-------------------
 1 file changed, 34 insertions(+), 24 deletions(-)

diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
index 9a6e70976f64..17dfbc51b85a 100644
--- a/drivers/watchdog/iTCO_wdt.c
+++ b/drivers/watchdog/iTCO_wdt.c
@@ -145,58 +145,65 @@ 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;
+	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,7 +510,10 @@ static int iTCO_wdt_probe(struct platform_device *dev)
 		pdata->name, pdata->iTCO_version, (u64)TCOBASE);
 
 	/* Clear out the (probably old) status */
-	if (iTCO_wdt_private.iTCO_version == 3) {
+	if (iTCO_wdt_private.iTCO_version == 4) {
+		outw(0x0008, TCO1_STS);	/* Clear the Time Out Status bit */
+		outw(0x0002, TCO2_STS);	/* Clear SECOND_TO_STS bit */
+	} else if (iTCO_wdt_private.iTCO_version == 3) {
 		outl(0x20008, TCO1_STS);
 	} else {
 		outw(0x0008, TCO1_STS);	/* Clear the Time Out Status bit */
-- 
2.1.0

[PATCH 4/5] iTCO_wdt: fixup for the header

[Matt Fleming]

From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Prevent nested inclusion of the header.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Wim Van Sebroeck <wim@iguana.be>
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---
 include/linux/platform_data/iTCO_wdt.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/platform_data/iTCO_wdt.h b/include/linux/platform_data/iTCO_wdt.h
index ce53c2b01f6d..0d4f0e87594f 100644
--- a/include/linux/platform_data/iTCO_wdt.h
+++ b/include/linux/platform_data/iTCO_wdt.h
@@ -3,6 +3,7 @@
  */
 
 #ifndef _ITCO_WDT_H_
+#define _ITCO_WDT_H_
 
 /* Watchdog resources */
 #define ICH_RES_IO_TCO		0
-- 
2.1.0

[PATCH 5/5] i2c-i801: fixup regarding watchdog timer

[Matt Fleming]

From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Change &array[0] to array since it's the same.

It fixes 80 character limit warning as well.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Jean Delvare <jdelvare@suse.com>
Cc: Wolfram Sang <wsa@the-dreams.de>
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---
 drivers/i2c/busses/i2c-i801.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index c79dbe116ccc..e41005927746 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1173,7 +1173,8 @@ static void i801_add_tco(struct i801_priv *priv)
 	if (!(tco_ctl & TCOCTL_EN))
 		return;
 
-	memset(&tco_res[0], 0, sizeof(tco_res));
+	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;
@@ -1226,7 +1227,7 @@ static void i801_add_tco(struct i801_priv *priv)
 	res->flags = IORESOURCE_MEM;
 
 	pdev = platform_device_register_resndata(&pci_dev->dev, "iTCO_wdt", -1,
-						 &tco_res[0], 3, &tco_platform_data,
+						 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");
-- 
2.1.0

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

[Guenter Roeck]

On 07/27/2015 06:38 AM, 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.
>
> 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>
> Cc: Lee Jones <lee.jones@linaro.org>
> Cc: Wim Van Sebroeck <wim@iguana.be>
> Signed-off-by: Matt Fleming <matt.fleming@intel.com>
> ---
>   drivers/mfd/lpc_ich.c                  | 32 +++++++++++++++++++++++++++++---
>   drivers/watchdog/Kconfig               |  2 +-
>   drivers/watchdog/iTCO_wdt.c            | 11 +++++------
>   include/linux/mfd/lpc_ich.h            |  6 ------
>   include/linux/platform_data/iTCO_wdt.h | 18 ++++++++++++++++++
>   5 files changed, 53 insertions(+), 16 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..d190b74a6321 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 = kzalloc(sizeof(*pdata), GFP_KERNEL);
> +	if (!pdata)
> +		return -ENOMEM;
> +

I don't see the platform data freed anywhere, neither in the error path nor
in the cleanup path of this driver. Can you use devm_kzalloc() ?
Otherwise I think you'll need a cleanup path.

Guenter

Re: [PATCH 2/5] i2c: i801: Create iTCO device on newer Intel PCHs

[Guenter Roeck]

On 07/27/2015 06:38 AM, Matt Fleming wrote:
> 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>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Signed-off-by: Matt Fleming <matt.fleming@intel.com>

That really asks for an mfd driver, but that might be a bit overkill.
Copying the i2c mailing list for additional feedback.

> ---
>   drivers/i2c/busses/i2c-i801.c | 127 +++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 126 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 5ecbb3fdc27e..c79dbe116ccc 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		BIT(7)

If you use BIT, you should include bitops.h.
Not sure if that makes too much sense here, though, without converting
the rest of the driver to use BIT as well.

> +
> +#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		BIT(8)
> +
>   /* 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,102 @@ 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",
> +	.iTCO_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], 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[0], 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 void i801_del_tco(struct i801_priv *priv)
> +{
> +	if (priv->tco_pdev) {

platform_device_unregister() handles NULL pointers, so this if statement
is strictly speaking unnecessary.

> +		platform_device_unregister(priv->tco_pdev);
> +		priv->tco_pdev = NULL;

Unnecessary; priv is going to be freed right afterwards.

> +	}
> +}
> +
>   static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>   {
>   	unsigned char temp;
> @@ -1149,6 +1261,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 +1386,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 +1419,8 @@ static void i801_remove(struct pci_dev *dev)
>   	i2c_del_adapter(&priv->adapter);
>   	pci_write_config_byte(dev, SMBHSTCFG, priv->original_hstcfg);
>
> +	i801_del_tco(priv);
> +
>   	/*
>   	 * do not call pci_disable_device(dev) since it can cause hard hangs on
>   	 * some systems during power-off (eg. Fujitsu-Siemens Lifebook E8010)
>

Re: [PATCH 4/5] iTCO_wdt: fixup for the header

[Guenter Roeck]

On 07/27/2015 06:38 AM, Matt Fleming wrote:
> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
> Prevent nested inclusion of the header.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Wim Van Sebroeck <wim@iguana.be>
> Signed-off-by: Matt Fleming <matt.fleming@intel.com>

Should be merged into patch 1.

Guenter

> ---
>   include/linux/platform_data/iTCO_wdt.h | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/include/linux/platform_data/iTCO_wdt.h b/include/linux/platform_data/iTCO_wdt.h
> index ce53c2b01f6d..0d4f0e87594f 100644
> --- a/include/linux/platform_data/iTCO_wdt.h
> +++ b/include/linux/platform_data/iTCO_wdt.h
> @@ -3,6 +3,7 @@
>    */
>
>   #ifndef _ITCO_WDT_H_
> +#define _ITCO_WDT_H_
>
>   /* Watchdog resources */
>   #define ICH_RES_IO_TCO		0
>

Re: [PATCH 5/5] i2c-i801: fixup regarding watchdog timer

[Guenter Roeck]

On 07/27/2015 06:38 AM, Matt Fleming wrote:
> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
> Change &array[0] to array since it's the same.
>
> It fixes 80 character limit warning as well.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Jean Delvare <jdelvare@suse.com>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> Signed-off-by: Matt Fleming <matt.fleming@intel.com>

Should be merged into patch 2.

Guenter

> ---
>   drivers/i2c/busses/i2c-i801.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index c79dbe116ccc..e41005927746 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -1173,7 +1173,8 @@ static void i801_add_tco(struct i801_priv *priv)
>   	if (!(tco_ctl & TCOCTL_EN))
>   		return;
>
> -	memset(&tco_res[0], 0, sizeof(tco_res));
> +	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;
> @@ -1226,7 +1227,7 @@ static void i801_add_tco(struct i801_priv *priv)
>   	res->flags = IORESOURCE_MEM;
>
>   	pdev = platform_device_register_resndata(&pci_dev->dev, "iTCO_wdt", -1,
> -						 &tco_res[0], 3, &tco_platform_data,
> +						 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");
>

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

[Matt Fleming]

On Mon, 27 Jul, at 06:49:08AM, Guenter Roeck wrote:
> 
> I don't see the platform data freed anywhere, neither in the error path nor
> in the cleanup path of this driver. Can you use devm_kzalloc() ?
> Otherwise I think you'll need a cleanup path.

Oops, good catch. Yes, devm_kzalloc() can be used here, thanks!

-- 
Matt Fleming, Intel Open Source Technology Center

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

[Guenter Roeck]

On 07/27/2015 06:38 AM, 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.
>
> Cc: Wim Van Sebroeck <wim@iguana.be>
> Signed-off-by: Matt Fleming <matt.fleming@intel.com>
> ---
>   drivers/watchdog/iTCO_wdt.c | 58 ++++++++++++++++++++++++++-------------------
>   1 file changed, 34 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
> index 9a6e70976f64..17dfbc51b85a 100644
> --- a/drivers/watchdog/iTCO_wdt.c
> +++ b/drivers/watchdog/iTCO_wdt.c
> @@ -145,58 +145,65 @@ 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;
> +	default:
> +		enable_bit = 0x00000002;

I think it would be better to explicitly list versions 1 and 4 here,
for clarification. We don't know what Intel will come up with for version 5,
and by then no one will remember that bit 2 applies for version 1 and 4 only.

Thanks,
Guenter

> +		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,7 +510,10 @@ static int iTCO_wdt_probe(struct platform_device *dev)
>   		pdata->name, pdata->iTCO_version, (u64)TCOBASE);
>
>   	/* Clear out the (probably old) status */
> -	if (iTCO_wdt_private.iTCO_version == 3) {
> +	if (iTCO_wdt_private.iTCO_version == 4) {
> +		outw(0x0008, TCO1_STS);	/* Clear the Time Out Status bit */
> +		outw(0x0002, TCO2_STS);	/* Clear SECOND_TO_STS bit */
> +	} else if (iTCO_wdt_private.iTCO_version == 3) {
>   		outl(0x20008, TCO1_STS);
>   	} else {
>   		outw(0x0008, TCO1_STS);	/* Clear the Time Out Status bit */
>

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

[Guenter Roeck]

On 07/27/2015 07:19 AM, Matt Fleming wrote:
> On Mon, 27 Jul, at 06:49:08AM, Guenter Roeck wrote:
>>
>> I don't see the platform data freed anywhere, neither in the error path nor
>> in the cleanup path of this driver. Can you use devm_kzalloc() ?
>> Otherwise I think you'll need a cleanup path.
>
> Oops, good catch. Yes, devm_kzalloc() can be used here, thanks!
>
Or maybe just use a static data structure, like in the i2c driver.

Guenter

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

On Mon, 27 Jul 2015, 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.
> 
> A simple translation layer is provided for converting from the existing
> 'struct lpc_ich_info' inside the lpc_ich mfd driver.

Is this patch related to Andy's patch-set?

  https://lkml.org/lkml/2015/7/27/599

> Cc: Peter Tyser <ptyser@xes-inc.com>
> Cc: Samuel Ortiz <sameo@linux.intel.com>
> Cc: Lee Jones <lee.jones@linaro.org>
> Cc: Wim Van Sebroeck <wim@iguana.be>
> Signed-off-by: Matt Fleming <matt.fleming@intel.com>
> ---
>  drivers/mfd/lpc_ich.c                  | 32 +++++++++++++++++++++++++++++---
>  drivers/watchdog/Kconfig               |  2 +-
>  drivers/watchdog/iTCO_wdt.c            | 11 +++++------
>  include/linux/mfd/lpc_ich.h            |  6 ------
>  include/linux/platform_data/iTCO_wdt.h | 18 ++++++++++++++++++
>  5 files changed, 53 insertions(+), 16 deletions(-)
>  create mode 100644 include/linux/platform_data/iTCO_wdt.h

How are all of these changes related?

Why do they all have to be in a single patch?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Lee Jones]

On Mon, 27 Jul 2015, 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.
> 
> A simple translation layer is provided for converting from the existing
> 'struct lpc_ich_info' inside the lpc_ich mfd driver.

Is this patch related to Andy's patch-set?

  https://lkml.org/lkml/2015/7/27/599

> Cc: Peter Tyser <ptyser@xes-inc.com>
> Cc: Samuel Ortiz <sameo@linux.intel.com>
> Cc: Lee Jones <lee.jones@linaro.org>
> Cc: Wim Van Sebroeck <wim@iguana.be>
> Signed-off-by: Matt Fleming <matt.fleming@intel.com>
> ---
>  drivers/mfd/lpc_ich.c                  | 32 +++++++++++++++++++++++++++++---
>  drivers/watchdog/Kconfig               |  2 +-
>  drivers/watchdog/iTCO_wdt.c            | 11 +++++------
>  include/linux/mfd/lpc_ich.h            |  6 ------
>  include/linux/platform_data/iTCO_wdt.h | 18 ++++++++++++++++++
>  5 files changed, 53 insertions(+), 16 deletions(-)
>  create mode 100644 include/linux/platform_data/iTCO_wdt.h

How are all of these changes related?

Why do they all have to be in a single patch?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Andy Shevchenko]

On Mon, 2015-07-27 at 16:33 +0100, Lee Jones wrote:
> On Mon, 27 Jul 2015, 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.
> > 
> > A simple translation layer is provided for converting from the 
> > existing
> > 'struct lpc_ich_info' inside the lpc_ich mfd driver.
> 
> Is this patch related to Andy's patch-set?

No, it's a separate stuff. They are pretty independent as I can see.


> 
>   https://lkml.org/lkml/2015/7/27/599
> 
> > Cc: Peter Tyser <ptyser@xes-inc.com>
> > Cc: Samuel Ortiz <sameo@linux.intel.com>
> > Cc: Lee Jones <lee.jones@linaro.org>
> > Cc: Wim Van Sebroeck <wim@iguana.be>
> > Signed-off-by: Matt Fleming <matt.fleming@intel.com>
> > ---
> >  drivers/mfd/lpc_ich.c                  | 32 
> > +++++++++++++++++++++++++++++---
> >  drivers/watchdog/Kconfig               |  2 +-
> >  drivers/watchdog/iTCO_wdt.c            | 11 +++++------
> >  include/linux/mfd/lpc_ich.h            |  6 ------
> >  include/linux/platform_data/iTCO_wdt.h | 18 ++++++++++++++++++
> >  5 files changed, 53 insertions(+), 16 deletions(-)
> >  create mode 100644 include/linux/platform_data/iTCO_wdt.h
> 
> How are all of these changes related?
> 
> Why do they all have to be in a single patch?
> 

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

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

[Matt Fleming]

On Mon, 27 Jul, at 04:33:44PM, Lee Jones wrote:
> On Mon, 27 Jul 2015, 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.
> > 
> > A simple translation layer is provided for converting from the existing
> > 'struct lpc_ich_info' inside the lpc_ich mfd driver.
> 
> Is this patch related to Andy's patch-set?
> 
>   https://lkml.org/lkml/2015/7/27/599
 
Nope, the two are independent.

> > Cc: Peter Tyser <ptyser@xes-inc.com>
> > Cc: Samuel Ortiz <sameo@linux.intel.com>
> > Cc: Lee Jones <lee.jones@linaro.org>
> > Cc: Wim Van Sebroeck <wim@iguana.be>
> > Signed-off-by: Matt Fleming <matt.fleming@intel.com>
> > ---
> >  drivers/mfd/lpc_ich.c                  | 32 +++++++++++++++++++++++++++++---
> >  drivers/watchdog/Kconfig               |  2 +-
> >  drivers/watchdog/iTCO_wdt.c            | 11 +++++------
> >  include/linux/mfd/lpc_ich.h            |  6 ------
> >  include/linux/platform_data/iTCO_wdt.h | 18 ++++++++++++++++++
> >  5 files changed, 53 insertions(+), 16 deletions(-)
> >  create mode 100644 include/linux/platform_data/iTCO_wdt.h
> 
> How are all of these changes related?
 
They create the platform data struct and use it in lpc_ich and iTCO_wdt.
The Kconfig change shouldn't have snuck into this patch though, that's
wrong, sorry.

> Why do they all have to be in a single patch?

I'll happily split them into more patches if you'd prefer. Maybe one
that introduces the platform data structure and then a separate one that
uses it in iTCO_wdt and lpc_ich?

Technically we could also split the change between iTCO_wdt and lpc_ich,
but then we'd be passing in an 'lpc_ich_info' and pulling out a
'iTCO_wdt_platform_data' and that just seems crazy.

-- 
Matt Fleming, Intel Open Source Technology Center

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

[Lee Jones]

On Mon, 27 Jul 2015, Matt Fleming wrote:

> On Mon, 27 Jul, at 04:33:44PM, Lee Jones wrote:
> > On Mon, 27 Jul 2015, 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.
> > > 
> > > A simple translation layer is provided for converting from the existing
> > > 'struct lpc_ich_info' inside the lpc_ich mfd driver.
> > 
> > Is this patch related to Andy's patch-set?
> > 
> >   https://lkml.org/lkml/2015/7/27/599
>  
> Nope, the two are independent.
> 
> > > Cc: Peter Tyser <ptyser@xes-inc.com>
> > > Cc: Samuel Ortiz <sameo@linux.intel.com>
> > > Cc: Lee Jones <lee.jones@linaro.org>
> > > Cc: Wim Van Sebroeck <wim@iguana.be>
> > > Signed-off-by: Matt Fleming <matt.fleming@intel.com>
> > > ---
> > >  drivers/mfd/lpc_ich.c                  | 32 +++++++++++++++++++++++++++++---
> > >  drivers/watchdog/Kconfig               |  2 +-
> > >  drivers/watchdog/iTCO_wdt.c            | 11 +++++------
> > >  include/linux/mfd/lpc_ich.h            |  6 ------
> > >  include/linux/platform_data/iTCO_wdt.h | 18 ++++++++++++++++++
> > >  5 files changed, 53 insertions(+), 16 deletions(-)
> > >  create mode 100644 include/linux/platform_data/iTCO_wdt.h
> > 
> > How are all of these changes related?
>  
> They create the platform data struct and use it in lpc_ich and iTCO_wdt.
> The Kconfig change shouldn't have snuck into this patch though, that's
> wrong, sorry.
> 
> > Why do they all have to be in a single patch?
> 
> I'll happily split them into more patches if you'd prefer. Maybe one
> that introduces the platform data structure and then a separate one that
> uses it in iTCO_wdt and lpc_ich?
> 
> Technically we could also split the change between iTCO_wdt and lpc_ich,
> but then we'd be passing in an 'lpc_ich_info' and pulling out a
> 'iTCO_wdt_platform_data' and that just seems crazy.

If it's possible to split them up per subsystem, that would be ideal.
Cross-subsystem patches are a pain in a backside.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Matt Fleming]

On Mon, 27 Jul, at 10:32:36PM, Lee Jones wrote:
> 
> If it's possible to split them up per subsystem, that would be ideal.
> Cross-subsystem patches are a pain in a backside.

Unfortunately that's not entirely possible because you need at least one
patch that touches both lpc_ich and iTCO_wdt when you make the
transition from 'lpc_ich_info' to 'iTCO_wdt_platform_data'.

And it's not like this patch series can be split up and taken through
separate maintainer trees anyway. It all really needs to go through one
tree (whichever one that may be).

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH 4/5] iTCO_wdt: fixup for the header

[Matt Fleming]

On Mon, 27 Jul, at 07:13:31AM, Guenter Roeck wrote:
> On 07/27/2015 06:38 AM, Matt Fleming wrote:
> >From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> >
> >Prevent nested inclusion of the header.
> >
> >Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> >Cc: Wim Van Sebroeck <wim@iguana.be>
> >Signed-off-by: Matt Fleming <matt.fleming@intel.com>
> 
> Should be merged into patch 1.

OK, will do.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH 5/5] i2c-i801: fixup regarding watchdog timer

[Matt Fleming]

On Mon, 27 Jul, at 07:14:08AM, Guenter Roeck wrote:
> On 07/27/2015 06:38 AM, Matt Fleming wrote:
> >From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> >
> >Change &array[0] to array since it's the same.
> >
> >It fixes 80 character limit warning as well.
> >
> >Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> >Cc: Jean Delvare <jdelvare@suse.com>
> >Cc: Wolfram Sang <wsa@the-dreams.de>
> >Signed-off-by: Matt Fleming <matt.fleming@intel.com>
> 
> Should be merged into patch 2.

OK, will do.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH 2/5] i2c: i801: Create iTCO device on newer Intel PCHs

[Matt Fleming]

On Mon, 27 Jul, at 07:08:08AM, Guenter Roeck wrote:
> >@@ -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		BIT(7)
> 
> If you use BIT, you should include bitops.h.
> Not sure if that makes too much sense here, though, without converting
> the rest of the driver to use BIT as well.
 
OK, I'll just switch to the existing notation used throughout the
driver rather than using bitops.

> >+static void i801_del_tco(struct i801_priv *priv)
> >+{
> >+	if (priv->tco_pdev) {
> 
> platform_device_unregister() handles NULL pointers, so this if statement
> is strictly speaking unnecessary.
 
Good point, I'll remove this check since it makes the code simpler too.

> >+		platform_device_unregister(priv->tco_pdev);
> >+		priv->tco_pdev = NULL;
> 
> Unnecessary; priv is going to be freed right afterwards.
 
I'll drop this.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH 2/5] i2c: i801: Create iTCO device on newer Intel PCHs

[Matt Fleming]

On Mon, 27 Jul, at 07:08:08AM, Guenter Roeck wrote:
> >@@ -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		BIT(7)
> 
> If you use BIT, you should include bitops.h.
> Not sure if that makes too much sense here, though, without converting
> the rest of the driver to use BIT as well.
 
OK, I'll just switch to the existing notation used throughout the
driver rather than using bitops.

> >+static void i801_del_tco(struct i801_priv *priv)
> >+{
> >+	if (priv->tco_pdev) {
> 
> platform_device_unregister() handles NULL pointers, so this if statement
> is strictly speaking unnecessary.
 
Good point, I'll remove this check since it makes the code simpler too.

> >+		platform_device_unregister(priv->tco_pdev);
> >+		priv->tco_pdev = NULL;
> 
> Unnecessary; priv is going to be freed right afterwards.
 
I'll drop this.

-- 
Matt Fleming, Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Lee Jones]

On Mon, 27 Jul 2015, 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.
> 
> 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>
> Cc: Lee Jones <lee.jones@linaro.org>
> Cc: Wim Van Sebroeck <wim@iguana.be>
> Signed-off-by: Matt Fleming <matt.fleming@intel.com>
> ---
>  drivers/mfd/lpc_ich.c                  | 32 +++++++++++++++++++++++++++++---
>  drivers/watchdog/Kconfig               |  2 +-
>  drivers/watchdog/iTCO_wdt.c            | 11 +++++------
>  include/linux/mfd/lpc_ich.h            |  6 ------
>  include/linux/platform_data/iTCO_wdt.h | 18 ++++++++++++++++++
>  5 files changed, 53 insertions(+), 16 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..d190b74a6321 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>

Lowercase please.

>  #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;

Lowercase please.

>  	struct lpc_ich_priv *priv = pci_get_drvdata(dev);
> +	struct lpc_ich_info *info;
> +	struct mfd_cell *cell = &lpc_ich_cells[LPC_WDT];
> +
> +	pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
> +	if (!pdata)
> +		return -ENOMEM;

Where is this freed?

Better to use devm_*

> +	info = &lpc_chipset_info[priv->chipset];
> +
> +	pdata->iTCO_version = info->iTCO_version;

Lowercase please.

> +	strcpy(pdata->name, info->name);

strncpy() is safer.

> +	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);

It's pretty hard to tell from the patch without applying it, but what
are the actual similarities and differences between the two finalise
functions?  They looks like they share enough lines for it to make
sense to have one function call and do different things in say a
switch statement, no?

> @@ -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);

Why do you have an mfd_add_devices() call for each device?

> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 241fafde42cb..5336fe2ff689 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -797,7 +797,7 @@ config ITCO_WDT
>  	tristate "Intel TCO Timer/Watchdog"
>  	depends on (X86 || IA64) && PCI
>  	select WATCHDOG_CORE
> -	select LPC_ICH
> +	depends on LPC_ICH || I2C_I801
>  	---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 3c3fd417ddeb..9a6e70976f64 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->iTCO_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->iTCO_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..ce53c2b01f6d
> --- /dev/null
> +++ b/include/linux/platform_data/iTCO_wdt.h
> @@ -0,0 +1,18 @@
> +/*
> + * Platform data for the Intel TCO Watchdog
> + */
> +
> +#ifndef _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 iTCO_version;
> +};
> +
> +#endif /* _ITCO_WDT_H_ */

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Lee Jones]

On Mon, 27 Jul 2015, 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.
> 
> 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>
> Cc: Lee Jones <lee.jones@linaro.org>
> Cc: Wim Van Sebroeck <wim@iguana.be>
> Signed-off-by: Matt Fleming <matt.fleming@intel.com>
> ---
>  drivers/mfd/lpc_ich.c                  | 32 +++++++++++++++++++++++++++++---
>  drivers/watchdog/Kconfig               |  2 +-
>  drivers/watchdog/iTCO_wdt.c            | 11 +++++------
>  include/linux/mfd/lpc_ich.h            |  6 ------
>  include/linux/platform_data/iTCO_wdt.h | 18 ++++++++++++++++++
>  5 files changed, 53 insertions(+), 16 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..d190b74a6321 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>

Lowercase please.

>  #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;

Lowercase please.

>  	struct lpc_ich_priv *priv = pci_get_drvdata(dev);
> +	struct lpc_ich_info *info;
> +	struct mfd_cell *cell = &lpc_ich_cells[LPC_WDT];
> +
> +	pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
> +	if (!pdata)
> +		return -ENOMEM;

Where is this freed?

Better to use devm_*

> +	info = &lpc_chipset_info[priv->chipset];
> +
> +	pdata->iTCO_version = info->iTCO_version;

Lowercase please.

> +	strcpy(pdata->name, info->name);

strncpy() is safer.

> +	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);

It's pretty hard to tell from the patch without applying it, but what
are the actual similarities and differences between the two finalise
functions?  They looks like they share enough lines for it to make
sense to have one function call and do different things in say a
switch statement, no?

> @@ -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);

Why do you have an mfd_add_devices() call for each device?

> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 241fafde42cb..5336fe2ff689 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -797,7 +797,7 @@ config ITCO_WDT
>  	tristate "Intel TCO Timer/Watchdog"
>  	depends on (X86 || IA64) && PCI
>  	select WATCHDOG_CORE
> -	select LPC_ICH
> +	depends on LPC_ICH || I2C_I801
>  	---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 3c3fd417ddeb..9a6e70976f64 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->iTCO_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->iTCO_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..ce53c2b01f6d
> --- /dev/null
> +++ b/include/linux/platform_data/iTCO_wdt.h
> @@ -0,0 +1,18 @@
> +/*
> + * Platform data for the Intel TCO Watchdog
> + */
> +
> +#ifndef _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 iTCO_version;
> +};
> +
> +#endif /* _ITCO_WDT_H_ */

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Matt Fleming]

On Mon, 27 Jul, at 07:24:09AM, Guenter Roeck wrote:
> On 07/27/2015 07:19 AM, Matt Fleming wrote:
> >On Mon, 27 Jul, at 06:49:08AM, Guenter Roeck wrote:
> >>
> >>I don't see the platform data freed anywhere, neither in the error path nor
> >>in the cleanup path of this driver. Can you use devm_kzalloc() ?
> >>Otherwise I think you'll need a cleanup path.
> >
> >Oops, good catch. Yes, devm_kzalloc() can be used here, thanks!
> >
> Or maybe just use a static data structure, like in the i2c driver.

The point of dynamically allocating it is that we can use the data from
the static lpc_ich_chipset_info array and munge into the correct
platform data.

The alternative would be to go and mass-modify that array to include
iTCO_wdt_platform_data objects that we could directly pass to the
iTCO_wdt driver. I wanted to avoid the code churn, but I'm not super
bothered either way if people have strong opinions about it.

-- 
Matt Fleming, Intel Open Source Technology Center

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

[Matt Fleming]

On Mon, 27 Jul, at 07:22:51AM, Guenter Roeck wrote:
> On 07/27/2015 06:38 AM, 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.
> >
> >Cc: Wim Van Sebroeck <wim@iguana.be>
> >Signed-off-by: Matt Fleming <matt.fleming@intel.com>
> >---
> >  drivers/watchdog/iTCO_wdt.c | 58 ++++++++++++++++++++++++++-------------------
> >  1 file changed, 34 insertions(+), 24 deletions(-)
> >
> >diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
> >index 9a6e70976f64..17dfbc51b85a 100644
> >--- a/drivers/watchdog/iTCO_wdt.c
> >+++ b/drivers/watchdog/iTCO_wdt.c
> >@@ -145,58 +145,65 @@ 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;
> >+	default:
> >+		enable_bit = 0x00000002;
> 
> I think it would be better to explicitly list versions 1 and 4 here,
> for clarification. We don't know what Intel will come up with for version 5,
> and by then no one will remember that bit 2 applies for version 1 and 4 only.

Sure, that makes sense. I'll update the patch.

-- 
Matt Fleming, Intel Open Source Technology Center

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

[Matt Fleming]

On Tue, 28 Jul, at 10:46:43AM, Lee Jones wrote:
> On Mon, 27 Jul 2015, 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.
> > 
> > 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>
> > Cc: Lee Jones <lee.jones@linaro.org>
> > Cc: Wim Van Sebroeck <wim@iguana.be>
> > Signed-off-by: Matt Fleming <matt.fleming@intel.com>
> > ---
> >  drivers/mfd/lpc_ich.c                  | 32 +++++++++++++++++++++++++++++---
> >  drivers/watchdog/Kconfig               |  2 +-
> >  drivers/watchdog/iTCO_wdt.c            | 11 +++++------
> >  include/linux/mfd/lpc_ich.h            |  6 ------
> >  include/linux/platform_data/iTCO_wdt.h | 18 ++++++++++++++++++
> >  5 files changed, 53 insertions(+), 16 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..d190b74a6321 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>
> 
> Lowercase please.
 
Even though the driver is called iTCO_wdt? It seemed to me to be more
confusing to start mixing cases rather than sticking with the ugly upper
case. Especially since when you look in the iTCO_wdt driver all the
function and type names are written that way.

> >  #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;
> 
> Lowercase please.
 
See above.

> >  	struct lpc_ich_priv *priv = pci_get_drvdata(dev);
> > +	struct lpc_ich_info *info;
> > +	struct mfd_cell *cell = &lpc_ich_cells[LPC_WDT];
> > +
> > +	pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
> > +	if (!pdata)
> > +		return -ENOMEM;
> 
> Where is this freed?
> 
> Better to use devm_*
 
Yeah, Guenter caught this too. devm_* would definitely be better.

> > +	info = &lpc_chipset_info[priv->chipset];
> > +
> > +	pdata->iTCO_version = info->iTCO_version;
> 
> Lowercase please.

Hmm... but then this line will read,

	pdata->itco_version = info->iTCO_version;

I'm not sure that's an improvement.

> 
> > +	strcpy(pdata->name, info->name);
> 
> strncpy() is safer.
 
OK, I'll update this. Though it's worth pointing out that the name[]
declarations are of identical size in these two objects (but I guess
that could change in the future).

> > +	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);
> 
> It's pretty hard to tell from the patch without applying it, but what
> are the actual similarities and differences between the two finalise
> functions?  They looks like they share enough lines for it to make
> sense to have one function call and do different things in say a
> switch statement, no?
 
For LPC_WDT we dynamically allocate the platform data, and for LPC_GPIO
we use the static lpc_chipsec_info array.

I'm just personally not a fan of performing memory allocations from
within switch statement bodies, which is why I implemented this as two
separate finalize functions.

> > @@ -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);
> 
> Why do you have an mfd_add_devices() call for each device?
 
Good question. This call has been present since March 2012 when support
was first added for iTCO_wdt in commit 887c8ec7219f ("watchdog: Convert
iTCO_wdt driver to mfd model").

There's no good reason that I can see. Aaron?

-- 
Matt Fleming, Intel Open Source Technology Center

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

[Lee Jones]

On Tue, 28 Jul 2015, Matt Fleming wrote:

> On Tue, 28 Jul, at 10:46:43AM, Lee Jones wrote:
> > On Mon, 27 Jul 2015, 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.
> > > 
> > > 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>
> > > Cc: Lee Jones <lee.jones@linaro.org>
> > > Cc: Wim Van Sebroeck <wim@iguana.be>
> > > Signed-off-by: Matt Fleming <matt.fleming@intel.com>
> > > ---
> > >  drivers/mfd/lpc_ich.c                  | 32 +++++++++++++++++++++++++++++---
> > >  drivers/watchdog/Kconfig               |  2 +-
> > >  drivers/watchdog/iTCO_wdt.c            | 11 +++++------
> > >  include/linux/mfd/lpc_ich.h            |  6 ------
> > >  include/linux/platform_data/iTCO_wdt.h | 18 ++++++++++++++++++
> > >  5 files changed, 53 insertions(+), 16 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..d190b74a6321 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>
> > 
> > Lowercase please.
>  
> Even though the driver is called iTCO_wdt? It seemed to me to be more
> confusing to start mixing cases rather than sticking with the ugly upper
> case. Especially since when you look in the iTCO_wdt driver all the
> function and type names are written that way.

The driver shouldn't be called that either.

You are the only one.  What makes iTCO 'special'?

$ ls drivers/watchdog/ | grep [A-Z]
 iTCO_vendor.h
 iTCO_vendor_support.c
 iTCO_wdt.c
 Kconfig
 Makefile

Mixed case names (filenames, variables, etc) are frowned upon and
shouldn't be allowed anywhere.  Please read Chapter 4 of
Documentation/CodingStyle.

> > >  #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;
> > 
> > Lowercase please.
>  
> See above.

Likewise. ;)

> > >  	struct lpc_ich_priv *priv = pci_get_drvdata(dev);
> > > +	struct lpc_ich_info *info;
> > > +	struct mfd_cell *cell = &lpc_ich_cells[LPC_WDT];
> > > +
> > > +	pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
> > > +	if (!pdata)
> > > +		return -ENOMEM;
> > 
> > Where is this freed?
> > 
> > Better to use devm_*
>  
> Yeah, Guenter caught this too. devm_* would definitely be better.

Great.

> > > +	info = &lpc_chipset_info[priv->chipset];
> > > +
> > > +	pdata->iTCO_version = info->iTCO_version;
> > 
> > Lowercase please.
> 
> Hmm... but then this line will read,
> 
> 	pdata->itco_version = info->iTCO_version;
> 
> I'm not sure that's an improvement.

Please consider making all of the variable names conform to the
coding standards we normally abide by.  You can submit it either as
patch 1 of this set, or independently.

> > > +	strcpy(pdata->name, info->name);
> > 
> > strncpy() is safer.
>  
> OK, I'll update this. Though it's worth pointing out that the name[]
> declarations are of identical size in these two objects (but I guess
> that could change in the future).

Better to err on the side of caution.

> > > +	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);
> > 
> > It's pretty hard to tell from the patch without applying it, but what
> > are the actual similarities and differences between the two finalise
> > functions?  They looks like they share enough lines for it to make
> > sense to have one function call and do different things in say a
> > switch statement, no?
>  
> For LPC_WDT we dynamically allocate the platform data, and for LPC_GPIO
> we use the static lpc_chipsec_info array.
> 
> I'm just personally not a fan of performing memory allocations from
> within switch statement bodies, which is why I implemented this as two
> separate finalize functions.

I'll assume this is okay, then take a look at the driver as a whole
once it's applied.

> > > @@ -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);
> > 
> > Why do you have an mfd_add_devices() call for each device?
>  
> Good question. This call has been present since March 2012 when support
> was first added for iTCO_wdt in commit 887c8ec7219f ("watchdog: Convert
> iTCO_wdt driver to mfd model").
> 
> There's no good reason that I can see. Aaron?

Thanks for checking.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Lee Jones]

On Tue, 28 Jul 2015, Matt Fleming wrote:

> On Tue, 28 Jul, at 10:46:43AM, Lee Jones wrote:
> > On Mon, 27 Jul 2015, 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.
> > > 
> > > 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>
> > > Cc: Lee Jones <lee.jones@linaro.org>
> > > Cc: Wim Van Sebroeck <wim@iguana.be>
> > > Signed-off-by: Matt Fleming <matt.fleming@intel.com>
> > > ---
> > >  drivers/mfd/lpc_ich.c                  | 32 +++++++++++++++++++++++++++++---
> > >  drivers/watchdog/Kconfig               |  2 +-
> > >  drivers/watchdog/iTCO_wdt.c            | 11 +++++------
> > >  include/linux/mfd/lpc_ich.h            |  6 ------
> > >  include/linux/platform_data/iTCO_wdt.h | 18 ++++++++++++++++++
> > >  5 files changed, 53 insertions(+), 16 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..d190b74a6321 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>
> > 
> > Lowercase please.
>  
> Even though the driver is called iTCO_wdt? It seemed to me to be more
> confusing to start mixing cases rather than sticking with the ugly upper
> case. Especially since when you look in the iTCO_wdt driver all the
> function and type names are written that way.

The driver shouldn't be called that either.

You are the only one.  What makes iTCO 'special'?

$ ls drivers/watchdog/ | grep [A-Z]
 iTCO_vendor.h
 iTCO_vendor_support.c
 iTCO_wdt.c
 Kconfig
 Makefile

Mixed case names (filenames, variables, etc) are frowned upon and
shouldn't be allowed anywhere.  Please read Chapter 4 of
Documentation/CodingStyle.

> > >  #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;
> > 
> > Lowercase please.
>  
> See above.

Likewise. ;)

> > >  	struct lpc_ich_priv *priv = pci_get_drvdata(dev);
> > > +	struct lpc_ich_info *info;
> > > +	struct mfd_cell *cell = &lpc_ich_cells[LPC_WDT];
> > > +
> > > +	pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
> > > +	if (!pdata)
> > > +		return -ENOMEM;
> > 
> > Where is this freed?
> > 
> > Better to use devm_*
>  
> Yeah, Guenter caught this too. devm_* would definitely be better.

Great.

> > > +	info = &lpc_chipset_info[priv->chipset];
> > > +
> > > +	pdata->iTCO_version = info->iTCO_version;
> > 
> > Lowercase please.
> 
> Hmm... but then this line will read,
> 
> 	pdata->itco_version = info->iTCO_version;
> 
> I'm not sure that's an improvement.

Please consider making all of the variable names conform to the
coding standards we normally abide by.  You can submit it either as
patch 1 of this set, or independently.

> > > +	strcpy(pdata->name, info->name);
> > 
> > strncpy() is safer.
>  
> OK, I'll update this. Though it's worth pointing out that the name[]
> declarations are of identical size in these two objects (but I guess
> that could change in the future).

Better to err on the side of caution.

> > > +	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);
> > 
> > It's pretty hard to tell from the patch without applying it, but what
> > are the actual similarities and differences between the two finalise
> > functions?  They looks like they share enough lines for it to make
> > sense to have one function call and do different things in say a
> > switch statement, no?
>  
> For LPC_WDT we dynamically allocate the platform data, and for LPC_GPIO
> we use the static lpc_chipsec_info array.
> 
> I'm just personally not a fan of performing memory allocations from
> within switch statement bodies, which is why I implemented this as two
> separate finalize functions.

I'll assume this is okay, then take a look at the driver as a whole
once it's applied.

> > > @@ -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);
> > 
> > Why do you have an mfd_add_devices() call for each device?
>  
> Good question. This call has been present since March 2012 when support
> was first added for iTCO_wdt in commit 887c8ec7219f ("watchdog: Convert
> iTCO_wdt driver to mfd model").
> 
> There's no good reason that I can see. Aaron?

Thanks for checking.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Matt Fleming]

On Tue, 28 Jul, at 12:37:21PM, Lee Jones wrote:
> 
> The driver shouldn't be called that either.
> 
> You are the only one.  What makes iTCO 'special'?

I don't know, I didn't write it. It looks like Wim did ~9 years ago, so
it must have made sense to him at the time.

> > > > +	info = &lpc_chipset_info[priv->chipset];
> > > > +
> > > > +	pdata->iTCO_version = info->iTCO_version;
> > > 
> > > Lowercase please.
> > 
> > Hmm... but then this line will read,
> > 
> > 	pdata->itco_version = info->iTCO_version;
> > 
> > I'm not sure that's an improvement.
> 
> Please consider making all of the variable names conform to the
> coding standards we normally abide by.  You can submit it either as
> patch 1 of this set, or independently.

Right, I figured we were fast approaching this rabit hole.
 
-- 
Matt Fleming, Intel Open Source Technology Center

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

[Lee Jones]

On Tue, 28 Jul 2015, Matt Fleming wrote:
> On Tue, 28 Jul, at 12:37:21PM, Lee Jones wrote:
> > 
> > The driver shouldn't be called that either.
> > 
> > You are the only one.  What makes iTCO 'special'?
> 
> I don't know, I didn't write it. It looks like Wim did ~9 years ago, so
> it must have made sense to him at the time.
> 
> > > > > +	info = &lpc_chipset_info[priv->chipset];
> > > > > +
> > > > > +	pdata->iTCO_version = info->iTCO_version;
> > > > 
> > > > Lowercase please.
> > > 
> > > Hmm... but then this line will read,
> > > 
> > > 	pdata->itco_version = info->iTCO_version;
> > > 
> > > I'm not sure that's an improvement.
> > 
> > Please consider making all of the variable names conform to the
> > coding standards we normally abide by.  You can submit it either as
> > patch 1 of this set, or independently.
> 
> Right, I figured we were fast approaching this rabit hole.

No rabbit hole, just some fixups.  If it takes you any more than 10
mins, I'd be surprised.

Let me know if you think it'll be too much trouble and I'll do the
fixups myself.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Guenter Roeck]

On 07/28/2015 08:00 AM, Lee Jones wrote:
> On Tue, 28 Jul 2015, Matt Fleming wrote:
>> On Tue, 28 Jul, at 12:37:21PM, Lee Jones wrote:
>>>
>>> The driver shouldn't be called that either.
>>>
>>> You are the only one.  What makes iTCO 'special'?
>>
>> I don't know, I didn't write it. It looks like Wim did ~9 years ago, so
>> it must have made sense to him at the time.
>>

Coding style wasn't as strict then as it is today. iTCO has just been kept
for historic reasons.

Sure, we could have changed it to lowercase, but so far no one bothered.
Plus, of course, there is always the element that some maintainers hate
that kind of cleanup, and patch submitters like Matt are stuck between
a rock and a hard place.

Guenter

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

[Lee Jones]

On Tue, 28 Jul 2015, Guenter Roeck wrote:

> On 07/28/2015 08:00 AM, Lee Jones wrote:
> >On Tue, 28 Jul 2015, Matt Fleming wrote:
> >>On Tue, 28 Jul, at 12:37:21PM, Lee Jones wrote:
> >>>
> >>>The driver shouldn't be called that either.
> >>>
> >>>You are the only one.  What makes iTCO 'special'?
> >>
> >>I don't know, I didn't write it. It looks like Wim did ~9 years ago, so
> >>it must have made sense to him at the time.
> >>
> 
> Coding style wasn't as strict then as it is today. iTCO has just been kept
> for historic reasons.

For sure, I get that, but it doesn't mean we can't do-the-right-thing
(tm) now does it?

> Sure, we could have changed it to lowercase, but so far no one bothered.
> Plus, of course, there is always the element that some maintainers hate
> that kind of cleanup,

Really?  Surely any kind of clean-up is good clean-up.  Especially as
Greg KH et. al, have been doing public presentations telling everyone
that there is always kernel work for anyone who has the time; spelling
corrections and all.

> and patch submitters like Matt are stuck between
> a rock and a hard place.

Matt,

Either do the 5min clean-up or don't, no biggy.  If you don't then I
can do it.  At the very least please don't add any _new_ camel case
variables or filenames.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Lee Jones]

On Tue, 28 Jul 2015, Guenter Roeck wrote:

> On 07/28/2015 08:00 AM, Lee Jones wrote:
> >On Tue, 28 Jul 2015, Matt Fleming wrote:
> >>On Tue, 28 Jul, at 12:37:21PM, Lee Jones wrote:
> >>>
> >>>The driver shouldn't be called that either.
> >>>
> >>>You are the only one.  What makes iTCO 'special'?
> >>
> >>I don't know, I didn't write it. It looks like Wim did ~9 years ago, so
> >>it must have made sense to him at the time.
> >>
> 
> Coding style wasn't as strict then as it is today. iTCO has just been kept
> for historic reasons.

For sure, I get that, but it doesn't mean we can't do-the-right-thing
(tm) now does it?

> Sure, we could have changed it to lowercase, but so far no one bothered.
> Plus, of course, there is always the element that some maintainers hate
> that kind of cleanup,

Really?  Surely any kind of clean-up is good clean-up.  Especially as
Greg KH et. al, have been doing public presentations telling everyone
that there is always kernel work for anyone who has the time; spelling
corrections and all.

> and patch submitters like Matt are stuck between
> a rock and a hard place.

Matt,

Either do the 5min clean-up or don't, no biggy.  If you don't then I
can do it.  At the very least please don't add any _new_ camel case
variables or filenames.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Matt Fleming]

On Tue, 28 Jul, at 04:28:32PM, Lee Jones wrote:
> On Tue, 28 Jul 2015, Guenter Roeck wrote:
> 
> > On 07/28/2015 08:00 AM, Lee Jones wrote:
> > >On Tue, 28 Jul 2015, Matt Fleming wrote:
> > >>On Tue, 28 Jul, at 12:37:21PM, Lee Jones wrote:
> > >>>
> > >>>The driver shouldn't be called that either.
> > >>>
> > >>>You are the only one.  What makes iTCO 'special'?
> > >>
> > >>I don't know, I didn't write it. It looks like Wim did ~9 years ago, so
> > >>it must have made sense to him at the time.
> > >>
> > 
> > Coding style wasn't as strict then as it is today. iTCO has just been kept
> > for historic reasons.
> 
> For sure, I get that, but it doesn't mean we can't do-the-right-thing
> (tm) now does it?
> 
> > Sure, we could have changed it to lowercase, but so far no one bothered.
> > Plus, of course, there is always the element that some maintainers hate
> > that kind of cleanup,
> 
> Really?

Yes, really. 

> Surely any kind of clean-up is good clean-up.  Especially as Greg KH
> et. al, have been doing public presentations telling everyone that
> there is always kernel work for anyone who has the time; spelling
> corrections and all.
 
That's what staging/ was invented for ;-) Greg is quite happy to take
those patches, other maintainers are less so.

> Matt,
> 
> Either do the 5min clean-up or don't, no biggy.  If you don't then I
> can do it.

Go for it.

> At the very least please don't add any _new_ camel case variables or
> filenames.

OK, sure.

-- 
Matt Fleming, Intel Open Source Technology Center

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

[Lee Jones]

On Tue, 28 Jul 2015, Matt Fleming wrote:

> On Tue, 28 Jul, at 04:28:32PM, Lee Jones wrote:
> > On Tue, 28 Jul 2015, Guenter Roeck wrote:
> > 
> > > On 07/28/2015 08:00 AM, Lee Jones wrote:
> > > >On Tue, 28 Jul 2015, Matt Fleming wrote:
> > > >>On Tue, 28 Jul, at 12:37:21PM, Lee Jones wrote:
> > > >>>
> > > >>>The driver shouldn't be called that either.
> > > >>>
> > > >>>You are the only one.  What makes iTCO 'special'?
> > > >>
> > > >>I don't know, I didn't write it. It looks like Wim did ~9 years ago, so
> > > >>it must have made sense to him at the time.
> > > >>
> > > 
> > > Coding style wasn't as strict then as it is today. iTCO has just been kept
> > > for historic reasons.
> > 
> > For sure, I get that, but it doesn't mean we can't do-the-right-thing
> > (tm) now does it?
> > 
> > > Sure, we could have changed it to lowercase, but so far no one bothered.
> > > Plus, of course, there is always the element that some maintainers hate
> > > that kind of cleanup,
> > 
> > Really?
> 
> Yes, really. 

Well then they are silly (replace silly with something less ML
friendly if you like).

> > Surely any kind of clean-up is good clean-up.  Especially as Greg KH
> > et. al, have been doing public presentations telling everyone that
> > there is always kernel work for anyone who has the time; spelling
> > corrections and all.
>  
> That's what staging/ was invented for ;-) Greg is quite happy to take
> those patches, other maintainers are less so.

No, staging was designed to take drivers which are either
controversial or still in a state of flux.  Not to over-rule
maintainers which didn't think a clean-up was important enough for
acceptance.

> > Either do the 5min clean-up or don't, no biggy.  If you don't then I
> > can do it.
> 
> Go for it.
> 
> > At the very least please don't add any _new_ camel case variables or
> > filenames.
> 
> OK, sure.

Great.  Resubmit when ready.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Jean Delvare]

Le Tuesday 28 July 2015 à 10:46 +0100, Lee Jones a écrit :
> On Mon, 27 Jul 2015, Matt Fleming wrote:
> > +	strcpy(pdata->name, info->name);
> 
> strncpy() is safer.

And strlcpy() is even better.

-- 
Jean Delvare
SUSE L3 Support

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

[Jean Delvare]

Le Tuesday 28 July 2015 à 10:46 +0100, Lee Jones a écrit :
> On Mon, 27 Jul 2015, Matt Fleming wrote:
> > +	strcpy(pdata->name, info->name);
> 
> strncpy() is safer.

And strlcpy() is even better.

-- 
Jean Delvare
SUSE L3 Support

--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jean Delvare]

Hi Matt,

Le Monday 27 July 2015 à 14:38 +0100, Matt Fleming a écrit :
> 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.
> 
> Cc: Wim Van Sebroeck <wim@iguana.be>
> Signed-off-by: Matt Fleming <matt.fleming@intel.com>
> ---
>  drivers/watchdog/iTCO_wdt.c | 58 ++++++++++++++++++++++++++-------------------
>  1 file changed, 34 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
> index 9a6e70976f64..17dfbc51b85a 100644
> --- a/drivers/watchdog/iTCO_wdt.c
> +++ b/drivers/watchdog/iTCO_wdt.c
> (...) 
> @@ -503,7 +510,10 @@ static int iTCO_wdt_probe(struct platform_device *dev)
>  		pdata->name, pdata->iTCO_version, (u64)TCOBASE);
>  
>  	/* Clear out the (probably old) status */
> -	if (iTCO_wdt_private.iTCO_version == 3) {
> +	if (iTCO_wdt_private.iTCO_version == 4) {
> +		outw(0x0008, TCO1_STS);	/* Clear the Time Out Status bit */
> +		outw(0x0002, TCO2_STS);	/* Clear SECOND_TO_STS bit */
> +	} else if (iTCO_wdt_private.iTCO_version == 3) {
>  		outl(0x20008, TCO1_STS);
>  	} else {
>  		outw(0x0008, TCO1_STS);	/* Clear the Time Out Status bit */

The "version == 4" branch is a subset of the "else" branch, so you could
merge both with a conditional. If you prefer not to, then it probably
makes sense to change the whole block to a switch/case construct.

-- 
Jean Delvare
SUSE L3 Support

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

[Jean Delvare]

Hi Matt,

Le Monday 27 July 2015 à 14:38 +0100, Matt Fleming a écrit :
> 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.
> 
> Cc: Wim Van Sebroeck <wim@iguana.be>
> Signed-off-by: Matt Fleming <matt.fleming@intel.com>
> ---
>  drivers/watchdog/iTCO_wdt.c | 58 ++++++++++++++++++++++++++-------------------
>  1 file changed, 34 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
> index 9a6e70976f64..17dfbc51b85a 100644
> --- a/drivers/watchdog/iTCO_wdt.c
> +++ b/drivers/watchdog/iTCO_wdt.c
> (...) 
> @@ -503,7 +510,10 @@ static int iTCO_wdt_probe(struct platform_device *dev)
>  		pdata->name, pdata->iTCO_version, (u64)TCOBASE);
>  
>  	/* Clear out the (probably old) status */
> -	if (iTCO_wdt_private.iTCO_version == 3) {
> +	if (iTCO_wdt_private.iTCO_version == 4) {
> +		outw(0x0008, TCO1_STS);	/* Clear the Time Out Status bit */
> +		outw(0x0002, TCO2_STS);	/* Clear SECOND_TO_STS bit */
> +	} else if (iTCO_wdt_private.iTCO_version == 3) {
>  		outl(0x20008, TCO1_STS);
>  	} else {
>  		outw(0x0008, TCO1_STS);	/* Clear the Time Out Status bit */

The "version == 4" branch is a subset of the "else" branch, so you could
merge both with a conditional. If you prefer not to, then it probably
makes sense to change the whole block to a switch/case construct.

-- 
Jean Delvare
SUSE L3 Support

--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Guenter Roeck]

On 07/28/2015 08:28 AM, Lee Jones wrote:
> On Tue, 28 Jul 2015, Guenter Roeck wrote:
>
>> On 07/28/2015 08:00 AM, Lee Jones wrote:
>>> On Tue, 28 Jul 2015, Matt Fleming wrote:
>>>> On Tue, 28 Jul, at 12:37:21PM, Lee Jones wrote:
>>>>>
>>>>> The driver shouldn't be called that either.
>>>>>
>>>>> You are the only one.  What makes iTCO 'special'?
>>>>
>>>> I don't know, I didn't write it. It looks like Wim did ~9 years ago, so
>>>> it must have made sense to him at the time.
>>>>
>>
>> Coding style wasn't as strict then as it is today. iTCO has just been kept
>> for historic reasons.
>
> For sure, I get that, but it doesn't mean we can't do-the-right-thing
> (tm) now does it?
>
>> Sure, we could have changed it to lowercase, but so far no one bothered.
>> Plus, of course, there is always the element that some maintainers hate
>> that kind of cleanup,
>
> Really?  Surely any kind of clean-up is good clean-up.  Especially as
> Greg KH et. al, have been doing public presentations telling everyone
> that there is always kernel work for anyone who has the time; spelling
> corrections and all.
>

Yes, really. Just try to submit cleanup patches to maintainers other than
Greg and myself, and you'll see. It is a minefield.

Guenter

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

[Lee Jones]

On Tue, 28 Jul 2015, Guenter Roeck wrote:

> On 07/28/2015 08:28 AM, Lee Jones wrote:
> >On Tue, 28 Jul 2015, Guenter Roeck wrote:
> >
> >>On 07/28/2015 08:00 AM, Lee Jones wrote:
> >>>On Tue, 28 Jul 2015, Matt Fleming wrote:
> >>>>On Tue, 28 Jul, at 12:37:21PM, Lee Jones wrote:
> >>>>>
> >>>>>The driver shouldn't be called that either.
> >>>>>
> >>>>>You are the only one.  What makes iTCO 'special'?
> >>>>
> >>>>I don't know, I didn't write it. It looks like Wim did ~9 years ago, so
> >>>>it must have made sense to him at the time.
> >>>>
> >>
> >>Coding style wasn't as strict then as it is today. iTCO has just been kept
> >>for historic reasons.
> >
> >For sure, I get that, but it doesn't mean we can't do-the-right-thing
> >(tm) now does it?
> >
> >>Sure, we could have changed it to lowercase, but so far no one bothered.
> >>Plus, of course, there is always the element that some maintainers hate
> >>that kind of cleanup,
> >
> >Really?  Surely any kind of clean-up is good clean-up.  Especially as
> >Greg KH et. al, have been doing public presentations telling everyone
> >that there is always kernel work for anyone who has the time; spelling
> >corrections and all.
> >
> 
> Yes, really. Just try to submit cleanup patches to maintainers other than
> Greg and myself, and you'll see. It is a minefield.

Admittedly some of us have our quirks, but I'm happy to challenge
anyone that won't accept clean-up patches that make things better.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Lee Jones]

On Tue, 28 Jul 2015, Guenter Roeck wrote:

> On 07/28/2015 08:28 AM, Lee Jones wrote:
> >On Tue, 28 Jul 2015, Guenter Roeck wrote:
> >
> >>On 07/28/2015 08:00 AM, Lee Jones wrote:
> >>>On Tue, 28 Jul 2015, Matt Fleming wrote:
> >>>>On Tue, 28 Jul, at 12:37:21PM, Lee Jones wrote:
> >>>>>
> >>>>>The driver shouldn't be called that either.
> >>>>>
> >>>>>You are the only one.  What makes iTCO 'special'?
> >>>>
> >>>>I don't know, I didn't write it. It looks like Wim did ~9 years ago, so
> >>>>it must have made sense to him at the time.
> >>>>
> >>
> >>Coding style wasn't as strict then as it is today. iTCO has just been kept
> >>for historic reasons.
> >
> >For sure, I get that, but it doesn't mean we can't do-the-right-thing
> >(tm) now does it?
> >
> >>Sure, we could have changed it to lowercase, but so far no one bothered.
> >>Plus, of course, there is always the element that some maintainers hate
> >>that kind of cleanup,
> >
> >Really?  Surely any kind of clean-up is good clean-up.  Especially as
> >Greg KH et. al, have been doing public presentations telling everyone
> >that there is always kernel work for anyone who has the time; spelling
> >corrections and all.
> >
> 
> Yes, really. Just try to submit cleanup patches to maintainers other than
> Greg and myself, and you'll see. It is a minefield.

Admittedly some of us have our quirks, but I'm happy to challenge
anyone that won't accept clean-up patches that make things better.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Aaron Sierra]

> > > > @@ -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);
> > > 
> > > Why do you have an mfd_add_devices() call for each device?
> >  
> > Good question. This call has been present since March 2012 when support
> > was first added for iTCO_wdt in commit 887c8ec7219f ("watchdog: Convert
> > iTCO_wdt driver to mfd model").
> > 
> > There's no good reason that I can see. Aaron?

I chose to call mfd_add_devices() in each device init function
because I thought it was the easiest way to avoid registering an
incomplete/invalid MFD cell should an error occur during init.

That way device registration wouldn't be an all-or-nothing affair.

Doesn't mfd_add_devices() bail out after the first unsuccessful
mfd to platform device translation?

-Aaron S.

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

[Guenter Roeck]

On Tue, Jul 28, 2015 at 06:32:16PM +0100, Lee Jones wrote:
> On Tue, 28 Jul 2015, Guenter Roeck wrote:
> 
> > On 07/28/2015 08:28 AM, Lee Jones wrote:
> > >On Tue, 28 Jul 2015, Guenter Roeck wrote:
> > >
> > >>On 07/28/2015 08:00 AM, Lee Jones wrote:
> > >>>On Tue, 28 Jul 2015, Matt Fleming wrote:
> > >>>>On Tue, 28 Jul, at 12:37:21PM, Lee Jones wrote:
> > >>>>>
> > >>>>>The driver shouldn't be called that either.
> > >>>>>
> > >>>>>You are the only one.  What makes iTCO 'special'?
> > >>>>
> > >>>>I don't know, I didn't write it. It looks like Wim did ~9 years ago, so
> > >>>>it must have made sense to him at the time.
> > >>>>
> > >>
> > >>Coding style wasn't as strict then as it is today. iTCO has just been kept
> > >>for historic reasons.
> > >
> > >For sure, I get that, but it doesn't mean we can't do-the-right-thing
> > >(tm) now does it?
> > >
> > >>Sure, we could have changed it to lowercase, but so far no one bothered.
> > >>Plus, of course, there is always the element that some maintainers hate
> > >>that kind of cleanup,
> > >
> > >Really?  Surely any kind of clean-up is good clean-up.  Especially as
> > >Greg KH et. al, have been doing public presentations telling everyone
> > >that there is always kernel work for anyone who has the time; spelling
> > >corrections and all.
> > >
> > 
> > Yes, really. Just try to submit cleanup patches to maintainers other than
> > Greg and myself, and you'll see. It is a minefield.
> 
> Admittedly some of us have our quirks, but I'm happy to challenge
> anyone that won't accept clean-up patches that make things better.
> 
There might possibly be a discussion at the kernel summit about that,
in the context of encouraging (or discouraging) new kernel developers.
If you are invited to the KS, it might make sense to attend that discussion.

Guenter

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

[Lee Jones]

On Tue, 28 Jul 2015, Jean Delvare wrote:

> Le Tuesday 28 July 2015 à 10:46 +0100, Lee Jones a écrit :
> > On Mon, 27 Jul 2015, Matt Fleming wrote:
> > > +	strcpy(pdata->name, info->name);
> > 
> > strncpy() is safer.
> 
> And strlcpy() is even better.

+1, thanks for that.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Lee Jones]

On Tue, 28 Jul 2015, Jean Delvare wrote:

> Le Tuesday 28 July 2015 à 10:46 +0100, Lee Jones a écrit :
> > On Mon, 27 Jul 2015, Matt Fleming wrote:
> > > +	strcpy(pdata->name, info->name);
> > 
> > strncpy() is safer.
> 
> And strlcpy() is even better.

+1, thanks for that.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jean Delvare]

Hi Matt,

On Mon, 27 Jul 2015 14:38:08 +0100, Matt Fleming wrote:
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 241fafde42cb..5336fe2ff689 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -797,7 +797,7 @@ config ITCO_WDT
>  	tristate "Intel TCO Timer/Watchdog"
>  	depends on (X86 || IA64) && PCI
>  	select WATCHDOG_CORE
> -	select LPC_ICH
> +	depends on LPC_ICH || I2C_I801
>  	---help---
>  	  Hardware driver for the intel TCO timer based watchdog devices.
>  	  These drivers are included in the Intel 82801 I/O Controller

I don't like that. Depending on the order of the subsystems in the
Kconfig tree, the user may not see the option and has no clue that the
option exists. When he/she later enables LPC_ICH or I2C_I801, the
option becomes visible but he/she has no reason to return here to
enable it.

I can think of several ways to address the issue. One of them is to
select both drivers, as this is what most users will want anyway:

	select LPC_ICH if !EXPERT
	select I2C_I801 if !EXPERT

Another possibility is to make config ITCO_WDT always visible, and add
sub-options ITCO_WDT_LPC and ITCO_WDT_I2C which depend on it. Selecting
either would select the driver it depends on:

config ITCO_WDT_LPC
	bool "Intel TCO Timer/Watchdog on LPC"
	depends on ITCO_WDT
	select LPC_ICH

config ITCO_WDT_I2C
	bool "Intel TCO Timer/Watchdog on I2C"
	depends on ITCO_WDT
	select I2C_I801

I did not test that, some care might be needed due to tristate vs.
boolean.

I personally prefer the first approach. It may not be as clean as the
second approach but it should be good enough in practice and avoids
cluttering Kconfig with even more options.

-- 
Jean Delvare
SUSE L3 Support

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

[Lee Jones]

On Tue, 28 Jul 2015, Guenter Roeck wrote:

> On Tue, Jul 28, 2015 at 06:32:16PM +0100, Lee Jones wrote:
> > On Tue, 28 Jul 2015, Guenter Roeck wrote:
> > 
> > > On 07/28/2015 08:28 AM, Lee Jones wrote:
> > > >On Tue, 28 Jul 2015, Guenter Roeck wrote:
> > > >
> > > >>On 07/28/2015 08:00 AM, Lee Jones wrote:
> > > >>>On Tue, 28 Jul 2015, Matt Fleming wrote:
> > > >>>>On Tue, 28 Jul, at 12:37:21PM, Lee Jones wrote:
> > > >>>>>
> > > >>>>>The driver shouldn't be called that either.
> > > >>>>>
> > > >>>>>You are the only one.  What makes iTCO 'special'?
> > > >>>>
> > > >>>>I don't know, I didn't write it. It looks like Wim did ~9 years ago, so
> > > >>>>it must have made sense to him at the time.
> > > >>>>
> > > >>
> > > >>Coding style wasn't as strict then as it is today. iTCO has just been kept
> > > >>for historic reasons.
> > > >
> > > >For sure, I get that, but it doesn't mean we can't do-the-right-thing
> > > >(tm) now does it?
> > > >
> > > >>Sure, we could have changed it to lowercase, but so far no one bothered.
> > > >>Plus, of course, there is always the element that some maintainers hate
> > > >>that kind of cleanup,
> > > >
> > > >Really?  Surely any kind of clean-up is good clean-up.  Especially as
> > > >Greg KH et. al, have been doing public presentations telling everyone
> > > >that there is always kernel work for anyone who has the time; spelling
> > > >corrections and all.
> > > >
> > > 
> > > Yes, really. Just try to submit cleanup patches to maintainers other than
> > > Greg and myself, and you'll see. It is a minefield.
> > 
> > Admittedly some of us have our quirks, but I'm happy to challenge
> > anyone that won't accept clean-up patches that make things better.
> > 
> There might possibly be a discussion at the kernel summit about that,
> in the context of encouraging (or discouraging) new kernel developers.
> If you are invited to the KS, it might make sense to attend that discussion.

I'd be happy to, but I don't have any first-hand experience of these
awkward (backward) Maintainers, who for some reason thing 'no change'
("don't touch it, it works"?) is better than 'improvement'. 

If/when you come across it, a link would be appreciated.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Lee Jones]

On Tue, 28 Jul 2015, Guenter Roeck wrote:

> On Tue, Jul 28, 2015 at 06:32:16PM +0100, Lee Jones wrote:
> > On Tue, 28 Jul 2015, Guenter Roeck wrote:
> > 
> > > On 07/28/2015 08:28 AM, Lee Jones wrote:
> > > >On Tue, 28 Jul 2015, Guenter Roeck wrote:
> > > >
> > > >>On 07/28/2015 08:00 AM, Lee Jones wrote:
> > > >>>On Tue, 28 Jul 2015, Matt Fleming wrote:
> > > >>>>On Tue, 28 Jul, at 12:37:21PM, Lee Jones wrote:
> > > >>>>>
> > > >>>>>The driver shouldn't be called that either.
> > > >>>>>
> > > >>>>>You are the only one.  What makes iTCO 'special'?
> > > >>>>
> > > >>>>I don't know, I didn't write it. It looks like Wim did ~9 years ago, so
> > > >>>>it must have made sense to him at the time.
> > > >>>>
> > > >>
> > > >>Coding style wasn't as strict then as it is today. iTCO has just been kept
> > > >>for historic reasons.
> > > >
> > > >For sure, I get that, but it doesn't mean we can't do-the-right-thing
> > > >(tm) now does it?
> > > >
> > > >>Sure, we could have changed it to lowercase, but so far no one bothered.
> > > >>Plus, of course, there is always the element that some maintainers hate
> > > >>that kind of cleanup,
> > > >
> > > >Really?  Surely any kind of clean-up is good clean-up.  Especially as
> > > >Greg KH et. al, have been doing public presentations telling everyone
> > > >that there is always kernel work for anyone who has the time; spelling
> > > >corrections and all.
> > > >
> > > 
> > > Yes, really. Just try to submit cleanup patches to maintainers other than
> > > Greg and myself, and you'll see. It is a minefield.
> > 
> > Admittedly some of us have our quirks, but I'm happy to challenge
> > anyone that won't accept clean-up patches that make things better.
> > 
> There might possibly be a discussion at the kernel summit about that,
> in the context of encouraging (or discouraging) new kernel developers.
> If you are invited to the KS, it might make sense to attend that discussion.

I'd be happy to, but I don't have any first-hand experience of these
awkward (backward) Maintainers, who for some reason thing 'no change'
("don't touch it, it works"?) is better than 'improvement'. 

If/when you come across it, a link would be appreciated.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Lee Jones]

On Tue, 28 Jul 2015, Aaron Sierra wrote:

> > > > > @@ -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);
> > > > 
> > > > Why do you have an mfd_add_devices() call for each device?
> > >  
> > > Good question. This call has been present since March 2012 when support
> > > was first added for iTCO_wdt in commit 887c8ec7219f ("watchdog: Convert
> > > iTCO_wdt driver to mfd model").
> > > 
> > > There's no good reason that I can see. Aaron?
> 
> I chose to call mfd_add_devices() in each device init function
> because I thought it was the easiest way to avoid registering an
> incomplete/invalid MFD cell should an error occur during init.
> 
> That way device registration wouldn't be an all-or-nothing affair.
> 
> Doesn't mfd_add_devices() bail out after the first unsuccessful
> mfd to platform device translation?

Right, as it should.

Under what circumstance would an error occur and you'd wish to carry
on registering devices?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Jean Delvare]

Hi Lee,

On Tue, 28 Jul 2015 18:32:16 +0100, Lee Jones wrote:
> On Tue, 28 Jul 2015, Guenter Roeck wrote:
> > On 07/28/2015 08:28 AM, Lee Jones wrote:
> > >On Tue, 28 Jul 2015, Guenter Roeck wrote:
> > >>Sure, we could have changed it to lowercase, but so far no one bothered.
> > >>Plus, of course, there is always the element that some maintainers hate
> > >>that kind of cleanup,
> > >
> > >Really?  Surely any kind of clean-up is good clean-up.  Especially as
> > >Greg KH et. al, have been doing public presentations telling everyone
> > >that there is always kernel work for anyone who has the time; spelling
> > >corrections and all.
> > 
> > Yes, really. Just try to submit cleanup patches to maintainers other than
> > Greg and myself, and you'll see. It is a minefield.
> 
> Admittedly some of us have our quirks, but I'm happy to challenge
> anyone that won't accept clean-up patches that make things better.

I may be one of these, to some degree, under certain circumstances.

The problem is that what you call "better" may not actually sound
better to me. Or it might be better in some respect but worse in others.

Specifically, your proposal to rename a kernel driver to remove
capitals, obviously has upsides, but it may also have downsides. Are
modprobe and friends case-insensitive? If not then renaming the module
that way may break existing "options" or "blacklist" statements
in /etc/modprobe.conf, or initialization scripts.

Such a change may also require some changes on the distribution side,
from a packaging perspective.

Likewise, renaming variables in the code makes it look better, but at
the cost of making future backports to that driver more difficult.

And if nothing else, the time you (or others) spend on this, is time
you won't spend somewhere else where it may be more useful. Or fun.

So in the end there's always a balance between the costs and the
benefits. Which may explain why sometimes some maintainers aren't so
interested in certain clean-up patches.

-- 
Jean Delvare
SUSE L3 Support

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

[Lee Jones]

On Wed, 29 Jul 2015, Jean Delvare wrote:

> Hi Lee,
> 
> On Tue, 28 Jul 2015 18:32:16 +0100, Lee Jones wrote:
> > On Tue, 28 Jul 2015, Guenter Roeck wrote:
> > > On 07/28/2015 08:28 AM, Lee Jones wrote:
> > > >On Tue, 28 Jul 2015, Guenter Roeck wrote:
> > > >>Sure, we could have changed it to lowercase, but so far no one bothered.
> > > >>Plus, of course, there is always the element that some maintainers hate
> > > >>that kind of cleanup,
> > > >
> > > >Really?  Surely any kind of clean-up is good clean-up.  Especially as
> > > >Greg KH et. al, have been doing public presentations telling everyone
> > > >that there is always kernel work for anyone who has the time; spelling
> > > >corrections and all.
> > > 
> > > Yes, really. Just try to submit cleanup patches to maintainers other than
> > > Greg and myself, and you'll see. It is a minefield.
> > 
> > Admittedly some of us have our quirks, but I'm happy to challenge
> > anyone that won't accept clean-up patches that make things better.
> 
> I may be one of these, to some degree, under certain circumstances.
> 
> The problem is that what you call "better" may not actually sound
> better to me. Or it might be better in some respect but worse in others.

Granted, there must always be a certain degree of common sense
involved.

> Specifically, your proposal to rename a kernel driver to remove
> capitals, obviously has upsides, but it may also have downsides. Are
> modprobe and friends case-insensitive? If not then renaming the module
> that way may break existing "options" or "blacklist" statements
> in /etc/modprobe.conf, or initialization scripts.

We rename/move/add/remove drivers all the time.  I understand that
you're speaking from a distribution PoV, but you guys have to take
these actions into account as a matter of course.  When you move to a
new kernel, you'll have teams who re-generate the aforementioned
files, or breakages would occur on every single release.

> Such a change may also require some changes on the distribution side,
> from a packaging perspective.

Right, which would be one of the responsibilities of the kernel
package maintainer at any given distro.  How is this any different to
any other kernel up-level?

> Likewise, renaming variables in the code makes it look better, but at
> the cost of making future backports to that driver more difficult.

This is something which has to be taken into consideration, granted.
Looking at this example, I can see 4 backports that happened in the
last 4 years, and each of them would have been trivial to fix.

> And if nothing else, the time you (or others) spend on this, is time
> you won't spend somewhere else where it may be more useful. Or fun.

There is no better way to pass the day than to abide coding standards
and my time is worthless. ;)

> So in the end there's always a balance between the costs and the
> benefits. Which may explain why sometimes some maintainers aren't so
> interested in certain clean-up patches.

If there are technical reasons why 'better' isn't really BETTER, then
I absolutely agree with you, but when I said 'better' before, I really
did mean BETTER.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Lee Jones]

On Wed, 29 Jul 2015, Jean Delvare wrote:

> Hi Lee,
> 
> On Tue, 28 Jul 2015 18:32:16 +0100, Lee Jones wrote:
> > On Tue, 28 Jul 2015, Guenter Roeck wrote:
> > > On 07/28/2015 08:28 AM, Lee Jones wrote:
> > > >On Tue, 28 Jul 2015, Guenter Roeck wrote:
> > > >>Sure, we could have changed it to lowercase, but so far no one bothered.
> > > >>Plus, of course, there is always the element that some maintainers hate
> > > >>that kind of cleanup,
> > > >
> > > >Really?  Surely any kind of clean-up is good clean-up.  Especially as
> > > >Greg KH et. al, have been doing public presentations telling everyone
> > > >that there is always kernel work for anyone who has the time; spelling
> > > >corrections and all.
> > > 
> > > Yes, really. Just try to submit cleanup patches to maintainers other than
> > > Greg and myself, and you'll see. It is a minefield.
> > 
> > Admittedly some of us have our quirks, but I'm happy to challenge
> > anyone that won't accept clean-up patches that make things better.
> 
> I may be one of these, to some degree, under certain circumstances.
> 
> The problem is that what you call "better" may not actually sound
> better to me. Or it might be better in some respect but worse in others.

Granted, there must always be a certain degree of common sense
involved.

> Specifically, your proposal to rename a kernel driver to remove
> capitals, obviously has upsides, but it may also have downsides. Are
> modprobe and friends case-insensitive? If not then renaming the module
> that way may break existing "options" or "blacklist" statements
> in /etc/modprobe.conf, or initialization scripts.

We rename/move/add/remove drivers all the time.  I understand that
you're speaking from a distribution PoV, but you guys have to take
these actions into account as a matter of course.  When you move to a
new kernel, you'll have teams who re-generate the aforementioned
files, or breakages would occur on every single release.

> Such a change may also require some changes on the distribution side,
> from a packaging perspective.

Right, which would be one of the responsibilities of the kernel
package maintainer at any given distro.  How is this any different to
any other kernel up-level?

> Likewise, renaming variables in the code makes it look better, but at
> the cost of making future backports to that driver more difficult.

This is something which has to be taken into consideration, granted.
Looking at this example, I can see 4 backports that happened in the
last 4 years, and each of them would have been trivial to fix.

> And if nothing else, the time you (or others) spend on this, is time
> you won't spend somewhere else where it may be more useful. Or fun.

There is no better way to pass the day than to abide coding standards
and my time is worthless. ;)

> So in the end there's always a balance between the costs and the
> benefits. Which may explain why sometimes some maintainers aren't so
> interested in certain clean-up patches.

If there are technical reasons why 'better' isn't really BETTER, then
I absolutely agree with you, but when I said 'better' before, I really
did mean BETTER.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Matt Fleming]

On Tue, 28 Jul, at 07:03:41PM, Jean Delvare wrote:
> Hi Matt,
> 
> Le Monday 27 July 2015 à 14:38 +0100, Matt Fleming a écrit :
> > 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.
> > 
> > Cc: Wim Van Sebroeck <wim@iguana.be>
> > Signed-off-by: Matt Fleming <matt.fleming@intel.com>
> > ---
> >  drivers/watchdog/iTCO_wdt.c | 58 ++++++++++++++++++++++++++-------------------
> >  1 file changed, 34 insertions(+), 24 deletions(-)
> > 
> > diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
> > index 9a6e70976f64..17dfbc51b85a 100644
> > --- a/drivers/watchdog/iTCO_wdt.c
> > +++ b/drivers/watchdog/iTCO_wdt.c
> > (...) 
> > @@ -503,7 +510,10 @@ static int iTCO_wdt_probe(struct platform_device *dev)
> >  		pdata->name, pdata->iTCO_version, (u64)TCOBASE);
> >  
> >  	/* Clear out the (probably old) status */
> > -	if (iTCO_wdt_private.iTCO_version == 3) {
> > +	if (iTCO_wdt_private.iTCO_version == 4) {
> > +		outw(0x0008, TCO1_STS);	/* Clear the Time Out Status bit */
> > +		outw(0x0002, TCO2_STS);	/* Clear SECOND_TO_STS bit */
> > +	} else if (iTCO_wdt_private.iTCO_version == 3) {
> >  		outl(0x20008, TCO1_STS);
> >  	} else {
> >  		outw(0x0008, TCO1_STS);	/* Clear the Time Out Status bit */
> 
> The "version == 4" branch is a subset of the "else" branch, so you could
> merge both with a conditional. If you prefer not to, then it probably
> makes sense to change the whole block to a switch/case construct.

I think the switch/case construct is the right choice here. I'll make the
update, thanks.

-- 
Matt Fleming, Intel Open Source Technology Center

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

[Matt Fleming]

On Tue, 28 Jul, at 07:03:41PM, Jean Delvare wrote:
> Hi Matt,
> 
> Le Monday 27 July 2015 à 14:38 +0100, Matt Fleming a écrit :
> > 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.
> > 
> > Cc: Wim Van Sebroeck <wim@iguana.be>
> > Signed-off-by: Matt Fleming <matt.fleming@intel.com>
> > ---
> >  drivers/watchdog/iTCO_wdt.c | 58 ++++++++++++++++++++++++++-------------------
> >  1 file changed, 34 insertions(+), 24 deletions(-)
> > 
> > diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
> > index 9a6e70976f64..17dfbc51b85a 100644
> > --- a/drivers/watchdog/iTCO_wdt.c
> > +++ b/drivers/watchdog/iTCO_wdt.c
> > (...) 
> > @@ -503,7 +510,10 @@ static int iTCO_wdt_probe(struct platform_device *dev)
> >  		pdata->name, pdata->iTCO_version, (u64)TCOBASE);
> >  
> >  	/* Clear out the (probably old) status */
> > -	if (iTCO_wdt_private.iTCO_version == 3) {
> > +	if (iTCO_wdt_private.iTCO_version == 4) {
> > +		outw(0x0008, TCO1_STS);	/* Clear the Time Out Status bit */
> > +		outw(0x0002, TCO2_STS);	/* Clear SECOND_TO_STS bit */
> > +	} else if (iTCO_wdt_private.iTCO_version == 3) {
> >  		outl(0x20008, TCO1_STS);
> >  	} else {
> >  		outw(0x0008, TCO1_STS);	/* Clear the Time Out Status bit */
> 
> The "version == 4" branch is a subset of the "else" branch, so you could
> merge both with a conditional. If you prefer not to, then it probably
> makes sense to change the whole block to a switch/case construct.

I think the switch/case construct is the right choice here. I'll make the
update, thanks.

-- 
Matt Fleming, Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Matt Fleming]

On Wed, 29 Jul, at 09:29:34AM, Jean Delvare wrote:
> Hi Matt,
> 
> On Mon, 27 Jul 2015 14:38:08 +0100, Matt Fleming wrote:
> > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> > index 241fafde42cb..5336fe2ff689 100644
> > --- a/drivers/watchdog/Kconfig
> > +++ b/drivers/watchdog/Kconfig
> > @@ -797,7 +797,7 @@ config ITCO_WDT
> >  	tristate "Intel TCO Timer/Watchdog"
> >  	depends on (X86 || IA64) && PCI
> >  	select WATCHDOG_CORE
> > -	select LPC_ICH
> > +	depends on LPC_ICH || I2C_I801
> >  	---help---
> >  	  Hardware driver for the intel TCO timer based watchdog devices.
> >  	  These drivers are included in the Intel 82801 I/O Controller
> 
> I don't like that. Depending on the order of the subsystems in the
> Kconfig tree, the user may not see the option and has no clue that the
> option exists. When he/she later enables LPC_ICH or I2C_I801, the
> option becomes visible but he/she has no reason to return here to
> enable it.
 
Good point!

> I can think of several ways to address the issue. One of them is to
> select both drivers, as this is what most users will want anyway:
> 
> 	select LPC_ICH if !EXPERT
> 	select I2C_I801 if !EXPERT
> 
> Another possibility is to make config ITCO_WDT always visible, and add
> sub-options ITCO_WDT_LPC and ITCO_WDT_I2C which depend on it. Selecting
> either would select the driver it depends on:
> 
> config ITCO_WDT_LPC
> 	bool "Intel TCO Timer/Watchdog on LPC"
> 	depends on ITCO_WDT
> 	select LPC_ICH
> 
> config ITCO_WDT_I2C
> 	bool "Intel TCO Timer/Watchdog on I2C"
> 	depends on ITCO_WDT
> 	select I2C_I801
> 
> I did not test that, some care might be needed due to tristate vs.
> boolean.
> 
> I personally prefer the first approach. It may not be as clean as the
> second approach but it should be good enough in practice and avoids
> cluttering Kconfig with even more options.

Yeah, I'm with you on that. The first approach seems superior to me
because it doesn't require users to know which bus the TCO watchdog
device lives on for their platforms.

-- 
Matt Fleming, Intel Open Source Technology Center

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

[Aaron Sierra]

> From: "Lee Jones" <lee.jones@linaro.org>
> Sent: Wednesday, July 29, 2015 2:38:41 AM
> 
> On Tue, 28 Jul 2015, Aaron Sierra wrote:
> 
> > > > > > @@ -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);
> > > > > 
> > > > > Why do you have an mfd_add_devices() call for each device?
> > > >  
> > > > Good question. This call has been present since March 2012 when support
> > > > was first added for iTCO_wdt in commit 887c8ec7219f ("watchdog: Convert
> > > > iTCO_wdt driver to mfd model").
> > > > 
> > > > There's no good reason that I can see. Aaron?
> > 
> > I chose to call mfd_add_devices() in each device init function
> > because I thought it was the easiest way to avoid registering an
> > incomplete/invalid MFD cell should an error occur during init.
> > 
> > That way device registration wouldn't be an all-or-nothing affair.
> > 
> > Doesn't mfd_add_devices() bail out after the first unsuccessful
> > mfd to platform device translation?
> 
> Right, as it should.
> 
> Under what circumstance would an error occur and you'd wish to carry
> on registering devices?

Lee,

The two devices that this driver is responsible for are conceptually
independent; they simply are lumped together in one PCI device. No
failure while preparing resources for the watchdog device should
prevent the GPIO device from being registered.

The most common real world circumstance that I experience is when a
BIOS reserves resources associated with the GPIO device, thus
preventing the GPIO resources (ICH_RES_GPE0 and/or ICH_RES_GPIO) from
being fully prepared.

I have not experienced issues with the watchdog device, but a similar
issue would exist if the RCBA were disabled in a "v2" device.

It seems like a dangerous change to simply attempt to register both
of these devices with a single call, when one or both of them could
be incomplete.

Perhaps your real issue with this driver structure is that these
cells are elements of a single lpc_ich_cells array for no clear
reason. If each had a dedicated mfd_cell variable, would that be
more acceptable to you?

-static struct mfd_cell lpc_ich_cells[] = {
+static struct mfd_cell lpc_ich_wdt_cell = {
...
+static struct mfd_cell lpc_ich_gpio_cell = {

That would eliminate the need for the lpc_cells enum, too.

-Aaron S.

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

[Lee Jones]

On Wed, 29 Jul 2015, Aaron Sierra wrote:

> > From: "Lee Jones" <lee.jones@linaro.org>
> > Sent: Wednesday, July 29, 2015 2:38:41 AM
> > 
> > On Tue, 28 Jul 2015, Aaron Sierra wrote:
> > 
> > > > > > > @@ -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);
> > > > > > 
> > > > > > Why do you have an mfd_add_devices() call for each device?
> > > > >  
> > > > > Good question. This call has been present since March 2012 when support
> > > > > was first added for iTCO_wdt in commit 887c8ec7219f ("watchdog: Convert
> > > > > iTCO_wdt driver to mfd model").
> > > > > 
> > > > > There's no good reason that I can see. Aaron?
> > > 
> > > I chose to call mfd_add_devices() in each device init function
> > > because I thought it was the easiest way to avoid registering an
> > > incomplete/invalid MFD cell should an error occur during init.
> > > 
> > > That way device registration wouldn't be an all-or-nothing affair.
> > > 
> > > Doesn't mfd_add_devices() bail out after the first unsuccessful
> > > mfd to platform device translation?
> > 
> > Right, as it should.
> > 
> > Under what circumstance would an error occur and you'd wish to carry
> > on registering devices?
> 
> Lee,
> 
> The two devices that this driver is responsible for are conceptually
> independent; they simply are lumped together in one PCI device. No
> failure while preparing resources for the watchdog device should
> prevent the GPIO device from being registered.

This makes me think that perhaps this isn't an MFD at all then?

Perhaps I should invest some time to looking into that.

> The most common real world circumstance that I experience is when a
> BIOS reserves resources associated with the GPIO device, thus
> preventing the GPIO resources (ICH_RES_GPE0 and/or ICH_RES_GPIO) from
> being fully prepared.
> 
> I have not experienced issues with the watchdog device, but a similar
> issue would exist if the RCBA were disabled in a "v2" device.
> 
> It seems like a dangerous change to simply attempt to register both
> of these devices with a single call, when one or both of them could
> be incomplete.
> 
> Perhaps your real issue with this driver structure is that these
> cells are elements of a single lpc_ich_cells array for no clear
> reason. If each had a dedicated mfd_cell variable, would that be
> more acceptable to you?
> 
> -static struct mfd_cell lpc_ich_cells[] = {
> +static struct mfd_cell lpc_ich_wdt_cell = {
> ...
> +static struct mfd_cell lpc_ich_gpio_cell = {
> 
> That would eliminate the need for the lpc_cells enum, too.

Yes, that would make more sense.  Also consider using mfd_add_device()
instead of mfd_add_devices(), as you are only attempting registration
for a single device.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Lee Jones]

On Wed, 29 Jul 2015, Aaron Sierra wrote:

> > From: "Lee Jones" <lee.jones@linaro.org>
> > Sent: Wednesday, July 29, 2015 2:38:41 AM
> > 
> > On Tue, 28 Jul 2015, Aaron Sierra wrote:
> > 
> > > > > > > @@ -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);
> > > > > > 
> > > > > > Why do you have an mfd_add_devices() call for each device?
> > > > >  
> > > > > Good question. This call has been present since March 2012 when support
> > > > > was first added for iTCO_wdt in commit 887c8ec7219f ("watchdog: Convert
> > > > > iTCO_wdt driver to mfd model").
> > > > > 
> > > > > There's no good reason that I can see. Aaron?
> > > 
> > > I chose to call mfd_add_devices() in each device init function
> > > because I thought it was the easiest way to avoid registering an
> > > incomplete/invalid MFD cell should an error occur during init.
> > > 
> > > That way device registration wouldn't be an all-or-nothing affair.
> > > 
> > > Doesn't mfd_add_devices() bail out after the first unsuccessful
> > > mfd to platform device translation?
> > 
> > Right, as it should.
> > 
> > Under what circumstance would an error occur and you'd wish to carry
> > on registering devices?
> 
> Lee,
> 
> The two devices that this driver is responsible for are conceptually
> independent; they simply are lumped together in one PCI device. No
> failure while preparing resources for the watchdog device should
> prevent the GPIO device from being registered.

This makes me think that perhaps this isn't an MFD at all then?

Perhaps I should invest some time to looking into that.

> The most common real world circumstance that I experience is when a
> BIOS reserves resources associated with the GPIO device, thus
> preventing the GPIO resources (ICH_RES_GPE0 and/or ICH_RES_GPIO) from
> being fully prepared.
> 
> I have not experienced issues with the watchdog device, but a similar
> issue would exist if the RCBA were disabled in a "v2" device.
> 
> It seems like a dangerous change to simply attempt to register both
> of these devices with a single call, when one or both of them could
> be incomplete.
> 
> Perhaps your real issue with this driver structure is that these
> cells are elements of a single lpc_ich_cells array for no clear
> reason. If each had a dedicated mfd_cell variable, would that be
> more acceptable to you?
> 
> -static struct mfd_cell lpc_ich_cells[] = {
> +static struct mfd_cell lpc_ich_wdt_cell = {
> ...
> +static struct mfd_cell lpc_ich_gpio_cell = {
> 
> That would eliminate the need for the lpc_cells enum, too.

Yes, that would make more sense.  Also consider using mfd_add_device()
instead of mfd_add_devices(), as you are only attempting registration
for a single device.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Guenter Roeck]

On 07/29/2015 08:32 AM, Lee Jones wrote:
> On Wed, 29 Jul 2015, Aaron Sierra wrote:
>
>>> From: "Lee Jones" <lee.jones@linaro.org>
>>> Sent: Wednesday, July 29, 2015 2:38:41 AM
>>>
>>> On Tue, 28 Jul 2015, Aaron Sierra wrote:
>>>
>>>>>>>> @@ -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);
>>>>>>>
>>>>>>> Why do you have an mfd_add_devices() call for each device?
>>>>>>
>>>>>> Good question. This call has been present since March 2012 when support
>>>>>> was first added for iTCO_wdt in commit 887c8ec7219f ("watchdog: Convert
>>>>>> iTCO_wdt driver to mfd model").
>>>>>>
>>>>>> There's no good reason that I can see. Aaron?
>>>>
>>>> I chose to call mfd_add_devices() in each device init function
>>>> because I thought it was the easiest way to avoid registering an
>>>> incomplete/invalid MFD cell should an error occur during init.
>>>>
>>>> That way device registration wouldn't be an all-or-nothing affair.
>>>>
>>>> Doesn't mfd_add_devices() bail out after the first unsuccessful
>>>> mfd to platform device translation?
>>>
>>> Right, as it should.
>>>
>>> Under what circumstance would an error occur and you'd wish to carry
>>> on registering devices?
>>
>> Lee,
>>
>> The two devices that this driver is responsible for are conceptually
>> independent; they simply are lumped together in one PCI device. No
>> failure while preparing resources for the watchdog device should
>> prevent the GPIO device from being registered.
>
> This makes me think that perhaps this isn't an MFD at all then?
>
> Perhaps I should invest some time to looking into that.
>

The alternative, unless I am missing something, would be to
bind two drivers to the same pci device, which is not currently
possible in Linux. How would you suggest to do that if not with
an mfd driver ?

Thanks,
Guenter

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

[Aaron Sierra]

> From: "Lee Jones" <lee.jones@linaro.org>
> Sent: Wednesday, July 29, 2015 10:32:26 AM
> 
> On Wed, 29 Jul 2015, Aaron Sierra wrote:
> 
> > > From: "Lee Jones" <lee.jones@linaro.org>
> > > Sent: Wednesday, July 29, 2015 2:38:41 AM
> > > 
> > > On Tue, 28 Jul 2015, Aaron Sierra wrote:
> > > 
> > > > > > > > @@ -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);
> > > > > > > 
> > > > > > > Why do you have an mfd_add_devices() call for each device?
> > > > > >  
> > > > > > Good question. This call has been present since March 2012 when
> > > > > > support
> > > > > > was first added for iTCO_wdt in commit 887c8ec7219f ("watchdog:
> > > > > > Convert
> > > > > > iTCO_wdt driver to mfd model").
> > > > > > 
> > > > > > There's no good reason that I can see. Aaron?
> > > > 
> > > > I chose to call mfd_add_devices() in each device init function
> > > > because I thought it was the easiest way to avoid registering an
> > > > incomplete/invalid MFD cell should an error occur during init.
> > > > 
> > > > That way device registration wouldn't be an all-or-nothing affair.
> > > > 
> > > > Doesn't mfd_add_devices() bail out after the first unsuccessful
> > > > mfd to platform device translation?
> > > 
> > > Right, as it should.
> > > 
> > > Under what circumstance would an error occur and you'd wish to carry
> > > on registering devices?
> > 
> > Lee,
> > 
> > The two devices that this driver is responsible for are conceptually
> > independent; they simply are lumped together in one PCI device. No
> > failure while preparing resources for the watchdog device should
> > prevent the GPIO device from being registered.
> 
> This makes me think that perhaps this isn't an MFD at all then?
> 
> Perhaps I should invest some time to looking into that.
> 
> > The most common real world circumstance that I experience is when a
> > BIOS reserves resources associated with the GPIO device, thus
> > preventing the GPIO resources (ICH_RES_GPE0 and/or ICH_RES_GPIO) from
> > being fully prepared.
> > 
> > I have not experienced issues with the watchdog device, but a similar
> > issue would exist if the RCBA were disabled in a "v2" device.
> > 
> > It seems like a dangerous change to simply attempt to register both
> > of these devices with a single call, when one or both of them could
> > be incomplete.
> > 
> > Perhaps your real issue with this driver structure is that these
> > cells are elements of a single lpc_ich_cells array for no clear
> > reason. If each had a dedicated mfd_cell variable, would that be
> > more acceptable to you?
> > 
> > -static struct mfd_cell lpc_ich_cells[] = {
> > +static struct mfd_cell lpc_ich_wdt_cell = {
> > ...
> > +static struct mfd_cell lpc_ich_gpio_cell = {
> > 
> > That would eliminate the need for the lpc_cells enum, too.
> 
> Yes, that would make more sense.  Also consider using mfd_add_device()
> instead of mfd_add_devices(), as you are only attempting registration
> for a single device.
> 

I can submit a patch the splits up the array elements, but I
only see mfd_add_device() as a static function in mfd-core.c;
Is that being exported in a development branch somewhere?

-Aaron S.

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

[Guenter Roeck]

On 07/29/2015 09:20 AM, Aaron Sierra wrote:
>> From: "Lee Jones" <lee.jones@linaro.org>
>> Sent: Wednesday, July 29, 2015 10:32:26 AM
>>
>> On Wed, 29 Jul 2015, Aaron Sierra wrote:
>>
>>>> From: "Lee Jones" <lee.jones@linaro.org>
>>>> Sent: Wednesday, July 29, 2015 2:38:41 AM
>>>>
>>>> On Tue, 28 Jul 2015, Aaron Sierra wrote:
>>>>
>>>>>>>>> @@ -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);
>>>>>>>>
>>>>>>>> Why do you have an mfd_add_devices() call for each device?
>>>>>>>
>>>>>>> Good question. This call has been present since March 2012 when
>>>>>>> support
>>>>>>> was first added for iTCO_wdt in commit 887c8ec7219f ("watchdog:
>>>>>>> Convert
>>>>>>> iTCO_wdt driver to mfd model").
>>>>>>>
>>>>>>> There's no good reason that I can see. Aaron?
>>>>>
>>>>> I chose to call mfd_add_devices() in each device init function
>>>>> because I thought it was the easiest way to avoid registering an
>>>>> incomplete/invalid MFD cell should an error occur during init.
>>>>>
>>>>> That way device registration wouldn't be an all-or-nothing affair.
>>>>>
>>>>> Doesn't mfd_add_devices() bail out after the first unsuccessful
>>>>> mfd to platform device translation?
>>>>
>>>> Right, as it should.
>>>>
>>>> Under what circumstance would an error occur and you'd wish to carry
>>>> on registering devices?
>>>
>>> Lee,
>>>
>>> The two devices that this driver is responsible for are conceptually
>>> independent; they simply are lumped together in one PCI device. No
>>> failure while preparing resources for the watchdog device should
>>> prevent the GPIO device from being registered.
>>
>> This makes me think that perhaps this isn't an MFD at all then?
>>
>> Perhaps I should invest some time to looking into that.
>>
>>> The most common real world circumstance that I experience is when a
>>> BIOS reserves resources associated with the GPIO device, thus
>>> preventing the GPIO resources (ICH_RES_GPE0 and/or ICH_RES_GPIO) from
>>> being fully prepared.
>>>
>>> I have not experienced issues with the watchdog device, but a similar
>>> issue would exist if the RCBA were disabled in a "v2" device.
>>>
>>> It seems like a dangerous change to simply attempt to register both
>>> of these devices with a single call, when one or both of them could
>>> be incomplete.
>>>
>>> Perhaps your real issue with this driver structure is that these
>>> cells are elements of a single lpc_ich_cells array for no clear
>>> reason. If each had a dedicated mfd_cell variable, would that be
>>> more acceptable to you?
>>>
>>> -static struct mfd_cell lpc_ich_cells[] = {
>>> +static struct mfd_cell lpc_ich_wdt_cell = {
>>> ...
>>> +static struct mfd_cell lpc_ich_gpio_cell = {
>>>
>>> That would eliminate the need for the lpc_cells enum, too.
>>
>> Yes, that would make more sense.  Also consider using mfd_add_device()
>> instead of mfd_add_devices(), as you are only attempting registration
>> for a single device.
>>
>
> I can submit a patch the splits up the array elements, but I
> only see mfd_add_device() as a static function in mfd-core.c;
> Is that being exported in a development branch somewhere?
>
Sure you want to do that ? You might have to move usage count
handling into the calling driver, and also provide mfd_remove_device().

Guenter

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

[Aaron Sierra]

----- Original Message -----
> From: "Guenter Roeck" <linux@roeck-us.net>
> Sent: Wednesday, July 29, 2015 11:38:08 AM
> 
> On 07/29/2015 09:20 AM, Aaron Sierra wrote:
> >> From: "Lee Jones" <lee.jones@linaro.org>
> >> Sent: Wednesday, July 29, 2015 10:32:26 AM
> >>
> >> On Wed, 29 Jul 2015, Aaron Sierra wrote:
> >>
> >>>> From: "Lee Jones" <lee.jones@linaro.org>
> >>>> Sent: Wednesday, July 29, 2015 2:38:41 AM
> >>>>
> >>>> On Tue, 28 Jul 2015, Aaron Sierra wrote:
> >>>>
> >>>>>>>>> @@ -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);
> >>>>>>>>
> >>>>>>>> Why do you have an mfd_add_devices() call for each device?
> >>>>>>>
> >>>>>>> Good question. This call has been present since March 2012 when
> >>>>>>> support
> >>>>>>> was first added for iTCO_wdt in commit 887c8ec7219f ("watchdog:
> >>>>>>> Convert
> >>>>>>> iTCO_wdt driver to mfd model").
> >>>>>>>
> >>>>>>> There's no good reason that I can see. Aaron?
> >>>>>
> >>>>> I chose to call mfd_add_devices() in each device init function
> >>>>> because I thought it was the easiest way to avoid registering an
> >>>>> incomplete/invalid MFD cell should an error occur during init.
> >>>>>
> >>>>> That way device registration wouldn't be an all-or-nothing affair.
> >>>>>
> >>>>> Doesn't mfd_add_devices() bail out after the first unsuccessful
> >>>>> mfd to platform device translation?
> >>>>
> >>>> Right, as it should.
> >>>>
> >>>> Under what circumstance would an error occur and you'd wish to carry
> >>>> on registering devices?
> >>>
> >>> Lee,
> >>>
> >>> The two devices that this driver is responsible for are conceptually
> >>> independent; they simply are lumped together in one PCI device. No
> >>> failure while preparing resources for the watchdog device should
> >>> prevent the GPIO device from being registered.
> >>
> >> This makes me think that perhaps this isn't an MFD at all then?
> >>
> >> Perhaps I should invest some time to looking into that.
> >>
> >>> The most common real world circumstance that I experience is when a
> >>> BIOS reserves resources associated with the GPIO device, thus
> >>> preventing the GPIO resources (ICH_RES_GPE0 and/or ICH_RES_GPIO) from
> >>> being fully prepared.
> >>>
> >>> I have not experienced issues with the watchdog device, but a similar
> >>> issue would exist if the RCBA were disabled in a "v2" device.
> >>>
> >>> It seems like a dangerous change to simply attempt to register both
> >>> of these devices with a single call, when one or both of them could
> >>> be incomplete.
> >>>
> >>> Perhaps your real issue with this driver structure is that these
> >>> cells are elements of a single lpc_ich_cells array for no clear
> >>> reason. If each had a dedicated mfd_cell variable, would that be
> >>> more acceptable to you?
> >>>
> >>> -static struct mfd_cell lpc_ich_cells[] = {
> >>> +static struct mfd_cell lpc_ich_wdt_cell = {
> >>> ...
> >>> +static struct mfd_cell lpc_ich_gpio_cell = {
> >>>
> >>> That would eliminate the need for the lpc_cells enum, too.
> >>
> >> Yes, that would make more sense.  Also consider using mfd_add_device()
> >> instead of mfd_add_devices(), as you are only attempting registration
> >> for a single device.
> >>
> >
> > I can submit a patch the splits up the array elements, but I
> > only see mfd_add_device() as a static function in mfd-core.c;
> > Is that being exported in a development branch somewhere?
> >
>
> Sure you want to do that ? You might have to move usage count
> handling into the calling driver, and also provide mfd_remove_device().

Nope, just the array split-up! Thanks Guenter.

-Aaron S.

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

[Lee Jones]

On Wed, 29 Jul 2015, Guenter Roeck wrote:

> On 07/29/2015 08:32 AM, Lee Jones wrote:
> >On Wed, 29 Jul 2015, Aaron Sierra wrote:
> >
> >>>From: "Lee Jones" <lee.jones@linaro.org>
> >>>Sent: Wednesday, July 29, 2015 2:38:41 AM
> >>>
> >>>On Tue, 28 Jul 2015, Aaron Sierra wrote:
> >>>
> >>>>>>>>@@ -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);
> >>>>>>>
> >>>>>>>Why do you have an mfd_add_devices() call for each device?
> >>>>>>
> >>>>>>Good question. This call has been present since March 2012 when support
> >>>>>>was first added for iTCO_wdt in commit 887c8ec7219f ("watchdog: Convert
> >>>>>>iTCO_wdt driver to mfd model").
> >>>>>>
> >>>>>>There's no good reason that I can see. Aaron?
> >>>>
> >>>>I chose to call mfd_add_devices() in each device init function
> >>>>because I thought it was the easiest way to avoid registering an
> >>>>incomplete/invalid MFD cell should an error occur during init.
> >>>>
> >>>>That way device registration wouldn't be an all-or-nothing affair.
> >>>>
> >>>>Doesn't mfd_add_devices() bail out after the first unsuccessful
> >>>>mfd to platform device translation?
> >>>
> >>>Right, as it should.
> >>>
> >>>Under what circumstance would an error occur and you'd wish to carry
> >>>on registering devices?
> >>
> >>Lee,
> >>
> >>The two devices that this driver is responsible for are conceptually
> >>independent; they simply are lumped together in one PCI device. No
> >>failure while preparing resources for the watchdog device should
> >>prevent the GPIO device from being registered.
> >
> >This makes me think that perhaps this isn't an MFD at all then?
> >
> >Perhaps I should invest some time to looking into that.
> >
> 
> The alternative, unless I am missing something, would be to
> bind two drivers to the same pci device, which is not currently
> possible in Linux. How would you suggest to do that if not with
> an mfd driver ?

As I said, I would need to look into it.  Perhaps this is the best way
we have of managing these devices in Linux.

Or perhaps I was just trying to provoke some thought/discussion. ;)

The MFD driver for this device looks fairly well written, so I'm not
offended that it's located there.  On the flip side, I am sensitive to
MFD becoming (more of?) a dumping ground for misfits that just don't
belong anywhere else.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog