[PATCH 1/2] dt-bindings: pinctrl: airoha: Add EN7581 pinctrl controller
Lorenzo Bianconi
lorenzo at kernel.org
Sun Aug 18 07:20:08 PDT 2024
[...]
> > +allOf:
> > + - $ref: pinctrl.yaml#
> > +
> > +required:
> > + - compatible
> > + - reg
> > + - gpio-controller
> > + - "#gpio-cells"
> > +
> > +patternProperties:
>
> Keep it after properties: block.
ack, I will fix it in v2
>
> > + '-pins$':
> > + type: object
> > + additionalProperties: false
> > +
> > + patternProperties:
> > + '^.*mux.*$':
> > + type: object
> > + additionalProperties: false
> > + description: |
>
> Do not need '|' unless you need to preserve formatting.
ack, I will fix it in v2
>
> > + pinmux configuration nodes.
> > +
> > + $ref: /schemas/pinctrl/pinmux-node.yaml
> > + properties:
> > + function:
> > + description:
> > + A string containing the name of the function to mux to the group.
> > + enum: [pon, tod_1pps, sipo, mdio, uart, i2c, jtag, pcm, spi,
> > + pcm_spi, i2s, emmc, pnand, pcie_reset, pwm, phy1_led0,
> > + phy2_led0, phy3_led0, phy4_led0, phy1_led1, phy2_led1,
> > + phy3_led1, phy4_led1]
> > + groups:
>
> minItems: 1
> maxItems: 32 (or whatever is sensible)
I was assuming the default values are minIems = maxItems = 1 and we can
overwrite maxItems if required (e.g. for "uart" or for "pcm_spi" blocks).
Am I missing something?
>
> > + description:
> > + An array of strings. Each string contains the name of a group.
> > + required:
> > + - function
> > + - groups
> > +
> > + allOf:
> > + - if:
> > + properties:
> > + function:
> > + const: pon
> > + then:
> > + properties:
> > + groups:
> > + enum: [pon]
> > + - if:
> > + properties:
> > + function:
> > + const: tod_1pps
> > + then:
> > + properties:
> > + groups:
> > + enum: [pon_tod_1pps, gsw_tod_1pps]
> > + - if:
> > + properties:
> > + function:
> > + const: sipo
> > + then:
> > + properties:
> > + groups:
> > + enum: [sipo, sipo_rclk]
> > + - if:
> > + properties:
> > + function:
> > + const: mdio
> > + then:
> > + properties:
> > + groups:
> > + enum: [mdio]
> > + - if:
> > + properties:
> > + function:
> > + const: uart
> > + then:
> > + properties:
> > + groups:
> > + items:
> > + enum: [uart2, uart2_cts_rts, hsuart, hsuart_cts_rts, uart4,
> > + uart5]
> > + maxItems: 2
> > + - if:
> > + properties:
> > + function:
> > + const: i2c
> > + then:
> > + properties:
> > + groups:
> > + enum: [i2c1]
> > + - if:
> > + properties:
> > + function:
> > + const: jtag
> > + then:
> > + properties:
> > + groups:
> > + enum: [jtag_udi, jtag_dfd]
> > + - if:
> > + properties:
> > + function:
> > + const: pcm
> > + then:
> > + properties:
> > + groups:
> > + enum: [pcm1, pcm2]
> > + - if:
> > + properties:
> > + function:
> > + const: spi
> > + then:
> > + properties:
> > + groups:
> > + items:
> > + enum: [spi_quad, spi_cs1]
> > + maxItems: 2
> > + - if:
> > + properties:
> > + function:
> > + const: pcm_spi
> > + then:
> > + properties:
> > + groups:
> > + items:
> > + enum: [pcm_spi, pcm_spi_int, pcm_spi_rst, pcm_spi_cs1,
> > + pcm_spi_cs2_p156, pcm_spi_cs2_p128, pcm_spi_cs3,
> > + pcm_spi_cs4]
> > + maxItems: 7
> > + - if:
> > + properties:
> > + function:
> > + const: i2c
> > + then:
> > + properties:
> > + groups:
> > + enum: [i2s]
> > + - if:
> > + properties:
> > + function:
> > + const: emmc
> > + then:
> > + properties:
> > + groups:
> > + enum: [emmc]
> > + - if:
> > + properties:
> > + function:
> > + const: pnand
> > + then:
> > + properties:
> > + groups:
> > + enum: [pnand]
> > + - if:
> > + properties:
> > + function:
> > + const: pcie_reset
> > + then:
> > + properties:
> > + groups:
> > + enum: [pcie_reset0, pcie_reset1, pcie_reset2]
> > + - if:
> > + properties:
> > + function:
> > + const: pwm
> > + then:
> > + properties:
> > + groups:
> > + enum: [gpio0, gpio1, gpio2, gpio3, gpio4, gpio5, gpio6,
> > + gpio7, gpio8, gpio9, gpio10, gpio11, gpio12, gpio13,
> > + gpio14, gpio15, gpio16, gpio17, gpio18, gpio19,
> > + gpio20, gpio21, gpio22, gpio23, gpio24, gpio25,
> > + gpio26, gpio27, gpio28, gpio29, gpio30, gpio31,
> > + gpio36, gpio37, gpio38, gpio39, gpio40, gpio41,
> > + gpio42, gpio43, gpio44, gpio45, gpio46, gpio47]
> > + - if:
> > + properties:
> > + function:
> > + const: phy1_led0
> > + then:
> > + properties:
> > + groups:
> > + enum: [gpio33, gpio34, gpio35, gpio42]
> > + - if:
> > + properties:
> > + function:
> > + const: phy2_led0
> > + then:
> > + properties:
> > + groups:
> > + enum: [gpio33, gpio34, gpio35, gpio42]
> > + - if:
> > + properties:
> > + function:
> > + const: phy3_led0
> > + then:
> > + properties:
> > + groups:
> > + enum: [gpio33, gpio34, gpio35, gpio42]
> > + - if:
> > + properties:
> > + function:
> > + const: phy4_led0
> > + then:
> > + properties:
> > + groups:
> > + enum: [gpio33, gpio34, gpio35, gpio42]
> > + - if:
> > + properties:
> > + function:
> > + const: phy1_led1
> > + then:
> > + properties:
> > + groups:
> > + enum: [gpio43, gpio44, gpio45, gpio46]
> > + - if:
> > + properties:
> > + function:
> > + const: phy2_led1
> > + then:
> > + properties:
> > + groups:
> > + enum: [gpio43, gpio44, gpio45, gpio46]
> > + - if:
> > + properties:
> > + function:
> > + const: phy3_led1
> > + then:
> > + properties:
> > + groups:
> > + enum: [gpio43, gpio44, gpio45, gpio46]
> > + - if:
> > + properties:
> > + function:
> > + const: phy4_led1
> > + then:
> > + properties:
> > + groups:
> > + enum: [gpio43, gpio44, gpio45, gpio46]
> > +
> > + '^.*conf.*$':
> > + type: object
> > + additionalProperties: false
> > + description:
> > + pinconf configuration nodes.
> > + $ref: /schemas/pinctrl/pincfg-node.yaml
> > +
> > + properties:
> > + pins:
> > + description:
> > + An array of strings. Each string contains the name of a pin.
> > + items:
> > + enum: [uart1_txd, uart1_rxd, i2c_scl, i2c_sda, spi_cs0, spi_clk,
> > + spi_mosi, spi_miso, gpio0, gpio1, gpio2, gpio3, gpio4,
> > + gpio5, gpio6, gpio7, gpio8, gpio9, gpio10, gpio11, gpio12,
> > + gpio13, gpio14, gpio15, gpio16, gpio17, gpio18, gpio19,
> > + gpio20, gpio21, gpio22, gpio23, gpio24, gpio25, gpio26,
> > + gpio27, gpio28, gpio29, gpio30, gpio31, gpio32, gpio33,
> > + gpio34, gpio35, gpio36, gpio37, gpio38, gpio39, gpio40,
> > + gpio41, gpio42, gpio43, gpio44, gpio45, gpio46,
> > + pcie_reset0, pcie_reset1, pcie_reset2]
>
> minItems
ack, I will fix it in v2.
>
> > + maxItems: 58
> > +
> > + bias-disable: true
> > +
> > + bias-pull-up: true
> > +
> > + bias-pull-down: true
> > +
> > + input-enable: true
> > +
> > + output-enable: true
> > +
> > + output-low: true
> > +
> > + output-high: true
> > +
> > + drive-open-drain: true
>
> Drop blank lines between these.
ack, I will fix it in v2.
> > +
> > + drive-strength:
>
> What are the units? Shouldn't this be drive-strength-microamp?
nope, it is in mA. I will add a description to specify it.
>
> > + enum: [2, 4, 6, 8]
> > +
> > + required:
> > + - pins
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > + #include <dt-bindings/interrupt-controller/irq.h>
> > + #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +
> > + soc {
> > + #address-cells = <2>;
> > + #size-cells = <2>;
> > +
> > + pio: pinctrl at 1fa20214 {
> > + compatible = "airoha,en7581-pinctrl";
> > + reg = <0x0 0x1fa20214 0x0 0x30>,
> > + <0x0 0x1fa2027c 0x0 0x8>,
> > + <0x0 0x1fbf0234 0x0 0x4>,
> > + <0x0 0x1fbf0268 0x0 0x4>,
> > + <0x0 0x1fa2001c 0x0 0x50>,
> > + <0x0 0x1fa2018c 0x0 0x4>,
> > + <0x0 0x1fbf0204 0x0 0x4>,
> > + <0x0 0x1fbf0270 0x0 0x4>,
> > + <0x0 0x1fbf0200 0x0 0x4>,
> > + <0x0 0x1fbf0220 0x0 0x4>,
> > + <0x0 0x1fbf0260 0x0 0x4>,
> > + <0x0 0x1fbf0264 0x0 0x4>,
> > + <0x0 0x1fbf0214 0x0 0x4>,
> > + <0x0 0x1fbf0278 0x0 0x4>,
> > + <0x0 0x1fbf0208 0x0 0x4>,
> > + <0x0 0x1fbf027c 0x0 0x4>,
> > + <0x0 0x1fbf0210 0x0 0x4>,
> > + <0x0 0x1fbf028c 0x0 0x4>,
> > + <0x0 0x1fbf0290 0x0 0x4>,
> > + <0x0 0x1fbf0294 0x0 0x4>,
> > + <0x0 0x1fbf020c 0x0 0x4>,
> > + <0x0 0x1fbf0280 0x0 0x4>,
> > + <0x0 0x1fbf0284 0x0 0x4>,
> > + <0x0 0x1fbf0288 0x0 0x4>;
>
> Why are you mapping individual registers? At least half of these are
> continuous.
>
> > +
> > + gpio-controller;
> > + #gpio-cells = <2>;
> > + gpio-ranges = <&pio 0 13 47>;
> > +
> > + interrupt-controller;
> > + #interrupt-cells = <2>;
> > + interrupt-parent = <&gic>;
> > + interrupts = <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>;
> > +
> > + pcie1-rst-pins {
> > + conf {
> > + pins = "pcie_reset1";
> > + drive-open-drain = <1>;
> > + };
> > + };
> > +
> > + pwm-pins {
> > + mux {
> > + function = "pwm";
> > + groups = "gpio18";
> > + };
> > + };
> > +
> > + spi-pins {
> > + mux {
> > + function = "spi";
> > + groups = "spi_quad", "spi_cs1";
> > + };
> > + };
> > +
> > + uuart2-pins {
> > + mux {
> > + function = "uart";
> > + groups = "uart2", "uart2_cts_rts";
> > + };
> > + };
> > +
> > + uar5-pins {
> > + mux {
> > + function = "uart";
> > + groups = "uart5";
> > + };
> > + };
> > +
> > + mmc-pins {
> > + mux {
> > + function = "emmc";
> > + groups = "emmc";
> > + };
> > + };
> > +
> > + mdio-pins {
> > + mux {
> > + function = "mdio";
> > + groups = "mdio";
> > + };
> > +
> > + conf {
> > + pins = "gpio2";
>
> What is the point of having both groups and pins?
"pins" is specific for "conf" block and it is used to specify the pins where
we need to apply the configuration while "group" is just used in the "mux"
block. Am I missing something?
>
> > + output-enable;
> > + };
> > + };
> > +
> > + gswp1-led0-pins {
> > + mux {
> > + function = "phy1_led0";
> > + groups = "gpio33";
> > + };
> > + };
> > +
> > + gswp2-led1-pins {
> > + mux {
> > + function = "phy2_led1";
> > + groups = "gpio44";
>
> That's not a group but pin name.
Looking at the patch 2/2, "gpio44" is actually a group used to specify a single
pin group.
Regards,
Lorenzo
>
> Best regards,
> Krzysztof
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20240818/3fde41b7/attachment.sig>
More information about the linux-arm-kernel
mailing list