On Thu, Jun 30, 2022 at 02:05:39PM -0300, Luiz Angelo Daros de Luca wrote:

quoted

Hi Luiz,

On Wed, Jun 29, 2022 at 01:43:45PM -0300, Luiz Angelo Daros de Luca wrote:

quoted

This RFC patch series cleans realtek-smi custom slave mii bus. Since
fe7324b932, dsa generic code provides everything needed for
realtek-smi driver. For extra caution, this series should be applied
in two steps: the first 2 patches introduce the new code path that
uses dsa generic code. It will show a warning message if the tree
contains deprecated references. It will still fall back to the old
code path if an "mdio"
is not found.

In principle I like your changes, but I'm not sure if what you are doing
is allowed, since DT is ABI. The fact that you have to split this into
two steps, with the first step warning about old "incompatible" DTs
(your point 3 below) before the second step breaks that compatibility,
suggests that you are aware that you could be breaking old DTs.

Thanks Alvin for your review. Yes, that is a good question for the ML.
I don't know at what level we can break compatibility (DT and driver).
That's why it is a RFC.

DT bindings are only extended in backwards-compatible ways. Only in the
case where you can prove that there is no DT user of a certain binding,
and that none should appear either, is when you can consider breaking
the backward compatibility. The idea here is that old DT blobs may live
forever and be provided by fixed firmware such as U-Boot, you can't
really force anyone to update them.

quoted

I'm not going to argue with you if you say "but the node with compatible
realtek,smi-mdio was also called mdio in the bindings, so it shouldn't
break old DTs", which is a valid point. But if that is your rationale,
then there's no need to split the series at all, right?

The DT requires "realtek,smi-mdio" but also mentions the "mdio" name,
not a generic name as "mdioX". If we agree that the name "mdio" is
already required by the DT bindings, it is the driver implementation
that is not compliant. Even if we are not violating the DT bindings,
we are changing the driver behavior. That's why I suggested the
transition process. I do believe that it would be very, very rare to
name that mdio as anything other than "mdio" and even the driver
itself is too fresh to be widespread. In a non-RFC series, I would
also drop the "realtek,smi-mdio" compatible string from the bindings
(as it is back compatible).

Technically the MDIO node should be named "mdio", at least that's what
Documentation/devicetree/bindings/net/mdio.yaml says, that it should be
"mdio" or "mdio@something". And since the #address-cells of the DSA
switch OF node itself (parent of MDIO node) is 0, it can't really be
"mdio@something", so that leaves us with "mdio".

However I don't think there is any validation for the node name.

quoted

If you want to avoid that debate, what you could do instead is add a
const char *slave_mii_compatible; member to struct dsa_switch, and try
searching in dsa_switch_setup() for a child node with that compatible if
the lookup of a node named "mdio" fails. I don't know if this would help
you do the same thing with other drivers.

The DSA change to accept "mdio" was an improvement to avoid adding a
custom slave mdio when you already have a single mdio and just need to
point to a DT node. Adding compatible strings for that situation does
not make much sense as a compatible string is not necessary when you
are already restricting your case to a single mdio. For more complex
setups, you still need to create your own slave mdio implementation.
Some drivers already depend on the "mdio" name and this series is also
a suggestion for them to try their drivers dropping their custom slave
mdio implementations.

I think we are going down a slippery slope here. DSA providing generic
code to register an MDIO bus is not something I'd consider core
functionality, and I don't consider any duplication of concerns or logic
if drivers register their own MDIO buses.

Here you are effectively proposing to gratuitously break the DT binding,
the obvious question is, what is there to even gain from this?

And why do you even need to remove the compatible string from the MDIO
node, can't you just ignore it, does it bother you in any way?