diff mbox series

[2/2] regulator: gpio: Correct default GPIO state to LOW

Message ID ffb1eb1d747dce00b2c09d7af9357cd43284d1c4.1706802756.git.geert+renesas@glider.be
State New
Headers show
Series [1/2] regulator: dt-bindings: gpio-regulator: Fix {gpios-,}states limits | expand

Commit Message

Geert Uytterhoeven Feb. 1, 2024, 3:58 p.m. UTC
According to the GPIO regulator DT bindings[1], the default GPIO state
is LOW.  However, the driver defaults to HIGH.

Before the conversion to descriptors in commit d6cd33ad71029a3f
("regulator: gpio: Convert to use descriptors"), the default state used
by the driver was rather ill-defined, too:
  - If the "gpio-states" property was missing or empty, the default was
    low, matching the bindings.
  - If the "gpio-states" property was present, the default for missing
    entries was the value of the last present entry.

Fix this by making the driver adhere to the DT bindings, i.e. default to
LOW.

[1] Documentation/devicetree/bindings/regulator/gpio-regulator.yaml

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
I have no idea if this has any impact.
I guess most/all DTS files have proper gpios-states properties?
---
 drivers/regulator/gpio-regulator.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Linus Walleij Feb. 2, 2024, 5:41 p.m. UTC | #1
On Thu, Feb 1, 2024 at 4:58 PM Geert Uytterhoeven
<geert+renesas@glider.be> wrote:

> According to the GPIO regulator DT bindings[1], the default GPIO state
> is LOW.  However, the driver defaults to HIGH.
>
> Before the conversion to descriptors in commit d6cd33ad71029a3f
> ("regulator: gpio: Convert to use descriptors"), the default state used
> by the driver was rather ill-defined, too:
>   - If the "gpio-states" property was missing or empty, the default was
>     low, matching the bindings.
>   - If the "gpio-states" property was present, the default for missing
>     entries was the value of the last present entry.
>
> Fix this by making the driver adhere to the DT bindings, i.e. default to
> LOW.
>
> [1] Documentation/devicetree/bindings/regulator/gpio-regulator.yaml
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

It's closer to the spec, but Mark's pick, anyway:
Acked-by: Linus Walleij <linus.walleij@linaro.org>

But on the subject, the bindings say this:

+  gpios-states:
+    description: |
+      On operating systems, that don't support reading back gpio values in
+      output mode (most notably linux), this array provides the state of GPIO
+      pins set when requesting them from the gpio controller. Systems, that are
+      capable of preserving state when requesting the lines, are free to ignore
+      this property.

Actually, Linux can read back the value just fine in output mode,
so what about just ignoring the property and update the document
to stop saying that about Linux?

See drivers/gpiolib.c:

static int gpio_chip_get_value(struct gpio_chip *gc, const struct
gpio_desc *desc)
{
        return gc->get ? gc->get(gc, gpio_chip_hwgpio(desc)) : -EIO;
}

static int gpiod_get_raw_value_commit(const struct gpio_desc *desc)
{
        struct gpio_chip *gc;
        int value;

        gc = desc->gdev->chip;
        value = gpio_chip_get_value(gc, desc);
        value = value < 0 ? value : !!value;
        trace_gpio_value(desc_to_gpio(desc), 1, value);
        return value;
}

int gpiod_get_value(const struct gpio_desc *desc)
{
        int value;

        VALIDATE_DESC(desc);
        /* Should be using gpiod_get_value_cansleep() */
        WARN_ON(desc->gdev->chip->can_sleep);

        value = gpiod_get_raw_value_commit(desc);
        if (value < 0)
                return value;

        if (test_bit(FLAG_ACTIVE_LOW, &desc->flags))
                value = !value;

        return value;
}

None of this path excludes lines in output mode...

If individual drivers don't support it, it's either because:

1. The driver is buggy (such as not implementing .get()
  and should be fixed.

2. The hardware is actually incapable of reading the
  value in output mode.

3. Reading the value hardware register is bogus when the
  line is in output mode.

All these are driver issues and have nothing to do with Linux per se.

Yours,
Linus Walleij
Mark Brown Feb. 3, 2024, 12:23 a.m. UTC | #2
On Fri, Feb 02, 2024 at 06:41:31PM +0100, Linus Walleij wrote:

> Actually, Linux can read back the value just fine in output mode,
> so what about just ignoring the property and update the document
> to stop saying that about Linux?

IIRC that was there because historically the gpiolib documentation
said that this was unsupported (though the code never actually prevented
you trying I think?) and will have made it's way through the DT
conversion and refactoring of the bindings.
Linus Walleij Feb. 4, 2024, 6:18 p.m. UTC | #3
On Sat, Feb 3, 2024 at 1:23 AM Mark Brown <broonie@kernel.org> wrote:
> On Fri, Feb 02, 2024 at 06:41:31PM +0100, Linus Walleij wrote:
>
> > Actually, Linux can read back the value just fine in output mode,
> > so what about just ignoring the property and update the document
> > to stop saying that about Linux?
>
> IIRC that was there because historically the gpiolib documentation
> said that this was unsupported (though the code never actually prevented
> you trying I think?) and will have made it's way through the DT
> conversion and refactoring of the bindings.

I have this gut feeling too but I can't find it!

Yours,
Linus Walleij
diff mbox series

Patch

diff --git a/drivers/regulator/gpio-regulator.c b/drivers/regulator/gpio-regulator.c
index 65927fa2ef161cda..5dfed8bae0c4cfdc 100644
--- a/drivers/regulator/gpio-regulator.c
+++ b/drivers/regulator/gpio-regulator.c
@@ -176,9 +176,9 @@  of_get_gpio_regulator_config(struct device *dev, struct device_node *np,
 			ret = of_property_read_u32_index(np, "gpios-states", i,
 							 &val);
 
-			/* Default to high per specification */
+			/* Default to low per specification */
 			if (ret)
-				config->gflags[i] = GPIOD_OUT_HIGH;
+				config->gflags[i] = GPIOD_OUT_LOW;
 			else
 				config->gflags[i] =
 					val ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW;