All of lore.kernel.org
 help / color / mirror / Atom feed
From: Linus Torvalds <torvalds@linux-foundation.org>
To: Olof Johansson <olof@lixom.net>,
	Patrick Venture <venture@google.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: 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:26:46 -0700	[thread overview]
Message-ID: <CAHk-=whb-KituxcvM6ZPuXqyPX+rJENb8cnGCPbGE9pyqwOmXA@mail.gmail.com> (raw)
In-Reply-To: <20190516064304.24057-4-olof@lixom.net>

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.

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.

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

WARNING: multiple messages have this Message-ID (diff)
From: Linus Torvalds <torvalds@linux-foundation.org>
To: Olof Johansson <olof@lixom.net>,
	Patrick Venture <venture@google.com>,
	 Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: 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:26:46 -0700	[thread overview]
Message-ID: <CAHk-=whb-KituxcvM6ZPuXqyPX+rJENb8cnGCPbGE9pyqwOmXA@mail.gmail.com> (raw)
In-Reply-To: <20190516064304.24057-4-olof@lixom.net>

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.

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.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  reply	other threads:[~2019-05-16 16:27 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 [this message]
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
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='CAHk-=whb-KituxcvM6ZPuXqyPX+rJENb8cnGCPbGE9pyqwOmXA@mail.gmail.com' \
    --to=torvalds@linux-foundation.org \
    --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=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: link
Be 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.