| Message ID | 1377744796-22175-1-git-send-email-dinguyen@altera.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 2013-08-28 at 21:53 -0500, Dinh Nguyen wrote: > From: Dinh Nguyen <dinguyen@altera.com> > > Move all dw_apb_timer defines to include/linux/dw_apb_timer.h. > > Signed-off-by: Dinh Nguyen <dinguyen@altera.com> > CC: Rob Herring <rob.herring@calxeda.com> > CC: Arnd Bergmann <arnd@arndb.de> > Cc: Olof Johansson <olof@lixom.net> > CC: Jamie Iles <jamie@jamieiles.com> > Cc: John Stultz <john.stultz@linaro.org> > Cc: Daniel Lezcano <daniel.lezcano@linaro.org> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Linus Walleij <linus.walleij@linaro.org> > Cc: Pavel Machek <pavel@denx.de> > Cc: Heiko Stuebner <heiko@sntech.de> > Cc: linux-arm-kernel@lists.infradead.org > > v2: > - Remove the defines in dw_apb_timer.c > --- > drivers/clocksource/dw_apb_timer.c | 19 ------------------- > include/linux/dw_apb_timer.h | 19 +++++++++++++++++++ > 2 files changed, 19 insertions(+), 19 deletions(-) > > diff --git a/drivers/clocksource/dw_apb_timer.c b/drivers/clocksource/dw_apb_timer.c > index e54ca10..c3a8f52 100644 > --- a/drivers/clocksource/dw_apb_timer.c > +++ b/drivers/clocksource/dw_apb_timer.c > @@ -18,25 +18,6 @@ > #include <linux/io.h> > #include <linux/slab.h> > > -#define APBT_MIN_PERIOD 4 > -#define APBT_MIN_DELTA_USEC 200 > - > -#define APBTMR_N_LOAD_COUNT 0x00 > -#define APBTMR_N_CURRENT_VALUE 0x04 > -#define APBTMR_N_CONTROL 0x08 > -#define APBTMR_N_EOI 0x0c > -#define APBTMR_N_INT_STATUS 0x10 > - > -#define APBTMRS_INT_STATUS 0xa0 > -#define APBTMRS_EOI 0xa4 > -#define APBTMRS_RAW_INT_STATUS 0xa8 > -#define APBTMRS_COMP_VERSION 0xac > - > -#define APBTMR_CONTROL_ENABLE (1 << 0) > -/* 1: periodic, 0:free running. */ > -#define APBTMR_CONTROL_MODE_PERIODIC (1 << 1) > -#define APBTMR_CONTROL_INT (1 << 2) > - > static inline struct dw_apb_clock_event_device * > ced_to_dw_apb_ced(struct clock_event_device *evt) > { > diff --git a/include/linux/dw_apb_timer.h b/include/linux/dw_apb_timer.h > index 1f79b20..1d2b949 100644 > --- a/include/linux/dw_apb_timer.h > +++ b/include/linux/dw_apb_timer.h > @@ -19,6 +19,25 @@ > > #define APBTMRS_REG_SIZE 0x14 > > +#define APBT_MIN_PERIOD 4 > +#define APBT_MIN_DELTA_USEC 200 > + > +#define APBTMR_N_LOAD_COUNT 0x00 > +#define APBTMR_N_CURRENT_VALUE 0x04 > +#define APBTMR_N_CONTROL 0x08 > +#define APBTMR_N_EOI 0x0c > +#define APBTMR_N_INT_STATUS 0x10 > + > +#define APBTMRS_INT_STATUS 0xa0 > +#define APBTMRS_EOI 0xa4 > +#define APBTMRS_RAW_INT_STATUS 0xa8 > +#define APBTMRS_COMP_VERSION 0xac > + > +#define APBTMR_CONTROL_ENABLE (1 << 0) > +/* 1: periodic, 0:free running. */ > +#define APBTMR_CONTROL_MODE_PERIODIC (1 << 1) > +#define APBTMR_CONTROL_INT (1 << 2) > + > struct dw_apb_timer { > void __iomem *base; > unsigned long freq; Ping? This patch series has sat idle for 2-weeks now. Thanks, Dinh
On Tue, 17 Sep 2013, Dinh Nguyen wrote: > On Wed, 2013-08-28 at 21:53 -0500, Dinh Nguyen wrote: > > From: Dinh Nguyen <dinguyen@altera.com> > > > > Move all dw_apb_timer defines to include/linux/dw_apb_timer.h. That's describing WHAT the patch does not WHY. I can see the WHAT part without that pointless changelog. > > Ping? This patch series has sat idle for 2-weeks now. So what? Sending non urgent (i.e. bug fixes, regression fixes etc.) patches at the beginning of the merge window begs for a 2 weeks delay. Well documented procedure... Thanks, tglx
On Tue 2013-09-17 23:45:11, Thomas Gleixner wrote: > On Tue, 17 Sep 2013, Dinh Nguyen wrote: > > > On Wed, 2013-08-28 at 21:53 -0500, Dinh Nguyen wrote: > > > From: Dinh Nguyen <dinguyen@altera.com> > > > > > > Move all dw_apb_timer defines to include/linux/dw_apb_timer.h. > > That's describing WHAT the patch does not WHY. I can see the WHAT part > without that pointless changelog. read_sched_clock() in dw_apb_timer_of.c accesses the same hardware as dw_apb_timer.c, therefore it benefits from #define's that describe the hardware. IOW 1/2 is neccessary for 2/2, but it can compile/work indepedently. Thanks, Pavel
On Tue, 2013-09-17 at 23:45 +0200, Thomas Gleixner wrote: > On Tue, 17 Sep 2013, Dinh Nguyen wrote: > > > On Wed, 2013-08-28 at 21:53 -0500, Dinh Nguyen wrote: > > > From: Dinh Nguyen <dinguyen@altera.com> > > > > > > Move all dw_apb_timer defines to include/linux/dw_apb_timer.h. > > That's describing WHAT the patch does not WHY. I can see the WHAT part > without that pointless changelog. Will fix.. > > > > > Ping? This patch series has sat idle for 2-weeks now. > > So what? > > Sending non urgent (i.e. bug fixes, regression fixes etc.) patches at > the beginning of the merge window begs for a 2 weeks delay. Well > documented procedure... Sorry, but it was just a friendly ping...didn't mean to imply any urgency. The original patch was sent on 8/21, so patch has been around for ~4 weeks.. Thanks for the review. Dinh > > > Thanks, > > tglx >
Pavel, On Tue, 17 Sep 2013, Pavel Machek wrote: > On Tue 2013-09-17 23:45:11, Thomas Gleixner wrote: > > On Tue, 17 Sep 2013, Dinh Nguyen wrote: > > > > > On Wed, 2013-08-28 at 21:53 -0500, Dinh Nguyen wrote: > > > > From: Dinh Nguyen <dinguyen@altera.com> > > > > > > > > Move all dw_apb_timer defines to include/linux/dw_apb_timer.h. > > > > That's describing WHAT the patch does not WHY. I can see the WHAT part > > without that pointless changelog. > > read_sched_clock() in dw_apb_timer_of.c accesses the same hardware as > dw_apb_timer.c, therefore it benefits from #define's that describe the > hardware. IOW 1/2 is neccessary for 2/2, but it can compile/work > indepedently. I'm really capable of figuring that out myself. But that's not the point. I want patch submitters to explain in the changelog WHY they are doing a change not WHAT they are doing, because that's (mostly) obvious from the patch itself. So thanks for your well meant, but completely superfluous explanation. tglx
Hi! > > > > > From: Dinh Nguyen <dinguyen@altera.com> > > > > > > > > > > Move all dw_apb_timer defines to include/linux/dw_apb_timer.h. > > > > > > That's describing WHAT the patch does not WHY. I can see the WHAT part > > > without that pointless changelog. > > > > read_sched_clock() in dw_apb_timer_of.c accesses the same hardware as > > dw_apb_timer.c, therefore it benefits from #define's that describe the > > hardware. IOW 1/2 is neccessary for 2/2, but it can compile/work > > indepedently. > > I'm really capable of figuring that out myself. But that's not the > point. I want patch submitters to explain in the changelog WHY they > are doing a change not WHAT they are doing, because that's (mostly) > obvious from the patch itself. But the end result is that anything takes months to do... We already have the dw_apb_timer issues merged in your tree, then it got dropped in merge conflict, then you dislike the changelog, and now you notice devicetree uglyness that we did not introduce. Yes, dw_apb_timer code is not exactly clean, but if we have to wait month before you lecture us how to write changelogs, we will not get anywhere... discussion why the DT was done that way is forgotten by now. Because just now, we are running in circles (and time does not work on socfpga for second major release). Regards, Pavel
diff --git a/drivers/clocksource/dw_apb_timer.c b/drivers/clocksource/dw_apb_timer.c index e54ca10..c3a8f52 100644 --- a/drivers/clocksource/dw_apb_timer.c +++ b/drivers/clocksource/dw_apb_timer.c @@ -18,25 +18,6 @@ #include <linux/io.h> #include <linux/slab.h> -#define APBT_MIN_PERIOD 4 -#define APBT_MIN_DELTA_USEC 200 - -#define APBTMR_N_LOAD_COUNT 0x00 -#define APBTMR_N_CURRENT_VALUE 0x04 -#define APBTMR_N_CONTROL 0x08 -#define APBTMR_N_EOI 0x0c -#define APBTMR_N_INT_STATUS 0x10 - -#define APBTMRS_INT_STATUS 0xa0 -#define APBTMRS_EOI 0xa4 -#define APBTMRS_RAW_INT_STATUS 0xa8 -#define APBTMRS_COMP_VERSION 0xac - -#define APBTMR_CONTROL_ENABLE (1 << 0) -/* 1: periodic, 0:free running. */ -#define APBTMR_CONTROL_MODE_PERIODIC (1 << 1) -#define APBTMR_CONTROL_INT (1 << 2) - static inline struct dw_apb_clock_event_device * ced_to_dw_apb_ced(struct clock_event_device *evt) { diff --git a/include/linux/dw_apb_timer.h b/include/linux/dw_apb_timer.h index 1f79b20..1d2b949 100644 --- a/include/linux/dw_apb_timer.h +++ b/include/linux/dw_apb_timer.h @@ -19,6 +19,25 @@ #define APBTMRS_REG_SIZE 0x14 +#define APBT_MIN_PERIOD 4 +#define APBT_MIN_DELTA_USEC 200 + +#define APBTMR_N_LOAD_COUNT 0x00 +#define APBTMR_N_CURRENT_VALUE 0x04 +#define APBTMR_N_CONTROL 0x08 +#define APBTMR_N_EOI 0x0c +#define APBTMR_N_INT_STATUS 0x10 + +#define APBTMRS_INT_STATUS 0xa0 +#define APBTMRS_EOI 0xa4 +#define APBTMRS_RAW_INT_STATUS 0xa8 +#define APBTMRS_COMP_VERSION 0xac + +#define APBTMR_CONTROL_ENABLE (1 << 0) +/* 1: periodic, 0:free running. */ +#define APBTMR_CONTROL_MODE_PERIODIC (1 << 1) +#define APBTMR_CONTROL_INT (1 << 2) + struct dw_apb_timer { void __iomem *base; unsigned long freq;