Menu
▾
▴
Re: [Linux-fbdev-devel] [PATCH 1/2] ARM S3C2410 framebuffer driver
|
From: Arnaud P. (R. <arn...@rt...> - 2005-09-04 20:57:38
|
"Antonino A. Daplas" <ad...@gm...> writes:
> Arnaud Patard (Rtp) wrote:
>> Hi,
>>
>> This set of two patches add support for the framebuffer of the Samsung
>> S3C2410 ARM SoC. This driver was started about one year ago and is now
>> used on iPAQ h1930/h1940, Acer n30 and probably other s3c2410-based
>> machines I'm not aware of. I've also heard yesterday that it's working also
>> on iPAQ rx3715/rx3115 (s3c2440-based machines).
>>
>> Theses patches are against current linux-2.6 git tree.
>> The first one add the driver by itself along with Kconfig, Makefile and
>> some headers. The second one add the plateform specific stuff needed to
>> configure and use the driver.
>>
>
> Do the ARM people know about this driver?
>
ARM People that have s3c2410 plateforms know it. Moreover, Ben Dooks, the
maintainer of this ARM architecture, helped me in writing this driver.
>> Regards,
>> Arnaud Patard
>>
>>
>> Signed-Off-By: Arnaud Patard <arn...@rt...>
>>
>
> <snip>
>
>> +
>> +static inline struct s3c2410fb_info *fb_to_s3cfb(struct fb_info *info)
>> +{
>> + return container_of(info, struct s3c2410fb_info, fb);
>> +}
>> +
>
> Instead of the private structure containing struct fb_info,
> any reason why the private data can't be placed in fb_info->par?...
>
>> + * s3c2410fb_check_var():
>> + * Get the video params out of 'var'. If a value doesn't fit, round it up,
>> + * if it's too big, return -EINVAL.
>> + *
>> + */
>> +static int s3c2410fb_check_var(struct fb_var_screeninfo *var,
>> + struct fb_info *info)
>> +{
>> + struct s3c2410fb_info *fbi = fb_to_s3cfb(info);
>> +
>
> ... and do it like this instead:
>
> struct s3c2410fb_info *fbi = info->par;
>
Corrected.
>> +
>> + if (var->bits_per_pixel == 16) {
>> + var->red.offset = 11;
>> + var->green.offset = 5;
>> + var->blue.offset = 0;
>> + var->red.length = 5;
>> + var->green.length = 6;
>> + var->blue.length = 5;
>> + var->transp.length = 0;
>> + } else {
>> + var->red.length = 8;
>> + var->red.offset = 0;
>> + var->green.length = 0;
>> + var->green.offset = 8;
>
> swapped green.length and green.offset?
>
hm.. I had corrected that some time ago.. It seems I forgot to merge the
patch in my git tree. New version is with patch applied.
>> + var->blue.length = 8;
>> + var->blue.offset = 0;
>> + var->transp.length = 0;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +/* s3c2410fb_activate_var
>> + *
>> + * activate (set) the controller from the given framebuffer
>> + * information
>> +*/
>> +
>> +static int s3c2410fb_activate_var(struct s3c2410fb_info *fbi,
>> + struct fb_var_screeninfo *var)
>> +{
>> + fbi->regs.lcdcon1 &= ~S3C2410_LCDCON1_MODEMASK;
>> +
>> + dprintk("%s: var->xres = %d\n", __FUNCTION__, var->xres);
>> + dprintk("%s: var->yres = %d\n", __FUNCTION__, var->yres);
>> + dprintk("%s: var->bpp = %d\n", __FUNCTION__, var->bits_per_pixel);
>> +
>> + switch (var->bits_per_pixel) {
>> + case 1:
>> + fbi->regs.lcdcon1 |= S3C2410_LCDCON1_TFT1BPP;
>> + break;
>> + case 2:
>> + fbi->regs.lcdcon1 |= S3C2410_LCDCON1_TFT2BPP;
>> + break;
>> + case 4:
>> + fbi->regs.lcdcon1 |= S3C2410_LCDCON1_TFT4BPP;
>> + break;
>> + case 8:
>> + fbi->regs.lcdcon1 |= S3C2410_LCDCON1_TFT8BPP;
>> + break;
>> + case 16:
>> + fbi->regs.lcdcon1 |= S3C2410_LCDCON1_TFT16BPP;
>> + break;
>> +
>> + default:
>> + /* invalid pixel depth */
>> + dev_err(fbi->dev, "invalid bpp %d\n", var->bits_per_pixel);
>
> You won't need to have an error path here if you round up bits_per_pixel
> in the check_var(). info->var will always contain a valid var as it would
> always go through the driver's check_var().
>
This was for debugging purpose and no more really needed. Removed.
>> +static int s3c2410fb_setcolreg(unsigned regno,
>> + unsigned red, unsigned green, unsigned blue,
>> + unsigned transp, struct fb_info *info)
>> +{
>> + struct s3c2410fb_info *fbi = (struct s3c2410fb_info *)info;
>> + unsigned int val;
>> +
>> + /* dprintk("setcol: regno=%d, rgb=%d,%d,%d\n", regno, red, green, blue); */
>> +
>> + switch (fbi->fb.fix.visual) {
>> + case FB_VISUAL_TRUECOLOR:
>> + /* true-colour, use pseuo-palette */
>> +
>> + if (regno < 16) {
>> + u32 *pal = fbi->fb.pseudo_palette;
>> +
>> + val = chan_to_field(red, &fbi->fb.var.red);
>> + val |= chan_to_field(green, &fbi->fb.var.green);
>> + val |= chan_to_field(blue, &fbi->fb.var.blue);
>> +
>> + pal[regno] = val;
>> + }
>
> Missing break statement?
You're right...
>> +
>> + case FB_VISUAL_STATIC_PSEUDOCOLOR:
>
> static pseudocolor does not have a modifiable CLUT. And I don't see
> your driver supporting static pseudocolor.
>
you're right. removed.
> Similarly, truecolor cannot have a modifiable clut, that's why you need
> the 'break' above. If you think your driver can modify the clut at
> 16 bpp, then advertise the visual as directcolor.
>
> The above may seem nitpicking, but it can confuse user apps if the
> driver incorrectly advertises its capability.
>
>> + case FB_VISUAL_PSEUDOCOLOR:
>> + if (regno < 256) {
>> + /* currently assume RGB 5-6-5 mode */
>> +
>> + val = ((red >> 0) & 0xf800);
>> + val |= ((green >> 5) & 0x07e0);
>> + val |= ((blue >> 11) & 0x001f);
>> +
>> + writel(val, S3C2410_TFTPAL(regno));
>> + schedule_palette_update(fbi, regno, val);
>> + }
>> +
>> + break;
>> +
>> + default:
>> + return 1; /* unknown type */
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +
>> +/**
>> + * s3c2410fb_pan_display
>> + * @var: frame buffer variable screen structure
>> + * @info: frame buffer structure that represents a single frame buffer
>> + *
>> + * Pan (or wrap, depending on the `vmode' field) the display using the
>> + * `xoffset' and `yoffset' fields of the `var' structure.
>> + * If the values don't fit, return -EINVAL.
>> + *
>> + * Returns negative errno on error, or zero on success.
>> + */
>> +static int s3c2410fb_pan_display(struct fb_var_screeninfo *var,
>> + struct fb_info *info)
>> +{
>> + dprintk("pan_display(var=%p, info=%p)\n", var, info);
>> +
>> + dprintk("pan_display: xoffset=%d\n", var->xoffset);
>> + dprintk("pan_display: yoffset=%d\n", var->yoffset);
>> +
>> + return 0;
>> +}
>
> Just comment out pan_display if it's not doing anything. You'll
> just confuse apps that do multi-buffering.
>
Okay. Corrected.
>> +
>> +
>> +/* Fake monspecs to fill in fbinfo structure */
>> +/* Don't know if the values are important */
>
> The values in monspecs are important if the driver
> uses it. Otherwise, you need not fake a monspecs.
>
Thanks. I didn't know that.
>> +int __init s3c2410fb_probe(struct device *dev)
>> +{
>> + struct platform_device *pdev = to_platform_device(dev);
>> + struct s3c2410fb_hw *mregs;
>> + int ret;
>> + int irq;
>> + int i;
>> +
>> + mach_info = dev->platform_data;
>> + if (mach_info == NULL) {
>> + dev_err(dev,"no platform data for lcd, cannot attach\n");
>> + return -EINVAL;
>> + }
>> +
>> + mregs = &mach_info->regs;
>> +
>> + irq = platform_get_irq(pdev, 0);
>> + if (irq < 0) {
>> + dev_err(dev, "no irq for device\n");
>> + return -ENOENT;
>> + }
>> +
>> + s3c2410fb_init_registers(&info);
>> +
>> + dprintk("devinit\n");
>> +
>> + strcpy(info.fb.fix.id, driver_name);
>> +
>> + memcpy(&info.regs, &mach_info->regs, sizeof(info.regs));
>> +
>> + info.mach_info = dev->platform_data;
>> +
>> + info.fb.fix.type = FB_TYPE_PACKED_PIXELS;
>> + info.fb.fix.type_aux = 0;
>> + info.fb.fix.xpanstep = 0;
>> + info.fb.fix.ypanstep = 0;
>> + info.fb.fix.ywrapstep = 0;
>> + info.fb.fix.accel = FB_ACCEL_NONE;
>> +
>> + info.fb.var.nonstd = 0;
>> + info.fb.var.activate = FB_ACTIVATE_NOW;
>> + info.fb.var.height = mach_info->height;
>> + info.fb.var.width = mach_info->width;
>> + info.fb.var.accel_flags = 0;
>> + info.fb.var.vmode = FB_VMODE_NONINTERLACED;
>> +
>> + info.fb.fbops = &s3c2410fb_ops;
>> + info.fb.flags = FBINFO_FLAG_DEFAULT;
>> + info.fb.monspecs = monspecs;
>> + info.fb.pseudo_palette = &info.pseudo_pal;
>> +
>> + info.fb.var.xres = mach_info->xres.defval;
>> + info.fb.var.xres_virtual = mach_info->xres.defval;
>> + info.fb.var.yres = mach_info->yres.defval;
>> + info.fb.var.yres_virtual = mach_info->yres.defval;
>> + info.fb.var.bits_per_pixel = mach_info->bpp.defval;
>> +
>> + info.fb.var.upper_margin = S3C2410_LCDCON2_GET_VBPD(mregs->lcdcon2) +1;
>> + info.fb.var.lower_margin = S3C2410_LCDCON2_GET_VFPD(mregs->lcdcon2) +1;
>> + info.fb.var.vsync_len = S3C2410_LCDCON2_GET_VSPW(mregs->lcdcon2) + 1;
>> +
>> + info.fb.var.left_margin = S3C2410_LCDCON3_GET_HFPD(mregs->lcdcon3) + 1;
>> + info.fb.var.right_margin = S3C2410_LCDCON3_GET_HBPD(mregs->lcdcon3) + 1;
>> + info.fb.var.hsync_len = S3C2410_LCDCON4_GET_HSPW(mregs->lcdcon4) + 1;
>> +
>> + info.fb.var.red.offset = 11;
>> + info.fb.var.green.offset = 5;
>> + info.fb.var.blue.offset = 0;
>> + info.fb.var.transp.offset = 0;
>> + info.fb.var.red.length = 5;
>> + info.fb.var.green.length = 6;
>> + info.fb.var.blue.length = 5;
>> + info.fb.var.transp.length = 0;
>> + info.fb.fix.smem_len = mach_info->xres.max *
>> + mach_info->yres.max *
>> + mach_info->bpp.max / 8;
>> +
>
> You may need to allocate info.fb.cmap also (with fb_alloc_cmap(),
> and check its return).
>
> And you can set info.fb.device = dev so your driver will become
> visible in sysfs.
>
> (The standard method is to use framebuffer_alloc() to allocate
> struct fb_info and framebuffer_release() to free. You may have your own
> reasons not to use them, so it's okay).
>
Okay. Rewritten to use framebuffer_alloc()
>
>> +/*
>> + * Cleanup
>> + */
>> +static void __exit s3c2410fb_cleanup(void)
>> +{
>> + s3c2410fb_stop_lcd();
>> + msleep(1);
>> +
>> + if (info.clk) {
>> + clk_disable(info.clk);
>> + clk_unuse(info.clk);
>> + clk_put(info.clk);
>> + info.clk = NULL;
>> + }
>> +
>
> and deallocate your info.fb.cmap here (with fb_dealloc_cmap())
>
>> + unregister_framebuffer(&info.fb);
>> + release_mem_region((unsigned long)S3C24XX_VA_LCD, S3C24XX_SZ_LCD);
>> +}
>> +
>
> Tony
New version attached.
Regards,
Arnaud
Signed-Off-By: Arnaud Patard <arn...@rt...>
---
|