Re: [PATCH 1/2] irqchip: define EXYNOS_IRQ_COMBINER

[Krzysztof Kozlowski]

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@xxxxxxxxxx> wrote:
> > > On Sat, Dec 07, 2019 at 10:00:48PM +0900, Hyunki Koo wrote:
> > > > From: Hyunki Koo <hyunki00.koo@xxxxxxxxxxx>
> > > > 
> > > > Not all exynos device have IRQ_COMBINER.
> > > > Thus add the config for EXYNOS_IRQ_COMBINER.
> > > > 
> > > > Signed-off-by: Hyunki Koo <hyunki00.koo@xxxxxxxxxxx>
> > > > ---
> > > >   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