Re: [PATCH v2 4/4] net: mdio: Add RTL9300 MDIO driver

[Chris Packham]

On 19/12/2024 22:40, Andrew Lunn wrote:
> On Thu, Dec 19, 2024 at 01:46:41AM -0300, Luiz Angelo Daros de Luca wrote:
> > Hello Chris,
> >
> > > +++ b/drivers/net/mdio/mdio-realtek-rtl.c
> > I wonder if the name might be dubious in the future with other realtek
> > products with MDIO. Realtek is quite a large company with many
> > products. Would a version/model/family/usage in that name help a far
> > future reader to identify what this file is about?
> Isnt rtl the family name? Or would you prefer mdio-realtek-rtl9300.c?

Yes my intention was that "rtl" was the family name. I'm happy to change 
to rtl9300.

I suspect this probably will be compatible with the rtl9310. I've just 
received a RTL9313 based board so will probably start looking at that in 
the new year.

> > > +static int realtek_mdio_wait_ready(struct realtek_mdio_priv *priv)
> > All those realtek_mdio_* prefix might collide with realtek_mdio_* from
> > drivers/net/dsa/realtek/realtek-mdio.c. This realtek_mdio_* is about a
> > Realtek SoC MDIO interface with the switch. The other realtek_mdio_*
> > is about the interface (MDIO or SMI) between (the other vendor) SoC
> > and the switch. I don't know if the maintainers are OK with it but
> > listing those symbols in alphabetic order from both sources might be
> > confusing.
> rtl9300_ as a prefix?

I'd happily  change to rtl_ or rtl9300_

> > > +static const struct of_device_id realtek_mdio_ids[] = {
> > > +       { .compatible = "realtek,rtl9301-mdio" },
> > > +       { .compatible = "realtek,rtl9302b-mdio" },
> > > +       { .compatible = "realtek,rtl9302c-mdio" },
> > > +       { .compatible = "realtek,rtl9303-mdio" },
> > Do these different compatible strings really matter? AFAIK, compatible
> > are not for listing all supported models/variants but to describe
> > devices that have a different behavior and indicating that (with
> > different strings) is needed to decide how the driver will work. If
> > the driver does not use which compatible was set, it might indicate
> > that we don't really need 4 compatible but 1.
> It can be useful when we initially think they are compatible, but
> later find out they are not, and we need different behaviour.

The way I've written the dt-binding any board should include 
"realtek,rtl9301-mdio" and may also include one of 
"realtek,rtl9302b-mdio", "realtek,rtl9302c-mdio", 
"realtek,rtl9303-mdio". For the MDIO driver the specific chip could 
possibly tell us the maximum SMI bus number. Unfortunately I've only got 
a block diagram of the RTL9302C, I know that does have 4 SMI interfaces, 
the others may have fewer. Things would probably work fine for now with 
just "realtek,rtl9301-mdio" but is there any harm in including the others?