[PATCH v2] input: touchscreen: silead: Add regulator support

[PATCH v2] input: touchscreen: silead: Add regulator support

[Hans de Goede]

On some tablets the touchscreen controller is powered by separate
regulators, add support for this.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Rob Herring <robh@kernel.org>
---
Changes in v2:
-Use devm_regulator_bulk_get() and friends
-Use devm_add_action_or_reset() to disable the regulator
---
 .../bindings/input/touchscreen/silead_gsl1680.txt  |  2 ++
 drivers/input/touchscreen/silead.c                 | 29 ++++++++++++++++++++++
 2 files changed, 31 insertions(+)

diff --git a/Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt b/Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt
index e844c3f..b726823 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt
+++ b/Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt
@@ -22,6 +22,8 @@ Optional properties:
 - touchscreen-inverted-y  : See touchscreen.txt
 - touchscreen-swapped-x-y : See touchscreen.txt
 - silead,max-fingers	  : maximum number of fingers the touchscreen can detect
+- vddio-supply		  : regulator phandle for controller VDDIO
+- avdd-supply		  : regulator phandle for controller AVDD
 
 Example:
 
diff --git a/drivers/input/touchscreen/silead.c b/drivers/input/touchscreen/silead.c
index f502c84..404830a 100644
--- a/drivers/input/touchscreen/silead.c
+++ b/drivers/input/touchscreen/silead.c
@@ -29,6 +29,7 @@
 #include <linux/input/touchscreen.h>
 #include <linux/pm.h>
 #include <linux/irq.h>
+#include <linux/regulator/consumer.h>
 
 #include <asm/unaligned.h>
 
