From mboxrd@z Thu Jan 1 00:00:00 1970 From: rurality.linux@gmail.com (Qiang Wang) Date: Mon, 1 Feb 2010 21:33:10 +0800 Subject: [Linux-arm-nuc900] Re: [PATCH]NUC900 LCD Controller Driver In-Reply-To: <20100129162755.b591a2fb.akpm@linux-foundation.org> References: <20100129162755.b591a2fb.akpm@linux-foundation.org> Message-ID: <71413c851002010533n5d6b8078i316f88668d23ba23@mail.gmail.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org ? 2010?1?30? ??8:27?Andrew Morton ??? > On Mon, 11 Jan 2010 17:05:44 +0900 > Wang Qiang wrote: > >> hi, Dear Wan >> >> There is the patch of LCD controller driver for nuc900s. >> The Linux LOGO is just fine and the FB-Test application was ok, too. >> >> best regards >> wangqiang >> >> >> ... >> >> @@ -380,6 +381,48 @@ struct platform_device nuc900_device_kpi = { >> ? ? ? .resource ? ? ? = nuc900_kpi_resource, >> ?}; >> >> +#ifdef CONFIG_FB_NUC900 >> + >> +static struct resource nuc900_lcd_resource[] = { >> + ? ? [0] = { >> + ? ? ? ? ? ? .start = W90X900_PA_LCD, >> + ? ? ? ? ? ? .end ? = W90X900_PA_LCD + W90X900_SZ_LCD - 1, >> + ? ? ? ? ? ? .flags = IORESOURCE_MEM, >> + ? ? }, >> + ? ? [1] = { >> + ? ? ? ? ? ? .start = IRQ_LCD, >> + ? ? ? ? ? ? .end ? = IRQ_LCD, >> + ? ? ? ? ? ? .flags = IORESOURCE_IRQ, >> + ? ? } >> +}; >> + >> +static u64 nuc900_device_lcd_dmamask = 0xffffffffUL; > > I suspect this should have type `dma_addr_t', but `struct device' uses > open-coded u64 too. ?Odd. > > It makes no sense to initialise a u64 with an unsigned long value - > it's wrong on a 32-bit machine. > > this: > > ? ? ? ?static u64 nuc900_device_lcd_dmamask = -1; > > will work. > >> +struct platform_device nuc900_device_lcd = { >> + ? ? .name ? ? ? ? ? ? = "nuc900-lcd", >> + ? ? .id ? ? ? ? ? ? ? = -1, >> + ? ? .num_resources ? ?= ARRAY_SIZE(nuc900_lcd_resource), >> + ? ? .resource ? ? ? ? = nuc900_lcd_resource, >> + ? ? .dev ? ? ? ? ? ? ?= { >> + ? ? ? ? ? ? .dma_mask ? ? ? ? ? ? ? = &nuc900_device_lcd_dmamask, >> + ? ? ? ? ? ? .coherent_dma_mask ? ? ?= 0xffffffffUL > > And this gets initialised to 0x00000000ffffffff also. ?Using -1 will fix. > >> + ? ? } >> +}; >> + >> >> ... >> >> + */ >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#ifdef CONFIG_PM > > Is this ifdef needed? > >> +#include >> +#endif >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#include "nuc900fb.h" >> + >> +/* Debugging stuff */ >> +#ifdef CONFIG_FB_NUC900_DEBUG >> +static int debug ? ? = 1; >> +#define dprintk(msg...) ? ? ?{ printk(KERN_DEBUG "nuc900 lcd: " msg); } >> +#else >> +static int debug; >> +#define dprintk(msg...) >> +#endif > > Would be better to use the standard pr_debug(), rather than inventing > private infrastructure. ?That way you get to enjoy the facilities which > lib/dynamic_debug.c offers. > >> + >> +/* >> + * ?Initialize the nuc900 video (dual) buffer address >> + */ >> +static void nuc900fb_set_lcdaddr(struct fb_info *info) >> +{ >> + ? ? struct nuc900fb_info *fbi = info->par; >> + ? ? void __iomem *regs = fbi->io; >> + ? ? unsigned long vbaddr1, vbaddr2; >> + ? ? vbaddr1 ?= info->fix.smem_start; > > It's conventional to put a blank line beterrn end-of-locals and start-of-code. > >> + ? ? vbaddr2 ?= info->fix.smem_start; >> + ? ? vbaddr2 += info->fix.line_length * info->var.yres; >> + ? ? /*set frambuffer start phy addr*/ > > Like this: > > ? ? ? ?/* set frambuffer start phy addr */ > >> + ? ? writel(vbaddr1, regs + REG_LCM_VA_BADDR0); >> + ? ? writel(vbaddr2, regs + REG_LCM_VA_BADDR1); >> + >> + ? ? writel(fbi->regs.lcd_va_fbctrl, regs + REG_LCM_VA_FBCTRL); >> + ? ? writel(fbi->regs.lcd_va_scale, regs + REG_LCM_VA_SCALE); >> +} >> + >> +//static void nuc900fb_clk_select(struct fb_info *info, unsigned long pixclk) >> +//{ >> +// ? struct nuc900fb_info *fbi = info->par; >> +// ? int div; >> +// ? if (clk <= 15 * 1000 * 1000) { >> +// ? ? ? ? ? div = (15 * 1000 * 1000)/ >> +// ? ? ? ? ? nuc900_driver_clksrc_div(fbi->dev, "ext", 0x2); >> +// ? } >> +// >> +//} > > Please feed the patch through scrupts/checkpatch.pl. > >> +/* >> + * ? calculate divider for lcd div >> + */ >> +static unsigned int nuc900fb_calc_pixclk(struct nuc900fb_info *fbi, >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?unsigned long pixclk) >> +{ >> + ? ? unsigned long clk = fbi->clk_rate; >> + ? ? unsigned long long div; >> + >> + ? ? /* pixclk is in picseconds. our clock is in Hz*/ >> + ? ? /* div = (clk * pixclk)/10^12 */ >> + ? ? div = (unsigned long long)clk * pixclk; >> + ? ? div >>= 12; >> + ? ? do_div(div, 625 * 625UL * 625); >> + >> + ? ? dprintk("pixclk %ld, divisor is %ld\n", pixclk, (long)div); > > Perhaps use %lld and remove the cast. > >> + ? ? return div; >> +} >> + >> +/* >> + * ? Check the video params of 'var'. >> + */ >> +static int nuc900fb_check_var(struct fb_var_screeninfo *var, >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ?struct fb_info *info) >> +{ >> + ? ? struct nuc900fb_info *fbi = info->par; >> + ? ? struct nuc900fb_mach_info *mach_info = fbi->dev->platform_data; >> + ? ? struct nuc900fb_display *display = NULL; >> + ? ? struct nuc900fb_display *default_display = mach_info->displays + >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?mach_info->default_display; >> + ? ? int i; >> + >> + ? ? dprintk("check_var(var=%p, info=%p)\n", var, info); >> + >> + ? ? /* validate x/y resolution */ >> + ? ? /* choose default mode if possible */ >> + ? ? if (var->xres == default_display->xres && >> + ? ? ? ? var->yres == default_display->yres && >> + ? ? ? ? var->bits_per_pixel == default_display->bpp) >> + ? ? ? ? ? ? display = default_display; >> + ? ? else >> + ? ? ? ? ? ? for (i = 0; i < mach_info->num_displays; i++) >> + ? ? ? ? ? ? ? ? ? ? if (var->xres == mach_info->displays[i].xres && >> + ? ? ? ? ? ? ? ? ? ? ? ? var->yres == mach_info->displays[i].yres && >> + ? ? ? ? ? ? ? ? ? ? ? ? var->bits_per_pixel == mach_info->displays[i].bpp) { >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? display = mach_info->displays + i; >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? break; >> + ? ? ? ? ? ? ? ? ? ? } >> + >> + ? ? if (display == NULL) { >> + ? ? ? ? ? ? dprintk("wrong resolution or depth %dx%d at %d bit per pixel\n", >> + ? ? ? ? ? ? ? ? ? ? var->xres, var->yres, var->bits_per_pixel); > > Some of the printks in here really should be printks, not the > compile-time-suppressible dprintk(). ?Please review them all and > convert the ones which will be useful to end-users into printk(). > >> + ? ? ? ? ? ? return -EINVAL; >> + ? ? } >> + >> + ? ? /* it should be the same size as the display */ >> + ? ? var->xres_virtual ? ? ? = display->xres; >> + ? ? var->yres_virtual ? ? ? = display->yres; >> + ? ? var->height ? ? ? ? ? ? = display->height; >> + ? ? var->width ? ? ? ? ? ? ?= display->width; >> + >> + ? ? /* copy lcd settings */ >> + ? ? var->pixclock ? ? ? ? ? = display->pixclock; >> + ? ? var->left_margin ? ? ? ?= display->left_margin; >> + ? ? var->right_margin ? ? ? = display->right_margin; >> + ? ? var->upper_margin ? ? ? = display->upper_margin; >> + ? ? var->lower_margin ? ? ? = display->lower_margin; >> + ? ? var->vsync_len ? ? ? ? ?= display->vsync_len; >> + ? ? var->hsync_len ? ? ? ? ?= display->hsync_len; >> + >> + ? ? var->transp.offset ? ? ?= 0; >> + ? ? var->transp.length ? ? ?= 0; >> + >> + ? ? fbi->regs.lcd_dccs = display->dccs; >> + ? ? fbi->regs.lcd_device_ctrl = display->devctl; >> + ? ? fbi->regs.lcd_va_fbctrl = display->fbctrl; >> + ? ? fbi->regs.lcd_va_scale = display->scale; >> + >> + ? ? /* set R/G/B possions */ >> + ? ? switch (var->bits_per_pixel) { >> + ? ? case 1: >> + ? ? case 2: >> + ? ? case 4: >> + ? ? case 8: > > The above four lines are unneeded, but it's OK keeping them for > documentation purposes. > >> + ? ? default: >> + ? ? ? ? ? ? var->red.offset ? ? ? ? = 0; >> + ? ? ? ? ? ? var->red.length ? ? ? ? = var->bits_per_pixel; >> + ? ? ? ? ? ? var->green ? ? ? ? ? ? ?= var->red; >> + ? ? ? ? ? ? var->blue ? ? ? ? ? ? ? = var->red; >> + ? ? ? ? ? ? break; >> + ? ? case 12: >> + ? ? ? ? ? ? var->red.length ? ? ? ? = 4; >> + ? ? ? ? ? ? var->green.length ? ? ? = 4; >> + ? ? ? ? ? ? var->blue.length ? ? ? ?= 4; >> + ? ? ? ? ? ? var->red.offset ? ? ? ? = 8; >> + ? ? ? ? ? ? var->green.offset ? ? ? = 4; >> + ? ? ? ? ? ? var->blue.offset ? ? ? ?= 0; >> + ? ? ? ? ? ? break; >> + ? ? case 16: >> + ? ? ? ? ? ? var->red.length ? ? ? ? = 5; >> + ? ? ? ? ? ? var->green.length ? ? ? = 6; >> + ? ? ? ? ? ? var->blue.length ? ? ? ?= 5; >> + ? ? ? ? ? ? var->red.offset ? ? ? ? = 11; >> + ? ? ? ? ? ? var->green.offset ? ? ? = 5; >> + ? ? ? ? ? ? var->blue.offset ? ? ? ?= 0; >> + ? ? ? ? ? ? break; >> + ? ? case 18: >> + ? ? ? ? ? ? var->red.length ? ? ? ? = 6; >> + ? ? ? ? ? ? var->green.length ? ? ? = 6; >> + ? ? ? ? ? ? var->blue.length ? ? ? ?= 6; >> + ? ? ? ? ? ? var->red.offset ? ? ? ? = 12; >> + ? ? ? ? ? ? var->green.offset ? ? ? = 6; >> + ? ? ? ? ? ? var->blue.offset ? ? ? ?= 0; >> + ? ? ? ? ? ? break; >> + ? ? case 32: >> + ? ? ? ? ? ? var->red.length ? ? ? ? = 8; >> + ? ? ? ? ? ? var->green.length ? ? ? = 8; >> + ? ? ? ? ? ? var->blue.length ? ? ? ?= 8; >> + ? ? ? ? ? ? var->red.offset ? ? ? ? = 16; >> + ? ? ? ? ? ? var->green.offset ? ? ? = 8; >> + ? ? ? ? ? ? var->blue.offset ? ? ? ?= 0; >> + ? ? ? ? ? ? break; >> + ? ? } >> + >> + ? ? return 0; >> +} >> + >> +/* >> + * ? Calculate lcd register values from var setting & save into hw >> + */ >> +static void nuc900fb_calculate_lcd_regs(const struct fb_info *info, >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct nuc900fb_hw *regs) >> +{ >> + ? ? const struct fb_var_screeninfo *var = &info->var; >> + ? ? int vtt = var->height + var->upper_margin + var->lower_margin; >> + ? ? int htt = var->width + var->left_margin + var->right_margin; >> + ? ? int hsync = var->width + var->right_margin; >> + ? ? int vsync = var->height + var->lower_margin; > > newline here, please. > >> + ? ? regs->lcd_crtc_size = LCM_CRTC_SIZE_VTTVAL(vtt) | >> + ? ? ? ? ? ? ? ? ? ? ? ? ? LCM_CRTC_SIZE_HTTVAL(htt); >> + ? ? regs->lcd_crtc_dend = LCM_CRTC_DEND_VDENDVAL(var->height) | >> + ? ? ? ? ? ? ? ? ? ? ? ? ? LCM_CRTC_DEND_HDENDVAL(var->width); >> + ? ? regs->lcd_crtc_hr = LCM_CRTC_HR_EVAL(var->width + 5) | >> + ? ? ? ? ? ? ? ? ? ? ? ? LCM_CRTC_HR_SVAL(var->width + 1); >> + ? ? regs->lcd_crtc_hsync = LCM_CRTC_HSYNC_EVAL(hsync + var->hsync_len) | >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ?LCM_CRTC_HSYNC_SVAL(hsync); >> + ? ? regs->lcd_crtc_vr = LCM_CRTC_VR_EVAL(vsync + var->vsync_len) | >> + ? ? ? ? ? ? ? ? ? ? ? ? LCM_CRTC_VR_SVAL(vsync); >> + >> +} >> + >> >> ... >> >> +static int nuc900fb_debug_store(struct device *dev, >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct device_attribute *attr, >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? const char *buf, size_t len) >> +{ >> + ? ? if (len < 1) >> + ? ? ? ? ? ? return -EINVAL; >> + >> + ? ? if (strnicmp(buf, "on", 2) == 0 || >> + ? ? ? ? strnicmp(buf, "1", 1) == 0) { >> + ? ? ? ? ? ? debug = 1; >> + ? ? } else if (strnicmp(buf, "off", 3) == 0 || >> + ? ? ? ? ? ? ? ?strnicmp(buf, "0", 1) == 0) { >> + ? ? ? ? ? ? debug = 0; >> + >> + ? ? } else { >> + ? ? ? ? ? ? return -EINVAL; >> + ? ? } >> + >> + ? ? return len; >> +} > > Duplicates the above-mentioned dynamic-debug facility. > >> +static DEVICE_ATTR(debug, 0666, nuc900fb_debug_show, nuc900fb_debug_store); >> + >> >> ... >> >> +static int __init nuc900fb_map_video_memory(struct fb_info *info) >> +{ >> + ? ? struct nuc900fb_info *fbi = info->par; >> + ? ? dma_addr_t map_dma; >> + ? ? unsigned long map_size = PAGE_ALIGN(info->fix.smem_len); >> + >> + ? ? dprintk("nuc900fb_map_video_memory(fbi=%p) map_size %lu\n", >> + ? ? ? ? ? ? fbi, map_size); >> + >> + ? ? info->screen_base = dma_alloc_writecombine(fbi->dev, map_size, >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? &map_dma, GFP_KERNEL); > > Here, do > > ? ? ? ?if (!info->screen_base) > ? ? ? ? ? ? ? ?return -ENOMEM; > > and the rest of the function gets neater. > > >> + ? ? if (info->screen_base != NULL) { >> + ? ? ? ? ? ? dprintk("nuc900fb_map_video_memory: clear %p:%08lx\n", >> + ? ? ? ? ? ? ? ? ? ? info->screen_base, map_size); >> + ? ? ? ? ? ? memset(info->screen_base, 0x00, map_size); >> + >> + ? ? ? ? ? ? info->fix.smem_start = map_dma; >> + ? ? ? ? ? ? dprintk("nuc900fb_map_video_memory: " >> + ? ? ? ? ? ? ? ? ? ? "dma=%08lx cpu=%p size=%08lx\n", >> + ? ? ? ? ? ? ? ? ? ? info->fix.smem_start, info->screen_base, map_size); >> + ? ? } >> + ? ? return (info->screen_base) ? 0 : -ENOMEM; >> +} >> + >> >> ... >> >> +static char driver_name[] = "nuc900fb"; >> + >> +static int __init nuc900fb_probe(struct platform_device *pdev) > > Should this be __devinit? > >> +{ >> + ? ? struct nuc900fb_info *fbi; >> + ? ? struct nuc900fb_display *display; >> + ? ? struct fb_info ? ? *fbinfo; >> + ? ? struct nuc900fb_mach_info *mach_info; >> + ? ? struct resource *res; >> + ? ? int ret; >> + ? ? int irq; >> + ? ? int i; >> + ? ? int size; >> + >> + ? ? dprintk("devinit\n"); >> + ? ? mach_info = pdev->dev.platform_data; >> + ? ? if (mach_info == NULL) { >> + ? ? ? ? ? ? dev_err(&pdev->dev, >> + ? ? ? ? ? ? ? ? ? ? "no platform data for lcd, cannot attach\n"); >> + ? ? ? ? ? ? return -EINVAL; >> + ? ? } >> + >> + ? ? if (mach_info->default_display > mach_info->num_displays) { >> + ? ? ? ? ? ? dev_err(&pdev->dev, >> + ? ? ? ? ? ? ? ? ? ? "default display No. is %d but only %d displays \n", >> + ? ? ? ? ? ? ? ? ? ? mach_info->default_display, mach_info->num_displays); >> + ? ? ? ? ? ? return -EINVAL; >> + ? ? } >> + >> + >> + ? ? display = mach_info->displays + mach_info->default_display; >> + >> + ? ? irq = platform_get_irq(pdev, 0); >> + ? ? if (irq < 0) { >> + ? ? ? ? ? ? dev_err(&pdev->dev, "no irq for device\n"); >> + ? ? ? ? ? ? return -ENOENT; >> + ? ? } >> + >> + ? ? fbinfo = framebuffer_alloc(sizeof(struct nuc900fb_info), &pdev->dev); >> + ? ? if (!fbinfo) >> + ? ? ? ? ? ? return -ENOMEM; >> + >> + ? ? platform_set_drvdata(pdev, fbinfo); >> + >> + ? ? fbi = fbinfo->par; >> + ? ? fbi->dev = &pdev->dev; >> + >> +#ifdef CONFIG_CPU_NUC950 >> + ? ? fbi->drv_type = LCDDRV_NUC950; >> +#endif >> + >> + ? ? res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + >> >> ... >> >> +static void nuc900fb_stop_lcd(struct fb_info *info) >> +{ >> + ? ? struct nuc900fb_info *fbi = info->par; >> + ? ? void __iomem *regs = fbi->io; >> + ? ? unsigned long flags; >> + >> + ? ? local_irq_save(flags); >> + ? ? writel((~LCM_DCCS_DISP_INT_EN) | (~LCM_DCCS_VA_EN) | (~LCM_DCCS_OSD_EN), >> + ? ? ? ? ? ? regs + REG_LCM_DCCS); >> + ? ? local_irq_restore(flags); >> +} > > The local_irq_save() is a worry. ?What's it doing here? ?It does > nothing to prevent an interrupt on another CPU! > >> +/* >> + * ?Cleanup >> + */ >> +static int nuc900fb_remove(struct platform_device *pdev) >> +{ >> + ? ? struct fb_info *fbinfo = platform_get_drvdata(pdev); >> + ? ? struct nuc900fb_info *fbi = fbinfo->par; >> + ? ? int irq; >> + >> + ? ? nuc900fb_stop_lcd(fbinfo); >> + ? ? msleep(1); >> + >> + ? ? nuc900fb_unmap_video_memory(fbinfo); >> + >> + ? ? iounmap(fbi->io); >> + >> + ? ? irq = platform_get_irq(pdev, 0); >> + ? ? free_irq(irq, fbi); >> + >> + ? ? release_resource(fbi->mem); >> + ? ? kfree(fbi->mem); >> + >> + ? ? platform_set_drvdata(pdev, NULL); >> + ? ? framebuffer_release(fbinfo); >> + >> + ? ? return 0; >> +} >> + >> >> ... >> > > The driver generally looks OK to me. ?Please cc me on any updates. > Thanks for your comments. I will update my patch as soon as possible. > -- > ????"NUC900 Linux BSP????". > NUC900 Linux???????????? > NUC900 at googlegroups.com > ????????? > NUC900+unsubscribe at googlegroups.com > ?????????? > https://groups.google.com/group/NUC900?hl=zh-CN >