Re: [PATCH net-next 05/10] net: dsa: qca8k: skip MDIO bus creation if its OF node has status = "disabled"

[Vladimir Oltean <vladimir.oltean@nxp.com>]

On Thu, Jan 04, 2024 at 11:19:20PM -0300, Luiz Angelo Daros de Luca wrote:
> Don't you still need to put the node that is not available? Just put
> it unconditionally whenever you exit this function after you get it.
> of_node_put() can handle even NULL.

You're right. I've prepared a patch to handle this case correctly.
I don't think it's worth sending to 'net' now that 'net-next' has
closed, because as you say below, it's quite possible that the
!of_device_is_available() code path is never exercised by existing
device trees. So the bug is like the tree that falls in the forest but
nobody hears it. I will submit the correction when net-next reopens,
together with Alvin's suggested "err" -> "ret" renaming.

> I'm not sure if this and other simple switches can be useful without a
> valid MDIO.

Will that always be the case? As implausible as this may sound, I've
received DSA questions from people using the sja1105 as a two-port
adapter between MII on the CPU side and RMII on the PHY side. It was the
cheapest way of adapting their SoC to RMII, using a switch as not even a
port multiplier. I see that AR8237 has 1x RGMII and 1x SerDes, so maybe
somebody would want to use it that way, and sidestep the internal PHYs?
I don't know.

> Anyway, wouldn't it be equivalent to having an empty mdio
> node? It looks like it would work as well but without a specific code
> path.

I guess you could also express this that way too. Although, in case it
matters, an 'empty node' has to pass schema validation (has to have all
required properties), and a disabled one doesn't.

The idea with this patch was to deliberately change the status = "disabled"
handling that the driver already had, to make things more consistent
across the board. Each driver binding has its own unique interpretation
of an absent MDIO OF node already. Some consider the OF node optional
and thus register it anyway, some say that absent means it's not needed.
But I think status = "disabled" should be an unambiguous way to specify
through DT that the hardware component is disabled. This is not how
qca8k was interpreting it prior to this change.