[RFC 0/2] i2c: core and si7020: Add adapter transfer callback

[RFC 0/2] i2c: core and si7020: Add adapter transfer callback

[Eddie James]

This series adds a callback function pointer to be executed after the
adapter performs a transfer. The purpose of such a callback is for a
client to execute some code while "owning" the bus entirely. Holding the
adapter lock is insufficient in the case where the client is behind a
mux, as the mux driver could perform mux selection operations on the
bus while locked. The only use case for now is the SI7020 driver. While
the SI7020 is starting up after power on or reset, any I2C commands on the
bus can potentially upset the startup sequence. Since the SI7020 may be
behind a mux, the driver needs to use the new transfer callback to sleep
while the chip starts up.

Eddie James (2):
  i2c: core: Add adapter transfer callback
  iio: humidity: si7020: Use core transfer callback to sleep after reset

 drivers/i2c/i2c-core-base.c   |  3 +++
 drivers/iio/humidity/si7020.c | 12 ++++++++++--
 include/linux/i2c.h           | 25 +++++++++++++++++++++++++
 3 files changed, 38 insertions(+), 2 deletions(-)

-- 
2.27.0

[RFC 1/2] i2c: core: Add adapter transfer callback

[Eddie James]

Add a callback function pointer to be executed after the adapter
performs a transfer. The purpose of such a callback is for a client
to execute some code while "owning" the bus entirely. Holding the
adapter lock is insufficient in the case where the client is behind a
mux, as the mux driver could perform mux selection operations on the
bus while locked.

Signed-off-by: Eddie James <eajames@linux.ibm.com>
---
 drivers/i2c/i2c-core-base.c |  3 +++
 include/linux/i2c.h         | 25 +++++++++++++++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index d43db2c3876e..a46bfee2d845 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -2128,6 +2128,9 @@ int __i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 		trace_i2c_result(adap, num, ret);
 	}
 
+	if (adap->xfer_callback)
+		adap->xfer_callback(adap->xfer_data, ret);
+
 	return ret;
 }
 EXPORT_SYMBOL(__i2c_transfer);
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index fbda5ada2afc..ea773f2ee9c8 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -747,6 +747,9 @@ struct i2c_adapter {
 
 	struct irq_domain *host_notify_domain;
 	struct regulator *bus_regulator;
+
+	void (*xfer_callback)(void *data, int xfer_rc);
+	void *xfer_data;
 };
 #define to_i2c_adapter(d) container_of(d, struct i2c_adapter, dev)
 
@@ -814,6 +817,7 @@ i2c_trylock_bus(struct i2c_adapter *adapter, unsigned int flags)
 static inline void
 i2c_unlock_bus(struct i2c_adapter *adapter, unsigned int flags)
 {
+	adapter->xfer_callback = NULL;
 	adapter->lock_ops->unlock_bus(adapter, flags);
 }
 
@@ -849,6 +853,27 @@ static inline void i2c_mark_adapter_resumed(struct i2c_adapter *adap)
 	i2c_unlock_bus(adap, I2C_LOCK_ROOT_ADAPTER);
 }
 
+/**
+ * i2c_adapter_xfer_callback - Register a callback function that is executed
+ *			       when a transfer completes.
+ * @adap: Adapter to which the callback function will be registered
+ * @cb: The callback function pointer
+ * @data: The data to pass to the callback function
+ *
+ * This function should be called with the adapter locked with
+ * I2C_LOCK_ROOT_ADAPTER to ensure that the whole bus is idle while the
+ * callback executes.
+ * The callback is automatically removed when the bus is unlocked to avoid
+ * spurious executions of the callback.
+ */
+static inline void i2c_adapter_xfer_callback(struct i2c_adapter *adap,
+					     void (*cb)(void *data, int rc),
+					     void *data)
+{
+	adap->xfer_callback = cb;
+	adap->xfer_data = data;
+}
+
 /* i2c adapter classes (bitmask) */
 #define I2C_CLASS_HWMON		(1<<0)	/* lm_sensors, ... */
 #define I2C_CLASS_DDC		(1<<3)	/* DDC bus on graphics adapters */
-- 
2.27.0

Re: [RFC 1/2] i2c: core: Add adapter transfer callback

[Peter Rosin]

Hi!

2022-05-18 at 22:41, Eddie James wrote:
> Add a callback function pointer to be executed after the adapter
> performs a transfer. The purpose of such a callback is for a client
> to execute some code while "owning" the bus entirely. Holding the
> adapter lock is insufficient in the case where the client is behind a
> mux, as the mux driver could perform mux selection operations on the
> bus while locked.
> 
> Signed-off-by: Eddie James <eajames@linux.ibm.com>
> ---
>  drivers/i2c/i2c-core-base.c |  3 +++
>  include/linux/i2c.h         | 25 +++++++++++++++++++++++++
>  2 files changed, 28 insertions(+)
> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index d43db2c3876e..a46bfee2d845 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -2128,6 +2128,9 @@ int __i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
>  		trace_i2c_result(adap, num, ret);
>  	}
>  
> +	if (adap->xfer_callback)
> +		adap->xfer_callback(adap->xfer_data, ret);
> +

