LKML Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/4] stm32-f7: Addition of SMBus Alert / Host-notify features
@ 2020-05-05  5:51 Alain Volmat
  2020-05-05  5:51 ` [PATCH 1/4] i2c: smbus: add core function handling SMBus host-notify Alain Volmat
                   ` (3 more replies)
  0 siblings, 4 replies; 18+ messages in thread
From: Alain Volmat @ 2020-05-05  5:51 UTC (permalink / raw)
  To: wsa, robh+dt
  Cc: mark.rutland, pierre-yves.mordret, mcoquelin.stm32,
	alexandre.torgue, linux-i2c, devicetree, linux-stm32,
	linux-arm-kernel, linux-kernel, fabrice.gasnier, alain.volmat

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

For that purpore, I propose two enhancements to the i2c framework.
1. Addition of host-notify client handling as part of the
i2c-core-smbus so that any other i2c adapter can benefit from it,
even those without specific HW support for Host-Notify.
2. Addition of two i2c_algorithm reg_client and unreg_client allowing
adapter to take some actions when a client is being probed.
Indeed, in case of host-notify, the binding/flag is related to the
client and the adapter might want to enable the host-notify feature
only when there is a client requesting the host-notify.
(Since SMBus host-notify is using the address 0x08 which is a valid
I2C address, keeping host-notify always enabled would make the 0x08
I2C slave address non usable).

Alain Volmat (4):
  i2c: smbus: add core function handling SMBus host-notify
  i2c: addition of client 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              | 198 +++++++++++++++++-
 drivers/i2c/i2c-core-base.c                   |  11 +
 drivers/i2c/i2c-core-smbus.c                  | 105 ++++++++++
 include/linux/i2c-smbus.h                     |   2 +
 include/linux/i2c.h                           |   6 +
 7 files changed, 317 insertions(+), 10 deletions(-)

-- 
2.17.1

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH 1/4] i2c: smbus: add core function handling SMBus host-notify
  2020-05-05  5:51 [PATCH 0/4] stm32-f7: Addition of SMBus Alert / Host-notify features Alain Volmat
@ 2020-05-05  5:51 ` Alain Volmat
  2020-05-11  8:26   ` Pierre Yves MORDRET
  2020-05-23 10:46   ` Wolfram Sang
  2020-05-05  5:51 ` [PATCH 2/4] i2c: addition of client reg/unreg callbacks Alain Volmat
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 18+ messages in thread
From: Alain Volmat @ 2020-05-05  5:51 UTC (permalink / raw)
  To: wsa, robh+dt
  Cc: mark.rutland, pierre-yves.mordret, mcoquelin.stm32,
	alexandre.torgue, linux-i2c, devicetree, linux-stm32,
	linux-arm-kernel, linux-kernel, fabrice.gasnier, 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>
---
 drivers/i2c/i2c-core-smbus.c | 105 +++++++++++++++++++++++++++++++++++
 include/linux/i2c-smbus.h    |   2 +
 2 files changed, 107 insertions(+)

diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
index b34d2ff06931..0c7e135c73e1 100644
--- a/drivers/i2c/i2c-core-smbus.c
+++ b/drivers/i2c/i2c-core-smbus.c
@@ -708,3 +708,108 @@ int of_i2c_setup_smbus_alert(struct i2c_adapter *adapter)
 }
 EXPORT_SYMBOL_GPL(of_i2c_setup_smbus_alert);
 #endif
+
+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 way 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:
+		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);
+			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);
diff --git a/include/linux/i2c-smbus.h b/include/linux/i2c-smbus.h
index 8c5459034f92..926f6d8ae30d 100644
--- a/include/linux/i2c-smbus.h
+++ b/include/linux/i2c-smbus.h
@@ -38,5 +38,7 @@ 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);
 
 #endif /* _LINUX_I2C_SMBUS_H */
-- 
2.17.1


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH 2/4] i2c: addition of client reg/unreg callbacks
  2020-05-05  5:51 [PATCH 0/4] stm32-f7: Addition of SMBus Alert / Host-notify features Alain Volmat
  2020-05-05  5:51 ` [PATCH 1/4] i2c: smbus: add core function handling SMBus host-notify Alain Volmat
@ 2020-05-05  5:51 ` Alain Volmat
  2020-05-11  8:28   ` Pierre Yves MORDRET
  2020-05-23 10:49   ` Wolfram Sang
  2020-05-05  5:51 ` [PATCH 3/4] dt-bindings: i2c-stm32: add SMBus Alert bindings Alain Volmat
  2020-05-05  5:51 ` [PATCH 4/4] i2c: stm32f7: Add SMBus-specific protocols support Alain Volmat
  3 siblings, 2 replies; 18+ messages in thread
