linux-spi.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: NeilBrown <neil@brown.name>
To: Stefan Roese <sr@denx.de>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: devel@driverdev.osuosl.org,
	Sankalp Negi <sankalpnegi2310@gmail.com>,
	linux-spi@vger.kernel.org, Mark Brown <broonie@kernel.org>,
	John Crispin <john@phrozen.org>
Subject: Re: [PATCH 2/2 v2] staging: spi: mt7621: Minor code cleanup
Date: Fri, 01 Feb 2019 21:07:30 +1100	[thread overview]
Message-ID: <87va23kejh.fsf@notabene.neil.brown.name> (raw)
In-Reply-To: <61b2570c-84c6-ccc3-2535-4197030f2cd6@denx.de>


[-- Attachment #1.1: Type: text/plain, Size: 2140 bytes --]

On Fri, Feb 01 2019, Stefan Roese wrote:

> On 01.02.19 10:03, Greg Kroah-Hartman wrote:
>> On Fri, Feb 01, 2019 at 09:57:12AM +0100, Stefan Roese wrote:
>>> This patch cleans up some minor issues with this driver:
>>> - Remove unnecessary header includes
>>> - Sort header alphabetically
>>> - Use correct comment style
>>> - Add return code check on device_reset()
>>> - Remove SPI_BPW_MASK definition (already available in
>>>    include/linux/spi/spi.h)
>>> - Use macros instead of hardcoded values for SPI_MASTER register access
>>>    as suggested by Neil Brown (in mt7621_spi_reset and mt7621_spi_prepare)
>> 
>> When you have to start listing the different things you do in a patch,
>> that's a huge sign you need to break this up into different patches :)
>
> I personally find this over complex for these type of changes, that's
> why I prefer to send such "minor" changes in a single patch. But I
> can definitely split this up into multiple patches, if this is what's
> preferred by you (and others).

I'll put my hand as an "other".
I understand the temptation to combine changes and I've been guilty of
it occasionally myself, but it is generally best avoided.

The key issue for me is ease of review.
I like to be able to look at the patch description, understand it, then
look at the code changes and think "yes, that looks right" though with
more complex changes I'll need to study the code are bit more closely of
course.

I would probably keep

>>> - Remove unnecessary header includes
>>> - Sort header alphabetically

together, but each of the other changes should be one-per-patch.

You also:
 - made some white-space changes
 - deleted a point pre-declaration of struct mt7621_spi

The white-space changes could be combined with comment-style fixes,
the deletion could be combined with the SPI_BPW_MASK deletation.

But in any case, they should be mentioned.

Thanks - these are all valuable improvements.

NeilBrown


>   
>> Please do that here, it should be a series, each one doing a single type
>> of thing.
>
> Sure, will do.
>   
> Thanks,
> Stefan

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 169 bytes --]

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

      parent reply	other threads:[~2019-02-01 10:07 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-02-01  8:57 [PATCH 1/2 v2] staging: spi: mt7621: Switch to SPDX identifier Stefan Roese
2019-02-01  8:57 ` [PATCH 2/2 v2] staging: spi: mt7621: Minor code cleanup Stefan Roese
2019-02-01  9:03   ` Greg Kroah-Hartman
2019-02-01  9:16     ` Stefan Roese
2019-02-01 10:04       ` Greg Kroah-Hartman
2019-02-01 10:07       ` NeilBrown [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87va23kejh.fsf@notabene.neil.brown.name \
    --to=neil@brown.name \
    --cc=broonie@kernel.org \
    --cc=devel@driverdev.osuosl.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=john@phrozen.org \
    --cc=linux-spi@vger.kernel.org \
    --cc=sankalpnegi2310@gmail.com \
    --cc=sr@denx.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).