Re: [PATCH V3 3/9] dt-bindings: pinctrl: qcom: Add support for ipq5018

[Sricharan Ramabadhran <quic_srichara@quicinc.com>]

On 4/16/2023 2:35 PM, Krzysztof Kozlowski wrote:
> On 14/04/2023 12:29, Sricharan Ramabadhran wrote:
>> Add device tree binding Documentation details for ipq5018
>> pinctrl driver.
>>
> 
> Thank you for your patch. There is something to discuss/improve.
> 
>> +
>> +  interrupt-controller: true
>> +  "#interrupt-cells": true
>> +  gpio-controller: true
>> +  "#gpio-cells": true
>> +  gpio-ranges: true
>> +  wakeup-parent: true
>> +
>> +  gpio-reserved-ranges:
>> +    minItems: 1
>> +    maxItems: 33
> 
> 24
> (you cannot have more than 24...)
> 
  ok, will change this.

>> +
>> +  gpio-line-names:
>> +    maxItems: 47
>> +
>> +patternProperties:
>> +  "-state$":
>> +    oneOf:
>> +      - $ref: "#/$defs/qcom-ipq5018-tlmm-state"
>> +      - patternProperties:
>> +          "-pins$":
>> +            $ref: "#/$defs/qcom-ipq5018-tlmm-state"
>> +        additionalProperties: false
>> +
>> +$defs:
>> +  qcom-ipq5018-tlmm-state:
>> +    type: object
>> +    description:
>> +      Pinctrl node's client devices use subnodes for desired pin configuration.
>> +      Client device subnodes use below standard properties.
>> +    $ref: qcom,tlmm-common.yaml#/$defs/qcom-tlmm-state
>> +
>> +    properties:
>> +      pins:
>> +        description:
>> +          List of gpio pins affected by the properties specified in this
>> +          subnode.
>> +        items:
>> +          pattern: "^gpio([0-9]|[1-3][0-9]|4[0-6])$"
>> +        minItems: 1
>> +        maxItems: 8
>> +
>> +      function:
>> +        description:
>> +          Specify the alternative function to be configured for the specified
>> +          pins.
>> +
>> +        enum: [ atest_char, audio_pdm0, audio_pdm1, audio_rxbclk, audio_rxd,
>> +                audio_rxfsync, audio_rxmclk, audio_txbclk, audio_txd,
>> +                audio_txfsync, audio_txmclk, blsp0_i2c, blsp0_spi, blsp0_uart0,
>> +                blsp0_uart1, blsp1_i2c0, blsp1_i2c1, blsp1_spi0, blsp1_spi1,
>> +                blsp1_uart0, blsp1_uart1, blsp1_uart2, blsp2_i2c0, blsp2_i2c1,
>> +                blsp2_spi, blsp2_spi0, blsp2_spi1, btss, burn0, burn1, cri_trng,
>> +                cri_trng0, cri_trng1, cxc_clk, cxc_data, dbg_out, eud_gpio,
>> +                gcc_plltest, gcc_tlmm, gpio, led0, led2, mac0, mac1, mdc, mdio,
>> +                pcie0_clk, pcie0_wake, pcie1_clk, pcie1_wake, pll_test,
>> +                prng_rosc, pwm0, pwm1, pwm2, pwm3, qdss_cti_trig_in_a0,
>> +                qdss_cti_trig_in_a1, qdss_cti_trig_in_b0, qdss_cti_trig_in_b1,
>> +                qdss_cti_trig_out_a0, qdss_cti_trig_out_a1,
>> +                qdss_cti_trig_out_b0, qdss_cti_trig_out_b1, qdss_traceclk_a,
>> +                qdss_traceclk_b, qdss_tracectl_a, qdss_tracectl_b,
>> +                qdss_tracedata_a, qdss_tracedata_b, qspi_clk, qspi_cs,
>> +                qspi_data, reset_out, sdc1_clk, sdc1_cmd, sdc1_data, wci_txd,
>> +                wci_rxd, wsa_swrm, wsi_clk3, wsi_data3, wsis_reset, xfem ]
>> +
>> +      bias-pull-down: true
>> +      bias-pull-up: true
>> +      bias-disable: true
>> +      drive-strength: true
>> +      input-enable: true
>> +      output-high: true
>> +      output-low: true
> 
> Drop all these 7, especially that input-enable is not allowed explicitly.
> 
   ok.

>> +
>> +    required:
>> +      - pins
>> +
>> +    additionalProperties: false
> 
> Instead:
> unevaluatedProperties: false
> and put it after the $ref above. Just like recent changes in the next.
> 
   ok, will check and fix it.

>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>> +
>> +    tlmm: pinctrl@1000000 {
>> +        compatible = "qcom,ipq5018-tlmm";
>> +        reg = <0x01000000 0x300000>;
>> +        gpio-controller;
>> +        #gpio-cells = <2>;
>> +        gpio-ranges = <&tlmm 0 0 47>;
>> +        interrupts = <GIC_SPI 208 IRQ_TYPE_LEVEL_HIGH>;
>> +        interrupt-controller;
>> +        #interrupt-cells = <2>;
>> +
>> +        uart2-state {
>> +            pins = "gpio34", "gpio35";
>> +            function = "blsp2_uart";
> 
> Does not look like you tested the bindings. Please run `make
> dt_binding_check` (see
> Documentation/devicetree/bindings/writing-schema.rst for instructions).

   I ran it against the TOT, will run it this time on nxt and post V4.


Regards,
  Sricharan