From: Alain Volmat @ 2020-05-05  5:51 UTC (permalink / raw)
  To: wsa, robh+dt
  Cc: mark.rutland, pierre-yves.mordret, mcoquelin.stm32,
	alexandre.torgue, linux-i2c, devicetree, linux-stm32,
	linux-arm-kernel, linux-kernel, fabrice.gasnier, alain.volmat

Addition of two callbacks reg_client and unreg_client that can be
implemented by adapter drivers in order to take action whenever a
client is being registered to it.

Signed-off-by: Alain Volmat <alain.volmat@st.com>
---
 drivers/i2c/i2c-core-base.c | 11 +++++++++++
 include/linux/i2c.h         |  6 ++++++
 2 files changed, 17 insertions(+)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 2e4560671183..4c84c6264314 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -319,6 +319,12 @@ static int i2c_device_probe(struct device *dev)
 	if (!client)
 		return 0;
 
+	if (client->adapter->algo->reg_client) {
+		status = client->adapter->algo->reg_client(client);
+		if (status)
+			return status;
+	}
+
 	driver = to_i2c_driver(dev->driver);
 
 	client->irq = client->init_irq;
@@ -417,6 +423,8 @@ static int i2c_device_probe(struct device *dev)
 put_sync_adapter:
 	if (client->flags & I2C_CLIENT_HOST_NOTIFY)
 		pm_runtime_put_sync(&client->adapter->dev);
+	if (client->adapter->algo->reg_client)
+		client->adapter->algo->unreg_client(client);
 
 	return status;
 }
@@ -445,6 +453,9 @@ static int i2c_device_remove(struct device *dev)
 	if (client->flags & I2C_CLIENT_HOST_NOTIFY)
 		pm_runtime_put(&client->adapter->dev);
 
+	if (client->adapter->algo->unreg_client)
+		client->adapter->algo->unreg_client(client);
+
 	return status;
 }
 
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 45d36ba4826b..61b838caf454 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -509,6 +509,8 @@ 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_client: Callback informing that a new client is being registered
+ * @unreg_client: Callback informing that a client 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
  *
@@ -545,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 client */
+	int (*reg_client)(struct i2c_client *client);
+	void (*unreg_client)(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.17.1


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH 3/4] dt-bindings: i2c-stm32: add SMBus Alert bindings
  2020-05-05  5:51 [PATCH 0/4] stm32-f7: Addition of SMBus Alert / Host-notify features Alain Volmat
  2020-05-05  5:51 ` [PATCH 1/4] i2c: smbus: add core function handling SMBus host-notify Alain Volmat
  2020-05-05  5:51 ` [PATCH 2/4] i2c: addition of client reg/unreg callbacks Alain Volmat
@ 2020-05-05  5:51 ` Alain Volmat
  2020-05-13  2:19   ` Rob Herring
  2020-05-05  5:51 ` [PATCH 4/4] i2c: stm32f7: Add SMBus-specific protocols support Alain Volmat
  3 siblings, 1 reply; 18+ messages in thread
From: Alain Volmat @ 2020-05-05  5:51 UTC (permalink / raw)
  To: wsa, robh+dt
  Cc: mark.rutland, pierre-yves.mordret, mcoquelin.stm32,
	alexandre.torgue, linux-i2c, devicetree, linux-stm32,
	linux-arm-kernel, linux-kernel, fabrice.gasnier, alain.volmat

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

Signed-off-by: Alain Volmat <alain.volmat@st.com>
---
 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 b50a2f420b36..04c0882c3661 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.17.1


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH 4/4] i2c: stm32f7: Add SMBus-specific protocols support
  2020-05-05  5:51 [PATCH 0/4] stm32-f7: Addition of SMBus Alert / Host-notify features Alain Volmat
                   ` (2 preceding siblings ...)
  2020-05-05  5:51 ` [PATCH 3/4] dt-bindings: i2c-stm32: add SMBus Alert bindings Alain Volmat
@ 2020-05-05  5:51 ` Alain Volmat
  2020-05-11  8:27   ` Pierre Yves MORDRET
  2020-05-23 11:01   ` Wolfram Sang
  3 siblings, 2 replies; 18+ messages in thread
From: Alain Volmat @ 2020-05-05  5:51 UTC (permalink / raw)
  To: wsa, robh+dt
  Cc: mark.rutland, pierre-yves.mordret, mcoquelin.stm32,
	alexandre.torgue, linux-i2c, devicetree, linux-stm32,
	linux-arm-kernel, linux-kernel, fabrice.gasnier, alain.volmat

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

