[U-Boot-Users] [PATCH v2] smc911x: add 16 bit support

[Ben Warren]

Hi Jens,

On Wed, Apr 30, 2008 at 5:34 AM, Jens Gehrlein <sew_s at tqs.de> wrote:
> Incorporated Ben's, Magnus's and Guennadi's proposals.
>  Thank you for reviewing.
>

Almost there.  This comment needs to go below the '---' line so that
it doesn't become part of the commit message.  Also, please add a
meaningful comment here (above your SOB line).  The patch title
contains all the useful information, so you may want to just copy it,
but there needs to be something.
>  Signed-off-by: Jens Gehrlein <sew_s at tqs.de>
>  ---
>
>   drivers/net/smc911x.c |   19 +++++++++++++++++--
>   1 files changed, 17 insertions(+), 2 deletions(-)
>
>
>  diff --git a/drivers/net/smc911x.c b/drivers/net/smc911x.c
>  index d22c889..bd82cf7 100644
>  --- a/drivers/net/smc911x.c
>  +++ b/drivers/net/smc911x.c
>  @@ -30,6 +30,10 @@
>   #include <net.h>
>   #include <miiphy.h>
>
>  +#if defined (CONFIG_DRIVER_SMC911X_32_BIT) && defined (CONFIG_DRIVER_SMC911X_16_BIT)
>  +#error "SMC911X: Only one of CONFIG_DRIVER_SMC911X_32_BIT and CONFIG_DRIVER_SMC911X_16_BIT shall be set"

Both of these lines are too long.  I suggest:

+#if defined (CONFIG_DRIVER_SMC911X_32_BIT) && \
+        defined (CONFIG_DRIVER_SMC911X_16_BIT)
+#error "SMC911X: Only one of CONFIG_DRIVER_SMC911X_32_BIT and \
+        CONFIG_DRIVER_SMC911X_16_BIT shall be set"

NOTE: I wrote this in gmail's web interface, so don't cut & paste - do it right!

>  +#endif
>  +
>   #ifdef CONFIG_DRIVER_SMC911X_32_BIT
>   static inline u32 reg_read(u32 addr)
>   {
>  @@ -39,9 +43,20 @@ static inline void reg_write(u32 addr, u32 val)
>   {
>         *(volatile u32*)addr = val;
>   }
>  +#elif CONFIG_DRIVER_SMC911X_16_BIT
>  +static inline u32 reg_read(u32 addr)
>  +{
>  +       volatile u16 *addr_16 = (u16 *)addr;
>  +       return ((*addr_16 & 0x0000ffff) | (*(addr_16 + 1) << 16));
>  +}
>  +static inline void reg_write(u32 addr, u32 val)
>  +{
>  +       *(volatile u16*)addr = (u16)val;
>  +       *(volatile u16*)(addr + 2) = (u16)(val >> 16);
>  +}
>   #else
>  -#error "SMC911X: Only 32-bit bus is supported"
>  -#endif
>  +#error "SMC911X: undefined buswidth"
s/buswidth/bus\ width/
>  +#endif /* CONFIG_DRIVER_SMC911X_16_BIT */
>
>   #define mdelay(n)       udelay((n)*1000)
>

Also, would you mind putting an entry into the README file for this
driver?  There's already one there for the SMC91111 driver, so this
should be really fast.

Thanks for your help!

regards,
Ben