From mboxrd@z Thu Jan 1 00:00:00 1970 From: Russell King - ARM Linux Subject: Re: [RFC PATCH 1/3] amba-clcd: Add Device Tree support to amba-clcd driver Date: Fri, 21 Sep 2012 12:02:35 +0100 Message-ID: <20120921110234.GD15609@n2100.arm.linux.org.uk> References: <1348070666-9153-1-git-send-email-ryan.harkin@linaro.org> <1348070666-9153-2-git-send-email-ryan.harkin@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <1348070666-9153-2-git-send-email-ryan.harkin-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: devicetree-discuss-bounces+gldd-devicetree-discuss=m.gmane.org-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org Sender: "devicetree-discuss" To: Ryan Harkin Cc: stigge-uj/7R2tJ6VmzQB+pC5nmwQ@public.gmane.org, tixy-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org, linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, pawel.moll-5wv7dgnIgG8@public.gmane.org, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org, spear-devel-nkJGhpqTU55BDgjK7y7TUQ@public.gmane.org, liviu.dudau-5wv7dgnIgG8@public.gmane.org, shiraz.hashim-qxv4g6HH51o@public.gmane.org, viresh.linux-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org, arnd.bergmann-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org List-Id: devicetree@vger.kernel.org This patch has a number of serious problems with it. On Wed, Sep 19, 2012 at 05:04:24PM +0100, Ryan Harkin wrote: > @@ -391,6 +394,19 @@ static int clcdfb_blank(int blank_mode, struct fb_info *info) > } > return 0; > } This needs a blank line. > +int clcdfb_mmap_dma(struct clcd_fb *fb, struct vm_area_struct *vma) > +{ > + return dma_mmap_writecombine(&fb->dev->dev, vma, > + fb->fb.screen_base, > + fb->fb.fix.smem_start, > + fb->fb.fix.smem_len); > +} > + > +void clcdfb_remove_dma(struct clcd_fb *fb) > +{ > + dma_free_writecombine(&fb->dev->dev, fb->fb.fix.smem_len, > + fb->fb.screen_base, fb->fb.fix.smem_start); > +} > > static int clcdfb_mmap(struct fb_info *info, > struct vm_area_struct *vma) > @@ -542,12 +558,249 @@ static int clcdfb_register(struct clcd_fb *fb) > return ret; > } > > +struct string_lookup { > + const char *string; > + const u32 val; > +}; > + > +static struct string_lookup vmode_lookups[] = { > + { "FB_VMODE_NONINTERLACED", FB_VMODE_NONINTERLACED}, > + { "FB_VMODE_INTERLACED", FB_VMODE_INTERLACED}, > + { "FB_VMODE_DOUBLE", FB_VMODE_DOUBLE}, > + { "FB_VMODE_ODD_FLD_FIRST", FB_VMODE_ODD_FLD_FIRST}, > + { NULL, 0 }, > +}; > + > +static struct string_lookup tim2_lookups[] = { > + { "TIM2_CLKSEL", TIM2_CLKSEL}, > + { "TIM2_IVS", TIM2_IVS}, > + { "TIM2_IHS", TIM2_IHS}, Inversion of the sync control signals are part of the framebuffer API, and should not be specified as part of this register definition. Instead, they should be specified using FB_SYNC_HOR_HIGH_ACT and FB_SYNC_VERT_HIGH_ACT. Setting them in ->tim2 is bad news because it just confuses these settings. > + { "TIM2_IOE", TIM2_IOE}, > + { "TIM2_BCD", TIM2_BCD}, > + { NULL, 0}, > +}; Another blank line required. > +static struct string_lookup cntl_lookups[] = { > + {"CNTL_LCDEN", CNTL_LCDEN}, Err, no. > + {"CNTL_LCDBPP1", CNTL_LCDBPP1}, > + {"CNTL_LCDBPP2", CNTL_LCDBPP2}, > + {"CNTL_LCDBPP4", CNTL_LCDBPP4}, > + {"CNTL_LCDBPP8", CNTL_LCDBPP8}, > + {"CNTL_LCDBPP16", CNTL_LCDBPP16}, > + {"CNTL_LCDBPP16_565", CNTL_LCDBPP16_565}, > + {"CNTL_LCDBPP16_444", CNTL_LCDBPP16_444}, > + {"CNTL_LCDBPP24", CNTL_LCDBPP24}, The colour depth is derived from the video mode setting, which will be overridden anyway - and the allowable depths are derived from the panel capabilities and the board capabilities. > + {"CNTL_LCDBW", CNTL_LCDBW}, > + {"CNTL_LCDTFT", CNTL_LCDTFT}, > + {"CNTL_LCDMONO8", CNTL_LCDMONO8}, > + {"CNTL_LCDDUAL", CNTL_LCDDUAL}, These four are properties of the panel, so they're fine. > + {"CNTL_BGR", CNTL_BGR}, BGR is also overridden by the driver. > + {"CNTL_BEBO", CNTL_BEBO}, > + {"CNTL_BEPO", CNTL_BEPO}, >>From what I remember, these are framebuffer layout control bits. They aren't panel properties, and so they shouldn't be specified in DT. > + {"CNTL_LCDPWR", CNTL_LCDPWR}, Err, no. Specifying power control here is bad news - have you read the data sheet for the CLCD controller (which specifies a certain sequence for LCDEN and LCDPWR)? Have you read the driver code and realized that the driver controls these bits, and therefore specifying them in DT is absurd? > + {"CNTL_LCDVCOMP(1)", CNTL_LCDVCOMP(1)}, > + {"CNTL_LCDVCOMP(2)", CNTL_LCDVCOMP(2)}, > + {"CNTL_LCDVCOMP(3)", CNTL_LCDVCOMP(3)}, > + {"CNTL_LCDVCOMP(4)", CNTL_LCDVCOMP(4)}, > + {"CNTL_LCDVCOMP(5)", CNTL_LCDVCOMP(5)}, > + {"CNTL_LCDVCOMP(6)", CNTL_LCDVCOMP(6)}, > + {"CNTL_LCDVCOMP(7)", CNTL_LCDVCOMP(7)}, > + {"CNTL_LDMAFIFOTIME", CNTL_LDMAFIFOTIME}, > + {"CNTL_WATERMARK", CNTL_WATERMARK}, > + { NULL, 0}, > +}; Another blank line required. > +static struct string_lookup caps_lookups[] = { > + {"CLCD_CAP_RGB444", CLCD_CAP_RGB444}, > + {"CLCD_CAP_RGB5551", CLCD_CAP_RGB5551}, > + {"CLCD_CAP_RGB565", CLCD_CAP_RGB565}, > + {"CLCD_CAP_RGB888", CLCD_CAP_RGB888}, > + {"CLCD_CAP_BGR444", CLCD_CAP_BGR444}, > + {"CLCD_CAP_BGR5551", CLCD_CAP_BGR5551}, > + {"CLCD_CAP_BGR565", CLCD_CAP_BGR565}, > + {"CLCD_CAP_BGR888", CLCD_CAP_BGR888}, > + {"CLCD_CAP_444", CLCD_CAP_444}, > + {"CLCD_CAP_5551", CLCD_CAP_5551}, > + {"CLCD_CAP_565", CLCD_CAP_565}, > + {"CLCD_CAP_888", CLCD_CAP_888}, > + {"CLCD_CAP_RGB", CLCD_CAP_RGB}, > + {"CLCD_CAP_BGR", CLCD_CAP_BGR}, > + {"CLCD_CAP_ALL", CLCD_CAP_ALL}, > + { NULL, 0}, > +}; > + > +u32 parse_setting(struct string_lookup *lookup, const char *name) > +{ > + int i = 0; > + while (lookup[i].string != NULL) { > + if (strcmp(lookup[i].string, name) == 0) > + return lookup[i].val; > + ++i; > + } > + return -EINVAL; > +} Why is this non-static? > + > +u32 get_string_lookup(struct device_node *node, const char *name, > + struct string_lookup *lookup) > +{ > + const char *string; > + int count, i, ret = 0; > + > + count = of_property_count_strings(node, name); > + if (count >= 0) > + for (i = 0; i < count; i++) > + if (of_property_read_string_index(node, name, i, > + &string) == 0) > + ret |= parse_setting(lookup, string); > + return ret; > +} And this? > + > +int get_val(struct device_node *node, const char *string) > +{ > + u32 ret = 0; > + > + if (of_property_read_u32(node, string, &ret)) > + ret = -1; > + return ret; > +} And this? > + > +struct clcd_panel *getPanel(struct device_node *node) Have you read coding style? We don't use capitals in function names. > +{ > + static struct clcd_panel panel; > + > + panel.mode.refresh = get_val(node, "refresh"); > + panel.mode.xres = get_val(node, "xres"); > + panel.mode.yres = get_val(node, "yres"); > + panel.mode.pixclock = get_val(node, "pixclock"); It's debatable whether we want to specify the pixel clock in DT or not - it can be calculated from the other parameters here 1e12/(refresh * htotal * vtotal) ps. > + panel.mode.left_margin = get_val(node, "left_margin"); > + panel.mode.right_margin = get_val(node, "right_margin"); > + panel.mode.upper_margin = get_val(node, "upper_margin"); > + panel.mode.lower_margin = get_val(node, "lower_margin"); > + panel.mode.hsync_len = get_val(node, "hsync_len"); > + panel.mode.vsync_len = get_val(node, "vsync_len"); > + panel.mode.sync = get_val(node, "sync"); An integer sync property? You're exposing kernel internal definitions into DT here which is unacceptable. > + panel.bpp = get_val(node, "bpp"); > + panel.width = (signed short) get_val(node, "width"); > + panel.height = (signed short) get_val(node, "height"); > + > + panel.mode.vmode = get_string_lookup(node, "vmode", vmode_lookups); > + panel.tim2 = get_string_lookup(node, "tim2", tim2_lookups); > + panel.cntl = get_string_lookup(node, "cntl", cntl_lookups); > + panel.caps = get_string_lookup(node, "caps", caps_lookups); > + > + return &panel; > +} > + > +struct clcd_panel *clcdfb_get_panel(const char *name) Why is this exported? > +{ > + struct device_node *node = NULL; > + const char *mode; > + struct clcd_panel *panel = NULL; > + > + do { > + node = of_find_compatible_node(node, NULL, "panel"); > + if (node) > + if (of_property_read_string(node, "mode", &mode) == 0) > + if (strcmp(mode, name) == 0) { > + panel = getPanel(node); > + panel->mode.name = name; > + } > + } while (node != NULL); > + > + return panel; > +} > + > +#ifdef CONFIG_OF > +static int clcdfb_dt_init(struct clcd_fb *fb) > +{ > + int err = 0; > + struct device_node *node; > + const char *mode; > + dma_addr_t dma; > + u32 use_dma; > + const __be32 *prop; > + int len, na, ns; > + phys_addr_t fb_base, fb_size; > + > + node = fb->dev->dev.of_node; > + if (!node) > + return -ENODEV; > + > + na = of_n_addr_cells(node); > + ns = of_n_size_cells(node); > + > + if (WARN_ON(of_property_read_string(node, "mode", &mode))) > + return -ENODEV; > + > + fb->panel = clcdfb_get_panel(mode); > + if (!fb->panel) > + return -EINVAL; > + fb->fb.fix.smem_len = fb->panel->mode.xres * fb->panel->mode.yres * 2; > + > + if (of_property_read_u32(node, "use_dma", &use_dma)) > + use_dma = 0; > + if (use_dma) { > + fb->fb.screen_base = dma_alloc_writecombine(&fb->dev->dev, > + fb->fb.fix.smem_len, &dma, GFP_KERNEL); > + if (!fb->fb.screen_base) { > + pr_err("CLCD: unable to map framebuffer\n"); > + err = -ENOMEM; > + } else > + fb->fb.fix.smem_start = dma; > + } else { > + prop = of_get_property(node, "framebuffer", &len); > + if (WARN_ON(!prop || len < (na + ns) * sizeof(*prop))) > + return -EINVAL; > + fb_base = of_read_number(prop, na); > + fb_size = of_read_number(prop + na, ns); > + > + if (memblock_remove(fb_base, fb_size) != 0) > + return -EINVAL; No. You can not do this here, calling this function after the reserve callback into the platform code has completed will corrupt the kernel. > + > + fb->fb.fix.smem_start = fb_base; > + fb->fb.screen_base = ioremap_wc(fb->fb.fix.smem_start, fb_size); > + } > + return err; > +} > +#endif /* CONFIG_OF */ > + > static int clcdfb_probe(struct amba_device *dev, const struct amba_id *id) > { > struct clcd_board *board = dev->dev.platform_data; > struct clcd_fb *fb; > int ret; > > +#ifdef CONFIG_OF > + if (dev->dev.of_node) { > + const __be32 *prop; > + int len, na, ns; > + phys_addr_t reg_base; > + > + na = of_n_addr_cells(dev->dev.of_node); > + ns = of_n_size_cells(dev->dev.of_node); > + > + prop = of_get_property(dev->dev.of_node, "reg", &len); > + if (WARN_ON(!prop || len < (na + ns) * sizeof(*prop))) > + return -EINVAL; > + reg_base = of_read_number(prop, na); > + > + if (dev->res.start != reg_base) > + return -EINVAL; > + > + if (!board) { > + board = kzalloc(sizeof(struct clcd_board), GFP_KERNEL); > + if (!board) > + return -EINVAL; > + board->name = "Device Tree CLCD PL111"; > + board->caps = CLCD_CAP_5551 | CLCD_CAP_565; This looks like it's been hard-coded for one particular board, and one particular board only. > + board->check = clcdfb_check; > + board->decode = clcdfb_decode; > + board->setup = clcdfb_dt_init; > + board->mmap = clcdfb_mmap_dma; > + board->remove = clcdfb_remove_dma; > + } > + } > +#endif /* CONFIG_OF */ > + > if (!board) > return -EINVAL; >