[PATCH v5 0/6] i2c: Host Notify / i801 fixes

[PATCH v5 0/6] i2c: Host Notify / i801 fixes

[Benjamin Tissoires]

Hi Wolfram and Dmitry,

5th revision of the series, hopefully this time it will be OK.

The changes were requested by Dmitry: now, SMBus Host Notify is transparent
for clients drivers. The IRQ is attributed if the adapter has the capability
and if nobody claimed an IRQ before. That means that adding an I2C device
through sysfs works, and we don't need to do anything in the client drivers
besides regular IRQ handling.

Cheers,
Benjamin

Benjamin Tissoires (6):
  i2c: i801: store and restore the SLVCMD register at load and unload
  i2c: i801: minor formatting issues
  i2c: i801: use BIT() macro for bits definition
  i2c: i801: use the BIT() macro for FEATURES_* also
  i2c: i801: remove SMBNTFDDAT reads as they always seem to return 0
  i2c: use an IRQ to report Host Notify events, not alert

 Documentation/i2c/smbus-protocol |  12 ++--
 drivers/i2c/Kconfig              |   1 +
 drivers/i2c/busses/i2c-i801.c    | 121 +++++++++++++++++++--------------------
 drivers/i2c/i2c-core.c           | 113 ++++++++++++++++++++++++++++++++++++
 drivers/i2c/i2c-smbus.c          | 102 ---------------------------------
 include/linux/i2c-smbus.h        |  27 ---------
 include/linux/i2c.h              |   4 ++
 7 files changed, 185 insertions(+), 195 deletions(-)

-- 
2.7.4

[PATCH v5 1/6] i2c: i801: store and restore the SLVCMD register at load and unload

[Benjamin Tissoires]

Also do not override any other configuration in this register.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

---

no changes in v5

changes in v4:
- add the i801_disable_host_notify function here as this gets the
  first in the series

no changes in v3

new in v2
---
 drivers/i2c/busses/i2c-i801.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 22a0ed4..3a2fdf5 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -240,6 +240,7 @@ struct i801_priv {
 	struct i2c_adapter adapter;
 	unsigned long smba;
 	unsigned char original_hstcfg;
+	unsigned char original_slvcmd;
 	struct pci_dev *pci_dev;
 	unsigned int features;
 
@@ -952,13 +953,26 @@ static int i801_enable_host_notify(struct i2c_adapter *adapter)
 	if (!priv->host_notify)
 		return -ENOMEM;
 
-	outb_p(SMBSLVCMD_HST_NTFY_INTREN, SMBSLVCMD(priv));
+	priv->original_slvcmd = inb_p(SMBSLVCMD(priv));
+
+	if (!(SMBSLVCMD_HST_NTFY_INTREN & priv->original_slvcmd))
+		outb_p(SMBSLVCMD_HST_NTFY_INTREN | priv->original_slvcmd,
+		       SMBSLVCMD(priv));
+
 	/* clear Host Notify bit to allow a new notification */
 	outb_p(SMBSLVSTS_HST_NTFY_STS, SMBSLVSTS(priv));
 
 	return 0;
 }
 
