| Message ID | 20220401072714.106403-1-krzysztof.kozlowski@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] dt-bindings: gpio: add common consumer GPIO lines | expand |
On Fri, 01 Apr 2022 09:27:14 +0200, Krzysztof Kozlowski wrote: > Typical GPIO lines like enable, powerdown, reset or wakeup are not > documented as common, which leads to new variations of these (e.g. > pwdn-gpios). Add a common schema which serves also as a documentation > for preferred naming. > > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > --- > > Changes since v2: > 1. Correct email. > > Changes since v1: > 1. Select-true, add maxItems and description for each entry (Rob). > 2. Mention ACTIVE_LOW in bindings description (Linus). > 3. Add allOf for pwrseq reset-gpios case. > > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > --- > .../bindings/gpio/gpio-consumer-common.yaml | 64 +++++++++++++++++++ > 1 file changed, 64 insertions(+) > create mode 100644 Documentation/devicetree/bindings/gpio/gpio-consumer-common.yaml > 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/linux-dt-review/Documentation/devicetree/bindings/regulator/richtek,rt4801-regulator.example.dt.yaml: rt4801@73: enable-gpios: [[4294967295, 2, 0], [4294967295, 3, 0]] is too long From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/gpio/gpio-consumer-common.yaml doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/patch/ This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1. 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.
On 01/04/2022 15:13, Rob Herring wrote: > On Fri, 01 Apr 2022 09:27:14 +0200, Krzysztof Kozlowski wrote: >> Typical GPIO lines like enable, powerdown, reset or wakeup are not >> documented as common, which leads to new variations of these (e.g. >> pwdn-gpios). Add a common schema which serves also as a documentation >> for preferred naming. >> >> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >> >> --- >> >> Changes since v2: >> 1. Correct email. >> >> Changes since v1: >> 1. Select-true, add maxItems and description for each entry (Rob). >> 2. Mention ACTIVE_LOW in bindings description (Linus). >> 3. Add allOf for pwrseq reset-gpios case. >> >> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >> --- >> .../bindings/gpio/gpio-consumer-common.yaml | 64 +++++++++++++++++++ >> 1 file changed, 64 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/gpio/gpio-consumer-common.yaml >> > > 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/linux-dt-review/Documentation/devicetree/bindings/regulator/richtek,rt4801-regulator.example.dt.yaml: rt4801@73: enable-gpios: [[4294967295, 2, 0], [4294967295, 3, 0]] is too long > From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/gpio/gpio-consumer-common.yaml > Hi Rob, With v1, you proposed to use maxItems for all these standard gpios, but as we see here there are two exceptions: 1. pwrseq might have up to 32 reset-gpios, 2. richtek,rt4801 uses up to 2 enable-gpios. One way is to add exceptions in gpio-consumer-common.yaml, like I did for reset-gpios and pwrseq. However this scales poor if more of such usages appear. Maybe let's drop the maxItems for all of them? Best regards, Krzysztof
On Fri, Apr 1, 2022 at 8:27 AM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 01/04/2022 15:13, Rob Herring wrote: > > On Fri, 01 Apr 2022 09:27:14 +0200, Krzysztof Kozlowski wrote: > >> Typical GPIO lines like enable, powerdown, reset or wakeup are not > >> documented as common, which leads to new variations of these (e.g. > >> pwdn-gpios). Add a common schema which serves also as a documentation > >> for preferred naming. > >> > >> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > >> > >> --- > >> > >> Changes since v2: > >> 1. Correct email. > >> > >> Changes since v1: > >> 1. Select-true, add maxItems and description for each entry (Rob). > >> 2. Mention ACTIVE_LOW in bindings description (Linus). > >> 3. Add allOf for pwrseq reset-gpios case. > >> > >> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > >> --- > >> .../bindings/gpio/gpio-consumer-common.yaml | 64 +++++++++++++++++++ > >> 1 file changed, 64 insertions(+) > >> create mode 100644 Documentation/devicetree/bindings/gpio/gpio-consumer-common.yaml > >> > > > > 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/linux-dt-review/Documentation/devicetree/bindings/regulator/richtek,rt4801-regulator.example.dt.yaml: rt4801@73: enable-gpios: [[4294967295, 2, 0], [4294967295, 3, 0]] is too long > > From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/gpio/gpio-consumer-common.yaml > > > > Hi Rob, > > With v1, you proposed to use maxItems for all these standard gpios, but > as we see here there are two exceptions: > 1. pwrseq might have up to 32 reset-gpios, > 2. richtek,rt4801 uses up to 2 enable-gpios. There's always an outlier... > One way is to add exceptions in gpio-consumer-common.yaml, like I did > for reset-gpios and pwrseq. However this scales poor if more of such > usages appear. I'd reject any new cases, but even just 2 I don't really like. > Maybe let's drop the maxItems for all of them? Let's just drop it at least for now (though it seems we can keep it for powerdown-gpios). A possible solution here may be adding 'maxItems: 1' automatically to schemas if not specified. I've been thinking of doing this on standard unit properties. That's another case of 99% of cases are a single entry with a few outliers. Rob
On 01/04/2022 17:01, Rob Herring wrote: > On Fri, Apr 1, 2022 at 8:27 AM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> >> On 01/04/2022 15:13, Rob Herring wrote: >>> On Fri, 01 Apr 2022 09:27:14 +0200, Krzysztof Kozlowski wrote: >>>> Typical GPIO lines like enable, powerdown, reset or wakeup are not >>>> documented as common, which leads to new variations of these (e.g. >>>> pwdn-gpios). Add a common schema which serves also as a documentation >>>> for preferred naming. >>>> >>>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >>>> >>>> --- >>>> >>>> Changes since v2: >>>> 1. Correct email. >>>> >>>> Changes since v1: >>>> 1. Select-true, add maxItems and description for each entry (Rob). >>>> 2. Mention ACTIVE_LOW in bindings description (Linus). >>>> 3. Add allOf for pwrseq reset-gpios case. >>>> >>>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >>>> --- >>>> .../bindings/gpio/gpio-consumer-common.yaml | 64 +++++++++++++++++++ >>>> 1 file changed, 64 insertions(+) >>>> create mode 100644 Documentation/devicetree/bindings/gpio/gpio-consumer-common.yaml >>>> >>> >>> 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/linux-dt-review/Documentation/devicetree/bindings/regulator/richtek,rt4801-regulator.example.dt.yaml: rt4801@73: enable-gpios: [[4294967295, 2, 0], [4294967295, 3, 0]] is too long >>> From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/gpio/gpio-consumer-common.yaml >>> >> >> Hi Rob, >> >> With v1, you proposed to use maxItems for all these standard gpios, but >> as we see here there are two exceptions: >> 1. pwrseq might have up to 32 reset-gpios, >> 2. richtek,rt4801 uses up to 2 enable-gpios. > > There's always an outlier... > >> One way is to add exceptions in gpio-consumer-common.yaml, like I did >> for reset-gpios and pwrseq. However this scales poor if more of such >> usages appear. > > I'd reject any new cases, but even just 2 I don't really like. The richtek,rt4801 enable-gpios are for controlling two separate regulators, so it should have been under regulator subnodes/children. Some other regulators follow this pattern, so only this one is done that way. That driver could be converted to enable-gpios per regulator, so if you are sure about rejection of new cases, how about keeping current exceptions in allOf:if? > >> Maybe let's drop the maxItems for all of them? > > Let's just drop it at least for now (though it seems we can keep it > for powerdown-gpios). > > A possible solution here may be adding 'maxItems: 1' automatically to > schemas if not specified. I've been thinking of doing this on standard > unit properties. That's another case of > 99% of cases are a single entry with a few outliers. > Best regards, Krzysztof
On Fri, Apr 01, 2022 at 05:11:57PM +0200, Krzysztof Kozlowski wrote: > On 01/04/2022 17:01, Rob Herring wrote: > > On Fri, Apr 1, 2022 at 8:27 AM Krzysztof Kozlowski > > <krzysztof.kozlowski@linaro.org> wrote: > >> > >> On 01/04/2022 15:13, Rob Herring wrote: > >>> On Fri, 01 Apr 2022 09:27:14 +0200, Krzysztof Kozlowski wrote: > >>>> Typical GPIO lines like enable, powerdown, reset or wakeup are not > >>>> documented as common, which leads to new variations of these (e.g. > >>>> pwdn-gpios). Add a common schema which serves also as a documentation > >>>> for preferred naming. > >>>> > >>>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > >>>> > >>>> --- > >>>> > >>>> Changes since v2: > >>>> 1. Correct email. > >>>> > >>>> Changes since v1: > >>>> 1. Select-true, add maxItems and description for each entry (Rob). > >>>> 2. Mention ACTIVE_LOW in bindings description (Linus). > >>>> 3. Add allOf for pwrseq reset-gpios case. > >>>> > >>>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > >>>> --- > >>>> .../bindings/gpio/gpio-consumer-common.yaml | 64 +++++++++++++++++++ > >>>> 1 file changed, 64 insertions(+) > >>>> create mode 100644 Documentation/devicetree/bindings/gpio/gpio-consumer-common.yaml > >>>> > >>> > >>> 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/linux-dt-review/Documentation/devicetree/bindings/regulator/richtek,rt4801-regulator.example.dt.yaml: rt4801@73: enable-gpios: [[4294967295, 2, 0], [4294967295, 3, 0]] is too long > >>> From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/gpio/gpio-consumer-common.yaml > >>> > >> > >> Hi Rob, > >> > >> With v1, you proposed to use maxItems for all these standard gpios, but > >> as we see here there are two exceptions: > >> 1. pwrseq might have up to 32 reset-gpios, > >> 2. richtek,rt4801 uses up to 2 enable-gpios. > > > > There's always an outlier... > > > >> One way is to add exceptions in gpio-consumer-common.yaml, like I did > >> for reset-gpios and pwrseq. However this scales poor if more of such > >> usages appear. > > > > I'd reject any new cases, but even just 2 I don't really like. > > The richtek,rt4801 enable-gpios are for controlling two separate > regulators, so it should have been under regulator subnodes/children. > Some other regulators follow this pattern, so only this one is done that > way. > > That driver could be converted to enable-gpios per regulator, so if you > are sure about rejection of new cases, how about keeping current > exceptions in allOf:if? Okay. Rob
diff --git a/Documentation/devicetree/bindings/gpio/gpio-consumer-common.yaml b/Documentation/devicetree/bindings/gpio/gpio-consumer-common.yaml new file mode 100644 index 000000000000..40d0be31e200 --- /dev/null +++ b/Documentation/devicetree/bindings/gpio/gpio-consumer-common.yaml @@ -0,0 +1,64 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/gpio/gpio-consumer-common.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Common GPIO lines + +maintainers: + - Bartosz Golaszewski <brgl@bgdev.pl> + - Linus Walleij <linus.walleij@linaro.org> + +description: + Pay attention to using proper GPIO flag (e.g. GPIO_ACTIVE_LOW) for the GPIOs + using inverted signal (e.g. RESETN). + +select: true + +properties: + enable-gpios: + maxItems: 1 + description: + GPIO connected to the enable control pin. + + reset-gpios: + description: + GPIO (or GPIOs for power sequence) connected to the device reset pin + (e.g. RESET or RESETN). + + powerdown-gpios: + maxItems: 1 + description: + GPIO connected to the power down pin (hardware power down or power cut, + e.g. PD or PWDN). + + pwdn-gpios: + maxItems: 1 + description: Use powerdown-gpios + deprecated: true + + wakeup-gpios: + maxItems: 1 + description: + GPIO connected to the pin waking up the device from suspend or other + power-saving modes. + +allOf: + - if: + properties: + compatible: + contains: + enum: + - mmc-pwrseq-simple + then: + properties: + reset-gpios: + minItems: 1 + maxItems: 32 + else: + properties: + reset-gpios: + maxItems: 1 + +additionalProperties: true