[PATCH 1/2] Input: atmel_mxt_ts: Convert bindings to YAML and extend

[PATCH 1/2] Input: atmel_mxt_ts: Convert bindings to YAML and extend

[Linus Walleij]

This converts the Armel MXT touchscreen bindings to YAML
format and extends them with the following two properties:

- vdda-supply: the optional analog supply voltage
- vdd-supply: the optional digital supply voltage

I also explained about the reset-gpios property that this
better be flagged as active high (0) despite actually
being active low, because all current device trees and
drivers assume that this is the case and will actively
drive the line low to assert RESET.

Tested the schema with all in-tree users and they verify
fine.

Cc: Nick Dyer <nick@shmanahar.org>
Cc: Stephan Gerhold <stephan@gerhold.net>
Cc: devicetree@vger.kernel.org
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 .../bindings/input/atmel,maxtouch.txt         | 41 ---------
 .../bindings/input/atmel,maxtouch.yaml        | 83 +++++++++++++++++++
 MAINTAINERS                                   |  2 +-
 3 files changed, 84 insertions(+), 42 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/input/atmel,maxtouch.txt
 create mode 100644 Documentation/devicetree/bindings/input/atmel,maxtouch.yaml

diff --git a/Documentation/devicetree/bindings/input/atmel,maxtouch.txt b/Documentation/devicetree/bindings/input/atmel,maxtouch.txt
deleted file mode 100644
index c88919480d37..000000000000
--- a/Documentation/devicetree/bindings/input/atmel,maxtouch.txt
+++ /dev/null
@@ -1,41 +0,0 @@
-Atmel maXTouch touchscreen/touchpad
-
-Required properties:
-- compatible:
-    atmel,maxtouch
-
-    The following compatibles have been used in various products but are
-    deprecated:
-	atmel,qt602240_ts
-	atmel,atmel_mxt_ts
-	atmel,atmel_mxt_tp
-	atmel,mXT224
-
-- reg: The I2C address of the device
-
-- interrupts: The sink for the touchpad's IRQ output
-    See ../interrupt-controller/interrupts.txt
-
-Optional properties for main touchpad device:
-
-- linux,gpio-keymap: When enabled, the SPT_GPIOPWN_T19 object sends messages
-    on GPIO bit changes. An array of up to 8 entries can be provided
-    indicating the Linux keycode mapped to each bit of the status byte,
-    starting at the LSB. Linux keycodes are defined in
-    <dt-bindings/input/input.h>.
-
-    Note: the numbering of the GPIOs and the bit they start at varies between
-    maXTouch devices. You must either refer to the documentation, or
-    experiment to determine which bit corresponds to which input. Use
-    KEY_RESERVED for unused padding values.
-
-- reset-gpios: GPIO specifier for the touchscreen's reset pin (active low)
-
-Example:
-
-	touch@4b {
-		compatible = "atmel,maxtouch";
-		reg = <0x4b>;
-		interrupt-parent = <&gpio>;
-		interrupts = <TEGRA_GPIO(W, 3) IRQ_TYPE_LEVEL_LOW>;
-	};
diff --git a/Documentation/devicetree/bindings/input/atmel,maxtouch.yaml b/Documentation/devicetree/bindings/input/atmel,maxtouch.yaml
new file mode 100644
index 000000000000..6173562f328a
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/atmel,maxtouch.yaml
@@ -0,0 +1,83 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/atmel,maxtouch.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Atmel maXTouch touchscreen/touchpad
+
+maintainers:
+  - Nick Dyer <nick@shmanahar.org>
+  - Linus Walleij <linus.walleij@linaro.org>
+
+description: |
+  Atmel maXTouch touchscreen or touchpads such as the mXT244
+  and similar devices.
+
+properties:
+  compatible:
+    const: atmel,maxtouch
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  vdda-supply:
+    description:
+      Optional regulator for the AVDD analog voltage.
+
+  vdd-supply:
+    description:
+      Optional regulator for the VDD digital voltage.
+
+  reset-gpios:
+    maxItems: 1
+    description:
+      Optional GPIO specifier for the touchscreen's reset pin
+      (active low). The operating system should actively drive
+      the line low to assert reset, so the line must NOT be
+      flagged with GPIO_ACTIVE_LOW, it should (counterintuitively)
+      be set to GPIO_ACTIVE_HIGH.
+
+  linux,gpio-keymap:
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    description: |
+      When enabled, the SPT_GPIOPWN_T19 object sends messages
+      on GPIO bit changes. An array of up to 8 entries can be provided
+      indicating the Linux keycode mapped to each bit of the status byte,
+      starting at the LSB. Linux keycodes are defined in
+      <dt-bindings/input/input.h>.
+
+      Note: the numbering of the GPIOs and the bit they start at varies
+      between maXTouch devices. You must either refer to the documentation,
+      or experiment to determine which bit corresponds to which input. Use
+      KEY_RESERVED for unused padding values.
+
+required:
+  - compatible
+  - reg
+  - interrupts
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/gpio/gpio.h>
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+      touchscreen@4a {
+        compatible = "atmel,maxtouch";
+        reg = <0x4a>;
+        interrupt-parent = <&gpio>;
+        interrupts = <26 IRQ_TYPE_EDGE_FALLING>;
+        reset-gpios = <&gpio 27 GPIO_ACTIVE_HIGH>;
+        vdda-supply = <&ab8500_ldo_aux2_reg>;
+        vdd-supply = <&ab8500_ldo_aux5_reg>;
+      };
+    };
+
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index e73636b75f29..b4b46fcb82db 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2966,7 +2966,7 @@ ATMEL MAXTOUCH DRIVER
 M:	Nick Dyer <nick@shmanahar.org>
 S:	Maintained
 T:	git git://github.com/ndyer/linux.git
