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

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Fri, Sep 02, 2022 at 12:28:08PM -0500, Rob Herring wrote:
> On Fri, Sep 02, 2022 at 05:06:43PM +0200, Mark Kettenis wrote:
> > > From: Rob Herring <robh+dt@kernel.org>
> > > Date: Thu, 1 Sep 2022 17:33:31 -0500
> > > 
> > > On Thu, Sep 1, 2022 at 11:47 AM Russell King (Oracle)
> > > <linux@armlinux.org.uk> 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@armlinux.org.uk>
> > > > > > 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@armlinux.org.uk>
> > > > >
> > > > > 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.
> 
> Well, it is convenient to have subnodes with compatibles so that your 
> drivers automagically probe. So yes, a 1:1 relationship of nodes to 
> drivers is nice and tidy. But h/w is not always packaged up neatly and 
> it's not DT's job to try to abstract it such that it is. Also, we 
> shouldn't design bindings around the *current* driver partitioning of 
> some OS.
> 
> This one is actually pretty odd in that the child nodes don't have a 
> compatible string which breaks the automagical probing.
> 
> > 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.
> 
> Sigh. If there are other bindings in use, please submit them even if the 
> Linux driver isn't ready. If a Linux subsystem maintainer doesn't want 
> to take it, then I will. 
> 
> It is a toss up though...
> 
> > > >                         };
> > > >
> > > >                         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.
> 
> It's really a question of whether the subset of functionality is going 
> to get reused on its own or has its own resources in DT. MFD bindings 
> are done both ways.

I'm guessing this series is blocked until a resolution is found for the
DT binding description.

Please can the Asahi folk and the DT maintainers sort this out and let
me know when I can proceed with this patch set. I'm just the man in
the middle here.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel