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

[Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>]

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