Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1193093 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/5] iTCO_wdt: Add support for Intel Sunrisepoint Matt Fleming <matt@codeblueprint.co.uk> - 2015-07-27 15:40 +0200
    [PATCH 2/5] i2c: i801: Create iTCO device on newer Intel PCHs Matt Fleming <matt@codeblueprint.co.uk> - 2015-07-27 15:40 +0200
      Re: [PATCH 2/5] i2c: i801: Create iTCO device on newer Intel PCHs Guenter Roeck <linux@roeck-us.net> - 2015-07-27 16:10 +0200
        Re: [PATCH 2/5] i2c: i801: Create iTCO device on newer Intel PCHs Matt Fleming <matt@codeblueprint.co.uk> - 2015-07-28 11:40 +0200
    [PATCH 1/5] iTCO_wdt: Expose watchdog properties using platform data Matt Fleming <matt@codeblueprint.co.uk> - 2015-07-27 15:40 +0200
      Re: [PATCH 1/5] iTCO_wdt: Expose watchdog properties using platform  data Guenter Roeck <linux@roeck-us.net> - 2015-07-27 15:50 +0200
        Re: [PATCH 1/5] iTCO_wdt: Expose watchdog properties using platform  data Matt Fleming <matt@codeblueprint.co.uk> - 2015-07-27 16:20 +0200
          Re: [PATCH 1/5] iTCO_wdt: Expose watchdog properties using platform  data Guenter Roeck <linux@roeck-us.net> - 2015-07-27 16:30 +0200
            Re: [PATCH 1/5] iTCO_wdt: Expose watchdog properties using platform  data Matt Fleming <matt@codeblueprint.co.uk> - 2015-07-28 12:00 +0200
      Re: [PATCH 1/5] iTCO_wdt: Expose watchdog properties using platform  data Lee Jones <lee.jones@linaro.org> - 2015-07-27 23:40 +0200
        Re: [PATCH 1/5] iTCO_wdt: Expose watchdog properties using platform  data Matt Fleming <matt@codeblueprint.co.uk> - 2015-07-28 11:20 +0200
      Re: [PATCH 1/5] iTCO_wdt: Expose watchdog properties using platform  data Lee Jones <lee.jones@linaro.org> - 2015-07-28 11:50 +0200
        Re: [PATCH 1/5] iTCO_wdt: Expose watchdog properties using platform  data Matt Fleming <matt@codeblueprint.co.uk> - 2015-07-28 13:10 +0200
          Re: [PATCH 1/5] iTCO_wdt: Expose watchdog properties using platform  data Jean Delvare <jdelvare@suse.de> - 2015-07-29 11:10 +0200
            Re: [PATCH 1/5] iTCO_wdt: Expose watchdog properties using platform  data Lee Jones <lee.jones@linaro.org> - 2015-07-29 12:10 +0200
          Re: [PATCH 1/5] iTCO_wdt: Expose watchdog properties using platform  data Aaron Sierra <asierra@xes-inc.com> - 2015-07-29 17:00 +0200
            Re: [PATCH 1/5] iTCO_wdt: Expose watchdog properties using platform  data Lee Jones <lee.jones@linaro.org> - 2015-07-30 11:00 +0200

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

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

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

[toc] | [next] | [standalone]

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

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

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

[toc] | [prev] | [next] | [standalone]

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

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

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

[toc] | [prev] | [next] | [standalone]

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

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-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1193096 — [PATCH 1/5] iTCO_wdt: Expose watchdog properties using platform data

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

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

[toc] | [prev] | [next] | [standalone]

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

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

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

[toc] | [prev] | [next] | [standalone]

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

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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

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

[toc] | [prev] | [next] | [standalone]

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

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1193953 — 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.
> 
> 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-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web