diff mbox series

[v4,1/2] dt-bindings: pinctrl: qcom: Add SM8350 pinctrl bindings

Message ID 20210106054950.303244-2-vkoul@kernel.org (mailing list archive)
State Superseded
Headers show
Series pinctrl: qcom: Add SM8350 pinctrl support | expand

Commit Message

Vinod Koul Jan. 6, 2021, 5:49 a.m. UTC 
Add device tree binding Documentation details for Qualcomm SM8350
pinctrl driver.

Signed-off-by: Vinod Koul <vkoul@kernel.org>
---
 .../bindings/pinctrl/qcom,sm8350-tlmm.yaml    | 149 ++++++++++++++++++
 1 file changed, 149 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/qcom,sm8350-tlmm.yaml

Comments

Bjorn Andersson Jan. 7, 2021, 5:17 p.m. UTC | #1 
On Tue 05 Jan 23:49 CST 2021, Vinod Koul wrote:

> Add device tree binding Documentation details for Qualcomm SM8350
> pinctrl driver.
> 
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> ---
>  .../bindings/pinctrl/qcom,sm8350-tlmm.yaml    | 149 ++++++++++++++++++
>  1 file changed, 149 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pinctrl/qcom,sm8350-tlmm.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,sm8350-tlmm.yaml b/Documentation/devicetree/bindings/pinctrl/qcom,sm8350-tlmm.yaml
> new file mode 100644
> index 000000000000..abdafd25bfc2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/qcom,sm8350-tlmm.yaml
> @@ -0,0 +1,149 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pinctrl/qcom,sm8350-tlmm.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm Technologies, Inc. SM8350 TLMM block
> +
> +maintainers:
> +  - Vinod Koul <vkoul@kernel.org>
> +
> +description: |
> +  This binding describes the Top Level Mode Multiplexer block found in the
> +  SM8350 platform.
> +
> +properties:
> +  compatible:
> +    const: qcom,sm8350-tlmm
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  interrupt-controller: true
> +
> +  '#interrupt-cells':
> +    description: Specifies the PIN numbers and Flags, as defined in
> +      include/dt-bindings/interrupt-controller/irq.h
> +    const: 2
> +
> +  gpio-controller: true
> +
> +  '#gpio-cells':
> +    description: Specifying the pin number and flags, as defined in
> +      include/dt-bindings/gpio/gpio.h
> +    const: 2
> +
> +  gpio-ranges:
> +    maxItems: 1
> +
> +  gpio-reserved-ranges:
> +    maxItems: 1
> +
> +#PIN CONFIGURATION NODES
> +patternProperties:
> +  '-pinmux$':

I believe that what Rob was asking for was the matter of describing the
mux and config subnodes under this one. But I don't know really how to
express this, because the following are all valid:

default_state: default-state {
	pins = "gpio1";
	bias-disable;
};

default_state: default-state {
	rx {
		pins = "gpio1";
		function = "gpio";
		bias-disable;
	};
};

default_state: default-state {
	pinmux {
		pins = "gpio1";
		function = "gpio";
	};

	pinconf {
		pins = "gpio1";
		bias-disable;
	};
};

I.e. the properties described here applies either to this node directly,
or any subnodes (1 level) down.


Also we've been using different "patternProperties" for this node since
the introduction of the binding 7 years ago. But to be "-state$" seems
to best represent what the node actually describes.

Regards,
Bjorn

