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 <[email protected]>
---
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.
drivers/watchdog/gpio_wdt.c | 13 +++++--------
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/drivers/watchdog/gpio_wdt.c b/drivers/watchdog/gpio_wdt.c
index 0923201ce874..f7686688e0e2 100644
--- a/drivers/watchdog/gpio_wdt.c
+++ b/drivers/watchdog/gpio_wdt.c
@@ -108,7 +108,6 @@ static int gpio_wdt_probe(struct platform_device *pdev)
struct device *dev = &pdev->dev;
struct device_node *np = dev->of_node;
struct gpio_wdt_priv *priv;
- enum gpiod_flags gflags;
unsigned int hw_margin;
const char *algo;
int ret;
@@ -122,17 +121,15 @@ static int gpio_wdt_probe(struct platform_device *pdev)
ret = of_property_read_string(np, "hw_algo", &algo);
if (ret)
return ret;
- if (!strcmp(algo, "toggle")) {
+
+ if (!strcmp(algo, "toggle"))
priv->hw_algo = HW_ALGO_TOGGLE;
- gflags = GPIOD_IN;
- } else if (!strcmp(algo, "level")) {
+ else if (!strcmp(algo, "level"))
priv->hw_algo = HW_ALGO_LEVEL;
- gflags = GPIOD_OUT_LOW;
- } else {
+ else
return -EINVAL;
- }
- priv->gpiod = devm_gpiod_get(dev, NULL, gflags);
+ priv->gpiod = devm_gpiod_get(dev, NULL, GPIOD_OUT_LOW);
if (IS_ERR(priv->gpiod))
return PTR_ERR(priv->gpiod);
--
2.25.1
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 <[email protected]>
> ---
>
> 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.
Guenter
> drivers/watchdog/gpio_wdt.c | 13 +++++--------
> 1 file changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/watchdog/gpio_wdt.c b/drivers/watchdog/gpio_wdt.c
> index 0923201ce874..f7686688e0e2 100644
> --- a/drivers/watchdog/gpio_wdt.c
> +++ b/drivers/watchdog/gpio_wdt.c
> @@ -108,7 +108,6 @@ static int gpio_wdt_probe(struct platform_device *pdev)
> struct device *dev = &pdev->dev;
> struct device_node *np = dev->of_node;
> struct gpio_wdt_priv *priv;
> - enum gpiod_flags gflags;
> unsigned int hw_margin;
> const char *algo;
> int ret;
> @@ -122,17 +121,15 @@ static int gpio_wdt_probe(struct platform_device *pdev)
> ret = of_property_read_string(np, "hw_algo", &algo);
> if (ret)
> return ret;
> - if (!strcmp(algo, "toggle")) {
> +
> + if (!strcmp(algo, "toggle"))
> priv->hw_algo = HW_ALGO_TOGGLE;
> - gflags = GPIOD_IN;
> - } else if (!strcmp(algo, "level")) {
> + else if (!strcmp(algo, "level"))
> priv->hw_algo = HW_ALGO_LEVEL;
> - gflags = GPIOD_OUT_LOW;
> - } else {
> + else
> return -EINVAL;
> - }
>
> - priv->gpiod = devm_gpiod_get(dev, NULL, gflags);
> + priv->gpiod = devm_gpiod_get(dev, NULL, GPIOD_OUT_LOW);
> if (IS_ERR(priv->gpiod))
> return PTR_ERR(priv->gpiod);
>
On Tue, Mar 22, 2022 at 17:04, Guenter Roeck <[email protected]> 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 <[email protected]>
>> ---
>>
>> 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.