All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/4] dt-bindings: gpio: Replace DW APB GPIO legacy bindings with YAML-based one
       [not found] <20200306132448.13917-1-Sergey.Semin@baikalelectronics.ru>
@ 2020-03-06 13:24 ` Sergey.Semin
  2020-03-12 13:47   ` Rob Herring
  2020-03-06 13:24 ` [PATCH 2/4] dt-bindings: gpio: Add DW GPIO debounce clocks bindings Sergey.Semin
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 12+ messages in thread
From: Sergey.Semin @ 2020-03-06 13:24 UTC (permalink / raw)
  To: Linus Walleij, Bartosz Golaszewski, Rob Herring, Mark Rutland, Hoan Tran
  Cc: Serge Semin, Serge Semin, Alexey Malahov, Thomas Bogendoerfer,
	Paul Burton, Ralf Baechle, linux-gpio, devicetree, linux-kernel

From: Serge Semin <Sergey.Semin@baikalelectronics.ru>

Modern device tree bindings are supposed to be created as YAML-files
in accordance with dt-schema. This commit replaces Synopsys DW GPIO
legacy bare text bindings with YAML file. As before the bindings file
states that the corresponding dts node is supposed to be compatible
with generic DW I2C controller indicated by the "snps,dw-apb-gpio"
compatible string and provide a mandatory registers memory range.
It may also have an optional clocks and resets phandle references.

There must be specified at least one subnode with
"snps,dw-apb-gpio-port" compatible string indicating the GPIO port,
which would actually export the GPIO controller functionality. Such
nodes should have traditional GPIO controller properties together
with optional interrupt-controller attributes if the corresponding
controller was synthesized to detected and report the input values
change to the parental IRQ controller.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
Signed-off-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Cc: Paul Burton <paulburton@kernel.org>
Cc: Ralf Baechle <ralf@linux-mips.org>

---

Synopsis DesignWare APB SSI controller has a bindings property
"snps,nr-gpios" of numeric type, which means the number of GPIO pins
exported by the corresponding controller port. There is also a generic
pattern-property "*-gpios", which corresponds to a GPIOs array. As you
can see the GPIOs array property wildcard matches the vendor-specific
property "snps,nr-gpios" property while having an incompatible type.
Due to this the DW APB GPIO dts-nodes evaluation will report the
following error:

snps,nr-gpios:0:0: 8 is not valid under any of the given schemas (Possible causes of the failure):
snps,nr-gpios:0:0: missing phandle tag in 8

I didn't manage to fix the problem by redefining the property schema (this
might be impossible anyway). In my opinion the best way to solve it would be
to change the DW APB SSI Controller bindings so the driver would accept the
standard "ngpios" property for the same purpose. But in this case we would have
to alter all the dts files currently having the "snps,dw-apb-ssi" compatible
nodes (it's a lot). I know the bindings modifications aren't that much welcome
in the kernel community and there are good reasons why. So what do you think
would be the better way to fix the problem with the property types collision?
---
 .../bindings/gpio/snps,dw-apb-gpio.yaml       | 136 ++++++++++++++++++
 .../bindings/gpio/snps-dwapb-gpio.txt         |  65 ---------
 2 files changed, 136 insertions(+), 65 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/gpio/snps,dw-apb-gpio.yaml
 delete mode 100644 Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt

diff --git a/Documentation/devicetree/bindings/gpio/snps,dw-apb-gpio.yaml b/Documentation/devicetree/bindings/gpio/snps,dw-apb-gpio.yaml
new file mode 100644
index 000000000000..d9bc12e9e515
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/snps,dw-apb-gpio.yaml
@@ -0,0 +1,136 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpio/snps,dw-apb-gpio.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Synopsys DesignWare APB GPIO controller
+
+description: |
+  Synopsys DesignWare GPIO controllers have a configurable number of ports,
+  each of which are intended to be represented as child nodes with the generic
+  GPIO-controller properties as desribed in this bindings file.
+
+maintainers:
+  - Hoan Tran <hoan@os.amperecomputing.com>
+
+properties:
+  $nodename:
+    pattern: "^gpio@[0-9a-fA-F]+$"
+
+  compatible:
+    const: snps,dw-apb-gpio
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 0
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    items:
+      - description: APB interface clock source
+
+  clock-names:
+    items:
+      - const: bus
+
+  resets:
+    maxItems: 1
+
+patternProperties:
+  "^.*@[0-9a-fA-F]+$":
+    type: object
+    properties:
+      compatible:
+        const: snps,dw-apb-gpio-port
+
+      reg:
+        maxItems: 1
+
+      gpio-controller: true
+
+      '#gpio-cells':
+        const: 2
+
+      snps,nr-gpios:
+        $ref: /schemas/types.yaml#/definitions/uint32
+        description: The number of GPIO pins exported by the port.
+        default: 32
+        minimum: 1
+        maximum: 32
+
+      interrupts:
+        description: |
+          The interrupts to the parent controller raised when GPIOs generate
+          the interrupts. If the controller provides one combined interrupt
+          for all GPIOs, specify a single interrupt. If the controller provides
+          one interrupt for each GPIO, provide a list of interrupts that
+          correspond to each of the GPIO pins.
+        minItems: 1
+        maxItems: 32
+
+      interrupts-extended:
+        description: |
+          When specifying multiple interrupts, if any are unconnected, use
+          this property to specify the interrupts and set the interrupt
+          controller handle for unused interrupts to 0.
+        minItems: 1
+        maxItems: 32
+
+      interrupt-controller: true
+
+      '#interrupt-cells':
+        const: 2
+
+    required:
+      - compatible
+      - reg
+      - gpio-controller
+      - '#gpio-cells'
+
+    dependencies:
+      interrupt-controller: [ interrupts ]
+
+    additionalProperties: false
+
+additionalProperties: false
+
+required:
+  - compatible
+  - reg
+  - "#address-cells"
+  - "#size-cells"
+
+examples:
+  - |
+    gpio: gpio@20000 {
+      compatible = "snps,dw-apb-gpio";
+      reg = <0x20000 0x1000>;
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      porta: gpio@0 {
+        compatible = "snps,dw-apb-gpio-port";
+        reg = <0>;
+        gpio-controller;
+        #gpio-cells = <2>;
+        snps,nr-gpios = <8>;
+        interrupt-controller;
+        #interrupt-cells = <2>;
+        interrupt-parent = <&vic1>;
+        interrupts = <0>;
+      };
+
+      portb: gpio@1 {
+        compatible = "snps,dw-apb-gpio-port";
+        reg = <1>;
+        gpio-controller;
+        #gpio-cells = <2>;
+        snps,nr-gpios = <8>;
+      };
+    };
+...
diff --git a/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt b/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt
deleted file mode 100644
index 839dd32ffe11..000000000000
--- a/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt
+++ /dev/null
@@ -1,65 +0,0 @@
-* Synopsys DesignWare APB GPIO controller
-
-Required properties:
-- compatible : Should contain "snps,dw-apb-gpio"
-- reg : Address and length of the register set for the device.
-- #address-cells : should be 1 (for addressing port subnodes).
-- #size-cells : should be 0 (port subnodes).
-
-The GPIO controller has a configurable number of ports, each of which are
-represented as child nodes with the following properties:
-
-Required properties:
-- compatible : "snps,dw-apb-gpio-port"
-- gpio-controller : Marks the device node as a gpio controller.
-- #gpio-cells : Should be two.  The first cell is the pin number and
-  the second cell is used to specify the gpio polarity:
-      0 = active high
-      1 = active low
-- reg : The integer port index of the port, a single cell.
-
-Optional properties:
-- interrupt-controller : The first port may be configured to be an interrupt
-controller.
-- #interrupt-cells : Specifies the number of cells needed to encode an
-  interrupt.  Shall be set to 2.  The first cell defines the interrupt number,
-  the second encodes the triger flags encoded as described in
-  Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
-- interrupts : The interrupts to the parent controller raised when GPIOs
-  generate the interrupts. If the controller provides one combined interrupt
-  for all GPIOs, specify a single interrupt. If the controller provides one
-  interrupt for each GPIO, provide a list of interrupts that correspond to each
-  of the GPIO pins. When specifying multiple interrupts, if any are unconnected,
-  use the interrupts-extended property to specify the interrupts and set the
-  interrupt controller handle for unused interrupts to 0.
-- snps,nr-gpios : The number of pins in the port, a single cell.
-- resets : Reset line for the controller.
-
-Example:
-
-gpio: gpio@20000 {
-	compatible = "snps,dw-apb-gpio";
-	reg = <0x20000 0x1000>;
-	#address-cells = <1>;
-	#size-cells = <0>;
-
-	porta: gpio@0 {
-		compatible = "snps,dw-apb-gpio-port";
-		gpio-controller;
-		#gpio-cells = <2>;
-		snps,nr-gpios = <8>;
-		reg = <0>;
-		interrupt-controller;
-		#interrupt-cells = <2>;
-		interrupt-parent = <&vic1>;
-		interrupts = <0>;
-	};
-
-	portb: gpio@1 {
-		compatible = "snps,dw-apb-gpio-port";
-		gpio-controller;
-		#gpio-cells = <2>;
-		snps,nr-gpios = <8>;
-		reg = <1>;
-	};
-};
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 2/4] dt-bindings: gpio: Add DW GPIO debounce clocks bindings
       [not found] <20200306132448.13917-1-Sergey.Semin@baikalelectronics.ru>
  2020-03-06 13:24 ` [PATCH 1/4] dt-bindings: gpio: Replace DW APB GPIO legacy bindings with YAML-based one Sergey.Semin