> +    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"
> +
> +    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]|1[0-9][0-9]|20[0-3])$"
> +            - enum: [ sdc1_clk, sdc1_cmd, sdc1_data, sdc2_clk, sdc2_cmd, sdc2_data ]
> +        minItems: 1
> +        maxItems: 36
> +
> +      function:
> +        description:
> +          Specify the alternative function to be configured for the specified
> +          pins. Functions are only valid for gpio pins.
> +        enum: [ atest_char, atest_usb, audio_ref, cam_mclk, cci_async,
> +                cci_i2c, cci_timer, cmu_rng, coex_uart1, coex_uart2, cri_trng,
> +                cri_trng0, cri_trng1, dbg_out, ddr_bist, ddr_pxi0, ddr_pxi1,
> +                ddr_pxi2, ddr_pxi3, dp_hot, dp_lcd, gcc_gp1, gcc_gp2, gcc_gp3,
> +                gpio, ibi_i3c, jitter_bist, lpass_slimbus, mdp_vsync, mdp_vsync0,
> +                mdp_vsync1, mdp_vsync2, mdp_vsync3, mi2s0_data0, mi2s0_data1,
> +                mi2s0_sck, mi2s0_ws, mi2s1_data0, mi2s1_data1, mi2s1_sck,
> +                mi2s1_ws, mi2s2_data0, mi2s2_data1, mi2s2_sck, mi2s2_ws,
> +                mss_grfc0, mss_grfc1, mss_grfc10, mss_grfc11, mss_grfc12,
> +                mss_grfc2, mss_grfc3, mss_grfc4, mss_grfc5, mss_grfc6,
> +                mss_grfc7, mss_grfc8, mss_grfc9, nav_gpio, pa_indicator,
> +                pcie0_clkreqn, pcie1_clkreqn, phase_flag, pll_bist, pll_clk,
> +                pri_mi2s, prng_rosc, qdss_cti, qdss_gpio, qlink0_enable,
> +                qlink0_request, qlink0_wmss, qlink1_enable, qlink1_request,
> +                qlink1_wmss, qlink2_enable, qlink2_request, qlink2_wmss, qspi0,
> +                qspi1, qspi2, qspi3, qspi_clk, qspi_cs, qup0, qup1, qup10,
> +                qup11, qup12, qup13, qup14, qup15, qup16, qup17, qup18, qup19,
> +                qup2, qup3, qup4, qup5, qup6, qup7, qup8, qup9, qup_l4, qup_l5,
> +                qup_l6, sd_write, sdc40, sdc41, sdc42, sdc43, sdc4_clk,
> +                sdc4_cmd, sec_mi2s, tb_trig, tgu_ch0, tgu_ch1, tgu_ch2,
> +                tgu_ch3, tsense_pwm1, tsense_pwm2, uim0_clk, uim0_data,
> +                uim0_present, uim0_reset, uim1_clk, uim1_data, uim1_present,
> +                uim1_reset, usb2phy_ac, usb_phy, vfr_0, vfr_1, vsense_trigger ]
> +
> +
> +      drive-strength:
> +        enum: [2, 4, 6, 8, 10, 12, 14, 16]
> +        default: 2
> +        description:
> +          Selects the drive strength for the specified pins, in mA.
> +
> +      bias-pull-down: true
> +
> +      bias-pull-up: true
> +
> +      bias-disable: true
> +
> +      output-high: true
> +
> +      output-low: true
> +
> +    required:
> +      - pins
> +      - function
> +
> +    additionalProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - interrupt-controller
> +  - '#interrupt-cells'
> +  - gpio-controller
> +  - '#gpio-cells'
> +  - gpio-ranges
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +        #include <dt-bindings/interrupt-controller/arm-gic.h>
> +        tlmm: pinctrl@f000000 {
> +          compatible = "qcom,sm8350-tlmm";
> +          reg = <0x0f100000 0x300000>;
> +          interrupts = <GIC_SPI 208 IRQ_TYPE_LEVEL_HIGH>;
> +          gpio-controller;
> +          #gpio-cells = <2>;
> +          interrupt-controller;
> +          #interrupt-cells = <2>;
> +          gpio-ranges = <&tlmm 0 0 203>;
> +          serial-pinmux {
> +            pins = "gpio18", "gpio19";
> +            function = "qup3";
> +            drive-strength = <8>;
> +            bias-disable;
> +          };
> +        };
> +
> +...
> -- 
> 2.26.2
>
Vinod Koul Jan. 8, 2021, 7:57 a.m. UTC | #2 
Hi Bjorn,

On 07-01-21, 11:17, Bjorn Andersson wrote:
> On Tue 05 Jan 23:49 CST 2021, Vinod Koul wrote:
> > +#PIN CONFIGURATION NODES
> > +patternProperties:
> > +  '-pinmux$':
> 
> I believe that what Rob was asking for was the matter of describing the
> mux and config subnodes under this one. But I don't know really how to
> express this, because the following are all valid:

I looked at the pinmux-node.yaml which describes subnodes with function
and groups, this is a generic description and should be in
pinmux-node.yaml not in every device description.. said that I am not
sure why else should we add here :)

