Re: [PATCH v2 04/18] clocksource: Add ARM System timer driver

Hello,

On Fri, Feb 20, 2015 at 07:01:03PM +0100, Maxime Coquelin wrote:

I assume this symbol is enabled later in the series. Would it make sense
to allow enabing the symbol for compile test coverage?

+	select CLKSRC_OF if OF

What happens if ARMV7M_SYSTICK=y but OF=n? Doesn't the driver fail to
compile?

+	select CLKSRC_MMIO
+	help
+	  This options enables support for the ARMv7M system timer unit

the right spelling is ARMv7-M.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

On Fri, 2015-02-20 at 20:54 +0100, Uwe Kleine-König wrote:

On Fri, Feb 20, 2015 at 07:01:03PM +0100, Maxime Coquelin wrote:

quoted

This patch adds clocksource support for ARMv7-M's System timer,
also known as SysTick.

Signed-off-by: Maxime Coquelin <mcoquelin.stm32@gmail.com>
---
 drivers/clocksource/Kconfig          |  7 ++++
 drivers/clocksource/Makefile         |  1 +
 drivers/clocksource/armv7m_systick.c | 78 ++++++++++++++++++++++++++++++++++++
 3 files changed, 86 insertions(+)
 create mode 100644 drivers/clocksource/armv7m_systick.c

diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index fc01ec2..fb6011e 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig

@@ -124,6 +124,13 @@ config CLKSRC_ARM_GLOBAL_TIMER_SCHED_CLOCK
 	help
 	 Use ARM global timer clock source as sched_clock
 
+config ARMV7M_SYSTICK
+	bool

I assume this symbol is enabled later in the series.

