diff mbox

[v14,1/9] clocksource/drivers/arm_arch_timer: Move enums and defines to header file

Message ID 1475086637-1914-2-git-send-email-fu.wei@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

fu.wei@linaro.org Sept. 28, 2016, 6:17 p.m. UTC 
From: Fu Wei <fu.wei@linaro.org>

To support the arm_arch_timer via ACPI we need to share defines and enums
between the driver and the ACPI parser code.

Split out the relevant defines and enums into arm_arch_timer.h.
No functional change.

Signed-off-by: Fu Wei <fu.wei@linaro.org>
---
 drivers/clocksource/arm_arch_timer.c | 11 -----------
 include/clocksource/arm_arch_timer.h | 11 +++++++++++
 2 files changed, 11 insertions(+), 11 deletions(-)

Comments

Mark Rutland Oct. 20, 2016, 2:45 p.m. UTC | #1 
Hi,

On Thu, Sep 29, 2016 at 02:17:09AM +0800, fu.wei@linaro.org wrote:
> diff --git a/include/clocksource/arm_arch_timer.h b/include/clocksource/arm_arch_timer.h
> index caedb74..6f06481 100644
> --- a/include/clocksource/arm_arch_timer.h
> +++ b/include/clocksource/arm_arch_timer.h
> @@ -19,6 +19,9 @@

Please add:

#include <linux/bitops.h>

... immediately before the includes below; it's needed to ensure that
BIT() is defined in all cases. Previously we were relying on implicit
header includes, which is not good practice.

>  #include <linux/timecounter.h>
>  #include <linux/types.h>
>  
> +#define ARCH_CP15_TIMER			BIT(0)
> +#define ARCH_MEM_TIMER			BIT(1)

If we're going to expose these in a header, it would be better to rename
them to something that makes their usage/meaning clear. These should
probably be ARCH_TIMER_TYPE_{CP15,MEM}.

I guess this can wait for subsequent cleanup.

> +enum ppi_nr {
> +	PHYS_SECURE_PPI,
> +	PHYS_NONSECURE_PPI,
> +	VIRT_PPI,
> +	HYP_PPI,
> +	MAX_TIMER_PPI
> +};

Please rename this to arch_timer_ppi_nr (updating the single user in 
drivers/clocksource/arm_arch_timer.c). That'll avoid the potential for
name clashes in files this happens to get included in (potentially
transitively via other headers).

With those changes (regardless of the ARCH_TIMER_TYPE_* bits):

Acked-by: Mark Rutland <mark.rutland@arm.com>

Thanks,
Mark.

> +
>  #define ARCH_TIMER_PHYS_ACCESS		0
>  #define ARCH_TIMER_VIRT_ACCESS		1
>  #define ARCH_TIMER_MEM_PHYS_ACCESS	2
> -- 
> 2.7.4
>
fu.wei@linaro.org Oct. 26, 2016, 8:31 a.m. UTC | #2 
Hi Mark

On 20 October 2016 at 22:45, Mark Rutland <mark.rutland@arm.com> wrote:
> Hi,
>
> On Thu, Sep 29, 2016 at 02:17:09AM +0800, fu.wei@linaro.org wrote:
>> diff --git a/include/clocksource/arm_arch_timer.h b/include/clocksource/arm_arch_timer.h
>> index caedb74..6f06481 100644
>> --- a/include/clocksource/arm_arch_timer.h
>> +++ b/include/clocksource/arm_arch_timer.h
>> @@ -19,6 +19,9 @@
>
> Please add:
>
> #include <linux/bitops.h>
>
> ... immediately before the includes below; it's needed to ensure that
> BIT() is defined in all cases. Previously we were relying on implicit
> header includes, which is not good practice.

yes, you are right!
added this, thanks !

>
>>  #include <linux/timecounter.h>
>>  #include <linux/types.h>
>>
>> +#define ARCH_CP15_TIMER                      BIT(0)
>> +#define ARCH_MEM_TIMER                       BIT(1)
>
> If we're going to expose these in a header, it would be better to rename
> them to something that makes their usage/meaning clear. These should
> probably be ARCH_TIMER_TYPE_{CP15,MEM}.
>
> I guess this can wait for subsequent cleanup.
>
>> +enum ppi_nr {
>> +     PHYS_SECURE_PPI,
>> +     PHYS_NONSECURE_PPI,
>> +     VIRT_PPI,
>> +     HYP_PPI,
>> +     MAX_TIMER_PPI
>> +};
>
> Please rename this to arch_timer_ppi_nr (updating the single user in
> drivers/clocksource/arm_arch_timer.c). That'll avoid the potential for
> name clashes in files this happens to get included in (potentially
> transitively via other headers).

OK, NP, I have fixed this.

>
> With those changes (regardless of the ARCH_TIMER_TYPE_* bits):
>
> Acked-by: Mark Rutland <mark.rutland@arm.com>

For ARCH_TIMER_TYPE_*, maybe I should add a patch at the end of this
patchset to fix it, OK ?

Thanks for ACK :-)

>
> Thanks,
> Mark.
>
>> +
>>  #define ARCH_TIMER_PHYS_ACCESS               0
>>  #define ARCH_TIMER_VIRT_ACCESS               1
>>  #define ARCH_TIMER_MEM_PHYS_ACCESS   2
>> --
>> 2.7.4
>>
Mark Rutland Oct. 26, 2016, 10:51 a.m. UTC | #3 
On Wed, Oct 26, 2016 at 04:31:55PM +0800, Fu Wei wrote:
> On 20 October 2016 at 22:45, Mark Rutland <mark.rutland@arm.com> wrote:
> > On Thu, Sep 29, 2016 at 02:17:09AM +0800, fu.wei@linaro.org wrote:
> >>  #include <linux/timecounter.h>
> >>  #include <linux/types.h>
> >>
> >> +#define ARCH_CP15_TIMER                      BIT(0)
> >> +#define ARCH_MEM_TIMER                       BIT(1)
> >
> > If we're going to expose these in a header, it would be better to rename
> > them to something that makes their usage/meaning clear. These should
> > probably be ARCH_TIMER_TYPE_{CP15,MEM}.

> > With those changes (regardless of the ARCH_TIMER_TYPE_* bits):
> >
> > Acked-by: Mark Rutland <mark.rutland@arm.com>
> 
> For ARCH_TIMER_TYPE_*, maybe I should add a patch at the end of this
> patchset to fix it, OK ?

Sure. If you could put that *earlier* in the patchset it would be
preferable so as to minimize churn.

Thanks,
Mark.
fu.wei@linaro.org Oct. 26, 2016, 10:54 a.m. UTC | #4 
Hi Mark

On 26 October 2016 at 18:51, Mark Rutland <mark.rutland@arm.com> wrote:
> On Wed, Oct 26, 2016 at 04:31:55PM +0800, Fu Wei wrote:
>> On 20 October 2016 at 22:45, Mark Rutland <mark.rutland@arm.com> wrote:
>> > On Thu, Sep 29, 2016 at 02:17:09AM +0800, fu.wei@linaro.org wrote:
>> >>  #include <linux/timecounter.h>
>> >>  #include <linux/types.h>
>> >>
>> >> +#define ARCH_CP15_TIMER                      BIT(0)
>> >> +#define ARCH_MEM_TIMER                       BIT(1)
>> >
>> > If we're going to expose these in a header, it would be better to rename
>> > them to something that makes their usage/meaning clear. These should
>> > probably be ARCH_TIMER_TYPE_{CP15,MEM}.
>
>> > With those changes (regardless of the ARCH_TIMER_TYPE_* bits):
>> >
>> > Acked-by: Mark Rutland <mark.rutland@arm.com>
>>
>> For ARCH_TIMER_TYPE_*, maybe I should add a patch at the end of this
>> patchset to fix it, OK ?
>
> Sure. If you could put that *earlier* in the patchset it would be
> preferable so as to minimize churn.

OK, NP, will do

>
> Thanks,
> Mark.
diff mbox

Patch

diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 5770054..aea6c10 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -51,8 +51,6 @@ 
 #define CNTV_TVAL	0x38
 #define CNTV_CTL	0x3c
 
-#define ARCH_CP15_TIMER	BIT(0)
-#define ARCH_MEM_TIMER	BIT(1)
 static unsigned arch_timers_present __initdata;
 
 static void __iomem *arch_counter_base;
@@ -65,15 +63,6 @@  struct arch_timer {
 #define to_arch_timer(e) container_of(e, struct arch_timer, evt)
 
 static u32 arch_timer_rate;
-
-enum ppi_nr {
-	PHYS_SECURE_PPI,
-	PHYS_NONSECURE_PPI,
-	VIRT_PPI,
-	HYP_PPI,
-	MAX_TIMER_PPI
-};
-
 static int arch_timer_ppi[MAX_TIMER_PPI];
 
 static struct clock_event_device __percpu *arch_timer_evt;
diff --git a/include/clocksource/arm_arch_timer.h b/include/clocksource/arm_arch_timer.h
index caedb74..6f06481 100644
--- a/include/clocksource/arm_arch_timer.h
+++ b/include/clocksource/arm_arch_timer.h
@@ -19,6 +19,9 @@ 
 #include <linux/timecounter.h>
 #include <linux/types.h>
 
+#define ARCH_CP15_TIMER			BIT(0)
+#define ARCH_MEM_TIMER			BIT(1)
+
 #define ARCH_TIMER_CTRL_ENABLE		(1 << 0)
 #define ARCH_TIMER_CTRL_IT_MASK		(1 << 1)
 #define ARCH_TIMER_CTRL_IT_STAT		(1 << 2)
@@ -34,6 +37,14 @@  enum arch_timer_reg {
 	ARCH_TIMER_REG_TVAL,
 };
 
+enum ppi_nr {
+	PHYS_SECURE_PPI,
+	PHYS_NONSECURE_PPI,
+	VIRT_PPI,
+	HYP_PPI,
+	MAX_TIMER_PPI
+};
+
 #define ARCH_TIMER_PHYS_ACCESS		0
 #define ARCH_TIMER_VIRT_ACCESS		1
 #define ARCH_TIMER_MEM_PHYS_ACCESS	2