Message ID | ffb1eb1d747dce00b2c09d7af9357cd43284d1c4.1706802756.git.geert+renesas@glider.be |
---|---|
State | Superseded |
Headers | show |
Series | [1/2] regulator: dt-bindings: gpio-regulator: Fix {gpios-,}states limits | expand |
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
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 <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 --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;
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(-)