Hi, Daniel:

2018-01-05 3:48 GMT+08:00 Daniel Lezcano [off-list ref]:

On 04/01/2018 15:06, Greentime Hu wrote:

quoted

Hi, Daniel:

2018-01-04 21:50 GMT+08:00 Daniel Lezcano [off-list ref]:

quoted

Hi,

sorry I missed your answer. Comments below.

On 13/12/2017 07:06, Greentime Hu wrote:

quoted

Hi, Daniel:

2017-12-12 18:05 GMT+08:00 Daniel Lezcano [off-list ref]:

quoted

On 12/12/2017 06:46, Rick Chen wrote:

quoted

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.

[ ... ]

quoted

+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?

quoted

quoted

quoted

quoted

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

quoted

quoted

quoted

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.

quoted

quoted

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.