[PATCH v2 0/2] Introduce new optional property to mark port as write only.

[PATCH v2 0/2] Introduce new optional property to mark port as write only.

[Niall Leonard via B4 Submission Endpoint]

Some electronics do not allow the data regsister to be read without
corrupting the existing data on the port. It a quirk of the board
design.
e.g. I have a couple of boards where the electronics engineer decided 
to only use the chip select line, so no read/write signal is connected. 
This means that reading the address activates the chip select and drives 
the contents of the data bus to the port.This makes it impossible to 
read the last data written to the port.

This solution is to use the existing shadow data register 'bgpio_data'.
It  can be used to return the last value written to the port by the read 
operation.
 
This is enabled for a particular port using a new flag and a new
device tree property "no-input" to allow it to be selected on a board by 
board basis. This means it will only effect hardware that requests it.

Signed-off-by: Niall Leonard <nl250060@ncr.com>
---
Changes in v2:
- Description of change updated to clarify why it is needed.
- Patches squashed as per request during review.
- wd,mbl-gpio bindings updated to yaml format.
- Link to v1: https://lore.kernel.org/r/20230126-gpio-mmio-fix-v1-0-8a20ce0e8275@ncr.com

---
Niall Leonard (2):
      dt-bindings: improve wb,mbl-gpio binding documentation.
      gpio: mmio: Use new flag BGPIOF_NO_INPUT.

 .../devicetree/bindings/gpio/wd,mbl-gpio.txt       | 38 -----------
 .../devicetree/bindings/gpio/wd,mbl-gpio.yaml      | 78 ++++++++++++++++++++++
 drivers/gpio/gpio-mmio.c                           | 24 ++++++-
 include/linux/gpio/driver.h                        |  1 +
 4 files changed, 100 insertions(+), 41 deletions(-)
---
base-commit: 1b929c02afd37871d5afb9d498426f83432e71c2
change-id: 20230126-gpio-mmio-fix-1a69d03ec9e7

Best regards,
-- 
Niall Leonard <nl250060@ncr.com>

[PATCH v2 0/2] Introduce new optional property to mark port as write only.

[Niall Leonard]

Some electronics do not allow the data regsister to be read without
corrupting the existing data on the port. It a quirk of the board
design.
e.g. I have a couple of boards where the electronics engineer decided 
to only use the chip select line, so no read/write signal is connected. 
This means that reading the address activates the chip select and drives 
the contents of the data bus to the port.This makes it impossible to 
read the last data written to the port.

This solution is to use the existing shadow data register 'bgpio_data'.
It  can be used to return the last value written to the port by the read 
operation.
 
This is enabled for a particular port using a new flag and a new
device tree property "no-input" to allow it to be selected on a board by 
board basis. This means it will only effect hardware that requests it.

Signed-off-by: Niall Leonard <nl250060@ncr.com>
---
Changes in v2:
- Description of change updated to clarify why it is needed.
- Patches squashed as per request during review.
- wd,mbl-gpio bindings updated to yaml format.
- Link to v1: https://lore.kernel.org/r/20230126-gpio-mmio-fix-v1-0-8a20ce0e8275@ncr.com

---
Niall Leonard (2):
      dt-bindings: improve wb,mbl-gpio binding documentation.
      gpio: mmio: Use new flag BGPIOF_NO_INPUT.

 .../devicetree/bindings/gpio/wd,mbl-gpio.txt       | 38 -----------
 .../devicetree/bindings/gpio/wd,mbl-gpio.yaml      | 78 ++++++++++++++++++++++
 drivers/gpio/gpio-mmio.c                           | 24 ++++++-
 include/linux/gpio/driver.h                        |  1 +
 4 files changed, 100 insertions(+), 41 deletions(-)
---
base-commit: 1b929c02afd37871d5afb9d498426f83432e71c2
change-id: 20230126-gpio-mmio-fix-1a69d03ec9e7

Best regards,
-- 
Niall Leonard <nl250060@ncr.com>

[PATCH v2 1/2] dt-bindings: improve wb,mbl-gpio binding documentation.

[Niall Leonard via B4 Submission Endpoint]

From: Niall Leonard <nl250060@ncr.com>

Convert existing wd,mbl-gpio binding documentation to YAML and add
new optional propery "no-input".

Signed-off-by: Niall Leonard <nl250060@ncr.com>
---
 .../devicetree/bindings/gpio/wd,mbl-gpio.txt       | 38 -----------
 .../devicetree/bindings/gpio/wd,mbl-gpio.yaml      | 78 ++++++++++++++++++++++
 2 files changed, 78 insertions(+), 38 deletions(-)