@ 2020-03-06 13:24 ` Sergey.Semin
  2020-03-12 21:44   ` Rob Herring
  2020-03-06 13:24 ` [PATCH 3/4] gpio: dwapb: Use optional-clocks interface for APB ref-clocks Sergey.Semin
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 12+ messages in thread
From: Sergey.Semin @ 2020-03-06 13:24 UTC (permalink / raw)
  To: Linus Walleij, Bartosz Golaszewski, Rob Herring, Mark Rutland
  Cc: Serge Semin, Serge Semin, Alexey Malahov, Thomas Bogendoerfer,
	Paul Burton, Ralf Baechle, linux-gpio, devicetree, linux-kernel

From: Serge Semin <Sergey.Semin@baikalelectronics.ru>

Port A of the DW GPIO controller may optionally have a debounce
logic enabled if it was synthesized with corresponding functionality
enabled. In this case a dedicated reference clocks should be provided
to the node with "db" clock-names.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
Signed-off-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Cc: Paul Burton <paulburton@kernel.org>
Cc: Ralf Baechle <ralf@linux-mips.org>
---
 Documentation/devicetree/bindings/gpio/snps,dw-apb-gpio.yaml | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/gpio/snps,dw-apb-gpio.yaml b/Documentation/devicetree/bindings/gpio/snps,dw-apb-gpio.yaml
index d9bc12e9e515..8b510802f5bc 100644
--- a/Documentation/devicetree/bindings/gpio/snps,dw-apb-gpio.yaml
+++ b/Documentation/devicetree/bindings/gpio/snps,dw-apb-gpio.yaml
@@ -31,12 +31,16 @@ properties:
     maxItems: 1
 
   clocks:
+    minItems: 1
     items:
       - description: APB interface clock source
+      - description: DW GPIO debounce reference clock source
 
   clock-names:
+    minItems: 1
     items:
       - const: bus
+      - const: db
 
   resets:
     maxItems: 1
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 3/4] gpio: dwapb: Use optional-clocks interface for APB ref-clocks
       [not found] <20200306132448.13917-1-Sergey.Semin@baikalelectronics.ru>
  2020-03-06 13:24 ` [PATCH 1/4] dt-bindings: gpio: Replace DW APB GPIO legacy bindings with YAML-based one Sergey.Semin
  2020-03-06 13:24 ` [PATCH 2/4] dt-bindings: gpio: Add DW GPIO debounce clocks bindings Sergey.Semin
@ 2020-03-06 13:24 ` Sergey.Semin
  2020-03-12 13:53   ` Linus Walleij
  2020-03-06 13:24 ` [PATCH 4/4] gpio: dwapb: Add debounce reference clock support Sergey.Semin
  2020-03-10  0:23 ` [PATCH 0/4] gpio: dwapb: Fix reference clocks usage Sergey Semin
  4 siblings, 1 reply; 12+ messages in thread
From: Sergey.Semin @ 2020-03-06 13:24 UTC (permalink / raw)
  To: Hoan Tran, Linus Walleij, Bartosz Golaszewski
  Cc: Serge Semin, Serge Semin, Alexey Malahov, Thomas Bogendoerfer,
	Paul Burton, Ralf Baechle, linux-gpio, linux-kernel

From: Serge Semin <Sergey.Semin@baikalelectronics.ru>

The common clocks kernel framework provide a generic way to use
an optional reference clock sources. If it's utilized there is no
need in checking whether the clocks descriptor is actually a
negative error at the moment of the prepare/unprepare clocks method
calling. So if the corresponding clock source is provided, then
getting an error shall actually terminate the device probe procedure.
If it isn't specified then the driver shall proceed with further
initializations.

We'll use the optional clocks getting method to handle the APB interface
reference clocks, which can be provided for instance in the device
of-node with "bus" clock-name.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
Signed-off-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Cc: Paul Burton <paulburton@kernel.org>
Cc: Ralf Baechle <ralf@linux-mips.org>
---
 drivers/gpio/gpio-dwapb.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 92e127e74813..bfa16172f973 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -690,13 +690,16 @@ static int dwapb_gpio_probe(struct platform_device *pdev)
 		return PTR_ERR(gpio->regs);
 
 	/* Optional bus clock */
-	gpio->clk = devm_clk_get(&pdev->dev, "bus");
-	if (!IS_ERR(gpio->clk)) {
-		err = clk_prepare_enable(gpio->clk);
-		if (err) {
-			dev_info(&pdev->dev, "Cannot enable clock\n");
-			return err;
-		}
+	gpio->clk = devm_clk_get_optional(&pdev->dev, "bus");
+	if (IS_ERR(gpio->clk)) {
+		dev_info(&pdev->dev, "Cannot get APB clock\n");
+		return PTR_ERR(gpio->clk);
+	}
+
+	err = clk_prepare_enable(gpio->clk);
+	if (err) {
+		dev_info(&pdev->dev, "Cannot enable APB clock\n");
+		return err;
 	}
 
 	gpio->flags = 0;
@@ -793,8 +796,7 @@ static int dwapb_gpio_resume(struct device *dev)
 	unsigned long flags;
 	int i;
 
-	if (!IS_ERR(gpio->clk))
-		clk_prepare_enable(gpio->clk);
+	clk_prepare_enable(gpio->clk);
 
 	spin_lock_irqsave(&gc->bgpio_lock, flags);
 	for (i = 0; i < gpio->nr_ports; i++) {
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 4/4] gpio: dwapb: Add debounce reference clock support
       [not found] <20200306132448.13917-1-Sergey.Semin@baikalelectronics.ru>
                   ` (2 preceding siblings ...)
  2020-03-06 13:24 ` [PATCH 3/4] gpio: dwapb: Use optional-clocks interface for APB ref-clocks Sergey.Semin
@ 2020-03-06 13:24 ` Sergey.Semin
  2020-03-12 13:55   ` Linus Walleij
  2020-03-10  0:23 ` [PATCH 0/4] gpio: dwapb: Fix reference clocks usage Sergey Semin
  4 siblings, 1 reply; 12+ messages in thread
