[RFC PATCH 1/3] amba-clcd: Add Device Tree support to amba-clcd driver
Russell King - ARM Linux
linux at arm.linux.org.uk
Fri Sep 21 07:02:35 EDT 2012
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;
>
More information about the linux-arm-kernel
mailing list