linux-kernel - Re: [PATCH 1/2] dt-bindings: pinctrl: qcom: Add sc8280xp lpass lpi pinctrl bindings

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Wed, 17 Aug 2022 09:57:42 +0100
From:   Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        agross@...nel.org, bjorn.andersson@...aro.org,
        linus.walleij@...aro.org
Cc:     konrad.dybcio@...ainline.org, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, linux-arm-msm@...r.kernel.org,
        linux-gpio@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] dt-bindings: pinctrl: qcom: Add sc8280xp lpass lpi
 pinctrl bindings

Thanks Krzysztof,

On 17/08/2022 07:05, Krzysztof Kozlowski wrote:
> On 16/08/2022 21:05, Srinivas Kandagatla wrote:
>> Add device tree binding Documentation details for Qualcomm SC8280XP
>> LPASS(Low Power Audio Sub System) LPI(Low Power Island) pinctrl driver.
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
> 
> Thank you for your patch. There is something to discuss/improve.
> 
>> +  gpio-ranges:
>> +    maxItems: 1
>> +
>> +#PIN CONFIGURATION NODES
>> +patternProperties:
>> +  '-pins$':
>> +    type: object
>> +    description:
>> +      Pinctrl node's client devices use subnodes for desired pin configuration.
>> +      Client device subnodes use below standard properties.
>> +    $ref: "/schemas/pinctrl/pincfg-node.yaml"
> 
> Drop the quotes
Will do that.
.
> 
>> +
>> +    properties:
>> +      pins:
>> +        description:
>> +          List of gpio pins affected by the properties specified in this
>> +          subnode.
>> +        items:
>> +          pattern: "^gpio([0-1]|[0-8]])$"
> 
> error in pattern - double ]. If you have 19 GPIOs, this should be
> probably: ^gpio([0-9]|1[0-8])$
> 
that is true..I did overlook  '|'

>> +
>> +      function:
>> +        enum: [ swr_tx_clk, swr_tx_data, swr_rx_clk, swr_rx_data,
>> +                dmic1_clk, dmic1_data, dmic2_clk, dmic2_data, dmic4_clk,
>> +                dmic4_data, i2s2_clk, i2s2_ws, dmic3_clk, dmic3_data,
>> +                qua_mi2s_sclk, qua_mi2s_ws, qua_mi2s_data, i2s1_clk, i2s1_ws,
>> +                i2s1_data, wsa_swr_clk, wsa_swr_data, wsa2_swr_clk,
>> +                wsa2_swr_data, i2s2_data, i2s3_clk, i2s3_ws, i2s3_data,
>> +                ext_mclk1_c, ext_mclk1_b, ext_mclk1_a ]
>> +
> 
> Skip blank line (confuses with a new property).
> 
okay

>> +        description:
>> +          Specify the alternative function to be configured for the specified
>> +          pins.
>> +
>> +      drive-strength:
>> +        enum: [2, 4, 6, 8, 10, 12, 14, 16]
>> +        default: 2
>> +        description:
>> +          Selects the drive strength for the specified pins, in mA.
>> +
>> +      slew-rate:
>> +        enum: [0, 1, 2, 3]
>> +        default: 0
>> +        description: |
>> +          0: No adjustments
>> +          1: Higher Slew rate (faster edges)
>> +          2: Lower Slew rate (slower edges)
>> +          3: Reserved (No adjustments)
>> +
>> +      bias-pull-down: true
>> +
>> +      bias-pull-up: true
>> +
>> +      bias-disable: true
>> +
>> +      output-high: true
>> +
>> +      output-low: true
>> +
>> +    required:
>> +      - pins
>> +      - function
>> +
>> +    additionalProperties: false
>> +
>> +allOf:
>> +  - $ref: "pinctrl.yaml#"
> 
> Drop the quotes.
> 
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - clocks
>> +  - clock-names
>> +  - gpio-controller
>> +  - '#gpio-cells'
>> +  - gpio-ranges
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/sound/qcom,q6afe.h>
>> +    lpi_tlmm: pinctrl@...0000 {
> 
> Drop the label, not used anywhere here.
> 
makes sense, I will address all the comments and post next version.
thanks,
srini
>> +        compatible = "qcom,sc8280xp-lpass-lpi-pinctrl";
>> +        reg = <0x33c0000 0x20000>,
>> +              <0x3550000 0x10000>;
>> +        clocks = <&q6afecc LPASS_HW_MACRO_VOTE LPASS_CLK_ATTRIBUTE_COUPLE_NO>,
>> +                 <&q6afecc LPASS_HW_DCODEC_VOTE LPASS_CLK_ATTRIBUTE_COUPLE_NO>;
>> +        clock-names = "core", "audio";
>> +        gpio-controller;
>> +        #gpio-cells = <2>;
>> +        gpio-ranges = <&lpi_tlmm 0 0 18>;
>> +    };
> 
> 
> Best regards,
> Krzysztof