> 
> default_state: default-state {
> 	pins = "gpio1";
> 	bias-disable;
> };
> 
> default_state: default-state {
> 	rx {
> 		pins = "gpio1";
> 		function = "gpio";
> 		bias-disable;
> 	};
> };
> 
> default_state: default-state {
> 	pinmux {
> 		pins = "gpio1";
> 		function = "gpio";
> 	};
> 
> 	pinconf {
> 		pins = "gpio1";
> 		bias-disable;
> 	};
> };
> 
> I.e. the properties described here applies either to this node directly,
> or any subnodes (1 level) down.
> 
> 
> Also we've been using different "patternProperties" for this node since
> the introduction of the binding 7 years ago. But to be "-state$" seems
> to best represent what the node actually describes.
> 
> Regards,
> Bjorn
> 
> > +    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"
> > +
> > +    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]|1[0-9][0-9]|20[0-3])$"
> > +            - enum: [ sdc1_clk, sdc1_cmd, sdc1_data, sdc2_clk, sdc2_cmd, sdc2_data ]
> > +        minItems: 1
> > +        maxItems: 36
> > +
> > +      function:
> > +        description:
> > +          Specify the alternative function to be configured for the specified
> > +          pins. Functions are only valid for gpio pins.
> > +        enum: [ atest_char, atest_usb, audio_ref, cam_mclk, cci_async,
> > +                cci_i2c, cci_timer, cmu_rng, coex_uart1, coex_uart2, cri_trng,
> > +                cri_trng0, cri_trng1, dbg_out, ddr_bist, ddr_pxi0, ddr_pxi1,
> > +                ddr_pxi2, ddr_pxi3, dp_hot, dp_lcd, gcc_gp1, gcc_gp2, gcc_gp3,
> > +                gpio, ibi_i3c, jitter_bist, lpass_slimbus, mdp_vsync, mdp_vsync0,
> > +                mdp_vsync1, mdp_vsync2, mdp_vsync3, mi2s0_data0, mi2s0_data1,
> > +                mi2s0_sck, mi2s0_ws, mi2s1_data0, mi2s1_data1, mi2s1_sck,
> > +                mi2s1_ws, mi2s2_data0, mi2s2_data1, mi2s2_sck, mi2s2_ws,
> > +                mss_grfc0, mss_grfc1, mss_grfc10, mss_grfc11, mss_grfc12,
> > +                mss_grfc2, mss_grfc3, mss_grfc4, mss_grfc5, mss_grfc6,
> > +                mss_grfc7, mss_grfc8, mss_grfc9, nav_gpio, pa_indicator,
> > +                pcie0_clkreqn, pcie1_clkreqn, phase_flag, pll_bist, pll_clk,
> > +                pri_mi2s, prng_rosc, qdss_cti, qdss_gpio, qlink0_enable,
> > +                qlink0_request, qlink0_wmss, qlink1_enable, qlink1_request,
> > +                qlink1_wmss, qlink2_enable, qlink2_request, qlink2_wmss, qspi0,
> > +                qspi1, qspi2, qspi3, qspi_clk, qspi_cs, qup0, qup1, qup10,
> > +                qup11, qup12, qup13, qup14, qup15, qup16, qup17, qup18, qup19,
> > +                qup2, qup3, qup4, qup5, qup6, qup7, qup8, qup9, qup_l4, qup_l5,
> > +                qup_l6, sd_write, sdc40, sdc41, sdc42, sdc43, sdc4_clk,
> > +                sdc4_cmd, sec_mi2s, tb_trig, tgu_ch0, tgu_ch1, tgu_ch2,
> > +                tgu_ch3, tsense_pwm1, tsense_pwm2, uim0_clk, uim0_data,
> > +                uim0_present, uim0_reset, uim1_clk, uim1_data, uim1_present,
> > +                uim1_reset, usb2phy_ac, usb_phy, vfr_0, vfr_1, vsense_trigger ]
> > +
> > +
> > +      drive-strength:
> > +        enum: [2, 4, 6, 8, 10, 12, 14, 16]
> > +        default: 2
> > +        description:
> > +          Selects the drive strength for the specified pins, in mA.
> > +
> > +      bias-pull-down: true
> > +
> > +      bias-pull-up: true
> > +
> > +      bias-disable: true
> > +
> > +      output-high: true
> > +
> > +      output-low: true
> > +
> > +    required:
> > +      - pins
> > +      - function
> > +
> > +    additionalProperties: false
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +  - interrupt-controller
> > +  - '#interrupt-cells'
> > +  - gpio-controller
> > +  - '#gpio-cells'
> > +  - gpio-ranges
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +        #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +        tlmm: pinctrl@f000000 {
> > +          compatible = "qcom,sm8350-tlmm";
> > +          reg = <0x0f100000 0x300000>;
> > +          interrupts = <GIC_SPI 208 IRQ_TYPE_LEVEL_HIGH>;
> > +          gpio-controller;
> > +          #gpio-cells = <2>;
> > +          interrupt-controller;
> > +          #interrupt-cells = <2>;
> > +          gpio-ranges = <&tlmm 0 0 203>;
> > +          serial-pinmux {
> > +            pins = "gpio18", "gpio19";
> > +            function = "qup3";
> > +            drive-strength = <8>;
> > +            bias-disable;
> > +          };
> > +        };
> > +
> > +...
> > -- 
> > 2.26.2
> >
Bjorn Andersson Jan. 8, 2021, 6:40 p.m. UTC | #3 
On Fri 08 Jan 01:57 CST 2021, Vinod Koul wrote:

