mbox series

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

Message ID 20241001-b4-sparx5-lan969x-switch-driver-v1-0-8c6896fdce66@microchip.com (mailing list archive)
Headers show
Series net: sparx5: prepare for lan969x switch driver | expand

Message

Daniel Machon Oct. 1, 2024, 1:50 p.m. UTC
== 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

To: David S. Miller <davem@davemloft.net>
To: Eric Dumazet <edumazet@google.com>
To: Jakub Kicinski <kuba@kernel.org>
To: Paolo Abeni <pabeni@redhat.com>
To: Lars Povlsen <lars.povlsen@microchip.com>
To: Steen Hegelund <Steen.Hegelund@microchip.com>
To: horatiu.vultur@microchip.com
To: jensemil.schulzostergaard@microchip.com
To: UNGLinuxDriver@microchip.com
To: Richard Cochran <richardcochran@gmail.com>
To: horms@kernel.org
To: justinstitt@google.com
To: gal@nvidia.com
To: aakash.r.menon@gmail.com
To: jacob.e.keller@intel.com
Cc: netdev@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org

Signed-off-by: Daniel Machon <daniel.machon@microchip.com>
---
Daniel Machon (15):
      net: sparx5: add support for private match data
      net: sparx5: add indirection layer to register macros
      net: sparx5: rename *spx5 to *sparx5 in a few places
      net: sparx5: modify SPX5_PORTS_ALL macro
      net: sparx5: add *sparx5 argument to a few functions
      net: sparx5: add constants to match data
      net: sparx5: use SPX5_CONST for constants which already have a symbol
      net: sparx5: use SPX5_CONST for constants which do not have a symbol
      net: sparx5: add ops to match data
      net: sparx5: ops out chip port to device index/bit functions
      net: sparx5: ops out functions for getting certain array values
      net: sparx5: ops out function for setting the port mux
      net: sparx5: ops out PTP IRQ handler
      net: sparx5: ops out function for DSM calendar calculation
      net: sparx5: add is_sparx5 macro and use it throughout

 drivers/net/ethernet/microchip/sparx5/Makefile     |    2 +-
 .../ethernet/microchip/sparx5/sparx5_calendar.c    |   41 +-
 drivers/net/ethernet/microchip/sparx5/sparx5_dcb.c |    5 +-
 .../net/ethernet/microchip/sparx5/sparx5_ethtool.c |   33 +-
 .../net/ethernet/microchip/sparx5/sparx5_fdma.c    |    6 +-
 .../ethernet/microchip/sparx5/sparx5_mactable.c    |    6 +-
 .../net/ethernet/microchip/sparx5/sparx5_main.c    |  198 +-
 .../net/ethernet/microchip/sparx5/sparx5_main.h    |  111 +-
 .../ethernet/microchip/sparx5/sparx5_main_regs.h   | 4128 +++++++++++---------
 .../net/ethernet/microchip/sparx5/sparx5_netdev.c  |    8 +-
 .../net/ethernet/microchip/sparx5/sparx5_packet.c  |    4 +-
 .../net/ethernet/microchip/sparx5/sparx5_pgid.c    |   24 +-
 .../net/ethernet/microchip/sparx5/sparx5_police.c  |    3 +-
 .../net/ethernet/microchip/sparx5/sparx5_port.c    |   71 +-
 .../net/ethernet/microchip/sparx5/sparx5_port.h    |   23 +-
 .../net/ethernet/microchip/sparx5/sparx5_psfp.c    |   45 +-
 drivers/net/ethernet/microchip/sparx5/sparx5_ptp.c |    6 +-
 drivers/net/ethernet/microchip/sparx5/sparx5_qos.c |    8 +-
 drivers/net/ethernet/microchip/sparx5/sparx5_qos.h |    4 +-
 .../net/ethernet/microchip/sparx5/sparx5_regs.c    |  219 ++
 .../net/ethernet/microchip/sparx5/sparx5_regs.h    |  244 ++
 .../net/ethernet/microchip/sparx5/sparx5_sdlb.c    |   15 +-
 .../ethernet/microchip/sparx5/sparx5_switchdev.c   |   53 +-
 drivers/net/ethernet/microchip/sparx5/sparx5_tc.c  |    8 +-
 .../net/ethernet/microchip/sparx5/sparx5_vlan.c    |   44 +-
 25 files changed, 3191 insertions(+), 2118 deletions(-)
