* [PATCH v4 1/8] i2c: i801: store and restore the SLVCMD register at load and unload
2016-10-10 16:42 [PATCH v4 0/8] i2c: Host Notify / i801 fixes Benjamin Tissoires
@ 2016-10-10 16:42 ` Benjamin Tissoires
2016-10-10 16:42 ` [PATCH v4 2/8] i2c: i801: minor formatting issues Benjamin Tissoires
` (6 subsequent siblings)
7 siblings, 0 replies; 12+ messages in thread
From: Benjamin Tissoires @ 2016-10-10 16:42 UTC (permalink / raw)
To: Wolfram Sang, Dmitry Torokhov
Cc: Jean Delvare, Jonathan Corbet, KT Liao, linux-i2c, linux-input,
linux-doc, linux-kernel
Also do not override any other configuration in this register.
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
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
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH v4 2/8] i2c: i801: minor formatting issues
2016-10-10 16:42 [PATCH v4 0/8] i2c: Host Notify / i801 fixes Benjamin Tissoires
2016-10-10 16:42 ` [PATCH v4 1/8] i2c: i801: store and restore the SLVCMD register at load and unload Benjamin Tissoires
@ 2016-10-10 16:42 ` Benjamin Tissoires
2016-10-10 16:42 ` [PATCH v4 3/8] i2c: i801: use BIT() macro for bits definition Benjamin Tissoires
` (5 subsequent siblings)
7 siblings, 0 replies; 12+ messages in thread
From: Benjamin Tissoires @ 2016-10-10 16:42 UTC (permalink / raw)
To: Wolfram Sang, Dmitry Torokhov
Cc: Jean Delvare, Jonathan Corbet, KT Liao, linux-i2c, linux-input,
linux-doc, linux-kernel
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 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
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH v4 3/8] i2c: i801: use BIT() macro for bits definition
2016-10-10 16:42 [PATCH v4 0/8] i2c: Host Notify / i801 fixes Benjamin Tissoires
2016-10-10 16:42 ` [PATCH v4 1/8] i2c: i801: store and restore the SLVCMD register at load and unload Benjamin Tissoires
2016-10-10 16:42 ` [PATCH v4 2/8] i2c: i801: minor formatting issues Benjamin Tissoires
@ 2016-10-10 16:42 ` Benjamin Tissoires
2016-10-10 16:42 ` [PATCH v4 4/8] i2c: i801: use the BIT() macro for FEATURES_* also Benjamin Tissoires
` (4 subsequent siblings)
7 siblings, 0 replies; 12+ messages in thread
From: Benjamin Tissoires @ 2016-10-10 16:42 UTC (permalink / raw)
To: Wolfram Sang, Dmitry Torokhov
Cc: Jean Delvare, Jonathan Corbet, KT Liao, linux-i2c, linux-input,
linux-doc, linux-kernel
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 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
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH v4 4/8] i2c: i801: use the BIT() macro for FEATURES_* also
2016-10-10 16:42 [PATCH v4 0/8] i2c: Host Notify / i801 fixes Benjamin Tissoires
` (2 preceding siblings ...)
2016-10-10 16:42 ` [PATCH v4 3/8] i2c: i801: use BIT() macro for bits definition Benjamin Tissoires
@ 2016-10-10 16:42 ` Benjamin Tissoires
2016-10-10 16:42 ` [PATCH v4 5/8] i2c: i801: remove SMBNTFDDAT reads as they always seem to return 0 Benjamin Tissoires
` (3 subsequent siblings)
7 siblings, 0 replies; 12+ messages in thread
From: Benjamin Tissoires @ 2016-10-10 16:42 UTC (permalink / raw)
To: Wolfram Sang, Dmitry Torokhov
Cc: Jean Delvare, Jonathan Corbet, KT Liao, linux-i2c, linux-input,
linux-doc, linux-kernel
no functional changes
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
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
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH v4 5/8] i2c: i801: remove SMBNTFDDAT reads as they always seem to return 0
2016-10-10 16:42 [PATCH v4 0/8] i2c: Host Notify / i801 fixes Benjamin Tissoires
` (3 preceding siblings ...)
2016-10-10 16:42 ` [PATCH v4 4/8] i2c: i801: use the BIT() macro for FEATURES_* also Benjamin Tissoires
@ 2016-10-10 16:42 ` Benjamin Tissoires
2016-10-10 16:42 ` [PATCH v4 6/8] i2c: use an IRQ to report Host Notify events, not alert Benjamin Tissoires
` (2 subsequent siblings)
7 siblings, 0 replies; 12+ messages in thread
From: Benjamin Tissoires @ 2016-10-10 16:42 UTC (permalink / raw)
To: Wolfram Sang, Dmitry Torokhov
Cc: Jean Delvare, Jonathan Corbet, KT Liao, linux-i2c, linux-input,
linux-doc, linux-kernel
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 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
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH v4 6/8] i2c: use an IRQ to report Host Notify events, not alert
2016-10-10 16:42 [PATCH v4 0/8] i2c: Host Notify / i801 fixes Benjamin Tissoires
` (4 preceding siblings ...)
2016-10-10 16:42 ` [PATCH v4 5/8] i2c: i801: remove SMBNTFDDAT reads as they always seem to return 0 Benjamin Tissoires
@ 2016-10-10 16:42 ` Benjamin Tissoires
2016-10-10 16:42 ` [PATCH v4 7/8] Input: elan_i2c - store the irq in struct elan_tp_data Benjamin Tissoires
2016-10-10 16:42 ` [PATCH v4 8/8] Input: elan_i2c - add Host Notify support Benjamin Tissoires
7 siblings, 0 replies; 12+ messages in thread
From: Benjamin Tissoires @ 2016-10-10 16:42 UTC (permalink / raw)
To: Wolfram Sang, Dmitry Torokhov
Cc: Jean Delvare, Jonathan Corbet, KT Liao, linux-i2c, linux-input,
linux-doc, linux-kernel
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 just needs to get the IRQ through
i2c_smbus_host_notify_to_irq(), 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, but the IRQ are allocated on demand
- the domain is automatically destroyed on remove
- fewer lines of code (minus 20, yeah!)
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
new in v4
---
Documentation/i2c/smbus-protocol | 10 ++--
drivers/i2c/busses/i2c-i801.c | 31 +++--------
drivers/i2c/i2c-core.c | 117 +++++++++++++++++++++++++++++++++++++++
drivers/i2c/i2c-smbus.c | 102 ----------------------------------
include/linux/i2c-smbus.h | 27 ---------
include/linux/i2c.h | 5 ++
6 files changed, 135 insertions(+), 157 deletions(-)
diff --git a/Documentation/i2c/smbus-protocol b/Documentation/i2c/smbus-protocol
index 14d4ec1..8a29ba3 100644
--- a/Documentation/i2c/smbus-protocol
+++ b/Documentation/i2c/smbus-protocol
@@ -200,10 +200,12 @@ 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 drivers for devices which can trigger SMBus Host Notify should request
+ a Host Notify IRQ through i2c_smbus_host_notify_to_irq().
+
+There is currently no way to retrieve the data parameter from the client.
Packet Error Checking (PEC)
diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 42a6a89..a016b48 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 */
@@ -1691,9 +1676,7 @@ static int i801_resume(struct device *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..9d5bcf9 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 */
@@ -1781,6 +1784,110 @@ 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);
+
+/**
+ * i2c_smbus_host_notify_to_irq - Gets a Host Notify IRQ number associated to
+ * the I2C client.
+ * @client: the I2C client
+ *
+ * Will return a valid IRQ number or an error. The Host Notify interrupts
+ * only work currently with 7 bits addresses.
+ */
+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) {
+ dev_err(&client->dev,
+ "Host Notify not supported on 10 bits addresses.\n");
+ 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;
+}
+EXPORT_SYMBOL_GPL(i2c_smbus_host_notify_to_irq);
+
static int i2c_register_adapter(struct i2c_adapter *adap)
{
int res = -EINVAL;
@@ -1812,6 +1919,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 +2164,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..5fbef64 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,8 @@ 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_smbus_host_notify_to_irq(const struct i2c_client *client);
+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
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH v4 7/8] Input: elan_i2c - store the irq in struct elan_tp_data
2016-10-10 16:42 [PATCH v4 0/8] i2c: Host Notify / i801 fixes Benjamin Tissoires
` (5 preceding siblings ...)
2016-10-10 16:42 ` [PATCH v4 6/8] i2c: use an IRQ to report Host Notify events, not alert Benjamin Tissoires
@ 2016-10-10 16:42 ` Benjamin Tissoires
2016-10-10 16:42 ` [PATCH v4 8/8] Input: elan_i2c - add Host Notify support Benjamin Tissoires
7 siblings, 0 replies; 12+ messages in thread
From: Benjamin Tissoires @ 2016-10-10 16:42 UTC (permalink / raw)
To: Wolfram Sang, Dmitry Torokhov
Cc: Jean Delvare, Jonathan Corbet, KT Liao, linux-i2c, linux-input,
linux-doc, linux-kernel
And make sure we have one available.
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
new in v4
---
drivers/input/mouse/elan_i2c_core.c | 34 ++++++++++++++++++++++------------
1 file changed, 22 insertions(+), 12 deletions(-)
diff --git a/drivers/input/mouse/elan_i2c_core.c b/drivers/input/mouse/elan_i2c_core.c
index d15b338..6f16eb4 100644
--- a/drivers/input/mouse/elan_i2c_core.c
+++ b/drivers/input/mouse/elan_i2c_core.c
@@ -62,6 +62,7 @@ struct elan_tp_data {
struct i2c_client *client;
struct input_dev *input;
struct regulator *vcc;
+ int irq;
const struct elan_transport_ops *ops;
@@ -457,7 +458,7 @@ static int elan_update_firmware(struct elan_tp_data *data,
dev_dbg(&client->dev, "Starting firmware update....\n");
- disable_irq(client->irq);
+ disable_irq(data->irq);
data->in_fw_update = true;
retval = __elan_update_firmware(data, fw);
@@ -471,7 +472,7 @@ static int elan_update_firmware(struct elan_tp_data *data,
}
data->in_fw_update = false;
- enable_irq(client->irq);
+ enable_irq(data->irq);
return retval;
}
@@ -599,7 +600,7 @@ static ssize_t calibrate_store(struct device *dev,
if (retval)
return retval;
- disable_irq(client->irq);
+ disable_irq(data->irq);
data->mode |= ETP_ENABLE_CALIBRATE;
retval = data->ops->set_mode(client, data->mode);
@@ -645,7 +646,7 @@ out_disable_calibrate:
retval = error;
}
out:
- enable_irq(client->irq);
+ enable_irq(data->irq);
mutex_unlock(&data->sysfs_mutex);
return retval ?: count;
}
@@ -711,7 +712,7 @@ static ssize_t acquire_store(struct device *dev, struct device_attribute *attr,
if (retval)
return retval;
- disable_irq(client->irq);
+ disable_irq(data->irq);
data->baseline_ready = false;
@@ -753,7 +754,7 @@ out_disable_calibrate:
retval = error;
}
out:
- enable_irq(client->irq);
+ enable_irq(data->irq);
mutex_unlock(&data->sysfs_mutex);
return retval ?: count;
}
@@ -1026,6 +1027,9 @@ static int elan_probe(struct i2c_client *client,
struct elan_tp_data *data;
unsigned long irqflags;
int error;
+ int irq;
+
+ irq = client->irq;
if (IS_ENABLED(CONFIG_MOUSE_ELAN_I2C_I2C) &&
i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
@@ -1041,6 +1045,11 @@ static int elan_probe(struct i2c_client *client,
return -EIO;
}
+ if (irq <= 0) {
+ dev_err(dev, "no IRQ provided.\n");
+ return -ENODEV;
+ }
+
data = devm_kzalloc(&client->dev, sizeof(struct elan_tp_data),
GFP_KERNEL);
if (!data)
@@ -1049,6 +1058,7 @@ static int elan_probe(struct i2c_client *client,
i2c_set_clientdata(client, data);
data->ops = transport_ops;
+ data->irq = irq;
data->client = client;
init_completion(&data->fw_completion);
mutex_init(&data->sysfs_mutex);
@@ -1121,12 +1131,12 @@ static int elan_probe(struct i2c_client *client,
*/
irqflags = client->dev.of_node ? 0 : IRQF_TRIGGER_FALLING;
- error = devm_request_threaded_irq(&client->dev, client->irq,
+ error = devm_request_threaded_irq(&client->dev, data->irq,
NULL, elan_isr,
irqflags | IRQF_ONESHOT,
client->name, data);
if (error) {
- dev_err(&client->dev, "cannot register irq=%d\n", client->irq);
+ dev_err(&client->dev, "cannot register irq=%d\n", data->irq);
return error;
}
@@ -1179,12 +1189,12 @@ static int __maybe_unused elan_suspend(struct device *dev)
if (ret)
return ret;
- disable_irq(client->irq);
+ disable_irq(data->irq);
if (device_may_wakeup(dev)) {
ret = elan_sleep(data);
/* Enable wake from IRQ */
- data->irq_wake = (enable_irq_wake(client->irq) == 0);
+ data->irq_wake = (enable_irq_wake(data->irq) == 0);
} else {
ret = elan_disable_power(data);
}
@@ -1200,7 +1210,7 @@ static int __maybe_unused elan_resume(struct device *dev)
int error;
if (device_may_wakeup(dev) && data->irq_wake) {
- disable_irq_wake(client->irq);
+ disable_irq_wake(data->irq);
data->irq_wake = false;
}
@@ -1215,7 +1225,7 @@ static int __maybe_unused elan_resume(struct device *dev)
dev_err(dev, "initialize when resuming failed: %d\n", error);
err:
- enable_irq(data->client->irq);
+ enable_irq(data->irq);
return error;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH v4 8/8] Input: elan_i2c - add Host Notify support
2016-10-10 16:42 [PATCH v4 0/8] i2c: Host Notify / i801 fixes Benjamin Tissoires
` (6 preceding siblings ...)
2016-10-10 16:42 ` [PATCH v4 7/8] Input: elan_i2c - store the irq in struct elan_tp_data Benjamin Tissoires
@ 2016-10-10 16:42 ` Benjamin Tissoires
2016-10-10 21:39 ` Dmitry Torokhov
7 siblings, 1 reply; 12+ messages in thread
From: Benjamin Tissoires @ 2016-10-10 16:42 UTC (permalink / raw)
To: Wolfram Sang, Dmitry Torokhov
Cc: Jean Delvare, Jonathan Corbet, KT Liao, linux-i2c, linux-input,
linux-doc, linux-kernel
The Thinkpad series 13 uses Host Notify to report the interrupt.
Add elan_smb_alert() to handle those interrupts and disable the irq
handling on this case.
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
new in v4 (was submitted on linux-input with the .alert callback)
---
drivers/input/mouse/elan_i2c_core.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/drivers/input/mouse/elan_i2c_core.c b/drivers/input/mouse/elan_i2c_core.c
index 6f16eb4..4aaac5d 100644
--- a/drivers/input/mouse/elan_i2c_core.c
+++ b/drivers/input/mouse/elan_i2c_core.c
@@ -1040,6 +1040,21 @@ static int elan_probe(struct i2c_client *client,
I2C_FUNC_SMBUS_BLOCK_DATA |
I2C_FUNC_SMBUS_I2C_BLOCK)) {
transport_ops = &elan_smbus_ops;
+
+ if (!irq) {
+ if (!i2c_check_functionality(client->adapter,
+ I2C_FUNC_SMBUS_HOST_NOTIFY)) {
+ dev_err(dev, "no Host Notify support\n");
+ return -ENODEV;
+ }
+
+ irq = i2c_smbus_host_notify_to_irq(client);
+ if (irq < 0) {
+ dev_err(dev,
+ "Unable to request a Host Notify IRQ.\n");
+ return irq;
+ }
+ }
} else {
dev_err(dev, "not a supported I2C/SMBus adapter\n");
return -EIO;
--
2.7.4
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v4 8/8] Input: elan_i2c - add Host Notify support
2016-10-10 16:42 ` [PATCH v4 8/8] Input: elan_i2c - add Host Notify support Benjamin Tissoires
@ 2016-10-10 21:39 ` Dmitry Torokhov
2016-10-11 14:20 ` Benjamin Tissoires
0 siblings, 1 reply; 12+ messages in thread
From: Dmitry Torokhov @ 2016-10-10 21:39 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Wolfram Sang, Jean Delvare, Jonathan Corbet, KT Liao, Linux I2C,
linux-input@vger.kernel.org, linux-doc@vger.kernel.org, lkml
Hi Benjamin,
On Mon, Oct 10, 2016 at 9:42 AM, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
> The Thinkpad series 13 uses Host Notify to report the interrupt.
> Add elan_smb_alert() to handle those interrupts and disable the irq
> handling on this case.
>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>
Why do we have to do this in the driver instead of having I2C core set
it up for us? I expect we'd be repeating this block of code for every
driver that has an option of using SMbus notify.
Thanks!
> ---
>
> new in v4 (was submitted on linux-input with the .alert callback)
> ---
> drivers/input/mouse/elan_i2c_core.c | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
> diff --git a/drivers/input/mouse/elan_i2c_core.c b/drivers/input/mouse/elan_i2c_core.c
> index 6f16eb4..4aaac5d 100644
> --- a/drivers/input/mouse/elan_i2c_core.c
> +++ b/drivers/input/mouse/elan_i2c_core.c
> @@ -1040,6 +1040,21 @@ static int elan_probe(struct i2c_client *client,
> I2C_FUNC_SMBUS_BLOCK_DATA |
> I2C_FUNC_SMBUS_I2C_BLOCK)) {
> transport_ops = &elan_smbus_ops;
> +
> + if (!irq) {
> + if (!i2c_check_functionality(client->adapter,
> + I2C_FUNC_SMBUS_HOST_NOTIFY)) {
> + dev_err(dev, "no Host Notify support\n");
> + return -ENODEV;
> + }
> +
> + irq = i2c_smbus_host_notify_to_irq(client);
> + if (irq < 0) {
> + dev_err(dev,
> + "Unable to request a Host Notify IRQ.\n");
> + return irq;
> + }
> + }
> } else {
> dev_err(dev, "not a supported I2C/SMBus adapter\n");
> return -EIO;
> --
> 2.7.4
>
--
Dmitry
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v4 8/8] Input: elan_i2c - add Host Notify support
2016-10-10 21:39 ` Dmitry Torokhov
@ 2016-10-11 14:20 ` Benjamin Tissoires
2016-10-11 17:46 ` Dmitry Torokhov
0 siblings, 1 reply; 12+ messages in thread
From: Benjamin Tissoires @ 2016-10-11 14:20 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Wolfram Sang, Jean Delvare, Jonathan Corbet, KT Liao, Linux I2C,
linux-input@vger.kernel.org, linux-doc@vger.kernel.org, lkml
On Oct 10 2016 or thereabouts, Dmitry Torokhov wrote:
> Hi Benjamin,
>
> On Mon, Oct 10, 2016 at 9:42 AM, Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
> > The Thinkpad series 13 uses Host Notify to report the interrupt.
> > Add elan_smb_alert() to handle those interrupts and disable the irq
> > handling on this case.
> >
> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> >
>
> Why do we have to do this in the driver instead of having I2C core set
> it up for us? I expect we'd be repeating this block of code for every
> driver that has an option of using SMbus notify.
I didn't want to assign blindly an IRQ for Host Notify because it's a
device (as I2C client) feature. Not all SMBus clients on the Host Notify
capable bus are capable of Host Notify, so I thought it was the
responsibility of the driver to assign it.
However, I can see your point, though I'd need some inputs (and I'll
have to resend the series as the Intel bot showed me 2 mistakes).
So, if i2c-core gets to assign itself the IRQ for Host Notify, do we:
1) assign an IRQ to any SMBus device on a Host Notify capable adapter
that doesn't have a valid provided IRQ?
2) have a new field in struct i2c_board_info that explicitly requires
Host Notify as an IRQ?
3) do not touch anything in i2c_core, let the caller of i2c_new_device
fill in the irq by a call to
i2c_smbus_host_notify_to_irq(adapter, address)?
1) has the advantage of being transparent for everybody, except we will
provide IRQs to devices that don't have one (this can be ignored), but
this may also lead to errors when IRQ is not correctly set by the caller
where it should be, and the driver/developer doesn't understand why it
is never triggered.
2) is a nice compromise, but we will need some OF binding, add some work
in for the callers of i2c_new_device() and will not work nicely with
sysfs instantiated i2c devices (the new_device sysfs entry). The sysfs
could be solved by adding some new address namespace, but that doesn't
make sense I think
3) requires less changes in i2c_core but it's going to be even harder
to add a device through sysfs that is Host Notify capable given that we
can't specify the IRQ.
After thinking a bit, I think I'd lean toward 1), but I am open to other
options as well.
Cheers,
Benjamin
>
> Thanks!
>
> > ---
> >
> > new in v4 (was submitted on linux-input with the .alert callback)
> > ---
> > drivers/input/mouse/elan_i2c_core.c | 15 +++++++++++++++
> > 1 file changed, 15 insertions(+)
> >
> > diff --git a/drivers/input/mouse/elan_i2c_core.c b/drivers/input/mouse/elan_i2c_core.c
> > index 6f16eb4..4aaac5d 100644
> > --- a/drivers/input/mouse/elan_i2c_core.c
> > +++ b/drivers/input/mouse/elan_i2c_core.c
> > @@ -1040,6 +1040,21 @@ static int elan_probe(struct i2c_client *client,
> > I2C_FUNC_SMBUS_BLOCK_DATA |
> > I2C_FUNC_SMBUS_I2C_BLOCK)) {
> > transport_ops = &elan_smbus_ops;
> > +
> > + if (!irq) {
> > + if (!i2c_check_functionality(client->adapter,
> > + I2C_FUNC_SMBUS_HOST_NOTIFY)) {
> > + dev_err(dev, "no Host Notify support\n");
> > + return -ENODEV;
> > + }
> > +
> > + irq = i2c_smbus_host_notify_to_irq(client);
> > + if (irq < 0) {
> > + dev_err(dev,
> > + "Unable to request a Host Notify IRQ.\n");
> > + return irq;
> > + }
> > + }
> > } else {
> > dev_err(dev, "not a supported I2C/SMBus adapter\n");
> > return -EIO;
> > --
> > 2.7.4
> >
>
>
>
> --
> Dmitry
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v4 8/8] Input: elan_i2c - add Host Notify support
2016-10-11 14:20 ` Benjamin Tissoires
@ 2016-10-11 17:46 ` Dmitry Torokhov
0 siblings, 0 replies; 12+ messages in thread
From: Dmitry Torokhov @ 2016-10-11 17:46 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Wolfram Sang, Jean Delvare, Jonathan Corbet, KT Liao, Linux I2C,
linux-input@vger.kernel.org, linux-doc@vger.kernel.org, lkml
On Tue, Oct 11, 2016 at 04:20:52PM +0200, Benjamin Tissoires wrote:
> On Oct 10 2016 or thereabouts, Dmitry Torokhov wrote:
> > Hi Benjamin,
> >
> > On Mon, Oct 10, 2016 at 9:42 AM, Benjamin Tissoires
> > <benjamin.tissoires@redhat.com> wrote:
> > > The Thinkpad series 13 uses Host Notify to report the interrupt.
> > > Add elan_smb_alert() to handle those interrupts and disable the irq
> > > handling on this case.
> > >
> > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > >
> >
> > Why do we have to do this in the driver instead of having I2C core set
> > it up for us? I expect we'd be repeating this block of code for every
> > driver that has an option of using SMbus notify.
>
> I didn't want to assign blindly an IRQ for Host Notify because it's a
> device (as I2C client) feature. Not all SMBus clients on the Host Notify
> capable bus are capable of Host Notify, so I thought it was the
> responsibility of the driver to assign it.
Right, it is device's feature and it is up to the driver to decide if it
should be using interrupt-driven model or not. From I2C core I'd try the
following logic: if client->irq is not already set up and
ACPI/DT/platform data does not have any mention of IERQ and device is on
Host Notify capable bus then assign IRQ and let the dirver decide if it
wants to use it or not. Having client->irq present does not mean that we
have to use it. Driver knows the device best.
>
> However, I can see your point, though I'd need some inputs (and I'll
> have to resend the series as the Intel bot showed me 2 mistakes).
>
> So, if i2c-core gets to assign itself the IRQ for Host Notify, do we:
> 1) assign an IRQ to any SMBus device on a Host Notify capable adapter
> that doesn't have a valid provided IRQ?
> 2) have a new field in struct i2c_board_info that explicitly requires
> Host Notify as an IRQ?
> 3) do not touch anything in i2c_core, let the caller of i2c_new_device
> fill in the irq by a call to
> i2c_smbus_host_notify_to_irq(adapter, address)?
>
> 1) has the advantage of being transparent for everybody, except we will
> provide IRQs to devices that don't have one (this can be ignored), but
> this may also lead to errors when IRQ is not correctly set by the caller
> where it should be, and the driver/developer doesn't understand why it
> is never triggered.
>
> 2) is a nice compromise, but we will need some OF binding, add some work
> in for the callers of i2c_new_device() and will not work nicely with
> sysfs instantiated i2c devices (the new_device sysfs entry). The sysfs
> could be solved by adding some new address namespace, but that doesn't
> make sense I think
>
> 3) requires less changes in i2c_core but it's going to be even harder
> to add a device through sysfs that is Host Notify capable given that we
> can't specify the IRQ.
>
> After thinking a bit, I think I'd lean toward 1), but I am open to other
> options as well.
Yes, I think #1 is what we should do (but ultimately it is up to Wolfram
to decide).
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 12+ messages in thread