+static void i801_disable_host_notify(struct i801_priv *priv)
+{
+	if (!(priv->features & FEATURE_HOST_NOTIFY))
+		return;
+
+	outb_p(priv->original_slvcmd, SMBSLVCMD(priv));
+}
+
 static const struct i2c_algorithm smbus_algorithm = {
 	.smbus_xfer	= i801_access,
 	.functionality	= i801_func,
@@ -1647,6 +1661,7 @@ static void i801_remove(struct pci_dev *dev)
 	pm_runtime_forbid(&dev->dev);
 	pm_runtime_get_noresume(&dev->dev);
 
+	i801_disable_host_notify(priv);
 	i801_del_mux(priv);
 	i2c_del_adapter(&priv->adapter);
 	i801_acpi_remove(priv);
-- 
2.7.4

[PATCH v5 2/6] i2c: i801: minor formatting issues

[Benjamin Tissoires]

No functional changes, just typos and remove unused #define.

Reviewed-by: Jean Delvare <jdelvare@suse.de>
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

---

no changes in v5

no changes in v4

no changes in v3

no changes in v2
---
 drivers/i2c/busses/i2c-i801.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 3a2fdf5..cfb74fc 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -186,10 +186,10 @@
 #define SMBHSTSTS_INTR		0x02
 #define SMBHSTSTS_HOST_BUSY	0x01
 
-/* Host Notify Status registers bits */
+/* Host Notify Status register bits */
 #define SMBSLVSTS_HST_NTFY_STS	1
 
-/* Host Notify Command registers bits */
+/* Host Notify Command register bits */
 #define SMBSLVCMD_HST_NTFY_INTREN	0x01
 
 #define STATUS_ERROR_FLAGS	(SMBHSTSTS_FAILED | SMBHSTSTS_BUS_ERR | \
@@ -270,8 +270,6 @@ struct i801_priv {
 	struct smbus_host_notify *host_notify;
 };
 
-#define SMBHSTNTFY_SIZE		8
-
 #define FEATURE_SMBUS_PEC	(1 << 0)
 #define FEATURE_BLOCK_BUFFER	(1 << 1)
 #define FEATURE_BLOCK_PROC	(1 << 2)
-- 
2.7.4

[PATCH v5 3/6] i2c: i801: use BIT() macro for bits definition

[Benjamin Tissoires]

i801 mixes hexadecimal and decimal values for defining bits. However,
we have a nice BIT() macro for this exact purpose.

No functional changes, cleanup only.

Reviewed-by: Jean Delvare <jdelvare@suse.de>
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

---

no changes in v5

no changes in v4

no changes in v3

no changes in v2
---
 drivers/i2c/busses/i2c-i801.c | 50 +++++++++++++++++++++----------------------
 1 file changed, 25 insertions(+), 25 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index cfb74fc..5928ee2 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -136,26 +136,26 @@
 #define SBREG_SMBCTRL		0xc6000c
 
 /* Host status bits for SMBPCISTS */
-#define SMBPCISTS_INTS		0x08
+#define SMBPCISTS_INTS		BIT(3)
 
 /* Control bits for SMBPCICTL */
-#define SMBPCICTL_INTDIS	0x0400
+#define SMBPCICTL_INTDIS	BIT(10)
 
 /* Host configuration bits for SMBHSTCFG */
-#define SMBHSTCFG_HST_EN	1
-#define SMBHSTCFG_SMB_SMI_EN	2
-#define SMBHSTCFG_I2C_EN	4
+#define SMBHSTCFG_HST_EN	BIT(0)
+#define SMBHSTCFG_SMB_SMI_EN	BIT(1)
+#define SMBHSTCFG_I2C_EN	BIT(2)
 
 /* TCO configuration bits for TCOCTL */
-#define TCOCTL_EN		0x0100
+#define TCOCTL_EN		BIT(8)
 
 /* Auxiliary status register bits, ICH4+ only */
-#define SMBAUXSTS_CRCE		1
-#define SMBAUXSTS_STCO		2
+#define SMBAUXSTS_CRCE		BIT(0)
+#define SMBAUXSTS_STCO		BIT(1)
 
 /* Auxiliary control register bits, ICH4+ only */
-#define SMBAUXCTL_CRC		1
-#define SMBAUXCTL_E32B		2
+#define SMBAUXCTL_CRC		BIT(0)
+#define SMBAUXCTL_E32B		BIT(1)
 
 /* Other settings */
 #define MAX_RETRIES		400
@@ -170,27 +170,27 @@
 #define I801_I2C_BLOCK_DATA	0x18	/* ICH5 and later */
 
 /* I801 Host Control register bits */
-#define SMBHSTCNT_INTREN	0x01
-#define SMBHSTCNT_KILL		0x02
-#define SMBHSTCNT_LAST_BYTE	0x20
-#define SMBHSTCNT_START		0x40
-#define SMBHSTCNT_PEC_EN	0x80	/* ICH3 and later */
+#define SMBHSTCNT_INTREN	BIT(0)
+#define SMBHSTCNT_KILL		BIT(1)
+#define SMBHSTCNT_LAST_BYTE	BIT(5)
+#define SMBHSTCNT_START		BIT(6)
+#define SMBHSTCNT_PEC_EN	BIT(7)	/* ICH3 and later */
 
 /* I801 Hosts Status register bits */
-#define SMBHSTSTS_BYTE_DONE	0x80
-#define SMBHSTSTS_INUSE_STS	0x40
-#define SMBHSTSTS_SMBALERT_STS	0x20
-#define SMBHSTSTS_FAILED	0x10
-#define SMBHSTSTS_BUS_ERR	0x08
-#define SMBHSTSTS_DEV_ERR	0x04
-#define SMBHSTSTS_INTR		0x02
-#define SMBHSTSTS_HOST_BUSY	0x01
+#define SMBHSTSTS_BYTE_DONE	BIT(7)
+#define SMBHSTSTS_INUSE_STS	BIT(6)
+#define SMBHSTSTS_SMBALERT_STS	BIT(5)
+#define SMBHSTSTS_FAILED	BIT(4)
+#define SMBHSTSTS_BUS_ERR	BIT(3)
+#define SMBHSTSTS_DEV_ERR	BIT(2)
+#define SMBHSTSTS_INTR		BIT(1)
+#define SMBHSTSTS_HOST_BUSY	BIT(0)
 
 /* Host Notify Status register bits */
-#define SMBSLVSTS_HST_NTFY_STS	1
+#define SMBSLVSTS_HST_NTFY_STS	BIT(0)
 
 /* Host Notify Command register bits */
-#define SMBSLVCMD_HST_NTFY_INTREN	0x01
+#define SMBSLVCMD_HST_NTFY_INTREN	BIT(0)
 
 #define STATUS_ERROR_FLAGS	(SMBHSTSTS_FAILED | SMBHSTSTS_BUS_ERR | \
 				 SMBHSTSTS_DEV_ERR)
-- 
2.7.4

[PATCH v5 4/6] i2c: i801: use the BIT() macro for FEATURES_* also

[Benjamin Tissoires]

no functional changes

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

---

no changes in v5

no changes in v4

no changes in v3

new in v2
---
 drivers/i2c/busses/i2c-i801.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 5928ee2..88b2e31 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -270,15 +270,15 @@ struct i801_priv {
 	struct smbus_host_notify *host_notify;
 };
 
-#define FEATURE_SMBUS_PEC	(1 << 0)
-#define FEATURE_BLOCK_BUFFER	(1 << 1)
-#define FEATURE_BLOCK_PROC	(1 << 2)
-#define FEATURE_I2C_BLOCK_READ	(1 << 3)
-#define FEATURE_IRQ		(1 << 4)
-#define FEATURE_HOST_NOTIFY	(1 << 5)
+#define FEATURE_SMBUS_PEC	BIT(0)
+#define FEATURE_BLOCK_BUFFER	BIT(1)
+#define FEATURE_BLOCK_PROC	BIT(2)
+#define FEATURE_I2C_BLOCK_READ	BIT(3)
+#define FEATURE_IRQ		BIT(4)
+#define FEATURE_HOST_NOTIFY	BIT(5)
 /* Not really a feature, but it's convenient to handle it as such */
