Re: [PATCH net-next v4 2/7] net: dsa: lantiq_gswip: convert accessors to use regmap

[Maxime Chevallier <maxime.chevallier@bootlin.com>]

Hi Daniel,

On 20/10/2025 14:57, Daniel Golle wrote:
> Use regmap for register access in preparation for supporting the MaxLinear
> GSW1xx family of switches connected via MDIO or SPI.
> Rewrite the existing accessor read-poll-timeout functions to use calls to
> the regmap API for now.
> 
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> ---
> v2: drop error handling, it wasn't there before and it would anyway be
>     removed again by a follow-up change
> 
>  drivers/net/dsa/lantiq/Kconfig        |   1 +
>  drivers/net/dsa/lantiq/lantiq_gswip.c | 107 +++++++++++++++-----------
>  drivers/net/dsa/lantiq/lantiq_gswip.h |   6 +-
>  3 files changed, 67 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/net/dsa/lantiq/Kconfig b/drivers/net/dsa/lantiq/Kconfig
> index 1cb053c823f7..3cfa16840cf5 100644
> --- a/drivers/net/dsa/lantiq/Kconfig
> +++ b/drivers/net/dsa/lantiq/Kconfig
> @@ -2,6 +2,7 @@ config NET_DSA_LANTIQ_GSWIP
>  	tristate "Lantiq / Intel GSWIP"
>  	depends on HAS_IOMEM
>  	select NET_DSA_TAG_GSWIP
> +	select REGMAP
>  	help
>  	  This enables support for the Lantiq / Intel GSWIP 2.1 found in
>  	  the xrx200 / VR9 SoC.
> diff --git a/drivers/net/dsa/lantiq/lantiq_gswip.c b/drivers/net/dsa/lantiq/lantiq_gswip.c
> index 86b410a40d32..046feba16a2f 100644
> --- a/drivers/net/dsa/lantiq/lantiq_gswip.c
> +++ b/drivers/net/dsa/lantiq/lantiq_gswip.c
> @@ -113,22 +113,22 @@ static const struct gswip_rmon_cnt_desc gswip_rmon_cnt[] = {
>  
>  static u32 gswip_switch_r(struct gswip_priv *priv, u32 offset)
>  {
> -	return __raw_readl(priv->gswip + (offset * 4));
> +	u32 val;
> +
> +	regmap_read(priv->gswip, offset, &val);
> +
> +	return val;
>  }
>  
>  static void gswip_switch_w(struct gswip_priv *priv, u32 val, u32 offset)
>  {
> -	__raw_writel(val, priv->gswip + (offset * 4));
> +	regmap_write(priv->gswip, offset, val);
>  }
>  
>  static void gswip_switch_mask(struct gswip_priv *priv, u32 clear, u32 set,
>  			      u32 offset)
>  {
> -	u32 val = gswip_switch_r(priv, offset);
> -
> -	val &= ~(clear);
> -	val |= set;
> -	gswip_switch_w(priv, val, offset);
> +	regmap_write_bits(priv->gswip, offset, clear | set, set);
>  }
>  
>  static u32 gswip_switch_r_timeout(struct gswip_priv *priv, u32 offset,
> @@ -136,48 +136,34 @@ static u32 gswip_switch_r_timeout(struct gswip_priv *priv, u32 offset,
>  {
>  	u32 val;
>  
> -	return readx_poll_timeout(__raw_readl, priv->gswip + (offset * 4), val,
> -				  (val & cleared) == 0, 20, 50000);
> +	return regmap_read_poll_timeout(priv->gswip, offset, val,
> +					!(val & cleared), 20, 50000);
>  }
>  
>  static u32 gswip_mdio_r(struct gswip_priv *priv, u32 offset)
>  {
> -	return __raw_readl(priv->mdio + (offset * 4));
> +	u32 val;
> +
> +	regmap_read(priv->mdio, offset, &val);
> +
> +	return val;
>  }
>  
>  static void gswip_mdio_w(struct gswip_priv *priv, u32 val, u32 offset)
>  {
> -	__raw_writel(val, priv->mdio + (offset * 4));
> +	regmap_write(priv->mdio, offset, val);
>  }
>  
>  static void gswip_mdio_mask(struct gswip_priv *priv, u32 clear, u32 set,
>  			    u32 offset)
>  {
> -	u32 val = gswip_mdio_r(priv, offset);
> -
> -	val &= ~(clear);
> -	val |= set;
> -	gswip_mdio_w(priv, val, offset);
> -}
> -
> -static u32 gswip_mii_r(struct gswip_priv *priv, u32 offset)
> -{
> -	return __raw_readl(priv->mii + (offset * 4));
> -}
> -
> -static void gswip_mii_w(struct gswip_priv *priv, u32 val, u32 offset)
> -{
> -	__raw_writel(val, priv->mii + (offset * 4));
> +	regmap_write_bits(priv->mdio, offset, clear | set, set);
>  }
>  
>  static void gswip_mii_mask(struct gswip_priv *priv, u32 clear, u32 set,
>  			   u32 offset)
>  {
> -	u32 val = gswip_mii_r(priv, offset);
> -
> -	val &= ~(clear);
> -	val |= set;
> -	gswip_mii_w(priv, val, offset);
> +	regmap_write_bits(priv->mii, offset, clear | set, set);
>  }
>  
>  static void gswip_mii_mask_cfg(struct gswip_priv *priv, u32 clear, u32 set,
> @@ -220,17 +206,10 @@ static void gswip_mii_mask_pcdu(struct gswip_priv *priv, u32 clear, u32 set,
>  
>  static int gswip_mdio_poll(struct gswip_priv *priv)
>  {
> -	int cnt = 100;
> -
> -	while (likely(cnt--)) {
> -		u32 ctrl = gswip_mdio_r(priv, GSWIP_MDIO_CTRL);
> -
> -		if ((ctrl & GSWIP_MDIO_CTRL_BUSY) == 0)
> -			return 0;
> -		usleep_range(20, 40);
> -	}
> +	u32 ctrl;
>  
> -	return -ETIMEDOUT;
> +	return regmap_read_poll_timeout(priv->mdio, GSWIP_MDIO_CTRL, ctrl,
> +					!(ctrl & GSWIP_MDIO_CTRL_BUSY), 40, 4000);
>  }
>  
>  static int gswip_mdio_wr(struct mii_bus *bus, int addr, int reg, u16 val)
> @@ -1893,9 +1872,37 @@ static int gswip_validate_cpu_port(struct dsa_switch *ds)
>  	return 0;
>  }
>  
> +static const struct regmap_config sw_regmap_config = {
> +	.name = "switch",
> +	.reg_bits = 32,
> +	.val_bits = 32,
> +	.reg_shift = -2,

I already commented that on V2, but looking back at my review I for some
reason signed my reply too early and made it unclear there were further
review items, I apologize for that.

To re-iterate, it would be better for clarity to use the dedicated macro
here :

  .reg_shift = REGMAP_UPSHIFT(2),

> +	.val_format_endian = REGMAP_ENDIAN_NATIVE,
> +	.max_register = GSWIP_SDMA_PCTRLp(6),
> +};
> +
> +static const struct regmap_config mdio_regmap_config = {
> +	.name = "mdio",
> +	.reg_bits = 32,
> +	.val_bits = 32,
> +	.reg_shift = -2,

here

> +	.val_format_endian = REGMAP_ENDIAN_NATIVE,
> +	.max_register = GSWIP_MDIO_PHYp(0),
> +};
> +
> +static const struct regmap_config mii_regmap_config = {
> +	.name = "mii",
> +	.reg_bits = 32,
> +	.val_bits = 32,
> +	.reg_shift = -2,

and here.

Maxime