Re: [PATCH 1/6] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Mark Kettenis <mark.kettenis@xxxxxxxxx>]

> From: Rob Herring <robh+dt@xxxxxxxxxx>
> Date: Thu, 1 Sep 2022 17:33:31 -0500
> 
> On Thu, Sep 1, 2022 at 11:47 AM Russell King (Oracle)
> <linux@xxxxxxxxxxxxxxx> wrote:
> >
> > On Thu, Sep 01, 2022 at 07:25:03PM +0300, Krzysztof Kozlowski wrote:
> > > On 01/09/2022 18:56, Russell King (Oracle) wrote:
> > > >
> > > > 8<===
> > > > From: "Russell King (Oracle)" <rmk+kernel@xxxxxxxxxxxxxxx>
> > > > Subject: [PATCH] dt-bindings: mfd: add binding for Apple Mac System Management
> > > >  Controller
> > > >
> > > > Add a DT binding for the Apple Mac System Management Controller.
> > > >
> > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@xxxxxxxxxxxxxxx>
> > >
> > > Yes, looks good.
> > >
> > > I won't add Reviewed-by tag, because I think it would confuse Patchwork,
> > > so please send a v2 at some point.
> >
> > Thanks. Do you have any suggestions for patch 2? Should I merge the
> > description in patch 2 into this file?
> >
> > The full dts for this series looks like this:
> >
> >                 smc: smc@23e400000 {
> >                         compatible = "apple,t8103-smc", "apple,smc";
> >                         reg = <0x2 0x3e400000 0x0 0x4000>,
> >                                 <0x2 0x3fe00000 0x0 0x100000>;
> >                         reg-names = "smc", "sram";
> >                         mboxes = <&smc_mbox>;
> >
> >                         smc_gpio: gpio {
> >                                 gpio-controller;
> >                                 #gpio-cells = <2>;
> >                         };
> >                 };
> >
> > but the fuller version in the asahi linux tree looks like:
> >
> >                 smc: smc@23e400000 {
> >                         compatible = "apple,t8103-smc", "apple,smc";
> >                         reg = <0x2 0x3e400000 0x0 0x4000>,
> >                                 <0x2 0x3fe00000 0x0 0x100000>;
> >                         reg-names = "smc", "sram";
> >                         mboxes = <&smc_mbox>;
> >
> >                         smc_gpio: gpio {
> >                                 gpio-controller;
> >                                 #gpio-cells = <2>;
> 
> Only 2 properties doesn't really need its own schema doc. However, I
> would just move these to the parent node.

When we designed the bindings, it was our understanding that having
separate nodes better matches Linux's MFD driver model.

Please be aware that OpenBSD is already using these bindings.  If
there are good reasons for moving things, we can probably deal with
that.  But this sounds a bit like a toss up.

> 
> >                         };
> >
> >                         smc_rtc: rtc {
> >                                 nvmem-cells = <&rtc_offset>;
> >                                 nvmem-cell-names = "rtc_offset";
> >                         };
> >
> >                         smc_reboot: reboot {
> >                                 nvmem-cells = <&shutdown_flag>, <&boot_stage>,
> >                                         <&boot_error_count>, <&panic_count>, <&pm_setting>;
> >                                 nvmem-cell-names = "shutdown_flag", "boot_stage",
> >                                         "boot_error_count", "panic_count", "pm_setting";
> 
> Not really much reason to split these up either because you can just
> fetch the entry you want by name.

Again the separate nodes are there because the RTC and the reboot
functionality are logically separate and handled by different MFD
sub-drivers in Linux.

> How confident are the asahi folks that this is a complete binding?

There is a lot of functionality in the SMC that is still largely
unexplored.  The idea of the design of the binding is that additional
functionality may be added by adding more subnodes to the smc node.
But we expect that the main SMC node itself should be complete with
the "smc" and "sram" regions and the reference to the mailbox.

Cheers,

Mark