Subject: Re: [PATCH 3/8] net: sh-eth: Remove duplicate sh_eth_set_duplex()

From: Sergei Shtylyov <[email-masked]>
Date: Thu, 18 Apr 2013 18:12:35 +0400

Hello.

On 18-04-2013 11:01, Nobuhiro Iwamatsu wrote:

> Signed-off-by: Nobuhiro Iwamatsu <[email-masked]>

    Step in the right direction.

> diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
> index 313d6ef..5c4e82c 100644
> --- a/drivers/net/ethernet/renesas/sh_eth.c
> +++ b/drivers/net/ethernet/renesas/sh_eth.c
> @@ -341,9 +341,11 @@ static void sh_eth_select_mii(struct net_device *ndev)
>   }
>   #endif
>
> -/* There is CPU dependent code */
> -#if defined(CONFIG_ARCH_R8A7779)
> -#define SH_ETH_RESET_DEFAULT	1
> +#if defined(CONFIG_CPU_SUBTYPE_SH7619) || \
> +	defined(CONFIG_CPU_SUBTYPE_SH7710) || \
> +	defined(CONFIG_CPU_SUBTYPE_SH7712)
> +#  define sh_eth_set_duplex NULL

    I don't think this is neeeded at all.

> +#else
>   static void sh_eth_set_duplex(struct net_device *ndev)
>   {
>   	struct sh_eth_private *mdp = netdev_priv(ndev);
> @@ -353,7 +355,11 @@ static void sh_eth_set_duplex(struct net_device *ndev)
>   	else		/* Half */
>   		sh_eth_write(ndev, sh_eth_read(ndev, ECMR) & ~ECMR_DM, ECMR);
>   }
> +#endif
>
> +/* There is CPU dependent code */
> +#if defined(CONFIG_ARCH_R8A7779)
> +#define SH_ETH_RESET_DEFAULT	1
>   static void sh_eth_set_rate(struct net_device *ndev)
>   {
>   	struct sh_eth_private *mdp = netdev_priv(ndev);
[...]
> @@ -834,6 +791,7 @@ static struct sh_eth_cpu_data sh_eth_my_cpu_data = {
>   #elif defined(CONFIG_CPU_SUBTYPE_SH7619)
>   #define SH_ETH_RESET_DEFAULT	1
>   static struct sh_eth_cpu_data sh_eth_my_cpu_data = {
> +	.set_duplex = sh_eth_set_duplex,

    Please, align = like in other initializers.
    Wait, is this change legitimate at all? You mean NULL will be set here?
But why bother doing this, when 0s will be set to all fields without
explicit initializer?

>   	.eesipr_value	= DMAC_M_RFRMER | DMAC_M_ECI | 0x003fffff,
>
>   	.apr		= 1,
> @@ -845,6 +803,7 @@ static struct sh_eth_cpu_data sh_eth_my_cpu_data = {
>   #define SH_ETH_RESET_DEFAULT	1
>   #define SH_ETH_HAS_TSU	1
>   static struct sh_eth_cpu_data sh_eth_my_cpu_data = {
> +	.set_duplex = sh_eth_set_duplex,

    Same coments here.

WBR, Sergei