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

[Eddie James <eajames@linux.ibm.com>]

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;