Message-ID: <20251216162447.erl5cuxlj7yd3ktv@skbuf>
Date: Tue, 16 Dec 2025 18:24:47 +0200
From: Vladimir Oltean <vladimir.oltean@....com>
To: Lee Jones <lee@...nel.org>
Cc: netdev@...r.kernel.org, Andrew Lunn <andrew@...n.ch>,
	Heiner Kallweit <hkallweit1@...il.com>,
	Russell King <linux@...linux.org.uk>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next 07/15] mfd: core: add ability for cells to probe
 on a custom parent OF node

On Tue, Dec 16, 2025 at 09:18:31AM +0000, Lee Jones wrote:
> Unless you add/convert more child devices that are outside of net/ and
> drivers/net AND move the core MFD usage to drivers/mfd/, then we can't
> conclude that [ this device is suitable for MFD ].

To me, the argument that child devices can't all be under drivers/net/
is superficial. An mii_bus is very different in purpose from a phylink_pcs
and from a net_device, yet all 3 live in drivers/net/.

Furthermore, I am looking at schemas such as /devicetree/bindings/mfd/adi,max77541.yaml:
"MAX77540 is a Power Management IC with 2 buck regulators."
and I don't understand how it possibly passed this criterion. It is one
chip with two devices of the same kind, and nothing else.


If moving the core MFD usage to drivers/mfd/ is another hard requirement,
this is also attacking form rather than substance. You as the MFD
maintainer can make an appeal to authority and NACK aesthetics you don't
like, but I just want everyone to be on the same page about this.

> > > There does appear to be at least some level of misunderstanding between
> > > us.  I'm not for one moment suggesting that a switch can't be an MFD. If
> > > it contains probe-able components that need to be split-up across
> > > multiple different subsystems, then by all means, move the core driver
> > > into drivers/mfd/ and register child devices 'till your heart's content.
> > 
> > Are you still speaking generically here, or have you actually looked at
> > any "nxp,sja1105q" or "nxp,sja1110a" device trees to see what it would
> > mean for these compatible strings to be probed by a driver in drivers/mfd?
> 
> It's not my role to go digging into existing implementations and
> previous submissions to prove whether a particular submission is
> suitable for inclusion into MFD.
> 
> Please put in front of me, in a concise way (please), why you think this
> is fit for inclusion.

No new information, I think the devices are fit for MFD because of their
memory map which was shown in the previous reply.

