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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html