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

[Hector Martin <marcan@marcan.st>]

On 31/10/2022 17.46, Lee Jones wrote:
> On Fri, 28 Oct 2022, Russell King (Oracle) wrote:
> 
>> On Mon, Sep 12, 2022 at 11:55:14AM +0100, Lee Jones wrote:
>>>> I'm guessing this series is now dead, and Hector needs to re-spin the
>>>> patch set according to your views. I'm guessing this is going to take
>>>> a major re-work of the patch series.
>>>>
>>>> I suspect my attempt and trying to get this upstream has made things
>>>> more complicated, because I doubt Hector has updated his patch set
>>>> with the review comments that have been made so far... so this is
>>>> now quite a mess. I think, once this is sorted, the entire series
>>>> will need to be re-reviewed entirely afresh.
>>>
>>> I have no insight into what Hector is doing, or plans to do.
>>
>> It seems there's no plans by Hector to address this, so it comes down
>> to me.
>>
>> So, guessing what you're after, would something like the following
>> work for you? I don't see *any* point in creating more yet more
>> platform devices unless we're on a mission to maximise wasted memory
>> resources (which this split will already be doing by creating two
>> small modules instead of one.)
>>
>> Obviously, this is not an official patch yet, it's just to find out
>> what code structure you are looking for.
>>
>> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
>> index 78c6d9d99c3f..8d4c0508a2c8 100644
>> --- a/drivers/mfd/Makefile
>> +++ b/drivers/mfd/Makefile
>> @@ -18,6 +18,8 @@ obj-$(CONFIG_MFD_ENE_KB3930)	+= ene-kb3930.o
>>  obj-$(CONFIG_MFD_EXYNOS_LPASS)	+= exynos-lpass.o
>>  obj-$(CONFIG_MFD_GATEWORKS_GSC)	+= gateworks-gsc.o
>>  
>> +obj-$(CONFIG_APPLE_SMC)		+= apple-smc.o
>> +
>>  obj-$(CONFIG_HTC_PASIC3)	+= htc-pasic3.o
>>  obj-$(CONFIG_HTC_I2CPLD)	+= htc-i2cpld.o
>>  
>> diff --git a/drivers/mfd/apple-smc.c b/drivers/mfd/apple-smc.c
>> new file mode 100644
>> index 000000000000..bc59d1c5e13d
>> --- /dev/null
>> +++ b/drivers/mfd/apple-smc.c
>> @@ -0,0 +1,38 @@
>> +#include <linux/mfd/core.h>
>> +#include <linux/mfd/apple-smc.h>
>> +
>> +static const struct mfd_cell apple_smc_devs[] = {
>> +	{
>> +		.name = "macsmc-gpio",
>> +		.of_compatible = "apple,smc-gpio",
>> +	},
>> +	{
>> +		.name = "macsmc-hid",
>> +	},
>> +	{
>> +		.name = "macsmc-power",
>> +	},
>> +	{
>> +		.name = "macsmc-reboot",
>> +	},
>> +	{
>> +		.name = "macsmc-rtc",
>> +	},
>> +};
>> +
>> +int apple_smc_mfd_probe(struct device *dev)
>> +{
>> +	return mfd_add_devices(dev, -1, apple_smc_devs,
>> +			       ARRAY_SIZE(apple_smc_devs), NULL, 0, NULL);
>> +}
>> +EXPORT_SYMBOL(apple_smc_mfd_probe);
>> +
>> +void apple_smc_mfd_remove(struct device *dev)
>> +{
>> +	mfd_remove_devices(dev);
>> +}
>> +EXPORT_SYMBOL(apple_smc_mfd_remove);
>> +
>> +MODULE_AUTHOR("Hector Martin <marcan@marcan.st>");
>> +MODULE_LICENSE("Dual MIT/GPL");
>> +MODULE_DESCRIPTION("Apple SMC MFD core");
> 
> Conceptually interesting, not seen this one before, but clearly a
> hack, no?  Pretty sure all of the other cores in MFD are represented
> by a Platform Device.
> 
> Why not implement the inverse?  The Apple SMC is clearly an MFD, in
> Linux terms, so why not move the Platform Device into here, fetch all
> of the global resources, register the sub-devices, then call into the
> rtkit implementation in drivers/platform? 

Because the RTKit implementation is *one* of several possible backends,
and the others aren't even necessarily platform devices, and may have
their own registration requirements (e.g. probing for ACPI stuff on
x86). The entry points are completely different depending on the flavor.
They will have to be different modules that compile on different
architectures and load based on completely different device IDs.

This is common in Linux. There's the core xHCI driver, then an xhci-plat
wrapper for platform devices, and an xhci-pci wrapper for PCI devices.
It makes no sense to have the driver entry point be the core and then
somehow call back out to xhci-pci and xhci-plat when those are different
drivers with different match lists and different registration requirements.

It sounds like you have a mental model of what you want for MFD that
doesn't actually fit how hardware works, and you're trying to shoehorn
this into it without thinking. Linux is perfectly capable of
representing things in a way that works with this hardware, but you need
to let go of this idea that "the mfd driver lives in drivers/mfd and is
the entry point but also I don't want any platform/abstraction/silliness
in there" because that just doesn't work.

- Hector

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