> Hi Bjorn,
> 
> On 07-01-21, 11:17, Bjorn Andersson wrote:
> > On Tue 05 Jan 23:49 CST 2021, Vinod Koul wrote:
> > > +#PIN CONFIGURATION NODES
> > > +patternProperties:
> > > +  '-pinmux$':
> > 
> > I believe that what Rob was asking for was the matter of describing the
> > mux and config subnodes under this one. But I don't know really how to
> > express this, because the following are all valid:
> 
> I looked at the pinmux-node.yaml which describes subnodes with function
> and groups, this is a generic description and should be in
> pinmux-node.yaml not in every device description.. said that I am not
> sure why else should we add here :)
> 

Unfortunately I don't think I understand how the pinmux-node.yaml
applies to the two possible levels of nodes.

Also the description of our state nodes would have to be a mix of
pinmux-node.yaml, pincfg-node.yaml and then the properties/constraints
described here, plus something indicating that many of the common
properties are not valid ones for this binding.


Regardless of how to describe that though, I would like the
patternProperties to be "-state$" :)

Regards,
Bjorn

> > 
> > default_state: default-state {
> > 	pins = "gpio1";
> > 	bias-disable;
> > };
> > 
> > default_state: default-state {
> > 	rx {
> > 		pins = "gpio1";
> > 		function = "gpio";
> > 		bias-disable;
> > 	};
> > };
> > 
> > default_state: default-state {
> > 	pinmux {
> > 		pins = "gpio1";
> > 		function = "gpio";
> > 	};
> > 
> > 	pinconf {
> > 		pins = "gpio1";
> > 		bias-disable;
> > 	};
> > };
> > 
> > I.e. the properties described here applies either to this node directly,
> > or any subnodes (1 level) down.
> > 
> > 
> > Also we've been using different "patternProperties" for this node since
> > the introduction of the binding 7 years ago. But to be "-state$" seems
> > to best represent what the node actually describes.
> > 
> > Regards,
> > Bjorn
> > 
> > > +    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"
> > > +
> > > +    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]|1[0-9][0-9]|20[0-3])$"
> > > +            - enum: [ sdc1_clk, sdc1_cmd, sdc1_data, sdc2_clk, sdc2_cmd, sdc2_data ]
> > > +        minItems: 1
> > > +        maxItems: 36
> > > +
> > > +      function:
> > > +        description:
> > > +          Specify the alternative function to be configured for the specified
> > > +          pins. Functions are only valid for gpio pins.
> > > +        enum: [ atest_char, atest_usb, audio_ref, cam_mclk, cci_async,
> > > +                cci_i2c, cci_timer, cmu_rng, coex_uart1, coex_uart2, cri_trng,
> > > +                cri_trng0, cri_trng1, dbg_out, ddr_bist, ddr_pxi0, ddr_pxi1,
> > > +                ddr_pxi2, ddr_pxi3, dp_hot, dp_lcd, gcc_gp1, gcc_gp2, gcc_gp3,
> > > +                gpio, ibi_i3c, jitter_bist, lpass_slimbus, mdp_vsync, mdp_vsync0,
> > > +                mdp_vsync1, mdp_vsync2, mdp_vsync3, mi2s0_data0, mi2s0_data1,
> > > +                mi2s0_sck, mi2s0_ws, mi2s1_data0, mi2s1_data1, mi2s1_sck,
> > > +                mi2s1_ws, mi2s2_data0, mi2s2_data1, mi2s2_sck, mi2s2_ws,
> > > +                mss_grfc0, mss_grfc1, mss_grfc10, mss_grfc11, mss_grfc12,
> > > +                mss_grfc2, mss_grfc3, mss_grfc4, mss_grfc5, mss_grfc6,
> > > +                mss_grfc7, mss_grfc8, mss_grfc9, nav_gpio, pa_indicator,
> > > +                pcie0_clkreqn, pcie1_clkreqn, phase_flag, pll_bist, pll_clk,
> > > +                pri_mi2s, prng_rosc, qdss_cti, qdss_gpio, qlink0_enable,
> > > +                qlink0_request, qlink0_wmss, qlink1_enable, qlink1_request,
> > > +                qlink1_wmss, qlink2_enable, qlink2_request, qlink2_wmss, qspi0,
> > > +                qspi1, qspi2, qspi3, qspi_clk, qspi_cs, qup0, qup1, qup10,
> > > +                qup11, qup12, qup13, qup14, qup15, qup16, qup17, qup18, qup19,
> > > +                qup2, qup3, qup4, qup5, qup6, qup7, qup8, qup9, qup_l4, qup_l5,
> > > +                qup_l6, sd_write, sdc40, sdc41, sdc42, sdc43, sdc4_clk,
> > > +                sdc4_cmd, sec_mi2s, tb_trig, tgu_ch0, tgu_ch1, tgu_ch2,
> > > +                tgu_ch3, tsense_pwm1, tsense_pwm2, uim0_clk, uim0_data,
> > > +                uim0_present, uim0_reset, uim1_clk, uim1_data, uim1_present,
> > > +                uim1_reset, usb2phy_ac, usb_phy, vfr_0, vfr_1, vsense_trigger ]
> > > +
> > > +
> > > +      drive-strength:
> > > +        enum: [2, 4, 6, 8, 10, 12, 14, 16]
> > > +        default: 2
> > > +        description:
> > > +          Selects the drive strength for the specified pins, in mA.
> > > +
> > > +      bias-pull-down: true
> > > +
> > > +      bias-pull-up: true
> > > +
> > > +      bias-disable: true
> > > +
> > > +      output-high: true
> > > +
> > > +      output-low: true
> > > +
> > > +    required:
> > > +      - pins
> > > +      - function
> > > +
> > > +    additionalProperties: false
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +  - interrupts
> > > +  - interrupt-controller
> > > +  - '#interrupt-cells'
> > > +  - gpio-controller
> > > +  - '#gpio-cells'
> > > +  - gpio-ranges
> > > +
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > +  - |
> > > +        #include <dt-bindings/interrupt-controller/arm-gic.h>
> > > +        tlmm: pinctrl@f000000 {
> > > +          compatible = "qcom,sm8350-tlmm";
> > > +          reg = <0x0f100000 0x300000>;
> > > +          interrupts = <GIC_SPI 208 IRQ_TYPE_LEVEL_HIGH>;
> > > +          gpio-controller;
> > > +          #gpio-cells = <2>;
> > > +          interrupt-controller;
> > > +          #interrupt-cells = <2>;
> > > +          gpio-ranges = <&tlmm 0 0 203>;
> > > +          serial-pinmux {
> > > +            pins = "gpio18", "gpio19";
> > > +            function = "qup3";
> > > +            drive-strength = <8>;
> > > +            bias-disable;
> > > +          };
> > > +        };
> > > +
> > > +...
> > > -- 
> > > 2.26.2
> > > 
> 
> -- 
> ~Vinod
Rob Herring Jan. 13, 2021, 2:56 a.m. UTC | #4 
On Thu, Jan 07, 2021 at 11:17:22AM -0600, Bjorn Andersson wrote:
> On Tue 05 Jan 23:49 CST 2021, Vinod Koul wrote:
> 
> > Add device tree binding Documentation details for Qualcomm SM8350
> > pinctrl driver.
> > 
> > Signed-off-by: Vinod Koul <vkoul@kernel.org>
> > ---
> >  .../bindings/pinctrl/qcom,sm8350-tlmm.yaml    | 149 ++++++++++++++++++
> >  1 file changed, 149 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/pinctrl/qcom,sm8350-tlmm.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,sm8350-tlmm.yaml b/Documentation/devicetree/bindings/pinctrl/qcom,sm8350-tlmm.yaml
> > new file mode 100644
> > index 000000000000..abdafd25bfc2
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pinctrl/qcom,sm8350-tlmm.yaml
> > @@ -0,0 +1,149 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/pinctrl/qcom,sm8350-tlmm.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Qualcomm Technologies, Inc. SM8350 TLMM block
> > +
> > +maintainers:
> > +  - Vinod Koul <vkoul@kernel.org>
> > +
> > +description: |
> > +  This binding describes the Top Level Mode Multiplexer block found in the
> > +  SM8350 platform.
> > +
> > +properties:
> > +  compatible:
> > +    const: qcom,sm8350-tlmm
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  interrupt-controller: true
> > +
> > +  '#interrupt-cells':
> > +    description: Specifies the PIN numbers and Flags, as defined in
> > +      include/dt-bindings/interrupt-controller/irq.h
> > +    const: 2
> > +
> > +  gpio-controller: true
> > +
> > +  '#gpio-cells':
> > +    description: Specifying the pin number and flags, as defined in
> > +      include/dt-bindings/gpio/gpio.h
> > +    const: 2
> > +
> > +  gpio-ranges:
> > +    maxItems: 1
> > +
> > +  gpio-reserved-ranges:
> > +    maxItems: 1
> > +
> > +#PIN CONFIGURATION NODES
> > +patternProperties:
> > +  '-pinmux$':
> 
> I believe that what Rob was asking for was the matter of describing the
> mux and config subnodes under this one. But I don't know really how to
> express this, because the following are all valid:
> 
> default_state: default-state {
> 	pins = "gpio1";
> 	bias-disable;
> };
> 
> default_state: default-state {
> 	rx {
> 		pins = "gpio1";
> 		function = "gpio";
> 		bias-disable;
> 	};
> };
> 
> default_state: default-state {
> 	pinmux {
> 		pins = "gpio1";
> 		function = "gpio";
> 	};
> 
> 	pinconf {
> 		pins = "gpio1";
> 		bias-disable;
> 	};
> };
> 
> I.e. the properties described here applies either to this node directly,
> or any subnodes (1 level) down.

