Re: [PATCH v2 1/2] i2c: smbus: add core function handling SMBus host-notify
> I've simplified the index handling as you suggested. The only impact is that > finally we do not consider anymore the I2C_SLAVE_WRITE_REQUESTED event as the > beginning of the transaction since we don't perform the "reset" of the > handling upon this event. One more comment on this one because I had to update the testunit, too. To be robust against multiple write messages in one transfer, we need to reset both, after STOP and when I2C_SLAVE_WRITE_REQUESTED. See here: 96 case I2C_SLAVE_STOP: 97 if (tu->reg_idx == TU_NUM_REGS) 98 queue_delayed_work(system_long_wq, &tu->worker, 99msecs_to_jiffies(100 * tu->regs[TU_REG_DELAY])); 100 fallthrough; 101 102 case I2C_SLAVE_WRITE_REQUESTED: 103 tu->reg_idx = 0; 104 break; As you see, I used 'fallthrough' to avoid code duplication and that only one reset part will be updated. Dunno if you really need it, too, as I haven't seen your latest code yet. signature.asc Description: PGP signature
Re: [PATCH v2 1/2] i2c: smbus: add core function handling SMBus host-notify
Hi Wolfram,
I've taken your comments and prepared a new serie including them.
I'll wait for the conclusion regarding the bindings before pushing it.
I also have an additional patch ready in order to add again the SMBus Alert
support within the stm32f7 driver since it has been removed from the
current serie. Hopefully I can push it once binding is acked so that it
can get merged also in this cycle.
> > 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_slave_host_notify_device
> > * i2c_free_slave_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
> > ---
> > v2: identical to v1
> >
> > drivers/i2c/i2c-core-smbus.c | 114
> > +++
>
> I came to the conclusion that this code should be in i2c-smbus.c.
> Because it is SMBus only. I agree that the current code layout is
> confusing. I will try to move the whole host-notify support to i2c-smbus
> in the next cycle.
>
> Yes, that means that one needs to select I2C_SMBUS in the config, too
> (unless you want to 'select' it with your driver). But most people won't
> need it so they can compile it away. This is what I2C_SMBUS is for.
Ok, code is now moved into i2c-smbus.c
In case of the stm32f7 anyway I2C_SMBUS was already selected hence there is no
impact.
> > +static int i2c_slave_host_notify_cb(struct i2c_client *client,
> > + enum i2c_slave_event event, u8 *val)
> > +{
> > + struct i2c_slave_host_notify_status *status = client->dev.platform_data;
> > + int ret;
> > +
>
> Can't we simplify 'index' handling similar to the testunit driver...
>
> > + switch (event) {
> > + case I2C_SLAVE_WRITE_REQUESTED:
> > + status->index = 0;
>
> ... by removing this line...
>
> > + 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->index == 0)
> > + status->addr = *val;
> > + if (status->index < U8_MAX)
> > + status->index++;
> > + break;
> > + case I2C_SLAVE_STOP:
> > + /* Handle host-notify if whole message received only */
> > + if (status->index != SMBUS_HOST_NOTIFY_LEN) {
> > + status->index = U8_MAX;
> > + break;
> > + }
> > +
> > + ret = i2c_handle_smbus_host_notify(client->adapter,
> > + status->addr);
> > + if (ret < 0)
> > + return ret;
> > + status->index = U8_MAX;
>
> ... and handling the logic here like:
>
> + if (status->index == SMBUS_HOST_NOTIFY_LEN)
> + i2c_handle_smbus_host_notify(client->adapter,
> status->addr);
> + status->index = 0;
>
> Note that I2C_SLAVE_STOP doesn't return a retval, so we don't need to check
> i2c_handle_smbus_host_notify().
>
> > + break;
> > + case I2C_SLAVE_READ_REQUESTED:
> > + case I2C_SLAVE_READ_PROCESSED:
> > + *val = 0xff;
> > + break;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * i2c_new_slave_host_notify_device - get a client for SMBus host-notify
> > support
> > + * @adapter: the target adapter
> > + * Context: can sleep
> > + *
> > + * Setup handling of the SMBus host-notify protocol on a given I2C bus
> > segment.
> > + *
> > + * Handling is done by creating a device and its callback and handling data
> > + * received via the SMBus host-notify address (0x8)
> > + *
> > + * This returns the client, which should be ultimately freed using
> > + * i2c_free_slave_host_notify_device(); or an ERRPTR to indicate an error.
> > + */
> > +struct i2c_client *i2c_new_slave_host_notify_device(struct i2c_adapter
> > *adapter)
> > +{
> > + struct i2c_board_info host_notify_board_info = {
> > + I2C_BOARD_INFO("smbus_host_notify", 0x08),
> > + .flags = I2C_CLIENT_SLAVE,
> > + };
> > + struct i2c_slave_host_notify_status *status;
> > + struct i2c_client *client;
> > + int ret;
> > +
> > + status = kzalloc(sizeof(struct i2c_slave_host_notify_status),
> > +GFP_KERNEL);
> > + if (!status)
> > + return ERR_PTR(-ENOMEM);
> > + status->index = U8_MAX;
>
> This line could go, too, if my simplification above works.
I've simplif
Re: [PATCH v2 1/2] i2c: smbus: add core function handling SMBus host-notify
Hi Alain, > I've taken your comments and prepared a new serie including them. > I'll wait for the conclusion regarding the bindings before pushing it. Thanks! I hope we can finish the discussion this week because Linus hasn't made a clear statement if there will be an rc8. But I still think we can do HostNotify for v5.9. > I also have an additional patch ready in order to add again the SMBus Alert > support within the stm32f7 driver since it has been removed from the > current serie. Hopefully I can push it once binding is acked so that it > can get merged also in this cycle. If it is super straight-forward, then yes. signature.asc Description: PGP signature
Re: [PATCH v2 1/2] i2c: smbus: add core function handling SMBus host-notify
Hi Alain,
some more comments for this one. I hope to come to a conclusion with Rob
regarding the binding for patch 2, so we are then ready to go.
On Fri, Jul 03, 2020 at 01:36:07PM +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_slave_host_notify_device
> * i2c_free_slave_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
> ---
> v2: identical to v1
>
> drivers/i2c/i2c-core-smbus.c | 114
> +++
I came to the conclusion that this code should be in i2c-smbus.c.
Because it is SMBus only. I agree that the current code layout is
confusing. I will try to move the whole host-notify support to i2c-smbus
in the next cycle.
Yes, that means that one needs to select I2C_SMBUS in the config, too
(unless you want to 'select' it with your driver). But most people won't
need it so they can compile it away. This is what I2C_SMBUS is for.
> include/linux/i2c-smbus.h| 12 +
> 2 files changed, 126 insertions(+)
>
> diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
> index f5c9787992e9..23ab9dc5ac85 100644
> --- a/drivers/i2c/i2c-core-smbus.c
> +++ b/drivers/i2c/i2c-core-smbus.c
> @@ -715,3 +715,117 @@ 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)
> +#define SMBUS_HOST_NOTIFY_LEN3
> +struct i2c_slave_host_notify_status {
> + u8 index;
> + u8 addr;
> +};
> +
> +static int i2c_slave_host_notify_cb(struct i2c_client *client,
> + enum i2c_slave_event event, u8 *val)
> +{
> + struct i2c_slave_host_notify_status *status = client->dev.platform_data;
> + int ret;
> +
Can't we simplify 'index' handling similar to the testunit driver...
> + switch (event) {
> + case I2C_SLAVE_WRITE_REQUESTED:
> + status->index = 0;
... by removing this line...
> + 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->index == 0)
> + status->addr = *val;
> + if (status->index < U8_MAX)
> + status->index++;
> + break;
> + case I2C_SLAVE_STOP:
> + /* Handle host-notify if whole message received only */
> + if (status->index != SMBUS_HOST_NOTIFY_LEN) {
> + status->index = U8_MAX;
> + break;
> + }
> +
> + ret = i2c_handle_smbus_host_notify(client->adapter,
> +status->addr);
> + if (ret < 0)
> + return ret;
> + status->index = U8_MAX;
... and handling the logic here like:
+ if (status->index == SMBUS_HOST_NOTIFY_LEN)
+ i2c_handle_smbus_host_notify(client->adapter,
status->addr);
+ status->index = 0;
Note that I2C_SLAVE_STOP doesn't return a retval, so we don't need to check
i2c_handle_smbus_host_notify().
> + break;
> + case I2C_SLAVE_READ_REQUESTED:
> + case I2C_SLAVE_READ_PROCESSED:
> + *val = 0xff;
> + break;
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * i2c_new_slave_host_notify_device - get a client for SMBus host-notify
> support
> + * @adapter: the target adapter
> + * Context: can sleep
> + *
> + * Setup handling of the SMBus host-notify protocol on a given I2C bus
> segment.
> + *
> + * Handling is done by creating a device and its callback and handling data
> + * received via the SMBus host-notify address (0x8)
> + *
> + * This returns the client, which should be ultimately freed using
> + * i2c_free_slave_host_notify_device(); or an ERRPTR to indicate an error.
> + */
> +struct i2c_client *i2c_new_slave_host_notify_device(struct i2c_adapter
> *adapter)
> +{
> + struct i2c_board_info host_notify_board_info = {
> + I2C_BOARD_INFO("smbus_host_notify", 0x08),
> + .flags = I2C_CLIENT_SLAVE,
> + };
> + struct i2c_slave_host_notify_status *status;
> + struct i2c_client *client;
> + int ret;
> +
> + status = kzalloc(sizeof(struct i2c_slave_host_notify_status),
> + GFP_KERNEL);
> + if (!status)
> + re
Re: [PATCH v2 1/2] i2c: smbus: add core function handling SMBus host-notify
> > +void i2c_free_slave_host_notify_device(struct i2c_client *client)
> > +{
> > + i2c_slave_unregister(client);
> > + kfree(client->dev.platform_data);
> > + i2c_unregister_device(client);
> > +}
> > +EXPORT_SYMBOL_GPL(i2c_free_slave_host_notify_device);
>
> Sidenote: With my recent series "i2c: slave: improve sanity checks when
> un-/registering" this code became NULL-safe (and IS_ERR safe, too).
Stupid me, it is not NULL safe. The functions are. But, we deregister
'client' on our own. It probably makes sense to add some sanity checking
of the parameters of the exported functions.
signature.asc
Description: PGP signature
