+
+#define PIT_IRQ_SHIFT 12
+#define PIT_PRIO_SHIFT 20
+#define PIT_ENABLE_SHIFT 26
+#define PIT_IRQ_MASK 0x3f
+#define PIT_PRIO_MASK 0xf

Can you please align the numbers as nicely as you did below and please use
TABS not spaces to seperate them.

+#define REG_PITEN 0x00
+#define REG_THROT 0x10
+#define REG_COUNT 0x14
+#define REG_BUSPD 0x18
+#define REG_SECHI 0x20
+#define REG_SECLO 0x24
+#define REG_NSEC  0x28
+
+struct jcore_clocksource {
+	struct clocksource cs;
+	__iomem void *base;
+};
+
+struct jcore_pit {
+	struct clock_event_device ced;
+	__iomem void *base;
+	unsigned long periodic_delta;
+	unsigned cpu;
+	u32 enable_val;
+};

Again. Please align the struct members as I asked in the other mail.

+
+struct jcore_pit_nb {
+	struct notifier_block nb;
+	struct jcore_pit __percpu *pit_percpu;
+};
+
+static struct clocksource *jcore_cs;
+
+static cycle_t jcore_clocksource_read(struct clocksource *cs)
+{
+	__iomem void *base =
+		container_of(cs, struct jcore_clocksource, cs)->base;
+	u32 sechi, seclo, nsec, sechi0, seclo0;

It would be way simpler to read if you avoid that line break by doing:

	u32 sechi, seclo, nsec, sechi0, seclo0;
	__iomem void *base;
   
	base = container_of(cs, struct jcore_clocksource, cs)->base; 

Hmm?

+
+	sechi = __raw_readl(base + REG_SECHI);
+	seclo = __raw_readl(base + REG_SECLO);
+	do {
+		sechi0 = sechi;
+		seclo0 = seclo;
+		nsec  = __raw_readl(base + REG_NSEC);
+		sechi = __raw_readl(base + REG_SECHI);
+		seclo = __raw_readl(base + REG_SECLO);
+	} while (sechi0 != sechi || seclo0 != seclo);
+
+	return ((u64)sechi << 32 | seclo) * NSEC_PER_SEC + nsec;

Wow, that's an expensive thing for a hotpath operation. You really don't have
binary readout register for that clock thingy?

+}
+
+static notrace u64 jcore_sched_clock_read(void)
+{
+	return jcore_clocksource_read(jcore_cs);

Why don't you stuff the above code into this function?

+}
+
+static int jcore_pit_disable(struct jcore_pit *pit)
+{
+	__raw_writel(0, pit->base + REG_PITEN);
+	return 0;
+}
+
+static int jcore_pit_set(unsigned long delta, struct jcore_pit *pit)
+{
+	jcore_pit_disable(pit);
+	__raw_writel(delta, pit->base + REG_THROT);
+	__raw_writel(pit->enable_val, pit->base + REG_PITEN);
+	return 0;
+}
+
+static int jcore_pit_set_state_shutdown(struct clock_event_device *ced)
+{
+	struct jcore_pit *pit = container_of(ced, struct jcore_pit, ced);

Newline between declaration and code please....

+	return jcore_pit_disable(pit);
+}

+static int jcore_pit_cpu_notify(struct notifier_block *self,
+			unsigned long action, void *hcpu)
+{
+	struct jcore_pit_nb *nb = container_of(self, struct jcore_pit_nb, nb);
+	switch (action & ~CPU_TASKS_FROZEN) {
+	case CPU_STARTING:
+		jcore_pit_local_init(this_cpu_ptr(nb->pit_percpu));
+		break;
+	}
+	return NOTIFY_OK;

Please convert this to the new state machine model of cpu
hotplug. CPU_STARTING will be gone very soon. Here is an example:

http://git.kernel.org/cgit/linux/kernel/git/tip/tip.git/commit/?h=smp/hotplug&id=7e86e8bd8dd67649d176e08d8dfb90039f0a1e98

+static int __init jcore_pit_init(struct device_node *node)
+{
+	int err;
+	__iomem void *pit_base;
+	unsigned pit_irq;
+	unsigned cpu;

Please put the same data types into a single line

+	struct jcore_pit_nb *nb = 0;

  = NULL;

Please

+	struct jcore_clocksource *cs = 0;
+	struct jcore_pit __percpu *pit_percpu = 0;
+
+	err = request_irq(pit_irq, jcore_timer_interrupt,
+		IRQF_TIMER | IRQF_PERCPU, "jcore_pit", pit_percpu);

If you need line breaks, then please keep the arguments aligned:

	err = request_irq(pit_irq, jcore_timer_interrupt,
			  IRQF_TIMER | IRQF_PERCPU, "jcore_pit", pit_percpu);

+	/* The J-Core PIT is not hard-wired to a particular IRQ, but
+	 * integrated with the interrupt controller such that the IRQ it
+	 * generates is programmable. The programming interface has a
+	 * legacy field which was an interrupt priority for AIC1, but
+	 * which is OR'd onto bits 2-5 of the generated IRQ number when
+	 * used with J-Core AIC2, so set it to match these bits. */

Proper multiline comments please

+	return 0;
+
+out:
+	pr_err("Could not initialize J-Core PIT driver\n");

So you have a pr_err in every error path and that goto just to pr_err the
obvious? Well, yes ...

Thanks,

	tglx
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html