Message-ID: <20260116140237.kfkegpkubzn7l63g@skbuf>
Date: Fri, 16 Jan 2026 16:02:37 +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 Fri, Jan 16, 2026 at 01:23:45PM +0000, Lee Jones wrote:
> Please send me the full and finalised DTS hunk.

I gave it to you earlier in this thread, it is (2) from:
https://lore.kernel.org/netdev/20260109121432.lu2o22iijd4i57qq@skbuf/
(the actual device tree has more irrelevant properties, the above is
just the relevant skeleton)

With the mention that in current device trees, the "regs" node and its
underlying hierachy is missing, and patch 14 from this patch set uses
the of_changeset API to dynamically fill it in before calling
mfd_add_devices().

> > I think you are missing patch 14.
> 
> Right, I hadn't seen that.
> 
> Would I be correct in saying that you're pulling out information from
> DT, then populating MFD cells with it?

No.
I am _creating_ information in DT based on hardcoded resources in the
driver, in order to probe MFD children on them.

The reason is because I didn't need to describe those children in DT
thus far, and still don't, except for the situation where they need to
operate in a non-default configuration, which is board-specific.
More concretely, I need board DTS writers to be able to add the
"rx-polarity" and "tx-polarity" properties to the ethernet-pcs children.
These properties are handled by a different driver. The DSA driver
stands in their way, so this patch set gives an OF node to the
ethernet-pcs driver to customize whatever it needs.

If no custom DT property needs to be specified by the board, it can just
as well omit specifying the "regs" node and its children, and the driver
will fill it in all the same.

> If so, that is one of the
> reasons I like to be able to keep an eye on how the MFD API is being
> used.  Populating one device registration API from another is also not
> allowed and has been the source of some of the most contentious
> submissions I've seen.
> 
> Looks like I briefly mentioned this before:
> 
>  "I've explained why liberalising the mfd_*() API is a bad idea.  "Clever"
>   developers like to do some pretty crazy stuff involving the use of
>   MULTIPLE DEVICE REGISTRATION APIS SIMULTANEOUSLY.  I've also seen some       <-----------
>   bonkers methods of DYNAMICALLY POPULATING MFD CELLS [*ahem* Patch 8          <-----------
>   =;-)] and various other things.  Keeping the API in-house allows me to
>   keep things simple, easily readable and maintainable."
> 
> Add in a few function pointers to be used as un-debugable call-backs and
> you're well on the way getting a perfect score for breaking all of the
> guidelines that I use to keep code "simple, easily readable and
> maintainable"!   ;)

Ok, but this is highly subjective to you. What you call "simple, easily
readable and maintainable" I can just as well call "does not scale
beyond sticks and stones".

> ... but if it is split-up into heterogeneous parts which get registered
> separately then it should meet the criteria.
> 
> You're hearing "once driver has been merged, it cannot be split-up in
> such as way that the MFD API cannot be used" and I'm saying the exact
> opposite of that.  It absolutely can be used if the new layout meets the
> criteria.
> 
> I'm not sure how much more I can make it.

The (DT) layout is given by the driver's history as non-MFD.

> > > Does this all boil down that pesky empty 'mdio' "container"?
> > 
> > Why do you keep calling it empty?
> 
> Because it has no compatible, unit address or any of its own values,
> which appears to make it untraversable using the present machinery.

The fact that the unit addresses are untraversable by the OF address API
is exactly the issue. They are addresses in the address space of the
Ethernet switch chip, you can't memory-map them into Linux, you have to
go through switch-specific SPI reads and writes to access them. Then you
wrap those SPI reads and writes into regmap, and you can present them to
platform device drivers and they (almost) wouldn't know any better.

