Re: [PATCH v5 1/3] clocksource/drivers/atcpit100: Add andestech atcpit100 timer

[Daniel Lezcano <daniel.lezcano@linaro.org>]

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

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.

Also, this driver is not a CLKSRC but a TIMER. Rename CLKSRC_ATCPIT100
to TIMER_ATCPIT100.

> +
>  endmenu
> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> index 72711f1..7d072f5 100644
> --- a/drivers/clocksource/Makefile
> +++ b/drivers/clocksource/Makefile
> @@ -75,3 +75,4 @@ obj-$(CONFIG_H8300_TMR16)		+= h8300_timer16.o
>  obj-$(CONFIG_H8300_TPU)			+= h8300_tpu.o
>  obj-$(CONFIG_CLKSRC_ST_LPC)		+= clksrc_st_lpc.o
>  obj-$(CONFIG_X86_NUMACHIP)		+= numachip.o
> +obj-$(CONFIG_CLKSRC_ATCPIT100)		+= timer-atcpit100.o

[ ... ]

> +static struct timer_of to = {
> +	.flags = TIMER_OF_IRQ | TIMER_OF_CLOCK | TIMER_OF_BASE,
> +
> +	.clkevt = {
> +		.name = "atcpit100_tick",
> +		.rating = 300,
> +		.features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT,
> +		.set_state_shutdown = atcpit100_clkevt_shutdown,
> +		.set_state_periodic = atcpit100_clkevt_set_periodic,
> +		.set_state_oneshot = atcpit100_clkevt_set_oneshot,
> +		.tick_resume = atcpit100_clkevt_shutdown,
> +		.set_next_event = atcpit100_clkevt_next_event,
> +		.cpumask = cpu_all_mask,

You may consider CLOCK_EVT_DYN_IRQ
https://lwn.net/Articles/540160/

> +	},
> +
> +	.of_irq = {
> +		.handler = atcpit100_timer_interrupt,
> +		.flags = IRQF_TIMER | IRQF_IRQPOLL,
> +	},
> +
> +	/*
> +	 * FIXME: we currently only support clocking using PCLK
> +	 * and using EXTCLK is not supported in the driver.
> +	 */
> +	.of_clk = {
> +		.name = "PCLK",
> +	}

What do you mean ? We can't specify several clock names with timer-of?


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog