Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1504949 > unrolled thread
| Started by | Mark Rutland <mark.rutland@arm.com> |
|---|---|
| First post | 2016-10-20 16:50 +0200 |
| Last post | 2016-10-26 13:00 +0200 |
| Articles | 4 — 2 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH v14 1/9] clocksource/drivers/arm_arch_timer: Move enums and defines to header file Mark Rutland <mark.rutland@arm.com> - 2016-10-20 16:50 +0200
Re: [PATCH v14 1/9] clocksource/drivers/arm_arch_timer: Move enums and defines to header file Fu Wei <fu.wei@linaro.org> - 2016-10-26 10:40 +0200
Re: [PATCH v14 1/9] clocksource/drivers/arm_arch_timer: Move enums and defines to header file Mark Rutland <mark.rutland@arm.com> - 2016-10-26 13:00 +0200
Re: [PATCH v14 1/9] clocksource/drivers/arm_arch_timer: Move enums and defines to header file Fu Wei <fu.wei@linaro.org> - 2016-10-26 13:00 +0200
| From | Mark Rutland <mark.rutland@arm.com> |
|---|---|
| Date | 2016-10-20 16:50 +0200 |
| Subject | Re: [PATCH v14 1/9] clocksource/drivers/arm_arch_timer: Move enums and defines to header file |
| Message-ID | <sumPn-t0-7@gated-at.bofh.it> |
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
>
[toc] | [next] | [standalone]
| From | Fu Wei <fu.wei@linaro.org> |
|---|---|
| Date | 2016-10-26 10:40 +0200 |
| Message-ID | <swrUC-18W-19@gated-at.bofh.it> |
| In reply to | #1504949 |
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
>>
--
Best regards,
Fu Wei
Software Engineer
Red Hat
[toc] | [prev] | [next] | [standalone]
| From | Mark Rutland <mark.rutland@arm.com> |
|---|---|
| Date | 2016-10-26 13:00 +0200 |
| Message-ID | <swu65-2w2-1@gated-at.bofh.it> |
| In reply to | #1508929 |
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.
[toc] | [prev] | [next] | [standalone]
| From | Fu Wei <fu.wei@linaro.org> |
|---|---|
| Date | 2016-10-26 13:00 +0200 |
| Message-ID | <swu65-2w2-15@gated-at.bofh.it> |
| In reply to | #1509049 |
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.
--
Best regards,
Fu Wei
Software Engineer
Red Hat
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web