Signed-off-by: Alain Volmat <alain.volmat@st.com>
---
 drivers/i2c/busses/Kconfig       |   1 +
 drivers/i2c/busses/i2c-stm32f7.c | 198 +++++++++++++++++++++++++++++--
 2 files changed, 189 insertions(+), 10 deletions(-)

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 2f6e39b41e6c..b82c2f7d7d50 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -1024,6 +1024,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 9c9e10ea9199..6d02ddbc1ab4 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
@@ -274,6 +280,29 @@ struct stm32f7_i2c_msg {
 	u8 smbus_buf[I2C_SMBUS_BLOCK_MAX + 3] __aligned(4);
 };
 
+/**
+ * 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
@@ -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,103 @@ 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_client(struct i2c_client *client)
+{
+	struct stm32f7_i2c_dev *i2c_dev = i2c_get_adapdata(client->adapter);
+	int ret;
+
+	if (client->flags & I2C_CLIENT_HOST_NOTIFY) {
+		/* 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_client(struct i2c_client *client)
+{
+	struct stm32f7_i2c_dev *i2c_dev = i2c_get_adapdata(client->adapter);
+
+	if (client->flags & I2C_CLIENT_HOST_NOTIFY) {
+		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 +2079,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 +2088,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_client = stm32f7_i2c_reg_client,
+	.unreg_client = stm32f7_i2c_unreg_client,
 };
 
 static int stm32f7_i2c_probe(struct platform_device *pdev)
@@ -2088,6 +2251,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 +2268,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 +2302,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.17.1


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/4] i2c: smbus: add core function handling SMBus host-notify
  2020-05-05  5:51 ` [PATCH 1/4] i2c: smbus: add core function handling SMBus host-notify Alain Volmat
@ 2020-05-11  8:26   ` Pierre Yves MORDRET
  2020-05-23 10:46   ` Wolfram Sang
  1 sibling, 0 replies; 18+ messages in thread
From: Pierre Yves MORDRET @ 2020-05-11  8:26 UTC (permalink / raw)
  To: Alain Volmat, wsa, robh+dt
  Cc: mark.rutland, mcoquelin.stm32, alexandre.torgue, linux-i2c,
	devicetree, linux-stm32, linux-arm-kernel, linux-kernel,
	fabrice.gasnier

Hi all,

Reviewed-by: Pierre-Yves MORDRET <pierre-yves.mordret@st.com>

Thanks

On 5/5/20 7:51 AM, 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>
> ---
>  drivers/i2c/i2c-core-smbus.c | 105 +++++++++++++++++++++++++++++++++++
>  include/linux/i2c-smbus.h    |   2 +
>  2 files changed, 107 insertions(+)
> 
> diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
> index b34d2ff06931..0c7e135c73e1 100644
> --- a/drivers/i2c/i2c-core-smbus.c
> +++ b/drivers/i2c/i2c-core-smbus.c
> @@ -708,3 +708,108 @@ int of_i2c_setup_smbus_alert(struct i2c_adapter *adapter)
>  }
>  EXPORT_SYMBOL_GPL(of_i2c_setup_smbus_alert);
>  #endif
> +
> +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 way 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:
> +		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);
> +			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);
> diff --git a/include/linux/i2c-smbus.h b/include/linux/i2c-smbus.h
> index 8c5459034f92..926f6d8ae30d 100644
> --- a/include/linux/i2c-smbus.h
> +++ b/include/linux/i2c-smbus.h
> @@ -38,5 +38,7 @@ 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);
>  
>  #endif /* _LINUX_I2C_SMBUS_H */
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 4/4] i2c: stm32f7: Add SMBus-specific protocols support
  2020-05-05  5:51 ` [PATCH 4/4] i2c: stm32f7: Add SMBus-specific protocols support Alain Volmat
@ 2020-05-11  8:27   ` Pierre Yves MORDRET
  2020-05-23 11:01   ` Wolfram Sang
  1 sibling, 0 replies; 18+ messages in thread
From: Pierre Yves MORDRET @ 2020-05-11  8:27 UTC (permalink / raw)
  To: Alain Volmat, wsa, robh+dt
  Cc: mark.rutland, mcoquelin.stm32, alexandre.torgue, linux-i2c,
	devicetree, linux-stm32, linux-arm-kernel, linux-kernel,
	fabrice.gasnier

Hi all,

Reviewed-by: Pierre-Yves MORDRET <pierre-yves.mordret@st.com>

Thanks

On 5/5/20 7:51 AM, Alain Volmat wrote:
> This patch adds the support for SMBus Host notify and SMBus Alert
> extensions protocols
> 
> Signed-off-by: Alain Volmat <alain.volmat@st.com>
> ---
>  drivers/i2c/busses/Kconfig       |   1 +
>  drivers/i2c/busses/i2c-stm32f7.c | 198 +++++++++++++++++++++++++++++--
>  2 files changed, 189 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 2f6e39b41e6c..b82c2f7d7d50 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -1024,6 +1024,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 9c9e10ea9199..6d02ddbc1ab4 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
> @@ -274,6 +280,29 @@ struct stm32f7_i2c_msg {
>  	u8 smbus_buf[I2C_SMBUS_BLOCK_MAX + 3] __aligned(4);
>  };
>  
> +/**
> + * 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
> @@ -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,103 @@ 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_client(struct i2c_client *client)
> +{
> +	struct stm32f7_i2c_dev *i2c_dev = i2c_get_adapdata(client->adapter);
> +	int ret;
> +
> +	if (client->flags & I2C_CLIENT_HOST_NOTIFY) {
> +		/* 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_client(struct i2c_client *client)
> +{
> +	struct stm32f7_i2c_dev *i2c_dev = i2c_get_adapdata(client->adapter);
> +
> +	if (client->flags & I2C_CLIENT_HOST_NOTIFY) {
> +		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 +2079,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 +2088,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_client = stm32f7_i2c_reg_client,
> +	.unreg_client = stm32f7_i2c_unreg_client,
>  };
>  
>  static int stm32f7_i2c_probe(struct platform_device *pdev)
> @@ -2088,6 +2251,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 +2268,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 +2302,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);
>  
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/4] i2c: addition of client reg/unreg callbacks
  2020-05-05  5:51 ` [PATCH 2/4] i2c: addition of client reg/unreg callbacks Alain Volmat
