Re: [PATCH resend 10/10] Input: goodix - Restore config on resume if necessary

[Bastien Nocera <hadess@hadess.net>]

On Fri, 2020-02-21 at 17:47 +0100, Hans de Goede wrote:
> Some devices, e.g the Trekstor Primetab S11B, loose there config over

"lose".

> a suspend/resume cycle (likely the controller looses power during 

"loses".

> suspend).
> 
> This commit reads back the config version on resume and if matches
> the
> expected config version it resets the controller and resends the
> config
> we read back and saved at probe time.
> 
> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1786317
> BugLink: https://github.com/nexus511/gpd-ubuntu-packages/issues/10
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=199207
> Cc: Dmitry Mastykin <mastichi@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Looks fine apart from the nitpicks.

Reviewed-by: Bastien Nocera <hadess@hadess.net>

> ---
>  drivers/input/touchscreen/goodix.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/input/touchscreen/goodix.c
> b/drivers/input/touchscreen/goodix.c
> index 0f39c499e3a9..389d3e044f97 100644
> --- a/drivers/input/touchscreen/goodix.c
> +++ b/drivers/input/touchscreen/goodix.c
> @@ -1232,6 +1232,7 @@ static int __maybe_unused goodix_resume(struct
> device *dev)
>  {
>  	struct i2c_client *client = to_i2c_client(dev);
>  	struct goodix_ts_data *ts = i2c_get_clientdata(client);
> +	u8 config_ver;
>  	int error;
>  
>  	if (ts->irq_pin_access_method == irq_pin_access_none) {
> @@ -1253,6 +1254,27 @@ static int __maybe_unused goodix_resume(struct
> device *dev)
>  	if (error)
>  		return error;
>  
> +	error = goodix_i2c_read(ts->client, ts->chip->config_addr,
> +				&config_ver, 1);
> +	if (error)
> +		dev_warn(dev, "Error reading config version: %d,
> resetting controller\n",
> +			 error);
> +	else if (config_ver != ts->config[0])
> +		dev_warn(dev, "Config version mismatch %d != %d,
> resetting controller\n",
> +			 config_ver, ts->config[0]);

Should it really be a warning if it happens regularly?

> +
> +	if (error != 0 || config_ver != ts->config[0]) {
> +		error = goodix_reset(ts);
> +		if (error) {
> +			dev_err(dev, "Controller reset failed.\n");
> +			return error;
> +		}
> +
> +		error = goodix_send_cfg(ts, ts->config, ts->chip-
> >config_len);
> +		if (error)
> +			return error;
> +	}
> +
>  	error = goodix_request_irq(ts);
>  	if (error)
>  		return error;