Re: [PATCH 6/6] brcmfmac: Add support for host platform NVRAM loading.

[Arend van Spriel <arend@broadcom.com>]

On 05/22/15 11:05, Rafał Miłecki wrote:
> On 22 May 2015 at 10:31, Arend van Spriel<arend@broadcom.com>  wrote:
>> On 05/20/15 17:02, Rafał Miłecki wrote:
>>>
>>> On 20 May 2015 at 14:09, Arend van Spriel<arend@broadcom.com>   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.
>