Ben Greear <greearb@xxxxxxxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
