From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from smtp.codeaurora.org ([198.145.29.96]) by bombadil.infradead.org with esmtps (Exim 4.92 #3 (Red Hat Linux)) id 1i115B-0007dm-Dn for ath11k@lists.infradead.org; Fri, 23 Aug 2019 04:21:50 +0000 MIME-Version: 1.0 Date: Fri, 23 Aug 2019 09:51:47 +0530 From: Vasanthakumar Thiagarajan Subject: Re: [PATCH 46/49] ath11k: add wmi.h In-Reply-To: References: <1566316095-27507-1-git-send-email-kvalo@codeaurora.org> <1566316095-27507-47-git-send-email-kvalo@codeaurora.org> (sfid-20190820_181541_270184_9285B240) Message-ID: <826dd60c50a2b821fe82c5c04730deb1@codeaurora.org> List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Sender: "ath11k" Errors-To: ath11k-bounces+kvalo=adurom.com@lists.infradead.org To: Johannes Berg Cc: linux-wireless-owner@vger.kernel.org, devicetree@vger.kernel.org, linux-wireless@vger.kernel.org, ath11k@lists.infradead.org, Kalle Valo On 2019-08-21 01:59, Johannes Berg wrote: > On Tue, 2019-08-20 at 18:48 +0300, Kalle Valo wrote: >> >> +enum wmi_cmd_group { >> + /* 0 to 2 are reserved */ >> + WMI_GRP_START = 0x3, >> + WMI_GRP_SCAN = WMI_GRP_START, /* 0x3 */ >> + WMI_GRP_PDEV, /* 0x4 */ > > If you're going to spell out the numbers anyway, why not do it in C > rather than a comment? > > WMI_GRP_PDEV = 0x4, > > would tell you just as much, and be much less error-prone. Sure, we'll do it that way. > >> +struct wmi_pdev_set_hw_mode_cmd_param { >> + u32 tlv_header; >> + u32 pdev_id; >> + u32 hw_mode_index; >> + u32 num_band_to_mac; >> +} __packed; > > Does it really makes sense for something to be using "u32" (i.e. host > endian) but then __packed (kinda tagging it as "I am using this with > the > hardware, don't change the layout")? Yes, this is mainly for tagging. Since Copy Engine does the byte-swapping when working with big-endian system, these are declared in host endian. Removing __packed also fine, I guess. > > That really applies to a lot of the things here. > >> +struct channel_param { >> + u8 chan_id; >> + u8 pwr; >> + u32 mhz; >> + u32 half_rate:1, >> + quarter_rate:1, >> + dfs_set:1, >> + dfs_set_cfreq2:1, >> + is_chan_passive:1, >> + allow_ht:1, >> + allow_vht:1, >> + set_agile:1; >> + u32 phy_mode; >> + u32 cfreq1; >> + u32 cfreq2; >> + char maxpower; >> + char minpower; >> + char maxregpower; >> + u8 antennamax; >> + u8 reg_class_id; >> +} __packed; > > Bitfields in FW structs are even less likely to work right, I'd avoid > that. > > (and if you have this copy engine do endian conversion, then the u8 > fields won't work right since that ending seems to be working on u32s?) > > That probably all applies elsewhere too, but the file is pretty long > ;-) Sure, we'll clean this up. > > Personally, I'd also consider splitting internal driver usage stuff and > FW API into different files, but that's your decision. I just find it > lets me understand it better even when I'm looking at it myself. > Sure, this will look better. Thanks, Vasanth _______________________________________________ ath11k mailing list ath11k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath11k