> I've explained what is usually required, but I'll (over-)simplify
> again for clarity:
> 
>  - The mfd_* API call-sites must only exist in drivers/mfd/
>    - Consumers usually spit out non-system specific logic into a 'core'
>  - MFDs need to have more than one child
>    - This is where the 'Multi' comes in
>  - Children should straddle different sub-systems
>    - drivers/net is not enough [0]
>    - If all of your sub-devices are in 'net' use the platform_* API
>  - <other stipulations less relevant to this stipulation> ...
> 
> There will always be exceptions, but previous mistakes are not good
> justifications for future ones.
> 
> [0]
> 
>   .../bindings/net/dsa/nxp,sja1105.yaml         |  28 +
>    .../bindings/net/pcs/snps,dw-xpcs.yaml        |   8 +
>    MAINTAINERS                                   |   2 +
>    drivers/mfd/mfd-core.c                        |  11 +-
>    drivers/net/dsa/sja1105/Kconfig               |   2 +
>    drivers/net/dsa/sja1105/Makefile              |   2 +-
>    drivers/net/dsa/sja1105/sja1105.h             |  42 +-
>    drivers/net/dsa/sja1105/sja1105_main.c        | 169 +++---
>    drivers/net/dsa/sja1105/sja1105_mdio.c        | 507 ------------------
>    drivers/net/dsa/sja1105/sja1105_mfd.c         | 293 ++++++++++
>    drivers/net/dsa/sja1105/sja1105_mfd.h         |  11 +
>    drivers/net/dsa/sja1105/sja1105_spi.c         | 113 +++-
>    drivers/net/mdio/Kconfig                      |  21 +-
>    drivers/net/mdio/Makefile                     |   2 +
>    drivers/net/mdio/mdio-regmap-simple.c         |  77 +++
>    drivers/net/mdio/mdio-regmap.c                |   7 +-
>    drivers/net/mdio/mdio-sja1110-cbt1.c          | 173 ++++++
>    drivers/net/pcs/pcs-xpcs-plat.c               | 146 +++--
>    drivers/net/pcs/pcs-xpcs.c                    |  12 +
>    drivers/net/phy/phylink.c                     |  75 ++-
>    include/linux/mdio/mdio-regmap.h              |   2 +
>    include/linux/mfd/core.h                      |   7 +
>    include/linux/pcs/pcs-xpcs.h                  |   1 +
>    include/linux/phylink.h                       |   5 +
>    24 files changed, 1033 insertions(+), 683 deletions(-)
>    delete mode 100644 drivers/net/dsa/sja1105/sja1105_mdio.c
>    create mode 100644 drivers/net/dsa/sja1105/sja1105_mfd.c
>    create mode 100644 drivers/net/dsa/sja1105/sja1105_mfd.h
>    create mode 100644 drivers/net/mdio/mdio-regmap-simple.c
>    create mode 100644 drivers/net/mdio/mdio-sja1110-cbt1.c
> 
> > What OF node would remain for the DSA switch (child) device driver? The same?
> > Or are you suggesting that the entire drivers/net/dsa/sja1105/ would
> > move to drivers/mfd/? Or?
> 
> See bullet 1.1 above.
> 
> [...]
> 
> > > I don't recall those discussions from 3 years ago, but the Ocelot
> > > platform, whatever it may be, seems to have quite a lot more
> > > cross-subsystem device support requirements going on than I see here:
> > > 
> > > drivers/i2c/busses/i2c-designware-platdrv.c
> > > drivers/irqchip/irq-mscc-ocelot.c
> > > drivers/mfd/ocelot-*
> > > drivers/net/dsa/ocelot/*
> > > drivers/net/ethernet/mscc/ocelot*
> > > drivers/net/mdio/mdio-mscc-miim.c
> > > drivers/phy/mscc/phy-ocelot-serdes.c
> > > drivers/pinctrl/pinctrl-microchip-sgpio.c
> > > drivers/pinctrl/pinctrl-ocelot.c
> > > drivers/power/reset/ocelot-reset.c
> > > drivers/spi/spi-dw-mmio.c
> > > net/dsa/tag_ocelot_8021q.c
> > 
> > This is a natural effect of Ocelot being "whatever it may be". It is a
> > family of networking SoCs, of which VSC7514 has a MIPS CPU and Linux
> > port, where the above drivers are used. The VSC7512 is then a simplified
> > variant with the MIPS CPU removed, and the internal components controlled
> > externally over SPI. Hence MFD to reuse the same drivers as Linux on
> > MIPS (using MMIO) did. This is all that matters, not the quantity.
> 
> From what I can see, Ocelot ticks all of the boxes for MFD API usage,
> whereas this submission does not.  The fact that the overarching device
> provides a similar function is neither here nor there.
> 
> These are the results from my searches of your device:
> 
>   git grep -i SJA1110 | grep -v 'net\|arch\|include'
>   <no results>
> 
> [...]
> 
> > > My point is, you don't seem to have have any of that here.
> > 
> > What do you want to see exactly which is not here?
> > 
> > I have converted three classes of sub-devices on the NXP SJA1110 to MFD
> > children in this patch set. Two MDIO buses and an Ethernet PCS for SGMII.
> > 
> > In the SJA1110 memory map, the important resources look something like this:
> > 
> > Name         Description                                         Start      End
> > SWITCH       Ethernet Switch Subsystem                           0x000000   0x3ffffc
> > 100BASE-T1   Internal MDIO bus for 100BASE-T1 PHY (port 5 - 10)  0x704000   0x704ffc
> > SGMII1       SGMII Port 1                                        0x705000   0x705ffc
> > SGMII2       SGMII Port 2                                        0x706000   0x706ffc
> > SGMII3       SGMII Port 3                                        0x707000   0x707ffc
> > SGMII4       SGMII Port 4                                        0x708000   0x708ffc
> > 100BASE-TX   Internal MDIO bus for 100BASE-TX PHY                0x709000   0x709ffc
> 
> All in drivers/net.
> 
> > ACU          Auxiliary Control Unit                              0x711000   0x711ffc
> > GPIO         General Purpose Input/Output                        0x712000   0x712ffc
> 
> Where are these drivers?

For the GPIO I have no driver yet.

For the ACU, there is a reusable group of 4 registers for which I wrote
a cascaded interrupt controller driver in 2022. This register group is
instantiated multiple times in the SJA1110, which justified a reusable
driver.

Upstreaming it was blocked by the inability to instantiate it from the
main DSA driver using backwards-compatible DT bindings.

In any case, on the older generation SJA1105 (common driver with SJA1110),
the GPIO and interrupt controller blocks are missing. There is an ACU
block, but it handles just pinmux and pad configuration, and the DSA
driver programs it directly rather than going through the pinmux subsystem.

This highlights a key requirement I have from the API for instantiating
sub-devices: that it is sufficiently flexible to split them out of the
main device when that starts making sense (we identify a reusable block,
or we need to configure it in the device tree, etc). Otherwise, chopping
up the switch address space upfront is a huge overhead that may have no
practical gains.

> 
> > I need to remind you that my purpose here is not to add drivers in
> > breadth for all SJA1110 sub-devices now.
> 
> You'll see from my discussions with Colin, sub-drivers (if they are to
> be used for MFD justification (point 3 above), then they must be added
> as part of the first submission.  Perhaps this isn't an MFD, "yet"?
> 
> [...]

IMHO, the concept of being or not being MFD "yet" is silly. Based on the
register map, you are, or are not.

> > The SGMII blocks are highly reusable IPs licensed from Synopsys, and
> > Linux already has DT bindings and a corresponding platform driver for
> > the case where their registers are viewed using MMIO.
> 
> This is a good reason for dividing them up into subordinate platform
> devices.  However, it is not a good use-case of MFD.  In it's current
> guise, your best bet is to use the platform_* API directly.
> 
> This is a well trodden path and it not challenging:
> 
>   % git grep platform_device_add -- arch drivers sound | wc -l
>   398
> 
> [...]
> 
> > In my opinion I do not need to add handling for any other sub-device,
> > for the support to be more "cross-system" like for Ocelot. What is here
> > is enough for you to decide if this is adequate for MFD or not.
> 
> Currently ... it's not.
> 
> [...]
> 
> Hopefully that helps to clarify my expectations a little.
> 
> TL;DR, this looks like a good candidate for direct platform_* usage.

I do have a local branch with platform devices created manually, and yet,
I considered the mfd_add_devices() form looked cleaner when submitting.

I expect the desire to split up reusable register regions into platform
sub-devices to pop up again, so the logic should be available as library
code at some level (possibly DSA).

Unless you have something against the idea, I'm thinking a good name for
this library code would be "nmfd", for "Not MFD". It is like MFD, except:
- the parent can simultaneously handle the main function of the device
  while delegating other regions to sub-devices
- the sub-devices can all have drivers in the same subsystem (debatable
  whether MFD follows this - just to avoid discussions)
- their OF nodes don't have to be direct children of the parent.

Powered by blists - more mailing lists