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

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Mon, Oct 31, 2022 at 05:24:53PM +0000, Lee Jones wrote:
> On Mon, 31 Oct 2022, Russell King (Oracle) wrote:
> 
> > 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@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.
> > 
> > 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.
> 
> Sorry Russell, I'm out of time today.  Please see my recent reply to
> Hector for now and I'll get back to you first thing.

Hi Lee,

Thanks - I look forward to it. Having read your response to Hector, I
am wondering whether there's a misunderstanding of the code, so I'm
hoping that my attempt in my reply helps to clear up any code
misunderstandings.

If you want to ask questions about the code, you know where to find
me on irc, and I'll more than happily answer anything you want to
know about the code structure.

Russell.

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

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