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

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 3/2/20 12:35 PM, Bastien Nocera wrote:
> 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?

Good point, I've changed this to a dev_info for v2.

Regards,

Hans



> 
>> +
>> +	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;
>