Hi Joe,

Thanks for your input.
See comments below.
 

Just trivia:

quoted

diff --git a/drivers/video/mcde/mcde_hw.c

b/drivers/video/mcde/mcde_hw.c

[]

quoted

+#define dsi_rfld(__i, __reg, __fld) \
+	((dsi_rreg(__i, __reg) & __reg##_##__fld##_MASK) >> \
+		__reg##_##__fld##_SHIFT)
+#define dsi_wfld(__i, __reg, __fld, __val) \
+	dsi_wreg(__i, __reg, (dsi_rreg(__i, __reg) & \
+	~__reg##_##__fld##_MASK) | (((__val) <<

__reg##_##__fld##_SHIFT) & \

quoted

+		 __reg##_##__fld##_MASK))

These macros are not particularly readable.
Perhaps use statement expression macros like:

#define dsi_rfld(__i, __reg, __fld)
		\
({
			\
	const u32 mask = __reg##_#__fld##_MASK;
		\
	const u32 shift = __reg##_##__fld##_SHIFT;
	\
	((dsi_rreg(__i, __reg) & mask) >> shift;
	\
})

#define dsi_wfld(__i, __reg, __fld, __val)
	\
({
			\
	const u32 mask = __reg##_#__fld##_MASK;
		\
	const u32 shift = __reg##_##__fld##_SHIFT;
	\
	dsi_wreg(__i, __reg,
		\
		 (dsi_rreg(__i, __reg) & ~mask) | (((__val) <<
shift) & mask));\
})

I agree, more readable.

quoted

+static struct mcde_chnl_state channels[] = {

Should more static structs be static const?

I think so, we got some strange behavior when we changed the structs to static const. But we will investigate it.

[]

quoted

+	dev_vdbg(&mcde_dev->dev, "%s\n", __func__);

If your dev_<level> logging messages use "%s", __func__
I suggest you use a set of local macros to preface this.

I don't generally find the function name useful.

Maybe only use the %s __func__ pair when you are also
setting verbose debugging.

Alright, will add some local macros for this.

#ifdef VERBOSE_DEBUG
#define mcde_printk(level, dev, fmt, args) \
	dev_printk(level, dev, "%s: " fmt, __func__, ##args)
#else
#define mcde_printk(level, dev, fmt, args) \
	dev_printk(level, dev, fmt, args)
#endif

#ifdef VERBOSE_DEBUG
#define mcde_vdbg(dev, fmt, args) \
	mcde_printk(KERN_DEBUG, dev, fmt, ##args)
#else
#define mcde_vdbg(dev, fmt, args) \
	do { if (0) mcde_printk(KERN_DEBUG, dev, fmt, ##args); }
while (0)
#endif

#ifdef DEBUG
#define mcde_dbg(dev, fmt, args) \
	mcde_printk(KERN_DEBUG, dev, fmt, ##args)
#else
#define mcde_dbg(dev, fmt, args) \
	do { if (0) mcde_printk(KERN_DEBUG, dev, fmt, ##args); }
while (0)
#endif

#define mcde_ERR(dev, fmt, args) \
	mcde_printk(KERN_ERR, dev, fmt, ##args)
#define mcde_warn(dev, fmt, args) \
	mcde_printk(KERN_WARNING, dev, fmt, ##args)
#define mcde_info(dev, fmt, args) \
	mcde_printk(KERN_INFO, dev, fmt, ##args)

quoted

+static void disable_channel(struct mcde_chnl_state *chnl)
+{
+	int i;
+	const struct mcde_port *port = &chnl->port;
+
+	dev_vdbg(&mcde_dev->dev, "%s\n", __func__);
+
+	if (hardware_version = MCDE_CHIP_VERSION_3_0_8 &&
+				!is_channel_enabled(chnl))

{

quoted

+		chnl->continous_running = false;

It'd be nice to change to continuous_running

Continous_running is normally set to true when a chnl_update is performed.
In disable channel continous_running must be set to false in order to get the hw registers updated in the next chnl_update.

quoted

+int mcde_dsi_dcs_write(struct mcde_chnl_state *chnl, u8 cmd, u8*

data, int len)

quoted

+{
+	int i;
+	u32 wrdat[4] = { 0, 0, 0, 0 };
+	u32 settings;
+	u8 link = chnl->port.link;
+	u8 virt_id = chnl->port.phy.dsi.virt_id;
+
+	/* REVIEW: One command at a time */
+	/* REVIEW: Allow read/write on unreserved ports */
+	if (len > MCDE_MAX_DCS_WRITE || chnl->port.type !> MCDE_PORTTYPE_DSI)
+		return -EINVAL;
+
+	wrdat[0] = cmd;
+	for (i = 1; i <= len; i++)
+		wrdat[i>>2] |= ((u32)data[i-1] << ((i & 3) * 8));

Ever overrun wrdat?
Maybe WARN_ON(len > 16, "oops?")

MCDE_MAX_DCS_WRITE is 15 so it will be an early return in that case.

/Jimmy
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ¥Šwÿº{.nÇ+‰·¥Š{±ıöİzÿâ�Ø^n‡r¡ö¦zË�ëh™¨è­Ú&£ûàz¿äz¹Ş—ú+€Ê+zf£¢·hšˆ§~†­†Ûiÿÿï�êÿ‘êçz_è®æj:+v‰¨ş)ߣøm