[PATCH 1/2] dt-bindings: input/ts/zinitix: Convert to YAML, fix and extend

[PATCH 1/2] dt-bindings: input/ts/zinitix: Convert to YAML, fix and extend

[Linus Walleij]

This converts the Zinitix BT4xx and BT5xx touchscreen bindings to YAML, fix
them up a bit and extends them.

We list all the existing BT4xx and BT5xx components with compatible strings.
These are all similar, use the same bindings and work in similar ways.

We rename the supplies from the erroneous vdd/vddo to the actual supply
names vcca/vdd as specified on the actual component. It is long established
that supplies shall be named after the supply pin names of a component.
The confusion probably stems from that in a certain product the rails to the
component were named vdd/vddo. Drop some notes on how OS implementations should
avoid confusion by first looking for vddo, and if that exists assume the
legacy binding pair and otherwise use vcca/vdd.

Add reset-gpios as sometimes manufacturers pulls a GPIO line to the reset
line on the chip.

Add optional touchscreen-fuzz-x and touchscreen-fuzz-y properties.

Cc: Mark Brown <broonie@kernel.org>
Cc: Michael Srba <Michael.Srba@seznam.cz>
Cc: phone-devel@vger.kernel.org
Cc: devicetree@vger.kernel.org
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 .../input/touchscreen/zinitix,bt400.yaml      | 115 ++++++++++++++++++
 .../bindings/input/touchscreen/zinitix.txt    |  40 ------
 2 files changed, 115 insertions(+), 40 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/input/touchscreen/zinitix,bt400.yaml
 delete mode 100644 Documentation/devicetree/bindings/input/touchscreen/zinitix.txt

