Re: [RFC, PATCH 1/4] net: sh_eth: modify the definitions of register

[Nobuhiro Iwamatsu <iwamatsu@nigauri.org>]

2011/2/15 Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>:
> 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 <yoshihiro.shimoda.uh@renesas.com>
> ---
>  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>

>
> +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;
> +#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?

>
> +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]);
> +}
> +
>  #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
-- 
Nobuhiro Iwamatsu
   iwamatsu at {nigauri.org / debian.org}
   GPG ID: 40AD1FA6