[PATCH v2 0/4] stm32-f7: Addition of SMBus Alert / Host-notify features

[PATCH v2 0/4] stm32-f7: Addition of SMBus Alert / Host-notify features

[Alain Volmat]

This serie adds SMBus Alert and SMBus Host-Notify features for the i2c-stm32f7.

This serie v2 rework comments from the 1st serie and replace the very generic
reg_client / unreg_client callback with HOST_NOTIFY only reg_hnotify_cli
and unreg_hnotify_cli callbacks.

Alain Volmat (4):
  i2c: smbus: add core function handling SMBus host-notify
  i2c: addition of client hnotify reg/unreg callbacks
  dt-bindings: i2c-stm32: add SMBus Alert bindings
  i2c: stm32f7: Add SMBus-specific protocols support

 .../devicetree/bindings/i2c/st,stm32-i2c.yaml      |   4 +
 drivers/i2c/busses/Kconfig                         |   1 +
 drivers/i2c/busses/i2c-stm32f7.c                   | 194 +++++++++++++++++++--
 drivers/i2c/i2c-core-base.c                        |  18 +-
 drivers/i2c/i2c-core-smbus.c                       | 110 ++++++++++++
 include/linux/i2c-smbus.h                          |   2 +
 include/linux/i2c.h                                |   8 +
 7 files changed, 325 insertions(+), 12 deletions(-)


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 1/4] i2c: smbus: add core function handling SMBus host-notify

[Alain Volmat]

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 <alain.volmat@st.com>
---
v2: remove useless dev_err message in case of hnotify handling error
    prevent handling hnotify in case of a incomplete write

 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;
+		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:
+		/* Only handle necessary events */
+		break;
+	}
+
+	return 0;
+}
+
+/**
+ * i2c_new_smbus_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_smbus_host_notify_device(); or an ERRPTR to indicate an error.
+ */
+struct i2c_client *i2c_new_smbus_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_smbus_host_notify_status *status;
+	struct i2c_client *client;
+	int ret;
+
+	status = kzalloc(sizeof(struct i2c_smbus_host_notify_status),
+			 GFP_KERNEL);
+	if (!status)
+		return ERR_PTR(-ENOMEM);
+
+	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_smbus_host_notify_cb);
+	if (ret) {
+		i2c_unregister_device(client);
+		kfree(status);
+		return ERR_PTR(ret);
+	}
+
+	return client;
+}
+EXPORT_SYMBOL_GPL(i2c_new_smbus_host_notify_device);
+
+/**
+ * i2c_free_smbus_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_smbus_host_notify_device
+ */
+void i2c_free_smbus_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_smbus_host_notify_device);
+#endif
diff --git a/include/linux/i2c-smbus.h b/include/linux/i2c-smbus.h
index 1e4e0de4ef8b..974038a684ed 100644
--- 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);
 
 #if IS_ENABLED(CONFIG_I2C_SMBUS) && IS_ENABLED(CONFIG_DMI)
 void i2c_register_spd(struct i2c_adapter *adap);
-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/4] i2c: smbus: add core function handling SMBus host-notify

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 4192 bytes --]

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 <alain.volmat@st.com>
> ---
> v2: remove useless dev_err message in case of hnotify handling error
>     prevent handling hnotify in case of a incomplete write

Okay, 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?

> 
>  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;
> +}
> +

...

> --- 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
> 