-F:	Documentation/devicetree/bindings/input/atmel,maxtouch.txt
+F:	Documentation/devicetree/bindings/input/atmel,maxtouch.yaml
 F:	drivers/input/touchscreen/atmel_mxt_ts.c
 
 ATMEL WIRELESS DRIVER
-- 
2.26.2

[PATCH 2/2] Input: atmel_mxt_ts: Support regulator supplies

[Linus Walleij]

This adds the code for the Atmel touchscreens such as
mXT224 to obtain power regulators for the supply voltages
AVDD and VDD. On mobile phones such as Samsung GT-I8190
(Golden) this is needed to explicitly bring power online.

We just enable the regulators at probe() and disable
them at remove() or in the errorpath for now.

As regulators are naturally stubbed if not available,
this should have no impact on existing systems.

Cc: Nick Dyer <nick@shmanahar.org>
Cc: Stephan Gerhold <stephan@gerhold.net>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/input/touchscreen/atmel_mxt_ts.c | 37 +++++++++++++++++++++++-
 1 file changed, 36 insertions(+), 1 deletion(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 98f17fa3a892..701269f9744f 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -24,6 +24,7 @@
 #include <linux/of.h>
 #include <linux/property.h>
 #include <linux/slab.h>
+#include <linux/regulator/consumer.h>
 #include <linux/gpio/consumer.h>
 #include <asm/unaligned.h>
 #include <media/v4l2-device.h>
@@ -309,6 +310,7 @@ struct mxt_data {
 	u8 multitouch;
 	struct t7_config t7_cfg;
 	struct mxt_dbg dbg;
+	struct regulator_bulk_data regulators[2];
 	struct gpio_desc *reset_gpio;
 	bool use_retrigen_workaround;
 
@@ -3134,6 +3136,21 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	if (error)
 		return error;
 
+	/*
+	 * VDDA is the analog voltage supply 2.57..3.47 V
+	 * VDD  is the digital voltage supply 1.71..3.47 V
+	 */
+	data->regulators[0].supply = "vdda";
+	data->regulators[1].supply = "vdd";
+	error = devm_regulator_bulk_get(&client->dev, ARRAY_SIZE(data->regulators),
+					data->regulators);
+	if (error) {
+		if (error != -EPROBE_DEFER)
+			dev_err(&client->dev, "Failed to get regulators %d\n",
+				error);
+		return error;
+	}
+
 	data->reset_gpio = devm_gpiod_get_optional(&client->dev,
 						   "reset", GPIOD_OUT_LOW);
 	if (IS_ERR(data->reset_gpio)) {
@@ -3152,6 +3169,19 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id)
 
 	disable_irq(client->irq);
 
+	error = regulator_bulk_enable(ARRAY_SIZE(data->regulators),
+				      data->regulators);
+	if (error) {
+		dev_err(&client->dev, "failed to enable regulators: %d\n",
+			error);
+		return error;
+	}
+	/*
+	 * The device takes 40ms to come up after power-on according
+	 * to the mXT224 datasheet, page 13.
+	 */
+	msleep(MXT_BACKUP_TIME);
+
 	if (data->reset_gpio) {
 		msleep(MXT_RESET_GPIO_TIME);
 		gpiod_set_value(data->reset_gpio, 1);
@@ -3160,7 +3190,7 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id)
 
 	error = mxt_initialize(data);
 	if (error)
-		return error;
+		goto err_disable_regulators;
 
 	error = sysfs_create_group(&client->dev.kobj, &mxt_attr_group);
 	if (error) {
@@ -3174,6 +3204,9 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id)
 err_free_object:
 	mxt_free_input_device(data);
 	mxt_free_object_table(data);
+err_disable_regulators:
+	regulator_bulk_disable(ARRAY_SIZE(data->regulators),
+			       data->regulators);
 	return error;
 }
 
@@ -3185,6 +3218,8 @@ static int mxt_remove(struct i2c_client *client)
 	sysfs_remove_group(&client->dev.kobj, &mxt_attr_group);
 	mxt_free_input_device(data);
 	mxt_free_object_table(data);
+	regulator_bulk_disable(ARRAY_SIZE(data->regulators),
+			       data->regulators);
 
 	return 0;
 }
