Messages in this thread | | | From | Patrick Venture <> | Date | Thu, 16 May 2019 09:35:14 -0700 | Subject | Re: [GIT PULL 3/4] ARM: SoC-related driver updates |
| |
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
| |