[-- 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

Re: [PATCH v2 1/4] i2c: smbus: add core function handling SMBus host-notify

[Alain Volmat]

Hi Wolfram,

> Okay, 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.

Ok with that.

> 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?

I have actually difficulties to understand clearly what should go within
i2c-smbus.c vs i2c-core-smbus or i2c-core-slave. My feeling is that
i2c-core-slave is more about the registering of a slave rather than one usage
of the slave mechanism. Hence I am not sure those functions/cb belong there.
But at the same time, I don't know about i2c-smbus vs i2c-core-smbus. I putted
it within i2c-core-smbus considering that the creation of the alert device
is also done there.

....

> > +{
> > +	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?

Yes I modified 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;

Ok, done as well.

> > --- 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.

Ok understood.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 2/4] i2c: addition of client hnotify reg/unreg callbacks

[Alain Volmat]

Addition of two callbacks reg_hnotify_cli and unreg_hnotify_cli
that can be implemented by adapter drivers in order to take action
whenever a client with HOST_NOTIFY flag is being registered to it.

Signed-off-by: Alain Volmat <alain.volmat@st.com>
---
v2: replace generic client reg/unreg callbacks with host-notify
    client reg/unreg callbacks

 drivers/i2c/i2c-core-base.c | 18 ++++++++++++++++--
 include/linux/i2c.h         |  8 ++++++++
 2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 26f03a14a478..46745e403e98 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -319,6 +319,14 @@ static int i2c_device_probe(struct device *dev)
 	if (!client)
 		return 0;
 
+	if (client->flags & I2C_CLIENT_HOST_NOTIFY) {
+		if (client->adapter->algo->reg_hnotify_cli) {
+			status = client->adapter->algo->reg_hnotify_cli(client);
+			if (status)
+				return status;
+		}
+	}
+
 	driver = to_i2c_driver(dev->driver);
 
 	client->irq = client->init_irq;
@@ -415,8 +423,11 @@ static int i2c_device_probe(struct device *dev)
 	dev_pm_clear_wake_irq(&client->dev);
 	device_init_wakeup(&client->dev, false);
 put_sync_adapter:
-	if (client->flags & I2C_CLIENT_HOST_NOTIFY)
+	if (client->flags & I2C_CLIENT_HOST_NOTIFY) {
 		pm_runtime_put_sync(&client->adapter->dev);
+		if (client->adapter->algo->unreg_hnotify_cli)
+			client->adapter->algo->unreg_hnotify_cli(client);
+	}
 
 	return status;
 }
@@ -442,8 +453,11 @@ static int i2c_device_remove(struct device *dev)
 	device_init_wakeup(&client->dev, false);
 
 	client->irq = 0;
-	if (client->flags & I2C_CLIENT_HOST_NOTIFY)
+	if (client->flags & I2C_CLIENT_HOST_NOTIFY) {
 		pm_runtime_put(&client->adapter->dev);
+		if (client->adapter->algo->unreg_hnotify_cli)
+			client->adapter->algo->unreg_hnotify_cli(client);
+	}
 
 	return status;
 }
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index b8b8963f8bb9..35ad108adc68 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -507,6 +507,10 @@ i2c_register_board_info(int busnum, struct i2c_board_info const *info,
  *   so e.g. PMICs can be accessed very late before shutdown. Optional.
  * @functionality: Return the flags that this algorithm/adapter pair supports
  *   from the ``I2C_FUNC_*`` flags.
+ * @reg_hnotify_cli: Callback informing that a new client with HOST_NOTIFY flag
+ *   is being registered
+ * @unreg_hnotify_cli: Callback informing that a client with HOST_NOTIFY flag
+ *   is being removed
  * @reg_slave: Register given client to I2C slave mode of this adapter
  * @unreg_slave: Unregister given client from I2C slave mode of this adapter
  *
@@ -543,6 +547,10 @@ struct i2c_algorithm {
 	/* To determine what the adapter supports */
 	u32 (*functionality)(struct i2c_adapter *adap);
 
+	/* To inform the adapter of the probe/remove of a HOST_NOTIFY client */
+	int (*reg_hnotify_cli)(struct i2c_client *client);
+	void (*unreg_hnotify_cli)(struct i2c_client *client);
+
 #if IS_ENABLED(CONFIG_I2C_SLAVE)
 	int (*reg_slave)(struct i2c_client *client);
 	int (*unreg_slave)(struct i2c_client *client);
-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 3/4] dt-bindings: i2c-stm32: add SMBus Alert bindings

[Alain Volmat]

Add a new binding of the i2c-stm32f7 driver to enable the handling
of the SMBUS-Alert.

The I2C/SMBUS framework already provides a mechanism to enable SMBus-Alert
by naming an IRQ line "smbus_alert". However, on stm32, the SMBus-Alert is
part of the i2c IRQ. Using the smbus_alert naming here would lead to having
2 handlers (the handler of the driver and the smbus_alert handler
from I2C/SMBUS framework) on the unique i2c IRQ of the stm32. Meaning that
the smbus_alert handler would get called for all IRQ generated by the stm32
I2C controller.

For that reason, the smbus_alert IRQ naming cannot be used and a dedicated
binding is introduced.

Signed-off-by: Alain Volmat <alain.volmat@st.com>
---
v2: Clarify commit message

 Documentation/devicetree/bindings/i2c/st,stm32-i2c.yaml | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/i2c/st,stm32-i2c.yaml b/Documentation/devicetree/bindings/i2c/st,stm32-i2c.yaml
index f2fcbb361180..6fde046fae5e 100644
--- a/Documentation/devicetree/bindings/i2c/st,stm32-i2c.yaml
+++ b/Documentation/devicetree/bindings/i2c/st,stm32-i2c.yaml
@@ -36,6 +36,10 @@ allOf:
             minItems: 3
             maxItems: 3
 
+        st,smbus-alert:
+          description: Enable the SMBus Alert feature
+          $ref: /schemas/types.yaml#/definitions/flag
+
   - if:
       properties:
         compatible:
-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 3/4] dt-bindings: i2c-stm32: add SMBus Alert bindings

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 1112 bytes --]

