On Mon, May 29, 2017 at 8:36 PM, Joe Perches <joe@xxxxxxxxxxx> wrote: > On Mon, 2017-05-29 at 20:11 +0300, Gilad Ben-Yossef wrote: >> On Mon, May 29, 2017 at 7:57 PM, Joe Perches <joe@xxxxxxxxxxx> wrote: >> > On Mon, 2017-05-29 at 16:37 +0200, Greg Kroah-Hartman wrote: >> > > On Sun, May 28, 2017 at 05:40:26PM +0300, Gilad Ben-Yossef wrote: >> > > > cc_crypto_ctx.h had multiple coding style violations reported by >> > > > checkpatch. Fix them all. >> > > >> > > Sorry, no. You need to do only one-thing-per-patch, and "fix all coding >> > > style issues is not "one thing". I wouldn't take this kind of patch >> > > from anyone else, so why should I take it from you? :) >> > >> > Because he's the named MAINTAINER of the subsystem >> > and you are acting as an upstream conduit. >> > >> >> LOL. Thank you Joe, but I have opted to upstream via staging to get the guidance >> and review of Greg and other developers kind enough to offer it, and I'd be a >> fool not to listen to them. > > For reviews of technical merit, true. > > For reviews of commit log wording of whitespace > changes, where git diff -w shows no difference, > less true. > > This patch seems almost entirely whitespace except > one bit reformatting a comment block. > > Breaking those down into changes like: > added space after commas > added spaces around bit shifts > added spaces around arithmetic > is simply excessive. I'll admit that this was my line of reasoning as well for including it in a single patch but I if Greg finds it easier to review broken down I'm fine with that. Something tells me he sees a lot of patches... :-) > The only comment I would have given would be to > change the patch context that added line comment > initiators to use the /** kernel-doc style. > > And maybe a style change of > > #define CC_MULTI2_SYSTEM_N_DATA_KEY_SIZE (CC_MULTI2_SYSTEM_KEY_SIZE + \ > CC_MULTI2_DATA_KEY_SIZE) > > to > > #define CC_MULTI2_SYSTEM_N_DATA_KEY_SIZE \ > (CC_MULTI2_SYSTEM_KEY_SIZE + CC_MULTI2_DATA_KEY_SIZE) > > as it's sometimes easier to scan arithmetic on a single line. > Thanks for the feedback. I will include it in the revised series. > btw: this #define seems misleading as it's used in both .min_keysize > and .max_keysize with "+ 1" and key[CC_MULTI2_SYSTEM_N_DATA_KEY_SIZE] > is also used. > I'll look into that. There are still areas in this driver I've inherited that I find... intriguing :-) Thanks, Gilad -- Gilad Ben-Yossef Chief Coffee Drinker "If you take a class in large-scale robotics, can you end up in a situation where the homework eats your dog?" -- Jean-Baptiste Queru
