| Message ID | 20200221164735.508324-10-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [resend,01/10] Input: goodix - Refactor IRQ pin GPIO accesses | expand |
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;
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; >
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]); + + 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;
Some devices, e.g the Trekstor Primetab S11B, loose there config over a suspend/resume cycle (likely the controller looses power during 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> --- drivers/input/touchscreen/goodix.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)