diff --git a/Documentation/devicetree/bindings/gpio/wd,mbl-gpio.txt b/Documentation/devicetree/bindings/gpio/wd,mbl-gpio.txt
deleted file mode 100644
index 038c3a6a1f4d..000000000000
--- a/Documentation/devicetree/bindings/gpio/wd,mbl-gpio.txt
+++ /dev/null
@@ -1,38 +0,0 @@
-Bindings for the Western Digital's MyBook Live memory-mapped GPIO controllers.
-
-The Western Digital MyBook Live has two memory-mapped GPIO controllers.
-Both GPIO controller only have a single 8-bit data register, where GPIO
-state can be read and/or written.
-
-Required properties:
-	- compatible: should be "wd,mbl-gpio"
-	- reg-names: must contain
-		"dat" - data register
-	- reg: address + size pairs describing the GPIO register sets;
-		order must correspond with the order of entries in reg-names
-	- #gpio-cells: must be set to 2. The first cell is the pin number and
-			the second cell is used to specify the gpio polarity:
-			0 = active high
-			1 = active low
-	- gpio-controller: Marks the device node as a gpio controller.
-
-Optional properties:
-	- no-output: GPIOs are read-only.
-
-Examples:
-	gpio0: gpio0@e0000000 {
-		compatible = "wd,mbl-gpio";
-		reg-names = "dat";
-		reg = <0xe0000000 0x1>;
-		#gpio-cells = <2>;
-		gpio-controller;
-	};
-
-	gpio1: gpio1@e0100000 {
-		compatible = "wd,mbl-gpio";
-		reg-names = "dat";
-		reg = <0xe0100000 0x1>;
-		#gpio-cells = <2>;
-		gpio-controller;
-		no-output;
-	};
diff --git a/Documentation/devicetree/bindings/gpio/wd,mbl-gpio.yaml b/Documentation/devicetree/bindings/gpio/wd,mbl-gpio.yaml
new file mode 100644
index 000000000000..d1c72a42c5bc
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/wd,mbl-gpio.yaml
@@ -0,0 +1,78 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpio/wd,mbl-gpio.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Western Digital's MyBook Live memory-mapped GPIO controllers.
+
+maintainers:
+  - Niall Leonard <nl250060@ncr.com>
+
+description: |+
+  Bindings for the Western Digital's MyBook Live memory-mapped GPIO controllers.
+
+  The Western Digital MyBook Live has two memory-mapped GPIO controllers.
+  Both GPIO controller only have a single 8-bit data register, where GPIO
+  state can be read and/or written.
+
+properties:
+  compatible:
+    enum:
+      - wd,mbl-gpio
+
+  reg-names:
+    items:
+      - const: dat
+
+  reg:
+    maxItems: 1
+
+  "#gpio-cells":
+    const: 2
+
+  gpio-controller: true
+
+  no-output:
+    description: GPIOs are read-only.
+
+  no-input:
+    description: GPIOs are write-only.
+
+required:
+  - compatible
+  - reg-names
+  - reg
+  - '#gpio-cells'
+  - gpio-controller
+
+additionalProperties: false
+
+examples:
+  - |
+    gpio0: gpio0@e0000000 {
+        compatible = "wd,mbl-gpio";
+        reg-names = "dat";
+        reg = <0xe0000000 0x1>;
+        #gpio-cells = <2>;
+        gpio-controller;
+    };
+
+    gpio1: gpio1@e0100000 {
+        compatible = "wd,mbl-gpio";
+        reg-names = "dat";
+        reg = <0xe0100000 0x1>;
+        #gpio-cells = <2>;
+        gpio-controller;
+        no-output;
+    };
+
+    gpio2: gpio1@e0200000 {
+        compatible = "wd,mbl-gpio";
+        reg-names = "dat";
+        reg = <0xe0200000 0x1>;
+        #gpio-cells = <2>;
+        gpio-controller;
+        no-input;
+    };
+

-- 
2.34.1

[PATCH v2 1/2] dt-bindings: improve wb,mbl-gpio binding documentation.

[Niall Leonard]

Convert existing wd,mbl-gpio binding documentation to YAML and add
new optional propery "no-input".

Signed-off-by: Niall Leonard <nl250060@ncr.com>
---
 .../devicetree/bindings/gpio/wd,mbl-gpio.txt       | 38 -----------
 .../devicetree/bindings/gpio/wd,mbl-gpio.yaml      | 78 ++++++++++++++++++++++
 2 files changed, 78 insertions(+), 38 deletions(-)

diff --git a/Documentation/devicetree/bindings/gpio/wd,mbl-gpio.txt b/Documentation/devicetree/bindings/gpio/wd,mbl-gpio.txt
deleted file mode 100644
index 038c3a6a1f4d..000000000000
--- a/Documentation/devicetree/bindings/gpio/wd,mbl-gpio.txt
+++ /dev/null
@@ -1,38 +0,0 @@
-Bindings for the Western Digital's MyBook Live memory-mapped GPIO controllers.
-
-The Western Digital MyBook Live has two memory-mapped GPIO controllers.
-Both GPIO controller only have a single 8-bit data register, where GPIO
-state can be read and/or written.
-
-Required properties:
-	- compatible: should be "wd,mbl-gpio"
-	- reg-names: must contain
-		"dat" - data register
-	- reg: address + size pairs describing the GPIO register sets;
-		order must correspond with the order of entries in reg-names
-	- #gpio-cells: must be set to 2. The first cell is the pin number and
-			the second cell is used to specify the gpio polarity:
-			0 = active high
-			1 = active low
-	- gpio-controller: Marks the device node as a gpio controller.
-
-Optional properties:
-	- no-output: GPIOs are read-only.
-
-Examples:
-	gpio0: gpio0@e0000000 {
-		compatible = "wd,mbl-gpio";
-		reg-names = "dat";
-		reg = <0xe0000000 0x1>;
-		#gpio-cells = <2>;
-		gpio-controller;
-	};
-
-	gpio1: gpio1@e0100000 {
-		compatible = "wd,mbl-gpio";
-		reg-names = "dat";
-		reg = <0xe0100000 0x1>;
-		#gpio-cells = <2>;
-		gpio-controller;
-		no-output;
-	};
diff --git a/Documentation/devicetree/bindings/gpio/wd,mbl-gpio.yaml b/Documentation/devicetree/bindings/gpio/wd,mbl-gpio.yaml
new file mode 100644
index 000000000000..d1c72a42c5bc
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/wd,mbl-gpio.yaml
@@ -0,0 +1,78 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpio/wd,mbl-gpio.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Western Digital's MyBook Live memory-mapped GPIO controllers.
+
+maintainers:
+  - Niall Leonard <nl250060@ncr.com>
+
+description: |+
+  Bindings for the Western Digital's MyBook Live memory-mapped GPIO controllers.
+
+  The Western Digital MyBook Live has two memory-mapped GPIO controllers.
+  Both GPIO controller only have a single 8-bit data register, where GPIO
+  state can be read and/or written.
+
+properties:
+  compatible:
+    enum:
+      - wd,mbl-gpio
+
+  reg-names:
+    items:
+      - const: dat
+
+  reg:
+    maxItems: 1
+
+  "#gpio-cells":
+    const: 2
+
+  gpio-controller: true
+
+  no-output:
+    description: GPIOs are read-only.
+
+  no-input:
+    description: GPIOs are write-only.
+
+required:
+  - compatible
+  - reg-names
+  - reg
+  - '#gpio-cells'
+  - gpio-controller
+
+additionalProperties: false
+
+examples:
+  - |
+    gpio0: gpio0@e0000000 {
+        compatible = "wd,mbl-gpio";
+        reg-names = "dat";
+        reg = <0xe0000000 0x1>;
+        #gpio-cells = <2>;
+        gpio-controller;
+    };
+
+    gpio1: gpio1@e0100000 {
+        compatible = "wd,mbl-gpio";
+        reg-names = "dat";
+        reg = <0xe0100000 0x1>;
+        #gpio-cells = <2>;
+        gpio-controller;
+        no-output;
+    };
+
+    gpio2: gpio1@e0200000 {
+        compatible = "wd,mbl-gpio";
+        reg-names = "dat";
+        reg = <0xe0200000 0x1>;
+        #gpio-cells = <2>;
+        gpio-controller;
+        no-input;
+    };
+

-- 
2.34.1

[PATCH v2 2/2] gpio: mmio: Use new flag BGPIOF_NO_INPUT.

[Niall Leonard via B4 Submission Endpoint]

From: Niall Leonard <nl250060@ncr.com>

Add new flag BGPIOF_NO_INPUT to header file.
Use the existing shadow data register 'bgpio_data' to allow
the last written value to be returned by the read operation
when BGPIOF_NO_INPUT flag is set.
Ensure this change only applies to the specific binding "wd,mbl-gpio".

Signed-off-by: Niall Leonard <nl250060@ncr.com>
---
 drivers/gpio/gpio-mmio.c    | 24 +++++++++++++++++++++---
 include/linux/gpio/driver.h |  1 +
 2 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpio-mmio.c b/drivers/gpio/gpio-mmio.c
index d9dff3dc92ae..9939fdbf6345 100644
--- a/drivers/gpio/gpio-mmio.c
+++ b/drivers/gpio/gpio-mmio.c
@@ -164,6 +164,11 @@ static int bgpio_get_set_multiple(struct gpio_chip *gc, unsigned long *mask,
 	return 0;
 }
 
