Hi, Daniel: 2018-01-05 3:48 GMT+08:00 Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>: > On 04/01/2018 15:06, Greentime Hu wrote: >> Hi, Daniel: >> >> 2018-01-04 21:50 GMT+08:00 Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>: >>> >>> Hi, >>> >>> sorry I missed your answer. Comments below. >>> >>> On 13/12/2017 07:06, Greentime Hu wrote: >>>> Hi, Daniel: >>>> >>>> 2017-12-12 18:05 GMT+08:00 Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>: >>>>> On 12/12/2017 06:46, Rick Chen wrote: >>>>>> ATCPIT100 is often used on the Andes architecture, >>>>>> This timer provide 4 PIT channels. Each PIT channel is a >>>>>> multi-function timer, can be configured as 32,16,8 bit timers >>>>>> or PWM as well. >>>>>> >>>>>> For system timer it will set channel 1 32-bit timer0 as clock >>>>>> source and count downwards until underflow and restart again. >>>>> >>>>> [ ... ] >>>>> >>>>>> +config CLKSRC_ATCPIT100 >>>>>> + bool "Clocksource for AE3XX platform" >>>>>> + depends on NDS32 || COMPILE_TEST >>>>>> + depends on HAS_IOMEM >>>>>> + help >>>>>> + This option enables support for the Andestech AE3XX platform timers. >>>>> >>>>> Hi Rick, >>>>> >>>>> the general rule for the Kconfig is: >>>>> >>>>> bool "Clocksource for AE3XX platform" if COMPILE_TEST > > BTW, select TIMER_OF is missing. We don't select here because we select TIMER_OF in arch/nds32/Kconfig I am not sure if I still need to select TIMER_OF here? >>>>> and no deps on the platform. >>>>> >>>>> It is up to the platform Kconfig to select the option. >>>>> >>>>> We want here a silent option but make it selectable in case of >>>>> compilation test coverage. >>>> >>>> >>>> The way we like to use it is because >>>> 1. This timer is a basic component to boot an nds32 CPU and it should >>>> be able to select without COMPILE_TEST for nds32 architecture. >>> >>> Yes, so you don't need it to be selectable, you must select it from the >>> platform's Kconfig. >> >> I am not sure that I get your point or not. >> We don't have a CONFIG_PLAT_AE3XX. >> Do you mean we should create one and select CLKSRC_ATCPIT100 under >> CONFIG_PLAT_AE3XX? > > No. Can't you add in arch/ndis32/Kconfig ? > > +select TIMER_ATCPIT100 > > Like: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/Kconfig#n50 IMHO, it might be a little bit wierd if we select TIMER_ATCPIT100 in arch/nds32/Kconfig because it is part of SoC instead of CPU. If we change to another SoC with another timer, we need to select another TIMER in arch/nds32/Kconfig and delete TIMER_ATCPIT100. It seems more flexible to be selected in driver layer. It seems to be the timer is part of the arch to be selected in arch's Kconfig. arch/arc/Kconfig: select ARC_TIMERS arch/arc/Kconfig: select ARC_TIMERS_64BIT arch/arm/Kconfig: select ARM_ARCH_TIMER arch/arm64/Kconfig: select ARM_ARCH_TIMER arch/blackfin/Kconfig: select BFIN_GPTIMERS >>>> 2. It seems conflict with debug info. I am not sure if there is >>>> another way to debug kernel(with debug info) with COMPILE_TEST and >>>> DEBUG_INFO because we need this driver for nds32 architecture. >>>> >>>> Symbol: DEBUG_INFO [=n] >>>> Type : boolean >>>> Prompt: Compile the kernel with debug info >>>> Location: >>>> -> Kernel hacking >>>> -> Compile-time checks and compiler options >>>> Defined at lib/Kconfig.debug:140 >>>> Depends on: DEBUG_KERNEL [=y] && !COMPILE_TEST [=n] >>> >>> The COMPILE_TEST option is only there to allow cross-compilation test >>> coverage, it does not select or unselect the driver in usual way. >>> >>> If the COMPILE_TEST is enabled, then the option will appear in the >>> menuconfig, so that gives the opportunity to select/unselect it. >>> >>> Otherwise, the Kconfig's platform selects automatically the driver and >>> the user *can't* unselect it from the menuconfig as it is a silent >>> option and that is certainly what you want. >>> >>>>> Also, this driver is not a CLKSRC but a TIMER. Rename CLKSRC_ATCPIT100 >>>>> to TIMER_ATCPIT100. >>>> >>>> Thanks. We will rename it in the next version patch. >>> >>> You just resend an entire series V5 for the architecture. I'm confused, >>> what is the merging path ? >> >> Sorry. I didn't get your point. >> We sent the timer patch and the architecture patch together because it >> would be easier for reviewer to check the vdso implementations. >> What do you mean about the merging path? > > I received a [Vx y/3] series and I received a [Vx y/39]. > > The former from Rick Chen means to me "please pick them through your tree". > > The latter from you means to me "can you ack the patches so I can merge > them through my tree". Note you will have to resend the entire arch > series for every single review/comment (that could end up upset the > Cc'ed people). > > Which one should I review ? I can not track different patchset > implementing the same thing. Which one should I comment, review ? Are > the comments I did on [Vx y/3] taken into account in the arch series ? > etc ... > > Please clarify, it is confusing and impossible to review in this situation. > > I suggest we stick to the x/3 series, so I can comment it and you can > resend a new version without resending the entire arch series. So I can > merge it through my tree, and you get it via eg. a shared immutable > branch. The arch series will be reduced by 3 patches. Thanks for your explanation. :) I will send these 2 patch set separately next time. -- To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
