mbox series

[net-next,v3,0/8] net: dsa: realtek: variants to drivers, interfaces to a common module

Message ID 20231223005253.17891-1-luizluca@gmail.com (mailing list archive)
Headers show
Series net: dsa: realtek: variants to drivers, interfaces to a common module | expand

Message

Luiz Angelo Daros de Luca Dec. 23, 2023, 12:46 a.m. UTC
The current driver consists of two interface modules (SMI and MDIO) and
two family/variant modules (RTL8365MB and RTL8366RB). The SMI and MDIO
modules serve as the platform and MDIO drivers, respectively, calling
functions from the variant modules. In this setup, one interface module
can be loaded independently of the other, but both variants must be
loaded (if not disabled at build time) for any type of interface. This
approach doesn't scale well, especially with the addition of more switch
variants (e.g., RTL8366B), leading to loaded but unused modules.
Additionally, this also seems upside down, as the specific driver code
normally depends on the more generic functions and not the other way
around.

The series begins by removing an unused function pointer at
realtek_ops->cleanup.

Each variant module was converted into real drivers, serving as both a
platform driver (for switches connected using the SMI interface) and an
MDIO driver (for MDIO-connected switches). The relationship between the
variant and interface modules is reversed, with the variant module now
calling both interface functions (if not disabled at build time). While
in most devices only one interface is likely used, the interface code is
significantly smaller than a variant module, consuming fewer resources
than the previous code. With variant modules now functioning as real
drivers, compatible strings are published only in a single variant
module, preventing conflicts.

The patch series introduces a new common module for functions shared by
both variants. This module also absorbs the two previous interface
modules, as they would always be loaded anyway.

The series relocates the user MII driver from realtek-smi to common. It
is now used by MDIO-connected switches instead of the generic DSA
driver. There's a change in how this driver locates the MDIO node. It
now only searches for a child node named "mdio", which is required by
both interfaces in binding docs.

The dsa_switch in realtek_priv->ds is now embedded in the struct. It is
always in use and avoids dynamic memory allocation.

Testing has been performed with an RTL8367S (rtl8365mb) using MDIO
interface and an RTL8366RB (rtl8366) with SMI interface.

Luiz

---

Changes:

v2-v3:
1) Look for the MDIO bus searching for a child node named "mdio" instead
   of the compatible string.
2) Removed the check for a phy-handle in ports. ds->user_mii_bus will
   not be used anymore.
3) Dropped comments for realtek_common_{probe,register_switch}
4) Fixed a compile error in "net: dsa: OF-ware slave_mii_bus"
5) Used the wrapper realtek_smi_driver_register instead of
   platform_driver_register

v1-v2:
1)  Renamed realtek_common module to realtek-dsa.
2)  Removed the warning when the MDIO node is not named "mdio."
3)  ds->user_mii_bus is only assigned if all user ports do not have a
    phy-handle.
4)  of_node_put is now back to the driver remove method.
5)  Renamed realtek_common_probe_{pre,post} to
    realtek_common_{probe,register_switch}.
6)  Added some comments for realtek_common_{probe,register_switch}.
7)  Using dev_err_probe whenever possible.
8)  Embedded priv->ds into realtek_priv, removing its dynamic
    allocation.
9)  Fixed realtek-common.h macros.
10) Save and check the return value in functions, even when it is the
    last one.
11) Added the #if expression as a comment to #else and #endif in header
    files.
12) Unregister the platform and the MDIO driver in the reverse order
    they are registered.
13) Unregister the first driver if the second one failed to register.
14) Added the revert patch for "net: dsa: OF-ware slave_mii_bus."

Comments

Vladimir Oltean Jan. 15, 2024, 9:54 p.m. UTC | #1
On Fri, Dec 22, 2023 at 09:46:28PM -0300, Luiz Angelo Daros de Luca wrote:
> The series begins by removing an unused function pointer at
> realtek_ops->cleanup.
> 
> Each variant module was converted into real drivers, serving as both a
> platform driver (for switches connected using the SMI interface) and an
> MDIO driver (for MDIO-connected switches). The relationship between the
> variant and interface modules is reversed, with the variant module now
> calling both interface functions (if not disabled at build time). While
> in most devices only one interface is likely used, the interface code is
> significantly smaller than a variant module, consuming fewer resources
> than the previous code. With variant modules now functioning as real
> drivers, compatible strings are published only in a single variant
> module, preventing conflicts.
> 
> The patch series introduces a new common module for functions shared by
> both variants. This module also absorbs the two previous interface
> modules, as they would always be loaded anyway.
> 
> The series relocates the user MII driver from realtek-smi to common. It
> is now used by MDIO-connected switches instead of the generic DSA
> driver. There's a change in how this driver locates the MDIO node. It
> now only searches for a child node named "mdio", which is required by
> both interfaces in binding docs.
> 
> The dsa_switch in realtek_priv->ds is now embedded in the struct. It is
> always in use and avoids dynamic memory allocation.

