| Message ID | 20201205054749.26487-3-digetx@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Support wakeup methods of Atmel maXTouch controllers | expand |
On Sat, Dec 5, 2020 at 6:48 AM Dmitry Osipenko <digetx@gmail.com> wrote: > According to datasheets, chips like mXT1386 have a WAKE line, it is used > to wake the chip up from deep sleep mode before communicating with it via > the I2C-compatible interface. > > If the WAKE line is connected to a GPIO line, the line must be asserted > 25 ms before the host attempts to communicate with the controller. If the > WAKE line is connected to the SCL pin, the controller will send a NACK on > the first attempt to address it, the host must then retry 25 ms later. > > Implement the wake-up methods in the driver. Touchscreen now works > properly on devices like Acer A500 tablet, fixing problems like this: > > atmel_mxt_ts 0-004c: __mxt_read_reg: i2c transfer failed (-121) > atmel_mxt_ts 0-004c: mxt_bootloader_read: i2c recv failed (-121) > atmel_mxt_ts 0-004c: Trying alternate bootloader address > atmel_mxt_ts 0-004c: mxt_bootloader_read: i2c recv failed (-121) > atmel_mxt_ts: probe of 0-004c failed with error -121 > > Signed-off-by: Jiada Wang <jiada_wang@mentor.com> > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> OK looks interesting! > + /* Request the WAKE line as asserted so controller won't sleep */ > + data->wake_gpio = devm_gpiod_get_optional(&client->dev, > + "wake", GPIOD_OUT_HIGH); > + if (IS_ERR(data->wake_gpio)) { > + error = PTR_ERR(data->wake_gpio); > + dev_err(&client->dev, "Failed to get wake gpio: %d\n", error); > + return error; > + } That is a bit brutal, don't you think? Now you force the controller to be on at all times. Even across suspend/resume. Shouldn't the same patch drive this low in mxt_suspend() and driver it high + wait 25 ms in mxt_resume()? Waiting 25ms in mxt_resume() is chill, it is anyway on the slowpath. Yours, Linus Walleij
06.12.2020 18:20, Linus Walleij пишет: > On Sat, Dec 5, 2020 at 6:48 AM Dmitry Osipenko <digetx@gmail.com> wrote: > >> According to datasheets, chips like mXT1386 have a WAKE line, it is used >> to wake the chip up from deep sleep mode before communicating with it via >> the I2C-compatible interface. >> >> If the WAKE line is connected to a GPIO line, the line must be asserted >> 25 ms before the host attempts to communicate with the controller. If the >> WAKE line is connected to the SCL pin, the controller will send a NACK on >> the first attempt to address it, the host must then retry 25 ms later. >> >> Implement the wake-up methods in the driver. Touchscreen now works >> properly on devices like Acer A500 tablet, fixing problems like this: >> >> atmel_mxt_ts 0-004c: __mxt_read_reg: i2c transfer failed (-121) >> atmel_mxt_ts 0-004c: mxt_bootloader_read: i2c recv failed (-121) >> atmel_mxt_ts 0-004c: Trying alternate bootloader address >> atmel_mxt_ts 0-004c: mxt_bootloader_read: i2c recv failed (-121) >> atmel_mxt_ts: probe of 0-004c failed with error -121 >> >> Signed-off-by: Jiada Wang <jiada_wang@mentor.com> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > > OK looks interesting! > >> + /* Request the WAKE line as asserted so controller won't sleep */ >> + data->wake_gpio = devm_gpiod_get_optional(&client->dev, >> + "wake", GPIOD_OUT_HIGH); >> + if (IS_ERR(data->wake_gpio)) { >> + error = PTR_ERR(data->wake_gpio); >> + dev_err(&client->dev, "Failed to get wake gpio: %d\n", error); >> + return error; >> + } > > That is a bit brutal, don't you think? Now you force the controller > to be on at all times. Even across suspend/resume. Still it's better than a disfunctional hardware :) > Shouldn't the same patch drive this low in mxt_suspend() > and driver it high + wait 25 ms in mxt_resume()? > Waiting 25ms in mxt_resume() is chill, it is anyway on the > slowpath. I don't have hardware which uses the wake-gpio in order to test that it works properly, hence wanted to keep it minimal. But indeed sounds like toggling the GPIO in suspend/resume should be okay to do. Alright, I'll improve it in the v3. Thank you!
diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c index 2b3fff0822fe..cd52420a1f2b 100644 --- a/drivers/input/touchscreen/atmel_mxt_ts.c +++ b/drivers/input/touchscreen/atmel_mxt_ts.c @@ -31,6 +31,7 @@ #include <media/v4l2-ioctl.h> #include <media/videobuf2-v4l2.h> #include <media/videobuf2-vmalloc.h> +#include <dt-bindings/input/atmel-maxtouch.h> /* Firmware files */ #define MXT_FW_NAME "maxtouch.fw" @@ -199,6 +200,7 @@ enum t100_type { #define MXT_CRC_TIMEOUT 1000 /* msec */ #define MXT_FW_RESET_TIME 3000 /* msec */ #define MXT_FW_CHG_TIMEOUT 300 /* msec */ +#define MXT_WAKEUP_TIME 25 /* msec */ /* Command to unlock bootloader */ #define MXT_UNLOCK_CMD_MSB 0xaa @@ -312,6 +314,7 @@ struct mxt_data { struct mxt_dbg dbg; struct regulator_bulk_data regulators[2]; struct gpio_desc *reset_gpio; + struct gpio_desc *wake_gpio; bool use_retrigen_workaround; /* Cached parameters from object table */ @@ -342,6 +345,8 @@ struct mxt_data { unsigned int t19_num_keys; enum mxt_suspend_mode suspend_mode; + + u32 wakeup_method; }; struct mxt_vb2_buffer { @@ -626,10 +631,25 @@ static int mxt_send_bootloader_cmd(struct mxt_data *data, bool unlock) return 0; } +static bool mxt_wake_up(struct i2c_client *client) +{ + struct mxt_data *data = i2c_get_clientdata(client); + + if (data->wakeup_method != ATMEL_MXT_WAKEUP_I2C_SCL) + return false; + + dev_dbg(&client->dev, "waking up controller\n"); + + msleep(MXT_WAKEUP_TIME); + + return true; +} + static int __mxt_read_reg(struct i2c_client *client, u16 reg, u16 len, void *val) { struct i2c_msg xfer[2]; + bool retried = false; u8 buf[2]; int ret; @@ -648,9 +668,13 @@ static int __mxt_read_reg(struct i2c_client *client, xfer[1].len = len; xfer[1].buf = val; +retry: ret = i2c_transfer(client->adapter, xfer, 2); if (ret == 2) { ret = 0; + } else if (!retried && mxt_wake_up(client)) { + retried = true; + goto retry; } else { if (ret >= 0) ret = -EIO; @@ -664,6 +688,7 @@ static int __mxt_read_reg(struct i2c_client *client, static int __mxt_write_reg(struct i2c_client *client, u16 reg, u16 len, const void *val) { + bool retried = false; u8 *buf; size_t count; int ret; @@ -677,9 +702,13 @@ static int __mxt_write_reg(struct i2c_client *client, u16 reg, u16 len, buf[1] = (reg >> 8) & 0xff; memcpy(&buf[2], val, len); +retry: ret = i2c_master_send(client, buf, count); if (ret == count) { ret = 0; + } else if (!retried && mxt_wake_up(client)) { + retried = true; + goto retry; } else { if (ret >= 0) ret = -EIO; @@ -3160,6 +3189,15 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id) return error; } + /* Request the WAKE line as asserted so controller won't sleep */ + data->wake_gpio = devm_gpiod_get_optional(&client->dev, + "wake", GPIOD_OUT_HIGH); + if (IS_ERR(data->wake_gpio)) { + error = PTR_ERR(data->wake_gpio); + dev_err(&client->dev, "Failed to get wake gpio: %d\n", error); + return error; + } + error = devm_request_threaded_irq(&client->dev, client->irq, NULL, mxt_interrupt, IRQF_ONESHOT, client->name, data); @@ -3190,6 +3228,23 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id) msleep(MXT_RESET_INVALID_CHG); } + /* + * Controllers like mXT1386 have a dedicated WAKE line that could be + * connected to a GPIO or to I2C SCL pin, or permanently asserted low. + * + * This WAKE line is used for waking controller from a deep-sleep and + * it needs to be asserted low for 25 milliseconds before I2C transfers + * could be accepted by controller if it was in a deep-sleep mode. + * + * If WAKE line is connected to I2C SCL pin, then the first I2C transfer + * will get an instant NAK and transfer needs to be retried after 25ms. + * + * If WAKE line is connected to a GPIO line, the line must be asserted + * 25ms before the host attempts to communicate with the controller. + */ + device_property_read_u32(&client->dev, "atmel,wakeup-method", + &data->wakeup_method); + error = mxt_initialize(data); if (error) goto err_disable_regulators;