[PATCH 0/2] regulator: gpio: Miscellaneous state fixes

[PATCH 0/2] regulator: gpio: Miscellaneous state fixes

[Geert Uytterhoeven]

	Hi all,

This patch series contains two fixes related to gpio-regulator states in
DTS.

Thanks for your comments!

Geert Uytterhoeven (2):
  regulator: dt-bindings: gpio-regulator: Fix {gpios-,}states limits
  [RFC] regulator: gpio: Correct default GPIO state to LOW

 .../devicetree/bindings/regulator/gpio-regulator.yaml         | 4 +++-
 drivers/regulator/gpio-regulator.c                            | 4 ++--
 2 files changed, 5 insertions(+), 3 deletions(-)

-- 
2.34.1

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

[PATCH 1/2] regulator: dt-bindings: gpio-regulator: Fix {gpios-,}states limits

[Geert Uytterhoeven]

make dtbs_check:

    arch/arm64/boot/dts/renesas/r8a77951-salvator-xs.dtb: regulator-vccq-sdhi0: Unevaluated properties are not allowed ('gpios-states', 'states' were unexpected)
	    from schema $id: http://devicetree.org/schemas/regulator/gpio-regulator.yaml#

The number of items in "gpios-states" must match the number of items in
"gpios", so their limits should be identical.

The number of items in "states" must lie within the range from zero up
to 2^{number of gpios}.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
The second issue did not cause any dtbs_check errors?
---
 .../devicetree/bindings/regulator/gpio-regulator.yaml         | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/regulator/gpio-regulator.yaml b/Documentation/devicetree/bindings/regulator/gpio-regulator.yaml
index f4c1f36e52e9c3d8..1cecf8faee5dc374 100644
--- a/Documentation/devicetree/bindings/regulator/gpio-regulator.yaml
+++ b/Documentation/devicetree/bindings/regulator/gpio-regulator.yaml
@@ -47,6 +47,7 @@ properties:
         1: HIGH
       Default is LOW if nothing else is specified.
     $ref: /schemas/types.yaml#/definitions/uint32-array
+    minItems: 1
     maxItems: 8
     items:
       enum: [0, 1]
@@ -57,7 +58,8 @@ properties:
       regulator and matching GPIO configurations to achieve them. If there are
       no states in the "states" array, use a fixed regulator instead.
     $ref: /schemas/types.yaml#/definitions/uint32-matrix
-    maxItems: 8
+    minItems: 0
+    maxItems: 256
     items:
       items:
         - description: Voltage in microvolts
-- 
2.34.1

Re: [PATCH 1/2] regulator: dt-bindings: gpio-regulator: Fix {gpios-,}states limits

[Rob Herring]

On Thu, 01 Feb 2024 16:58:41 +0100, Geert Uytterhoeven wrote:
> make dtbs_check:
> 
>     arch/arm64/boot/dts/renesas/r8a77951-salvator-xs.dtb: regulator-vccq-sdhi0: Unevaluated properties are not allowed ('gpios-states', 'states' were unexpected)
> 	    from schema $id: http://devicetree.org/schemas/regulator/gpio-regulator.yaml#
> 
> The number of items in "gpios-states" must match the number of items in
> "gpios", so their limits should be identical.
> 
> The number of items in "states" must lie within the range from zero up
> to 2^{number of gpios}.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> The second issue did not cause any dtbs_check errors?
> ---
>  .../devicetree/bindings/regulator/gpio-regulator.yaml         | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/regulator/gpio-regulator.yaml: properties:states:minItems: 0 is less than the minimum of 1
	hint: An array property has at least 1 item or is not present
	from schema $id: http://devicetree.org/meta-schemas/keywords.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/b20aab137058c02ab5af9aaa1280729a02c6ea49.1706802756.git.geert+renesas@glider.be

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH 1/2] regulator: dt-bindings: gpio-regulator: Fix {gpios-,}states limits

[Geert Uytterhoeven]

Hi Rob The Robot ;-)

