linux-kernel - Re: [PATCH v3 2/2] clocksource: driver for Conexant Digicolor SoC timer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20150125144656.GO2555@sapphire.tkos.co.il>
Date:	Sun, 25 Jan 2015 16:46:56 +0200
From:	Baruch Siach <baruch@...s.co.il>
To:	Daniel Lezcano <daniel.lezcano@...aro.org>
Cc:	Thomas Gleixner <tglx@...utronix.de>, linux-kernel@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v3 2/2] clocksource: driver for Conexant Digicolor SoC
 timer

Hi Daniel,

On Thu, Jan 22, 2015 at 01:54:39PM +0100, Daniel Lezcano wrote:
> On 01/21/2015 07:36 AM, Baruch Siach wrote:
> >+ * Based on:
> >+ *	Allwinner SoCs hstimer driver
> 
> If this is based on the Allwinnner driver, may be you can have a look at the
> patchset sent by Maxim to make sure your implementation is complete ?
> 
> https://lkml.org/lkml/2015/1/11/52

Thanks for the tip. I'll take the applicable parts from that series.

[...]

> >+static void __iomem *timer_base;
> >+static u32 ticks_per_jiffy;
> 
> Mind to encapsulate those global variables in a structure and use
> container_of to access those fields like:
> 
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/clocksource/mtk_timer.c#n56
> 
> ?

The trouble is that the sched_clock callback (digicolor_timer_sched_read) 
needs a static timer_base to find the timer counter. Do you think adding a 
structure is worth it only for ticks_per_jiffy?

> >+static void digicolor_clkevt_mode(enum clock_event_mode mode,
> >+				  struct clock_event_device *clk)
> >+{
> >+	switch (mode) {
> >+	case CLOCK_EVT_MODE_PERIODIC:
> >+		writeb(0, timer_base + CONTROL(TIMER_C));
> 
> Even if that sounds overkill, please replace '0', '1' with whatever macro
> name (eg. TIMER_DISABLE/ENABLE).

Will do for the entire file.

[...]

> >+static struct irqaction digicolor_timer_irq = {
> >+	.name = "digicolor_timerC",
> >+	.flags = IRQF_TIMER | IRQF_IRQPOLL,
> >+	.handler = digicolor_timer_interrupt,
> >+	.dev_id = &digicolor_clockevent,
> >+};
> 
> The current trend is to use 'request_irq', so no such structure
> declaration/initialization is needed.

Thanks. Will do.

> >+	writel(~0, timer_base + COUNT(TIMER_B));
> 
> s/~0/UINT_MAX/ ?

Ack.

Thanks for reviewing. I'll post an updated series shortly.

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@...s.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/