[U-Boot] [PATCH V2 1/2] mmc: change magic number to macro define

[Wolfgang Denk]

Dear Lei Wen,

In message <CALZhoSSygHZ22N=odN4qv44a_1ZxguPQLSRwA3pBbPnXbXjC8g at mail.gmail.com> you wrote:
> 
> > And both the "index" and "value" arguments are never used in I/O
> > context, i. e. they are actual plain integer parameters.  So just keep
> > it as
> >
> >        err = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_HS_TIMING, 1);
>
> Should I also keep the EXT_CSD_HS_TIMING like macro for previous
> ext_csd parsing?
> Or just add another ext_csd structure definition to the parse the
> ext_csd, while keep macros
> to be called by mmc_switch?

Let's try to understand what we are trying to acchieve. The purpose
of using C structs instead of a notation of "base address + register
offset" is that this way the struct entries that represent the
registers now have types, and the compiler can perform proper type
checking when using these data in I/O accessors.

So we use structs to describe the "memory map" of the hardware, the
mapping of device registers to addresses, and the data types give
information about the width of the register (8, 16, 32, ... bit).

Note that all the time we are talking about device registers and I/O
operations - that is situations where I/O accessors will be used.


On the other hand, when it comes to definitions of bit fields, like
here:

/*
 * EXT_CSD field definitions
 */  

#define EXT_CSD_CMD_SET_NORMAL          (1 << 0)
#define EXT_CSD_CMD_SET_SECURE          (1 << 1)
#define EXT_CSD_CMD_SET_CPSECURE        (1 << 2)
...

or of indexes, like here:

/*
 * EXT_CSD fields
 */  

#define EXT_CSD_PART_CONF       179     /* R/W */
#define EXT_CSD_BUS_WIDTH       183     /* R/W */
#define EXT_CSD_HS_TIMING       185     /* R/W */
...

..then a struct makes no sense - we have plain integer constants
here.


To verify, please have a look at the code of mmc_switch():

int mmc_switch(struct mmc *mmc, u8 set, u8 index, u8 value)
{
        struct mmc_cmd cmd; 
        int timeout = 1000;
        int ret; 
 
        cmd.cmdidx = MMC_CMD_SWITCH;
        cmd.resp_type = MMC_RSP_R1b;
        cmd.cmdarg = (MMC_SWITCH_MODE_WRITE_BYTE << 24) |
                                 (index << 16) |
                                 (value << 8);
...
        ret = mmc_send_cmd(mmc, &cmd, NULL);


As you can see, the arguments are ORed together to form an argument
to mmc_send_cmd() - they are not used in a I/O accessor or any
similar function.


In short: neither EXT_CSD_CMD_SET_NORMAL nor EXT_CSD_HS_TIMING
describe a device register that is used in any form of I/O
operations, so it is OK to leave these as simple #define's; the use
of a struct would not make sense here.



Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Life would be so much easier if everyone read the manual.