* [PATCH v2 1/2] Input: atmel_mxt_ts: Add support for optional regulators.
@ 2018-07-17 21:00 ` Nick Dyer
0 siblings, 0 replies; 19+ messages in thread
From: Nick Dyer @ 2018-07-17 21:00 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Jul 17, 2018 at 08:16:25PM +0200, Pawe? Chmiel wrote:
> This patch adds optional regulators, which can be used to power
> up touchscreen. After enabling regulators, we need to wait 150msec.
> This value is taken from official driver.
>
> It was tested on Samsung Galaxy i9000 (based on Samsung S5PV210 SOC).
>
> Signed-off-by: Pawe? Chmiel <pawel.mikolaj.chmiel@gmail.com>
> ---
> Changes from v1:
> - Enable regulators only if reset_gpio is present.
> - Switch from devm_regulator_get_optional to devm_regulator_get
> ---
> drivers/input/touchscreen/atmel_mxt_ts.c | 46 ++++++++++++++++++++++++++++++--
> 1 file changed, 44 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index 54fe190fd4bc..005f0fee9fc8 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -27,6 +27,7 @@
> #include <linux/interrupt.h>
> #include <linux/of.h>
> #include <linux/property.h>
> +#include <linux/regulator/consumer.h>
> #include <linux/slab.h>
> #include <linux/gpio/consumer.h>
> #include <linux/property.h>
> @@ -194,10 +195,10 @@ enum t100_type {
>
> /* Delay times */
> #define MXT_BACKUP_TIME 50 /* msec */
> -#define MXT_RESET_GPIO_TIME 20 /* msec */
> #define MXT_RESET_INVALID_CHG 100 /* msec */
> #define MXT_RESET_TIME 200 /* msec */
> #define MXT_RESET_TIMEOUT 3000 /* msec */
> +#define MXT_REGULATOR_DELAY 150 /* msec */
> #define MXT_CRC_TIMEOUT 1000 /* msec */
> #define MXT_FW_RESET_TIME 3000 /* msec */
> #define MXT_FW_CHG_TIMEOUT 300 /* msec */
> @@ -310,6 +311,8 @@ struct mxt_data {
> struct t7_config t7_cfg;
> struct mxt_dbg dbg;
> struct gpio_desc *reset_gpio;
> + struct regulator *vdd_reg;
> + struct regulator *avdd_reg;
>
> /* Cached parameters from object table */
> u16 T5_address;
> @@ -3076,6 +3079,22 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id)
> return error;
> }
>
> + data->vdd_reg = devm_regulator_get(&client->dev, "vdd");
> + if (IS_ERR(data->vdd_reg)) {
> + error = PTR_ERR(data->vdd_reg);
> + dev_err(&client->dev, "Failed to get vdd regulator: %d\n",
> + error);
> + return error;
> + }
> +
> + data->avdd_reg = devm_regulator_get(&client->dev, "avdd");
> + if (IS_ERR(data->avdd_reg)) {
> + error = PTR_ERR(data->avdd_reg);
> + dev_err(&client->dev, "Failed to get avdd regulator: %d\n",
> + error);
> + return error;
> + }
> +
> error = devm_request_threaded_irq(&client->dev, client->irq,
> NULL, mxt_interrupt, IRQF_ONESHOT,
> client->name, data);
> @@ -3087,7 +3106,26 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id)
> disable_irq(client->irq);
>
> if (data->reset_gpio) {
> - msleep(MXT_RESET_GPIO_TIME);
> + error = regulator_enable(data->vdd_reg);
> + if (error) {
> + dev_err(&client->dev, "Failed to enable vdd regulator: %d\n",
> + error);
> + return error;
> + }
> +
> + error = regulator_enable(data->avdd_reg);
> + if (error) {
> + dev_err(&client->dev, "Failed to enable avdd regulator: %d\n",
> + error);
> + return error;
> + }
> +
> + /*
> + * According to maXTouch power sequencing specification, RESET line
> + * must be kept low until some time after regulators come up to
> + * voltage
> + */
> + msleep(MXT_REGULATOR_DELAY);
> gpiod_set_value(data->reset_gpio, 1);
> msleep(MXT_RESET_INVALID_CHG);
Hi Pawel-
I see you've borrowed some of the logic from the patch I wrote a while
back (see https://github.com/ndyer/linux/commit/8e9687e41ed062 )
The correct behaviour according to Atmel should be:
* Make RESET zero
* Bring up regulators
* Wait for a period to settle (150 msec)
* Release RESET
* Wait for 100 msec whilst device gets through bootloader
* Wait for CHG line assert before reading info block
I can't see the first and last steps in your patch at present.
The only downside with this approach is that there are a lot of
delays during driver probe, but I believe the asynchronous probe stuff
that's landed since I wrote the original patch should address that.
cheers
Nick
> }
> @@ -3116,6 +3154,10 @@ static int mxt_remove(struct i2c_client *client)
> struct mxt_data *data = i2c_get_clientdata(client);
>
> disable_irq(data->irq);
> + if (data->reset_gpio) {
> + regulator_disable(data->avdd_reg);
> + regulator_disable(data->vdd_reg);
> + }
> sysfs_remove_group(&client->dev.kobj, &mxt_attr_group);
> mxt_free_input_device(data);
> mxt_free_object_table(data);
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v2 1/2] Input: atmel_mxt_ts: Add support for optional regulators.
2018-07-17 21:00 ` Nick Dyer
@ 2018-07-18 16:21 ` Paweł Chmiel
-1 siblings, 0 replies; 19+ messages in thread
From: Paweł Chmiel @ 2018-07-18 16:21 UTC (permalink / raw)
To: Nick Dyer
Cc: dmitry.torokhov, robh+dt, mark.rutland, nicolas.ferre,
alexandre.belloni, linux-input, devicetree, linux-arm-kernel,
linux-kernel
On Tuesday, July 17, 2018 10:00:05 PM CEST Nick Dyer wrote:
> On Tue, Jul 17, 2018 at 08:16:25PM +0200, Paweł Chmiel wrote:
> > This patch adds optional regulators, which can be used to power
> > up touchscreen. After enabling regulators, we need to wait 150msec.
> > This value is taken from official driver.
> >
> > It was tested on Samsung Galaxy i9000 (based on Samsung S5PV210 SOC).
> >
> > Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
> > ---
> > Changes from v1:
> > - Enable regulators only if reset_gpio is present.
> > - Switch from devm_regulator_get_optional to devm_regulator_get
> > ---
> > drivers/input/touchscreen/atmel_mxt_ts.c | 46 ++++++++++++++++++++++++++++++--
> > 1 file changed, 44 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> > index 54fe190fd4bc..005f0fee9fc8 100644
> > --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> > +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> > @@ -27,6 +27,7 @@
> > #include <linux/interrupt.h>
> > #include <linux/of.h>
> > #include <linux/property.h>
> > +#include <linux/regulator/consumer.h>
> > #include <linux/slab.h>
> > #include <linux/gpio/consumer.h>
> > #include <linux/property.h>
> > @@ -194,10 +195,10 @@ enum t100_type {
> >
> > /* Delay times */
> > #define MXT_BACKUP_TIME 50 /* msec */
> > -#define MXT_RESET_GPIO_TIME 20 /* msec */
> > #define MXT_RESET_INVALID_CHG 100 /* msec */
> > #define MXT_RESET_TIME 200 /* msec */
> > #define MXT_RESET_TIMEOUT 3000 /* msec */
> > +#define MXT_REGULATOR_DELAY 150 /* msec */
> > #define MXT_CRC_TIMEOUT 1000 /* msec */
> > #define MXT_FW_RESET_TIME 3000 /* msec */
> > #define MXT_FW_CHG_TIMEOUT 300 /* msec */
> > @@ -310,6 +311,8 @@ struct mxt_data {
> > struct t7_config t7_cfg;
> > struct mxt_dbg dbg;
> > struct gpio_desc *reset_gpio;
> > + struct regulator *vdd_reg;
> > + struct regulator *avdd_reg;
> >
> > /* Cached parameters from object table */
> > u16 T5_address;
> > @@ -3076,6 +3079,22 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id)
> > return error;
> > }
> >
> > + data->vdd_reg = devm_regulator_get(&client->dev, "vdd");
> > + if (IS_ERR(data->vdd_reg)) {
> > + error = PTR_ERR(data->vdd_reg);
> > + dev_err(&client->dev, "Failed to get vdd regulator: %d\n",
> > + error);
> > + return error;
> > + }
> > +
> > + data->avdd_reg = devm_regulator_get(&client->dev, "avdd");
> > + if (IS_ERR(data->avdd_reg)) {
> > + error = PTR_ERR(data->avdd_reg);
> > + dev_err(&client->dev, "Failed to get avdd regulator: %d\n",
> > + error);
> > + return error;
> > + }
> > +
> > error = devm_request_threaded_irq(&client->dev, client->irq,
> > NULL, mxt_interrupt, IRQF_ONESHOT,
> > client->name, data);
> > @@ -3087,7 +3106,26 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id)
> > disable_irq(client->irq);
> >
> > if (data->reset_gpio) {
> > - msleep(MXT_RESET_GPIO_TIME);
> > + error = regulator_enable(data->vdd_reg);
> > + if (error) {
> > + dev_err(&client->dev, "Failed to enable vdd regulator: %d\n",
> > + error);
> > + return error;
> > + }
> > +
> > + error = regulator_enable(data->avdd_reg);
> > + if (error) {
> > + dev_err(&client->dev, "Failed to enable avdd regulator: %d\n",
> > + error);
> > + return error;
> > + }
> > +
> > + /*
> > + * According to maXTouch power sequencing specification, RESET line
> > + * must be kept low until some time after regulators come up to
> > + * voltage
> > + */
> > + msleep(MXT_REGULATOR_DELAY);
> > gpiod_set_value(data->reset_gpio, 1);
> > msleep(MXT_RESET_INVALID_CHG);
>
> Hi Pawel-
>
> I see you've borrowed some of the logic from the patch I wrote a while
> back (see https://github.com/ndyer/linux/commit/8e9687e41ed062 )
Actually, i was looking at https://github.com/atmel-maxtouch/linux/blob/maxtouch-v3.14/drivers/input/touchscreen/atmel_mxt_ts.c (and didn't saw Your patch till now).
Are You going to submit it? (it has more functionalities - for example suspend mode read from device tree).
>
> The correct behaviour according to Atmel should be:
>
> * Make RESET zero
> * Bring up regulators
> * Wait for a period to settle (150 msec)
> * Release RESET
> * Wait for 100 msec whilst device gets through bootloader
> * Wait for CHG line assert before reading info block
>
> I can't see the first and last steps in your patch at present.
About first step - reset_gpio is readed by using devm_gpiod_get_optional with GPIOD_OUT_LOW flag, so i think (but might be wrong) that we don't need to set this gpio value again to 0 before enabling regulators,
since currently only place where reset_gpio is used is in driver probe (in Your patch it is used in other cases/places - for example in mxt_start/stop, when we enable regulators).
About missing wait after releasing reset, shouldn't this be separate patch (since currently driver is not doing it)? I can prepare it and send with other in next version.
Thanks for feedback
>
> The only downside with this approach is that there are a lot of
> delays during driver probe, but I believe the asynchronous probe stuff
> that's landed since I wrote the original patch should address that.
>
> cheers
>
> Nick
>
> > }
> > @@ -3116,6 +3154,10 @@ static int mxt_remove(struct i2c_client *client)
> > struct mxt_data *data = i2c_get_clientdata(client);
> >
> > disable_irq(data->irq);
> > + if (data->reset_gpio) {
> > + regulator_disable(data->avdd_reg);
> > + regulator_disable(data->vdd_reg);
> > + }
> > sysfs_remove_group(&client->dev.kobj, &mxt_attr_group);
> > mxt_free_input_device(data);
> > mxt_free_object_table(data);
>
^ permalink raw reply [flat|nested] 19+ messages in thread* [PATCH v2 1/2] Input: atmel_mxt_ts: Add support for optional regulators.
@ 2018-07-18 16:21 ` Paweł Chmiel
0 siblings, 0 replies; 19+ messages in thread
From: Paweł Chmiel @ 2018-07-18 16:21 UTC (permalink / raw)
To: linux-arm-kernel
On Tuesday, July 17, 2018 10:00:05 PM CEST Nick Dyer wrote:
> On Tue, Jul 17, 2018 at 08:16:25PM +0200, Pawe? Chmiel wrote:
> > This patch adds optional regulators, which can be used to power
> > up touchscreen. After enabling regulators, we need to wait 150msec.
> > This value is taken from official driver.
> >
> > It was tested on Samsung Galaxy i9000 (based on Samsung S5PV210 SOC).
> >
> > Signed-off-by: Pawe? Chmiel <pawel.mikolaj.chmiel@gmail.com>
> > ---
> > Changes from v1:
> > - Enable regulators only if reset_gpio is present.
> > - Switch from devm_regulator_get_optional to devm_regulator_get
> > ---
> > drivers/input/touchscreen/atmel_mxt_ts.c | 46 ++++++++++++++++++++++++++++++--
> > 1 file changed, 44 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> > index 54fe190fd4bc..005f0fee9fc8 100644
> > --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> > +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> > @@ -27,6 +27,7 @@
> > #include <linux/interrupt.h>
> > #include <linux/of.h>
> > #include <linux/property.h>
> > +#include <linux/regulator/consumer.h>
> > #include <linux/slab.h>
> > #include <linux/gpio/consumer.h>
> > #include <linux/property.h>
> > @@ -194,10 +195,10 @@ enum t100_type {
> >
> > /* Delay times */
> > #define MXT_BACKUP_TIME 50 /* msec */
> > -#define MXT_RESET_GPIO_TIME 20 /* msec */
> > #define MXT_RESET_INVALID_CHG 100 /* msec */
> > #define MXT_RESET_TIME 200 /* msec */
> > #define MXT_RESET_TIMEOUT 3000 /* msec */
> > +#define MXT_REGULATOR_DELAY 150 /* msec */
> > #define MXT_CRC_TIMEOUT 1000 /* msec */
> > #define MXT_FW_RESET_TIME 3000 /* msec */
> > #define MXT_FW_CHG_TIMEOUT 300 /* msec */
> > @@ -310,6 +311,8 @@ struct mxt_data {
> > struct t7_config t7_cfg;
> > struct mxt_dbg dbg;
> > struct gpio_desc *reset_gpio;
> > + struct regulator *vdd_reg;
> > + struct regulator *avdd_reg;
> >
> > /* Cached parameters from object table */
> > u16 T5_address;
> > @@ -3076,6 +3079,22 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id)
> > return error;
> > }
> >
> > + data->vdd_reg = devm_regulator_get(&client->dev, "vdd");
> > + if (IS_ERR(data->vdd_reg)) {
> > + error = PTR_ERR(data->vdd_reg);
> > + dev_err(&client->dev, "Failed to get vdd regulator: %d\n",
> > + error);
> > + return error;
> > + }
> > +
> > + data->avdd_reg = devm_regulator_get(&client->dev, "avdd");
> > + if (IS_ERR(data->avdd_reg)) {
> > + error = PTR_ERR(data->avdd_reg);
> > + dev_err(&client->dev, "Failed to get avdd regulator: %d\n",
> > + error);
> > + return error;
> > + }
> > +
> > error = devm_request_threaded_irq(&client->dev, client->irq,
> > NULL, mxt_interrupt, IRQF_ONESHOT,
> > client->name, data);
> > @@ -3087,7 +3106,26 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id)
> > disable_irq(client->irq);
> >
> > if (data->reset_gpio) {
> > - msleep(MXT_RESET_GPIO_TIME);
> > + error = regulator_enable(data->vdd_reg);
> > + if (error) {
> > + dev_err(&client->dev, "Failed to enable vdd regulator: %d\n",
> > + error);
> > + return error;
> > + }
> > +
> > + error = regulator_enable(data->avdd_reg);
> > + if (error) {
> > + dev_err(&client->dev, "Failed to enable avdd regulator: %d\n",
> > + error);
> > + return error;
> > + }
> > +
> > + /*
> > + * According to maXTouch power sequencing specification, RESET line
> > + * must be kept low until some time after regulators come up to
> > + * voltage
> > + */
> > + msleep(MXT_REGULATOR_DELAY);
> > gpiod_set_value(data->reset_gpio, 1);
> > msleep(MXT_RESET_INVALID_CHG);
>
> Hi Pawel-
>
> I see you've borrowed some of the logic from the patch I wrote a while
> back (see https://github.com/ndyer/linux/commit/8e9687e41ed062 )
Actually, i was looking at https://github.com/atmel-maxtouch/linux/blob/maxtouch-v3.14/drivers/input/touchscreen/atmel_mxt_ts.c (and didn't saw Your patch till now).
Are You going to submit it? (it has more functionalities - for example suspend mode read from device tree).
>
> The correct behaviour according to Atmel should be:
>
> * Make RESET zero
> * Bring up regulators
> * Wait for a period to settle (150 msec)
> * Release RESET
> * Wait for 100 msec whilst device gets through bootloader
> * Wait for CHG line assert before reading info block
>
> I can't see the first and last steps in your patch at present.
About first step - reset_gpio is readed by using devm_gpiod_get_optional with GPIOD_OUT_LOW flag, so i think (but might be wrong) that we don't need to set this gpio value again to 0 before enabling regulators,
since currently only place where reset_gpio is used is in driver probe (in Your patch it is used in other cases/places - for example in mxt_start/stop, when we enable regulators).
About missing wait after releasing reset, shouldn't this be separate patch (since currently driver is not doing it)? I can prepare it and send with other in next version.
Thanks for feedback
>
> The only downside with this approach is that there are a lot of
> delays during driver probe, but I believe the asynchronous probe stuff
> that's landed since I wrote the original patch should address that.
>
> cheers
>
> Nick
>
> > }
> > @@ -3116,6 +3154,10 @@ static int mxt_remove(struct i2c_client *client)
> > struct mxt_data *data = i2c_get_clientdata(client);
> >
> > disable_irq(data->irq);
> > + if (data->reset_gpio) {
> > + regulator_disable(data->avdd_reg);
> > + regulator_disable(data->vdd_reg);
> > + }
> > sysfs_remove_group(&client->dev.kobj, &mxt_attr_group);
> > mxt_free_input_device(data);
> > mxt_free_object_table(data);
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v2 1/2] Input: atmel_mxt_ts: Add support for optional regulators.
2018-07-18 16:21 ` Paweł Chmiel
@ 2018-07-19 20:54 ` Nick Dyer
-1 siblings, 0 replies; 19+ messages in thread
From: Nick Dyer @ 2018-07-19 20:54 UTC (permalink / raw)
To: Paweł Chmiel
Cc: dmitry.torokhov, robh+dt, mark.rutland, nicolas.ferre,
alexandre.belloni, linux-input, devicetree, linux-arm-kernel,
linux-kernel
On Wed, Jul 18, 2018 at 06:21:30PM +0200, Paweł Chmiel wrote:
> On Tuesday, July 17, 2018 10:00:05 PM CEST Nick Dyer wrote:
> > On Tue, Jul 17, 2018 at 08:16:25PM +0200, Paweł Chmiel wrote:
> > > This patch adds optional regulators, which can be used to power
> > > up touchscreen. After enabling regulators, we need to wait 150msec.
> > > This value is taken from official driver.
> > >
> > > It was tested on Samsung Galaxy i9000 (based on Samsung S5PV210 SOC).
> > >
> > > Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
> > > ---
> > > Changes from v1:
> > > - Enable regulators only if reset_gpio is present.
> > > - Switch from devm_regulator_get_optional to devm_regulator_get
> > > ---
> > > drivers/input/touchscreen/atmel_mxt_ts.c | 46 ++++++++++++++++++++++++++++++--
> > > 1 file changed, 44 insertions(+), 2 deletions(-)
> >
> > Hi Pawel-
> >
> > I see you've borrowed some of the logic from the patch I wrote a while
> > back (see https://github.com/ndyer/linux/commit/8e9687e41ed062 )
> Actually, i was looking at https://github.com/atmel-maxtouch/linux/blob/maxtouch-v3.14/drivers/input/touchscreen/atmel_mxt_ts.c (and didn't saw Your patch till now).
> Are You going to submit it? (it has more functionalities - for example
> suspend mode read from device tree).
Getting that work upstream has stalled for a couple of years because I
changed jobs. I have actually started recently to dust it off again, it
was later on in my queue but if you have the time to work on it that is
great.
> > The correct behaviour according to Atmel should be:
> >
> > * Make RESET zero
> > * Bring up regulators
> > * Wait for a period to settle (150 msec)
> > * Release RESET
> > * Wait for 100 msec whilst device gets through bootloader
> > * Wait for CHG line assert before reading info block
> >
> > I can't see the first and last steps in your patch at present.
> About first step - reset_gpio is readed by using
> devm_gpiod_get_optional with GPIOD_OUT_LOW flag, so i think (but might
> be wrong) that we don't need to set this gpio value again to 0 before
> enabling regulators,
I see what you mean - that is fair enough.
> since currently only place where reset_gpio is used is in driver probe
> (in Your patch it is used in other cases/places - for example in
> mxt_start/stop, when we enable regulators).
> About missing wait after releasing reset, shouldn't this be separate
> patch (since currently driver is not doing it)? I can prepare it and
> send with other in next version.
According to the maxtouch documentation, it isn't ready for comms until
the firmware asserts the CHG line. I've seen a bunch of devices that get
by without an explicit wait because the board file does the power on,
and by the time the driver gets to probe it's a few hundred ms later
anyway, so it doesn't matter. But if we put it all in the driver, it
will attempt to read the info block straight after the 100 msec delay
without waiting for CHG, and I suspect we'll end up with occasional
probe failures. It'll depend on the maxtouch device, though: they have a
range of different power on timings.
Which platform are you doing this for? Is it a Chromebook?
> Thanks for feedback
> >
> > The only downside with this approach is that there are a lot of
> > delays during driver probe, but I believe the asynchronous probe stuff
> > that's landed since I wrote the original patch should address that.
> >
> > cheers
> >
> > Nick
> >
> > > }
> > > @@ -3116,6 +3154,10 @@ static int mxt_remove(struct i2c_client *client)
> > > struct mxt_data *data = i2c_get_clientdata(client);
> > >
> > > disable_irq(data->irq);
> > > + if (data->reset_gpio) {
> > > + regulator_disable(data->avdd_reg);
> > > + regulator_disable(data->vdd_reg);
> > > + }
> > > sysfs_remove_group(&client->dev.kobj, &mxt_attr_group);
> > > mxt_free_input_device(data);
> > > mxt_free_object_table(data);
> >
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread* [PATCH v2 1/2] Input: atmel_mxt_ts: Add support for optional regulators.
@ 2018-07-19 20:54 ` Nick Dyer
0 siblings, 0 replies; 19+ messages in thread
From: Nick Dyer @ 2018-07-19 20:54 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Jul 18, 2018 at 06:21:30PM +0200, Pawe? Chmiel wrote:
> On Tuesday, July 17, 2018 10:00:05 PM CEST Nick Dyer wrote:
> > On Tue, Jul 17, 2018 at 08:16:25PM +0200, Pawe? Chmiel wrote:
> > > This patch adds optional regulators, which can be used to power
> > > up touchscreen. After enabling regulators, we need to wait 150msec.
> > > This value is taken from official driver.
> > >
> > > It was tested on Samsung Galaxy i9000 (based on Samsung S5PV210 SOC).
> > >
> > > Signed-off-by: Pawe? Chmiel <pawel.mikolaj.chmiel@gmail.com>
> > > ---
> > > Changes from v1:
> > > - Enable regulators only if reset_gpio is present.
> > > - Switch from devm_regulator_get_optional to devm_regulator_get
> > > ---
> > > drivers/input/touchscreen/atmel_mxt_ts.c | 46 ++++++++++++++++++++++++++++++--
> > > 1 file changed, 44 insertions(+), 2 deletions(-)
> >
> > Hi Pawel-
> >
> > I see you've borrowed some of the logic from the patch I wrote a while
> > back (see https://github.com/ndyer/linux/commit/8e9687e41ed062 )
> Actually, i was looking at https://github.com/atmel-maxtouch/linux/blob/maxtouch-v3.14/drivers/input/touchscreen/atmel_mxt_ts.c (and didn't saw Your patch till now).
> Are You going to submit it? (it has more functionalities - for example
> suspend mode read from device tree).
Getting that work upstream has stalled for a couple of years because I
changed jobs. I have actually started recently to dust it off again, it
was later on in my queue but if you have the time to work on it that is
great.
> > The correct behaviour according to Atmel should be:
> >
> > * Make RESET zero
> > * Bring up regulators
> > * Wait for a period to settle (150 msec)
> > * Release RESET
> > * Wait for 100 msec whilst device gets through bootloader
> > * Wait for CHG line assert before reading info block
> >
> > I can't see the first and last steps in your patch at present.
> About first step - reset_gpio is readed by using
> devm_gpiod_get_optional with GPIOD_OUT_LOW flag, so i think (but might
> be wrong) that we don't need to set this gpio value again to 0 before
> enabling regulators,
I see what you mean - that is fair enough.
> since currently only place where reset_gpio is used is in driver probe
> (in Your patch it is used in other cases/places - for example in
> mxt_start/stop, when we enable regulators).
> About missing wait after releasing reset, shouldn't this be separate
> patch (since currently driver is not doing it)? I can prepare it and
> send with other in next version.
According to the maxtouch documentation, it isn't ready for comms until
the firmware asserts the CHG line. I've seen a bunch of devices that get
by without an explicit wait because the board file does the power on,
and by the time the driver gets to probe it's a few hundred ms later
anyway, so it doesn't matter. But if we put it all in the driver, it
will attempt to read the info block straight after the 100 msec delay
without waiting for CHG, and I suspect we'll end up with occasional
probe failures. It'll depend on the maxtouch device, though: they have a
range of different power on timings.
Which platform are you doing this for? Is it a Chromebook?
> Thanks for feedback
> >
> > The only downside with this approach is that there are a lot of
> > delays during driver probe, but I believe the asynchronous probe stuff
> > that's landed since I wrote the original patch should address that.
> >
> > cheers
> >
> > Nick
> >
> > > }
> > > @@ -3116,6 +3154,10 @@ static int mxt_remove(struct i2c_client *client)
> > > struct mxt_data *data = i2c_get_clientdata(client);
> > >
> > > disable_irq(data->irq);
> > > + if (data->reset_gpio) {
> > > + regulator_disable(data->avdd_reg);
> > > + regulator_disable(data->vdd_reg);
> > > + }
> > > sysfs_remove_group(&client->dev.kobj, &mxt_attr_group);
> > > mxt_free_input_device(data);
> > > mxt_free_object_table(data);
> >
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v2 1/2] Input: atmel_mxt_ts: Add support for optional regulators.
2018-07-19 20:54 ` Nick Dyer
(?)
@ 2018-07-28 13:16 ` Paweł Chmiel
-1 siblings, 0 replies; 19+ messages in thread
From: Paweł Chmiel @ 2018-07-28 13:16 UTC (permalink / raw)
To: Nick Dyer
Cc: dmitry.torokhov, robh+dt, mark.rutland, nicolas.ferre,
alexandre.belloni, linux-input, devicetree, linux-arm-kernel,
linux-kernel
On Thursday, July 19, 2018 9:54:04 PM CEST Nick Dyer wrote:
> On Wed, Jul 18, 2018 at 06:21:30PM +0200, Paweł Chmiel wrote:
> > On Tuesday, July 17, 2018 10:00:05 PM CEST Nick Dyer wrote:
> > > On Tue, Jul 17, 2018 at 08:16:25PM +0200, Paweł Chmiel wrote:
> > > > This patch adds optional regulators, which can be used to power
> > > > up touchscreen. After enabling regulators, we need to wait 150msec.
> > > > This value is taken from official driver.
> > > >
> > > > It was tested on Samsung Galaxy i9000 (based on Samsung S5PV210 SOC).
> > > >
> > > > Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
> > > > ---
> > > > Changes from v1:
> > > > - Enable regulators only if reset_gpio is present.
> > > > - Switch from devm_regulator_get_optional to devm_regulator_get
> > > > ---
> > > > drivers/input/touchscreen/atmel_mxt_ts.c | 46 ++++++++++++++++++++++++++++++--
> > > > 1 file changed, 44 insertions(+), 2 deletions(-)
> > >
> > > Hi Pawel-
> > >
> > > I see you've borrowed some of the logic from the patch I wrote a while
> > > back (see https://github.com/ndyer/linux/commit/8e9687e41ed062 )
> > Actually, i was looking at https://github.com/atmel-maxtouch/linux/blob/maxtouch-v3.14/drivers/input/touchscreen/atmel_mxt_ts.c (and didn't saw Your patch till now).
> > Are You going to submit it? (it has more functionalities - for example
> > suspend mode read from device tree).
>
> Getting that work upstream has stalled for a couple of years because I
> changed jobs. I have actually started recently to dust it off again, it
> was later on in my queue but if you have the time to work on it that is
> great.
>
> > > The correct behaviour according to Atmel should be:
> > >
> > > * Make RESET zero
> > > * Bring up regulators
> > > * Wait for a period to settle (150 msec)
> > > * Release RESET
> > > * Wait for 100 msec whilst device gets through bootloader
> > > * Wait for CHG line assert before reading info block
> > >
> > > I can't see the first and last steps in your patch at present.
> > About first step - reset_gpio is readed by using
> > devm_gpiod_get_optional with GPIOD_OUT_LOW flag, so i think (but might
> > be wrong) that we don't need to set this gpio value again to 0 before
> > enabling regulators,
>
> I see what you mean - that is fair enough.
>
> > since currently only place where reset_gpio is used is in driver probe
> > (in Your patch it is used in other cases/places - for example in
> > mxt_start/stop, when we enable regulators).
> > About missing wait after releasing reset, shouldn't this be separate
> > patch (since currently driver is not doing it)? I can prepare it and
> > send with other in next version.
>
> According to the maxtouch documentation, it isn't ready for comms until
> the firmware asserts the CHG line. I've seen a bunch of devices that get
> by without an explicit wait because the board file does the power on,
> and by the time the driver gets to probe it's a few hundred ms later
> anyway, so it doesn't matter. But if we put it all in the driver, it
> will attempt to read the info block straight after the 100 msec delay
> without waiting for CHG, and I suspect we'll end up with occasional
> probe failures. It'll depend on the maxtouch device, though: they have a
> range of different power on timings.
>
> Which platform are you doing this for? Is it a Chromebook?
No, it's Samsung Galaxy S (i9000) phone with S5PV210 Samsung Soc.
I'm preparing v3 version with separate patch adding this wait/delay.
>
> > Thanks for feedback
> > >
> > > The only downside with this approach is that there are a lot of
> > > delays during driver probe, but I believe the asynchronous probe stuff
> > > that's landed since I wrote the original patch should address that.
> > >
> > > cheers
> > >
> > > Nick
> > >
> > > > }
> > > > @@ -3116,6 +3154,10 @@ static int mxt_remove(struct i2c_client *client)
> > > > struct mxt_data *data = i2c_get_clientdata(client);
> > > >
> > > > disable_irq(data->irq);
> > > > + if (data->reset_gpio) {
> > > > + regulator_disable(data->avdd_reg);
> > > > + regulator_disable(data->vdd_reg);
> > > > + }
> > > > sysfs_remove_group(&client->dev.kobj, &mxt_attr_group);
> > > > mxt_free_input_device(data);
> > > > mxt_free_object_table(data);
> > >
> >
> >
>
^ permalink raw reply [flat|nested] 19+ messages in thread* [PATCH v2 1/2] Input: atmel_mxt_ts: Add support for optional regulators.
@ 2018-07-28 13:16 ` Paweł Chmiel
0 siblings, 0 replies; 19+ messages in thread
From: Paweł Chmiel @ 2018-07-28 13:16 UTC (permalink / raw)
To: linux-arm-kernel
On Thursday, July 19, 2018 9:54:04 PM CEST Nick Dyer wrote:
> On Wed, Jul 18, 2018 at 06:21:30PM +0200, Pawe? Chmiel wrote:
> > On Tuesday, July 17, 2018 10:00:05 PM CEST Nick Dyer wrote:
> > > On Tue, Jul 17, 2018 at 08:16:25PM +0200, Pawe? Chmiel wrote:
> > > > This patch adds optional regulators, which can be used to power
> > > > up touchscreen. After enabling regulators, we need to wait 150msec.
> > > > This value is taken from official driver.
> > > >
> > > > It was tested on Samsung Galaxy i9000 (based on Samsung S5PV210 SOC).
> > > >
> > > > Signed-off-by: Pawe? Chmiel <pawel.mikolaj.chmiel@gmail.com>
> > > > ---
> > > > Changes from v1:
> > > > - Enable regulators only if reset_gpio is present.
> > > > - Switch from devm_regulator_get_optional to devm_regulator_get
> > > > ---
> > > > drivers/input/touchscreen/atmel_mxt_ts.c | 46 ++++++++++++++++++++++++++++++--
> > > > 1 file changed, 44 insertions(+), 2 deletions(-)
> > >
> > > Hi Pawel-
> > >
> > > I see you've borrowed some of the logic from the patch I wrote a while
> > > back (see https://github.com/ndyer/linux/commit/8e9687e41ed062 )
> > Actually, i was looking at https://github.com/atmel-maxtouch/linux/blob/maxtouch-v3.14/drivers/input/touchscreen/atmel_mxt_ts.c (and didn't saw Your patch till now).
> > Are You going to submit it? (it has more functionalities - for example
> > suspend mode read from device tree).
>
> Getting that work upstream has stalled for a couple of years because I
> changed jobs. I have actually started recently to dust it off again, it
> was later on in my queue but if you have the time to work on it that is
> great.
>
> > > The correct behaviour according to Atmel should be:
> > >
> > > * Make RESET zero
> > > * Bring up regulators
> > > * Wait for a period to settle (150 msec)
> > > * Release RESET
> > > * Wait for 100 msec whilst device gets through bootloader
> > > * Wait for CHG line assert before reading info block
> > >
> > > I can't see the first and last steps in your patch at present.
> > About first step - reset_gpio is readed by using
> > devm_gpiod_get_optional with GPIOD_OUT_LOW flag, so i think (but might
> > be wrong) that we don't need to set this gpio value again to 0 before
> > enabling regulators,
>
> I see what you mean - that is fair enough.
>
> > since currently only place where reset_gpio is used is in driver probe
> > (in Your patch it is used in other cases/places - for example in
> > mxt_start/stop, when we enable regulators).
> > About missing wait after releasing reset, shouldn't this be separate
> > patch (since currently driver is not doing it)? I can prepare it and
> > send with other in next version.
>
> According to the maxtouch documentation, it isn't ready for comms until
> the firmware asserts the CHG line. I've seen a bunch of devices that get
> by without an explicit wait because the board file does the power on,
> and by the time the driver gets to probe it's a few hundred ms later
> anyway, so it doesn't matter. But if we put it all in the driver, it
> will attempt to read the info block straight after the 100 msec delay
> without waiting for CHG, and I suspect we'll end up with occasional
> probe failures. It'll depend on the maxtouch device, though: they have a
> range of different power on timings.
>
> Which platform are you doing this for? Is it a Chromebook?
No, it's Samsung Galaxy S (i9000) phone with S5PV210 Samsung Soc.
I'm preparing v3 version with separate patch adding this wait/delay.
>
> > Thanks for feedback
> > >
> > > The only downside with this approach is that there are a lot of
> > > delays during driver probe, but I believe the asynchronous probe stuff
> > > that's landed since I wrote the original patch should address that.
> > >
> > > cheers
> > >
> > > Nick
> > >
> > > > }
> > > > @@ -3116,6 +3154,10 @@ static int mxt_remove(struct i2c_client *client)
> > > > struct mxt_data *data = i2c_get_clientdata(client);
> > > >
> > > > disable_irq(data->irq);
> > > > + if (data->reset_gpio) {
> > > > + regulator_disable(data->avdd_reg);
> > > > + regulator_disable(data->vdd_reg);
> > > > + }
> > > > sysfs_remove_group(&client->dev.kobj, &mxt_attr_group);
> > > > mxt_free_input_device(data);
> > > > mxt_free_object_table(data);
> > >
> >
> >
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v2 1/2] Input: atmel_mxt_ts: Add support for optional regulators.
@ 2018-07-28 13:16 ` Paweł Chmiel
0 siblings, 0 replies; 19+ messages in thread
From: Paweł Chmiel @ 2018-07-28 13:16 UTC (permalink / raw)
To: Nick Dyer
Cc: mark.rutland, devicetree, alexandre.belloni, dmitry.torokhov,
linux-kernel, robh+dt, linux-input, linux-arm-kernel
On Thursday, July 19, 2018 9:54:04 PM CEST Nick Dyer wrote:
> On Wed, Jul 18, 2018 at 06:21:30PM +0200, Paweł Chmiel wrote:
> > On Tuesday, July 17, 2018 10:00:05 PM CEST Nick Dyer wrote:
> > > On Tue, Jul 17, 2018 at 08:16:25PM +0200, Paweł Chmiel wrote:
> > > > This patch adds optional regulators, which can be used to power
> > > > up touchscreen. After enabling regulators, we need to wait 150msec.
> > > > This value is taken from official driver.
> > > >
> > > > It was tested on Samsung Galaxy i9000 (based on Samsung S5PV210 SOC).
> > > >
> > > > Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
> > > > ---
> > > > Changes from v1:
> > > > - Enable regulators only if reset_gpio is present.
> > > > - Switch from devm_regulator_get_optional to devm_regulator_get
> > > > ---
> > > > drivers/input/touchscreen/atmel_mxt_ts.c | 46 ++++++++++++++++++++++++++++++--
> > > > 1 file changed, 44 insertions(+), 2 deletions(-)
> > >
> > > Hi Pawel-
> > >
> > > I see you've borrowed some of the logic from the patch I wrote a while
> > > back (see https://github.com/ndyer/linux/commit/8e9687e41ed062 )
> > Actually, i was looking at https://github.com/atmel-maxtouch/linux/blob/maxtouch-v3.14/drivers/input/touchscreen/atmel_mxt_ts.c (and didn't saw Your patch till now).
> > Are You going to submit it? (it has more functionalities - for example
> > suspend mode read from device tree).
>
> Getting that work upstream has stalled for a couple of years because I
> changed jobs. I have actually started recently to dust it off again, it
> was later on in my queue but if you have the time to work on it that is
> great.
>
> > > The correct behaviour according to Atmel should be:
> > >
> > > * Make RESET zero
> > > * Bring up regulators
> > > * Wait for a period to settle (150 msec)
> > > * Release RESET
> > > * Wait for 100 msec whilst device gets through bootloader
> > > * Wait for CHG line assert before reading info block
> > >
> > > I can't see the first and last steps in your patch at present.
> > About first step - reset_gpio is readed by using
> > devm_gpiod_get_optional with GPIOD_OUT_LOW flag, so i think (but might
> > be wrong) that we don't need to set this gpio value again to 0 before
> > enabling regulators,
>
> I see what you mean - that is fair enough.
>
> > since currently only place where reset_gpio is used is in driver probe
> > (in Your patch it is used in other cases/places - for example in
> > mxt_start/stop, when we enable regulators).
> > About missing wait after releasing reset, shouldn't this be separate
> > patch (since currently driver is not doing it)? I can prepare it and
> > send with other in next version.
>
> According to the maxtouch documentation, it isn't ready for comms until
> the firmware asserts the CHG line. I've seen a bunch of devices that get
> by without an explicit wait because the board file does the power on,
> and by the time the driver gets to probe it's a few hundred ms later
> anyway, so it doesn't matter. But if we put it all in the driver, it
> will attempt to read the info block straight after the 100 msec delay
> without waiting for CHG, and I suspect we'll end up with occasional
> probe failures. It'll depend on the maxtouch device, though: they have a
> range of different power on timings.
>
> Which platform are you doing this for? Is it a Chromebook?
No, it's Samsung Galaxy S (i9000) phone with S5PV210 Samsung Soc.
I'm preparing v3 version with separate patch adding this wait/delay.
>
> > Thanks for feedback
> > >
> > > The only downside with this approach is that there are a lot of
> > > delays during driver probe, but I believe the asynchronous probe stuff
> > > that's landed since I wrote the original patch should address that.
> > >
> > > cheers
> > >
> > > Nick
> > >
> > > > }
> > > > @@ -3116,6 +3154,10 @@ static int mxt_remove(struct i2c_client *client)
> > > > struct mxt_data *data = i2c_get_clientdata(client);
> > > >
> > > > disable_irq(data->irq);
> > > > + if (data->reset_gpio) {
> > > > + regulator_disable(data->avdd_reg);
> > > > + regulator_disable(data->vdd_reg);
> > > > + }
> > > > sysfs_remove_group(&client->dev.kobj, &mxt_attr_group);
> > > > mxt_free_input_device(data);
> > > > mxt_free_object_table(data);
> > >
> >
> >
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 1/2] Input: atmel_mxt_ts: Add support for optional regulators.
2018-07-17 21:00 ` Nick Dyer
@ 2018-08-22 22:53 ` George G. Davis
-1 siblings, 0 replies; 19+ messages in thread
From: George G. Davis @ 2018-08-22 22:53 UTC (permalink / raw)
To: Nick Dyer
Cc: Paweł Chmiel, mark.rutland, devicetree, alexandre.belloni,
dmitry.torokhov, linux-kernel, robh+dt, linux-input,
linux-arm-kernel
Hello Nick,
On Tue, Jul 17, 2018 at 10:00:05PM +0100, Nick Dyer wrote:
> On Tue, Jul 17, 2018 at 08:16:25PM +0200, Paweł Chmiel wrote:
> > This patch adds optional regulators, which can be used to power
> > up touchscreen. After enabling regulators, we need to wait 150msec.
> > This value is taken from official driver.
> >
> > It was tested on Samsung Galaxy i9000 (based on Samsung S5PV210 SOC).
> >
> > Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
> > ---
> > Changes from v1:
> > - Enable regulators only if reset_gpio is present.
> > - Switch from devm_regulator_get_optional to devm_regulator_get
> > ---
> > drivers/input/touchscreen/atmel_mxt_ts.c | 46 ++++++++++++++++++++++++++++++--
> > 1 file changed, 44 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> > index 54fe190fd4bc..005f0fee9fc8 100644
> > --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> > +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> > @@ -27,6 +27,7 @@
> > #include <linux/interrupt.h>
> > #include <linux/of.h>
> > #include <linux/property.h>
> > +#include <linux/regulator/consumer.h>
> > #include <linux/slab.h>
> > #include <linux/gpio/consumer.h>
> > #include <linux/property.h>
> > @@ -194,10 +195,10 @@ enum t100_type {
> >
> > /* Delay times */
> > #define MXT_BACKUP_TIME 50 /* msec */
> > -#define MXT_RESET_GPIO_TIME 20 /* msec */
> > #define MXT_RESET_INVALID_CHG 100 /* msec */
> > #define MXT_RESET_TIME 200 /* msec */
> > #define MXT_RESET_TIMEOUT 3000 /* msec */
> > +#define MXT_REGULATOR_DELAY 150 /* msec */
> > #define MXT_CRC_TIMEOUT 1000 /* msec */
> > #define MXT_FW_RESET_TIME 3000 /* msec */
> > #define MXT_FW_CHG_TIMEOUT 300 /* msec */
> > @@ -310,6 +311,8 @@ struct mxt_data {
> > struct t7_config t7_cfg;
> > struct mxt_dbg dbg;
> > struct gpio_desc *reset_gpio;
> > + struct regulator *vdd_reg;
> > + struct regulator *avdd_reg;
> >
> > /* Cached parameters from object table */
> > u16 T5_address;
> > @@ -3076,6 +3079,22 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id)
> > return error;
> > }
> >
> > + data->vdd_reg = devm_regulator_get(&client->dev, "vdd");
> > + if (IS_ERR(data->vdd_reg)) {
> > + error = PTR_ERR(data->vdd_reg);
> > + dev_err(&client->dev, "Failed to get vdd regulator: %d\n",
> > + error);
> > + return error;
> > + }
> > +
> > + data->avdd_reg = devm_regulator_get(&client->dev, "avdd");
> > + if (IS_ERR(data->avdd_reg)) {
> > + error = PTR_ERR(data->avdd_reg);
> > + dev_err(&client->dev, "Failed to get avdd regulator: %d\n",
> > + error);
> > + return error;
> > + }
> > +
> > error = devm_request_threaded_irq(&client->dev, client->irq,
> > NULL, mxt_interrupt, IRQF_ONESHOT,
> > client->name, data);
> > @@ -3087,7 +3106,26 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id)
> > disable_irq(client->irq);
> >
> > if (data->reset_gpio) {
> > - msleep(MXT_RESET_GPIO_TIME);
> > + error = regulator_enable(data->vdd_reg);
> > + if (error) {
> > + dev_err(&client->dev, "Failed to enable vdd regulator: %d\n",
> > + error);
> > + return error;
> > + }
> > +
> > + error = regulator_enable(data->avdd_reg);
> > + if (error) {
> > + dev_err(&client->dev, "Failed to enable avdd regulator: %d\n",
> > + error);
> > + return error;
> > + }
> > +
> > + /*
> > + * According to maXTouch power sequencing specification, RESET line
> > + * must be kept low until some time after regulators come up to
> > + * voltage
> > + */
> > + msleep(MXT_REGULATOR_DELAY);
> > gpiod_set_value(data->reset_gpio, 1);
> > msleep(MXT_RESET_INVALID_CHG);
>
> Hi Pawel-
>
> I see you've borrowed some of the logic from the patch I wrote a while
> back (see https://github.com/ndyer/linux/commit/8e9687e41ed062 )
I'm actually quite interested in your work and notice that it has
languished for some time now. If you have rebased any of your earlier
work to more recent kernel versions, I'd be happy to assist with testing
and review.
--
Regards,
George
> The correct behaviour according to Atmel should be:
>
> * Make RESET zero
> * Bring up regulators
> * Wait for a period to settle (150 msec)
> * Release RESET
> * Wait for 100 msec whilst device gets through bootloader
> * Wait for CHG line assert before reading info block
>
> I can't see the first and last steps in your patch at present.
>
> The only downside with this approach is that there are a lot of
> delays during driver probe, but I believe the asynchronous probe stuff
> that's landed since I wrote the original patch should address that.
>
> cheers
>
> Nick
>
> > }
> > @@ -3116,6 +3154,10 @@ static int mxt_remove(struct i2c_client *client)
> > struct mxt_data *data = i2c_get_clientdata(client);
> >
> > disable_irq(data->irq);
> > + if (data->reset_gpio) {
> > + regulator_disable(data->avdd_reg);
> > + regulator_disable(data->vdd_reg);
> > + }
> > sysfs_remove_group(&client->dev.kobj, &mxt_attr_group);
> > mxt_free_input_device(data);
> > mxt_free_object_table(data);
> > --
> > 2.7.4
> >
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 19+ messages in thread* [PATCH v2 1/2] Input: atmel_mxt_ts: Add support for optional regulators.
@ 2018-08-22 22:53 ` George G. Davis
0 siblings, 0 replies; 19+ messages in thread
From: George G. Davis @ 2018-08-22 22:53 UTC (permalink / raw)
To: linux-arm-kernel
Hello Nick,
On Tue, Jul 17, 2018 at 10:00:05PM +0100, Nick Dyer wrote:
> On Tue, Jul 17, 2018 at 08:16:25PM +0200, Pawe? Chmiel wrote:
> > This patch adds optional regulators, which can be used to power
> > up touchscreen. After enabling regulators, we need to wait 150msec.
> > This value is taken from official driver.
> >
> > It was tested on Samsung Galaxy i9000 (based on Samsung S5PV210 SOC).
> >
> > Signed-off-by: Pawe? Chmiel <pawel.mikolaj.chmiel@gmail.com>
> > ---
> > Changes from v1:
> > - Enable regulators only if reset_gpio is present.
> > - Switch from devm_regulator_get_optional to devm_regulator_get
> > ---
> > drivers/input/touchscreen/atmel_mxt_ts.c | 46 ++++++++++++++++++++++++++++++--
> > 1 file changed, 44 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> > index 54fe190fd4bc..005f0fee9fc8 100644
> > --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> > +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> > @@ -27,6 +27,7 @@
> > #include <linux/interrupt.h>
> > #include <linux/of.h>
> > #include <linux/property.h>
> > +#include <linux/regulator/consumer.h>
> > #include <linux/slab.h>
> > #include <linux/gpio/consumer.h>
> > #include <linux/property.h>
> > @@ -194,10 +195,10 @@ enum t100_type {
> >
> > /* Delay times */
> > #define MXT_BACKUP_TIME 50 /* msec */
> > -#define MXT_RESET_GPIO_TIME 20 /* msec */
> > #define MXT_RESET_INVALID_CHG 100 /* msec */
> > #define MXT_RESET_TIME 200 /* msec */
> > #define MXT_RESET_TIMEOUT 3000 /* msec */
> > +#define MXT_REGULATOR_DELAY 150 /* msec */
> > #define MXT_CRC_TIMEOUT 1000 /* msec */
> > #define MXT_FW_RESET_TIME 3000 /* msec */
> > #define MXT_FW_CHG_TIMEOUT 300 /* msec */
> > @@ -310,6 +311,8 @@ struct mxt_data {
> > struct t7_config t7_cfg;
> > struct mxt_dbg dbg;
> > struct gpio_desc *reset_gpio;
> > + struct regulator *vdd_reg;
> > + struct regulator *avdd_reg;
> >
> > /* Cached parameters from object table */
> > u16 T5_address;
> > @@ -3076,6 +3079,22 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id)
> > return error;
> > }
> >
> > + data->vdd_reg = devm_regulator_get(&client->dev, "vdd");
> > + if (IS_ERR(data->vdd_reg)) {
> > + error = PTR_ERR(data->vdd_reg);
> > + dev_err(&client->dev, "Failed to get vdd regulator: %d\n",
> > + error);
> > + return error;
> > + }
> > +
> > + data->avdd_reg = devm_regulator_get(&client->dev, "avdd");
> > + if (IS_ERR(data->avdd_reg)) {
> > + error = PTR_ERR(data->avdd_reg);
> > + dev_err(&client->dev, "Failed to get avdd regulator: %d\n",
> > + error);
> > + return error;
> > + }
> > +
> > error = devm_request_threaded_irq(&client->dev, client->irq,
> > NULL, mxt_interrupt, IRQF_ONESHOT,
> > client->name, data);
> > @@ -3087,7 +3106,26 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id)
> > disable_irq(client->irq);
> >
> > if (data->reset_gpio) {
> > - msleep(MXT_RESET_GPIO_TIME);
> > + error = regulator_enable(data->vdd_reg);
> > + if (error) {
> > + dev_err(&client->dev, "Failed to enable vdd regulator: %d\n",
> > + error);
> > + return error;
> > + }
> > +
> > + error = regulator_enable(data->avdd_reg);
> > + if (error) {
> > + dev_err(&client->dev, "Failed to enable avdd regulator: %d\n",
> > + error);
> > + return error;
> > + }
> > +
> > + /*
> > + * According to maXTouch power sequencing specification, RESET line
> > + * must be kept low until some time after regulators come up to
> > + * voltage
> > + */
> > + msleep(MXT_REGULATOR_DELAY);
> > gpiod_set_value(data->reset_gpio, 1);
> > msleep(MXT_RESET_INVALID_CHG);
>
> Hi Pawel-
>
> I see you've borrowed some of the logic from the patch I wrote a while
> back (see https://github.com/ndyer/linux/commit/8e9687e41ed062 )
I'm actually quite interested in your work and notice that it has
languished for some time now. If you have rebased any of your earlier
work to more recent kernel versions, I'd be happy to assist with testing
and review.
--
Regards,
George
> The correct behaviour according to Atmel should be:
>
> * Make RESET zero
> * Bring up regulators
> * Wait for a period to settle (150 msec)
> * Release RESET
> * Wait for 100 msec whilst device gets through bootloader
> * Wait for CHG line assert before reading info block
>
> I can't see the first and last steps in your patch at present.
>
> The only downside with this approach is that there are a lot of
> delays during driver probe, but I believe the asynchronous probe stuff
> that's landed since I wrote the original patch should address that.
>
> cheers
>
> Nick
>
> > }
> > @@ -3116,6 +3154,10 @@ static int mxt_remove(struct i2c_client *client)
> > struct mxt_data *data = i2c_get_clientdata(client);
> >
> > disable_irq(data->irq);
> > + if (data->reset_gpio) {
> > + regulator_disable(data->avdd_reg);
> > + regulator_disable(data->vdd_reg);
> > + }
> > sysfs_remove_group(&client->dev.kobj, &mxt_attr_group);
> > mxt_free_input_device(data);
> > mxt_free_object_table(data);
> > --
> > 2.7.4
> >
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 19+ messages in thread