Received-SPF: pass (google.com: best guess record for domain of linux-wireless-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <1555653004-1795-1-git-send-email-yhchuang@realtek.com>
 <1555653004-1795-5-git-send-email-yhchuang@realtek.com> <87bm0npsp7.fsf@kamboji.qca.qualcomm.com>
 <04c534eb-af32-adfc-62bb-90d83526af1a@lwfinger.net> <87a7g6ni31.fsf@kamboji.qca.qualcomm.com>
In-Reply-To: <87a7g6ni31.fsf@kamboji.qca.qualcomm.com>
From:   Brian Norris <briannorris@chromium.org>
Date:   Thu, 2 May 2019 11:23:56 -0700
Message-ID: <CA+ASDXPB3_uhg4dkoQBk+33E0kFay_BnHXKyudyhvorePuH2qA@mail.gmail.com>
Subject: Re: [PATCH v9 04/14] rtw88: trx files
To:     Kalle Valo <kvalo@codeaurora.org>
Cc:     Larry Finger <Larry.Finger@lwfinger.net>,
        Tony Chuang <yhchuang@realtek.com>,
        linux-wireless <linux-wireless@vger.kernel.org>,
        Johannes Berg <johannes@sipsolutions.net>,
        Pkshih <pkshih@realtek.com>, Andy Huang <tehuang@realtek.com>,
        Stanislaw Gruszka <sgruszka@redhat.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

On Wed, May 1, 2019 at 11:30 AM Kalle Valo <kvalo@codeaurora.org> wrote:
> My comment was about handling firmware commands and events as a byte
> array, not about bitfields. So that instead of accessing 'index + 1' and
> 'index + 4' you should create a proper struct for the command and access
> it using 'cmd->foo' and 'cmd->bar'. Sure, bitfields you still need to
> access using FIELD_GET() or similar but having a struct for commands is
> a lot cleaner approach. And most upstream drivers do this: ath10k,
> ath6kl, iwlwifi, p54 and whatnot.

I think I pushed Tony away from the bitfields (he was using a struct
plus some ugly bitfields / #ifdefs pre-v8), and he ended up with this
(in v8). I noted on the v8 cover letter that one can still use a
struct, but just avoid using bitfields -- so you would still have 'u8'
and '__le32' fields (or similar), and do the right le32_to_cpu()
accessors (sparse will help you) plus FIELD_GET() (for any necessary
bitfields).

Brian