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

[Hector Martin <marcan@xxxxxxxxx>]

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.

- Hector