[PATCH 0/2] Input: atmel_mxt_ts: Add support for optional regulators

[PATCH 0/2] Input: atmel_mxt_ts: Add support for optional regulators

[Paweł Chmiel]

This two patches add optional regulator support to atmel_mxt_ts.
First patch adds regulators to driver.
Second patch updates documentation.

Paweł Chmiel (2):
  Input: atmel_mxt_ts: Add support for  optional regulators.
  Input: atmel_mxt_ts: Document optional voltage regulators

 .../devicetree/bindings/input/atmel,maxtouch.txt   |  8 ++++
 drivers/input/touchscreen/atmel_mxt_ts.c           | 45 ++++++++++++++++++++++
 2 files changed, 53 insertions(+)

-- 
2.7.4

[PATCH 1/2] Input: atmel_mxt_ts: Add support for optional regulators.

[Paweł Chmiel]

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>
---
 drivers/input/touchscreen/atmel_mxt_ts.c | 45 ++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 54fe190fd4bc..a7625ec8fb9f 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>
@@ -198,6 +199,7 @@ enum t100_type {
 #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 +312,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 +3080,40 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id)
 		return error;
 	}
 
+	data->vdd_reg = devm_regulator_get_optional(&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;
+	}
+
+	if (data->vdd_reg) {
+		error = regulator_enable(data->vdd_reg);
+		if (error) {
+			dev_err(&client->dev, "Failed to enable vdd regulator: %d\n",
+				error);
+			return error;
+		}
+	}
+
+	data->avdd_reg = devm_regulator_get_optional(&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;
+	}
+
+	if (data->avdd_reg) {
+		error = regulator_enable(data->avdd_reg);
+		if (error) {
+			dev_err(&client->dev, "Failed to enable avdd regulator: %d\n",
+				error);
+			return error;
+		}
+	}
+
 	error = devm_request_threaded_irq(&client->dev, client->irq,
 					  NULL, mxt_interrupt, IRQF_ONESHOT,
 					  client->name, data);
@@ -3086,6 +3124,9 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id)
 
 	disable_irq(client->irq);
 
