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

[Conor Dooley <conor@kernel.org>]

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

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@kernel.org> wrote:
> >
> > On Thu, Sep 03, 2026 at 02:09:42PM -0600, James Hilliard wrote:
> > > From: Jernej Skrabec <jernej.skrabec@gmail.com>
> > >
> > > 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@gmail.com>
> > > Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
> > > ---
> > >  .../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?

Personally I would just implement these devices using IRQF_SHARED.

> 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.

> 
> > 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?

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@lists.linux.dev
> > >  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@kernel.org>
> > > 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
> > >

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