On Wednesday 10 November 2010, Jimmy Rubin wrote:

This patch adds support for MCDE, Memory-to-display controller
found in the ST-Ericsson ux500 products.

Hi Jimmy,

I haven't looked at what this device does, but I've tried to do
a review based on coding style and common practices. I hope this
is useful to you.

This patch adds the hardware abstraction layer.
All calls to the hardware is handled in mcde_hw.c

A "hardware abstraction layer" is generally considered a bad thing,
you're usually better off not advertising your code as being one.

As a rule, the device driver *is* the hardware abstraction, so you
should not add another one ;-)

+static void disable_channel(struct mcde_chnl_state *chnl);
+static void enable_channel(struct mcde_chnl_state *chnl);
+static void watchdog_auto_sync_timer_function(unsigned long arg);

I generally recomment avoiding forward declarations of static functions.
Just reorder the code so you don't need them.

+u8 *mcdeio;
+u8 **dsiio;
+DEFINE_SPINLOCK(mcde_lock); /* REVIEW: Remove or use */
+struct platform_device *mcde_dev;
+u8 num_dsilinks;

You should try hard to avoid global variables in a well-designed driver.
There are many ways around them, like accessor functions or splitting the
driver into files in a more logical way where each file only accesses
its own data. If you really cannot think of a way to avoid these,
put them in a proper name space in the way that you have done for the
global functions, by prefixing each identifier with "mcde_".

+static u8 hardware_version;
+
+static struct regulator *regulator;
+static struct clk *clock_dsi;
+static struct clk *clock_mcde;
+static struct clk *clock_dsi_lp;
+static u8 mcde_is_enabled;

Even static variables like these can cause problems. Ideally all of these
are referenced through a driver private data structure that is passed around
with the device. This way you can trivially support multiple devices if 
that ever becomes necessary.

+static inline u32 dsi_rreg(int i, u32 reg)
+{
+	return readl(dsiio[i] + reg);
+}
+static inline void dsi_wreg(int i, u32 reg, u32 val)
+{
+	writel(val, dsiio[i] + reg);
+}

dsiio is not marked __iomem, so there is something wrong here.
Moreover, why do you need two indexes? If you have multiple identical
"dsiio" structures, maybe each of them should just be a device by itself?

+struct mcde_ovly_state {
+	bool inuse;
+	u8 idx; /* MCDE overlay index */
+	struct mcde_chnl_state *chnl; /* Owner channel */
+	u32 transactionid; /* Apply time stamp */
+	u32 transactionid_regs; /* Register update time stamp */
+	u32 transactionid_hw; /* HW completed time stamp */
+	wait_queue_head_t waitq_hw; /* Waitq for transactionid_hw */
+
+	/* Staged settings */
+	u32 paddr;
+	u16 stride;
+	enum mcde_ovly_pix_fmt pix_fmt;
+
+	u16 src_x;
+	u16 src_y;
+	u16 dst_x;
+	u16 dst_y;
+	u16 dst_z;
+	u16 w;
+	u16 h;
+
+	/* Applied settings */
+	struct ovly_regs regs;
+};

There should probably be a "struct device" pointer in this, so you can pass
it around as a real object.

+	/* Handle channel irqs */
+	irq_status = mcde_rreg(MCDE_RISPP);
+	if (irq_status & MCDE_RISPP_VCMPARIS_MASK) {
+		chnl = &channels[MCDE_CHNL_A];
...
+	}
+	if (irq_status & MCDE_RISPP_VCMPBRIS_MASK) {
+		chnl = &channels[MCDE_CHNL_B];
...
+	}
+	if (irq_status & MCDE_RISPP_VCMPC0RIS_MASK) {
+		chnl = &channels[MCDE_CHNL_C0];
...
+	}

This looks a bit like you actually have multiple interrupt lines multiplexed
through a private interrupt controller. Have you considered making this controller
a separate device to multiplex the interrupt numbers?

+void wait_for_overlay(struct mcde_ovly_state *ovly)

Not an appropriate name for a global function. Either make this static or
call it mcde_wait_for_overlay. Same for some other functions.

+#ifdef CONFIG_AV8100_SDTV
+	/* TODO: check if these watermark levels work for HDMI as well. */
+	pixelfetchwtrmrklevel = MCDE_PIXFETCH_SMALL_WTRMRKLVL;
+#else
+	if ((fifo = MCDE_FIFO_A || fifo = MCDE_FIFO_B) &&
+					regs->ppl >= fifo_size * 2)
+		pixelfetchwtrmrklevel = MCDE_PIXFETCH_LARGE_WTRMRKLVL;
+	else
+		pixelfetchwtrmrklevel = MCDE_PIXFETCH_MEDIUM_WTRMRKLVL;
+#endif /* CONFIG_AV8100_SDTV */

Be careful with config options like this. If you want to build a kernel
to run on all machines, the first part probably needs to check where it
is running and consider the other pixelfetchwtrmrklevel values as well.

+/* Channel path */
+#define MCDE_CHNLPATH(__chnl, __fifo, __type, __ifc, __link) \
+	(((__chnl) << 16) | ((__fifo) << 12) | \
+	 ((__type) << 8) | ((__ifc) << 4) | ((__link) << 0))
+enum mcde_chnl_path {
+	/* Channel A */
+	MCDE_CHNLPATH_CHNLA_FIFOA_DPI_0 = MCDE_CHNLPATH(MCDE_CHNL_A,
+		MCDE_FIFO_A, MCDE_PORTTYPE_DPI, 0, 0),
+	MCDE_CHNLPATH_CHNLA_FIFOA_DSI_IFC0_0 = MCDE_CHNLPATH(MCDE_CHNL_A,
+		MCDE_FIFO_A, MCDE_PORTTYPE_DSI, 0, 0),
+	MCDE_CHNLPATH_CHNLA_FIFOA_DSI_IFC0_1 = MCDE_CHNLPATH(MCDE_CHNL_A,
+		MCDE_FIFO_A, MCDE_PORTTYPE_DSI, 0, 1),
+	MCDE_CHNLPATH_CHNLA_FIFOC0_DSI_IFC0_2 = MCDE_CHNLPATH(MCDE_CHNL_A,
+		MCDE_FIFO_C0, MCDE_PORTTYPE_DSI, 0, 2),
+	MCDE_CHNLPATH_CHNLA_FIFOC0_DSI_IFC1_0 = MCDE_CHNLPATH(MCDE_CHNL_A,
+		MCDE_FIFO_C0, MCDE_PORTTYPE_DSI, 1, 0),
+	MCDE_CHNLPATH_CHNLA_FIFOC0_DSI_IFC1_1 = MCDE_CHNLPATH(MCDE_CHNL_A,
+		MCDE_FIFO_C0, MCDE_PORTTYPE_DSI, 1, 1),
+	MCDE_CHNLPATH_CHNLA_FIFOA_DSI_IFC1_2 = MCDE_CHNLPATH(MCDE_CHNL_A,
+		MCDE_FIFO_A, MCDE_PORTTYPE_DSI, 1, 2),

A table like this would become more readable by making each entry a single line,
even if that goes beyond the 80-character limit.

	Arnd