diff --git a/Documentation/devicetree/bindings/input/touchscreen/zinitix,bt400.yaml b/Documentation/devicetree/bindings/input/touchscreen/zinitix,bt400.yaml
new file mode 100644
index 000000000000..6e8b4dede9d7
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/touchscreen/zinitix,bt400.yaml
@@ -0,0 +1,115 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/touchscreen/zinitix,bt400.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Zinitix BT4xx and BT5xx series touchscreen controller bindings
+
+description: The Zinitix BT4xx and BT5xx series of touchscreen controllers
+  are Korea-produced touchscreens with embedded microcontrollers. The
+  BT4xx series was produced 2010-2013 and the BT5xx series 2013-2014.
+
+maintainers:
+  - Michael Srba <Michael.Srba@seznam.cz>
+  - Linus Walleij <linus.walleij@linaro.org>
+
+allOf:
+  - $ref: touchscreen.yaml#
+
+properties:
+  $nodename:
+    pattern: "^touchscreen(@.*)?$"
+
+  compatible:
+    oneOf:
+      - const: zinitix,bt402
+      - const: zinitix,bt403
+      - const: zinitix,bt404
+      - const: zinitix,bt412
+      - const: zinitix,bt413
+      - const: zinitix,bt431
+      - const: zinitix,bt432
+      - const: zinitix,bt531
+      - const: zinitix,bt532
+      - const: zinitix,bt538
+      - const: zinitix,bt541
+      - const: zinitix,bt548
+      - const: zinitix,bt554
+      - const: zinitix,at100
+
+  reg:
+    description: I2C address on the I2C bus
+
+  clock-frequency:
+    description: I2C client clock frequency, defined for host when using
+      the device on the I2C bus
+    minimum: 0
+    maximum: 400000
+
+  interrupts:
+    description: Interrupt to host
+    maxItems: 1
+
+  vcca-supply:
+    description: Analog power supply regulator on the VCCA pin
+
+  vdd-supply:
+    description: Digital power supply regulator on the VDD pin.
+      In older device trees this can be the accidental name for the analog
+      supply on the VCCA pin, and in that case the deprecated vddo-supply is
+      used for the digital power supply.
+
+  vddo-supply:
+    description: Deprecated name for the digital power supply, use vdd-supply
+      as this reflects the real name of the pin. If this supply is present,
+      the vdd-supply represents VCCA instead of VDD. Implementers should first
+      check for this property, and if it is present assume that the vdd-supply
+      represents the analog supply.
+    deprecated: true
+
+  reset-gpios:
+    description: Reset line for the touchscreen, should be tagged
+      as GPIO_ACTIVE_LOW
+
+  zinitix,mode:
+    description: Mode of reporting touch points. Some modes may not work
+      with a particular ts firmware for unknown reasons. Available modes are
+      1 and 2. Mode 2 is the default and preferred.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [1, 2]
+
+  touchscreen-size-x: true
+  touchscreen-size-y: true
+  touchscreen-fuzz-x: true
+  touchscreen-fuzz-y: true
+
+additionalProperties: false
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - touchscreen-size-x
+  - touchscreen-size-y
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/gpio/gpio.h>
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      touchscreen@20 {
+        compatible = "zinitix,bt541";
+        reg = <0x20>;
+        interrupt-parent = <&gpio>;
+        interrupts = <13 IRQ_TYPE_EDGE_FALLING>;
+        vcca-supply = <&reg_vcca_tsp>;
+        vdd-supply = <&reg_vdd_tsp>;
+        touchscreen-size-x = <540>;
+        touchscreen-size-y = <960>;
+        zinitix,mode = <2>;
+      };
+    };
diff --git a/Documentation/devicetree/bindings/input/touchscreen/zinitix.txt b/Documentation/devicetree/bindings/input/touchscreen/zinitix.txt
deleted file mode 100644
index 446efb9f5f55..000000000000
--- a/Documentation/devicetree/bindings/input/touchscreen/zinitix.txt
+++ /dev/null
@@ -1,40 +0,0 @@
-Device tree bindings for Zinitx BT541 touchscreen controller
-
-Required properties:
-
- - compatible		: Should be "zinitix,bt541"
- - reg			: I2C address of the chip. Should be 0x20
- - interrupts		: Interrupt to which the chip is connected
-
-Optional properties:
-
- - vdd-supply		: Analog power supply regulator on VCCA pin
- - vddo-supply		: Digital power supply regulator on VDD pin
- - zinitix,mode		: Mode of reporting touch points. Some modes may not work
-			  with a particular ts firmware for unknown reasons. Available
-			  modes are 1 and 2. Mode 2 is the default and preferred.
-
-The touchscreen-* properties are documented in touchscreen.txt in this
-directory.
-
-Example:
-
-	i2c@00000000 {
-		/* ... */
-
-		bt541@20 {
-			compatible = "zinitix,bt541";
-			reg = <0x20>;
-			interrupt-parent = <&msmgpio>;
-			interrupts = <13 IRQ_TYPE_EDGE_FALLING>;
-			pinctrl-names = "default";
-			pinctrl-0 = <&tsp_default>;
-			vdd-supply = <&reg_vdd_tsp>;
-			vddo-supply = <&pm8916_l6>;
-			touchscreen-size-x = <540>;
-			touchscreen-size-y = <960>;
-			zinitix,mode = <2>;
-		};
-
-		/* ... */
-	};
-- 
2.31.1

[PATCH 2/2] Input: zinitix - Handle proper supply names

[Linus Walleij]

The supply names of the Zinitix touchscreen were a bit confused, the new
bindings rectifies this.

To deal with old and new devicetrees, first check if we have "vddo" and in
case that exists assume the old supply names. Else go and look for the new
ones.

We cannot just get the regulators since we would get an OK and a dummy
regulator: we need to check explicitly for the old supply name.

Use struct device *dev as a local variable instead of the I2C client since
the device is what we are actually obtaining the resources from.

Cc: Mark Brown <broonie@kernel.org>
Cc: Michael Srba <Michael.Srba@seznam.cz>
Cc: phone-devel@vger.kernel.org
Cc: devicetree@vger.kernel.org
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
Mark: please check that I'm doing this check the right way, I assume
that since we get regulator dummies this is the way I need to check
for the old regulator name but maybe there are better ways.
---
 drivers/input/touchscreen/zinitix.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/input/touchscreen/zinitix.c b/drivers/input/touchscreen/zinitix.c
