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

[Lee Jones <lee@xxxxxxxxxx>]

On Mon, 31 Oct 2022, Russell King (Oracle) wrote:

> 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@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.
> > 
> > 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.

That might be helpful, thanks.

Let's keep in on-list for now, in case others are following along.

For now, I'll go take a look at your other response.

-- 
Lee Jones [李琼斯]