-#define FEATURE_IDF		(1 << 15)
-#define FEATURE_TCO		(1 << 16)
+#define FEATURE_IDF		BIT(15)
+#define FEATURE_TCO		BIT(16)
 
 static const char *i801_feature_names[] = {
 	"SMBus PEC",
-- 
2.7.4

[PATCH v5 5/6] i2c: i801: remove SMBNTFDDAT reads as they always seem to return 0

[Benjamin Tissoires]

On the platform tested, reading SMBNTFDDAT always returns 0 (using 1 read
of a word or 2 of 2 bytes). Given that we are not sure why and that we
don't need to rely on the data parameter in the current users of Host
Notify, remove this part of the code.

If someone wants to re-enable it, just revert this commit and data should
be available.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

---

no changes in v5

no changes in v4

no changes in v3

new in v2
---
 drivers/i2c/busses/i2c-i801.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 88b2e31..42a6a89 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -117,7 +117,6 @@
 #define SMBSLVSTS(p)	(16 + (p)->smba)	/* ICH3 and later */
 #define SMBSLVCMD(p)	(17 + (p)->smba)	/* ICH3 and later */
 #define SMBNTFDADD(p)	(20 + (p)->smba)	/* ICH3 and later */
-#define SMBNTFDDAT(p)	(22 + (p)->smba)	/* ICH3 and later */
 
 /* PCI Address Constants */
 #define SMBBAR		4
@@ -578,12 +577,15 @@ static void i801_isr_byte_done(struct i801_priv *priv)
 static irqreturn_t i801_host_notify_isr(struct i801_priv *priv)
 {
 	unsigned short addr;
-	unsigned int data;
 
 	addr = inb_p(SMBNTFDADD(priv)) >> 1;
-	data = inw_p(SMBNTFDDAT(priv));
 
-	i2c_handle_smbus_host_notify(priv->host_notify, addr, data);
+	/*
+	 * With the tested platforms, reading SMBNTFDDAT (22 + (p)->smba)
+	 * always returns 0 and is safe to read.
+	 * We just use 0 given we have no use of the data right now.
+	 */
+	i2c_handle_smbus_host_notify(priv->host_notify, addr, 0);
 
 	/* clear Host Notify bit and return */
 	outb_p(SMBSLVSTS_HST_NTFY_STS, SMBSLVSTS(priv));
-- 
2.7.4

[PATCH v5 6/6] i2c: use an IRQ to report Host Notify events, not alert

[Benjamin Tissoires]

The current SMBus Host Notify implementation relies on .alert() to
relay its notifications. However, the use cases where SMBus Host
Notify is needed currently is to signal data ready on touchpads.

This is closer to an IRQ than a custom API through .alert().
Given that the 2 touchpad manufacturers (Synaptics and Elan) that
use SMBus Host Notify don't put any data in the SMBus payload, the
concept actually matches one to one.

Benefits are multiple:
- simpler code and API: the client will just have an IRQ, and
  nothing needs to be added in the adapter beside internally
  enabling it.
- no more specific workqueue, the threading is handled by IRQ core
  directly (when required)
- no more races when removing the device (the drivers are already
  required to disable irq on remove)
- simpler handling for drivers: use plain regular IRQs
- no more dependency on i2c-smbus for i2c-i801 (and any other adapter)
- the IRQ domain is created automatically when the adapter exports
  the Host Notify capability
- the IRQ are assign only if ACPI, OF and the caller did not assign
  one already
- the domain is automatically destroyed on remove
- fewer lines of code (minus 20, yeah!)

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

---

changes in v5:
- force IRQ_DOMAIN in Kconfig
- remove unnused variable
- assign the SMBus Host Notify irq in i2c_device_probe() if the
  irq was not assigned by the caller, and if both ACPI and OF
  failed to assign one.

new in v4
---
 Documentation/i2c/smbus-protocol |  12 +++--
 drivers/i2c/Kconfig              |   1 +
 drivers/i2c/busses/i2c-i801.c    |  32 +++--------
 drivers/i2c/i2c-core.c           | 113 +++++++++++++++++++++++++++++++++++++++
 drivers/i2c/i2c-smbus.c          | 102 -----------------------------------
 include/linux/i2c-smbus.h        |  27 ----------
 include/linux/i2c.h              |   4 ++
 7 files changed, 133 insertions(+), 158 deletions(-)

diff --git a/Documentation/i2c/smbus-protocol b/Documentation/i2c/smbus-protocol
index 14d4ec1..9771c28 100644
--- a/Documentation/i2c/smbus-protocol
+++ b/Documentation/i2c/smbus-protocol
@@ -200,10 +200,14 @@ alerting device's address.
 [S] [HostAddr] [Wr] A [DevAddr] A [DataLow] A [DataHigh] A [P]
 
 This is implemented in the following way in the Linux kernel:
-* I2C bus drivers which support SMBus Host Notify should call
-  i2c_setup_smbus_host_notify() to setup SMBus Host Notify support.
-* I2C drivers for devices which can trigger SMBus Host Notify should implement
-  the optional alert() callback.
+* I2C bus drivers which support SMBus Host Notify should report
+  I2C_FUNC_SMBUS_HOST_NOTIFY.
+* I2C bus drivers trigger SMBus Host Notify by a call to
+  i2c_handle_smbus_host_notify().
+* I2C drivers for devices which can trigger SMBus Host Notify will have
+  client->irq assigned to a Host Notify IRQ if noone else specified an other.
+
+There is currently no way to retrieve the data parameter from the client.
 
 
 Packet Error Checking (PEC)
diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig
index d223650..de305f8 100644
--- a/drivers/i2c/Kconfig
+++ b/drivers/i2c/Kconfig
@@ -7,6 +7,7 @@ menu "I2C support"
 config I2C
 	tristate "I2C support"
 	select RT_MUTEXES
+	select IRQ_DOMAIN
 	---help---
 	  I2C (pronounce: I-squared-C) is a slow serial bus protocol used in
 	  many micro controller applications and developed by Philips.  SMBus,
diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 42a6a89..ca63adb 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -266,7 +266,6 @@ struct i801_priv {
 	 */
 	bool acpi_reserved;
 	struct mutex acpi_lock;
-	struct smbus_host_notify *host_notify;
 };
 
 #define FEATURE_SMBUS_PEC	BIT(0)
@@ -582,10 +581,10 @@ static irqreturn_t i801_host_notify_isr(struct i801_priv *priv)
 
 	/*
 	 * With the tested platforms, reading SMBNTFDDAT (22 + (p)->smba)
-	 * always returns 0 and is safe to read.
-	 * We just use 0 given we have no use of the data right now.
+	 * always returns 0. Our current implementation doesn't provide
+	 * data, so we just ignore it.
 	 */
-	i2c_handle_smbus_host_notify(priv->host_notify, addr, 0);
+	i2c_handle_smbus_host_notify(&priv->adapter, addr);
 
 	/* clear Host Notify bit and return */
 	outb_p(SMBSLVSTS_HST_NTFY_STS, SMBSLVSTS(priv));
@@ -941,17 +940,12 @@ static u32 i801_func(struct i2c_adapter *adapter)
 		I2C_FUNC_SMBUS_HOST_NOTIFY : 0);
 }
 
-static int i801_enable_host_notify(struct i2c_adapter *adapter)
+static void i801_enable_host_notify(struct i2c_adapter *adapter)
 {
 	struct i801_priv *priv = i2c_get_adapdata(adapter);
 
 	if (!(priv->features & FEATURE_HOST_NOTIFY))
-		return -ENOTSUPP;
-
-	if (!priv->host_notify)
-		priv->host_notify = i2c_setup_smbus_host_notify(adapter);
-	if (!priv->host_notify)
-		return -ENOMEM;
+		return;
 
 	priv->original_slvcmd = inb_p(SMBSLVCMD(priv));
 
@@ -961,8 +955,6 @@ static int i801_enable_host_notify(struct i2c_adapter *adapter)
 
 	/* clear Host Notify bit to allow a new notification */
 	outb_p(SMBSLVSTS_HST_NTFY_STS, SMBSLVSTS(priv));
-
-	return 0;
 }
 
 static void i801_disable_host_notify(struct i801_priv *priv)
@@ -1631,14 +1623,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 		return err;
 	}
 
-	/*
-	 * Enable Host Notify for chips that supports it.
-	 * It is done after i2c_add_adapter() so that we are sure the work queue
-	 * is not used if i2c_add_adapter() fails.
-	 */
-	err = i801_enable_host_notify(&priv->adapter);
-	if (err && err != -ENOTSUPP)
-		dev_warn(&dev->dev, "Unable to enable SMBus Host Notify\n");
+	i801_enable_host_notify(&priv->adapter);
 
 	i801_probe_optional_slaves(priv);
 	/* We ignore errors - multiplexing is optional */
@@ -1689,11 +1674,8 @@ static int i801_resume(struct device *dev)
 {
 	struct pci_dev *pci_dev = to_pci_dev(dev);
 	struct i801_priv *priv = pci_get_drvdata(pci_dev);
-	int err;
 
-	err = i801_enable_host_notify(&priv->adapter);
-	if (err && err != -ENOTSUPP)
-		dev_warn(dev, "Unable to enable SMBus Host Notify\n");
+	i801_enable_host_notify(&priv->adapter);
 
 	return 0;
 }
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 79ce9e1..4f4fc60 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -65,6 +65,9 @@
 #define I2C_ADDR_OFFSET_TEN_BIT	0xa000
 #define I2C_ADDR_OFFSET_SLAVE	0x1000
 
+#define I2C_ADDR_7BITS_MAX	0x77
+#define I2C_ADDR_7BITS_COUNT	(I2C_ADDR_7BITS_MAX + 1)
+
 /* core_lock protects i2c_adapter_idr, and guarantees
    that device detection, deletion of detected devices, and attach_adapter
    calls are serialized */
@@ -880,6 +883,25 @@ static void i2c_init_recovery(struct i2c_adapter *adap)
 	adap->bus_recovery_info = NULL;
 }
 
+static int i2c_smbus_host_notify_to_irq(const struct i2c_client *client)
+{
+	struct i2c_adapter *adap = client->adapter;
+	unsigned int irq;
+
+	if (!adap->host_notify_domain)
+		return -ENXIO;
+
+	if (client->flags & I2C_CLIENT_TEN)
+		return -EINVAL;
+
+	irq = irq_find_mapping(adap->host_notify_domain, client->addr);
+	if (!irq)
+		irq = irq_create_mapping(adap->host_notify_domain,
+					 client->addr);
+
+	return irq > 0 ? irq : -ENXIO;
+}
+
 static int i2c_device_probe(struct device *dev)
 {
 	struct i2c_client	*client = i2c_verify_client(dev);
@@ -901,6 +923,14 @@ static int i2c_device_probe(struct device *dev)
 		}
 		if (irq == -EPROBE_DEFER)
 			return irq;
