From: Wolfram Sang <wsa@kernel.org>
To: Alain Volmat <alain.volmat@st.com>
Cc: alexandre.torgue@st.com, linux-kernel@vger.kernel.org,
pierre-yves.mordret@st.com, linux-i2c@vger.kernel.org,
fabrice.gasnier@st.com, linux-stm32@st-md-mailman.stormreply.com,
linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v2 1/2] i2c: smbus: add core function handling SMBus host-notify
Date: Sat, 25 Jul 2020 22:27:33 +0200 [thread overview]
Message-ID: <20200725202733.GA946@kunai> (raw)
In-Reply-To: <1593776168-17867-2-git-send-email-alain.volmat@st.com>
[-- Attachment #1.1: Type: text/plain, Size: 6866 bytes --]
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
>
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2020-07-25 20:30 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-03 11:36 [PATCH v2 0/2] i2c: stm32: add host-notify support via i2c slave Alain Volmat
2020-07-03 11:36 ` [PATCH v2 1/2] i2c: smbus: add core function handling SMBus host-notify Alain Volmat
2020-07-25 20:27 ` Wolfram Sang [this message]
2020-07-26 13:35 ` Wolfram Sang
2020-07-28 12:10 ` Alain Volmat
2020-07-28 16:36 ` Wolfram Sang
2020-08-02 19:15 ` Wolfram Sang
2020-07-03 11:36 ` [PATCH v2 2/2] i2c: stm32f7: Add SMBus Host-Notify protocol support Alain Volmat
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200725202733.GA946@kunai \
--to=wsa@kernel.org \
--cc=alain.volmat@st.com \
--cc=alexandre.torgue@st.com \
--cc=fabrice.gasnier@st.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-i2c@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-stm32@st-md-mailman.stormreply.com \
--cc=pierre-yves.mordret@st.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).