On Thu, Jun 25, 2020 at 09:39:28AM +0200, Alain Volmat wrote:
> Add a new binding of the i2c-stm32f7 driver to enable the handling
> of the SMBUS-Alert.
> 
> The I2C/SMBUS framework already provides a mechanism to enable SMBus-Alert
> by naming an IRQ line "smbus_alert". However, on stm32, the SMBus-Alert is
> part of the i2c IRQ. Using the smbus_alert naming here would lead to having
> 2 handlers (the handler of the driver and the smbus_alert handler
> from I2C/SMBUS framework) on the unique i2c IRQ of the stm32. Meaning that
> the smbus_alert handler would get called for all IRQ generated by the stm32
> I2C controller.
> 
> For that reason, the smbus_alert IRQ naming cannot be used and a dedicated
> binding is introduced.

What if we update the core to not register another irq handler if the
"smbus_alert" and main irq are the same?

I think it could work. However, while trying to make a proof-of-concept,
I found that irq descriptions in the generic i2c binding document are
probably mixed up. And before fixing that, I'd like to get HostNotify
done first.

Makes sense?


[-- 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

Re: [PATCH v2 3/4] dt-bindings: i2c-stm32: add SMBus Alert bindings

[Rob Herring]

On Tue, Jun 30, 2020 at 09:41:07PM +0200, Wolfram Sang wrote:
> On Thu, Jun 25, 2020 at 09:39:28AM +0200, Alain Volmat wrote:
> > Add a new binding of the i2c-stm32f7 driver to enable the handling
> > of the SMBUS-Alert.
> > 
> > The I2C/SMBUS framework already provides a mechanism to enable SMBus-Alert
> > by naming an IRQ line "smbus_alert". However, on stm32, the SMBus-Alert is
> > part of the i2c IRQ. Using the smbus_alert naming here would lead to having
> > 2 handlers (the handler of the driver and the smbus_alert handler
> > from I2C/SMBUS framework) on the unique i2c IRQ of the stm32. Meaning that
> > the smbus_alert handler would get called for all IRQ generated by the stm32
> > I2C controller.
> > 
> > For that reason, the smbus_alert IRQ naming cannot be used and a dedicated
> > binding is introduced.
> 
> What if we update the core to not register another irq handler if the
> "smbus_alert" and main irq are the same?
> 
> I think it could work. However, while trying to make a proof-of-concept,
> I found that irq descriptions in the generic i2c binding document are
> probably mixed up. And before fixing that, I'd like to get HostNotify
> done first.

Why does this even need to be in DT? Can't the driver just register that 
it supports SMBus alert or have some call to the core signaling an SMBus 
alert? 

Rob

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 3/4] dt-bindings: i2c-stm32: add SMBus Alert bindings

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 1529 bytes --]

Hi Rob,

