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

Lei Wen adrian.wenl at gmail.com
Tue Oct 11 15:52:35 CEST 2011


Hi Graeme,

On Tue, Oct 11, 2011 at 9:09 AM, Graeme Russ <graeme.russ at gmail.com> wrote:
> Hi Lei Wen,
>
> On Tue, Oct 11, 2011 at 11:48 AM, Lei Wen <adrian.wenl at gmail.com> wrote:
>> Hi Wolfgang,
>>
>> On Tue, Oct 11, 2011 at 1:33 AM, Wolfgang Denk <wd at denx.de> wrote:
>>> Dear Lei Wen,
>>>
>>> In message <CALZhoSQbvKj0MtqryeHX-4LkvQJR2=B9u_m4yJjfM1mjv2MSEw at mail.gmail.com> you wrote:
>>>>
>>>> >> So macro may looks more concise and could parse from its meaning easily eno=
>>>> >> ugh.
>>>> >
>>>> > We do not accept (typeless) register offset definitions. Please use a
>>>> > struct, so the compiler has a chance to perform type checking.
>>>>
>>>> I check the code again, and find there is a reason for previous
>>>> defined macro to use.
>>>> That is those register offset defined as macro may need later passing
>>>> as a function
>>>> parameter like:
>>>>         err = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_HS_TIMING, 1);
>>>
>>> I don;t see that any register (which now would be an address in a
>>> struct insteadd of being an offset to be added to a base address)
>>> would be passed as parameter to mmc_switch().
>>>
>>> So this is not an issue at all.
>>>
>>>> So if the ext_csd change to structure, maybe the function call here
>>>> don't looks like so
>>>> concise as before... What do you think for this?
>>>
>>> The EXT_CSD field definitions are completely independent of the way
>>> how you access the registers.
>>>
>>
>> So do you means I should keep the EXT_CSD_HS_TIMING? Or I change like below?
>>
>> from
>> err = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_HS_TIMING, 1);
>> to:
>> err = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL, &ext_csd.hs_timing - &ext_csd, 1);
>
> I imagine the compiler will complain that the types for &ext_csd.hs_timing
> and &ext_csd are different.
>
> Try:
>        struct ext_csd *ext_csd_ptr = 0;
>
>        err = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL, &ext_csd_ptr->hs_timing, 1);

Yes, it looks more clear than mine. If have no other suggestion, I
would do like this to format my patches.

Thanks,
Lei


More information about the U-Boot mailing list