> > > Or even if it doesn't: if what you have is a truly valid DT, then
> > > why not adapt drivers/of/platform.c to cater for your use-case?
> > > Then you could take your pick from whatever works better for you out
> > > of of_platform_populate(), 'simple-bus' or even 'simple-mfd'.
> > 
> > I asked 3 years ago whether there's any interest in expanding
> > of_platform_populate() for IORESOURCE_REG and there wasn't any
> > response. It's a big task with overreaching side effects and you don't
> > just pick up on this on a Friday afternoon.
> 
> Enjoy your weekend - we can wait until Monday! ;)
> 
> There clearly wasn't any other users.  Or at least people that haven't
> found other ways around the issue.  But if you need it, and you can
> justify the work with a clear use-case, write support for it.

I'm not convinced that I won't be wasting my time.
I already know that this will be the case for the MDIO buses, because I
didn't have the inspiration at the time to create a "regs" node and to
describe their resource there. So their unit address is a random
"mdio@0" and "mdio@1", and obviously of_platform_populate() ->
of_address_to_resource() can't work with that.

For the children of the "regs" node, maybe this could help, but then I'd
run into the "multiple registration APIs being used" thing that you also
dislike (and which is currently _not_ the case).

So I pretty much know that of_platform_populate() won't help with my
currently established dt-bindings. Changing them is highly unattractive.

> > > > > given the points you've put forward, I would be content for you
> > > > > to house the child device registration (via mfd_add_devices) in
> > > > > drivers/mfd if you so wish.
> > > > 
> > > > Thanks! But I don't know how this helps me :)
> > > > 
> > > > Since your offer involves changing dt-bindings in order to
> > > > separate the MFD parent from the DSA switch (currently the DSA
> > > > driver probes on the spi_device, clashing with the MFD parent
> > > > which wants the same thing), I will have to pass.
> > > 
> > > I haven't taken a look at the DT bindings in close enough detail to
> > > provide a specific solution, but _perhaps_ it would be possible to
> > > match the MFD driver to the existing compatible, then use the MFD
> > > driver to register the current DSA driver.
> > 
> > The MFD driver and the DSA driver would compete for the same OF node.
> > And again, you'd still return to the problem of where to attach the
> > DSA switch's sub-devices in the device tree (currently to the "mdios"
> > and "regs" child nodes, which MFD doesn't support probing on, unless
> > we apply the mfd_cell.parent_of_node patch).
> 
> No, the MFD driver would adopt the compatible and register the DSA
> driver for device-driver matching. You could then obtain the node for
> parsing the DSA using node->parent.

By node->parent do you actually mean device_set_node(dsa_dev, mfd_dev->fwnode)?
We're not addressing the central problem that the spi_device doesn't
have DT bindings that conform to both the DSA expectations and to the
MFD expectations. That's where my "regs" node comes in, but you don't
like it because it involves the parent_of_node patch.

