Re: [PATCH 4/6] platform/apple: Add new Apple Mac SMC driver

["Russell King (Oracle)" <linux@xxxxxxxxxxxxxxx>]

On Mon, Oct 31, 2022 at 05:23:12PM +0000, Lee Jones wrote:
> I see that you pass a bunch of function pointers from the RTKit
> implementation into the SMC.  Which in turn offers an exported
> (apple_smc_*) API.  In most of the frameworks I have knowledge of, the
> core provides the Ops structure and it's populated by the client
> device.

Sorry Lee, I don't get this point. From what I can see, the
apple_smc_backend_ops struct is owned by the core System Management
Controller code, and RTKit backend fills in an instance of these ops
and provides that to the core SMC code. The RTKit backend is just
how we walk to the System Management Controller. It is not a client.

I don't see this being any different to struct file_operations,
seq_operations, vm_operations_struct, block_Device_operations,
and so on and so forth.

Having read your response, I wonder if you're confused about what the
smc_core and smc_rtkit code actually are - because you seem to think
that smc_rtkit is a _client_ of the smc_core code. It isn't, as I
explain above, it's how we talk to the System Management Controller,
and smc_core provides a uniform interface to the client drivers such
as GPIO, RTC etc.

Essentially, we have:

Hardware   Backend    Core             Clients
                                 .---- RTC
                                / .--- GPIO
Mailbox -- RTKit -- SMC Core -- MFD -- HID
                                \ `--- Power
                                 `---- Reboot

RTKit is just _one_ possible backend, there are other backends that
can be used to interface to the underlying platform implementation to
talk to the SMC.

> I'm sure having that clear in my head will go some ways to put me in a
> position to advise you further.
> 
> > > Request the device-wide memory (and other shared resources) here.
> > 
> > That's what smc_rtkit.c does, but you seem not to want that code in mfd.
> 
> I'm not sure I explicitly said that.

On Fri, Sep 09, 2022 at 08:50:07AM +0100, Lee Jones wrote:
| If we were to design and build it up again from scratch, I'd suggest
| that the MFD part would be the core-driver / entry-point.  That driver
| should request and initialise shared resources and register the other
| devices, which is essentially the MFD's mantra.

This is exactly what smc_rtkit is doing, which as I've mentioned above
is the backend provider of access to the System Management Controller.
Backend-independent access to the System Management Controller is done
via smc_core which - at least to me - seems to be entirely correct,
and it seems entirely appropriate that this should be responsible for
creating the individual clients that make use of the System Management
Controller's facilities such as GPIO, RTC etc.

> "call into" was not a good choice of words here.  Simply, let the
> child devices go about their business and do whatever they were
> designed to do.

... by calling into the code which provides them with access to the
System Management Controller - that being through smc_core and
ultimately which ever backend is used to finally communicate with the
System Management Controller.

At this point, I'm wondering whether you're somehow expecting client
devices to map memory and read/write some registers. This is not that
kind of setup. The address space is entirely virtual, through a set
of four byte keys that indicate to the System Management Controller
which fine-grained resource one wants to access. That being an
individual GPIO line or some other parameter of the system.

The memory that you see smc_rtkit claim is for passing messages, none
of the clients have a right to directly access that memory - indeed,
doing so would be a total layering violation and really bad bit of
design.

So, I hope my response helps fill in some of the detail about what
this code is doing, how it works and how it's been designed.

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