Message-ID: <CAO=notySOzSjJS9jBCF9fyXEUK7VDZQiJp3WaSLs4Y7X7PC8=Q@mail.gmail.com>
Date:   Thu, 16 May 2019 09:35:14 -0700
From:   Patrick Venture <venture@...gle.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Olof Johansson <olof@...om.net>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        ARM SoC <arm@...nel.org>,
        Linux List Kernel Mailing <linux-kernel@...r.kernel.org>,
        "linux-alpha@...r.kernel.org" <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [GIT PULL 3/4] ARM: SoC-related driver updates

From: Linus Torvalds <torvalds@...ux-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@...r.kernel.org

> On Wed, May 15, 2019 at 11:43 PM Olof Johansson <olof@...om.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

Powered by blists - more mailing lists