On Thu, Feb 1, 2024 at 6:31 PM Rob Herring <robh@kernel.org> wrote:
> On Thu, 01 Feb 2024 16:58:41 +0100, Geert Uytterhoeven wrote:
> > make dtbs_check:
> >
> >     arch/arm64/boot/dts/renesas/r8a77951-salvator-xs.dtb: regulator-vccq-sdhi0: Unevaluated properties are not allowed ('gpios-states', 'states' were unexpected)
> >           from schema $id: http://devicetree.org/schemas/regulator/gpio-regulator.yaml#
> >
> > The number of items in "gpios-states" must match the number of items in
> > "gpios", so their limits should be identical.
> >
> > The number of items in "states" must lie within the range from zero up
> > to 2^{number of gpios}.
> >
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > ---
> > The second issue did not cause any dtbs_check errors?
> > ---
> >  .../devicetree/bindings/regulator/gpio-regulator.yaml         | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
>
> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
>
> yamllint warnings/errors:
>
> dtschema/dtc warnings/errors:
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/regulator/gpio-regulator.yaml: properties:states:minItems: 0 is less than the minimum of 1
>         hint: An array property has at least 1 item or is not present
>         from schema $id: http://devicetree.org/meta-schemas/keywords.yaml#

Oops, I changed this from 1 to 0 _after_ running dt_binding_check, so
I'm totally to blame for this.

The description says:

    If there are no states in the "states" array, use a fixed regulator instead.

which I misinterpreted as "states can be empty", especially as the
driver does seem to support that?

I guess 1 is the proper minimum?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/2] regulator: dt-bindings: gpio-regulator: Fix {gpios-,}states limits

[Rob Herring]

On Thu, Feb 1, 2024 at 2:06 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Rob The Robot ;-)

:)

> On Thu, Feb 1, 2024 at 6:31 PM Rob Herring <robh@kernel.org> wrote:
> > On Thu, 01 Feb 2024 16:58:41 +0100, Geert Uytterhoeven wrote:
> > > make dtbs_check:
> > >
> > >     arch/arm64/boot/dts/renesas/r8a77951-salvator-xs.dtb: regulator-vccq-sdhi0: Unevaluated properties are not allowed ('gpios-states', 'states' were unexpected)
> > >           from schema $id: http://devicetree.org/schemas/regulator/gpio-regulator.yaml#
> > >
> > > The number of items in "gpios-states" must match the number of items in
> > > "gpios", so their limits should be identical.
> > >
> > > The number of items in "states" must lie within the range from zero up
> > > to 2^{number of gpios}.
> > >
> > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > ---
> > > The second issue did not cause any dtbs_check errors?
> > > ---
> > >  .../devicetree/bindings/regulator/gpio-regulator.yaml         | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> >
> > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> > on your patch (DT_CHECKER_FLAGS is new in v5.13):
> >
> > yamllint warnings/errors:
> >
> > dtschema/dtc warnings/errors:
> > /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/regulator/gpio-regulator.yaml: properties:states:minItems: 0 is less than the minimum of 1
> >         hint: An array property has at least 1 item or is not present
> >         from schema $id: http://devicetree.org/meta-schemas/keywords.yaml#
>
> Oops, I changed this from 1 to 0 _after_ running dt_binding_check, so
> I'm totally to blame for this.
>
> The description says:
>
>     If there are no states in the "states" array, use a fixed regulator instead.
>
> which I misinterpreted as "states can be empty", especially as the
> driver does seem to support that?
>
> I guess 1 is the proper minimum?

Yes. While JSON can for example have "foo: []", that's not really
defined for DT given we store no type info. An empty property is a
boolean.

Rob

Re: [PATCH 1/2] regulator: dt-bindings: gpio-regulator: Fix {gpios-,}states limits

[Rob Herring]

On Thu, Feb 01, 2024 at 04:58:41PM +0100, Geert Uytterhoeven wrote:
> make dtbs_check:
> 
>     arch/arm64/boot/dts/renesas/r8a77951-salvator-xs.dtb: regulator-vccq-sdhi0: Unevaluated properties are not allowed ('gpios-states', 'states' were unexpected)
> 	    from schema $id: http://devicetree.org/schemas/regulator/gpio-regulator.yaml#

