| Message ID | 20160908172346.27506-1-netz.kernel@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Marty, + Andrew, Russell, Since this is your first series, I'll go ahead and gather a few comments right here: - Make sure to change the subject line for each patch. The subject is retrieved from the first line of the commit. you can look at the output of 'git log --oneline' to see a nice, concise view of your series. - Include 'PATCH' or 'Patch' inside the square brackets on the subject line. When you use the prefix option, as you did, it *replaces* 'PATCH', so you need '--subject-prefix="RFC PATCH"' or '--subject-prefix="RFC PATCH V2"' - Please Cc Andrew, Russell and myself on future series. Personally, I do that at the 'send-email' stage instead of 'format-patch'. But whichever you prefer. Don't be shy about Cc'ing. We all are accustomed to handling lots of email. It makes it easier for us to spot stuff we are actively involved in. :-) thx, Jason. On Thu, Sep 08, 2016 at 12:23:40PM -0500, Marty Plummer wrote: > This is a preparatory series for adding the ARMv5/v6 hi3520 SoCs. > Assumptions were made when adding hi3620 that don't hold water in > light of adding support for the hi3520 SoC. Fix the issue by renaming > config options and other namespaces to avoid collisions with the new > work. > > Only internal APIs are modified with this series. > > Signed-off-by: Marty Plummer <netz.kernel@gmail.com> > --- > arch/arm/Kconfig.debug | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug > index a9693b6..9094ca6 100644 > --- a/arch/arm/Kconfig.debug > +++ b/arch/arm/Kconfig.debug > @@ -280,7 +280,7 @@ choice > > config DEBUG_HI3620_UART > bool "Hisilicon HI3620 Debug UART" > - depends on ARCH_HI3xxx > + depends on ARCH_HI3620 > select DEBUG_UART_PL01X > help > Say Y here if you want kernel low-level debugging support > -- > 2.9.3 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 09/08/2016 12:47 PM, Jason Cooper wrote: > Hi Marty, > > + Andrew, Russell, > > > Since this is your first series, I'll go ahead and gather a few comments > right here: > > - Make sure to change the subject line for each patch. > > The subject is retrieved from the first line of the commit. you can > look at the output of 'git log --oneline' to see a nice, concise view of > your series. > Ah crap. I knew the subject was taken from the first line of the commit, but to be honest I didn't think there was any real room for unique subjects here, since its quite literally the exact same changes in each file affected, but I take it this is a general rule that each patch in a series has to have a unique subject, then? > - Include 'PATCH' or 'Patch' inside the square brackets on the subject > line. > > When you use the prefix option, as you did, it *replaces* 'PATCH', so > you need '--subject-prefix="RFC PATCH"' or '--subject-prefix="RFC PATCH V2"' Ah ok. I didn't realize you needed both RFC and PATCH in this instance. > > - Please Cc Andrew, Russell and myself on future series. > > Personally, I do that at the 'send-email' stage instead of > 'format-patch'. But whichever you prefer. Don't be shy about Cc'ing. > We all are accustomed to handling lots of email. It makes it easier for > us to spot stuff we are actively involved in. :-) > > thx, > > Jason. > Alrighty, I sorta realized this issue right after I sent, will do in future instances. Also, you say at 'send-email' instead of 'format-patch'; I take it there is a way to do the latter? I'm not aware of it :) > On Thu, Sep 08, 2016 at 12:23:40PM -0500, Marty Plummer wrote: >> This is a preparatory series for adding the ARMv5/v6 hi3520 SoCs. >> Assumptions were made when adding hi3620 that don't hold water in >> light of adding support for the hi3520 SoC. Fix the issue by renaming >> config options and other namespaces to avoid collisions with the new >> work. >> >> Only internal APIs are modified with this series. >> >> Signed-off-by: Marty Plummer <netz.kernel@gmail.com> >> --- >> arch/arm/Kconfig.debug | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug >> index a9693b6..9094ca6 100644 >> --- a/arch/arm/Kconfig.debug >> +++ b/arch/arm/Kconfig.debug >> @@ -280,7 +280,7 @@ choice >> >> config DEBUG_HI3620_UART >> bool "Hisilicon HI3620 Debug UART" >> - depends on ARCH_HI3xxx >> + depends on ARCH_HI3620 >> select DEBUG_UART_PL01X >> help >> Say Y here if you want kernel low-level debugging support >> -- >> 2.9.3 >> >> >> _______________________________________________ >> linux-arm-kernel mailing list >> linux-arm-kernel@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Thu, Sep 08, 2016 at 12:55:50PM -0500, Marty Plummer wrote: > On 09/08/2016 12:47 PM, Jason Cooper wrote: > > - Please Cc Andrew, Russell and myself on future series. > > > > Personally, I do that at the 'send-email' stage instead of > > 'format-patch'. But whichever you prefer. Don't be shy about Cc'ing. > > We all are accustomed to handling lots of email. It makes it easier for > > us to spot stuff we are actively involved in. :-) > > > Alrighty, I sorta realized this issue right after I sent, will do in > future instances. Also, you say at 'send-email' instead of > 'format-patch'; I take it there is a way to do the latter? I'm not aware > of it :) Both git send-email and git format-patch take --to, --cc, --bcc as arguments. I prefer to do all of the email addressing with send-email instead of while doing format-patch. But it's personal preference, that's why the options are in both. thx, Jason.
Hi Marty Adding to Jasons comments... a [0/7] "patch" created via --cover should be include with all multi-patch series. It should explain the big picture for the whole set. What does this set do. Then you can leave each patch just explaining what it does. Some maintainers will use the cover note as the log for the merge commit. So keep it formal in the same way as the individual patches comment. Andrew
On Thursday, September 8, 2016 12:23:40 PM CEST Marty Plummer wrote: > arch/arm/Kconfig.debug | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug > index a9693b6..9094ca6 100644 > --- a/arch/arm/Kconfig.debug > +++ b/arch/arm/Kconfig.debug > @@ -280,7 +280,7 @@ choice > > config DEBUG_HI3620_UART > bool "Hisilicon HI3620 Debug UART" > - depends on ARCH_HI3xxx > + depends on ARCH_HI3620 > select DEBUG_UART_PL01X > help > Say Y here if you want kernel low-level debugging support > While patches should normally be split up into per-subsystem changes, they also have to be done in an 'atomic' way: applying just the first patch without the second one must not introduce a regression. In this case, I'd suggest simply doing a larger patch for the global rename to collect 'Acked-by's and send that through the arm-soc tree. If you get in a similar situation with more complex changes, you should come up with a way to do it independently. Here you would first have to introduce a CONFIG_ARCH_HI3620 symbol in one patch and make that always selected at the same time as CONFIG_ARCH_HI3xxx, then change all users of that symbol, and finally remove the original. Arnd
On 09/08/2016 03:05 PM, Arnd Bergmann wrote: > On Thursday, September 8, 2016 12:23:40 PM CEST Marty Plummer wrote: > >> arch/arm/Kconfig.debug | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug >> index a9693b6..9094ca6 100644 >> --- a/arch/arm/Kconfig.debug >> +++ b/arch/arm/Kconfig.debug >> @@ -280,7 +280,7 @@ choice >> >> config DEBUG_HI3620_UART >> bool "Hisilicon HI3620 Debug UART" >> - depends on ARCH_HI3xxx >> + depends on ARCH_HI3620 >> select DEBUG_UART_PL01X >> help >> Say Y here if you want kernel low-level debugging support >> > > While patches should normally be split up into per-subsystem > changes, they also have to be done in an 'atomic' way: > applying just the first patch without the second one must > not introduce a regression. > > In this case, I'd suggest simply doing a larger patch for > the global rename to collect 'Acked-by's and send that through > the arm-soc tree. > > If you get in a similar situation with more complex changes, > you should come up with a way to do it independently. Here > you would first have to introduce a CONFIG_ARCH_HI3620 > symbol in one patch and make that always selected at > the same time as CONFIG_ARCH_HI3xxx, then change all users > of that symbol, and finally remove the original. > > Arnd > So, in this case it should be one single patch? Geeze this is hella complicated to just get a name changed XD
On Thursday, September 8, 2016 3:12:26 PM CEST Marty Plummer wrote: > So, in this case it should be one single patch? Geeze this is hella > complicated to just get a name changed Yes and yes. Sometimes it's easier to leave an slightly misleading name than to change it. ;-) If there is any relation between Hi3620 and Hi3520, we could also decide to use the "HI3xxx" name for both after all, and just change the Kconfig files like config ARCH_HI3xxx bool "Hisilicon Hi36xx family" depends on ARCH_MULTI_V5 || ARCH_MULTI_V6 || ARCH_MULTI_V7 select CACHE_L2X0 if CPU_V7 select HAVE_ARM_SCU if SMP ... config DEBUG_HI3520_UART bool "Hisilicon HI3520 Debug UART" depends on ARCH_HI3xxx select DEBUG_UART_PL01X help Say Y here if you want kernel low-level debugging support config DEBUG_HI3620_UART bool "Hisilicon HI3620 Debug UART" depends on ARCH_HI3xxx select DEBUG_UART_PL01X help Say Y here if you want kernel low-level debugging support Arnd
diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug index a9693b6..9094ca6 100644 --- a/arch/arm/Kconfig.debug +++ b/arch/arm/Kconfig.debug @@ -280,7 +280,7 @@ choice config DEBUG_HI3620_UART bool "Hisilicon HI3620 Debug UART" - depends on ARCH_HI3xxx + depends on ARCH_HI3620 select DEBUG_UART_PL01X help Say Y here if you want kernel low-level debugging support
This is a preparatory series for adding the ARMv5/v6 hi3520 SoCs. Assumptions were made when adding hi3620 that don't hold water in light of adding support for the hi3520 SoC. Fix the issue by renaming config options and other namespaces to avoid collisions with the new work. Only internal APIs are modified with this series. Signed-off-by: Marty Plummer <netz.kernel@gmail.com> --- arch/arm/Kconfig.debug | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)