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

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

On Mon, Oct 31, 2022 at 08:46:25AM +0000, 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@xxxxxxxxx>");
> > +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.

No one seems to understand what you actually want to see with the
smc-core.c part, so I'm trying to find out what code structure
would suit you.

It seemed from the thread that moving smc-core.c to drivers/mfd
wasn't desirable, but there was the desire to move the mfd bits
into there - so that's what I've done with this patch. It doesn't
make any sense what so ever to add yet another platform device
into this structure with all of the complication around what happens
if the user forces it to unbind, so I didn't.

> Why not implement the inverse?

What do you mean "the inverse" ? The inverse of this patch is moving
everything of smc-core.c except the MFD bits into drivers/mfd leaving
the MFD bits in drivers/platform/apple, which makes no sense.

> 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? 

I thought you had previously ruled out the idea of moving the contents
of drivers/platform/apple into drivers/mfd, but maybe your position on
that had changed through the course of the discussion. It's really not
obvious to me what you want from what's been said in this thread.

So, I ask the direct question - would moving the code that is in this
patch set from drivers/platform/apple to drivers/mfd then make it
acceptable to you? In other words:

 drivers/platform/apple/smc_core.c
 drivers/platform/apple/smc.h
 drivers/platform/apple/smc_rtkit.c

If not, then please clearly and fully state what you want to see.

Thanks.

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