This patch has a number of serious problems with it.
On Wed, Sep 19, 2012 at 05:04:24PM +0100, Ryan Harkin wrote:
quoted hunk
@@ -391,6 +394,19 @@ static int clcdfb_blank(int blank_mode, struct fb_info *info)
}
return 0;
}
This needs a blank line.
quoted hunk
+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.
quoted hunk
+ { "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.
quoted hunk
+ {"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.
quoted hunk
+ {"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?
quoted hunk
+ {"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.
quoted hunk
+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?
quoted hunk
+
+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?
quoted hunk
+
+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.
quoted hunk
+{
+ 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.
quoted hunk
+ 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.
quoted hunk
+ 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?
quoted hunk
+{
+ 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.
quoted hunk
+
+ 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.
quoted hunk
+ 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;