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

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

On Thu, Feb 1, 2024 at 4:58 PM Geert Uytterhoeven
<geert+renesas@xxxxxxxxx> 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@xxxxxxxxx>

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

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