linux-kernel - Re: [PATCH v3 2/4] Input: goodix - Add AVDD28-supply regulator support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAMty3ZB_=VaX+_03B=ATVKzRyBHnTijC3Z8y3+DNAZcPf-=3-Q@mail.gmail.com>
Date:   Fri, 1 Feb 2019 01:30:04 +0530
From:   Jagan Teki <jagan@...rulasolutions.com>
To:     Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc:     Chen-Yu Tsai <wens@...e.org>, linux-input@...r.kernel.org,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Michael Trimarchi <michael@...rulasolutions.com>,
        linux-amarula@...rulasolutions.com
Subject: Re: [PATCH v3 2/4] Input: goodix - Add AVDD28-supply regulator support

On Wed, Jan 9, 2019 at 1:10 PM Dmitry Torokhov
<dmitry.torokhov@...il.com> wrote:
>
> Hi Jagan,
>
> On Sat, Dec 15, 2018 at 08:48:00PM +0530, Jagan Teki wrote:
> > Goodix CTP controllers have AVDD28 pin connected to voltage
> > regulator which may not be turned on by default, like for GT5663.
> >
> > Add support for such ctp used boards by adding voltage regulator
> > handling code to goodix ctp driver.
> >
> > Signed-off-by: Jagan Teki <jagan@...rulasolutions.com>
> > ---
> >  drivers/input/touchscreen/goodix.c | 33 +++++++++++++++++++++++++-----
> >  1 file changed, 28 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
> > index f2d9c2c41885..7371f6946098 100644
> > --- a/drivers/input/touchscreen/goodix.c
> > +++ b/drivers/input/touchscreen/goodix.c
> > @@ -27,6 +27,7 @@
> >  #include <linux/delay.h>
> >  #include <linux/irq.h>
> >  #include <linux/interrupt.h>
> > +#include <linux/regulator/consumer.h>
> >  #include <linux/slab.h>
> >  #include <linux/acpi.h>
> >  #include <linux/of.h>
> > @@ -47,6 +48,7 @@ struct goodix_ts_data {
> >       struct touchscreen_properties prop;
> >       unsigned int max_touch_num;
> >       unsigned int int_trigger_type;
> > +     struct regulator *avdd28;
> >       struct gpio_desc *gpiod_int;
> >       struct gpio_desc *gpiod_rst;
> >       u16 id;
> > @@ -786,25 +788,41 @@ static int goodix_ts_probe(struct i2c_client *client,
> >       if (error)
> >               return error;
> >
> > +     ts->avdd28 = devm_regulator_get(&client->dev, "AVDD28");
> > +     if (IS_ERR(ts->avdd28)) {
> > +             error = PTR_ERR(ts->avdd28);
> > +             if (error != -EPROBE_DEFER)
> > +                     dev_err(&client->dev,
> > +                             "Failed to get AVDD28 regulator: %d\n", error);
> > +             return error;
> > +     }
> > +
> > +     /* power the controller */
> > +     error = regulator_enable(ts->avdd28);
> > +     if (error) {
> > +             dev_err(&client->dev, "Controller fail to enable AVDD28\n");
> > +             return error;
> > +     }
> > +
> >       if (ts->gpiod_int && ts->gpiod_rst) {
> >               /* reset the controller */
> >               error = goodix_reset(ts);
> >               if (error) {
> >                       dev_err(&client->dev, "Controller reset failed.\n");
> > -                     return error;
> > +                     goto error;
> >               }
> >       }
> >
> >       error = goodix_i2c_test(client);
> >       if (error) {
> >               dev_err(&client->dev, "I2C communication failure: %d\n", error);
> > -             return error;
> > +             goto error;
> >       }
> >
> >       error = goodix_read_version(ts);
> >       if (error) {
> >               dev_err(&client->dev, "Read version failed.\n");
> > -             return error;
> > +             goto error;
> >       }
> >
> >       ts->chip = goodix_get_chip_data(ts->id);
> > @@ -823,23 +841,28 @@ static int goodix_ts_probe(struct i2c_client *client,
> >                       dev_err(&client->dev,
> >                               "Failed to invoke firmware loader: %d\n",
> >                               error);
> > -                     return error;
> > +                     goto error;
> >               }
> >
> >               return 0;
> >       } else {
> >               error = goodix_configure_dev(ts);
> >               if (error)
> > -                     return error;
> > +                     goto error;
> >       }
> >
> >       return 0;
> > +
> > +error:
> > +     regulator_disable(ts->avdd28);
> > +     return error;
> >  }
> >
> >  static int goodix_ts_remove(struct i2c_client *client)
> >  {
> >       struct goodix_ts_data *ts = i2c_get_clientdata(client);
> >
> > +     regulator_disable(ts->avdd28);
>
> This may be disabling the regulator too early. Please use
> devm_add_action_or_reset() to install a custom devm handler that would
> disable the regulator in line with the rest of devm unwinding flow.

Like handler with only regulator_disable? and would that call from
suspend or not need?