Have you actually tested this_ Because it is not handling your issue
AFAICT. The master_xfer for muxes include the select/deselect which
(potentially) does xfers on the parent bus, so your hook is in the
wrong place when a I2C mux is involved.

Also, the hook should probably trigger for SMBus xfers.

Cheers,
Peter

>  	return ret;
>  }
>  EXPORT_SYMBOL(__i2c_transfer);
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index fbda5ada2afc..ea773f2ee9c8 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -747,6 +747,9 @@ struct i2c_adapter {
>  
>  	struct irq_domain *host_notify_domain;
>  	struct regulator *bus_regulator;
> +
> +	void (*xfer_callback)(void *data, int xfer_rc);
> +	void *xfer_data;
>  };
>  #define to_i2c_adapter(d) container_of(d, struct i2c_adapter, dev)
>  
> @@ -814,6 +817,7 @@ i2c_trylock_bus(struct i2c_adapter *adapter, unsigned int flags)
>  static inline void
>  i2c_unlock_bus(struct i2c_adapter *adapter, unsigned int flags)
>  {
> +	adapter->xfer_callback = NULL;
>  	adapter->lock_ops->unlock_bus(adapter, flags);
>  }
>  
> @@ -849,6 +853,27 @@ static inline void i2c_mark_adapter_resumed(struct i2c_adapter *adap)
>  	i2c_unlock_bus(adap, I2C_LOCK_ROOT_ADAPTER);
>  }
>  
> +/**
> + * i2c_adapter_xfer_callback - Register a callback function that is executed
> + *			       when a transfer completes.
> + * @adap: Adapter to which the callback function will be registered
> + * @cb: The callback function pointer
> + * @data: The data to pass to the callback function
> + *
> + * This function should be called with the adapter locked with
> + * I2C_LOCK_ROOT_ADAPTER to ensure that the whole bus is idle while the
> + * callback executes.
> + * The callback is automatically removed when the bus is unlocked to avoid
> + * spurious executions of the callback.
> + */
> +static inline void i2c_adapter_xfer_callback(struct i2c_adapter *adap,
> +					     void (*cb)(void *data, int rc),
> +					     void *data)
> +{
> +	adap->xfer_callback = cb;
> +	adap->xfer_data = data;
> +}
> +
>  /* i2c adapter classes (bitmask) */
>  #define I2C_CLASS_HWMON		(1<<0)	/* lm_sensors, ... */
>  #define I2C_CLASS_DDC		(1<<3)	/* DDC bus on graphics adapters */

[RFC 2/2] iio: humidity: si7020: Use core transfer callback to sleep after reset

[Eddie James]

While the SI7020 is starting up after power on or reset, any I2C commands
on the bus can potentially upset the startup sequence. Therefore, the host
needs to wait for the startup sequence to finish before issuing further
i2c commands. This is impractical in cases where the SI7020 is on a shared
bus or behind a mux, which may switch channels at any time (potentially
generating I2C traffic). To resolve this, use the new transfer callback
on the adapter to sleep the required interval.

Signed-off-by: Eddie James <eajames@linux.ibm.com>
---
 drivers/iio/humidity/si7020.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/humidity/si7020.c b/drivers/iio/humidity/si7020.c
index ab6537f136ba..f19e88818bd6 100644
--- a/drivers/iio/humidity/si7020.c
+++ b/drivers/iio/humidity/si7020.c
@@ -103,6 +103,13 @@ static const struct iio_info si7020_info = {
 	.read_raw = si7020_read_raw,
 };
 
+static void si7020_xfer_callback(void *data, int xfer_rc)
+{
+	/* Wait the maximum power-up time after software reset. */
+	if (!xfer_rc)
+		msleep(15);
+}
+
 static int si7020_probe(struct i2c_client *client,
 			const struct i2c_device_id *id)
 {
@@ -115,12 +122,13 @@ static int si7020_probe(struct i2c_client *client,
 				     I2C_FUNC_SMBUS_READ_WORD_DATA))
 		return -EOPNOTSUPP;
 
+	i2c_lock_bus(client->adapter, I2C_LOCK_ROOT_ADAPTER);
+	i2c_adapter_xfer_callback(client->adapter, si7020_xfer_callback, NULL);
 	/* Reset device, loads default settings. */
 	ret = i2c_smbus_write_byte(client, SI7020CMD_RESET);
+	i2c_unlock_bus(client->adapter, I2C_LOCK_ROOT_ADAPTER);
 	if (ret < 0)
 		return ret;
-	/* Wait the maximum power-up time after software reset. */
-	msleep(15);
 
 	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
 	if (!indio_dev)
-- 
2.27.0