[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <c7f6baa3-8181-a612-72cc-23f570f31ab3@linux.ibm.com>
Date: Wed, 7 Sep 2022 08:53:20 -0500
From: Eddie James <eajames@...ux.ibm.com>
To: Peter Rosin <peda@...ntia.se>, linux-i2c@...r.kernel.org
Cc: linux-iio@...r.kernel.org, wsa@...nel.org, jic23@...nel.org,
lars@...afoo.de, miltonm@...ibm.com, joel@....id.au,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] iio: si7020: Lock root adapter to wait for reset
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@...ux.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;
Powered by blists - more mailing lists