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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <fe34b6b0-01fd-4dc7-a1b4-6c27ad2c9e74@alliedtelesis.co.nz>
Date: Fri, 20 Dec 2024 09:36:37 +1300
From: Chris Packham <chris.packham@...iedtelesis.co.nz>
To: Andrew Lunn <andrew@...n.ch>,
 Luiz Angelo Daros de Luca <luizluca@...il.com>
Cc: lee@...nel.org, robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
 andrew+netdev@...n.ch, davem@...emloft.net, edumazet@...gle.com,
 kuba@...nel.org, pabeni@...hat.com, tsbogend@...ha.franken.de,
 hkallweit1@...il.com, linux@...linux.org.uk, markus.stockhausen@....de,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 netdev@...r.kernel.org, linux-mips@...r.kernel.org
Subject: Re: [PATCH v2 4/4] net: mdio: Add RTL9300 MDIO driver


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?