From: Lorenzo Bianconi <lorenzo@kernel.org>
To: Krzysztof Kozlowski <krzk@kernel.org>
Cc: linux-gpio@vger.kernel.org, linus.walleij@linaro.org,
sean.wang@kernel.org, linux-mediatek@lists.infradead.org,
lorenzo.bianconi83@gmail.com, krzk+dt@kernel.org,
robh@kernel.org, devicetree@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, upstream@airoha.com,
angelogioacchino.delregno@collabora.com,
benjamin.larsson@genexis.eu, conor+dt@kernel.org,
ansuelsmth@gmail.com
Subject: Re: [PATCH 1/2] dt-bindings: pinctrl: airoha: Add EN7581 pinctrl controller
Date: Sun, 18 Aug 2024 16:20:08 +0200 [thread overview]
Message-ID: <ZsIDGP7WxvvzVUp4@lore-rh-laptop> (raw)
In-Reply-To: <22144671-fc7c-4cb2-8bb6-ee7d3fbfcb0e@kernel.org>
[-- Attachment #1: Type: text/plain, Size: 14475 bytes --]
[...]
> > +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@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
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
next prev parent reply other threads:[~2024-08-18 14:20 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-08-11 16:12 [PATCH 0/2] Add pinctrl support to EN7581 SoC Lorenzo Bianconi
2024-08-11 16:12 ` [PATCH 1/2] dt-bindings: pinctrl: airoha: Add EN7581 pinctrl controller Lorenzo Bianconi
2024-08-12 6:48 ` Krzysztof Kozlowski
2024-08-13 8:06 ` Benjamin Larsson
2024-08-16 22:52 ` Rob Herring
2024-08-17 20:46 ` Benjamin Larsson
2024-08-17 21:39 ` Andrew Lunn
2024-08-18 12:48 ` Benjamin Larsson
2024-08-18 16:02 ` Andrew Lunn
2024-08-18 15:42 ` Christian Marangi
2024-08-18 16:15 ` Lorenzo Bianconi
2024-08-18 16:59 ` Benjamin Larsson
2024-08-18 17:35 ` Andrew Lunn
2024-08-18 14:22 ` Lorenzo Bianconi
2024-08-18 14:20 ` Lorenzo Bianconi [this message]
2024-08-11 16:12 ` [PATCH 2/2] pinctrl: airoha: Add support for EN7581 SoC Lorenzo Bianconi
2024-08-11 21:08 ` kernel test robot
2024-08-11 21:49 ` kernel test robot
2024-08-11 21:59 ` kernel test robot
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZsIDGP7WxvvzVUp4@lore-rh-laptop \
--to=lorenzo@kernel.org \
--cc=angelogioacchino.delregno@collabora.com \
--cc=ansuelsmth@gmail.com \
--cc=benjamin.larsson@genexis.eu \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=krzk+dt@kernel.org \
--cc=krzk@kernel.org \
--cc=linus.walleij@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-mediatek@lists.infradead.org \
--cc=lorenzo.bianconi83@gmail.com \
--cc=robh@kernel.org \
--cc=sean.wang@kernel.org \
--cc=upstream@airoha.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox