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

[skakit@codeaurora.org]

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