[U-Boot] [PATCH 1/3] drivers/smc911x: Add support for shifted register read/write

[Albert ARIBAUD]

Hi Roy,

Re: the subject: please next time put the version in the subjetc line
too. You may benefit from using patman, which helps managing patch
series and handles such things as adding version in suject lines and
Cc:ing people according to tags, etc. see tools/patman/README.

Also:

On Fri, 20 Dec 2013 13:32:34 +0100, Roy Spliet <rspliet at eclipso.eu>
wrote:

> Required for (but potentially not limited to) the snowball board. Implementation
> is inspired by the linux smsc911x implementation, but by using a (pre-compiler)
> constant, things should be optimised by the compiler for a shift of 0.
> 
> Signed-off-by: Roy Spliet <rspliet at eclipso.eu>
> ---
>  drivers/net/smc911x.h | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/smc911x.h b/drivers/net/smc911x.h
> index acae0cf..7144722 100644
> --- a/drivers/net/smc911x.h
> +++ b/drivers/net/smc911x.h
> @@ -19,6 +19,12 @@
>  	CONFIG_SMC911X_16_BIT shall be set"
>  #endif
>  
> +#ifndef CONFIG_SMC911X_SHIFT
> +#define CONFIG_SMC911X_SHIFT 0
> +#endif
> +
> +#define smc911x_shift(i) ((i) << CONFIG_SMC911X_SHIFT)

I find that defining a macro just to shift a field by a constant value
is over-engineering. It is not really worse to do the (i << NNN) at the
point(s) of use, and the reader will immediately understand what's
being done.

>  #if defined (CONFIG_SMC911X_32_BIT)
>  static inline u32 __smc911x_reg_read(struct eth_device *dev, u32 offset)
>  {
> @@ -37,14 +43,14 @@ void smc911x_reg_write(struct eth_device *dev, u32 offset, u32 val)
>  #elif defined (CONFIG_SMC911X_16_BIT)
>  static inline u32 smc911x_reg_read(struct eth_device *dev, u32 offset)
>  {
> -	volatile u16 *addr_16 = (u16 *)(dev->iobase + offset);
> -	return ((*addr_16 & 0x0000ffff) | (*(addr_16 + 1) << 16));
> +	volatile u16 *addr_16 = (u16 *)(dev->iobase + smc911x_shift(offset));
> +	return (*addr_16 & 0x0000FFFF) | (*(addr_16 + smc911x_shift(1)) << 16);
>  }
>  static inline void smc911x_reg_write(struct eth_device *dev,
>  					u32 offset, u32 val)
>  {
> -	*(volatile u16 *)(dev->iobase + offset) = (u16)val;
> -	*(volatile u16 *)(dev->iobase + offset + 2) = (u16)(val >> 16);
> +	*(volatile u16 *)(dev->iobase + smc911x_shift(offset)) = (u16)val;
> +	*(volatile u16 *)(dev->iobase + smc911x_shift(offset + 2)) = (u16)val;
>  }
>  #else
>  #error "SMC911X: undefined bus width"

Amicalement,
-- 
Albert.