Re: [PATCH v2 2/3] dt-bindings: pinctrl: Add EcoNet EN7528 pin controller

[Conor Dooley]

On Mon, Aug 31, 2026 at 08:46:52PM +0200, Benjamin Larsson wrote:
> On 31/08/2026 18:57, Conor Dooley wrote:
> > On Sat, Aug 29, 2026 at 03:47:00PM +0400, Ahmed Naseef wrote:
> > > On Fri, Aug 28, 2026 at 04:51:27PM +0100, Conor Dooley wrote:
> > > > On Fri, Aug 28, 2026 at 07:35:14PM +0400, Ahmed Naseef wrote:
> > > > > Add the binding for the pin controller found on the EcoNet EN7528 MIPS
> > > > > SoC. It shares the IP with the Airoha pin controllers, but its IOMUX and
> > > > > pin configuration registers have a different layout, so it gets its own
> > > > > compatible.
> > > > > 
> > > > > The CHIP SCU holding those registers is referenced by the airoha,chip-scu
> > > > > phandle.
> > > > > 
> > > > > Signed-off-by: Ahmed Naseef <naseefkm@xxxxxxxxx>
> > > > > ---
> > > > >   .../pinctrl/econet,en7528-pinctrl.yaml        | 190 ++++++++++++++++++
> > > > >   1 file changed, 190 insertions(+)
> > > > >   create mode 100644 Documentation/devicetree/bindings/pinctrl/econet,en7528-pinctrl.yaml
> > > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/pinctrl/econet,en7528-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/econet,en7528-pinctrl.yaml
> > > > > new file mode 100644
> > > > > index 000000000000..3ce1a5309d09
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/pinctrl/econet,en7528-pinctrl.yaml
> > > > > @@ -0,0 +1,190 @@
> > > > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > > > +%YAML 1.2
> > > > > +---
> > > > > +$id: http://devicetree.org/schemas/pinctrl/econet,en7528-pinctrl.yaml#
> > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > +
> > > > > +title: EcoNet EN7528 Pin Controller
> > > > > +
> > > > > +maintainers:
> > > > > +  - Lorenzo Bianconi <lorenzo@xxxxxxxxxx>
> > > > > +
> > > > > +description:
> > > > > +  The EcoNet EN7528 pin controller is used to control SoC pins. It shares the
> > > > > +  IP with the Airoha pin controllers, but its IOMUX and pin configuration
> > > > > +  registers have a different layout.
> > > > > +
> > > > > +properties:
> > > > > +  compatible:
> > > > > +    const: econet,en7528-pinctrl
> > > > > +
> > > > > +  airoha,chip-scu:
> > > > > +    description: phandle to the chip SCU syscon
> > > > > +    $ref: /schemas/types.yaml#/definitions/phandle
> > > > > +
> > > > > +  interrupts:
> > > > > +    maxItems: 1
> > > > > +
> > > > > +  gpio-controller: true
> > > > > +
> > > > > +  '#gpio-cells':
> > > > > +    const: 2
> > > > > +
> > > > > +  gpio-ranges:
> > > > > +    maxItems: 1
> > > > > +
> > > > > +  interrupt-controller: true
> > > > > +
> > > > > +  '#interrupt-cells':
> > > > > +    const: 2
> > > > > +
> > > > > +patternProperties:
> > > > > +  '-pins$':
> > > > > +    type: object
> > > > > +
> > > > > +    patternProperties:
> > > > > +      '^mux(-|$)':
> > > > > +        type: object
> > > > > +
> > > > > +        description:
> > > > > +          pinmux configuration nodes.
> > > > > +
> > > > > +        $ref: /schemas/pinctrl/pinmux-node.yaml
> > > > > +
> > > > > +        properties:
> > > > > +          function:
> > > > > +            description:
> > > > > +              A string containing the name of the function to mux to the group.
> > > > > +            enum: [gpio, jtag, pcie_reset, pcm, pcm_spi, phy1_led0,
> > > > > +                   phy1_led1, phy2_led0, phy2_led1, phy3_led0, phy3_led1,
> > > > > +                   phy4_led0, phy4_led1, pnand, pon, pwm, sipo, spi, uart]
> > > > > +
> > > > > +          groups:
> > > > > +            description:
> > > > > +              An array of strings. Each string contains the name of a group.
> > > > > +            items:
> > > > > +              enum: [pon, sipo, sipo_rclk, uart2, uart2_cts_rts, uart3,
> > > > > +                     ejtag, pcm1, pcm2, spi_quad, spi_cs1, pcm_spi,
> > > > > +                     pcm_spi_int, pcm_spi_rst, pcm_spi_cs1, pcm_spi_cs2,
> > > > > +                     pcm_spi_cs3, pcm_spi_cs4, pcm_spi_cs5, pcm_spi_cs6,
> > > > > +                     pcm_spi_cs7, pnand, gpio0, gpio1, gpio2, gpio3, gpio4,
> > > > > +                     gpio5, gpio6, gpio7, gpio8, gpio9, gpio10, gpio11,
> > > > > +                     gpio12, gpio13, gpio14, gpio15, gpio16, gpio17,
> > > > > +                     gpio18, gpio19, gpio20, gpio21, gpio22, gpio23,
> > > > > +                     gpio24, gpio25, gpio26, gpio27, gpio28, gpio29,
> > > > > +                     gpio30, gpio31, gpio32, gpio33, gpio34, gpio35,
> > > > > +                     gpio36, gpio37, gpio38, gpio39, gpio40, gpio41,
> > > > > +                     pcie_reset0, pcie_reset1]
> > > > > +
> > > > > +        required:
> > > > > +          - function
> > > > > +          - groups
> > > > > +
> > > > > +        additionalProperties: false
> > > > > +
> > > > > +      '^conf(-|$)':
> > > > 
> > > > Why is this needed? Why can you not tell from the group which pins the
> > > > config applies to?
> > > 
> > > Not for all of them: i2c_sda, i2c_scl, uart_txd and uart_rxd are in no
> > > group at all.
> > 
> > Why's that? Are they just fixed output pins?
> > 
> > > The spi_cs0/clk/mosi/miso pads are only in pnand, which is
> > > 17 pins wide, so no group names them individually either.
> > 
> > That's easily resolved by breaking up pnand though.
> > 
> > Devils advocate thing here might be to say that the dedicated
> > configuration node should only be for the things that are fixed outputs,
> > and the muxable ones should get their config from a combined
> > pinmux/pincfg node. Personally I think that presents a nicer interface,
> > where someone doesn't have to go figure out what pins are in the group
> > they just assigned a function to to also set the config properties for
> > it.
> > 
> > Cheers,
> > Conor.
> > 
> Hi, I dont fully understand the exact implications but there are at least 2
> more Econet pinctrl drivers coming. I would prefer it if we could keep them
> looking exactly like the other Airoha drivers that are already merged. All
> the SoCs share the same structure and logic just different pins and
> addresses.

FWIW, "I want to do X cos Y other driver does X" is not really a
compelling argument in general. In this case, what's being done isn't
terrible (and I was just about to tell Ahmed that I didn't notice that
there's actually code reuse going on and his patch was adding 1200 lines
of definitions rather than functions), but in general this is not an
acceptable argument.

Also, in this case, I amn't overly opposed to what's going on here, I
just want to be convinced that this isn't trying to shoehorn something
into an existing model. It looks very clumsy to apply muxing by group
but config by pin, when a little bit more judicious use of groups could
get it all done in one go.

I think I have already made similar comments on Airoha pinctrl in the
past, so I guess I will just let this go as long as this is effectively
describing the same "IP", or "structure and logic" as you put it, and
expect that when the mechanism changes (which it will undoubtedly at
some point) that either pins or groups, not both, is used then. If I
remember or haven't burnt out and quit...

Cheers,
Conor.

Attachment: signature.asc
Description: PGP signature