Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <c7f6baa3-8181-a612-72cc-23f570f31ab3@linux.ibm.com>
Date:   Wed, 7 Sep 2022 08:53:20 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.13.0
Subject: Re: [PATCH v2 2/2] iio: si7020: Lock root adapter to wait for reset
Content-Language: en-US
To:     Peter Rosin <peda@axentia.se>, linux-i2c@vger.kernel.org
Cc:     linux-iio@vger.kernel.org, wsa@kernel.org, jic23@kernel.org,
        lars@metafoo.de, miltonm@us.ibm.com, joel@jms.id.au,
        linux-kernel@vger.kernel.org
References: <20220906202829.1921114-1-eajames@linux.ibm.com>
 <20220906202829.1921114-3-eajames@linux.ibm.com>
 <31b44b63-4cf3-6fdd-b2b8-6f00070af89a@axentia.se>
From:   Eddie James <eajames@linux.ibm.com>
In-Reply-To: <31b44b63-4cf3-6fdd-b2b8-6f00070af89a@axentia.se>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
Precedence: bulk


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;