On 14/10/2022 14:02, Doug Anderson wrote: > Hi, > > On Thu, Oct 13, 2022 at 11:49 AM Krzysztof Kozlowski > <krzysztof.kozlowski@xxxxxxxxxx> wrote: >> >> diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,sc7180-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/qcom,sc7180-pinctrl.yaml >> new file mode 100644 >> index 000000000000..464f1031d15d >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/pinctrl/qcom,sc7180-pinctrl.yaml > > [ ... cut ... ] > >> @@ -0,0 +1,162 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/pinctrl/qcom,sc7180-pinctrl.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Qualcomm SC7180 TLMM pin controller >> + >> +maintainers: >> + - Bjorn Andersson <andersson@xxxxxxxxxx> >> + - Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> >> + >> +description: >> + Top Level Mode Multiplexer pin controller in Qualcomm SC7180 SoC. >> + >> +properties: >> + compatible: >> + const: qcom,sc7180-pinctrl >> + >> + reg: >> + maxItems: 3 >> + >> + reg-names: >> + items: >> + - const: west >> + - const: north >> + - const: south >> + >> + interrupts: true >> + interrupt-controller: true >> + "#interrupt-cells": true >> + gpio-controller: true >> + "#gpio-cells": true >> + gpio-ranges: true >> + wakeup-parent: true > > Do you need interrupts/interrupt-controller/.../... ? Below you > include allOf "/schemas/pinctrl/qcom,tlmm-common.yaml". Won't you > magically get those from there? Why do you need to duplicate this? There is additionalProperties:false, so every property from referenced schema has to be included. The true solution would be to switch to unevaluatedProperties:false but then we accept all properties from: 1. qcom,tlmm-common.yaml - not a problem, we already mention all of them here with :true 2. pinctrl.yaml - we would now allow to use pinctrl-use-default. 3. any other future common properties. > > >> + gpio-reserved-ranges: >> + minItems: 1 >> + maxItems: 60 >> + >> + gpio-line-names: >> + maxItems: 119 >> + >> +patternProperties: >> + "-state$": >> + oneOf: >> + - $ref: "#/$defs/qcom-sc7180-tlmm-state" >> + - patternProperties: >> + "-pins$": >> + $ref: "#/$defs/qcom-sc7180-tlmm-state" >> + additionalProperties: false >> + >> +$defs: >> + qcom-sc7180-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: >> + oneOf: >> + - pattern: "^gpio([0-9]|[1-9][0-9]|10[0-9]|11[0-8])$" >> + - enum: [ sdc1_rclk, sdc1_clk, sdc1_cmd, sdc1_data, sdc2_clk, >> + sdc2_cmd, sdc2_data, ufs_reset ] >> + minItems: 1 >> + maxItems: 36 >> + >> + function: >> + description: >> + Specify the alternative function to be configured for the specified >> + pins. >> + >> + enum: [ adsp_ext, agera_pll, aoss_cti, atest_char, atest_char0, >> + atest_char1, atest_char2, atest_char3, atest_tsens, >> + atest_tsens2, atest_usb1, atest_usb10, atest_usb11, >> + atest_usb12, atest_usb13, atest_usb2, atest_usb20, atest_usb21, >> + atest_usb22, atest_usb23, audio_ref, btfm_slimbus, cam_mclk, >> + cci_async, cci_i2c, cci_timer0, cci_timer1, cci_timer2, >> + cci_timer3, cci_timer4, cri_trng, dbg_out, ddr_bist, ddr_pxi0, >> + ddr_pxi1, ddr_pxi2, ddr_pxi3, dp_hot, edp_lcd, gcc_gp1, >> + gcc_gp2, gcc_gp3, gpio, gp_pdm0, gp_pdm1, gp_pdm2, gps_tx, >> + jitter_bist, ldo_en, ldo_update, lpass_ext, mdp_vsync, >> + mdp_vsync0, mdp_vsync1, mdp_vsync2, mdp_vsync3, mi2s_0, mi2s_1, >> + mi2s_2, mss_lte, m_voc, pa_indicator, phase_flag, PLL_BIST, >> + pll_bypassnl, pll_reset, prng_rosc, qdss, qdss_cti, >> + qlink_enable, qlink_request, qspi_clk, qspi_cs, qspi_data, >> + qup00, qup01, qup02_i2c, qup02_uart, qup03, qup04_i2c, >> + qup04_uart, qup05, qup10, qup11_i2c, qup11_uart, qup12, >> + qup13_i2c, qup13_uart, qup14, qup15, sdc1_tb, sdc2_tb, >> + sd_write, sp_cmu, tgu_ch0, tgu_ch1, tgu_ch2, tgu_ch3, >> + tsense_pwm1, tsense_pwm2, uim1, uim2, uim_batt, usb_phy, vfr_1, >> + _V_GPIO, _V_PPS_IN, _V_PPS_OUT, vsense_trigger, wlan1_adc0, >> + wlan1_adc1, wlan2_adc0, wlan2_adc1 ] >> + >> + drive-strength: >> + enum: [2, 4, 6, 8, 10, 12, 14, 16] >> + description: >> + Selects the drive strength for the specified pins, in mA. > > Again, why do you need to duplicate. The yaml is weak in me, but I > think you're effectively subclassing > "qcom,tlmm-common.yaml#/$defs/qcom-tlmm-state", right? It has the > exact same definition of drive-strength. Why duplicate it? Yes, this can be dropped. > > >> + bias-pull-down: true >> + bias-pull-up: true >> + bias-disable: true >> + input-enable: true >> + output-high: true >> + output-low: true > > Again, maybe the above properties aren't needed? Don't you inherit > them from tlmm-common? >From tlmm-common yes, however without this and additionalProperties:false we would accept everything from pincfg-node which too much. The binding/hardware does not support all of them, I think. Best regards, Krzysztof
