On 06/05/2014 04:29 AM, Kalle Valo wrote: > Ben Greear <greearb@xxxxxxxxxxxxxxx> writes: > >> On 06/04/2014 02:04 AM, Michal Kazior wrote: >>> On 3 June 2014 18:25, <greearb@xxxxxxxxxxxxxxx> wrote: >>> >>>> --- a/drivers/net/wireless/ath/ath10k/core.h >>>> +++ b/drivers/net/wireless/ath/ath10k/core.h >>>> @@ -41,6 +41,8 @@ >>>> #define ATH10K_FLUSH_TIMEOUT_HZ (5*HZ) >>>> #define ATH10K_NUM_CHANS 38 >>>> >>>> +#define REG_DUMP_COUNT_QCA988X 60 /* from pci.h */ >>> >>> I don't like this. > > Me neither. > >> You want me to make a different define for this that duplicates >> the '60' value? At least with my method above, we should get compile >> errors if the values ever diverge in the two files. > > There should be only one define. Somewhere (forgot where) Michal > suggested hw.h, I think this define would be a good candidate to move > there too. Ok, I'll move it to hw.h >>>> +/* 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? To really understand this code you need details from how the firmware encodes the messages. I would rather a QCA person add that info just in case it is considered protected info... Thanks, Ben -- Ben Greear <greearb@xxxxxxxxxxxxxxx> Candela Technologies Inc http://www.candelatech.com -- 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