From: Sergey.Semin @ 2020-03-06 13:24 UTC (permalink / raw)
  To: Hoan Tran, Linus Walleij, Bartosz Golaszewski
  Cc: Serge Semin, Serge Semin, Alexey Malahov, Thomas Bogendoerfer,
	Paul Burton, Ralf Baechle, linux-gpio, linux-kernel

From: Serge Semin <Sergey.Semin@baikalelectronics.ru>

Aside from the APB reference clock DW GPIO controller can have a
dedicated clock connected to setup a debounce time interval for
GPIO-based IRQs. Since this functionality is optional the corresponding
clock source is also optional. Due to this lets handle the debounce
clock in the same way as it was created for APB reference clock,
but using the bulk request/enable-disable methods.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
Signed-off-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Cc: Paul Burton <paulburton@kernel.org>
Cc: Ralf Baechle <ralf@linux-mips.org>
---
 drivers/gpio/gpio-dwapb.c | 35 ++++++++++++++++++++++-------------
 1 file changed, 22 insertions(+), 13 deletions(-)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index bfa16172f973..495b87f7c351 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -62,6 +62,8 @@
 #define GPIO_INTSTATUS_V2	0x3c
 #define GPIO_PORTA_EOI_V2	0x40
 
+#define DWAPB_NR_CLOCKS		2
+
 struct dwapb_gpio;
 
 #ifdef CONFIG_PM_SLEEP
@@ -97,7 +99,7 @@ struct dwapb_gpio {
 	struct irq_domain	*domain;
 	unsigned int		flags;
 	struct reset_control	*rst;
-	struct clk		*clk;
+	struct clk_bulk_data	clks[DWAPB_NR_CLOCKS];
 };
 
 static inline u32 gpio_reg_v2_convert(unsigned int offset)
@@ -689,16 +691,19 @@ static int dwapb_gpio_probe(struct platform_device *pdev)
 	if (IS_ERR(gpio->regs))
 		return PTR_ERR(gpio->regs);
 
-	/* Optional bus clock */
-	gpio->clk = devm_clk_get_optional(&pdev->dev, "bus");
-	if (IS_ERR(gpio->clk)) {
-		dev_info(&pdev->dev, "Cannot get APB clock\n");
-		return PTR_ERR(gpio->clk);
+	/* Optional bus and debounce clocks */
+	gpio->clks[0].id = "bus";
+	gpio->clks[1].id = "db";
+	err = devm_clk_bulk_get_optional(&pdev->dev, DWAPB_NR_CLOCKS,
+					 gpio->clks);
+	if (err) {
+		dev_info(&pdev->dev, "Cannot get APB/Debounce clocks\n");
+		return err;
 	}
 
-	err = clk_prepare_enable(gpio->clk);
+	err = clk_bulk_prepare_enable(DWAPB_NR_CLOCKS, gpio->clks);
 	if (err) {
-		dev_info(&pdev->dev, "Cannot enable APB clock\n");
+		dev_info(&pdev->dev, "Cannot enable APB/Debounce clocks\n");
 		return err;
 	}
 
@@ -727,7 +732,7 @@ static int dwapb_gpio_probe(struct platform_device *pdev)
 out_unregister:
 	dwapb_gpio_unregister(gpio);
 	dwapb_irq_teardown(gpio);
-	clk_disable_unprepare(gpio->clk);
+	clk_bulk_disable_unprepare(DWAPB_NR_CLOCKS, gpio->clks);
 
 	return err;
 }
@@ -739,7 +744,7 @@ static int dwapb_gpio_remove(struct platform_device *pdev)
 	dwapb_gpio_unregister(gpio);
 	dwapb_irq_teardown(gpio);
 	reset_control_assert(gpio->rst);
-	clk_disable_unprepare(gpio->clk);
+	clk_bulk_disable_unprepare(DWAPB_NR_CLOCKS, gpio->clks);
 
 	return 0;
 }
@@ -784,7 +789,7 @@ static int dwapb_gpio_suspend(struct device *dev)
 	}
 	spin_unlock_irqrestore(&gc->bgpio_lock, flags);
 
-	clk_disable_unprepare(gpio->clk);
+	clk_bulk_disable_unprepare(DWAPB_NR_CLOCKS, gpio->clks);
 
 	return 0;
 }
@@ -794,9 +799,13 @@ static int dwapb_gpio_resume(struct device *dev)
 	struct dwapb_gpio *gpio = dev_get_drvdata(dev);
 	struct gpio_chip *gc	= &gpio->ports[0].gc;
 	unsigned long flags;
-	int i;
+	int i, err;
 
-	clk_prepare_enable(gpio->clk);
+	err = clk_bulk_prepare_enable(DWAPB_NR_CLOCKS, gpio->clks);
+	if (err) {
+		dev_err(gpio->dev, "Cannot reenable APB/Debounce clocks\n");
+		return err;
+	}
 
 	spin_lock_irqsave(&gc->bgpio_lock, flags);
 	for (i = 0; i < gpio->nr_ports; i++) {
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH 0/4] gpio: dwapb: Fix reference clocks usage
       [not found] <20200306132448.13917-1-Sergey.Semin@baikalelectronics.ru>
                   ` (3 preceding siblings ...)
  2020-03-06 13:24 ` [PATCH 4/4] gpio: dwapb: Add debounce reference clock support Sergey.Semin
