? 2010?1?30? ??8:27?Andrew Morton [off-list ref] ???

On Mon, 11 Jan 2010 17:05:44 +0900
Wang Qiang [off-list ref] wrote:

quoted

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.

quoted

+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.

quoted

+ ? ? }
+};
+

...

+ */
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/string.h>
+#include <linux/mm.h>
+#include <linux/tty.h>
+#include <linux/slab.h>
+#include <linux/delay.h>
+#include <linux/fb.h>
+#include <linux/init.h>
+#include <linux/dma-mapping.h>
+#include <linux/interrupt.h>
+#include <linux/workqueue.h>
+#include <linux/wait.h>
+#include <linux/platform_device.h>
+#include <linux/clk.h>
+#include <linux/cpufreq.h>
+#include <linux/io.h>
+
+#ifdef CONFIG_PM

Is this ifdef needed?

quoted

+#include <linux/pm.h>
+#endif
+
+#include <mach/map.h>
+#include <mach/regs-clock.h>
+#include <mach/regs-ldm.h>
+#include <mach/fb.h>
+#include <mach/clkdev.h>
+
+#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.

quoted

+
+/*
+ * ?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.

quoted

+ ? ? 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 */

quoted

+ ? ? 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.

quoted

+/*
+ * ? 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.

quoted

+ ? ? 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().

quoted

+ ? ? ? ? ? ? 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.

quoted

+ ? ? 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.

quoted

+ ? ? 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.

quoted

+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.

quoted

+ ? ? 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?

quoted

+{
+ ? ? 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!

quoted

+/*
+ * ?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