Re: [PATCH v9 3/4] dt-bindings: mfd: x-powers: Describe AC200 functions

[James Hilliard]

On Mon, Sep 7, 2026 at 12:28 PM Conor Dooley <conor@xxxxxxxxxx> wrote:
>
> On Fri, Sep 04, 2026 at 01:06:08PM -0400, James Hilliard wrote:
> > On Fri, Sep 4, 2026 at 9:50 AM Conor Dooley <conor@xxxxxxxxxx> wrote:
> > >
> > > On Thu, Sep 03, 2026 at 02:09:42PM -0600, James Hilliard wrote:
> > > > From: Jernej Skrabec <jernej.skrabec@xxxxxxxxx>
> > > >
> > > > Describe the AC200 audio codec and TV encoder as child nodes of the
> > > > shared I2C register provider. Keep their analog supplies on the function
> > > > consumers and describe the TV encoder display graph and optional bandgap
> > > > calibration cell.
> > > >
> > > > Add the shared interrupt-controller properties and interrupt numbers needed
> > > > by the TV encoder. The Ethernet PHY remains represented on its primary MDIO
> > > > bus and is therefore not an MFD child.
> > > >
> > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@xxxxxxxxx>
> > > > Signed-off-by: James Hilliard <james.hilliard1@xxxxxxxxx>
> > > > ---
> > > >  .../devicetree/bindings/mfd/x-powers,ac200.yaml    | 136 +++++++++++++++++++++
> > > >  MAINTAINERS                                        |   1 +
> > > >  include/dt-bindings/mfd/x-powers,ac200.h           |  13 ++
> > > >  3 files changed, 150 insertions(+)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/mfd/x-powers,ac200.yaml b/Documentation/devicetree/bindings/mfd/x-powers,ac200.yaml
> > > > index ca7a910b2c73..935dc07138cb 100644
> > > > --- a/Documentation/devicetree/bindings/mfd/x-powers,ac200.yaml
> > > > +++ b/Documentation/devicetree/bindings/mfd/x-powers,ac200.yaml
> > > > @@ -28,15 +28,114 @@ properties:
> > > >        be 24 or 27 MHz, matching the rates encoded by the documented EPHY clock
> > > >        selector.
> > > >
> > > > +  interrupts:
> > > > +    maxItems: 1
> > > > +    description:
> > > > +      The shared open-drain INTB output for the TV encoder, Ethernet PHY and
> > > > +      RTC interrupts.
> > > > +
> > > > +  interrupt-controller: true
> > > > +
> > > > +  '#interrupt-cells':
> > > > +    const: 1
> > > > +    description:
> > > > +      The interrupt number, as defined in
> > > > +      include/dt-bindings/mfd/x-powers,ac200.h.
> > > > +
> > > > +  codec:
> > > > +    type: object
> > > > +    $ref: /schemas/sound/dai-common.yaml#
> > > > +    unevaluatedProperties: false
> > > > +
> > > > +    properties:
> > > > +      compatible:
> > > > +        const: x-powers,ac200-codec
> > > > +
> > > > +      '#sound-dai-cells':
> > > > +        const: 0
> > > > +
> > > > +      ac-ldoin-supply:
> > > > +        description: The 3.3 V supply for the audio codec LDO input.
> > > > +
> > > > +    required:
> > > > +      - compatible
> > > > +      - '#sound-dai-cells'
> > > > +      - ac-ldoin-supply
> > > > +
> > > > +  tv-encoder:
> > > > +    type: object
> > > > +    additionalProperties: false
> > > > +
> > > > +    properties:
> > > > +      compatible:
> > > > +        const: x-powers,ac200-tve
> > > > +
> > > > +      interrupts:
> > > > +        maxItems: 1
> > > > +        description: Cable detection interrupt.
> > >
> > > Given the example, this looks like a hack.
> > > Is this mfd actually an interrupt controller, or is that just part of
> > > this hack too?
> >
> > The AC200 has separate TV encoder, Ethernet PHY and RTC status and
> > enable bits which are multiplexed onto its shared INTB output, so the
> > intent was to represent that demultiplexer as an interrupt controller.
> >
> > I notice that the example's tv-encoder interrupts property inherits
> > interrupt-parent = <&pio> rather than explicitly referencing the AC200
> > interrupt domain. Is that incorrect interrupt relationship what looks
> > like a hack here, or do you also object to representing the AC200
>
> No, I noticed the lack of an explicit parent but attributed it to a
> mistake. The define used in the child node, and the interrupt properties
> in the parent are why I considered it a hack - the device looked like it
> was pointing to itself as it's own interrupt parent. How would this work
> for the rtc, since that is implemented without a child node?

There isn't an RTC driver in this series; only its interrupt source was
described. A future RTC MFD cell could receive the shared parent IRQ
without requiring a separate DT node.

> Personally I would just implement these devices using IRQF_SHARED.

I've implemented that approach in my local v11 tree. The binding now
describes the optional INTB connection without interrupt-controller
properties or internal interrupt numbers.

The MFD driver passes the physical IRQ to the TV encoder cell when
connected. Without INTB connected, the cell has no IRQ resource.

Function drivers remain separate work. They will need to request
IRQF_SHARED | IRQF_ONESHOT handlers before enabling their own source,
check and clear their own status in the threaded handler, and mask their
source before freeing the IRQ. The parent initially masks all sources
before enabling INTB.

> > interrupt demultiplexer as an interrupt controller?
> >
> > > Quite frankly, I am not really sure why either the tv-encoder or codec
> > > have dedicated child nodes, they don't appear to have conflicting
> > > properties.
> >
> > Do you mean that the DT properties for both functions should be folded
> > into the AC200 parent node, while the MFD driver still creates separate
> > codec and TV encoder platform devices?
> >
> > Lee requested at least two MFD children in this series, so I want to
> > distinguish the Linux MFD cells from whether those cells need dedicated
> > firmware child nodes.
>
> I don't see Lee requesting that they be in the devicetree though, just
> that there are two mfd children. mfd_cell would (IMO) qualify for that.
>
> I don't see anything here that'd be problematic in terms of folding the
> codec and encodering into the parent mfd device and using mfd_cell, given
> the phy is not represented here.

I've moved the audio DAI properties, supplies, bandgap calibration cell
and display ports into the AC200 parent node. The codec and TV encoder
remain separate Linux devices described by static MFD cells, without
separate DT nodes. Their supply lookups use MFD parent-supply aliases.
The PHY remains on MDIO.

> >
> > > Additionally, why is this not part of patch 1? Add the binding in a
> > > complete state from the get-go. On that basis, at least,
> > > pw-bot: changes-requested
> >
> > Do you want patches 1 and 3 combined into one complete binding patch,
> > while retaining separate implementation commits for the base provider
> > and the MFD cells?
>
> Correct.
>
> > > > +
> > > > +      tv-vcc-supply:
> > > > +        description: The 3.3 V supply for the composite-video DAC.
>
> Are these genuinely different 3.3 V supplies btw? Or are you just
> representing them as two different ones because of different device
> nodes using them?

They correspond to distinct physical supply inputs: TV_VCC is pin 1 and
AC_LDOIN is pin 2, both nominally 3.3 V. The AC200 datasheet lists these
separately:

https://linux-sunxi.org/images/0/0f/AC200_Datasheet_V1.1.pdf

They do not require separate external regulators; the example connects
both to reg_aldo2. The revised parent-node descriptions now explicitly
state that these inputs may share a regulator.