@ 2020-05-11  8:28   ` Pierre Yves MORDRET
  2020-05-23 10:49   ` Wolfram Sang
  1 sibling, 0 replies; 18+ messages in thread
From: Pierre Yves MORDRET @ 2020-05-11  8:28 UTC (permalink / raw)
  To: Alain Volmat, wsa, robh+dt
  Cc: mark.rutland, mcoquelin.stm32, alexandre.torgue, linux-i2c,
	devicetree, linux-stm32, linux-arm-kernel, linux-kernel,
	fabrice.gasnier

Hi all,

Reviewed-by: Pierre-Yves MORDRET <pierre-yves.mordret@st.com>

Thanks

On 5/5/20 7:51 AM, Alain Volmat wrote:
> Addition of two callbacks reg_client and unreg_client that can be
> implemented by adapter drivers in order to take action whenever a
> client is being registered to it.
> 
> Signed-off-by: Alain Volmat <alain.volmat@st.com>
> ---
>  drivers/i2c/i2c-core-base.c | 11 +++++++++++
>  include/linux/i2c.h         |  6 ++++++
>  2 files changed, 17 insertions(+)
> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 2e4560671183..4c84c6264314 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -319,6 +319,12 @@ static int i2c_device_probe(struct device *dev)
>  	if (!client)
>  		return 0;
>  
> +	if (client->adapter->algo->reg_client) {
> +		status = client->adapter->algo->reg_client(client);
> +		if (status)
> +			return status;
> +	}
> +
>  	driver = to_i2c_driver(dev->driver);
>  
>  	client->irq = client->init_irq;
> @@ -417,6 +423,8 @@ static int i2c_device_probe(struct device *dev)
>  put_sync_adapter:
>  	if (client->flags & I2C_CLIENT_HOST_NOTIFY)
>  		pm_runtime_put_sync(&client->adapter->dev);
> +	if (client->adapter->algo->reg_client)
> +		client->adapter->algo->unreg_client(client);
>  
>  	return status;
>  }
> @@ -445,6 +453,9 @@ static int i2c_device_remove(struct device *dev)
>  	if (client->flags & I2C_CLIENT_HOST_NOTIFY)
>  		pm_runtime_put(&client->adapter->dev);
>  
> +	if (client->adapter->algo->unreg_client)
> +		client->adapter->algo->unreg_client(client);
> +
>  	return status;
>  }
>  
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index 45d36ba4826b..61b838caf454 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -509,6 +509,8 @@ 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_client: Callback informing that a new client is being registered
> + * @unreg_client: Callback informing that a client 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
>   *
> @@ -545,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 client */
> +	int (*reg_client)(struct i2c_client *client);
> +	void (*unreg_client)(struct i2c_client *client);
> +
>  #if IS_ENABLED(CONFIG_I2C_SLAVE)
>  	int (*reg_slave)(struct i2c_client *client);
>  	int (*unreg_slave)(struct i2c_client *client);
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 3/4] dt-bindings: i2c-stm32: add SMBus Alert bindings
  2020-05-05  5:51 ` [PATCH 3/4] dt-bindings: i2c-stm32: add SMBus Alert bindings Alain Volmat
@ 2020-05-13  2:19   ` Rob Herring
  2020-05-13  5:42     ` Alain Volmat
  0 siblings, 1 reply; 18+ messages in thread
From: Rob Herring @ 2020-05-13  2:19 UTC (permalink / raw)
  To: Alain Volmat
  Cc: wsa, mark.rutland, pierre-yves.mordret, mcoquelin.stm32,
	alexandre.torgue, linux-i2c, devicetree, linux-stm32,
	linux-arm-kernel, linux-kernel, fabrice.gasnier

On Tue, May 05, 2020 at 07:51:10AM +0200, Alain Volmat wrote:
> Add a new binding of the i2c-stm32f7 driver to enable the handling
> of the SMBUS-Alert
> 
> Signed-off-by: Alain Volmat <alain.volmat@st.com>
> ---
>  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 b50a2f420b36..04c0882c3661 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
> +

We already have smbus_alert interrupt. Can't you just check for this in 
the slave nodes and enable if found?

>    - if:
>        properties:
>          compatible:
> -- 
> 2.17.1
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 3/4] dt-bindings: i2c-stm32: add SMBus Alert bindings
  2020-05-13  2:19   ` Rob Herring
