[PATCH 4/6] platform/apple: Add new Apple Mac SMC driver
Lee Jones
lee at kernel.org
Mon Oct 31 02:29:26 PDT 2022
On Mon, 31 Oct 2022, Hector Martin wrote:
> On 31/10/2022 17.48, Lee Jones wrote:
> > On Sat, 29 Oct 2022, Hector Martin wrote:
> >
> >> On 09/09/2022 16.50, Lee Jones wrote:
> >>>> What's the point of just having effectively an array of mfd_cell and
> >>>> wrappers to call into the mfd core in the drivers/mfd/ tree and the
> >>>> rest of the driver elsewhere?
> >>>
> >>> They should be separate drivers, with MFD registering the Platform.
> >>
> >> Why? What purpose does this serve? I'm still confused. There's one
> >> parent device, which provides services to the child devices. There isn't
> >> one parent device which wraps a platform service which is used by
> >> children. This makes no sense. The platform device is the root, if it
> >> exposes MFD services, then it has to be in that direction, not the other
> >> way around.
> >>
> >> Look at how this patch series is architected. There is smc_core.c, which
> >> implements SMC helpers and wrappers on top of a generic backend, and
> >> registers with the MFD subsystem. And then there is smc_rtkit.c which is
> >> the actual platform implementation on top of the RTKit framework, and is
> >> the actual platform device entry point.
> >>
> >> A priori, the only thing that makes sense to me right now would be to
> >> move smc_core.c into drivers/mfd, and leave smc_rtkit.c in platform.
> >> That way the mfd registration would be in drivers/mfd (as would be the
> >> services offered to sub-drivers), but the actual backend implementation
> >> would be in platform/ (and there would eventually be others, e.g. at
> >> least two more for x86 systems). That does mean that the driver entry
> >> point will be in platform/, with mfd/smc_core.c serving as effectively
> >> library code to plumb in the mfd stuff into one of several possible
> >> platform devices. Would that work for you?
> >
> > Yes, sounds sensible. However, keep all of the abstraction craziness
> > somewhere else and fetch and share all of your shared resources from
> > the MFD (SMC) driver.
>
> I'm not sure what you mean by that. The abstraction (smc_core.c) *is*
> the shared resource. All it does is wrap ops callbacks with a mutex and
> add a couple helpers for finding keys. Do you literally want us to just
> have this in drivers/mfd?
>
> // SPDX-License-Identifier: GPL-2.0-only OR MIT
> /*
> * Apple SMC MFD wrapper
> * Copyright The Asahi Linux Contributors
> */
>
> #include <linux/device.h>
> #include "smc.h"
>
> static const struct mfd_cell apple_smc_devs[] = {
> {
> .name = "macsmc-gpio",
> },
> {
> .name = "macsmc-hid",
> },
> {
> .name = "macsmc-power",
> },
> {
> .name = "macsmc-reboot",
> },
> {
> .name = "macsmc-rtc",
> },
> };
>
> int apple_smc_add_mfd_devices(struct device *dev)
> {
> ret = mfd_add_devices(dev, -1, apple_smc_devs,
> ARRAY_SIZE(apple_smc_devs), NULL, 0, NULL);
> if (ret)
> return dev_err_probe(dev, ret, "Subdevice initialization failed");
>
> return 0;
> }
> EXPORT_SYMBOL(apple_smc_add_mfd_devices);
>
> int apple_smc_remove_mfd_devices(struct device *dev)
> {
> mfd_remove_devices(smc->dev);
>
> return 0;
> }
> EXPORT_SYMBOL(apple_smc_add_mfd_devices);
>
> MODULE_AUTHOR("Hector Martin <marcan at marcan.st>");
> MODULE_LICENSE("Dual MIT/GPL");
> MODULE_DESCRIPTION("Apple SMC MFD wrapper");
>
> Because this feels *immensely* silly and pointless.
... and hacky. I agree.
[BTW: if this is all you want to do, have you considered simple-mfd?]
No, I want you to author a proper MFD device.
The hardware you're describing in this submission *is* an MFD. So use
the subsystem properly, instead of abusing it as a shim API to simply
register platform devices.
Request the device-wide memory (and other shared resources) here.
Conduct core operations and initialisation here, then call into your
Platform and other child devices to initiate the real work.
--
Lee Jones [李琼斯]
More information about the linux-arm-kernel
mailing list