index b8d901099378..7001307382f0 100644
--- a/drivers/input/touchscreen/zinitix.c
+++ b/drivers/input/touchscreen/zinitix.c
@@ -252,16 +252,28 @@ static int zinitix_init_touch(struct bt541_ts_data *bt541)
 
 static int zinitix_init_regulators(struct bt541_ts_data *bt541)
 {
-	struct i2c_client *client = bt541->client;
+	struct device *dev = &bt541->client->dev;
 	int error;
 
-	bt541->supplies[0].supply = "vdd";
-	bt541->supplies[1].supply = "vddo";
-	error = devm_regulator_bulk_get(&client->dev,
+	/*
+	 * Some older device trees have erroneous names for the regulators,
+	 * so check if "vddo" is present and in that case use these names
+	 * and warn. Else use the proper supply names on the component.
+	 */
+	if (IS_ENABLED(CONFIG_OF) &&
+	    of_property_read_bool(dev->of_node, "vddo-supply")) {
+		bt541->supplies[0].supply = "vdd";
+		bt541->supplies[1].supply = "vddo";
+	} else {
+		/* Else use the proper supply names */
+		bt541->supplies[0].supply = "vcca";
+		bt541->supplies[1].supply = "vdd";
+	}
+	error = devm_regulator_bulk_get(dev,
 					ARRAY_SIZE(bt541->supplies),
 					bt541->supplies);
 	if (error < 0) {
-		dev_err(&client->dev, "Failed to get regulators: %d\n", error);
+		dev_err(dev, "Failed to get regulators: %d\n", error);
 		return error;
 	}
 
-- 
2.31.1

Re: [PATCH 1/2] dt-bindings: input/ts/zinitix: Convert to YAML, fix and extend

[Rob Herring]

On Fri, 25 Jun 2021 13:34:34 +0200, Linus Walleij wrote:
> This converts the Zinitix BT4xx and BT5xx touchscreen bindings to YAML, fix
> them up a bit and extends them.
> 
> We list all the existing BT4xx and BT5xx components with compatible strings.
> These are all similar, use the same bindings and work in similar ways.
> 
> We rename the supplies from the erroneous vdd/vddo to the actual supply
> names vcca/vdd as specified on the actual component. It is long established
> that supplies shall be named after the supply pin names of a component.
> The confusion probably stems from that in a certain product the rails to the
> component were named vdd/vddo. Drop some notes on how OS implementations should
> avoid confusion by first looking for vddo, and if that exists assume the
> legacy binding pair and otherwise use vcca/vdd.
> 
> Add reset-gpios as sometimes manufacturers pulls a GPIO line to the reset
> line on the chip.
> 
> Add optional touchscreen-fuzz-x and touchscreen-fuzz-y properties.
> 
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Michael Srba <Michael.Srba@seznam.cz>
> Cc: phone-devel@vger.kernel.org
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  .../input/touchscreen/zinitix,bt400.yaml      | 115 ++++++++++++++++++
>  .../bindings/input/touchscreen/zinitix.txt    |  40 ------
>  2 files changed, 115 insertions(+), 40 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/input/touchscreen/zinitix,bt400.yaml
>  delete mode 100644 Documentation/devicetree/bindings/input/touchscreen/zinitix.txt
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH 2/2] Input: zinitix - Handle proper supply names

[Dmitry Torokhov]

Hi Linus,

On Fri, Jun 25, 2021 at 01:34:35PM +0200, Linus Walleij wrote:
> The supply names of the Zinitix touchscreen were a bit confused, the new
> bindings rectifies this.
> 
> To deal with old and new devicetrees, first check if we have "vddo" and in
> case that exists assume the old supply names. Else go and look for the new
> ones.
> 
> We cannot just get the regulators since we would get an OK and a dummy
> regulator: we need to check explicitly for the old supply name.
> 
> Use struct device *dev as a local variable instead of the I2C client since
> the device is what we are actually obtaining the resources from.
> 
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Michael Srba <Michael.Srba@seznam.cz>
> Cc: phone-devel@vger.kernel.org
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> Mark: please check that I'm doing this check the right way, I assume
> that since we get regulator dummies this is the way I need to check
> for the old regulator name but maybe there are better ways.
> ---
>  drivers/input/touchscreen/zinitix.c | 22 +++++++++++++++++-----
>  1 file changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/zinitix.c b/drivers/input/touchscreen/zinitix.c
> index b8d901099378..7001307382f0 100644
> --- a/drivers/input/touchscreen/zinitix.c
> +++ b/drivers/input/touchscreen/zinitix.c
> @@ -252,16 +252,28 @@ static int zinitix_init_touch(struct bt541_ts_data *bt541)
>  
>  static int zinitix_init_regulators(struct bt541_ts_data *bt541)
>  {
> -	struct i2c_client *client = bt541->client;
> +	struct device *dev = &bt541->client->dev;
>  	int error;
>  
> -	bt541->supplies[0].supply = "vdd";
> -	bt541->supplies[1].supply = "vddo";
> -	error = devm_regulator_bulk_get(&client->dev,
> +	/*
> +	 * Some older device trees have erroneous names for the regulators,
> +	 * so check if "vddo" is present and in that case use these names
> +	 * and warn. Else use the proper supply names on the component.
> +	 */
> +	if (IS_ENABLED(CONFIG_OF) &&

Why is this check needed? The of_property_*() are stubbed out properly I
believe. We might need to check that dev->of_node is not NULL, although
I think of_* API handles this properly.

> +	    of_property_read_bool(dev->of_node, "vddo-supply")) {

If we go with this I do not like using of_property_read_bool() as this
is not a boolean property, but rather of_find_property().

However maybe we should use regulator_get_optional() which will not give
a dummy regulator? Still quite awkward, a dedicated API to see if a
regulator is defined would be nice.

> +		bt541->supplies[0].supply = "vdd";
> +		bt541->supplies[1].supply = "vddo";
> +	} else {
> +		/* Else use the proper supply names */
> +		bt541->supplies[0].supply = "vcca";
> +		bt541->supplies[1].supply = "vdd";
> +	}
> +	error = devm_regulator_bulk_get(dev,
>  					ARRAY_SIZE(bt541->supplies),
>  					bt541->supplies);
>  	if (error < 0) {
> -		dev_err(&client->dev, "Failed to get regulators: %d\n", error);
> +		dev_err(dev, "Failed to get regulators: %d\n", error);
>  		return error;
>  	}
>  
> -- 
> 2.31.1
> 

Thanks.

-- 
Dmitry

Re: [PATCH 2/2] Input: zinitix - Handle proper supply names

[Linus Walleij]

Hi Dmitry, sorry for late reply!

On Sat, Jul 24, 2021 at 3:13 AM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Fri, Jun 25, 2021 at 01:34:35PM +0200, Linus Walleij wrote:

> > +     /*
> > +      * Some older device trees have erroneous names for the regulators,
> > +      * so check if "vddo" is present and in that case use these names
> > +      * and warn. Else use the proper supply names on the component.
> > +      */
> > +     if (IS_ENABLED(CONFIG_OF) &&
>
> Why is this check needed? The of_property_*() are stubbed out properly I
> believe. We might need to check that dev->of_node is not NULL, although
> I think of_* API handles this properly.
(...)
> > +         of_property_read_bool(dev->of_node, "vddo-supply")) {
>
> If we go with this I do not like using of_property_read_bool() as this
> is not a boolean property, but rather of_find_property().

These comments are fixed up in Nikita's respin of the series:
https://lore.kernel.org/linux-input/20211027181350.91630-4-nikita@trvn.ru/

> However maybe we should use regulator_get_optional() which will not give
> a dummy regulator? Still quite awkward, a dedicated API to see if a
> regulator is defined would be nice.

I guess the option would be to get all four regulators by name and
optional, but then we don't detect if more than 2 out of 4 are missing.
Not sure, it feels like we have less control over the supplies then.

I guess it sadly gets ugly because making mistakes in bindings is ugly
in the first place.

Yours,
Linus Walleij