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

[Wang Huan-B18965]

Hi, Sascha,

> On Fri, Jul 12, 2013 at 02:07:55PM +0800, Alison Wang wrote:
> > 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.
> 
> > +
> > +#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).
> 
> > +static struct fb_videomode dcu_mode_db[] = {
> > +	{
> > +		.name		= "480x272",
> > +		.refresh	= 75,
> > +		.xres		= 480,
> > +		.yres		= 272,
> > +		.pixclock	= 91996,
> > +		.left_margin	= 2,
> > +		.right_margin	= 2,
> > +		.upper_margin	= 1,
> > +		.lower_margin	= 1,
> > +		.hsync_len	= 41,
> > +		.vsync_len	= 2,
> > +		.sync		= FB_SYNC_COMP_HIGH_ACT | FB_SYNC_VERT_HIGH_ACT,
> > +		.vmode		= FB_VMODE_NONINTERLACED,
> > +	},
> > +};
> 
> We have ways to describe a panel in dt. Use them.
[Alison Wang] Ok. I don't know it is common to describe the panel in dts for the latest kernel. Thanks.
> 
> > +
> > +static struct mfb_info mfb_template[] = {
> > +	{
> > +	.index = LAYER0,
> > +	.id = "Layer0",
> > +	.alpha = 0xff,
> > +	.blend = 0,
> > +	.count = 0,
> > +	.x_layer_d = 0,
> > +	.y_layer_d = 0,
> > +	},
> 
> Wrong indentation.
[Alison Wang] I will fix it in next version.
> 
> > +	default:
> > +		printk(KERN_ERR "unsupported color depth: %u\n",
> > +			var->bits_per_pixel);
> 
> Use dev_* for printing messages in drivers.
[Alison Wang] Ok.
> 
> > +static int fsl_dcu_check_var(struct fb_var_screeninfo *var,
> > +		struct fb_info *info)
> > +{
> > +	if (var->xres_virtual < var->xres)
> > +		var->xres_virtual = var->xres;
> > +	if (var->yres_virtual < var->yres)
> > +		var->yres_virtual = var->yres;
> > +
> > +	if (var->xoffset < 0)
> > +		var->xoffset = 0;
> > +
> > +	if (var->yoffset < 0)
> > +		var->yoffset = 0;
> 
> Ever seen an unsigned value going below zero?
[Alison Wang] You are right, I will remove them in next version.
> 
> > +	default:
> > +		printk(KERN_ERR "unsupported color depth: %u\n",
> > +			var->bits_per_pixel);
> 
> BUG(). This can't happen since you make that sure above.
[Alison Wang] You are right, I will remove it in next version.
> 
> > +		return -EINVAL;
> > +	}
> > +
> > +	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.
> 
> > +	writel(DCU_THRESHOLD_LS_BF_VS(0x3) |
> DCU_THRESHOLD_OUT_BUF_HIGH(0x78) |
> > +		DCU_THRESHOLD_OUT_BUF_LOW(0), dcufb->reg_base +
> DCU_THRESHOLD);
> > +
> > +	enable_controller(info);
> > +}
> 
> Make your functions symmetric. If there's update_controller(), the
> function should do exactly that, it should *not* enable the controller.
> Call this from the users of this function if necessary.
[Alison Wang] Ok, I will move this function out, and add it here.
if (mfbi->index == LAYER0) {
	update_controller(info);
+	enable_controller(info);
}
> 
> > +		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/?
> 
> > +	default:
> > +		printk(KERN_ERR "unknown ioctl command (0x%08X)\n", cmd);
> 
> What shall a reader of the kernel log do with a message like this? It's
> utterly useless when he doesn't even now which device failed here. Just
> drop this.
[Alison Wang] Ok.
> 
> > +static void enable_interrupts(struct dcu_fb_data *dcufb) {
> > +	u32 int_mask = readl(dcufb->reg_base + DCU_INT_MASK);
> > +
> > +	writel(int_mask & ~DCU_INT_MASK_UNDRUN, dcufb->reg_base +
> > +DCU_INT_MASK); }
> 
> Inline this code where you need it. Introducing a function for a single
> register write seems quite useless.
[Alison Wang] Ok, I will inline it.
> 
> > +static int install_framebuffer(struct fb_info *info) {
> > +	struct mfb_info *mfbi = info->par;
> > +	struct fb_videomode *db = dcu_mode_db;
> > +	unsigned int dbsize = ARRAY_SIZE(dcu_mode_db);
> > +	int ret;
> > +
> > +	info->var.activate = FB_ACTIVATE_NOW;
> > +	info->fbops = &fsl_dcu_ops;
> > +	info->flags = FBINFO_FLAG_DEFAULT;
> > +	info->pseudo_palette = &mfbi->pseudo_palette;
> > +
> > +	fb_alloc_cmap(&info->cmap, 16, 0);
> > +
> > +	ret = fb_find_mode(&info->var, info, fb_mode, db, dbsize,
> > +			NULL, default_bpp);
> > +
> > +	if (fsl_dcu_check_var(&info->var, info)) {
> > +		ret = -EINVAL;
> 
> Propagate the error.
[Alison Wang] I will fix in next version.
> 
> > +		goto failed_checkvar;
> > +	}
> > +
> > +	if (register_framebuffer(info) < 0) {
> > +		ret = -EINVAL;
> 
> ditto
[Alison Wang] I will fix in next version.
> 
> > +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?
> 
> > +		if (status & DCU_INT_STATUS_UNDRUN) {
> > +			dcu_mode = readl(dcufb->reg_base + DCU_DCU_MODE);
> > +			dcu_mode &= ~DCU_MODE_DCU_MODE_MASK;
> > +			writel(dcu_mode | DCU_MODE_DCU_MODE(DCU_MODE_OFF),
> > +				dcufb->reg_base + DCU_DCU_MODE);
> > +			udelay(1);
> > +			writel(dcu_mode | DCU_MODE_DCU_MODE(DCU_MODE_NORMAL),
> > +				dcufb->reg_base + DCU_DCU_MODE);
> > +		}
> > +		writel(status, dcufb->reg_base + DCU_INT_STATUS);
> > +		return IRQ_HANDLED;
> > +	}
> > +	return IRQ_NONE;
> > +}
> > +
> > +	if (IS_ERR(tcon_clk)) {
> > +		ret = PTR_ERR(tcon_clk);
> > +		goto failed_getclock;
> > +	}
> > +	clk_prepare_enable(tcon_clk);
> > +
> > +	tcon_reg = of_iomap(tcon_np, 0);
> 
> Use devm_*
[Alison Wang] Ok.
> 
> > +	dcufb->irq = platform_get_irq(pdev, 0);
> > +	if (!dcufb->irq) {
> > +		ret = -EINVAL;
> > +		goto failed_getirq;
> > +	}
> > +
> > +	ret = request_irq(dcufb->irq, fsl_dcu_irq, 0, DRIVER_NAME, dcufb);
> 
> Use devm_request_irq
[Alison Wang] Ok.
> 
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "could not request irq\n");
> > +		goto failed_requestirq;
> > +	}
> > +
> > +	/* Put TCON in bypass mode, so the input signals from DCU are
> passed
> > +	 * through TCON unchanged */
> > +	ret = bypass_tcon(pdev->dev.of_node);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "could not bypass TCON\n");
> > +		goto failed_bypasstcon;
> > +	}
> > +
> > +	dcufb->clk = devm_clk_get(&pdev->dev, "dcu");
> > +	if (IS_ERR(dcufb->clk)) {
> > +		dev_err(&pdev->dev, "could not get clock\n");
> > +		goto failed_getclock;
> 
> You will return 0 here.
[Alison Wang] You are right, I will fix it in next version.
> 
> > +	}
> > +	clk_prepare_enable(dcufb->clk);
> > +
> > +	for (i = 0; i < ARRAY_SIZE(dcufb->fsl_dcu_info); i++) {
> > +		dcufb->fsl_dcu_info[i] =
> > +			framebuffer_alloc(sizeof(struct mfb_info), &pdev-
> >dev);
> > +		if (!dcufb->fsl_dcu_info[i]) {
> > +			ret = ENOMEM;
> 
> -ENOMEM
> 
> > +failed_alloc_framebuffer:
> > +failed_getclock:
> > +failed_bypasstcon:
> > +	free_irq(dcufb->irq, dcufb);
> > +failed_requestirq:
> > +failed_getirq:
> > +	iounmap(dcufb->reg_base);
> 
> You used devm_ioremap, so drop this.
[Alison Wang] Ok.
> 
> > +failed_ioremap:
> > +	kfree(dcufb);
> 
> This is allocated with devm_*. Drop this.
[Alison Wang] Ok.

Thanks for your comments.


Best Regards,
Alison Wang