> Cheers,
> Conor.
>
> > > > +
> > > > +      nvmem-cells:
> > > > +        maxItems: 1
> > > > +
> > > > +      nvmem-cell-names:
> > > > +        items:
> > > > +          - const: bandgap
> > > > +
> > > > +      ports:
> > > > +        $ref: /schemas/graph.yaml#/properties/ports
> > > > +
> > > > +        properties:
> > > > +          port@0:
> > > > +            $ref: /schemas/graph.yaml#/properties/port
> > > > +            description: Input from the display pipeline, carrying CCIR656.
> > > > +
> > > > +          port@1:
> > > > +            $ref: /schemas/graph.yaml#/properties/port
> > > > +            description: Output to the composite-video connector.
> > > > +
> > > > +        required:
> > > > +          - port@0
> > > > +          - port@1
> > > > +
> > > > +    required:
> > > > +      - compatible
> > > > +      - interrupts
> > > > +      - tv-vcc-supply
> > > > +      - ports
> > > > +
> > > > +    dependencies:
> > > > +      nvmem-cells: [ nvmem-cell-names ]
> > > > +      nvmem-cell-names: [ nvmem-cells ]
> > > > +
> > > >  required:
> > > >    - compatible
> > > >    - reg
> > > >    - clocks
> > > >
> > > > +allOf:
> > > > +  - if:
> > > > +      required:
> > > > +        - tv-encoder
> > > > +    then:
> > > > +      required:
> > > > +        - interrupts
> > > > +        - interrupt-controller
> > > > +        - '#interrupt-cells'
> > > > +
> > > > +dependencies:
> > > > +  interrupt-controller: [ '#interrupt-cells', interrupts ]
> > > > +  '#interrupt-cells': [ interrupt-controller ]
> > > > +
> > > >  additionalProperties: false
> > > >
> > > >  examples:
> > > >    - |
> > > > +    #include <dt-bindings/interrupt-controller/irq.h>
> > > > +    #include <dt-bindings/mfd/x-powers,ac200.h>
> > > > +
> > > >      i2c {
> > > >          #address-cells = <1>;
> > > >          #size-cells = <0>;
> > > > @@ -45,6 +144,43 @@ examples:
> > > >              compatible = "x-powers,ac200";
> > > >              reg = <0x10>;
> > > >              clocks = <&pwm 5>;
> > > > +            interrupt-parent = <&pio>;
> > > > +            interrupts = <1 20 IRQ_TYPE_LEVEL_LOW>;
> > > > +            interrupt-controller;
> > > > +            #interrupt-cells = <1>;
> > > > +
> > > > +            codec {
> > > > +                compatible = "x-powers,ac200-codec";
> > > > +                #sound-dai-cells = <0>;
> > > > +                ac-ldoin-supply = <&reg_aldo2>;
> > > > +            };
> > > > +
> > > > +            tv-encoder {
> > > > +                compatible = "x-powers,ac200-tve";
> > > > +                interrupts = <AC200_IRQ_TVE>;
> > > > +                tv-vcc-supply = <&reg_aldo2>;
> > > > +
> > > > +                ports {
> > > > +                    #address-cells = <1>;
> > > > +                    #size-cells = <0>;
> > > > +
> > > > +                    port@0 {
> > > > +                        reg = <0>;
> > > > +
> > > > +                        tve_in: endpoint {
> > > > +                            remote-endpoint = <&tcon_out_tve>;
> > > > +                        };
> > > > +                    };
> > > > +
> > > > +                    port@1 {
> > > > +                        reg = <1>;
> > > > +
> > > > +                        tve_out: endpoint {
> > > > +                            remote-endpoint = <&composite_in>;
> > > > +                        };
> > > > +                    };
> > > > +                };
> > > > +            };
> > > >          };
> > > >      };
> > > >  ...
> > > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > > index 1d03b0060bda..8a48f6a1e593 100644
> > > > --- a/MAINTAINERS
> > > > +++ b/MAINTAINERS
> > > > @@ -29511,6 +29511,7 @@ L:    linux-sunxi@xxxxxxxxxxxxxxx
> > > >  S:   Maintained
> > > >  F:   Documentation/devicetree/bindings/mfd/x-powers,ac200.yaml
> > > >  F:   drivers/mfd/ac200.c
> > > > +F:   include/dt-bindings/mfd/x-powers,ac200.h
> > > >
> > > >  X-POWERS AXP288 PMIC DRIVERS
> > > >  M:   Hans de Goede <hansg@xxxxxxxxxx>
> > > > diff --git a/include/dt-bindings/mfd/x-powers,ac200.h b/include/dt-bindings/mfd/x-powers,ac200.h
> > > > new file mode 100644
> > > > index 000000000000..cc59e2ab4912
> > > > --- /dev/null
> > > > +++ b/include/dt-bindings/mfd/x-powers,ac200.h
> > > > @@ -0,0 +1,13 @@
> > > > +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
> > > > +/*
> > > > + * Interrupt numbers of the X-Powers AC200 interrupt controller.
> > > > + */
> > > > +
> > > > +#ifndef _DT_BINDINGS_MFD_X_POWERS_AC200_H
> > > > +#define _DT_BINDINGS_MFD_X_POWERS_AC200_H
> > > > +
> > > > +#define AC200_IRQ_TVE                        0
> > > > +#define AC200_IRQ_EPHY                       1
> > > > +#define AC200_IRQ_RTC                        2
> > > > +
> > > > +#endif /* _DT_BINDINGS_MFD_X_POWERS_AC200_H */
> > > >
> > > > --
> > > > 2.53.0
> > > >