| Message ID | 20220322222911.519238-1-tobias@waldekranz.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | watchdog: gpio_wdt: Support GPO lines with the toggle algorithm | expand |
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. 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 <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.
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);
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. drivers/watchdog/gpio_wdt.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)