Re: [PATCH net-next 07/15] mfd: core: add ability for cells to probe on a custom parent OF node

[Lee Jones <lee@kernel.org>]

> > Side note: The implementation is also janky.
> 
> Yes, this is why it's up for review, so I can learn why it's janky and
> fix it.

I'd be happy to discuss this in great detail if we finally conclude that
this device is suitable for MFD.

Spoiler alert: 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.

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

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

[...]

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

-- 
Lee Jones [李琼斯]