> > == 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@intel.com>
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