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

[Lee Jones <lee@xxxxxxxxxx>]

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@xxxxxxxxx>");
> 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 [李琼斯]