From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
To: Alex Soo <yuklin.soo@starfivetech.com>,
Linus Walleij <linus.walleij@linaro.org>,
Bartosz Golaszewski <bartosz.golaszewski@linaro.org>,
Hal Feng <hal.feng@starfivetech.com>,
Ley Foon Tan <leyfoon.tan@starfivetech.com>,
Jianlong Huang <jianlong.huang@starfivetech.com>,
Emil Renner Berthing <kernel@esmil.dk>,
Rob Herring <robh@kernel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
Conor Dooley <conor+dt@kernel.org>,
Drew Fustini <drew@beagleboard.org>
Cc: linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org,
devicetree@vger.kernel.org, linux-riscv@lists.infradead.org,
Paul Walmsley <paul.walmsley@sifive.com>,
Palmer Dabbelt <palmer@dabbelt.com>,
Albert Ou <aou@eecs.berkeley.edu>
Subject: Re: [RFC PATCH 1/6] dt-bindings: pinctrl: starfive: add JH8100 pinctrl bindings
Date: Thu, 21 Dec 2023 16:49:52 +0100 [thread overview]
Message-ID: <3b9201e0-e1d9-463c-aa1a-cbbedd1f8907@linaro.org> (raw)
In-Reply-To: <20231221083622.3445726-2-yuklin.soo@starfivetech.com>
On 21/12/2023 09:36, Alex Soo wrote:
> Add dt-binding documentation and header file for JH8100 pinctrl
> driver.
>
> Signed-off-by: Alex Soo <yuklin.soo@starfivetech.com>
> Reviewed-by: Ley Foon Tan <leyfoon.tan@starfivetech.com>
> ---
A nit, subject: drop second/last, redundant "bindings". The
"dt-bindings" prefix is already stating that these are bindings.
...
> + i
nterrupt-controller: true
> +
> + gpio-controller: true
> +
> + '#gpio-cells':
> + const: 2
> +
> +patternProperties:
> + '-[0-9]+$':
This is a bit too wide pattern. Consider some suffix like -grp or
-group. Look at other bindings how they do it.
> + type: object
> + additionalProperties: false
> + patternProperties:
> + '-pins$':
> + type: object
> + description: |
> + A pinctrl node should contain at least one subnode representing the
> + pinctrl groups available in the domain. Each subnode will list the
> + pins it needs, and how they should be configured, with regard to
> + muxer configuration, bias, input enable/disable, input schmitt
> + trigger enable/disable, slew-rate and drive strength.
> + allOf:
> + - $ref: /schemas/pinctrl/pincfg-node.yaml
> + - $ref: /schemas/pinctrl/pinmux-node.yaml
> + additionalProperties: false
Why the rest of the properties is not applicable?
> +
> + properties:
> + pinmux:
> + description: |
> + The list of GPIOs and their mux settings or function select.
> + The GPIOMUX and PINMUX macros are used to configure the
> + I/O multiplexing and function selection respectively.
> +
> + bias-disable: true
> +
> + bias-pull-up:
> + type: boolean
> +
> + bias-pull-down:
> + type: boolean
> +
> + drive-strength:
> + enum: [ 2, 4, 8, 12 ]
> +
> + input-enable: true
> +
> + input-disable: true
> +
> + input-schmitt-enable: true
> +
> + input-schmitt-disable: true
> +
> + slew-rate:
> + maximum: 1
> +
> +required:
> + - compatible
> + - reg
> + - resets
> + - interrupts
> + - interrupt-controller
> + - gpio-controller
> + - '#gpio-cells'
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/clock/starfive,jh8100.h>
> + #include <dt-bindings/reset/starfive-jh8100.h>
> + #include <dt-bindings/pinctrl/starfive,jh8100-pinctrl.h>
> +
> + soc {
> + #address-cells = <2>;
> + #size-cells = <2>;
> +
> + pinctrl_aon: pinctrl@1f300000 {
> + compatible = "starfive,jh8100-aon-pinctrl",
> + "syscon", "simple-mfd";
You did not even bother to test it before sending.
> + reg = <0x0 0x1f300000 0x0 0x10000>;
> + resets = <&aoncrg 0>;
Use 4 spaces for example indentation.
> + interrupts = <160>;
> + interrupt-controller;
> + gpio-controller;
> + #gpio-cells = <2>;
Incomplete example.
I'll stop review, except one more comment, this was not tested and does
not work. Reviewing code which was never tested is a waste of reviewer's
time.
...
> diff --git a/include/dt-bindings/pinctrl/starfive,jh8100-pinctrl.h b/include/dt-bindings/pinctrl/starfive,jh8100-pinctrl.h
> new file mode 100644
> index 000000000000..c57b7ece37a2
> --- /dev/null
> +++ b/include/dt-bindings/pinctrl/starfive,jh8100-pinctrl.h
> @@ -0,0 +1,303 @@
> +/* SPDX-License-Identifier: GPL-2.0 OR MIT */
> +/*
> + * Copyright (C) 2023 StarFive Technology Co., Ltd.
> + * Author: Alex Soo <yuklin.soo@starfivetech.com>
> + *
> + */
> +
> +#ifndef __DT_BINDINGS_PINCTRL_STARFIVE_JH8100_H__
> +#define __DT_BINDINGS_PINCTRL_STARFIVE_JH8100_H__
> +
> +/* sys_iomux_west pins */
> +#define PAD_GPIO0_W 0
> +#define PAD_GPIO1_W 1
> +#define PAD_GPIO2_W 2
> +#define PAD_GPIO3_W 3
> +#define PAD_GPIO4_W 4
> +#define PAD_GPIO5_W 5
> +#define PAD_GPIO6_W 6
> +#define PAD_GPIO7_W 7
> +#define PAD_GPIO8_W 8
> +#define PAD_GPIO9_W 9
> +#define PAD_GPIO10_W 10
> +#define PAD_GPIO11_W 11
> +#define PAD_GPIO12_W 12
> +#define PAD_GPIO13_W 13
> +#define PAD_GPIO14_W 14
> +#define PAD_GPIO15_W 15
> +
> +/* sys_iomux_west syscon */
> +#define SYS_W_VREF_GPIO_W_SYSCON_REG 0x6c
> +#define SYS_W_VREF_GPIO_W_SYSCON_MASK GENMASK(1, 0)
> +#define SYS_W_VREF_GPIO_W_SYSCON_SHIFT 0
> +#define SYS_W_VREF_ATB_ISP_VOUT_SYSCON_REG 0x70
> +#define SYS_W_VREF_ATB_ISP_VOUT_SYSCON_MASK GENMASK(1, 0)
> +#define SYS_W_VREF_ATB_ISP_VOUT_SYSCON_SHIFT 0
None of these are bindings, drop.
> +
> +/* sys_iomux_east pins */
> +#define PAD_GPIO0_E 0
> +#define PAD_GPIO1_E 1
> +#define PAD_GPIO2_E 2
> +#define PAD_GPIO3_E 3
> +#define PAD_GPIO4_E 4
> +#define PAD_GPIO5_E 5
> +#define PAD_GPIO6_E 6
> +#define PAD_GPIO7_E 7
> +#define PAD_GPIO8_E 8
> +#define PAD_GPIO9_E 9
> +#define PAD_GPIO10_E 10
> +#define PAD_GPIO11_E 11
> +#define PAD_GPIO12_E 12
> +#define PAD_GPIO13_E 13
> +#define PAD_GPIO14_E 14
> +#define PAD_GPIO15_E 15
> +#define PAD_GPIO16_E 16
> +#define PAD_GPIO17_E 17
> +#define PAD_GPIO18_E 18
> +#define PAD_GPIO19_E 19
> +#define PAD_GPIO20_E 20
> +#define PAD_GPIO21_E 21
> +#define PAD_GPIO22_E 22
> +#define PAD_GPIO23_E 23
> +#define PAD_GPIO24_E 24
> +#define PAD_GPIO25_E 25
> +#define PAD_GPIO26_E 26
> +#define PAD_GPIO27_E 27
> +#define PAD_GPIO28_E 28
> +#define PAD_GPIO29_E 29
> +#define PAD_GPIO30_E 30
> +#define PAD_GPIO31_E 31
> +#define PAD_GPIO32_E 32
> +#define PAD_GPIO33_E 33
> +#define PAD_GPIO34_E 34
> +#define PAD_GPIO35_E 35
> +#define PAD_GPIO36_E 36
> +#define PAD_GPIO37_E 37
> +#define PAD_GPIO38_E 38
> +#define PAD_GPIO39_E 39
> +#define PAD_GPIO40_E 40
> +#define PAD_GPIO41_E 41
> +#define PAD_GPIO42_E 42
> +#define PAD_GPIO43_E 43
> +#define PAD_GPIO44_E 44
> +#define PAD_GPIO45_E 45
> +#define PAD_GPIO46_E 46
> +#define PAD_GPIO47_E 47
Please explain why do you think these are bindings?
> +
> +/* sys_iomux_east syscon */
> +#define SYS_E_VREF_GPIO_E0_SYSCON_REG 0x0fc
> +#define SYS_E_VREF_GPIO_E0_SYSCON_MASK GENMASK(1, 0)
> +#define SYS_E_VREF_GPIO_E0_SYSCON_SHIFT 0
> +#define SYS_E_VREF_GPIO_E1_SYSCON_REG 0x100
> +#define SYS_E_VREF_GPIO_E1_SYSCON_MASK GENMASK(1, 0)
> +#define SYS_E_VREF_GPIO_E1_SYSCON_SHIFT 0
> +#define SYS_E_VREF_GPIO_E2_SYSCON_REG 0x104
> +#define SYS_E_VREF_GPIO_E2_SYSCON_MASK GENMASK(1, 0)
> +#define SYS_E_VREF_GPIO_E2_SYSCON_SHIFT 0
> +#define SYS_E_VREF_GPIO_E3_SYSCON_REG 0x108
> +#define SYS_E_VREF_GPIO_E3_SYSCON_MASK GENMASK(1, 0)
> +#define SYS_E_VREF_GPIO_E3_SYSCON_SHIFT 0
> +#define SYS_E_VREF_ATB_STG1_SYSCON_REG 0x10c
> +#define SYS_E_VREF_ATB_STG1_SYSCON_MASK GENMASK(1, 0)
> +#define SYS_E_VREF_ATB_STG1_SYSCON_SHIFT 0
> +#define SYS_E_VREF_ATB_USB_SYSCON_REG 0x110
> +#define SYS_E_VREF_ATB_USB_SYSCON_MASK GENMASK(1, 0)
> +#define SYS_E_VREF_ATB_USB_SYSCON_SHIFT 0
Drop all of this, not bindings.
> +
> +/* sys_iomux_gmac pins */
> +#define PAD_GMAC1_MDC 0
> +#define PAD_GMAC1_MDIO 1
> +#define PAD_GMAC1_RXD0 2
> +#define PAD_GMAC1_RXD1 3
> +#define PAD_GMAC1_RXD2 4
> +#define PAD_GMAC1_RXD3 5
> +#define PAD_GMAC1_RXDV 6
> +#define PAD_GMAC1_RXC 7
> +#define PAD_GMAC1_TXD0 8
> +#define PAD_GMAC1_TXD1 9
> +#define PAD_GMAC1_TXD2 10
> +#define PAD_GMAC1_TXD3 11
> +#define PAD_GMAC1_TXEN 12
> +#define PAD_GMAC1_TXC 13
> +
> +/* sys_iomux_gmac vref syscon registers */
> +#define SYS_G_VREF_GMAC1_SYSCON_REG 0x08
> +#define SYS_G_VREF_GMAC1_SYSCON_MASK GENMASK(1, 0)
> +#define SYS_G_VREF_GMAC1_SYSCON_SHIFT 0
> +#define SYS_G_VREF_SDIO1_SYSCON_REG 0x0c
> +#define SYS_G_VREF_SDIO1_SYSCON_MASK GENMASK(1, 0)
> +#define SYS_G_VREF_SDIO1_SYSCON_SHIFT 0
Drop all this.
> +
> +/* sys_iomux_gmac interface (rmii/rgmii) syscon registers */
> +#define SYS_G_GMAC1_MDC_SYSCON_REG 0x10
> +#define SYS_G_GMAC1_MDC_SYSCON_MASK GENMASK(1, 0)
> +#define SYS_G_GMAC1_MDC_SYSCON_SHIFT 0
> +#define SYS_G_GMAC1_MDIO_SYSCON_REG 0x14
> +#define SYS_G_GMAC1_MDIO_SYSCON_MASK GENMASK(1, 0)
> +#define SYS_G_GMAC1_MDIO_SYSCON_SHIFT 0
> +#define SYS_G_GMAC1_RXD0_SYSCON_REG 0x18
> +#define SYS_G_GMAC1_RXD0_SYSCON_MASK GENMASK(1, 0)
> +#define SYS_G_GMAC1_RXD0_SYSCON_SHIFT 0
> +#define SYS_G_GMAC1_RXD1_SYSCON_REG 0x1c
> +#define SYS_G_GMAC1_RXD1_SYSCON_MASK GENMASK(1, 0)
> +#define SYS_G_GMAC1_RXD1_SYSCON_SHIFT 0
> +#define SYS_G_GMAC1_RXD2_SYSCON_REG 0x20
> +#define SYS_G_GMAC1_RXD2_SYSCON_MASK GENMASK(1, 0)
> +#define SYS_G_GMAC1_RXD2_SYSCON_SHIFT 0
> +#define SYS_G_GMAC1_RXD3_SYSCON_REG 0x24
> +#define SYS_G_GMAC1_RXD3_SYSCON_MASK GENMASK(1, 0)
> +#define SYS_G_GMAC1_RXD3_SYSCON_SHIFT 0
> +#define SYS_G_GMAC1_RXDV_SYSCON_REG 0x28
> +#define SYS_G_GMAC1_RXDV_SYSCON_MASK GENMASK(1, 0)
> +#define SYS_G_GMAC1_RXDV_SYSCON_SHIFT 0
> +#define SYS_G_GMAC1_RXC_SYSCON_REG 0x2c
> +#define SYS_G_GMAC1_RXC_SYSCON_MASK GENMASK(1, 0)
> +#define SYS_G_GMAC1_RXC_SYSCON_SHIFT 0
> +#define SYS_G_GMAC1_TXD0_SYSCON_REG 0x30
> +#define SYS_G_GMAC1_TXD0_SYSCON_MASK GENMASK(1, 0)
> +#define SYS_G_GMAC1_TXD0_SYSCON_SHIFT 0
> +#define SYS_G_GMAC1_TXD1_SYSCON_REG 0x34
> +#define SYS_G_GMAC1_TXD1_SYSCON_MASK GENMASK(1, 0)
> +#define SYS_G_GMAC1_TXD1_SYSCON_SHIFT 0
> +#define SYS_G_GMAC1_TXD2_SYSCON_REG 0x38
> +#define SYS_G_GMAC1_TXD2_SYSCON_MASK GENMASK(1, 0)
> +#define SYS_G_GMAC1_TXD2_SYSCON_SHIFT 0
> +#define SYS_G_GMAC1_TXD3_SYSCON_REG 0x3c
> +#define SYS_G_GMAC1_TXD3_SYSCON_MASK GENMASK(1, 0)
> +#define SYS_G_GMAC1_TXD3_SYSCON_SHIFT 0
> +#define SYS_G_GMAC1_TXEN_SYSCON_REG 0x40
> +#define SYS_G_GMAC1_TXEN_SYSCON_MASK GENMASK(1, 0)
> +#define SYS_G_GMAC1_TXEN_SYSCON_SHIFT 0
> +#define SYS_G_GMAC1_TXC_SYSCON_REG 0x44
> +#define SYS_G_GMAC1_TXC_SYSCON_MASK GENMASK(1, 0)
> +#define SYS_G_GMAC1_TXC_SYSCON_SHIFT 0
Drop all this.
> +
> +/* sys_iomux_gmac timing (slew rate) registers */
Srsly, "registers", so not bindings.
> +
> +#define GPOUT_LOW 0
> +#define GPOUT_HIGH 1
That's it. Really. Please do not redefine existing bindings.
> +
> +#define GPOEN_ENABLE 0
> +#define GPOEN_DISABLE 1
> +
> +#define GPI_NONE 255
> +
> +/* vref syscon value */
> +#define PAD_VREF_SYSCON_IO_3V3 0
> +#define PAD_VREF_SYSCON_IO_1V8 2
> +
> +/* gmac interface (rmii/rgmii) syscon value */
> +#define GMAC_RMII_MODE 0 /* RMII mode, DVDD 2.5V/3.3V */
> +#define GMAC_RGMII_MODE 1 /* RGMII mode, DVDD 1.8V/2.5V */
> +
> +/* gmac timing syscon value */
> +#define GMAC_SLEW_RATE_FAST 0
> +#define GMAC_SLEW_RATE_SLOW 1
Drop all above.
> +
> +#endif
Best regards,
Krzysztof
next prev parent reply other threads:[~2023-12-21 15:49 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-12-21 8:36 [RFC PATCH 0/6] Add Pinctrl driver for Starfive JH8100 SoC Alex Soo
2023-12-21 8:36 ` [RFC PATCH 1/6] dt-bindings: pinctrl: starfive: add JH8100 pinctrl bindings Alex Soo
2023-12-21 9:25 ` Rob Herring
2023-12-21 13:57 ` Linus Walleij
2023-12-21 15:45 ` Krzysztof Kozlowski
2024-01-04 3:12 ` Yuklin Soo
2024-01-04 18:07 ` Conor Dooley
2024-01-04 2:57 ` Yuklin Soo
2023-12-21 15:49 ` Krzysztof Kozlowski [this message]
2024-02-07 2:42 ` Yuklin Soo
2024-02-20 8:09 ` Krzysztof Kozlowski
2024-03-05 12:00 ` Yuklin Soo
2024-03-05 14:07 ` Krzysztof Kozlowski
2024-03-27 11:25 ` Yuklin Soo
2023-12-21 8:36 ` [RFC PATCH 2/6] pinctrl: starfive: jh8100: add pinctrl driver for sys_east domain Alex Soo
2024-01-27 23:33 ` Linus Walleij
2024-02-20 5:55 ` Yuklin Soo
2023-12-21 8:36 ` [RFC PATCH 3/6] pinctrl: starfive: jh8100: add pinctrl driver for sys_west domain Alex Soo
2023-12-21 8:36 ` [RFC PATCH 4/6] pinctrl: starfive: jh8100: add pinctrl driver for sys_gmac domain Alex Soo
2023-12-21 8:36 ` [RFC PATCH 5/6] pinctrl: starfive: jh8100: add pinctrl driver for AON domain Alex Soo
2023-12-21 8:36 ` [RFC PATCH 6/6] riscv: dts: starfive: jh8100: add pinctrl device tree nodes Alex Soo
2023-12-21 15:53 ` Krzysztof Kozlowski
2024-02-07 3:09 ` Yuklin Soo
2023-12-21 16:19 ` [RFC PATCH 0/6] Add Pinctrl driver for Starfive JH8100 SoC Krzysztof Kozlowski
2024-01-04 8:28 ` Yuklin Soo
2023-12-22 17:58 ` Linus Walleij
2024-02-20 5:38 ` Yuklin Soo
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=3b9201e0-e1d9-463c-aa1a-cbbedd1f8907@linaro.org \
--to=krzysztof.kozlowski@linaro.org \
--cc=aou@eecs.berkeley.edu \
--cc=bartosz.golaszewski@linaro.org \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=drew@beagleboard.org \
--cc=hal.feng@starfivetech.com \
--cc=jianlong.huang@starfivetech.com \
--cc=kernel@esmil.dk \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=leyfoon.tan@starfivetech.com \
--cc=linus.walleij@linaro.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-riscv@lists.infradead.org \
--cc=palmer@dabbelt.com \
--cc=paul.walmsley@sifive.com \
--cc=robh@kernel.org \
--cc=yuklin.soo@starfivetech.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;
as well as URLs for NNTP newsgroup(s).