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

[Nobuhiro Iwamatsu <iwamatsu@nigauri.org>]

2011/2/17 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]); \
> }
>

Oh sorry, this is bad coding style.
Please ignore this comment.

Nobuhiro


-- 
Nobuhiro Iwamatsu
   iwamatsu at {nigauri.org / debian.org}
   GPG ID: 40AD1FA6