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: <31b44b63-4cf3-6fdd-b2b8-6f00070af89a@axentia.se>
Date:   Wed, 7 Sep 2022 09:10:50 +0200
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; 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:     Eddie James <eajames@linux.ibm.com>, 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>
From:   Peter Rosin <peda@axentia.se>
In-Reply-To: <20220906202829.1921114-3-eajames@linux.ibm.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?bmo0NmZnWlZYYkhTTkg0Yk1Sam9KSHhjUFd6Z3V2TXpIcGRPWGdNeHFQc3Q0?=
 =?utf-8?B?d09hUi92d28yQkZ0UWZjQTIvUHlDQkJISFoxSjBEME9PeFVaZVkwa21oWDZB?=
 =?utf-8?B?TUl5c3dPSGwvd2pwKzg3UWJnVCtlTzJaL1NnUEY3aTQ2ZmpyWm9iaUVyUkpR?=
 =?utf-8?B?TEFUcW1kU1NhTzVNd0RseXVQejdKT0kvMFdvNjIzZnVTSDZubStVaUJTdFFU?=
 =?utf-8?B?aG9RSXJYV0swNndqMzNvUmxGWjFNdW5OWjduMUlMNW5wUDFQM3I3SGllcG44?=
 =?utf-8?B?di93Lzl2TExOajhYb1ZvaTZURWhKUmkwZ2ZudXMyOG1tUDdWNXhVYm9sTy9h?=
 =?utf-8?B?NTRReS9FMy84TFJzZFdYREhyaGtpS2tMUHpNcnp2ZmsyK0o2ZnYzQUZPZmdR?=
 =?utf-8?B?b0dqU3NZdUtDR055bGwxKzNaTXloQlg1WDVRQjNrV3pVclByVC8rZHFSZHhu?=
 =?utf-8?B?dnBYdkdWQkJXWlN5ZlBQS28xb213ZkVudHA3bnNEQUNYWDZpR0xjMlpzZ1ZV?=
 =?utf-8?B?SnNJVTgwRVVkb2NTdDYzRnY4K01NR1hKNE1WdGxBdWUwa2xHQlBDN3dJZHV3?=
 =?utf-8?B?bFpqMVFrSC9BcFZvQ2NtalhJdVdBYWRRaGt5aUsyN1VpTHA0a0xjY3BGbFdK?=
 =?utf-8?B?djVxSWI5WExlWWNvNFV3RXJSNFliSUxLYnhWOHlQbVNEUUVEUlZTTHlyRitu?=
 =?utf-8?B?R2l3NFFMSmxhb1RwNkkzeXEvSDVFeXpoNEtuS1czTUFraVY3MFJwWHRrNk5M?=
 =?utf-8?B?MkRWcDk1T0pDTFliRTVJQUd6cUR1b3ZwQms2RzBleHl2d0JlemJIaitjTnh0?=
 =?utf-8?B?NE1TY0o2SDFmbjZhZFh3K0xGelVYRTh0M2h4bXJoSHZRS3d5QVBIOUljdzdn?=
 =?utf-8?B?b2l1dEwwbHJWS2Y5M3FwZUEzY0F6MkJ5UTlObHVNeUpvZnU1TG1obFFaV3BF?=
 =?utf-8?B?b3pFZkxabUtvTHExOXZrQmdib2wwNEFiRDg4R3MxNk0xVy8wK3YvR0Y4RHNY?=
 =?utf-8?B?ckNpc1VtZE9aNDdkV3ZjaXByTkp2czBrT0Q1MUVyV05RdUFUWndLRG1GQmpl?=
 =?utf-8?B?anVnRE5UTmUyTklFWGVmN21lL1Zod3JvSUl1amlhOE1CbHEwZmFHamZ5TFpY?=
 =?utf-8?B?RVRSZVAwVkwySVNNS1ZjeHpZeFdLOWtvNzcrRFNaUmh6bFg1TTFCNm1oNmR2?=
 =?utf-8?B?ZjZLbmEwT2xQMHNkdTJNU3J5d1VKSEdCT1R6S1BJLzJsZ2M1Z2J4VW9qdWN0?=
 =?utf-8?B?RStxVnFrNUsvU2grMXVVam4wNEdzV0NKb04yNDExeGZ6U05DaE9oS0wwck5s?=
 =?utf-8?B?WXRuTlJNMHo5czNuK3M0U1h5eDBxYXFtKy9rb2lweHJVWjVreEU0ZkhTc0l5?=
 =?utf-8?B?Q2JOb296K3Y0cGZIOGlZTGVqN1VwSFZLN3pvdEdRbW92NkowQzdIVU92VmJn?=
 =?utf-8?B?L2trbWwzNEEyTjRIYW1ESEpWd0p3bk9LV21BdlN4dWwva1FEMGRzdytLTFM0?=
 =?utf-8?B?aHhJSDdKZlF4UXAwYitTY1lOQlJCT294dXhpTVBZU3FScU9VWDRjdjdrTytr?=
 =?utf-8?B?eUJkK093djFieXZaeGR6Q2dJa21Ua2VjM3dhY1h6V1RkM2RhL21rR1c1bkdQ?=
 =?utf-8?B?anRHckdVQk5TYS9pY3FHY2dBejNIVmVUTEtPbzZkLytENHVVdC85VW5nZUhE?=
 =?utf-8?B?Rjh3S1I2UUtqYjV2N2FBdXViOTVsQWpPWFMxYXJDd3R2KzRwVlhyK2VVcFpX?=
 =?utf-8?B?Z0RrTVR0RnMxbW04RmZnYkZhbHR0VTQzb0Zla2RFcGJMKzNoTlRTUzFhS25Z?=
 =?utf-8?B?d3dQbmx4NnZrbGp0ZlVHeXV2TWVQaytzTVphS1htYk03aFhQZFp2cUoyQkEw?=
 =?utf-8?B?SWJDcXlpSmdLS3FnekJWMGdVd1p0VTEyL29TOUE1cTZSVncrSlNVMCsweUx6?=
 =?utf-8?B?ZjIyTXkyaFdtdzdHeXpqUkdLZDVoQUlGa2hRYzkvdVRCUi9iUjNXSGRpcDNQ?=
 =?utf-8?B?OHJZaXdrQ3ZBUHd1em8zMmFmZWpFNGpJc2NldFJtMnFtNmFsTFBvdHBRNlJF?=
 =?utf-8?B?VUgvMWRIcnprUDlSa0s0VTdRZWxBUmlNaWdJMWl1TFR5TEdDMk15NklIUGcv?=
 =?utf-8?Q?9xHzmPwYvtUU5hX1lYkUoPSqB?=
X-OriginatorOrg: axentia.se
X-MS-Exchange-CrossTenant-Network-Message-Id: aa96388c-4460-4b3c-d091-08da90a018ff
X-MS-Exchange-CrossTenant-AuthSource: AM0PR02MB4436.eurprd02.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 Sep 2022 07:10:51.6806
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4ee68585-03e1-4785-942a-df9c1871a234
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: E7fU5U+NgkBN0IQ1DOc98dxnzV77JsGpxE1G65hZxBiJKWj9YwVqJIghfTRbHkMR
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB9PR02MB7916
X-Spam-Status: No, score=-3.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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;