Yes, I noticed it was selected in 14/18 ("ARM: Add STM32 family
machine").

Would it make sense
to allow enabing the symbol for compile test coverage?

quoted

+	select CLKSRC_OF if OF

What happens if ARMV7M_SYSTICK=y but OF=n? Doesn't the driver fail to
compile?

quoted

+	select CLKSRC_MMIO
+	help
+	  This options enables support for the ARMv7M system timer unit

the right spelling is ARMv7-M.

This Kconfig entry has no prompt, so no one is going to see this text
during make *config. Perhaps this should be made a comment. In that case
the right spelling should still be used.

Thanks,


Paul Bolle

Hi Paul, Uwe,

2015-02-20 22:48 GMT+01:00 Paul Bolle [off-list ref]:

On Fri, 2015-02-20 at 20:54 +0100, Uwe Kleine-König wrote:

quoted

On Fri, Feb 20, 2015 at 07:01:03PM +0100, Maxime Coquelin wrote:

quoted

This patch adds clocksource support for ARMv7-M's System timer,
also known as SysTick.

Signed-off-by: Maxime Coquelin <mcoquelin.stm32@gmail.com>
---
 drivers/clocksource/Kconfig          |  7 ++++
 drivers/clocksource/Makefile         |  1 +
 drivers/clocksource/armv7m_systick.c | 78 ++++++++++++++++++++++++++++++++++++
 3 files changed, 86 insertions(+)
 create mode 100644 drivers/clocksource/armv7m_systick.c

diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index fc01ec2..fb6011e 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig

@@ -124,6 +124,13 @@ config CLKSRC_ARM_GLOBAL_TIMER_SCHED_CLOCK
    help
     Use ARM global timer clock source as sched_clock

+config ARMV7M_SYSTICK
+   bool

I assume this symbol is enabled later in the series.

Yes, I noticed it was selected in 14/18 ("ARM: Add STM32 family
machine").

quoted

Would it make sense
to allow enabing the symbol for compile test coverage?

quoted

+   select CLKSRC_OF if OF

What happens if ARMV7M_SYSTICK=y but OF=n? Doesn't the driver fail to
compile?

quoted

+   select CLKSRC_MMIO
+   help
+     This options enables support for the ARMv7M system timer unit

the right spelling is ARMv7-M.

This Kconfig entry has no prompt, so no one is going to see this text
during make *config. Perhaps this should be made a comment. In that case
the right spelling should still be used.

Yes, you are right.
Do you agree if I define it like this:

config ARMV7M_SYSTICK
    bool "Clocksource driver for ARMv7-M System timer"
    depends on OF && (CPU_V7M || COMPILE_TEST)
    select CLKSRC_OF
    select CLKSRC_MMIO
    help
      This options enables clocksource support for the ARMv7-M system
      timer unit.

Thanks,
Maxime

Thanks,


Paul Bolle

Maxime Coquelin schreef op ma 02-03-2015 om 17:53 [+0100]:

Do you agree if I define it like this:

config ARMV7M_SYSTICK
    bool "Clocksource driver for ARMv7-M System timer"
    depends on OF && (CPU_V7M || COMPILE_TEST)
    select CLKSRC_OF
    select CLKSRC_MMIO
    help
      This options enables clocksource support for the ARMv7-M system
      timer unit.

I don't really have strong feelings on whatever way you choose to fix
the, well, minor problem I pointed out.

Having said that, if a Kconfig entry without a prompt (and therefor,
without help) actually does what you want it to do, why bother adding a
prompt and a one line help text?


Paul Bolle

2015-03-03 20:43 GMT+01:00 Paul Bolle [off-list ref]:

Maxime Coquelin schreef op ma 02-03-2015 om 17:53 [+0100]:

quoted

Do you agree if I define it like this:

config ARMV7M_SYSTICK
    bool "Clocksource driver for ARMv7-M System timer"
    depends on OF && (CPU_V7M || COMPILE_TEST)
    select CLKSRC_OF
    select CLKSRC_MMIO
    help
      This options enables clocksource support for the ARMv7-M system
      timer unit.

I don't really have strong feelings on whatever way you choose to fix
the, well, minor problem I pointed out.

Having said that, if a Kconfig entry without a prompt (and therefor,
without help) actually does what you want it to do, why bother adding a
prompt and a one line help text?

This is because I added also support for COMPILE_TEST coverage as per
Uwe advice,
and thought it was necessary to have an entry for this.
Maybe I'm just wrong?

Thanks,
Maxime

Paul Bolle

On Wed, 2015-03-04 at 13:08 +0100, Maxime Coquelin wrote:

This is because I added also support for COMPILE_TEST coverage as per
Uwe advice,
and thought it was necessary to have an entry for this.
Maybe I'm just wrong?

I missed that you added COMPILE_TEST.

A quick scan of your idea doesn't show any obvious issues. (Note that I
don't really know how people actually use COMPILE_TEST. I guess things
like "make allyesconfig" are involved.)


Paul Bolle

Hello,

On Mon, Mar 09, 2015 at 10:12:32PM +0100, Paul Bolle wrote:

On Wed, 2015-03-04 at 13:08 +0100, Maxime Coquelin wrote:

quoted

This is because I added also support for COMPILE_TEST coverage as per
Uwe advice,
and thought it was necessary to have an entry for this.
Maybe I'm just wrong?

I missed that you added COMPILE_TEST.

A quick scan of your idea doesn't show any obvious issues. (Note that I
don't really know how people actually use COMPILE_TEST. I guess things
like "make allyesconfig" are involved.)

Maybe this can clearify the purpose of COMPILE_TEST:

diff --git a/init/Kconfig b/init/Kconfig
--- a/init/Kconfig
+++ b/init/Kconfig

@@ -67,6 +67,26 @@ config COMPILE_TEST
 	  here. If you are a user/distributor, say N here to exclude useless
 	  drivers to be distributed.
 
+	# If you are a driver author consider to adjust your driver's
+	# dependencies to make it buildable with minimal preconditions if
+	# COMPILE_TEST is enabled. This helps contributers and maintainers
+	# that might not have the necessary toolchain or kernel config handy and
+	# also increases compile test coverage. It's your advantage if others can
+	# build your driver more easily! So for a device that is only found on the
+	# foo cpu use:
+	#
+	# 	depends on CPU_FOO || COMPILE_TEST
+	#
+	# . You might have to use
+	#
+	# 	depends on CPU_FOO || (COMPILE_TEST && COOKIE)
+	#
+	# or
+	#
+	#	depends on COOKIE && (CPU_FOO || COMPILE_TEST)
+	#
+	# if your driver uses features that are only available if COOKIE is on.
+
 config LOCALVERSION
 	string "Local version - append to kernel release"
 	help

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |