From: Olof Johansson <olof@lixom.net> To: Linus Torvalds <torvalds@linux-foundation.org> Cc: Patrick Venture <venture@google.com>, 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 10:39:08 -0700 [thread overview] Message-ID: <CAOesGMjY+OOLP6oYgz9f4+VHwj-H1rA7K+u=QHTG1t2xG9YhUw@mail.gmail.com> (raw) In-Reply-To: <CAHk-=whb-KituxcvM6ZPuXqyPX+rJENb8cnGCPbGE9pyqwOmXA@mail.gmail.com> On Thu, May 16, 2019 at 9:27 AM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > 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. Yeah, that's the approach we're heading towards with aspeed. Part of the reason for why I wasn't 100% sure we wanted to move all drivers over, is that most of drivers/soc has been for "soc glue logic" code, not for the little SoC-specific drivers where we've pushed hard to get out into their best-matching driver directories instead. Aspeed is an unusually "messy" SoC in that it has a handful of little widgets used to communicate with the host (in its role as BMC), and either we'd squint and put all of them in drivers/misc, or we could pick them up in drivers/soc as we're now doing. Either way the code will be in the kernel, and keeping it together might not be a bad idea. We might get more of a kitchen sink in drivers/soc over time with this slight change in approach, but we've dealt with messes before and if it happens, we'll clean it up when it gets too bad. Sometimes letting it happen is the best way of seeing the bigger picture and not over-engineer something upfront. > 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. Yeah. I think that's fine in this case. I've got some horror stories from botched rebases where merges ended up containing actual code changes and that caused immense confusion, but that's not the case 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. Yep, looks good -- thanks! > 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. We'll follow up with patches for that, thanks for pointing it out. I have to admit that most of my focus tends to be to check for new errors, not whether everything still is built. Easy to automate, so I'll poke around with it a bit. -Olof
WARNING: multiple messages have this Message-ID (diff)
From: Olof Johansson <olof@lixom.net> To: Linus Torvalds <torvalds@linux-foundation.org> Cc: Patrick Venture <venture@google.com>, 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>, Greg Kroah-Hartman <gregkh@linuxfoundation.org> Subject: Re: [GIT PULL 3/4] ARM: SoC-related driver updates Date: Thu, 16 May 2019 10:39:08 -0700 [thread overview] Message-ID: <CAOesGMjY+OOLP6oYgz9f4+VHwj-H1rA7K+u=QHTG1t2xG9YhUw@mail.gmail.com> (raw) In-Reply-To: <CAHk-=whb-KituxcvM6ZPuXqyPX+rJENb8cnGCPbGE9pyqwOmXA@mail.gmail.com> On Thu, May 16, 2019 at 9:27 AM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > 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. Yeah, that's the approach we're heading towards with aspeed. Part of the reason for why I wasn't 100% sure we wanted to move all drivers over, is that most of drivers/soc has been for "soc glue logic" code, not for the little SoC-specific drivers where we've pushed hard to get out into their best-matching driver directories instead. Aspeed is an unusually "messy" SoC in that it has a handful of little widgets used to communicate with the host (in its role as BMC), and either we'd squint and put all of them in drivers/misc, or we could pick them up in drivers/soc as we're now doing. Either way the code will be in the kernel, and keeping it together might not be a bad idea. We might get more of a kitchen sink in drivers/soc over time with this slight change in approach, but we've dealt with messes before and if it happens, we'll clean it up when it gets too bad. Sometimes letting it happen is the best way of seeing the bigger picture and not over-engineer something upfront. > 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. Yeah. I think that's fine in this case. I've got some horror stories from botched rebases where merges ended up containing actual code changes and that caused immense confusion, but that's not the case 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. Yep, looks good -- thanks! > 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. We'll follow up with patches for that, thanks for pointing it out. I have to admit that most of my focus tends to be to check for new errors, not whether everything still is built. Easy to automate, so I'll poke around with it a bit. -Olof _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2019-05-16 17:39 UTC|newest] Thread overview: 36+ 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 ` Olof Johansson 2019-05-16 6:43 ` [GIT PULL 1/4] ARM: SoC platform updates Olof Johansson 2019-05-16 6:43 ` Olof Johansson 2019-05-16 15:33 ` Linus Torvalds 2019-05-16 15:33 ` Linus Torvalds 2019-05-16 15:53 ` Arnd Bergmann 2019-05-16 15:53 ` Arnd Bergmann 2019-05-16 17:10 ` Olof Johansson 2019-05-16 17:10 ` Olof Johansson 2019-05-20 21:56 ` Linus Walleij 2019-05-20 21:56 ` Linus Walleij 2019-05-16 15:59 ` Marc Gonzalez 2019-05-16 15:59 ` Marc Gonzalez 2019-05-16 16:34 ` Linus Torvalds 2019-05-16 16:34 ` Linus Torvalds 2019-05-16 16:40 ` pr-tracker-bot 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 6:43 ` Olof Johansson 2019-05-16 16:40 ` pr-tracker-bot 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 6:43 ` Olof Johansson 2019-05-16 16:26 ` Linus Torvalds 2019-05-16 16:26 ` Linus Torvalds 2019-05-16 16:35 ` Patrick Venture 2019-05-16 16:35 ` Patrick Venture 2019-05-16 17:39 ` Olof Johansson [this message] 2019-05-16 17:39 ` Olof Johansson 2019-05-16 16:40 ` pr-tracker-bot 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 6:43 ` Olof Johansson 2019-05-16 16:40 ` pr-tracker-bot 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='CAOesGMjY+OOLP6oYgz9f4+VHwj-H1rA7K+u=QHTG1t2xG9YhUw@mail.gmail.com' \ --to=olof@lixom.net \ --cc=arm@kernel.org \ --cc=gregkh@linuxfoundation.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=torvalds@linux-foundation.org \ --cc=venture@google.com \ /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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.