@ 2020-03-10  0:23 ` Sergey Semin
  4 siblings, 0 replies; 12+ messages in thread
From: Sergey Semin @ 2020-03-10  0:23 UTC (permalink / raw)
  To: Alexey Malahov, Maxim Kaurkin, Pavel Parkhomenko, Ramil Zaripov,
	Ekaterina Skachko, Vadim Vlasov, Thomas Bogendoerfer,
	Paul Burton, Ralf Baechle, Hoan Tran, Linus Walleij,
	Bartosz Golaszewski, Philipp Zabel, Rob Herring, Mark Rutland,
	linux-gpio, devicetree, linux-kernel

On Fri, Mar 06, 2020 at 04:24:44PM +0300, Sergey.Semin@baikalelectronics.ru wrote:
> From: Serge Semin <fancer.lancer@gmail.com>
> 
> There is no need in any fixes to have the Baikal-T1 SoC DW GPIO controllers
> supported by the kernel DW APB GPIO driver. It works for them just fine with
> no modifications. But still there is a room for optimizations there.
> 
> First of all as it tends to be traditional for all Baikal-T1 SoC related
> patchset we replaced the legacy plain text-based dt-binding file with
> yaml-based one. Baikal-T1 DW GPIO port A supports a debounce functionality,
> but in order to use it the corresponding reference clock must be enabled.
> We added support of that clock in the driver and made sure the dt-bindings
> had its declaration. In addition seeing both APB and debounce reference
> clocks are optional, we replaced the standard devm_clk_get() usage with
> the function of optional clocks acquisition.
> 
> This patchset is rebased and tested on the mainline Linux kernel 5.6-rc4:
> commit 98d54f81e36b ("Linux 5.6-rc4").
> 
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> Signed-off-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> Cc: Maxim Kaurkin <Maxim.Kaurkin@baikalelectronics.ru>
> Cc: Pavel Parkhomenko <Pavel.Parkhomenko@baikalelectronics.ru>
> Cc: Ramil Zaripov <Ramil.Zaripov@baikalelectronics.ru>
> Cc: Ekaterina Skachko <Ekaterina.Skachko@baikalelectronics.ru>
> Cc: Vadim Vlasov <V.Vlasov@baikalelectronics.ru>
> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> Cc: Paul Burton <paulburton@kernel.org>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> Cc: Hoan Tran <hoan@os.amperecomputing.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: linux-gpio@vger.kernel.org
> Cc: devicetree@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> 
> Serge Semin (4):
>   dt-bindings: gpio: Replace DW APB GPIO legacy bindings with YAML-based
>     one
>   dt-bindings: gpio: Add DW GPIO debounce clocks bindings
>   gpio: dwapb: Use optional-clocks interface for APB ref-clocks
>   gpio: dwapb: Add debounce reference clock support
> 
>  .../bindings/gpio/snps,dw-apb-gpio.yaml       | 140 ++++++++++++++++++
>  .../bindings/gpio/snps-dwapb-gpio.txt         |  65 --------
>  drivers/gpio/gpio-dwapb.c                     |  41 +++--
>  3 files changed, 166 insertions(+), 80 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/gpio/snps,dw-apb-gpio.yaml
>  delete mode 100644 Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt
> 
> -- 
> 2.25.1
> 

Folks,

It appears our corporate email server changes the Message-Id field of 
messages passing through it. Due to that the emails threading gets to be
broken. I'll resubmit the properly structured patchset as soon as our system
administrator fixes the problem. Sorry for the inconvenience caused by it.

Regards,
-Sergey

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/4] dt-bindings: gpio: Replace DW APB GPIO legacy bindings with YAML-based one
  2020-03-06 13:24 ` [PATCH 1/4] dt-bindings: gpio: Replace DW APB GPIO legacy bindings with YAML-based one Sergey.Semin
@ 2020-03-12 13:47   ` Rob Herring
  2020-03-13 18:40     ` Sergey Semin
  0 siblings, 1 reply; 12+ messages in thread
From: Rob Herring @ 2020-03-12 13:47 UTC (permalink / raw)
  To: Sergey.Semin
  Cc: Linus Walleij, Bartosz Golaszewski, Mark Rutland, Hoan Tran,
	Serge Semin, Alexey Malahov, Thomas Bogendoerfer, Paul Burton,
	Ralf Baechle, open list:GPIO SUBSYSTEM, devicetree, linux-kernel

On Fri, Mar 6, 2020 at 7:25 AM <Sergey.Semin@baikalelectronics.ru> wrote:
>

Subject is kind of long and wordy. Perhaps:

dt-bindings: gpio: Convert snps,dw-apb-gpio to DT schema

> From: Serge Semin <Sergey.Semin@baikalelectronics.ru>
>
> Modern device tree bindings are supposed to be created as YAML-files
> in accordance with dt-schema. This commit replaces Synopsys DW GPIO
> legacy bare text bindings with YAML file. As before the bindings file
> states that the corresponding dts node is supposed to be compatible
> with generic DW I2C controller indicated by the "snps,dw-apb-gpio"
> compatible string and provide a mandatory registers memory range.
> It may also have an optional clocks and resets phandle references.
>
> There must be specified at least one subnode with
> "snps,dw-apb-gpio-port" compatible string indicating the GPIO port,
> which would actually export the GPIO controller functionality. Such
> nodes should have traditional GPIO controller properties together
> with optional interrupt-controller attributes if the corresponding
> controller was synthesized to detected and report the input values
> change to the parental IRQ controller.
>
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> Signed-off-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> Cc: Paul Burton <paulburton@kernel.org>
> Cc: Ralf Baechle <ralf@linux-mips.org>
>
> ---
>
> Synopsis DesignWare APB SSI controller has a bindings property
> "snps,nr-gpios" of numeric type, which means the number of GPIO pins
> exported by the corresponding controller port. There is also a generic
> pattern-property "*-gpios", which corresponds to a GPIOs array. As you
> can see the GPIOs array property wildcard matches the vendor-specific
> property "snps,nr-gpios" property while having an incompatible type.
> Due to this the DW APB GPIO dts-nodes evaluation will report the
> following error:
>
> snps,nr-gpios:0:0: 8 is not valid under any of the given schemas (Possible causes of the failure):
> snps,nr-gpios:0:0: missing phandle tag in 8
>
> I didn't manage to fix the problem by redefining the property schema (this
> might be impossible anyway). In my opinion the best way to solve it would be
> to change the DW APB SSI Controller bindings so the driver would accept the
> standard "ngpios" property for the same purpose. But in this case we would have
> to alter all the dts files currently having the "snps,dw-apb-ssi" compatible
> nodes (it's a lot). I know the bindings modifications aren't that much welcome
> in the kernel community and there are good reasons why. So what do you think
> would be the better way to fix the problem with the property types collision?

Does this change (to dt-schema) work for you?

diff --git a/schemas/gpio/gpio.yaml b/schemas/gpio/gpio.yaml
index 1d9c109f9791..d1c08ccfdc1a 100644
--- a/schemas/gpio/gpio.yaml
+++ b/schemas/gpio/gpio.yaml
@@ -34,7 +34,7 @@ properties:
       - $ref: "/schemas/types.yaml#/definitions/phandle-array"

 patternProperties:
-  ".*-gpios?$":
+  "(?<!,nr)-gpios?$":
     $ref: "/schemas/types.yaml#/definitions/phandle-array"
   "^gpios$":
     $ref: "/schemas/types.yaml#/definitions/phandle-array"


> ---
>  .../bindings/gpio/snps,dw-apb-gpio.yaml       | 136 ++++++++++++++++++
>  .../bindings/gpio/snps-dwapb-gpio.txt         |  65 ---------
>  2 files changed, 136 insertions(+), 65 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/gpio/snps,dw-apb-gpio.yaml
>  delete mode 100644 Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt
>
> diff --git a/Documentation/devicetree/bindings/gpio/snps,dw-apb-gpio.yaml b/Documentation/devicetree/bindings/gpio/snps,dw-apb-gpio.yaml
> new file mode 100644
> index 000000000000..d9bc12e9e515
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/snps,dw-apb-gpio.yaml
> @@ -0,0 +1,136 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)

