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

[Ivan Khoronzhuk]

On 05.08.16 15:14, Grygorii Strashko wrote:
> 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
Ok. v2 will include it.

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

>

--
Regards,
Ivan Khoronzhuk