> > > The I2C/SMBUS framework already provides a mechanism to enable SMBus-Alert
> > > by naming an IRQ line "smbus_alert". However, on stm32, the SMBus-Alert is
> > > part of the i2c IRQ. Using the smbus_alert naming here would lead to having
> > > 2 handlers (the handler of the driver and the smbus_alert handler
> > > from I2C/SMBUS framework) on the unique i2c IRQ of the stm32. Meaning that
> > > the smbus_alert handler would get called for all IRQ generated by the stm32
> > > I2C controller.
> > > 
> > > For that reason, the smbus_alert IRQ naming cannot be used and a dedicated
> > > binding is introduced.
> > 
> > What if we update the core to not register another irq handler if the
> > "smbus_alert" and main irq are the same?
> > 
> > I think it could work. However, while trying to make a proof-of-concept,
> > I found that irq descriptions in the generic i2c binding document are
> > probably mixed up. And before fixing that, I'd like to get HostNotify
> > done first.
> 
> Why does this even need to be in DT? Can't the driver just register that 
> it supports SMBus alert or have some call to the core signaling an SMBus 
> alert? 

If we emulate this SMBus behaviour with I2C, it means we apply
additional restrictions. In this case, there is an address which can't
be used anymore. Because there is another case of additional
restrictions, I proposed the binding "smbus" which means this bus is not
I2C but SMBus, so it is more restricted.

Thanks,

   Wolfram