@ 2020-05-13  5:42     ` Alain Volmat
  2020-05-18 10:53       ` Alain Volmat
  2020-05-23 10:36       ` wsa
  0 siblings, 2 replies; 18+ messages in thread
From: Alain Volmat @ 2020-05-13  5:42 UTC (permalink / raw)
  To: Rob Herring
  Cc: wsa, mark.rutland, Pierre Yves MORDRET, mcoquelin.stm32,
	Alexandre TORGUE, linux-i2c, devicetree, linux-stm32,
	linux-arm-kernel, linux-kernel, Fabrice GASNIER

Hello Rob,

On Wed, May 13, 2020 at 02:19:32AM +0000, Rob Herring wrote:
> On Tue, May 05, 2020 at 07:51:10AM +0200, Alain Volmat wrote:
> > Add a new binding of the i2c-stm32f7 driver to enable the handling
> > of the SMBUS-Alert
> > 
> > Signed-off-by: Alain Volmat <alain.volmat@st.com>
> > ---
> >  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 b50a2f420b36..04c0882c3661 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
> > +
> 
> We already have smbus_alert interrupt. Can't you just check for this in 
> the slave nodes and enable if found?

My understanding reading the code (smbalert_probe within i2c-smbus.c, of_i2c_setup_smbus_alert called when
registering an adapter within i2c-core-smbus.c) is that smbus_alert refers to an interrupt on the
adapter side. That is an interrupt that would be triggered when the adapter is receiving an smbus_alert
message.
In our case (stm32f7), we do not have specific interrupt for that purpose. The interrupt triggered when
an SMBUS Alert is received (by the adapter) is the same interrupt as for other reasons and we check
within the irq handler within stm32f7 the reason before calling i2c_handle_smbus_alert if the status
register indicated an SMBUS Alert.
So my understanding is that we cannot rely on the mechanism of naming an interrupt smbus_alert.
Did I misunderstood something ?

> 
> >    - if:
> >        properties:
> >          compatible:
> > -- 
> > 2.17.1
> > 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 3/4] dt-bindings: i2c-stm32: add SMBus Alert bindings
  2020-05-13  5:42     ` Alain Volmat
@ 2020-05-18 10:53       ` Alain Volmat
  2020-05-23 10:36       ` wsa
  1 sibling, 0 replies; 18+ messages in thread
From: Alain Volmat @ 2020-05-18 10:53 UTC (permalink / raw)
  To: Rob Herring, wsa
  Cc: mark.rutland, Pierre Yves MORDRET, mcoquelin.stm32,
	Alexandre TORGUE, linux-i2c, devicetree, linux-stm32,
	linux-arm-kernel, linux-kernel, Fabrice GASNIER

Gentle Reminder, as I wrote in my previous responce, smbus_alert interrupt
refers to an host and not a client. And since we do not have a dedicated
irq for smbus_alert, I propose to add this st, binding to enable the
smbus_alert mechanism.

