RE: [PATCH 7/9] input: goodix: add power management support

[Tirdea, Irina]

> -----Original Message-----
> From: Bastien Nocera [mailto:hadess@xxxxxxxxxx]
> Sent: 04 June, 2015 16:01
> To: Tirdea, Irina
> Cc: Dmitry Torokhov; linux-input@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Purdila, Octavian
> Subject: Re: [PATCH 7/9] input: goodix: add power management support
>
> On Thu, 2015-05-28 at 15:47 +0300, Irina Tirdea wrote:
> > Implement suspend/resume for goodix driver.
> >
> > Signed-off-by: Octavian Purdila <octavian.purdila@xxxxxxxxx>
> > Signed-off-by: Irina Tirdea <irina.tirdea@xxxxxxxxx>
> > ---
> >  drivers/input/touchscreen/goodix.c | 81
> > +++++++++++++++++++++++++++++++++++---
> >  1 file changed, 76 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/input/touchscreen/goodix.c
> > b/drivers/input/touchscreen/goodix.c
> > index 22052c9..ce7e834 100644
> > --- a/drivers/input/touchscreen/goodix.c
> > +++ b/drivers/input/touchscreen/goodix.c
> > @@ -37,6 +37,7 @@ struct goodix_ts_data {
> >       unsigned int int_trigger_type;
> >       struct gpio_desc *gpiod_int;
> >       struct gpio_desc *gpiod_rst;
> > +     unsigned long irq_flags;
> >  };
> >
> >  #define GOODIX_GPIO_INT_NAME         "irq"
> > @@ -52,6 +53,9 @@ struct goodix_ts_data {
> >  #define GOODIX_CONFIG_MAX_LENGTH     240
> >
> >  /* Register defines */
> > +#define GOODIX_REG_COMMAND           0x8040
> > +#define GOODIX_CMD_SCREEN_OFF                0x05
> > +
> >  #define GOODIX_READ_COOR_ADDR                0x814E
> >  #define GOODIX_REG_CONFIG_DATA               0x8047
> >  #define GOODIX_REG_ID                        0x8140
> > @@ -129,6 +133,11 @@ static int goodix_i2c_write(struct i2c_client
> > *client, u16 reg, u8 *buf,
> >       return ret < 0 ? ret : (ret != 1 ? -EIO : 0);
> >  }
> >
> > +static int goodix_i2c_write_u8(struct i2c_client *client, u16 reg,
> > u8 value)
> > +{
> > +     return goodix_i2c_write(client, reg, &value, sizeof(value));
> > +}
> > +
> >  static int goodix_ts_read_input_report(struct goodix_ts_data *ts, u8
> > *data)
> >  {
> >       int touch_num;
> > @@ -227,6 +236,18 @@ static irqreturn_t goodix_ts_irq_handler(int
> > irq, void *dev_id)
> >       return IRQ_HANDLED;
> >  }
> >
> > +static void goodix_free_irq(struct goodix_ts_data *ts)
> > +{
> > +     devm_free_irq(&ts->client->dev, ts->client->irq, ts);
> > +}
> > +
> > +static int goodix_request_irq(struct goodix_ts_data *ts)
> > +{
> > +     return devm_request_threaded_irq(&ts->client->dev, ts
> > ->client->irq,
> > +                                      NULL,
> > goodix_ts_irq_handler,
> > +                                      ts->irq_flags, ts->client
> > ->name, ts);
> > +}
> > +
> >  /**
> >   * goodix_get_cfg - Get device config from ACPI/DT.
> >   *
> > @@ -562,7 +583,6 @@ static int goodix_ts_probe(struct i2c_client
> > *client,
> >                          const struct i2c_device_id *id)
> >  {
> >       struct goodix_ts_data *ts;
> > -     unsigned long irq_flags;
> >       int error;
> >       u16 version_info, id_info;
> >
> > @@ -614,10 +634,8 @@ static int goodix_ts_probe(struct i2c_client
> > *client,
> >       if (error)
> >               return error;
> >
> > -     irq_flags = goodix_irq_flags[ts->int_trigger_type] |
> > IRQF_ONESHOT;
> > -     error = devm_request_threaded_irq(&ts->client->dev, client
> > ->irq,
> > -                                       NULL,
> > goodix_ts_irq_handler,
> > -                                       irq_flags, client->name,
> > ts);
> > +     ts->irq_flags = goodix_irq_flags[ts->int_trigger_type] |
> > IRQF_ONESHOT;
> > +     error = goodix_request_irq(ts);
> >       if (error) {
> >               dev_err(&client->dev, "request IRQ failed: %d\n",
> > error);
> >               return error;
> > @@ -626,6 +644,58 @@ static int goodix_ts_probe(struct i2c_client
> > *client,
> >       return 0;
> >  }
> >
> > +#ifdef CONFIG_PM_SLEEP
> > +static int goodix_suspend(struct device *dev)
> > +{
> > +     struct i2c_client *client = to_i2c_client(dev);
> > +     struct goodix_ts_data *ts = i2c_get_clientdata(client);
> > +     int ret;
> > +
> > +     goodix_free_irq(ts);
> > +     ret = gpiod_direction_output(ts->gpiod_int, 0);
> > +     if (ret) {
> > +             goodix_request_irq(ts);
> > +             return ret;
> > +     }
> > +     usleep_range(5000, 6000);
>
> I'd appreciate some references and explanations for those magic
> numbers.
>

Most of these  numbers are taken directly from the datasheet or
from the Android gt9xx.c driver. I used datasheets and
programming guides from Goodix that are not publicly available
(for gt911 and gt9271). I have later found some datasheets specified
in a previous patch for Goodix:
https://drive.google.com/folderview?id=0BxCVOQS3ZymGfmJyY2RKbE5XbVlKNlktVTlwV0lxNEdxd2dzeWZER094cmJPVnMxN1F0Yzg&usp=sharing
However, I am not sure of their provenience so I would rather
not add them to the commit message or source code.

In this particular case, the datasheet does not specify how long to
output low on the INT pin, but 5 ms is the time used by the Android driver.

> > +
> > +     ret = goodix_i2c_write_u8(ts->client, GOODIX_REG_COMMAND,
> > +                               GOODIX_CMD_SCREEN_OFF);
> > +     if (ret) {
> > +             dev_err(&ts->client->dev, "Screen off command
> > failed\n");
> > +             gpiod_direction_input(ts->gpiod_int);
> > +             goodix_request_irq(ts);
> > +             return -EAGAIN;
> > +     }
> > +
> > +     /*
> > +      * To avoid waking up while is not sleeping,
>
> You're missing a subject in that sentence.

Ok, will fix.

>
> > +      * delay 58ms to ensure reliability
> > +      */
> > +     msleep(58);
>
> Why 58ms?
>

The datasheet specifies that the interval between sending screen-off
command and wake-up should be longer than 58 ms. I will add a comment
stating this.

For the publicly available datasheet for GT9271, you can find this in
Section 7.1 c. Sleep Mode.

> > +     return 0;
> > +}
> > +
> > +static int goodix_resume(struct device *dev)
> > +{
> > +     struct i2c_client *client = to_i2c_client(dev);
> > +     struct goodix_ts_data *ts = i2c_get_clientdata(client);
> > +     int ret;
> > +
> > +     ret = gpiod_direction_output(ts->gpiod_int, 1);
> > +     if (ret)
> > +             return ret;
> > +     usleep_range(2000, 5000);               /* 2ms~5ms */
>
> Good comment, just missing a reference (anything that could help find
> that information in the datasheets would be good).
>

Ok, will add a comment describing thiat we have to output low on the
INT pin for 2-5 ms.

For the publicly available datasheet for GT9271, you can find this also in
Section 7.1 c. Sleep Mode.

> > +     ret = goodix_int_sync(ts);
> > +     if (ret)
> > +             return ret;
> > +
> > +     return goodix_request_irq(ts);
> > +}
> > +#endif
> > +
> > +static SIMPLE_DEV_PM_OPS(goodix_pm_ops, goodix_suspend,
> > goodix_resume);
> > +
> >  static const struct i2c_device_id goodix_ts_id[] = {
> >       { "GDIX1001:00", 0 },
> >       { }
> > @@ -663,6 +733,7 @@ static struct i2c_driver goodix_ts_driver = {
> >               .owner = THIS_MODULE,
> >               .acpi_match_table = ACPI_PTR(goodix_acpi_match),
> >               .of_match_table = of_match_ptr(goodix_of_match),
> > +             .pm = &goodix_pm_ops,
> >       },
> >  };
> >  module_i2c_driver(goodix_ts_driver);
N‹§²æ¸›yú²X¬¶ÇvØ–)Þ{.nlj·¥Š{±‘êX§¶›¡Ü}©ž²ÆzÚj:+v‰¨¾«‘êZ+€Êzf£¢·hšˆ§~†­†Ûÿû®w¥¢¸?™¨è&¢)ßf”ùy§m…á«a¶Úÿ0¶ìå