Re: [PATCH] mfd: Add Simple PCI MFD driver

[Greg Kroah-Hartman]

On Wed, Jan 25, 2023 at 02:06:26PM +0100, Vincent Whitchurch wrote:
> On Wed, Jan 25, 2023 at 01:29:32PM +0100, Greg Kroah-Hartman wrote:
> > On Wed, Jan 25, 2023 at 11:15:38AM +0100, Vincent Whitchurch wrote:
> > > I hope it's clear from my other replies in this thread that the entire
> > > purpose of this driver is to allow arbitrary platform devices to be used
> > > via a PCI device in virtual environments like User Mode Linux in order
> > > to test existing platform drivers using mocked hardware.
> > 
> > That still feels wrong, why is PCI involved here at all?
> >
> > Don't abuse platform devices like this please, mock up a platform device
> > framework instead if you want to test them that way, don't think that
> > adding a platform device "below" a PCI device is somehow allowed at all.
> 
> As you know, PCI allows exposing an MMIO region to the host, so the host
> can use ioremap() and readl()/writel() on it.  This allows reusing
> platform drivers even though the device is on the other side of a PCI
> bus.
> 
> There is hardware already supported by the kernel since a long time ago
> which is handled by putting platform devices below PCI devices.  See
> add_bus_probe() in arch/x86/kernel/devicetree.c.

Those are not platform devices below a PCI device from what I can tell,
and if it is, it's wrong and should be fixed up.  Don't perpetuate bad
design decisions of the past please.

> And this hardware also wants to do the same thing:
> 
>  https://lore.kernel.org/lkml/1674183732-5157-1-git-send-email-lizhi.hou@xxxxxxx/

That too is wrong.

That is NOT what the platform devices and drivers are for at all, make
them some other type of device please, which is why the aux bus was
created.

> Also, UML already supports out-of-process PCI, and there is ongoing work
> in QEMU to add support for out-of-process PCI emulation.  So using PCI
> will allow this to work on different kinds of virtual environments
> without having to invent a new method specifically for platform devices.

I don't see how PCI is relevant here, sorry.

> > > The auxiliary bus cannot be used since it naturally does
> > > not support platform devices.
> > 
> > The aux bus can support any type of bus (it's there to be used as you
> > want, it's just that people are currently using it for PCI devices right
> > now).
> 
> I assume we're talking about drivers/base/auxiliary.c?  The kernel doc
> says:
> 
>  * A key requirement for utilizing the auxiliary bus is that there is no
>  * dependency on a physical bus, device, register accesses or regmap support.
>  * These individual devices split from the core cannot live on the platform bus
>  * as they are not physical devices that are controlled by DT/ACPI.
> 
> But this case the sub-devices do need standard register access with
> readl()/writel() and _are_ controlled by devicetree.

Ok, let's make a new bus for them then.  As obviously they are not
platform devices if they live on the PCI bus.

> > > A hard coded list of sub-devices cannot be used since arbitrary
> > > platform devices with arbitrary devicetree properties need to be
> > > supported.
> > 
> > Then make a new bus type and again, do not abuse platform devices.
> 
> How can existing platform drivers be re-used if you invent a new bus
> type and don't create platform devices?

The same way we reuse lots of drivers for devices on different busses
today.

> > > I could move this driver to drivers/bus/ and pitch it as a
> > > "PCI<->platform bridge for testing in virtual environments", if that
> > > makes more sense.
> > 
> > Again, nope, a platform device is NOT ever a child of a PCI device.
> > That's just not how PCI works at all.
> > 
> > Would you do the attempt to do this for USB?  (hint, no.)  So why is PCI
> > somehow special here?
> 
> PCI is special because it allows exposing an MMIO region to the host and
> allowing the host to access it like any other I/O memory.  USB doesn't
> allow that.

So you are saying that just because a bus type exports MMIO regions, it
should be allowed to be a platform device?  Sorry, but make that a new
bus type as obviously it is something else.  Maybe these are all just
real PCI devices (or sub devices), so treat them like that please.

thanks,

greg k-h