Re: [PATCH net-next 1/3] net: dsa: yt921x: Refactor long register helpers

[Andrew Lunn]

> -/* Some registers, like VLANn_CTRL, should always be written in 64-bit, even if
> - * you are to write only the lower / upper 32 bits.
> +/* Some registers, like VLANn_CTRL, should always be written in chunks, even if
> + * you only want to write parts of 32 bits.
>   *
> - * There is no such restriction for reading, but we still provide 64-bit read
> - * wrappers so that we always handle u64 values.
> + * There is no such restriction for reading, but we still provide multi-chunk
> + * read wrappers so that we can handle them consistently.
>   */
>  
> -static int yt921x_reg64_read(struct yt921x_priv *priv, u32 reg, u64 *valp)
> +static int
> +yt921x_longreg_read(struct yt921x_priv *priv, u32 reg, u32 *vals,
> +		    unsigned int span)

Just looking at this, it is not obvious what units span is. Many bulk
read operations takes bytes, even if the underlying code works in
words. I would probably rename span to something like num_u32.

I would also keep yt921x_reg64_read, yt921x_reg64_write,
yt921x_reg64_update_bits() and just makes them wrappers which call
yt921x_longreg_read(). 

> -static int
> -yt921x_vlan_del(struct yt921x_priv *priv, int port, u16 vid)
> +static int yt921x_vlan_del(struct yt921x_priv *priv, int port, u16 vid)
>  {
> -	u64 mask64;
> +	u32 masks[YT921X_VLAN_CTRL_S];
>  
> -	mask64 = YT921X_VLAN_CTRL_PORTS(port) |
> -		 YT921X_VLAN_CTRL_UNTAG_PORTn(port);
> +	masks[0] = YT921X_VLAN_CTRLa_PORTn(port);
> +	masks[1] = YT921X_VLAN_CTRLb_UNTAG_PORTn(port);
>  
> -	return yt921x_reg64_clear_bits(priv, YT921X_VLANn_CTRL(vid), mask64);
> +	return yt921x_longreg_clear_bits(priv, YT921X_VLANn_CTRL(vid), masks,
> +					 YT921X_VLAN_CTRL_S);

By keeping the names, you don't need changes like this. And
yt921x_reg64_clear_bits() is much more obvious than
yt921x_longreg_clear_bits(). And the patch will be much smaller.

	Andrew