[PATCH v2 0/2] i2c: core: Add mux root adapter operations

[PATCH v2 0/2] i2c: core: Add mux root adapter operations

[Eddie James]

Some I2C clients need the ability to control the root I2C bus even if the
endpoint device is behind a mux. For example, a driver for a chip that
can't handle any I2C traffic on the bus while coming out of reset
(including an I2C-driven mux switching channels) may need to lock the root
bus with the mux selection fixed for the entire time the device is in
reset.
The SI7020 is such a device. This series adds the ability to the I2C core
to lock the root adapter of a client and fix the mux channel selection
until unlocked. The patch to the SI7020 driver then uses the new functions
to do just that and make sure the chip reset is safe.
I included the IIO patch for context, I can split and resubmit to iio list
only if necessary.
Thanks to Milton for the general idea here.

Changes since v1:
 - Correct spelling of i2c_unlock_deselect_bus comment.

Eddie James (2):
  i2c: core: Add mux root adapter operations
  iio: si7020: Lock root adapter to wait for reset

 drivers/i2c/i2c-core-base.c   | 38 ++++++++++++++++++++++++++
 drivers/i2c/i2c-mux.c         | 50 +++++++++++++++++++++++++++++++++++
 drivers/iio/humidity/si7020.c | 16 +++++++++--
 include/linux/i2c.h           | 42 +++++++++++++++++++++++++++++
 4 files changed, 144 insertions(+), 2 deletions(-)

-- 
2.31.1

[PATCH v2 1/2] i2c: core: Add mux root adapter operations

[Eddie James]

Some I2C clients need the ability to control the root I2C bus even if the
endpoint device is behind a mux. For example, a driver for a chip that
can't handle any I2C traffic on the bus while coming out of reset
(including an I2C-driven mux switching channels) may need to lock the root
bus with the mux selection fixed for the entire time the device is in
reset.
For this purpose, add a new structure containing two function pointers to
the adapter structure. These functions pointers should be defined for
every adapter. The lock_select operation, for a mux adapter, locks the
parent adpaters up to the root and selects the adapter's channel. The
unlock_deselect operation deselects the mux channel and unlocks all the
adapters. For a non-mux adapter, the operations lock and unlock the
adapters up to the root. This scheme should work with multiple levels of
muxes and regular adapters in between.

Signed-off-by: Eddie James <eajames@linux.ibm.com>
Reviewed-by: Joel Stanley <joel@jms.id.au>
---
 drivers/i2c/i2c-core-base.c | 38 ++++++++++++++++++++++++++++
 drivers/i2c/i2c-mux.c       | 50 +++++++++++++++++++++++++++++++++++++
 include/linux/i2c.h         | 42 +++++++++++++++++++++++++++++++
 3 files changed, 130 insertions(+)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 8c7e3494ca5f..b1a1e88ef478 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1347,6 +1347,41 @@ static const struct i2c_lock_operations i2c_adapter_lock_ops = {
 	.unlock_bus =  i2c_adapter_unlock_bus,
 };
 
