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.
> +
> +#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)?
> +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.
> +
> +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.
> + default:
> + printk(KERN_ERR "unsupported color depth: %u\n",
> + var->bits_per_pixel);
Use dev_* for printing messages in drivers.
> +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?
> + default:
> + printk(KERN_ERR "unsupported color depth: %u\n",
> + var->bits_per_pixel);
BUG(). This can't happen since you make that sure above.
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int calc_div_ratio(struct fb_info *info)
> +{
Use a consistent namespace for function names (fsl_dcu_)
> + 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.
> + 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?
> + 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.
> +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.
> +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.
> + goto failed_checkvar;
> + }
> +
> + if (register_framebuffer(info) < 0) {
> + ret = -EINVAL;
ditto
> +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.
> + 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_*
> + 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
> + 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.
> + }
> + 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.
> +failed_ioremap:
> + kfree(dcufb);
This is allocated with devm_*. Drop this.
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 |
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
