Re: [PATCH RFC RFT net-next 03/10] net: dsa: mv88e6060: Replace REG_WRITE macro

[Pavel Machek <pavel@ucw.cz>]

[-- Attachment #1: Type: text/plain, Size: 5780 bytes --]

On Wed 2019-01-30 01:37:51, Andrew Lunn wrote:
> The REG_WRITE macro contains a return statement, making it not very
> safe. Remove it by inlining the code.

Not bad, but maybe there should be dev_err() or something in case of
reg_write() returns an error?

Because no errors are expected in this case... AFAICT.
								Pavel

> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  drivers/net/dsa/mv88e6060.c | 73 +++++++++++++++++++++----------------
>  1 file changed, 41 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/net/dsa/mv88e6060.c b/drivers/net/dsa/mv88e6060.c
> index 631358bf3d6b..da88c56e092c 100644
> --- a/drivers/net/dsa/mv88e6060.c
> +++ b/drivers/net/dsa/mv88e6060.c
> @@ -39,15 +39,6 @@ static int reg_write(struct mv88e6060_priv *priv, int addr, int reg, u16 val)
>  	return mdiobus_write_nested(priv->bus, priv->sw_addr + addr, reg, val);
>  }
>  
> -#define REG_WRITE(addr, reg, val)				\
> -	({							\
> -		int __ret;					\
> -								\
> -		__ret = reg_write(priv, addr, reg, val);		\
> -		if (__ret < 0)					\
> -			return __ret;				\
> -	})
> -
>  static const char *mv88e6060_get_name(struct mii_bus *bus, int sw_addr)
>  {
>  	int ret;
> @@ -102,17 +93,21 @@ static int mv88e6060_switch_reset(struct mv88e6060_priv *priv)
>  	/* Set all ports to the disabled state. */
>  	for (i = 0; i < MV88E6060_PORTS; i++) {
>  		ret = REG_READ(REG_PORT(i), PORT_CONTROL);
> -		REG_WRITE(REG_PORT(i), PORT_CONTROL,
> -			  ret & ~PORT_CONTROL_STATE_MASK);
> +		ret = reg_write(priv, REG_PORT(i), PORT_CONTROL,
> +				ret & ~PORT_CONTROL_STATE_MASK);
> +		if (ret)
> +			return ret;
>  	}
>  
>  	/* Wait for transmit queues to drain. */
>  	usleep_range(2000, 4000);
>  
>  	/* Reset the switch. */
> -	REG_WRITE(REG_GLOBAL, GLOBAL_ATU_CONTROL,
> -		  GLOBAL_ATU_CONTROL_SWRESET |
> -		  GLOBAL_ATU_CONTROL_LEARNDIS);
> +	ret = reg_write(priv, REG_GLOBAL, GLOBAL_ATU_CONTROL,
> +			GLOBAL_ATU_CONTROL_SWRESET |
> +			GLOBAL_ATU_CONTROL_LEARNDIS);
> +	if (ret)
> +		return ret;
>  
>  	/* Wait up to one second for reset to complete. */
>  	timeout = jiffies + 1 * HZ;
> @@ -131,59 +126,67 @@ static int mv88e6060_switch_reset(struct mv88e6060_priv *priv)
>  
>  static int mv88e6060_setup_global(struct mv88e6060_priv *priv)
>  {
> +	int ret;
> +
>  	/* Disable discarding of frames with excessive collisions,
>  	 * set the maximum frame size to 1536 bytes, and mask all
>  	 * interrupt sources.
>  	 */
> -	REG_WRITE(REG_GLOBAL, GLOBAL_CONTROL, GLOBAL_CONTROL_MAX_FRAME_1536);
> +	ret = reg_write(priv, REG_GLOBAL, GLOBAL_CONTROL,
> +			GLOBAL_CONTROL_MAX_FRAME_1536);
> +	if (ret)
> +		return ret;
>  
>  	/* Disable automatic address learning.
>  	 */
> -	REG_WRITE(REG_GLOBAL, GLOBAL_ATU_CONTROL,
> -		  GLOBAL_ATU_CONTROL_LEARNDIS);
> -
> -	return 0;
> +	return reg_write(priv, REG_GLOBAL, GLOBAL_ATU_CONTROL,
> +			 GLOBAL_ATU_CONTROL_LEARNDIS);
>  }
>  
>  static int mv88e6060_setup_port(struct mv88e6060_priv *priv, int p)
>  {
>  	int addr = REG_PORT(p);
> +	int ret;
>  
>  	/* Do not force flow control, disable Ingress and Egress
>  	 * Header tagging, disable VLAN tunneling, and set the port
>  	 * state to Forwarding.  Additionally, if this is the CPU
>  	 * port, enable Ingress and Egress Trailer tagging mode.
>  	 */
> -	REG_WRITE(addr, PORT_CONTROL,
> -		  dsa_is_cpu_port(priv->ds, p) ?
> +	ret = reg_write(priv, addr, PORT_CONTROL,
> +			dsa_is_cpu_port(priv->ds, p) ?
>  			PORT_CONTROL_TRAILER |
>  			PORT_CONTROL_INGRESS_MODE |
>  			PORT_CONTROL_STATE_FORWARDING :
>  			PORT_CONTROL_STATE_FORWARDING);
> +	if (ret)
> +		return ret;
>  
>  	/* Port based VLAN map: give each port its own address
>  	 * database, allow the CPU port to talk to each of the 'real'
>  	 * ports, and allow each of the 'real' ports to only talk to
>  	 * the CPU port.
>  	 */
> -	REG_WRITE(addr, PORT_VLAN_MAP,
> -		  ((p & 0xf) << PORT_VLAN_MAP_DBNUM_SHIFT) |
> -		   (dsa_is_cpu_port(priv->ds, p) ? dsa_user_ports(priv->ds) :
> -		    BIT(dsa_to_port(priv->ds, p)->cpu_dp->index)));
> +	ret = reg_write(priv, addr, PORT_VLAN_MAP,
> +			((p & 0xf) << PORT_VLAN_MAP_DBNUM_SHIFT) |
> +			(dsa_is_cpu_port(priv->ds, p) ?
> +			 dsa_user_ports(priv->ds) :
> +			 BIT(dsa_to_port(priv->ds, p)->cpu_dp->index)));
> +	if (ret)
> +		return ret;
>  
>  	/* Port Association Vector: when learning source addresses
>  	 * of packets, add the address to the address database using
>  	 * a port bitmap that has only the bit for this port set and
>  	 * the other bits clear.
>  	 */
> -	REG_WRITE(addr, PORT_ASSOC_VECTOR, BIT(p));
> -
> -	return 0;
> +	return reg_write(priv, addr, PORT_ASSOC_VECTOR, BIT(p));
>  }
>  
>  static int mv88e6060_setup_addr(struct mv88e6060_priv *priv)
>  {
>  	u8 addr[ETH_ALEN];
> +	int ret;
>  	u16 val;
>  
>  	eth_random_addr(addr);
> @@ -195,11 +198,17 @@ static int mv88e6060_setup_addr(struct mv88e6060_priv *priv)
>  	 */
>  	val &= 0xfeff;
>  
> -	REG_WRITE(REG_GLOBAL, GLOBAL_MAC_01, val);
> -	REG_WRITE(REG_GLOBAL, GLOBAL_MAC_23, (addr[2] << 8) | addr[3]);
> -	REG_WRITE(REG_GLOBAL, GLOBAL_MAC_45, (addr[4] << 8) | addr[5]);
> +	ret = reg_write(priv, REG_GLOBAL, GLOBAL_MAC_01, val);
> +	if (ret)
> +		return ret;
> +
> +	ret = reg_write(priv, REG_GLOBAL, GLOBAL_MAC_23,
> +			(addr[2] << 8) | addr[3]);
> +	if (ret)
> +		return ret;
>  
> -	return 0;
> +	return reg_write(priv, REG_GLOBAL, GLOBAL_MAC_45,
> +			 (addr[4] << 8) | addr[5]);
>  }
>  
>  static int mv88e6060_setup(struct dsa_switch *ds)

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]