Re: [PATCH v2 2/2] Input: atmel_mxt_ts - wake mXT1386 from deep-sleep mode

[Dmitry Osipenko <digetx@gmail.com>]

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@mentor.com>

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@gmail.com>
> ---
>  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);
>