+	if (!IS_ERR(data->vdd_reg) || !IS_ERR(data->avdd_reg))
+		msleep(MXT_REGULATOR_DELAY);
+
 	if (data->reset_gpio) {
 		msleep(MXT_RESET_GPIO_TIME);
 		gpiod_set_value(data->reset_gpio, 1);
@@ -3116,6 +3157,10 @@ static int mxt_remove(struct i2c_client *client)
 	struct mxt_data *data = i2c_get_clientdata(client);
 
 	disable_irq(data->irq);
+	if (!IS_ERR(data->avdd_reg))
+		regulator_disable(data->avdd_reg);
+	if (!IS_ERR(data->vdd_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

[PATCH 2/2] Input: atmel_mxt_ts: Document optional voltage regulators

[Paweł Chmiel]

Document new optional voltage regulators, which can be used
to power down/up touchscreen.

Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
---
 Documentation/devicetree/bindings/input/atmel,maxtouch.txt | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/input/atmel,maxtouch.txt b/Documentation/devicetree/bindings/input/atmel,maxtouch.txt
index c88919480d37..17930ecadad3 100644
--- a/Documentation/devicetree/bindings/input/atmel,maxtouch.txt
+++ b/Documentation/devicetree/bindings/input/atmel,maxtouch.txt
@@ -31,6 +31,12 @@ Optional properties for main touchpad device:
 
 - reset-gpios: GPIO specifier for the touchscreen's reset pin (active low)
 
+- avdd-supply: Analog power supply. It powers up the analog channel block
+    of the controller to detect the touches.
+
+- vdd-supply: Digital power supply. It powers up the digital block
+    of the controller to enable i2c communication.
+
 Example:
 
 	touch@4b {
@@ -38,4 +44,6 @@ Example:
 		reg = <0x4b>;
 		interrupt-parent = <&gpio>;
 		interrupts = <TEGRA_GPIO(W, 3) IRQ_TYPE_LEVEL_LOW>;
+		avdd-supply = <&atsp_reg>;
+		vdd-supply = <&tsp_reg>;
 	};
-- 
2.7.4

Re: [PATCH 1/2] Input: atmel_mxt_ts: Add support for optional regulators.

[Dmitry Torokhov]

Hi Paweł,

On Fri, Jul 13, 2018 at 08:30:07PM +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>
> ---
>  drivers/input/touchscreen/atmel_mxt_ts.c | 45 ++++++++++++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
> 
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index 54fe190fd4bc..a7625ec8fb9f 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>
> @@ -198,6 +199,7 @@ enum t100_type {
>  #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 +312,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 +3080,40 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  		return error;
>  	}
>  
> +	data->vdd_reg = devm_regulator_get_optional(&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;
> +	}
> +
> +	if (data->vdd_reg) {
> +		error = regulator_enable(data->vdd_reg);
> +		if (error) {
> +			dev_err(&client->dev, "Failed to enable vdd regulator: %d\n",
> +				error);
> +			return error;
> +		}
> +	}
> +
> +	data->avdd_reg = devm_regulator_get_optional(&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;
> +	}
> +
> +	if (data->avdd_reg) {

devm_regulator_get_optional() does not return NULL for regulators
not present in device tree, but rather -ENOENT, so this code is not
correct; neither is the simple IS_ERR() check above.

Moreover, the optional regulators should be used when there is a part of
IP block that can be optionally powered up, but the device can work
(with limited functionality) even when it is powered down. They should
not be used in cases when supplies are mandatory (such as vdd/avdd) but
may not be exposed to the kernel by the firmware.

Simply use devm_regulator_get() and rely on the fact that on fully
constrained system you will get a dummy regulator.

> +		error = regulator_enable(data->avdd_reg);
> +		if (error) {
> +			dev_err(&client->dev, "Failed to enable avdd regulator: %d\n",
> +				error);
> +			return error;
> +		}
> +	}
> +
>  	error = devm_request_threaded_irq(&client->dev, client->irq,
>  					  NULL, mxt_interrupt, IRQF_ONESHOT,
>  					  client->name, data);
> @@ -3086,6 +3124,9 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  
>  	disable_irq(client->irq);
>  
> +	if (!IS_ERR(data->vdd_reg) || !IS_ERR(data->avdd_reg))
> +		msleep(MXT_REGULATOR_DELAY);
> +
>  	if (data->reset_gpio) {

I think you should require that if regulators are exposed then reset
gpio should also be present to ensure proper power up timings.

>  		msleep(MXT_RESET_GPIO_TIME);
>  		gpiod_set_value(data->reset_gpio, 1);
> @@ -3116,6 +3157,10 @@ static int mxt_remove(struct i2c_client *client)
>  	struct mxt_data *data = i2c_get_clientdata(client);
>  
>  	disable_irq(data->irq);
> +	if (!IS_ERR(data->avdd_reg))
> +		regulator_disable(data->avdd_reg);
> +	if (!IS_ERR(data->vdd_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
> 

Thanks.

-- 
Dmitry

Re: [PATCH 1/2] Input: atmel_mxt_ts: Add support for optional regulators.

[Paweł Chmiel]

On Sunday, July 15, 2018 10:25:47 AM CEST Dmitry Torokhov wrote:
> Hi Paweł,
> 
> On Fri, Jul 13, 2018 at 08:30:07PM +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>
> > ---
> >  drivers/input/touchscreen/atmel_mxt_ts.c | 45 ++++++++++++++++++++++++++++++++
> >  1 file changed, 45 insertions(+)
> > 
> > diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> > index 54fe190fd4bc..a7625ec8fb9f 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>
> > @@ -198,6 +199,7 @@ enum t100_type {
> >  #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 +312,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 +3080,40 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id)
> >  		return error;
> >  	}
> >  
> > +	data->vdd_reg = devm_regulator_get_optional(&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;
> > +	}
> > +
> > +	if (data->vdd_reg) {
> > +		error = regulator_enable(data->vdd_reg);
> > +		if (error) {
> > +			dev_err(&client->dev, "Failed to enable vdd regulator: %d\n",
> > +				error);
> > +			return error;
> > +		}
> > +	}
> > +
> > +	data->avdd_reg = devm_regulator_get_optional(&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;
> > +	}
> > +
> > +	if (data->avdd_reg) {
> 
> devm_regulator_get_optional() does not return NULL for regulators
> not present in device tree, but rather -ENOENT, so this code is not
> correct; neither is the simple IS_ERR() check above.
> 
> Moreover, the optional regulators should be used when there is a part of
> IP block that can be optionally powered up, but the device can work
> (with limited functionality) even when it is powered down. They should
> not be used in cases when supplies are mandatory (such as vdd/avdd) but
> may not be exposed to the kernel by the firmware.
> 
> Simply use devm_regulator_get() and rely on the fact that on fully
> constrained system you will get a dummy regulator.
> 
> > +		error = regulator_enable(data->avdd_reg);
> > +		if (error) {
> > +			dev_err(&client->dev, "Failed to enable avdd regulator: %d\n",
> > +				error);
> > +			return error;
> > +		}
> > +	}
> > +
> >  	error = devm_request_threaded_irq(&client->dev, client->irq,
> >  					  NULL, mxt_interrupt, IRQF_ONESHOT,
> >  					  client->name, data);
> > @@ -3086,6 +3124,9 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id)
> >  
> >  	disable_irq(client->irq);
> >  
> > +	if (!IS_ERR(data->vdd_reg) || !IS_ERR(data->avdd_reg))
> > +		msleep(MXT_REGULATOR_DELAY);
> > +
> >  	if (data->reset_gpio) {
> 
> I think you should require that if regulators are exposed then reset
> gpio should also be present to ensure proper power up timings.

Thanks for comments. I'll prepare v2 version with those changes applied.
> 
> >  		msleep(MXT_RESET_GPIO_TIME);
> >  		gpiod_set_value(data->reset_gpio, 1);
> > @@ -3116,6 +3157,10 @@ static int mxt_remove(struct i2c_client *client)
> >  	struct mxt_data *data = i2c_get_clientdata(client);
> >  
> >  	disable_irq(data->irq);
> > +	if (!IS_ERR(data->avdd_reg))
> > +		regulator_disable(data->avdd_reg);
> > +	if (!IS_ERR(data->vdd_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);
> 
> Thanks.
> 
>