Unevaluated properties warning here is not interesting. If a property 
fails validation, then it is considered unevaluated. It's that warning 
which is interesting:

arch/arm64/boot/dts/renesas/r8a77951-salvator-xs.dtb: regulator-vccq-sdhi0: gpios-states:0: [1] is too short
        from schema $id: http://devicetree.org/schemas/regulator/gpio-regulator.yaml#

> 
> The number of items in "gpios-states" must match the number of items in
> "gpios", so their limits should be identical.
> 
> The number of items in "states" must lie within the range from zero up
> to 2^{number of gpios}.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> The second issue did not cause any dtbs_check errors?

I'm not seeing 'states' fail, but it looks like you did? Is that the 
issue you mean? Looks like in the matrix case, we're now setting 
minItems if unspecified.

> ---
>  .../devicetree/bindings/regulator/gpio-regulator.yaml         | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/regulator/gpio-regulator.yaml b/Documentation/devicetree/bindings/regulator/gpio-regulator.yaml
> index f4c1f36e52e9c3d8..1cecf8faee5dc374 100644
> --- a/Documentation/devicetree/bindings/regulator/gpio-regulator.yaml
> +++ b/Documentation/devicetree/bindings/regulator/gpio-regulator.yaml
> @@ -47,6 +47,7 @@ properties:
>          1: HIGH
>        Default is LOW if nothing else is specified.
>      $ref: /schemas/types.yaml#/definitions/uint32-array
> +    minItems: 1
>      maxItems: 8
>      items:
>        enum: [0, 1]
> @@ -57,7 +58,8 @@ properties:
>        regulator and matching GPIO configurations to achieve them. If there are
>        no states in the "states" array, use a fixed regulator instead.
>      $ref: /schemas/types.yaml#/definitions/uint32-matrix
> -    maxItems: 8
> +    minItems: 0
> +    maxItems: 256
>      items:
>        items:
>          - description: Voltage in microvolts
> -- 
> 2.34.1
> 

Re: [PATCH 1/2] regulator: dt-bindings: gpio-regulator: Fix {gpios-,}states limits

[Geert Uytterhoeven]

Hi Rob,

On Thu, Feb 1, 2024 at 11:11 PM Rob Herring <robh@kernel.org> wrote:
> On Thu, Feb 01, 2024 at 04:58:41PM +0100, Geert Uytterhoeven wrote:
> > make dtbs_check:
> >
> >     arch/arm64/boot/dts/renesas/r8a77951-salvator-xs.dtb: regulator-vccq-sdhi0: Unevaluated properties are not allowed ('gpios-states', 'states' were unexpected)
> >           from schema $id: http://devicetree.org/schemas/regulator/gpio-regulator.yaml#
>
> Unevaluated properties warning here is not interesting. If a property
> fails validation, then it is considered unevaluated. It's that warning
> which is interesting:
>
> arch/arm64/boot/dts/renesas/r8a77951-salvator-xs.dtb: regulator-vccq-sdhi0: gpios-states:0: [1] is too short
>         from schema $id: http://devicetree.org/schemas/regulator/gpio-regulator.yaml#

Oops (again, I'm afraid my mind is already living at FOSDEM ;-),
I copy-'n-pasted the wrong message...

> > The number of items in "gpios-states" must match the number of items in
> > "gpios", so their limits should be identical.
> >
> > The number of items in "states" must lie within the range from zero up
> > to 2^{number of gpios}.
> >
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > ---
> > The second issue did not cause any dtbs_check errors?
>
> I'm not seeing 'states' fail, but it looks like you did? Is that the
> issue you mean? Looks like in the matrix case, we're now setting
> minItems if unspecified.

No, I did not see states fail, only gpios-states.
Hence "the second issue did not cause errors".

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Geert Uytterhoeven]

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(-)

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;
-- 
2.34.1

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

[Linus Walleij]

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

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 483 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Linus Walleij]

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