[PATCH v8 0/4] i2c-smbus: add support for HOST NOTIFY

[PATCH v8 0/4] i2c-smbus: add support for HOST NOTIFY

[Benjamin Tissoires]

Hi,

and here comes the v8 of Host Notify.

To minimize the merge conflicts, I first applied the following 2 patches:
http://patchwork.ozlabs.org/patch/632768/
http://patchwork.ozlabs.org/patch/626051/

The series Corey sent (http://patchwork.ozlabs.org/patch/627567/ and the 9
after) does not conflict with the i2c-i801 patch (3/4).

I haven't seen any other pending i801 patches in the recent history, but please
tell me if you need extra eyes on some others.

I guess you won't take 4/4 in the I2C tree (unless Dmitry gives his ACK), but
I felt it was important to have one driver submitted with the use of this "new"
feature.

Cheers,
Benjamin

Benjamin Tissoires (4):
  i2c: add a protocol parameter to the alert callback
  i2c-smbus: add SMBus Host Notify support
  i2c: i801: add support of Host Notify
  Input: synaptics-rmi4 - add SMBus support

 Documentation/i2c/smbus-protocol |   6 +
 drivers/char/ipmi/ipmi_ssif.c    |   6 +-
 drivers/hwmon/lm90.c             |   6 +-
 drivers/i2c/busses/Kconfig       |   1 +
 drivers/i2c/busses/i2c-i801.c    |  79 ++++++-
 drivers/i2c/i2c-smbus.c          | 112 +++++++++-
 drivers/input/rmi4/Kconfig       |  12 +
 drivers/input/rmi4/Makefile      |   1 +
 drivers/input/rmi4/rmi_bus.h     |  12 +
 drivers/input/rmi4/rmi_smbus.c   | 470 +++++++++++++++++++++++++++++++++++++++
 include/linux/i2c-smbus.h        |  44 ++++
 include/linux/i2c.h              |  10 +-
 include/uapi/linux/i2c.h         |   1 +
 13 files changed, 750 insertions(+), 10 deletions(-)
 create mode 100644 drivers/input/rmi4/rmi_smbus.c

-- 
2.5.0

[PATCH v8 1/4] i2c: add a protocol parameter to the alert callback

[Benjamin Tissoires]

.alert() is meant to be generic, but there is currently no way
for the device driver to know which protocol generated the alert.
Add a parameter in .alert() to help the device driver to understand
what is given in data.

This patch is required to have the support of SMBus Host Notify protocol
through .alert().

Tested-by: Andrew Duggan <aduggan@synaptics.com>
For hwmon:
Acked-by: Guenter Roeck <linux@roeck-us.net>
For IPMI:
Acked-by: Corey Minyard <cminyard@mvista.com>
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
new in v2

changes in v3:
- added also lm90.c to support the new API
 
no changes in v4
 
no changes in v5

changes in v6:
- made sure lm90 also checks for the type of alert first

no changes in v7

changes in v8:
- fixed Andrew's email address

 drivers/char/ipmi/ipmi_ssif.c | 6 +++++-
 drivers/hwmon/lm90.c          | 6 +++++-
 drivers/i2c/i2c-smbus.c       | 3 ++-
 include/linux/i2c.h           | 7 ++++++-
 4 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
index 097c868..5673fff 100644
--- a/drivers/char/ipmi/ipmi_ssif.c
+++ b/drivers/char/ipmi/ipmi_ssif.c
@@ -568,12 +568,16 @@ static void retry_timeout(unsigned long data)
 }
 
 
-static void ssif_alert(struct i2c_client *client, unsigned int data)
+static void ssif_alert(struct i2c_client *client, enum i2c_alert_protocol type,
+		       unsigned int data)
 {
 	struct ssif_info *ssif_info = i2c_get_clientdata(client);
 	unsigned long oflags, *flags;
 	bool do_get = false;
 
+	if (type != I2C_PROTOCOL_SMBUS_ALERT)
+		return;
+
 	ssif_inc_stat(ssif_info, alerts);
 
 	flags = ipmi_ssif_lock_cond(ssif_info, &oflags);
diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index c9ff08d..a00fd38 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -1624,10 +1624,14 @@ static int lm90_remove(struct i2c_client *client)
 	return 0;
 }
 
