Re: [PATCH net-next 00/15] net: sparx5: prepare for lan969x switch driver

[Daniel Machon]

> > == Description:
> >
> > This series is the first of a multi-part series, that prepares and adds
> > support for the new lan969x switch driver.
> >
> > The upstreaming efforts is split into multiple series (might change a
> > bit as we go along):
> >
> >     1) Prepare the Sparx5 driver for lan969x (this series)
> >     2) Add support lan969x (same basic features as Sparx5 provides +
> >        RGMII, excl.  FDMA and VCAP)
> >     3) Add support for lan969x FDMA
> >     4) Add support for lan969x VCAP
> >
> > == Lan969x in short:
> >
> > The lan969x Ethernet switch family [1] provides a rich set of
> > switching features and port configurations (up to 30 ports) from 10Mbps
> > to 10Gbps, with support for RGMII, SGMII, QSGMII, USGMII, and USXGMII,
> > ideal for industrial & process automation infrastructure applications,
> > transport, grid automation, power substation automation, and ring &
> > intra-ring topologies. The LAN969x family is hardware and software
> > compatible and scalable supporting 46Gbps to 102Gbps switch bandwidths.
> >
> > == Preparing Sparx5 for lan969x:
> >
> > The lan969x switch chip reuses many of the IP's of the Sparx5 switch
> > chip, therefore it has been decided to add support through the existing
> > Sparx5 driver, in order to avoid a bunch of duplicate code. However, in
> > order to reuse the Sparx5 switch driver, we have to introduce some
> > mechanisms to handle the chip differences that are there.  These
> > mechanisms are:
> >
> >     - Platform match data to contain all the differences that needs to
> >       be handled (constants, ops etc.)
> >
> >     - Register macro indirection layer so that we can reuse the existing
> >       register macros.
> >
> >     - Function for branching out on platform type where required.
> >
> > In some places we ops out functions and in other places we branch on the
> > chip type. Exactly when we choose one over the other, is an estimate in
> > each case.
> >
> > After this series is applied, the Sparx5 driver will be prepared for
> > lan969x and still function exactly as before.
> >
> > == Patch breakdown:
> >
> > Patch #1     adds private match data
> >
> > Patch #2     adds register macro indirection layer
> >
> > Patch #3-#5  does some preparation work
> >
> > Patch #6-#8  adds chip constants and updates the code to use them
> >
> > Patch #9-#14 adds and uses ops for handling functions differently on the
> >              two platforms.
> >
> > Patch #15    adds and uses a macro for branching out on the chip type
> >
> > [1] https://www.microchip.com/en-us/product/lan9698
> >
> 
> The series seems ok to me. I'm not personally a fan of the implicit
> local variables used by macros. I do not know how common that is, or
> what others on the list feel about this.
> 
> For everything else:
> 
> Reviewed-by: Jacob Keller <jacob.e.keller@xxxxxxxxx>

Hi Jakob,

First off, thank you for your reviews - I really appreciate it.

Let me address your "variable scope" conerns:

I had the feeling that this could pontentially be somewhat contentious.

Basically, this comes down to making the least invasive changes to the
existing driver code. With this approach:

    For the SPX5_CONST macro this means shorter lines, and less 80 char
    wrapping.

    For the "*regs" variable this means not having to pass in the sparx5
    pointer to *all* register macros, which requires changes *all* over
    the code.

I thought the solution with an in-scope implicit variable was less
invasive (and maybe even more readable?). Just my opinion, given the
alternative.

Obviously I did a bit of research on this upfront, and I can point to
*many* places where drivers do the exact same (not justifying the use,
just pointing that out). Here is an intel driver that does the same [1]
(look at the *hw variable)

I am willing to come up with something different, if you really think
this is a no-go. Let me hear your thoughts. I think this applies to your
comments on #2, #3 and #6 as well.

/Daniel

[1] https://elixir.bootlin.com/linux/v6.12-rc1/source/drivers/net/ethernet/intel/igb/igb_main.c#L4746