pinctrl: nuvoton: add pinmux and GPIO driver for NPCM8XX
Subsystems (4)
Tomer Maimon
maintainercover pinctrl: nuvoton: add pinmux and GPIO driver for NPCM8XX
This patch set adds pinmux and GPIO controller for the Arbel NPCM8XX Baseboard Management Controller (BMC).
Arbel BMC NPCM8XX pinctrl driver based on Poleg NPCM7XX, except the pin mux mapping difference the NPCM8XX GPIO supports adjust debounce period time.
Arbel BMC NPCM8XX Pinmux functions accessible only for pin groups and pin configuration parameters available only for individual pins.
Arbel BMC NPCM8XX has eight identical GPIO modules, each module has 32 GPIO ports.
Most of the GPIO ports are multiplexed with other system functions.
The NPCM8XX pinctrl and GPIO driver were tested on NPCM845 evaluation board.
Tomer Maimon (2):
dt-binding: pinctrl: Add NPCM8XX pinctrl and GPIO documentation
pinctrl: nuvoton: add NPCM8XX pinctrl and GPIO driver
.../pinctrl/nuvoton,npcm845-pinctrl.yaml | 205 ++ drivers/pinctrl/nuvoton/Kconfig | 13 + drivers/pinctrl/nuvoton/Makefile | 1 + drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c | 2634 +++++++++++++++++ 4 files changed, 2853 insertions(+) create mode 100644 Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml create mode 100644 drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c
Signature · 1 line
2.33.0
Tomer Maimon
maintainerPATCH 1/2 dt-binding: pinctrl: Add NPCM8XX pinctrl and GPIO documentation
Added device tree binding documentation for Nuvoton Arbel BMC NPCM8XX pinmux and GPIO controller.
--- .../pinctrl/nuvoton,npcm845-pinctrl.yaml | 205 ++++++++++++++++++ 1 file changed, 205 insertions(+) create mode 100644 Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml
Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml new file +205 -0
@@ -0,0 +1,205 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/pinctrl/nuvoton,npcm845-pinctrl.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Nuvoton NPCM845 Pin Controller and GPIO + +maintainers: + - Tomer Maimon <[email protected]> + +description: + The Nuvoton BMC NPCM8XX Pin Controller multi-function routed through + the multiplexing block, Each pin supports GPIO functionality (GPIOx) + and multiple functions that directly connect the pin to different + hardware blocks. + +properties: + compatible: + const: nuvoton,npcm845-pinctrl + + ranges: + maxItems: 1 + + '#address-cells': + const: 1 + + '#size-cells': + const: 1 + +patternProperties: + "^gpio@": + type: object + + description: + Eight GPIO banks that each contain between 32 GPIOs. + + properties: + + gpio-controller: true + + '#gpio-cells': + const: 2 + + reg: + maxItems: 1 + + interrupts: + maxItems: 1 + + gpio-ranges: + maxItems: 1 + + required: + - gpio-controller + - '#gpio-cells' + - reg + - interrupts + - gpio-ranges + + "-pin": + $ref: pinmux-node.yaml# + + properties: + groups: + description: + One or more groups of pins to mux to a certain function + items: + enum: [ iox1, iox2, smb1d, smb2d, lkgpo1, lkgpo2, ioxh, gspi, + smb5b, smb5c, lkgpo0, pspi2, jm1, jm2, smb4den, smb4b, + smb4c, smb15, smb16, smb17, smb18, smb19, smb20, smb21, + smb22, smb23, smb4d, smb14, smb5, smb4, smb3, spi0cs1, + spi0cs2, spi0cs3, smb3c, smb3b, bmcuart0a, uart1, jtag2, + bmcuart1, uart2, bmcuart0b, r1err, r1md, r1oen, r2oen, + rmii3, r3oen, smb3d, fanin0, fanin1, fanin2, fanin3, fanin4, + fanin5, fanin6, fanin7, fanin8, fanin9, fanin10, fanin11, + fanin12, fanin13, fanin14, fanin15, pwm0, pwm1, pwm2, pwm3, + r2, r2err, r2md, r3rxer, ga20kbc, smb5d, lpc, espi, rg1, + rg1mdio, rg2, ddr, i3c0, i3c1, i3c2, i3c3, i3c4, i3c5, + smb0, smb1, smb2, smb2c, smb2b, smb1c, smb1b, smb8, smb9, + smb10, smb11, sd1, sd1pwr, pwm4, pwm5, pwm6, pwm7, pwm8, + pwm9, pwm10, pwm11, mmc8, mmc, mmcwp, mmccd, mmcrst, clkout, + serirq, lpcclk, scipme, sci, smb6, smb7, spi1, faninx, r1, + spi3, spi3cs1, spi3quad, spi3cs2, spi3cs3, nprd_smi, smb0b, + smb0c, smb0den, smb0d, ddc, rg2mdio, wdog1, wdog2, smb12, + smb13, spix, spixcs1, clkreq, hgpio0, hgpio1, hgpio2, hgpio3, + hgpio4, hgpio5, hgpio6, hgpio7 ] + + function: + description: + The function that a group of pins is muxed to + enum: [ iox1, iox2, smb1d, smb2d, lkgpo1, lkgpo2, ioxh, gspi, + smb5b, smb5c, lkgpo0, pspi2, jm1, jm2, smb4den, smb4b, + smb4c, smb15, smb16, smb17, smb18, smb19, smb20, smb21, + smb22, smb23, smb4d, smb14, smb5, smb4, smb3, spi0cs1, + spi0cs2, spi0cs3, smb3c, smb3b, bmcuart0a, uart1, jtag2, + bmcuart1, uart2, bmcuart0b, r1err, r1md, r1oen, r2oen, + rmii3, r3oen, smb3d, fanin0, fanin1, fanin2, fanin3, fanin4, + fanin5, fanin6, fanin7, fanin8, fanin9, fanin10, fanin11, + fanin12, fanin13, fanin14, fanin15, pwm0, pwm1, pwm2, pwm3, + r2, r2err, r2md, r3rxer, ga20kbc, smb5d, lpc, espi, rg1, + rg1mdio, rg2, ddr, i3c0, i3c1, i3c2, i3c3, i3c4, i3c5, + smb0, smb1, smb2, smb2c, smb2b, smb1c, smb1b, smb8, smb9, + smb10, smb11, sd1, sd1pwr, pwm4, pwm5, pwm6, pwm7, pwm8, + pwm9, pwm10, pwm11, mmc8, mmc, mmcwp, mmccd, mmcrst, clkout, + serirq, lpcclk, scipme, sci, smb6, smb7, spi1, faninx, r1, + spi3, spi3cs1, spi3quad, spi3cs2, spi3cs3, nprd_smi, smb0b, + smb0c, smb0den, smb0d, ddc, rg2mdio, wdog1, wdog2, smb12, + smb13, spix, spixcs1, clkreq, hgpio0, hgpio1, hgpio2, hgpio3, + hgpio4, hgpio5, hgpio6, hgpio7 ] + + dependencies: + groups: [ function ] + function: [ groups ] + + additionalProperties: false + + "^pin": + $ref: pincfg-node.yaml# + + properties: + pins: + description: + A list of pins to configure in certain ways, such as enabling + debouncing + + bias-disable: true + + bias-pull-up: true + + bias-pull-down: true + + input-enable: true + + output-low: true + + output-high: true + + drive-push-pull: true + + drive-open-drain: true + + input-debounce: + description: + Debouncing periods in microseconds, one period per interrupt + bank found in the controller + $ref: /schemas/types.yaml#/definitions/uint32-array + minItems: 1 + maxItems: 4 + + slew-rate: + description: | + 0: Low rate + 1: High rate + $ref: /schemas/types.yaml#/definitions/uint32 + enum: [0, 1] + + drive-strength: + enum: [ 0, 1, 2, 4, 8, 12 ] + + additionalProperties: false + +required: + - compatible + - ranges + - '#address-cells' + - '#size-cells' + +additionalProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/arm-gic.h> + #include <dt-bindings/gpio/gpio.h> + + soc { + #address-cells = <2>; + #size-cells = <2>; + + pinctrl: pinctrl@f0800000 { + compatible = "nuvoton,npcm845-pinctrl"; + ranges = <0x0 0x0 0xf0010000 0x8000>; + #address-cells = <1>; + #size-cells = <1>; + + gpio0: gpio@f0010000 { + gpio-controller; + #gpio-cells = <2>; + reg = <0x0 0xB0>; + interrupts = <GIC_SPI 116 IRQ_TYPE_LEVEL_HIGH>; + gpio-ranges = <&pinctrl 0 0 32>; + }; + + fanin0_pin: fanin0-pin { + groups = "fanin0"; + function = "fanin0"; + }; + + pin34_slew: pin34-slew { + pins = "GPIO34/I3C4_SDA"; + bias-disable; + }; + }; + }; +
Signature · 1 line
2.33.0
Re: [PATCH v1 1/2] dt-binding: pinctrl: Add NPCM8XX pinctrl and GPIO documentation
Quoted 28 lines · Tomer Maimon
Added device tree binding documentation for Nuvoton Arbel BMC NPCM8XX pinmux and GPIO controller.
Signed-off-by Tomer Maimon <[email protected]>--- .../pinctrl/nuvoton,npcm845-pinctrl.yaml | 205 ++++++++++++++++++ 1 file changed, 205 insertions(+) create mode 100644 Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yamlDocumentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml new file +13
-0@@ -0,0 +1,205 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/pinctrl/nuvoton,npcm845-pinctrl.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Nuvoton NPCM845 Pin Controller and GPIO + +maintainers: + - Tomer Maimon <[email protected]> + +description: + The Nuvoton BMC NPCM8XX Pin Controller multi-function routed through
Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml line 23
Quoted 10 lines
Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml new file +10
-0+ the multiplexing block, Each pin supports GPIO functionality (GPIOx) + and multiple functions that directly connect the pin to different + hardware blocks. + +properties: + compatible: + const: nuvoton,npcm845-pinctrl + + ranges: + maxItems: 1
ranges without reg? Does it even work? Did you test the bindings?
Quoted 6 lines
Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml new file +6
-0+ + '#address-cells': + const: 1 + + '#size-cells': + const: 1
Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml line 39
Quoted 10 lines
Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml new file +10
-0+ +patternProperties: + "^gpio@": + type: object + + description: + Eight GPIO banks that each contain between 32 GPIOs. + + properties: +
No blank line.
Quoted 13 lines
Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml new file +13
-0+ gpio-controller: true + + '#gpio-cells': + const: 2 + + reg: + maxItems: 1 + + interrupts: + maxItems: 1 + + gpio-ranges: + maxItems: 1
Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml line 62
Quoted 10 lines
Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml new file +10
-0+ + required: + - gpio-controller + - '#gpio-cells' + - reg + - interrupts + - gpio-ranges + + "-pin": + $ref: pinmux-node.yaml#
Shouldn't this be under bank?
Quoted 46 lines
Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml new file +46
-0+ + properties: + groups: + description: + One or more groups of pins to mux to a certain function + items: + enum: [ iox1, iox2, smb1d, smb2d, lkgpo1, lkgpo2, ioxh, gspi, + smb5b, smb5c, lkgpo0, pspi2, jm1, jm2, smb4den, smb4b, + smb4c, smb15, smb16, smb17, smb18, smb19, smb20, smb21, + smb22, smb23, smb4d, smb14, smb5, smb4, smb3, spi0cs1, + spi0cs2, spi0cs3, smb3c, smb3b, bmcuart0a, uart1, jtag2, + bmcuart1, uart2, bmcuart0b, r1err, r1md, r1oen, r2oen, + rmii3, r3oen, smb3d, fanin0, fanin1, fanin2, fanin3, fanin4, + fanin5, fanin6, fanin7, fanin8, fanin9, fanin10, fanin11, + fanin12, fanin13, fanin14, fanin15, pwm0, pwm1, pwm2, pwm3, + r2, r2err, r2md, r3rxer, ga20kbc, smb5d, lpc, espi, rg1, + rg1mdio, rg2, ddr, i3c0, i3c1, i3c2, i3c3, i3c4, i3c5, + smb0, smb1, smb2, smb2c, smb2b, smb1c, smb1b, smb8, smb9, + smb10, smb11, sd1, sd1pwr, pwm4, pwm5, pwm6, pwm7, pwm8, + pwm9, pwm10, pwm11, mmc8, mmc, mmcwp, mmccd, mmcrst, clkout, + serirq, lpcclk, scipme, sci, smb6, smb7, spi1, faninx, r1, + spi3, spi3cs1, spi3quad, spi3cs2, spi3cs3, nprd_smi, smb0b, + smb0c, smb0den, smb0d, ddc, rg2mdio, wdog1, wdog2, smb12, + smb13, spix, spixcs1, clkreq, hgpio0, hgpio1, hgpio2, hgpio3, + hgpio4, hgpio5, hgpio6, hgpio7 ] + + function: + description: + The function that a group of pins is muxed to + enum: [ iox1, iox2, smb1d, smb2d, lkgpo1, lkgpo2, ioxh, gspi, + smb5b, smb5c, lkgpo0, pspi2, jm1, jm2, smb4den, smb4b, + smb4c, smb15, smb16, smb17, smb18, smb19, smb20, smb21, + smb22, smb23, smb4d, smb14, smb5, smb4, smb3, spi0cs1, + spi0cs2, spi0cs3, smb3c, smb3b, bmcuart0a, uart1, jtag2, + bmcuart1, uart2, bmcuart0b, r1err, r1md, r1oen, r2oen, + rmii3, r3oen, smb3d, fanin0, fanin1, fanin2, fanin3, fanin4, + fanin5, fanin6, fanin7, fanin8, fanin9, fanin10, fanin11, + fanin12, fanin13, fanin14, fanin15, pwm0, pwm1, pwm2, pwm3, + r2, r2err, r2md, r3rxer, ga20kbc, smb5d, lpc, espi, rg1, + rg1mdio, rg2, ddr, i3c0, i3c1, i3c2, i3c3, i3c4, i3c5, + smb0, smb1, smb2, smb2c, smb2b, smb1c, smb1b, smb8, smb9, + smb10, smb11, sd1, sd1pwr, pwm4, pwm5, pwm6, pwm7, pwm8, + pwm9, pwm10, pwm11, mmc8, mmc, mmcwp, mmccd, mmcrst, clkout, + serirq, lpcclk, scipme, sci, smb6, smb7, spi1, faninx, r1, + spi3, spi3cs1, spi3quad, spi3cs2, spi3cs3, nprd_smi, smb0b, + smb0c, smb0den, smb0d, ddc, rg2mdio, wdog1, wdog2, smb12,
Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml line 118
Quoted 10 lines
Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml new file +10
-0+ smb13, spix, spixcs1, clkreq, hgpio0, hgpio1, hgpio2, hgpio3, + hgpio4, hgpio5, hgpio6, hgpio7 ] + + dependencies: + groups: [ function ] + function: [ groups ] + + additionalProperties: false + + "^pin":
This is almost the same as previous property. Confusing and I think it does not work.
Quoted 39 lines
Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml new file +39
-0+ $ref: pincfg-node.yaml# + + properties: + pins: + description: + A list of pins to configure in certain ways, such as enabling + debouncing + + bias-disable: true + + bias-pull-up: true + + bias-pull-down: true + + input-enable: true + + output-low: true + + output-high: true + + drive-push-pull: true + + drive-open-drain: true + + input-debounce: + description: + Debouncing periods in microseconds, one period per interrupt + bank found in the controller + $ref: /schemas/types.yaml#/definitions/uint32-array + minItems: 1 + maxItems: 4 + + slew-rate: + description: | + 0: Low rate + 1: High rate + $ref: /schemas/types.yaml#/definitions/uint32 + enum: [0, 1] +
Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml line 167
Quoted 10 lines
Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml new file +10
-0+ drive-strength: + enum: [ 0, 1, 2, 4, 8, 12 ] + + additionalProperties: false + +required: + - compatible + - ranges + - '#address-cells' + - '#size-cells'
Missing allOf with ref to pinctrl.yaml.
Quoted 22 lines
Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml new file +22
-0+ +additionalProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/arm-gic.h> + #include <dt-bindings/gpio/gpio.h> + + soc { + #address-cells = <2>; + #size-cells = <2>; + + pinctrl: pinctrl@f0800000 { + compatible = "nuvoton,npcm845-pinctrl"; + ranges = <0x0 0x0 0xf0010000 0x8000>; + #address-cells = <1>; + #size-cells = <1>; + + gpio0: gpio@f0010000 { + gpio-controller; + #gpio-cells = <2>; + reg = <0x0 0xB0>;
Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml line 199
Quoted 10 lines
Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml new file +10
-0+ interrupts = <GIC_SPI 116 IRQ_TYPE_LEVEL_HIGH>; + gpio-ranges = <&pinctrl 0 0 32>; + }; + + fanin0_pin: fanin0-pin { + groups = "fanin0"; + function = "fanin0"; + }; + + pin34_slew: pin34-slew {
and how does it pass your checks?
Did you test the bindings?
Best regards,
Krzysztof
Tomer Maimon
maintainerRe: [PATCH v1 1/2] dt-binding: pinctrl: Add NPCM8XX pinctrl and GPIO documentation
Hi Krzysztof,
Thanks for your comments.
Quoted 34 lines · Krzysztof Kozlowski
Quoted 32 lines · Tomer Maimon
Added device tree binding documentation for Nuvoton Arbel BMC NPCM8XX pinmux and GPIO controller.
Signed-off-by Tomer Maimon <[email protected]>--- .../pinctrl/nuvoton,npcm845-pinctrl.yaml | 205 ++++++++++++++++++ 1 file changed, 205 insertions(+) create mode 100644 Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yamlDocumentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml new file +17
-0@@ -0,0 +1,205 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/pinctrl/nuvoton,npcm845-pinctrl.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Nuvoton NPCM845 Pin Controller and GPIO + +maintainers: + - Tomer Maimon <[email protected]> + +description: + The Nuvoton BMC NPCM8XX Pin Controller multi-function routed through + the multiplexing block, Each pin supports GPIO functionality (GPIOx) + and multiple functions that directly connect the pin to different + hardware blocks. +
Quoted 8 lines
Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml line 23
Quoted 6 lines
Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml new file +6
-0+properties: + compatible: + const: nuvoton,npcm845-pinctrl + + ranges: + maxItems: 1ranges without reg? Does it even work? Did you test the bindings?
The ranges related to GPIO node reg
I did test the pin controller document and it passed.
bash-4.2$ make ARCH=arm64 dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml
LINT Documentation/devicetree/bindings CHKDT Documentation/devicetree/bindings/processed-schema.json SCHEMA Documentation/devicetree/bindings/processed-schema.json DTEX Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.example.dts DTC Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.example.dtb CHECK Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.example.dtb
Did I need to run anything else than dt_binding_check for testing the document?
Quoted 11 lines
Quoted 11 lines
Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml new file +10
-0+ + '#address-cells': + const: 1 + + '#size-cells': + const: 1 + +patternProperties: + "^gpio@": + type: object
Quoted 8 lines
Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml line 39
Quoted 6 lines
Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml new file +6
-0+ + description: + Eight GPIO banks that each contain between 32 GPIOs. + + properties: +No blank line.
O.K.
Quoted 18 lines
Quoted 18 lines
Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml new file +17
-0+ gpio-controller: true + + '#gpio-cells': + const: 2 + + reg: + maxItems: 1 + + interrupts: + maxItems: 1 + + gpio-ranges: + maxItems: 1 + + required: + - gpio-controller + - '#gpio-cells'
Quoted 8 lines
Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml line 62
Quoted 6 lines
Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml new file +6
-0+ - reg + - interrupts + - gpio-ranges + + "-pin": + $ref: pinmux-node.yaml#Shouldn't this be under bank?
Do you mean after the group and function properties? The -pin shouldn't use for the group property naming?
Quoted 52 lines
Quoted 52 lines
Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml new file +51
-0+ + properties: + groups: + description: + One or more groups of pins to mux to a certain function + items: + enum: [ iox1, iox2, smb1d, smb2d, lkgpo1, lkgpo2, ioxh, gspi, + smb5b, smb5c, lkgpo0, pspi2, jm1, jm2, smb4den, smb4b, + smb4c, smb15, smb16, smb17, smb18, smb19, smb20, smb21, + smb22, smb23, smb4d, smb14, smb5, smb4, smb3, spi0cs1, + spi0cs2, spi0cs3, smb3c, smb3b, bmcuart0a, uart1, jtag2, + bmcuart1, uart2, bmcuart0b, r1err, r1md, r1oen, r2oen, + rmii3, r3oen, smb3d, fanin0, fanin1, fanin2, fanin3, fanin4, + fanin5, fanin6, fanin7, fanin8, fanin9, fanin10, fanin11, + fanin12, fanin13, fanin14, fanin15, pwm0, pwm1, pwm2, pwm3, + r2, r2err, r2md, r3rxer, ga20kbc, smb5d, lpc, espi, rg1, + rg1mdio, rg2, ddr, i3c0, i3c1, i3c2, i3c3, i3c4, i3c5, + smb0, smb1, smb2, smb2c, smb2b, smb1c, smb1b, smb8, smb9, + smb10, smb11, sd1, sd1pwr, pwm4, pwm5, pwm6, pwm7, pwm8, + pwm9, pwm10, pwm11, mmc8, mmc, mmcwp, mmccd, mmcrst, clkout, + serirq, lpcclk, scipme, sci, smb6, smb7, spi1, faninx, r1, + spi3, spi3cs1, spi3quad, spi3cs2, spi3cs3, nprd_smi, smb0b, + smb0c, smb0den, smb0d, ddc, rg2mdio, wdog1, wdog2, smb12, + smb13, spix, spixcs1, clkreq, hgpio0, hgpio1, hgpio2, hgpio3, + hgpio4, hgpio5, hgpio6, hgpio7 ] + + function: + description: + The function that a group of pins is muxed to + enum: [ iox1, iox2, smb1d, smb2d, lkgpo1, lkgpo2, ioxh, gspi, + smb5b, smb5c, lkgpo0, pspi2, jm1, jm2, smb4den, smb4b, + smb4c, smb15, smb16, smb17, smb18, smb19, smb20, smb21, + smb22, smb23, smb4d, smb14, smb5, smb4, smb3, spi0cs1, + spi0cs2, spi0cs3, smb3c, smb3b, bmcuart0a, uart1, jtag2, + bmcuart1, uart2, bmcuart0b, r1err, r1md, r1oen, r2oen, + rmii3, r3oen, smb3d, fanin0, fanin1, fanin2, fanin3, fanin4, + fanin5, fanin6, fanin7, fanin8, fanin9, fanin10, fanin11, + fanin12, fanin13, fanin14, fanin15, pwm0, pwm1, pwm2, pwm3, + r2, r2err, r2md, r3rxer, ga20kbc, smb5d, lpc, espi, rg1, + rg1mdio, rg2, ddr, i3c0, i3c1, i3c2, i3c3, i3c4, i3c5, + smb0, smb1, smb2, smb2c, smb2b, smb1c, smb1b, smb8, smb9, + smb10, smb11, sd1, sd1pwr, pwm4, pwm5, pwm6, pwm7, pwm8, + pwm9, pwm10, pwm11, mmc8, mmc, mmcwp, mmccd, mmcrst, clkout, + serirq, lpcclk, scipme, sci, smb6, smb7, spi1, faninx, r1, + spi3, spi3cs1, spi3quad, spi3cs2, spi3cs3, nprd_smi, smb0b, + smb0c, smb0den, smb0d, ddc, rg2mdio, wdog1, wdog2, smb12, + smb13, spix, spixcs1, clkreq, hgpio0, hgpio1, hgpio2, hgpio3, + hgpio4, hgpio5, hgpio6, hgpio7 ] + + dependencies: + groups: [ function ]
Quoted 8 lines
Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml line 118
Quoted 5 lines
Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml new file +5
-0+ function: [ groups ] + + additionalProperties: false + + "^pin":This is almost the same as previous property. Confusing and I think it does not work.
if I remove it I get the following error:
pinctrl@f0800000: 'pin34-slew' does not match any of the regexes:
'-pin', '^gpio@', 'pinctrl-[0-9]+'
Can you advise what I should do?
Quoted 44 lines
Quoted 44 lines
Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml new file +43
-0+ $ref: pincfg-node.yaml# + + properties: + pins: + description: + A list of pins to configure in certain ways, such as enabling + debouncing + + bias-disable: true + + bias-pull-up: true + + bias-pull-down: true + + input-enable: true + + output-low: true + + output-high: true + + drive-push-pull: true + + drive-open-drain: true + + input-debounce: + description: + Debouncing periods in microseconds, one period per interrupt + bank found in the controller + $ref: /schemas/types.yaml#/definitions/uint32-array + minItems: 1 + maxItems: 4 + + slew-rate: + description: | + 0: Low rate + 1: High rate + $ref: /schemas/types.yaml#/definitions/uint32 + enum: [0, 1] + + drive-strength: + enum: [ 0, 1, 2, 4, 8, 12 ] + + additionalProperties: false
Quoted 8 lines
Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml line 167
Quoted 6 lines
Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml new file +6
-0+ +required: + - compatible + - ranges + - '#address-cells' + - '#size-cells'Missing allOf with ref to
pinctrl.yaml.
Do you mean adding
allOf:
- $ref: "pinctrl.yaml#"
Quoted 27 lines
Quoted 27 lines
Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml new file +26
-0+ +additionalProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/arm-gic.h> + #include <dt-bindings/gpio/gpio.h> + + soc { + #address-cells = <2>; + #size-cells = <2>; + + pinctrl: pinctrl@f0800000 { + compatible = "nuvoton,npcm845-pinctrl"; + ranges = <0x0 0x0 0xf0010000 0x8000>; + #address-cells = <1>; + #size-cells = <1>; + + gpio0: gpio@f0010000 { + gpio-controller; + #gpio-cells = <2>; + reg = <0x0 0xB0>; + interrupts = <GIC_SPI 116 IRQ_TYPE_LEVEL_HIGH>; + gpio-ranges = <&pinctrl 0 0 32>; + }; +
Quoted 8 lines
Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml line 199
Quoted 6 lines
Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml new file +6
-0+ fanin0_pin: fanin0-pin { + groups = "fanin0"; + function = "fanin0"; + }; + + pin34_slew: pin34-slew {and how does it pass your checks?
Yes
Quoted 7 lines
Did you test the bindings?
Best regards,
Krzysztof
Best regards,
Tomer
Re: [PATCH v1 1/2] dt-binding: pinctrl: Add NPCM8XX pinctrl and GPIO documentation
Quoted 48 lines · Tomer Maimon
Hi Krzysztof,
Thanks for your comments.
Quoted 34 lines · Krzysztof Kozlowski
Quoted 32 lines · Tomer Maimon
Added device tree binding documentation for Nuvoton Arbel BMC NPCM8XX pinmux and GPIO controller.
Signed-off-by Tomer Maimon <[email protected]>--- .../pinctrl/nuvoton,npcm845-pinctrl.yaml | 205 ++++++++++++++++++ 1 file changed, 205 insertions(+) create mode 100644 Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yamlDocumentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml new file +17
-0@@ -0,0 +1,205 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/pinctrl/nuvoton,npcm845-pinctrl.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Nuvoton NPCM845 Pin Controller and GPIO + +maintainers: + - Tomer Maimon <[email protected]> + +description: + The Nuvoton BMC NPCM8XX Pin Controller multi-function routed through + the multiplexing block, Each pin supports GPIO functionality (GPIOx) + and multiple functions that directly connect the pin to different + hardware blocks. +Quoted 8 lines
Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml line 23
Quoted 6 lines
Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml new file +6
-0+properties: + compatible: + const: nuvoton,npcm845-pinctrl + + ranges: + maxItems: 1ranges without reg? Does it even work? Did you test the bindings?
Quoted 1 line
The ranges related to GPIO node reg
But you do not allow here a 'reg', do you? So how can you have an unit address in pinctrl node?
Quoted 11 lines
I did test the pin controller document and it passed.
bash-4.2$ make ARCH=arm64dt_binding_checkDT_SCHEMA_FILES=Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yamlLINT Documentation/devicetree/bindings CHKDT Documentation/devicetree/bindings/processed-schema.json SCHEMA Documentation/devicetree/bindings/processed-schema.json DTEX Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.example.dts DTC Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.example.dtb CHECK Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.example.dtbDid I need to run anything else than
dt_binding_checkfor testing the document?
Indeed it will pass, because you do not have reg in pinctrl node. But your dts won't pass make dtbs W=1
Quoted 46 lines
Quoted 11 lines
Quoted 11 lines
Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml new file +10
-0+ + '#address-cells': + const: 1 + + '#size-cells': + const: 1 + +patternProperties: + "^gpio@": + type: objectQuoted 8 lines
Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml line 39
Quoted 6 lines
Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml new file +6
-0+ + description: + Eight GPIO banks that each contain between 32 GPIOs. + + properties: +No blank line.
O.K.
Quoted 18 lines
Quoted 18 lines
Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml new file +17
-0+ gpio-controller: true + + '#gpio-cells': + const: 2 + + reg: + maxItems: 1 + + interrupts: + maxItems: 1 + + gpio-ranges: + maxItems: 1 + + required: + - gpio-controller + - '#gpio-cells'Quoted 8 lines
Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml line 62
Quoted 6 lines
Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml new file +6
-0+ - reg + - interrupts + - gpio-ranges + + "-pin": + $ref: pinmux-node.yaml#Shouldn't this be under bank?
Quoted 2 lines
Do you mean after the group and function properties? The -pin shouldn't use for the group property naming?
Hm, I guess it's fine, I actually don't remember the recommendation for gpio banks in relation to pinmux nodes.
Quoted 60 lines
Quoted 52 lines
Quoted 52 lines
Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml new file +51
-0+ + properties: + groups: + description: + One or more groups of pins to mux to a certain function + items: + enum: [ iox1, iox2, smb1d, smb2d, lkgpo1, lkgpo2, ioxh, gspi, + smb5b, smb5c, lkgpo0, pspi2, jm1, jm2, smb4den, smb4b, + smb4c, smb15, smb16, smb17, smb18, smb19, smb20, smb21, + smb22, smb23, smb4d, smb14, smb5, smb4, smb3, spi0cs1, + spi0cs2, spi0cs3, smb3c, smb3b, bmcuart0a, uart1, jtag2, + bmcuart1, uart2, bmcuart0b, r1err, r1md, r1oen, r2oen, + rmii3, r3oen, smb3d, fanin0, fanin1, fanin2, fanin3, fanin4, + fanin5, fanin6, fanin7, fanin8, fanin9, fanin10, fanin11, + fanin12, fanin13, fanin14, fanin15, pwm0, pwm1, pwm2, pwm3, + r2, r2err, r2md, r3rxer, ga20kbc, smb5d, lpc, espi, rg1, + rg1mdio, rg2, ddr, i3c0, i3c1, i3c2, i3c3, i3c4, i3c5, + smb0, smb1, smb2, smb2c, smb2b, smb1c, smb1b, smb8, smb9, + smb10, smb11, sd1, sd1pwr, pwm4, pwm5, pwm6, pwm7, pwm8, + pwm9, pwm10, pwm11, mmc8, mmc, mmcwp, mmccd, mmcrst, clkout, + serirq, lpcclk, scipme, sci, smb6, smb7, spi1, faninx, r1, + spi3, spi3cs1, spi3quad, spi3cs2, spi3cs3, nprd_smi, smb0b, + smb0c, smb0den, smb0d, ddc, rg2mdio, wdog1, wdog2, smb12, + smb13, spix, spixcs1, clkreq, hgpio0, hgpio1, hgpio2, hgpio3, + hgpio4, hgpio5, hgpio6, hgpio7 ] + + function: + description: + The function that a group of pins is muxed to + enum: [ iox1, iox2, smb1d, smb2d, lkgpo1, lkgpo2, ioxh, gspi, + smb5b, smb5c, lkgpo0, pspi2, jm1, jm2, smb4den, smb4b, + smb4c, smb15, smb16, smb17, smb18, smb19, smb20, smb21, + smb22, smb23, smb4d, smb14, smb5, smb4, smb3, spi0cs1, + spi0cs2, spi0cs3, smb3c, smb3b, bmcuart0a, uart1, jtag2, + bmcuart1, uart2, bmcuart0b, r1err, r1md, r1oen, r2oen, + rmii3, r3oen, smb3d, fanin0, fanin1, fanin2, fanin3, fanin4, + fanin5, fanin6, fanin7, fanin8, fanin9, fanin10, fanin11, + fanin12, fanin13, fanin14, fanin15, pwm0, pwm1, pwm2, pwm3, + r2, r2err, r2md, r3rxer, ga20kbc, smb5d, lpc, espi, rg1, + rg1mdio, rg2, ddr, i3c0, i3c1, i3c2, i3c3, i3c4, i3c5, + smb0, smb1, smb2, smb2c, smb2b, smb1c, smb1b, smb8, smb9, + smb10, smb11, sd1, sd1pwr, pwm4, pwm5, pwm6, pwm7, pwm8, + pwm9, pwm10, pwm11, mmc8, mmc, mmcwp, mmccd, mmcrst, clkout, + serirq, lpcclk, scipme, sci, smb6, smb7, spi1, faninx, r1, + spi3, spi3cs1, spi3quad, spi3cs2, spi3cs3, nprd_smi, smb0b, + smb0c, smb0den, smb0d, ddc, rg2mdio, wdog1, wdog2, smb12, + smb13, spix, spixcs1, clkreq, hgpio0, hgpio1, hgpio2, hgpio3, + hgpio4, hgpio5, hgpio6, hgpio7 ] + + dependencies: + groups: [ function ]Quoted 8 lines
Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml line 118
Quoted 5 lines
Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml new file +5
-0+ function: [ groups ] + + additionalProperties: false + + "^pin":This is almost the same as previous property. Confusing and I think it does not work.
Quoted 4 lines
if I remove it I get the following error:
pinctrl@f0800000: 'pin34-slew' does not match any of the regexes:
'-pin', '^gpio@', 'pinctrl-[0-9]+'
Can you advise what I should do?
Ah, the pattern is indeed different - you start with pin. Anyway it's confusing to have cfg starting with pin and mux ending in pin. How "pin-pin" would work? :)
Use maybe similar pattern, so start with mux for mux and pin for cfg. Look at wpcm450 pinctrl.
Quoted 52 lines
Quoted 44 lines
Quoted 44 lines
Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml new file +43
-0+ $ref: pincfg-node.yaml# + + properties: + pins: + description: + A list of pins to configure in certain ways, such as enabling + debouncing + + bias-disable: true + + bias-pull-up: true + + bias-pull-down: true + + input-enable: true + + output-low: true + + output-high: true + + drive-push-pull: true + + drive-open-drain: true + + input-debounce: + description: + Debouncing periods in microseconds, one period per interrupt + bank found in the controller + $ref: /schemas/types.yaml#/definitions/uint32-array + minItems: 1 + maxItems: 4 + + slew-rate: + description: | + 0: Low rate + 1: High rate + $ref: /schemas/types.yaml#/definitions/uint32 + enum: [0, 1] + + drive-strength: + enum: [ 0, 1, 2, 4, 8, 12 ] + + additionalProperties: falseQuoted 8 lines
Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml line 167
Quoted 6 lines
Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml new file +6
-0+ +required: + - compatible + - ranges + - '#address-cells' + - '#size-cells'Missing allOf with ref to
pinctrl.yaml.
Quoted 3 lines
Do you mean adding
allOf:
- $ref: "pinctrl.yaml#"
Yes.
Best regards,
Krzysztof
Tomer Maimon
maintainerRe: [PATCH v1 1/2] dt-binding: pinctrl: Add NPCM8XX pinctrl and GPIO documentation
Hi Krzysztof,
Thanks for your clarifications.
Quoted 50 lines · Krzysztof Kozlowski
Quoted 48 lines · Tomer Maimon
Hi Krzysztof,
Thanks for your comments.
Quoted 42 lines · Krzysztof Kozlowski
Quoted 28 lines · Tomer Maimon
Added device tree binding documentation for Nuvoton Arbel BMC NPCM8XX pinmux and GPIO controller.
Signed-off-by Tomer Maimon <[email protected]>--- .../pinctrl/nuvoton,npcm845-pinctrl.yaml | 205 ++++++++++++++++++ 1 file changed, 205 insertions(+) create mode 100644 Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yamlDocumentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml new file +13
-0@@ -0,0 +1,205 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/pinctrl/nuvoton,npcm845-pinctrl.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Nuvoton NPCM845 Pin Controller and GPIO + +maintainers: + - Tomer Maimon <[email protected]> + +description: + The Nuvoton BMC NPCM8XX Pin Controller multi-function routed throughDocumentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml line 23
Quoted 10 lines
Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml new file +10
-0+ the multiplexing block, Each pin supports GPIO functionality (GPIOx) + and multiple functions that directly connect the pin to different + hardware blocks. + +properties: + compatible: + const: nuvoton,npcm845-pinctrl + + ranges: + maxItems: 1ranges without reg? Does it even work? Did you test the bindings?
Quoted 4 lines
Quoted 1 line
The ranges related to GPIO node reg
But you do not allow here a 'reg', do you? So how can you have an unit address in pinctrl node?
I allow the reg unit address in the GPIO node.
This is why reg is in the GPIO node as follow:
compatible = "nuvoton,npcm845-pinctrl";
ranges = <0x0 0x0 0xf0010000 0x8000>;
#address-cells = <1>;
#size-cells = <1>;
status = "okay";
gpio0: gpio@f0010000 {
gpio-controller;
#gpio-cells = <2>;
reg = <0x0 0xB0>;
interrupts = <GIC_SPI 116 IRQ_TYPE_LEVEL_HIGH>;
gpio-ranges = <&pinctrl 0 0 32>;
};
gpio1: gpio@f0011000 {
gpio-controller;
#gpio-cells = <2>;
reg = <0x1000 0xB0>;
interrupts = <GIC_SPI 117 IRQ_TYPE_LEVEL_HIGH>;
gpio-ranges = <&pinctrl 0 32 32>;
};
gpio2: gpio@f0012000 {
gpio-controller;
#gpio-cells = <2>;
reg = <0x2000 0xB0>;
interrupts = <GIC_SPI 118 IRQ_TYPE_LEVEL_HIGH>;
gpio-ranges = <&pinctrl 0 64 32>;
};...
Is it problematic?
Quoted 13 lines
Quoted 5 lines
I did test the pin controller document and it passed.
bash-4.2$ make ARCH=arm64dt_binding_checkDT_SCHEMA_FILES=Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yamlQuoted 7 lines
LINT Documentation/devicetree/bindings CHKDT Documentation/devicetree/bindings/processed-schema.json SCHEMA Documentation/devicetree/bindings/processed-schema.json DTEX Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.example.dts DTC Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.example.dtb CHECK Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.example.dtbDid I need to run anything else than
dt_binding_checkfor testing the document?
Quoted 2 lines
Indeed it will pass, because you do not have reg in pinctrl node. But your dts won't pass make dtbs W=1
After running make ARCH=arm64 dtbs W=1 I don't see warning related to pinctrl bash-4.2$ make ARCH=arm64 dtbs W=1
DTC arch/arm64/boot/dts/nuvoton/nuvoton-npcm845-evb.dtb
arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi:69.7-183.5:
Warning (unit_address_vs_reg): /ahb/apb: node has a reg or ranges property, but no unit name arch/arm64/boot/dts/nuvoton/nuvoton-npcm845-evb.dts:20.9-22.4: Warning (unit_address_vs_reg): /memory: node has a reg or ranges property, but no unit name arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi:69.7-183.5:
Warning (simple_bus_reg): /ahb/apb: simple-bus unit address format error, expected "f0000000" arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi:56.35-61.5:
Warning (unique_unit_address): /ahb/reset-controller@f0801000:
duplicate unit-address (also used in node /ahb/clock-controller@f0801000)
I did got warning but it dont related to the pinctrl, Maybe I didn't run the test correct?
Quoted 119 lines
Quoted 48 lines
Quoted 13 lines
Quoted 13 lines
Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml new file +10
-0+ + '#address-cells': + const: 1 + + '#size-cells': + const: 1 + +patternProperties: + "^gpio@": + type: objectQuoted 8 lines
Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml line 39
Quoted 6 lines
Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml new file +6
-0+ + description: + Eight GPIO banks that each contain between 32 GPIOs. + + properties: +No blank line.
O.K.
Quoted 18 lines
Quoted 18 lines
Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml new file +17
-0+ gpio-controller: true + + '#gpio-cells': + const: 2 + + reg: + maxItems: 1 + + interrupts: + maxItems: 1 + + gpio-ranges: + maxItems: 1 + + required: + - gpio-controller + - '#gpio-cells'Quoted 8 lines
Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml line 62
Quoted 6 lines
Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml new file +6
-0+ - reg + - interrupts + - gpio-ranges + + "-pin": + $ref: pinmux-node.yaml#Shouldn't this be under bank?
Quoted 2 lines
Do you mean after the group and function properties? The -pin shouldn't use for the group property naming?
Hm, I guess it's fine, I actually don't remember the recommendation for gpio banks in relation to pinmux nodes.
Quoted 60 lines
Quoted 52 lines
Quoted 52 lines
Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml new file +51
-0+ + properties: + groups: + description: + One or more groups of pins to mux to a certain function + items: + enum: [ iox1, iox2, smb1d, smb2d, lkgpo1, lkgpo2, ioxh, gspi, + smb5b, smb5c, lkgpo0, pspi2, jm1, jm2, smb4den, smb4b, + smb4c, smb15, smb16, smb17, smb18, smb19, smb20, smb21, + smb22, smb23, smb4d, smb14, smb5, smb4, smb3, spi0cs1, + spi0cs2, spi0cs3, smb3c, smb3b, bmcuart0a, uart1, jtag2, + bmcuart1, uart2, bmcuart0b, r1err, r1md, r1oen, r2oen, + rmii3, r3oen, smb3d, fanin0, fanin1, fanin2, fanin3, fanin4, + fanin5, fanin6, fanin7, fanin8, fanin9, fanin10, fanin11, + fanin12, fanin13, fanin14, fanin15, pwm0, pwm1, pwm2, pwm3, + r2, r2err, r2md, r3rxer, ga20kbc, smb5d, lpc, espi, rg1, + rg1mdio, rg2, ddr, i3c0, i3c1, i3c2, i3c3, i3c4, i3c5, + smb0, smb1, smb2, smb2c, smb2b, smb1c, smb1b, smb8, smb9, + smb10, smb11, sd1, sd1pwr, pwm4, pwm5, pwm6, pwm7, pwm8, + pwm9, pwm10, pwm11, mmc8, mmc, mmcwp, mmccd, mmcrst, clkout, + serirq, lpcclk, scipme, sci, smb6, smb7, spi1, faninx, r1, + spi3, spi3cs1, spi3quad, spi3cs2, spi3cs3, nprd_smi, smb0b, + smb0c, smb0den, smb0d, ddc, rg2mdio, wdog1, wdog2, smb12, + smb13, spix, spixcs1, clkreq, hgpio0, hgpio1, hgpio2, hgpio3, + hgpio4, hgpio5, hgpio6, hgpio7 ] + + function: + description: + The function that a group of pins is muxed to + enum: [ iox1, iox2, smb1d, smb2d, lkgpo1, lkgpo2, ioxh, gspi, + smb5b, smb5c, lkgpo0, pspi2, jm1, jm2, smb4den, smb4b, + smb4c, smb15, smb16, smb17, smb18, smb19, smb20, smb21, + smb22, smb23, smb4d, smb14, smb5, smb4, smb3, spi0cs1, + spi0cs2, spi0cs3, smb3c, smb3b, bmcuart0a, uart1, jtag2, + bmcuart1, uart2, bmcuart0b, r1err, r1md, r1oen, r2oen, + rmii3, r3oen, smb3d, fanin0, fanin1, fanin2, fanin3, fanin4, + fanin5, fanin6, fanin7, fanin8, fanin9, fanin10, fanin11, + fanin12, fanin13, fanin14, fanin15, pwm0, pwm1, pwm2, pwm3, + r2, r2err, r2md, r3rxer, ga20kbc, smb5d, lpc, espi, rg1, + rg1mdio, rg2, ddr, i3c0, i3c1, i3c2, i3c3, i3c4, i3c5, + smb0, smb1, smb2, smb2c, smb2b, smb1c, smb1b, smb8, smb9, + smb10, smb11, sd1, sd1pwr, pwm4, pwm5, pwm6, pwm7, pwm8, + pwm9, pwm10, pwm11, mmc8, mmc, mmcwp, mmccd, mmcrst, clkout, + serirq, lpcclk, scipme, sci, smb6, smb7, spi1, faninx, r1, + spi3, spi3cs1, spi3quad, spi3cs2, spi3cs3, nprd_smi, smb0b, + smb0c, smb0den, smb0d, ddc, rg2mdio, wdog1, wdog2, smb12, + smb13, spix, spixcs1, clkreq, hgpio0, hgpio1, hgpio2, hgpio3, + hgpio4, hgpio5, hgpio6, hgpio7 ] + + dependencies: + groups: [ function ]Quoted 8 lines
Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml line 118
Quoted 5 lines
Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml new file +5
-0+ function: [ groups ] + + additionalProperties: false + + "^pin":This is almost the same as previous property. Confusing and I think it does not work.
Quoted 4 lines
if I remove it I get the following error:
pinctrl@f0800000: 'pin34-slew' does not match any of the regexes:
'-pin', '^gpio@', 'pinctrl-[0-9]+'
Can you advise what I should do?
Quoted 6 lines
Ah, the pattern is indeed different - you start with pin. Anyway it's confusing to have cfg starting with pin and mux ending in pin. How "pin-pin" would work? :)
Use maybe similar pattern, so start with mux for mux and pin for cfg. Look at wpcm450 pinctrl.
It indeed confusing, I will work with different naming.
Quoted 58 lines
Quoted 54 lines
Quoted 46 lines
Quoted 46 lines
Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml new file +43
-0+ $ref: pincfg-node.yaml# + + properties: + pins: + description: + A list of pins to configure in certain ways, such as enabling + debouncing + + bias-disable: true + + bias-pull-up: true + + bias-pull-down: true + + input-enable: true + + output-low: true + + output-high: true + + drive-push-pull: true + + drive-open-drain: true + + input-debounce: + description: + Debouncing periods in microseconds, one period per interrupt + bank found in the controller + $ref: /schemas/types.yaml#/definitions/uint32-array + minItems: 1 + maxItems: 4 + + slew-rate: + description: | + 0: Low rate + 1: High rate + $ref: /schemas/types.yaml#/definitions/uint32 + enum: [0, 1] + + drive-strength: + enum: [ 0, 1, 2, 4, 8, 12 ] + + additionalProperties: falseQuoted 8 lines
Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml line 167
Quoted 6 lines
Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml new file +6
-0+ +required: + - compatible + - ranges + - '#address-cells' + - '#size-cells'Missing allOf with ref to
pinctrl.yaml.Quoted 3 lines
Do you mean adding
allOf:
- $ref: "pinctrl.yaml#"
Quoted 6 lines
Yes.
Best regards,
Krzysztof
Best regards,
Tomer
Re: [PATCH v1 1/2] dt-binding: pinctrl: Add NPCM8XX pinctrl and GPIO documentation
Quoted 89 lines · Tomer Maimon
Hi Krzysztof,
Thanks for your clarifications.
Quoted 50 lines · Krzysztof Kozlowski
Quoted 48 lines · Tomer Maimon
Hi Krzysztof,
Thanks for your comments.
Quoted 42 lines · Krzysztof Kozlowski
Quoted 28 lines · Tomer Maimon
Added device tree binding documentation for Nuvoton Arbel BMC NPCM8XX pinmux and GPIO controller.
Signed-off-by Tomer Maimon <[email protected]>--- .../pinctrl/nuvoton,npcm845-pinctrl.yaml | 205 ++++++++++++++++++ 1 file changed, 205 insertions(+) create mode 100644 Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yamlDocumentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml new file +13
-0@@ -0,0 +1,205 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/pinctrl/nuvoton,npcm845-pinctrl.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Nuvoton NPCM845 Pin Controller and GPIO + +maintainers: + - Tomer Maimon <[email protected]> + +description: + The Nuvoton BMC NPCM8XX Pin Controller multi-function routed throughDocumentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml line 23
Quoted 10 lines
Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml new file +10
-0+ the multiplexing block, Each pin supports GPIO functionality (GPIOx) + and multiple functions that directly connect the pin to different + hardware blocks. + +properties: + compatible: + const: nuvoton,npcm845-pinctrl + + ranges: + maxItems: 1ranges without reg? Does it even work? Did you test the bindings?
Quoted 4 lines
Quoted 1 line
The ranges related to GPIO node reg
But you do not allow here a 'reg', do you? So how can you have an unit address in pinctrl node?
I allow the reg unit address in the GPIO node.
This is why reg is in the GPIO node as follow:compatible = "nuvoton,npcm845-pinctrl"; ranges = <0x0 0x0 0xf0010000 0x8000>; #address-cells = <1>; #size-cells = <1>; status = "okay"; gpio0: gpio@f0010000 { gpio-controller; #gpio-cells = <2>; reg = <0x0 0xB0>; interrupts = <GIC_SPI 116 IRQ_TYPE_LEVEL_HIGH>; gpio-ranges = <&pinctrl 0 0 32>; }; gpio1: gpio@f0011000 { gpio-controller; #gpio-cells = <2>; reg = <0x1000 0xB0>; interrupts = <GIC_SPI 117 IRQ_TYPE_LEVEL_HIGH>; gpio-ranges = <&pinctrl 0 32 32>; }; gpio2: gpio@f0012000 { gpio-controller; #gpio-cells = <2>; reg = <0x2000 0xB0>; interrupts = <GIC_SPI 118 IRQ_TYPE_LEVEL_HIGH>; gpio-ranges = <&pinctrl 0 64 32>; };
Quoted 2 lines
...
Is it problematic?
It seems not, looks ok because of ranges, although it is a bit confusing that your pinctrl unit address is 0xf0800000 but ranges is 0xf0010000.
Quoted 25 lines
Quoted 13 lines
Quoted 5 lines
I did test the pin controller document and it passed.
bash-4.2$ make ARCH=arm64dt_binding_checkDT_SCHEMA_FILES=Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yamlQuoted 7 lines
LINT Documentation/devicetree/bindings CHKDT Documentation/devicetree/bindings/processed-schema.json SCHEMA Documentation/devicetree/bindings/processed-schema.json DTEX Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.example.dts DTC Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.example.dtb CHECK Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.example.dtbDid I need to run anything else than
dt_binding_checkfor testing the document?Quoted 2 lines
Indeed it will pass, because you do not have reg in pinctrl node. But your dts won't pass make dtbs W=1
After running make ARCH=arm64 dtbs W=1 I don't see warning related to pinctrl bash-4.2$ make ARCH=arm64 dtbs W=1
DTCarch/arm64/boot/dts/nuvoton/nuvoton-npcm845-evb.dtb
arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi:69.7-183.5:
Warning (unit_address_vs_reg): /ahb/apb: node has a reg or ranges property, but no unit name arch/arm64/boot/dts/nuvoton/nuvoton-npcm845-evb.dts:20.9-22.4: Warning (unit_address_vs_reg): /memory: node has a reg or ranges property, but no unit name arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi:69.7-183.5:
Quoted 8 lines
Warning (
simple_bus_reg): /ahb/apb: simple-bus unit address format error, expected "f0000000" arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi:56.35-61.5:
Warning (unique_unit_address): /ahb/reset-controller@f0801000:
duplicate unit-address (also used in node /ahb/clock-controller@f0801000)
I did got warning but it dont related to the pinctrl, Maybe I didn't run the test correct?
Looks correct, indeed.
Best regards,
Krzysztof
Tomer Maimon
maintainerRe: [PATCH v1 1/2] dt-binding: pinctrl: Add NPCM8XX pinctrl and GPIO documentation
Hi Krzysztof,
Thanks for your clarifications.
Quoted 95 lines · Krzysztof Kozlowski
Quoted 89 lines · Tomer Maimon
Hi Krzysztof,
Thanks for your clarifications.
Quoted 50 lines · Krzysztof Kozlowski
Quoted 48 lines · Tomer Maimon
Hi Krzysztof,
Thanks for your comments.
Quoted 42 lines · Krzysztof Kozlowski
Quoted 28 lines · Tomer Maimon
Added device tree binding documentation for Nuvoton Arbel BMC NPCM8XX pinmux and GPIO controller.
Signed-off-by Tomer Maimon <[email protected]>--- .../pinctrl/nuvoton,npcm845-pinctrl.yaml | 205 ++++++++++++++++++ 1 file changed, 205 insertions(+) create mode 100644 Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yamlDocumentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml new file +13
-0@@ -0,0 +1,205 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/pinctrl/nuvoton,npcm845-pinctrl.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Nuvoton NPCM845 Pin Controller and GPIO + +maintainers: + - Tomer Maimon <[email protected]> + +description: + The Nuvoton BMC NPCM8XX Pin Controller multi-function routed throughDocumentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml line 23
Quoted 10 lines
Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yaml new file +10
-0+ the multiplexing block, Each pin supports GPIO functionality (GPIOx) + and multiple functions that directly connect the pin to different + hardware blocks. + +properties: + compatible: + const: nuvoton,npcm845-pinctrl + + ranges: + maxItems: 1ranges without reg? Does it even work? Did you test the bindings?
Quoted 4 lines
Quoted 1 line
The ranges related to GPIO node reg
But you do not allow here a 'reg', do you? So how can you have an unit address in pinctrl node?
I allow the reg unit address in the GPIO node.
This is why reg is in the GPIO node as follow:compatible = "nuvoton,npcm845-pinctrl"; ranges = <0x0 0x0 0xf0010000 0x8000>; #address-cells = <1>; #size-cells = <1>; status = "okay"; gpio0: gpio@f0010000 { gpio-controller; #gpio-cells = <2>; reg = <0x0 0xB0>; interrupts = <GIC_SPI 116 IRQ_TYPE_LEVEL_HIGH>; gpio-ranges = <&pinctrl 0 0 32>; }; gpio1: gpio@f0011000 { gpio-controller; #gpio-cells = <2>; reg = <0x1000 0xB0>; interrupts = <GIC_SPI 117 IRQ_TYPE_LEVEL_HIGH>; gpio-ranges = <&pinctrl 0 32 32>; }; gpio2: gpio@f0012000 { gpio-controller; #gpio-cells = <2>; reg = <0x2000 0xB0>; interrupts = <GIC_SPI 118 IRQ_TYPE_LEVEL_HIGH>; gpio-ranges = <&pinctrl 0 64 32>; };Quoted 2 lines
...
Is it problematic?
Quoted 2 lines
It seems not, looks ok because of ranges, although it is a bit confusing that your pinctrl unit address is
0xf0800000but ranges is0xf0010000.
The reason the pinctrl address 0xf0800000 because the pin mux is handled by the GCR registers and the ranges related to the GPIO.
Quoted 35 lines
Quoted 26 lines
Quoted 14 lines
Quoted 6 lines
I did test the pin controller document and it passed.
bash-4.2$ make ARCH=arm64dt_binding_checkDT_SCHEMA_FILES=Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.yamlQuoted 7 lines
LINT Documentation/devicetree/bindings CHKDT Documentation/devicetree/bindings/processed-schema.json SCHEMA Documentation/devicetree/bindings/processed-schema.json DTEX Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.example.dts DTC Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.example.dtb CHECK Documentation/devicetree/bindings/pinctrl/nuvoton,npcm845-pinctrl.example.dtbDid I need to run anything else than
dt_binding_checkfor testing the document?Quoted 2 lines
Indeed it will pass, because you do not have reg in pinctrl node. But your dts won't pass make dtbs W=1
After running make ARCH=arm64 dtbs W=1 I don't see warning related to pinctrl bash-4.2$ make ARCH=arm64 dtbs W=1
DTCarch/arm64/boot/dts/nuvoton/nuvoton-npcm845-evb.dtb
arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi:69.7-183.5:
Warning (unit_address_vs_reg): /ahb/apb: node has a reg or ranges property, but no unit name arch/arm64/boot/dts/nuvoton/nuvoton-npcm845-evb.dts:20.9-22.4: Warning (unit_address_vs_reg): /memory: node has a reg or ranges property, but no unit name arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi:69.7-183.5:Quoted 8 lines
Warning (
simple_bus_reg): /ahb/apb: simple-bus unit address format error, expected "f0000000" arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi:56.35-61.5:
Warning (unique_unit_address): /ahb/reset-controller@f0801000:
duplicate unit-address (also used in node /ahb/clock-controller@f0801000)
I did got warning but it dont related to the pinctrl, Maybe I didn't run the test correct?
Quoted 4 lines
Looks correct, indeed.
Best regards,
Krzysztof
Best regards,
Tomer
Tomer Maimon
maintainerPATCH 2/2 pinctrl: nuvoton: add NPCM8XX pinctrl and GPIO driver
In reply to Tomer MaimonAdd pinctrl and GPIO controller driver support to Arbel BMC NPCM8XX SoC.
Arbel BMC NPCM8XX pinctrl driver based on Poleg NPCM7XX, except the pin mux mapping difference the NPCM8XX GPIO supports adjust debounce period time.
--- drivers/pinctrl/nuvoton/Kconfig | 13 + drivers/pinctrl/nuvoton/Makefile | 1 + drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c | 2634 +++++++++++++++++++++ 3 files changed, 2648 insertions(+) create mode 100644 drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c
drivers/pinctrl/nuvoton/Kconfig +13 -0
@@ -31,3 +31,16 @@ config PINCTRL_NPCM7XX help Say Y here to enable pin controller and GPIO support for Nuvoton NPCM750/730/715/705 SoCs. + +config PINCTRL_NPCM8XX + bool "Pinctrl and GPIO driver for Nuvoton NPCM8XX" + depends on (ARCH_NPCM || COMPILE_TEST) && OF + select PINMUX + select PINCONF + select GENERIC_PINCONF + select GPIOLIB + select GPIO_GENERIC + select GPIOLIB_IRQCHIP + help + Say Y here to enable pin controller and GPIO support + for Nuvoton NPCM8XX SoC.
drivers/pinctrl/nuvoton/Makefile +1 -0
@@ -3,3 +3,4 @@ obj-$(CONFIG_PINCTRL_WPCM450) += pinctrl-wpcm450.o obj-$(CONFIG_PINCTRL_NPCM7XX) += pinctrl-npcm7xx.o +obj-$(CONFIG_PINCTRL_NPCM8XX) += pinctrl-npcm8xx.o
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +2634 -0
2,634 lines not on this page · raw message
Signature · 1 line
2.33.0
Re: [PATCH v1 2/2] pinctrl: nuvoton: add NPCM8XX pinctrl and GPIO driver
Quoted 6 lines · Tomer Maimon
Add pinctrl and GPIO controller driver support to Arbel BMC NPCM8XX SoC.
Arbel BMC NPCM8XX pinctrl driver based on Poleg NPCM7XX, except the pin mux mapping difference the NPCM8XX GPIO supports adjust debounce period time.
Below my comments from a very brief review.
...
Looking at the length of it, and noticing some debug prints I believe you may reduce the LoC amount by ~5-10% easily, starting from removing debug prints (like in IRQ set type callback). Also some lines look split by two where it's fine to have at one (with container_of() in them, for example). Besides that read the latest examples on how to provide constant IRQ chip description and what the proper IRQ data accessors should be used.
Additionally pay attention to the headers the driver uses, for example it misses bits.h (or bitops.h depending on the code, I haven't looked deeply).
...
Quoted 1 line
+#define
DRIVE_STRENGTH_MASK0x0000FF00
Why not GENMASK()?
...
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c line 1398
Quoted 2 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +2
-0+#define DSLO (x) (((x) >> DRIVE_STRENGTH_LO_SHIFT) & 0xF) +#define DSHI (x) (((x) >> DRIVE_STRENGTH_HI_SHIFT) & 0xF)
Ditto.
...
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c line 1403
Quoted 4 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +4
-0+#define GPI 0x1/* Not GPO */ +#define GPO 0x2/* Not GPI */ +#define SLEW 0x4/* Has Slew Control, NPCM8XX_GP_N_OSRC */ +#define SLEWLPC 0x8/* Has Slew Control, SRCNT.3 */
Why not BIT(x) for them?
...
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c line 1917
Quoted 4 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +4
-0+regmap_update_bits (gcr_regmap, cfg->reg0, +BIT (cfg->bit0), + !!(cfg->fn0 == mode) ? +BIT (cfg->bit0) :0 );
What the purpose of '!!(x)' in all similar lines? Why wouldn't 'x' simply work?
...
Quoted 6 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +6
-0+if (pincfg[pin].flag &SLEWLPC ) { +switch (arg) { +case 0 : +regmap_update_bits (gcr_regmap,NPCM8XX_GCR_SRCNT , +SRCNT_ESPI ,0 ); +return 0 ;
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c line 1998
Quoted 10 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +10
-0+case 1 : +regmap_update_bits (gcr_regmap,NPCM8XX_GCR_SRCNT , +SRCNT_ESPI ,SRCNT_ESPI ); +return 0 ; +default : +return -EINVAL ; + } + } + +return -EINVAL ;
Why not to use usual pattern, i.e.
if (error_condition)
return -EINVAL;
here and everywhere in the similar cases?
...
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c line 2017
Quoted 2 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +2
-0+ val =ioread32 (bank->base +NPCM8XX_GP_N_ODSC ) + & pinmask;
What was happened to indentation? Check entire file for indentation to be okay.
...
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c line 2034
Quoted 4 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +4
-0+int v; +struct npcm8xx_gpio *bank = + &npcm->gpio_bank[pin /NPCM8XX_GPIO_PER_BANK ]; +int gpio =BIT (pin % bank->gc.ngpio);
Keep it ordered in reversed xmas tree manner.
...
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c line 2044
Quoted 6 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +6
-0+if (DSLO (v) == nval) { +dev_dbg (bank->gc.parent, +"setting pin %d to low strength [%d]\n" , pin, nval); +npcm_gpio_clr (&bank->gc, bank->base +NPCM8XX_GP_N_ODSC , gpio); +return 0 ; + }else if (DSHI (v) == nval) {
Redundant 'else'. Check entire file for the similar pattern and drop redundancy.
Quoted 5 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +5
-0+dev_dbg (bank->gc.parent, +"setting pin %d to high strength [%d]\n" , pin, nval); +npcm_gpio_set (&bank->gc, bank->base +NPCM8XX_GP_N_ODSC , gpio); +return 0 ; + }
...
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c line 2059
Quoted 5 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +5
-0+static void npcm8xx_pin_dbg_show (struct pinctrl_dev *pctldev, +struct seq_file *s,unsigned int offset) +{ +seq_printf (s,"pinctrl_ops.dbg: %d" , offset); +}
Not sure it brings any additional information to what pin control core issues on debug.
...
Quoted 1 line
+
dev_dbg(npcm->dev, "group size: %d\n", (int)ARRAY_SIZE(npcm8xx_groups));
Drop this noise. It's production. Or not? If not, why bother with submitting not yet ready material?
...
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c line 2251
Quoted 12 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +12
-0+ }else if ((nanosecs >1640 ) && (nanosecs <=2280 )) { +iowrite32 (0x20 , bank->base +NPCM8XX_GP_N_DBNCP0 + (i *4 )); + }else if ((nanosecs >2280 ) && (nanosecs <=2700 )) { +iowrite32 (0x30 , bank->base +NPCM8XX_GP_N_DBNCP0 + (i *4 )); + }else if ((nanosecs >2700 ) && (nanosecs <=2856 )) { +iowrite32 (0x40 , bank->base +NPCM8XX_GP_N_DBNCP0 + (i *4 )); + }else if ((nanosecs >2856 ) && (nanosecs <=3496 )) { +iowrite32 (0x50 , bank->base +NPCM8XX_GP_N_DBNCP0 + (i *4 )); + }else if ((nanosecs >3496 ) && (nanosecs <=4136 )) { +iowrite32 (0x60 , bank->base +NPCM8XX_GP_N_DBNCP0 + (i *4 )); + }else if ((nanosecs >4136 ) && (nanosecs <=5025 )) { +iowrite32 (0x70 , bank->base +NPCM8XX_GP_N_DBNCP0 + (i *4 ));
With switch-case with ranges it will be much more visible what's going on. Also think about it, maybe you can use some formula instead? Or table (array of integers) approach where index will show the lowest range value.
...
Quoted 1 line
+ struct
device_node*np =to_of_node(child);
Why?!
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c line 2464
Quoted 7 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +7
-0+ ret =of_address_to_resource (np,0 , &res); +if (ret <0 ) { +dev_err (dev,"Resource fail for GPIO bank %u\n" , id); +return ret; + } + + pctrl->gpio_bank[id].base =ioremap (res.start,resource_size (&res));
fwnode_iomap()
...
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c line 2476
Quoted 4 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +4
-0+if (ret) { +dev_err (dev,"bgpio_init() failed\n" ); +return ret; + }
Use
return dev_err_probe(...);
In ->probe() and satellite functions.
...
Quoted 1 line
+ ret =
irq_of_parse_and_map(np, 0);
fwnode_irq_get()
Quoted 4 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +4
-0+if (!ret) { +dev_err (dev,"No IRQ for GPIO bank %u\n" , id); +return -EINVAL ; + }
...
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c line 2530
Quoted 2 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +2
-0+ girq->num_parents =1 ; + girq->parents = devm_kcalloc(pctrl->dev, 1,
Replace 1 with girq->num_parents
Quoted 6 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +6
-0+sizeof (*girq->parents), + GFP_KERNEL); +if (!girq->parents) { + ret = -ENOMEM ; + goto err_register; + }
...
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c line 2558
Quoted 11 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +11
-0+ ret =gpiochip_add_pin_range (&pctrl->gpio_bank[id].gc, +dev_name (pctrl->dev), + pctrl->gpio_bank[id].pinctrl_id, + pctrl->gpio_bank[id].gc.base, + pctrl->gpio_bank[id].gc.ngpio); +if (ret <0 ) { +dev_err (pctrl->dev,"Failed to add GPIO bank %u\n" , id); +gpiochip_remove (&pctrl->gpio_bank[id].gc); + goto err_register; + } + }
We have a callback for ranges, can you use it?
...
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c line 2563
Quoted 2 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +2
-0+err_register: +for (; id >0 ; id--)
while(id--)
Quoted 1 line
+
gpiochip_remove(&pctrl->gpio_bank[id - 1].gc);
But why not devm_gpiochip_add_... in the first place?
...
Quoted 1 line
+
dev_set_drvdata(&pdev->dev, pctrl);
platform_set_drv_data() ?
...
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c line 2613
Quoted 3 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +3
-0+static const struct of_device_id npcm8xx_pinctrl_match[] = { + { .compatible ="nuvoton,npcm845-pinctrl" }, + { },
No comma for terminator entry.
Quoted 1 line
+};
Signature · 2 lines
With Best Regards, Andy Shevchenko
Tomer Maimon
maintainerRe: [PATCH v1 2/2] pinctrl: nuvoton: add NPCM8XX pinctrl and GPIO driver
Hi Andy,
Thanks for your comments
Quoted 13 lines · Andy Shevchenko
Quoted 6 lines · Tomer Maimon
Add pinctrl and GPIO controller driver support to Arbel BMC NPCM8XX SoC.
Arbel BMC NPCM8XX pinctrl driver based on Poleg NPCM7XX, except the pin mux mapping difference the NPCM8XX GPIO supports adjust debounce period time.
Below my comments from a very brief review.
...
Quoted 10 lines
Looking at the length of it, and noticing some debug prints I believe you may reduce the LoC amount by ~5-10% easily, starting from removing debug prints (like in IRQ set type callback). Also some lines look split by two where it's fine to have at one (with
container_of()in them, for example). Besides that read the latest examples on how to provide constant IRQ chip description and what the proper IRQ data accessors should be used.
Additionally pay attention to the headers the driver uses, for example it missesbits.h(orbitops.hdepending on the code, I haven't looked deeply).
I will try to reduce lines as possible
Quoted 23 lines
...
Quoted 1 line
+#define
DRIVE_STRENGTH_MASK0x0000FF00Why not
GENMASK()?...
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c line 1398
Quoted 2 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +2
-0+#define DSLO (x) (((x) >> DRIVE_STRENGTH_LO_SHIFT) & 0xF) +#define DSHI (x) (((x) >> DRIVE_STRENGTH_HI_SHIFT) & 0xF)Ditto.
...
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c line 1403
Quoted 4 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +4
-0+#define GPI 0x1/* Not GPO */ +#define GPO 0x2/* Not GPI */ +#define SLEW 0x4/* Has Slew Control, NPCM8XX_GP_N_OSRC */ +#define SLEWLPC 0x8/* Has Slew Control, SRCNT.3 */ Why not BIT(x) for them?
Quoted 8 lines
...
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c line 1917
Quoted 4 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +4
-0+regmap_update_bits (gcr_regmap, cfg->reg0, +BIT (cfg->bit0), + !!(cfg->fn0 == mode) ? +BIT (cfg->bit0) :0 );What the purpose of '!!(x)' in all similar lines? Why wouldn't 'x' simply work?
taken by mistake, 'x' will work.
Quoted 20 lines
...
Quoted 6 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +6
-0+if (pincfg[pin].flag &SLEWLPC ) { +switch (arg) { +case 0 : +regmap_update_bits (gcr_regmap,NPCM8XX_GCR_SRCNT , +SRCNT_ESPI ,0 ); +return 0 ;drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c line 1998
Quoted 10 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +10
-0+case 1 : +regmap_update_bits (gcr_regmap,NPCM8XX_GCR_SRCNT , +SRCNT_ESPI ,SRCNT_ESPI ); +return 0 ; +default : +return -EINVAL ; + } + } + +return -EINVAL ;
Quoted 4 lines
Why not to use usual pattern, i.e.
if (
error_condition)return -EINVAL;
What do you mean? like if (arg>1) return -EINVAL? It just seems more readable.
Quoted 2 lines
here and everywhere in the similar cases?
can you point me to which more cases you mean?
Quoted 7 lines
...
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c line 2017
Quoted 2 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +2
-0+ val =ioread32 (bank->base +NPCM8XX_GP_N_ODSC ) + & pinmask;What was happened to indentation? Check entire file for indentation to be okay.
Sorry, I didn't understand, could you explain the comment again?
Quoted 9 lines
...
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c line 2034
Quoted 4 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +4
-0+int v; +struct npcm8xx_gpio *bank = + &npcm->gpio_bank[pin /NPCM8XX_GPIO_PER_BANK ]; +int gpio =BIT (pin % bank->gc.ngpio);Keep it ordered in reversed xmas tree manner.
O.K.
Quoted 11 lines
...
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c line 2044
Quoted 6 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +6
-0+if (DSLO (v) == nval) { +dev_dbg (bank->gc.parent, +"setting pin %d to low strength [%d]\n" , pin, nval); +npcm_gpio_clr (&bank->gc, bank->base +NPCM8XX_GP_N_ODSC , gpio); +return 0 ; + }else if (DSHI (v) == nval) {Redundant 'else'. Check entire file for the similar pattern and drop redundancy.
O.K.
Quoted 40 lines
Quoted 6 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +5
-0+dev_dbg (bank->gc.parent, +"setting pin %d to high strength [%d]\n" , pin, nval); +npcm_gpio_set (&bank->gc, bank->base +NPCM8XX_GP_N_ODSC , gpio); +return 0 ; + }...
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c line 2059
Quoted 5 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +5
-0+static void npcm8xx_pin_dbg_show (struct pinctrl_dev *pctldev, +struct seq_file *s,unsigned int offset) +{ +seq_printf (s,"pinctrl_ops.dbg: %d" , offset); +}Not sure it brings any additional information to what pin control core issues on debug.
...
Quoted 1 line
+
dev_dbg(npcm->dev, "group size: %d\n", (int)ARRAY_SIZE(npcm8xx_groups));Drop this noise. It's production. Or not? If not, why bother with submitting not yet ready material?
...
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c line 2251
Quoted 12 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +12
-0+ }else if ((nanosecs >1640 ) && (nanosecs <=2280 )) { +iowrite32 (0x20 , bank->base +NPCM8XX_GP_N_DBNCP0 + (i *4 )); + }else if ((nanosecs >2280 ) && (nanosecs <=2700 )) { +iowrite32 (0x30 , bank->base +NPCM8XX_GP_N_DBNCP0 + (i *4 )); + }else if ((nanosecs >2700 ) && (nanosecs <=2856 )) { +iowrite32 (0x40 , bank->base +NPCM8XX_GP_N_DBNCP0 + (i *4 )); + }else if ((nanosecs >2856 ) && (nanosecs <=3496 )) { +iowrite32 (0x50 , bank->base +NPCM8XX_GP_N_DBNCP0 + (i *4 )); + }else if ((nanosecs >3496 ) && (nanosecs <=4136 )) { +iowrite32 (0x60 , bank->base +NPCM8XX_GP_N_DBNCP0 + (i *4 )); + }else if ((nanosecs >4136 ) && (nanosecs <=5025 )) { +iowrite32 (0x70 , bank->base +NPCM8XX_GP_N_DBNCP0 + (i *4 ));
Quoted 4 lines
With switch-case with ranges it will be much more visible what's going on. Also think about it, maybe you can use some formula instead? Or table (array of integers) approach where index will show the lowest range value.
There it can be described in a formula. Will be done with switch-case
Quoted 6 lines
...
Quoted 1 line
+ struct
device_node*np =to_of_node(child);Why?!
Will remove and replaced with fwnode functions.
Quoted 13 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c line 2464
Quoted 8 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +7
-0+ ret =of_address_to_resource (np,0 , &res); +if (ret <0 ) { +dev_err (dev,"Resource fail for GPIO bank %u\n" , id); +return ret; + } + + pctrl->gpio_bank[id].base =ioremap (res.start,resource_size (&res));
fwnode_iomap()...
Quoted 8 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c line 2476
Quoted 4 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +4
-0+if (ret) { +dev_err (dev,"bgpio_init() failed\n" ); +return ret; + }Use
return
dev_err_probe(...)
Why it is better to use dev_err_probe?
I am not sure that the error will be EPROBE_DEFER, all the failure cases the driver returned the
error in the code.
Quoted 42 lines
In ->probe() and satellite functions.
...
Quoted 1 line
+ ret =
irq_of_parse_and_map(np, 0);
fwnode_irq_get()Quoted 4 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +4
-0+if (!ret) { +dev_err (dev,"No IRQ for GPIO bank %u\n" , id); +return -EINVAL ; + }...
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c line 2530
Quoted 2 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +2
-0+ girq->num_parents =1 ; + girq->parents = devm_kcalloc(pctrl->dev, 1,Replace 1 with
girq->num_parentsQuoted 6 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +6
-0+sizeof (*girq->parents), + GFP_KERNEL); +if (!girq->parents) { + ret = -ENOMEM ; + goto err_register; + }...
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c line 2558
Quoted 11 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +11
-0+ ret =gpiochip_add_pin_range (&pctrl->gpio_bank[id].gc, +dev_name (pctrl->dev), + pctrl->gpio_bank[id].pinctrl_id, + pctrl->gpio_bank[id].gc.base, + pctrl->gpio_bank[id].gc.ngpio); +if (ret <0 ) { +dev_err (pctrl->dev,"Failed to add GPIO bank %u\n" , id); +gpiochip_remove (&pctrl->gpio_bank[id].gc); + goto err_register; + } + }
Quoted 1 line
We have a callback for ranges, can you use it?
Yes,
Quoted 11 lines
...
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c line 2563
Quoted 2 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +2
-0+err_register: +for (; id >0 ; id--)while(id--)
Quoted 1 line
+
gpiochip_remove(&pctrl->gpio_bank[id - 1].gc);But why not
devm_gpiochip_add_... in the first place?
using, devm_gpiochip_add_data, will remove gpiochip_remove.
Quoted 6 lines
...
Quoted 1 line
+
dev_set_drvdata(&pdev->dev, pctrl);
platform_set_drv_data()?
O.K.
Quoted 8 lines
...
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c line 2613
Quoted 3 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +3
-0+static const struct of_device_id npcm8xx_pinctrl_match[] = { + { .compatible ="nuvoton,npcm845-pinctrl" }, + { },No comma for terminator entry.
O.K.
Quoted 6 lines
Quoted 2 lines
+};
--
With Best Regards,
Andy Shevchenko
Best regards,
Tomer
Re: [PATCH v1 2/2] pinctrl: nuvoton: add NPCM8XX pinctrl and GPIO driver
Quoted 2 lines · Tomer Maimon
Quoted 1 line · Andy Shevchenko
On Sun, Jul 10, 2022 at 12:44 PM Tomer Maimon <[email protected]> wrote:
Please, remove unneeded context when replying!
...
Quoted 17 lines
Quoted 17 lines
Quoted 16 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +16
-0+if (pincfg[pin].flag &SLEWLPC ) { +switch (arg) { +case 0 : +regmap_update_bits (gcr_regmap,NPCM8XX_GCR_SRCNT , +SRCNT_ESPI ,0 ); +return 0 ; +case 1 : +regmap_update_bits (gcr_regmap,NPCM8XX_GCR_SRCNT , +SRCNT_ESPI ,SRCNT_ESPI ); +return 0 ; +default : +return -EINVAL ; + } + } + +return -EINVAL ;
Quoted 5 lines
Quoted 4 lines
Why not to use usual pattern, i.e.
if (
error_condition)return -EINVAL;What do you mean? like if (arg>1) return
-EINVAL? It just seems more readable.
if (!(pincfg[pin].flag & SLEWLPC))
return -EINVAL;
switch(...) {
...
}Quoted 2 lines
Quoted 1 line
here and everywhere in the similar cases?
can you point me to which more cases you mean?
Any you find that follows this pattern. Actually the rule of thumb is to address all places in the code even if reviewer has given a comment against one occurrence of something.
...
Quoted 5 lines
Quoted 4 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c line 2017
Quoted 2 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +2
-0+ val =ioread32 (bank->base +NPCM8XX_GP_N_ODSC ) + & pinmask;What was happened to indentation? Check entire file for indentation to be okay.
Sorry, I didn't understand, could you explain the comment again?
Indentation is a code formatting technique that allows the text to be more readable. In particular when lines are split they should logically follow what code does and point to the code relation. Here you have '&' on the next line with indentation starting at the 'val's column. This is not readable and confusing. In this case formatting on one line fixes all issues. Possible alternative is to clearly show how the 'val' is being modified:
val = ioread32(...); val &= mask;
But see above about the amount of LoCs.
...
Quoted 10 lines
Quoted 9 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c line 2251
Quoted 4 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +4
-0+ }else if ((nanosecs >3496 ) && (nanosecs <=4136 )) { +iowrite32 (0x60 , bank->base +NPCM8XX_GP_N_DBNCP0 + (i *4 )); + }else if ((nanosecs >4136 ) && (nanosecs <=5025 )) { +iowrite32 (0x70 , bank->base +NPCM8XX_GP_N_DBNCP0 + (i *4 ));With switch-case with ranges it will be much more visible what's going on. Also think about it, maybe you can use some formula instead? Or table (array of integers) approach where index will show the lowest range value.
There it can be described in a formula. Will be done with switch-case
I'm not sure I follow. If you can use a formula, use it!
...
Quoted 9 lines
Quoted 8 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c line 2476
Quoted 4 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +4
-0+if (ret) { +dev_err (dev,"bgpio_init() failed\n" ); +return ret; + }Use
return
dev_err_probe(...)Why it is better to use
dev_err_probe?
(beside deferred probe, which may be not the case here)
- standardized format
- less LoCs
Quoted 3 lines
I am not sure that the error will be
EPROBE_DEFER, all the failure cases the driver returned the
error in the code.
...and it's fine to use dev_err_probe() as stated in its documentation.
Quoted 1 line
Quoted 1 line
In ->probe() and satellite functions.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Tomer Maimon
maintainerRe: [PATCH v1 2/2] pinctrl: nuvoton: add NPCM8XX pinctrl and GPIO driver
Hi Andy,
Thanks for your clarifications, they will be addressed in the next version.
Best regards,
Tomer
Re: [PATCH v1 2/2] pinctrl: nuvoton: add NPCM8XX pinctrl and GPIO driver
In reply to Tomer MaimonQuoted 7 lines · Tomer Maimon
Add pinctrl and GPIO controller driver support to Arbel BMC NPCM8XX SoC.
Arbel BMC NPCM8XX pinctrl driver based on Poleg NPCM7XX, except the pin mux mapping difference the NPCM8XX GPIO supports adjust debounce period time.
Signed-off-by Tomer Maimon <[email protected]>
(...)
Quoted 5 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +5
-0+ +static int npcm8xx_pinctrl_probe (struct platform_device *pdev) +{ +struct npcm8xx_pinctrl *pctrl; +int ret;
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c line 2582
Quoted 10 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +10
-0+ + pctrl =devm_kzalloc (&pdev->dev,sizeof (*pctrl),GFP_KERNEL ); +if (!pctrl) +return -ENOMEM ; + + pctrl->dev = &pdev->dev; +dev_set_drvdata (&pdev->dev, pctrl); + + pctrl->gcr_regmap = +syscon_regmap_lookup_by_compatible ("nuvoton,npcm845-gcr" );
No. Use property. By this patchset, I would expect that you learnt from previous mistakes around this. Why repeating the same trouble second time?
Quoted 15 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +15
-0+if (IS_ERR (pctrl->gcr_regmap)) { +dev_err (pctrl->dev,"didn't find nuvoton,npcm845-gcr\n" ); +return PTR_ERR (pctrl->gcr_regmap); + } + + ret =npcm8xx_gpio_of (pctrl); +if (ret <0 ) { +dev_err (pctrl->dev,"Failed to gpio dt-binding %u\n" , ret); +return ret; + } + + pctrl->pctldev =devm_pinctrl_register (&pdev->dev, + &npcm8xx_pinctrl_desc, pctrl); +if (IS_ERR (pctrl->pctldev)) { +dev_err (&pdev->dev,"Failed to register pinctrl device\n" );
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c line 2607
Quoted 10 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +10
-0+return PTR_ERR (pctrl->pctldev); + } + + ret =npcm8xx_gpio_register (pctrl); +if (ret <0 ) { +dev_err (pctrl->dev,"Failed to register gpio %u\n" , ret); +return ret; + } + +pr_info ("npcm8xx Pinctrl driver probed\n" );
No pr_ in devices. No success debug messages.
Quoted 17 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +17
-0+return 0 ; +} + +static const struct of_device_id npcm8xx_pinctrl_match[] = { + { .compatible ="nuvoton,npcm845-pinctrl" }, + { }, +}; +MODULE_DEVICE_TABLE (of, npcm8xx_pinctrl_match); + +static struct platform_driver npcm8xx_pinctrl_driver = { + .probe = npcm8xx_pinctrl_probe, + .driver = { + .name ="npcm8xx-pinctrl" , + .of_match_table = npcm8xx_pinctrl_match, + .suppress_bind_attrs = true, + }, +};
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c line 2634
Quoted 10 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +10
-0+ +static int __init npcm8xx_pinctrl_register (void ) +{ +return platform_driver_register (&npcm8xx_pinctrl_driver); +} +arch_initcall (npcm8xx_pinctrl_register); + +MODULE_LICENSE ("GPL v2" ); +MODULE_AUTHOR ("[email protected]" ); +MODULE_DESCRIPTION ("Nuvoton NPCM8XX Pinctrl and GPIO driver" );
Best regards,
Krzysztof
Tomer Maimon
maintainerRe: [PATCH v1 2/2] pinctrl: nuvoton: add NPCM8XX pinctrl and GPIO driver
HI Krzysztof,
Thanks for your comment, they will be addressed next version,
Quoted 83 lines · Krzysztof Kozlowski
Quoted 7 lines · Tomer Maimon
Add pinctrl and GPIO controller driver support to Arbel BMC NPCM8XX SoC.
Arbel BMC NPCM8XX pinctrl driver based on Poleg NPCM7XX, except the pin mux mapping difference the NPCM8XX GPIO supports adjust debounce period time.
Signed-off-by Tomer Maimon <[email protected]>(...)
Quoted 5 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +5
-0+ +static int npcm8xx_pinctrl_probe (struct platform_device *pdev) +{ +struct npcm8xx_pinctrl *pctrl; +int ret;drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c line 2582
Quoted 10 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +10
-0+ + pctrl =devm_kzalloc (&pdev->dev,sizeof (*pctrl),GFP_KERNEL ); +if (!pctrl) +return -ENOMEM ; + + pctrl->dev = &pdev->dev; +dev_set_drvdata (&pdev->dev, pctrl); + + pctrl->gcr_regmap = +syscon_regmap_lookup_by_compatible ("nuvoton,npcm845-gcr" );No. Use property. By this patchset, I would expect that you learnt from previous mistakes around this. Why repeating the same trouble second time?
Quoted 15 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +15
-0+if (IS_ERR (pctrl->gcr_regmap)) { +dev_err (pctrl->dev,"didn't find nuvoton,npcm845-gcr\n" ); +return PTR_ERR (pctrl->gcr_regmap); + } + + ret =npcm8xx_gpio_of (pctrl); +if (ret <0 ) { +dev_err (pctrl->dev,"Failed to gpio dt-binding %u\n" , ret); +return ret; + } + + pctrl->pctldev =devm_pinctrl_register (&pdev->dev, + &npcm8xx_pinctrl_desc, pctrl); +if (IS_ERR (pctrl->pctldev)) { +dev_err (&pdev->dev,"Failed to register pinctrl device\n" );drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c line 2607
Quoted 10 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +10
-0+return PTR_ERR (pctrl->pctldev); + } + + ret =npcm8xx_gpio_register (pctrl); +if (ret <0 ) { +dev_err (pctrl->dev,"Failed to register gpio %u\n" , ret); +return ret; + } + +pr_info ("npcm8xx Pinctrl driver probed\n" );No
pr_in devices. No success debug messages.Quoted 23 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +23
-0+return 0 ; +} + +static const struct of_device_id npcm8xx_pinctrl_match[] = { + { .compatible ="nuvoton,npcm845-pinctrl" }, + { }, +}; +MODULE_DEVICE_TABLE (of, npcm8xx_pinctrl_match); + +static struct platform_driver npcm8xx_pinctrl_driver = { + .probe = npcm8xx_pinctrl_probe, + .driver = { + .name ="npcm8xx-pinctrl" , + .of_match_table = npcm8xx_pinctrl_match, + .suppress_bind_attrs = true, + }, +}; + +static int __init npcm8xx_pinctrl_register (void ) +{ +return platform_driver_register (&npcm8xx_pinctrl_driver); +} +arch_initcall (npcm8xx_pinctrl_register);
Quoted 8 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c line 2634
Quoted 4 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +4
-0+ +MODULE_LICENSE ("GPL v2" ); +MODULE_AUTHOR ("[email protected]" ); +MODULE_DESCRIPTION ("Nuvoton NPCM8XX Pinctrl and GPIO driver" );Best regards,
Krzysztof
Best regards,
Tomer
Tomer Maimon
maintainerRe: [PATCH v1 2/2] pinctrl: nuvoton: add NPCM8XX pinctrl and GPIO driver
In reply to Krzysztof KozlowskiHi Krzysztof,
Thanks for your comments.
Quoted 22 lines · Krzysztof Kozlowski
Quoted 7 lines · Tomer Maimon
Add pinctrl and GPIO controller driver support to Arbel BMC NPCM8XX SoC.
Arbel BMC NPCM8XX pinctrl driver based on Poleg NPCM7XX, except the pin mux mapping difference the NPCM8XX GPIO supports adjust debounce period time.
Signed-off-by Tomer Maimon <[email protected]>(...)
Quoted 10 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +10
-0+ +static int npcm8xx_pinctrl_probe (struct platform_device *pdev) +{ +struct npcm8xx_pinctrl *pctrl; +int ret; + + pctrl =devm_kzalloc (&pdev->dev,sizeof (*pctrl),GFP_KERNEL ); +if (!pctrl) +return -ENOMEM ; +
Quoted 8 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c line 2582
Quoted 5 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +5
-0+ pctrl->dev = &pdev->dev; +dev_set_drvdata (&pdev->dev, pctrl); + + pctrl->gcr_regmap = +syscon_regmap_lookup_by_compatible ("nuvoton,npcm845-gcr" );No. Use property. By this patchset, I would expect that you learnt from previous mistakes around this. Why repeating the same trouble second time?
You suggest to use phandle property like nuvoton,sysgcr even that the NPCM8XX pin controller driver is used only NPCM8XX SoC, so the only GCR node in the NPCM8XX SoC is nuvoton,npcm845-gcr?
Quoted 53 lines
Quoted 16 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +15
-0+if (IS_ERR (pctrl->gcr_regmap)) { +dev_err (pctrl->dev,"didn't find nuvoton,npcm845-gcr\n" ); +return PTR_ERR (pctrl->gcr_regmap); + } + + ret =npcm8xx_gpio_of (pctrl); +if (ret <0 ) { +dev_err (pctrl->dev,"Failed to gpio dt-binding %u\n" , ret); +return ret; + } + + pctrl->pctldev =devm_pinctrl_register (&pdev->dev, + &npcm8xx_pinctrl_desc, pctrl); +if (IS_ERR (pctrl->pctldev)) { +dev_err (&pdev->dev,"Failed to register pinctrl device\n" );drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c line 2607
Quoted 10 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +10
-0+return PTR_ERR (pctrl->pctldev); + } + + ret =npcm8xx_gpio_register (pctrl); +if (ret <0 ) { +dev_err (pctrl->dev,"Failed to register gpio %u\n" , ret); +return ret; + } + +pr_info ("npcm8xx Pinctrl driver probed\n" );No
pr_in devices. No success debug messages.Quoted 23 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +23
-0+return 0 ; +} + +static const struct of_device_id npcm8xx_pinctrl_match[] = { + { .compatible ="nuvoton,npcm845-pinctrl" }, + { }, +}; +MODULE_DEVICE_TABLE (of, npcm8xx_pinctrl_match); + +static struct platform_driver npcm8xx_pinctrl_driver = { + .probe = npcm8xx_pinctrl_probe, + .driver = { + .name ="npcm8xx-pinctrl" , + .of_match_table = npcm8xx_pinctrl_match, + .suppress_bind_attrs = true, + }, +}; + +static int __init npcm8xx_pinctrl_register (void ) +{ +return platform_driver_register (&npcm8xx_pinctrl_driver); +} +arch_initcall (npcm8xx_pinctrl_register);
Quoted 8 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c line 2634
Quoted 4 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +4
-0+ +MODULE_LICENSE ("GPL v2" ); +MODULE_AUTHOR ("[email protected]" ); +MODULE_DESCRIPTION ("Nuvoton NPCM8XX Pinctrl and GPIO driver" );Best regards,
Krzysztof
Best regards,
Tomer
Re: [PATCH v1 2/2] pinctrl: nuvoton: add NPCM8XX pinctrl and GPIO driver
Quoted 17 lines · Tomer Maimon
Quoted 9 lines
Quoted 9 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +9
-0+static int npcm8xx_pinctrl_probe (struct platform_device *pdev) +{ +struct npcm8xx_pinctrl *pctrl; +int ret; + + pctrl =devm_kzalloc (&pdev->dev,sizeof (*pctrl),GFP_KERNEL ); +if (!pctrl) +return -ENOMEM ; +Quoted 8 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c line 2582
Quoted 5 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +5
-0+ pctrl->dev = &pdev->dev; +dev_set_drvdata (&pdev->dev, pctrl); + + pctrl->gcr_regmap = +syscon_regmap_lookup_by_compatible ("nuvoton,npcm845-gcr" );No. Use property. By this patchset, I would expect that you learnt from previous mistakes around this. Why repeating the same trouble second time?
Quoted 3 lines
You suggest to use phandle property like nuvoton,sysgcr even that the NPCM8XX pin controller driver is used only NPCM8XX SoC, so the only GCR node in the NPCM8XX SoC is nuvoton,npcm845-gcr?
Yes. The previous case (reset driver, AFAIR) was also about driver used only in one SoC, wasn't it?
Best regards,
Krzysztof
Tomer Maimon
maintainerRe: [PATCH v1 2/2] pinctrl: nuvoton: add NPCM8XX pinctrl and GPIO driver
Quoted 20 lines · Krzysztof Kozlowski
Quoted 17 lines · Tomer Maimon
Quoted 17 lines
Quoted 4 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +4
-0+static int npcm8xx_pinctrl_probe (struct platform_device *pdev) +{ +struct npcm8xx_pinctrl *pctrl; +int ret;drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c line 2582
Quoted 10 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +10
-0+ + pctrl =devm_kzalloc (&pdev->dev,sizeof (*pctrl),GFP_KERNEL ); +if (!pctrl) +return -ENOMEM ; + + pctrl->dev = &pdev->dev; +dev_set_drvdata (&pdev->dev, pctrl); + + pctrl->gcr_regmap = +syscon_regmap_lookup_by_compatible ("nuvoton,npcm845-gcr" );No. Use property. By this patchset, I would expect that you learnt from previous mistakes around this. Why repeating the same trouble second time?
Quoted 6 lines
Quoted 3 lines
You suggest to use phandle property like nuvoton,sysgcr even that the NPCM8XX pin controller driver is used only NPCM8XX SoC, so the only GCR node in the NPCM8XX SoC is nuvoton,npcm845-gcr?
Yes. The previous case (reset driver, AFAIR) was also about driver used only in one SoC, wasn't it?
Actually not, the NPCM reset driver serves NPCM7XX and NPCM8XX and probably other future BMC SoC's
Still, you suggest using the phandle property in the driver even if the driver serves one SoC?
Quoted 3 lines
Best regards,
Krzysztof
Best regards,
Tomer
Re: [PATCH v1 2/2] pinctrl: nuvoton: add NPCM8XX pinctrl and GPIO driver
Quoted 22 lines · Tomer Maimon
Quoted 20 lines · Krzysztof Kozlowski
Quoted 17 lines · Tomer Maimon
Quoted 17 lines
Quoted 4 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +4
-0+static int npcm8xx_pinctrl_probe (struct platform_device *pdev) +{ +struct npcm8xx_pinctrl *pctrl; +int ret;drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c line 2582
Quoted 10 lines
drivers/pinctrl/nuvoton/pinctrl-npcm8xx.c new file +10
-0+ + pctrl =devm_kzalloc (&pdev->dev,sizeof (*pctrl),GFP_KERNEL ); +if (!pctrl) +return -ENOMEM ; + + pctrl->dev = &pdev->dev; +dev_set_drvdata (&pdev->dev, pctrl); + + pctrl->gcr_regmap = +syscon_regmap_lookup_by_compatible ("nuvoton,npcm845-gcr" );No. Use property. By this patchset, I would expect that you learnt from previous mistakes around this. Why repeating the same trouble second time?
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
You suggest to use phandle property like nuvoton,sysgcr even that the NPCM8XX pin controller driver is used only NPCM8XX SoC, so the only GCR node in the NPCM8XX SoC is nuvoton,npcm845-gcr?
Yes. The previous case (reset driver, AFAIR) was also about driver used only in one SoC, wasn't it?
Actually not, the NPCM reset driver serves NPCM7XX and NPCM8XX and probably other future BMC SoC's
No, when someone developed reset driver, it served only NPCM7XX. So using this argument - only one SoC is supported - that person use exactly the same API as here.
And it was wrong...
Now you use the same argument - only one SoC is supported.
I clearly see a pattern here...
Quoted 9 lines
Still, you suggest using the phandle property in the driver even if the driver serves one SoC?
Quoted 3 lines
Best regards,
KrzysztofBest regards,
Tomer
Best regards,
Krzysztof