-static void lm90_alert(struct i2c_client *client, unsigned int flag)
+static void lm90_alert(struct i2c_client *client, enum i2c_alert_protocol type,
+		       unsigned int flag)
 {
 	u16 alarms;
 
+	if (type != I2C_PROTOCOL_SMBUS_ALERT)
+		return;
+
 	if (lm90_is_tripped(client, &alarms)) {
 		/*
 		 * Disable ALERT# output, because these chips don't implement
diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c
index abb55d3..3b6765a 100644
--- a/drivers/i2c/i2c-smbus.c
+++ b/drivers/i2c/i2c-smbus.c
@@ -56,7 +56,8 @@ static int smbus_do_alert(struct device *dev, void *addrp)
 	if (client->dev.driver) {
 		driver = to_i2c_driver(client->dev.driver);
 		if (driver->alert)
-			driver->alert(client, data->flag);
+			driver->alert(client, I2C_PROTOCOL_SMBUS_ALERT,
+				      data->flag);
 		else
 			dev_warn(&client->dev, "no driver alert()!\n");
 	} else
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 96a25ae..29a2d3b7c 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -126,6 +126,10 @@ i2c_smbus_read_i2c_block_data_or_emulated(const struct i2c_client *client,
 					  u8 command, u8 length, u8 *values);
 #endif /* I2C */
 
+enum i2c_alert_protocol {
+	I2C_PROTOCOL_SMBUS_ALERT,
+};
+
 /**
  * struct i2c_driver - represent an I2C device driver
  * @class: What kind of i2c device we instantiate (for detect)
@@ -181,7 +185,8 @@ struct i2c_driver {
 	 * For the SMBus alert protocol, there is a single bit of data passed
 	 * as the alert response's low bit ("event flag").
 	 */
-	void (*alert)(struct i2c_client *, unsigned int data);
+	void (*alert)(struct i2c_client *, enum i2c_alert_protocol protocol,
+		      unsigned int data);
 
 	/* a ioctl like command that can be used to perform specific functions
 	 * with the device.
-- 
2.5.0

Re: [PATCH v8 1/4] i2c: add a protocol parameter to the alert callback

[Wolfram Sang]

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

On Thu, Jun 09, 2016 at 04:53:47PM +0200, Benjamin Tissoires wrote:
> .alert() is meant to be generic, but there is currently no way
> for the device driver to know which protocol generated the alert.
> Add a parameter in .alert() to help the device driver to understand
> what is given in data.
> 
> This patch is required to have the support of SMBus Host Notify protocol
> through .alert().
> 
> Tested-by: Andrew Duggan <aduggan@synaptics.com>
> For hwmon:
> Acked-by: Guenter Roeck <linux@roeck-us.net>
> For IPMI:
> Acked-by: Corey Minyard <cminyard@mvista.com>
> 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 --]

[PATCH v8 2/4] i2c-smbus: add SMBus Host Notify support

[Benjamin Tissoires]

SMBus Host Notify allows a slave device to act as a master on a bus to
notify the host of an interrupt. On Intel chipsets, the functionality
is directly implemented in the firmware. We just need to export a
function to call .alert() on the proper device driver.

i2c_handle_smbus_host_notify() behaves like i2c_handle_smbus_alert().
When called, it schedules a task that will be able to sleep to go through
the list of devices attached to the adapter.

The current implementation allows one Host Notification to be scheduled
while an other is running.

Tested-by: Andrew Duggan <aduggan@synaptics.com>
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
changes in v2:
- do the actual processing of finding the device in i2c-smbus.c
- remove the i2c-core implementations
- remove the manual toggle of SMBus Host Notify

no changes in v3

changes in v4:
- schedule the worker in i2c_handle_smbus_host_notify() -> it can now be called
  in an interrupt context.
- introduce i2c_setup_smbus_host_notify()

no changes in v5

changes in v6:
- fix the parameters of the inlined functions when the module is not compiled

changes in v7:
- fixed typo in i2c_handle_smbus_host_notify() warning message ('.' and '\n'
  were inverted)

changes in v8:
- Documentation updated to reflect the code
- s/-EFAULT/-EBUSY
- use of IS_ENABLED(CONFIG_I2C_SMBUS) instead of a custom made test :)

 Documentation/i2c/smbus-protocol |   6 +++
 drivers/i2c/i2c-smbus.c          | 113 +++++++++++++++++++++++++++++++++++++--
 include/linux/i2c-smbus.h        |  44 +++++++++++++++
 include/linux/i2c.h              |   3 ++
 include/uapi/linux/i2c.h         |   1 +
 5 files changed, 162 insertions(+), 5 deletions(-)

diff --git a/Documentation/i2c/smbus-protocol b/Documentation/i2c/smbus-protocol
index 6012b12..14d4ec1 100644
--- a/Documentation/i2c/smbus-protocol
+++ b/Documentation/i2c/smbus-protocol
@@ -199,6 +199,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.
+
 
 Packet Error Checking (PEC)
 ===========================
diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c
index 3b6765a..f574995 100644
--- a/drivers/i2c/i2c-smbus.c
+++ b/drivers/i2c/i2c-smbus.c
@@ -33,7 +33,8 @@ struct i2c_smbus_alert {
 
 struct alert_data {
 	unsigned short		addr;
-	u8			flag:1;
+	enum i2c_alert_protocol	type;
+	unsigned int		data;
 };
 
 /* If this is the alerting device, notify its driver */
@@ -56,8 +57,7 @@ static int smbus_do_alert(struct device *dev, void *addrp)
 	if (client->dev.driver) {
 		driver = to_i2c_driver(client->dev.driver);
 		if (driver->alert)
-			driver->alert(client, I2C_PROTOCOL_SMBUS_ALERT,
-				      data->flag);
+			driver->alert(client, data->type, data->data);
 		else
 			dev_warn(&client->dev, "no driver alert()!\n");
 	} else
@@ -97,8 +97,9 @@ static void smbus_alert(struct work_struct *work)
 		if (status < 0)
 			break;
 
-		data.flag = status & 1;
+		data.data = status & 1;
 		data.addr = status >> 1;
+		data.type = I2C_PROTOCOL_SMBUS_ALERT;
 
 		if (data.addr == prev_addr) {
 			dev_warn(&ara->dev, "Duplicate SMBALERT# from dev "
@@ -106,7 +107,7 @@ static void smbus_alert(struct work_struct *work)
 			break;
 		}
 		dev_dbg(&ara->dev, "SMBALERT# from dev 0x%02x, flag %d\n",
-			data.addr, data.flag);
+			data.addr, data.data);
 
 		/* Notify driver for the device which issued the alert */
 		device_for_each_child(&ara->adapter->dev, &data,
@@ -240,6 +241,108 @@ 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
+ * @data: the Host Notify data which contains the payload and address of the
+ * client
+ * 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 8f1b086..4ac95bb 100644
--- a/include/linux/i2c-smbus.h
+++ b/include/linux/i2c-smbus.h
@@ -23,6 +23,8 @@
 #define _LINUX_I2C_SMBUS_H
 
 #include <linux/i2c.h>
+#include <linux/spinlock.h>
+#include <linux/workqueue.h>
 
 
 /**
@@ -48,4 +50,46 @@ 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;
+};
+
+#if IS_ENABLED(CONFIG_I2C_SMBUS)
+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);
+#else
+static inline struct smbus_host_notify *
+i2c_setup_smbus_host_notify(struct i2c_adapter *adap)
+{
+	return NULL;
+}
+
+static inline int
+i2c_handle_smbus_host_notify(struct smbus_host_notify *host_notify,
+			     unsigned short addr, unsigned int data)
+{
+	return 0;
+}
+#endif /* I2C_SMBUS */
+
 #endif /* _LINUX_I2C_SMBUS_H */
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 29a2d3b7c..e8ccdfe 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -128,6 +128,7 @@ i2c_smbus_read_i2c_block_data_or_emulated(const struct i2c_client *client,
 
 enum i2c_alert_protocol {
 	I2C_PROTOCOL_SMBUS_ALERT,
+	I2C_PROTOCOL_SMBUS_HOST_NOTIFY,
 };
 
 /**
@@ -184,6 +185,8 @@ struct i2c_driver {
 	 * The format and meaning of the data value depends on the protocol.
 	 * For the SMBus alert protocol, there is a single bit of data passed
 	 * as the alert response's low bit ("event flag").
+	 * For the SMBus Host Notify protocol, the data corresponds to the
+	 * 16-bit payload data reported by the slave device acting as master.
 	 */
 	void (*alert)(struct i2c_client *, enum i2c_alert_protocol protocol,
 		      unsigned int data);
diff --git a/include/uapi/linux/i2c.h b/include/uapi/linux/i2c.h
index adcbef4..009e27b 100644
--- a/include/uapi/linux/i2c.h
+++ b/include/uapi/linux/i2c.h
@@ -102,6 +102,7 @@ struct i2c_msg {
 #define I2C_FUNC_SMBUS_WRITE_BLOCK_DATA 0x02000000
 #define I2C_FUNC_SMBUS_READ_I2C_BLOCK	0x04000000 /* I2C-like block xfer  */
 #define I2C_FUNC_SMBUS_WRITE_I2C_BLOCK	0x08000000 /* w/ 1-byte reg. addr. */
+#define I2C_FUNC_SMBUS_HOST_NOTIFY	0x10000000
 
 #define I2C_FUNC_SMBUS_BYTE		(I2C_FUNC_SMBUS_READ_BYTE | \
 					 I2C_FUNC_SMBUS_WRITE_BYTE)
-- 
2.5.0

Re: [PATCH v8 2/4] i2c-smbus: add SMBus Host Notify support

[Wolfram Sang]

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

On Thu, Jun 09, 2016 at 04:53:48PM +0200, Benjamin Tissoires wrote:
> SMBus Host Notify allows a slave device to act as a master on a bus to
> notify the host of an interrupt. On Intel chipsets, the functionality
> is directly implemented in the firmware. We just need to export a
> function to call .alert() on the proper device driver.
> 
> i2c_handle_smbus_host_notify() behaves like i2c_handle_smbus_alert().
> When called, it schedules a task that will be able to sleep to go through
> the list of devices attached to the adapter.
> 
> The current implementation allows one Host Notification to be scheduled
> while an other is running.
> 
> Tested-by: Andrew Duggan <aduggan@synaptics.com>
> 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 --]

Re: [PATCH v8 2/4] i2c-smbus: add SMBus Host Notify support

[Jean Delvare]

[As it doesn't look like this message was delivered, I am sending it
again. I apologize if this is a duplicate for some of you.]

Hi Benjamin, Wolfram,

Sorry for being late to the party. I finally found some time to look at
the patches. Looks good overall, with just two minor comments:

On jeu., 2016-06-09 at 16:53 +0200, Benjamin Tissoires wrote:
> SMBus Host Notify allows a slave device to act as a master on a bus to
> notify the host of an interrupt. On Intel chipsets, the functionality
> is directly implemented in the firmware. We just need to export a
> function to call .alert() on the proper device driver.
> 
> i2c_handle_smbus_host_notify() behaves like i2c_handle_smbus_alert().
> When called, it schedules a task that will be able to sleep to go through
> the list of devices attached to the adapter.
> 
> The current implementation allows one Host Notification to be scheduled
> while an other is running.
> 
> Tested-by: Andrew Duggan <aduggan@synaptics.com>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
> (...)
> diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c
> index 3b6765a..f574995 100644
> --- a/drivers/i2c/i2c-smbus.c
> +++ b/drivers/i2c/i2c-smbus.c
> (...)
> +/**
> + * 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
> + * @data: the Host Notify data which contains the payload and address of the
> + * client

Doesn't look correct. The data parameter doesn't contain the address,
that would be in the (undocumented) address parameter. I'll send a
patch.

> diff --git a/include/linux/i2c-smbus.h b/include/linux/i2c-smbus.h
> index 8f1b086..4ac95bb 100644
> --- a/include/linux/i2c-smbus.h
> +++ b/include/linux/i2c-smbus.h
> (...)
> +#if IS_ENABLED(CONFIG_I2C_SMBUS)
> +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);
> +#else
> +static inline struct smbus_host_notify *
> +i2c_setup_smbus_host_notify(struct i2c_adapter *adap)
> +{
> +	return NULL;
> +}
> +
> +static inline int
> +i2c_handle_smbus_host_notify(struct smbus_host_notify *host_notify,
> +			     unsigned short addr, unsigned int data)
> +{
> +	return 0;
> +}
> +#endif /* I2C_SMBUS */

You provide stubs for SMBus Host Notify support if CONFIG_I2C_SMBUS is
not selected. There are no such stubs for SMBus Alert support, for which
I assumed drivers would select I2C_SMBUS if they have support. Which is
what you are actually doing for i2c-i801 in a latter patch.

Is there any reason for this difference? For consistency I'd rather
provide stubs for all or none. My preference being for none, unless you
have a use case which requires them.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH v8 2/4] i2c-smbus: add SMBus Host Notify support

[Benjamin Tissoires]

On Jul 18 2016 or thereabouts, Jean Delvare wrote:
> [As it doesn't look like this message was delivered, I am sending it
> again. I apologize if this is a duplicate for some of you.]
> 
> Hi Benjamin, Wolfram,
> 
> Sorry for being late to the party. I finally found some time to look at
> the patches. Looks good overall, with just two minor comments:
> 
> On jeu., 2016-06-09 at 16:53 +0200, Benjamin Tissoires wrote:
> > SMBus Host Notify allows a slave device to act as a master on a bus to
> > notify the host of an interrupt. On Intel chipsets, the functionality
> > is directly implemented in the firmware. We just need to export a
> > function to call .alert() on the proper device driver.
> > 
> > i2c_handle_smbus_host_notify() behaves like i2c_handle_smbus_alert().
> > When called, it schedules a task that will be able to sleep to go through
> > the list of devices attached to the adapter.
> > 
> > The current implementation allows one Host Notification to be scheduled
> > while an other is running.
> > 
> > Tested-by: Andrew Duggan <aduggan@synaptics.com>
> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > ---
> > (...)
> > diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c
> > index 3b6765a..f574995 100644
> > --- a/drivers/i2c/i2c-smbus.c
> > +++ b/drivers/i2c/i2c-smbus.c
> > (...)
> > +/**
> > + * 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
> > + * @data: the Host Notify data which contains the payload and address of the
> > + * client
> 
> Doesn't look correct. The data parameter doesn't contain the address,
> that would be in the (undocumented) address parameter. I'll send a
> patch.

Thanks for the fixup patch already :)

> 
> > diff --git a/include/linux/i2c-smbus.h b/include/linux/i2c-smbus.h
> > index 8f1b086..4ac95bb 100644
> > --- a/include/linux/i2c-smbus.h
> > +++ b/include/linux/i2c-smbus.h
> > (...)
> > +#if IS_ENABLED(CONFIG_I2C_SMBUS)
> > +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);
> > +#else
> > +static inline struct smbus_host_notify *
> > +i2c_setup_smbus_host_notify(struct i2c_adapter *adap)
> > +{
> > +	return NULL;
> > +}
> > +
> > +static inline int
> > +i2c_handle_smbus_host_notify(struct smbus_host_notify *host_notify,
> > +			     unsigned short addr, unsigned int data)
> > +{
> > +	return 0;
> > +}
> > +#endif /* I2C_SMBUS */
> 
> You provide stubs for SMBus Host Notify support if CONFIG_I2C_SMBUS is
> not selected. There are no such stubs for SMBus Alert support, for which
> I assumed drivers would select I2C_SMBUS if they have support. Which is
> what you are actually doing for i2c-i801 in a latter patch.
> 
> Is there any reason for this difference? For consistency I'd rather
> provide stubs for all or none. My preference being for none, unless you
> have a use case which requires them.

Looks like you are right. There is no need for the stubs and they can be
dropped. I think I had them in the first place for a previous
implementation, and they just stayed here.

Given that you already sent a few cleanup patches, do you want to send
this fix also, or do you expect me to send it? (I don't think there will
be a conflict, so either is fine).


Cheers,
Benjamin

> 
> -- 
> Jean Delvare
> SUSE L3 Support

Re: [PATCH v8 2/4] i2c-smbus: add SMBus Host Notify support

[Jean Delvare]

On Mon, 18 Jul 2016 17:59:02 +0200, Benjamin Tissoires wrote:
> On Jul 18 2016 or thereabouts, Jean Delvare wrote:
> > You provide stubs for SMBus Host Notify support if CONFIG_I2C_SMBUS is
> > not selected. There are no such stubs for SMBus Alert support, for which
> > I assumed drivers would select I2C_SMBUS if they have support. Which is
> > what you are actually doing for i2c-i801 in a latter patch.
> > 
> > Is there any reason for this difference? For consistency I'd rather
> > provide stubs for all or none. My preference being for none, unless you
> > have a use case which requires them.
> 
> Looks like you are right. There is no need for the stubs and they can be
> dropped. I think I had them in the first place for a previous
> implementation, and they just stayed here.
> 
> Given that you already sent a few cleanup patches, do you want to send
> this fix also, or do you expect me to send it? (I don't think there will
> be a conflict, so either is fine).

I can do it, I just wanted to make sure it was OK with you first.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH v8 2/4] i2c-smbus: add SMBus Host Notify support

[Jean Delvare]

Hi Benjamin, Wolfram,

Now that I have reviewed the i2c-i801 part of the implementation, I'm
wondering...

On Thu,  9 Jun 2016 16:53:48 +0200, Benjamin Tissoires wrote:
> +/**
> + * 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);

Here we initialize a workqueue.

> +
> +	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
> + * @data: the Host Notify data which contains the payload and address of the
> + * client
> + * 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);

And here we use it.

> +}
> +EXPORT_SYMBOL_GPL(i2c_handle_smbus_host_notify);

But what happens on i2c_adapter removal? What prevents the following
sequence from happening?

1* A Host Notify event happens.
2* The event is handled and queued by i2c_handle_smbus_host_notify().
3* Someone tears down the underlying i2c_adapter (for example "rmmod
   i2c-i801".)
4* The workqueue is processed, accessing memory which has already been
   freed.

Of course it would be back luck, but that's pretty much the definition
of a race condition ;-)

To be on the safe side, don't we need a teardown function in i2c-smbus,
that could be called before i2c_del_adapter, which would remove the
host notify handle and flush the workqueue?

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH v8 2/4] i2c-smbus: add SMBus Host Notify support

[Benjamin Tissoires]

On Jul 18 2016 or thereabouts, Jean Delvare wrote:
> Hi Benjamin, Wolfram,
> 
> Now that I have reviewed the i2c-i801 part of the implementation, I'm
> wondering...
> 
> On Thu,  9 Jun 2016 16:53:48 +0200, Benjamin Tissoires wrote:
> > +/**
> > + * 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);
> 
> Here we initialize a workqueue.
> 
> > +
> > +	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
> > + * @data: the Host Notify data which contains the payload and address of the
> > + * client
> > + * 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);
> 
> And here we use it.
> 
> > +}
> > +EXPORT_SYMBOL_GPL(i2c_handle_smbus_host_notify);
> 
> But what happens on i2c_adapter removal? What prevents the following
> sequence from happening?
> 
> 1* A Host Notify event happens.
> 2* The event is handled and queued by i2c_handle_smbus_host_notify().
> 3* Someone tears down the underlying i2c_adapter (for example "rmmod
>    i2c-i801".)
> 4* The workqueue is processed, accessing memory which has already been
>    freed.
> 
> Of course it would be back luck, but that's pretty much the definition
> of a race condition ;-)

Yes, you are right :(
Sorry for not doing things properly :/

> 
> To be on the safe side, don't we need a teardown function in i2c-smbus,
> that could be called before i2c_del_adapter, which would remove the
> host notify handle and flush the workqueue?

I was thinking at adding a devm action on the release of the struct
smbus_host_notify, but it's actually a bad idea because some other
resources (children moslty) might already be released when the devres
action will be called.

I think it might be easier to add a i2c_remove_host_notify() (or such)
which would make sure we call the cancel_work_sync() function. It would
be the responsibility of the caller to call it once
i2c_setup_smbus_host_notify() has been called. I'd say it has the
advantage of not adding any hidden data in the adapter to the cost of a
small pain in the adapter driver.

Cheers,
Benjamin

> 
> -- 
> Jean Delvare
> SUSE L3 Support

Re: [PATCH v8 2/4] i2c-smbus: add SMBus Host Notify support

[Jean Delvare]

On Mon, 18 Jul 2016 18:35:19 +0200, Benjamin Tissoires wrote:
> On Jul 18 2016 or thereabouts, Jean Delvare wrote:
> > But what happens on i2c_adapter removal? What prevents the following
> > sequence from happening?
> > 
> > 1* A Host Notify event happens.
> > 2* The event is handled and queued by i2c_handle_smbus_host_notify().
> > 3* Someone tears down the underlying i2c_adapter (for example "rmmod
> >    i2c-i801".)
> > 4* The workqueue is processed, accessing memory which has already been
> >    freed.
> > 
> > Of course it would be back luck, but that's pretty much the definition
> > of a race condition ;-)
> 
> Yes, you are right :(
> Sorry for not doing things properly :/

No worry. Bugs happen everywhere, we find them and fix them. That's
part of the process. If we only submit patches which we are 100%
certain are perfect, we never submit anything. I know something about
that...

> > To be on the safe side, don't we need a teardown function in i2c-smbus,
> > that could be called before i2c_del_adapter, which would remove the
> > host notify handle and flush the workqueue?
> 
> I was thinking at adding a devm action on the release of the struct
> smbus_host_notify, but it's actually a bad idea because some other
> resources (children moslty) might already be released when the devres
> action will be called.
> 
> I think it might be easier to add a i2c_remove_host_notify() (or such)
> which would make sure we call the cancel_work_sync() function. It would
> be the responsibility of the caller to call it once
> i2c_setup_smbus_host_notify() has been called. I'd say it has the
> advantage of not adding any hidden data in the adapter to the cost of a
> small pain in the adapter driver.

That's what I had in mind as well, but I'm open to any option which
solves the problem really.

-- 
Jean Delvare
SUSE L3 Support

[PATCH v8 3/4] i2c: i801: add support of Host Notify

[Benjamin Tissoires]

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

 drivers/i2c/busses/Kconfig    |  1 +
 drivers/i2c/busses/i2c-i801.c | 79 +++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 77 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index f167021..ad95376 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..169772e 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,27 @@ 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;
+
+	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 +1440,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 +1460,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 +1571,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);
-- 
2.5.0

Re: [PATCH v8 3/4] i2c: i801: add support of Host Notify

[Benjamin Tissoires]

On Jun 09 2016 or thereabouts, 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>
> ---
> 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).

Actually, this hook seemed to be required on the Lenovo T440 (Haswell)
but not on the T450 (Broadwell) laptop I have now here.

Wolfram, I can resend the whole series or a follow-up patch to re-enable
after resume Host Notify. How do you prefer I deal with that?

Cheers,
Benjamin

Re: [PATCH v8 3/4] i2c: i801: add support of Host Notify

[Wolfram Sang]

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

> > - 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).
> 
> Actually, this hook seemed to be required on the Lenovo T440 (Haswell)
> but not on the T450 (Broadwell) laptop I have now here.
> 
> Wolfram, I can resend the whole series or a follow-up patch to re-enable
> after resume Host Notify. How do you prefer I deal with that?

That depends a little how we want to handle patch 4. I am going to apply
patches 1+2 today to my tree. Then you can just resend patch 3 which I
hope will get some review soon, but I will pick it up for 4.8 later
nonetheless. If it is not causing too much dependency hassle, I'd prefer
that patch 4 goes via Dmitry's input tree.


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

Re: [PATCH v8 3/4] i2c: i801: add support of Host Notify

[Benjamin Tissoires]

On Jun 16 2016 or thereabouts, Wolfram Sang wrote:
> > > - 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).
> > 
> > Actually, this hook seemed to be required on the Lenovo T440 (Haswell)
> > but not on the T450 (Broadwell) laptop I have now here.
> > 
> > Wolfram, I can resend the whole series or a follow-up patch to re-enable
> > after resume Host Notify. How do you prefer I deal with that?
> 
> That depends a little how we want to handle patch 4. I am going to apply
> patches 1+2 today to my tree. Then you can just resend patch 3 which I
> hope will get some review soon, but I will pick it up for 4.8 later
> nonetheless. If it is not causing too much dependency hassle, I'd prefer
> that patch 4 goes via Dmitry's input tree.
> 

Works for me. Thanks for picking up the core changes. As soon as this
gets merged, the input part (patch 4), which is independent of patch 3
(i2c-i801) can be carried over through the input tree. So as long as
you don't need to have a new feature without users for a short period
of time, that's fine by me :)

Cheers,
Benjamin

Re: [PATCH v8 3/4] i2c: i801: add support of Host Notify

[Wolfram Sang]

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


> (i2c-i801) can be carried over through the input tree. So as long as
> you don't need to have a new feature without users for a short period
> of time, that's fine by me :)

That's fine. I have extremly high trust that the user of the feature
will be added soon ;)


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

Re: [PATCH v8 3/4] i2c: i801: add support of Host Notify

[Dmitry Torokhov]

On Thu, Jun 16, 2016 at 08:09:42AM +0200, Wolfram Sang wrote:
> > > - 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).
> > 
> > Actually, this hook seemed to be required on the Lenovo T440 (Haswell)
> > but not on the T450 (Broadwell) laptop I have now here.
> > 
> > Wolfram, I can resend the whole series or a follow-up patch to re-enable
> > after resume Host Notify. How do you prefer I deal with that?
> 
> That depends a little how we want to handle patch 4. I am going to apply
> patches 1+2 today to my tree. Then you can just resend patch 3 which I
> hope will get some review soon, but I will pick it up for 4.8 later
> nonetheless. If it is not causing too much dependency hassle, I'd prefer
> that patch 4 goes via Dmitry's input tree.

Any chance I could get a stable branch with these 2 patches based on 4.6
so that I can pull it and merge the #4? This way we do not need to wait
for 2 releases to merge everything...

Thanks.

-- 
Dmitry

Re: [PATCH v8 3/4] i2c: i801: add support of Host Notify

[Wolfram Sang]

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

On Thu, Jun 23, 2016 at 01:55:52PM -0700, Dmitry Torokhov wrote:
> On Thu, Jun 16, 2016 at 08:09:42AM +0200, Wolfram Sang wrote:
> > > > - 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).
> > > 
> > > Actually, this hook seemed to be required on the Lenovo T440 (Haswell)
> > > but not on the T450 (Broadwell) laptop I have now here.
> > > 
> > > Wolfram, I can resend the whole series or a follow-up patch to re-enable
> > > after resume Host Notify. How do you prefer I deal with that?
> > 
> > That depends a little how we want to handle patch 4. I am going to apply
> > patches 1+2 today to my tree. Then you can just resend patch 3 which I
> > hope will get some review soon, but I will pick it up for 4.8 later
> > nonetheless. If it is not causing too much dependency hassle, I'd prefer
> > that patch 4 goes via Dmitry's input tree.
> 
> Any chance I could get a stable branch with these 2 patches based on 4.6
> so that I can pull it and merge the #4? This way we do not need to wait
> for 2 releases to merge everything...

Sure.

git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/host-notify

Thanks,

   Wolfram


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

Re: [PATCH v8 3/4] i2c: i801: add support of Host Notify

[Dmitry Torokhov]

On Thu, Jun 23, 2016 at 11:46:55PM +0200, Wolfram Sang wrote:
> On Thu, Jun 23, 2016 at 01:55:52PM -0700, Dmitry Torokhov wrote:
> > On Thu, Jun 16, 2016 at 08:09:42AM +0200, Wolfram Sang wrote:
> > > > > - 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).
> > > > 
> > > > Actually, this hook seemed to be required on the Lenovo T440 (Haswell)
> > > > but not on the T450 (Broadwell) laptop I have now here.
> > > > 
> > > > Wolfram, I can resend the whole series or a follow-up patch to re-enable
> > > > after resume Host Notify. How do you prefer I deal with that?
> > > 
> > > That depends a little how we want to handle patch 4. I am going to apply
> > > patches 1+2 today to my tree. Then you can just resend patch 3 which I
> > > hope will get some review soon, but I will pick it up for 4.8 later
> > > nonetheless. If it is not causing too much dependency hassle, I'd prefer
> > > that patch 4 goes via Dmitry's input tree.
> > 
> > Any chance I could get a stable branch with these 2 patches based on 4.6
> > so that I can pull it and merge the #4? This way we do not need to wait
> > for 2 releases to merge everything...
> 
> Sure.
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/host-notify

Thanks!

-- 
Dmitry

[PATCH v8 4/4] Input: synaptics-rmi4 - add SMBus support

[Benjamin Tissoires]

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

 drivers/input/rmi4/Kconfig     |  12 ++
 drivers/input/rmi4/Makefile    |   1 +
 drivers/input/rmi4/rmi_bus.h   |  12 ++
 drivers/input/rmi4/rmi_smbus.c | 470 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 495 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..d60b0e9
--- /dev/null
+++ b/drivers/input/rmi4/rmi_smbus.c
@@ -0,0 +1,470 @@
+/*
+ * 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 {
+	u16 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 struct i2c_driver rmi_smb_driver;
+
+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;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int 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;
+}
+#endif
+
+#ifdef CONFIG_PM
+static int 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 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;
+}
+#endif
+
+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.0

Re: [PATCH v8 4/4] Input: synaptics-rmi4 - add SMBus support

[Dmitry Torokhov]

Hi Benjamin,

On Thu, Jun 09, 2016 at 04:53:50PM +0200, Benjamin Tissoires wrote:
> +
> +struct mapping_table_entry {
> +	u16 rmiaddr;

Should be __le16 rmiaddr, otherwise:

  CHECK   drivers/input/rmi4/rmi_smbus.c
drivers/input/rmi4/rmi_smbus.c:116:33: warning: incorrect type in assignment (different base types)
drivers/input/rmi4/rmi_smbus.c:116:33:    expected unsigned short [unsigned] [usertype] rmiaddr
drivers/input/rmi4/rmi_smbus.c:116:33:    got restricted __le16 [usertype] <noident>

> +
> +static struct i2c_driver rmi_smb_driver;
> +

I do not think this forward declaration is needed.

> +
> +#ifdef CONFIG_PM_SLEEP
> +static int rmi_smb_suspend(struct device *dev)

__maybe_unused instead of #ifdef.

> +{
> +	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;
> +}
> +#endif
> +
> +#ifdef CONFIG_PM
> +static int rmi_smb_runtime_suspend(struct device *dev)

Same here?

> +{
> +	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 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;
> +}
> +#endif
> +
> +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,

Why rmi_smb_resume is not part of rmi_smb_pm?

Thanks.

-- 
Dmitry

Re: [PATCH v8 4/4] Input: synaptics-rmi4 - add SMBus support

[Benjamin Tissoires]

On Jun 23 2016 or thereabouts, Dmitry Torokhov wrote:
> Hi Benjamin,
> 
> On Thu, Jun 09, 2016 at 04:53:50PM +0200, Benjamin Tissoires wrote:
> > +
> > +struct mapping_table_entry {
> > +	u16 rmiaddr;
> 
> Should be __le16 rmiaddr, otherwise:
> 
>   CHECK   drivers/input/rmi4/rmi_smbus.c
> drivers/input/rmi4/rmi_smbus.c:116:33: warning: incorrect type in assignment (different base types)
> drivers/input/rmi4/rmi_smbus.c:116:33:    expected unsigned short [unsigned] [usertype] rmiaddr
> drivers/input/rmi4/rmi_smbus.c:116:33:    got restricted __le16 [usertype] <noident>
> 
> > +
> > +static struct i2c_driver rmi_smb_driver;
> > +
> 
> I do not think this forward declaration is needed.
> 
> > +
> > +#ifdef CONFIG_PM_SLEEP
> > +static int rmi_smb_suspend(struct device *dev)
> 
> __maybe_unused instead of #ifdef.
> 
> > +{
> > +	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;
> > +}
> > +#endif
> > +
> > +#ifdef CONFIG_PM
> > +static int rmi_smb_runtime_suspend(struct device *dev)
> 
> Same here?
> 
> > +{
> > +	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 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;
> > +}
> > +#endif
> > +
> > +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,
> 
> Why rmi_smb_resume is not part of rmi_smb_pm?
> 

This is because rmi_smbus device both have a PS/2 interface and a SMBus
one. I'll have to check again now that I have a slightly different way
of binding smbus devices in my tree, but the issue was:
- having resume part of pm means it will get caught by PM directly
- the PS/2 node gets also resumed by PM
- calling PS/2 commands during resume switches the devices back into
  PS/2 and stops the SMBus communication.

So it's easier to wait only for the PS/2 PM resume call which will call
the SMBus resume function when the device is in a proper state.

I'll send out the updated patch with your comments next week hopefully.

Cheers,
Benjamin

Re: [PATCH v8 4/4] Input: synaptics-rmi4 - add SMBus support

[Dmitry Torokhov]

On Fri, Jun 24, 2016 at 09:19:32AM +0200, Benjamin Tissoires wrote:
> On Jun 23 2016 or thereabouts, Dmitry Torokhov wrote:
> > On Thu, Jun 09, 2016 at 04:53:50PM +0200, Benjamin Tissoires wrote:
> > > +
> > > +static struct i2c_driver rmi_smb_driver = {
> > > +	.driver = {
> > > +		.owner	= THIS_MODULE,
> > > +		.name	= "rmi4_smbus",
> > > +		.pm	= &rmi_smb_pm,
> > > +		.resume	= rmi_smb_resume,
> > 
> > Why rmi_smb_resume is not part of rmi_smb_pm?
> > 
> 
> This is because rmi_smbus device both have a PS/2 interface and a SMBus
> one. I'll have to check again now that I have a slightly different way
> of binding smbus devices in my tree, but the issue was:
> - having resume part of pm means it will get caught by PM directly
> - the PS/2 node gets also resumed by PM
> - calling PS/2 commands during resume switches the devices back into
>   PS/2 and stops the SMBus communication.
> 
> So it's easier to wait only for the PS/2 PM resume call which will call
> the SMBus resume function when the device is in a proper state.
> 
> I'll send out the updated patch with your comments next week hopefully.

Hmm, I think you will have to walk me through resume process. How do we
tie in PS/2 and I2C on these devices abd have PS/2 code call into this
driver?

Thanks.

-- 
Dmitry

Re: [PATCH v8 4/4] Input: synaptics-rmi4 - add SMBus support

[Benjamin Tissoires]

On Jun 24 2016 or thereabouts, Dmitry Torokhov wrote:
> On Fri, Jun 24, 2016 at 09:19:32AM +0200, Benjamin Tissoires wrote:
> > On Jun 23 2016 or thereabouts, Dmitry Torokhov wrote:
> > > On Thu, Jun 09, 2016 at 04:53:50PM +0200, Benjamin Tissoires wrote:
> > > > +
> > > > +static struct i2c_driver rmi_smb_driver = {
> > > > +	.driver = {
> > > > +		.owner	= THIS_MODULE,
> > > > +		.name	= "rmi4_smbus",
> > > > +		.pm	= &rmi_smb_pm,
> > > > +		.resume	= rmi_smb_resume,
> > > 
> > > Why rmi_smb_resume is not part of rmi_smb_pm?
> > > 
> > 
> > This is because rmi_smbus device both have a PS/2 interface and a SMBus
> > one. I'll have to check again now that I have a slightly different way
> > of binding smbus devices in my tree, but the issue was:
> > - having resume part of pm means it will get caught by PM directly
> > - the PS/2 node gets also resumed by PM
> > - calling PS/2 commands during resume switches the devices back into
> >   PS/2 and stops the SMBus communication.
> > 
> > So it's easier to wait only for the PS/2 PM resume call which will call
> > the SMBus resume function when the device is in a proper state.
> > 
> > I'll send out the updated patch with your comments next week hopefully.
> 
> Hmm, I think you will have to walk me through resume process. How do we
> tie in PS/2 and I2C on these devices abd have PS/2 code call into this
> driver?
> 

Sure.

For starters, here is my latest WIP (I need to rework on the series for
commit messages and probably squash some patches):
https://github.com/bentiss/linux/commits/synaptics-rmi4-smbus-v4.7-rc4%2B

Then, let me explain the problems we have with those touchpads and the
resume process.

The touchpads are both connected to PS/2 and SMBus as you know. However,
there is no SMBus enumeration entry anywhere in the system. Luckily,
the PS/2 chip is aware of the other bus, and can be polled to
request whether or not we are confronted to a RMI4 over SMBus device
(see https://github.com/bentiss/linux/commit/a3e67de764656201522962028bc783fc4b921de3 )

Of course, to make those touchpads robust with reboots and allow legacy
drivers (PS/2) to use them, the firmware tend to switch back to PS/2 as
soon as you issue a PS/2 reset command or if you send some other PS/2
commands. The problem we have is that if you send some various PS/2
commands to the touchpad, it disconnects from the SMBus entirely (it
took me one year to understand why the device was not showing up on
I2C).

The last interesting fact for those touchpad is that you need to enable
SMBus communication by issuing a SMB_PROTOCOL_VERSION_ADDRESS command.
If you do not issue this after a PS/2 reset, the device is muted over
SMBus.

So, to be sure we can query the touchpad from PS/2 and to control the
PS/2 commands and the resume, I have in the series above a separate PS/2
driver for this touchpad. The regular psmouse driver probes the PS/2
chip, but aborts seeing the SMBus capability. Then rmi_ps2 takes over
and binds to the PS/2 chip to fill in the platform data required by
rmi_smbus (https://github.com/bentiss/linux/commit/3ceb7c80ecee17a86b8ae8476211c7498cc823d2)

If we simply unbind the PS/2 node and let it dangling, the serio driver
issues a reconnect after resume on both the PS/2 chip of the touchpad,
and the PS/2 pass-through node of the trackstick.

But if the PS/2 chip of the touchpad is left dangling, the resume
process will first call a reconnect on the pass-through, then on
the touchpad through the enumeration of the serio bus, which will reset
the touchpad and messed up the pass-through node and the rmi-smbus
driver.
So keeping around a reference to the PS/2 node allows to set this node
as a parent of the pass-through trackstick node and guarantees that the
touchpad will be resumed before the trackstick.

One final thing. I tried not having rmi_ps2 calling the .resume callback
of rmi_smbus and keeping rmi_smbus as a PM. But the issue is that the
serio driver sends the reset command in a workqueue as it can takes some
time:

- serio gets resume called -> prepare the worker for the
  resume/reconnect process
- rmi_smbus gets resumed -> OK
- the worker kicks in, reset the PS/2 lines, and shuts down the
  rmi_smbus device


I also tried one thing where I did not bind the PS/2 touchpad at all
(and having some kind of platform driver to bind rmi_smbus). And of
course, grub initializes the touchpad, so it disconnects on I2C and you
can't bind it on SMBus, ever :)


I think that's all I know on these touchpads and this is all the mess I can
present. If you have a better option, I'm all ears as this is not clean,
but I can't figure out a better way.

Cheers,
Benjamin