[PATCH 4/5] fb: Add DCU framebuffer driver for Vybrid VF610 platform

On Mon, Aug 05, 2013 at 09:51:40AM +0000, Wang Huan-B18965 wrote:

Hi, Sascha,

quoted

On Fri, Jul 12, 2013 at 02:07:55PM +0800, Alison Wang wrote:

quoted

The Display Controller Unit (DCU) module is a system master that
fetches graphics stored in internal or external memory and displays
them on a TFT LCD panel. A wide range of panel sizes is supported and
the timing of the interface signals is highly configurable.
Graphics are read directly from memory and then blended in real-time,
which allows for dynamic content creation with minimal CPU

intervention.

Only a review of the code inline.

Maybe the real question is whether we want to introduce another
framebuffer driver at all instead of making it a DRM driver.

[Alison Wang] I think DCU module is more suitable to be designed as a framebuffer driver than a DRM driver. Just like DIU framebuffer driver for PowerPC.

quoted

quoted

+
+#define DRIVER_NAME			"fsl-dcu-fb"
+
+#define DCU_DCU_MODE			0x0010
+#define DCU_MODE_BLEND_ITER(x)		(x << 20)

What's the result of DCU_MODE_BLEND_ITER(1 + 1)?

[Alison Wang] Is it really necessary? I don't use this macro like DCU_MODE_BLEND_ITER(1 + 1), just use DCU_MODE_BLEND_ITER(2).

<irony>
No it's not necessary. We can just add incorrect macros everywhere and
fix them as necessary after a long bug squashing session
</irony>

YES! This is necessary.

quoted

quoted

+	return 0;
+}
+
+static int calc_div_ratio(struct fb_info *info) {

Use a consistent namespace for function names (fsl_dcu_)

[Alison Wang] Is it necessary to use a consistent namespace for this generic function? I think it is necessary to use a consistent namespace (fsl_dcu_) for the function names in structure fsl_dcu_ops.

This function calculates some divider out of a struct fb_info. This is
by no means generic.

quoted

quoted

+		if (copy_to_user(buf, &alpha, sizeof(alpha)))
+			return -EFAULT;
+		break;
+	case MFB_SET_ALPHA:
+		if (copy_from_user(&alpha, buf, sizeof(alpha)))
+			return -EFAULT;
+		mfbi->blend = 1;
+		mfbi->alpha = alpha;
+		fsl_dcu_set_par(info);
+		break;

Is it still state of the art to introduce ioctls in the fb framework
without any kind of documentation?

[Alison Wang] Do you mean I need to add a documentation in Documentation/fb/?

This was more a question to the fb maintainers.

quoted

quoted

+static irqreturn_t fsl_dcu_irq(int irq, void *dev_id) {
+	struct dcu_fb_data *dcufb = dev_id;
+	unsigned int status = readl(dcufb->reg_base + DCU_INT_STATUS);
+	u32 dcu_mode;
+
+	if (status) {

Save indendation level by bailing out early.

[Alison Wang] What do you mean?

Instead of doing:

	if (bla) {
		dothis();
		dothat();
		return;
	}

	return;

it's easier to read:

	if (!bla)
		return;

	dothis();
	dothat();
	return;

Note that dothis() and dothat() are one indentation level to the left
and thus have more space per line.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |