On Thu, Feb 1, 2024 at 4:58 PM Geert Uytterhoeven
<[email protected]> 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 <[email protected]>
It's closer to the spec, but Mark's pick, anyway:
Acked-by: Linus Walleij <[email protected]>
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
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.
On Sat, Feb 3, 2024 at 1:23 AM Mark Brown <[email protected]> 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