Re: [PATCH 3/3] net: ethernet: ti: cpsw: split common driver data and private net data

[Grygorii Strashko]

On 08/05/2016 12:14 AM, Ivan Khoronzhuk wrote:
> Simplify driver by splitting common driver data and net dev
> private data. In case of dual_emac mode 2 networks devices
> are created, each of them contains its own private data.
> But 2 net devices share a bunch of h/w resources, that shouldn't
> be duplicated.
> This patch leads to the following:
> - no functional changes
> - reduce code size
> - reduce memory usage
> - reduce number of conversion to priv function
> - reduce number of arguments for some functions
> - increase code readability
> - create prerequisites to add multichannel support,
>   when channels are shared between net devices

Even if it sounds reasonable, I have to NACK this patch -
main reason below, but there are few more:
- could you pls split this change as it's too big and I'm pretty sure it's possible;
- could you pls avoid unrelated changes like variable reordering in structures

and thanks a lot for working on this.

> 
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@xxxxxxxxxx>
> ---
>  drivers/net/ethernet/ti/cpsw.c | 775 +++++++++++++++++++----------------------
>  1 file changed, 364 insertions(+), 411 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
> index 85ee9f5..7a84515 100644
> --- a/drivers/net/ethernet/ti/cpsw.c
> +++ b/drivers/net/ethernet/ti/cpsw.c
> @@ -141,8 +141,8 @@ do {								\
>  #define CPSW_CMINTMIN_INTVL	((1000 / CPSW_CMINTMAX_CNT) + 1)
>  
>  #define cpsw_slave_index(priv)				\
> -		((priv->data.dual_emac) ? priv->emac_port :	\
> -		priv->data.active_slave)
> +		((cpsw->data.dual_emac) ? priv->emac_port :	\
> +		cpsw->data.active_slave)
>  
>  static int debug_level;
>  module_param(debug_level, int, 0);
> @@ -364,29 +364,34 @@ static inline void slave_write(struct cpsw_slave *slave, u32 val, u32 offset)
>  }
>  
>  struct cpsw_priv {
> -	struct platform_device		*pdev;
>  	struct net_device		*ndev;
> -	struct napi_struct		napi_rx;
> -	struct napi_struct		napi_tx;
>  	struct device			*dev;
> +	u8				mac_addr[ETH_ALEN];
> +	bool				rx_pause;
> +	bool				tx_pause;
> +	u32				msg_enable;
> +	u32				emac_port;
> +};
> +
> +struct cpsw_common {
> +	struct net_device		*ndev; /* holds base ndev */
> +	struct platform_device		*pdev;
>  	struct cpsw_platform_data	data;
> +	struct napi_struct		napi_rx;
> +	struct napi_struct		napi_tx;
> +	struct cpdma_chan		*txch, *rxch;
> +	struct cpsw_slave		*slaves;
>  	struct cpsw_ss_regs __iomem	*regs;
>  	struct cpsw_wr_regs __iomem	*wr_regs;
>  	u8 __iomem			*hw_stats;
>  	struct cpsw_host_regs __iomem	*host_port_regs;
> -	u32				msg_enable;
> -	u32				version;
> -	u32				coal_intvl;
> -	u32				bus_freq_mhz;
> -	int				rx_packet_max;
>  	struct clk			*clk;
> -	u8				mac_addr[ETH_ALEN];
> -	struct cpsw_slave		*slaves;
>  	struct cpdma_ctlr		*dma;
> -	struct cpdma_chan		*txch, *rxch;
>  	struct cpsw_ale			*ale;
> -	bool				rx_pause;
> -	bool				tx_pause;
> +	int				rx_packet_max;
> +	u32				bus_freq_mhz;
> +	u32				version;
> +	u32				coal_intvl;
>  	bool				quirk_irq;
>  	bool				rx_irq_disabled;
>  	bool				tx_irq_disabled;
> @@ -394,9 +399,10 @@ struct cpsw_priv {
>  	u32 irqs_table[4];
>  	u32 num_irqs;
>  	struct cpts *cpts;
> -	u32 emac_port;
>  };



>  
> +static struct cpsw_common *cpsw;
> +

Sry, but NACK - no new static variables pls.

-- 
regards,
-grygorii