+/*
+ * For a non-mux adapter, the lock_select operation locks the chain of
+ * adapters upwards, returning the root. If there's a mux above this adapter
+ * somehow, it should also get locked and the desired channel selected.
+ */
+static struct i2c_adapter *i2c_adapter_lock_select(struct i2c_adapter *adapter)
+{
+	struct i2c_adapter *ret = adapter;
+	struct i2c_adapter *parent = i2c_parent_is_i2c_adapter(adapter);
+
+	if (parent) {
+		ret = parent->mux_root_ops->lock_select(parent);
+		if (IS_ERR(ret))
+			return ret;
+	}
+
+	adapter->lock_ops->lock_bus(adapter, I2C_LOCK_ROOT_ADAPTER);
+	return ret;
+}
+
+static void i2c_adapter_unlock_deselect(struct i2c_adapter *adapter)
+{
+	struct i2c_adapter *parent = i2c_parent_is_i2c_adapter(adapter);
+
+	adapter->lock_ops->unlock_bus(adapter, I2C_LOCK_ROOT_ADAPTER);
+
+	if (parent)
+		parent->mux_root_ops->unlock_deselect(parent);
+}
+
+static const struct i2c_mux_root_operations i2c_adapter_mux_root_ops = {
+	.lock_select = i2c_adapter_lock_select,
+	.unlock_deselect = i2c_adapter_unlock_deselect,
+};
+
 static void i2c_host_notify_irq_teardown(struct i2c_adapter *adap)
 {
 	struct irq_domain *domain = adap->host_notify_domain;
@@ -1442,6 +1477,9 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
 	if (!adap->lock_ops)
 		adap->lock_ops = &i2c_adapter_lock_ops;
 
+	if (!adap->mux_root_ops)
+		adap->mux_root_ops = &i2c_adapter_mux_root_ops;
+
 	adap->locked_flags = 0;
 	rt_mutex_init(&adap->bus_lock);
 	rt_mutex_init(&adap->mux_lock);
diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
index 774507b54b57..c7db770e4198 100644
--- a/drivers/i2c/i2c-mux.c
+++ b/drivers/i2c/i2c-mux.c
@@ -210,6 +210,49 @@ static void i2c_parent_unlock_bus(struct i2c_adapter *adapter,
 	rt_mutex_unlock(&parent->mux_lock);
 }
 
+/*
+ * For a mux adapter, the lock_select operation first locks just like the
+ * lock_bus operation. Then it selects the channel for this adapter and
+ * returns the root adapter. If there is another mux above this one, calling
+ * the parent lock_select should ensure that the channel is correctly
+ * selected.
+ */
+static struct i2c_adapter *i2c_mux_lock_select(struct i2c_adapter *adapter)
+{
+	int ret;
+	struct i2c_mux_priv *priv = adapter->algo_data;
+	struct i2c_mux_core *muxc = priv->muxc;
+	struct i2c_adapter *parent = muxc->parent;
+
+	rt_mutex_lock_nested(&parent->mux_lock, i2c_adapter_depth(adapter));
+
+	adapter = parent->mux_root_ops->lock_select(parent);
+	if (IS_ERR(adapter))
+		return adapter;
+
+	ret = muxc->select(muxc, priv->chan_id);
+	if (ret < 0) {
+		parent->mux_root_ops->unlock_deselect(parent);
+		rt_mutex_unlock(&parent->mux_lock);
+		return ERR_PTR(ret);
+	}
+
+	return adapter;
+}
+
+static void i2c_mux_unlock_deselect(struct i2c_adapter *adapter)
+{
+	struct i2c_mux_priv *priv = adapter->algo_data;
+	struct i2c_mux_core *muxc = priv->muxc;
+	struct i2c_adapter *parent = muxc->parent;
+
+	if (muxc->deselect)
+		muxc->deselect(muxc, priv->chan_id);
+
+	parent->mux_root_ops->unlock_deselect(parent);
+	rt_mutex_unlock(&parent->mux_lock);
+}
+
 struct i2c_adapter *i2c_root_adapter(struct device *dev)
 {
 	struct device *i2c;
@@ -279,6 +322,11 @@ static const struct i2c_lock_operations i2c_parent_lock_ops = {
 	.unlock_bus =  i2c_parent_unlock_bus,
 };
 
+static const struct i2c_mux_root_operations i2c_mux_root_ops = {
+	.lock_select = i2c_mux_lock_select,
+	.unlock_deselect = i2c_mux_unlock_deselect,
+};
+
 int i2c_mux_add_adapter(struct i2c_mux_core *muxc,
 			u32 force_nr, u32 chan_id,
 			unsigned int class)
@@ -339,6 +387,8 @@ int i2c_mux_add_adapter(struct i2c_mux_core *muxc,
 	else
 		priv->adap.lock_ops = &i2c_parent_lock_ops;
 
+	priv->adap.mux_root_ops = &i2c_mux_root_ops;
+
 	/* Sanity check on class */
 	if (i2c_mux_parent_classes(parent) & class)
 		dev_err(&parent->dev,
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index f7c49bbdb8a1..51342a0c03e9 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -584,6 +584,26 @@ struct i2c_lock_operations {
 	void (*unlock_bus)(struct i2c_adapter *adapter, unsigned int flags);
 };
 
+/**
+ * struct i2c_mux_root_operations - represent operations to lock and select
+ * the adapter's mux channel (if a mux is present)
+ * @lock_select: Get exclusive access to the root I2C bus adapter with the
+ *   correct mux channel selected for the adapter
+ * @unlock_deslect: Release exclusive access to the root I2C bus adapter and
+ *   deselect the mux channel for the adapter
+ *
+ * Some I2C clients need the ability to control the root I2C bus even if the
+ * endpoint device is behind a mux. For example, a driver for a chip that
+ * can't handle any I2C traffic on the bus while coming out of reset (including
+ * an I2C-driven mux switching channels) may need to lock the root bus with
+ * the mux selection fixed for the entire time the device is in reset.
+ * These operations are for such a purpose.
+ */
+struct i2c_mux_root_operations {
+	struct i2c_adapter *(*lock_select)(struct i2c_adapter *adapter);
+	void (*unlock_deselect)(struct i2c_adapter *adapter);
+};
+
 /**
  * struct i2c_timings - I2C timing information
  * @bus_freq_hz: the bus frequency in Hz
@@ -726,6 +746,7 @@ struct i2c_adapter {
 
 	/* data fields that are valid for all devices	*/
 	const struct i2c_lock_operations *lock_ops;
+	const struct i2c_mux_root_operations *mux_root_ops;
 	struct rt_mutex bus_lock;
 	struct rt_mutex mux_lock;
 
@@ -818,6 +839,27 @@ i2c_unlock_bus(struct i2c_adapter *adapter, unsigned int flags)
 	adapter->lock_ops->unlock_bus(adapter, flags);
 }
 
+/**
+ * i2c_lock_select_bus - Get exclusive access to the root I2C bus with the
+ *   target's mux channel (if a mux is present) selected.
+ * @adapter: Target I2C bus
+ *
+ * Return the root I2C bus if mux selection succeeds, an ERR_PTR otherwise
+ */
+static inline struct i2c_adapter *i2c_lock_select_bus(struct i2c_adapter *adapter)
+{
+	return adapter->mux_root_ops->lock_select(adapter);
+}
+
+/**
+ * i2c_unlock_deselect_bus - Release exclusive access to the root I2C bus
+ * @adapter: Target I2C bus
+ */
+static inline void i2c_unlock_deselect_bus(struct i2c_adapter *adapter)
+{
+	adapter->mux_root_ops->unlock_deselect(adapter);
+}
+
 /**
  * i2c_mark_adapter_suspended - Report suspended state of the adapter to the core
  * @adap: Adapter to mark as suspended
-- 
2.31.1

[PATCH v2 2/2] iio: si7020: Lock root adapter to wait for reset

[Eddie James]

Use the new mux root operations to lock the root adapter while waiting for
the reset to complete. I2C commands issued after the SI7020 is starting up
or after reset can potentially upset the startup sequence. Therefore, the
host needs to wait for the startup sequence to finish before issuing
further I2C commands.

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

diff --git a/drivers/iio/humidity/si7020.c b/drivers/iio/humidity/si7020.c
index ab6537f136ba..76ca7863f35b 100644
--- a/drivers/iio/humidity/si7020.c
+++ b/drivers/iio/humidity/si7020.c
@@ -106,6 +106,7 @@ static const struct iio_info si7020_info = {
 static int si7020_probe(struct i2c_client *client,
 			const struct i2c_device_id *id)
 {
+	struct i2c_adapter *root;
 	struct iio_dev *indio_dev;
 	struct i2c_client **data;
 	int ret;
@@ -115,13 +116,24 @@ static int si7020_probe(struct i2c_client *client,
 				     I2C_FUNC_SMBUS_READ_WORD_DATA))
 		return -EOPNOTSUPP;
 
+	root = i2c_lock_select_bus(client->adapter);
+	if (IS_ERR(root))
+		return PTR_ERR(root);
+
 	/* Reset device, loads default settings. */
-	ret = i2c_smbus_write_byte(client, SI7020CMD_RESET);
-	if (ret < 0)
+	ret = __i2c_smbus_xfer(root, client->addr, client->flags,
+			       I2C_SMBUS_WRITE, SI7020CMD_RESET,
+			       I2C_SMBUS_BYTE, NULL);
+	if (ret < 0) {
+		i2c_unlock_deselect_bus(client->adapter);
 		return ret;
+	}
+
 	/* Wait the maximum power-up time after software reset. */
 	msleep(15);
 
+	i2c_unlock_deselect_bus(client->adapter);
+
 	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
 	if (!indio_dev)
 		return -ENOMEM;
-- 
2.31.1

Re: [PATCH v2 2/2] iio: si7020: Lock root adapter to wait for reset

[Peter Rosin]

Hi!

First off, I'm very sorry for being too busy and too unresponsive.

2022-09-06 at 22:28, Eddie James wrote:
> Use the new mux root operations to lock the root adapter while waiting for
> the reset to complete. I2C commands issued after the SI7020 is starting up
> or after reset can potentially upset the startup sequence. Therefore, the
> host needs to wait for the startup sequence to finish before issuing
> further I2C commands.
> 
> Signed-off-by: Eddie James <eajames@linux.ibm.com>
> ---
>  drivers/iio/humidity/si7020.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/humidity/si7020.c b/drivers/iio/humidity/si7020.c
> index ab6537f136ba..76ca7863f35b 100644
> --- a/drivers/iio/humidity/si7020.c
> +++ b/drivers/iio/humidity/si7020.c
> @@ -106,6 +106,7 @@ static const struct iio_info si7020_info = {
>  static int si7020_probe(struct i2c_client *client,
>  			const struct i2c_device_id *id)
>  {
> +	struct i2c_adapter *root;
>  	struct iio_dev *indio_dev;
>  	struct i2c_client **data;
>  	int ret;
> @@ -115,13 +116,24 @@ static int si7020_probe(struct i2c_client *client,
>  				     I2C_FUNC_SMBUS_READ_WORD_DATA))
>  		return -EOPNOTSUPP;
>  
> +	root = i2c_lock_select_bus(client->adapter);
> +	if (IS_ERR(root))
> +		return PTR_ERR(root);
> +
>  	/* Reset device, loads default settings. */
> -	ret = i2c_smbus_write_byte(client, SI7020CMD_RESET);
> -	if (ret < 0)
> +	ret = __i2c_smbus_xfer(root, client->addr, client->flags,
> +			       I2C_SMBUS_WRITE, SI7020CMD_RESET,
> +			       I2C_SMBUS_BYTE, NULL);

I'd say that this is too ugly. We should not add stuff that basically
hides the actual xfer from the mux like this. That is too much of a
break in the abstraction.

Looking back, expanding on the previous series [1] so that it installs
the hook on the root adapter, handles smbus xfers and clears out the
callback afterwards is much more sensible. No?

Maybe the callback in that series should also include a reference to
the xfer that has just been done, so that the hook can potentially
discriminate and only do the delay for the key xfer. But maybe that's
overkill?

Cheers,
Peter

[1] https://lore.kernel.org/lkml/20220518204119.38943-1-eajames@linux.ibm.com/

> +	if (ret < 0) {
> +		i2c_unlock_deselect_bus(client->adapter);
>  		return ret;
> +	}
> +
>  	/* Wait the maximum power-up time after software reset. */
>  	msleep(15);
>  
> +	i2c_unlock_deselect_bus(client->adapter);
> +
>  	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
>  	if (!indio_dev)
>  		return -ENOMEM;

Re: [PATCH v2 2/2] iio: si7020: Lock root adapter to wait for reset

[Eddie James]

On 9/7/22 02:10, Peter Rosin wrote:
> Hi!
>
> First off, I'm very sorry for being too busy and too unresponsive.
>
> 2022-09-06 at 22:28, Eddie James wrote:
>> Use the new mux root operations to lock the root adapter while waiting for
>> the reset to complete. I2C commands issued after the SI7020 is starting up
>> or after reset can potentially upset the startup sequence. Therefore, the
>> host needs to wait for the startup sequence to finish before issuing
>> further I2C commands.
>>
>> Signed-off-by: Eddie James <eajames@linux.ibm.com>
>> ---
>>   drivers/iio/humidity/si7020.c | 16 ++++++++++++++--
>>   1 file changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/iio/humidity/si7020.c b/drivers/iio/humidity/si7020.c
>> index ab6537f136ba..76ca7863f35b 100644
>> --- a/drivers/iio/humidity/si7020.c
>> +++ b/drivers/iio/humidity/si7020.c
>> @@ -106,6 +106,7 @@ static const struct iio_info si7020_info = {
>>   static int si7020_probe(struct i2c_client *client,
>>   			const struct i2c_device_id *id)
>>   {
>> +	struct i2c_adapter *root;
>>   	struct iio_dev *indio_dev;
>>   	struct i2c_client **data;
>>   	int ret;
>> @@ -115,13 +116,24 @@ static int si7020_probe(struct i2c_client *client,
>>   				     I2C_FUNC_SMBUS_READ_WORD_DATA))
>>   		return -EOPNOTSUPP;
>>   
>> +	root = i2c_lock_select_bus(client->adapter);
>> +	if (IS_ERR(root))
>> +		return PTR_ERR(root);
>> +
>>   	/* Reset device, loads default settings. */
>> -	ret = i2c_smbus_write_byte(client, SI7020CMD_RESET);
>> -	if (ret < 0)
>> +	ret = __i2c_smbus_xfer(root, client->addr, client->flags,
>> +			       I2C_SMBUS_WRITE, SI7020CMD_RESET,
>> +			       I2C_SMBUS_BYTE, NULL);
> I'd say that this is too ugly. We should not add stuff that basically
> hides the actual xfer from the mux like this. That is too much of a
> break in the abstraction.


Hm, I guess I'm not sure I follow - I see several drivers that use the 
raw __i2c_smbus_xfer or __i2c_transfer, some without a lock in sight. If 
it's not acceptable to use the unlocked versions in some cases, why are 
they exported in the header file?


>
> Looking back, expanding on the previous series [1] so that it installs
> the hook on the root adapter, handles smbus xfers and clears out the
> callback afterwards is much more sensible. No?


Maybe so, though adding the callback is a more intrusive change, in my 
opinion, since every transfer has to check if the pointer is null.


Thanks for your feedback!

Eddie



>
> Maybe the callback in that series should also include a reference to
> the xfer that has just been done, so that the hook can potentially
> discriminate and only do the delay for the key xfer. But maybe that's
> overkill?
>
> Cheers,
> Peter
>
> [1] https://lore.kernel.org/lkml/20220518204119.38943-1-eajames@linux.ibm.com/
>
>> +	if (ret < 0) {
>> +		i2c_unlock_deselect_bus(client->adapter);
>>   		return ret;
>> +	}
>> +
>>   	/* Wait the maximum power-up time after software reset. */
>>   	msleep(15);
>>   
>> +	i2c_unlock_deselect_bus(client->adapter);
>> +
>>   	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
>>   	if (!indio_dev)
>>   		return -ENOMEM;

Re: [PATCH v2 2/2] iio: si7020: Lock root adapter to wait for reset

[Peter Rosin]

Hi!

2022-09-07 at 15:53, Eddie James wrote:
> 
> On 9/7/22 02:10, Peter Rosin wrote:
>> Hi!
>>
>> First off, I'm very sorry for being too busy and too unresponsive.
>>
>> 2022-09-06 at 22:28, Eddie James wrote:
>>> Use the new mux root operations to lock the root adapter while waiting for
>>> the reset to complete. I2C commands issued after the SI7020 is starting up
>>> or after reset can potentially upset the startup sequence. Therefore, the
>>> host needs to wait for the startup sequence to finish before issuing
>>> further I2C commands.
>>>
>>> Signed-off-by: Eddie James <eajames@linux.ibm.com>
>>> ---
>>>   drivers/iio/humidity/si7020.c | 16 ++++++++++++++--
>>>   1 file changed, 14 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/iio/humidity/si7020.c b/drivers/iio/humidity/si7020.c
>>> index ab6537f136ba..76ca7863f35b 100644
>>> --- a/drivers/iio/humidity/si7020.c
>>> +++ b/drivers/iio/humidity/si7020.c
>>> @@ -106,6 +106,7 @@ static const struct iio_info si7020_info = {
>>>   static int si7020_probe(struct i2c_client *client,
>>>               const struct i2c_device_id *id)
>>>   {
>>> +    struct i2c_adapter *root;
>>>       struct iio_dev *indio_dev;
>>>       struct i2c_client **data;
>>>       int ret;
>>> @@ -115,13 +116,24 @@ static int si7020_probe(struct i2c_client *client,
>>>                        I2C_FUNC_SMBUS_READ_WORD_DATA))
>>>           return -EOPNOTSUPP;
>>>   +    root = i2c_lock_select_bus(client->adapter);
>>> +    if (IS_ERR(root))
>>> +        return PTR_ERR(root);
>>> +
>>>       /* Reset device, loads default settings. */
>>> -    ret = i2c_smbus_write_byte(client, SI7020CMD_RESET);
>>> -    if (ret < 0)
>>> +    ret = __i2c_smbus_xfer(root, client->addr, client->flags,
>>> +                   I2C_SMBUS_WRITE, SI7020CMD_RESET,
>>> +                   I2C_SMBUS_BYTE, NULL);
>> I'd say that this is too ugly. We should not add stuff that basically
>> hides the actual xfer from the mux like this. That is too much of a
>> break in the abstraction.
> 
> 
> Hm, I guess I'm not sure I follow - I see several drivers that use the raw __i2c_smbus_xfer or __i2c_transfer, some without a lock in sight. If it's not acceptable to use the unlocked versions in some cases, why are they exported in the header file?

Doing unlocked xfers w/o manually doing the locking is a bug. If you are
aware of code doing this, please point them out!

Issuing an unlocked xfer on the same adapter that has been manually locked
has been a thing since forever. But issuing that xfer on the root adapter
"behind the back" of e.g. an address translator is simply not going to work
at all if it never sees the xfer and thus never gets a chance to modify it.
A mux might also in theory add quirks or adjust xfers for whatever reason,
and that possibility will be made impossible by hiding the xfer from the
mux.

> 
> 
>>
>> Looking back, expanding on the previous series [1] so that it installs
>> the hook on the root adapter, handles smbus xfers and clears out the
>> callback afterwards is much more sensible. No?
> 
> 
> Maybe so, though adding the callback is a more intrusive change, in my opinion, since every transfer has to check if the pointer is null.

The runtime cost will be negligible. The bigger cost is IMO the maintenance
overhead.

Cheers,
Peter

> 
> 
> Thanks for your feedback!
> 
> Eddie
> 
> 
> 
>>
>> Maybe the callback in that series should also include a reference to
>> the xfer that has just been done, so that the hook can potentially
>> discriminate and only do the delay for the key xfer. But maybe that's
>> overkill?
>>
>> Cheers,
>> Peter
>>
>> [1] https://lore.kernel.org/lkml/20220518204119.38943-1-eajames@linux.ibm.com/
>>
>>> +    if (ret < 0) {
>>> +        i2c_unlock_deselect_bus(client->adapter);
>>>           return ret;
>>> +    }
>>> +
>>>       /* Wait the maximum power-up time after software reset. */
>>>       msleep(15);
>>>   +    i2c_unlock_deselect_bus(client->adapter);
>>> +
>>>       indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
>>>       if (!indio_dev)
>>>           return -ENOMEM;

Re: [PATCH v2 2/2] iio: si7020: Lock root adapter to wait for reset

[Eddie James]

On 9/7/22 09:17, Peter Rosin wrote:
> Hi!
>
> 2022-09-07 at 15:53, Eddie James wrote:
>> On 9/7/22 02:10, Peter Rosin wrote:
>>> Hi!
>>>
>>> First off, I'm very sorry for being too busy and too unresponsive.
>>>
>>> 2022-09-06 at 22:28, Eddie James wrote:
>>>> Use the new mux root operations to lock the root adapter while waiting for
>>>> the reset to complete. I2C commands issued after the SI7020 is starting up
>>>> or after reset can potentially upset the startup sequence. Therefore, the
>>>> host needs to wait for the startup sequence to finish before issuing
>>>> further I2C commands.
>>>>
>>>> Signed-off-by: Eddie James <eajames@linux.ibm.com>
>>>> ---
>>>>    drivers/iio/humidity/si7020.c | 16 ++++++++++++++--
>>>>    1 file changed, 14 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/iio/humidity/si7020.c b/drivers/iio/humidity/si7020.c
>>>> index ab6537f136ba..76ca7863f35b 100644
>>>> --- a/drivers/iio/humidity/si7020.c
>>>> +++ b/drivers/iio/humidity/si7020.c
>>>> @@ -106,6 +106,7 @@ static const struct iio_info si7020_info = {
>>>>    static int si7020_probe(struct i2c_client *client,
>>>>                const struct i2c_device_id *id)
>>>>    {
>>>> +    struct i2c_adapter *root;
>>>>        struct iio_dev *indio_dev;
>>>>        struct i2c_client **data;
>>>>        int ret;
>>>> @@ -115,13 +116,24 @@ static int si7020_probe(struct i2c_client *client,
>>>>                         I2C_FUNC_SMBUS_READ_WORD_DATA))
>>>>            return -EOPNOTSUPP;
>>>>    +    root = i2c_lock_select_bus(client->adapter);
>>>> +    if (IS_ERR(root))
>>>> +        return PTR_ERR(root);
>>>> +
>>>>        /* Reset device, loads default settings. */
>>>> -    ret = i2c_smbus_write_byte(client, SI7020CMD_RESET);
>>>> -    if (ret < 0)
>>>> +    ret = __i2c_smbus_xfer(root, client->addr, client->flags,
>>>> +                   I2C_SMBUS_WRITE, SI7020CMD_RESET,
>>>> +                   I2C_SMBUS_BYTE, NULL);
>>> I'd say that this is too ugly. We should not add stuff that basically
>>> hides the actual xfer from the mux like this. That is too much of a
>>> break in the abstraction.
>>
>> Hm, I guess I'm not sure I follow - I see several drivers that use the raw __i2c_smbus_xfer or __i2c_transfer, some without a lock in sight. If it's not acceptable to use the unlocked versions in some cases, why are they exported in the header file?
> Doing unlocked xfers w/o manually doing the locking is a bug. If you are
> aware of code doing this, please point them out!


Ah, I just meant the lock wasn't right next to the transfer call, since 
I didn't fully understand you. I'm sure everything is being locked 
correctly.


>
> Issuing an unlocked xfer on the same adapter that has been manually locked
> has been a thing since forever. But issuing that xfer on the root adapter
> "behind the back" of e.g. an address translator is simply not going to work
> at all if it never sees the xfer and thus never gets a chance to modify it.
> A mux might also in theory add quirks or adjust xfers for whatever reason,
> and that possibility will be made impossible by hiding the xfer from the
> mux.


OK, I see what you're saying. But that's what lock_select does, it locks 
the bus and goes through the mux channel selection function. With the 
way muxes work right now, they can't modify the xfer at all, they just 
deselect/select channels. So this code should work fine, unless I'm 
still misunderstanding something.

I do see your point about a possible enhancement of the mux interface... 
I guess I'd argue that whoever wants to do that should have to worry 
about it! But if you're firm on this, I'll try the other way. I'll also 
mention we (IBM/OpenBMC) has been running this series on lab systems for 
a couple of months now, so it has some testing, though admittedly with 
just one configuration of device/mux/adapter.


Thanks!

Eddie


>>
>>> Looking back, expanding on the previous series [1] so that it installs
>>> the hook on the root adapter, handles smbus xfers and clears out the
>>> callback afterwards is much more sensible. No?
>>
>> Maybe so, though adding the callback is a more intrusive change, in my opinion, since every transfer has to check if the pointer is null.
> The runtime cost will be negligible. The bigger cost is IMO the maintenance
> overhead.
>
> Cheers,
> Peter
>
>>
>> Thanks for your feedback!
>>
>> Eddie
>>
>>
>>
>>> Maybe the callback in that series should also include a reference to
>>> the xfer that has just been done, so that the hook can potentially
>>> discriminate and only do the delay for the key xfer. But maybe that's
>>> overkill?
>>>
>>> Cheers,
>>> Peter
>>>
>>> [1] https://lore.kernel.org/lkml/20220518204119.38943-1-eajames@linux.ibm.com/
>>>
>>>> +    if (ret < 0) {
>>>> +        i2c_unlock_deselect_bus(client->adapter);
>>>>            return ret;
>>>> +    }
>>>> +
>>>>        /* Wait the maximum power-up time after software reset. */
>>>>        msleep(15);
>>>>    +    i2c_unlock_deselect_bus(client->adapter);
>>>> +
>>>>        indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
>>>>        if (!indio_dev)
>>>>            return -ENOMEM;

Re: [PATCH v2 2/2] iio: si7020: Lock root adapter to wait for reset

[Peter Rosin]

Hi!

2022-09-07 at 18:02, Eddie James wrote:
> 
> On 9/7/22 09:17, Peter Rosin wrote:
>> Hi!
>>
>> 2022-09-07 at 15:53, Eddie James wrote:
>>> On 9/7/22 02:10, Peter Rosin wrote:
>>>> Hi!
>>>>
>>>> First off, I'm very sorry for being too busy and too unresponsive.
>>>>
>>>> 2022-09-06 at 22:28, Eddie James wrote:
>>>>> Use the new mux root operations to lock the root adapter while waiting for
>>>>> the reset to complete. I2C commands issued after the SI7020 is starting up
>>>>> or after reset can potentially upset the startup sequence. Therefore, the
>>>>> host needs to wait for the startup sequence to finish before issuing
>>>>> further I2C commands.
>>>>>
>>>>> Signed-off-by: Eddie James <eajames@linux.ibm.com>
>>>>> ---
>>>>>    drivers/iio/humidity/si7020.c | 16 ++++++++++++++--
>>>>>    1 file changed, 14 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/iio/humidity/si7020.c b/drivers/iio/humidity/si7020.c
>>>>> index ab6537f136ba..76ca7863f35b 100644
>>>>> --- a/drivers/iio/humidity/si7020.c
>>>>> +++ b/drivers/iio/humidity/si7020.c
>>>>> @@ -106,6 +106,7 @@ static const struct iio_info si7020_info = {
>>>>>    static int si7020_probe(struct i2c_client *client,
>>>>>                const struct i2c_device_id *id)
>>>>>    {
>>>>> +    struct i2c_adapter *root;
>>>>>        struct iio_dev *indio_dev;
>>>>>        struct i2c_client **data;
>>>>>        int ret;
>>>>> @@ -115,13 +116,24 @@ static int si7020_probe(struct i2c_client *client,
>>>>>                         I2C_FUNC_SMBUS_READ_WORD_DATA))
>>>>>            return -EOPNOTSUPP;
>>>>>    +    root = i2c_lock_select_bus(client->adapter);
>>>>> +    if (IS_ERR(root))
>>>>> +        return PTR_ERR(root);
>>>>> +
>>>>>        /* Reset device, loads default settings. */
>>>>> -    ret = i2c_smbus_write_byte(client, SI7020CMD_RESET);
>>>>> -    if (ret < 0)
>>>>> +    ret = __i2c_smbus_xfer(root, client->addr, client->flags,
>>>>> +                   I2C_SMBUS_WRITE, SI7020CMD_RESET,
>>>>> +                   I2C_SMBUS_BYTE, NULL);
>>>> I'd say that this is too ugly. We should not add stuff that basically
>>>> hides the actual xfer from the mux like this. That is too much of a
>>>> break in the abstraction.
>>>
>>> Hm, I guess I'm not sure I follow - I see several drivers that use the raw __i2c_smbus_xfer or __i2c_transfer, some without a lock in sight. If it's not acceptable to use the unlocked versions in some cases, why are they exported in the header file?
>> Doing unlocked xfers w/o manually doing the locking is a bug. If you are
>> aware of code doing this, please point them out!
> 
> 
> Ah, I just meant the lock wasn't right next to the transfer call, since I didn't fully understand you. I'm sure everything is being locked correctly.
> 
> 
>>
>> Issuing an unlocked xfer on the same adapter that has been manually locked
>> has been a thing since forever. But issuing that xfer on the root adapter
>> "behind the back" of e.g. an address translator is simply not going to work
>> at all if it never sees the xfer and thus never gets a chance to modify it.
>> A mux might also in theory add quirks or adjust xfers for whatever reason,
>> and that possibility will be made impossible by hiding the xfer from the
>> mux.
> 
> 
> OK, I see what you're saying. But that's what lock_select does, it locks the bus and goes through the mux channel selection function. With the way muxes work right now, they can't modify the xfer at all, they just deselect/select channels. So this code should work fine, unless I'm still misunderstanding something.
> 
> I do see your point about a possible enhancement of the mux interface... I guess I'd argue that whoever wants to do that should have to worry about it! But if you're firm on this, I'll try the other way. I'll also mention we (IBM/OpenBMC) has been running this series on lab systems for a couple of months now, so it has some testing, though admittedly with just one configuration of device/mux/adapter.
> 

Good, thanks, because I do not like how the approach in this series paints us
into a corner, making e.g. the address translation series much more difficult:

https://lore.kernel.org/linux-media/20190723203723.11730-3-luca@lucaceresoli.net/

Cheers,
Peter

> 
> Thanks!
> 
> Eddie
> 
> 
>>>
>>>> Looking back, expanding on the previous series [1] so that it installs
>>>> the hook on the root adapter, handles smbus xfers and clears out the
>>>> callback afterwards is much more sensible. No?
>>>
>>> Maybe so, though adding the callback is a more intrusive change, in my opinion, since every transfer has to check if the pointer is null.
>> The runtime cost will be negligible. The bigger cost is IMO the maintenance
>> overhead.
>>
>> Cheers,
>> Peter
>>
>>>
>>> Thanks for your feedback!
>>>
>>> Eddie
>>>
>>>
>>>
>>>> Maybe the callback in that series should also include a reference to
>>>> the xfer that has just been done, so that the hook can potentially
>>>> discriminate and only do the delay for the key xfer. But maybe that's
>>>> overkill?
>>>>
>>>> Cheers,
>>>> Peter
>>>>
>>>> [1] https://lore.kernel.org/lkml/20220518204119.38943-1-eajames@linux.ibm.com/
>>>>
>>>>> +    if (ret < 0) {
>>>>> +        i2c_unlock_deselect_bus(client->adapter);
>>>>>            return ret;
>>>>> +    }
>>>>> +
>>>>>        /* Wait the maximum power-up time after software reset. */
>>>>>        msleep(15);
>>>>>    +    i2c_unlock_deselect_bus(client->adapter);
>>>>> +
>>>>>        indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
>>>>>        if (!indio_dev)
>>>>>            return -ENOMEM;