---
base-commit: 3a39d672e7f48b8d6b91a09afa4b55352773b4b5
change-id: 20240927-b4-sparx5-lan969x-switch-driver-dfcd5277fa70

Best regards,

Comments

Jacob Keller Oct. 1, 2024, 5:52 p.m. UTC | #1
On 10/1/2024 6:50 AM, Daniel Machon wrote:
> The register macros are used to read and write to the switch registers.
> The registers are largely the same on Sparx5 and lan969x, however in some
> cases they differ. The differences can be one or more of the following:
> target size, register address, register count, group address, group
> count, group size, field position, field size.
> 
> In order to handle these differences, we introduce a new indirection
> layer, that defines and maps them to corresponding values, based on the
> platform. As the register macro arguments can now be non-constants, we
> also add non-constant variants of FIELD_GET and FIELD_PREP.
> 
> Since the indirection layer contributes to longer macros, we have
> changed the formatting of them slightly, to adhere to a 80 character
> limit, and added a comment if a macro is platform-specific.
> 
> With these additions, we can reuse all the existing macros for
> lan969x.
> 
> Signed-off-by: Daniel Machon <daniel.machon@microchip.com>
> Reviewed-by: Steen Hegelund <Steen.Hegelund@microchip.com>
> ---
>  drivers/net/ethernet/microchip/sparx5/Makefile     |    2 +-
>  .../net/ethernet/microchip/sparx5/sparx5_main.c    |   17 +
>  .../net/ethernet/microchip/sparx5/sparx5_main.h    |   15 +
>  .../ethernet/microchip/sparx5/sparx5_main_regs.h   | 4128 +++++++++++---------
>  .../net/ethernet/microchip/sparx5/sparx5_regs.c    |  219 ++
>  .../net/ethernet/microchip/sparx5/sparx5_regs.h    |  244 ++
>  6 files changed, 2755 insertions(+), 1870 deletions(-)
> 
> diff --git a/drivers/net/ethernet/microchip/sparx5/Makefile b/drivers/net/ethernet/microchip/sparx5/Makefile
> index 288de95add18..3435ca86dd70 100644
> --- a/drivers/net/ethernet/microchip/sparx5/Makefile
> +++ b/drivers/net/ethernet/microchip/sparx5/Makefile
> @@ -11,7 +11,7 @@ sparx5-switch-y  := sparx5_main.o sparx5_packet.o \
>   sparx5_ptp.o sparx5_pgid.o sparx5_tc.o sparx5_qos.o \
>   sparx5_vcap_impl.o sparx5_vcap_ag_api.o sparx5_tc_flower.o \
>   sparx5_tc_matchall.o sparx5_pool.o sparx5_sdlb.o sparx5_police.o \
> - sparx5_psfp.o sparx5_mirror.o
> + sparx5_psfp.o sparx5_mirror.o sparx5_regs.o
>  
>  sparx5-switch-$(CONFIG_SPARX5_DCB) += sparx5_dcb.o
>  sparx5-switch-$(CONFIG_DEBUG_FS) += sparx5_vcap_debugfs.o
> diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_main.c b/drivers/net/ethernet/microchip/sparx5/sparx5_main.c
> index 179a1dc0d8f6..9a8d2e8c02a5 100644
> --- a/drivers/net/ethernet/microchip/sparx5/sparx5_main.c
> +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_main.c
> @@ -29,6 +29,8 @@
>  #include "sparx5_port.h"
>  #include "sparx5_qos.h"
>  
> +const struct sparx5_regs *regs;
> +
>  #define QLIM_WM(fraction) \
>  	((SPX5_BUFFER_MEMORY / SPX5_BUFFER_CELL_SZ - 100) * (fraction) / 100)
>  #define IO_RANGES 3
> @@ -759,6 +761,9 @@ static int mchp_sparx5_probe(struct platform_device *pdev)
>  	sparx5->data = device_get_match_data(sparx5->dev);
>  	if (!sparx5->data)
>  		return -EINVAL;
> +
> +	regs = sparx5->data->regs;
> +
>  	/* Do switch core reset if available */
>  	reset = devm_reset_control_get_optional_shared(&pdev->dev, "switch");
>  	if (IS_ERR(reset))
> @@ -937,10 +942,22 @@ static void mchp_sparx5_remove(struct platform_device *pdev)
>  	destroy_workqueue(sparx5->mact_queue);
>  }
>  
> +static const struct sparx5_regs sparx5_regs = {
> +	.tsize = sparx5_tsize,
> +	.gaddr = sparx5_gaddr,
> +	.gcnt = sparx5_gcnt,
> +	.gsize = sparx5_gsize,
> +	.raddr = sparx5_raddr,
> +	.rcnt = sparx5_rcnt,
> +	.fpos = sparx5_fpos,
> +	.fsize = sparx5_fsize,
> +};
> +
>  static const struct sparx5_match_data sparx5_desc = {
>  	.iomap = sparx5_main_iomap,
>  	.iomap_size = ARRAY_SIZE(sparx5_main_iomap),
>  	.ioranges = 3,
> +	.regs = &sparx5_regs,
>  };
>  
>  static const struct of_device_id mchp_sparx5_match[] = {
> diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_main.h b/drivers/net/ethernet/microchip/sparx5/sparx5_main.h
> index 845f918aaf5e..e3f22b730d80 100644
> --- a/drivers/net/ethernet/microchip/sparx5/sparx5_main.h
> +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_main.h
> @@ -226,6 +226,17 @@ struct sparx5_mall_entry {
>  #define SPARX5_SKB_CB(skb) \
>  	((struct sparx5_skb_cb *)((skb)->cb))
>  
> +struct sparx5_regs {
> +	const unsigned int *tsize;
> +	const unsigned int *gaddr;
> +	const unsigned int *gcnt;
> +	const unsigned int *gsize;
> +	const unsigned int *raddr;
> +	const unsigned int *rcnt;
> +	const unsigned int *fpos;
> +	const unsigned int *fsize;
> +};
> +
>  struct sparx5_main_io_resource {
>  	enum sparx5_target id;
>  	phys_addr_t offset;
> @@ -233,6 +244,7 @@ struct sparx5_main_io_resource {
>  };
>  
>  struct sparx5_match_data {
> +	const struct sparx5_regs *regs;
>  	const struct sparx5_main_io_resource *iomap;
>  	int ioranges;
>  	int iomap_size;
> @@ -308,6 +320,9 @@ struct sparx5 {
>  	const struct sparx5_match_data *data;
>  };
>  
> +/* sparx5_main.c */
> +extern const struct sparx5_regs *regs;
> +
>  /* sparx5_switchdev.c */
>  int sparx5_register_notifier_blocks(struct sparx5 *sparx5);
>  void sparx5_unregister_notifier_blocks(struct sparx5 *sparx5);
> diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_main_regs.h b/drivers/net/ethernet/microchip/sparx5/sparx5_main_regs.h
> index 22acc1f3380c..3783cfd1d855 100644
> --- a/drivers/net/ethernet/microchip/sparx5/sparx5_main_regs.h
> +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_main_regs.h
> @@ -1,11 +1,11 @@
>  /* SPDX-License-Identifier: GPL-2.0+
>   * Microchip Sparx5 Switch driver
>   *
> - * Copyright (c) 2021 Microchip Technology Inc.
> + * Copyright (c) 2024 Microchip Technology Inc.
>   */
>  
> -/* This file is autogenerated by cml-utils 2023-02-10 11:18:53 +0100.
> - * Commit ID: c30fb4bf0281cd4a7133bdab6682f9e43c872ada
> +/* This file is autogenerated by cml-utils 2024-09-24 14:13:28 +0200.
> + * Commit ID: 9d07b8d19363f3cd3590ddb3f7a2e2768e16524b
>   */
>  
>  #ifndef _SPARX5_MAIN_REGS_H_
> @@ -15,6 +15,8 @@
>  #include <linux/types.h>
>  #include <linux/bug.h>
>  
> +#include "sparx5_regs.h"
> +
>  enum sparx5_target {
>  	TARGET_ANA_AC = 1,
>  	TARGET_ANA_ACL = 2,
> @@ -52,14 +54,28 @@ enum sparx5_target {
>  	TARGET_VCAP_SUPER = 326,
>  	TARGET_VOP = 327,
>  	TARGET_XQS = 331,
> -	NUM_TARGETS = 332
> +	NUM_TARGETS = 517
>  };
>  
> +/* Non-constant mask variant of FIELD_GET() and FIELD_PREP() */
> +#define spx5_field_get(_mask, _reg) (((_reg) & (_mask)) >> (ffs(_mask) - 1))
> +#define spx5_field_prep(_mask, _val) (((_val) << (ffs(_mask) - 1)) & (_mask))
> +

FIELD_GET and FIELD_SET have restrictions in place to enforce constant
mask, which enables strict checks to ensure things fit and determine the
bit shifts at compile time without ffs.

Would it make sense for these to exist in <linux/bitfields.h>? I'm not
sure how common it is to have non-const masks..

> +#define GADDR(o)  regs->gaddr[o]
> +#define GCNT(o)   regs->gcnt[o]
> +#define GSIZE(o)  regs->gsize[o]
> +#define RADDR(o)  regs->raddr[o]
> +#define RCNT(o)   regs->rcnt[o]
> +#define FPOS(o)   regs->fpos[o]
> +#define FSIZE(o)  regs->fsize[o]
> +#define TSIZE(o)  regs->tsize[o]

This implementation requires 'regs' to be in scope without being passed
as a parameter. I guess thats just assumed here?
Jacob Keller Oct. 1, 2024, 6:03 p.m. UTC | #2
On 10/1/2024 6:50 AM, Daniel Machon wrote:
> == 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>
Daniel Machon Oct. 2, 2024, 7:47 a.m. UTC | #3
> > == 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
Daniel Machon Oct. 2, 2024, 8:07 a.m. UTC | #4
> > +/* Non-constant mask variant of FIELD_GET() and FIELD_PREP() */
> > +#define spx5_field_get(_mask, _reg) (((_reg) & (_mask)) >> (ffs(_mask) - 1))
> > +#define spx5_field_prep(_mask, _val) (((_val) << (ffs(_mask) - 1)) & (_mask))
> > +
> 
> FIELD_GET and FIELD_SET have restrictions in place to enforce constant
> mask, which enables strict checks to ensure things fit and determine the
> bit shifts at compile time without ffs.
> 
> Would it make sense for these to exist in <linux/bitfields.h>? I'm not
> sure how common it is to have non-const masks..

There was a patch for this some time ago [1], it got some push-back and
never got in, AFAICS. 

[1] https://patchwork.kernel.org/project/linux-pm/patch/3a54a6703879d10f08cf0275a2a69297ebd2b1d4.1637592133.git.geert+renesas@glider.be/#24611465

/Daniel
Jacob Keller Oct. 2, 2024, 9:44 p.m. UTC | #5
On 10/2/2024 12:47 AM, Daniel Machon wrote:
> 
> 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 there is style preference here, and someone working
day-in/day-out on the code is likely to know which macros have which
variable dependencies. As an external reviewer, I would not know that,
so I would find it surprising when looking at some code which passes a
parameter which is never directly used.

> 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)

Yea, I'm sure a lot of the old Intel drivers have bad examples :D I've
spent a career trying to improve this.

> 
> 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.
> 

It seems that Jakub Kicinski wants the entire macro removed, and his
opinion as maintainer matters more than mine.

Personally, I'm not opposed to a macro itself especially if the direct
access starts to get very long. However, I think the parameter being
accessed should be, well, a parameter of the macro.

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

As an example of *why* I don't like this practice: It took me a while to
realize the hw variable was implicit to wr32. And I worked on this driver.

Thanks,
Jake