+static int bgpio_get_shadow(struct gpio_chip *gc, unsigned int gpio)
+{
+	return !!(gc->bgpio_data & bgpio_line2mask(gc, gpio));
+}
+
 static int bgpio_get(struct gpio_chip *gc, unsigned int gpio)
 {
 	return !!(gc->read_reg(gc->reg_dat) & bgpio_line2mask(gc, gpio));
@@ -526,7 +531,10 @@ static int bgpio_setup_io(struct gpio_chip *gc,
 		 * reading each line individually in that fringe case.
 		 */
 	} else {
-		gc->get = bgpio_get;
+		if (flags & BGPIOF_NO_INPUT)
+			gc->get = bgpio_get_shadow;
+		else
+			gc->get = bgpio_get;
 		if (gc->be_bits)
 			gc->get_multiple = bgpio_get_multiple_be;
 		else
@@ -630,7 +638,11 @@ int bgpio_init(struct gpio_chip *gc, struct device *dev,
 	if (ret)
 		return ret;
 
-	gc->bgpio_data = gc->read_reg(gc->reg_dat);
+	if (flags & BGPIOF_NO_INPUT)
+		gc->bgpio_data = 0;
+	else
+		gc->bgpio_data = gc->read_reg(gc->reg_dat);
+
 	if (gc->set == bgpio_set_set &&
 			!(flags & BGPIOF_UNREADABLE_REG_SET))
 		gc->bgpio_data = gc->read_reg(gc->reg_set);
@@ -694,8 +706,9 @@ static struct bgpio_pdata *bgpio_parse_dt(struct platform_device *pdev,
 					  unsigned long *flags)
 {
 	struct bgpio_pdata *pdata;
+	const struct of_device_id *of_id;
 
-	if (!of_match_device(bgpio_of_match, &pdev->dev))
+	if (!(of_id = of_match_device(bgpio_of_match, &pdev->dev)))
 		return NULL;
 
 	pdata = devm_kzalloc(&pdev->dev, sizeof(struct bgpio_pdata),
@@ -711,6 +724,11 @@ static struct bgpio_pdata *bgpio_parse_dt(struct platform_device *pdev,
 	if (of_property_read_bool(pdev->dev.of_node, "no-output"))
 		*flags |= BGPIOF_NO_OUTPUT;
 
+	if (!strcmp(of_id->compatible, "wd,mbl-gpio")) {
+		if (of_property_read_bool(pdev->dev.of_node, "no-input"))
+			*flags |= BGPIOF_NO_INPUT;
+	}
+
 	return pdata;
 }
 #else
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index 44783fc16125..e8e57310e3b8 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -682,6 +682,7 @@ int bgpio_init(struct gpio_chip *gc, struct device *dev,
 #define BGPIOF_READ_OUTPUT_REG_SET	BIT(4) /* reg_set stores output value */
 #define BGPIOF_NO_OUTPUT		BIT(5) /* only input */
 #define BGPIOF_NO_SET_ON_INPUT		BIT(6)
+#define BGPIOF_NO_INPUT				BIT(7)	/* only output */
 
 int gpiochip_irq_map(struct irq_domain *d, unsigned int irq,
 		     irq_hw_number_t hwirq);

-- 
2.34.1

[PATCH v2 2/2] gpio: mmio: Use new flag BGPIOF_NO_INPUT.

[Niall Leonard]

Add new flag BGPIOF_NO_INPUT to header file.
Use the existing shadow data register 'bgpio_data' to allow
the last written value to be returned by the read operation
when BGPIOF_NO_INPUT flag is set.
Ensure this change only applies to the specific binding "wd,mbl-gpio".

Signed-off-by: Niall Leonard <nl250060@ncr.com>
---
 drivers/gpio/gpio-mmio.c    | 24 +++++++++++++++++++++---
 include/linux/gpio/driver.h |  1 +
 2 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpio-mmio.c b/drivers/gpio/gpio-mmio.c
index d9dff3dc92ae..9939fdbf6345 100644
--- a/drivers/gpio/gpio-mmio.c
+++ b/drivers/gpio/gpio-mmio.c
@@ -164,6 +164,11 @@ static int bgpio_get_set_multiple(struct gpio_chip *gc, unsigned long *mask,
 	return 0;
 }
 
+static int bgpio_get_shadow(struct gpio_chip *gc, unsigned int gpio)
+{
+	return !!(gc->bgpio_data & bgpio_line2mask(gc, gpio));
+}
+
 static int bgpio_get(struct gpio_chip *gc, unsigned int gpio)
 {
 	return !!(gc->read_reg(gc->reg_dat) & bgpio_line2mask(gc, gpio));
@@ -526,7 +531,10 @@ static int bgpio_setup_io(struct gpio_chip *gc,
 		 * reading each line individually in that fringe case.
 		 */
 	} else {
-		gc->get = bgpio_get;
+		if (flags & BGPIOF_NO_INPUT)
+			gc->get = bgpio_get_shadow;
+		else
+			gc->get = bgpio_get;
 		if (gc->be_bits)
 			gc->get_multiple = bgpio_get_multiple_be;
 		else
@@ -630,7 +638,11 @@ int bgpio_init(struct gpio_chip *gc, struct device *dev,
 	if (ret)
 		return ret;
 
-	gc->bgpio_data = gc->read_reg(gc->reg_dat);
+	if (flags & BGPIOF_NO_INPUT)
+		gc->bgpio_data = 0;
+	else
+		gc->bgpio_data = gc->read_reg(gc->reg_dat);
+
 	if (gc->set == bgpio_set_set &&
 			!(flags & BGPIOF_UNREADABLE_REG_SET))
 		gc->bgpio_data = gc->read_reg(gc->reg_set);
@@ -694,8 +706,9 @@ static struct bgpio_pdata *bgpio_parse_dt(struct platform_device *pdev,
 					  unsigned long *flags)
 {
 	struct bgpio_pdata *pdata;
+	const struct of_device_id *of_id;
 
-	if (!of_match_device(bgpio_of_match, &pdev->dev))
+	if (!(of_id = of_match_device(bgpio_of_match, &pdev->dev)))
 		return NULL;
 
 	pdata = devm_kzalloc(&pdev->dev, sizeof(struct bgpio_pdata),
@@ -711,6 +724,11 @@ static struct bgpio_pdata *bgpio_parse_dt(struct platform_device *pdev,
 	if (of_property_read_bool(pdev->dev.of_node, "no-output"))
 		*flags |= BGPIOF_NO_OUTPUT;
 
+	if (!strcmp(of_id->compatible, "wd,mbl-gpio")) {
+		if (of_property_read_bool(pdev->dev.of_node, "no-input"))
+			*flags |= BGPIOF_NO_INPUT;
+	}
+
 	return pdata;
 }
 #else
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index 44783fc16125..e8e57310e3b8 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -682,6 +682,7 @@ int bgpio_init(struct gpio_chip *gc, struct device *dev,
 #define BGPIOF_READ_OUTPUT_REG_SET	BIT(4) /* reg_set stores output value */
 #define BGPIOF_NO_OUTPUT		BIT(5) /* only input */
 #define BGPIOF_NO_SET_ON_INPUT		BIT(6)
+#define BGPIOF_NO_INPUT				BIT(7)	/* only output */
 
 int gpiochip_irq_map(struct irq_domain *d, unsigned int irq,
 		     irq_hw_number_t hwirq);

-- 
2.34.1

Re: [PATCH v2 1/2] dt-bindings: improve wb,mbl-gpio binding documentation.

[Krzysztof Kozlowski]

On 31/01/2023 14:49, Niall Leonard via B4 Submission Endpoint wrote:
> From: Niall Leonard <nl250060@ncr.com>
> 
> Convert existing wd,mbl-gpio binding documentation to YAML and add
> new optional propery "no-input".

Subject: drop full stop

Use subject prefixes matching the subsystem (which you can get for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching).

Subject: improve is vague. Instead: convert to DT schema


> 
> Signed-off-by: Niall Leonard <nl250060@ncr.com>
> ---
>  .../devicetree/bindings/gpio/wd,mbl-gpio.txt       | 38 -----------
>  .../devicetree/bindings/gpio/wd,mbl-gpio.yaml      | 78 ++++++++++++++++++++++
>  2 files changed, 78 insertions(+), 38 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/gpio/wd,mbl-gpio.txt b/Documentation/devicetree/bindings/gpio/wd,mbl-gpio.txt
> deleted file mode 100644
> index 038c3a6a1f4d..000000000000
> --- a/Documentation/devicetree/bindings/gpio/wd,mbl-gpio.txt
> +++ /dev/null
> @@ -1,38 +0,0 @@
> -Bindings for the Western Digital's MyBook Live memory-mapped GPIO controllers.
> -
> -The Western Digital MyBook Live has two memory-mapped GPIO controllers.
> -Both GPIO controller only have a single 8-bit data register, where GPIO
> -state can be read and/or written.
> -
> -Required properties:
> -	- compatible: should be "wd,mbl-gpio"
> -	- reg-names: must contain
> -		"dat" - data register
> -	- reg: address + size pairs describing the GPIO register sets;
> -		order must correspond with the order of entries in reg-names
> -	- #gpio-cells: must be set to 2. The first cell is the pin number and
> -			the second cell is used to specify the gpio polarity:
> -			0 = active high
> -			1 = active low
> -	- gpio-controller: Marks the device node as a gpio controller.
> -
> -Optional properties:
> -	- no-output: GPIOs are read-only.
> -
> -Examples:
> -	gpio0: gpio0@e0000000 {
> -		compatible = "wd,mbl-gpio";
> -		reg-names = "dat";
> -		reg = <0xe0000000 0x1>;
> -		#gpio-cells = <2>;
> -		gpio-controller;
> -	};
> -
> -	gpio1: gpio1@e0100000 {
> -		compatible = "wd,mbl-gpio";
> -		reg-names = "dat";
> -		reg = <0xe0100000 0x1>;
> -		#gpio-cells = <2>;
> -		gpio-controller;
> -		no-output;
> -	};
> diff --git a/Documentation/devicetree/bindings/gpio/wd,mbl-gpio.yaml b/Documentation/devicetree/bindings/gpio/wd,mbl-gpio.yaml
> new file mode 100644
> index 000000000000..d1c72a42c5bc
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/wd,mbl-gpio.yaml
> @@ -0,0 +1,78 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/gpio/wd,mbl-gpio.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Western Digital's MyBook Live memory-mapped GPIO controllers.

Drop full stop.

> +
> +maintainers:
> +  - Niall Leonard <nl250060@ncr.com>
> +
> +description: |+
> +  Bindings for the Western Digital's MyBook Live memory-mapped GPIO controllers.

Drop "Bindings for". Actually drop entire line - it's redundant, you
repeat the title.

> +
> +  The Western Digital MyBook Live has two memory-mapped GPIO controllers.
> +  Both GPIO controller only have a single 8-bit data register, where GPIO
> +  state can be read and/or written.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - wd,mbl-gpio
> +
> +  reg-names:
> +    items:
> +      - const: dat
> +
> +  reg:
> +    maxItems: 1
> +
> +  "#gpio-cells":
> +    const: 2
> +
> +  gpio-controller: true
> +
> +  no-output:
> +    description: GPIOs are read-only.
> +
> +  no-input:
> +    description: GPIOs are write-only.


Split adding new property into separate patch. Each patch should do one
logical change. New feature is another logical change. Conversion is
that logical change (with any fixes needed for successful conversion,
but not with new features).


> +
> +required:
> +  - compatible
> +  - reg-names
> +  - reg
> +  - '#gpio-cells'

Use consistent quotes - either ' or "

> +  - gpio-controller
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    gpio0: gpio0@e0000000 {

gpio@

(0 is not correct name of node)

> +        compatible = "wd,mbl-gpio";
> +        reg-names = "dat";
> +        reg = <0xe0000000 0x1>;
> +        #gpio-cells = <2>;
> +        gpio-controller;
> +    };
> +
> +    gpio1: gpio1@e0100000 {

gpio@

> +        compatible = "wd,mbl-gpio";
> +        reg-names = "dat";
> +        reg = <0xe0100000 0x1>;
> +        #gpio-cells = <2>;
> +        gpio-controller;
> +        no-output;
> +    };
> +

Drop all examples below, they are not needed. Actually even these two
above could be combined as they differ with only one property.

Best regards,
Krzysztof

Re: [PATCH v2 1/2] dt-bindings: improve wb,mbl-gpio binding documentation.

[Leonard, Niall]

On 01/02/2023 08:27, Krzysztof Kozlowski wrote:
> *External Message* - Use caution before opening links or attachments
> 
> On 31/01/2023 14:49, Niall Leonard via B4 Submission Endpoint wrote:
>> From: Niall Leonard <nl250060@ncr.com>
>>
>> Convert existing wd,mbl-gpio binding documentation to YAML and add
>> new optional propery "no-input".
> 
> Subject: drop full stop
> 
> Use subject prefixes matching the subsystem (which you can get for
> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
> your patch is touching).
> 
> Subject: improve is vague. Instead: convert to DT schema
> 
> 
>>
>> Signed-off-by: Niall Leonard <nl250060@ncr.com>
>> ---
>>   .../devicetree/bindings/gpio/wd,mbl-gpio.txt       | 38 -----------
>>   .../devicetree/bindings/gpio/wd,mbl-gpio.yaml      | 78 ++++++++++++++++++++++
>>   2 files changed, 78 insertions(+), 38 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/gpio/wd,mbl-gpio.txt b/Documentation/devicetree/bindings/gpio/wd,mbl-gpio.txt
>> deleted file mode 100644
>> index 038c3a6a1f4d..000000000000
>> --- a/Documentation/devicetree/bindings/gpio/wd,mbl-gpio.txt
>> +++ /dev/null
>> @@ -1,38 +0,0 @@
>> -Bindings for the Western Digital's MyBook Live memory-mapped GPIO controllers.
>> -
>> -The Western Digital MyBook Live has two memory-mapped GPIO controllers.
>> -Both GPIO controller only have a single 8-bit data register, where GPIO
>> -state can be read and/or written.
>> -
>> -Required properties:
>> -	- compatible: should be "wd,mbl-gpio"
>> -	- reg-names: must contain
>> -		"dat" - data register
>> -	- reg: address + size pairs describing the GPIO register sets;
>> -		order must correspond with the order of entries in reg-names
>> -	- #gpio-cells: must be set to 2. The first cell is the pin number and
>> -			the second cell is used to specify the gpio polarity:
>> -			0 = active high
>> -			1 = active low
>> -	- gpio-controller: Marks the device node as a gpio controller.
>> -
>> -Optional properties:
>> -	- no-output: GPIOs are read-only.
>> -
>> -Examples:
>> -	gpio0: gpio0@e0000000 {
>> -		compatible = "wd,mbl-gpio";
>> -		reg-names = "dat";
>> -		reg = <0xe0000000 0x1>;
>> -		#gpio-cells = <2>;
>> -		gpio-controller;
>> -	};
>> -
>> -	gpio1: gpio1@e0100000 {
>> -		compatible = "wd,mbl-gpio";
>> -		reg-names = "dat";
>> -		reg = <0xe0100000 0x1>;
>> -		#gpio-cells = <2>;
>> -		gpio-controller;
>> -		no-output;
>> -	};
>> diff --git a/Documentation/devicetree/bindings/gpio/wd,mbl-gpio.yaml b/Documentation/devicetree/bindings/gpio/wd,mbl-gpio.yaml
>> new file mode 100644
>> index 000000000000..d1c72a42c5bc
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/gpio/wd,mbl-gpio.yaml
>> @@ -0,0 +1,78 @@
>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>> +%YAML 1.2
>> +---
>> +$id: https://urldefense.com/v3/__http://devicetree.org/schemas/gpio/wd,mbl-gpio.yaml*__;Iw!!In4Qlw!qBa7N0ccPrNQmjMeU9Hi-Ao3KxY4Fj2fXQutcNmpGxlXF7GLy7qjRk7k2ABk3yN7oi7tio6PoZyY-lsOTTreIaCxp-hI$
>> +$schema: https://urldefense.com/v3/__http://devicetree.org/meta-schemas/core.yaml*__;Iw!!In4Qlw!qBa7N0ccPrNQmjMeU9Hi-Ao3KxY4Fj2fXQutcNmpGxlXF7GLy7qjRk7k2ABk3yN7oi7tio6PoZyY-lsOTTreIbT2TzBT$
>> +
>> +title: Western Digital's MyBook Live memory-mapped GPIO controllers.
> 
> Drop full stop.
> 
>> +
>> +maintainers:
>> +  - Niall Leonard <nl250060@ncr.com>
>> +
>> +description: |+
>> +  Bindings for the Western Digital's MyBook Live memory-mapped GPIO controllers.
> 
> Drop "Bindings for". Actually drop entire line - it's redundant, you
> repeat the title.
> 
>> +
>> +  The Western Digital MyBook Live has two memory-mapped GPIO controllers.
>> +  Both GPIO controller only have a single 8-bit data register, where GPIO
>> +  state can be read and/or written.
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - wd,mbl-gpio
>> +
>> +  reg-names:
>> +    items:
>> +      - const: dat
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  "#gpio-cells":
>> +    const: 2
>> +
>> +  gpio-controller: true
>> +
>> +  no-output:
>> +    description: GPIOs are read-only.
>> +
>> +  no-input:
>> +    description: GPIOs are write-only.
> 
> 
> Split adding new property into separate patch. Each patch should do one
> logical change. New feature is another logical change. Conversion is
> that logical change (with any fixes needed for successful conversion,
> but not with new features).
> 
> 
>> +
>> +required:
>> +  - compatible
>> +  - reg-names
>> +  - reg
>> +  - '#gpio-cells'
> 
> Use consistent quotes - either ' or "
> 
>> +  - gpio-controller
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    gpio0: gpio0@e0000000 {
> 
> gpio@
> 
> (0 is not correct name of node)
> 
>> +        compatible = "wd,mbl-gpio";
>> +        reg-names = "dat";
>> +        reg = <0xe0000000 0x1>;
>> +        #gpio-cells = <2>;
>> +        gpio-controller;
>> +    };
>> +
>> +    gpio1: gpio1@e0100000 {
> 
> gpio@
> 
>> +        compatible = "wd,mbl-gpio";
>> +        reg-names = "dat";
>> +        reg = <0xe0100000 0x1>;
>> +        #gpio-cells = <2>;
>> +        gpio-controller;
>> +        no-output;
>> +    };
>> +
> 
> Drop all examples below, they are not needed. Actually even these two
> above could be combined as they differ with only one property.
> 
> Best regards,
> Krzysztof
> 
Thanks for reviewing. I'll make the suggested changes.

You might note that I added myself as maintainer of this file (as it 
wouldn't pass the dt checks otherwise) but that can't really be the case 
going forwards. I have only converted it from txt to yaml. Who should be 
the maintainer ? There was no-one listed as maintainer previously ?

Regards,
Niall.

Re: [PATCH v2 1/2] dt-bindings: improve wb,mbl-gpio binding documentation.

[Krzysztof Kozlowski]

On 07/02/2023 10:49, Leonard, Niall wrote:
>> Drop all examples below, they are not needed. Actually even these two
>> above could be combined as they differ with only one property.
>>
>> Best regards,
>> Krzysztof
>>
> Thanks for reviewing. I'll make the suggested changes.
> 
> You might note that I added myself as maintainer of this file (as it 
> wouldn't pass the dt checks otherwise) but that can't really be the case 
> going forwards. I have only converted it from txt to yaml. Who should be 
> the maintainer ? There was no-one listed as maintainer previously ?

If you are not interested in maintaining this binding, then as last
resort add here subsystem maintainers (Linus and Bartosz) but it isn't
really correct... just better than nothing.

Best regards,
Krzysztof

Re: [PATCH v2 2/2] gpio: mmio: Use new flag BGPIOF_NO_INPUT.

[Andy Shevchenko]

On Tue, Jan 31, 2023 at 01:49:38PM +0000, Niall Leonard wrote:
> Add new flag BGPIOF_NO_INPUT to header file.
> Use the existing shadow data register 'bgpio_data' to allow
> the last written value to be returned by the read operation
> when BGPIOF_NO_INPUT flag is set.
> Ensure this change only applies to the specific binding "wd,mbl-gpio".

I'm wondering why do we need that.

I mean the reading back the (possible cached) output value is the right thing
to do by default for GPIO (in output mode) or GPO. So, instead you can simply
check the current direction of the pin and return (cached) value.

Or did I miss something?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/2] gpio: mmio: Use new flag BGPIOF_NO_INPUT.

[Leonard, Niall]

On 12/02/2023 12:38, Andy Shevchenko wrote:
> *External Message* - Use caution before opening links or attachments
> 
> On Tue, Jan 31, 2023 at 01:49:38PM +0000, Niall Leonard wrote:
>> Add new flag BGPIOF_NO_INPUT to header file.
>> Use the existing shadow data register 'bgpio_data' to allow
>> the last written value to be returned by the read operation
>> when BGPIOF_NO_INPUT flag is set.
>> Ensure this change only applies to the specific binding "wd,mbl-gpio".
> 
> I'm wondering why do we need that.
> 
> I mean the reading back the (possible cached) output value is the right thing
> to do by default for GPIO (in output mode) or GPO. So, instead you can simply
> check the current direction of the pin and return (cached) value.
> 
> Or did I miss something?
> 
My thinking here was that I don't want to break any existing code which 
relies on the read always reading the physical port.
I'm going to rethink my approach here as I'm starting to think the 
better approach would be to modify the gpio-74xx-mmio.c driver to cater 
for this hardware.

Regards,
Niall.

Re: [PATCH v2 2/2] gpio: mmio: Use new flag BGPIOF_NO_INPUT.

[Andy Shevchenko]

On Mon, Mar 13, 2023 at 01:56:24PM +0000, Leonard, Niall wrote:
> On 12/02/2023 12:38, Andy Shevchenko wrote:
> > On Tue, Jan 31, 2023 at 01:49:38PM +0000, Niall Leonard wrote:
> >> Add new flag BGPIOF_NO_INPUT to header file.
> >> Use the existing shadow data register 'bgpio_data' to allow
> >> the last written value to be returned by the read operation
> >> when BGPIOF_NO_INPUT flag is set.
> >> Ensure this change only applies to the specific binding "wd,mbl-gpio".
> > 
> > I'm wondering why do we need that.
> > 
> > I mean the reading back the (possible cached) output value is the right thing
> > to do by default for GPIO (in output mode) or GPO. So, instead you can simply
> > check the current direction of the pin and return (cached) value.
> > 
> > Or did I miss something?
> > 
> My thinking here was that I don't want to break any existing code which 
> relies on the read always reading the physical port.
> I'm going to rethink my approach here as I'm starting to think the 
> better approach would be to modify the gpio-74xx-mmio.c driver to cater 
> for this hardware.

That's why we need a greatest denominator here, right?

Currently we have a full inconsistency on how drivers are implementing
all this. What I'm suggesting is to always have the following:
1) if pin is input or OS/OD/OE/OC -- read input buffer;
2) if pin is output, always read (cached) value.

Yes, there is an opinion to find a short circuit by reading input in
the output mode, but I consider that impractical complication.

Note, that above will also satisfy all (common) hardware limitations.

-- 
With Best Regards,
Andy Shevchenko