* [PATCH v9 1/2] i2c: i801: add support of Host Notify
@ 2016-06-24 14:39 Benjamin Tissoires
2016-06-24 14:41 ` [PATCH v9 2/2] Input: synaptics-rmi4 - add SMBus support Benjamin Tissoires
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Benjamin Tissoires @ 2016-06-24 14:39 UTC (permalink / raw)
To: Wolfram Sang, Jean Delvare, Dmitry Torokhov, Andrew Duggan,
Christopher Heiny
Cc: linux-i2c, linux-kernel
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
Enable the functionality unconditionally and propagate the alert
on each notification.
With a T440s and a Synaptics touchpad that implements Host Notify, the
payload data is always 0x0000, so I am not sure if the device actually
sends the payload or if there is a problem regarding the implementation.
Tested-by: Andrew Duggan <aduggan@synaptics.com>
Acked-by: Wolfram Sang <wsa@the-dreams.de>
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
changes in v2:
- removed the description of the Slave functionality support in the chip table
(the table shows what is supported, not what the hardware is capable of)
- use i2c-smbus to forward the notification
- remove the fifo, and directly retrieve the address and payload in the worker
- do not check for Host Notification is the feature is not enabled
- use inw_p() to read the payload instead of 2 inb_p() calls
- add /* fall-through */ comment
- unconditionally enable Host Notify if the hardware supports it (can be
disabled by the user)
no changes in v3
changes in v4:
- make use of the new API -> no more worker spawning here
- solved a race between the access of the Host Notify registers and the actual
I2C transfers.
changes in v5:
- added SKL Host Notify support
changes in v6:
- select I2C_SMBUS in Kconfig to prevent an undefined reference when I2C_I801
is set to 'Y' while I2C_SMBUS is set to 'M'
no changes in v7
changes in v8:
- reapplied after http://patchwork.ozlabs.org/patch/632768/ and merged the
conflict (minor conflict in the struct i801_priv).
- removed the .resume hook as upstream changed suspend/resume hooks and there
is no need in the end to re-enable host notify on resume (tested on Lenovo
t440 and t450).
- kept Wolfram's Acked-by as the changes were minor
changes in v9:
- re-add the .resume hook. Looks like the Lenovo T440 sometimes forget to
re-enable Host Notify on resume, so better have it reset for everybody
drivers/i2c/busses/Kconfig | 1 +
drivers/i2c/busses/i2c-i801.c | 88 +++++++++++++++++++++++++++++++++++++++++--
2 files changed, 86 insertions(+), 3 deletions(-)
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index efa3d9b..b4b6cb0 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -91,6 +91,7 @@ config I2C_I801
tristate "Intel 82801 (ICH/PCH)"
depends on PCI
select CHECK_SIGNATURE if X86 && DMI
+ select I2C_SMBUS
help
If you say yes to this option, support will be included for the Intel
801 family of mainboard I2C interfaces. Specifically, the following
diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index b436963..312caed 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -72,6 +72,7 @@
* Block process call transaction no
* I2C block read transaction yes (doesn't use the block buffer)
* Slave mode no
+ * SMBus Host Notify yes
* Interrupt processing yes
*
* See the file Documentation/i2c/busses/i2c-i801 for details.
@@ -86,6 +87,7 @@
#include <linux/ioport.h>
#include <linux/init.h>
#include <linux/i2c.h>
+#include <linux/i2c-smbus.h>
#include <linux/acpi.h>
#include <linux/io.h>
#include <linux/dmi.h>
@@ -113,6 +115,10 @@
#define SMBPEC(p) (8 + (p)->smba) /* ICH3 and later */
#define SMBAUXSTS(p) (12 + (p)->smba) /* ICH4 and later */
#define SMBAUXCTL(p) (13 + (p)->smba) /* ICH4 and later */
+#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
@@ -177,6 +183,12 @@
#define SMBHSTSTS_INTR 0x02
#define SMBHSTSTS_HOST_BUSY 0x01
+/* Host Notify Status registers bits */
+#define SMBSLVSTS_HST_NTFY_STS 1
+
+/* Host Notify Command registers bits */
+#define SMBSLVCMD_HST_NTFY_INTREN 0x01
+
#define STATUS_ERROR_FLAGS (SMBHSTSTS_FAILED | SMBHSTSTS_BUS_ERR | \
SMBHSTSTS_DEV_ERR)
@@ -252,13 +264,17 @@ struct i801_priv {
*/
bool acpi_reserved;
struct mutex acpi_lock;
+ 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)
#define FEATURE_I2C_BLOCK_READ (1 << 3)
#define FEATURE_IRQ (1 << 4)
+#define FEATURE_HOST_NOTIFY (1 << 5)
/* Not really a feature, but it's convenient to handle it as such */
#define FEATURE_IDF (1 << 15)
#define FEATURE_TCO (1 << 16)
@@ -269,6 +285,7 @@ static const char *i801_feature_names[] = {
"Block process call",
"I2C block read",
"Interrupt",
+ "SMBus Host Notify",
};
static unsigned int disable_features;
@@ -277,7 +294,8 @@ MODULE_PARM_DESC(disable_features, "Disable selected driver features:\n"
"\t\t 0x01 disable SMBus PEC\n"
"\t\t 0x02 disable the block buffer\n"
"\t\t 0x08 disable the I2C block read functionality\n"
- "\t\t 0x10 don't use interrupts ");
+ "\t\t 0x10 don't use interrupts\n"
+ "\t\t 0x20 disable SMBus Host Notify ");
/* Make sure the SMBus host is ready to start transmitting.
Return 0 if it is, -EBUSY if it is not. */
@@ -511,8 +529,23 @@ static void i801_isr_byte_done(struct i801_priv *priv)
outb_p(SMBHSTSTS_BYTE_DONE, SMBHSTSTS(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);
+
+ /* clear Host Notify bit and return */
+ outb_p(SMBSLVSTS_HST_NTFY_STS, SMBSLVSTS(priv));
+ return IRQ_HANDLED;
+}
+
/*
- * There are two kinds of interrupts:
+ * There are three kinds of interrupts:
*
* 1) i801 signals transaction completion with one of these interrupts:
* INTR - Success
@@ -524,6 +557,8 @@ static void i801_isr_byte_done(struct i801_priv *priv)
*
* 2) For byte-by-byte (I2C read/write) transactions, one BYTE_DONE interrupt
* occurs for each byte of a byte-by-byte to prepare the next byte.
+ *
+ * 3) Host Notify interrupts
*/
static irqreturn_t i801_isr(int irq, void *dev_id)
{
@@ -536,6 +571,12 @@ static irqreturn_t i801_isr(int irq, void *dev_id)
if (!(pcists & SMBPCISTS_INTS))
return IRQ_NONE;
+ if (priv->features & FEATURE_HOST_NOTIFY) {
+ status = inb_p(SMBSLVSTS(priv));
+ if (status & SMBSLVSTS_HST_NTFY_STS)
+ return i801_host_notify_isr(priv);
+ }
+
status = inb_p(SMBHSTSTS(priv));
if (status & SMBHSTSTS_BYTE_DONE)
i801_isr_byte_done(priv);
@@ -847,7 +888,28 @@ 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_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;
+
+ 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));
+
+ return 0;
}
static const struct i2c_algorithm smbus_algorithm = {
@@ -1379,6 +1441,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
priv->features |= FEATURE_SMBUS_PEC;
priv->features |= FEATURE_BLOCK_BUFFER;
priv->features |= FEATURE_TCO;
+ priv->features |= FEATURE_HOST_NOTIFY;
break;
case PCI_DEVICE_ID_INTEL_PATSBURG_SMBUS_IDF0:
@@ -1398,6 +1461,8 @@ 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;
+ /* fall through */
case PCI_DEVICE_ID_INTEL_82801BA_2:
case PCI_DEVICE_ID_INTEL_82801AB_3:
case PCI_DEVICE_ID_INTEL_82801AA_3:
@@ -1507,6 +1572,15 @@ 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_probe_optional_slaves(priv);
/* We ignore errors - multiplexing is optional */
i801_add_mux(priv);
@@ -1553,6 +1627,14 @@ static int i801_suspend(struct device *dev)
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");
+
return 0;
}
#endif
--
2.5.5
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v9 2/2] Input: synaptics-rmi4 - add SMBus support
2016-06-24 14:39 [PATCH v9 1/2] i2c: i801: add support of Host Notify Benjamin Tissoires
@ 2016-06-24 14:41 ` Benjamin Tissoires
2016-06-24 21:47 ` [PATCH] Input: fix platform_no_drv_owner.cocci warnings kbuild test robot
2016-06-24 21:47 ` [PATCH v9 2/2] Input: synaptics-rmi4 - add SMBus support kbuild test robot
2016-07-05 15:40 ` [PATCH v9 1/2] i2c: i801: add support of Host Notify Wolfram Sang
2016-07-18 14:12 ` Jean Delvare
2 siblings, 2 replies; 9+ messages in thread
From: Benjamin Tissoires @ 2016-06-24 14:41 UTC (permalink / raw)
To: Wolfram Sang, Jean Delvare, Dmitry Torokhov, Andrew Duggan,
Christopher Heiny
Cc: linux-i2c, linux-kernel
Code obtained from https://raw.githubusercontent.com/mightybigcar/synaptics-rmi4/jf/drivers/input/rmi4/rmi_smbus.c
and updated to match upstream. And fixed to make it work.
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Signed-off-by: Andrew Duggan <aduggan@synaptics.com>
---
new in v5
no changes in v6
changes in v7:
- fixed typos as per Andrew's requests
- changed module author from Allie to Andrew as per Andrew's request
- add suspend/resume callbacks to match upstream rmi4 behavior
no changes in v8
changes in v8:
- fixes from Dmitry: __le16, __maybe_unused and removed forward declaration
drivers/input/rmi4/Kconfig | 12 ++
drivers/input/rmi4/Makefile | 1 +
drivers/input/rmi4/rmi_bus.h | 12 ++
drivers/input/rmi4/rmi_smbus.c | 464 +++++++++++++++++++++++++++++++++++++++++
4 files changed, 489 insertions(+)
create mode 100644 drivers/input/rmi4/rmi_smbus.c
diff --git a/drivers/input/rmi4/Kconfig b/drivers/input/rmi4/Kconfig
index f73df24..86a180b 100644
--- a/drivers/input/rmi4/Kconfig
+++ b/drivers/input/rmi4/Kconfig
@@ -27,6 +27,18 @@ config RMI4_SPI
If unsure, say N.
+config RMI4_SMB
+ tristate "RMI4 SMB Support"
+ depends on RMI4_CORE && I2C
+ help
+ Say Y here if you want to support RMI4 devices connected to an SMB
+ bus.
+
+ If unsure, say N.
+
+ To compile this driver as a module, choose M here: the module will be
+ called rmi_smbus.
+
config RMI4_2D_SENSOR
bool
depends on RMI4_CORE
diff --git a/drivers/input/rmi4/Makefile b/drivers/input/rmi4/Makefile
index 95c00a7..3c8ebf2 100644
--- a/drivers/input/rmi4/Makefile
+++ b/drivers/input/rmi4/Makefile
@@ -11,3 +11,4 @@ rmi_core-$(CONFIG_RMI4_F30) += rmi_f30.o
# Transports
obj-$(CONFIG_RMI4_I2C) += rmi_i2c.o
obj-$(CONFIG_RMI4_SPI) += rmi_spi.o
+obj-$(CONFIG_RMI4_SMB) += rmi_smbus.o
diff --git a/drivers/input/rmi4/rmi_bus.h b/drivers/input/rmi4/rmi_bus.h
index 8995798..b7625a9 100644
--- a/drivers/input/rmi4/rmi_bus.h
+++ b/drivers/input/rmi4/rmi_bus.h
@@ -105,6 +105,18 @@ rmi_get_platform_data(struct rmi_device *d)
bool rmi_is_physical_device(struct device *dev);
/**
+ * rmi_reset - reset a RMI4 device
+ * @d: Pointer to an RMI device
+ *
+ * Calls for a reset of each function implemented by a specific device.
+ * Returns 0 on success or a negative error code.
+ */
+static inline int rmi_reset(struct rmi_device *d)
+{
+ return d->driver->reset_handler(d);
+}
+
+/**
* rmi_read - read a single byte
* @d: Pointer to an RMI device
* @addr: The address to read from
diff --git a/drivers/input/rmi4/rmi_smbus.c b/drivers/input/rmi4/rmi_smbus.c
new file mode 100644
index 0000000..92c6ab1
--- /dev/null
+++ b/drivers/input/rmi4/rmi_smbus.c
@@ -0,0 +1,464 @@
+/*
+ * Copyright (c) 2015 - 2016 Red Hat, Inc
+ * Copyright (c) 2011, 2012 Synaptics Incorporated
+ * Copyright (c) 2011 Unixphere
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ */
+
+#include <linux/kernel.h>
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/kconfig.h>
+#include <linux/lockdep.h>
+#include <linux/module.h>
+#include <linux/pm.h>
+#include <linux/rmi.h>
+#include <linux/slab.h>
+#include "rmi_driver.h"
+
+#define SMB_PROTOCOL_VERSION_ADDRESS 0xfd
+#define SMB_MAX_COUNT 32
+#define RMI_SMB2_MAP_SIZE 8 /* 8 entry of 4 bytes each */
+#define RMI_SMB2_MAP_FLAGS_WE 0x01
+
+struct mapping_table_entry {
+ __le16 rmiaddr;
+ u8 readcount;
+ u8 flags;
+};
+
+struct rmi_smb_xport {
+ struct rmi_transport_dev xport;
+ struct i2c_client *client;
+
+ struct mutex page_mutex;
+ int page;
+ u8 table_index;
+ struct mutex mappingtable_mutex;
+ struct mapping_table_entry mapping_table[RMI_SMB2_MAP_SIZE];
+};
+
+static int rmi_smb_get_version(struct rmi_smb_xport *rmi_smb)
+{
+ struct i2c_client *client = rmi_smb->client;
+ int retval;
+
+ /* Check if for SMBus new version device by reading version byte. */
+ retval = i2c_smbus_read_byte_data(client, SMB_PROTOCOL_VERSION_ADDRESS);
+ if (retval < 0) {
+ dev_err(&client->dev, "failed to get SMBus version number!\n");
+ return retval;
+ }
+ return retval + 1;
+}
+
+/* SMB block write - wrapper over ic2_smb_write_block */
+static int smb_block_write(struct rmi_transport_dev *xport,
+ u8 commandcode, const void *buf, size_t len)
+{
+ struct rmi_smb_xport *rmi_smb =
+ container_of(xport, struct rmi_smb_xport, xport);
+ struct i2c_client *client = rmi_smb->client;
+ int retval;
+
+ retval = i2c_smbus_write_block_data(client, commandcode, len, buf);
+
+ rmi_dbg(RMI_DEBUG_XPORT, &client->dev,
+ "wrote %zd bytes at %#04x: %d (%*ph)\n",
+ len, commandcode, retval, (int)len, buf);
+
+ return retval;
+}
+
+/*
+ * The function to get command code for smbus operations and keeps
+ * records to the driver mapping table
+ */
+static int rmi_smb_get_command_code(struct rmi_transport_dev *xport,
+ u16 rmiaddr, int bytecount, bool isread, u8 *commandcode)
+{
+ struct rmi_smb_xport *rmi_smb =
+ container_of(xport, struct rmi_smb_xport, xport);
+ int i;
+ int retval;
+ struct mapping_table_entry mapping_data[1];
+
+ mutex_lock(&rmi_smb->mappingtable_mutex);
+ for (i = 0; i < RMI_SMB2_MAP_SIZE; i++) {
+ if (rmi_smb->mapping_table[i].rmiaddr == rmiaddr) {
+ if (isread) {
+ if (rmi_smb->mapping_table[i].readcount
+ == bytecount) {
+ *commandcode = i;
+ retval = 0;
+ goto exit;
+ }
+ } else {
+ if (rmi_smb->mapping_table[i].flags &
+ RMI_SMB2_MAP_FLAGS_WE) {
+ *commandcode = i;
+ retval = 0;
+ goto exit;
+ }
+ }
+ }
+ }
+ i = rmi_smb->table_index;
+ rmi_smb->table_index = (i + 1) % RMI_SMB2_MAP_SIZE;
+
+ /* constructs mapping table data entry. 4 bytes each entry */
+ memset(mapping_data, 0, sizeof(mapping_data));
+
+ mapping_data[0].rmiaddr = cpu_to_le16(rmiaddr);
+ mapping_data[0].readcount = bytecount;
+ mapping_data[0].flags = !isread ? RMI_SMB2_MAP_FLAGS_WE : 0;
+
+ retval = smb_block_write(xport, i + 0x80, mapping_data,
+ sizeof(mapping_data));
+
+ if (retval < 0) {
+ /*
+ * if not written to device mapping table
+ * clear the driver mapping table records
+ */
+ rmi_smb->mapping_table[i].rmiaddr = 0x0000;
+ rmi_smb->mapping_table[i].readcount = 0;
+ rmi_smb->mapping_table[i].flags = 0;
+ goto exit;
+ }
+ /* save to the driver level mapping table */
+ rmi_smb->mapping_table[i].rmiaddr = rmiaddr;
+ rmi_smb->mapping_table[i].readcount = bytecount;
+ rmi_smb->mapping_table[i].flags = !isread ? RMI_SMB2_MAP_FLAGS_WE : 0;
+ *commandcode = i;
+
+exit:
+ mutex_unlock(&rmi_smb->mappingtable_mutex);
+
+ return retval;
+}
+
+static int rmi_smb_write_block(struct rmi_transport_dev *xport, u16 rmiaddr,
+ const void *databuff, size_t len)
+{
+ int retval = 0;
+ u8 commandcode;
+ struct rmi_smb_xport *rmi_smb =
+ container_of(xport, struct rmi_smb_xport, xport);
+ int cur_len = (int)len;
+
+ mutex_lock(&rmi_smb->page_mutex);
+
+ while (cur_len > 0) {
+ /*
+ * break into 32 bytes chunks to write get command code
+ */
+ int block_len = min_t(int, len, SMB_MAX_COUNT);
+
+ retval = rmi_smb_get_command_code(xport, rmiaddr, block_len,
+ false, &commandcode);
+ if (retval < 0)
+ goto exit;
+
+ retval = smb_block_write(xport, commandcode,
+ databuff, block_len);
+ if (retval < 0)
+ goto exit;
+
+ /* prepare to write next block of bytes */
+ cur_len -= SMB_MAX_COUNT;
+ databuff += SMB_MAX_COUNT;
+ rmiaddr += SMB_MAX_COUNT;
+ }
+exit:
+ mutex_unlock(&rmi_smb->page_mutex);
+ return retval;
+}
+
+/* SMB block read - wrapper over ic2_smb_read_block */
+static int smb_block_read(struct rmi_transport_dev *xport,
+ u8 commandcode, void *buf, size_t len)
+{
+ struct rmi_smb_xport *rmi_smb =
+ container_of(xport, struct rmi_smb_xport, xport);
+ struct i2c_client *client = rmi_smb->client;
+ int retval;
+
+ retval = i2c_smbus_read_block_data(client, commandcode, buf);
+ if (retval < 0)
+ return retval;
+
+ return retval;
+}
+
+static int rmi_smb_read_block(struct rmi_transport_dev *xport, u16 rmiaddr,
+ void *databuff, size_t len)
+{
+ struct rmi_smb_xport *rmi_smb =
+ container_of(xport, struct rmi_smb_xport, xport);
+ int retval;
+ u8 commandcode;
+ int cur_len = (int)len;
+
+ mutex_lock(&rmi_smb->page_mutex);
+ memset(databuff, 0, len);
+
+ while (cur_len > 0) {
+ /* break into 32 bytes chunks to write get command code */
+ int block_len = min_t(int, cur_len, SMB_MAX_COUNT);
+
+ retval = rmi_smb_get_command_code(xport, rmiaddr, block_len,
+ true, &commandcode);
+ if (retval < 0)
+ goto exit;
+
+ retval = smb_block_read(xport, commandcode,
+ databuff, block_len);
+ if (retval < 0)
+ goto exit;
+
+ /* prepare to read next block of bytes */
+ cur_len -= SMB_MAX_COUNT;
+ databuff += SMB_MAX_COUNT;
+ rmiaddr += SMB_MAX_COUNT;
+ }
+
+ retval = 0;
+
+exit:
+ mutex_unlock(&rmi_smb->page_mutex);
+ return retval;
+}
+
+static void rmi_smb_clear_state(struct rmi_smb_xport *rmi_smb)
+{
+ /* the mapping table has been flushed, discard the current one */
+ mutex_lock(&rmi_smb->mappingtable_mutex);
+ memset(rmi_smb->mapping_table, 0, sizeof(rmi_smb->mapping_table));
+ mutex_unlock(&rmi_smb->mappingtable_mutex);
+}
+
+static int rmi_smb_enable_smbus_mode(struct rmi_smb_xport *rmi_smb)
+{
+ int retval;
+
+ /* we need to get the smbus version to activate the touchpad */
+ retval = rmi_smb_get_version(rmi_smb);
+ if (retval < 0)
+ return retval;
+
+ return 0;
+}
+
+static int rmi_smb_reset(struct rmi_transport_dev *xport, u16 reset_addr)
+{
+ struct rmi_smb_xport *rmi_smb =
+ container_of(xport, struct rmi_smb_xport, xport);
+
+ rmi_smb_clear_state(rmi_smb);
+
+ /*
+ * we do not call the actual reset command, it has to be handled in
+ * PS/2 or there will be races between PS/2 and SMBus.
+ * PS/2 should ensure that a psmouse_reset is called before
+ * intializing the device and after it has been removed to be in a known
+ * state.
+ */
+ return rmi_smb_enable_smbus_mode(rmi_smb);
+}
+
+static const struct rmi_transport_ops rmi_smb_ops = {
+ .write_block = rmi_smb_write_block,
+ .read_block = rmi_smb_read_block,
+ .reset = rmi_smb_reset,
+};
+
+static void rmi_smb_alert(struct i2c_client *client,
+ enum i2c_alert_protocol type, unsigned int data)
+{
+ struct rmi_smb_xport *rmi_smb = i2c_get_clientdata(client);
+ struct rmi_device *rmi_dev = rmi_smb->xport.rmi_dev;
+ struct rmi_driver_data *drv_data = dev_get_drvdata(&rmi_dev->dev);
+
+ if (type != I2C_PROTOCOL_SMBUS_HOST_NOTIFY)
+ return;
+
+ if (!drv_data) {
+ dev_err(&client->dev,
+ "Something went wrong, driver data is NULL.\n");
+ return;
+ }
+
+ rmi_process_interrupt_requests(rmi_dev);
+}
+
+static int rmi_smb_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ struct rmi_device_platform_data *pdata = dev_get_platdata(&client->dev);
+ struct rmi_smb_xport *rmi_smb;
+ int retval;
+ int smbus_version;
+
+ if (!i2c_check_functionality(client->adapter,
+ I2C_FUNC_SMBUS_READ_BLOCK_DATA |
+ I2C_FUNC_SMBUS_HOST_NOTIFY)) {
+ dev_err(&client->dev,
+ "adapter does not support required functionality.\n");
+ return -ENODEV;
+ }
+
+ rmi_smb = devm_kzalloc(&client->dev, sizeof(struct rmi_smb_xport),
+ GFP_KERNEL);
+ if (!rmi_smb)
+ return -ENOMEM;
+
+ if (!pdata) {
+ dev_err(&client->dev, "no platform data, aborting\n");
+ return -ENOMEM;
+ }
+
+ rmi_dbg(RMI_DEBUG_XPORT, &client->dev, "Probing %s.\n",
+ dev_name(&client->dev));
+
+ rmi_smb->client = client;
+ mutex_init(&rmi_smb->page_mutex);
+ mutex_init(&rmi_smb->mappingtable_mutex);
+
+ rmi_smb->xport.dev = &client->dev;
+ rmi_smb->xport.pdata = *pdata;
+ rmi_smb->xport.proto_name = "smb2";
+ rmi_smb->xport.ops = &rmi_smb_ops;
+
+ /* Check if for SMBus new version device by reading version byte. */
+ retval = rmi_smb_get_version(rmi_smb);
+ if (retval < 0)
+ return retval;
+
+ smbus_version = retval;
+ rmi_dbg(RMI_DEBUG_XPORT, &client->dev, "Smbus version is %d",
+ smbus_version);
+
+ if (smbus_version != 2) {
+ dev_err(&client->dev, "Unrecognized SMB version %d.\n",
+ smbus_version);
+ return -ENODEV;
+ }
+
+ i2c_set_clientdata(client, rmi_smb);
+
+ retval = rmi_register_transport_device(&rmi_smb->xport);
+ if (retval) {
+ dev_err(&client->dev, "Failed to register transport driver at 0x%.2X.\n",
+ client->addr);
+ i2c_set_clientdata(client, NULL);
+ return retval;
+ }
+
+ dev_info(&client->dev, "registered rmi smb driver at %#04x.\n",
+ client->addr);
+ return 0;
+
+}
+
+static int rmi_smb_remove(struct i2c_client *client)
+{
+ struct rmi_smb_xport *rmi_smb = i2c_get_clientdata(client);
+
+ rmi_unregister_transport_device(&rmi_smb->xport);
+
+ return 0;
+}
+
+static int __maybe_unused rmi_smb_suspend(struct device *dev)
+{
+ struct i2c_client *client = to_i2c_client(dev);
+ struct rmi_smb_xport *rmi_smb = i2c_get_clientdata(client);
+ int ret;
+
+ ret = rmi_driver_suspend(rmi_smb->xport.rmi_dev);
+ if (ret)
+ dev_warn(dev, "Failed to suspend device: %d\n", ret);
+
+ return ret;
+}
+
+static int __maybe_unused rmi_smb_runtime_suspend(struct device *dev)
+{
+ struct i2c_client *client = to_i2c_client(dev);
+ struct rmi_smb_xport *rmi_smb = i2c_get_clientdata(client);
+ int ret;
+
+ ret = rmi_driver_suspend(rmi_smb->xport.rmi_dev);
+ if (ret)
+ dev_warn(dev, "Failed to suspend device: %d\n", ret);
+
+ return 0;
+}
+
+static int __maybe_unused rmi_smb_runtime_resume(struct device *dev)
+{
+ struct i2c_client *client = to_i2c_client(dev);
+ struct rmi_smb_xport *rmi_smb = i2c_get_clientdata(client);
+ int ret;
+
+ ret = rmi_driver_resume(rmi_smb->xport.rmi_dev);
+ if (ret)
+ dev_warn(dev, "Failed to resume device: %d\n", ret);
+
+ return 0;
+}
+
+static const struct dev_pm_ops rmi_smb_pm = {
+ SET_SYSTEM_SLEEP_PM_OPS(rmi_smb_suspend, NULL)
+ SET_RUNTIME_PM_OPS(rmi_smb_runtime_suspend, rmi_smb_runtime_resume,
+ NULL)
+};
+
+static int rmi_smb_resume(struct device *dev)
+{
+ struct i2c_client *client = container_of(dev, struct i2c_client, dev);
+ struct rmi_smb_xport *rmi_smb = i2c_get_clientdata(client);
+ struct rmi_device *rmi_dev = rmi_smb->xport.rmi_dev;
+ int ret;
+
+ rmi_smb_reset(&rmi_smb->xport, 0);
+
+ rmi_reset(rmi_dev);
+
+ ret = rmi_driver_resume(rmi_smb->xport.rmi_dev);
+ if (ret)
+ dev_warn(dev, "Failed to resume device: %d\n", ret);
+
+ return 0;
+}
+
+static const struct i2c_device_id rmi_id[] = {
+ { "rmi4_smbus", 0 },
+ { }
+};
+MODULE_DEVICE_TABLE(i2c, rmi_id);
+
+static struct i2c_driver rmi_smb_driver = {
+ .driver = {
+ .owner = THIS_MODULE,
+ .name = "rmi4_smbus",
+ .pm = &rmi_smb_pm,
+ .resume = rmi_smb_resume,
+ },
+ .id_table = rmi_id,
+ .probe = rmi_smb_probe,
+ .remove = rmi_smb_remove,
+ .alert = rmi_smb_alert,
+};
+
+module_i2c_driver(rmi_smb_driver);
+
+MODULE_AUTHOR("Andrew Duggan <aduggan@synaptics.com>");
+MODULE_AUTHOR("Benjamin Tissoires <benjamin.tissoires@redhat.com>");
+MODULE_DESCRIPTION("RMI4 SMBus driver");
+MODULE_LICENSE("GPL");
--
2.5.5
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH] Input: fix platform_no_drv_owner.cocci warnings
2016-06-24 14:41 ` [PATCH v9 2/2] Input: synaptics-rmi4 - add SMBus support Benjamin Tissoires
@ 2016-06-24 21:47 ` kbuild test robot
2016-06-24 21:47 ` [PATCH v9 2/2] Input: synaptics-rmi4 - add SMBus support kbuild test robot
1 sibling, 0 replies; 9+ messages in thread
From: kbuild test robot @ 2016-06-24 21:47 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: kbuild-all, Wolfram Sang, Jean Delvare, Dmitry Torokhov,
Andrew Duggan, Christopher Heiny, linux-i2c, linux-kernel
drivers/input/rmi4/rmi_smbus.c:448:3-8: No need to set .owner here. The core will do it.
Remove .owner field if calls are used which set it automatically
Generated by: scripts/coccinelle/api/platform_no_drv_owner.cocci
CC: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
---
rmi_smbus.c | 1 -
1 file changed, 1 deletion(-)
--- a/drivers/input/rmi4/rmi_smbus.c
+++ b/drivers/input/rmi4/rmi_smbus.c
@@ -445,7 +445,6 @@ MODULE_DEVICE_TABLE(i2c, rmi_id);
static struct i2c_driver rmi_smb_driver = {
.driver = {
- .owner = THIS_MODULE,
.name = "rmi4_smbus",
.pm = &rmi_smb_pm,
.resume = rmi_smb_resume,
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v9 2/2] Input: synaptics-rmi4 - add SMBus support
2016-06-24 14:41 ` [PATCH v9 2/2] Input: synaptics-rmi4 - add SMBus support Benjamin Tissoires
2016-06-24 21:47 ` [PATCH] Input: fix platform_no_drv_owner.cocci warnings kbuild test robot
@ 2016-06-24 21:47 ` kbuild test robot
1 sibling, 0 replies; 9+ messages in thread
From: kbuild test robot @ 2016-06-24 21:47 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: kbuild-all, Wolfram Sang, Jean Delvare, Dmitry Torokhov,
Andrew Duggan, Christopher Heiny, linux-i2c, linux-kernel
Hi,
[auto build test WARNING on wsa/i2c/for-next]
[also build test WARNING on v4.7-rc4 next-20160624]
[cannot apply to input/next]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Benjamin-Tissoires/i2c-i801-add-support-of-Host-Notify/20160624-224821
base: https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux i2c/for-next
coccinelle warnings: (new ones prefixed by >>)
>> drivers/input/rmi4/rmi_smbus.c:448:3-8: No need to set .owner here. The core will do it.
Please review and possibly fold the followup patch.
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v9 1/2] i2c: i801: add support of Host Notify
2016-06-24 14:39 [PATCH v9 1/2] i2c: i801: add support of Host Notify Benjamin Tissoires
2016-06-24 14:41 ` [PATCH v9 2/2] Input: synaptics-rmi4 - add SMBus support Benjamin Tissoires
@ 2016-07-05 15:40 ` Wolfram Sang
2016-07-18 14:12 ` Jean Delvare
2 siblings, 0 replies; 9+ messages in thread
From: Wolfram Sang @ 2016-07-05 15:40 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Jean Delvare, Dmitry Torokhov, Andrew Duggan, Christopher Heiny,
linux-i2c, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 771 bytes --]
On Fri, Jun 24, 2016 at 04:39:49PM +0200, 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
>
> Enable the functionality unconditionally and propagate the alert
> on each notification.
>
> With a T440s and a Synaptics touchpad that implements Host Notify, the
> payload data is always 0x0000, so I am not sure if the device actually
> sends the payload or if there is a problem regarding the implementation.
>
> Tested-by: Andrew Duggan <aduggan@synaptics.com>
> Acked-by: Wolfram Sang <wsa@the-dreams.de>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Applied to for-next, thanks!
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v9 1/2] i2c: i801: add support of Host Notify
2016-06-24 14:39 [PATCH v9 1/2] i2c: i801: add support of Host Notify Benjamin Tissoires
2016-06-24 14:41 ` [PATCH v9 2/2] Input: synaptics-rmi4 - add SMBus support Benjamin Tissoires
2016-07-05 15:40 ` [PATCH v9 1/2] i2c: i801: add support of Host Notify Wolfram Sang
@ 2016-07-18 14:12 ` Jean Delvare
2016-07-28 9:44 ` Benjamin Tissoires
2 siblings, 1 reply; 9+ messages in thread
From: Jean Delvare @ 2016-07-18 14:12 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Wolfram Sang, Dmitry Torokhov, Andrew Duggan, Christopher Heiny,
linux-i2c, linux-kernel
Hi Benjamin,
Once again, sorry for the late review.
On Fri, 24 Jun 2016 16:39:49 +0200, 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
>
> Enable the functionality unconditionally and propagate the alert
> on each notification.
>
> With a T440s and a Synaptics touchpad that implements Host Notify, the
> payload data is always 0x0000, so I am not sure if the device actually
> sends the payload or if there is a problem regarding the implementation.
Out of curiosity, does it work on at least one machine?
> Tested-by: Andrew Duggan <aduggan@synaptics.com>
> Acked-by: Wolfram Sang <wsa@the-dreams.de>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
> changes in v2:
> - removed the description of the Slave functionality support in the chip table
> (the table shows what is supported, not what the hardware is capable of)
> - use i2c-smbus to forward the notification
> - remove the fifo, and directly retrieve the address and payload in the worker
> - do not check for Host Notification is the feature is not enabled
> - use inw_p() to read the payload instead of 2 inb_p() calls
> - add /* fall-through */ comment
> - unconditionally enable Host Notify if the hardware supports it (can be
> disabled by the user)
>
> no changes in v3
>
> changes in v4:
> - make use of the new API -> no more worker spawning here
> - solved a race between the access of the Host Notify registers and the actual
> I2C transfers.
>
> changes in v5:
> - added SKL Host Notify support
>
> changes in v6:
> - select I2C_SMBUS in Kconfig to prevent an undefined reference when I2C_I801
> is set to 'Y' while I2C_SMBUS is set to 'M'
>
> no changes in v7
>
> changes in v8:
> - reapplied after http://patchwork.ozlabs.org/patch/632768/ and merged the
> conflict (minor conflict in the struct i801_priv).
> - removed the .resume hook as upstream changed suspend/resume hooks and there
> is no need in the end to re-enable host notify on resume (tested on Lenovo
> t440 and t450).
> - kept Wolfram's Acked-by as the changes were minor
>
> changes in v9:
> - re-add the .resume hook. Looks like the Lenovo T440 sometimes forget to
> re-enable Host Notify on resume, so better have it reset for everybody
>
> drivers/i2c/busses/Kconfig | 1 +
> drivers/i2c/busses/i2c-i801.c | 88 +++++++++++++++++++++++++++++++++++++++++--
> 2 files changed, 86 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index efa3d9b..b4b6cb0 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -91,6 +91,7 @@ config I2C_I801
> tristate "Intel 82801 (ICH/PCH)"
> depends on PCI
> select CHECK_SIGNATURE if X86 && DMI
> + select I2C_SMBUS
> help
> If you say yes to this option, support will be included for the Intel
> 801 family of mainboard I2C interfaces. Specifically, the following
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index b436963..312caed 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -72,6 +72,7 @@
> * Block process call transaction no
> * I2C block read transaction yes (doesn't use the block buffer)
> * Slave mode no
> + * SMBus Host Notify yes
> * Interrupt processing yes
> *
> * See the file Documentation/i2c/busses/i2c-i801 for details.
> @@ -86,6 +87,7 @@
> #include <linux/ioport.h>
> #include <linux/init.h>
> #include <linux/i2c.h>
> +#include <linux/i2c-smbus.h>
> #include <linux/acpi.h>
> #include <linux/io.h>
> #include <linux/dmi.h>
> @@ -113,6 +115,10 @@
> #define SMBPEC(p) (8 + (p)->smba) /* ICH3 and later */
> #define SMBAUXSTS(p) (12 + (p)->smba) /* ICH4 and later */
> #define SMBAUXCTL(p) (13 + (p)->smba) /* ICH4 and later */
> +#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
> @@ -177,6 +183,12 @@
> #define SMBHSTSTS_INTR 0x02
> #define SMBHSTSTS_HOST_BUSY 0x01
>
> +/* Host Notify Status registers bits */
> +#define SMBSLVSTS_HST_NTFY_STS 1
> +
> +/* Host Notify Command registers bits */
> +#define SMBSLVCMD_HST_NTFY_INTREN 0x01
"register" needs no "s" in these 2 comments. Also it would be nice to
stick to hexadecimal for consistency (I know it's not the case
elsewhere in the driver, but let's not add to it.)
> +
> #define STATUS_ERROR_FLAGS (SMBHSTSTS_FAILED | SMBHSTSTS_BUS_ERR | \
> SMBHSTSTS_DEV_ERR)
>
> @@ -252,13 +264,17 @@ struct i801_priv {
> */
> bool acpi_reserved;
> struct mutex acpi_lock;
> + struct smbus_host_notify *host_notify;
> };
>
> +#define SMBHSTNTFY_SIZE 8
This constant isn't used anywhere?
> +
> #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)
> /* Not really a feature, but it's convenient to handle it as such */
> #define FEATURE_IDF (1 << 15)
> #define FEATURE_TCO (1 << 16)
> @@ -269,6 +285,7 @@ static const char *i801_feature_names[] = {
> "Block process call",
> "I2C block read",
> "Interrupt",
> + "SMBus Host Notify",
> };
>
> static unsigned int disable_features;
> @@ -277,7 +294,8 @@ MODULE_PARM_DESC(disable_features, "Disable selected driver features:\n"
> "\t\t 0x01 disable SMBus PEC\n"
> "\t\t 0x02 disable the block buffer\n"
> "\t\t 0x08 disable the I2C block read functionality\n"
> - "\t\t 0x10 don't use interrupts ");
> + "\t\t 0x10 don't use interrupts\n"
> + "\t\t 0x20 disable SMBus Host Notify ");
>
> /* Make sure the SMBus host is ready to start transmitting.
> Return 0 if it is, -EBUSY if it is not. */
> @@ -511,8 +529,23 @@ static void i801_isr_byte_done(struct i801_priv *priv)
> outb_p(SMBHSTSTS_BYTE_DONE, SMBHSTSTS(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));
The ICH9 datasheet I'm looking at says the data is in 2 8-bit
registers. I wonder if a 16-bit read is OK an all chipsets. Well, if it
isn't we'll learn about it someday, I suppose.
> +
> + i2c_handle_smbus_host_notify(priv->host_notify, addr, data);
This can fail, in theory. Maybe this should be handled?
> +
> + /* clear Host Notify bit and return */
> + outb_p(SMBSLVSTS_HST_NTFY_STS, SMBSLVSTS(priv));
> + return IRQ_HANDLED;
> +}
> +
> /*
> - * There are two kinds of interrupts:
> + * There are three kinds of interrupts:
> *
> * 1) i801 signals transaction completion with one of these interrupts:
> * INTR - Success
> @@ -524,6 +557,8 @@ static void i801_isr_byte_done(struct i801_priv *priv)
> *
> * 2) For byte-by-byte (I2C read/write) transactions, one BYTE_DONE interrupt
> * occurs for each byte of a byte-by-byte to prepare the next byte.
> + *
> + * 3) Host Notify interrupts
> */
> static irqreturn_t i801_isr(int irq, void *dev_id)
> {
> @@ -536,6 +571,12 @@ static irqreturn_t i801_isr(int irq, void *dev_id)
> if (!(pcists & SMBPCISTS_INTS))
> return IRQ_NONE;
>
> + if (priv->features & FEATURE_HOST_NOTIFY) {
> + status = inb_p(SMBSLVSTS(priv));
> + if (status & SMBSLVSTS_HST_NTFY_STS)
> + return i801_host_notify_isr(priv);
> + }
> +
> status = inb_p(SMBHSTSTS(priv));
> if (status & SMBHSTSTS_BYTE_DONE)
> i801_isr_byte_done(priv);
> @@ -847,7 +888,28 @@ 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_enable_host_notify(struct i2c_adapter *adapter)
> +{
> + struct i801_priv *priv = i2c_get_adapdata(adapter);
> +
> + if (!(priv->features & FEATURE_HOST_NOTIFY))
> + return -ENOTSUPP;
Why do you return an error here? This forces you to special-case it on
further error handling. You could simply return 0 instead, and save
some tests.
> +
> + if (!priv->host_notify)
> + priv->host_notify = i2c_setup_smbus_host_notify(adapter);
> + if (!priv->host_notify)
> + return -ENOMEM;
Minor optimization: if priv->host_notify was already set, the second
test is not needed.
> +
> + 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));
> +
> + return 0;
> }
>
> static const struct i2c_algorithm smbus_algorithm = {
> @@ -1379,6 +1441,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
> priv->features |= FEATURE_SMBUS_PEC;
> priv->features |= FEATURE_BLOCK_BUFFER;
> priv->features |= FEATURE_TCO;
> + priv->features |= FEATURE_HOST_NOTIFY;
> break;
>
> case PCI_DEVICE_ID_INTEL_PATSBURG_SMBUS_IDF0:
> @@ -1398,6 +1461,8 @@ 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;
> + /* fall through */
> case PCI_DEVICE_ID_INTEL_82801BA_2:
> case PCI_DEVICE_ID_INTEL_82801AB_3:
> case PCI_DEVICE_ID_INTEL_82801AA_3:
> @@ -1507,6 +1572,15 @@ 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_probe_optional_slaves(priv);
> /* We ignore errors - multiplexing is optional */
> i801_add_mux(priv);
> @@ -1553,6 +1627,14 @@ static int i801_suspend(struct device *dev)
>
> 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");
> +
> return 0;
> }
> #endif
--
Jean Delvare
SUSE L3 Support
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v9 1/2] i2c: i801: add support of Host Notify
2016-07-18 14:12 ` Jean Delvare
@ 2016-07-28 9:44 ` Benjamin Tissoires
2016-08-01 13:33 ` Jean Delvare
0 siblings, 1 reply; 9+ messages in thread
From: Benjamin Tissoires @ 2016-07-28 9:44 UTC (permalink / raw)
To: Jean Delvare
Cc: Wolfram Sang, Dmitry Torokhov, Andrew Duggan, Christopher Heiny,
linux-i2c, linux-kernel
Hi Jean,
On Jul 18 2016 or thereabouts, Jean Delvare wrote:
> Hi Benjamin,
>
> Once again, sorry for the late review.
>
> On Fri, 24 Jun 2016 16:39:49 +0200, 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
> >
> > Enable the functionality unconditionally and propagate the alert
> > on each notification.
> >
> > With a T440s and a Synaptics touchpad that implements Host Notify, the
> > payload data is always 0x0000, so I am not sure if the device actually
> > sends the payload or if there is a problem regarding the implementation.
>
> Out of curiosity, does it work on at least one machine?
I have tested the T440, t450 and t460 (3 different generation of Intel
processor), and none seems to be working. The Synaptics touchpad used is
mostly the same, so maybe that's a device issue.
>
> > Tested-by: Andrew Duggan <aduggan@synaptics.com>
> > Acked-by: Wolfram Sang <wsa@the-dreams.de>
> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > ---
> > changes in v2:
> > - removed the description of the Slave functionality support in the chip table
> > (the table shows what is supported, not what the hardware is capable of)
> > - use i2c-smbus to forward the notification
> > - remove the fifo, and directly retrieve the address and payload in the worker
> > - do not check for Host Notification is the feature is not enabled
> > - use inw_p() to read the payload instead of 2 inb_p() calls
> > - add /* fall-through */ comment
> > - unconditionally enable Host Notify if the hardware supports it (can be
> > disabled by the user)
> >
> > no changes in v3
> >
> > changes in v4:
> > - make use of the new API -> no more worker spawning here
> > - solved a race between the access of the Host Notify registers and the actual
> > I2C transfers.
> >
> > changes in v5:
> > - added SKL Host Notify support
> >
> > changes in v6:
> > - select I2C_SMBUS in Kconfig to prevent an undefined reference when I2C_I801
> > is set to 'Y' while I2C_SMBUS is set to 'M'
> >
> > no changes in v7
> >
> > changes in v8:
> > - reapplied after http://patchwork.ozlabs.org/patch/632768/ and merged the
> > conflict (minor conflict in the struct i801_priv).
> > - removed the .resume hook as upstream changed suspend/resume hooks and there
> > is no need in the end to re-enable host notify on resume (tested on Lenovo
> > t440 and t450).
> > - kept Wolfram's Acked-by as the changes were minor
> >
> > changes in v9:
> > - re-add the .resume hook. Looks like the Lenovo T440 sometimes forget to
> > re-enable Host Notify on resume, so better have it reset for everybody
> >
> > drivers/i2c/busses/Kconfig | 1 +
> > drivers/i2c/busses/i2c-i801.c | 88 +++++++++++++++++++++++++++++++++++++++++--
> > 2 files changed, 86 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> > index efa3d9b..b4b6cb0 100644
> > --- a/drivers/i2c/busses/Kconfig
> > +++ b/drivers/i2c/busses/Kconfig
> > @@ -91,6 +91,7 @@ config I2C_I801
> > tristate "Intel 82801 (ICH/PCH)"
> > depends on PCI
> > select CHECK_SIGNATURE if X86 && DMI
> > + select I2C_SMBUS
> > help
> > If you say yes to this option, support will be included for the Intel
> > 801 family of mainboard I2C interfaces. Specifically, the following
> > diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> > index b436963..312caed 100644
> > --- a/drivers/i2c/busses/i2c-i801.c
> > +++ b/drivers/i2c/busses/i2c-i801.c
> > @@ -72,6 +72,7 @@
> > * Block process call transaction no
> > * I2C block read transaction yes (doesn't use the block buffer)
> > * Slave mode no
> > + * SMBus Host Notify yes
> > * Interrupt processing yes
> > *
> > * See the file Documentation/i2c/busses/i2c-i801 for details.
> > @@ -86,6 +87,7 @@
> > #include <linux/ioport.h>
> > #include <linux/init.h>
> > #include <linux/i2c.h>
> > +#include <linux/i2c-smbus.h>
> > #include <linux/acpi.h>
> > #include <linux/io.h>
> > #include <linux/dmi.h>
> > @@ -113,6 +115,10 @@
> > #define SMBPEC(p) (8 + (p)->smba) /* ICH3 and later */
> > #define SMBAUXSTS(p) (12 + (p)->smba) /* ICH4 and later */
> > #define SMBAUXCTL(p) (13 + (p)->smba) /* ICH4 and later */
> > +#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
> > @@ -177,6 +183,12 @@
> > #define SMBHSTSTS_INTR 0x02
> > #define SMBHSTSTS_HOST_BUSY 0x01
> >
> > +/* Host Notify Status registers bits */
> > +#define SMBSLVSTS_HST_NTFY_STS 1
> > +
> > +/* Host Notify Command registers bits */
> > +#define SMBSLVCMD_HST_NTFY_INTREN 0x01
>
> "register" needs no "s" in these 2 comments. Also it would be nice to
> stick to hexadecimal for consistency (I know it's not the case
> elsewhere in the driver, but let's not add to it.)
>
> > +
> > #define STATUS_ERROR_FLAGS (SMBHSTSTS_FAILED | SMBHSTSTS_BUS_ERR | \
> > SMBHSTSTS_DEV_ERR)
> >
> > @@ -252,13 +264,17 @@ struct i801_priv {
> > */
> > bool acpi_reserved;
> > struct mutex acpi_lock;
> > + struct smbus_host_notify *host_notify;
> > };
> >
> > +#define SMBHSTNTFY_SIZE 8
>
> This constant isn't used anywhere?
>
> > +
> > #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)
> > /* Not really a feature, but it's convenient to handle it as such */
> > #define FEATURE_IDF (1 << 15)
> > #define FEATURE_TCO (1 << 16)
> > @@ -269,6 +285,7 @@ static const char *i801_feature_names[] = {
> > "Block process call",
> > "I2C block read",
> > "Interrupt",
> > + "SMBus Host Notify",
> > };
> >
> > static unsigned int disable_features;
> > @@ -277,7 +294,8 @@ MODULE_PARM_DESC(disable_features, "Disable selected driver features:\n"
> > "\t\t 0x01 disable SMBus PEC\n"
> > "\t\t 0x02 disable the block buffer\n"
> > "\t\t 0x08 disable the I2C block read functionality\n"
> > - "\t\t 0x10 don't use interrupts ");
> > + "\t\t 0x10 don't use interrupts\n"
> > + "\t\t 0x20 disable SMBus Host Notify ");
> >
> > /* Make sure the SMBus host is ready to start transmitting.
> > Return 0 if it is, -EBUSY if it is not. */
> > @@ -511,8 +529,23 @@ static void i801_isr_byte_done(struct i801_priv *priv)
> > outb_p(SMBHSTSTS_BYTE_DONE, SMBHSTSTS(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));
>
> The ICH9 datasheet I'm looking at says the data is in 2 8-bit
> registers. I wonder if a 16-bit read is OK an all chipsets. Well, if it
> isn't we'll learn about it someday, I suppose.
Well, yes, I suppose too :)
I am about to send a series to address all of your remarks. Thanks for
the review!
Cheers,
Benjamin
>
> > +
> > + i2c_handle_smbus_host_notify(priv->host_notify, addr, data);
>
> This can fail, in theory. Maybe this should be handled?
>
> > +
> > + /* clear Host Notify bit and return */
> > + outb_p(SMBSLVSTS_HST_NTFY_STS, SMBSLVSTS(priv));
> > + return IRQ_HANDLED;
> > +}
> > +
> > /*
> > - * There are two kinds of interrupts:
> > + * There are three kinds of interrupts:
> > *
> > * 1) i801 signals transaction completion with one of these interrupts:
> > * INTR - Success
> > @@ -524,6 +557,8 @@ static void i801_isr_byte_done(struct i801_priv *priv)
> > *
> > * 2) For byte-by-byte (I2C read/write) transactions, one BYTE_DONE interrupt
> > * occurs for each byte of a byte-by-byte to prepare the next byte.
> > + *
> > + * 3) Host Notify interrupts
> > */
> > static irqreturn_t i801_isr(int irq, void *dev_id)
> > {
> > @@ -536,6 +571,12 @@ static irqreturn_t i801_isr(int irq, void *dev_id)
> > if (!(pcists & SMBPCISTS_INTS))
> > return IRQ_NONE;
> >
> > + if (priv->features & FEATURE_HOST_NOTIFY) {
> > + status = inb_p(SMBSLVSTS(priv));
> > + if (status & SMBSLVSTS_HST_NTFY_STS)
> > + return i801_host_notify_isr(priv);
> > + }
> > +
> > status = inb_p(SMBHSTSTS(priv));
> > if (status & SMBHSTSTS_BYTE_DONE)
> > i801_isr_byte_done(priv);
> > @@ -847,7 +888,28 @@ 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_enable_host_notify(struct i2c_adapter *adapter)
> > +{
> > + struct i801_priv *priv = i2c_get_adapdata(adapter);
> > +
> > + if (!(priv->features & FEATURE_HOST_NOTIFY))
> > + return -ENOTSUPP;
>
> Why do you return an error here? This forces you to special-case it on
> further error handling. You could simply return 0 instead, and save
> some tests.
>
> > +
> > + if (!priv->host_notify)
> > + priv->host_notify = i2c_setup_smbus_host_notify(adapter);
> > + if (!priv->host_notify)
> > + return -ENOMEM;
>
> Minor optimization: if priv->host_notify was already set, the second
> test is not needed.
>
> > +
> > + 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));
> > +
> > + return 0;
> > }
> >
> > static const struct i2c_algorithm smbus_algorithm = {
> > @@ -1379,6 +1441,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
> > priv->features |= FEATURE_SMBUS_PEC;
> > priv->features |= FEATURE_BLOCK_BUFFER;
> > priv->features |= FEATURE_TCO;
> > + priv->features |= FEATURE_HOST_NOTIFY;
> > break;
> >
> > case PCI_DEVICE_ID_INTEL_PATSBURG_SMBUS_IDF0:
> > @@ -1398,6 +1461,8 @@ 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;
> > + /* fall through */
> > case PCI_DEVICE_ID_INTEL_82801BA_2:
> > case PCI_DEVICE_ID_INTEL_82801AB_3:
> > case PCI_DEVICE_ID_INTEL_82801AA_3:
> > @@ -1507,6 +1572,15 @@ 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_probe_optional_slaves(priv);
> > /* We ignore errors - multiplexing is optional */
> > i801_add_mux(priv);
> > @@ -1553,6 +1627,14 @@ static int i801_suspend(struct device *dev)
> >
> > 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");
> > +
> > return 0;
> > }
> > #endif
>
>
> --
> Jean Delvare
> SUSE L3 Support
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v9 1/2] i2c: i801: add support of Host Notify
2016-07-28 9:44 ` Benjamin Tissoires
@ 2016-08-01 13:33 ` Jean Delvare
2016-08-19 13:19 ` Benjamin Tissoires
0 siblings, 1 reply; 9+ messages in thread
From: Jean Delvare @ 2016-08-01 13:33 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Wolfram Sang, Dmitry Torokhov, Andrew Duggan, Christopher Heiny,
linux-i2c, linux-kernel
On Thu, 28 Jul 2016 11:44:57 +0200, Benjamin Tissoires wrote:
> On Jul 18 2016 or thereabouts, Jean Delvare wrote:
> > On Fri, 24 Jun 2016 16:39:49 +0200, 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
> > >
> > > Enable the functionality unconditionally and propagate the alert
> > > on each notification.
> > >
> > > With a T440s and a Synaptics touchpad that implements Host Notify, the
> > > payload data is always 0x0000, so I am not sure if the device actually
> > > sends the payload or if there is a problem regarding the implementation.
> >
> > Out of curiosity, does it work on at least one machine?
>
> I have tested the T440, t450 and t460 (3 different generation of Intel
> processor), and none seems to be working. The Synaptics touchpad used is
> mostly the same, so maybe that's a device issue.
May I ask why you are pushing the code upstream then, if it has no
known (working) user?
--
Jean Delvare
SUSE L3 Support
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v9 1/2] i2c: i801: add support of Host Notify
2016-08-01 13:33 ` Jean Delvare
@ 2016-08-19 13:19 ` Benjamin Tissoires
0 siblings, 0 replies; 9+ messages in thread
From: Benjamin Tissoires @ 2016-08-19 13:19 UTC (permalink / raw)
To: Jean Delvare
Cc: Wolfram Sang, Dmitry Torokhov, Andrew Duggan, Christopher Heiny,
linux-i2c, linux-kernel
On Aug 01 2016 or thereabouts, Jean Delvare wrote:
> On Thu, 28 Jul 2016 11:44:57 +0200, Benjamin Tissoires wrote:
> > On Jul 18 2016 or thereabouts, Jean Delvare wrote:
> > > On Fri, 24 Jun 2016 16:39:49 +0200, 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
> > > >
> > > > Enable the functionality unconditionally and propagate the alert
> > > > on each notification.
> > > >
> > > > With a T440s and a Synaptics touchpad that implements Host Notify, the
> > > > payload data is always 0x0000, so I am not sure if the device actually
> > > > sends the payload or if there is a problem regarding the implementation.
> > >
> > > Out of curiosity, does it work on at least one machine?
> >
> > I have tested the T440, t450 and t460 (3 different generation of Intel
> > processor), and none seems to be working. The Synaptics touchpad used is
> > mostly the same, so maybe that's a device issue.
>
> May I ask why you are pushing the code upstream then, if it has no
> known (working) user?
>
K. Then I guess I'll just have to remove these reads from the driver.
Cheers,
Benjamin
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-08-19 13:19 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-24 14:39 [PATCH v9 1/2] i2c: i801: add support of Host Notify Benjamin Tissoires
2016-06-24 14:41 ` [PATCH v9 2/2] Input: synaptics-rmi4 - add SMBus support Benjamin Tissoires
2016-06-24 21:47 ` [PATCH] Input: fix platform_no_drv_owner.cocci warnings kbuild test robot
2016-06-24 21:47 ` [PATCH v9 2/2] Input: synaptics-rmi4 - add SMBus support kbuild test robot
2016-07-05 15:40 ` [PATCH v9 1/2] i2c: i801: add support of Host Notify Wolfram Sang
2016-07-18 14:12 ` Jean Delvare
2016-07-28 9:44 ` Benjamin Tissoires
2016-08-01 13:33 ` Jean Delvare
2016-08-19 13:19 ` Benjamin Tissoires
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).