On Wed, May 13, 2020 at 07:42:31AM +0200, Alain Volmat wrote:
> Hello Rob,
> 
> On Wed, May 13, 2020 at 02:19:32AM +0000, Rob Herring wrote:
> > On Tue, May 05, 2020 at 07:51:10AM +0200, Alain Volmat wrote:
> > > Add a new binding of the i2c-stm32f7 driver to enable the handling
> > > of the SMBUS-Alert
> > > 
> > > Signed-off-by: Alain Volmat <alain.volmat@st.com>
> > > ---
> > >  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 b50a2f420b36..04c0882c3661 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
> > > +
> > 
> > We already have smbus_alert interrupt. Can't you just check for this in 
> > the slave nodes and enable if found?
> 
> My understanding reading the code (smbalert_probe within i2c-smbus.c, of_i2c_setup_smbus_alert called when
> registering an adapter within i2c-core-smbus.c) is that smbus_alert refers to an interrupt on the
> adapter side. That is an interrupt that would be triggered when the adapter is receiving an smbus_alert
> message.
> In our case (stm32f7), we do not have specific interrupt for that purpose. The interrupt triggered when
> an SMBUS Alert is received (by the adapter) is the same interrupt as for other reasons and we check
> within the irq handler within stm32f7 the reason before calling i2c_handle_smbus_alert if the status
> register indicated an SMBUS Alert.
> So my understanding is that we cannot rely on the mechanism of naming an interrupt smbus_alert.
> Did I misunderstood something ?
> 
> > 
> > >    - if:
> > >        properties:
> > >          compatible:
> > > -- 
> > > 2.17.1
> > > 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 3/4] dt-bindings: i2c-stm32: add SMBus Alert bindings
  2020-05-13  5:42     ` Alain Volmat
  2020-05-18 10:53       ` Alain Volmat
@ 2020-05-23 10:36       ` wsa
  2020-05-26 10:31         ` Alain Volmat
  1 sibling, 1 reply; 18+ messages in thread
From: wsa @ 2020-05-23 10:36 UTC (permalink / raw)
  To: Rob Herring, mark.rutland, Pierre Yves MORDRET, mcoquelin.stm32,
	Alexandre TORGUE, linux-i2c, devicetree, linux-stm32,
	linux-arm-kernel, linux-kernel, Fabrice GASNIER


[-- Attachment #1: Type: text/plain, Size: 1309 bytes --]


> > > +        st,smbus-alert:
> > > +          description: Enable the SMBus Alert feature
> > > +          $ref: /schemas/types.yaml#/definitions/flag
> > > +
> > 
> > We already have smbus_alert interrupt. Can't you just check for this in 
> > the slave nodes and enable if found?
> 
> My understanding reading the code (smbalert_probe within i2c-smbus.c, of_i2c_setup_smbus_alert called when
> registering an adapter within i2c-core-smbus.c) is that smbus_alert refers to an interrupt on the
> adapter side. That is an interrupt that would be triggered when the adapter is receiving an smbus_alert
> message.
> In our case (stm32f7), we do not have specific interrupt for that purpose. The interrupt triggered when
> an SMBUS Alert is received (by the adapter) is the same interrupt as for other reasons and we check
> within the irq handler within stm32f7 the reason before calling i2c_handle_smbus_alert if the status
> register indicated an SMBUS Alert.
> So my understanding is that we cannot rely on the mechanism of naming an interrupt smbus_alert.
> Did I misunderstood something ?

I just wonder what is bad about specifying the same interrupt twice in
the interrupt properties? You could then check in probe if "smbus_alert"
is populated and if it matches the main irq.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/4] i2c: smbus: add core function handling SMBus host-notify
  2020-05-05  5:51 ` [PATCH 1/4] i2c: smbus: add core function handling SMBus host-notify Alain Volmat
  2020-05-11  8:26   ` Pierre Yves MORDRET
@ 2020-05-23 10:46   ` Wolfram Sang
  2020-05-26 10:23     ` Alain Volmat
  1 sibling, 1 reply; 18+ messages in thread
From: Wolfram Sang @ 2020-05-23 10:46 UTC (permalink / raw)
  To: Alain Volmat, Benjamin Tissoires
  Cc: robh+dt, mark.rutland, pierre-yves.mordret, mcoquelin.stm32,
	alexandre.torgue, linux-i2c, devicetree, linux-stm32,
	linux-arm-kernel, linux-kernel, fabrice.gasnier


[-- Attachment #1: Type: text/plain, Size: 2278 bytes --]


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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/4] i2c: addition of client reg/unreg callbacks
  2020-05-05  5:51 ` [PATCH 2/4] i2c: addition of client reg/unreg callbacks Alain Volmat
  2020-05-11  8:28   ` Pierre Yves MORDRET