[-- 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

[PATCH v2 4/4] i2c: stm32f7: Add SMBus-specific protocols support

[Alain Volmat]

This patch adds the support for SMBus Host notify and SMBus Alert
extensions protocols

This patch introduces the st,smbus-alert binding in order to enable
SMBus-Alert handling. Indeed, while the I2C/SMBUS framework already
provide a mechanism to enable SMBus-Alert by naming an IRQ line
"smbus_alert", on stm32 the SMBus-Alert is part of the i2c IRQ.
Using the smbus_alert naming here would lead to having 2 handlers
(the handler of the driver and the smbus_alert handler from I2C/SMBUS
framework) on the unique i2c IRQ of the stm32. Meaning that the
smbus_alert handler would get called for all IRQ generated by the stm32
I2C controller.

Signed-off-by: Alain Volmat <alain.volmat@st.com>
---
v2: adapt code to use reg_hnotify_cli / unreg_hnotify_cli instead
    of reg_client / unreg_client
    clarify commit message

 drivers/i2c/busses/Kconfig       |   1 +
 drivers/i2c/busses/i2c-stm32f7.c | 194 +++++++++++++++++++++++++++++++++++++--
 2 files changed, 185 insertions(+), 10 deletions(-)

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 735bf31a3fdf..ae8671727a4c 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -1036,6 +1036,7 @@ config I2C_STM32F7
 	tristate "STMicroelectronics STM32F7 I2C support"
 	depends on ARCH_STM32 || COMPILE_TEST
 	select I2C_SLAVE
+	select I2C_SMBUS
 	help
 	  Enable this option to add support for STM32 I2C controller embedded
 	  in STM32F7 SoCs.
diff --git a/drivers/i2c/busses/i2c-stm32f7.c b/drivers/i2c/busses/i2c-stm32f7.c
index bff3479fe122..d8f483ceeabb 100644
--- a/drivers/i2c/busses/i2c-stm32f7.c
+++ b/drivers/i2c/busses/i2c-stm32f7.c
@@ -14,10 +14,12 @@
  * This driver is based on i2c-stm32f4.c
  *
  */
+#include <linux/atomic.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/i2c.h>
+#include <linux/i2c-smbus.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/iopoll.h>
@@ -50,6 +52,8 @@
 
 /* STM32F7 I2C control 1 */
 #define STM32F7_I2C_CR1_PECEN			BIT(23)
+#define STM32F7_I2C_CR1_ALERTEN			BIT(22)
+#define STM32F7_I2C_CR1_SMBHEN			BIT(20)
 #define STM32F7_I2C_CR1_WUPEN			BIT(18)
 #define STM32F7_I2C_CR1_SBC			BIT(16)
 #define STM32F7_I2C_CR1_RXDMAEN			BIT(15)
@@ -121,6 +125,7 @@
 				(((n) & STM32F7_I2C_ISR_ADDCODE_MASK) >> 17)
 #define STM32F7_I2C_ISR_DIR			BIT(16)
 #define STM32F7_I2C_ISR_BUSY			BIT(15)
+#define STM32F7_I2C_ISR_ALERT			BIT(13)
 #define STM32F7_I2C_ISR_PECERR			BIT(11)
 #define STM32F7_I2C_ISR_ARLO			BIT(9)
 #define STM32F7_I2C_ISR_BERR			BIT(8)
@@ -134,6 +139,7 @@
 #define STM32F7_I2C_ISR_TXE			BIT(0)
 
 /* STM32F7 I2C Interrupt Clear */
+#define STM32F7_I2C_ICR_ALERTCF			BIT(13)
 #define STM32F7_I2C_ICR_PECCF			BIT(11)
 #define STM32F7_I2C_ICR_ARLOCF			BIT(9)
 #define STM32F7_I2C_ICR_BERRCF			BIT(8)
@@ -150,7 +156,7 @@
 
 #define STM32F7_I2C_MAX_LEN			0xff
 #define STM32F7_I2C_DMA_LEN_MIN			0x16
-#define STM32F7_I2C_MAX_SLAVE			0x2
+#define STM32F7_I2C_MAX_SLAVE			0x3
 
 #define STM32F7_I2C_DNF_DEFAULT			0
 #define STM32F7_I2C_DNF_MAX			16
@@ -275,6 +281,29 @@ struct stm32f7_i2c_msg {
 };
 
 /**
+ * struct stm32f7_i2c_host - SMBus host specific data
+ * @client: I2C slave device that represents SMBus host
+ * @notify_start: indicate that this is the start of the notify transaction
+ * @addr: device address of SMBus device that initiate SMBus host protocol
+ */
+struct stm32f7_i2c_host {
+	struct i2c_client *client;
+	bool notify_start;
+	u8 addr;
+};
+
+/**
+ * struct stm32f7_i2c_alert - SMBus alert specific data
+ * @setup: platform data for the smbus_alert i2c client
+ * @ara: I2C slave device used to respond to the SMBus Alert with Alert
+ * Response Address
+ */
+struct stm32f7_i2c_alert {
+	struct i2c_smbus_alert_setup setup;
+	struct i2c_client *ara;
+};
+
+/**
  * struct stm32f7_i2c_dev - private data of the controller
  * @adap: I2C adapter for this controller
  * @dev: device for this controller
@@ -301,6 +330,9 @@ struct stm32f7_i2c_msg {
  * @fmp_creg: register address for clearing Fast Mode Plus bits
  * @fmp_mask: mask for Fast Mode Plus bits in set register
  * @wakeup_src: boolean to know if the device is a wakeup source
+ * @host_notify_cnt: atomic to know number of host_notify enabled clients
+ * @host_notify_client: SMBus host-notify client
+ * @alert: SMBus alert specific data
  */
 struct stm32f7_i2c_dev {
 	struct i2c_adapter adap;
@@ -327,6 +359,9 @@ struct stm32f7_i2c_dev {
 	u32 fmp_creg;
 	u32 fmp_mask;
 	bool wakeup_src;
+	atomic_t host_notify_cnt;
+	struct i2c_client *host_notify_client;
+	struct stm32f7_i2c_alert *alert;
 };
 
 /*
@@ -1321,10 +1356,20 @@ static int stm32f7_i2c_get_free_slave_id(struct stm32f7_i2c_dev *i2c_dev,
 	int i;
 
 	/*
-	 * slave[0] supports 7-bit and 10-bit slave address
-	 * slave[1] supports 7-bit slave address only
+	 * slave[0] support only SMBus Host address (0x8)
+	 * slave[1] supports 7-bit and 10-bit slave address
+	 * slave[2] supports 7-bit slave address only
 	 */
-	for (i = STM32F7_I2C_MAX_SLAVE - 1; i >= 0; i--) {
+	if (atomic_read(&i2c_dev->host_notify_cnt)) {
+		if (slave->addr == 0x08) {
+			if (i2c_dev->slave[0])
+				goto fail;
+			*id = 0;
+			return 0;
+		}
+	}
+
+	for (i = STM32F7_I2C_MAX_SLAVE - 1; i > 0; i--) {
 		if (i == 1 && (slave->flags & I2C_CLIENT_TEN))
 			continue;
 		if (!i2c_dev->slave[i]) {
@@ -1333,6 +1378,7 @@ static int stm32f7_i2c_get_free_slave_id(struct stm32f7_i2c_dev *i2c_dev,
 		}
 	}
 
+fail:
 	dev_err(dev, "Slave 0x%x could not be registered\n", slave->addr);
 
 	return -EINVAL;
@@ -1586,6 +1632,13 @@ static irqreturn_t stm32f7_i2c_isr_error(int irq, void *data)
 		f7_msg->result = -EINVAL;
 	}
 
+	if (status & STM32F7_I2C_ISR_ALERT) {
+		dev_dbg(dev, "<%s>: SMBus alert received\n", __func__);
+		writel_relaxed(STM32F7_I2C_ICR_ALERTCF, base + STM32F7_I2C_ICR);
+		i2c_handle_smbus_alert(i2c_dev->alert->ara);
+		return IRQ_HANDLED;
+	}
+
 	if (!i2c_dev->slave_running) {
 		u32 mask;
 		/* Disable interrupts */
@@ -1776,7 +1829,13 @@ static int stm32f7_i2c_reg_slave(struct i2c_client *slave)
 	if (!stm32f7_i2c_is_slave_registered(i2c_dev))
 		stm32f7_i2c_enable_wakeup(i2c_dev, true);
 
-	if (id == 0) {
+	switch (id) {
+	case 0:
+		/* Slave SMBus Host */
+		i2c_dev->slave[id] = slave;
+		break;
+
+	case 1:
 		/* Configure Own Address 1 */
 		oar1 = readl_relaxed(i2c_dev->base + STM32F7_I2C_OAR1);
 		oar1 &= ~STM32F7_I2C_OAR1_MASK;
@@ -1789,7 +1848,9 @@ static int stm32f7_i2c_reg_slave(struct i2c_client *slave)
 		oar1 |= STM32F7_I2C_OAR1_OA1EN;
 		i2c_dev->slave[id] = slave;
 		writel_relaxed(oar1, i2c_dev->base + STM32F7_I2C_OAR1);
-	} else if (id == 1) {
+		break;
+
+	case 2:
 		/* Configure Own Address 2 */
 		oar2 = readl_relaxed(i2c_dev->base + STM32F7_I2C_OAR2);
 		oar2 &= ~STM32F7_I2C_OAR2_MASK;
@@ -1802,7 +1863,10 @@ static int stm32f7_i2c_reg_slave(struct i2c_client *slave)
 		oar2 |= STM32F7_I2C_OAR2_OA2EN;
 		i2c_dev->slave[id] = slave;
 		writel_relaxed(oar2, i2c_dev->base + STM32F7_I2C_OAR2);
-	} else {
+		break;
+
+	default:
+		dev_err(dev, "I2C slave id not supported\n");
 		ret = -ENODEV;
 		goto pm_free;
 	}
@@ -1843,10 +1907,10 @@ static int stm32f7_i2c_unreg_slave(struct i2c_client *slave)
 	if (ret < 0)
 		return ret;
 
-	if (id == 0) {
+	if (id == 1) {
 		mask = STM32F7_I2C_OAR1_OA1EN;
 		stm32f7_i2c_clr_bits(base + STM32F7_I2C_OAR1, mask);
-	} else {
+	} else if (id == 2) {
 		mask = STM32F7_I2C_OAR2_OA2EN;
 		stm32f7_i2c_clr_bits(base + STM32F7_I2C_OAR2, mask);
 	}
@@ -1911,6 +1975,99 @@ static int stm32f7_i2c_setup_fm_plus_bits(struct platform_device *pdev,
 					  &i2c_dev->fmp_mask);
 }
 
+static int stm32f7_i2c_enable_smbus_host(struct stm32f7_i2c_dev *i2c_dev)
+{
+	struct i2c_adapter *adap = &i2c_dev->adap;
+	void __iomem *base = i2c_dev->base;
+	struct i2c_client *client;
+
+	client = i2c_new_smbus_host_notify_device(adap);
+	if (IS_ERR(client))
+		return PTR_ERR(client);
+
+	i2c_dev->host_notify_client = client;
+
+	/* Enable SMBus Host address */
+	stm32f7_i2c_set_bits(base + STM32F7_I2C_CR1, STM32F7_I2C_CR1_SMBHEN);
+
+	return 0;
+}
+
+static void stm32f7_i2c_disable_smbus_host(struct stm32f7_i2c_dev *i2c_dev)
+{
+	void __iomem *base = i2c_dev->base;
+
+	if (i2c_dev->host_notify_client) {
+		/* Disable SMBus Host address */
+		stm32f7_i2c_clr_bits(base + STM32F7_I2C_CR1,
+				     STM32F7_I2C_CR1_SMBHEN);
+		i2c_free_smbus_host_notify_device(i2c_dev->host_notify_client);
+	}
+}
+
+static int stm32f7_i2c_reg_hnotify_client(struct i2c_client *client)
+{
+	struct stm32f7_i2c_dev *i2c_dev = i2c_get_adapdata(client->adapter);
+	int ret;
+
+	/* Only enable on the first device registration */
+	if (atomic_inc_return(&i2c_dev->host_notify_cnt) == 1) {
+		ret = stm32f7_i2c_enable_smbus_host(i2c_dev);
+		if (ret) {
+			dev_err(i2c_dev->dev,
+				"failed to enable SMBus host notify (%d)\n",
+				ret);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+static void stm32f7_i2c_unreg_hnotify_client(struct i2c_client *client)
+{
+	struct stm32f7_i2c_dev *i2c_dev = i2c_get_adapdata(client->adapter);
+
+	if (atomic_dec_return(&i2c_dev->host_notify_cnt) == 0)
+		stm32f7_i2c_disable_smbus_host(i2c_dev);
+}
+
+static int stm32f7_i2c_enable_smbus_alert(struct stm32f7_i2c_dev *i2c_dev)
+{
+	struct stm32f7_i2c_alert *alert;
+	struct i2c_adapter *adap = &i2c_dev->adap;
+	struct device *dev = i2c_dev->dev;
+	void __iomem *base = i2c_dev->base;
+
+	alert = devm_kzalloc(dev, sizeof(*alert), GFP_KERNEL);
+	if (!alert)
+		return -ENOMEM;
+
+	alert->ara = i2c_new_smbus_alert_device(adap, &alert->setup);
+	if (IS_ERR(alert->ara))
+		return PTR_ERR(alert->ara);
+
+	i2c_dev->alert = alert;
+
+	/* Enable SMBus Alert */
+	stm32f7_i2c_set_bits(base + STM32F7_I2C_CR1, STM32F7_I2C_CR1_ALERTEN);
+
+	return 0;
+}
+
+static void stm32f7_i2c_disable_smbus_alert(struct stm32f7_i2c_dev *i2c_dev)
+{
+	struct stm32f7_i2c_alert *alert = i2c_dev->alert;
+	void __iomem *base = i2c_dev->base;
+
+	if (alert) {
+		/* Disable SMBus Alert */
+		stm32f7_i2c_clr_bits(base + STM32F7_I2C_CR1,
+				     STM32F7_I2C_CR1_ALERTEN);
+		i2c_unregister_device(alert->ara);
+	}
+}
+
 static u32 stm32f7_i2c_func(struct i2c_adapter *adap)
 {
 	return I2C_FUNC_I2C | I2C_FUNC_10BIT_ADDR | I2C_FUNC_SLAVE |
@@ -1918,7 +2075,7 @@ static u32 stm32f7_i2c_func(struct i2c_adapter *adap)
 		I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA |
 		I2C_FUNC_SMBUS_BLOCK_DATA | I2C_FUNC_SMBUS_BLOCK_PROC_CALL |
 		I2C_FUNC_SMBUS_PROC_CALL | I2C_FUNC_SMBUS_PEC |
-		I2C_FUNC_SMBUS_I2C_BLOCK;
+		I2C_FUNC_SMBUS_I2C_BLOCK | I2C_FUNC_SMBUS_HOST_NOTIFY;
 }
 
 static const struct i2c_algorithm stm32f7_i2c_algo = {
@@ -1927,6 +2084,8 @@ static const struct i2c_algorithm stm32f7_i2c_algo = {
 	.functionality = stm32f7_i2c_func,
 	.reg_slave = stm32f7_i2c_reg_slave,
 	.unreg_slave = stm32f7_i2c_unreg_slave,
+	.reg_hnotify_cli = stm32f7_i2c_reg_hnotify_client,
+	.unreg_hnotify_cli = stm32f7_i2c_unreg_hnotify_client,
 };
 
 static int stm32f7_i2c_probe(struct platform_device *pdev)
@@ -2088,6 +2247,16 @@ static int stm32f7_i2c_probe(struct platform_device *pdev)
 	if (ret)
 		goto pm_disable;
 
+	if (device_property_read_bool(&pdev->dev, "st,smbus-alert")) {
+		ret = stm32f7_i2c_enable_smbus_alert(i2c_dev);
+		if (ret) {
+			dev_err(i2c_dev->dev,
+				"failed to enable SMBus alert protocol (%d)\n",
+				ret);
+			goto i2c_adapter_remove;
+		}
+	}
+
 	dev_info(i2c_dev->dev, "STM32F7 I2C-%d bus adapter\n", adap->nr);
 
 	pm_runtime_mark_last_busy(i2c_dev->dev);
@@ -2095,6 +2264,9 @@ static int stm32f7_i2c_probe(struct platform_device *pdev)
 
 	return 0;
 
+i2c_adapter_remove:
+	i2c_del_adapter(adap);
+
 pm_disable:
 	pm_runtime_put_noidle(i2c_dev->dev);
 	pm_runtime_disable(i2c_dev->dev);
@@ -2126,6 +2298,8 @@ static int stm32f7_i2c_remove(struct platform_device *pdev)
 {
 	struct stm32f7_i2c_dev *i2c_dev = platform_get_drvdata(pdev);
 
+	stm32f7_i2c_disable_smbus_alert(i2c_dev);
+
 	i2c_del_adapter(&i2c_dev->adap);
 	pm_runtime_get_sync(i2c_dev->dev);
 
-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 0/4] stm32-f7: Addition of SMBus Alert / Host-notify features

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 273 bytes --]

On Thu, Jun 25, 2020 at 09:39:25AM +0200, Alain Volmat wrote:
> This serie adds SMBus Alert and SMBus Host-Notify features for the i2c-stm32f7.

If it is not too much work for you, I think it makes sense to split the
series into two, i.e. HostNotify and SMBusAlert parts.


[-- 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

Re: [PATCH v2 0/4] stm32-f7: Addition of SMBus Alert / Host-notify features

[Alain Volmat]

On Tue, Jun 30, 2020 at 06:05:00PM +0200, Wolfram Sang wrote:
> On Thu, Jun 25, 2020 at 09:39:25AM +0200, Alain Volmat wrote:
> > This serie adds SMBus Alert and SMBus Host-Notify features for the i2c-stm32f7.
> 
> If it is not too much work for you, I think it makes sense to split the
> series into two, i.e. HostNotify and SMBusAlert parts.
> 

I've just prepared the 1st new serie with only the HostNotify bits in it.
(basically, the core part, the dt-bindings with the enable-host-notify, and
the usage within i2c-stm32f7).
You mentioned in the other thread that you still have some more review comment
I believe. Is that right ? If that is so, I'll wait for those comment and
then push that new serie for review.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 0/4] stm32-f7: Addition of SMBus Alert / Host-notify features

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 458 bytes --]


> I've just prepared the 1st new serie with only the HostNotify bits in it.
> (basically, the core part, the dt-bindings with the enable-host-notify, and
> the usage within i2c-stm32f7).

Cool, thanks!

> You mentioned in the other thread that you still have some more review comment
> I believe. Is that right ? If that is so, I'll wait for those comment and
> then push that new serie for review.

Yes, for the core part. Please wait for these comments.


[-- 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