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.khoronz...@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.
--
regards,
-grygorii
