Date:   Sun, 2 Dec 2018 15:13:28 -0800
From:   Darren Hart <dvhart@...radead.org>
To:     Lubomir Rintel <lkundrak@...sk>
Cc:     Mark Brown <broonie@...nel.org>,
        Geert Uytterhoeven <geert+renesas@...der.be>,
        Andy Shevchenko <andy@...radead.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        James Cameron <quozl@...top.org>,
        Sebastian Reichel <sre@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Eric Miao <eric.y.miao@...il.com>,
        Haojian Zhuang <haojian.zhuang@...il.com>,
        Daniel Mack <daniel@...que.org>,
        Robert Jarzmik <robert.jarzmik@...e.fr>,
        linux-spi@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        platform-driver-x86@...r.kernel.org, devel@...verdev.osuosl.org,
        linux-pm@...r.kernel.org
Subject: Re: [PATCH v2 06/17] Platform: OLPC: Add XO-1.75 EC driver

On Fri, Nov 16, 2018 at 05:23:52PM +0100, Lubomir Rintel wrote:
> It's based off the driver from the OLPC kernel sources. Somewhat
> modernized and cleaned up, for better or worse.
> 
> Modified to plug into the olpc-ec driver infrastructure (so that battery
> interface and debugfs could be reused) and the SPI slave framework.
> 
> Signed-off-by: Lubomir Rintel <lkundrak@...sk>
> 

Hi Lubomir,

You asked for some tips on how to incorporate the changes in a patch
series like this.

Keep in mind that each patch should create an independent small
functional change. This makes it easier to review the patch and verify
that what you said you were going to do matches what the patch does. For
example, if you separate out the style, whitespace, ordering of
declarations into an initial first patch, then all that noise is removed
when the reviewer goes to check to implementation of one of the features
below.

This is a large patch, and should most certainly be broken up into
several smaller patches. Do cleanups first, followed by functional
changes. This ordering ensure that when a "git blame" is used in the
future to understand why a given line is what it is, the first hit will
be a functional change, and not a cleanup.

> ---
> Changes since v1:
> - Cosmetic style changes; whitespace, ordering of declarations and
>   #includes, remoted extra comas from sentinels

Please make this a separate change, possibly more than one, depending on
how many of these there are. This will reduce the size of the subsequent
patches, making them easier to review.

> - Count the terminating NUL in LOG_BUF_SIZE
> - Make olpc_xo175_ec_is_valid_cmd() return -EINVAL instead of -1
>   on error
> - Spell keyboard/touchpad in full for CHAN_KEYBOARD/TOUCHPAD messages
> - Use a #define for PM wakeup processing time
> - Log a message on unknown event
> - Escape logging payload with %*pE
> - Replace an open-coded min()
> - Correct an error code on short read
> - replaced PM callback #ifdefs with __maybe_unusedm SET_RUNTIME_PM_OPS
>   and SET_NOIRQ_SYSTEM_SLEEP_PM_OPS
> - dev_get_drvdata() instead of a round-trip through platform device
> - s/unsigned char x/u8 x/ in olpc_xo175_ec_resume()
> - Use GENMASK() instead of 0xffff for the event mask
> - Replace cmd tx/resp rx buffers with structures
> - Turned suspend hint arguments into a struct, and tidied up the comment

Just from these comments, each of these could be a separate patch. You
can group related things together, or those that change the same line or
function for example. Order them with cleanups / non-functional-changes
first, followed by functional changes.

> 
> Basically all of the above is based on the review by Andy Shevchenko.

Andy, what was your intent for Lubomir here? From the above, this looks
like it should be several patches to me.

Thanks,

-- 
Darren Hart
VMware Open Source Technology Center

Powered by blists - more mailing lists