Re: [PATCH 2/2] i2c: i801: add support of Host Notify
Hi Benjamin,
On Tue, 23 Jun 2015 14:58:19 -0400, Benjamin Tissoires wrote:
The i801 chip can handle the Host Notify feature since ICH 3 as mentioned
in
http://www.intel.com/content/dam/doc/datasheet/82801ca-io-controller-hub-3-datasheet.pdf
Implement .toggle_smbus_host_notify() and rely on .alert() to notify
the actual client that it has a notification.
With a T440s and a Synaptics touchpad that implements Host Notify, the
payload data is always 0x, so I am not sure if the device actually
sends the payload or if there is a problem regarding the implementation.
Part of this code is inspired by i2c-smbus.c.
Signed-off-by: Benjamin Tissoires benjamin.tissoi...@redhat.com
An explanation on why a fifo is needed would be good to add, as I can't
see any obvious reason.
No time for a full review but just a few random comments.
---
drivers/i2c/busses/i2c-i801.c | 223
+-
drivers/i2c/i2c-core.c| 34 +++
drivers/i2c/i2c-smbus.c | 17 +---
include/linux/i2c.h | 3 +
4 files changed, 217 insertions(+), 60 deletions(-)
You really shouldn't mix core changes with driver specific changes.
Anyone should be able to backport the core changes alone, and then add
support to a different bus driver, without drawing in the i2c-i801
specific changes (that may not be needed and may not even apply.)
diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 5ecbb3f..22a3218 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
(...)
@@ -221,6 +233,17 @@ struct i801_priv {
const struct i801_mux_config *mux_drvdata;
struct platform_device *mux_pdev;
#endif
+
+ bool host_notify_requested;
+ struct work_struct host_notify;
+ struct kfifo host_notify_fifo;
+};
+
+#define SMBHSTNTFY_SIZE 8
+
+struct smbus_host_notify_data {
+ u8 addr;
+ u16 payload;
};
This structure seems generic to the protocol and not specific to the
Intel 82801 implementation, so it should go to linux/i2c-smbus.h.
#define FEATURE_SMBUS_PEC(1 0)
(...)
@@ -801,12 +894,35 @@ static u32 i801_func(struct i2c_adapter *adapter)
I2C_FUNC_SMBUS_BLOCK_DATA | I2C_FUNC_SMBUS_WRITE_I2C_BLOCK |
((priv-features FEATURE_SMBUS_PEC) ? I2C_FUNC_SMBUS_PEC : 0) |
((priv-features FEATURE_I2C_BLOCK_READ) ?
- I2C_FUNC_SMBUS_READ_I2C_BLOCK : 0);
+ I2C_FUNC_SMBUS_READ_I2C_BLOCK : 0) |
+((priv-features FEATURE_HOST_NOTIFY) ?
+ I2C_FUNC_SMBUS_HOST_NOTIFY : 0);
+}
+
+static int i801_toggle_host_ntfy(struct i2c_adapter *adapter, bool state)
Probably no longer relevant, but please spell out notify completely
in function and variable names (NTFY in register defines is OK.)
+{
+ struct i801_priv *priv = i2c_get_adapdata(adapter);
+
+ if (!(priv-features FEATURE_HOST_NOTIFY))
+ return -EOPNOTSUPP;
+
+ if (state) {
+ outb_p(SMBSLVCMD_HST_NTFY_INTREN, SMBSLVCMD(priv));
+ /* clear Host Notify bit to allow a new notification */
+ outb_p(SMBSLVSTS_HST_NTFY_STS, SMBSLVSTS(priv));
+ } else {
+ outb_p(0, SMBSLVCMD(priv));
+ }
+
+ priv-host_notify_requested = state;
+
+ return 0;
}
static const struct i2c_algorithm smbus_algorithm = {
- .smbus_xfer = i801_access,
- .functionality = i801_func,
+ .smbus_xfer = i801_access,
+ .functionality = i801_func,
+ .toggle_smbus_host_notify = i801_toggle_host_ntfy,
};
static const struct pci_device_id i801_ids[] = {
@@ -1166,6 +1282,7 @@ static int i801_probe(struct pci_dev *dev, const struct
pci_device_id *id)
priv-features |= FEATURE_BLOCK_BUFFER;
/* fall through */
case PCI_DEVICE_ID_INTEL_82801CA_3:
+ priv-features |= FEATURE_HOST_NOTIFY;
Please add a /* fall through */ comment for consistency and clarity.
case PCI_DEVICE_ID_INTEL_82801BA_2:
case PCI_DEVICE_ID_INTEL_82801AB_3:
case PCI_DEVICE_ID_INTEL_82801AA_3:
@@ -1250,6 +1367,15 @@ static int i801_probe(struct pci_dev *dev, const
struct pci_device_id *id)
}
}
+ INIT_WORK(priv-host_notify, i801_host_notify);
+ if (kfifo_alloc(priv-host_notify_fifo,
+ SMBHSTNTFY_SIZE * sizeof(struct smbus_host_notify_data),
+ GFP_KERNEL)) {
+ dev_err(dev-dev,
+ %s:failed allocating host_notify_fifo\n, __func__);
Adding the function name does not add value. Other messages in this
driver do not include it, so for consistency do not either. A leading
capital would be good too. Also kfifo_alloc() returns an actual error
code, which you should save, include in the error message and return to
the caller instead of hard-coding
Re: [PATCH 2/2] i2c: i801: add support of Host Notify
On Jul 07 2015 or thereabouts, Jean Delvare wrote:
Hi Benjamin,
On Tue, 23 Jun 2015 14:58:19 -0400, Benjamin Tissoires wrote:
The i801 chip can handle the Host Notify feature since ICH 3 as mentioned
in
http://www.intel.com/content/dam/doc/datasheet/82801ca-io-controller-hub-3-datasheet.pdf
Implement .toggle_smbus_host_notify() and rely on .alert() to notify
the actual client that it has a notification.
With a T440s and a Synaptics touchpad that implements Host Notify, the
payload data is always 0x, so I am not sure if the device actually
sends the payload or if there is a problem regarding the implementation.
Part of this code is inspired by i2c-smbus.c.
Signed-off-by: Benjamin Tissoires benjamin.tissoi...@redhat.com
An explanation on why a fifo is needed would be good to add, as I can't
see any obvious reason.
I inserted the kfifo to be able to read the payload and address in the
same thread that the one handling the IRQ. But a few tests shows that I
can simply trigger the work in the interrupt, and read the data and
clear the Host Notification bit in the work thread directly. So consider
I will remove this in the v2.
No time for a full review but just a few random comments.
---
drivers/i2c/busses/i2c-i801.c | 223
+-
drivers/i2c/i2c-core.c| 34 +++
drivers/i2c/i2c-smbus.c | 17 +---
include/linux/i2c.h | 3 +
4 files changed, 217 insertions(+), 60 deletions(-)
You really shouldn't mix core changes with driver specific changes.
Anyone should be able to backport the core changes alone, and then add
support to a different bus driver, without drawing in the i2c-i801
specific changes (that may not be needed and may not even apply.)
OK, I will split the series for each driver/feature.
diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 5ecbb3f..22a3218 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
(...)
@@ -221,6 +233,17 @@ struct i801_priv {
const struct i801_mux_config *mux_drvdata;
struct platform_device *mux_pdev;
#endif
+
+ bool host_notify_requested;
+ struct work_struct host_notify;
+ struct kfifo host_notify_fifo;
+};
+
+#define SMBHSTNTFY_SIZE8
+
+struct smbus_host_notify_data {
+ u8 addr;
+ u16 payload;
};
This structure seems generic to the protocol and not specific to the
Intel 82801 implementation, so it should go to linux/i2c-smbus.h.
fair enough
#define FEATURE_SMBUS_PEC (1 0)
(...)
@@ -801,12 +894,35 @@ static u32 i801_func(struct i2c_adapter *adapter)
I2C_FUNC_SMBUS_BLOCK_DATA | I2C_FUNC_SMBUS_WRITE_I2C_BLOCK |
((priv-features FEATURE_SMBUS_PEC) ? I2C_FUNC_SMBUS_PEC : 0) |
((priv-features FEATURE_I2C_BLOCK_READ) ?
- I2C_FUNC_SMBUS_READ_I2C_BLOCK : 0);
+ I2C_FUNC_SMBUS_READ_I2C_BLOCK : 0) |
+ ((priv-features FEATURE_HOST_NOTIFY) ?
+ I2C_FUNC_SMBUS_HOST_NOTIFY : 0);
+}
+
+static int i801_toggle_host_ntfy(struct i2c_adapter *adapter, bool state)
Probably no longer relevant, but please spell out notify completely
in function and variable names (NTFY in register defines is OK.)
Still relevant actually :)
I still need to enable the feature in the driver and on resume. So I
will amend in the v2.
+{
+ struct i801_priv *priv = i2c_get_adapdata(adapter);
+
+ if (!(priv-features FEATURE_HOST_NOTIFY))
+ return -EOPNOTSUPP;
+
+ if (state) {
+ outb_p(SMBSLVCMD_HST_NTFY_INTREN, SMBSLVCMD(priv));
+ /* clear Host Notify bit to allow a new notification */
+ outb_p(SMBSLVSTS_HST_NTFY_STS, SMBSLVSTS(priv));
+ } else {
+ outb_p(0, SMBSLVCMD(priv));
+ }
+
+ priv-host_notify_requested = state;
+
+ return 0;
}
static const struct i2c_algorithm smbus_algorithm = {
- .smbus_xfer = i801_access,
- .functionality = i801_func,
+ .smbus_xfer = i801_access,
+ .functionality = i801_func,
+ .toggle_smbus_host_notify = i801_toggle_host_ntfy,
};
static const struct pci_device_id i801_ids[] = {
@@ -1166,6 +1282,7 @@ static int i801_probe(struct pci_dev *dev, const
struct pci_device_id *id)
priv-features |= FEATURE_BLOCK_BUFFER;
/* fall through */
case PCI_DEVICE_ID_INTEL_82801CA_3:
+ priv-features |= FEATURE_HOST_NOTIFY;
Please add a /* fall through */ comment for consistency and clarity.
Oops, my mistake.
case PCI_DEVICE_ID_INTEL_82801BA_2:
case PCI_DEVICE_ID_INTEL_82801AB_3:
case PCI_DEVICE_ID_INTEL_82801AA_3:
@@ -1250,6 +1367,15 @@ static int i801_probe(struct pci_dev *dev, const
struct pci_device_id *id)
}
}
+
Re: [PATCH 2/2] i2c: i801: add support of Host Notify
On Tue, 7 Jul 2015 16:16:38 -0400, Benjamin Tissoires wrote: On Jul 07 2015 or thereabouts, Jean Delvare wrote: So you use the same driver callback for SMBus Alert and SMBus Host Notify. This makes some sense, but if a given driver supports both, how does it know which event happened? The data is completely different and most probably the action required from the driver as well. Yeah, this gets messy. I re-used the .alert() callback because of the documentation: Alert callback, for example for the SMBus alert protocol. It would seem that the alert is generic and could be re-used. But OTOH, it is not prepared to receive anything else than a SMBus Alert. Given that I had a toggle_host_notify() call, I figured that this was not a problem unless you write a driver which implements both (I can not find a sane use case for this though). But now that this call has disappeared, we would need a way to differentiate the too of them. I can see two solutions out of my head right now: - add a protocol parameter (with an enum) to .alert() - add a new callback .host_notify() in struct i2c_driver. I came to the same conclusion. I think I like the second option more given that it will allow to not touch the current code in i2c_smbus. No strong preference so do it the way you prefer. Thanks, -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] i2c: i801: add support of Host Notify
The i801 chip can handle the Host Notify feature since ICH 3 as mentioned in http://www.intel.com/content/dam/doc/datasheet/82801ca-io-controller-hub-3-datasheet.pdf Implement .toggle_smbus_host_notify() and rely on .alert() to notify the actual client that it has a notification. With a T440s and a Synaptics touchpad that implements Host Notify, the payload data is always 0x, so I am not sure if the device actually sends the payload or if there is a problem regarding the implementation. Part of this code is inspired by i2c-smbus.c. Signed-off-by: Benjamin Tissoires benjamin.tissoi...@redhat.com --- drivers/i2c/busses/i2c-i801.c | 223 +- drivers/i2c/i2c-core.c| 34 +++ drivers/i2c/i2c-smbus.c | 17 +--- include/linux/i2c.h | 3 + 4 files changed, 217 insertions(+), 60 deletions(-) diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c index 5ecbb3f..22a3218 100644 --- a/drivers/i2c/busses/i2c-i801.c +++ b/drivers/i2c/busses/i2c-i801.c @@ -20,46 +20,46 @@ /* * Supports the following Intel I/O Controller Hubs (ICH): * - * I/O Block I2C - * region SMBus Block proc. block - * Chip name PCI ID sizePEC buffer callread - * --- - * 82801AA (ICH) 0x2413 16 no no no no - * 82801AB (ICH0) 0x2423 16 no no no no - * 82801BA (ICH2) 0x2443 16 no no no no - * 82801CA (ICH3) 0x2483 32 softno no no - * 82801DB (ICH4) 0x24c3 32 hardyes no no - * 82801E (ICH5) 0x24d3 32 hardyes yes yes - * 6300ESB 0x25a4 32 hardyes yes yes - * 82801F (ICH6) 0x266a 32 hardyes yes yes - * 6310ESB/6320ESB 0x269b 32 hardyes yes yes - * 82801G (ICH7) 0x27da 32 hardyes yes yes - * 82801H (ICH8) 0x283e 32 hardyes yes yes - * 82801I (ICH9) 0x2930 32 hardyes yes yes - * EP80579 (Tolapai) 0x5032 32 hardyes yes yes - * ICH10 0x3a30 32 hardyes yes yes - * ICH10 0x3a60 32 hardyes yes yes - * 5/3400 Series (PCH) 0x3b30 32 hardyes yes yes - * 6 Series (PCH) 0x1c22 32 hardyes yes yes - * Patsburg (PCH) 0x1d22 32 hardyes yes yes - * Patsburg (PCH) IDF 0x1d70 32 hardyes yes yes - * Patsburg (PCH) IDF 0x1d71 32 hardyes yes yes - * Patsburg (PCH) IDF 0x1d72 32 hardyes yes yes - * DH89xxCC (PCH) 0x2330 32 hardyes yes yes - * Panther Point (PCH) 0x1e22 32 hardyes yes yes - * Lynx Point (PCH)0x8c22 32 hardyes yes yes - * Lynx Point-LP (PCH) 0x9c22 32 hardyes yes yes - * Avoton (SOC)0x1f3c 32 hardyes yes yes - * Wellsburg (PCH) 0x8d22 32 hardyes yes yes - * Wellsburg (PCH) MS 0x8d7d 32 hardyes yes yes - * Wellsburg (PCH) MS 0x8d7e 32 hardyes yes yes - * Wellsburg (PCH) MS 0x8d7f 32 hardyes yes yes - * Coleto Creek (PCH) 0x23b0 32 hardyes yes yes - * Wildcat Point (PCH) 0x8ca2 32 hardyes yes yes - * Wildcat Point-LP (PCH) 0x9ca2 32 hardyes yes yes - * BayTrail (SOC) 0x0f12 32 hardyes yes yes - * Sunrise Point-H (PCH) 0xa123 32 hardyes yes yes - * Sunrise Point-LP (PCH) 0x9d23 32 hardyes yes yes + * I/O SMBus Block I2C + * region Slave HostSMBus Block proc. block + * Chip name PCI ID sizemodenotify PEC buffer callread + * -- + * 82801AA (ICH) 0x2413 16 no no no no no no + * 82801AB (ICH0) 0x2423 16 no no no no no no + * 82801BA (ICH2) 0x2443 16 yes no no no no no + * 82801CA (ICH3) 0x2483 32 yes yes softno no no + * 82801DB (ICH4) 0x24c3 32 yes yes hardyes
