This is the mail archive of the mailing list for the eCos project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Fixed some bugs in hal_io.h

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.


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_)           \
         );                                      \
-}                                               \
+    (_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_)                                         \
         );                                                      \

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.

eCosCentric    The eCos and RedBoot experts
------["The best things in life aren't things."]------      Opinions==mine

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]