18.09.2020 15:56, Jiada Wang пишет:
> According to datasheet, mXT1386 chip has 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 mXT1386.
> If the WAKE line is connected to the SCL pin, the mXT1386 will send a
> NACK on the first attempt to address it, the host must then retry 25 ms
> later.
>
> This patch introduces mxt_wake() which does a dummy i2c read, follows
> with a 25 ms sleep for mXT1386 chip. mxt_wake() is added to
> mxt_initialize(), mxt_load_fw() and mxt_start() to ensure before any
> actual i2c transfer, mxt_wake() can be executed.
>
> Added new compatible string "atmel,mXT1386".
>
> Signed-off-by: Jiada Wang <jiada_wang@xxxxxxxxxx>
Hello, Jiada!
This not critical, but yours suggested-by tag always should be the last
line of the commit message. It's like you're signing all the words that
were written by you.
> Suggested-by: Dmitry Osipenko <digetx@xxxxxxxxx>
> ---
> drivers/input/touchscreen/atmel_mxt_ts.c | 27 ++++++++++++++++++++++++
> 1 file changed, 27 insertions(+)
>
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index a2189739e30f..d580050a237f 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -196,6 +196,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
> @@ -2099,12 +2100,33 @@ static void mxt_config_cb(const struct firmware *cfg, void *ctx)
> release_firmware(cfg);
> }
>
> +static void mxt_wake(struct mxt_data *data)
> +{
> + struct i2c_client *client = data->client;
> + struct device *dev = &data->client->dev;
> + struct device_node *np = dev->of_node;
> + union i2c_smbus_data dummy;
> +
> + if (!of_device_is_compatible(np, "atmel,mXT1386"))
> + return;
I'm not sure whether you misses the previous answers from Dmitry
Torokhov and Rob Herring, but they suggested to add a new device-tree
property which should specify the atmel,wakeup-method.
There are 3 possible variants:
- NONE
- GPIO
- I2C-SCL
Hence we should bail out from mxt_wake() if method is set to NONE or GPIO.
Perhaps we could even skip the GPIO method entirely by not mentioning it
anywhere, since this method isn't needed for now.
Nevertheless, I think it will be good to add DT compatible for the
"atmel,mXT1386", hence this part is good to me.
> + /* TODO: add WAKE-GPIO support */
> +
> + i2c_smbus_xfer(client->adapter, client->addr,
> + 0, I2C_SMBUS_READ, 0, I2C_SMBUS_BYTE,
> + &dummy);
> +
There is no need to sleep if there was no I2C error, meaning that device
wasn't in a deep-sleep mode. Please add a check for the value returned
by i2c_smbus_xfer() and invoke the msleep() only in a case of the I2C
error condition.
> + msleep(MXT_WAKEUP_TIME);
> +}
> +
> static int mxt_initialize(struct mxt_data *data)
> {
> struct i2c_client *client = data->client;
> int recovery_attempts = 0;
> int error;
>
> + mxt_wake(data);
> +
> while (1) {
I assume the mxt_wake() should be placed here, since there is a 3
seconds timeout in the end of the while-loop, meaning that device may
get back into deep-sleep on a retry.
> error = mxt_read_info_block(data);
> if (!error)
> @@ -2787,6 +2809,8 @@ static int mxt_load_fw(struct device *dev, const char *fn)
> if (ret)
> goto release_firmware;
>
> + mxt_wake(data);
> +
> if (!data->in_bootloader) {
> /* Change to the bootloader mode */
> data->in_bootloader = true;
> @@ -2928,6 +2952,7 @@ static const struct attribute_group mxt_attr_group = {
>
> static void mxt_start(struct mxt_data *data)
> {
> + mxt_wake(data);
Shouldn't the mxt_wake() be under the MXT_SUSPEND_DEEP_SLEEP switch? The
wake-up should be needed only for waking from deep-sleep mode.
> switch (data->suspend_mode) {
> case MXT_SUSPEND_T9_CTRL:
> mxt_soft_reset(data);
> @@ -3185,6 +3210,7 @@ static SIMPLE_DEV_PM_OPS(mxt_pm_ops, mxt_suspend, mxt_resume);
>
> static const struct of_device_id mxt_of_match[] = {
> { .compatible = "atmel,maxtouch", },
> + { .compatible = "atmel,mXT1386", },
> /* Compatibles listed below are deprecated */
> { .compatible = "atmel,qt602240_ts", },
> { .compatible = "atmel,atmel_mxt_ts", },
> @@ -3209,6 +3235,7 @@ static const struct i2c_device_id mxt_id[] = {
> { "atmel_mxt_tp", 0 },
> { "maxtouch", 0 },
> { "mXT224", 0 },
> + { "mXT1386", 0 },
> { }
> };
> MODULE_DEVICE_TABLE(i2c, mxt_id);
>
