From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from mail2.candelatech.com ([208.74.158.173]) by bombadil.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1WsZv0-0008AX-JB for ath10k@lists.infradead.org; Thu, 05 Jun 2014 15:49:31 +0000 Message-ID: <53909175.30009@candelatech.com> Date: Thu, 05 Jun 2014 08:49:09 -0700 From: Ben Greear MIME-Version: 1.0 Subject: Re: [RFC 1/4] ath10k: provide firmware crash info via debugfs. References: <1401812719-25061-1-git-send-email-greearb@candelatech.com> <538F4DEE.6040405@candelatech.com> <8738fjppba.fsf@kamboji.qca.qualcomm.com> In-Reply-To: <8738fjppba.fsf@kamboji.qca.qualcomm.com> List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "ath10k" Errors-To: ath10k-bounces+kvalo=adurom.com@lists.infradead.org To: Kalle Valo Cc: linux-wireless , Michal Kazior , "ath10k@lists.infradead.org" On 06/05/2014 04:29 AM, Kalle Valo wrote: > Ben Greear writes: > >> On 06/04/2014 02:04 AM, Michal Kazior wrote: >>> On 3 June 2014 18:25, 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 Candela Technologies Inc http://www.candelatech.com _______________________________________________ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k