Why!?

You can create a definition and reuse it. Something like this:

$defs:
  pin-node:
    type: object
    properties:
      ...

patternProperties:
  '-state$':
    oneOf:
      - $ref: #/$defs/pin-node

      - patternProperties:
          '.*':
            $ref: #/$defs/pin-node


Rob
Bjorn Andersson Jan. 19, 2021, 5:10 p.m. UTC | #5 
On Tue 12 Jan 20:56 CST 2021, Rob Herring wrote:

> On Thu, Jan 07, 2021 at 11:17:22AM -0600, Bjorn Andersson wrote:
> > On Tue 05 Jan 23:49 CST 2021, Vinod Koul wrote:
> > 
> > > Add device tree binding Documentation details for Qualcomm SM8350
> > > pinctrl driver.
> > > 
> > > Signed-off-by: Vinod Koul <vkoul@kernel.org>
> > > ---
> > >  .../bindings/pinctrl/qcom,sm8350-tlmm.yaml    | 149 ++++++++++++++++++
> > >  1 file changed, 149 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/pinctrl/qcom,sm8350-tlmm.yaml
> > > 
> > > diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,sm8350-tlmm.yaml b/Documentation/devicetree/bindings/pinctrl/qcom,sm8350-tlmm.yaml
> > > new file mode 100644
> > > index 000000000000..abdafd25bfc2
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/pinctrl/qcom,sm8350-tlmm.yaml
> > > @@ -0,0 +1,149 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/pinctrl/qcom,sm8350-tlmm.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Qualcomm Technologies, Inc. SM8350 TLMM block
> > > +
> > > +maintainers:
> > > +  - Vinod Koul <vkoul@kernel.org>
> > > +
> > > +description: |
> > > +  This binding describes the Top Level Mode Multiplexer block found in the
> > > +  SM8350 platform.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    const: qcom,sm8350-tlmm
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  interrupts:
> > > +    maxItems: 1
> > > +
> > > +  interrupt-controller: true
> > > +
> > > +  '#interrupt-cells':
> > > +    description: Specifies the PIN numbers and Flags, as defined in
> > > +      include/dt-bindings/interrupt-controller/irq.h
> > > +    const: 2
> > > +
> > > +  gpio-controller: true
> > > +
> > > +  '#gpio-cells':
> > > +    description: Specifying the pin number and flags, as defined in
> > > +      include/dt-bindings/gpio/gpio.h
> > > +    const: 2
> > > +
> > > +  gpio-ranges:
> > > +    maxItems: 1
> > > +
> > > +  gpio-reserved-ranges:
> > > +    maxItems: 1
> > > +
> > > +#PIN CONFIGURATION NODES
> > > +patternProperties:
> > > +  '-pinmux$':
> > 
> > I believe that what Rob was asking for was the matter of describing the
> > mux and config subnodes under this one. But I don't know really how to
> > express this, because the following are all valid:
> > 
> > default_state: default-state {
> > 	pins = "gpio1";
> > 	bias-disable;
> > };
> > 
> > default_state: default-state {
> > 	rx {
> > 		pins = "gpio1";
> > 		function = "gpio";
> > 		bias-disable;
> > 	};
> > };
> > 
> > default_state: default-state {
> > 	pinmux {
> > 		pins = "gpio1";
> > 		function = "gpio";
> > 	};
> > 
> > 	pinconf {
> > 		pins = "gpio1";
> > 		bias-disable;
> > 	};
> > };
> > 
> > I.e. the properties described here applies either to this node directly,
> > or any subnodes (1 level) down.
> 
> Why!?
> 

