* [PATCH RFC 0/3] Pinctrl: Add Amlogic pinctrl driver
@ 2024-12-11 6:47 Xianwei Zhao via B4 Relay
2024-12-11 6:47 ` [PATCH RFC 1/3] dt-bindings: pinctrl: Add support for Amlogic SoCs Xianwei Zhao via B4 Relay
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Xianwei Zhao via B4 Relay @ 2024-12-11 6:47 UTC (permalink / raw)
To: Linus Walleij, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Neil Armstrong, Kevin Hilman, Jerome Brunet, Martin Blumenstingl
Cc: linux-gpio, devicetree, linux-kernel, linux-arm-kernel,
linux-amlogic, Xianwei Zhao
Add pinctrl driver support for Amloigc SoCs
Base on the previous discussion,
https://lore.kernel.org/r/20241113-a4_pinctrl-v6-0-35ba2401ee35@amlogic.com
The existed meson driver failed to meet the requirement of the current dt-binding.
So we start this new pinctrl driver to solve problem.
The advantage of this version: Once the source file and binding document
are added, adding dts node will be only operation for subsequent Amlogic SoCs
(such as A4, A5).
The code in DTS file is also readable when using GPIO, as below:
reset-gpios = <&gpiob 6 GPIO_ACTIVE_LOW>;
Signed-off-by: Xianwei Zhao <xianwei.zhao@amlogic.com>
---
Xianwei Zhao (3):
dt-bindings: pinctrl: Add support for Amlogic SoCs
pinctrl: Add driver support for Amlogic SoCs
arm64: dts: amlogic: a4: add pinctrl node
.../bindings/pinctrl/amlogic,pinctrl.yaml | 150 +++
arch/arm64/boot/dts/amlogic/amlogic-a4.dtsi | 146 +++
drivers/pinctrl/Kconfig | 18 +
drivers/pinctrl/Makefile | 1 +
drivers/pinctrl/pinctrl-amlogic.c | 1190 ++++++++++++++++++++
include/dt-bindings/pinctrl/amlogic,pinctrl.h | 68 ++
6 files changed, 1573 insertions(+)
---
base-commit: 171aa289a6fe65faffeb92a1fda283c055435a62
change-id: 20241211-amlogic-pinctrl-22ea61820d0d
Best regards,
--
Xianwei Zhao <xianwei.zhao@amlogic.com>
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH RFC 1/3] dt-bindings: pinctrl: Add support for Amlogic SoCs 2024-12-11 6:47 [PATCH RFC 0/3] Pinctrl: Add Amlogic pinctrl driver Xianwei Zhao via B4 Relay @ 2024-12-11 6:47 ` Xianwei Zhao via B4 Relay 2024-12-17 14:20 ` Rob Herring 2024-12-11 6:47 ` [PATCH RFC 2/3] pinctrl: Add driver " Xianwei Zhao via B4 Relay 2024-12-11 6:47 ` [PATCH RFC 3/3] arm64: dts: amlogic: a4: add pinctrl node Xianwei Zhao via B4 Relay 2 siblings, 1 reply; 14+ messages in thread From: Xianwei Zhao via B4 Relay @ 2024-12-11 6:47 UTC (permalink / raw) To: Linus Walleij, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Neil Armstrong, Kevin Hilman, Jerome Brunet, Martin Blumenstingl Cc: linux-gpio, devicetree, linux-kernel, linux-arm-kernel, linux-amlogic, Xianwei Zhao From: Xianwei Zhao <xianwei.zhao@amlogic.com> Add the dt-bindings for Amlogic pin controller, and add a new dt-binding header file which document the GPIO bank names and alternative func value of all Amlogic subsequent SoCs. Signed-off-by: Xianwei Zhao <xianwei.zhao@amlogic.com> --- .../bindings/pinctrl/amlogic,pinctrl.yaml | 150 +++++++++++++++++++++ include/dt-bindings/pinctrl/amlogic,pinctrl.h | 68 ++++++++++ 2 files changed, 218 insertions(+) diff --git a/Documentation/devicetree/bindings/pinctrl/amlogic,pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/amlogic,pinctrl.yaml new file mode 100644 index 000000000000..b0c2ae585d7d --- /dev/null +++ b/Documentation/devicetree/bindings/pinctrl/amlogic,pinctrl.yaml @@ -0,0 +1,150 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/pinctrl/amlogic,pinctrl.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Amlogic pinmux controller + +maintainers: + - Xianwei Zhao <xianwei.zhao@amlogic.com> + +allOf: + - $ref: pinctrl.yaml# + +properties: + compatible: + const: amlogic,pinctrl + + reg: + minItems: 2 + + reg-names: + items: + - const: mux + - const: gpio + + "#address-cells": + const: 2 + + "#size-cells": + const: 2 + +required: + - compatible + - reg + - reg-names + - "#address-cells" + - "#size-cells" + +patternProperties: + "^gpio[a-z]": + type: object + + properties: + gpio-controller: true + + "#gpio-cells": + const: 2 + + bank-name: + $ref: /schemas/types.yaml#/definitions/string + + npins: + $ref: /schemas/types.yaml#/definitions/uint32 + minimum: 1 + maximum: 32 + + bank-index: + $ref: /schemas/types.yaml#/definitions/uint32 + minimum: 0 + maximum: 29 + + reg-mux-offset: + $ref: /schemas/types.yaml#/definitions/uint32 + + bit-mux-offset: + $ref: /schemas/types.yaml#/definitions/uint32 + + reg-gpio-offset: + $ref: /schemas/types.yaml#/definitions/uint32 + + required: + - gpio-controller + - "#gpio-cells" + - npins + - bank-index + - reg-mux-offset + - reg-gpio-offset + - bank-name + + additionalProperties: false + unevaluatedProperties: false + + "^func-[0-9a-z]": + type: object + additionalProperties: + type: object + allOf: + - $ref: pincfg-node.yaml# + - $ref: pinmux-node.yaml# + +additionalProperties: false + +examples: + - | + apb { + #address-cells = <2>; + #size-cells = <2>; + periphs_pinctrl: pinctrl@8e700 { + compatible = "amlogic,pinctrl"; + #address-cells = <2>; + #size-cells = <2>; + reg = <0x0 0x8e700 0x0 0x04>, + <0x0 0x8e704 0x0 0x60>; + reg-names = "mux", "gpio"; + + gpiob { + gpio-controller; + #gpio-cells = <2>; + npins = <10>; + bank-index = <1>; + reg-mux-offset = <0x14>; + bit-mux-offset = <0x14>; + reg-gpio-offset = <0x30>; + bank-name = "GPIOB"; + }; + + gpioe { + gpio-controller; + #gpio-cells = <2>; + npins = <10>; + bank-index = <5>; + reg-mux-offset = <0x14>; + reg-gpio-offset = <0x30>; + bank-name = "GPIOE"; + }; + + func-uart-b { + uart-b-default{ + pinmux = <3>; + bias-pull-up; + drive-strength-microamp = <4000>; + }; + + uart-c-default{ + pinmux = <4>; + bias-pull-up; + drive-strength-microamp = <4000>; + }; + }; + + func-uart-c { + uart-c-default{ + pinmux = <3>; + bias-pull-up; + drive-strength-microamp = <4000>; + }; + }; + }; + }; diff --git a/include/dt-bindings/pinctrl/amlogic,pinctrl.h b/include/dt-bindings/pinctrl/amlogic,pinctrl.h new file mode 100644 index 000000000000..03db0a730e8b --- /dev/null +++ b/include/dt-bindings/pinctrl/amlogic,pinctrl.h @@ -0,0 +1,68 @@ +/* SPDX-License-Identifier: (GPL-2.0-only OR MIT) */ +/* + * Copyright (c) 2024 Amlogic, Inc. All rights reserved. + * Author: Xianwei Zhao <xianwei.zhao@amlogic.com> + */ + +#ifndef _DT_BINDINGS_AMLOGIC_PINCTRL_H +#define _DT_BINDINGS_AMLOGIC_PINCTRL_H + +/* define PIN modes */ +#define AF0 0x0 +#define AF1 0x1 +#define AF2 0x2 +#define AF3 0x3 +#define AF4 0x4 +#define AF5 0x5 +#define AF6 0x6 +#define AF7 0x7 +#define AF8 0x8 +#define AF9 0x9 +#define AF10 0xa +#define AF11 0xb +#define AF12 0xc +#define AF13 0xd +#define AF14 0xe +#define AF15 0xf + +#define AML_PIN_ALT_FUNC_MASK 0xf + +/* Normal PIN bank */ +#define AMLOGIC_GPIO_A 0 +#define AMLOGIC_GPIO_B 1 +#define AMLOGIC_GPIO_C 2 +#define AMLOGIC_GPIO_D 3 +#define AMLOGIC_GPIO_E 4 +#define AMLOGIC_GPIO_F 5 +#define AMLOGIC_GPIO_G 6 +#define AMLOGIC_GPIO_H 7 +#define AMLOGIC_GPIO_I 8 +#define AMLOGIC_GPIO_J 9 +#define AMLOGIC_GPIO_K 10 +#define AMLOGIC_GPIO_L 11 +#define AMLOGIC_GPIO_M 12 +#define AMLOGIC_GPIO_N 13 +#define AMLOGIC_GPIO_O 14 +#define AMLOGIC_GPIO_P 15 +#define AMLOGIC_GPIO_Q 16 +#define AMLOGIC_GPIO_R 17 +#define AMLOGIC_GPIO_S 18 +#define AMLOGIC_GPIO_T 19 +#define AMLOGIC_GPIO_U 20 +#define AMLOGIC_GPIO_V 21 +#define AMLOGIC_GPIO_W 22 +#define AMLOGIC_GPIO_X 23 +#define AMLOGIC_GPIO_Y 24 +#define AMLOGIC_GPIO_Z 25 + +/* Special PIN bank */ +#define AMLOGIC_GPIO_DV 26 +#define AMLOGIC_GPIO_AO 27 +#define AMLOGIC_GPIO_CC 28 +#define AMLOGIC_GPIO_TEST_N 29 + +#define AML_PINMUX(bank, offset, mode) (((((bank) << 8) + (offset)) << 8) | (mode)) +#define AML_PINMUX_TO_BANK(pinmux) (((pinmux) >> 16) & 0xff) +#define AML_PINMUX_TO_OFFSET(pinmux) (((pinmux) >> 8) & 0xff) + +#endif /* _DT_BINDINGS_AMLOGIC_PINCTRL_H */ -- 2.37.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH RFC 1/3] dt-bindings: pinctrl: Add support for Amlogic SoCs 2024-12-11 6:47 ` [PATCH RFC 1/3] dt-bindings: pinctrl: Add support for Amlogic SoCs Xianwei Zhao via B4 Relay @ 2024-12-17 14:20 ` Rob Herring 2024-12-18 8:47 ` Xianwei Zhao 0 siblings, 1 reply; 14+ messages in thread From: Rob Herring @ 2024-12-17 14:20 UTC (permalink / raw) To: Xianwei Zhao Cc: Linus Walleij, Krzysztof Kozlowski, Conor Dooley, Neil Armstrong, Kevin Hilman, Jerome Brunet, Martin Blumenstingl, linux-gpio, devicetree, linux-kernel, linux-arm-kernel, linux-amlogic On Wed, Dec 11, 2024 at 02:47:49PM +0800, Xianwei Zhao wrote: > Add the dt-bindings for Amlogic pin controller, and add a new > dt-binding header file which document the GPIO bank names and > alternative func value of all Amlogic subsequent SoCs. > > Signed-off-by: Xianwei Zhao <xianwei.zhao@amlogic.com> > --- > .../bindings/pinctrl/amlogic,pinctrl.yaml | 150 +++++++++++++++++++++ > include/dt-bindings/pinctrl/amlogic,pinctrl.h | 68 ++++++++++ > 2 files changed, 218 insertions(+) > > diff --git a/Documentation/devicetree/bindings/pinctrl/amlogic,pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/amlogic,pinctrl.yaml > new file mode 100644 > index 000000000000..b0c2ae585d7d > --- /dev/null > +++ b/Documentation/devicetree/bindings/pinctrl/amlogic,pinctrl.yaml > @@ -0,0 +1,150 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/pinctrl/amlogic,pinctrl.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Amlogic pinmux controller > + > +maintainers: > + - Xianwei Zhao <xianwei.zhao@amlogic.com> > + > +allOf: > + - $ref: pinctrl.yaml# > + > +properties: > + compatible: > + const: amlogic,pinctrl Only one Amlogic pinctrl block? Pretty sure there's more than 1 SoC and pinctrl is unlikely identical for all. This must be SoC specific. > + > + reg: > + minItems: 2 > + > + reg-names: > + items: > + - const: mux > + - const: gpio > + > + "#address-cells": > + const: 2 > + > + "#size-cells": > + const: 2 Why do you need these? You don't have any child nodes with addresses. But maybe you should? > + > +required: > + - compatible > + - reg > + - reg-names > + - "#address-cells" > + - "#size-cells" > + > +patternProperties: > + "^gpio[a-z]": This allows "gpioanythingyouwant" for example. If there is no unit-address, then it should be '^gpio-[0-9]+$'. > + type: object > + > + properties: > + gpio-controller: true > + > + "#gpio-cells": > + const: 2 > + > + bank-name: > + $ref: /schemas/types.yaml#/definitions/string You must define values, but why do you need this? No other GPIO controller needs something like this. > + > + npins: We have a standard property for this: ngpios > + $ref: /schemas/types.yaml#/definitions/uint32 > + minimum: 1 > + maximum: 32 > + > + bank-index: > + $ref: /schemas/types.yaml#/definitions/uint32 > + minimum: 0 > + maximum: 29 Consider if 'bank' should be part of the gpio cells rather than separate controller nodes. That works better if each bank is not its own separate h/w block. If banks share a register, then that's not separate blocks. > + > + reg-mux-offset: > + $ref: /schemas/types.yaml#/definitions/uint32 > + > + bit-mux-offset: > + $ref: /schemas/types.yaml#/definitions/uint32 > + > + reg-gpio-offset: > + $ref: /schemas/types.yaml#/definitions/uint32 Put these into 'reg'. We even have a way to deal with bit offsets. See leds/register-bit-led.yaml for example. > + > + required: > + - gpio-controller > + - "#gpio-cells" > + - npins > + - bank-index > + - reg-mux-offset > + - reg-gpio-offset > + - bank-name > + > + additionalProperties: false > + unevaluatedProperties: false Don't need both. 'additionalProperties' is more restrictive, so drop unevaluatedProperties. > + > + "^func-[0-9a-z]": "^func-[0-9a-z-]+$" > + type: object > + additionalProperties: > + type: object > + allOf: > + - $ref: pincfg-node.yaml# > + - $ref: pinmux-node.yaml# > + > +additionalProperties: false > + > +examples: > + - | > + apb { > + #address-cells = <2>; > + #size-cells = <2>; > + periphs_pinctrl: pinctrl@8e700 { > + compatible = "amlogic,pinctrl"; > + #address-cells = <2>; > + #size-cells = <2>; > + reg = <0x0 0x8e700 0x0 0x04>, > + <0x0 0x8e704 0x0 0x60>; > + reg-names = "mux", "gpio"; > + > + gpiob { > + gpio-controller; > + #gpio-cells = <2>; > + npins = <10>; > + bank-index = <1>; > + reg-mux-offset = <0x14>; > + bit-mux-offset = <0x14>; > + reg-gpio-offset = <0x30>; > + bank-name = "GPIOB"; > + }; > + > + gpioe { > + gpio-controller; > + #gpio-cells = <2>; > + npins = <10>; > + bank-index = <5>; > + reg-mux-offset = <0x14>; > + reg-gpio-offset = <0x30>; > + bank-name = "GPIOE"; > + }; > + > + func-uart-b { > + uart-b-default{ > + pinmux = <3>; > + bias-pull-up; > + drive-strength-microamp = <4000>; > + }; > + > + uart-c-default{ > + pinmux = <4>; > + bias-pull-up; > + drive-strength-microamp = <4000>; > + }; > + }; > + > + func-uart-c { > + uart-c-default{ > + pinmux = <3>; > + bias-pull-up; > + drive-strength-microamp = <4000>; > + }; > + }; > + }; > + }; > diff --git a/include/dt-bindings/pinctrl/amlogic,pinctrl.h b/include/dt-bindings/pinctrl/amlogic,pinctrl.h > new file mode 100644 > index 000000000000..03db0a730e8b > --- /dev/null > +++ b/include/dt-bindings/pinctrl/amlogic,pinctrl.h > @@ -0,0 +1,68 @@ > +/* SPDX-License-Identifier: (GPL-2.0-only OR MIT) */ > +/* > + * Copyright (c) 2024 Amlogic, Inc. All rights reserved. > + * Author: Xianwei Zhao <xianwei.zhao@amlogic.com> > + */ > + > +#ifndef _DT_BINDINGS_AMLOGIC_PINCTRL_H > +#define _DT_BINDINGS_AMLOGIC_PINCTRL_H > + > +/* define PIN modes */ > +#define AF0 0x0 > +#define AF1 0x1 > +#define AF2 0x2 > +#define AF3 0x3 > +#define AF4 0x4 > +#define AF5 0x5 > +#define AF6 0x6 > +#define AF7 0x7 > +#define AF8 0x8 > +#define AF9 0x9 > +#define AF10 0xa > +#define AF11 0xb > +#define AF12 0xc > +#define AF13 0xd > +#define AF14 0xe > +#define AF15 0xf > + > +#define AML_PIN_ALT_FUNC_MASK 0xf > + > +/* Normal PIN bank */ > +#define AMLOGIC_GPIO_A 0 > +#define AMLOGIC_GPIO_B 1 > +#define AMLOGIC_GPIO_C 2 > +#define AMLOGIC_GPIO_D 3 > +#define AMLOGIC_GPIO_E 4 > +#define AMLOGIC_GPIO_F 5 > +#define AMLOGIC_GPIO_G 6 > +#define AMLOGIC_GPIO_H 7 > +#define AMLOGIC_GPIO_I 8 > +#define AMLOGIC_GPIO_J 9 > +#define AMLOGIC_GPIO_K 10 > +#define AMLOGIC_GPIO_L 11 > +#define AMLOGIC_GPIO_M 12 > +#define AMLOGIC_GPIO_N 13 > +#define AMLOGIC_GPIO_O 14 > +#define AMLOGIC_GPIO_P 15 > +#define AMLOGIC_GPIO_Q 16 > +#define AMLOGIC_GPIO_R 17 > +#define AMLOGIC_GPIO_S 18 > +#define AMLOGIC_GPIO_T 19 > +#define AMLOGIC_GPIO_U 20 > +#define AMLOGIC_GPIO_V 21 > +#define AMLOGIC_GPIO_W 22 > +#define AMLOGIC_GPIO_X 23 > +#define AMLOGIC_GPIO_Y 24 > +#define AMLOGIC_GPIO_Z 25 > + > +/* Special PIN bank */ > +#define AMLOGIC_GPIO_DV 26 > +#define AMLOGIC_GPIO_AO 27 > +#define AMLOGIC_GPIO_CC 28 > +#define AMLOGIC_GPIO_TEST_N 29 > + > +#define AML_PINMUX(bank, offset, mode) (((((bank) << 8) + (offset)) << 8) | (mode)) > +#define AML_PINMUX_TO_BANK(pinmux) (((pinmux) >> 16) & 0xff) > +#define AML_PINMUX_TO_OFFSET(pinmux) (((pinmux) >> 8) & 0xff) > + > +#endif /* _DT_BINDINGS_AMLOGIC_PINCTRL_H */ > > -- > 2.37.1 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH RFC 1/3] dt-bindings: pinctrl: Add support for Amlogic SoCs 2024-12-17 14:20 ` Rob Herring @ 2024-12-18 8:47 ` Xianwei Zhao 0 siblings, 0 replies; 14+ messages in thread From: Xianwei Zhao @ 2024-12-18 8:47 UTC (permalink / raw) To: Rob Herring Cc: Linus Walleij, Krzysztof Kozlowski, Conor Dooley, Neil Armstrong, Kevin Hilman, Jerome Brunet, Martin Blumenstingl, linux-gpio, devicetree, linux-kernel, linux-arm-kernel, linux-amlogic Hi Rob, Thanks for your reply. On 2024/12/17 22:20, Rob Herring wrote: > [ EXTERNAL EMAIL ] > > On Wed, Dec 11, 2024 at 02:47:49PM +0800, Xianwei Zhao wrote: >> Add the dt-bindings for Amlogic pin controller, and add a new >> dt-binding header file which document the GPIO bank names and >> alternative func value of all Amlogic subsequent SoCs. >> >> Signed-off-by: Xianwei Zhao <xianwei.zhao@amlogic.com> >> --- >> .../bindings/pinctrl/amlogic,pinctrl.yaml | 150 +++++++++++++++++++++ >> include/dt-bindings/pinctrl/amlogic,pinctrl.h | 68 ++++++++++ >> 2 files changed, 218 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/pinctrl/amlogic,pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/amlogic,pinctrl.yaml >> new file mode 100644 >> index 000000000000..b0c2ae585d7d >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/pinctrl/amlogic,pinctrl.yaml >> @@ -0,0 +1,150 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/pinctrl/amlogic,pinctrl.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Amlogic pinmux controller >> + >> +maintainers: >> + - Xianwei Zhao <xianwei.zhao@amlogic.com> >> + >> +allOf: >> + - $ref: pinctrl.yaml# >> + >> +properties: >> + compatible: >> + const: amlogic,pinctrl > > Only one Amlogic pinctrl block? Pretty sure there's more than 1 SoC and > pinctrl is unlikely identical for all. This must be SoC specific. > Will do, "amlogic,pinctrl" -> "amlogic,pinctrl-a4". >> + >> + reg: >> + minItems: 2 >> + >> + reg-names: >> + items: >> + - const: mux >> + - const: gpio >> + >> + "#address-cells": >> + const: 2 >> + >> + "#size-cells": >> + const: 2 > > Why do you need these? You don't have any child nodes with addresses. > But maybe you should? > I will modify "reg_mux_offset" to "reg", So #address-cells = <1> #size-cells = <0> >> + >> +required: >> + - compatible >> + - reg >> + - reg-names >> + - "#address-cells" >> + - "#size-cells" >> + >> +patternProperties: >> + "^gpio[a-z]": > > This allows "gpioanythingyouwant" for example. If there is no > unit-address, then it should be '^gpio-[0-9]+$'. > Will do. >> + type: object >> + >> + properties: >> + gpio-controller: true >> + >> + "#gpio-cells": >> + const: 2 >> + >> + bank-name: >> + $ref: /schemas/types.yaml#/definitions/string > > You must define values, but why do you need this? No other GPIO > controller needs something like this. > We define the bank name(GPIOA,GPIOB) on the chip specification, so it is more readable to describe the source code with such a name in the pin(GPIOA-0,GPIOA-1). Bank indexes are defined primarily for use in pinmux implicit queries. Bank-name and bank-index, they both describe the characteristics of the Bank, they just use it differently. I can keep bank-index, I can query the bank-name through bank-index. I will delete this property, keep bank-index property. >> + >> + npins: > > We have a standard property for this: ngpios > Will do. >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + minimum: 1 >> + maximum: 32 >> + >> + bank-index: >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + minimum: 0 >> + maximum: 29 > > Consider if 'bank' should be part of the gpio cells rather than > separate controller nodes. That works better if each bank is not its own > separate h/w block. If banks share a register, then that's not separate > blocks. > Each bank is its own separate h/w block(With TEST_N bank exception,it share mux reg with AO bank). Change the name of this property to "bank-identity" make it more appropriate. >> + >> + reg-mux-offset: >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + >> + bit-mux-offset: >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + >> + reg-gpio-offset: >> + $ref: /schemas/types.yaml#/definitions/uint32 > > Put these into 'reg'. We even have a way to deal with bit offsets. See > leds/register-bit-led.yaml for example. > Will do. > >> + >> + required: >> + - gpio-controller >> + - "#gpio-cells" >> + - npins >> + - bank-index >> + - reg-mux-offset >> + - reg-gpio-offset >> + - bank-name >> + >> + additionalProperties: false >> + unevaluatedProperties: false > > Don't need both. 'additionalProperties' is more restrictive, so drop > unevaluatedProperties. > Will drop unevaluatedProperties. >> + >> + "^func-[0-9a-z]": > > "^func-[0-9a-z-]+$" > Will do. >> + type: object >> + additionalProperties: >> + type: object >> + allOf: >> + - $ref: pincfg-node.yaml# >> + - $ref: pinmux-node.yaml# >> + >> +additionalProperties: false >> + >> +examples: >> + - | >> + apb { >> + #address-cells = <2>; >> + #size-cells = <2>; >> + periphs_pinctrl: pinctrl@8e700 { >> + compatible = "amlogic,pinctrl"; >> + #address-cells = <2>; >> + #size-cells = <2>; >> + reg = <0x0 0x8e700 0x0 0x04>, >> + <0x0 0x8e704 0x0 0x60>; >> + reg-names = "mux", "gpio"; >> + >> + gpiob { >> + gpio-controller; >> + #gpio-cells = <2>; >> + npins = <10>; >> + bank-index = <1>; >> + reg-mux-offset = <0x14>; >> + bit-mux-offset = <0x14>; >> + reg-gpio-offset = <0x30>; >> + bank-name = "GPIOB"; >> + }; >> + >> + gpioe { >> + gpio-controller; >> + #gpio-cells = <2>; >> + npins = <10>; >> + bank-index = <5>; >> + reg-mux-offset = <0x14>; >> + reg-gpio-offset = <0x30>; >> + bank-name = "GPIOE"; >> + }; >> + >> + func-uart-b { >> + uart-b-default{ >> + pinmux = <3>; >> + bias-pull-up; >> + drive-strength-microamp = <4000>; >> + }; >> + >> + uart-c-default{ >> + pinmux = <4>; >> + bias-pull-up; >> + drive-strength-microamp = <4000>; >> + }; >> + }; >> + >> + func-uart-c { >> + uart-c-default{ >> + pinmux = <3>; >> + bias-pull-up; >> + drive-strength-microamp = <4000>; >> + }; >> + }; >> + }; >> + }; >> diff --git a/include/dt-bindings/pinctrl/amlogic,pinctrl.h b/include/dt-bindings/pinctrl/amlogic,pinctrl.h >> new file mode 100644 >> index 000000000000..03db0a730e8b >> --- /dev/null >> +++ b/include/dt-bindings/pinctrl/amlogic,pinctrl.h >> @@ -0,0 +1,68 @@ >> +/* SPDX-License-Identifier: (GPL-2.0-only OR MIT) */ >> +/* >> + * Copyright (c) 2024 Amlogic, Inc. All rights reserved. >> + * Author: Xianwei Zhao <xianwei.zhao@amlogic.com> >> + */ >> + >> +#ifndef _DT_BINDINGS_AMLOGIC_PINCTRL_H >> +#define _DT_BINDINGS_AMLOGIC_PINCTRL_H >> + >> +/* define PIN modes */ >> +#define AF0 0x0 >> +#define AF1 0x1 >> +#define AF2 0x2 >> +#define AF3 0x3 >> +#define AF4 0x4 >> +#define AF5 0x5 >> +#define AF6 0x6 >> +#define AF7 0x7 >> +#define AF8 0x8 >> +#define AF9 0x9 >> +#define AF10 0xa >> +#define AF11 0xb >> +#define AF12 0xc >> +#define AF13 0xd >> +#define AF14 0xe >> +#define AF15 0xf >> + >> +#define AML_PIN_ALT_FUNC_MASK 0xf >> + >> +/* Normal PIN bank */ >> +#define AMLOGIC_GPIO_A 0 >> +#define AMLOGIC_GPIO_B 1 >> +#define AMLOGIC_GPIO_C 2 >> +#define AMLOGIC_GPIO_D 3 >> +#define AMLOGIC_GPIO_E 4 >> +#define AMLOGIC_GPIO_F 5 >> +#define AMLOGIC_GPIO_G 6 >> +#define AMLOGIC_GPIO_H 7 >> +#define AMLOGIC_GPIO_I 8 >> +#define AMLOGIC_GPIO_J 9 >> +#define AMLOGIC_GPIO_K 10 >> +#define AMLOGIC_GPIO_L 11 >> +#define AMLOGIC_GPIO_M 12 >> +#define AMLOGIC_GPIO_N 13 >> +#define AMLOGIC_GPIO_O 14 >> +#define AMLOGIC_GPIO_P 15 >> +#define AMLOGIC_GPIO_Q 16 >> +#define AMLOGIC_GPIO_R 17 >> +#define AMLOGIC_GPIO_S 18 >> +#define AMLOGIC_GPIO_T 19 >> +#define AMLOGIC_GPIO_U 20 >> +#define AMLOGIC_GPIO_V 21 >> +#define AMLOGIC_GPIO_W 22 >> +#define AMLOGIC_GPIO_X 23 >> +#define AMLOGIC_GPIO_Y 24 >> +#define AMLOGIC_GPIO_Z 25 >> + >> +/* Special PIN bank */ >> +#define AMLOGIC_GPIO_DV 26 >> +#define AMLOGIC_GPIO_AO 27 >> +#define AMLOGIC_GPIO_CC 28 >> +#define AMLOGIC_GPIO_TEST_N 29 >> + >> +#define AML_PINMUX(bank, offset, mode) (((((bank) << 8) + (offset)) << 8) | (mode)) >> +#define AML_PINMUX_TO_BANK(pinmux) (((pinmux) >> 16) & 0xff) >> +#define AML_PINMUX_TO_OFFSET(pinmux) (((pinmux) >> 8) & 0xff) >> + >> +#endif /* _DT_BINDINGS_AMLOGIC_PINCTRL_H */ >> >> -- >> 2.37.1 >> ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH RFC 2/3] pinctrl: Add driver support for Amlogic SoCs 2024-12-11 6:47 [PATCH RFC 0/3] Pinctrl: Add Amlogic pinctrl driver Xianwei Zhao via B4 Relay 2024-12-11 6:47 ` [PATCH RFC 1/3] dt-bindings: pinctrl: Add support for Amlogic SoCs Xianwei Zhao via B4 Relay @ 2024-12-11 6:47 ` Xianwei Zhao via B4 Relay 2024-12-17 14:49 ` Linus Walleij 2024-12-11 6:47 ` [PATCH RFC 3/3] arm64: dts: amlogic: a4: add pinctrl node Xianwei Zhao via B4 Relay 2 siblings, 1 reply; 14+ messages in thread From: Xianwei Zhao via B4 Relay @ 2024-12-11 6:47 UTC (permalink / raw) To: Linus Walleij, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Neil Armstrong, Kevin Hilman, Jerome Brunet, Martin Blumenstingl Cc: linux-gpio, devicetree, linux-kernel, linux-arm-kernel, linux-amlogic, Xianwei Zhao From: Xianwei Zhao <xianwei.zhao@amlogic.com> Add a new pinctrl driver for Amlogic SoCs. All future Amlogic SoCs pinctrl drives use this, such A4, A5, S6, S7 etc. To support new Amlogic SoCs, only need to add the corresponding dts file. Signed-off-by: Xianwei Zhao <xianwei.zhao@amlogic.com> --- drivers/pinctrl/Kconfig | 18 + drivers/pinctrl/Makefile | 1 + drivers/pinctrl/pinctrl-amlogic.c | 1190 +++++++++++++++++++++++++++++++++++++ 3 files changed, 1209 insertions(+) diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig index 354536de564b..41bd85aa433d 100644 --- a/drivers/pinctrl/Kconfig +++ b/drivers/pinctrl/Kconfig @@ -49,6 +49,24 @@ config PINCTRL_AMD Requires ACPI/FDT device enumeration code to set up a platform device. +config PINCTRL_AMLOGIC + bool "AMLOGIC pincontrol" + depends on ARCH_MESON || COMPILE_TEST + depends on OF + default y + select PINMUX + select PINCONF + select GENERIC_PINCONF + select GPIOLIB + select OF_GPIO + select REGMAP_MMIO + help + This is the driver for the pin controller found on Amlogic SoCs. + + This driver is simplify subsequent support for new amlogic SoCs, + to support new Amlogic SoCs, only need to add the corresponding dts file, + no additional binding header files or C file are added. + config PINCTRL_APPLE_GPIO tristate "Apple SoC GPIO pin controller driver" depends on ARCH_APPLE diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile index 97823f52b972..f24c0b92a462 100644 --- a/drivers/pinctrl/Makefile +++ b/drivers/pinctrl/Makefile @@ -10,6 +10,7 @@ obj-$(CONFIG_GENERIC_PINCONF) += pinconf-generic.o obj-$(CONFIG_OF) += devicetree.o obj-$(CONFIG_PINCTRL_AMD) += pinctrl-amd.o +obj-$(CONFIG_PINCTRL_AMLOGIC) += pinctrl-amlogic.o obj-$(CONFIG_PINCTRL_APPLE_GPIO) += pinctrl-apple-gpio.o obj-$(CONFIG_PINCTRL_ARTPEC6) += pinctrl-artpec6.o obj-$(CONFIG_PINCTRL_AS3722) += pinctrl-as3722.o diff --git a/drivers/pinctrl/pinctrl-amlogic.c b/drivers/pinctrl/pinctrl-amlogic.c new file mode 100644 index 000000000000..61351b0d8fb0 --- /dev/null +++ b/drivers/pinctrl/pinctrl-amlogic.c @@ -0,0 +1,1190 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (c) 2024 Amlogic, Inc. All rights reserved. + * Author: Xianwei Zhao <xianwei.zhao@amlogic.com> + */ + +#include <linux/err.h> +#include <linux/gpio/driver.h> +#include <linux/init.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/of_address.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> +#include <linux/seq_file.h> +#include <linux/slab.h> +#include <linux/string_helpers.h> + +#include <linux/pinctrl/consumer.h> +#include <linux/pinctrl/pinconf.h> +#include <linux/pinctrl/pinctrl.h> +#include <linux/pinctrl/pinmux.h> +#include <dt-bindings/pinctrl/amlogic,pinctrl.h> + +#include "core.h" +#include "pinconf.h" + +#define gpio_chip_to_bank(chip) \ + container_of(chip, struct aml_gpio_bank, gpio_chip) +/** + * enum aml_reg_type - type of registers encoded in @aml_reg_desc + */ +enum aml_reg_type { + REG_PULLEN, + REG_PULL, + REG_DIR, + REG_OUT, + REG_IN, + REG_DS, + NUM_REG, +}; + +static const unsigned int aml_bit_strides[NUM_REG] = { + 1, 1, 1, 1, 1, 2 +}; + +static const unsigned int aml_reg_type_offset[NUM_REG] = { + 3, 4, 2, 1, 0, 7 +}; + +enum aml_pinconf_drv { + PINCONF_DRV_500UA, + PINCONF_DRV_2500UA, + PINCONF_DRV_3000UA, + PINCONF_DRV_4000UA, +}; + +struct aml_pio_control { + int reg_mux_offset; + int bit_mux_offset; + int reg_gpio_offset; + int reg_offset[NUM_REG]; +}; + +struct aml_pctl_data { +}; + +struct aml_pinconf { + int pin; + int altfunc; +}; + +struct aml_pmx_func { + const char *name; + const char **groups; + unsigned int ngroups; +}; + +struct aml_pctl_group { + const char *name; + unsigned short npins; + unsigned int *pins; + struct aml_pinconf *pin_conf; + unsigned long *configs; + unsigned int num_configs; +}; + +struct aml_gpio_bank { + struct gpio_chip gpio_chip; + struct aml_pio_control pc; + unsigned int bank_idx; + unsigned int pin_base; +}; + +struct aml_pinctrl { + struct device *dev; + struct pinctrl_dev *pctl; + struct aml_gpio_bank *banks; + int nbanks; + struct aml_pmx_func *functions; + int nfunctions; + struct aml_pctl_group *groups; + int ngroups; + + struct regmap *reg_mux; + struct regmap *reg_gpio; + struct regmap *reg_pullen; + struct regmap *reg_pull; + struct regmap *reg_ds; + const struct aml_pctl_data *data; +}; + +static int aml_pmx_calc_reg_and_offset(struct pinctrl_gpio_range *range, + unsigned int pin, unsigned int *reg, + unsigned int *offset) +{ + struct aml_gpio_bank *bank = gpio_chip_to_bank(range->gc); + unsigned int shift; + + shift = ((pin - range->pin_base) << 2) + bank->pc.bit_mux_offset; + *reg = (bank->pc.reg_mux_offset + (shift / 32)) * 4; + *offset = shift % 32; + + return 0; +} + +static int aml_pctl_set_function(struct aml_pinctrl *info, + struct pinctrl_gpio_range *range, + int pin_id, int func) +{ + int reg; + int offset; + + aml_pmx_calc_reg_and_offset(range, pin_id, ®, &offset); + return regmap_update_bits(info->reg_mux, reg, + 0xf << offset, (func & 0xf) << offset); +} + +static int aml_pmx_get_funcs_count(struct pinctrl_dev *pctldev) +{ + struct aml_pinctrl *info = pinctrl_dev_get_drvdata(pctldev); + + return info->nfunctions; +} + +static const char *aml_pmx_get_fname(struct pinctrl_dev *pctldev, + unsigned int selector) +{ + struct aml_pinctrl *info = pinctrl_dev_get_drvdata(pctldev); + + return info->functions[selector].name; +} + +static int aml_pmx_get_groups(struct pinctrl_dev *pctldev, + unsigned int selector, + const char * const **grps, + unsigned * const ngrps) +{ + struct aml_pinctrl *info = pinctrl_dev_get_drvdata(pctldev); + + *grps = info->functions[selector].groups; + *ngrps = info->functions[selector].ngroups; + + return 0; +} + +static int aml_pmx_set_mux(struct pinctrl_dev *pctldev, unsigned int fselector, + unsigned int group) +{ + struct aml_pinctrl *info = pinctrl_dev_get_drvdata(pctldev); + struct aml_pinconf *conf = info->groups[group].pin_conf; + struct pinctrl_gpio_range *range; + int i; + + for (i = 0; i < info->groups[group].npins; i++) { + range = pinctrl_find_gpio_range_from_pin(pctldev, conf[i].pin); + aml_pctl_set_function(info, range, conf[i].pin, conf[i].altfunc); + } + + return 0; +} + +static int aml_pmx_request_gpio(struct pinctrl_dev *pctldev, + struct pinctrl_gpio_range *range, + unsigned int pin) +{ + struct aml_pinctrl *info = pinctrl_dev_get_drvdata(pctldev); + + return aml_pctl_set_function(info, range, pin, 0); +} + +static const struct pinmux_ops aml_pmx_ops = { + .set_mux = aml_pmx_set_mux, + .get_functions_count = aml_pmx_get_funcs_count, + .get_function_name = aml_pmx_get_fname, + .get_function_groups = aml_pmx_get_groups, + .gpio_request_enable = aml_pmx_request_gpio, +}; + +static int aml_calc_reg_and_bit(struct pinctrl_dev *pctldev, + unsigned int pin, + enum aml_reg_type reg_type, + unsigned int *reg, unsigned int *bit) +{ + struct pinctrl_gpio_range *range = + pinctrl_find_gpio_range_from_pin(pctldev, pin); + struct aml_gpio_bank *bank = gpio_chip_to_bank(range->gc); + + *bit = (pin - range->pin_base) * aml_bit_strides[reg_type]; + *reg = (bank->pc.reg_offset[reg_type] + (*bit / 32)) * 4; + *bit &= 0x1f; + + return 0; +} + +static int aml_pinconf_get_pull(struct aml_pinctrl *info, unsigned int pin) +{ + unsigned int reg, bit, val; + int ret, conf; + + aml_calc_reg_and_bit(info->pctl, pin, REG_PULLEN, ®, &bit); + + ret = regmap_read(info->reg_pullen, reg, &val); + if (ret) + return ret; + + if (!(val & BIT(bit))) { + conf = PIN_CONFIG_BIAS_DISABLE; + } else { + aml_calc_reg_and_bit(info->pctl, pin, REG_PULL, ®, &bit); + + ret = regmap_read(info->reg_pull, reg, &val); + if (ret) + return ret; + + if (val & BIT(bit)) + conf = PIN_CONFIG_BIAS_PULL_UP; + else + conf = PIN_CONFIG_BIAS_PULL_DOWN; + } + + return conf; +} + +static int aml_pinconf_get_drive_strength(struct aml_pinctrl *info, + unsigned int pin, + u16 *drive_strength_ua) +{ + unsigned int reg, bit; + unsigned int val; + int ret; + + if (!info->reg_ds) + return -EOPNOTSUPP; + + aml_calc_reg_and_bit(info->pctl, pin, REG_DS, ®, &bit); + ret = regmap_read(info->reg_ds, reg, &val); + if (ret) + return ret; + + switch ((val >> bit) & 0x3) { + case PINCONF_DRV_500UA: + *drive_strength_ua = 500; + break; + case PINCONF_DRV_2500UA: + *drive_strength_ua = 2500; + break; + case PINCONF_DRV_3000UA: + *drive_strength_ua = 3000; + break; + case PINCONF_DRV_4000UA: + *drive_strength_ua = 4000; + break; + default: + return -EINVAL; + } + + return 0; +} + +static int aml_pinconf_get_gpio_bit(struct aml_pinctrl *info, + unsigned int pin, + unsigned int reg_type) +{ + unsigned int reg, bit, val; + int ret; + + aml_calc_reg_and_bit(info->pctl, pin, reg_type, ®, &bit); + ret = regmap_read(info->reg_gpio, reg, &val); + if (ret) + return ret; + + return BIT(bit) & val ? 1 : 0; +} + +static int aml_pinconf_get_output(struct aml_pinctrl *info, + unsigned int pin) +{ + int ret = aml_pinconf_get_gpio_bit(info, pin, REG_DIR); + + if (ret < 0) + return ret; + + return !ret; +} + +static int aml_pinconf_get_drive(struct aml_pinctrl *info, + unsigned int pin) +{ + return aml_pinconf_get_gpio_bit(info, pin, REG_OUT); +} + +static int aml_pinconf_get(struct pinctrl_dev *pcdev, unsigned int pin, + unsigned long *config) +{ + struct aml_pinctrl *info = pinctrl_dev_get_drvdata(pcdev); + enum pin_config_param param = pinconf_to_config_param(*config); + u16 arg; + int ret; + + switch (param) { + case PIN_CONFIG_BIAS_DISABLE: + case PIN_CONFIG_BIAS_PULL_DOWN: + case PIN_CONFIG_BIAS_PULL_UP: + if (aml_pinconf_get_pull(info, pin) == param) + arg = 1; + else + return -EINVAL; + break; + case PIN_CONFIG_DRIVE_STRENGTH_UA: + ret = aml_pinconf_get_drive_strength(info, pin, &arg); + if (ret) + return ret; + break; + case PIN_CONFIG_OUTPUT_ENABLE: + ret = aml_pinconf_get_output(info, pin); + if (ret <= 0) + return -EINVAL; + arg = 1; + break; + case PIN_CONFIG_OUTPUT: + ret = aml_pinconf_get_output(info, pin); + if (ret <= 0) + return -EINVAL; + + ret = aml_pinconf_get_drive(info, pin); + if (ret < 0) + return -EINVAL; + + arg = ret; + break; + + default: + return -ENOTSUPP; + } + + *config = pinconf_to_config_packed(param, arg); + dev_dbg(info->dev, "pinconf for pin %u is %lu\n", pin, *config); + + return 0; +} + +static int aml_pinconf_disable_bias(struct aml_pinctrl *info, + unsigned int pin) +{ + unsigned int reg, bit = 0; + + aml_calc_reg_and_bit(info->pctl, pin, REG_PULLEN, ®, &bit); + + return regmap_update_bits(info->reg_pullen, reg, BIT(bit), 0); +} + +static int aml_pinconf_enable_bias(struct aml_pinctrl *info, unsigned int pin, + bool pull_up) +{ + unsigned int reg, bit, val = 0; + int ret; + + aml_calc_reg_and_bit(info->pctl, pin, REG_PULL, ®, &bit); + if (pull_up) + val = BIT(bit); + + ret = regmap_update_bits(info->reg_pull, reg, BIT(bit), val); + if (ret) + return ret; + + aml_calc_reg_and_bit(info->pctl, pin, REG_PULLEN, ®, &bit); + return regmap_update_bits(info->reg_pullen, reg, BIT(bit), BIT(bit)); +} + +static int aml_pinconf_set_drive_strength(struct aml_pinctrl *info, + unsigned int pin, + u16 drive_strength_ua) +{ + unsigned int reg, bit, ds_val; + + if (!info->reg_ds) { + dev_err(info->dev, "drive-strength not supported\n"); + return -EOPNOTSUPP; + } + + aml_calc_reg_and_bit(info->pctl, pin, REG_DS, ®, &bit); + + if (drive_strength_ua <= 500) { + ds_val = PINCONF_DRV_500UA; + } else if (drive_strength_ua <= 2500) { + ds_val = PINCONF_DRV_2500UA; + } else if (drive_strength_ua <= 3000) { + ds_val = PINCONF_DRV_3000UA; + } else if (drive_strength_ua <= 4000) { + ds_val = PINCONF_DRV_4000UA; + } else { + dev_warn_once(info->dev, + "pin %u: invalid drive-strength : %d , default to 4mA\n", + pin, drive_strength_ua); + ds_val = PINCONF_DRV_4000UA; + } + + return regmap_update_bits(info->reg_ds, reg, 0x3 << bit, ds_val << bit); +} + +static int aml_pinconf_set_gpio_bit(struct aml_pinctrl *info, + unsigned int pin, + unsigned int reg_type, + bool arg) +{ + unsigned int reg, bit; + + aml_calc_reg_and_bit(info->pctl, pin, reg_type, ®, &bit); + return regmap_update_bits(info->reg_gpio, reg, BIT(bit), + arg ? BIT(bit) : 0); +} + +static int aml_pinconf_set_output(struct aml_pinctrl *info, + unsigned int pin, + bool out) +{ + return aml_pinconf_set_gpio_bit(info, pin, REG_DIR, !out); +} + +static int aml_pinconf_set_drive(struct aml_pinctrl *info, + unsigned int pin, + bool high) +{ + return aml_pinconf_set_gpio_bit(info, pin, REG_OUT, high); +} + +static int aml_pinconf_set_output_drive(struct aml_pinctrl *info, + unsigned int pin, + bool high) +{ + int ret; + + ret = aml_pinconf_set_output(info, pin, true); + if (ret) + return ret; + + return aml_pinconf_set_drive(info, pin, high); +} + +static int aml_pinconf_set(struct pinctrl_dev *pcdev, unsigned int pin, + unsigned long *configs, unsigned int num_configs) +{ + struct aml_pinctrl *info = pinctrl_dev_get_drvdata(pcdev); + enum pin_config_param param; + unsigned int arg = 0; + int i, ret; + + for (i = 0; i < num_configs; i++) { + param = pinconf_to_config_param(configs[i]); + + switch (param) { + case PIN_CONFIG_DRIVE_STRENGTH_UA: + case PIN_CONFIG_OUTPUT_ENABLE: + case PIN_CONFIG_OUTPUT: + arg = pinconf_to_config_argument(configs[i]); + break; + + default: + break; + } + + switch (param) { + case PIN_CONFIG_BIAS_DISABLE: + ret = aml_pinconf_disable_bias(info, pin); + break; + case PIN_CONFIG_BIAS_PULL_UP: + ret = aml_pinconf_enable_bias(info, pin, true); + break; + case PIN_CONFIG_BIAS_PULL_DOWN: + ret = aml_pinconf_enable_bias(info, pin, false); + break; + case PIN_CONFIG_DRIVE_STRENGTH_UA: + ret = aml_pinconf_set_drive_strength(info, pin, arg); + break; + case PIN_CONFIG_OUTPUT_ENABLE: + ret = aml_pinconf_set_output(info, pin, arg); + break; + case PIN_CONFIG_OUTPUT: + ret = aml_pinconf_set_output_drive(info, pin, arg); + break; + default: + ret = -ENOTSUPP; + } + + if (ret) + return ret; + } + + return 0; +} + +static int aml_pinconf_group_set(struct pinctrl_dev *pcdev, + unsigned int num_group, + unsigned long *configs, + unsigned int num_configs) +{ + struct aml_pinctrl *info = pinctrl_dev_get_drvdata(pcdev); + int i; + + for (i = 0; i < info->groups[num_group].npins; i++) { + aml_pinconf_set(pcdev, info->groups[num_group].pins[i], configs, + num_configs); + } + + return 0; +} + +static int aml_pinconf_group_get(struct pinctrl_dev *pcdev, + unsigned int group, unsigned long *config) +{ + return -EOPNOTSUPP; +} + +static const struct pinconf_ops aml_pinconf_ops = { + .pin_config_get = aml_pinconf_get, + .pin_config_set = aml_pinconf_set, + .pin_config_group_get = aml_pinconf_group_get, + .pin_config_group_set = aml_pinconf_group_set, + .is_generic = true, +}; + +static int aml_get_groups_count(struct pinctrl_dev *pctldev) +{ + struct aml_pinctrl *info = pinctrl_dev_get_drvdata(pctldev); + + return info->ngroups; +} + +static const char *aml_get_group_name(struct pinctrl_dev *pctldev, + unsigned int selector) +{ + struct aml_pinctrl *info = pinctrl_dev_get_drvdata(pctldev); + + return info->groups[selector].name; +} + +static int aml_get_group_pins(struct pinctrl_dev *pctldev, + unsigned int selector, const unsigned int **pins, + unsigned int *npins) +{ + struct aml_pinctrl *info = pinctrl_dev_get_drvdata(pctldev); + + if (selector >= info->ngroups) + return -EINVAL; + + *pins = info->groups[selector].pins; + *npins = info->groups[selector].npins; + + return 0; +} + +static inline const struct aml_pctl_group * + aml_pctl_find_group_by_name(const struct aml_pinctrl *info, + const char *name) +{ + int i; + + for (i = 0; i < info->ngroups; i++) { + if (!strcmp(info->groups[i].name, name)) + return &info->groups[i]; + } + + return NULL; +} + +static int aml_dt_node_to_map(struct pinctrl_dev *pctldev, + struct device_node *np, + struct pinctrl_map **map, + unsigned int *num_maps) +{ + struct aml_pinctrl *info = pinctrl_dev_get_drvdata(pctldev); + const struct aml_pctl_group *grp; + struct device *dev = info->dev; + struct pinctrl_map *new_map; + struct device_node *parent; + int map_num, i; + + grp = aml_pctl_find_group_by_name(info, np->name); + if (!grp) { + dev_err(dev, "unable to find group for node %pOFn\n", np); + return -EINVAL; + } + + if (grp->num_configs) + map_num = grp->npins + 1; + else + map_num = 1; + new_map = devm_kcalloc(dev, map_num, sizeof(*new_map), GFP_KERNEL); + if (!new_map) + return -ENOMEM; + + parent = of_get_parent(np); + if (!parent) { + devm_kfree(dev, new_map); + return -EINVAL; + } + + *map = new_map; + *num_maps = map_num; + new_map[0].type = PIN_MAP_TYPE_MUX_GROUP; + new_map[0].data.mux.function = parent->name; + new_map[0].data.mux.group = np->name; + of_node_put(parent); + + if (grp->num_configs) { + new_map++; + for (i = 0; i < grp->npins; i++) { + new_map[i].type = PIN_MAP_TYPE_CONFIGS_PIN; + new_map[i].data.configs.group_or_pin = + pin_get_name(pctldev, grp->pins[i]); + new_map[i].data.configs.configs = grp->configs; + new_map[i].data.configs.num_configs = grp->num_configs; + } + } + + dev_info(dev, "maps: function %s group %s num %d\n", + (*map)->data.mux.function, grp->name, map_num); + + return 0; +} + +static void aml_dt_free_map(struct pinctrl_dev *pctldev, + struct pinctrl_map *map, unsigned int num_maps) +{ +} + +static void aml_pin_dbg_show(struct pinctrl_dev *pcdev, struct seq_file *s, + unsigned int offset) +{ + seq_printf(s, " %s", dev_name(pcdev->dev)); +} + +static const struct pinctrl_ops aml_pctrl_ops = { + .get_groups_count = aml_get_groups_count, + .get_group_name = aml_get_group_name, + .get_group_pins = aml_get_group_pins, + .dt_node_to_map = aml_dt_node_to_map, + .dt_free_map = aml_dt_free_map, + .pin_dbg_show = aml_pin_dbg_show, +}; + +static int aml_pctl_dt_calculate_pin(struct aml_pinctrl *info, + unsigned int bank_idx, unsigned int offset) +{ + struct aml_gpio_bank *bank; + int retval = -EINVAL; + int i; + + for (i = 0; i < info->nbanks; i++) { + bank = &info->banks[i]; + if (bank->bank_idx == bank_idx) { + if (offset < bank->gpio_chip.ngpio) + retval = bank->pin_base + offset; + break; + } + } + if (retval == -EINVAL) + dev_err(info->dev, "pin [bank:%d, offset:%d] is not present\n", bank_idx, offset); + + return retval; +} + +static int aml_pctl_dt_parse_groups(struct device_node *np, + struct aml_pctl_group *grp, + struct aml_pinctrl *info, int idx) +{ + struct device *dev = info->dev; + struct aml_pinconf *conf; + struct property *of_pins; + unsigned int bank_idx; + unsigned int offset, npins; + int i = 0; + int ret; + + of_pins = of_find_property(np, "pinmux", NULL); + if (!of_pins) { + dev_info(dev, "Missing pinmux property\n"); + return -ENOENT; + } + + npins = of_pins->length / sizeof(u32); + grp->npins = npins; + grp->name = np->name; + grp->pins = devm_kcalloc(dev, npins, sizeof(*grp->pins), GFP_KERNEL); + grp->pin_conf = devm_kcalloc(dev, npins, sizeof(*grp->pin_conf), GFP_KERNEL); + + if (!grp->pins || !grp->pin_conf) + return -ENOMEM; + + ret = pinconf_generic_parse_dt_config(np, info->pctl, &grp->configs, + &grp->num_configs); + if (ret < 0) { + dev_err(dev, "%pOF: could not parse node property\n", np); + return ret; + } + + for (i = 0; i < npins; i++) { + u32 value; + + ret = of_property_read_u32_index(np, "pinmux", i, &value); + if (ret) + return ret; + + conf = &grp->pin_conf[i]; + + bank_idx = AML_PINMUX_TO_BANK(value); + offset = AML_PINMUX_TO_OFFSET(value); + conf->pin = aml_pctl_dt_calculate_pin(info, bank_idx, offset); + if (conf->pin < 0) + return dev_err_probe(dev, -EINVAL, "pinmux get pin failed\n"); + conf->altfunc = value & AML_PIN_ALT_FUNC_MASK; + + grp->pins[i] = conf->pin; + } + + return 0; +} + +static int aml_pctl_parse_functions(struct device_node *np, + struct aml_pinctrl *info, u32 index, + int *grp_index) +{ + struct device *dev = info->dev; + struct aml_pmx_func *func; + struct aml_pctl_group *grp; + int ret, i; + + func = &info->functions[index]; + func->name = np->name; + func->ngroups = of_get_child_count(np); + if (func->ngroups == 0) + return dev_err_probe(dev, -EINVAL, "No groups defined\n"); + + func->groups = devm_kcalloc(dev, func->ngroups, sizeof(*func->groups), GFP_KERNEL); + if (!func->groups) + return -ENOMEM; + + i = 0; + for_each_child_of_node_scoped(np, child) { + func->groups[i] = child->name; + grp = &info->groups[*grp_index]; + *grp_index += 1; + ret = aml_pctl_dt_parse_groups(child, grp, info, i++); + if (ret) + return ret; + } + dev_info(dev, "Function[%d\t name:%s,\tgroups:%d]\n", index, func->name, func->ngroups); + + return 0; +} + +static u32 aml_bank_pins(struct device_node *np) +{ + u32 value; + + if (of_property_read_u32(np, "npins", &value) < 0) + return 0; + else + return value; +} + +static u32 aml_bank_reg_gpio_offset(struct device_node *np) +{ + u32 value; + + if (of_property_read_u32(np, "reg-gpio-offset", &value) < 0) + return 0; + else + return value; +} + +static u32 aml_bank_reg_mux_offset(struct device_node *np) +{ + u32 value; + + if (of_property_read_u32(np, "reg-mux-offset", &value) < 0) + return 0; + else + return value; +} + +static u32 aml_bank_bit_mux_offset(struct device_node *np) +{ + u32 value; + + if (of_property_read_u32(np, "bit-mux-offset", &value) < 0) + return 0; + else + return value; +} + +static u32 aml_bank_index(struct device_node *np) +{ + u32 value; + + if (of_property_read_u32(np, "bank-index", &value) < 0) + return 0; + else + return value; +} + +static unsigned int aml_count_pins(struct device_node *np) +{ + struct device_node *child; + unsigned int pins = 0; + + for_each_child_of_node(np, child) { + if (of_property_read_bool(child, "gpio-controller")) + pins += aml_bank_pins(child); + } + + return pins; +} + +static void aml_pctl_dt_child_count(struct aml_pinctrl *info, + struct device_node *np) +{ + struct device_node *child; + + for_each_child_of_node(np, child) { + if (of_property_read_bool(child, "gpio-controller")) { + info->nbanks++; + } else { + info->nfunctions++; + info->ngroups += of_get_child_count(child); + } + } +} + +static struct regmap_config aml_regmap_config = { + .reg_bits = 32, + .val_bits = 32, + .reg_stride = 4, +}; + +static struct regmap *aml_map_resource(struct aml_pinctrl *info, + struct device_node *node, char *name) +{ + struct resource res; + void __iomem *base; + int i; + + i = of_property_match_string(node, "reg-names", name); + if (of_address_to_resource(node, i, &res)) + return NULL; + + base = devm_ioremap_resource(info->dev, &res); + if (IS_ERR(base)) + return ERR_CAST(base); + + aml_regmap_config.max_register = resource_size(&res) - 4; + aml_regmap_config.name = devm_kasprintf(info->dev, GFP_KERNEL, + "%pOFn-%s", node, + name); + if (!aml_regmap_config.name) + return ERR_PTR(-ENOMEM); + + return devm_regmap_init_mmio(info->dev, base, &aml_regmap_config); +} + +static inline int aml_gpio_calc_reg_and_bit(struct aml_gpio_bank *bank, + enum aml_reg_type reg_type, + unsigned int gpio, + unsigned int *reg, + unsigned int *bit) +{ + *bit = gpio * aml_bit_strides[reg_type]; + *reg = (bank->pc.reg_offset[reg_type] + (*bit / 32)) * 4; + *bit &= 0x1f; + + return 0; +} + +static int aml_gpio_get_direction(struct gpio_chip *chip, unsigned int gpio) +{ + struct aml_gpio_bank *bank = gpiochip_get_data(chip); + struct aml_pinctrl *info = dev_get_drvdata(chip->parent); + unsigned int bit, reg, val; + int ret; + + aml_gpio_calc_reg_and_bit(bank, REG_DIR, gpio, ®, &bit); + + ret = regmap_read(info->reg_gpio, reg, &val); + if (ret) + return ret; + + return BIT(bit) & val ? GPIO_LINE_DIRECTION_OUT : GPIO_LINE_DIRECTION_IN; +} + +static int aml_gpio_direction_input(struct gpio_chip *chip, unsigned int gpio) +{ + struct aml_gpio_bank *bank = gpiochip_get_data(chip); + struct aml_pinctrl *info = dev_get_drvdata(chip->parent); + unsigned int bit, reg; + + aml_gpio_calc_reg_and_bit(bank, REG_DIR, gpio, ®, &bit); + + return regmap_update_bits(info->reg_gpio, reg, BIT(bit), BIT(bit)); +} + +static int aml_gpio_direction_output(struct gpio_chip *chip, unsigned int gpio, + int value) +{ + struct aml_gpio_bank *bank = gpiochip_get_data(chip); + struct aml_pinctrl *info = dev_get_drvdata(chip->parent); + unsigned int bit, reg; + int ret; + + aml_gpio_calc_reg_and_bit(bank, REG_DIR, gpio, ®, &bit); + ret = regmap_update_bits(info->reg_gpio, reg, BIT(bit), BIT(bit)); + if (ret < 0) + return ret; + + aml_gpio_calc_reg_and_bit(bank, REG_OUT, gpio, ®, &bit); + + return regmap_update_bits(info->reg_gpio, reg, BIT(bit), + value ? BIT(bit) : 0); +} + +static void aml_gpio_set(struct gpio_chip *chip, unsigned int gpio, int value) +{ + struct aml_gpio_bank *bank = gpiochip_get_data(chip); + struct aml_pinctrl *info = dev_get_drvdata(chip->parent); + unsigned int bit, reg; + + aml_gpio_calc_reg_and_bit(bank, REG_OUT, gpio, ®, &bit); + + regmap_update_bits(info->reg_gpio, reg, BIT(bit), + value ? BIT(bit) : 0); +} + +static int aml_gpio_get(struct gpio_chip *chip, unsigned int gpio) +{ + struct aml_gpio_bank *bank = gpiochip_get_data(chip); + struct aml_pinctrl *info = dev_get_drvdata(chip->parent); + unsigned int reg, bit, val; + + aml_gpio_calc_reg_and_bit(bank, REG_IN, gpio, ®, &bit); + regmap_read(info->reg_gpio, reg, &val); + + return !!(val & BIT(bit)); +} + +static const struct gpio_chip aml_gpio_template = { + .request = gpiochip_generic_request, + .free = gpiochip_generic_free, + .set_config = gpiochip_generic_config, + .set = aml_gpio_set, + .get = aml_gpio_get, + .direction_input = aml_gpio_direction_input, + .direction_output = aml_gpio_direction_output, + .get_direction = aml_gpio_get_direction, + .can_sleep = false, +}; + +static int aml_gpiolib_register_bank(struct aml_pinctrl *info, + int bank_nr, struct device_node *np, + int pin_next_index) +{ + struct aml_gpio_bank *bank = &info->banks[bank_nr]; + struct device *dev = info->dev; + int err; + int i; + + bank->pc.reg_gpio_offset = aml_bank_reg_gpio_offset(np); + bank->pc.reg_mux_offset = aml_bank_reg_mux_offset(np); + bank->pc.bit_mux_offset = aml_bank_bit_mux_offset(np); + for (i = 0; i < NUM_REG; i++) + bank->pc.reg_offset[i] = bank->pc.reg_gpio_offset + aml_reg_type_offset[i]; + + bank->gpio_chip = aml_gpio_template; + bank->gpio_chip.base = -1; + bank->gpio_chip.ngpio = aml_bank_pins(np); + bank->gpio_chip.fwnode = of_fwnode_handle(np); + bank->gpio_chip.parent = dev; + + of_property_read_string(np, "bank-name", &bank->gpio_chip.label); + + bank->pin_base = pin_next_index; + bank->bank_idx = aml_bank_index(np); + + err = gpiochip_add_data(&bank->gpio_chip, bank); + if (err) + return dev_err_probe(dev, err, "Failed to add gpiochip(%d)!\n", bank_nr); + + dev_dbg(dev, "%s bank added.\n", bank->gpio_chip.label); + + return 0; +} + +static void aml_pctl_reg_extra(struct aml_pinctrl *info) +{ + if (!info->reg_pull) + info->reg_pull = info->reg_gpio; + if (!info->reg_pullen) + info->reg_pullen = info->reg_gpio; + if (!info->reg_ds) + info->reg_ds = info->reg_gpio; +} + +static int aml_pctl_probe_dt(struct platform_device *pdev, + struct pinctrl_desc *pctl_desc, + struct aml_pinctrl *info) +{ + struct device *dev = &pdev->dev; + struct pinctrl_pin_desc *pdesc; + struct device_node *np = dev->of_node; + int grp_index = 0; + int i = 0, j = 0, k = 0, bank; + int ret = 0; + + aml_pctl_dt_child_count(info, np); + if (!info->nbanks) + return dev_err_probe(dev, -EINVAL, "you need at least one gpio bank\n"); + + dev_dbg(dev, "nbanks = %d\n", info->nbanks); + dev_dbg(dev, "nfunctions = %d\n", info->nfunctions); + dev_dbg(dev, "ngroups = %d\n", info->ngroups); + + info->functions = devm_kcalloc(dev, info->nfunctions, sizeof(*info->functions), GFP_KERNEL); + + info->groups = devm_kcalloc(dev, info->ngroups, sizeof(*info->groups), GFP_KERNEL); + + info->banks = devm_kcalloc(dev, info->nbanks, sizeof(*info->banks), GFP_KERNEL); + + if (!info->functions || !info->groups || !info->banks) + return -ENOMEM; + + info->reg_mux = aml_map_resource(info, np, "mux"); + if (IS_ERR_OR_NULL(info->reg_mux)) + return dev_err_probe(dev, info->reg_mux ? PTR_ERR(info->reg_mux) : -ENOENT, + "mux registers not found\n"); + + info->reg_gpio = aml_map_resource(info, np, "gpio"); + if (IS_ERR_OR_NULL(info->reg_gpio)) + return dev_err_probe(dev, info->reg_gpio ? PTR_ERR(info->reg_gpio) : -ENOENT, + "gpio registers not found\n"); + + info->reg_pull = aml_map_resource(info, np, "pull"); + if (IS_ERR(info->reg_pull)) + info->reg_pull = NULL; + + info->reg_pullen = aml_map_resource(info, np, "pull-enable"); + if (IS_ERR(info->reg_pullen)) + info->reg_pullen = NULL; + + info->reg_ds = aml_map_resource(info, np, "ds"); + if (IS_ERR(info->reg_ds)) { + dev_dbg(info->dev, "ds registers not found - skipping\n"); + info->reg_ds = NULL; + } + + aml_pctl_reg_extra(info); + + info->data = (struct aml_pctl_data *)of_device_get_match_data(dev); + + pctl_desc->npins = aml_count_pins(np); + + pdesc = devm_kcalloc(dev, pctl_desc->npins, sizeof(*pdesc), GFP_KERNEL); + if (!pdesc) + return -ENOMEM; + + pctl_desc->pins = pdesc; + + bank = 0; + for_each_child_of_node_scoped(np, child) { + if (of_property_read_bool(child, "gpio-controller")) { + const char *bank_name = NULL; + char **pin_names; + + ret = aml_gpiolib_register_bank(info, bank, child, k); + if (ret) + return ret; + + k = info->banks[bank].pin_base; + bank_name = info->banks[bank].gpio_chip.label; + + pin_names = devm_kasprintf_strarray(dev, bank_name, + info->banks[bank].gpio_chip.ngpio); + if (IS_ERR(pin_names)) + return PTR_ERR(pin_names); + + for (j = 0; j < info->banks[bank].gpio_chip.ngpio; j++, k++) { + pdesc->number = k; + pdesc->name = pin_names[j]; + pdesc++; + } + bank++; + } else { + ret = aml_pctl_parse_functions(child, info, + i++, &grp_index); + if (ret) + return ret; + } + } + + return 0; +} + +static int aml_pctl_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct aml_pinctrl *info; + struct pinctrl_desc *pctl_desc; + int ret, i; + + pctl_desc = devm_kzalloc(dev, sizeof(*pctl_desc), GFP_KERNEL); + if (!pctl_desc) + return -ENOMEM; + + info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL); + if (!info) + return -ENOMEM; + + info->dev = dev; + platform_set_drvdata(pdev, info); + ret = aml_pctl_probe_dt(pdev, pctl_desc, info); + if (ret) + return ret; + + pctl_desc->owner = THIS_MODULE; + pctl_desc->pctlops = &aml_pctrl_ops; + pctl_desc->pmxops = &aml_pmx_ops; + pctl_desc->confops = &aml_pinconf_ops; + pctl_desc->name = dev_name(dev); + + info->pctl = devm_pinctrl_register(dev, pctl_desc, info); + if (IS_ERR(info->pctl)) { + dev_err(dev, "Failed pinctrl registration\n"); + ret = PTR_ERR(info->pctl); + goto error; + } + + for (i = 0; i < info->nbanks; i++) { + ret = gpiochip_add_pin_range(&info->banks[i].gpio_chip, dev_name(dev), 0, + info->banks[i].pin_base, + info->banks[i].gpio_chip.ngpio); + if (ret < 0) { + dev_err(dev, "Failed add pin range\n"); + goto error; + } + } + + return 0; +error: + for (i = 0; i < info->nbanks; i++) + gpiochip_remove(&info->banks[i].gpio_chip); + return ret; +} + +static const struct of_device_id aml_pctl_of_match[] = { + { .compatible = "amlogic,pinctrl", }, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(of, aml_pctl_dt_match); + +static struct platform_driver aml_pctl_driver = { + .driver = { + .name = "amlogic-pinctrl", + .of_match_table = aml_pctl_of_match, + }, + .probe = aml_pctl_probe, +}; +module_platform_driver(aml_pctl_driver); + +MODULE_AUTHOR("Xianwei Zhao <xianwei.zhao@amlogic.com>"); +MODULE_DESCRIPTION("Pin controller and GPIO driver for Amlogic SoC"); +MODULE_LICENSE("Dual BSD/GPL"); -- 2.37.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH RFC 2/3] pinctrl: Add driver support for Amlogic SoCs 2024-12-11 6:47 ` [PATCH RFC 2/3] pinctrl: Add driver " Xianwei Zhao via B4 Relay @ 2024-12-17 14:49 ` Linus Walleij 2024-12-17 14:53 ` Neil Armstrong 2024-12-18 9:37 ` Xianwei Zhao 0 siblings, 2 replies; 14+ messages in thread From: Linus Walleij @ 2024-12-17 14:49 UTC (permalink / raw) To: xianwei.zhao Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Neil Armstrong, Kevin Hilman, Jerome Brunet, Martin Blumenstingl, linux-gpio, devicetree, linux-kernel, linux-arm-kernel, linux-amlogic Hi Xianwei, thanks for your patch! On Wed, Dec 11, 2024 at 7:48 AM Xianwei Zhao via B4 Relay <devnull+xianwei.zhao.amlogic.com@kernel.org> wrote: > From: Xianwei Zhao <xianwei.zhao@amlogic.com> > > Add a new pinctrl driver for Amlogic SoCs. All future Amlogic > SoCs pinctrl drives use this, such A4, A5, S6, S7 etc. To support > new Amlogic SoCs, only need to add the corresponding dts file. > > Signed-off-by: Xianwei Zhao <xianwei.zhao@amlogic.com> First: are we sure these new SoCs have nothing in common with sunxi? Because then the sunxi code should be reused. In any way I recommend: - Renaming drivers/pinctrl/sunxi to drivers/pinctrl/amlogic so we keep this sorted by actual vendor, sunxi is apparently yours (AMlogic:s) isn't it? - Also fix MAINTAINERS accordingly. - Add new driver under drivers/pinctrl/amlogic - Do not change the Kconfig symbols for sunxi and we should be fine. > +static int aml_dt_node_to_map(struct pinctrl_dev *pctldev, > + struct device_node *np, > + struct pinctrl_map **map, > + unsigned int *num_maps) > +{ > + struct aml_pinctrl *info = pinctrl_dev_get_drvdata(pctldev); > + const struct aml_pctl_group *grp; > + struct device *dev = info->dev; > + struct pinctrl_map *new_map; > + struct device_node *parent; > + int map_num, i; > + > + grp = aml_pctl_find_group_by_name(info, np->name); > + if (!grp) { > + dev_err(dev, "unable to find group for node %pOFn\n", np); > + return -EINVAL; > + } > + > + if (grp->num_configs) > + map_num = grp->npins + 1; > + else > + map_num = 1; > + new_map = devm_kcalloc(dev, map_num, sizeof(*new_map), GFP_KERNEL); > + if (!new_map) > + return -ENOMEM; > + > + parent = of_get_parent(np); > + if (!parent) { > + devm_kfree(dev, new_map); > + return -EINVAL; > + } > + > + *map = new_map; > + *num_maps = map_num; > + new_map[0].type = PIN_MAP_TYPE_MUX_GROUP; > + new_map[0].data.mux.function = parent->name; > + new_map[0].data.mux.group = np->name; > + of_node_put(parent); > + > + if (grp->num_configs) { > + new_map++; > + for (i = 0; i < grp->npins; i++) { > + new_map[i].type = PIN_MAP_TYPE_CONFIGS_PIN; > + new_map[i].data.configs.group_or_pin = > + pin_get_name(pctldev, grp->pins[i]); > + new_map[i].data.configs.configs = grp->configs; > + new_map[i].data.configs.num_configs = grp->num_configs; > + } > + } > + > + dev_info(dev, "maps: function %s group %s num %d\n", > + (*map)->data.mux.function, grp->name, map_num); > + > + return 0; > +} > + > +static void aml_dt_free_map(struct pinctrl_dev *pctldev, > + struct pinctrl_map *map, unsigned int num_maps) > +{ > +} > + > +static void aml_pin_dbg_show(struct pinctrl_dev *pcdev, struct seq_file *s, > + unsigned int offset) > +{ > + seq_printf(s, " %s", dev_name(pcdev->dev)); > +} > + > +static const struct pinctrl_ops aml_pctrl_ops = { > + .get_groups_count = aml_get_groups_count, > + .get_group_name = aml_get_group_name, > + .get_group_pins = aml_get_group_pins, > + .dt_node_to_map = aml_dt_node_to_map, > + .dt_free_map = aml_dt_free_map, > + .pin_dbg_show = aml_pin_dbg_show, > +}; > + > +static int aml_pctl_dt_calculate_pin(struct aml_pinctrl *info, > + unsigned int bank_idx, unsigned int offset) > +{ > + struct aml_gpio_bank *bank; > + int retval = -EINVAL; > + int i; > + > + for (i = 0; i < info->nbanks; i++) { > + bank = &info->banks[i]; > + if (bank->bank_idx == bank_idx) { > + if (offset < bank->gpio_chip.ngpio) > + retval = bank->pin_base + offset; > + break; > + } > + } > + if (retval == -EINVAL) > + dev_err(info->dev, "pin [bank:%d, offset:%d] is not present\n", bank_idx, offset); > + > + return retval; > +} > + > +static int aml_pctl_dt_parse_groups(struct device_node *np, > + struct aml_pctl_group *grp, > + struct aml_pinctrl *info, int idx) > +{ > + struct device *dev = info->dev; > + struct aml_pinconf *conf; > + struct property *of_pins; > + unsigned int bank_idx; > + unsigned int offset, npins; > + int i = 0; > + int ret; > + > + of_pins = of_find_property(np, "pinmux", NULL); > + if (!of_pins) { > + dev_info(dev, "Missing pinmux property\n"); > + return -ENOENT; > + } > + > + npins = of_pins->length / sizeof(u32); > + grp->npins = npins; > + grp->name = np->name; > + grp->pins = devm_kcalloc(dev, npins, sizeof(*grp->pins), GFP_KERNEL); > + grp->pin_conf = devm_kcalloc(dev, npins, sizeof(*grp->pin_conf), GFP_KERNEL); > + > + if (!grp->pins || !grp->pin_conf) > + return -ENOMEM; > + > + ret = pinconf_generic_parse_dt_config(np, info->pctl, &grp->configs, > + &grp->num_configs); But can't you just move this code around? grp->num_configs give the number of configs, so why do you have to go and look up pinmux above, can't you just use grp->num_configs instead of of_pins and npins above? > +static int aml_pctl_parse_functions(struct device_node *np, > + struct aml_pinctrl *info, u32 index, > + int *grp_index) > +{ > + struct device *dev = info->dev; > + struct aml_pmx_func *func; > + struct aml_pctl_group *grp; > + int ret, i; > + > + func = &info->functions[index]; > + func->name = np->name; > + func->ngroups = of_get_child_count(np); > + if (func->ngroups == 0) > + return dev_err_probe(dev, -EINVAL, "No groups defined\n"); > + > + func->groups = devm_kcalloc(dev, func->ngroups, sizeof(*func->groups), GFP_KERNEL); > + if (!func->groups) > + return -ENOMEM; > + > + i = 0; > + for_each_child_of_node_scoped(np, child) { > + func->groups[i] = child->name; > + grp = &info->groups[*grp_index]; > + *grp_index += 1; > + ret = aml_pctl_dt_parse_groups(child, grp, info, i++); > + if (ret) > + return ret; > + } > + dev_info(dev, "Function[%d\t name:%s,\tgroups:%d]\n", index, func->name, func->ngroups); > + > + return 0; > +} > + > +static u32 aml_bank_pins(struct device_node *np) > +{ > + u32 value; > + > + if (of_property_read_u32(np, "npins", &value) < 0) > + return 0; > + else > + return value; > +} > + > +static u32 aml_bank_reg_gpio_offset(struct device_node *np) > +{ > + u32 value; > + > + if (of_property_read_u32(np, "reg-gpio-offset", &value) < 0) > + return 0; > + else > + return value; > +} > + > +static u32 aml_bank_reg_mux_offset(struct device_node *np) > +{ > + u32 value; > + > + if (of_property_read_u32(np, "reg-mux-offset", &value) < 0) > + return 0; > + else > + return value; > +} > + > +static u32 aml_bank_bit_mux_offset(struct device_node *np) > +{ > + u32 value; > + > + if (of_property_read_u32(np, "bit-mux-offset", &value) < 0) > + return 0; > + else > + return value; > +} > + > +static u32 aml_bank_index(struct device_node *np) > +{ > + u32 value; > + > + if (of_property_read_u32(np, "bank-index", &value) < 0) > + return 0; > + else > + return value; > +} Do we really need helpers for all of this? Can't you just open code it, at least if it's just used in one place? > +static unsigned int aml_count_pins(struct device_node *np) > +{ > + struct device_node *child; > + unsigned int pins = 0; > + > + for_each_child_of_node(np, child) { > + if (of_property_read_bool(child, "gpio-controller")) > + pins += aml_bank_pins(child); > + } > + > + return pins; > +} > + > +static void aml_pctl_dt_child_count(struct aml_pinctrl *info, > + struct device_node *np) > +{ > + struct device_node *child; > + > + for_each_child_of_node(np, child) { > + if (of_property_read_bool(child, "gpio-controller")) { > + info->nbanks++; > + } else { > + info->nfunctions++; > + info->ngroups += of_get_child_count(child); > + } > + } > +} This looks like a weird dependency between gpio chips and pins that I don't quite understand. Some comments and references to the bindings will be needed so it is clear what is going on. > +static struct regmap *aml_map_resource(struct aml_pinctrl *info, > + struct device_node *node, char *name) > +{ > + struct resource res; > + void __iomem *base; > + int i; > + > + i = of_property_match_string(node, "reg-names", name); > + if (of_address_to_resource(node, i, &res)) > + return NULL; > + > + base = devm_ioremap_resource(info->dev, &res); > + if (IS_ERR(base)) > + return ERR_CAST(base); This looks like reimplementation of devm_platform_ioremap_resource_byname(), can't you just pass your platform device here? > +static int aml_pctl_probe_dt(struct platform_device *pdev, > + struct pinctrl_desc *pctl_desc, > + struct aml_pinctrl *info) Because there is clearly a platform device involved. I guess I will have more comments as the series progress, but this is a good starting point! Yours, Linus Walleij ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH RFC 2/3] pinctrl: Add driver support for Amlogic SoCs 2024-12-17 14:49 ` Linus Walleij @ 2024-12-17 14:53 ` Neil Armstrong 2024-12-18 9:37 ` Xianwei Zhao 1 sibling, 0 replies; 14+ messages in thread From: Neil Armstrong @ 2024-12-17 14:53 UTC (permalink / raw) To: Linus Walleij, xianwei.zhao Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Kevin Hilman, Jerome Brunet, Martin Blumenstingl, linux-gpio, devicetree, linux-kernel, linux-arm-kernel, linux-amlogic Hi, On 17/12/2024 15:49, Linus Walleij wrote: > Hi Xianwei, > > thanks for your patch! > > On Wed, Dec 11, 2024 at 7:48 AM Xianwei Zhao via B4 Relay > <devnull+xianwei.zhao.amlogic.com@kernel.org> wrote: > >> From: Xianwei Zhao <xianwei.zhao@amlogic.com> >> >> Add a new pinctrl driver for Amlogic SoCs. All future Amlogic >> SoCs pinctrl drives use this, such A4, A5, S6, S7 etc. To support >> new Amlogic SoCs, only need to add the corresponding dts file. >> >> Signed-off-by: Xianwei Zhao <xianwei.zhao@amlogic.com> > > First: are we sure these new SoCs have nothing in common > with sunxi? Because then the sunxi code should be reused. > > In any way I recommend: > > - Renaming drivers/pinctrl/sunxi to drivers/pinctrl/amlogic > so we keep this sorted by actual vendor, sunxi is apparently > yours (AMlogic:s) isn't it? Sunxi is Allwinner SoCs. Neil > > - Also fix MAINTAINERS accordingly. > > - Add new driver under drivers/pinctrl/amlogic > > - Do not change the Kconfig symbols for sunxi and > we should be fine. > >> +static int aml_dt_node_to_map(struct pinctrl_dev *pctldev, >> + struct device_node *np, >> + struct pinctrl_map **map, >> + unsigned int *num_maps) >> +{ >> + struct aml_pinctrl *info = pinctrl_dev_get_drvdata(pctldev); >> + const struct aml_pctl_group *grp; >> + struct device *dev = info->dev; >> + struct pinctrl_map *new_map; >> + struct device_node *parent; >> + int map_num, i; >> + >> + grp = aml_pctl_find_group_by_name(info, np->name); >> + if (!grp) { >> + dev_err(dev, "unable to find group for node %pOFn\n", np); >> + return -EINVAL; >> + } >> + >> + if (grp->num_configs) >> + map_num = grp->npins + 1; >> + else >> + map_num = 1; >> + new_map = devm_kcalloc(dev, map_num, sizeof(*new_map), GFP_KERNEL); >> + if (!new_map) >> + return -ENOMEM; >> + >> + parent = of_get_parent(np); >> + if (!parent) { >> + devm_kfree(dev, new_map); >> + return -EINVAL; >> + } >> + >> + *map = new_map; >> + *num_maps = map_num; >> + new_map[0].type = PIN_MAP_TYPE_MUX_GROUP; >> + new_map[0].data.mux.function = parent->name; >> + new_map[0].data.mux.group = np->name; >> + of_node_put(parent); >> + >> + if (grp->num_configs) { >> + new_map++; >> + for (i = 0; i < grp->npins; i++) { >> + new_map[i].type = PIN_MAP_TYPE_CONFIGS_PIN; >> + new_map[i].data.configs.group_or_pin = >> + pin_get_name(pctldev, grp->pins[i]); >> + new_map[i].data.configs.configs = grp->configs; >> + new_map[i].data.configs.num_configs = grp->num_configs; >> + } >> + } >> + >> + dev_info(dev, "maps: function %s group %s num %d\n", >> + (*map)->data.mux.function, grp->name, map_num); >> + >> + return 0; >> +} >> + >> +static void aml_dt_free_map(struct pinctrl_dev *pctldev, >> + struct pinctrl_map *map, unsigned int num_maps) >> +{ >> +} >> + >> +static void aml_pin_dbg_show(struct pinctrl_dev *pcdev, struct seq_file *s, >> + unsigned int offset) >> +{ >> + seq_printf(s, " %s", dev_name(pcdev->dev)); >> +} >> + >> +static const struct pinctrl_ops aml_pctrl_ops = { >> + .get_groups_count = aml_get_groups_count, >> + .get_group_name = aml_get_group_name, >> + .get_group_pins = aml_get_group_pins, >> + .dt_node_to_map = aml_dt_node_to_map, >> + .dt_free_map = aml_dt_free_map, >> + .pin_dbg_show = aml_pin_dbg_show, >> +}; >> + >> +static int aml_pctl_dt_calculate_pin(struct aml_pinctrl *info, >> + unsigned int bank_idx, unsigned int offset) >> +{ >> + struct aml_gpio_bank *bank; >> + int retval = -EINVAL; >> + int i; >> + >> + for (i = 0; i < info->nbanks; i++) { >> + bank = &info->banks[i]; >> + if (bank->bank_idx == bank_idx) { >> + if (offset < bank->gpio_chip.ngpio) >> + retval = bank->pin_base + offset; >> + break; >> + } >> + } >> + if (retval == -EINVAL) >> + dev_err(info->dev, "pin [bank:%d, offset:%d] is not present\n", bank_idx, offset); >> + >> + return retval; >> +} >> + >> +static int aml_pctl_dt_parse_groups(struct device_node *np, >> + struct aml_pctl_group *grp, >> + struct aml_pinctrl *info, int idx) >> +{ >> + struct device *dev = info->dev; >> + struct aml_pinconf *conf; >> + struct property *of_pins; >> + unsigned int bank_idx; >> + unsigned int offset, npins; >> + int i = 0; >> + int ret; >> + >> + of_pins = of_find_property(np, "pinmux", NULL); >> + if (!of_pins) { >> + dev_info(dev, "Missing pinmux property\n"); >> + return -ENOENT; >> + } >> + >> + npins = of_pins->length / sizeof(u32); >> + grp->npins = npins; >> + grp->name = np->name; >> + grp->pins = devm_kcalloc(dev, npins, sizeof(*grp->pins), GFP_KERNEL); >> + grp->pin_conf = devm_kcalloc(dev, npins, sizeof(*grp->pin_conf), GFP_KERNEL); >> + >> + if (!grp->pins || !grp->pin_conf) >> + return -ENOMEM; >> + >> + ret = pinconf_generic_parse_dt_config(np, info->pctl, &grp->configs, >> + &grp->num_configs); > > But can't you just move this code around? grp->num_configs give the > number of configs, so why do you have to go and look up pinmux > above, can't you just use grp->num_configs instead of of_pins > and npins above? > >> +static int aml_pctl_parse_functions(struct device_node *np, >> + struct aml_pinctrl *info, u32 index, >> + int *grp_index) >> +{ >> + struct device *dev = info->dev; >> + struct aml_pmx_func *func; >> + struct aml_pctl_group *grp; >> + int ret, i; >> + >> + func = &info->functions[index]; >> + func->name = np->name; >> + func->ngroups = of_get_child_count(np); >> + if (func->ngroups == 0) >> + return dev_err_probe(dev, -EINVAL, "No groups defined\n"); >> + >> + func->groups = devm_kcalloc(dev, func->ngroups, sizeof(*func->groups), GFP_KERNEL); >> + if (!func->groups) >> + return -ENOMEM; >> + >> + i = 0; >> + for_each_child_of_node_scoped(np, child) { >> + func->groups[i] = child->name; >> + grp = &info->groups[*grp_index]; >> + *grp_index += 1; >> + ret = aml_pctl_dt_parse_groups(child, grp, info, i++); >> + if (ret) >> + return ret; >> + } >> + dev_info(dev, "Function[%d\t name:%s,\tgroups:%d]\n", index, func->name, func->ngroups); >> + >> + return 0; >> +} >> + >> +static u32 aml_bank_pins(struct device_node *np) >> +{ >> + u32 value; >> + >> + if (of_property_read_u32(np, "npins", &value) < 0) >> + return 0; >> + else >> + return value; >> +} >> + >> +static u32 aml_bank_reg_gpio_offset(struct device_node *np) >> +{ >> + u32 value; >> + >> + if (of_property_read_u32(np, "reg-gpio-offset", &value) < 0) >> + return 0; >> + else >> + return value; >> +} >> + >> +static u32 aml_bank_reg_mux_offset(struct device_node *np) >> +{ >> + u32 value; >> + >> + if (of_property_read_u32(np, "reg-mux-offset", &value) < 0) >> + return 0; >> + else >> + return value; >> +} >> + >> +static u32 aml_bank_bit_mux_offset(struct device_node *np) >> +{ >> + u32 value; >> + >> + if (of_property_read_u32(np, "bit-mux-offset", &value) < 0) >> + return 0; >> + else >> + return value; >> +} >> + >> +static u32 aml_bank_index(struct device_node *np) >> +{ >> + u32 value; >> + >> + if (of_property_read_u32(np, "bank-index", &value) < 0) >> + return 0; >> + else >> + return value; >> +} > > Do we really need helpers for all of this? Can't you just > open code it, at least if it's just used in one place? > >> +static unsigned int aml_count_pins(struct device_node *np) >> +{ >> + struct device_node *child; >> + unsigned int pins = 0; >> + >> + for_each_child_of_node(np, child) { >> + if (of_property_read_bool(child, "gpio-controller")) >> + pins += aml_bank_pins(child); >> + } >> + >> + return pins; >> +} >> + >> +static void aml_pctl_dt_child_count(struct aml_pinctrl *info, >> + struct device_node *np) >> +{ >> + struct device_node *child; >> + >> + for_each_child_of_node(np, child) { >> + if (of_property_read_bool(child, "gpio-controller")) { >> + info->nbanks++; >> + } else { >> + info->nfunctions++; >> + info->ngroups += of_get_child_count(child); >> + } >> + } >> +} > > This looks like a weird dependency between gpio chips and > pins that I don't quite understand. Some comments and > references to the bindings will be needed so it is clear > what is going on. > >> +static struct regmap *aml_map_resource(struct aml_pinctrl *info, >> + struct device_node *node, char *name) >> +{ >> + struct resource res; >> + void __iomem *base; >> + int i; >> + >> + i = of_property_match_string(node, "reg-names", name); >> + if (of_address_to_resource(node, i, &res)) >> + return NULL; >> + >> + base = devm_ioremap_resource(info->dev, &res); >> + if (IS_ERR(base)) >> + return ERR_CAST(base); > > This looks like reimplementation of > devm_platform_ioremap_resource_byname(), can't you just > pass your platform device here? > >> +static int aml_pctl_probe_dt(struct platform_device *pdev, >> + struct pinctrl_desc *pctl_desc, >> + struct aml_pinctrl *info) > > Because there is clearly a platform device involved. > > I guess I will have more comments as the series progress, but this > is a good starting point! > > Yours, > Linus Walleij ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH RFC 2/3] pinctrl: Add driver support for Amlogic SoCs 2024-12-17 14:49 ` Linus Walleij 2024-12-17 14:53 ` Neil Armstrong @ 2024-12-18 9:37 ` Xianwei Zhao 2024-12-22 9:08 ` Linus Walleij 1 sibling, 1 reply; 14+ messages in thread From: Xianwei Zhao @ 2024-12-18 9:37 UTC (permalink / raw) To: Linus Walleij Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Neil Armstrong, Kevin Hilman, Jerome Brunet, Martin Blumenstingl, linux-gpio, devicetree, linux-kernel, linux-arm-kernel, linux-amlogic Hi Linus, Thanks for your reply. On 2024/12/17 22:49, Linus Walleij wrote: > [ EXTERNAL EMAIL ] > > Hi Xianwei, > > thanks for your patch! > > On Wed, Dec 11, 2024 at 7:48 AM Xianwei Zhao via B4 Relay > <devnull+xianwei.zhao.amlogic.com@kernel.org> wrote: > >> From: Xianwei Zhao <xianwei.zhao@amlogic.com> >> >> Add a new pinctrl driver for Amlogic SoCs. All future Amlogic >> SoCs pinctrl drives use this, such A4, A5, S6, S7 etc. To support >> new Amlogic SoCs, only need to add the corresponding dts file. >> >> Signed-off-by: Xianwei Zhao <xianwei.zhao@amlogic.com> > > First: are we sure these new SoCs have nothing in common > with sunxi? Because then the sunxi code should be reused. > > In any way I recommend: > > - Renaming drivers/pinctrl/sunxi to drivers/pinctrl/amlogic > so we keep this sorted by actual vendor, sunxi is apparently > yours (AMlogic:s) isn't it? > It isn't. Sunxi is Allwinner SoCs. > - Also fix MAINTAINERS accordingly. > Sending the official version will be synchronized. > - Add new driver under drivers/pinctrl/amlogic > > - Do not change the Kconfig symbols for sunxi and > we should be fine. > >> +static int aml_dt_node_to_map(struct pinctrl_dev *pctldev, >> + struct device_node *np, >> + struct pinctrl_map **map, >> + unsigned int *num_maps) >> +{ >> + struct aml_pinctrl *info = pinctrl_dev_get_drvdata(pctldev); >> + const struct aml_pctl_group *grp; >> + struct device *dev = info->dev; >> + struct pinctrl_map *new_map; >> + struct device_node *parent; >> + int map_num, i; >> + >> + grp = aml_pctl_find_group_by_name(info, np->name); >> + if (!grp) { >> + dev_err(dev, "unable to find group for node %pOFn\n", np); >> + return -EINVAL; >> + } >> + >> + if (grp->num_configs) >> + map_num = grp->npins + 1; >> + else >> + map_num = 1; >> + new_map = devm_kcalloc(dev, map_num, sizeof(*new_map), GFP_KERNEL); >> + if (!new_map) >> + return -ENOMEM; >> + >> + parent = of_get_parent(np); >> + if (!parent) { >> + devm_kfree(dev, new_map); >> + return -EINVAL; >> + } >> + >> + *map = new_map; >> + *num_maps = map_num; >> + new_map[0].type = PIN_MAP_TYPE_MUX_GROUP; >> + new_map[0].data.mux.function = parent->name; >> + new_map[0].data.mux.group = np->name; >> + of_node_put(parent); >> + >> + if (grp->num_configs) { >> + new_map++; >> + for (i = 0; i < grp->npins; i++) { >> + new_map[i].type = PIN_MAP_TYPE_CONFIGS_PIN; >> + new_map[i].data.configs.group_or_pin = >> + pin_get_name(pctldev, grp->pins[i]); >> + new_map[i].data.configs.configs = grp->configs; >> + new_map[i].data.configs.num_configs = grp->num_configs; >> + } >> + } >> + >> + dev_info(dev, "maps: function %s group %s num %d\n", >> + (*map)->data.mux.function, grp->name, map_num); >> + >> + return 0; >> +} >> + >> +static void aml_dt_free_map(struct pinctrl_dev *pctldev, >> + struct pinctrl_map *map, unsigned int num_maps) >> +{ >> +} >> + >> +static void aml_pin_dbg_show(struct pinctrl_dev *pcdev, struct seq_file *s, >> + unsigned int offset) >> +{ >> + seq_printf(s, " %s", dev_name(pcdev->dev)); >> +} >> + >> +static const struct pinctrl_ops aml_pctrl_ops = { >> + .get_groups_count = aml_get_groups_count, >> + .get_group_name = aml_get_group_name, >> + .get_group_pins = aml_get_group_pins, >> + .dt_node_to_map = aml_dt_node_to_map, >> + .dt_free_map = aml_dt_free_map, >> + .pin_dbg_show = aml_pin_dbg_show, >> +}; >> + >> +static int aml_pctl_dt_calculate_pin(struct aml_pinctrl *info, >> + unsigned int bank_idx, unsigned int offset) >> +{ >> + struct aml_gpio_bank *bank; >> + int retval = -EINVAL; >> + int i; >> + >> + for (i = 0; i < info->nbanks; i++) { >> + bank = &info->banks[i]; >> + if (bank->bank_idx == bank_idx) { >> + if (offset < bank->gpio_chip.ngpio) >> + retval = bank->pin_base + offset; >> + break; >> + } >> + } >> + if (retval == -EINVAL) >> + dev_err(info->dev, "pin [bank:%d, offset:%d] is not present\n", bank_idx, offset); >> + >> + return retval; >> +} >> + >> +static int aml_pctl_dt_parse_groups(struct device_node *np, >> + struct aml_pctl_group *grp, >> + struct aml_pinctrl *info, int idx) >> +{ >> + struct device *dev = info->dev; >> + struct aml_pinconf *conf; >> + struct property *of_pins; >> + unsigned int bank_idx; >> + unsigned int offset, npins; >> + int i = 0; >> + int ret; >> + >> + of_pins = of_find_property(np, "pinmux", NULL); >> + if (!of_pins) { >> + dev_info(dev, "Missing pinmux property\n"); >> + return -ENOENT; >> + } >> + >> + npins = of_pins->length / sizeof(u32); >> + grp->npins = npins; >> + grp->name = np->name; >> + grp->pins = devm_kcalloc(dev, npins, sizeof(*grp->pins), GFP_KERNEL); >> + grp->pin_conf = devm_kcalloc(dev, npins, sizeof(*grp->pin_conf), GFP_KERNEL); >> + >> + if (!grp->pins || !grp->pin_conf) >> + return -ENOMEM; >> + >> + ret = pinconf_generic_parse_dt_config(np, info->pctl, &grp->configs, >> + &grp->num_configs); > > But can't you just move this code around? grp->num_configs give the > number of configs, so why do you have to go and look up pinmux > above, can't you just use grp->num_configs instead of of_pins > and npins above? > They are different. The of_pins(grp->npins) specifies the mux values for pin-mux register and pin index in pinctrl. It can include multiple pins in groups. The grp->configs and grp->num_configs specify the configuration information for all pins of this groups(such as bias-pull-up, drive-strength-microamp) uart-d-pins2{ pinmux= <AML_PINMUX(AMLOGIC_GPIO_T, 7, AF2)>, <AML_PINMUX(AMLOGIC_GPIO_T, 8, AF2)>, <AML_PINMUX(AMLOGIC_GPIO_T, 9, AF2)>, <AML_PINMUX(AMLOGIC_GPIO_T, 10, AF2)>; bias-pull-up; drive-strength-microamp = <4000>; }; >> +static int aml_pctl_parse_functions(struct device_node *np, >> + struct aml_pinctrl *info, u32 index, >> + int *grp_index) >> +{ >> + struct device *dev = info->dev; >> + struct aml_pmx_func *func; >> + struct aml_pctl_group *grp; >> + int ret, i; >> + >> + func = &info->functions[index]; >> + func->name = np->name; >> + func->ngroups = of_get_child_count(np); >> + if (func->ngroups == 0) >> + return dev_err_probe(dev, -EINVAL, "No groups defined\n"); >> + >> + func->groups = devm_kcalloc(dev, func->ngroups, sizeof(*func->groups), GFP_KERNEL); >> + if (!func->groups) >> + return -ENOMEM; >> + >> + i = 0; >> + for_each_child_of_node_scoped(np, child) { >> + func->groups[i] = child->name; >> + grp = &info->groups[*grp_index]; >> + *grp_index += 1; >> + ret = aml_pctl_dt_parse_groups(child, grp, info, i++); >> + if (ret) >> + return ret; >> + } >> + dev_info(dev, "Function[%d\t name:%s,\tgroups:%d]\n", index, func->name, func->ngroups); >> + >> + return 0; >> +} >> + >> +static u32 aml_bank_pins(struct device_node *np) >> +{ >> + u32 value; >> + >> + if (of_property_read_u32(np, "npins", &value) < 0) >> + return 0; >> + else >> + return value; >> +} >> + >> +static u32 aml_bank_reg_gpio_offset(struct device_node *np) >> +{ >> + u32 value; >> + >> + if (of_property_read_u32(np, "reg-gpio-offset", &value) < 0) >> + return 0; >> + else >> + return value; >> +} >> + >> +static u32 aml_bank_reg_mux_offset(struct device_node *np) >> +{ >> + u32 value; >> + >> + if (of_property_read_u32(np, "reg-mux-offset", &value) < 0) >> + return 0; >> + else >> + return value; >> +} >> + >> +static u32 aml_bank_bit_mux_offset(struct device_node *np) >> +{ >> + u32 value; >> + >> + if (of_property_read_u32(np, "bit-mux-offset", &value) < 0) >> + return 0; >> + else >> + return value; >> +} >> + >> +static u32 aml_bank_index(struct device_node *np) >> +{ >> + u32 value; >> + >> + if (of_property_read_u32(np, "bank-index", &value) < 0) >> + return 0; >> + else >> + return value; >> +} > > Do we really need helpers for all of this? Can't you just > open code it, at least if it's just used in one place? > I will delete this function, I will move the logic to where it was called. >> +static unsigned int aml_count_pins(struct device_node *np) >> +{ >> + struct device_node *child; >> + unsigned int pins = 0; >> + >> + for_each_child_of_node(np, child) { >> + if (of_property_read_bool(child, "gpio-controller")) >> + pins += aml_bank_pins(child); >> + } >> + >> + return pins; >> +} >> + >> +static void aml_pctl_dt_child_count(struct aml_pinctrl *info, >> + struct device_node *np) >> +{ >> + struct device_node *child; >> + >> + for_each_child_of_node(np, child) { >> + if (of_property_read_bool(child, "gpio-controller")) { >> + info->nbanks++; >> + } else { >> + info->nfunctions++; >> + info->ngroups += of_get_child_count(child); >> + } >> + } >> +} > > This looks like a weird dependency between gpio chips and > pins that I don't quite understand. Some comments and > references to the bindings will be needed so it is clear > what is going on. > A pinctrl device contains two types of nodes. The one named GPIO bank which includes "gpio-controller" property. The other one named function which includes one or more pin groups. The pin group include pinmux property(pin index in pinctrl dev,and mux vlaue in mux reg) and pin configuration properties. I will add comment in next verison. >> +static struct regmap *aml_map_resource(struct aml_pinctrl *info, >> + struct device_node *node, char *name) >> +{ >> + struct resource res; >> + void __iomem *base; >> + int i; >> + >> + i = of_property_match_string(node, "reg-names", name); >> + if (of_address_to_resource(node, i, &res)) >> + return NULL; >> + >> + base = devm_ioremap_resource(info->dev, &res); >> + if (IS_ERR(base)) >> + return ERR_CAST(base); > > This looks like reimplementation of > devm_platform_ioremap_resource_byname(), can't you just > pass your platform device here? > I will fix it. >> +static int aml_pctl_probe_dt(struct platform_device *pdev, >> + struct pinctrl_desc *pctl_desc, >> + struct aml_pinctrl *info) > > Because there is clearly a platform device involved. > > I guess I will have more comments as the series progress, but this > is a good starting point! > > Yours, > Linus Walleij ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH RFC 2/3] pinctrl: Add driver support for Amlogic SoCs 2024-12-18 9:37 ` Xianwei Zhao @ 2024-12-22 9:08 ` Linus Walleij 2024-12-26 7:49 ` Xianwei Zhao 2024-12-27 17:19 ` Linus Walleij 0 siblings, 2 replies; 14+ messages in thread From: Linus Walleij @ 2024-12-22 9:08 UTC (permalink / raw) To: Xianwei Zhao Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Neil Armstrong, Kevin Hilman, Jerome Brunet, Martin Blumenstingl, linux-gpio, devicetree, linux-kernel, linux-arm-kernel, linux-amlogic Hi Xianwei! On Wed, Dec 18, 2024 at 10:37 AM Xianwei Zhao <xianwei.zhao@amlogic.com> wrote: > [Me] > > In any way I recommend: > > > > - Renaming drivers/pinctrl/sunxi to drivers/pinctrl/amlogic > > so we keep this sorted by actual vendor, sunxi is apparently > > yours (AMlogic:s) isn't it? > > > > It isn't. Sunxi is Allwinner SoCs. My apologies. I mixed it up completely. :( What do you think of the idea of a separate drivers/pinctrl/amlogic directory though? I think there are already quite a few amlogic SoCs that need to be supported and more will come. > >> + ret = pinconf_generic_parse_dt_config(np, info->pctl, &grp->configs, > >> + &grp->num_configs); > > > > But can't you just move this code around? grp->num_configs give the > > number of configs, so why do you have to go and look up pinmux > > above, can't you just use grp->num_configs instead of of_pins > > and npins above? > > > They are different. > The of_pins(grp->npins) specifies the mux values for pin-mux register > and pin index in pinctrl. It can include multiple pins in groups. > > The grp->configs and grp->num_configs specify the configuration > information for all pins of this groups(such as bias-pull-up, > drive-strength-microamp) > > uart-d-pins2{ > pinmux= <AML_PINMUX(AMLOGIC_GPIO_T, 7, AF2)>, > <AML_PINMUX(AMLOGIC_GPIO_T, 8, AF2)>, > <AML_PINMUX(AMLOGIC_GPIO_T, 9, AF2)>, > <AML_PINMUX(AMLOGIC_GPIO_T, 10, AF2)>; > bias-pull-up; > drive-strength-microamp = <4000>; > }; OK I get it ... I think. It's nice that you combine muxing and pin config into the same node like this, it's very readable. Think about if you even want to add generic helpers for this in the generic code. > >> +static void aml_pctl_dt_child_count(struct aml_pinctrl *info, > >> + struct device_node *np) > >> +{ > >> + struct device_node *child; > >> + > >> + for_each_child_of_node(np, child) { > >> + if (of_property_read_bool(child, "gpio-controller")) { > >> + info->nbanks++; > >> + } else { > >> + info->nfunctions++; > >> + info->ngroups += of_get_child_count(child); > >> + } > >> + } > >> +} > > > > This looks like a weird dependency between gpio chips and > > pins that I don't quite understand. Some comments and > > references to the bindings will be needed so it is clear > > what is going on. > > > > A pinctrl device contains two types of nodes. The one named GPIO bank > which includes "gpio-controller" property. The other one named function > which includes one or more pin groups. > The pin group include pinmux property(pin index in pinctrl dev,and mux > vlaue in mux reg) and pin configuration properties. OK I guess the binding patch explains why you need several separate gpio controller "bank" nodes instead of just the base node being one for all of the pins (which is the most common). In a way I like it because it often helps to divide up GPIOs by bank. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH RFC 2/3] pinctrl: Add driver support for Amlogic SoCs 2024-12-22 9:08 ` Linus Walleij @ 2024-12-26 7:49 ` Xianwei Zhao 2024-12-27 17:19 ` Linus Walleij 1 sibling, 0 replies; 14+ messages in thread From: Xianwei Zhao @ 2024-12-26 7:49 UTC (permalink / raw) To: Linus Walleij Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Neil Armstrong, Kevin Hilman, Jerome Brunet, Martin Blumenstingl, linux-gpio, devicetree, linux-kernel, linux-arm-kernel, linux-amlogic Hi Linus, Thanks for your reply. On 2024/12/22 17:08, Linus Walleij wrote: > [ EXTERNAL EMAIL ] > > Hi Xianwei! > > On Wed, Dec 18, 2024 at 10:37 AM Xianwei Zhao <xianwei.zhao@amlogic.com> wrote: > >> [Me] >>> In any way I recommend: >>> >>> - Renaming drivers/pinctrl/sunxi to drivers/pinctrl/amlogic >>> so we keep this sorted by actual vendor, sunxi is apparently >>> yours (AMlogic:s) isn't it? >>> >> >> It isn't. Sunxi is Allwinner SoCs. > > My apologies. I mixed it up completely. :( > > What do you think of the idea of a separate drivers/pinctrl/amlogic directory > though? I think there are already quite a few amlogic SoCs that need > to be supported and more will come. > From the existing specifications of several subsequent chips, the support for the new chip does not require additional files, and there may be a little difference for special bank in the future chip, which can be solved by private architecture. >>>> + ret = pinconf_generic_parse_dt_config(np, info->pctl, &grp->configs, >>>> + &grp->num_configs); >>> >>> But can't you just move this code around? grp->num_configs give the >>> number of configs, so why do you have to go and look up pinmux >>> above, can't you just use grp->num_configs instead of of_pins >>> and npins above? >>> >> They are different. >> The of_pins(grp->npins) specifies the mux values for pin-mux register >> and pin index in pinctrl. It can include multiple pins in groups. >> >> The grp->configs and grp->num_configs specify the configuration >> information for all pins of this groups(such as bias-pull-up, >> drive-strength-microamp) >> >> uart-d-pins2{ >> pinmux= <AML_PINMUX(AMLOGIC_GPIO_T, 7, AF2)>, >> <AML_PINMUX(AMLOGIC_GPIO_T, 8, AF2)>, >> <AML_PINMUX(AMLOGIC_GPIO_T, 9, AF2)>, >> <AML_PINMUX(AMLOGIC_GPIO_T, 10, AF2)>; >> bias-pull-up; >> drive-strength-microamp = <4000>; >> }; > > OK I get it ... I think. It's nice that you combine muxing and pin config > into the same node like this, it's very readable. > > Think about if you even want to add generic helpers for this in > the generic code. > I will try to add API for pinmux property to pinconf-generic.c. >>>> +static void aml_pctl_dt_child_count(struct aml_pinctrl *info, >>>> + struct device_node *np) >>>> +{ >>>> + struct device_node *child; >>>> + >>>> + for_each_child_of_node(np, child) { >>>> + if (of_property_read_bool(child, "gpio-controller")) { >>>> + info->nbanks++; >>>> + } else { >>>> + info->nfunctions++; >>>> + info->ngroups += of_get_child_count(child); >>>> + } >>>> + } >>>> +} >>> >>> This looks like a weird dependency between gpio chips and >>> pins that I don't quite understand. Some comments and >>> references to the bindings will be needed so it is clear >>> what is going on. >>> >> >> A pinctrl device contains two types of nodes. The one named GPIO bank >> which includes "gpio-controller" property. The other one named function >> which includes one or more pin groups. >> The pin group include pinmux property(pin index in pinctrl dev,and mux >> vlaue in mux reg) and pin configuration properties. > > OK I guess the binding patch explains why you need several > separate gpio controller "bank" nodes instead of just the base > node being one for all of the pins (which is the most > common). In a way I like it because it often helps to divide > up GPIOs by bank. > > Yours, > Linus Walleij ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH RFC 2/3] pinctrl: Add driver support for Amlogic SoCs 2024-12-22 9:08 ` Linus Walleij 2024-12-26 7:49 ` Xianwei Zhao @ 2024-12-27 17:19 ` Linus Walleij 2024-12-27 21:01 ` Martin Blumenstingl 2024-12-31 8:26 ` Xianwei Zhao 1 sibling, 2 replies; 14+ messages in thread From: Linus Walleij @ 2024-12-27 17:19 UTC (permalink / raw) To: Xianwei Zhao, Huqiang Qin, Qianggui Song, Hyeonki Hong, Martin Blumenstingl, Beniamino Galvani, Carlo Caione Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Neil Armstrong, Kevin Hilman, Jerome Brunet, linux-gpio, devicetree, linux-kernel, linux-arm-kernel, linux-amlogic Newcomers, latest patch set: https://lore.kernel.org/linux-gpio/20241226-amlogic-pinctrl-v2-0-cdae42a67b76@amlogic.com/ I included some of the prior meson authors on the to line to see if their mail addresses still work and if they have some feedback on this. On Sun, Dec 22, 2024 at 10:08 AM Linus Walleij <linus.walleij@linaro.org> wrote: > > > - Renaming drivers/pinctrl/sunxi to drivers/pinctrl/amlogic > > > so we keep this sorted by actual vendor, sunxi is apparently > > > yours (AMlogic:s) isn't it? > > > > It isn't. Sunxi is Allwinner SoCs. > > My apologies. I mixed it up completely. :( But wait a minute. I see there is meson. And in the "meson" subdirectory there is stuff named "amlogic" ... $ ls -1 drivers/pinctrl/meson/ Kconfig Makefile pinctrl-amlogic-c3.c pinctrl-amlogic-t7.c pinctrl-meson8b.c pinctrl-meson8.c pinctrl-meson8-pmx.c pinctrl-meson8-pmx.h pinctrl-meson-a1.c pinctrl-meson-axg.c pinctrl-meson-axg-pmx.c pinctrl-meson-axg-pmx.h pinctrl-meson.c pinctrl-meson-g12a.c pinctrl-meson-gxbb.c pinctrl-meson-gxl.c pinctrl-meson.h pinctrl-meson-s4.c > What do you think of the idea of a separate drivers/pinctrl/amlogic directory > though? I think there are already quite a few amlogic SoCs that need > to be supported and more will come. So what about renaming the existing subdir "meson" to "amlogic" and put the driver there. Also I want to know if this driver and hardware shares anything with the existing drivers in that directory. It sometimes happen that developers start something from scratch despite the existence of prior art simply because of organizational issues, and we don't want that kind of situation to leak over to the kernel. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH RFC 2/3] pinctrl: Add driver support for Amlogic SoCs 2024-12-27 17:19 ` Linus Walleij @ 2024-12-27 21:01 ` Martin Blumenstingl 2024-12-31 8:26 ` Xianwei Zhao 1 sibling, 0 replies; 14+ messages in thread From: Martin Blumenstingl @ 2024-12-27 21:01 UTC (permalink / raw) To: Linus Walleij Cc: Xianwei Zhao, Huqiang Qin, Qianggui Song, Hyeonki Hong, Beniamino Galvani, Carlo Caione, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Neil Armstrong, Kevin Hilman, Jerome Brunet, linux-gpio, devicetree, linux-kernel, linux-arm-kernel, linux-amlogic Hi Linus, On Fri, Dec 27, 2024 at 6:19 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > Newcomers, latest patch set: > https://lore.kernel.org/linux-gpio/20241226-amlogic-pinctrl-v2-0-cdae42a67b76@amlogic.com/ > > I included some of the prior meson authors on the to line to see if > their mail addresses still work and if they have some feedback on this. > > On Sun, Dec 22, 2024 at 10:08 AM Linus Walleij <linus.walleij@linaro.org> wrote: > > > > > - Renaming drivers/pinctrl/sunxi to drivers/pinctrl/amlogic > > > > so we keep this sorted by actual vendor, sunxi is apparently > > > > yours (AMlogic:s) isn't it? > > > > > > It isn't. Sunxi is Allwinner SoCs. > > > > My apologies. I mixed it up completely. :( > > But wait a minute. I see there is meson. And in the "meson" subdirectory > there is stuff named "amlogic" ... Until some point Amlogic had all of their SoCs named meson along with a suffix which identifies the silicon die (for example: Amlogic Meson8, Amlogic Meson GXM, ...). The "meson" name was dropped at some point. Taking the example from this series, the SoC name is: Amlogic A4 (which would have been called Amlogic Meson A4 previously). [...] > > What do you think of the idea of a separate drivers/pinctrl/amlogic directory > > though? I think there are already quite a few amlogic SoCs that need > > to be supported and more will come. > > So what about renaming the existing subdir "meson" to "amlogic" > and put the driver there. That will make it consistent with other Amlogic related directories within the Linux kernel: $ find {arch,drivers} -type d -name "amlogic" arch/arm64/boot/dts/amlogic arch/arm/boot/dts/amlogic drivers/soc/amlogic drivers/phy/amlogic drivers/pmdomain/amlogic drivers/reset/amlogic drivers/media/platform/amlogic drivers/crypto/amlogic drivers/perf/amlogic > Also I want to know if this driver and hardware shares anything with > the existing drivers in that directory. It sometimes happen that > developers start something from scratch despite the existence of > prior art simply because of organizational issues, and we don't want > that kind of situation to leak over to the kernel. I will summarize the situation in my own words. If there are any inconsistencies then I'm asking others (Xianwei, Rob, Neil, ...) to point them out and correct me. Initially Xianwei started adding support for the Amlogic A4 SoC in June: [0] In this initial series he re-used all the support code from drivers/pinctrl/meson (the new A4 SoC driver simply selected PINCTRL_MESON_AXG_PMX). This means that we have two gpio-cells: - the first is a number which identifies the pin/pad (which included the bank and pin/pad within that bank as a consecutive numbering starting at 0) - the GPIO flags Up until v4 of that series [1] the driver side was pretty much the same as it was in v1. However, even with v1 there sparked some feedback on the dt-bindings. With v5 of that series [2] Xianwei changed the dt-binding (based on the input from the DT maintainers) to take the GPIO to take three gpio-cells: - first the GPIO bank number - then the pin/pad within that bank - the GPIO flags An overview of the changes up until this point can be found in the cover-letter of v6 of that series: [3] With the current series Xianwei started over, with a new driver (v1: [4] - and Linus, you have linked to v2 yourself: [5]) so it fits (to what I understand is) the desired dt-bindings. The main difference to the "old" dt-bindings is that driver changes are not needed to add or fix pin muxes (old binding: .dts only had references to pin groups and pin functions, the mapping to register bits happened within the driver - new binding: all pin muxing registers and bits are described in .dts). The "old" approach is more similar to what Allwinner SoC drivers/dt-bindings use whereas the "new" approach is more similar to what Rockchip SoC drivers/dt-bindings use. To put it into other words: the new driver (from this series) is a new dt-binding to hardware translation layer. The hardware side matches what we already have in drivers/pinctrl/meson/{pinctrl-meson.c,pinctrl-meson-axg-pmx.c} but the dt-binding side is completely different. Generally I expect the new driver to be backwards compatible to all SoCs that used the old dt-binding and the PINCTRL_MESON_AXG_PMX sub-driver in drivers/pinctrl/meson/ Those are: $ git grep -l '&meson_axg_pmx_ops' drivers/pinctrl/meson/ drivers/pinctrl/meson/pinctrl-amlogic-c3.c drivers/pinctrl/meson/pinctrl-amlogic-t7.c drivers/pinctrl/meson/pinctrl-meson-a1.c drivers/pinctrl/meson/pinctrl-meson-axg.c drivers/pinctrl/meson/pinctrl-meson-g12a.c drivers/pinctrl/meson/pinctrl-meson-s4.c (That said, I am not aware of any plans to migrate these SoCs over) So to conclude: This whole series is not about a new pinmux controller IP but a new dt-binding for the existing IP. In the old series (v1 at [0] - which shows that the underlying pinmux IP has not changed since the AXG generation) Xianwei tried to add support for the new SoC with the dt-binding that other Amlogic SoCs are already using - but this got rejected by the DT maintainers. Best regards, Martin [0] https://lore.kernel.org/all/20240611-a4_pinctrl-v1-2-dc487b1977b3@amlogic.com/ [1] https://lore.kernel.org/all/20241101-a4_pinctrl-v4-0-efd98edc3ad4@amlogic.com/ [2] https://lore.kernel.org/all/20241112-a4_pinctrl-v5-0-3460ce10c480@amlogic.com/ [3] https://lore.kernel.org/all/20241113-a4_pinctrl-v6-0-35ba2401ee35@amlogic.com/ [4] https://lore.kernel.org/all/20241211-amlogic-pinctrl-v1-0-410727335119@amlogic.com/ [5] https://lore.kernel.org/linux-gpio/20241226-amlogic-pinctrl-v2-0-cdae42a67b76@amlogic.com/ ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH RFC 2/3] pinctrl: Add driver support for Amlogic SoCs 2024-12-27 17:19 ` Linus Walleij 2024-12-27 21:01 ` Martin Blumenstingl @ 2024-12-31 8:26 ` Xianwei Zhao 1 sibling, 0 replies; 14+ messages in thread From: Xianwei Zhao @ 2024-12-31 8:26 UTC (permalink / raw) To: Linus Walleij, Huqiang Qin, Qianggui Song, Hyeonki Hong, Martin Blumenstingl, Beniamino Galvani, Carlo Caione Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Neil Armstrong, Kevin Hilman, Jerome Brunet, linux-gpio, devicetree, linux-kernel, linux-arm-kernel, linux-amlogic Hi Linus, Thanks for your reply. On 2024/12/28 01:19, Linus Walleij wrote: > [ EXTERNAL EMAIL ] > > Newcomers, latest patch set: > https://lore.kernel.org/linux-gpio/20241226-amlogic-pinctrl-v2-0-cdae42a67b76@amlogic.com/ > > I included some of the prior meson authors on the to line to see if > their mail addresses still work and if they have some feedback on this. > > On Sun, Dec 22, 2024 at 10:08 AM Linus Walleij <linus.walleij@linaro.org> wrote: > >>>> - Renaming drivers/pinctrl/sunxi to drivers/pinctrl/amlogic >>>> so we keep this sorted by actual vendor, sunxi is apparently >>>> yours (AMlogic:s) isn't it? >>> >>> It isn't. Sunxi is Allwinner SoCs. >> >> My apologies. I mixed it up completely. :( > > But wait a minute. I see there is meson. And in the "meson" subdirectory > there is stuff named "amlogic" ... > > $ ls -1 drivers/pinctrl/meson/ > Kconfig > Makefile > pinctrl-amlogic-c3.c > pinctrl-amlogic-t7.c > pinctrl-meson8b.c > pinctrl-meson8.c > pinctrl-meson8-pmx.c > pinctrl-meson8-pmx.h > pinctrl-meson-a1.c > pinctrl-meson-axg.c > pinctrl-meson-axg-pmx.c > pinctrl-meson-axg-pmx.h > pinctrl-meson.c > pinctrl-meson-g12a.c > pinctrl-meson-gxbb.c > pinctrl-meson-gxl.c > pinctrl-meson.h > pinctrl-meson-s4.c > >> What do you think of the idea of a separate drivers/pinctrl/amlogic directory >> though? I think there are already quite a few amlogic SoCs that need >> to be supported and more will come. > > So what about renaming the existing subdir "meson" to "amlogic" > and put the driver there. > > Also I want to know if this driver and hardware shares anything with > the existing drivers in that directory. It sometimes happen that > developers start something from scratch despite the existence of > prior art simply because of organizational issues, and we don't want > that kind of situation to leak over to the kernel. > Initially, I developed the driver for the A4 chip based on the existing framework (driver/pinctrl/meson/). Following the pattern of previous chips, I added the pin definitions directly to the bindings, but this approach was rejected by the maintainers ([0]). I then attempted an alternative approach: adding the bank to the bindings and converting pin definitions based on the bank and offset within the source code. I moved the pin definitions into the source code itself, but this was also not accepted ([1]). Realizing that the issue could not be resolved within the limitations of the old framework, I decided to introduce a new framework driver to address these problems. In the new framework, I made significant changes to simplify support for subsequent chips. Now, after adding the source files and binding documents, the only required operation for supporting future Amlogic SoCs (such as A4, A5, as well as existing chips like S4, C3, and T7) is to add the corresponding DTS node. [0] https://lore.kernel.org/all/20240611-a4_pinctrl-v1-2-dc487b1977b3@amlogic.com/ [1] https://lore.kernel.org/all/20241113-a4_pinctrl-v6-0-35ba2401ee35@amlogic.com/ > Yours, > Linus Walleij ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH RFC 3/3] arm64: dts: amlogic: a4: add pinctrl node 2024-12-11 6:47 [PATCH RFC 0/3] Pinctrl: Add Amlogic pinctrl driver Xianwei Zhao via B4 Relay 2024-12-11 6:47 ` [PATCH RFC 1/3] dt-bindings: pinctrl: Add support for Amlogic SoCs Xianwei Zhao via B4 Relay 2024-12-11 6:47 ` [PATCH RFC 2/3] pinctrl: Add driver " Xianwei Zhao via B4 Relay @ 2024-12-11 6:47 ` Xianwei Zhao via B4 Relay 2 siblings, 0 replies; 14+ messages in thread From: Xianwei Zhao via B4 Relay @ 2024-12-11 6:47 UTC (permalink / raw) To: Linus Walleij, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Neil Armstrong, Kevin Hilman, Jerome Brunet, Martin Blumenstingl Cc: linux-gpio, devicetree, linux-kernel, linux-arm-kernel, linux-amlogic, Xianwei Zhao From: Xianwei Zhao <xianwei.zhao@amlogic.com> Add pinctrl device to support Amlogic A4 and add uart pinconf. Signed-off-by: Xianwei Zhao <xianwei.zhao@amlogic.com> --- arch/arm64/boot/dts/amlogic/amlogic-a4.dtsi | 146 ++++++++++++++++++++++++++++ 1 file changed, 146 insertions(+) diff --git a/arch/arm64/boot/dts/amlogic/amlogic-a4.dtsi b/arch/arm64/boot/dts/amlogic/amlogic-a4.dtsi index de10e7aebf21..fccae7c9758a 100644 --- a/arch/arm64/boot/dts/amlogic/amlogic-a4.dtsi +++ b/arch/arm64/boot/dts/amlogic/amlogic-a4.dtsi @@ -5,6 +5,7 @@ #include "amlogic-a4-common.dtsi" #include <dt-bindings/power/amlogic,a4-pwrc.h> +#include <dt-bindings/pinctrl/amlogic,pinctrl.h> / { cpus { #address-cells = <2>; @@ -48,3 +49,148 @@ pwrc: power-controller { }; }; }; + +&apb { + periphs_pinctrl: pinctrl@4000 { + compatible = "amlogic,pinctrl"; + reg = <0x0 0x4000 0x0 0x0050>, + <0x0 0x40c0 0x0 0x0220>; + reg-names = "mux", "gpio"; + #address-cells = <2>; + #size-cells = <2>; + + gpiob: gpiob { + gpio-controller; + #gpio-cells = <2>; + npins = <14>; + bank-index = <AMLOGIC_GPIO_B>; + reg-mux-offset = <0>; + reg-gpio-offset = <0x60>; + bank-name = "GPIOB"; + }; + + gpiod: gpiod { + gpio-controller; + #gpio-cells = <2>; + npins = <16>; + bank-index = <AMLOGIC_GPIO_D>; + reg-mux-offset = <0x10>; + reg-gpio-offset = <0x30>; + bank-name = "GPIOD"; + }; + + gpioe: gpioe { + gpio-controller; + #gpio-cells = <2>; + npins = <2>; + bank-index = <AMLOGIC_GPIO_E>; + reg-mux-offset = <0x12>; + reg-gpio-offset = <0x40>; + bank-name = "GPIOE"; + }; + + gpiot: gpiot { + gpio-controller; + #gpio-cells = <2>; + npins = <23>; + bank-index = <AMLOGIC_GPIO_T>; + reg-mux-offset = <0xb>; + reg-gpio-offset = <0x20>; + bank-name = "GPIOT"; + }; + + gpiox: gpiox { + gpio-controller; + #gpio-cells = <2>; + npins = <18>; + bank-index = <AMLOGIC_GPIO_X>; + reg-mux-offset = <0x3>; + reg-gpio-offset = <0x10>; + bank-name = "GPIOX"; + }; + + func-uart-a { + uart_a_default: uart-a-pins1{ + pinmux= <AML_PINMUX(AMLOGIC_GPIO_X, 11, AF1)>, + <AML_PINMUX(AMLOGIC_GPIO_X, 12, AF1)>, + <AML_PINMUX(AMLOGIC_GPIO_X, 13, AF1)>, + <AML_PINMUX(AMLOGIC_GPIO_X, 13, AF1)>; + }; + + uart-a-pins2{ + pinmux= <AML_PINMUX(AMLOGIC_GPIO_D, 2, AF3)>, + <AML_PINMUX(AMLOGIC_GPIO_D, 3, AF3)>; + bias-pull-up; + drive-strength-microamp = <4000>; + }; + }; + + func-uart-b { + uart_b_default: uart-b-default{ + pinmux= <AML_PINMUX(AMLOGIC_GPIO_E, 0, AF3)>, + <AML_PINMUX(AMLOGIC_GPIO_E, 1, AF3)>; + bias-pull-up; + drive-strength-microamp = <4000>; + }; + }; + + func-uart-d { + uart_d_default: uart-d-pins1{ + pinmux= <AML_PINMUX(AMLOGIC_GPIO_T, 18, AF4)>, + <AML_PINMUX(AMLOGIC_GPIO_T, 19, AF4)>; + bias-pull-up; + drive-strength-microamp = <4000>; + }; + + uart-d-pins2{ + pinmux= <AML_PINMUX(AMLOGIC_GPIO_T, 7, AF2)>, + <AML_PINMUX(AMLOGIC_GPIO_T, 8, AF2)>, + <AML_PINMUX(AMLOGIC_GPIO_T, 9, AF2)>, + <AML_PINMUX(AMLOGIC_GPIO_T, 10, AF2)>; + bias-pull-up; + drive-strength-microamp = <4000>; + }; + }; + + func-uart-e { + uart_e_default: uart-e-pins{ + pinmux= <AML_PINMUX(AMLOGIC_GPIO_T, 14, AF3)>, + <AML_PINMUX(AMLOGIC_GPIO_T, 15, AF3)>, + <AML_PINMUX(AMLOGIC_GPIO_T, 16, AF3)>, + <AML_PINMUX(AMLOGIC_GPIO_T, 17, AF3)>; + bias-pull-up; + drive-strength-microamp = <4000>; + }; + }; + }; + + aobus_pinctrl: pinctrl@8e700 { + compatible = "amlogic,pinctrl"; + reg = <0x0 0x8e700 0x0 0x04>, + <0x0 0x8e704 0x0 0x60>; + reg-names = "mux", "gpio"; + #address-cells = <2>; + #size-cells = <2>; + + gpioao: gpioao { + gpio-controller; + #gpio-cells = <2>; + npins = <7>; + bank-index = <AMLOGIC_GPIO_AO>; + reg-mux-offset = <0>; + reg-gpio-offset = <0>; + bank-name = "GPIOAO"; + }; + + test_n: gpiotestn { + gpio-controller; + #gpio-cells = <2>; + npins = <1>; + bank-index = <AMLOGIC_GPIO_TEST_N>; + reg-mux-offset = <0>; + bit-mux-offset = <28>; + reg-gpio-offset = <0x10>; + bank-name = "TEST_N"; + }; + }; +}; -- 2.37.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
end of thread, other threads:[~2024-12-31 8:26 UTC | newest] Thread overview: 14+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-12-11 6:47 [PATCH RFC 0/3] Pinctrl: Add Amlogic pinctrl driver Xianwei Zhao via B4 Relay 2024-12-11 6:47 ` [PATCH RFC 1/3] dt-bindings: pinctrl: Add support for Amlogic SoCs Xianwei Zhao via B4 Relay 2024-12-17 14:20 ` Rob Herring 2024-12-18 8:47 ` Xianwei Zhao 2024-12-11 6:47 ` [PATCH RFC 2/3] pinctrl: Add driver " Xianwei Zhao via B4 Relay 2024-12-17 14:49 ` Linus Walleij 2024-12-17 14:53 ` Neil Armstrong 2024-12-18 9:37 ` Xianwei Zhao 2024-12-22 9:08 ` Linus Walleij 2024-12-26 7:49 ` Xianwei Zhao 2024-12-27 17:19 ` Linus Walleij 2024-12-27 21:01 ` Martin Blumenstingl 2024-12-31 8:26 ` Xianwei Zhao 2024-12-11 6:47 ` [PATCH RFC 3/3] arm64: dts: amlogic: a4: add pinctrl node Xianwei Zhao via B4 Relay
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox