diff mbox series

[2/2] input: ps2-gpio: enforce and document open drain

Message ID 20220211233137.99624-3-danilokrummrich@dk-develop.de (mailing list archive)
State Superseded
Headers show
Series [1/2] dt-bindings: ps2-gpio: convert binding to json-schema | expand

Commit Message

Danilo Krummrich Feb. 11, 2022, 11:31 p.m. UTC
The PS/2 bus defines the data and clock line be open drain, therefore
document this in the dt-binding and enforce the particular flag in the
driver.

Without enforcing to flag at least the clock gpio as open drain we run
into the following warning:

WARNING: CPU: 1 PID: 40 at drivers/gpio/gpiolib.c:3175 gpiochip_enable_irq+0x54/0x90

gpiochip_enable_irq() warns on a GPIO being configured as output and
using IRQ without being flagged as open drain.

Signed-off-by: Danilo Krummrich <danilokrummrich@dk-develop.de>
---
 .../devicetree/bindings/serio/ps2-gpio.yaml        | 14 ++++++++++----
 drivers/input/serio/ps2-gpio.c                     |  9 +++++++--
 2 files changed, 17 insertions(+), 6 deletions(-)

Comments

Krzysztof Kozlowski Feb. 14, 2022, 9:50 p.m. UTC | #1
On 12/02/2022 00:31, Danilo Krummrich wrote:
> The PS/2 bus defines the data and clock line be open drain, therefore
> document this in the dt-binding and enforce the particular flag in the
> driver.
> 
> Without enforcing to flag at least the clock gpio as open drain we run
> into the following warning:
> 
> WARNING: CPU: 1 PID: 40 at drivers/gpio/gpiolib.c:3175 gpiochip_enable_irq+0x54/0x90
> 
> gpiochip_enable_irq() warns on a GPIO being configured as output and
> using IRQ without being flagged as open drain.
> 
> Signed-off-by: Danilo Krummrich <danilokrummrich@dk-develop.de>
> ---
>  .../devicetree/bindings/serio/ps2-gpio.yaml        | 14 ++++++++++----
>  drivers/input/serio/ps2-gpio.c                     |  9 +++++++--
>  2 files changed, 17 insertions(+), 6 deletions(-)
> 

Bindings as separate patch, please.

Best regards,
Krzysztof
Danilo Krummrich Feb. 14, 2022, 10:42 p.m. UTC | #2
On Mon, Feb 14, 2022 at 10:50:21PM +0100, Krzysztof Kozlowski wrote:
> On 12/02/2022 00:31, Danilo Krummrich wrote:
> > The PS/2 bus defines the data and clock line be open drain, therefore
> > document this in the dt-binding and enforce the particular flag in the
> > driver.
> > 
> > Without enforcing to flag at least the clock gpio as open drain we run
> > into the following warning:
> > 
> > WARNING: CPU: 1 PID: 40 at drivers/gpio/gpiolib.c:3175 gpiochip_enable_irq+0x54/0x90
> > 
> > gpiochip_enable_irq() warns on a GPIO being configured as output and
> > using IRQ without being flagged as open drain.
> > 
> > Signed-off-by: Danilo Krummrich <danilokrummrich@dk-develop.de>
> > ---
> >  .../devicetree/bindings/serio/ps2-gpio.yaml        | 14 ++++++++++----
> >  drivers/input/serio/ps2-gpio.c                     |  9 +++++++--
> >  2 files changed, 17 insertions(+), 6 deletions(-)
> > 
> 
> Bindings as separate patch, please.
Are you fine with adding this additional documentation directly to the patch
converting the binding to a json-schema or do you prefer it to be a separate
patch?
> 
> Best regards,
> Krzysztof

- Danilo
Krzysztof Kozlowski Feb. 15, 2022, 7:05 a.m. UTC | #3
On 14/02/2022 23:42, Danilo Krummrich wrote:
> On Mon, Feb 14, 2022 at 10:50:21PM +0100, Krzysztof Kozlowski wrote:
>> On 12/02/2022 00:31, Danilo Krummrich wrote:
>>> The PS/2 bus defines the data and clock line be open drain, therefore
>>> document this in the dt-binding and enforce the particular flag in the
>>> driver.
>>>
>>> Without enforcing to flag at least the clock gpio as open drain we run
>>> into the following warning:
>>>
>>> WARNING: CPU: 1 PID: 40 at drivers/gpio/gpiolib.c:3175 gpiochip_enable_irq+0x54/0x90
>>>
>>> gpiochip_enable_irq() warns on a GPIO being configured as output and
>>> using IRQ without being flagged as open drain.
>>>
>>> Signed-off-by: Danilo Krummrich <danilokrummrich@dk-develop.de>
>>> ---
>>>  .../devicetree/bindings/serio/ps2-gpio.yaml        | 14 ++++++++++----
>>>  drivers/input/serio/ps2-gpio.c                     |  9 +++++++--
>>>  2 files changed, 17 insertions(+), 6 deletions(-)
>>>
>>
>> Bindings as separate patch, please.
> Are you fine with adding this additional documentation directly to the patch
> converting the binding to a json-schema or do you prefer it to be a separate
> patch?

I propose a separate patch, so you will have two bindings patches.

Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/serio/ps2-gpio.yaml b/Documentation/devicetree/bindings/serio/ps2-gpio.yaml
index ec6fa7b40851..5e1951b4f511 100644
--- a/Documentation/devicetree/bindings/serio/ps2-gpio.yaml
+++ b/Documentation/devicetree/bindings/serio/ps2-gpio.yaml
@@ -16,12 +16,18 @@  properties:
 
   data-gpios:
     description:
-      the gpio used for the data signal
+      the gpio used for the data signal - this should be flagged as
+      active high using open drain with (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)
+      from <dt-bindings/gpio/gpio.h> since the signal is open drain by
+      definition
     maxItems: 1
 
   clk-gpios:
     description:
-      the gpio used for the clock signal
+      the gpio used for the clock signal - this should be flagged as
+      active high using open drain with (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)
+      from <dt-bindings/gpio/gpio.h> since the signal is open drain by
+      definition
     maxItems: 1
 
   interrupts:
@@ -46,7 +52,7 @@  examples:
         compatible = "ps2-gpio";
         interrupt-parent = <&gpio>;
         interrupts = <23 IRQ_TYPE_EDGE_FALLING>;
-        data-gpios = <&gpio 24 GPIO_ACTIVE_HIGH>;
-        clk-gpios = <&gpio 23 GPIO_ACTIVE_HIGH>;
+        data-gpios = <&gpio 24 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
+        clk-gpios = <&gpio 23 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
         write-enable;
     };
diff --git a/drivers/input/serio/ps2-gpio.c b/drivers/input/serio/ps2-gpio.c
index 460d520ac865..fdaff7a415cd 100644
--- a/drivers/input/serio/ps2-gpio.c
+++ b/drivers/input/serio/ps2-gpio.c
@@ -362,14 +362,19 @@  static irqreturn_t ps2_gpio_irq(int irq, void *dev_id)
 static int ps2_gpio_get_props(struct device *dev,
 				 struct ps2_gpio_data *drvdata)
 {
-	drvdata->gpio_data = devm_gpiod_get(dev, "data", GPIOD_IN);
+	enum gpiod_flags gflags;
+
+	/* Enforce open drain, since this is required by the PS/2 bus. */
+	gflags = GPIOD_IN | GPIOD_FLAGS_BIT_OPEN_DRAIN;
+
+	drvdata->gpio_data = devm_gpiod_get(dev, "data", gflags);
 	if (IS_ERR(drvdata->gpio_data)) {
 		dev_err(dev, "failed to request data gpio: %ld",
 			PTR_ERR(drvdata->gpio_data));
 		return PTR_ERR(drvdata->gpio_data);
 	}
 
-	drvdata->gpio_clk = devm_gpiod_get(dev, "clk", GPIOD_IN);
+	drvdata->gpio_clk = devm_gpiod_get(dev, "clk", gflags);
 	if (IS_ERR(drvdata->gpio_clk)) {
 		dev_err(dev, "failed to request clock gpio: %ld",
 			PTR_ERR(drvdata->gpio_clk));