On Sat, May 23, 2020 at 10:46:25AM +0000, Wolfram Sang wrote:

Adding Benjamin who mainly implemented this.

On Tue, May 05, 2020 at 07:51:08AM +0200, Alain Volmat wrote:

quoted

SMBus Host-Notify protocol, from the adapter point of view
consist of receiving a message from a client, including the
client address and some other data.

It can be simply handled by creating a new slave device
and registering a callback performing the parsing of the
message received from the client.

This commit introduces two new core functions
  * i2c_new_smbus_host_notify_device
  * i2c_free_smbus_host_notify_device
that take care of registration of the new slave device and
callback and will call i2c_handle_smbus_host_notify once a
Host-Notify event is received.

Yay, cool idea to use the slave interface. I like it a lot!

quoted

+static int i2c_smbus_host_notify_cb(struct i2c_client *client,
+				    enum i2c_slave_event event, u8 *val)
+{
+	struct i2c_smbus_host_notify_status *status = client->dev.platform_data;
+	int ret;
+
+	switch (event) {
+	case I2C_SLAVE_WRITE_REQUESTED:
+		status->notify_start = true;
+		break;
+	case I2C_SLAVE_WRITE_RECEIVED:
+		/* We only retrieve the first byte received (addr)
+		 * since there is currently no way to retrieve the data
+		 * parameter from the client.

Maybe s/no way/no support/ ? I still wonder if we couldn't add it
somehow. Once we find a device which needs this, of course.

Indeed. Such support can be added later on once such device is found. For the
time being I will state "no support"

quoted

+		 */
+		if (!status->notify_start)
+			break;
+		status->addr = *val;
+		status->notify_start = false;
+		break;
+	case I2C_SLAVE_STOP:

What about setting 'notify_start' to false here as well? In the case of
an incomplete write?

Ok. I will check that notify_start is false before calling host_notify
(since otherwise it will call i2c_handle_smbus_host_notify with a bad addr
value) and reset notify_start to false if it is still true.

quoted

+		ret = i2c_handle_smbus_host_notify(client->adapter,
+						   status->addr);
+		if (ret < 0) {
+			dev_warn(&client->adapter->dev, "failed to handle host_notify (%d)\n",
+				ret);

I think we should rather add such error strings to the core if we think
they are needed. I am not convinced they are, though.

Agreed, this error can be removed.

quoted

+			return ret;
+		}
+		break;
+	default:
+		/* Only handle necessary events */
+		break;
+	}
+
+	return 0;
+}
+

Rest of the code looks good. Maybe we should compile all this only when
I2C_SLAVE is enabled?

Yes, I will enclose that around I2C_SLAVE support check.