Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210304072357.31108-1-zajec5@gmail.com> <20210305055501.13099-1-zajec5@gmail.com>
 <CAAdtpL7iWiumiOwMOH1xiBZvyOB0HB7W-9MMHoPPxkb3Srme=w@mail.gmail.com> <f4045af5-4866-6fc9-f34a-d789a7febb77@milecki.pl>
In-Reply-To: <f4045af5-4866-6fc9-f34a-d789a7febb77@milecki.pl>
From:   =?UTF-8?Q?Philippe_Mathieu=2DDaud=C3=A9?= <f4bug@amsat.org>
Date:   Fri, 5 Mar 2021 12:47:59 +0100
Message-ID: <CAAdtpL5CMTaB6qCR=nZj+1MoGC97_BVd-r30E2RRYOhiktOiZQ@mail.gmail.com>
Subject: Re: [PATCH V2 mips/linux.git] firmware: bcm47xx_nvram: refactor
 finding & reading NVRAM
To:     =?UTF-8?B?UmFmYcWCIE1pxYJlY2tp?= <rafal@milecki.pl>
Cc:     =?UTF-8?B?UmFmYcWCIE1pxYJlY2tp?= <zajec5@gmail.com>,
        Thomas Bogendoerfer <tsbogend@alpha.franken.de>,
        "open list:BROADCOM NVRAM DRIVER" <linux-mips@vger.kernel.org>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Vivek Unune <npcomplete13@gmail.com>,
        bcm-kernel-feedback-list <bcm-kernel-feedback-list@broadcom.com>,
        open list <linux-kernel@vger.kernel.org>,
        kernel test robot <lkp@intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Precedence: bulk

On Fri, Mar 5, 2021 at 11:16 AM Rafał Miłecki <rafal@milecki.pl> wrote:
>
> Hi,
>
> On 05.03.2021 10:58, Philippe Mathieu-Daudé wrote:
> > On Fri, Mar 5, 2021 at 6:55 AM Rafał Miłecki <zajec5@gmail.com> wrote:
> >>
> >> From: Rafał Miłecki <rafal@milecki.pl>
> >>
> >> 1. Use meaningful variable names (e.g. "flash_start", "res_size" instead
> >>     of e.g. "iobase", "end")
> >> 2. Always operate on "offset" instead of mix of start, end, size, etc.
> >
> > "instead of a mix"
> >
> >> 3. Add helper checking for NVRAM to avoid duplicating code
> >> 4. Use "found" variable instead of goto
> >> 5. Use simpler checking of offsets and sizes (2 nested loops with
> >>     trivial check instead of extra function)
> >
> > This could be a series of trivial patches, why did you choose to make a mixed
> > bag harder to review?
>
> It's a subjective thing and often a matter of maintainer taste. I can
> say that after contributing to various Linux subsystems. If you split a
> similar patch for MTD subsystem you'll get complains about making
> changes too small & too hard to review (sic!).

Fine. MTD subsystem developers are probably smarter than I'm :)

> This isn't a bomb really: 63 insertions(+), 48 deletions(-)

Too many changes at once for my brain stack doesn't mean others are
willing to review it. But to me that means each time I'll have to pass over
it while bisecting or reviewing git history I'll suffer the same overflow.
Anyway, matter of taste as you said.

>
> That said I admit I don't know MIPS tree habits. Thomas: do you prefer
> smaller patches in case like this?