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

[Wang, Jiada]

Hi Dmitry

Thanks for your quick comments

On 2020/09/18 22:32, Dmitry Osipenko wrote:
> 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.
I think Rob Herring prefers for the compatible solution than property.

> 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.
for "GPIO", we still need 25 ms sleep. but rather than a dummy read, 
WAKE line need to be asserted before sleep.

> 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.
OK. will do this

> > +	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.
Can you elaborate a little more why exit from bootload mode after sleep 
for 3 second could enter deep-sleep mode.

> >   		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.
OK, I will move it under MXT_SUSPEND_DEEP_SLEEP

Thanks,
Jiada
> >   	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);