Do you have rights to add BSD?

> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/gpio/snps,dw-apb-gpio.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Synopsys DesignWare APB GPIO controller
> +
> +description: |
> +  Synopsys DesignWare GPIO controllers have a configurable number of ports,
> +  each of which are intended to be represented as child nodes with the generic
> +  GPIO-controller properties as desribed in this bindings file.
> +
> +maintainers:
> +  - Hoan Tran <hoan@os.amperecomputing.com>
> +
> +properties:
> +  $nodename:
> +    pattern: "^gpio@[0-9a-fA-F]+$"

Lowercase hex for unit-addresses.

> +
> +  compatible:
> +    const: snps,dw-apb-gpio
> +
> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 0
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    items:
> +      - description: APB interface clock source
> +
> +  clock-names:
> +    items:
> +      - const: bus
> +
> +  resets:
> +    maxItems: 1
> +
> +patternProperties:
> +  "^.*@[0-9a-fA-F]+$":

Shouldn't it be "^gpio@..." And unit-addresses should be lowercase.

> +    type: object
> +    properties:
> +      compatible:
> +        const: snps,dw-apb-gpio-port
> +
> +      reg:
> +        maxItems: 1
> +
> +      gpio-controller: true
> +
> +      '#gpio-cells':
> +        const: 2
> +
> +      snps,nr-gpios:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        description: The number of GPIO pins exported by the port.
> +        default: 32
> +        minimum: 1
> +        maximum: 32
> +
> +      interrupts:
> +        description: |
> +          The interrupts to the parent controller raised when GPIOs generate
> +          the interrupts. If the controller provides one combined interrupt
> +          for all GPIOs, specify a single interrupt. If the controller provides
> +          one interrupt for each GPIO, provide a list of interrupts that
> +          correspond to each of the GPIO pins.
> +        minItems: 1
> +        maxItems: 32
> +
> +      interrupts-extended:

Drop this. It gets added by the tools automatically.

> +        description: |
> +          When specifying multiple interrupts, if any are unconnected, use
> +          this property to specify the interrupts and set the interrupt
> +          controller handle for unused interrupts to 0.
> +        minItems: 1
> +        maxItems: 32
> +
> +      interrupt-controller: true
> +
> +      '#interrupt-cells':
> +        const: 2
> +
> +    required:
> +      - compatible
> +      - reg
> +      - gpio-controller
> +      - '#gpio-cells'
> +
> +    dependencies:
> +      interrupt-controller: [ interrupts ]
> +
> +    additionalProperties: false
> +
> +additionalProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +  - "#address-cells"
> +  - "#size-cells"
> +
> +examples:
> +  - |
> +    gpio: gpio@20000 {
> +      compatible = "snps,dw-apb-gpio";
> +      reg = <0x20000 0x1000>;
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      porta: gpio@0 {
> +        compatible = "snps,dw-apb-gpio-port";
> +        reg = <0>;
> +        gpio-controller;
> +        #gpio-cells = <2>;
> +        snps,nr-gpios = <8>;
> +        interrupt-controller;
> +        #interrupt-cells = <2>;
> +        interrupt-parent = <&vic1>;
> +        interrupts = <0>;
> +      };
> +
> +      portb: gpio@1 {
> +        compatible = "snps,dw-apb-gpio-port";
> +        reg = <1>;
> +        gpio-controller;
> +        #gpio-cells = <2>;
> +        snps,nr-gpios = <8>;
> +      };
> +    };
> +...

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/4] gpio: dwapb: Use optional-clocks interface for APB ref-clocks
  2020-03-06 13:24 ` [PATCH 3/4] gpio: dwapb: Use optional-clocks interface for APB ref-clocks Sergey.Semin
@ 2020-03-12 13:53   ` Linus Walleij
  0 siblings, 0 replies; 12+ messages in thread
From: Linus Walleij @ 2020-03-12 13:53 UTC (permalink / raw)
  To: Sergey.Semin
  Cc: Hoan Tran, Bartosz Golaszewski, Serge Semin, Alexey Malahov,
	Thomas Bogendoerfer, Paul Burton, Ralf Baechle,
	open list:GPIO SUBSYSTEM, linux-kernel

Hi Sergey,

thanks for your patch!

On Fri, Mar 6, 2020 at 2:25 PM <Sergey.Semin@baikalelectronics.ru> wrote:

