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

[Rob Herring]

On Wed, Jan 25, 2023 at 6:29 AM Greg Kroah-Hartman
<gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On Wed, Jan 25, 2023 at 11:15:38AM +0100, Vincent Whitchurch wrote:
> > On Mon, Jan 23, 2023 at 05:31:21PM +0100, Greg Kroah-Hartman wrote:
> > > On Mon, Jan 23, 2023 at 03:32:55PM +0000, Lee Jones wrote:
> > > > On Mon, 23 Jan 2023, Vincent Whitchurch wrote:
> > > >
> > > > > Add a PCI driver which registers all child nodes specified in the
> > > > > devicetree.  It will allow platform devices to be used on virtual
> > > > > systems which already support PCI and devicetree, such as UML with
> > > > > virt-pci.
> > > > >
> > > > > The driver has no id_table by default; user space needs to provide one
> > > > > using the new_id mechanism in sysfs.
> > > >
> > > > This feels wrong for several reasons.
> > > >
> > > > Firstly, I think Greg (Cc:ed) will have something to say about this.
> > >
> > > Yes, this isn't ok.  Please write a real driver for the hardware under
> > > control here, and that would NOT be a MFD driver (hint, if you want to
> > > split up a PCI device into different drivers, use the aux bus code, that
> > > is what it is there for.)
> >
> > 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.

My question as well. However, that's only for Vincent's usecase. The
other ones I'm aware of are definitely non-discoverable MMIO devices
behind a PCI device.

It is perfectly valid in DT to have the same device either directly on
an MMIO bus or behind some other MMIO capable bus. So what bus type
should they all be?

> > Given this "hardware", it's not clear what a "real driver" would do
> > differently.
>
> Again, you can not have a platform device below a PCI device, that's not
> what a platform device is for at all.
>
> > 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).
>
> > 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 about of_platform_bus[1]?

At this point, it would be easier to create a new bus type for
whatever it is you think *should* be a platform device and move those
to the new bus leaving platform_bus as the DT/ACPI devices bus.

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

Actually, yes. It is limited since USB cannot tunnel MMIO accesses
(though I suppose USB4 PCIe tunneling can?), but we do have some
platform drivers which don't do MMIO.

Suppose I have an FTDI chip with GPIOs on it and I want to do GPIO
LEDs, keys, bitbanged I2C, etc. Those would use the leds-gpio,
gpio-keys, i2c-gpio platform drivers.

Rob

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=eca3930163ba8884060ce9d9ff5ef0d9b7c7b00f