-- 
2.26.2

Re: [PATCH 1/2] Input: atmel_mxt_ts: Convert bindings to YAML and extend

[Dmitry Torokhov]

Hi Linus,

On Wed, Oct 28, 2020 at 11:17:10AM +0100, Linus Walleij wrote:
> This converts the Armel MXT touchscreen bindings to YAML
> format and extends them with the following two properties:
> 
> - vdda-supply: the optional analog supply voltage
> - vdd-supply: the optional digital supply voltage
> 
> I also explained about the reset-gpios property that this
> better be flagged as active high (0) despite actually
> being active low, because all current device trees and
> drivers assume that this is the case and will actively
> drive the line low to assert RESET.

I wonder if we should fix that in driver and in DTs instead of doing
this cludge...

Thanks.

-- 
Dmitry

Re: [PATCH 1/2] Input: atmel_mxt_ts: Convert bindings to YAML and extend

[Linus Walleij]

On Wed, Oct 28, 2020 at 7:01 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Wed, Oct 28, 2020 at 11:17:10AM +0100, Linus Walleij wrote:

> > This converts the Armel MXT touchscreen bindings to YAML
> > format and extends them with the following two properties:
> >
> > - vdda-supply: the optional analog supply voltage
> > - vdd-supply: the optional digital supply voltage
> >
> > I also explained about the reset-gpios property that this
> > better be flagged as active high (0) despite actually
> > being active low, because all current device trees and
> > drivers assume that this is the case and will actively
> > drive the line low to assert RESET.
>
> I wonder if we should fix that in driver and in DTs instead of doing
> this cludge...

Unfortunately I think there are deployed systems with flashed-in
system descriptions depending on this bug in the system
description already.

I am not thinking about device trees now, but instead ACPI
chromebooks, that have their reset line flagged as whatever
ACPI or DT-to-ACPI use to indicate an active high line.
Despite being active low.

I could fix all the in-tree devicetrees and do it the natural way
(I have certainly done so before) and then add a quirk if used
with ACPI. But it's really risky. I'm afraid of regressions here.

Yours,
Linus Walleij

Re: [PATCH 1/2] Input: atmel_mxt_ts: Convert bindings to YAML and extend

[Dmitry Torokhov]

On Thu, Oct 29, 2020 at 02:47:36PM +0100, Linus Walleij wrote:
> On Wed, Oct 28, 2020 at 7:01 PM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > On Wed, Oct 28, 2020 at 11:17:10AM +0100, Linus Walleij wrote:
> 
> > > This converts the Armel MXT touchscreen bindings to YAML
> > > format and extends them with the following two properties:
> > >
> > > - vdda-supply: the optional analog supply voltage
> > > - vdd-supply: the optional digital supply voltage
> > >
> > > I also explained about the reset-gpios property that this
> > > better be flagged as active high (0) despite actually
> > > being active low, because all current device trees and
> > > drivers assume that this is the case and will actively
> > > drive the line low to assert RESET.
> >
> > I wonder if we should fix that in driver and in DTs instead of doing
> > this cludge...
> 
> Unfortunately I think there are deployed systems with flashed-in
> system descriptions depending on this bug in the system
> description already.
> 
> I am not thinking about device trees now, but instead ACPI
> chromebooks, that have their reset line flagged as whatever
> ACPI or DT-to-ACPI use to indicate an active high line.
> Despite being active low.

The only ARM Chromebook that exposed reset line to the kernel was RK3288
Asus Chromebook "Minnie". DTS specifies correct polarity (active low),
but uses different binding (atmel,reset-gpios) from the driver found
upstream (I have never reconciled Atmel driver we ship with Chromebooks
with the upstream one). DT there is also part of the kernel, not flashed
separately.

x86 Chromebooks do not export reset line or regulators to the kernel but
rather handle power up/down sequence in firmware (either at boot or
exposing ACPI power control methods that kernel invokes form ACPI power
domain code).

> 
> I could fix all the in-tree devicetrees and do it the natural way
> (I have certainly done so before) and then add a quirk if used
> with ACPI. But it's really risky. I'm afraid of regressions here.

Unless there are unofficial firmwares that rework power handling on some
x86 Chromebooks and we want to support them I'd rather we did not quirk.

Thanks.

-- 
Dmitry