[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAJM55Z9+dpbqt-c=55WXUXsw=Dhk6m6Q1_Js3s-T+8W7dtrURQ@mail.gmail.com>
Date: Wed, 26 Mar 2025 02:57:57 -0700
From: Emil Renner Berthing <emil.renner.berthing@...onical.com>
To: Krzysztof Kozlowski <krzk@...nel.org>,
Emil Renner Berthing <emil.renner.berthing@...onical.com>
Cc: Pinkesh Vaghela <pinkesh.vaghela@...fochips.com>,
Pritesh Patel <pritesh.patel@...fochips.com>, Min Lin <linmin@...incomputing.com>,
Samuel Holland <samuel.holland@...ive.com>, Linus Walleij <linus.walleij@...aro.org>,
Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
Paul Walmsley <paul.walmsley@...ive.com>, Palmer Dabbelt <palmer@...belt.com>,
Alexandre Ghiti <alex@...ti.fr>, Bartosz Golaszewski <brgl@...ev.pl>, linux-gpio@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-riscv@...ts.infradead.org
Subject: Re: [RFC PATCH 1/4] dt-bindings: pinctrl: Add eswin,eic7700-pinctrl binding
Krzysztof Kozlowski wrote:
> On Tue, Mar 25, 2025 at 03:13:03PM +0100, Emil Renner Berthing wrote:
> > +properties:
> > + compatible:
> > + enum:
> > + - eswin,eic7700-pinctrl
>
> Blank line
>
> > + reg:
> > + maxItems: 1
> > +
> > +required:
>
> required: goes after patternProperties.
>
> > + - compatible
> > + - reg
> > +
> > +patternProperties:
> > + '-[0-9]+$':
>
> Recommended is to have more meaningful prefix or suffix, e.g.
> -grp/-group. I also don't get why it has to end with number.
>
> > + type: object
> > + additionalProperties: false
> > +
> > + patternProperties:
> > + '-pins$':
> > + type: object
> > + allOf:
> > + - $ref: /schemas/pinctrl/pincfg-node.yaml#
> > + - $ref: /schemas/pinctrl/pinmux-node.yaml#
> > +
> > + additionalProperties: false
> > +
> > + description:
> > + A pinctrl node should contain at least one subnode describing one
> > + or more pads and their associated pinmux and pinconf settings.
> > +
> > + properties:
> > + pins:
> > + items:
> > + enum: [ CHIP_MODE, MODE_SET0, MODE_SET1, MODE_SET2, MODE_SET3,
> > + XIN, RTC_XIN, RST_OUT_N, KEY_RESET_N, GPIO0, POR_SEL,
> > + JTAG0_TCK, JTAG0_TMS, JTAG0_TDI, JTAG0_TDO, GPIO5, SPI2_CS0_N,
> > + JTAG1_TCK, JTAG1_TMS, JTAG1_TDI, JTAG1_TDO, GPIO11, SPI2_CS1_N,
> > + PCIE_CLKREQ_N, PCIE_WAKE_N, PCIE_PERST_N, HDMI_SCL, HDMI_SDA,
> > + HDMI_CEC, JTAG2_TRST, RGMII0_CLK_125, RGMII0_TXEN,
> > + RGMII0_TXCLK, RGMII0_TXD0, RGMII0_TXD1, RGMII0_TXD2,
> > + RGMII0_TXD3, I2S0_BCLK, I2S0_WCLK, I2S0_SDI, I2S0_SDO,
> > + I2S_MCLK, RGMII0_RXCLK, RGMII0_RXDV, RGMII0_RXD0, RGMII0_RXD1,
> > + RGMII0_RXD2, RGMII0_RXD3, I2S2_BCLK, I2S2_WCLK, I2S2_SDI,
> > + I2S2_SDO, GPIO27, GPIO28, GPIO29, RGMII0_MDC, RGMII0_MDIO,
> > + RGMII0_INTB, RGMII1_CLK_125, RGMII1_TXEN, RGMII1_TXCLK,
> > + RGMII1_TXD0, RGMII1_TXD1, RGMII1_TXD2, RGMII1_TXD3, I2S1_BCLK,
> > + I2S1_WCLK, I2S1_SDI, I2S1_SDO, GPIO34, RGMII1_RXCLK,
> > + RGMII2_RXDV, RGMII2_RXD0, RGMII2_RXD1, RGMII2_RXD2,
> > + RGMII2_RXD3, SPI1_CS0_N, SPI1_CLK, SPI1_D0, SPI1_D1, SPI1_D2,
> > + SPI1_D3, SPI1_CS1_N, RGMII1_MDC, RGMII1_MDIO, RGMII1_INTB,
> > + USB0_PWREN, USB1_PWREN, I2C0_SCL, I2C0_SDA, I2C1_SCL, I2C1_SDA,
> > + I2C2_SCL, I2C2_SDA, I2C3_SCL, I2C3_SDA, I2C4_SCL, I2C4_SDA,
> > + I2C5_SCL, I2C5_SDA, UART0_TX, UART0_RX, UART1_TX, UART1_RX,
> > + UART1_CTS, UART1_RTS, UART2_TX, UART2_RX, JTAG2_TCK, JTAG2_TMS,
> > + JTAG2_TDI, JTAG2_TDO, FAN_PWM, FAN_TACH, MIPI_CSI0_XVS,
> > + MIPI_CSI0_XHS, MIPI_CSI0_MCLK, MIPI_CSI1_XVS, MIPI_CSI1_XHS,
> > + MIPI_CSI1_MCLK, MIPI_CSI2_XVS, MIPI_CSI2_XHS, MIPI_CSI2_MCLK,
> > + MIPI_CSI3_XVS, MIPI_CSI3_XHS, MIPI_CSI3_MCLK, MIPI_CSI4_XVS,
> > + MIPI_CSI4_XHS, MIPI_CSI4_MCLK, MIPI_CSI5_XVS, MIPI_CSI5_XHS,
> > + MIPI_CSI5_MCLK, SPI3_CS_N, SPI3_CLK, SPI3_DI, SPI3_DO, GPIO92,
> > + GPIO93, S_MODE, GPIO95, SPI0_CS_N, SPI0_CLK, SPI0_D0, SPI0_D1,
> > + SPI0_D2, SPI0_D3, I2C10_SCL, I2C10_SDA, I2C11_SCL, I2C11_SDA,
> > + GPIO106, BOOT_SEL0, BOOT_SEL1, BOOT_SEL2, BOOT_SEL3, GPIO111,
> > + LPDDR_REF_CLK ]
>
> All these should be lowercase.
Plenty of pinctrl drivers use uppercase names for the pins, intel, amd,
mediatek to name a few, and this is also what the EIC7700 documentation uses.
Do you still wan't Linux to call the pins something else?
>
> > + description: List of pads that properties in the node apply to.
> > +
> > + function:
> > + enum: [ csi, debug, ddr, fan, gpio, hdmi, i2c, i2s, jtag, mipi,
> > + mode, oscillator, pci, pwm, rgmii, reset, sata, spi, sdio,
> > + uart, usb ]
> > + description: The mux function to select for the given pins.
> > +
> > + bias-disable: true
> > +
> > + bias-pull-up:
> > + oneOf:
> > + - type: boolean
> > + - const: 25000
> > + description: Enable internal 25kOhm pull-up
>
> Why bool and fixed value? Do they have different meaning? Description
> says they are the same.
>
> Anyway, don't repeat constraints in free form text.
>
> > +
> > + bias-pull-down:
> > + oneOf:
> > + - type: boolean
> > + - const: 22000
> > + description: Enable internal 22kOhm pull-down
>
> Same questions
>
> > +
> > + drive-strength-microamp:
> > + enum: [ 3100, 6700, 9600, 12900, 18000, 20900, 23200, 25900 ]
> > +
> > + input-enable: true
> > +
> > + input-disable: true
> > +
> > + input-schmitt-enable: true
> > +
> > + input-schmitt-disable: true
> > +
> > + required:
> > + - pins
>
> Best regards,
> Krzysztof
>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
Powered by blists - more mailing lists