Message ID | 20191207130049.27533-2-hyunki00.koo@gmail.com (mailing list archive) |
---|---|
State | Changes Requested |
Headers | show |
Series | remove direct dependancy between irq and exynos | expand |
On Sat, Dec 07, 2019 at 10:00:48PM +0900, Hyunki Koo wrote: > From: Hyunki Koo <hyunki00.koo@samsung.com> > > Not all exynos device have IRQ_COMBINER. > Thus add the config for EXYNOS_IRQ_COMBINER. > > Signed-off-by: Hyunki Koo <hyunki00.koo@samsung.com> > --- > drivers/irqchip/Kconfig | 7 +++++++ > drivers/irqchip/Makefile | 2 +- > 2 files changed, 8 insertions(+), 1 deletion(-) > Hi, There is no changelog and versioning of this patch so I do not understand how it differs with previous. It's a resend? v2? It brings the confusion and looks like you're ignoring previous comments. Looks the same and looks like breaking Exynos platforms in the same way. If you not want to skip combiner on ARMv8, it makes sense, then please follow the approach we did for Pinctrl drivers (PINCTRL_EXYNOS_ARM and PINCTRL_EXYNOS_ARM64). Best regards, Krzysztof > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig > index ba152954324b..3ed7b7f2ae26 100644 > --- a/drivers/irqchip/Kconfig > +++ b/drivers/irqchip/Kconfig > @@ -499,4 +499,11 @@ config SIFIVE_PLIC > > If you don't know what to do here, say Y. > > +config EXYNOS_IRQ_COMBINER > + bool "Samsung Exynos IRQ combiner support" > + depends on ARCH_EXYNOS > + help > + Say yes here to add support for the IRQ combiner devices embedded > + in Samsung Exynos chips. > + > endmenu > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile > index e806dda690ea..60d7c7260fc3 100644 > --- a/drivers/irqchip/Makefile > +++ b/drivers/irqchip/Makefile > @@ -9,7 +9,7 @@ obj-$(CONFIG_ARCH_BCM2835) += irq-bcm2835.o > obj-$(CONFIG_ARCH_BCM2835) += irq-bcm2836.o > obj-$(CONFIG_DAVINCI_AINTC) += irq-davinci-aintc.o > obj-$(CONFIG_DAVINCI_CP_INTC) += irq-davinci-cp-intc.o > -obj-$(CONFIG_ARCH_EXYNOS) += exynos-combiner.o > +obj-$(CONFIG_EXYNOS_IRQ_COMBINER) += exynos-combiner.o > obj-$(CONFIG_FARADAY_FTINTC010) += irq-ftintc010.o > obj-$(CONFIG_ARCH_HIP04) += irq-hip04.o > obj-$(CONFIG_ARCH_LPC32XX) += irq-lpc32xx.o > -- > 2.17.1 >
On Sat, 7 Dec 2019 at 14:28, Krzysztof Kozlowski <krzk@kernel.org> wrote: > > On Sat, Dec 07, 2019 at 10:00:48PM +0900, Hyunki Koo wrote: > > From: Hyunki Koo <hyunki00.koo@samsung.com> > > > > Not all exynos device have IRQ_COMBINER. > > Thus add the config for EXYNOS_IRQ_COMBINER. > > > > Signed-off-by: Hyunki Koo <hyunki00.koo@samsung.com> > > --- > > drivers/irqchip/Kconfig | 7 +++++++ > > drivers/irqchip/Makefile | 2 +- > > 2 files changed, 8 insertions(+), 1 deletion(-) > > > > Hi, > > There is no changelog and versioning of this patch so I do not > understand how it differs with previous. It's a resend? v2? It brings > the confusion and looks like you're ignoring previous comments. > > Looks the same and looks like breaking Exynos platforms in the same way. > > If you not want to skip combiner on ARMv8, it makes sense, then please > follow the approach we did for Pinctrl drivers (PINCTRL_EXYNOS_ARM and > PINCTRL_EXYNOS_ARM64). > > Best regards, > Krzysztof Ah, now I see the second patch. Still you break bisect which requires specific ordering of patches or squashing them into one. Optionally this could be default=y if ARCH_EXYNOS && ARM. I prefer just squashing both into one patch in this case. > > > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig > > index ba152954324b..3ed7b7f2ae26 100644 > > --- a/drivers/irqchip/Kconfig > > +++ b/drivers/irqchip/Kconfig > > @@ -499,4 +499,11 @@ config SIFIVE_PLIC > > > > If you don't know what to do here, say Y. > > > > +config EXYNOS_IRQ_COMBINER > > + bool "Samsung Exynos IRQ combiner support" Now point it to be visible. Only for COMPILE_TEST > > + depends on ARCH_EXYNOS Since you make it a separate option, make it COMPILE_TEST. Best regards, Krzysztof > > + help > > + Say yes here to add support for the IRQ combiner devices embedded > > + in Samsung Exynos chips. > > + > > endmenu > > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile > > index e806dda690ea..60d7c7260fc3 100644 > > --- a/drivers/irqchip/Makefile > > +++ b/drivers/irqchip/Makefile > > @@ -9,7 +9,7 @@ obj-$(CONFIG_ARCH_BCM2835) += irq-bcm2835.o > > obj-$(CONFIG_ARCH_BCM2835) += irq-bcm2836.o > > obj-$(CONFIG_DAVINCI_AINTC) += irq-davinci-aintc.o > > obj-$(CONFIG_DAVINCI_CP_INTC) += irq-davinci-cp-intc.o > > -obj-$(CONFIG_ARCH_EXYNOS) += exynos-combiner.o > > +obj-$(CONFIG_EXYNOS_IRQ_COMBINER) += exynos-combiner.o > > obj-$(CONFIG_FARADAY_FTINTC010) += irq-ftintc010.o > > obj-$(CONFIG_ARCH_HIP04) += irq-hip04.o > > obj-$(CONFIG_ARCH_LPC32XX) += irq-lpc32xx.o > > -- > > 2.17.1 > >
On 19. 12. 7. 오후 10:37, Krzysztof Kozlowski wrote: > On Sat, 7 Dec 2019 at 14:28, Krzysztof Kozlowski <krzk@kernel.org> wrote: >> On Sat, Dec 07, 2019 at 10:00:48PM +0900, Hyunki Koo wrote: >>> From: Hyunki Koo <hyunki00.koo@samsung.com> >>> >>> Not all exynos device have IRQ_COMBINER. >>> Thus add the config for EXYNOS_IRQ_COMBINER. >>> >>> Signed-off-by: Hyunki Koo <hyunki00.koo@samsung.com> >>> --- >>> drivers/irqchip/Kconfig | 7 +++++++ >>> drivers/irqchip/Makefile | 2 +- >>> 2 files changed, 8 insertions(+), 1 deletion(-) >>> >> Hi, >> >> There is no changelog and versioning of this patch so I do not >> understand how it differs with previous. It's a resend? v2? It brings >> the confusion and looks like you're ignoring previous comments. >> >> Looks the same and looks like breaking Exynos platforms in the same way. >> >> If you not want to skip combiner on ARMv8, it makes sense, then please >> follow the approach we did for Pinctrl drivers (PINCTRL_EXYNOS_ARM and >> PINCTRL_EXYNOS_ARM64). >> >> Best regards, >> Krzysztof > Ah, now I see the second patch. Still you break bisect which requires > specific ordering of patches or squashing them into one. Optionally > this could be default=y if ARCH_EXYNOS && ARM. I prefer just squashing > both into one patch in this case. you mean squashing two files arch/arm/mach-exynos/Kconfig and arch/arm/mach-exynos/Kconfig into one patch or squashing into only one file like blow? +config EXYNOS_IRQ_COMBINER + bool "Samsung Exynos IRQ combiner support" + depends on (ARCH_EXYNOS && ARM) || COMPILE_TEST + default y I prefer first one (squashing two files into one patch) > >>> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig >>> index ba152954324b..3ed7b7f2ae26 100644 >>> --- a/drivers/irqchip/Kconfig >>> +++ b/drivers/irqchip/Kconfig >>> @@ -499,4 +499,11 @@ config SIFIVE_PLIC >>> >>> If you don't know what to do here, say Y. >>> >>> +config EXYNOS_IRQ_COMBINER >>> + bool "Samsung Exynos IRQ combiner support" > Now point it to be visible. Only for COMPILE_TEST > >>> + depends on ARCH_EXYNOS > Since you make it a separate option, make it COMPILE_TEST. Is this good ? +config EXYNOS_IRQ_COMBINER + bool "Samsung Exynos IRQ combiner support" + depends on (ARCH_EXYNOS && ARM) || COMPILE_TEST > > Best regards, > Krzysztof > >>> + help >>> + Say yes here to add support for the IRQ combiner devices embedded >>> + in Samsung Exynos chips. >>> + >>> endmenu >>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile >>> index e806dda690ea..60d7c7260fc3 100644 >>> --- a/drivers/irqchip/Makefile >>> +++ b/drivers/irqchip/Makefile >>> @@ -9,7 +9,7 @@ obj-$(CONFIG_ARCH_BCM2835) += irq-bcm2835.o >>> obj-$(CONFIG_ARCH_BCM2835) += irq-bcm2836.o >>> obj-$(CONFIG_DAVINCI_AINTC) += irq-davinci-aintc.o >>> obj-$(CONFIG_DAVINCI_CP_INTC) += irq-davinci-cp-intc.o >>> -obj-$(CONFIG_ARCH_EXYNOS) += exynos-combiner.o >>> +obj-$(CONFIG_EXYNOS_IRQ_COMBINER) += exynos-combiner.o >>> obj-$(CONFIG_FARADAY_FTINTC010) += irq-ftintc010.o >>> obj-$(CONFIG_ARCH_HIP04) += irq-hip04.o >>> obj-$(CONFIG_ARCH_LPC32XX) += irq-lpc32xx.o >>> -- >>> 2.17.1 >>>
On Sun, Dec 08, 2019 at 07:24:49AM +0900, Hyunki Koo wrote: > > On 19. 12. 7. 오후 10:37, Krzysztof Kozlowski wrote: > > On Sat, 7 Dec 2019 at 14:28, Krzysztof Kozlowski <krzk@kernel.org> wrote: > > > On Sat, Dec 07, 2019 at 10:00:48PM +0900, Hyunki Koo wrote: > > > > From: Hyunki Koo <hyunki00.koo@samsung.com> > > > > > > > > Not all exynos device have IRQ_COMBINER. > > > > Thus add the config for EXYNOS_IRQ_COMBINER. > > > > > > > > Signed-off-by: Hyunki Koo <hyunki00.koo@samsung.com> > > > > --- > > > > drivers/irqchip/Kconfig | 7 +++++++ > > > > drivers/irqchip/Makefile | 2 +- > > > > 2 files changed, 8 insertions(+), 1 deletion(-) > > > > > > > Hi, > > > > > > There is no changelog and versioning of this patch so I do not > > > understand how it differs with previous. It's a resend? v2? It brings > > > the confusion and looks like you're ignoring previous comments. > > > > > > Looks the same and looks like breaking Exynos platforms in the same way. > > > > > > If you not want to skip combiner on ARMv8, it makes sense, then please > > > follow the approach we did for Pinctrl drivers (PINCTRL_EXYNOS_ARM and > > > PINCTRL_EXYNOS_ARM64). > > > > > > Best regards, > > > Krzysztof > > Ah, now I see the second patch. Still you break bisect which requires > > specific ordering of patches or squashing them into one. Optionally > > this could be default=y if ARCH_EXYNOS && ARM. I prefer just squashing > > both into one patch in this case. > > you mean squashing two files arch/arm/mach-exynos/Kconfig and > arch/arm/mach-exynos/Kconfig into one patch > > or squashing into only one file like blow? > > +config EXYNOS_IRQ_COMBINER > + bool "Samsung Exynos IRQ combiner support" > + depends on (ARCH_EXYNOS && ARM) || COMPILE_TEST > + default y > > I prefer first one (squashing two files into one patch) Squashing two patches into one. > > > > > > > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig > > > > index ba152954324b..3ed7b7f2ae26 100644 > > > > --- a/drivers/irqchip/Kconfig > > > > +++ b/drivers/irqchip/Kconfig > > > > @@ -499,4 +499,11 @@ config SIFIVE_PLIC > > > > > > > > If you don't know what to do here, say Y. > > > > > > > > +config EXYNOS_IRQ_COMBINER > > > > + bool "Samsung Exynos IRQ combiner support" > > Now point it to be visible. Only for COMPILE_TEST Typo from my side. I wanted to say "No point" - this should not be selectable by user. > > > > > > + depends on ARCH_EXYNOS > > Since you make it a separate option, make it COMPILE_TEST. > > Is this good ? Not entirely. The bool should be also with "if COMPILE TEST" so: config EXYNOS_IRQ_COMBINER bool "Samsung Exynos IRQ combiner support" if COMPILE_TEST depends on (ARCH_EXYNOS && ARM) || COMPILE_TEST Best regards, Krzysztof
diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig index ba152954324b..3ed7b7f2ae26 100644 --- a/drivers/irqchip/Kconfig +++ b/drivers/irqchip/Kconfig @@ -499,4 +499,11 @@ config SIFIVE_PLIC If you don't know what to do here, say Y. +config EXYNOS_IRQ_COMBINER + bool "Samsung Exynos IRQ combiner support" + depends on ARCH_EXYNOS + help + Say yes here to add support for the IRQ combiner devices embedded + in Samsung Exynos chips. + endmenu diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile index e806dda690ea..60d7c7260fc3 100644 --- a/drivers/irqchip/Makefile +++ b/drivers/irqchip/Makefile @@ -9,7 +9,7 @@ obj-$(CONFIG_ARCH_BCM2835) += irq-bcm2835.o obj-$(CONFIG_ARCH_BCM2835) += irq-bcm2836.o obj-$(CONFIG_DAVINCI_AINTC) += irq-davinci-aintc.o obj-$(CONFIG_DAVINCI_CP_INTC) += irq-davinci-cp-intc.o -obj-$(CONFIG_ARCH_EXYNOS) += exynos-combiner.o +obj-$(CONFIG_EXYNOS_IRQ_COMBINER) += exynos-combiner.o obj-$(CONFIG_FARADAY_FTINTC010) += irq-ftintc010.o obj-$(CONFIG_ARCH_HIP04) += irq-hip04.o obj-$(CONFIG_ARCH_LPC32XX) += irq-lpc32xx.o