> I suspect you'd be better off manually crawling through the 'mdio' cell
> in the child drivers using node->parent.
> 
> > > However, after this most recent exchange, I am even less confident
> > > that using the MFD API to register only 2 MDIO controllers is the
> > > right thing to do.
> > > 
> > > > Not because I insist on being difficult, but because I know that
> > > > when I change dt-bindings, the old ones don't just disappear and
> > > > will continue to have to be supported, likely through a separate
> > > > code path that would also increase code complexity.
> > > 
> > > Right, they have to be backwardly compatible, I get that.
> > > 
> > > > > Although I still don't think modifying the core to ignore
> > > > > bespoke empty "container" nodes is acceptable.  It looks like
> > > > > this was merged without a proper DT review.  I'm surprised that
> > > > > this was accepted.
> > > > 
> > > > There was a debate when this was accepted, but we didn't come up
> > > > with anything better to fulfill the following constraints: - As
> > > > per mdio.yaml, the $nodename has to follow the pattern:
> > > > '^mdio(-(bus|external))?(@.+|-([0-9]+))?$' - There are two MDIO
> > > > buses. So we have to choose the variant with a unit-address (both
> > > > MDIO buses are for internal PHYs, so we can't call one "mdio" and
> > > > the other "mdio-external"). - Nodes with a unit address can't be
> > > > hierarchical neighbours with nodes with no unit address
> > > > (concretely: "ethernet-ports" from
> > > > Documentation/devicetree/bindings/net/ethernet-switch.yaml, the
> > > > main schema that the DSA switch conforms to). This is because
> > > > their parent either has #address-cells = <0>, or #address-cells =
> > > > <1>. It can't simultaneously have two values.
> > > > 
> > > > Simply put, there is no good place to attach child nodes with unit
> > > > addresses to a DT node following the DSA (or the more general
> > > > ethernet-switch) schema. The "mdios" container node serves exactly
> > > > that adaptation purpose.
> > > > 
> > > > I am genuinely curious how you would have handled this better, so
> > > > that I also know better next time when I'm in a similar situation.
> > > > 
> > > > Especially since "mdios" is not the only container node with this
> > > > issue. The "regs" node proposed in patch 14 serves exactly the
> > > > same purpose (#address-cells adaptation), and needs the exact same
> > > > ".parent_of_node = regs_node" workaround in the mfd_cell.
> > > 
> > > Please correct me if I'm wrong, but from what I have gathered, all
> > > you're trying to do here is probe a couple of child devices
> > > (controllers, whatever) and you've chosen to use MFD for this
> > > purpose because the other, more generic machinery that would
> > > normally _just work_ for simple scenarios like this, do not because
> > > you are attempting to support a non-standard DT.  Or at least one
> > > that isn't supported.
> > 
> > Sorry, what makes the DT non-standard?
> 
> The fact that the current OF APIs can't parse / traverse it.

Are there any standards in this area that I can refer to?

> 
> > > With that in mind, some suggestions going forward in order of
> > > preference:
> > > 
> > > - Adapt the current auto-registering infrastructure to support your
> > > DT layout - of_platform_populate(), simple-bus, simple-mfd, etc -
> > > Use fundamental / generic / flexible APIs that do not have specific
> > > rules - platform_*() - Move the mfd_device_add() usage into
> > > drivers/mfd - Although after this exchange, this is now my least
> > > preferred option
> > 
> > I could explore other options, but I want to be prepared to answer
> > other maintainers' question "why didn't you use MFD?". There is no
> > clear answer to that, you are not providing answers that have taken
> > all evidence into account, so that is why I'm being extremely pushy,
> > sorry.
> 
> That's fine.  If questioned, point them to this summary:
> 
> What you're doing here; attempting to reverse engineer old DTBs by
> extracting information from them to populate a different device
> registration API (DT, MFD, Plat, ACPI, etc) is not suitable for MFD due
> to reasons pertaining to; keeping things simple, easily readable and
> maintainable (see Patch 14 for an example of this).
> 
>  - MFDs should contain multiple, varying, heterogeneous devices

Check

>  - MFD parents should only use one registration API at a time

Check

>  - The MFD API should not be used outside of drivers/mfd

Not fulfilled, although I'm sorry to say, but if it comes only to this,
and the alternatives are of much higher complexity, then I'm not sure
that it really makes sense to NACK based on this.

>  - Dynamically allocating mfd_cells is STRONGLY discouraged

This comes from the multi-generational issue I am talking about (not all
generations have the same children), and dynamically generating the
cells was the most elegant way to avoid these warnings in mfd_add_device():

match:
		if (!pdev->dev.of_node)
			pr_warn("%s: Failed to locate of_node [id: %d]\n",
				cell->name, platform_id);

aka never provide a cell for a child you know you don't have.

> 
> If you think that you can match all of the above criteria, then you may
> use the MFD API.  If not, then it is not suitable for your use-case and
> you should seek other means of device registration.  I suggest;
> of_platform_populate(), platform_add_devices() [and friends],
> 'simple-bus' and 'simple-mfd', although I'm sure there are others.
> 
> -- 
> Lee Jones [李琼斯]

Powered by blists - more mailing lists