@ 2020-05-23 10:49   ` Wolfram Sang
  1 sibling, 0 replies; 18+ messages in thread
From: Wolfram Sang @ 2020-05-23 10:49 UTC (permalink / raw)
  To: Alain Volmat
  Cc: robh+dt, mark.rutland, pierre-yves.mordret, mcoquelin.stm32,
	alexandre.torgue, linux-i2c, devicetree, linux-stm32,
	linux-arm-kernel, linux-kernel, fabrice.gasnier


[-- Attachment #1: Type: text/plain, Size: 540 bytes --]

On Tue, May 05, 2020 at 07:51:09AM +0200, Alain Volmat wrote:
> Addition of two callbacks reg_client and unreg_client that can be
> implemented by adapter drivers in order to take action whenever a
> client is being registered to it.
> 
> Signed-off-by: Alain Volmat <alain.volmat@st.com>

Sorry, but NACK. After years of work, we just recently got rid of attach
and detach callbacks. They were abused in quite a lot of ways. I think
we can find other solutions to what you need them for. Let's move the
discussion to patch 4.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 4/4] i2c: stm32f7: Add SMBus-specific protocols support
  2020-05-05  5:51 ` [PATCH 4/4] i2c: stm32f7: Add SMBus-specific protocols support Alain Volmat
  2020-05-11  8:27   ` Pierre Yves MORDRET
@ 2020-05-23 11:01   ` Wolfram Sang
  2020-05-26 10:39     ` Alain Volmat
  1 sibling, 1 reply; 18+ messages in thread
From: Wolfram Sang @ 2020-05-23 11:01 UTC (permalink / raw)
  To: Alain Volmat, Benjamin Tissoires
  Cc: robh+dt, mark.rutland, pierre-yves.mordret, mcoquelin.stm32,
	alexandre.torgue, linux-i2c, devicetree, linux-stm32,
	linux-arm-kernel, linux-kernel, fabrice.gasnier


[-- Attachment #1: Type: text/plain, Size: 980 bytes --]


> +static int stm32f7_i2c_reg_client(struct i2c_client *client)
> +{
> +	struct stm32f7_i2c_dev *i2c_dev = i2c_get_adapdata(client->adapter);
> +	int ret;
> +
> +	if (client->flags & I2C_CLIENT_HOST_NOTIFY) {
> +		/* 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;
> +}

So, as mentioned in the other review, I'd like to evaluate other
possibilities for the above:

- One option is to enable it globally in probe(). Then you lose the
  possibility to have a device at address 0x08.
- Enable it in probe() only if there is a generic binding "host-notify".
- Let the core scan for a device with HOST_NOTIFY when registering an
  adapter and then call back into the driver somehow?

Other ideas?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/4] i2c: smbus: add core function handling SMBus host-notify
  2020-05-23 10:46   ` Wolfram Sang
@ 2020-05-26 10:23     ` Alain Volmat
  0 siblings, 0 replies; 18+ messages in thread
From: Alain Volmat @ 2020-05-26 10:23 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: Benjamin Tissoires, robh+dt, mark.rutland, Pierre Yves MORDRET,
	mcoquelin.stm32, Alexandre TORGUE, linux-i2c, devicetree,
	linux-stm32, linux-arm-kernel, linux-kernel, Fabrice GASNIER

On Sat, May 23, 2020 at 10:46:25AM +0000, Wolfram Sang wrote:
> 
> 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.

Indeed. Such support can be added later on once such device is found. For the
time being I will state "no support"

> 
> > +		 */
> > +		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?

Ok. I will check that notify_start is false before calling host_notify
(since otherwise it will call i2c_handle_smbus_host_notify with a bad addr
value) and reset notify_start to false if it is still true.

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

Agreed, this error can be removed.

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

Yes, I will enclose that around I2C_SLAVE support check.



^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 3/4] dt-bindings: i2c-stm32: add SMBus Alert bindings
  2020-05-23 10:36       ` wsa
