Fixed some bugs in hal_io.h

[Jonathan Larmour]

David Fernandez wrote:
> Hi there,
> 
> This fixes some bugs in hal_io.h regarding the STRING macros that were
> lacking the "rep" prefix.

Those changes are fair enough, good catch.

> It also adds memory string macros,

I'm a bit hesitant about these, as they aren't part of the HAL API, which 
could be detrimental to code reuse. But I'll let them pass as they're 
entirely novel and no-one can fail to notice if their HAL doesn't have them.

> and does a
> slight reformat to keep everything with 80 columns.

Ok.

But those aren't the only changes:

>  #define HAL_READ_UINT8( _register_, _value_ )   \
> -CYG_MACRO_START                                 \
> -{                                               \
> -    asm volatile ( "xor %%eax,%%eax ;"          \
> -                   "inb %%dx, %%al"             \
> +({                                              \
> +    asm volatile(                               \
> +    "       xor     %%eax,%%eax     \n"         \
> +    "       inb     %%dx, %%al      \n"         \
>                     : "=a" (_value_)             \
>                     :  "d"(_register_)           \
>          );                                      \
> -}                                               \
> -CYG_MACRO_END
> +    (_value_);                                  \
> +})

You make this an expression which can be an rvalue. This is outside the 
HAL spec, and would mean that code that might initially seem usable by 
non-i386 packages, would not be. It does not really add anything. I would 
be against this change and the others like it.

>  #define HAL_READ_UINT8_STRING( _register_, _buf_, _count_)      \
>  CYG_MACRO_START                                                 \
> -    asm volatile ( "insb"                                       \
> -                   :                                            \
> -                   : "c" (_count_), "d"(_register_), "D"(_buf_) \
> +    typedef volatile struct { CYG_BYTE m[_count_]; } *mp;   \
> +    asm volatile(                                           \
> +    "       rep insb                \n"                     \
> +    :  "=m" (*(mp) (_buf_))                                 \
> +    :   "c" (_count_),                                      \
> +        "d" (_register_),                                   \
> +        "D" (_buf_)                                         \
>          );                                                      \
>  CYG_MACRO_END

Here you have added an output constraint for the _buf_ array. Won't this 
make GCC allocate an extra unused register? I'm not sure what it is 
intended to achieve either. Similarly in other _STRING functions.

A ChangeLog entry would also be appreciated, thanks.

Jifl
-- 
eCosCentric    http://www.eCosCentric.com/    The eCos and RedBoot experts
------["The best things in life aren't things."]------      Opinions==mine