git format-patch --cover-letter generates a nice patch series overview
with a diffstat and the commit titles, you should include it next time.
Arınç ÜNAL Jan. 17, 2024, 10:25 a.m. UTC | #2
On 16.01.2024 00:54, Vladimir Oltean wrote:
> git format-patch --cover-letter generates a nice patch series overview
> with a diffstat and the commit titles, you should include it next time.

Thanks a lot for mentioning this. I didn't know this and now that I use it,
it helps a lot.

Arınç
Linus Walleij Jan. 17, 2024, 12:48 p.m. UTC | #3
On Wed, Jan 17, 2024 at 11:26 AM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
> On 16.01.2024 00:54, Vladimir Oltean wrote:

> > git format-patch --cover-letter generates a nice patch series overview
> > with a diffstat and the commit titles, you should include it next time.
>
> Thanks a lot for mentioning this. I didn't know this and now that I use it,
> it helps a lot.

There are some even nicer tools you can use on top, i.e. "b4":
https://people.kernel.org/monsieuricon/sending-a-kernel-patch-with-b4-part-1

This blog post doesn't mention the magic trick:
b4 prep --set-prefixes net-next

And
b4 trailers -u

Which is what you need to make it an awesome net contribution
patch series tool.

Yours,
Linus Walleij
Arınç ÜNAL Jan. 20, 2024, 10:13 p.m. UTC | #4
On 17.01.2024 15:48, Linus Walleij wrote:
> On Wed, Jan 17, 2024 at 11:26 AM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
>> On 16.01.2024 00:54, Vladimir Oltean wrote:
> 
>>> git format-patch --cover-letter generates a nice patch series overview
>>> with a diffstat and the commit titles, you should include it next time.
>>
>> Thanks a lot for mentioning this. I didn't know this and now that I use it,
>> it helps a lot.
> 
> There are some even nicer tools you can use on top, i.e. "b4":
> https://people.kernel.org/monsieuricon/sending-a-kernel-patch-with-b4-part-1
> 
> This blog post doesn't mention the magic trick:
> b4 prep --set-prefixes net-next
> 
> And
> b4 trailers -u
> 
> Which is what you need to make it an awesome net contribution
> patch series tool.

I've spent a day thinking I probably don't need this. I've spent the next
day giving it a go. I need this. Diffstat, the auto formatting of new
version change log, and linking to the previous version on the cover
letter, patch version comparison, and auto adding trailers features make my
life so much easier.

I've had trouble with every mail provider's SMTP server that I've ever used
for submitting patches, so the web endpoint is a godsend. It would've been
great if b4 supported openssh keys to submit patches via the web endpoint.
Patatt at least supports it to sign patches. I've got a single ed25519
openssh keypair I use across all my devices, now I'll have to backup
another key pair. Or create a new key and authenticate with the web
endpoint on each device.

Safe to say, I will submit my next patch series using b4. Thanks for
telling me about this tool Linus!

Arınç
Konstantin Ryabitsev Jan. 29, 2024, 5:45 p.m. UTC | #5
On Sun, Jan 21, 2024 at 01:13:36AM +0300, Arınç ÜNAL wrote:
> I've had trouble with every mail provider's SMTP server that I've ever used
> for submitting patches, so the web endpoint is a godsend. It would've been
> great if b4 supported openssh keys to submit patches via the web endpoint.

The only reason it's not currently supported is because we don't have a recent
enough version of openssh on the system where the endpoint is listening. This
will change in the near future, at which point using ssh keys will be
possible.

> Patatt at least supports it to sign patches. I've got a single ed25519
> openssh keypair I use across all my devices, now I'll have to backup
> another key pair. Or create a new key and authenticate with the web
> endpoint on each device.
> 
> Safe to say, I will submit my next patch series using b4. Thanks for
> telling me about this tool Linus!

\o/

Please feel free to provide any feedback you have to the tools@kernel.org
list.

-K
Luiz Angelo Daros de Luca Jan. 30, 2024, 11:21 p.m. UTC | #6
> > I've had trouble with every mail provider's SMTP server that I've ever used
> > for submitting patches, so the web endpoint is a godsend. It would've been
> > great if b4 supported openssh keys to submit patches via the web endpoint.
>
> The only reason it's not currently supported is because we don't have a recent
> enough version of openssh on the system where the endpoint is listening. This
> will change in the near future, at which point using ssh keys will be
> possible.
>
> > Patatt at least supports it to sign patches. I've got a single ed25519
> > openssh keypair I use across all my devices, now I'll have to backup
> > another key pair. Or create a new key and authenticate with the web
> > endpoint on each device.
> >
> > Safe to say, I will submit my next patch series using b4. Thanks for
> > telling me about this tool Linus!
>
> \o/
>
> Please feel free to provide any feedback you have to the tools@kernel.org
> list.
>
> -K

+1 to b4 users: v5 just sent using b4. Great tool.

Regards,

Luiz