Re: [PATCH v2 1/4] i2c: smbus: add core function handling SMBus host-notify
From: Wolfram Sang <wsa@kernel.org>
Date: 2020-07-01 10:49:52
Also in:
linux-devicetree, linux-i2c, lkml
On Thu, Jun 25, 2020 at 09:39:26AM +0200, Alain Volmat wrote:
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.
Signed-off-by: Alain Volmat <redacted>
---
v2: remove useless dev_err message in case of hnotify handling error
prevent handling hnotify in case of a incomplete writeOkay, now I got it to work, I also noted a few more issues. First, I'd suggest s/i2c_smbus_host_notify/i2c_slave_host_notify/g for all occurences in this patch. This makes a stronger distinction between the generic HostNotify support and the slave specific one. Also, I wonder if this shouldn't go to i2c-smbus.c instead but I haven't checked if we end up in dependency hell then. Second best thought: at least move to i2c-core-slave.c, then we could save the #ifdeffery in the c-file?
quoted hunk ↗ jump to hunk
drivers/i2c/i2c-core-smbus.c | 110 +++++++++++++++++++++++++++++++++++++++++++ include/linux/i2c-smbus.h | 2 + 2 files changed, 112 insertions(+)diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c index 56bb840142e3..3a37664fb5f6 100644 --- a/drivers/i2c/i2c-core-smbus.c +++ b/drivers/i2c/i2c-core-smbus.c@@ -708,3 +708,113 @@ int of_i2c_setup_smbus_alert(struct i2c_adapter *adapter) } EXPORT_SYMBOL_GPL(of_i2c_setup_smbus_alert); #endif + +#if IS_ENABLED(CONFIG_I2C_SLAVE) +struct i2c_smbus_host_notify_status { + bool notify_start; + u8 addr; +}; + +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 support to retrieve the data + * parameter from the client. + */ + if (!status->notify_start) + break; + status->addr = *val; + status->notify_start = false;
So, we are safe if the message is too short. Otherwise, we capture the first byte (== address) only, right. Further bytes until STOP are discarded. So, we don't check if the message is too long and contains more than the status word. Maybe we should add that?
+ break;
+ case I2C_SLAVE_STOP:
+ /* In case of incomplete write, don't handle host-notify */
+ if (status->notify_start) {
+ status->notify_start = false;
+ break;
+ }
+
+ ret = i2c_handle_smbus_host_notify(client->adapter,
+ status->addr);
+ if (ret < 0)
+ return ret;
+ break;
+ default:
The missing cases are mandatory. From my testunit driver:
case I2C_SLAVE_READ_REQUESTED:
case I2C_SLAVE_READ_PROCESSED:
*val = 0xff;
break;
+ /* Only handle necessary events */ + break; + } + + return 0; +} +
...
quoted hunk ↗ jump to hunk
--- a/include/linux/i2c-smbus.h +++ b/include/linux/i2c-smbus.h@@ -38,6 +38,8 @@ static inline int of_i2c_setup_smbus_alert(struct i2c_adapter *adap) return 0; } #endif +struct i2c_client *i2c_new_smbus_host_notify_device(struct i2c_adapter *adapter); +void i2c_free_smbus_host_notify_device(struct i2c_client *client);
Those need to be guarded with I2C_SLAVE as well. And an #else branch with empty/successful placeholders.
#if IS_ENABLED(CONFIG_I2C_SMBUS) && IS_ENABLED(CONFIG_DMI) void i2c_register_spd(struct i2c_adapter *adap); -- 2.7.4