Date:   Sat, 18 Dec 2021 03:08:56 -0300
From:   Luiz Angelo Daros de Luca <luizluca@...il.com>
To:     Alvin Šipraga <ALSI@...g-olufsen.dk>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linus.walleij@...aro.org" <linus.walleij@...aro.org>,
        "andrew@...n.ch" <andrew@...n.ch>,
        "vivien.didelot@...il.com" <vivien.didelot@...il.com>,
        "f.fainelli@...il.com" <f.fainelli@...il.com>,
        "olteanv@...il.com" <olteanv@...il.com>,
        "arinc.unal@...nc9.com" <arinc.unal@...nc9.com>
Subject: Re: [PATCH net-next 07/13] net: dsa: rtl8365mb: rename rtl8365mb to rtl8367c

Hi Alvin,

> I don't think the current name is really hurting anybody. And as Arınç
> says, it’s a huge chunk of change with no real benefits.

Until now, the name was used internally and it would only matter for
developers. However now it will be exposed to userland. That is the
name they will need to find if a device is supported.

The rtl8367c was simply the name used by Realtek API distributed to
its patterns. It eventually got into multiple devices, including
Arduino, u-boot, and Linux kernel.

It is a nice game to decode Realtek product names. Realtek seems to
like to use letters as versions, but only with the second generation.
Letters like "S", "M", "N", "R" are some kind of feature indicator. If
you find a device rtl8366sc, it is likely there is a
rtl8366sb and probably a rtl8366s. but never a rtl8366.

So we have:

RTL - Realtek
83 - switch controllers?
6 - family
7 - number of ports, including external with 0 as 10
S, M, M, R - features. Might include an extra "-V" suffix
B, C, D... variant indicator. Even the extra suffix might use it

When you get a new product and a name collides, you add the next
variant suffix. It is not a generation indicator, just a way to avoid
collisions when two chips would have the same name.

And about the driver's name? Well, at first they might have worked
with a generic rtl8367 device and used that as the driver name. If it
was a Linux driver, it would probably be called rtl836x. What do they
do when a new driver version is needed? Use the same logic. So, we
have driver versions rtl8367, rtl8367b, rtl8367c, rtl8367d... And I
don't think there is a major breakage between these versions that
justify having them in parallel inside the Linux kernel. And it also
applies to rtl8366rb and rtl8365mb. From what I see, both are
independently doing the same thing, with different features
implemented. A good path would be to merge these two drivers into a
single rtl836x module. Maybe it could start by renaming
rtl8366(-core).c to rtl836x.c and try to migrate code from subdrivers
into this file, and create a shared register header file.

Yes, I agree the name rtl8367c is not good at all. I'll remove the
rename patch but it will take some time. I'll be two weeks away.

> Besides, not all renaming seems consistent - what is mb to mean here?:
>
> -       struct rtl8365mb *mb = priv->chip_data;
> +       struct rtl8367c *mb = priv->chip_data;

As I said, I do care about names and I have no idea. I was hoping that
you would know.

> The naming scheme for rtl8366rb is also mysterious - why not fix that
> too? Is that rtl8367b?

If we follow the same logic, yes, it would be rtl8367b. However, I
think the right path is to have a single rtl836x driver.
That name would be perfect for a user to find the correct driver by name.

> Honestly it seems like more effort than it is worth. The comments at the
> top of the driver should be sufficient to explain to any future
> developer what exactly is going on. If something is unclear, why not
> just add/remove some lines there?

End users do not read driver comments. It is more likely that they
will read kernel docs first.

> Since you don't feel strongly about the name, I would suggest you drop
> the renaming from your MDIO/RTL8367S series for now. It will also make
> the review process a bit easier.

Agreed

>
>         Alvin

Powered by blists - more mailing lists