@@ -73,6 +74,7 @@ struct silead_ts_data {
 	struct i2c_client *client;
 	struct gpio_desc *gpio_power;
 	struct input_dev *input;
+	struct regulator_bulk_data regulators[2];
 	char fw_name[64];
 	struct touchscreen_properties prop;
 	u32 max_fingers;
@@ -433,6 +435,13 @@ static int silead_ts_set_default_fw_name(struct silead_ts_data *data,
 }
 #endif
 
+static void silead_disable_regulator(void *arg)
+{
+	struct silead_ts_data *data = arg;
+
+	regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators);
+}
+
 static int silead_ts_probe(struct i2c_client *client,
 			   const struct i2c_device_id *id)
 {
@@ -465,6 +474,26 @@ static int silead_ts_probe(struct i2c_client *client,
 	if (client->irq <= 0)
 		return -ENODEV;
 
+	data->regulators[0].supply = "vddio";
+	data->regulators[1].supply = "avdd";
+	error = devm_regulator_bulk_get(dev, ARRAY_SIZE(data->regulators),
+					data->regulators);
+	if (error)
+		return error;
+
+	/*
+	 * Enable regulators at probe and disable them at remove, we need
+	 * to keep the chip powered otherwise it forgets its firmware.
+	 */
+	error = regulator_bulk_enable(ARRAY_SIZE(data->regulators),
+				      data->regulators);
+	if (error)
+		return error;
+
+	error = devm_add_action_or_reset(dev, silead_disable_regulator, data);
+	if (error)
+		return error;
+
 	/* Power GPIO pin */
 	data->gpio_power = devm_gpiod_get_optional(dev, "power", GPIOD_OUT_LOW);
 	if (IS_ERR(data->gpio_power)) {
-- 
2.9.3

Re: [PATCH v2] input: touchscreen: silead: Add regulator support

[Dmitry Torokhov]

On Wed, Nov 16, 2016 at 12:55:07PM +0100, Hans de Goede wrote:
> On some tablets the touchscreen controller is powered by separate
> regulators, add support for this.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Acked-by: Rob Herring <robh@kernel.org>
> ---
> Changes in v2:
> -Use devm_regulator_bulk_get() and friends
> -Use devm_add_action_or_reset() to disable the regulator
> ---
>  .../bindings/input/touchscreen/silead_gsl1680.txt  |  2 ++
>  drivers/input/touchscreen/silead.c                 | 29 ++++++++++++++++++++++
>  2 files changed, 31 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt b/Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt
> index e844c3f..b726823 100644
> --- a/Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt
> +++ b/Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt
> @@ -22,6 +22,8 @@ Optional properties:
>  - touchscreen-inverted-y  : See touchscreen.txt
>  - touchscreen-swapped-x-y : See touchscreen.txt
>  - silead,max-fingers	  : maximum number of fingers the touchscreen can detect
> +- vddio-supply		  : regulator phandle for controller VDDIO
> +- avdd-supply		  : regulator phandle for controller AVDD
>  
>  Example:
>  
> diff --git a/drivers/input/touchscreen/silead.c b/drivers/input/touchscreen/silead.c
> index f502c84..404830a 100644
> --- a/drivers/input/touchscreen/silead.c
> +++ b/drivers/input/touchscreen/silead.c
> @@ -29,6 +29,7 @@
>  #include <linux/input/touchscreen.h>
>  #include <linux/pm.h>
>  #include <linux/irq.h>
> +#include <linux/regulator/consumer.h>
>  
>  #include <asm/unaligned.h>
>  
> @@ -73,6 +74,7 @@ struct silead_ts_data {
>  	struct i2c_client *client;
>  	struct gpio_desc *gpio_power;
>  	struct input_dev *input;
> +	struct regulator_bulk_data regulators[2];
>  	char fw_name[64];
>  	struct touchscreen_properties prop;
>  	u32 max_fingers;
> @@ -433,6 +435,13 @@ static int silead_ts_set_default_fw_name(struct silead_ts_data *data,
>  }
>  #endif
>  
> +static void silead_disable_regulator(void *arg)
> +{
> +	struct silead_ts_data *data = arg;
> +
> +	regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators);
> +}
> +
>  static int silead_ts_probe(struct i2c_client *client,
>  			   const struct i2c_device_id *id)
>  {
> @@ -465,6 +474,26 @@ static int silead_ts_probe(struct i2c_client *client,
>  	if (client->irq <= 0)
>  		return -ENODEV;
>  
> +	data->regulators[0].supply = "vddio";
> +	data->regulators[1].supply = "avdd";
> +	error = devm_regulator_bulk_get(dev, ARRAY_SIZE(data->regulators),
> +					data->regulators);
> +	if (error)
> +		return error;
> +
> +	/*
> +	 * Enable regulators at probe and disable them at remove, we need
> +	 * to keep the chip powered otherwise it forgets its firmware.
> +	 */

Hmm, this burns power though. Why can't we reload firmware on resume (it
should be already cached)? Does it take too long?

Thanks.

> +	error = regulator_bulk_enable(ARRAY_SIZE(data->regulators),
> +				      data->regulators);
> +	if (error)
> +		return error;
> +
> +	error = devm_add_action_or_reset(dev, silead_disable_regulator, data);
> +	if (error)
> +		return error;
> +
>  	/* Power GPIO pin */
>  	data->gpio_power = devm_gpiod_get_optional(dev, "power", GPIOD_OUT_LOW);
>  	if (IS_ERR(data->gpio_power)) {
> -- 
> 2.9.3
> 

-- 
Dmitry

Re: [PATCH v2] input: touchscreen: silead: Add regulator support

[Hans de Goede]

HI,

On 16-11-16 18:51, Dmitry Torokhov wrote:
> On Wed, Nov 16, 2016 at 12:55:07PM +0100, Hans de Goede wrote:
>> On some tablets the touchscreen controller is powered by separate
>> regulators, add support for this.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> Acked-by: Rob Herring <robh@kernel.org>
>> ---
>> Changes in v2:
>> -Use devm_regulator_bulk_get() and friends
>> -Use devm_add_action_or_reset() to disable the regulator
>> ---
>>  .../bindings/input/touchscreen/silead_gsl1680.txt  |  2 ++
>>  drivers/input/touchscreen/silead.c                 | 29 ++++++++++++++++++++++
>>  2 files changed, 31 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt b/Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt
>> index e844c3f..b726823 100644
>> --- a/Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt
>> +++ b/Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt
>> @@ -22,6 +22,8 @@ Optional properties:
>>  - touchscreen-inverted-y  : See touchscreen.txt
>>  - touchscreen-swapped-x-y : See touchscreen.txt
>>  - silead,max-fingers	  : maximum number of fingers the touchscreen can detect
>> +- vddio-supply		  : regulator phandle for controller VDDIO
>> +- avdd-supply		  : regulator phandle for controller AVDD
>>
>>  Example:
>>
>> diff --git a/drivers/input/touchscreen/silead.c b/drivers/input/touchscreen/silead.c
>> index f502c84..404830a 100644
>> --- a/drivers/input/touchscreen/silead.c
>> +++ b/drivers/input/touchscreen/silead.c
>> @@ -29,6 +29,7 @@
>>  #include <linux/input/touchscreen.h>
>>  #include <linux/pm.h>
>>  #include <linux/irq.h>
>> +#include <linux/regulator/consumer.h>
>>
>>  #include <asm/unaligned.h>
>>
>> @@ -73,6 +74,7 @@ struct silead_ts_data {
>>  	struct i2c_client *client;
>>  	struct gpio_desc *gpio_power;
>>  	struct input_dev *input;
>> +	struct regulator_bulk_data regulators[2];
>>  	char fw_name[64];
>>  	struct touchscreen_properties prop;
>>  	u32 max_fingers;
>> @@ -433,6 +435,13 @@ static int silead_ts_set_default_fw_name(struct silead_ts_data *data,
>>  }
>>  #endif
>>
>> +static void silead_disable_regulator(void *arg)
>> +{
>> +	struct silead_ts_data *data = arg;
>> +
>> +	regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators);
>> +}
>> +
>>  static int silead_ts_probe(struct i2c_client *client,
>>  			   const struct i2c_device_id *id)
>>  {
>> @@ -465,6 +474,26 @@ static int silead_ts_probe(struct i2c_client *client,
>>  	if (client->irq <= 0)
>>  		return -ENODEV;
>>
>> +	data->regulators[0].supply = "vddio";
>> +	data->regulators[1].supply = "avdd";
>> +	error = devm_regulator_bulk_get(dev, ARRAY_SIZE(data->regulators),
>> +					data->regulators);
>> +	if (error)
>> +		return error;
>> +
>> +	/*
>> +	 * Enable regulators at probe and disable them at remove, we need
>> +	 * to keep the chip powered otherwise it forgets its firmware.
>> +	 */
>
> Hmm, this burns power though. Why can't we reload firmware on resume (it
> should be already cached)?

We already put the device in low-power mode using the power pin. Of the
20 or so different tablets I've with this touchscreen controller only
2 actually have a separate regulator for the controller, so I do not
believe that powering down the regulator will be a big win, otherwise
all tablets would have had this.

 > Does it take too long?

It is a couple of kB written one 32-bit word at a time over i2c, so
it's not fast.

Regards,

Hans


>
> Thanks.
>
>> +	error = regulator_bulk_enable(ARRAY_SIZE(data->regulators),
>> +				      data->regulators);
>> +	if (error)
>> +		return error;
>> +
>> +	error = devm_add_action_or_reset(dev, silead_disable_regulator, data);
>> +	if (error)
>> +		return error;
>> +
>>  	/* Power GPIO pin */
>>  	data->gpio_power = devm_gpiod_get_optional(dev, "power", GPIOD_OUT_LOW);
>>  	if (IS_ERR(data->gpio_power)) {
>> --
>> 2.9.3
>>
>

Re: [PATCH v2] input: touchscreen: silead: Add regulator support

[Dmitry Torokhov]

On Wed, Nov 16, 2016 at 07:58:26PM +0100, Hans de Goede wrote:
> HI,
> 
> On 16-11-16 18:51, Dmitry Torokhov wrote:
> >On Wed, Nov 16, 2016 at 12:55:07PM +0100, Hans de Goede wrote:
> >>On some tablets the touchscreen controller is powered by separate
> >>regulators, add support for this.
> >>
> >>Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> >>Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> >>---
> >>Changes in v2:
> >>-Use devm_regulator_bulk_get() and friends
> >>-Use devm_add_action_or_reset() to disable the regulator
> >>---
> >> .../bindings/input/touchscreen/silead_gsl1680.txt  |  2 ++
> >> drivers/input/touchscreen/silead.c                 | 29 ++++++++++++++++++++++
> >> 2 files changed, 31 insertions(+)
> >>
> >>diff --git a/Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt b/Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt
> >>index e844c3f..b726823 100644
> >>--- a/Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt
> >>+++ b/Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt
> >>@@ -22,6 +22,8 @@ Optional properties:
> >> - touchscreen-inverted-y  : See touchscreen.txt
> >> - touchscreen-swapped-x-y : See touchscreen.txt
> >> - silead,max-fingers	  : maximum number of fingers the touchscreen can detect
> >>+- vddio-supply		  : regulator phandle for controller VDDIO
> >>+- avdd-supply		  : regulator phandle for controller AVDD
> >>
> >> Example:
> >>
> >>diff --git a/drivers/input/touchscreen/silead.c b/drivers/input/touchscreen/silead.c
> >>index f502c84..404830a 100644
> >>--- a/drivers/input/touchscreen/silead.c
> >>+++ b/drivers/input/touchscreen/silead.c
> >>@@ -29,6 +29,7 @@
> >> #include <linux/input/touchscreen.h>
> >> #include <linux/pm.h>
> >> #include <linux/irq.h>
> >>+#include <linux/regulator/consumer.h>
> >>
> >> #include <asm/unaligned.h>
> >>
> >>@@ -73,6 +74,7 @@ struct silead_ts_data {
> >> 	struct i2c_client *client;
> >> 	struct gpio_desc *gpio_power;
> >> 	struct input_dev *input;
> >>+	struct regulator_bulk_data regulators[2];
> >> 	char fw_name[64];
> >> 	struct touchscreen_properties prop;
> >> 	u32 max_fingers;
> >>@@ -433,6 +435,13 @@ static int silead_ts_set_default_fw_name(struct silead_ts_data *data,
> >> }
> >> #endif
> >>
> >>+static void silead_disable_regulator(void *arg)
> >>+{
> >>+	struct silead_ts_data *data = arg;
> >>+
> >>+	regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators);
> >>+}
> >>+
> >> static int silead_ts_probe(struct i2c_client *client,
> >> 			   const struct i2c_device_id *id)
> >> {
> >>@@ -465,6 +474,26 @@ static int silead_ts_probe(struct i2c_client *client,
> >> 	if (client->irq <= 0)
> >> 		return -ENODEV;
> >>
> >>+	data->regulators[0].supply = "vddio";
> >>+	data->regulators[1].supply = "avdd";
> >>+	error = devm_regulator_bulk_get(dev, ARRAY_SIZE(data->regulators),
> >>+					data->regulators);
> >>+	if (error)
> >>+		return error;
> >>+
> >>+	/*
> >>+	 * Enable regulators at probe and disable them at remove, we need
> >>+	 * to keep the chip powered otherwise it forgets its firmware.
> >>+	 */
> >
> >Hmm, this burns power though. Why can't we reload firmware on resume (it
> >should be already cached)?
> 
> We already put the device in low-power mode using the power pin. Of the
> 20 or so different tablets I've with this touchscreen controller only
> 2 actually have a separate regulator for the controller, so I do not
> believe that powering down the regulator will be a big win, otherwise
> all tablets would have had this.
> 
> > Does it take too long?
> 
> It is a couple of kB written one 32-bit word at a time over i2c, so
> it's not fast.

OK, fair enough. Applied, thank you.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html