That's is how the generic pinctrl framework in Linux has parsed these
nodes for the last several years, so I expect all pinctrl bindings to
show this.

And in particular, rather than forcing people to use the third form
above the free naming of the subnodes allows to describe an entire 4 pin
UART in a single pin state (the last requires 3-4 nodes + duplicates for
sleep state), and for single gpio things the first one doesn't force
people to make up phony names - which typically lands them in the last
case (using pinmux/pinconf).

> You can create a definition and reuse it. Something like this:
> 
> $defs:
>   pin-node:
>     type: object
>     properties:
>       ...
> 
> patternProperties:
>   '-state$':
>     oneOf:
>       - $ref: #/$defs/pin-node
> 
>       - patternProperties:
>           '.*':
>             $ref: #/$defs/pin-node

So I presume then that it would make sense to convert
{pinmux,pinconf}-node.yaml this form and then in our binding we should
somehow just refer to these.

But what's the appropriate method of extending and limiting pin-node? We
have additional constraints and not all the properties are valid for the
Qualcomm TLMM.

Regards,
Bjorn

> 
> 
> Rob
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,sm8350-tlmm.yaml b/Documentation/devicetree/bindings/pinctrl/qcom,sm8350-tlmm.yaml
new file mode 100644
index 000000000000..abdafd25bfc2
--- /dev/null
+++ b/Documentation/devicetree/bindings/pinctrl/qcom,sm8350-tlmm.yaml
@@ -0,0 +1,149 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pinctrl/qcom,sm8350-tlmm.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm Technologies, Inc. SM8350 TLMM block
+
+maintainers:
+  - Vinod Koul <vkoul@kernel.org>
+
+description: |
+  This binding describes the Top Level Mode Multiplexer block found in the
+  SM8350 platform.
+
+properties:
+  compatible:
+    const: qcom,sm8350-tlmm
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  interrupt-controller: true
+
+  '#interrupt-cells':
+    description: Specifies the PIN numbers and Flags, as defined in
+      include/dt-bindings/interrupt-controller/irq.h
+    const: 2
+
+  gpio-controller: true
+
+  '#gpio-cells':
+    description: Specifying the pin number and flags, as defined in
+      include/dt-bindings/gpio/gpio.h
+    const: 2
+
+  gpio-ranges:
+    maxItems: 1
+
+  gpio-reserved-ranges:
+    maxItems: 1
+
+#PIN CONFIGURATION NODES
+patternProperties:
+  '-pinmux$':
+    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"
+
+    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]|1[0-9][0-9]|20[0-3])$"
+            - enum: [ sdc1_clk, sdc1_cmd, sdc1_data, sdc2_clk, sdc2_cmd, sdc2_data ]
+        minItems: 1
+        maxItems: 36
+
+      function:
+        description:
+          Specify the alternative function to be configured for the specified
+          pins. Functions are only valid for gpio pins.
+        enum: [ atest_char, atest_usb, audio_ref, cam_mclk, cci_async,
+                cci_i2c, cci_timer, cmu_rng, coex_uart1, coex_uart2, cri_trng,
+                cri_trng0, cri_trng1, dbg_out, ddr_bist, ddr_pxi0, ddr_pxi1,
+                ddr_pxi2, ddr_pxi3, dp_hot, dp_lcd, gcc_gp1, gcc_gp2, gcc_gp3,
+                gpio, ibi_i3c, jitter_bist, lpass_slimbus, mdp_vsync, mdp_vsync0,
+                mdp_vsync1, mdp_vsync2, mdp_vsync3, mi2s0_data0, mi2s0_data1,
+                mi2s0_sck, mi2s0_ws, mi2s1_data0, mi2s1_data1, mi2s1_sck,
+                mi2s1_ws, mi2s2_data0, mi2s2_data1, mi2s2_sck, mi2s2_ws,
+                mss_grfc0, mss_grfc1, mss_grfc10, mss_grfc11, mss_grfc12,
+                mss_grfc2, mss_grfc3, mss_grfc4, mss_grfc5, mss_grfc6,
+                mss_grfc7, mss_grfc8, mss_grfc9, nav_gpio, pa_indicator,
+                pcie0_clkreqn, pcie1_clkreqn, phase_flag, pll_bist, pll_clk,
+                pri_mi2s, prng_rosc, qdss_cti, qdss_gpio, qlink0_enable,
+                qlink0_request, qlink0_wmss, qlink1_enable, qlink1_request,
+                qlink1_wmss, qlink2_enable, qlink2_request, qlink2_wmss, qspi0,
+                qspi1, qspi2, qspi3, qspi_clk, qspi_cs, qup0, qup1, qup10,
+                qup11, qup12, qup13, qup14, qup15, qup16, qup17, qup18, qup19,
+                qup2, qup3, qup4, qup5, qup6, qup7, qup8, qup9, qup_l4, qup_l5,
+                qup_l6, sd_write, sdc40, sdc41, sdc42, sdc43, sdc4_clk,
+                sdc4_cmd, sec_mi2s, tb_trig, tgu_ch0, tgu_ch1, tgu_ch2,
+                tgu_ch3, tsense_pwm1, tsense_pwm2, uim0_clk, uim0_data,
+                uim0_present, uim0_reset, uim1_clk, uim1_data, uim1_present,
+                uim1_reset, usb2phy_ac, usb_phy, vfr_0, vfr_1, vsense_trigger ]
+
+
+      drive-strength:
+        enum: [2, 4, 6, 8, 10, 12, 14, 16]
+        default: 2
+        description:
+          Selects the drive strength for the specified pins, in mA.
+
+      bias-pull-down: true
+
+      bias-pull-up: true
+
+      bias-disable: true
+
+      output-high: true
+
+      output-low: true
+
+    required:
+      - pins
+      - function
+
+    additionalProperties: false
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - interrupt-controller
+  - '#interrupt-cells'
+  - gpio-controller
+  - '#gpio-cells'
+  - gpio-ranges
+
+additionalProperties: false
+
+examples:
+  - |
+        #include <dt-bindings/interrupt-controller/arm-gic.h>
+        tlmm: pinctrl@f000000 {
+          compatible = "qcom,sm8350-tlmm";
+          reg = <0x0f100000 0x300000>;
+          interrupts = <GIC_SPI 208 IRQ_TYPE_LEVEL_HIGH>;
+          gpio-controller;
+          #gpio-cells = <2>;
+          interrupt-controller;
+          #interrupt-cells = <2>;
+          gpio-ranges = <&tlmm 0 0 203>;
+          serial-pinmux {
+            pins = "gpio18", "gpio19";
+            function = "qup3";
+            drive-strength = <8>;
+            bias-disable;
+          };
+        };
+
+...