RE: [PATCH v5 3/8] dt-bindings: mfd: realtek,rtd1xxx: Add interrupt controller child node

[Yu-Chun Lin [林祐君]]

Hi Krzysztof,

> >>>  patternProperties:
> >>> +  "^interrupt-controller":
> >>> +    $ref: /schemas/interrupt-controller/realtek,rtd1319-intc-iso.yaml
> >>> +    unevaluatedProperties: false
> >>> +
> >>>    '@[0-9a-f]+$':
> >>>      type: object
> >>
> >> What is this for then? Should it be removed?
> >>
> >
> > The original syscon compatible has other child nodes that do have 'reg'
> > properties and unit addresses, so this pattern is still needed for them.
> >
> >>>
> >>> @@ -76,3 +80,27 @@ required:
> >>>    - reg
> >>>
> >>>  additionalProperties: false
> >>> +
> >>> +examples:
> >>> +  - |
> >>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> >>> +    #include <dt-bindings/interrupt-controller/irq.h>
> >>> +
> >>> +    syscon@7000 {
> >>> +        compatible = "realtek,rtd1625-iso", "syscon", "simple-mfd";
> >>> +        reg = <0x7000 0x1000>;
> >>> +        ranges = <0x0 0x7000 0x1000>;
> >>> +        #address-cells = <1>;
> >>> +        #size-cells = <1>;
> >>
> >> Why do you have cells it your child has no addressing?
> >>
> >>> +
> >>> +        interrupt-controller {
> >>
> >> So the binding requires children with unit address, yes?
> >>
> >
> > The interrupt-controller does not need a unit address because it
> > accesses the registers via the parent's regmap.
> 
> That's not really relevant. Child drivers still access via parent's regmap and
> have unit address. This binding has bus addressing for children, so use it. Do
> not add random or inconsistent code.
> 

The registers for interrupt controller are quite scattered within the parent
syscon block. Take rtd1625-intc-misc as an example, the offsets are 0x4,
0xc, 0x80, and 0x88 After adding unit-address and reg.
It would be like:

interrupt-controller@4 {
    compatible = "realtek,rtd1625-intc-misc";
    reg = <0x4 0x4>,
          <0xc 0x4>,
          <0x80 0x4>,
          <0x88 0x4>;
    interrupt-controller;
    ...
};

Could you please confirm if this is the correct and preferred approach?

> >
> > Will update the example to remove the unnecessary properties from the
> > parent as follows:
> >
> > syscon@7000 {
> >     compatible = "realtek,rtd1625-iso", "syscon", "simple-mfd";
> >     reg = <0x7000 0x1000>;
> >
> 
> Still incomplete. Where are other devices?
> 

In the past, Andreas used a generic '@[0-9a-f]+$"' to match child nodes.
Because of this, in older SoCs (e.g., rtd129x.dtsi, rtd16xx.dtsi,
rtd139x.dtsi), devices like reset-controller, watchdog, and serial were
allowed and placed under the iso or misc syscon nodes.

However, I have some doubts about continuing to use that exact structure:

1. We previously discussed and agreed [1] that devices like serial should
actually be placed under rbus, not syscon.
2. For the SoCs introduced in this patch (rtd1319,rtd1319d, rtd1325,
rtd1625), the drivers for the other child devices (like watchdog) are not
upstreamed yet.

To properly fix the "incomplete example" for the simple-mfd structure,
could you please advise:

1. Should I replace the generic '@[0-9a-f]+$' in the YAML binding with
explicit device definitions for these specific child nodes?
2. For the example section, should I just populate it with placeholders
(e.g., watchdog) referencing Andreas's older DTs (like rtd129x.dtsi) to
illustrate the MFD structure?

[1] https://lore.kernel.org/lkml/15951264289d44f797ebd4e05238b380@xxxxxxxxxxx/

Best Regards,
Yu-Chun

> 
> Best regards,
> Krzysztof