Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Tobias Waldekranz <tobias@waldekranz.com>
To:     Guenter Roeck <linux@roeck-us.net>,
        Wim Van Sebroeck <wim@linux-watchdog.org>
Cc:     linux-watchdog@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] watchdog: gpio_wdt: Support GPO lines with the toggle
 algorithm
In-Reply-To: <8b41a486-92af-2f2d-ba05-205650a90ee2@roeck-us.net>
References: <20220322222911.519238-1-tobias@waldekranz.com>
 <8b41a486-92af-2f2d-ba05-205650a90ee2@roeck-us.net>
Date:   Wed, 23 Mar 2022 09:57:49 +0100
Message-ID: <87tubphwqa.fsf@waldekranz.com>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

On Tue, Mar 22, 2022 at 17:04, Guenter Roeck <linux@roeck-us.net> wrote:
> On 3/22/22 15:29, Tobias Waldekranz wrote:
>> Support using GPO lines (i.e. GPIOs that are output-only) with
>> gpio_wdt using the "toggle" algorithm.
>> 
>> Since its inception, gpio_wdt has configured its GPIO line as an input
>> when using the "toggle" algorithm, even though it is used as an output
>> when kicking. This needlessly barred hardware with output-only pins
>> from using the driver.
>> 
>> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
>> ---
>> 
>> Hi,
>> 
>> This patch has been in our downstream tree for a long time. We need it
>> because our kick GPIO can't be used as an input.
>> 
>> What I really can't figure out is why the driver would request the pin
>> as in input, when it's always going to end up being used as an output
>> anyway.
>> 
>> So I thought I'd send it upstream in the hopes of either getting it
>> merged, or an explanation as to why it is needed.
>> 
>
> I _think_ the assumption / idea was that "toggle" implies that the output
> is connected to a pull-up resistor and that the pin either floats or is
> pulled down to ground, causing the signal to toggle. I don't know if/how
> that works in practice, though.

Ah I see. Yeah I'm not sure it has the intended effect. AFAIK, that type
of connection should be specified using the GPIOD_OUT_LOW_OPEN_DRAIN
flag, no?

Beyond that though, it seems unnecessary for the driver to impose that
restriction. If an open drain configuration is required on the GPIO,
then I think that should be specified via the device tree. Looking at
some code, gpiod_configure_flags seems to support this:

	if (lflags & GPIO_OPEN_DRAIN)
		set_bit(FLAG_OPEN_DRAIN, &desc->flags);
	else if (dflags & GPIOD_FLAGS_BIT_OPEN_DRAIN) {
		/*
		 * This enforces open drain mode from the consumer side.
		 * This is necessary for some busses like I2C, but the lookup
		 * should *REALLY* have specified them as open drain in the
		 * first place, so print a little warning here.
		 */
		set_bit(FLAG_OPEN_DRAIN, &desc->flags);
		gpiod_warn(desc,
			   "enforced open drain please flag it properly in DT/ACPI DSDT/board file\n");
	}

In other words, a DT node like this...

	&watchdog {
		compatible = "linux,wdt-gpio";
        	gpios = <&my-gpio 42 GPIO_ACTIVE_HIGH>;
	};

...the GPIO will behave like it does with the "level" algo today. If an
open-drain config is required, using this binding...

	&watchdog {
		compatible = "linux,wdt-gpio";
        	gpios = <&my-gpio 42 (GPIO_ACTIVE_LOW | GPIO_OPEN_DRAIN)>;
	};

...will give you that.

In other words, with this patch applied, the setup of the GPIO is
completely deferred to the device tree. In the general case, my guess is
that who ever wrote that has a better chance of specifying the correct
flags.