@ 2020-05-26 10:31         ` Alain Volmat
  0 siblings, 0 replies; 18+ messages in thread
From: Alain Volmat @ 2020-05-26 10:31 UTC (permalink / raw)
  To: wsa
  Cc: Rob Herring, mark.rutland, Pierre Yves MORDRET, mcoquelin.stm32,
	Alexandre TORGUE, linux-i2c, devicetree, linux-stm32,
	linux-arm-kernel, linux-kernel, Fabrice GASNIER

On Sat, May 23, 2020 at 10:36:01AM +0000, wsa@kernel.org wrote:
> 
> > > > +        st,smbus-alert:
> > > > +          description: Enable the SMBus Alert feature
> > > > +          $ref: /schemas/types.yaml#/definitions/flag
> > > > +
> > > 
> > > We already have smbus_alert interrupt. Can't you just check for this in 
> > > the slave nodes and enable if found?
> > 
> > My understanding reading the code (smbalert_probe within i2c-smbus.c, of_i2c_setup_smbus_alert called when
> > registering an adapter within i2c-core-smbus.c) is that smbus_alert refers to an interrupt on the
> > adapter side. That is an interrupt that would be triggered when the adapter is receiving an smbus_alert
> > message.
> > In our case (stm32f7), we do not have specific interrupt for that purpose. The interrupt triggered when
> > an SMBUS Alert is received (by the adapter) is the same interrupt as for other reasons and we check
> > within the irq handler within stm32f7 the reason before calling i2c_handle_smbus_alert if the status
> > register indicated an SMBUS Alert.
> > So my understanding is that we cannot rely on the mechanism of naming an interrupt smbus_alert.
> > Did I misunderstood something ?
> 
> I just wonder what is bad about specifying the same interrupt twice in
> the interrupt properties? You could then check in probe if "smbus_alert"
> is populated and if it matches the main irq.
> 

Here's my understanding of the current implementation.
During the adapter registration, the function of_i2c_setup_smbus_alert is called
and if a interrupt is named "smbus_alert" in the adapter node, a new device
"smbus_alert" will be created. This will leads to smbalert_probe to be called,
and a request_irq will be done with the irq value read from the adapter node.
This means that we will have both our handle (the handler of the main irq
of the stm32 i2c driver) and the smbus_alert handler on the same irq. Leading
to smbus_alert being called everytime there is a irq (most of the time not
smbus_alert related) coming from the stm32_i2c. (since this is our main irq).

So to me this approach can't work. I'd understand if the smbus_alert property
was on the client node in the same way as it is done for host-notify however
that's not the case.
This is why I was proposing to have our own st,smbus-alert property to decide
to enable or not the smbus_alert. In our case, we cannot rely on the mechanism
done by of_i2c_setup_smbus_alert since for us, smbus_alert irq is just one
case of all the other stm32 i2c irq. (this is the same irq, and we check after
by reading the interrupt status register).

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 4/4] i2c: stm32f7: Add SMBus-specific protocols support
  2020-05-23 11:01   ` Wolfram Sang
@ 2020-05-26 10:39     ` Alain Volmat
  0 siblings, 0 replies; 18+ messages in thread
From: Alain Volmat @ 2020-05-26 10:39 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: Benjamin Tissoires, robh+dt, mark.rutland, pierre-yves.mordret,
	mcoquelin.stm32, alexandre.torgue, linux-i2c, devicetree,
	linux-stm32, linux-arm-kernel, linux-kernel, fabrice.gasnier

On Sat, May 23, 2020 at 01:01:40PM +0200, Wolfram Sang wrote:
> 
> > +static int stm32f7_i2c_reg_client(struct i2c_client *client)
> > +{
> > +	struct stm32f7_i2c_dev *i2c_dev = i2c_get_adapdata(client->adapter);
> > +	int ret;
> > +
> > +	if (client->flags & I2C_CLIENT_HOST_NOTIFY) {
> > +		/* 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;
> > +}
> 
> So, as mentioned in the other review, I'd like to evaluate other
> possibilities for the above:
> 
> - One option is to enable it globally in probe(). Then you lose the
>   possibility to have a device at address 0x08.

I'd prefer avoid this solution to not lose the address 0x08.

> - Enable it in probe() only if there is a generic binding "host-notify".

Do you mean having the adapter walk through childs node and see if at least
one of them have the host-notify property ? This mean that such solution
wouldn't work for device relying on platform data rather than DT nodes.

> - Let the core scan for a device with HOST_NOTIFY when registering an
>   adapter and then call back into the driver somehow?

You mean at adapter registration time only ? Not device probing time ?
At probing time, we could have the core (i2c_device_probe) check for the flag
HOST_NOTIFY and if setted call a dedicated host-notify reg callback ?

> 
> Other ideas?
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, back to index

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-05  5:51 [PATCH 0/4] stm32-f7: Addition of SMBus Alert / Host-notify features Alain Volmat
2020-05-05  5:51 ` [PATCH 1/4] i2c: smbus: add core function handling SMBus host-notify Alain Volmat
2020-05-11  8:26   ` Pierre Yves MORDRET
2020-05-23 10:46   ` Wolfram Sang
2020-05-26 10:23     ` Alain Volmat
2020-05-05  5:51 ` [PATCH 2/4] i2c: addition of client reg/unreg callbacks Alain Volmat
2020-05-11  8:28   ` Pierre Yves MORDRET
2020-05-23 10:49   ` Wolfram Sang
2020-05-05  5:51 ` [PATCH 3/4] dt-bindings: i2c-stm32: add SMBus Alert bindings Alain Volmat
2020-05-13  2:19   ` Rob Herring
2020-05-13  5:42     ` Alain Volmat
2020-05-18 10:53       ` Alain Volmat
2020-05-23 10:36       ` wsa
2020-05-26 10:31         ` Alain Volmat
2020-05-05  5:51 ` [PATCH 4/4] i2c: stm32f7: Add SMBus-specific protocols support Alain Volmat
2020-05-11  8:27   ` Pierre Yves MORDRET
2020-05-23 11:01   ` Wolfram Sang
2020-05-26 10:39     ` Alain Volmat

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git