Re: [PATCH v2 1/2] dt-bindings: pinctrl: airoha: Add EN7581 pinctrl controller

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2249 bytes --]

On Thu, Aug 22, 2024 at 11:40:52AM +0200, Lorenzo Bianconi wrote:
> Introduce device-tree binding documentation for Airoha EN7581 pinctrl
> controller.
> 
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> +  reg:
> +    items:
> +      - description: IOMUX base address
> +      - description: LED IOMUX base address
> +      - description: GPIO flash mode base address
> +      - description: GPIO flash mode extended base address
> +      - description: IO pin configuration base address
> +      - description: PCIE reset open-drain base address
> +      - description: GPIO bank0 register base address
> +      - description: GPIO bank0 second control register base address
> +      - description: GPIO bank1 second control register base address
> +      - description: GPIO bank1 register base address

> +      pinctrl@1fa20214 {
> +        compatible = "airoha,en7581-pinctrl";
> +        reg = <0x0 0x1fa20214 0x0 0x30>,
> +              <0x0 0x1fa2027c 0x0 0x8>,
> +              <0x0 0x1fbf0234 0x0 0x4>,
> +              <0x0 0x1fbf0268 0x0 0x4>,
> +              <0x0 0x1fa2001c 0x0 0x50>,
> +              <0x0 0x1fa2018c 0x0 0x4>,
> +              <0x0 0x1fbf0200 0x0 0x18>,
> +              <0x0 0x1fbf0220 0x0 0x4>,
> +              <0x0 0x1fbf0260 0x0 0x8>,
> +              <0x0 0x1fbf0270 0x0 0x28>;
> +        reg-names = "iomux", "led-iomux",
> +                    "gpio-flash-mode", "gpio-flash-mode-ext",
> +                    "ioconf", "pcie-rst-od",
> +                    "gpio-bank0", "gpio-ctrl1",
> +                    "gpio-ctrl2", "gpio-bank1";

before looking at v1:
I would really like to see an explanation for why this is a correct
model of the hardware as part of the commit message. To me this screams
syscon/MFD and instead of describing this as a child of a syscon and
using regmap to access it you're doing whatever this is...

after looking at v1:
AFAICT the PWM driver does not currently exist in mainline, so I am now
doubly of the opinion that this needs to be an MFD and a wee bit annoyed
that you didn't include any rationale in your cover letter or w/e for
not going with an MFD given there was discussion on the topic in v1.

Thanks,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]