Re: [RFC 1/4] ath10k: provide firmware crash info via debugfs.

[Kalle Valo <kvalo@qca.qualcomm.com>]

Ben Greear <greearb@candelatech.com> writes:

>>>>> +/* This will store at least the last 128 entries. */
>>>>> +#define ATH10K_DBGLOG_DATA_LEN (128 * 7 * 4)
>>>>
>>>> Where does the 7 and 4 come from? Can't we use sizeof() to compute this?
>>>
>>> It is from the guts of how the firmware does debug logs.
>>>
>>> Each entry is a max of 7 32-bit integers in length.
>>>
>>>> The 128 should probably be a separate #define?
>>>
>>> I don't see why...
>> 
>> To make the code more readable.
>> 
>>> dbglog messages are variable number of 32-bit integers in length, so
>>> the 128 is fairly arbitrary.
>> 
>> A person should immeaditely understand where the numbers are coming from
>> and not start googling about it. The minimum is to put your description
>> above to the comment. And it would be clearer to replace 4 with
>> sizeof(u32).
>
> Would the comments I put in the PATCH 1/4 combined with the sizeof(4)
> be sufficient, or do you want actual defines?

What you wrote about should be enough, I think you can skip the defines
for now. Something like:

        /* Each entry is a max of 7 32-bit integers in length, let's choose
         * arbitrarily 128 entries */

-- 
Kalle Valo