>         /* Optional bus clock */
> -       gpio->clk = devm_clk_get(&pdev->dev, "bus");
> -       if (!IS_ERR(gpio->clk)) {
> -               err = clk_prepare_enable(gpio->clk);
> -               if (err) {
> -                       dev_info(&pdev->dev, "Cannot enable clock\n");
> -                       return err;
> -               }
> +       gpio->clk = devm_clk_get_optional(&pdev->dev, "bus");
> +       if (IS_ERR(gpio->clk)) {
> +               dev_info(&pdev->dev, "Cannot get APB clock\n");

Turn this into dev_err() while you're at it.

> +       err = clk_prepare_enable(gpio->clk);
> +       if (err) {
> +               dev_info(&pdev->dev, "Cannot enable APB clock\n");

Also this.

With those changes:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 4/4] gpio: dwapb: Add debounce reference clock support
  2020-03-06 13:24 ` [PATCH 4/4] gpio: dwapb: Add debounce reference clock support Sergey.Semin
@ 2020-03-12 13:55   ` Linus Walleij
  2020-03-12 13:56     ` Linus Walleij
  0 siblings, 1 reply; 12+ messages in thread
From: Linus Walleij @ 2020-03-12 13:55 UTC (permalink / raw)
  To: Sergey.Semin
  Cc: Hoan Tran, Bartosz Golaszewski, Serge Semin, Alexey Malahov,
	Thomas Bogendoerfer, Paul Burton, Ralf Baechle,
	open list:GPIO SUBSYSTEM, linux-kernel

Hi Sergey,

thanks for your patch!

On Fri, Mar 6, 2020 at 2:25 PM <Sergey.Semin@baikalelectronics.ru> wrote:

> +       err = devm_clk_bulk_get_optional(&pdev->dev, DWAPB_NR_CLOCKS,
> +                                        gpio->clks);
> +       if (err) {
> +               dev_info(&pdev->dev, "Cannot get APB/Debounce clocks\n");

dev_err() again.

> +       err = clk_bulk_prepare_enable(DWAPB_NR_CLOCKS, gpio->clks);
>         if (err) {
> -               dev_info(&pdev->dev, "Cannot enable APB clock\n");
> +               dev_info(&pdev->dev, "Cannot enable APB/Debounce clocks\n");

dev_err() again.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 4/4] gpio: dwapb: Add debounce reference clock support
  2020-03-12 13:55   ` Linus Walleij
@ 2020-03-12 13:56     ` Linus Walleij
  0 siblings, 0 replies; 12+ messages in thread
From: Linus Walleij @ 2020-03-12 13:56 UTC (permalink / raw)
  To: Sergey.Semin
  Cc: Hoan Tran, Bartosz Golaszewski, Serge Semin, Alexey Malahov,
	Thomas Bogendoerfer, Paul Burton, Ralf Baechle,
	open list:GPIO SUBSYSTEM, linux-kernel

On Thu, Mar 12, 2020 at 2:55 PM Linus Walleij <linus.walleij@linaro.org> wrote:

> Hi Sergey,
>
> thanks for your patch!
>
> On Fri, Mar 6, 2020 at 2:25 PM <Sergey.Semin@baikalelectronics.ru> wrote:
>
> > +       err = devm_clk_bulk_get_optional(&pdev->dev, DWAPB_NR_CLOCKS,
> > +                                        gpio->clks);
> > +       if (err) {
> > +               dev_info(&pdev->dev, "Cannot get APB/Debounce clocks\n");
>
> dev_err() again.
>
> > +       err = clk_bulk_prepare_enable(DWAPB_NR_CLOCKS, gpio->clks);
> >         if (err) {
> > -               dev_info(&pdev->dev, "Cannot enable APB clock\n");
> > +               dev_info(&pdev->dev, "Cannot enable APB/Debounce clocks\n");
>
> dev_err() again.

With that:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/4] dt-bindings: gpio: Add DW GPIO debounce clocks bindings
  2020-03-06 13:24 ` [PATCH 2/4] dt-bindings: gpio: Add DW GPIO debounce clocks bindings Sergey.Semin
@ 2020-03-12 21:44   ` Rob Herring
  0 siblings, 0 replies; 12+ messages in thread
From: Rob Herring @ 2020-03-12 21:44 UTC (permalink / raw)
  To: Sergey.Semin
  Cc: Linus Walleij, Bartosz Golaszewski, Mark Rutland, Serge Semin,
	Serge Semin, Alexey Malahov, Thomas Bogendoerfer, Paul Burton,
	Ralf Baechle, linux-gpio, devicetree, linux-kernel

On Fri, 6 Mar 2020 16:24:46 +0300, <Sergey.Semin@baikalelectronics.ru> wrote:
> From: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> 
> Port A of the DW GPIO controller may optionally have a debounce
> logic enabled if it was synthesized with corresponding functionality
> enabled. In this case a dedicated reference clocks should be provided
> to the node with "db" clock-names.
> 
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> Signed-off-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> Cc: Paul Burton <paulburton@kernel.org>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> ---
>  Documentation/devicetree/bindings/gpio/snps,dw-apb-gpio.yaml | 4 ++++
>  1 file changed, 4 insertions(+)
> 

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/4] dt-bindings: gpio: Replace DW APB GPIO legacy bindings with YAML-based one
  2020-03-12 13:47   ` Rob Herring
@ 2020-03-13 18:40     ` Sergey Semin
  2020-03-13 18:53       ` Rob Herring
  0 siblings, 1 reply; 12+ messages in thread
From: Sergey Semin @ 2020-03-13 18:40 UTC (permalink / raw)
  To: Rob Herring
  Cc: Linus Walleij, Bartosz Golaszewski, Mark Rutland, Hoan Tran,
	Alexey Malahov, Thomas Bogendoerfer, Paul Burton, Ralf Baechle,
	open list:GPIO SUBSYSTEM, devicetree, linux-kernel

On Thu, Mar 12, 2020 at 08:47:56AM -0500, Rob Herring wrote:
> On Fri, Mar 6, 2020 at 7:25 AM <Sergey.Semin@baikalelectronics.ru> wrote:
> >
> 
> Subject is kind of long and wordy. Perhaps:
> 
> dt-bindings: gpio: Convert snps,dw-apb-gpio to DT schema
> 

Ok. I'll also do this for all similar patches in another patchsets.

> > From: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> >
> > Modern device tree bindings are supposed to be created as YAML-files
> > in accordance with dt-schema. This commit replaces Synopsys DW GPIO
> > legacy bare text bindings with YAML file. As before the bindings file
> > states that the corresponding dts node is supposed to be compatible
> > with generic DW I2C controller indicated by the "snps,dw-apb-gpio"
> > compatible string and provide a mandatory registers memory range.
> > It may also have an optional clocks and resets phandle references.
> >
> > There must be specified at least one subnode with
> > "snps,dw-apb-gpio-port" compatible string indicating the GPIO port,
> > which would actually export the GPIO controller functionality. Such
> > nodes should have traditional GPIO controller properties together
> > with optional interrupt-controller attributes if the corresponding
> > controller was synthesized to detected and report the input values
> > change to the parental IRQ controller.
> >
> > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > Signed-off-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> > Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> > Cc: Paul Burton <paulburton@kernel.org>
> > Cc: Ralf Baechle <ralf@linux-mips.org>
> >
> > ---
> >
> > Synopsis DesignWare APB SSI controller has a bindings property
> > "snps,nr-gpios" of numeric type, which means the number of GPIO pins
> > exported by the corresponding controller port. There is also a generic
> > pattern-property "*-gpios", which corresponds to a GPIOs array. As you
> > can see the GPIOs array property wildcard matches the vendor-specific
> > property "snps,nr-gpios" property while having an incompatible type.
> > Due to this the DW APB GPIO dts-nodes evaluation will report the
> > following error:
> >
> > snps,nr-gpios:0:0: 8 is not valid under any of the given schemas (Possible causes of the failure):
> > snps,nr-gpios:0:0: missing phandle tag in 8
> >
> > I didn't manage to fix the problem by redefining the property schema (this
> > might be impossible anyway). In my opinion the best way to solve it would be
> > to change the DW APB SSI Controller bindings so the driver would accept the
> > standard "ngpios" property for the same purpose. But in this case we would have
> > to alter all the dts files currently having the "snps,dw-apb-ssi" compatible
> > nodes (it's a lot). I know the bindings modifications aren't that much welcome
> > in the kernel community and there are good reasons why. So what do you think
> > would be the better way to fix the problem with the property types collision?
> 
> Does this change (to dt-schema) work for you?
> 
> diff --git a/schemas/gpio/gpio.yaml b/schemas/gpio/gpio.yaml
> index 1d9c109f9791..d1c08ccfdc1a 100644
> --- a/schemas/gpio/gpio.yaml
> +++ b/schemas/gpio/gpio.yaml
> @@ -34,7 +34,7 @@ properties:
>        - $ref: "/schemas/types.yaml#/definitions/phandle-array"
> 
>  patternProperties:
> -  ".*-gpios?$":
> +  "(?<!,nr)-gpios?$":
>      $ref: "/schemas/types.yaml#/definitions/phandle-array"
>    "^gpios$":
>      $ref: "/schemas/types.yaml#/definitions/phandle-array"
> 

It partly fixes the problem. There is meta-schems/gpios.yaml ,
which also has a rule for the properties with "-gpios" suffix. So yours
alteration together with the next one shall fix the problem completely:

--- a/meta-schemas/gpios.yaml	2020-03-13 20:20:10.072900019 +0300
+++ b/meta-schemas/gpios.yaml	2020-03-13 20:20:16.000953216 +0300
@@ -19,9 +19,7 @@
     $ref: "cell.yaml#array"

 patternProperties:
-  '.*-gpio$':
-    $ref: "cell.yaml#array"
-  '.*-gpios$':
+  '(?<!,nr)-gpios?$':
     $ref: "cell.yaml#array"

 dependencies:

Regarding the generic schemas/gpio/gpio.yaml . Do you think I should be
also allOf-ing it in my DT schema?

> 
> > ---
> >  .../bindings/gpio/snps,dw-apb-gpio.yaml       | 136 ++++++++++++++++++
> >  .../bindings/gpio/snps-dwapb-gpio.txt         |  65 ---------
> >  2 files changed, 136 insertions(+), 65 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/gpio/snps,dw-apb-gpio.yaml
> >  delete mode 100644 Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt
> >
> > diff --git a/Documentation/devicetree/bindings/gpio/snps,dw-apb-gpio.yaml b/Documentation/devicetree/bindings/gpio/snps,dw-apb-gpio.yaml
> > new file mode 100644
> > index 000000000000..d9bc12e9e515
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/gpio/snps,dw-apb-gpio.yaml
> > @@ -0,0 +1,136 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> 
> Do you have rights to add BSD?

I asked my superviser regarding this. They don't object against using
two licenses for dt-schema files. So I'll update all the schemas I
submit to have dual license header.

If you referring to the original plain text dt bindings file. I don't
really know under what license it was submitted.

> 
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/gpio/snps,dw-apb-gpio.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Synopsys DesignWare APB GPIO controller
> > +
> > +description: |
> > +  Synopsys DesignWare GPIO controllers have a configurable number of ports,
> > +  each of which are intended to be represented as child nodes with the generic
> > +  GPIO-controller properties as desribed in this bindings file.
> > +
> > +maintainers:
> > +  - Hoan Tran <hoan@os.amperecomputing.com>
> > +
> > +properties:
> > +  $nodename:
> > +    pattern: "^gpio@[0-9a-fA-F]+$"
> 
> Lowercase hex for unit-addresses.
> 

Ok. I'll also make sure the lowercased unit-addresses are used in the
rest of the patchsets.

> > +
> > +  compatible:
> > +    const: snps,dw-apb-gpio
> > +
> > +  "#address-cells":
> > +    const: 1
> > +
> > +  "#size-cells":
> > +    const: 0
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    items:
> > +      - description: APB interface clock source
> > +
> > +  clock-names:
> > +    items:
> > +      - const: bus
> > +
> > +  resets:
> > +    maxItems: 1
> > +
> > +patternProperties:
> > +  "^.*@[0-9a-fA-F]+$":
> 
> Shouldn't it be "^gpio@..." And unit-addresses should be lowercase.

Sub-nodes define the GPIO controller ports, so "^gpio-port@" would be
better. But some available dts-files declares these sub-nodes as
"^gpio-controller@". So in order to have them also compatible we have no
other choice but either to define the property name as
"^gpio-(port|controller)@..." or leave it as suggested in my patch. What
do you think? Anyway to be honest I don't really understand why at all do
we need to have the limitations applied on $nodename . It's just a name
of the node and doesn't participate in any device-driver bindings
or in anything else meaningful.

> 
> > +    type: object
> > +    properties:
> > +      compatible:
> > +        const: snps,dw-apb-gpio-port
> > +
> > +      reg:
> > +        maxItems: 1
> > +
> > +      gpio-controller: true
> > +
> > +      '#gpio-cells':
> > +        const: 2
> > +
> > +      snps,nr-gpios:
> > +        $ref: /schemas/types.yaml#/definitions/uint32
> > +        description: The number of GPIO pins exported by the port.
> > +        default: 32
> > +        minimum: 1
> > +        maximum: 32
> > +
> > +      interrupts:
> > +        description: |
> > +          The interrupts to the parent controller raised when GPIOs generate
> > +          the interrupts. If the controller provides one combined interrupt
> > +          for all GPIOs, specify a single interrupt. If the controller provides
> > +          one interrupt for each GPIO, provide a list of interrupts that
> > +          correspond to each of the GPIO pins.
> > +        minItems: 1
> > +        maxItems: 32
> > +
> > +      interrupts-extended:
> 
> Drop this. It gets added by the tools automatically.
> 

Ok. I'll remove the whole property from the dt schema. But don't you think
since there is "additionalProperties: false" I should have at least left the
"interrupts-extended: true" declaration?

> > +        description: |
> > +          When specifying multiple interrupts, if any are unconnected, use
> > +          this property to specify the interrupts and set the interrupt
> > +          controller handle for unused interrupts to 0.
> > +        minItems: 1
> > +        maxItems: 32
> > +
> > +      interrupt-controller: true
> > +
> > +      '#interrupt-cells':
> > +        const: 2
> > +
> > +    required:
> > +      - compatible
> > +      - reg
> > +      - gpio-controller
> > +      - '#gpio-cells'
> > +
> > +    dependencies:
> > +      interrupt-controller: [ interrupts ]
> > +
> > +    additionalProperties: false
> > +
> > +additionalProperties: false
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - "#address-cells"
> > +  - "#size-cells"
> > +
> > +examples:
> > +  - |
> > +    gpio: gpio@20000 {
> > +      compatible = "snps,dw-apb-gpio";
> > +      reg = <0x20000 0x1000>;
> > +      #address-cells = <1>;
> > +      #size-cells = <0>;
> > +
> > +      porta: gpio@0 {
> > +        compatible = "snps,dw-apb-gpio-port";
> > +        reg = <0>;
> > +        gpio-controller;
> > +        #gpio-cells = <2>;
> > +        snps,nr-gpios = <8>;
> > +        interrupt-controller;
> > +        #interrupt-cells = <2>;
> > +        interrupt-parent = <&vic1>;
> > +        interrupts = <0>;
> > +      };
> > +
> > +      portb: gpio@1 {
> > +        compatible = "snps,dw-apb-gpio-port";
> > +        reg = <1>;
> > +        gpio-controller;
> > +        #gpio-cells = <2>;
> > +        snps,nr-gpios = <8>;
> > +      };
> > +    };
> > +...

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/4] dt-bindings: gpio: Replace DW APB GPIO legacy bindings with YAML-based one
  2020-03-13 18:40     ` Sergey Semin
@ 2020-03-13 18:53       ` Rob Herring
  0 siblings, 0 replies; 12+ messages in thread
From: Rob Herring @ 2020-03-13 18:53 UTC (permalink / raw)
  To: Sergey Semin
  Cc: Linus Walleij, Bartosz Golaszewski, Mark Rutland, Hoan Tran,
	Alexey Malahov, Thomas Bogendoerfer, Paul Burton, Ralf Baechle,
	open list:GPIO SUBSYSTEM, devicetree, linux-kernel

On Fri, Mar 13, 2020 at 1:40 PM Sergey Semin
<Sergey.Semin@baikalelectronics.ru> wrote:
>
> On Thu, Mar 12, 2020 at 08:47:56AM -0500, Rob Herring wrote:
> > On Fri, Mar 6, 2020 at 7:25 AM <Sergey.Semin@baikalelectronics.ru> wrote:
> > >
> >
> > Subject is kind of long and wordy. Perhaps:
> >
> > dt-bindings: gpio: Convert snps,dw-apb-gpio to DT schema
> >
>
> Ok. I'll also do this for all similar patches in another patchsets.
>
> > > From: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > >
> > > Modern device tree bindings are supposed to be created as YAML-files
> > > in accordance with dt-schema. This commit replaces Synopsys DW GPIO
> > > legacy bare text bindings with YAML file. As before the bindings file
> > > states that the corresponding dts node is supposed to be compatible
> > > with generic DW I2C controller indicated by the "snps,dw-apb-gpio"
> > > compatible string and provide a mandatory registers memory range.
> > > It may also have an optional clocks and resets phandle references.
> > >
> > > There must be specified at least one subnode with
> > > "snps,dw-apb-gpio-port" compatible string indicating the GPIO port,
> > > which would actually export the GPIO controller functionality. Such
> > > nodes should have traditional GPIO controller properties together
> > > with optional interrupt-controller attributes if the corresponding
> > > controller was synthesized to detected and report the input values
> > > change to the parental IRQ controller.
> > >
> > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > > Signed-off-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> > > Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> > > Cc: Paul Burton <paulburton@kernel.org>
> > > Cc: Ralf Baechle <ralf@linux-mips.org>
> > >
> > > ---
> > >
> > > Synopsis DesignWare APB SSI controller has a bindings property
> > > "snps,nr-gpios" of numeric type, which means the number of GPIO pins
> > > exported by the corresponding controller port. There is also a generic
> > > pattern-property "*-gpios", which corresponds to a GPIOs array. As you
> > > can see the GPIOs array property wildcard matches the vendor-specific
> > > property "snps,nr-gpios" property while having an incompatible type.
> > > Due to this the DW APB GPIO dts-nodes evaluation will report the
> > > following error:
> > >
> > > snps,nr-gpios:0:0: 8 is not valid under any of the given schemas (Possible causes of the failure):
> > > snps,nr-gpios:0:0: missing phandle tag in 8
> > >
> > > I didn't manage to fix the problem by redefining the property schema (this
> > > might be impossible anyway). In my opinion the best way to solve it would be
> > > to change the DW APB SSI Controller bindings so the driver would accept the
> > > standard "ngpios" property for the same purpose. But in this case we would have
> > > to alter all the dts files currently having the "snps,dw-apb-ssi" compatible
> > > nodes (it's a lot). I know the bindings modifications aren't that much welcome
> > > in the kernel community and there are good reasons why. So what do you think
> > > would be the better way to fix the problem with the property types collision?
> >
> > Does this change (to dt-schema) work for you?
> >
> > diff --git a/schemas/gpio/gpio.yaml b/schemas/gpio/gpio.yaml
> > index 1d9c109f9791..d1c08ccfdc1a 100644
> > --- a/schemas/gpio/gpio.yaml
> > +++ b/schemas/gpio/gpio.yaml
> > @@ -34,7 +34,7 @@ properties:
> >        - $ref: "/schemas/types.yaml#/definitions/phandle-array"
> >
> >  patternProperties:
> > -  ".*-gpios?$":
> > +  "(?<!,nr)-gpios?$":
> >      $ref: "/schemas/types.yaml#/definitions/phandle-array"
> >    "^gpios$":
> >      $ref: "/schemas/types.yaml#/definitions/phandle-array"
> >
>
> It partly fixes the problem. There is meta-schems/gpios.yaml ,
> which also has a rule for the properties with "-gpios" suffix. So yours
> alteration together with the next one shall fix the problem completely:

Oh right. I'll commit a fix for both.

>
> --- a/meta-schemas/gpios.yaml   2020-03-13 20:20:10.072900019 +0300
> +++ b/meta-schemas/gpios.yaml   2020-03-13 20:20:16.000953216 +0300
> @@ -19,9 +19,7 @@
>      $ref: "cell.yaml#array"
>
>  patternProperties:
> -  '.*-gpio$':
> -    $ref: "cell.yaml#array"
> -  '.*-gpios$':
> +  '(?<!,nr)-gpios?$':
>      $ref: "cell.yaml#array"
>
>  dependencies:
>
> Regarding the generic schemas/gpio/gpio.yaml . Do you think I should be
> also allOf-ing it in my DT schema?

Only if you want to check everything twice. Most of the core schemas
always get applied. The exception is generally bus controllers like
SPI, I2C, etc.

Rob

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2020-03-13 18:53 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20200306132448.13917-1-Sergey.Semin@baikalelectronics.ru>
2020-03-06 13:24 ` [PATCH 1/4] dt-bindings: gpio: Replace DW APB GPIO legacy bindings with YAML-based one Sergey.Semin
2020-03-12 13:47   ` Rob Herring
2020-03-13 18:40     ` Sergey Semin
2020-03-13 18:53       ` Rob Herring
2020-03-06 13:24 ` [PATCH 2/4] dt-bindings: gpio: Add DW GPIO debounce clocks bindings Sergey.Semin
2020-03-12 21:44   ` Rob Herring
2020-03-06 13:24 ` [PATCH 3/4] gpio: dwapb: Use optional-clocks interface for APB ref-clocks Sergey.Semin
2020-03-12 13:53   ` Linus Walleij
2020-03-06 13:24 ` [PATCH 4/4] gpio: dwapb: Add debounce reference clock support Sergey.Semin
2020-03-12 13:55   ` Linus Walleij
2020-03-12 13:56     ` Linus Walleij
2020-03-10  0:23 ` [PATCH 0/4] gpio: dwapb: Fix reference clocks usage Sergey Semin

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.