| Message ID | 1373062725-19625-9-git-send-email-maxime.ripard@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Maxime, On Sat, 6 Jul 2013, Maxime Ripard wrote: > @@ -168,8 +166,7 @@ static void __init sun4i_timer_init(struct device_node *node) > clocksource_mmio_init(timer_base + TIMER_CNTVAL_REG(1), node->name, > rate, 300, 32, clocksource_mmio_readl_down); > > - writel(rate / (TIMER_SCAL * HZ), > - timer_base + TIMER_INTVAL_REG(0)); > + writel(rate / HZ, timer_base + TIMER_INTVAL_REG(0)); > > /* set clock source to HOSC, 16 pre-division */ > val = readl(timer_base + TIMER_CTL_REG(0)); > @@ -192,8 +189,8 @@ static void __init sun4i_timer_init(struct device_node *node) > > sun4i_clockevent.cpumask = cpumask_of(0); > > - clockevents_config_and_register(&sun4i_clockevent, rate / TIMER_SCAL, > - 0x1, 0xff); > + clockevents_config_and_register(&sun4i_clockevent, rate, 0x1, > + 0xffffffff); I really recommend that you go out for lots of beer/wine NOW and resume reading this mail when you recovered from that. I definitely appreciate your responsivness to feedback, but please go back and read my reply to the previous version of this patch carefully. You might eventually find out that I pointed you to another redundant clk_get_rate() call in that code. After you did this, please go through the other patches in that series and check how many new instances of clk_get_rate() calls you add down the road. I did not even bother to look whether you cleaned it up between v3 and v4, but I'm quite sure you did not. If I'm wrong, I owe you a beer at the next conference. Please take your time to address all concerns and look over the whole thing carefullly before resending. This is not a speed coding contest! Taking time and reconsidering whether a comment for patch N/M might apply to other parts of the code or other parts of the patch series is not optional. Review comments are mostly hints. So it's up to you to check whether such a comment might apply to more than the particular patch line which was commented. Taking time and being careful actually spares time on both and aside of that it spares a lot of pointless wasted electrons sent through the intertubes. Have a good weekend! tglx
On Sat, Jul 06, 2013 at 01:17:41AM +0200, Thomas Gleixner wrote: > I really recommend that you go out for lots of beer/wine NOW and > resume reading this mail when you recovered from that. > > I definitely appreciate your responsivness to feedback, but please go > back and read my reply to the previous version of this patch > carefully. You might eventually find out that I pointed you to another > redundant clk_get_rate() call in that code. > > After you did this, please go through the other patches in that series > and check how many new instances of clk_get_rate() calls you add down > the road. I did not even bother to look whether you cleaned it up > between v3 and v4, but I'm quite sure you did not. If I'm wrong, I owe > you a beer at the next conference. Wow, you really want me to drink, do you? :) Actually, I did clean up. The other user you spotted that was previously introduced in the patch 4/10, and if you take a look at it, you'll see that it actually uses the rate variable like you suggested. Now, your mail made me realize that patch 10 introduced a direct clk_get_rate call, that I forgot to cleanup. After applying these patches, it's the only user left. I'll send a v5. Do you have any additionnal comments on those patches to avoid wasting more electrons? Maxime
On 06-07-13 10:10, Maxime Ripard wrote: > > I'll send a v5. Do you have any additionnal comments on those patches to > avoid wasting more electrons? Pff, we all know Maxime's electrons are free > > Maxime >
On Saturday 06 July 2013 11:16:51 Oliver Schinagl wrote: > On 06-07-13 10:10, Maxime Ripard wrote: > > > > I'll send a v5. Do you have any additionnal comments on those patches to > > avoid wasting more electrons? > > Pff, we all know Maxime's electrons are free Only free as in Drude-Sommerfeld, not free as in beer. Arnd
On Sat, 6 Jul 2013, Maxime Ripard wrote: > On Sat, Jul 06, 2013 at 01:17:41AM +0200, Thomas Gleixner wrote: > > I really recommend that you go out for lots of beer/wine NOW and > > resume reading this mail when you recovered from that. > > > > I definitely appreciate your responsivness to feedback, but please go > > back and read my reply to the previous version of this patch > > carefully. You might eventually find out that I pointed you to another > > redundant clk_get_rate() call in that code. > > > > After you did this, please go through the other patches in that series > > and check how many new instances of clk_get_rate() calls you add down > > the road. I did not even bother to look whether you cleaned it up > > between v3 and v4, but I'm quite sure you did not. If I'm wrong, I owe > > you a beer at the next conference. > > Wow, you really want me to drink, do you? :) Taking a break really helps :) > Actually, I did clean up. The other user you spotted that was previously > introduced in the patch 4/10, and if you take a look at it, you'll see > that it actually uses the rate variable like you suggested. Ah, missed that :) > Now, your mail made me realize that patch 10 introduced a direct > clk_get_rate call, that I forgot to cleanup. After applying these > patches, it's the only user left. So no beer for you! > I'll send a v5. Do you have any additionnal comments on those patches to > avoid wasting more electrons? No, I just wanted to make you aware that sending patches faster than I can review them is not really helpful, unless the fast new version is perfect. Thanks, tglx
diff --git a/drivers/clocksource/sun4i_timer.c b/drivers/clocksource/sun4i_timer.c index dd78b63..3217adc 100644 --- a/drivers/clocksource/sun4i_timer.c +++ b/drivers/clocksource/sun4i_timer.c @@ -34,8 +34,6 @@ #define TIMER_INTVAL_REG(val) (0x10 * (val) + 0x14) #define TIMER_CNTVAL_REG(val) (0x10 * (val) + 0x18) -#define TIMER_SCAL 16 - static void __iomem *timer_base; /* @@ -168,8 +166,7 @@ static void __init sun4i_timer_init(struct device_node *node) clocksource_mmio_init(timer_base + TIMER_CNTVAL_REG(1), node->name, rate, 300, 32, clocksource_mmio_readl_down); - writel(rate / (TIMER_SCAL * HZ), - timer_base + TIMER_INTVAL_REG(0)); + writel(rate / HZ, timer_base + TIMER_INTVAL_REG(0)); /* set clock source to HOSC, 16 pre-division */ val = readl(timer_base + TIMER_CTL_REG(0)); @@ -192,8 +189,8 @@ static void __init sun4i_timer_init(struct device_node *node) sun4i_clockevent.cpumask = cpumask_of(0); - clockevents_config_and_register(&sun4i_clockevent, rate / TIMER_SCAL, - 0x1, 0xff); + clockevents_config_and_register(&sun4i_clockevent, rate, 0x1, + 0xffffffff); } CLOCKSOURCE_OF_DECLARE(sun4i, "allwinner,sun4i-timer", sun4i_timer_init);
The prescaler is only used when using the internal low frequency oscillator (at 32kHz). Since we're using the higher frequency oscillator at 24MHz, we can just remove it. Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> --- drivers/clocksource/sun4i_timer.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)