| Message ID | 1593776168-17867-2-git-send-email-alain.volmat@st.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | i2c: stm32: add host-notify support via i2c slave | expand |
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 <alain.volmat@st.com> > --- > 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_LEN 3 > +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) > + return ERR_PTR(-ENOMEM); > + status->index = U8_MAX; This line could go, too, if my simplification above works. > + > + host_notify_board_info.platform_data = status; > + > + client = i2c_new_client_device(adapter, &host_notify_board_info); > + if (IS_ERR(client)) { > + kfree(status); > + return client; > + } > + > + ret = i2c_slave_register(client, i2c_slave_host_notify_cb); > + if (ret) { > + i2c_unregister_device(client); > + kfree(status); > + return ERR_PTR(ret); > + } > + > + return client; > +} > +EXPORT_SYMBOL_GPL(i2c_new_slave_host_notify_device); > + > +/** > + * i2c_free_slave_host_notify_device - free the client for SMBus host-notify > + * support > + * @client: the client to free > + * Context: can sleep > + * > + * Free the i2c_client allocated via i2c_new_slave_host_notify_device > + */ > +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). > +#endif > diff --git a/include/linux/i2c-smbus.h b/include/linux/i2c-smbus.h > index 1e4e0de4ef8b..52e62f398f1c 100644 > --- a/include/linux/i2c-smbus.h > +++ b/include/linux/i2c-smbus.h > @@ -38,6 +38,18 @@ static inline int of_i2c_setup_smbus_alert(struct i2c_adapter *adap) > return 0; > } > #endif > +#if IS_ENABLED(CONFIG_I2C_SLAVE) > +struct i2c_client *i2c_new_slave_host_notify_device(struct i2c_adapter *adapter); > +void i2c_free_slave_host_notify_device(struct i2c_client *client); > +#else > +static inline struct i2c_client *i2c_new_slave_host_notify_device(struct i2c_adapter *adapter) > +{ > + return NULL; return ERR_PTR(-ENOSYS); As the docs say, this function either returns a valid client or an ERR_PTR. > +} > +static inline void i2c_free_slave_host_notify_device(struct i2c_client *client) > +{ > +} > +#endif > > #if IS_ENABLED(CONFIG_I2C_SMBUS) && IS_ENABLED(CONFIG_DMI) > void i2c_register_spd(struct i2c_adapter *adap); > -- > 2.7.4 >
> > +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.
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 <alain.volmat@st.com> > > --- > > 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 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. > > +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). Sanity test on client added at the beginning of i2c_free_slave_host_notify_device in order to ensure that we do not dereference a null pointer. > > +static inline struct i2c_client *i2c_new_slave_host_notify_device(struct i2c_adapter *adapter) > > +{ > > + return NULL; > > return ERR_PTR(-ENOSYS); > > As the docs say, this function either returns a valid client or an > ERR_PTR. Done. > > > +} > > +static inline void i2c_free_slave_host_notify_device(struct i2c_client *client) > > +{ > > +} > > +#endif > > > > #if IS_ENABLED(CONFIG_I2C_SMBUS) && IS_ENABLED(CONFIG_DMI) > > void i2c_register_spd(struct i2c_adapter *adap); > > -- > > 2.7.4 > >
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.
> 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, 99 msecs_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.
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_LEN 3 +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; + + switch (event) { + case I2C_SLAVE_WRITE_REQUESTED: + status->index = 0; + 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; + 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; + + host_notify_board_info.platform_data = status; + + client = i2c_new_client_device(adapter, &host_notify_board_info); + if (IS_ERR(client)) { + kfree(status); + return client; + } + + ret = i2c_slave_register(client, i2c_slave_host_notify_cb); + if (ret) { + i2c_unregister_device(client); + kfree(status); + return ERR_PTR(ret); + } + + return client; +} +EXPORT_SYMBOL_GPL(i2c_new_slave_host_notify_device); + +/** + * i2c_free_slave_host_notify_device - free the client for SMBus host-notify + * support + * @client: the client to free + * Context: can sleep + * + * Free the i2c_client allocated via i2c_new_slave_host_notify_device + */ +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); +#endif diff --git a/include/linux/i2c-smbus.h b/include/linux/i2c-smbus.h index 1e4e0de4ef8b..52e62f398f1c 100644 --- a/include/linux/i2c-smbus.h +++ b/include/linux/i2c-smbus.h @@ -38,6 +38,18 @@ static inline int of_i2c_setup_smbus_alert(struct i2c_adapter *adap) return 0; } #endif +#if IS_ENABLED(CONFIG_I2C_SLAVE) +struct i2c_client *i2c_new_slave_host_notify_device(struct i2c_adapter *adapter); +void i2c_free_slave_host_notify_device(struct i2c_client *client); +#else +static inline struct i2c_client *i2c_new_slave_host_notify_device(struct i2c_adapter *adapter) +{ + return NULL; +} +static inline void i2c_free_slave_host_notify_device(struct i2c_client *client) +{ +} +#endif #if IS_ENABLED(CONFIG_I2C_SMBUS) && IS_ENABLED(CONFIG_DMI) void i2c_register_spd(struct i2c_adapter *adap);
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 <alain.volmat@st.com> --- v2: identical to v1 drivers/i2c/i2c-core-smbus.c | 114 +++++++++++++++++++++++++++++++++++++++++++ include/linux/i2c-smbus.h | 12 +++++ 2 files changed, 126 insertions(+)