From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from mail-gw3-out.broadcom.com ([216.31.210.64]:53931 "EHLO mail-gw3-out.broadcom.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1946419AbbEVJUT (ORCPT ); Fri, 22 May 2015 05:20:19 -0400 Message-ID: <555EF4D0.3070302@broadcom.com> (sfid-20150522_112049_957272_80C408E9) Date: Fri, 22 May 2015 11:20:16 +0200 From: Arend van Spriel MIME-Version: 1.0 To: =?UTF-8?B?UmFmYcWCIE1pxYJlY2tp?= CC: Kalle Valo , linux-wireless , Hante Meuleman , "linux-mips@linux-mips.org" Subject: Re: [PATCH 6/6] brcmfmac: Add support for host platform NVRAM loading. References: <1432123792-4155-1-git-send-email-arend@broadcom.com> <1432123792-4155-7-git-send-email-arend@broadcom.com> <555EE972.7040801@broadcom.com> In-Reply-To: Content-Type: text/plain; charset="UTF-8"; format=flowed Sender: linux-wireless-owner@vger.kernel.org List-ID: On 05/22/15 11:05, Rafał Miłecki wrote: > On 22 May 2015 at 10:31, Arend van Spriel wrote: >> On 05/20/15 17:02, Rafał Miłecki wrote: >>> >>> On 20 May 2015 at 14:09, Arend van Spriel wrote: >>>> >>>> @@ -139,11 +165,11 @@ brcmf_nvram_handle_value(struct nvram_parser *nvp) >>>> char *ekv; >>>> u32 cplen; >>>> >>>> - c = nvp->fwnv->data[nvp->pos]; >>>> - if (!is_nvram_char(c)) { >>>> + c = nvp->data[nvp->pos]; >>>> + if (!is_nvram_char(c)&& (c != ' ')) { >>> >>> >>> Don't smuggle behavior changes in patches doing something else! >> >> >> The subject is "Add support for host platform NVRAM loading" and guess what. >> That type of NVRAM turned out to have spaces in the entries so in my opinion >> it is related to this patch. I can split it up if you feel strongly about >> this. > > I'd expect such patch to just implement *loading* from different > source and nothing else. If there are additional changes needed, I > think they should go in separated patch if possible. > > I noticed the same problem with parsing NVRAM values and sent > [PATCH] brcmfmac: allow NVRAM values to contain space and '#' chars > , so you should be able to drop this patch of your patch anyway. > You may give me an Ack if you have a moment :) Whoops. I did not :-p I don't want to deal with '#' in value field as it is either invalid or irrelevant to firmware on the device. Regards, Arend >>>> @@ -406,19 +434,34 @@ static void brcmf_fw_request_nvram_done(const >>>> struct firmware *fw, void *ctx) >>>> struct brcmf_fw *fwctx = ctx; >>>> u32 nvram_length = 0; >>>> void *nvram = NULL; >>>> + u8 *data = NULL; >>>> + size_t data_len; >>>> + bool raw_nvram; >>>> >>>> brcmf_dbg(TRACE, "enter: dev=%s\n", dev_name(fwctx->dev)); >>>> - if (!fw&& !(fwctx->flags& BRCMF_FW_REQ_NV_OPTIONAL)) >>>> - goto fail; >>>> + if ((fw)&& (fw->data)) { >>> >>> >>> if (fw&& fw->data) >>> will work just fine, I'm surprised checkpatch doesn't complain. >> >> I ran checkpatch.pl --strict and did not get complaint about this change. > > I know, it's weird. Maybe I'll report this an improvement idea to > checkpatch maintainer. >