Adding Benjamin who mainly implemented this.
On Tue, May 05, 2020 at 07:51:08AM +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.
Yay, cool idea to use the slave interface. I like it a lot!
> +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.
> + */
> + 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?
> + 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.
> + 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?
Attachment:
signature.asc
Description: PGP signature
