Re: [PATCH V5 1/2] dt-bindings: pinctrl: qcom-pmic-gpio: Convert qcom pmic gpio bindings to YAML

[skakit]

On 2021-07-14 08:16, Bjorn Andersson wrote:
> On Wed 30 Jun 00:50 CDT 2021, satya priya wrote:
>
> > Convert Qualcomm PMIC GPIO bindings from .txt to .yaml format.
>
> Thanks for updating this Satya, this is looking quite good now. Just 
> got
> one issue with the definition of the state child node.
[..]
> > +
> > +  interrupts:
> > +    minItems: 1
> > +    maxItems: 44
> > +    description: |
> > +        Must contain an array of encoded interrupt specifiers for
> > +        each available GPIO
> > +
> > +  '#interrupt-cells':
> > +    const: 2
> > +
> > +  interrupt-controller: true
> > +
> > +  gpio-controller: true
> > +
> > +  gpio-ranges:
> > +    maxItems: 1
> > +
> > +  '#gpio-cells':
> > +    const: 2
> > +    description: |
>
> Not need for the '|', as the formatting isn't significant.

Okay.

> > +        The first cell will be used to define gpio number and the
> > +        second denotes the flags for this gpio
> > +
> > +additionalProperties: false
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - gpio-controller
> > +  - '#gpio-cells'
> > +  - gpio-ranges
> > +
> > +patternProperties:
> > +  '.*':
>
> I would prefer this match to follow tlmm and go '-state$'

okay.

> > +    anyOf:
>
> Either we want the immediate child node to match gpio-pinctrl-state
> or we want one of more children matching gpio-pinctrl-state, but not
> both. So "oneOf".

Okay.

> > +      - $ref: "pinmux-node.yaml"
> > +      - $ref: "pincfg-node.yaml"
>
> The generic definition is not sufficient, you want this to be
>
> 	- $ref: "#/$defs/gpio-pinctrl-state"

Okay, I will add this ref and remove generic ones as we are already 
referring them in 'qcom-pmic-gpio-state' definition.

> > +      - patternProperties:
> > +          ".*":
>
> The subnodes of the state can be named whatever, so this (the .*) is
> good.
>
> > +            $ref: "#/$defs/gpio-pinctrl-state"
> > +
> > +$defs:
> > +  gpio-pinctrl-state:
>
> This is too generic, how about qcom-pmic-gpio-state?

ok.

> > +    type: object
> > +    anyOf:
>
> I have this as "allOf" in the TLMM binding, not entirely sure what the
> implications of anyOf here would be though...

Sure, will change it to allOf.

> > +      - $ref: "pinmux-node.yaml"
> > +      - $ref: "pincfg-node.yaml"
> > +    properties:
> > +      pins:
> > +        description: |
> > +            List of gpio pins affected by the properties specified in
> > +            this subnode.  Valid pins are
> > +                 - gpio1-gpio10 for pm6150
> > +                 - gpio1-gpio12 for pm6150l
> > +                 - gpio1-gpio10 for pm7325
> > +                 - gpio1-gpio4 for pm8005
> > +                 - gpio1-gpio2 for pm8008
> > +                 - gpio1-gpio6 for pm8018
> > +                 - gpio1-gpio12 for pm8038
> > +                 - gpio1-gpio40 for pm8058
> > +                 - gpio1-gpio10 for pm8150 (holes on gpio2, gpio5,
> > +                                            gpio7 and gpio8)
> > +                 - gpio1-gpio12 for pm8150b (holes on gpio3, gpio4
> > +                                             and gpio7)
> > +                 - gpio1-gpio12 for pm8150l (hole on gpio7)
> > +                 - gpio1-gpio4 for pm8916
> > +                 - gpio1-gpio10 for pm8350
> > +                 - gpio1-gpio8 for pm8350b
> > +                 - gpio1-gpio9 for pm8350c
> > +                 - gpio1-gpio38 for pm8917
> > +                 - gpio1-gpio44 for pm8921
> > +                 - gpio1-gpio36 for pm8941
> > +                 - gpio1-gpio8 for pm8950 (hole on gpio3)
> > +                 - gpio1-gpio22 for pm8994
> > +                 - gpio1-gpio26 for pm8998
> > +                 - gpio1-gpio22 for pma8084
> > +                 - gpio1-gpio2 for pmi8950
> > +                 - gpio1-gpio10 for pmi8994
> > +                 - gpio1-gpio4 for pmk8350
> > +                 - gpio1-gpio4 for pmr735a
> > +                 - gpio1-gpio4 for pmr735b
> > +                 - gpio1-gpio12 for pms405 (holes on gpio1, gpio9
> > +                                            and gpio10)
> > +                 - gpio1-gpio11 for pmx55 (holes on gpio3, gpio7, 
> > gpio10
> > +                                            and gpio11)
> > +
> > +        items:
> > +          pattern: "^gpio([0-9]+)$"
> > +
> > +      function:
> > +        items:
> > +          - enum:
> > +              - normal
> > +              - paired
> > +              - func1
> > +              - func2
> > +              - dtest1
> > +              - dtest2
> > +              - dtest3
> > +              - dtest4
> > +              - func3  # supported by LV/MV GPIO subtypes
> > +              - func4  # supported by LV/MV GPIO subtypes
> > +
> > +      bias-disable: true
> > +      bias-pull-down: true
> > +      bias-pull-up: true
> > +
> > +      qcom,pull-up-strength:
> > +        $ref: /schemas/types.yaml#/definitions/uint32
> > +        description: |
> > +            Specifies the strength to use for pull up, if selected.
> > +            Valid values are defined in
> > +            <dt-bindings/pinctrl/qcom,pmic-gpio.h>
> > +            If this property is omitted 30uA strength will be used
> > +            if pull up is selected
>
>
> enum: [0, 1, 2, 3]
okay.
> > +
> > +      bias-high-impedance: true

> > +
> > +      gpio-controller;
> > +      gpio-ranges = <&pm8921_gpio 0 0 44>;
> > +      #gpio-cells = <2>;
> > +
> > +      pm8921_gpio_keys: gpio-keys {
>
> Per my above request of changing the pattern, this would have to be
> "gpio-keys-state" instead.

okay.

Thanks,
Satya Priya

> Regards,
> bjorn
>
> > +        volume-keys {
> > +          pins = "gpio20", "gpio21";
> > +          function = "normal";
> > +
> > +          input-enable;
> > +          bias-pull-up;
> > +          drive-push-pull;
> > +          qcom,drive-strength = <PMIC_GPIO_STRENGTH_NO>;
> > +          power-source = <PM8921_GPIO_S4>;
> > +        };
> > +      };
> > +    };
> > +...
> > --
> > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a 
> > member
> > of Code Aurora Forum, hosted by The Linux Foundation