Re: [PATCH v3 5/9] input: elants: refactor elants_i2c_execute_command()

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
To: "Michał Mirosław" <mirq-linux@rere.qmqm.pl>
Cc: Dmitry Osipenko <digetx@gmail.com>,
	Henrik Rydberg <rydberg@bitmath.org>,
	James Chen <james.chen@emc.com.tw>,
	Johnny Chuang <johnny.chuang@emc.com.tw>,
	Rob Herring <robh+dt@kernel.org>,
	Scott Liu <scott.liu@emc.com.tw>,
	David Heidelberg <david@ixit.cz>,
	linux-input@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 5/9] input: elants: refactor elants_i2c_execute_command()
Date: Sat, 25 Apr 2020 22:08:47 -0700	[thread overview]
Message-ID: <20200426050847.GO125362@dtor-ws> (raw)
In-Reply-To: <934e4ed8808de930f7380ce50cb3063c4039514e.1586784389.git.mirq-linux@rere.qmqm.pl>

On Mon, Apr 13, 2020 at 03:32:24PM +0200, Michał Mirosław wrote:
> Apply some DRY-ing to elants_i2c_execute_command() callers.  This pulls
> polling and error printk()s into a single function.
> 
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> ---
>  drivers/input/touchscreen/elants_i2c.c | 189 +++++++++++++------------
>  1 file changed, 96 insertions(+), 93 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/elants_i2c.c b/drivers/input/touchscreen/elants_i2c.c
> index 87d686ce08f2..b0f083f7f2a9 100644
> --- a/drivers/input/touchscreen/elants_i2c.c
> +++ b/drivers/input/touchscreen/elants_i2c.c
> @@ -205,7 +205,8 @@ static int elants_i2c_read(struct i2c_client *client, void *data, size_t size)
>  
>  static int elants_i2c_execute_command(struct i2c_client *client,
>  				      const u8 *cmd, size_t cmd_size,
> -				      u8 *resp, size_t resp_size)
> +				      u8 *resp, size_t resp_size,
> +				      int retries, const char *cmd_name)
>  {
>  	struct i2c_msg msgs[2];
>  	int ret;
> @@ -225,30 +226,55 @@ static int elants_i2c_execute_command(struct i2c_client *client,
>  		break;
>  
>  	default:
> -		dev_err(&client->dev, "%s: invalid command %*ph\n",
> -			__func__, (int)cmd_size, cmd);
> +		dev_err(&client->dev, "(%s): invalid command: %*ph\n",
> +			cmd_name, (int)cmd_size, cmd);
>  		return -EINVAL;
>  	}
>  
> -	msgs[0].addr = client->addr;
> -	msgs[0].flags = client->flags & I2C_M_TEN;
> -	msgs[0].len = cmd_size;
> -	msgs[0].buf = (u8 *)cmd;
> +	for (;;) {
> +		msgs[0].addr = client->addr;
> +		msgs[0].flags = client->flags & I2C_M_TEN;
> +		msgs[0].len = cmd_size;
> +		msgs[0].buf = (u8 *)cmd;
>  
> -	msgs[1].addr = client->addr;
> -	msgs[1].flags = client->flags & I2C_M_TEN;
> -	msgs[1].flags |= I2C_M_RD;
> -	msgs[1].len = resp_size;
> -	msgs[1].buf = resp;
> +		msgs[1].addr = client->addr;
> +		msgs[1].flags = client->flags & I2C_M_TEN;
> +		msgs[1].flags |= I2C_M_RD;
> +		msgs[1].len = resp_size;
> +		msgs[1].buf = resp;
>  
> -	ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
> -	if (ret < 0)
> -		return ret;
> +		ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
> +		if (ret < 0) {
> +			if (--retries > 0) {
> +				dev_dbg(&client->dev,
> +					"(%s) I2C transfer failed: %d (retrying)\n",
> +					cmd_name, ret);
> +				continue;
> +			}
>  
> -	if (ret != ARRAY_SIZE(msgs) || resp[FW_HDR_TYPE] != expected_response)
> -		return -EIO;
> +			dev_err(&client->dev,
> +				"(%s) I2C transfer failed: %d\n",
> +				cmd_name, ret);
> +			return ret;
> +		}
>  
> -	return 0;
> +		if (ret != ARRAY_SIZE(msgs) ||
> +		    resp[FW_HDR_TYPE] != expected_response) {
> +			if (--retries > 0) {
> +				dev_dbg(&client->dev,
> +					"(%s) unexpected response: %*ph (retrying)\n",
> +					cmd_name, ret, resp);
> +				continue;
> +			}
> +
> +			dev_err(&client->dev,
> +				"(%s) unexpected response: %*ph\n",
> +				cmd_name, ret, resp);
> +			return -EIO;
> +		}
> +
> +		return --retries;

I'd prefer if we returned 0 on success and I'd be OK when flashing
firmware to have separate retry counters for the command themselves and
for checking the response.

Thanks.

-- 
Dmitry