LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Patrick Venture <venture@google.com>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Olof Johansson <olof@lixom.net>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	ARM SoC <arm@kernel.org>,
	Linux List Kernel Mailing <linux-kernel@vger.kernel.org>,
	"linux-alpha@vger.kernel.org"
	<linux-arm-kernel@lists.infradead.org>
Subject: Re: [GIT PULL 3/4] ARM: SoC-related driver updates
Date: Thu, 16 May 2019 09:35:14 -0700	[thread overview]
Message-ID: <CAO=notySOzSjJS9jBCF9fyXEUK7VDZQiJp3WaSLs4Y7X7PC8=Q@mail.gmail.com> (raw)
In-Reply-To: <CAHk-=whb-KituxcvM6ZPuXqyPX+rJENb8cnGCPbGE9pyqwOmXA@mail.gmail.com>

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Thu, May 16, 2019 at 9:27 AM
To: Olof Johansson, Patrick Venture, Greg Kroah-Hartman
Cc: ARM SoC, Linux List Kernel Mailing, linux-alpha@vger.kernel.org

> On Wed, May 15, 2019 at 11:43 PM Olof Johansson <olof@lixom.net> wrote:
> >
> > Various driver updates for platforms and a couple of the small driver
> > subsystems we merge through our tree:
>
> Hmm. This moved the aspeed drivers from drivers/misc to
> drivers/soc/aspeed (in commit 524feb799408 "soc: add aspeed folder and
> misc drivers"), but in the meantime we also had a new aspeed soc
> driver added (in commit 01c60dcea9f7 "drivers/misc: Add Aspeed P2A
> control driver").
>
> I ended up resolving that "conflict" by moving the new aspeed P2A
> control driver to be with the other aspeed drivers too. That seemed to
> be the cleanest model.

Thank you.  I agree.  There was some back-and-forth about the SoC move
w.r.t any new aspeed misc drivers. Whether moving them into SoC was a
good approach versus leaving the growing list in misc.  Another aspeed
driver, controlling UART was headed to misc and received push-back
that it was sufficiently specialized to go into SoC
(https://patchwork.ozlabs.org/patch/969238/).  This feedback triggered
this staging move.

I think storing the growing misc drivers for these SoCs (Aspeed,
Nuvoton) in a SoC folder is a reasonable grouping.

>
> I'm used to doing these kinds of fixups in a merge, but I have to
> admit that maybe I should have made it a separate commit, because now
> it's kind of non-obvious, and it's sometimes harder to see changes
> that are in a merge commit than in a separate commit.
>
> In particular, it looks like "git log --follow" is not smart enough to
> follow a rename through a merge. But I think that is a git problem,
> and not a very serious one at that ("git blame" has no such problem).
>
> And it means that now the merge has
>
>  drivers/{misc => soc/aspeed}/aspeed-lpc-ctrl.c                   |   0
>  drivers/{misc => soc/aspeed}/aspeed-lpc-snoop.c                  |   0
>  drivers/{misc => soc/aspeed}/aspeed-p2a-ctrl.c                   |   0
>
> when you do "git show --stat" on it, which looks correct, and it feels
> like conceptually the right merge resolution to me.
>
> Sending out this explanatory email to everybody involved, just so that
> this doesn't take you by surprise. But it looks like Patrick Venture
> is not just the author of that moved driver, he was also involved in
> the move of the two other drivers, so I'm guessing there's not going
> to be a lot of confusion here.
>
> HOWEVER. More subtly, as part of my *testing* for this, I also
> realized that commit 524feb799408 is buggy. In my tests, the config
> worked fine, but the aspeed drivers were never actually *built*. The
> reason is that commit 524feb799408 ends up doing
>
>    obj-$(CONFIG_ARCH_ASPEED)      += aspeed/
>
> which is completely wrong, because the Kconfig fules are
>
>         depends on (ARCH_ASPEED || COMPILE_TEST) && REGMAP && MFD_SYSCON
>
> so those drivers can be configured even if ARCH_ASPEED *isn't* set.
> The Kconfig part works fine, because the soc/aspeed/Kconfig file is
> included unconditionally, but the actual build process then never
> builds anything in the drivers/soc/aspeed/ subdirectory.
>
> I solved _that_ problem by adding a new config option:
>
>   config SOC_ASPEED
>       def_bool y
>       depends on ARCH_ASPEED || COMPILE_TEST
>
> and using that instead of ARCH_ASPEED.

Thank you, that makes perfect sense.  When moving the drivers, I was
only considering the case where one is compiling them for use and
forgot to check for COMPILE_TEST.

>
> End result: this was a somewhat messy merge, and the most subtle mess
> was because of that buggy 524feb799408 "soc: add aspeed folder and
> misc drivers").
>
> I *think* I sorted it all out correctly, and now I see the aspeed
> drivers being built (and cleanly at that) but I really *really* want
> people to double-check this all.
>
> Also, I think that the same "we don't actually build-test the end
> result" problem exists else-where for the same reasons.
>
> At the very least, drivers/soc/{atmel,rockchip,zte} seem to have the
> exact same pattern: the Kconfig files enable the drivers, but the
> Makefile in drivers/soc doesn't actually traverse into the
> subdirectories.
>
> End result: CONFIG_COMPILE_TEST doesn't actually do any compile
> testing for those drivers.
>
> I did not try to fix all of those things up, because I didn't do the
> driver movements there.
>
>                   Linus

  reply	other threads:[~2019-05-16 16:35 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-16  6:43 [GIT PULL 0/4] ARM: SoC contents for 5.2 merge window Olof Johansson
2019-05-16  6:43 ` [GIT PULL 1/4] ARM: SoC platform updates Olof Johansson
2019-05-16 15:33   ` Linus Torvalds
2019-05-16 15:53     ` Arnd Bergmann
2019-05-16 17:10       ` Olof Johansson
2019-05-20 21:56         ` Linus Walleij
2019-05-16 15:59     ` Marc Gonzalez
2019-05-16 16:34       ` Linus Torvalds
2019-05-16 16:40   ` pr-tracker-bot
2019-05-16  6:43 ` [GIT PULL 2/4] ARM: Device-tree updates Olof Johansson
2019-05-16 16:40   ` pr-tracker-bot
2019-05-16  6:43 ` [GIT PULL 3/4] ARM: SoC-related driver updates Olof Johansson
2019-05-16 16:26   ` Linus Torvalds
2019-05-16 16:35     ` Patrick Venture [this message]
2019-05-16 17:39     ` Olof Johansson
2019-05-16 16:40   ` pr-tracker-bot
2019-05-16  6:43 ` [GIT PULL 4/4] ARM: SoC defconfig updates Olof Johansson
2019-05-16 16:40   ` pr-tracker-bot

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='CAO=notySOzSjJS9jBCF9fyXEUK7VDZQiJp3WaSLs4Y7X7PC8=Q@mail.gmail.com' \
    --to=venture@google.com \
    --cc=arm@kernel.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=olof@lixom.net \
    --cc=torvalds@linux-foundation.org \
    --subject='Re: [GIT PULL 3/4] ARM: SoC-related driver updates' \
    /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

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).