Messages in this thread | | | Subject | Re: [PATCH 3/3] net: ethernet: ti: cpsw: split common driver data and private net data | From | Ivan Khoronzhuk <> | Date | Fri, 5 Aug 2016 17:38:47 +0300 |
| |
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@linaro.org> >> --- >> 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
| |