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

Sricharan Ramabadhran
2023-04-17T06:13:42Z

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