[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:

quoted

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.

quoted

quoted

quoted

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

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.

[Alison Wang] Ok, I will change it as follows.
#define DCU_MODE_BLEND_ITER(x)	((x) << 20)

quoted

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.

[Alison Wang] Ok, I will use a consistent namespace for this function.

quoted

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

quoted

quoted

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

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);

quoted

quoted

quoted

+	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.

[Alison Wang] I see. Thanks.


Best Regards,
Alison Wang