+		/*
+		 * ACPI and OF did not find any useful IRQ, try to see
+		 * if Host Notify can be used.
+		 */
+		if (irq < 0) {
+			dev_dbg(dev, "Using Host Notify IRQ\n");
+			irq = i2c_smbus_host_notify_to_irq(client);
+		}
 		if (irq < 0)
 			irq = 0;
 
@@ -1781,6 +1811,79 @@ static const struct i2c_lock_operations i2c_adapter_lock_ops = {
 	.unlock_bus =  i2c_adapter_unlock_bus,
 };
 
+static void i2c_host_notify_irq_teardown(struct i2c_adapter *adap)
+{
+	struct irq_domain *domain = adap->host_notify_domain;
+	irq_hw_number_t hwirq;
+
+	if (!domain)
+		return;
+
+	for (hwirq = 0 ; hwirq < I2C_ADDR_7BITS_COUNT ; hwirq++)
+		irq_dispose_mapping(irq_find_mapping(domain, hwirq));
+
+	irq_domain_remove(domain);
+	adap->host_notify_domain = NULL;
+}
+
+static int i2c_host_notify_irq_map(struct irq_domain *h,
+					  unsigned int virq,
+					  irq_hw_number_t hw_irq_num)
+{
+	irq_set_chip_and_handler(virq, &dummy_irq_chip, handle_simple_irq);
+
+	return 0;
+}
+
+static struct irq_domain_ops i2c_host_notify_irq_ops = {
+	.map = i2c_host_notify_irq_map,
+};
+
+static int i2c_setup_host_notify_irq_domain(struct i2c_adapter *adap)
+{
+	struct irq_domain *domain;
+
+	if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_HOST_NOTIFY))
+		return 0;
+
+	domain = irq_domain_create_linear(adap->dev.fwnode,
+					  I2C_ADDR_7BITS_COUNT,
+					  &i2c_host_notify_irq_ops, adap);
+	if (!domain)
+		return -ENOMEM;
+
+	adap->host_notify_domain = domain;
+
+	return 0;
+}
+
+/**
+ * i2c_handle_smbus_host_notify - Forward a Host Notify event to the correct
+ * I2C client.
+ * @adap: the adapter
+ * @addr: the I2C address of the notifying device
+ * Context: can't sleep
+ *
+ * Helper function to be called from an I2C bus driver's interrupt
+ * handler. It will schedule the Host Notify IRQ.
+ */
+int i2c_handle_smbus_host_notify(struct i2c_adapter *adap, unsigned short addr)
+{
+	int irq;
+
+	if (!adap)
+		return -EINVAL;
+
+	irq = irq_find_mapping(adap->host_notify_domain, addr);
+	if (irq <= 0)
+		return -ENXIO;
+
+	generic_handle_irq(irq);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(i2c_handle_smbus_host_notify);
+
 static int i2c_register_adapter(struct i2c_adapter *adap)
 {
 	int res = -EINVAL;
@@ -1812,6 +1915,14 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
 	if (adap->timeout == 0)
 		adap->timeout = HZ;
 
+	/* register soft irqs for Host Notify */
+	res = i2c_setup_host_notify_irq_domain(adap);
+	if (res) {
+		pr_err("adapter '%s': can't create Host Notify IRQs (%d)\n",
+		       adap->name, res);
+		goto out_list;
+	}
+
 	dev_set_name(&adap->dev, "i2c-%d", adap->nr);
 	adap->dev.bus = &i2c_bus_type;
 	adap->dev.type = &i2c_adapter_type;
@@ -2049,6 +2160,8 @@ void i2c_del_adapter(struct i2c_adapter *adap)
 
 	pm_runtime_disable(&adap->dev);
 
+	i2c_host_notify_irq_teardown(adap);
+
 	/* wait until all references to the device are gone
 	 *
 	 * FIXME: This is old code and should ideally be replaced by an
diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c
index b0d2679..f9271c7 100644
--- a/drivers/i2c/i2c-smbus.c
+++ b/drivers/i2c/i2c-smbus.c
@@ -241,108 +241,6 @@ int i2c_handle_smbus_alert(struct i2c_client *ara)
 }
 EXPORT_SYMBOL_GPL(i2c_handle_smbus_alert);
 
-static void smbus_host_notify_work(struct work_struct *work)
-{
-	struct alert_data alert;
-	struct i2c_adapter *adapter;
-	unsigned long flags;
-	u16 payload;
-	u8 addr;
-	struct smbus_host_notify *data;
-
-	data = container_of(work, struct smbus_host_notify, work);
-
-	spin_lock_irqsave(&data->lock, flags);
-	payload = data->payload;
-	addr = data->addr;
-	adapter = data->adapter;
-
-	/* clear the pending bit and release the spinlock */
-	data->pending = false;
-	spin_unlock_irqrestore(&data->lock, flags);
-
-	if (!adapter || !addr)
-		return;
-
-	alert.type = I2C_PROTOCOL_SMBUS_HOST_NOTIFY;
-	alert.addr = addr;
-	alert.data = payload;
-
-	device_for_each_child(&adapter->dev, &alert, smbus_do_alert);
-}
-
-/**
- * i2c_setup_smbus_host_notify - Allocate a new smbus_host_notify for the given
- * I2C adapter.
- * @adapter: the adapter we want to associate a Host Notify function
- *
- * Returns a struct smbus_host_notify pointer on success, and NULL on failure.
- * The resulting smbus_host_notify must not be freed afterwards, it is a
- * managed resource already.
- */
-struct smbus_host_notify *i2c_setup_smbus_host_notify(struct i2c_adapter *adap)
-{
-	struct smbus_host_notify *host_notify;
-
-	host_notify = devm_kzalloc(&adap->dev, sizeof(struct smbus_host_notify),
-				   GFP_KERNEL);
-	if (!host_notify)
-		return NULL;
-
-	host_notify->adapter = adap;
-
-	spin_lock_init(&host_notify->lock);
-	INIT_WORK(&host_notify->work, smbus_host_notify_work);
-
-	return host_notify;
-}
-EXPORT_SYMBOL_GPL(i2c_setup_smbus_host_notify);
-
-/**
- * i2c_handle_smbus_host_notify - Forward a Host Notify event to the correct
- * I2C client.
- * @host_notify: the struct host_notify attached to the relevant adapter
- * @addr: the I2C address of the notifying device
- * @data: the payload of the notification
- * Context: can't sleep
- *
- * Helper function to be called from an I2C bus driver's interrupt
- * handler. It will schedule the Host Notify work, in turn calling the
- * corresponding I2C device driver's alert function.
- *
- * host_notify should be a valid pointer previously returned by
- * i2c_setup_smbus_host_notify().
- */
-int i2c_handle_smbus_host_notify(struct smbus_host_notify *host_notify,
-				 unsigned short addr, unsigned int data)
-{
-	unsigned long flags;
-	struct i2c_adapter *adapter;
-
-	if (!host_notify || !host_notify->adapter)
-		return -EINVAL;
-
-	adapter = host_notify->adapter;
-
-	spin_lock_irqsave(&host_notify->lock, flags);
-
-	if (host_notify->pending) {
-		spin_unlock_irqrestore(&host_notify->lock, flags);
-		dev_warn(&adapter->dev, "Host Notify already scheduled.\n");
-		return -EBUSY;
-	}
-
-	host_notify->payload = data;
-	host_notify->addr = addr;
-
-	/* Mark that there is a pending notification and release the lock */
-	host_notify->pending = true;
-	spin_unlock_irqrestore(&host_notify->lock, flags);
-
-	return schedule_work(&host_notify->work);
-}
-EXPORT_SYMBOL_GPL(i2c_handle_smbus_host_notify);
-
 module_i2c_driver(smbalert_driver);
 
 MODULE_AUTHOR("Jean Delvare <jdelvare@suse.de>");
diff --git a/include/linux/i2c-smbus.h b/include/linux/i2c-smbus.h
index c2e3324..a138502 100644
--- a/include/linux/i2c-smbus.h
+++ b/include/linux/i2c-smbus.h
@@ -50,31 +50,4 @@ struct i2c_client *i2c_setup_smbus_alert(struct i2c_adapter *adapter,
 					 struct i2c_smbus_alert_setup *setup);
 int i2c_handle_smbus_alert(struct i2c_client *ara);
 
-/**
- * smbus_host_notify - internal structure used by the Host Notify mechanism.
- * @adapter: the I2C adapter associated with this struct
- * @work: worker used to schedule the IRQ in the slave device
- * @lock: spinlock to check if a notification is already pending
- * @pending: flag set when a notification is pending (any new notification will
- *		be rejected if pending is true)
- * @payload: the actual payload of the Host Notify event
- * @addr: the address of the slave device which raised the notification
- *
- * This struct needs to be allocated by i2c_setup_smbus_host_notify() and does
- * not need to be freed. Internally, i2c_setup_smbus_host_notify() uses a
- * managed resource to clean this up when the adapter get released.
- */
-struct smbus_host_notify {
-	struct i2c_adapter	*adapter;
-	struct work_struct	work;
-	spinlock_t		lock;
-	bool			pending;
-	u16			payload;
-	u8			addr;
-};
-
-struct smbus_host_notify *i2c_setup_smbus_host_notify(struct i2c_adapter *adap);
-int i2c_handle_smbus_host_notify(struct smbus_host_notify *host_notify,
-				 unsigned short addr, unsigned int data);
-
 #endif /* _LINUX_I2C_SMBUS_H */
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 4a4099d..65c4d5e 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -30,6 +30,7 @@
 #include <linux/device.h>	/* for struct device */
 #include <linux/sched.h>	/* for completion */
 #include <linux/mutex.h>
+#include <linux/irqdomain.h>		/* for Host Notify IRQ */
 #include <linux/of.h>		/* for struct device_node */
 #include <linux/swab.h>		/* for swab16 */
 #include <uapi/linux/i2c.h>
@@ -567,6 +568,8 @@ struct i2c_adapter {
 
 	struct i2c_bus_recovery_info *bus_recovery_info;
 	const struct i2c_adapter_quirks *quirks;
+
+	struct irq_domain *host_notify_domain;
 };
 #define to_i2c_adapter(d) container_of(d, struct i2c_adapter, dev)
 
@@ -738,6 +741,7 @@ static inline u8 i2c_8bit_addr_from_msg(const struct i2c_msg *msg)
 	return (msg->addr << 1) | (msg->flags & I2C_M_RD ? 1 : 0);
 }
 
+int i2c_handle_smbus_host_notify(struct i2c_adapter *adap, unsigned short addr);
 /**
  * module_i2c_driver() - Helper macro for registering a modular I2C driver
  * @__i2c_driver: i2c_driver struct
-- 
2.7.4

Re: [PATCH v5 6/6] i2c: use an IRQ to report Host Notify events, not alert

[Wolfram Sang]

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

On Thu, Oct 13, 2016 at 02:10:40PM +0200, Benjamin Tissoires wrote:
> The current SMBus Host Notify implementation relies on .alert() to
> relay its notifications. However, the use cases where SMBus Host
> Notify is needed currently is to signal data ready on touchpads.
> 
> This is closer to an IRQ than a custom API through .alert().
> Given that the 2 touchpad manufacturers (Synaptics and Elan) that
> use SMBus Host Notify don't put any data in the SMBus payload, the
> concept actually matches one to one.

I see the advantages. The only question I have: What if we encounter
devices in the future which do put data in the payload? Can this
mechanism be extended to handle that?

> 
> Benefits are multiple:
> - simpler code and API: the client will just have an IRQ, and
>   nothing needs to be added in the adapter beside internally
>   enabling it.
> - no more specific workqueue, the threading is handled by IRQ core
>   directly (when required)
> - no more races when removing the device (the drivers are already
>   required to disable irq on remove)
> - simpler handling for drivers: use plain regular IRQs
> - no more dependency on i2c-smbus for i2c-i801 (and any other adapter)
> - the IRQ domain is created automatically when the adapter exports
>   the Host Notify capability
> - the IRQ are assign only if ACPI, OF and the caller did not assign
>   one already
> - the domain is automatically destroyed on remove
> - fewer lines of code (minus 20, yeah!)
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

Thanks for keeping at it!


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

Re: [PATCH v5 6/6] i2c: use an IRQ to report Host Notify events, not alert

[Benjamin Tissoires]

On Nov 07 2016 or thereabouts, Wolfram Sang wrote:
> On Thu, Oct 13, 2016 at 02:10:40PM +0200, Benjamin Tissoires wrote:
> > The current SMBus Host Notify implementation relies on .alert() to
> > relay its notifications. However, the use cases where SMBus Host
> > Notify is needed currently is to signal data ready on touchpads.
> > 
> > This is closer to an IRQ than a custom API through .alert().
> > Given that the 2 touchpad manufacturers (Synaptics and Elan) that
> > use SMBus Host Notify don't put any data in the SMBus payload, the
> > concept actually matches one to one.
> 
> I see the advantages. The only question I have: What if we encounter
> devices in the future which do put data in the payload? Can this
> mechanism be extended to handle that?

I had this very same problem when dealing with hid-rmi. In that case, we
have an interrupt that contains data.

I somewhat solved this by the following commit transposed in the hid-rmi
world:
https://github.com/bentiss/linux/commit/d24d938e1eabba026c3c5e4daeee3a16403295de

The idea is the following:
- we extend the internal call of i2c_handle_smbus_host_notify() by
  re-adding a data payload.
- this call takes a spinlock, and copy the data into an internal
  placeholder, releases the spinlock and then triggers the irq
- when the client gets an interrupt, it calls
  i2c_retrieve_host_notify_data() which is protected by the spinlock and
  returns the *current* data

The only difficulty for the client now is that it might access newer
data than the origin interrupt. This can be solved by 2 solutions. Either
we use a kfifo in i2c_core, to the risk of having some complex processing.
Either we just let the client dealing with that by implementing itself
the kfifo. The data just needs to be retrieved in the non threaded part
of the interrupt handler, and stored by the client itself.

So all in all, I think adding the data will be just adding a spinlock, a
placeholder and a new internal API to retrieve it.

I am not sure we have other means of extending the IRQ processing, but I
am open to suggestions.

Cheers,
Benjamin

> 
> > 
> > Benefits are multiple:
> > - simpler code and API: the client will just have an IRQ, and
> >   nothing needs to be added in the adapter beside internally
> >   enabling it.
> > - no more specific workqueue, the threading is handled by IRQ core
> >   directly (when required)
> > - no more races when removing the device (the drivers are already
> >   required to disable irq on remove)
> > - simpler handling for drivers: use plain regular IRQs
> > - no more dependency on i2c-smbus for i2c-i801 (and any other adapter)
> > - the IRQ domain is created automatically when the adapter exports
> >   the Host Notify capability
> > - the IRQ are assign only if ACPI, OF and the caller did not assign
> >   one already
> > - the domain is automatically destroyed on remove
> > - fewer lines of code (minus 20, yeah!)
> > 
> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> 
> Thanks for keeping at it!
> 

Re: [PATCH v5 6/6] i2c: use an IRQ to report Host Notify events, not alert

[Benjamin Tissoires]

Hi Wolfram,

On Nov 07 2016 or thereabouts, Wolfram Sang wrote:
> On Thu, Oct 13, 2016 at 02:10:40PM +0200, Benjamin Tissoires wrote:
> > The current SMBus Host Notify implementation relies on .alert() to
> > relay its notifications. However, the use cases where SMBus Host
> > Notify is needed currently is to signal data ready on touchpads.
> > 
> > This is closer to an IRQ than a custom API through .alert().
> > Given that the 2 touchpad manufacturers (Synaptics and Elan) that
> > use SMBus Host Notify don't put any data in the SMBus payload, the
> > concept actually matches one to one.
> 
> I see the advantages. The only question I have: What if we encounter
> devices in the future which do put data in the payload? Can this
> mechanism be extended to handle that?

I guess I haven't convinced you with my answer. Is there anything I can
do to get this series in v4.10 or do you prefer waiting for v4.11?

Cheers,
Benjamin

> 
> > 
> > Benefits are multiple:
> > - simpler code and API: the client will just have an IRQ, and
> >   nothing needs to be added in the adapter beside internally
> >   enabling it.
> > - no more specific workqueue, the threading is handled by IRQ core
> >   directly (when required)
> > - no more races when removing the device (the drivers are already
> >   required to disable irq on remove)
> > - simpler handling for drivers: use plain regular IRQs
> > - no more dependency on i2c-smbus for i2c-i801 (and any other adapter)
> > - the IRQ domain is created automatically when the adapter exports
> >   the Host Notify capability
> > - the IRQ are assign only if ACPI, OF and the caller did not assign
> >   one already
> > - the domain is automatically destroyed on remove
> > - fewer lines of code (minus 20, yeah!)
> > 
> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> 
> Thanks for keeping at it!
> 

Re: [PATCH v5 6/6] i2c: use an IRQ to report Host Notify events, not alert

[Wolfram Sang]

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

On Mon, Nov 21, 2016 at 11:52:48AM +0100, Benjamin Tissoires wrote:
> Hi Wolfram,
> 
> On Nov 07 2016 or thereabouts, Wolfram Sang wrote:
> > On Thu, Oct 13, 2016 at 02:10:40PM +0200, Benjamin Tissoires wrote:
> > > The current SMBus Host Notify implementation relies on .alert() to
> > > relay its notifications. However, the use cases where SMBus Host
> > > Notify is needed currently is to signal data ready on touchpads.
> > > 
> > > This is closer to an IRQ than a custom API through .alert().
> > > Given that the 2 touchpad manufacturers (Synaptics and Elan) that
> > > use SMBus Host Notify don't put any data in the SMBus payload, the
> > > concept actually matches one to one.
> > 
> > I see the advantages. The only question I have: What if we encounter
> > devices in the future which do put data in the payload? Can this
> > mechanism be extended to handle that?
> 
> I guess I haven't convinced you with my answer. Is there anything I can
> do to get this series in v4.10 or do you prefer waiting for v4.11?

I consider this v4.10 material. I was thinking a little about how to not
lose data with consecutive interrupts but then -EBUSY came along.
Nonetheless, it looks to me like the proper path to follow...

Thanks,

   Wolfram


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

Re: [PATCH v5 6/6] i2c: use an IRQ to report Host Notify events, not alert

[Wolfram Sang]

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

On Tue, Nov 22, 2016 at 12:49:22PM +0100, Wolfram Sang wrote:
> On Mon, Nov 21, 2016 at 11:52:48AM +0100, Benjamin Tissoires wrote:
> > Hi Wolfram,
> > 
> > On Nov 07 2016 or thereabouts, Wolfram Sang wrote:
> > > On Thu, Oct 13, 2016 at 02:10:40PM +0200, Benjamin Tissoires wrote:
> > > > The current SMBus Host Notify implementation relies on .alert() to
> > > > relay its notifications. However, the use cases where SMBus Host
> > > > Notify is needed currently is to signal data ready on touchpads.
> > > > 
> > > > This is closer to an IRQ than a custom API through .alert().
> > > > Given that the 2 touchpad manufacturers (Synaptics and Elan) that
> > > > use SMBus Host Notify don't put any data in the SMBus payload, the
> > > > concept actually matches one to one.
> > > 
> > > I see the advantages. The only question I have: What if we encounter
> > > devices in the future which do put data in the payload? Can this
> > > mechanism be extended to handle that?
> > 
> > I guess I haven't convinced you with my answer. Is there anything I can
> > do to get this series in v4.10 or do you prefer waiting for v4.11?
> 
> I consider this v4.10 material. I was thinking a little about how to not
> lose data with consecutive interrupts but then -EBUSY came along.
> Nonetheless, it looks to me like the proper path to follow...

Applied to for-next, thanks!

Fixed the following checkpatch warning for you:

WARNING: struct irq_domain_ops should normally be const
#250: FILE: drivers/i2c/i2c-core.c:1838:
+static struct irq_domain_ops i2c_host_notify_irq_ops = {


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

Re: [PATCH v5 6/6] i2c: use an IRQ to report Host Notify events, not alert

[Benjamin Tissoires]

On Nov 24 2016 or thereabouts, Wolfram Sang wrote:
> On Tue, Nov 22, 2016 at 12:49:22PM +0100, Wolfram Sang wrote:
> > On Mon, Nov 21, 2016 at 11:52:48AM +0100, Benjamin Tissoires wrote:
> > > Hi Wolfram,
> > > 
> > > On Nov 07 2016 or thereabouts, Wolfram Sang wrote:
> > > > On Thu, Oct 13, 2016 at 02:10:40PM +0200, Benjamin Tissoires wrote:
> > > > > The current SMBus Host Notify implementation relies on .alert() to
> > > > > relay its notifications. However, the use cases where SMBus Host
> > > > > Notify is needed currently is to signal data ready on touchpads.
> > > > > 
> > > > > This is closer to an IRQ than a custom API through .alert().
> > > > > Given that the 2 touchpad manufacturers (Synaptics and Elan) that
> > > > > use SMBus Host Notify don't put any data in the SMBus payload, the
> > > > > concept actually matches one to one.
> > > > 
> > > > I see the advantages. The only question I have: What if we encounter
> > > > devices in the future which do put data in the payload? Can this
> > > > mechanism be extended to handle that?
> > > 
> > > I guess I haven't convinced you with my answer. Is there anything I can
> > > do to get this series in v4.10 or do you prefer waiting for v4.11?
> > 
> > I consider this v4.10 material. I was thinking a little about how to not
> > lose data with consecutive interrupts but then -EBUSY came along.
> > Nonetheless, it looks to me like the proper path to follow...
> 
> Applied to for-next, thanks!

Thanks!

> 
> Fixed the following checkpatch warning for you:
> 
> WARNING: struct irq_domain_ops should normally be const
> #250: FILE: drivers/i2c/i2c-core.c:1838:
> +static struct irq_domain_ops i2c_host_notify_irq_ops = {
> 

Thanks again :)

Cheers,
Benjamin