net: sh_eth: modify the definitions of register
Subsystems (3)
RFC PATCH 1/4 net: sh_eth: modify the definitions of register
The previous code cannot handle the ETHER and GETHER both as same time because the definitions of register was hardcoded.
--- arch/sh/include/asm/sh_eth.h | 7 + drivers/net/sh_eth.c | 322 +++++++++++----------- drivers/net/sh_eth.h | 623 ++++++++++++++++++++++++------------------ 3 files changed, 537 insertions(+), 415 deletions(-)
arch/sh/include/asm/sh_eth.h +7 -0
@@ -2,10 +2,17 @@#define __ASM_SH_ETH_H__enum {EDMAC_LITTLE_ENDIAN ,EDMAC_BIG_ENDIAN }; +enum { +SH_ETH_REG_DEFAULT =0 , +SH_ETH_REG_GIGABIT , +SH_ETH_REG_FAST_SH4 , +SH_ETH_REG_FAST_SH3_SH2 +};struct sh_eth_plat_data {int phy;int edmac_endian; +int register_type;unsigned char mac_addr[6 ];unsigned no_ether_link:1 ;
drivers/net/sh_eth.c +166 -156
652 lines not on this page · raw message
drivers/net/sh_eth.h +364 -259
659 lines not on this page · raw message
Signature · 1 line
1.7.1
2011/2/15 Yoshihiro Shimoda <[email protected]>:
Quoted 10 lines
The previous code cannot handle the ETHER and GETHER both as same time because the definitions of register was hardcoded.
Signed-off-by Yoshihiro Shimoda <[email protected]>---
arch/sh/include/asm/sh_eth.h | 7 +
drivers/net/sh_eth.c | 322 +++++++++++-----------drivers/net/sh_eth.h| 623 ++++++++++++++++++++++++------------------3 files changed, 537 insertions(+), 415 deletions(-)
<snip>
Quoted 22 lines
drivers/net/sh_eth.c +21
-0+static const u16 *sh_eth_get_register_offset (int register_type) +{ + const u16 *reg_offset = NULL; + + switch (register_type) { + case SH_ETH_REG_GIGABIT: + reg_offset = sh_eth_offset_gigabit; + break; + case SH_ETH_REG_FAST_SH4: + reg_offset = sh_eth_offset_fast_sh4; + break; + case SH_ETH_REG_FAST_SH3_SH2: + reg_offset = sh_eth_offset_fast_sh3_sh2; + break; + case SH_ETH_REG_DEFAULT: +#if defined(CONFIG_CPU_SUBTYPE_SH7763 ) + reg_offset = sh_eth_offset_gigabit; +#elif defined(CONFIG_CPU_SH4 ) + reg_offset = sh_eth_offset_fast_sh4; +#else + reg_offset = sh_eth_offset_fast_sh3_sh2;
drivers/net/sh_eth.c out: · line 1430
Quoted 10 lines
drivers/net/sh_eth.c +10
-0+#endif + break; + default: + printk(KERN_ERR "Unknown register type (%d)\n", register_type); + break; + } + + return reg_offset; +} +
Is the handling of SH_ETH_REG_DEFAULT necessary?
Quoted 29 lines
drivers/net/sh_eth.h +28
-0+static inline void sh_eth_write (struct net_device *ndev,unsigned long data, + int enum_index) +{ + struct sh_eth_private *mdp = netdev_priv(ndev); + + writel(data, ndev->base_addr + mdp->reg_offset[enum_index]); +} + +static inline unsigned long sh_eth_read (struct net_device *ndev, + int enum_index) +{ + struct sh_eth_private *mdp = netdev_priv(ndev); + + return readl(ndev->base_addr + mdp->reg_offset[enum_index]); +} + +static inline void sh_eth_tsu_write (struct sh_eth_private *mdp, + unsigned long data, int enum_index) +{ + writel(data, mdp->tsu_addr + mdp->reg_offset[enum_index]); +} + +static inline unsigned long sh_eth_tsu_read (struct sh_eth_private *mdp, + int enum_index) +{ + return readl(mdp->tsu_addr + mdp->reg_offset[enum_index]); +} +
Quoted 1 line
#endif /* #ifndef
__SH_ETH_H__*/
I do not think that a new function is necessary.
I think it use MACRO.
For example,
#define sh_eth_write(data,reg) \
{\
writel(data, ndev->base_addr + mdp->reg_offset[reg]); \
}Best regards,
Nobuhiro
Signature · 3 lines
Nobuhiro Iwamatsu
iwamatsu at {nigauri.org / debian.org}
GPG ID: 40AD1FA62011/2/17 Nobuhiro Iwamatsu <[email protected]>:
Quoted 83 lines
2011/2/15 Yoshihiro Shimoda <[email protected]>:
Quoted 10 lines
The previous code cannot handle the ETHER and GETHER both as same time because the definitions of register was hardcoded.
Signed-off-by Yoshihiro Shimoda <[email protected]>---
arch/sh/include/asm/sh_eth.h | 7 +
drivers/net/sh_eth.c | 322 +++++++++++-----------drivers/net/sh_eth.h| 623 ++++++++++++++++++++++++------------------3 files changed, 537 insertions(+), 415 deletions(-)<snip>
Quoted 22 lines
drivers/net/sh_eth.c +21
-0+static const u16 *sh_eth_get_register_offset (int register_type) +{ + const u16 *reg_offset = NULL; + + switch (register_type) { + case SH_ETH_REG_GIGABIT: + reg_offset = sh_eth_offset_gigabit; + break; + case SH_ETH_REG_FAST_SH4: + reg_offset = sh_eth_offset_fast_sh4; + break; + case SH_ETH_REG_FAST_SH3_SH2: + reg_offset = sh_eth_offset_fast_sh3_sh2; + break; + case SH_ETH_REG_DEFAULT: +#if defined(CONFIG_CPU_SUBTYPE_SH7763 ) + reg_offset = sh_eth_offset_gigabit; +#elif defined(CONFIG_CPU_SH4 ) + reg_offset = sh_eth_offset_fast_sh4; +#else + reg_offset = sh_eth_offset_fast_sh3_sh2;drivers/net/sh_eth.c out: · line 1430
Quoted 10 lines
drivers/net/sh_eth.c +10
-0+#endif + break; + default: + printk(KERN_ERR "Unknown register type (%d)\n", register_type); + break; + } + + return reg_offset; +} +Is the handling of
SH_ETH_REG_DEFAULTnecessary?Quoted 29 lines
drivers/net/sh_eth.h +28
-0+static inline void sh_eth_write (struct net_device *ndev,unsigned long data, + int enum_index) +{ + struct sh_eth_private *mdp = netdev_priv(ndev); + + writel(data, ndev->base_addr + mdp->reg_offset[enum_index]); +} + +static inline unsigned long sh_eth_read (struct net_device *ndev, + int enum_index) +{ + struct sh_eth_private *mdp = netdev_priv(ndev); + + return readl(ndev->base_addr + mdp->reg_offset[enum_index]); +} + +static inline void sh_eth_tsu_write (struct sh_eth_private *mdp, + unsigned long data, int enum_index) +{ + writel(data, mdp->tsu_addr + mdp->reg_offset[enum_index]); +} + +static inline unsigned long sh_eth_tsu_read (struct sh_eth_private *mdp, + int enum_index) +{ + return readl(mdp->tsu_addr + mdp->reg_offset[enum_index]); +} +Quoted 1 line
#endif /* #ifndef
__SH_ETH_H__*/I do not think that a new function is necessary.
I think it use MACRO.
Quoted 6 lines
For example,
#definesh_eth_write(data,reg)\
{\
writel(data,ndev->base_addr+ mdp->reg_offset[reg]); \
}
Oh sorry, this is bad coding style.
Please ignore this comment.
Nobuhiro
Signature · 3 lines
Nobuhiro Iwamatsu
iwamatsu at {nigauri.org / debian.org}
GPG ID: 40AD1FA6Quoted 34 lines · Nobuhiro Iwamatsu
2011/2/17 Nobuhiro Iwamatsu <[email protected]>:
Quoted 33 lines
Quoted 21 lines
drivers/net/sh_eth.c +21
-0+static const u16 *sh_eth_get_register_offset (int register_type) +{ +const u16 *reg_offset =NULL ; + +switch (register_type) { +case SH_ETH_REG_GIGABIT : + reg_offset = sh_eth_offset_gigabit; +break ; +case SH_ETH_REG_FAST_SH4 : + reg_offset = sh_eth_offset_fast_sh4; +break ; +case SH_ETH_REG_FAST_SH3_SH2 : + reg_offset = sh_eth_offset_fast_sh3_sh2; +break ; +case SH_ETH_REG_DEFAULT : +#if defined(CONFIG_CPU_SUBTYPE_SH7763 ) + reg_offset = sh_eth_offset_gigabit; +#elif defined(CONFIG_CPU_SH4 ) + reg_offset = sh_eth_offset_fast_sh4; +#else + reg_offset = sh_eth_offset_fast_sh3_sh2;drivers/net/sh_eth.c out: · line 1430
Quoted 10 lines
drivers/net/sh_eth.c +10
-0+#endif +break ; +default : +printk (KERN_ERR "Unknown register type (%d)\n" , register_type); +break ; + } + +return reg_offset; +} +Is the handling of
SH_ETH_REG_DEFAULTnecessary?
Thank you for your review!
No, it isn't necessary. I will remove it and I will add NULL checking of mdp->reg_offset.
Best regards,
Yoshihiro Shimoda