* [PATCH] Trivial patch in efifb.c to solve common indent issue and indent error
From: Parmeshwr Prasad @ 2015-02-09 12:55 UTC (permalink / raw)
To: pjones-H+wXaHxf7aLQT0dZR+AlfA, plagnioj-sclMFOaUSTBWk0Htik3J/w,
tomi.valkeinen-l0cyMroinI0
Cc: linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-efi-u79uwXL29TY76Z2rM5mHXA
Hi All,
Please review this patch.
this patch is aimed to solve some indentation issue. It has also solved
three trivial error in efifb.c file.
And I have also changed printk with pr_err, pr_info ... at respective places.
From c49139fac1d15fe2da80d06e2a79eb8be7c079a7 Mon Sep 17 00:00:00 2001
From: Parmeshwr Prasad <parmeshwr_prasad@dell.com>
Date: Mon, 9 Feb 2015 07:33:59 -0500
Subject: [PATCH] Trival patch: improved indentation, and removed some ERROR
from code
Signed-off-by: Parmeshwr Prasad <parmeshwr_prasad@dell.com>
---
drivers/video/fbdev/efifb.c | 158 +++++++++++++++++++++++---------------------
1 file changed, 84 insertions(+), 74 deletions(-)
diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
index 4bfff34..5c03035 100644
--- a/drivers/video/fbdev/efifb.c
+++ b/drivers/video/fbdev/efifb.c
@@ -17,29 +17,28 @@
#include <video/vga.h>
#include <asm/sysfb.h>
-static bool request_mem_succeeded = false;
+static bool request_mem_succeeded;
static struct fb_var_screeninfo efifb_defined = {
- .activate = FB_ACTIVATE_NOW,
- .height = -1,
- .width = -1,
- .right_margin = 32,
- .upper_margin = 16,
- .lower_margin = 4,
- .vsync_len = 4,
- .vmode = FB_VMODE_NONINTERLACED,
+ .activate = FB_ACTIVATE_NOW,
+ .height = -1,
+ .width = -1,
+ .right_margin = 32,
+ .upper_margin = 16,
+ .lower_margin = 4,
+ .vsync_len = 4,
+ .vmode = FB_VMODE_NONINTERLACED,
};
static struct fb_fix_screeninfo efifb_fix = {
- .id = "EFI VGA",
- .type = FB_TYPE_PACKED_PIXELS,
- .accel = FB_ACCEL_NONE,
- .visual = FB_VISUAL_TRUECOLOR,
+ .id = "EFI VGA",
+ .type = FB_TYPE_PACKED_PIXELS,
+ .accel = FB_ACCEL_NONE,
+ .visual = FB_VISUAL_TRUECOLOR,
};
static int efifb_setcolreg(unsigned regno, unsigned red, unsigned green,
- unsigned blue, unsigned transp,
- struct fb_info *info)
+ unsigned blue, unsigned transp, struct fb_info *info)
{
/*
* Set a single color register. The values supplied are
@@ -52,13 +51,13 @@ static int efifb_setcolreg(unsigned regno, unsigned red,
unsigned green,
return 1;
if (regno < 16) {
- red >>= 8;
+ red >>= 8;
green >>= 8;
- blue >>= 8;
- ((u32 *)(info->pseudo_palette))[regno] - (red << info->var.red.offset) |
- (green << info->var.green.offset) |
- (blue << info->var.blue.offset);
+ blue >>= 8;
+ ((u32 *) (info->pseudo_palette))[regno] + (red << info->var.red.offset) |
+ (green << info->var.green.offset) |
+ (blue << info->var.blue.offset);
}
return 0;
}
@@ -74,12 +73,12 @@ static void efifb_destroy(struct fb_info *info)
}
static struct fb_ops efifb_ops = {
- .owner = THIS_MODULE,
- .fb_destroy = efifb_destroy,
- .fb_setcolreg = efifb_setcolreg,
- .fb_fillrect = cfb_fillrect,
- .fb_copyarea = cfb_copyarea,
- .fb_imageblit = cfb_imageblit,
+ .owner = THIS_MODULE,
+ .fb_destroy = efifb_destroy,
+ .fb_setcolreg = efifb_setcolreg,
+ .fb_fillrect = cfb_fillrect,
+ .fb_copyarea = cfb_copyarea,
+ .fb_imageblit = cfb_imageblit,
};
static int efifb_setup(char *options)
@@ -89,25 +88,35 @@ static int efifb_setup(char *options)
if (options && *options) {
while ((this_opt = strsep(&options, ",")) != NULL) {
- if (!*this_opt) continue;
+ if (!*this_opt)
+ continue;
for (i = 0; i < M_UNKNOWN; i++) {
if (efifb_dmi_list[i].base != 0 &&
- !strcmp(this_opt,
efifb_dmi_list[i].optname)) {
- screen_info.lfb_base efifb_dmi_list[i].base;
- screen_info.lfb_linelength efifb_dmi_list[i].stride;
- screen_info.lfb_width efifb_dmi_list[i].width;
- screen_info.lfb_height efifb_dmi_list[i].height;
+ !strcmp(this_opt,
+ efifb_dmi_list[i].optname)) {
+ screen_info.lfb_base + efifb_dmi_list[i].base;
+ screen_info.lfb_linelength + efifb_dmi_list[i].stride;
+ screen_info.lfb_width + efifb_dmi_list[i].width;
+ screen_info.lfb_height + efifb_dmi_list[i].height;
}
}
if (!strncmp(this_opt, "base:", 5))
- screen_info.lfb_base simple_strtoul(this_opt+5, NULL, 0);
+ screen_info.lfb_base + simple_strtoul(this_opt + 5, NULL, 0);
else if (!strncmp(this_opt, "stride:", 7))
- screen_info.lfb_linelength simple_strtoul(this_opt+7, NULL, 0) * 4;
+ screen_info.lfb_linelength + simple_strtoul(this_opt + 7, NULL, 0) * 4;
else if (!strncmp(this_opt, "height:", 7))
- screen_info.lfb_height simple_strtoul(this_opt+7, NULL, 0);
+ screen_info.lfb_height + simple_strtoul(this_opt + 7, NULL, 0);
else if (!strncmp(this_opt, "width:", 6))
- screen_info.lfb_width simple_strtoul(this_opt+6, NULL, 0);
+ screen_info.lfb_width + simple_strtoul(this_opt + 6, NULL, 0);
}
}
@@ -142,10 +151,10 @@ static int efifb_probe(struct platform_device *dev)
if (!screen_info.pages)
screen_info.pages = 1;
if (!screen_info.lfb_base) {
- printk(KERN_DEBUG "efifb: invalid framebuffer address\n");
+ pr_err("efifb: invalid framebuffer address\n");
return -ENODEV;
}
- printk(KERN_INFO "efifb: probing for efifb\n");
+ pr_info("efifb: probing for efifb\n");
/* just assume they're all unset if any are */
if (!screen_info.blue_size) {
@@ -181,7 +190,7 @@ static int efifb_probe(struct platform_device *dev)
* use for efifb. With modern cards it is no
* option to simply use size_total as that
* wastes plenty of kernel address space. */
- size_remap = size_vmode * 2;
+ size_remap = size_vmode * 2;
if (size_remap > size_total)
size_remap = size_total;
if (size_remap % PAGE_SIZE)
@@ -193,14 +202,14 @@ static int efifb_probe(struct platform_device *dev)
} else {
/* We cannot make this fatal. Sometimes this comes from magic
spaces our resource handlers simply don't know about */
- printk(KERN_WARNING
+ pr_warn(
"efifb: cannot reserve video memory at 0x%lx\n",
- efifb_fix.smem_start);
+ efifb_fix.smem_start);
}
info = framebuffer_alloc(sizeof(u32) * 16, &dev->dev);
if (!info) {
- printk(KERN_ERR "efifb: cannot allocate framebuffer\n");
+ pr_err("efifb: cannot allocate framebuffer\n");
err = -ENOMEM;
goto err_release_mem;
}
@@ -216,46 +225,46 @@ static int efifb_probe(struct platform_device *dev)
info->apertures->ranges[0].base = efifb_fix.smem_start;
info->apertures->ranges[0].size = size_remap;
- info->screen_base = ioremap_wc(efifb_fix.smem_start,
efifb_fix.smem_len);
+ info->screen_base + ioremap_wc(efifb_fix.smem_start, efifb_fix.smem_len);
if (!info->screen_base) {
- printk(KERN_ERR "efifb: abort, cannot ioremap video memory "
- "0x%x @ 0x%lx\n",
- efifb_fix.smem_len, efifb_fix.smem_start);
+ pr_err("efifb: abort, cannot ioremap video memory "
+ "0x%x @ 0x%lx\n",
+ efifb_fix.smem_len, efifb_fix.smem_start);
err = -EIO;
goto err_release_fb;
}
- printk(KERN_INFO "efifb: framebuffer at 0x%lx, mapped to 0x%p, "
+ pr_info("efifb: framebuffer at 0x%lx, mapped to 0x%p, "
"using %dk, total %dk\n",
efifb_fix.smem_start, info->screen_base,
- size_remap/1024, size_total/1024);
- printk(KERN_INFO "efifb: mode is %dx%dx%d, linelength=%d, pages=%d\n",
+ size_remap / 1024, size_total / 1024);
+ pr_info("efifb: mode is %dx%dx%d, linelength=%d, pages=%d\n",
efifb_defined.xres, efifb_defined.yres,
efifb_defined.bits_per_pixel, efifb_fix.line_length,
screen_info.pages);
efifb_defined.xres_virtual = efifb_defined.xres;
- efifb_defined.yres_virtual = efifb_fix.smem_len /
- efifb_fix.line_length;
- printk(KERN_INFO "efifb: scrolling: redraw\n");
+ efifb_defined.yres_virtual = efifb_fix.smem_len / efifb_fix.line_length;
+ pr_info("efifb: scrolling: redraw\n");
efifb_defined.yres_virtual = efifb_defined.yres;
/* some dummy values for timing to make fbset happy */
- efifb_defined.pixclock = 10000000 / efifb_defined.xres *
- 1000 / efifb_defined.yres;
- efifb_defined.left_margin = (efifb_defined.xres / 8) & 0xf8;
- efifb_defined.hsync_len = (efifb_defined.xres / 8) & 0xf8;
-
- efifb_defined.red.offset = screen_info.red_pos;
- efifb_defined.red.length = screen_info.red_size;
- efifb_defined.green.offset = screen_info.green_pos;
- efifb_defined.green.length = screen_info.green_size;
- efifb_defined.blue.offset = screen_info.blue_pos;
- efifb_defined.blue.length = screen_info.blue_size;
+ efifb_defined.pixclock = 10000000 / efifb_defined.xres *
+ 1000 / efifb_defined.yres;
+ efifb_defined.left_margin = (efifb_defined.xres / 8) & 0xf8;
+ efifb_defined.hsync_len = (efifb_defined.xres / 8) & 0xf8;
+
+ efifb_defined.red.offset = screen_info.red_pos;
+ efifb_defined.red.length = screen_info.red_size;
+ efifb_defined.green.offset = screen_info.green_pos;
+ efifb_defined.green.length = screen_info.green_size;
+ efifb_defined.blue.offset = screen_info.blue_pos;
+ efifb_defined.blue.length = screen_info.blue_size;
efifb_defined.transp.offset = screen_info.rsvd_pos;
efifb_defined.transp.length = screen_info.rsvd_size;
- printk(KERN_INFO "efifb: %s: "
+ pr_info("efifb: %s: "
"size=%d:%d:%d:%d, shift=%d:%d:%d:%d\n",
"Truecolor",
screen_info.rsvd_size,
@@ -264,10 +273,9 @@ static int efifb_probe(struct platform_device *dev)
screen_info.blue_size,
screen_info.rsvd_pos,
screen_info.red_pos,
- screen_info.green_pos,
- screen_info.blue_pos);
+ screen_info.green_pos, screen_info.blue_pos);
- efifb_fix.ypanstep = 0;
+ efifb_fix.ypanstep = 0;
efifb_fix.ywrapstep = 0;
info->fbops = &efifb_ops;
@@ -275,12 +283,14 @@ static int efifb_probe(struct platform_device *dev)
info->fix = efifb_fix;
info->flags = FBINFO_FLAG_DEFAULT | FBINFO_MISC_FIRMWARE;
- if ((err = fb_alloc_cmap(&info->cmap, 256, 0)) < 0) {
- printk(KERN_ERR "efifb: cannot allocate colormap\n");
+ err = fb_alloc_cmap(&info->cmap, 256, 0);
+ if ((err < 0) {
+ pr_err("efifb: cannot allocate colormap\n");
goto err_unmap;
}
- if ((err = register_framebuffer(info)) < 0) {
- printk(KERN_ERR "efifb: cannot register framebuffer\n");
+ err = register_framebuffer(info);
+ if (err < 0) {
+ pr_err("efifb: cannot register framebuffer\n");
goto err_fb_dealoc;
}
fb_info(info, "%s frame buffer device\n", info->fix.id);
@@ -310,8 +320,8 @@ static int efifb_remove(struct platform_device *pdev)
static struct platform_driver efifb_driver = {
.driver = {
- .name = "efi-framebuffer",
- },
+ .name = "efi-framebuffer",
+ },
.probe = efifb_probe,
.remove = efifb_remove,
};
--
1.9.3
^ permalink raw reply related
* Re: [PATCH] Trivial patch in efifb.c to solve common indent issue and indent error
From: Lad, Prabhakar @ 2015-02-09 14:24 UTC (permalink / raw)
To: Parmeshwr Prasad
Cc: pjones-H+wXaHxf7aLQT0dZR+AlfA, Jean-Christophe Plagniol-Villard,
Tomi Valkeinen, LFBDEV, LKML, linux-efi-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20150209125548.GA27277-uSknhY0EtF8ok8EplQz5xE1Vg53cnbg30E9HWUfgJXw@public.gmane.org>
Hi,
Thanks for the patch.
On Mon, Feb 9, 2015 at 12:55 PM, Parmeshwr Prasad
<parmeshwr_prasad@dell.com> wrote:
> Hi All,
>
> Please review this patch.
> this patch is aimed to solve some indentation issue. It has also solved
> three trivial error in efifb.c file.
>
> And I have also changed printk with pr_err, pr_info ... at respective places.
>
>
> From c49139fac1d15fe2da80d06e2a79eb8be7c079a7 Mon Sep 17 00:00:00 2001
> From: Parmeshwr Prasad <parmeshwr_prasad@dell.com>
> Date: Mon, 9 Feb 2015 07:33:59 -0500
> Subject: [PATCH] Trival patch: improved indentation, and removed some ERROR
> from code
>
> Signed-off-by: Parmeshwr Prasad <parmeshwr_prasad@dell.com>
> ---
1: did you use git to send this patch ?
2: did you checkpatch it (I see some issues)?
3: break this patch into 2 one fixing the 3 issues and other fixing
the indentation.
4: have proper commit message.
Cheers,
--Prabhakar Lad
^ permalink raw reply
* Re: [PATCH1/2] Trivial patch in efifb.c to solve common indent issue and indent error
From: Parmeshwr Prasad @ 2015-02-10 8:06 UTC (permalink / raw)
To: Lad, Prabhakar
Cc: pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org,
Jean-Christophe Plagniol-Villard, Tomi Valkeinen, LFBDEV, LKML,
linux-efi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <CA+V-a8todzzHHTSaJ1UzT+1sXrdbNaaBah66gFr2kEigNBhQYw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
I have created two patch now.
0001-Trival-patch-improved-indentation-in-efifb.c-file.patch:
This patch solves indentation issue in efifb.c file.
This don't has any code change.
I checked with checkpatch.pl it has some warning to remove some old functions in
efifb.c. my changes are not to address that. That changes I can try in my next
patch.
0002-Trivial-patch-In-this-patch-printk-is-replaced-by-pr.patch:
This is addressing following changes:
1- It has removed "quoted string split across lines" warning.
2- There was static initialization of request_mem_succeeded, which is removed.
3- Assignment in if condition, this is fixed.
This one I have checked with checkpatch.pl it don't has any warning or comment.
I am sending these patches with mutt.
From 8ce800b5f5a048109014994bcfc4fae2ef9cc271 Mon Sep 17 00:00:00 2001
From: Parmeshwr Prasad <parmeshwr_prasad@dell.com>
Date: Tue, 10 Feb 2015 00:33:30 -0500
Subject: [PATCH] Trival patch: improved indentation in efifb.c file
Signed-off-by: Parmeshwr Prasad <parmeshwr_prasad@dell.com>
---
drivers/video/fbdev/efifb.c | 128 +++++++++++++++++++++++---------------------
1 file changed, 68 insertions(+), 60 deletions(-)
diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
index 4bfff34..d757d0f 100644
--- a/drivers/video/fbdev/efifb.c
+++ b/drivers/video/fbdev/efifb.c
@@ -20,26 +20,25 @@
static bool request_mem_succeeded = false;
static struct fb_var_screeninfo efifb_defined = {
- .activate = FB_ACTIVATE_NOW,
- .height = -1,
- .width = -1,
- .right_margin = 32,
- .upper_margin = 16,
- .lower_margin = 4,
- .vsync_len = 4,
- .vmode = FB_VMODE_NONINTERLACED,
+ .activate = FB_ACTIVATE_NOW,
+ .height = -1,
+ .width = -1,
+ .right_margin = 32,
+ .upper_margin = 16,
+ .lower_margin = 4,
+ .vsync_len = 4,
+ .vmode = FB_VMODE_NONINTERLACED,
};
static struct fb_fix_screeninfo efifb_fix = {
- .id = "EFI VGA",
- .type = FB_TYPE_PACKED_PIXELS,
- .accel = FB_ACCEL_NONE,
- .visual = FB_VISUAL_TRUECOLOR,
+ .id = "EFI VGA",
+ .type = FB_TYPE_PACKED_PIXELS,
+ .accel = FB_ACCEL_NONE,
+ .visual = FB_VISUAL_TRUECOLOR,
};
static int efifb_setcolreg(unsigned regno, unsigned red, unsigned green,
- unsigned blue, unsigned transp,
- struct fb_info *info)
+ unsigned blue, unsigned transp, struct fb_info *info)
{
/*
* Set a single color register. The values supplied are
@@ -52,13 +51,13 @@ static int efifb_setcolreg(unsigned regno, unsigned red,
unsigned green,
return 1;
if (regno < 16) {
- red >>= 8;
+ red >>= 8;
green >>= 8;
- blue >>= 8;
- ((u32 *)(info->pseudo_palette))[regno] - (red << info->var.red.offset) |
- (green << info->var.green.offset) |
- (blue << info->var.blue.offset);
+ blue >>= 8;
+ ((u32 *) (info->pseudo_palette))[regno] + (red << info->var.red.offset) |
+ (green << info->var.green.offset) |
+ (blue << info->var.blue.offset);
}
return 0;
}
@@ -74,12 +73,12 @@ static void efifb_destroy(struct fb_info *info)
}
static struct fb_ops efifb_ops = {
- .owner = THIS_MODULE,
- .fb_destroy = efifb_destroy,
- .fb_setcolreg = efifb_setcolreg,
- .fb_fillrect = cfb_fillrect,
- .fb_copyarea = cfb_copyarea,
- .fb_imageblit = cfb_imageblit,
+ .owner = THIS_MODULE,
+ .fb_destroy = efifb_destroy,
+ .fb_setcolreg = efifb_setcolreg,
+ .fb_fillrect = cfb_fillrect,
+ .fb_copyarea = cfb_copyarea,
+ .fb_imageblit = cfb_imageblit,
};
static int efifb_setup(char *options)
@@ -89,25 +88,35 @@ static int efifb_setup(char *options)
if (options && *options) {
while ((this_opt = strsep(&options, ",")) != NULL) {
- if (!*this_opt) continue;
+ if (!*this_opt)
+ continue;
for (i = 0; i < M_UNKNOWN; i++) {
if (efifb_dmi_list[i].base != 0 &&
- !strcmp(this_opt,
efifb_dmi_list[i].optname)) {
- screen_info.lfb_base efifb_dmi_list[i].base;
- screen_info.lfb_linelength efifb_dmi_list[i].stride;
- screen_info.lfb_width efifb_dmi_list[i].width;
- screen_info.lfb_height efifb_dmi_list[i].height;
+ !strcmp(this_opt,
+ efifb_dmi_list[i].optname)) {
+ screen_info.lfb_base + efifb_dmi_list[i].base;
+ screen_info.lfb_linelength + efifb_dmi_list[i].stride;
+ screen_info.lfb_width + efifb_dmi_list[i].width;
+ screen_info.lfb_height + efifb_dmi_list[i].height;
}
}
if (!strncmp(this_opt, "base:", 5))
- screen_info.lfb_base simple_strtoul(this_opt+5, NULL, 0);
+ screen_info.lfb_base + simple_strtoul(this_opt + 5, NULL, 0);
else if (!strncmp(this_opt, "stride:", 7))
- screen_info.lfb_linelength simple_strtoul(this_opt+7, NULL, 0) * 4;
+ screen_info.lfb_linelength + simple_strtoul(this_opt + 7, NULL, 0) * 4;
else if (!strncmp(this_opt, "height:", 7))
- screen_info.lfb_height simple_strtoul(this_opt+7, NULL, 0);
+ screen_info.lfb_height + simple_strtoul(this_opt + 7, NULL, 0);
else if (!strncmp(this_opt, "width:", 6))
- screen_info.lfb_width simple_strtoul(this_opt+6, NULL, 0);
+ screen_info.lfb_width + simple_strtoul(this_opt + 6, NULL, 0);
}
}
@@ -181,7 +190,7 @@ static int efifb_probe(struct platform_device *dev)
* use for efifb. With modern cards it is no
* option to simply use size_total as that
* wastes plenty of kernel address space. */
- size_remap = size_vmode * 2;
+ size_remap = size_vmode * 2;
if (size_remap > size_total)
size_remap = size_total;
if (size_remap % PAGE_SIZE)
@@ -195,7 +204,7 @@ static int efifb_probe(struct platform_device *dev)
spaces our resource handlers simply don't know about */
printk(KERN_WARNING
"efifb: cannot reserve video memory at 0x%lx\n",
- efifb_fix.smem_start);
+ efifb_fix.smem_start);
}
info = framebuffer_alloc(sizeof(u32) * 16, &dev->dev);
@@ -216,11 +225,12 @@ static int efifb_probe(struct platform_device *dev)
info->apertures->ranges[0].base = efifb_fix.smem_start;
info->apertures->ranges[0].size = size_remap;
- info->screen_base = ioremap_wc(efifb_fix.smem_start,
efifb_fix.smem_len);
+ info->screen_base + ioremap_wc(efifb_fix.smem_start, efifb_fix.smem_len);
if (!info->screen_base) {
printk(KERN_ERR "efifb: abort, cannot ioremap video memory "
- "0x%x @ 0x%lx\n",
- efifb_fix.smem_len, efifb_fix.smem_start);
+ "0x%x @ 0x%lx\n",
+ efifb_fix.smem_len, efifb_fix.smem_start);
err = -EIO;
goto err_release_fb;
}
@@ -228,30 +238,29 @@ static int efifb_probe(struct platform_device *dev)
printk(KERN_INFO "efifb: framebuffer at 0x%lx, mapped to 0x%p, "
"using %dk, total %dk\n",
efifb_fix.smem_start, info->screen_base,
- size_remap/1024, size_total/1024);
+ size_remap / 1024, size_total / 1024);
printk(KERN_INFO "efifb: mode is %dx%dx%d, linelength=%d, pages=%d\n",
efifb_defined.xres, efifb_defined.yres,
efifb_defined.bits_per_pixel, efifb_fix.line_length,
screen_info.pages);
efifb_defined.xres_virtual = efifb_defined.xres;
- efifb_defined.yres_virtual = efifb_fix.smem_len /
- efifb_fix.line_length;
+ efifb_defined.yres_virtual = efifb_fix.smem_len / efifb_fix.line_length;
printk(KERN_INFO "efifb: scrolling: redraw\n");
efifb_defined.yres_virtual = efifb_defined.yres;
/* some dummy values for timing to make fbset happy */
- efifb_defined.pixclock = 10000000 / efifb_defined.xres *
- 1000 / efifb_defined.yres;
- efifb_defined.left_margin = (efifb_defined.xres / 8) & 0xf8;
- efifb_defined.hsync_len = (efifb_defined.xres / 8) & 0xf8;
-
- efifb_defined.red.offset = screen_info.red_pos;
- efifb_defined.red.length = screen_info.red_size;
- efifb_defined.green.offset = screen_info.green_pos;
- efifb_defined.green.length = screen_info.green_size;
- efifb_defined.blue.offset = screen_info.blue_pos;
- efifb_defined.blue.length = screen_info.blue_size;
+ efifb_defined.pixclock = 10000000 / efifb_defined.xres *
+ 1000 / efifb_defined.yres;
+ efifb_defined.left_margin = (efifb_defined.xres / 8) & 0xf8;
+ efifb_defined.hsync_len = (efifb_defined.xres / 8) & 0xf8;
+
+ efifb_defined.red.offset = screen_info.red_pos;
+ efifb_defined.red.length = screen_info.red_size;
+ efifb_defined.green.offset = screen_info.green_pos;
+ efifb_defined.green.length = screen_info.green_size;
+ efifb_defined.blue.offset = screen_info.blue_pos;
+ efifb_defined.blue.length = screen_info.blue_size;
efifb_defined.transp.offset = screen_info.rsvd_pos;
efifb_defined.transp.length = screen_info.rsvd_size;
@@ -264,10 +273,9 @@ static int efifb_probe(struct platform_device *dev)
screen_info.blue_size,
screen_info.rsvd_pos,
screen_info.red_pos,
- screen_info.green_pos,
- screen_info.blue_pos);
+ screen_info.green_pos, screen_info.blue_pos);
- efifb_fix.ypanstep = 0;
+ efifb_fix.ypanstep = 0;
efifb_fix.ywrapstep = 0;
info->fbops = &efifb_ops;
@@ -310,8 +318,8 @@ static int efifb_remove(struct platform_device *pdev)
static struct platform_driver efifb_driver = {
.driver = {
- .name = "efi-framebuffer",
- },
+ .name = "efi-framebuffer",
+ },
.probe = efifb_probe,
.remove = efifb_remove,
};
--
1.9.3
-Parmeshwr
On Mon, Feb 09, 2015 at 08:24:50AM -0600, Lad, Prabhakar wrote:
> Hi,
>
> Thanks for the patch.
>
> On Mon, Feb 9, 2015 at 12:55 PM, Parmeshwr Prasad
> <parmeshwr_prasad@dell.com> wrote:
> > Hi All,
> >
> > Please review this patch.
> > this patch is aimed to solve some indentation issue. It has also solved
> > three trivial error in efifb.c file.
> >
> > And I have also changed printk with pr_err, pr_info ... at respective places.
> >
> >
> > From c49139fac1d15fe2da80d06e2a79eb8be7c079a7 Mon Sep 17 00:00:00 2001
> > From: Parmeshwr Prasad <parmeshwr_prasad@dell.com>
> > Date: Mon, 9 Feb 2015 07:33:59 -0500
> > Subject: [PATCH] Trival patch: improved indentation, and removed some ERROR
> > from code
> >
> > Signed-off-by: Parmeshwr Prasad <parmeshwr_prasad@dell.com>
> > ---
>
> 1: did you use git to send this patch ?
> 2: did you checkpatch it (I see some issues)?
> 3: break this patch into 2 one fixing the 3 issues and other fixing
> the indentation.
> 4: have proper commit message.
>
> Cheers,
> --Prabhakar Lad
^ permalink raw reply related
* Re: [PATCH2/2] Trivial patch in efifb.c to solve common indent issue and indent error
From: Parmeshwr Prasad @ 2015-02-10 8:10 UTC (permalink / raw)
To: Lad, Prabhakar
Cc: pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org,
Jean-Christophe Plagniol-Villard, Tomi Valkeinen, LFBDEV, LKML,
linux-efi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <20150210080614.GA8438-uSknhY0EtF8ok8EplQz5xE1Vg53cnbg30E9HWUfgJXw@public.gmane.org>
On Tue, Feb 10, 2015 at 02:06:24AM -0600, Prasad, Parmeshwr wrote:
This is secong patch.
This is addressing following changes:
1- It has removed "quoted string split across lines" warning.
2- There was static initialization of request_mem_succeeded, which is removed.
3- Assignment in if condition, this is fixed.
From bf6328196341ea36ec79fba71b9b5c2c36d61043 Mon Sep 17 00:00:00 2001
From: Parmeshwr Prasad <parmeshwr_prasad@dell.com>
Date: Tue, 10 Feb 2015 02:49:26 -0500
Subject: [PATCH 2/2] Trivial patch: In this patch printk is replaced by pr_*
macros and static initialization of request_mem_succeeded is removed and
assignment in if condition is removed
Signed-off-by: Parmeshwr Prasad <parmeshwr_prasad@dell.com>
---
drivers/video/fbdev/efifb.c | 42 +++++++++++++++++++++---------------------
1 file changed, 21 insertions(+), 21 deletions(-)
diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
index d757d0f..133fbfb 100644
--- a/drivers/video/fbdev/efifb.c
+++ b/drivers/video/fbdev/efifb.c
@@ -17,8 +17,7 @@
#include <video/vga.h>
#include <asm/sysfb.h>
-static bool request_mem_succeeded = false;
-
+static bool request_mem_succeeded;
static struct fb_var_screeninfo efifb_defined = {
.activate = FB_ACTIVATE_NOW,
.height = -1,
@@ -30,6 +29,9 @@ static struct fb_var_screeninfo efifb_defined = {
.vmode = FB_VMODE_NONINTERLACED,
};
+
+
+
static struct fb_fix_screeninfo efifb_fix = {
.id = "EFI VGA",
.type = FB_TYPE_PACKED_PIXELS,
@@ -151,10 +153,10 @@ static int efifb_probe(struct platform_device *dev)
if (!screen_info.pages)
screen_info.pages = 1;
if (!screen_info.lfb_base) {
- printk(KERN_DEBUG "efifb: invalid framebuffer address\n");
+ pr_err("efifb: invalid framebuffer address\n");
return -ENODEV;
}
- printk(KERN_INFO "efifb: probing for efifb\n");
+ pr_info("efifb: probing for efifb\n");
/* just assume they're all unset if any are */
if (!screen_info.blue_size) {
@@ -202,14 +204,14 @@ static int efifb_probe(struct platform_device *dev)
} else {
/* We cannot make this fatal. Sometimes this comes from magic
spaces our resource handlers simply don't know about */
- printk(KERN_WARNING
+ pr_warn(
"efifb: cannot reserve video memory at 0x%lx\n",
efifb_fix.smem_start);
}
info = framebuffer_alloc(sizeof(u32) * 16, &dev->dev);
if (!info) {
- printk(KERN_ERR "efifb: cannot allocate framebuffer\n");
+ pr_err("efifb: cannot allocate framebuffer\n");
err = -ENOMEM;
goto err_release_mem;
}
@@ -228,25 +230,23 @@ static int efifb_probe(struct platform_device *dev)
info->screen_base ioremap_wc(efifb_fix.smem_start, efifb_fix.smem_len);
if (!info->screen_base) {
- printk(KERN_ERR "efifb: abort, cannot ioremap video memory "
- "0x%x @ 0x%lx\n",
- efifb_fix.smem_len, efifb_fix.smem_start);
+ pr_err("efifb: abort, cannot ioremap video memory 0x%x @ 0x%lx
+ \n", efifb_fix.smem_len, efifb_fix.smem_start);
err = -EIO;
goto err_release_fb;
}
- printk(KERN_INFO "efifb: framebuffer at 0x%lx, mapped to 0x%p, "
- "using %dk, total %dk\n",
- efifb_fix.smem_start, info->screen_base,
+ pr_info("efifb: framebuffer at 0x%lx, mapped to 0x%p,using %dk,
total%dk\n"
+ , efifb_fix.smem_start, info->screen_base,
size_remap / 1024, size_total / 1024);
- printk(KERN_INFO "efifb: mode is %dx%dx%d, linelength=%d, pages=%d\n",
+ pr_info("efifb: mode is %dx%dx%d, linelength=%d, pages=%d\n",
efifb_defined.xres, efifb_defined.yres,
efifb_defined.bits_per_pixel, efifb_fix.line_length,
screen_info.pages);
efifb_defined.xres_virtual = efifb_defined.xres;
efifb_defined.yres_virtual = efifb_fix.smem_len / efifb_fix.line_length;
- printk(KERN_INFO "efifb: scrolling: redraw\n");
+ pr_info("efifb: scrolling: redraw\n");
efifb_defined.yres_virtual = efifb_defined.yres;
/* some dummy values for timing to make fbset happy */
@@ -264,9 +264,7 @@ static int efifb_probe(struct platform_device *dev)
efifb_defined.transp.offset = screen_info.rsvd_pos;
efifb_defined.transp.length = screen_info.rsvd_size;
- printk(KERN_INFO "efifb: %s: "
- "size=%d:%d:%d:%d, shift=%d:%d:%d:%d\n",
- "Truecolor",
+ pr_info("efifb: %s:ize=%d:%d:%d:%d, shift=%d:%d:%d:%d\n", "Truecolor",
screen_info.rsvd_size,
screen_info.red_size,
screen_info.green_size,
@@ -283,12 +281,14 @@ static int efifb_probe(struct platform_device *dev)
info->fix = efifb_fix;
info->flags = FBINFO_FLAG_DEFAULT | FBINFO_MISC_FIRMWARE;
- if ((err = fb_alloc_cmap(&info->cmap, 256, 0)) < 0) {
- printk(KERN_ERR "efifb: cannot allocate colormap\n");
+ err = fb_alloc_cmap(&info->cmap, 256, 0);
+ if (err < 0) {
+ pr_err("efifb: cannot allocate colormap\n");
goto err_unmap;
}
- if ((err = register_framebuffer(info)) < 0) {
- printk(KERN_ERR "efifb: cannot register framebuffer\n");
+ err = register_framebuffer(info);
+ if (err < 0) {
+ pr_err("efifb: cannot register framebuffer\n");
goto err_fb_dealoc;
}
fb_info(info, "%s frame buffer device\n", info->fix.id);
--
1.9.3
This don't has any checkpatch.pl comment/warning.
-Parmeshwr
> I have created two patch now.
> 0001-Trival-patch-improved-indentation-in-efifb.c-file.patch:
> This patch solves indentation issue in efifb.c file.
> This don't has any code change.
> I checked with checkpatch.pl it has some warning to remove some old functions in
> efifb.c. my changes are not to address that. That changes I can try in my next
> patch.
> 0002-Trivial-patch-In-this-patch-printk-is-replaced-by-pr.patch:
> This is addressing following changes:
> 1- It has removed "quoted string split across lines" warning.
> 2- There was static initialization of request_mem_succeeded, which is removed.
> 3- Assignment in if condition, this is fixed.
>
> This one I have checked with checkpatch.pl it don't has any warning or comment.
>
>
> I am sending these patches with mutt.
>
> From 8ce800b5f5a048109014994bcfc4fae2ef9cc271 Mon Sep 17 00:00:00 2001
> From: Parmeshwr Prasad <parmeshwr_prasad@dell.com>
> Date: Tue, 10 Feb 2015 00:33:30 -0500
> Subject: [PATCH] Trival patch: improved indentation in efifb.c file
>
> Signed-off-by: Parmeshwr Prasad <parmeshwr_prasad@dell.com>
> ---
> drivers/video/fbdev/efifb.c | 128 +++++++++++++++++++++++---------------------
> 1 file changed, 68 insertions(+), 60 deletions(-)
>
> diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
> index 4bfff34..d757d0f 100644
> --- a/drivers/video/fbdev/efifb.c
> +++ b/drivers/video/fbdev/efifb.c
> @@ -20,26 +20,25 @@
> static bool request_mem_succeeded = false;
>
> static struct fb_var_screeninfo efifb_defined = {
> - .activate = FB_ACTIVATE_NOW,
> - .height = -1,
> - .width = -1,
> - .right_margin = 32,
> - .upper_margin = 16,
> - .lower_margin = 4,
> - .vsync_len = 4,
> - .vmode = FB_VMODE_NONINTERLACED,
> + .activate = FB_ACTIVATE_NOW,
> + .height = -1,
> + .width = -1,
> + .right_margin = 32,
> + .upper_margin = 16,
> + .lower_margin = 4,
> + .vsync_len = 4,
> + .vmode = FB_VMODE_NONINTERLACED,
> };
>
> static struct fb_fix_screeninfo efifb_fix = {
> - .id = "EFI VGA",
> - .type = FB_TYPE_PACKED_PIXELS,
> - .accel = FB_ACCEL_NONE,
> - .visual = FB_VISUAL_TRUECOLOR,
> + .id = "EFI VGA",
> + .type = FB_TYPE_PACKED_PIXELS,
> + .accel = FB_ACCEL_NONE,
> + .visual = FB_VISUAL_TRUECOLOR,
> };
>
> static int efifb_setcolreg(unsigned regno, unsigned red, unsigned green,
> - unsigned blue, unsigned transp,
> - struct fb_info *info)
> + unsigned blue, unsigned transp, struct fb_info *info)
> {
> /*
> * Set a single color register. The values supplied are
> @@ -52,13 +51,13 @@ static int efifb_setcolreg(unsigned regno, unsigned red,
> unsigned green,
> return 1;
>
> if (regno < 16) {
> - red >>= 8;
> + red >>= 8;
> green >>= 8;
> - blue >>= 8;
> - ((u32 *)(info->pseudo_palette))[regno] > - (red << info->var.red.offset) |
> - (green << info->var.green.offset) |
> - (blue << info->var.blue.offset);
> + blue >>= 8;
> + ((u32 *) (info->pseudo_palette))[regno] > + (red << info->var.red.offset) |
> + (green << info->var.green.offset) |
> + (blue << info->var.blue.offset);
> }
> return 0;
> }
> @@ -74,12 +73,12 @@ static void efifb_destroy(struct fb_info *info)
> }
>
> static struct fb_ops efifb_ops = {
> - .owner = THIS_MODULE,
> - .fb_destroy = efifb_destroy,
> - .fb_setcolreg = efifb_setcolreg,
> - .fb_fillrect = cfb_fillrect,
> - .fb_copyarea = cfb_copyarea,
> - .fb_imageblit = cfb_imageblit,
> + .owner = THIS_MODULE,
> + .fb_destroy = efifb_destroy,
> + .fb_setcolreg = efifb_setcolreg,
> + .fb_fillrect = cfb_fillrect,
> + .fb_copyarea = cfb_copyarea,
> + .fb_imageblit = cfb_imageblit,
> };
>
> static int efifb_setup(char *options)
> @@ -89,25 +88,35 @@ static int efifb_setup(char *options)
>
> if (options && *options) {
> while ((this_opt = strsep(&options, ",")) != NULL) {
> - if (!*this_opt) continue;
> + if (!*this_opt)
> + continue;
>
> for (i = 0; i < M_UNKNOWN; i++) {
> if (efifb_dmi_list[i].base != 0 &&
> - !strcmp(this_opt,
> efifb_dmi_list[i].optname)) {
> - screen_info.lfb_base > efifb_dmi_list[i].base;
> - screen_info.lfb_linelength > efifb_dmi_list[i].stride;
> - screen_info.lfb_width > efifb_dmi_list[i].width;
> - screen_info.lfb_height > efifb_dmi_list[i].height;
> + !strcmp(this_opt,
> + efifb_dmi_list[i].optname)) {
> + screen_info.lfb_base > + efifb_dmi_list[i].base;
> + screen_info.lfb_linelength > + efifb_dmi_list[i].stride;
> + screen_info.lfb_width > + efifb_dmi_list[i].width;
> + screen_info.lfb_height > + efifb_dmi_list[i].height;
> }
> }
> if (!strncmp(this_opt, "base:", 5))
> - screen_info.lfb_base > simple_strtoul(this_opt+5, NULL, 0);
> + screen_info.lfb_base > + simple_strtoul(this_opt + 5, NULL, 0);
> else if (!strncmp(this_opt, "stride:", 7))
> - screen_info.lfb_linelength > simple_strtoul(this_opt+7, NULL, 0) * 4;
> + screen_info.lfb_linelength > + simple_strtoul(this_opt + 7, NULL, 0) * 4;
> else if (!strncmp(this_opt, "height:", 7))
> - screen_info.lfb_height > simple_strtoul(this_opt+7, NULL, 0);
> + screen_info.lfb_height > + simple_strtoul(this_opt + 7, NULL, 0);
> else if (!strncmp(this_opt, "width:", 6))
> - screen_info.lfb_width > simple_strtoul(this_opt+6, NULL, 0);
> + screen_info.lfb_width > + simple_strtoul(this_opt + 6, NULL, 0);
> }
> }
>
> @@ -181,7 +190,7 @@ static int efifb_probe(struct platform_device *dev)
> * use for efifb. With modern cards it is no
> * option to simply use size_total as that
> * wastes plenty of kernel address space. */
> - size_remap = size_vmode * 2;
> + size_remap = size_vmode * 2;
> if (size_remap > size_total)
> size_remap = size_total;
> if (size_remap % PAGE_SIZE)
> @@ -195,7 +204,7 @@ static int efifb_probe(struct platform_device *dev)
> spaces our resource handlers simply don't know about */
> printk(KERN_WARNING
> "efifb: cannot reserve video memory at 0x%lx\n",
> - efifb_fix.smem_start);
> + efifb_fix.smem_start);
> }
>
> info = framebuffer_alloc(sizeof(u32) * 16, &dev->dev);
> @@ -216,11 +225,12 @@ static int efifb_probe(struct platform_device *dev)
> info->apertures->ranges[0].base = efifb_fix.smem_start;
> info->apertures->ranges[0].size = size_remap;
>
> - info->screen_base = ioremap_wc(efifb_fix.smem_start,
> efifb_fix.smem_len);
> + info->screen_base > + ioremap_wc(efifb_fix.smem_start, efifb_fix.smem_len);
> if (!info->screen_base) {
> printk(KERN_ERR "efifb: abort, cannot ioremap video memory "
> - "0x%x @ 0x%lx\n",
> - efifb_fix.smem_len, efifb_fix.smem_start);
> + "0x%x @ 0x%lx\n",
> + efifb_fix.smem_len, efifb_fix.smem_start);
> err = -EIO;
> goto err_release_fb;
> }
> @@ -228,30 +238,29 @@ static int efifb_probe(struct platform_device *dev)
> printk(KERN_INFO "efifb: framebuffer at 0x%lx, mapped to 0x%p, "
> "using %dk, total %dk\n",
> efifb_fix.smem_start, info->screen_base,
> - size_remap/1024, size_total/1024);
> + size_remap / 1024, size_total / 1024);
> printk(KERN_INFO "efifb: mode is %dx%dx%d, linelength=%d, pages=%d\n",
> efifb_defined.xres, efifb_defined.yres,
> efifb_defined.bits_per_pixel, efifb_fix.line_length,
> screen_info.pages);
>
> efifb_defined.xres_virtual = efifb_defined.xres;
> - efifb_defined.yres_virtual = efifb_fix.smem_len /
> - efifb_fix.line_length;
> + efifb_defined.yres_virtual = efifb_fix.smem_len / efifb_fix.line_length;
> printk(KERN_INFO "efifb: scrolling: redraw\n");
> efifb_defined.yres_virtual = efifb_defined.yres;
>
> /* some dummy values for timing to make fbset happy */
> - efifb_defined.pixclock = 10000000 / efifb_defined.xres *
> - 1000 / efifb_defined.yres;
> - efifb_defined.left_margin = (efifb_defined.xres / 8) & 0xf8;
> - efifb_defined.hsync_len = (efifb_defined.xres / 8) & 0xf8;
> -
> - efifb_defined.red.offset = screen_info.red_pos;
> - efifb_defined.red.length = screen_info.red_size;
> - efifb_defined.green.offset = screen_info.green_pos;
> - efifb_defined.green.length = screen_info.green_size;
> - efifb_defined.blue.offset = screen_info.blue_pos;
> - efifb_defined.blue.length = screen_info.blue_size;
> + efifb_defined.pixclock = 10000000 / efifb_defined.xres *
> + 1000 / efifb_defined.yres;
> + efifb_defined.left_margin = (efifb_defined.xres / 8) & 0xf8;
> + efifb_defined.hsync_len = (efifb_defined.xres / 8) & 0xf8;
> +
> + efifb_defined.red.offset = screen_info.red_pos;
> + efifb_defined.red.length = screen_info.red_size;
> + efifb_defined.green.offset = screen_info.green_pos;
> + efifb_defined.green.length = screen_info.green_size;
> + efifb_defined.blue.offset = screen_info.blue_pos;
> + efifb_defined.blue.length = screen_info.blue_size;
> efifb_defined.transp.offset = screen_info.rsvd_pos;
> efifb_defined.transp.length = screen_info.rsvd_size;
>
> @@ -264,10 +273,9 @@ static int efifb_probe(struct platform_device *dev)
> screen_info.blue_size,
> screen_info.rsvd_pos,
> screen_info.red_pos,
> - screen_info.green_pos,
> - screen_info.blue_pos);
> + screen_info.green_pos, screen_info.blue_pos);
>
> - efifb_fix.ypanstep = 0;
> + efifb_fix.ypanstep = 0;
> efifb_fix.ywrapstep = 0;
>
> info->fbops = &efifb_ops;
> @@ -310,8 +318,8 @@ static int efifb_remove(struct platform_device *pdev)
>
> static struct platform_driver efifb_driver = {
> .driver = {
> - .name = "efi-framebuffer",
> - },
> + .name = "efi-framebuffer",
> + },
> .probe = efifb_probe,
> .remove = efifb_remove,
> };
> --
> 1.9.3
>
>
> -Parmeshwr
>
>
> On Mon, Feb 09, 2015 at 08:24:50AM -0600, Lad, Prabhakar wrote:
> > Hi,
> >
> > Thanks for the patch.
> >
> > On Mon, Feb 9, 2015 at 12:55 PM, Parmeshwr Prasad
> > <parmeshwr_prasad@dell.com> wrote:
> > > Hi All,
> > >
> > > Please review this patch.
> > > this patch is aimed to solve some indentation issue. It has also solved
> > > three trivial error in efifb.c file.
> > >
> > > And I have also changed printk with pr_err, pr_info ... at respective places.
> > >
> > >
> > > From c49139fac1d15fe2da80d06e2a79eb8be7c079a7 Mon Sep 17 00:00:00 2001
> > > From: Parmeshwr Prasad <parmeshwr_prasad@dell.com>
> > > Date: Mon, 9 Feb 2015 07:33:59 -0500
> > > Subject: [PATCH] Trival patch: improved indentation, and removed some ERROR
> > > from code
> > >
> > > Signed-off-by: Parmeshwr Prasad <parmeshwr_prasad@dell.com>
> > > ---
> >
> > 1: did you use git to send this patch ?
> > 2: did you checkpatch it (I see some issues)?
> > 3: break this patch into 2 one fixing the 3 issues and other fixing
> > the indentation.
> > 4: have proper commit message.
> >
> > Cheers,
> > --Prabhakar Lad
> --
> To unsubscribe from this list: send the line "unsubscribe linux-efi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH] staging: sm7xxfb: make vgamode static
From: Sudip Mukherjee @ 2015-02-10 12:20 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: Sudip Mukherjee, linux-fbdev, devel, linux-kernel
the variable vgamode is used only in this file and hence can be
safely made as static.
Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---
drivers/staging/sm7xxfb/sm7xx.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/sm7xxfb/sm7xx.h b/drivers/staging/sm7xxfb/sm7xx.h
index 7cc1896..c5d6253 100644
--- a/drivers/staging/sm7xxfb/sm7xx.h
+++ b/drivers/staging/sm7xxfb/sm7xx.h
@@ -119,7 +119,7 @@ struct ModeInit {
/**********************************************************************
SM712 Mode table.
**********************************************************************/
-struct ModeInit vgamode[] = {
+static struct ModeInit vgamode[] = {
{
/* mode#0: 640 x 480 16Bpp 60Hz */
640, 480, 16, 60,
--
1.8.1.2
^ permalink raw reply related
* [PATCH] fbdev: sm501fb: use memset_io
From: Sudip Mukherjee @ 2015-02-10 12:49 UTC (permalink / raw)
To: Jean-Christophe Plagniol-Villard, Tomi Valkeinen
Cc: Sudip Mukherjee, linux-fbdev, linux-kernel
we should really be using memset_io() instead of using memset() as
this is actually io space mapped into our memory.
Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---
it has only been build tested. I only have the hardware of sm712 and sm750.
drivers/video/fbdev/sm501fb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/sm501fb.c b/drivers/video/fbdev/sm501fb.c
index e8d4121..d0a4e2f 100644
--- a/drivers/video/fbdev/sm501fb.c
+++ b/drivers/video/fbdev/sm501fb.c
@@ -1606,7 +1606,7 @@ static int sm501fb_start(struct sm501fb_info *info,
info->fbmem_len = resource_size(res);
/* clear framebuffer memory - avoids garbage data on unused fb */
- memset(info->fbmem, 0, info->fbmem_len);
+ memset_io(info->fbmem, 0, info->fbmem_len);
/* clear palette ram - undefined at power on */
for (k = 0; k < (256 * 3); k++)
--
1.8.1.2
^ permalink raw reply related
* [GIT PULL] fbdev changes for 3.20
From: Tomi Valkeinen @ 2015-02-11 9:35 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Linux Fbdev development list, LKML
[-- Attachment #1: Type: text/plain, Size: 6317 bytes --]
Hi Linus,
Please pull fbdev changes for 3.20.
Tomi
The following changes since commit eaa27f34e91a14cdceed26ed6c6793ec1d186115:
linux 3.19-rc4 (2015-01-11 12:44:53 -0800)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/tomba/linux.git tags/fbdev-3.20
for you to fetch changes up to d6c2152b3efd73be265f426b5a1bb50ec436d999:
Merge branches '3.20/fbdev' and '3.20/omapdss' into for-next (2015-02-04 12:44:11 +0200)
----------------------------------------------------------------
fbdev changes for v3.20
* omapdss: add DRA7xxx SoC support
* fbdev: support DMT (Display Monitor Timing) calculation
----------------------------------------------------------------
Arnd Bergmann (2):
fb: via: turn gpiolib and i2c selects into dependencies
video/mmpfb: allow modular build
Asaf Vertz (1):
fbdev: vgastate: remove trailing whitespaces
David Ung (4):
video: fbdev: Add additional vesa modes
video: fbdev: Check Standard Timing against DMT
video: fbdev: Validate mode timing against monspec
video: fbdev: Fix sparse warning messages
Geert Uytterhoeven (2):
console/dummy: Move screen size selection from CPP to Kconfig
video: Drop superfluous "select VT_HW_CONSOLE_BINDING"
Lad, Prabhakar (2):
fbdev: ssd1307fb: drop unused function ssd1307fb_write_data()
fbdev: ssd1307fb: set default height if not found in DT node
Mans Rullgard (1):
video: fbdev: fix sys_copyarea
Marek Belisko (2):
video: omapdss: Add opa362 driver
Documentation: DT: Add documentation for ti,opa362 bindings
Mathias Krause (1):
atyfb: mark DMI system id table as __initconst
Peter Meerwald (1):
omapfb: Return error code when applying overlay settings fails
Prabhakar Lad (1):
fbdev: ssd1307fb: return proper error code if write command fails
Qiang Chen (1):
video: ocfb: Fix data type warning
Rasmus Villemoes (1):
fbdev: omap2: Fix typo in tvc_probe_pdata
Scot Doyle (1):
fbcon: Remove unused vblank cursor code
Sudip Mukherjee (5):
fbdev: geocode: remove unneeded NULL check
fbdev: geocode: remove unneeded NULL check
fbdev: savage: remove unused variable
video: vt8500lcdfb: remove unneeded continue
video: hgafb: remove unneeded comparison
Tomi Valkeinen (16):
fbdev: fix CVT vertical front and back porch values
OMAPDSS: Add enum dss_pll_id
OMAPDSS: PLL: add dss_pll_wait_reset_done()
OMAPDSS: constify port arrays
OMAPDSS: OMAP5: fix digit output's allowed mgrs
OMAPDSS: encoder-tpd12s015: Fix race issue with LS_OE
OMAPDSS: add define for DRA7xx HW version
Doc/DT: Add DT binding doc for DRA7xx DSS
OMAPDSS: DSS: Add DRA7xx base support
OMAPDSS: Add functions for external control of PLL
OMAPDSS: Add Video PLLs for DRA7xx
OMAPDSS: DISPC: Add DRA7xx support
OMAPDSS: DISPC: program dispc polarities to control module
OMAPDSS: HDMI: Add DRA7xx support
OMAPDSS: DPI: DRA7xx support
Merge branches '3.20/fbdev' and '3.20/omapdss' into for-next
.../devicetree/bindings/video/ti,dra7-dss.txt | 69 +++++
.../devicetree/bindings/video/ti,opa362.txt | 38 +++
drivers/video/console/Kconfig | 16 +-
drivers/video/console/dummycon.c | 5 +-
drivers/video/console/fbcon.c | 4 -
drivers/video/fbdev/Kconfig | 5 +-
drivers/video/fbdev/aty/atyfb_base.c | 9 +-
drivers/video/fbdev/core/fbcvt.c | 6 +-
drivers/video/fbdev/core/fbmon.c | 103 ++++----
drivers/video/fbdev/core/modedb.c | 111 ++++++++
drivers/video/fbdev/core/syscopyarea.c | 137 +++++-----
drivers/video/fbdev/geode/gx1fb_core.c | 6 +-
drivers/video/fbdev/geode/gxfb_core.c | 6 +-
drivers/video/fbdev/geode/lxfb_core.c | 6 +-
drivers/video/fbdev/hgafb.c | 3 +-
drivers/video/fbdev/mmp/Makefile | 4 +-
drivers/video/fbdev/mmp/fb/Kconfig | 2 +-
drivers/video/fbdev/ocfb.c | 2 +-
drivers/video/fbdev/omap2/displays-new/Kconfig | 6 +
drivers/video/fbdev/omap2/displays-new/Makefile | 1 +
.../fbdev/omap2/displays-new/connector-analog-tv.c | 2 +-
.../fbdev/omap2/displays-new/encoder-opa362.c | 285 +++++++++++++++++++++
.../fbdev/omap2/displays-new/encoder-tpd12s015.c | 57 +----
drivers/video/fbdev/omap2/dss/Makefile | 2 +-
drivers/video/fbdev/omap2/dss/dispc.c | 54 ++++
drivers/video/fbdev/omap2/dss/dpi.c | 26 ++
drivers/video/fbdev/omap2/dss/dsi.c | 1 +
drivers/video/fbdev/omap2/dss/dss.c | 219 +++++++++++++++-
drivers/video/fbdev/omap2/dss/dss.h | 22 ++
drivers/video/fbdev/omap2/dss/dss_features.c | 3 +-
drivers/video/fbdev/omap2/dss/hdmi5.c | 1 +
drivers/video/fbdev/omap2/dss/hdmi_phy.c | 1 +
drivers/video/fbdev/omap2/dss/hdmi_pll.c | 6 +
drivers/video/fbdev/omap2/dss/omapdss-boot-init.c | 1 +
drivers/video/fbdev/omap2/dss/pll.c | 10 +
drivers/video/fbdev/omap2/dss/video-pll.c | 211 +++++++++++++++
drivers/video/fbdev/omap2/omapfb/omapfb-ioctl.c | 7 +-
drivers/video/fbdev/savage/savagefb.h | 12 +-
drivers/video/fbdev/ssd1307fb.c | 86 ++++---
drivers/video/fbdev/vt8500lcdfb.c | 4 +-
drivers/video/vgastate.c | 82 +++---
include/linux/fb.h | 12 +-
include/video/omapdss.h | 1 +
43 files changed, 1342 insertions(+), 302 deletions(-)
create mode 100644 Documentation/devicetree/bindings/video/ti,dra7-dss.txt
create mode 100644 Documentation/devicetree/bindings/video/ti,opa362.txt
create mode 100644 drivers/video/fbdev/omap2/displays-new/encoder-opa362.c
create mode 100644 drivers/video/fbdev/omap2/dss/video-pll.c
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* [PATCH] video: ARM CLCD: Add missing error check for devm_kzalloc
From: Kiran Padwal @ 2015-02-11 9:48 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Russell King, Jean-Christophe Plagniol-Villard, linux-fbdev,
linux-kernel, Kiran Padwal, Kiran Padwal
This patch add a missing check on the return value of devm_kzalloc,
which would cause a NULL pointer dereference in a OOM situation.
Signed-off-by: Kiran Padwal <kiran.padwal@smartplayin.com>
---
drivers/video/fbdev/amba-clcd.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/video/fbdev/amba-clcd.c b/drivers/video/fbdev/amba-clcd.c
index 32c0b6b..9362424 100644
--- a/drivers/video/fbdev/amba-clcd.c
+++ b/drivers/video/fbdev/amba-clcd.c
@@ -599,6 +599,9 @@ static int clcdfb_of_get_mode(struct device *dev, struct device_node *endpoint,
len = clcdfb_snprintf_mode(NULL, 0, mode);
name = devm_kzalloc(dev, len + 1, GFP_KERNEL);
+ if (!name)
+ return -ENOMEM;
+
clcdfb_snprintf_mode(name, len + 1, mode);
mode->name = name;
--
1.7.9.5
^ permalink raw reply related
* [bug report] potential memory corruption in intelfb_cursor()
From: Dan Carpenter @ 2015-02-11 15:45 UTC (permalink / raw)
To: linux-fbdev
Some static checker stuff I'm working on complains:
drivers/video/fbdev/intelfb/intelfbdrv.c:1678 intelfb_cursor()
error: overflow detected. __memcpy() 'dinfo->cursor_src' is 64
bytes. limit = '1-128'
The math here is:
size = cursor->image.width / 8 * cursor->image.height;
Since the limit for ".width" and ".height" is 32x32 then
"32 / 4 * 32 = 128" but 128 is more than the sizeof(dinfo->cursor_src)
so it creates a buffer overflow warning.
I believe that Smatch is getting 32 as the max size from the tests in
con_font_set().
drivers/video/fbdev/intelfb/intelfbdrv.c
1632 if (cursor->set & FB_CUR_SETSIZE) {
1633 if (cursor->image.width > 64 || cursor->image.height > 64)
Here the limit is assumed to be 64 which would make the overflow worse
than the 32 limit mentioned earlier.
1634 return -ENXIO;
1635
1636 intelfbhw_cursor_reset(dinfo);
1637 }
1638
1639 if (cursor->set & FB_CUR_SETCMAP) {
1640 u32 fg, bg;
1641
1642 if (dinfo->depth != 8) {
1643 fg = dinfo->pseudo_palette[cursor->image.fg_color];
1644 bg = dinfo->pseudo_palette[cursor->image.bg_color];
1645 } else {
1646 fg = cursor->image.fg_color;
1647 bg = cursor->image.bg_color;
1648 }
1649
1650 intelfbhw_cursor_setcolor(dinfo, bg, fg);
1651 }
1652
1653 if (cursor->set & (FB_CUR_SETSHAPE | FB_CUR_SETIMAGE)) {
1654 u32 s_pitch = (ROUND_UP_TO(cursor->image.width, 8) / 8);
1655 u32 size = s_pitch * cursor->image.height;
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Size is calculated here.
1656 u8 *dat = (u8 *) cursor->image.data;
1657 u8 *msk = (u8 *) cursor->mask;
1658 u8 src[64];
1659 u32 i;
1660
1661 if (cursor->image.depth != 1)
1662 return -ENXIO;
1663
1664 switch (cursor->rop) {
1665 case ROP_XOR:
1666 for (i = 0; i < size; i++)
1667 src[i] = dat[i] ^ msk[i];
1668 break;
1669 case ROP_COPY:
1670 default:
1671 for (i = 0; i < size; i++)
1672 src[i] = dat[i] & msk[i];
1673 break;
1674 }
1678 memcpy(dinfo->cursor_src, src, size);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This is what Smatch complains about. The source and dest are 64 bytes
each but it says the user can specify a size which is 128 bytes.
1679
1680 intelfbhw_cursor_load(dinfo, cursor->image.width,
1681 cursor->image.height, src);
1682 }
I am a newbie to this code so I don't know if the warning is real or not
or how to fix it. Sorry.
regards,
dan carpenter
^ permalink raw reply
* Odp: [PATCH 1/4] tridentfb: fix hang on Blade3D with CONFIG_CC_OPTIMIZE_FOR_SIZE
From: Krzysztof Helt @ 2015-02-11 18:01 UTC (permalink / raw)
To: Ondrej Zary; +Cc: linux-fbdev, Kernel development list
Acked-by: Krzysztof Helt <krzysztof.h1@wp.pl>
Dnia Sobota, 7 Lutego 2015 21:53 Ondrej Zary <linux@rainbow-software.org> napisa³(a)
> When the kernel is compiled with -Os (CONFIG_CC_OPTIMIZE_FOR_SIZE), tridentfb
> hangs the machine upon load with Blade3D cards unless acceleration is disabled.
>
> This is caused by memcpy() which copies data byte-by-byte (rep movsb) when
> compiled with -Os. The card does not like that - it requires 32-bit access.
>
> Use iowrite_32() instead.
>
> Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
> ---
> drivers/video/fbdev/tridentfb.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/video/fbdev/tridentfb.c b/drivers/video/fbdev/tridentfb.c
> index 7ed9a22..7429713 100644
> --- a/drivers/video/fbdev/tridentfb.c
> +++ b/drivers/video/fbdev/tridentfb.c
> @@ -226,7 +226,7 @@ static void blade_image_blit(struct tridentfb_par *par, const char *data,
> writemmr(par, DST1, point(x, y));
> writemmr(par, DST2, point(x + w - 1, y + h - 1));
>
> - memcpy(par->io_virt + 0x10000, data, 4 * size);
> + iowrite32_rep(par->io_virt + 0x10000, data, size);
> }
>
> static void blade_copy_rect(struct tridentfb_par *par,
> --
> Ondrej Zary
^ permalink raw reply
* Odp: [PATCH 3/4] fb_ddc: Allow I2C adapters without SCL read capability
From: Krzysztof Helt @ 2015-02-11 18:02 UTC (permalink / raw)
To: Ondrej Zary; +Cc: linux-fbdev, Kernel development list
Acked-by: Krzysztof Helt <krzysztof.h1@wp.pl>
Dnia Sobota, 7 Lutego 2015 21:53 Ondrej Zary <linux@rainbow-software.org> napisa³(a)
> i2c-algo-bit allows I2C adapters without SCL read capability to work but
> fb_ddc_read fails to work on them.
>
> Fix fb_ddc_read to work with I2C adapters not capable of reading SCL.
>
> Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
> ---
> drivers/video/fbdev/core/fb_ddc.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/video/fbdev/core/fb_ddc.c b/drivers/video/fbdev/core/fb_ddc.c
> index 94322cc..22c694a 100644
> --- a/drivers/video/fbdev/core/fb_ddc.c
> +++ b/drivers/video/fbdev/core/fb_ddc.c
> @@ -69,10 +69,11 @@ unsigned char *fb_ddc_read(struct i2c_adapter *adapter)
> algo_data->setscl(algo_data->data, 1);
> for (j = 0; j < 5; j++) {
> msleep(10);
> - if (algo_data->getscl(algo_data->data))
> + if (algo_data->getscl &&
> + algo_data->getscl(algo_data->data))
> break;
> }
> - if (j = 5)
> + if (algo_data->getscl && j = 5)
> continue;
>
> algo_data->setsda(algo_data->data, 0);
> @@ -91,7 +92,8 @@ unsigned char *fb_ddc_read(struct i2c_adapter *adapter)
> algo_data->setscl(algo_data->data, 1);
> for (j = 0; j < 10; j++) {
> msleep(10);
> - if (algo_data->getscl(algo_data->data))
> + if (algo_data->getscl &&
> + algo_data->getscl(algo_data->data))
> break;
> }
>
> --
> Ondrej Zary
^ permalink raw reply
* Re: [PATCH 4/8] fbdev: ssd1307fb: Use vmalloc to allocate video memory.
From: Maxime Ripard @ 2015-02-12 15:11 UTC (permalink / raw)
To: Thomas Niederprüm
Cc: linux-fbdev, plagnioj, tomi.valkeinen, linux-kernel
In-Reply-To: <20150207163541.30047a33@maestro.intranet>
[-- Attachment #1: Type: text/plain, Size: 2879 bytes --]
On Sat, Feb 07, 2015 at 04:35:41PM +0100, Thomas Niederprüm wrote:
> Am Sat, 7 Feb 2015 12:18:21 +0100
> schrieb Maxime Ripard <maxime.ripard@free-electrons.com>:
>
> > Hi,
> >
> > On Fri, Feb 06, 2015 at 11:28:10PM +0100, niederp@physik.uni-kl.de
> > wrote:
> > > From: Thomas Niederprüm <niederp@physik.uni-kl.de>
> > >
> > > It makes sense to use vmalloc to allocate the video buffer since it
> > > has to be page aligned memory for using it with mmap.
> >
> > Please wrap your commit log at 80 chars.
>
> I'll try to do so in future, sorry for that.
>
> >
> > It looks like there's numerous fbdev drivers using this (especially
> > since you copy pasted that code, without mentionning it).
>
> Yes, I should have mentioned that in the commit message. As
> implicitly indicated in the cover letter the rvmalloc() and rvfree() are
> copy pasted from the vfb driver. Honestly, I didn't give this one too
> much thought. It seemed a viable solution to the mmap problem. For a
> bit more history on that, see my comment below.
>
> >
> > That should be turned into an allocator so that drivers all get this
> > right.
> >
> > > Also deffered io seems buggy in combination with kmalloc'ed memory
> > > (crash on unloading the module).
> >
> > And maybe that's the real issue to fix.
>
> The problem is solved by using vmalloc ;)
Yep, but why do you need to mark the reserved pages?
...
> > > @@ -570,7 +608,7 @@ static int ssd1307fb_probe(struct i2c_client
> > > *client, info->var.blue.offset = 0;
> > >
> > > info->screen_base = (u8 __force __iomem *)vmem;
> > > - info->fix.smem_start = (unsigned long)vmem;
> > > + info->fix.smem_start = __pa(vmem);
> >
> > Why are you changing from virtual to physical address here?
>
> Oh, good catch! This is still a residual of my attempts to get this
> working with kmalloc'ed memory. In the current state the driver is
> presenting a completely wrong memory address upon mmap. As reported in
> [0] info->fix.smem_start has to hold the physical address of the video
> memory if it was allocated using kmalloc. Correcting this let me run
> into the problem that the kmalloc'ed memory was not page aligned but,
> the memory address handed to userspace mmap was aligned to the next
> full page, resulting in an inaccessable display region. At that point I
> just copied the vmalloc approach from the vfb driver.
>
> [0] http://stackoverflow.com/questions/22285151/kernel-panic-using-deferred-io-on-kmalloced-buffer
And the documentation of fb_fix_screeninfo indeed state that this
should be the physical address:
http://lxr.free-electrons.com/source/include/uapi/linux/fb.h#L158
Could you make that a separate patch?
Thanks,
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* [PATCH] video: fbdev: fix possible null dereference
From: Sudip Mukherjee @ 2015-02-12 15:59 UTC (permalink / raw)
To: Jean-Christophe Plagniol-Villard, Tomi Valkeinen
Cc: Sudip Mukherjee, linux-fbdev, linux-kernel
we were dereferencing edid first and the NULL check was after
accessing that. now we are using edid only if we know that
it is not NULL.
Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---
drivers/video/fbdev/core/fbmon.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/video/fbdev/core/fbmon.c b/drivers/video/fbdev/core/fbmon.c
index 9533859..868facd 100644
--- a/drivers/video/fbdev/core/fbmon.c
+++ b/drivers/video/fbdev/core/fbmon.c
@@ -624,9 +624,6 @@ static struct fb_videomode *fb_create_modedb(unsigned char *edid, int *dbsize,
int num = 0, i, first = 1;
int ver, rev;
- ver = edid[EDID_STRUCT_VERSION];
- rev = edid[EDID_STRUCT_REVISION];
-
mode = kzalloc(50 * sizeof(struct fb_videomode), GFP_KERNEL);
if (mode = NULL)
return NULL;
@@ -637,6 +634,9 @@ static struct fb_videomode *fb_create_modedb(unsigned char *edid, int *dbsize,
return NULL;
}
+ ver = edid[EDID_STRUCT_VERSION];
+ rev = edid[EDID_STRUCT_REVISION];
+
*dbsize = 0;
DPRINTK(" Detailed Timings\n");
--
1.8.1.2
^ permalink raw reply related
* Re: [PATCH 2/8] fbdev: ssd1307fb: Unify init code and make controller configurable from device tree
From: Maxime Ripard @ 2015-02-12 16:41 UTC (permalink / raw)
To: Thomas Niederprüm
Cc: linux-fbdev, plagnioj, tomi.valkeinen, linux-kernel
In-Reply-To: <20150207155933.4f1d0998@maestro.intranet>
[-- Attachment #1: Type: text/plain, Size: 5372 bytes --]
On Sat, Feb 07, 2015 at 03:59:33PM +0100, Thomas Niederprüm wrote:
> Am Sat, 7 Feb 2015 11:42:25 +0100
> schrieb Maxime Ripard <maxime.ripard@free-electrons.com>:
>
> > Hi,
> >
> > On Fri, Feb 06, 2015 at 11:28:08PM +0100, niederp@physik.uni-kl.de
> > wrote:
> > > From: Thomas Niederprüm <niederp@physik.uni-kl.de>
> > >
> > > This patches unifies the init code for the ssd130X chips and
> > > adds device tree bindings to describe the hardware configuration
> > > of the used controller. This gets rid of the magic bit values
> > > used in the init code so far.
> > >
> > > Signed-off-by: Thomas Niederprüm <niederp@physik.uni-kl.de>
> > > ---
> > > .../devicetree/bindings/video/ssd1307fb.txt | 11 +
> > > drivers/video/fbdev/ssd1307fb.c | 243
> > > ++++++++++++++------- 2 files changed, 174 insertions(+), 80
> > > deletions(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/video/ssd1307fb.txt
> > > b/Documentation/devicetree/bindings/video/ssd1307fb.txt index
> > > 7a12542..1230f68 100644 ---
> > > a/Documentation/devicetree/bindings/video/ssd1307fb.txt +++
> > > b/Documentation/devicetree/bindings/video/ssd1307fb.txt @@ -15,6
> > > +15,17 @@ Required properties:
> > > Optional properties:
> > > - reset-active-low: Is the reset gpio is active on physical low?
> > > + - solomon,segment-remap: Invert the order of data column to
> > > segment mapping
> > > + - solomon,offset: Map the display start line to one of COM0 -
> > > COM63
> > > + - solomon,contrast: Set initial contrast of the display
> > > + - solomon,prechargep1: Set the duration of the precharge period
> > > phase1
> > > + - solomon,prechargep2: Set the duration of the precharge period
> > > phase2
> > > + - solomon,com-alt: Enable/disable alternate COM pin configuration
> > > + - solomon,com-lrremap: Enable/disable left-right remap of COM
> > > pins
> > > + - solomon,com-invdir: Invert COM scan direction
> > > + - solomon,vcomh: Set VCOMH regulator voltage
> > > + - solomon,dclk-div: Set display clock divider
> > > + - solomon,dclk-frq: Set display clock frequency
> >
> > I'm sorry, but this is the wrong approach, for at least two reasons:
> > you broke all existing users of that driver, which is a clear no-go,
>
> Unfortunately this is true. The problem is that the SSD130X
> controllers allow for a very versatile wiring of the display to the
> controller. It's over to the manufacturer of the OLED module
> (disp+controller) to decide how it's actually wired and during
> device initialization the driver has to take care to configure the
> SSD130X controller according to that wiring. If the driver fails to
> do so you will end up having your display showing
> garbage.
How so?
Does it depend on the X, or can it change from one same controller to
another? to what extent?
The 1306 for example seems to not be using these values at all, while
the 1307 does.
> Unfortunately the current sate of the initialization code of the
> ssd1307fb driver is not very flexible in that respect. Taking a look
> at the initialization code for the ssd1306 shows that it was written
> with one very special display module in mind. Most of the magic bit
> values set there are non-default values according to the
> datasheet. The result is that the driver works with that one
> particular display module but many other (differently wired) display
> modules using a ssd1306 controller won't work without changing the
> hardcoded magic bit values.
>
> My idea here was to set all configuration to the default values (as
> given in the datasheet) unless it is overwritten by DT. Of course,
> without a change in DT, this breaks the driver for all existing users.
> The only alternative would be to set the current values as default.
> Somehow this feels wrong to me as these values look arbitrary when you
> don't know what exact display module they were set for. But if you
> insist, I will change the default values.
Unfortunately, the DT bindings are to be considered an ABI, and we
should support booting with older DTs (not that I personally care
about it, but that's another issue). So we really don't have much
choice here.
Moreover, that issue left aside, modifying bindings like this without
fixing up the in-tree users is considered quite rude :)
> > and the DT itself should not contain any direct mapping of the
> > registers.
> >
>
> I think I don't get what you mean here. Is it because I do no sanity
> checks of the numbers set in DT? I was just looking for a way to hand
> over the information about the wiring of display to the driver. How
> would you propose to solve this?
What I meant was that replicating direct registers value is usually a
recipe for a later failure, especially if we can have the information
under a generic and easy to understand manner.
For example, replacing the solomon,dclk-div and solomon,dclk-frq
properties by a clock-frequency property in Hz, and computing the
divider and that register in your driver is usually better, also
because it allows to have different requirements / algorithms to
compute that if some other chip needs it.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH 2/8] fbdev: ssd1307fb: Unify init code and make controller configurable from device tree
From: Maxime Ripard @ 2015-02-12 16:58 UTC (permalink / raw)
To: Noralf Trønnes
Cc: Thomas Niederprüm, linux-fbdev, plagnioj, tomi.valkeinen,
linux-kernel
In-Reply-To: <54D62D09.8080404@tronnes.org>
[-- Attachment #1: Type: text/plain, Size: 1390 bytes --]
On Sat, Feb 07, 2015 at 04:19:37PM +0100, Noralf Trønnes wrote:
> >>and the DT itself should not contain any direct mapping of the
> >>registers.
> >>
> >I think I don't get what you mean here. Is it because I do no sanity
> >checks of the numbers set in DT? I was just looking for a way to hand
> >over the information about the wiring of display to the driver. How
> >would you propose to solve this?
>
> I have the exact same challenge with the staging/fbtft drivers.
> I have asked about this on the DT list a couple of days ago, but no answer
> yet:
>
> Can I do register initialization from Device Tree?
> http://www.spinics.net/lists/devicetree/msg68174.html
The DT is just an hardware representation, and should not contain any
logic. Any attempts at doing so have been failures, mostly because
that kind of construct are really fragile.
What would happen if at some point some new controller pops up and
need to poke a GPIO before every write?
Every driver should contain all the needed code to initialise properly
its hardware. The only exception being stuff that are volatile by
essence, like bus addresses, GPIOs, etc.
In your case, using (and maybe extending) the generic panel bindings
look like a better way forward.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* gxt4500 on x86 (Fire GL2)
From: Ondrej Zary @ 2015-02-12 21:11 UTC (permalink / raw)
To: Paul Mackerras; +Cc: linux-fbdev, Kernel development list
Hello,
I'm trying to get gxt4500 driver to work on x86 with Diamond/ATI Fire GL2 AGP
card:
01:00.0 VGA compatible controller [0300]: IBM GXT6000P Graphics Adapter [1014:0170] (rev 02) (prog-if 00 [VGA controller])
Subsystem: Diamond Multimedia Systems Fire GL2 [1092:0172]
Control: I/O+ Mem+ BusMaster- SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR+ FastB2B+ DisINTx-
Status: Cap+ 66MHz+ UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
Interrupt: pin A routed to IRQ 16
Region 0: Memory at f5000000 (32-bit, non-prefetchable) [size\x128K]
Region 1: Memory at f0000000 (32-bit, non-prefetchable) [sizedM]
[virtual] Expansion ROM at 30000000 [disabled] [size%6K]
Capabilities: [60] Power Management version 0
Flags: PMEClk- DSI+ D1+ D2+ AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot-,D3cold-)
Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME-
Capabilities: [70] AGP version 2.0
Status: RQ2 Iso- ArqSz=0 Cal=0 SBA+ ITACoh- GART64- HTrans- 64bit- FW- AGP3- Rate=x1,x2,x4
Command: RQ=1 ArqSz=0 Cal=0 SBA- AGP- GART64- 64bit- FW- Rate=x1
Kernel driver in use: gxt4500
After patching Kconfig, the driver compiled but does not work - monitor shows
no signal after driver load. Realized that PPC is big-endian while x86 is
little-endian and there are no endian conversions in the driver so did this:
--- a/drivers/video/fbdev/gxt4500.c
+++ b/drivers/video/fbdev/gxt4500.c
@@ -137,8 +137,8 @@ static const unsigned char watfmt[] = {
/* Colormap array; 1k entries of 4 bytes each */
#define CMAP 0x6000
-#define readreg(par, reg) readl((par)->regs + (reg))
-#define writereg(par, reg, val) writel((val), (par)->regs + (reg))
+#define readreg(par, reg) be32_to_cpu(readl((par)->regs + (reg)))
+#define writereg(par, reg, val) writel(cpu_to_be32(val), (par)->regs + (reg))
struct gxt4500_par {
void __iomem *regs;
But nothing changed, still no signal. Any ideas?
I was able to get the ancient fgl23 driver (from AMD web site) to work by
installing Debian Woody to a small partition. It's not very useful today but
it might be possible to dump the chip's registers and compare with gxt4500.
--
Ondrej Zary
^ permalink raw reply
* [PATCH 1/2]Trivial patch: to solve indentation warnings in amba-clcd.c driver
From: Parmeshwr Prasad @ 2015-02-13 11:21 UTC (permalink / raw)
To: linux; +Cc: plagnioj, tomi.valkeinen, trivial, linux-fbdev, linux-kernel
Hi All,
This patch soves the indentation warning in amba-clcd.c file.
Please review the same.
From a5f58880b2a6fd7cc532c4fb2bf6543cf1945195 Mon Sep 17 00:00:00 2001
From: Parmeshwr Prasad <parmeshwr_prasad@dell.com>
Date: Thu, 12 Feb 2015 07:30:04 -0500
Subject: [PATCH 1/2] Trivial patch: Removed indentation warnings
Signed-off-by: Parmeshwr Prasad <parmeshwr_prasad@dell.com>
---
drivers/video/fbdev/amba-clcd.c | 234 ++++++++++++++++++++--------------------
1 file changed, 118 insertions(+), 116 deletions(-)
diff --git a/drivers/video/fbdev/amba-clcd.c b/drivers/video/fbdev/amba-clcd.c
index 32c0b6b..0ddc1f0 100644
--- a/drivers/video/fbdev/amba-clcd.c
+++ b/drivers/video/fbdev/amba-clcd.c
@@ -135,8 +135,7 @@ clcdfb_set_bitfields(struct clcd_fb *fb, struct
fb_var_screeninfo *var)
caps = fb->panel->caps & fb->board->caps;
else {
/* Old way of specifying what can be used */
- caps = fb->panel->cntl & CNTL_BGR ?
- CLCD_CAP_BGR : CLCD_CAP_RGB;
+ caps = fb->panel->cntl & CNTL_BGR ? CLCD_CAP_BGR : CLCD_CAP_RGB;
/* But mask out 444 modes as they weren't supported */
caps &= ~CLCD_CAP_444;
}
@@ -163,12 +162,12 @@ clcdfb_set_bitfields(struct clcd_fb *fb, struct
fb_var_screeninfo *var)
break;
}
- var->red.length = var->bits_per_pixel;
- var->red.offset = 0;
- var->green.length = var->bits_per_pixel;
- var->green.offset = 0;
- var->blue.length = var->bits_per_pixel;
- var->blue.offset = 0;
+ var->red.length = var->bits_per_pixel;
+ var->red.offset = 0;
+ var->green.length = var->bits_per_pixel;
+ var->green.offset = 0;
+ var->blue.length = var->bits_per_pixel;
+ var->blue.offset = 0;
break;
case 16:
@@ -256,7 +255,8 @@ clcdfb_set_bitfields(struct clcd_fb *fb, struct
fb_var_screeninfo *var)
} else {
var->red.offset = 0;
var->green.offset = var->red.offset + var->red.length;
- var->blue.offset = var->green.offset +
var->green.length;
+ var->blue.offset + var->green.offset + var->green.length;
}
}
@@ -288,7 +288,7 @@ static int clcdfb_set_par(struct fb_info *info)
struct clcd_regs regs;
fb->fb.fix.line_length = fb->fb.var.xres_virtual *
- fb->fb.var.bits_per_pixel / 8;
+ fb->fb.var.bits_per_pixel / 8;
if (fb->fb.var.bits_per_pixel <= 8)
fb->fb.fix.visual = FB_VISUAL_PSEUDOCOLOR;
@@ -317,10 +317,10 @@ static int clcdfb_set_par(struct fb_info *info)
"CLCD: Registers set to\n"
" %08x %08x %08x %08x\n"
" %08x %08x %08x %08x\n",
- readl(fb->regs + CLCD_TIM0), readl(fb->regs + CLCD_TIM1),
- readl(fb->regs + CLCD_TIM2), readl(fb->regs + CLCD_TIM3),
- readl(fb->regs + CLCD_UBAS), readl(fb->regs + CLCD_LBAS),
- readl(fb->regs + fb->off_ienb), readl(fb->regs + fb->off_cntl));
+ readl(fb->regs + CLCD_TIM0), readl(fb->regs + CLCD_TIM1),
+ readl(fb->regs + CLCD_TIM2), readl(fb->regs + CLCD_TIM3),
+ readl(fb->regs + CLCD_UBAS), readl(fb->regs + CLCD_LBAS),
+ readl(fb->regs + fb->off_ienb), readl(fb->regs + fb->off_cntl));
#endif
return 0;
@@ -345,17 +345,17 @@ clcdfb_setcolreg(unsigned int regno, unsigned int red,
unsigned int green,
if (regno < 16)
fb->cmap[regno] = convert_bitfield(transp, &fb->fb.var.transp) |
- convert_bitfield(blue, &fb->fb.var.blue) |
- convert_bitfield(green, &fb->fb.var.green) |
- convert_bitfield(red, &fb->fb.var.red);
+ convert_bitfield(blue, &fb->fb.var.blue) |
+ convert_bitfield(green, &fb->fb.var.green) |
+ convert_bitfield(red, &fb->fb.var.red);
if (fb->fb.fix.visual = FB_VISUAL_PSEUDOCOLOR && regno < 256) {
int hw_reg = CLCD_PALETTE + ((regno * 2) & ~3);
u32 val, mask, newval;
- newval = (red >> 11) & 0x001f;
+ newval = (red >> 11) & 0x001f;
newval |= (green >> 6) & 0x03e0;
- newval |= (blue >> 1) & 0x7c00;
+ newval |= (blue >> 1) & 0x7c00;
/*
* 3.2.11: if we're configured for big endian
@@ -400,8 +400,7 @@ static int clcdfb_blank(int blank_mode, struct fb_info
*info)
return 0;
}
-static int clcdfb_mmap(struct fb_info *info,
- struct vm_area_struct *vma)
+static int clcdfb_mmap(struct fb_info *info, struct vm_area_struct *vma)
{
struct clcd_fb *fb = to_clcd(info);
unsigned long len, off = vma->vm_pgoff << PAGE_SHIFT;
@@ -417,15 +416,15 @@ static int clcdfb_mmap(struct fb_info *info,
}
static struct fb_ops clcdfb_ops = {
- .owner = THIS_MODULE,
- .fb_check_var = clcdfb_check_var,
- .fb_set_par = clcdfb_set_par,
- .fb_setcolreg = clcdfb_setcolreg,
- .fb_blank = clcdfb_blank,
- .fb_fillrect = cfb_fillrect,
- .fb_copyarea = cfb_copyarea,
- .fb_imageblit = cfb_imageblit,
- .fb_mmap = clcdfb_mmap,
+ .owner = THIS_MODULE,
+ .fb_check_var = clcdfb_check_var,
+ .fb_set_par = clcdfb_set_par,
+ .fb_setcolreg = clcdfb_setcolreg,
+ .fb_blank = clcdfb_blank,
+ .fb_fillrect = cfb_fillrect,
+ .fb_copyarea = cfb_copyarea,
+ .fb_imageblit = cfb_imageblit,
+ .fb_mmap = clcdfb_mmap,
};
static int clcdfb_register(struct clcd_fb *fb)
@@ -459,10 +458,10 @@ static int clcdfb_register(struct clcd_fb *fb)
if (ret)
goto free_clk;
- fb->fb.device = &fb->dev->dev;
+ fb->fb.device = &fb->dev->dev;
- fb->fb.fix.mmio_start = fb->dev->res.start;
- fb->fb.fix.mmio_len = resource_size(&fb->dev->res);
+ fb->fb.fix.mmio_start = fb->dev->res.start;
+ fb->fb.fix.mmio_len = resource_size(&fb->dev->res);
fb->regs = ioremap(fb->fb.fix.mmio_start, fb->fb.fix.mmio_len);
if (!fb->regs) {
@@ -471,45 +470,45 @@ static int clcdfb_register(struct clcd_fb *fb)
goto clk_unprep;
}
- fb->fb.fbops = &clcdfb_ops;
- fb->fb.flags = FBINFO_FLAG_DEFAULT;
- fb->fb.pseudo_palette = fb->cmap;
+ fb->fb.fbops = &clcdfb_ops;
+ fb->fb.flags = FBINFO_FLAG_DEFAULT;
+ fb->fb.pseudo_palette = fb->cmap;
strncpy(fb->fb.fix.id, clcd_name, sizeof(fb->fb.fix.id));
- fb->fb.fix.type = FB_TYPE_PACKED_PIXELS;
- fb->fb.fix.type_aux = 0;
- fb->fb.fix.xpanstep = 0;
- fb->fb.fix.ypanstep = 0;
- fb->fb.fix.ywrapstep = 0;
- fb->fb.fix.accel = FB_ACCEL_NONE;
-
- fb->fb.var.xres = fb->panel->mode.xres;
- fb->fb.var.yres = fb->panel->mode.yres;
- fb->fb.var.xres_virtual = fb->panel->mode.xres;
- fb->fb.var.yres_virtual = fb->panel->mode.yres;
+ fb->fb.fix.type = FB_TYPE_PACKED_PIXELS;
+ fb->fb.fix.type_aux = 0;
+ fb->fb.fix.xpanstep = 0;
+ fb->fb.fix.ypanstep = 0;
+ fb->fb.fix.ywrapstep = 0;
+ fb->fb.fix.accel = FB_ACCEL_NONE;
+
+ fb->fb.var.xres = fb->panel->mode.xres;
+ fb->fb.var.yres = fb->panel->mode.yres;
+ fb->fb.var.xres_virtual = fb->panel->mode.xres;
+ fb->fb.var.yres_virtual = fb->panel->mode.yres;
fb->fb.var.bits_per_pixel = fb->panel->bpp;
- fb->fb.var.grayscale = fb->panel->grayscale;
- fb->fb.var.pixclock = fb->panel->mode.pixclock;
- fb->fb.var.left_margin = fb->panel->mode.left_margin;
- fb->fb.var.right_margin = fb->panel->mode.right_margin;
- fb->fb.var.upper_margin = fb->panel->mode.upper_margin;
- fb->fb.var.lower_margin = fb->panel->mode.lower_margin;
- fb->fb.var.hsync_len = fb->panel->mode.hsync_len;
- fb->fb.var.vsync_len = fb->panel->mode.vsync_len;
- fb->fb.var.sync = fb->panel->mode.sync;
- fb->fb.var.vmode = fb->panel->mode.vmode;
- fb->fb.var.activate = FB_ACTIVATE_NOW;
- fb->fb.var.nonstd = 0;
- fb->fb.var.height = fb->panel->height;
- fb->fb.var.width = fb->panel->width;
- fb->fb.var.accel_flags = 0;
-
- fb->fb.monspecs.hfmin = 0;
- fb->fb.monspecs.hfmax = 100000;
- fb->fb.monspecs.vfmin = 0;
- fb->fb.monspecs.vfmax = 400;
+ fb->fb.var.grayscale = fb->panel->grayscale;
+ fb->fb.var.pixclock = fb->panel->mode.pixclock;
+ fb->fb.var.left_margin = fb->panel->mode.left_margin;
+ fb->fb.var.right_margin = fb->panel->mode.right_margin;
+ fb->fb.var.upper_margin = fb->panel->mode.upper_margin;
+ fb->fb.var.lower_margin = fb->panel->mode.lower_margin;
+ fb->fb.var.hsync_len = fb->panel->mode.hsync_len;
+ fb->fb.var.vsync_len = fb->panel->mode.vsync_len;
+ fb->fb.var.sync = fb->panel->mode.sync;
+ fb->fb.var.vmode = fb->panel->mode.vmode;
+ fb->fb.var.activate = FB_ACTIVATE_NOW;
+ fb->fb.var.nonstd = 0;
+ fb->fb.var.height = fb->panel->height;
+ fb->fb.var.width = fb->panel->width;
+ fb->fb.var.accel_flags = 0;
+
+ fb->fb.monspecs.hfmin = 0;
+ fb->fb.monspecs.hfmax = 100000;
+ fb->fb.monspecs.vfmin = 0;
+ fb->fb.monspecs.vfmax = 400;
fb->fb.monspecs.dclkmin = 1000000;
- fb->fb.monspecs.dclkmax = 100000000;
+ fb->fb.monspecs.dclkmax = 100000000;
/*
* Make sure that the bitfields are set appropriately.
@@ -531,7 +530,7 @@ static int clcdfb_register(struct clcd_fb *fb)
fb_set_var(&fb->fb, &fb->fb.var);
dev_info(&fb->dev->dev, "%s hardware, %s display\n",
- fb->board->name, fb->panel->mode.name);
+ fb->board->name, fb->panel->mode.name);
ret = register_framebuffer(&fb->fb);
if (ret = 0)
@@ -540,19 +539,19 @@ static int clcdfb_register(struct clcd_fb *fb)
printk(KERN_ERR "CLCD: cannot register framebuffer (%d)\n", ret);
fb_dealloc_cmap(&fb->fb.cmap);
- unmap:
+unmap:
iounmap(fb->regs);
- clk_unprep:
+clk_unprep:
clk_unprepare(fb->clk);
- free_clk:
+free_clk:
clk_put(fb->clk);
- out:
+out:
return ret;
}
#ifdef CONFIG_OF
static int clcdfb_of_get_dpi_panel_mode(struct device_node *node,
- struct fb_videomode *mode)
+ struct fb_videomode *mode)
{
int err;
struct display_timing timing;
@@ -578,7 +577,7 @@ static int clcdfb_snprintf_mode(char *buf, int size, struct
fb_videomode *mode)
}
static int clcdfb_of_get_mode(struct device *dev, struct device_node *endpoint,
- struct fb_videomode *mode)
+ struct fb_videomode *mode)
{
int err;
struct device_node *panel;
@@ -612,15 +611,15 @@ static int clcdfb_of_init_tft_panel(struct clcd_fb *fb,
u32 r0, u32 g0, u32 b0)
u32 r0, g0, b0;
u32 caps;
} panels[] = {
- { 0x110, 1, 7, 13, CLCD_CAP_5551 },
- { 0x110, 0, 8, 16, CLCD_CAP_888 },
- { 0x111, 4, 14, 20, CLCD_CAP_444 },
- { 0x111, 3, 11, 19, CLCD_CAP_444 | CLCD_CAP_5551 },
- { 0x111, 3, 10, 19, CLCD_CAP_444 | CLCD_CAP_5551 |
- CLCD_CAP_565 },
- { 0x111, 0, 8, 16, CLCD_CAP_444 | CLCD_CAP_5551 |
- CLCD_CAP_565 | CLCD_CAP_888 },
- };
+ {
+ 0x110, 1, 7, 13, CLCD_CAP_5551}, {
+ 0x110, 0, 8, 16, CLCD_CAP_888}, {
+ 0x111, 4, 14, 20, CLCD_CAP_444}, {
+ 0x111, 3, 11, 19, CLCD_CAP_444 | CLCD_CAP_5551}, {
+ 0x111, 3, 10, 19, CLCD_CAP_444 | CLCD_CAP_5551 |
+ CLCD_CAP_565}, {
+ 0x111, 0, 8, 16, CLCD_CAP_444 | CLCD_CAP_5551 |
+ CLCD_CAP_565 | CLCD_CAP_888},};
int i;
/* Bypass pixel clock divider, data output on the falling edge */
@@ -665,7 +664,7 @@ static int clcdfb_of_init_display(struct clcd_fb *fb)
return err;
err = of_property_read_u32(fb->dev->dev.of_node, "max-memory-bandwidth",
- &max_bandwidth);
+ &max_bandwidth);
if (!err) {
/*
* max_bandwidth is in bytes per second and pixclock in
@@ -675,7 +674,7 @@ static int clcdfb_of_init_display(struct clcd_fb *fb)
* result is a valid format.
*/
bpp = max_bandwidth / (1000 / 8)
- / PICOS2KHZ(fb->panel->mode.pixclock);
+ / PICOS2KHZ(fb->panel->mode.pixclock);
bpp = rounddown_pow_of_two(bpp);
if (bpp > 32)
bpp = 32;
@@ -690,10 +689,10 @@ static int clcdfb_of_init_display(struct clcd_fb *fb)
fb->panel->height = -1;
if (of_property_read_u32_array(endpoint,
- "arm,pl11x,tft-r0g0b0-pads",
- tft_r0b0g0, ARRAY_SIZE(tft_r0b0g0)) = 0)
+ "arm,pl11x,tft-r0g0b0-pads",
+ tft_r0b0g0, ARRAY_SIZE(tft_r0b0g0)) = 0)
return clcdfb_of_init_tft_panel(fb, tft_r0b0g0[0],
- tft_r0b0g0[1], tft_r0b0g0[2]);
+ tft_r0b0g0[1], tft_r0b0g0[2]);
return -ENOENT;
}
@@ -717,7 +716,9 @@ static int clcdfb_of_vram_setup(struct clcd_fb *fb)
return -ENOMEM;
fb->fb.fix.smem_start = of_translate_address(memory,
- of_get_address(memory, 0, &size, NULL));
+ of_get_address(memory, 0,
+ &size,
+ NULL));
fb->fb.fix.smem_len = size;
return 0;
@@ -727,7 +728,6 @@ static int clcdfb_of_vram_mmap(struct clcd_fb *fb, struct
vm_area_struct *vma)
{
unsigned long off, user_size, kernel_size;
-
off = vma->vm_pgoff << PAGE_SHIFT;
user_size = vma->vm_end - vma->vm_start;
kernel_size = fb->fb.fix.smem_len;
@@ -736,9 +736,9 @@ static int clcdfb_of_vram_mmap(struct clcd_fb *fb, struct
vm_area_struct *vma)
return -ENXIO;
return remap_pfn_range(vma, vma->vm_start,
- __phys_to_pfn(fb->fb.fix.smem_start) + vma->vm_pgoff,
- user_size,
- pgprot_writecombine(vma->vm_page_prot));
+ __phys_to_pfn(fb->fb.fix.smem_start) +
+ vma->vm_pgoff, user_size,
+ pgprot_writecombine(vma->vm_page_prot));
}
static void clcdfb_of_vram_remove(struct clcd_fb *fb)
@@ -757,9 +757,9 @@ static int clcdfb_of_dma_setup(struct clcd_fb *fb)
return err;
framesize = fb->panel->mode.xres * fb->panel->mode.yres *
- fb->panel->bpp / 8;
+ fb->panel->bpp / 8;
fb->fb.screen_base = dma_alloc_coherent(&fb->dev->dev, framesize,
- &dma, GFP_KERNEL);
+ &dma, GFP_KERNEL);
if (!fb->fb.screen_base)
return -ENOMEM;
@@ -772,19 +772,20 @@ static int clcdfb_of_dma_setup(struct clcd_fb *fb)
static int clcdfb_of_dma_mmap(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);
+ fb->fb.fix.smem_start,
+ fb->fb.fix.smem_len);
}
static void clcdfb_of_dma_remove(struct clcd_fb *fb)
{
dma_free_coherent(&fb->dev->dev, fb->fb.fix.smem_len,
- fb->fb.screen_base, fb->fb.fix.smem_start);
+ fb->fb.screen_base, fb->fb.fix.smem_start);
}
static struct clcd_board *clcdfb_of_get_board(struct amba_device *dev)
{
struct clcd_board *board = devm_kzalloc(&dev->dev, sizeof(*board),
- GFP_KERNEL);
+ GFP_KERNEL);
struct device_node *node = dev->dev.of_node;
if (!board)
@@ -837,7 +838,8 @@ static int clcdfb_probe(struct amba_device *dev, const
struct amba_id *id)
fb = kzalloc(sizeof(struct clcd_fb), GFP_KERNEL);
if (!fb) {
- printk(KERN_INFO "CLCD: could not allocate new clcd_fb
struct\n");
+ printk(KERN_INFO
+ "CLCD: could not allocate new clcd_fb struct\n");
ret = -ENOMEM;
goto free_region;
}
@@ -846,25 +848,25 @@ static int clcdfb_probe(struct amba_device *dev, const
struct amba_id *id)
fb->board = board;
dev_info(&fb->dev->dev, "PL%03x rev%u at 0x%08llx\n",
- amba_part(dev), amba_rev(dev),
- (unsigned long long)dev->res.start);
+ amba_part(dev), amba_rev(dev),
+ (unsigned long long)dev->res.start);
ret = fb->board->setup(fb);
if (ret)
goto free_fb;
- ret = clcdfb_register(fb);
+ ret = clcdfb_register(fb);
if (ret = 0) {
amba_set_drvdata(dev, fb);
goto out;
}
fb->board->remove(fb);
- free_fb:
+free_fb:
kfree(fb);
- free_region:
+free_region:
amba_release_regions(dev);
- out:
+out:
return ret;
}
@@ -891,21 +893,21 @@ static int clcdfb_remove(struct amba_device *dev)
static struct amba_id clcdfb_id_table[] = {
{
- .id = 0x00041110,
- .mask = 0x000ffffe,
- },
- { 0, 0 },
+ .id = 0x00041110,
+ .mask = 0x000ffffe,
+ },
+ {0, 0},
};
MODULE_DEVICE_TABLE(amba, clcdfb_id_table);
static struct amba_driver clcd_driver = {
- .drv = {
- .name = "clcd-pl11x",
- },
- .probe = clcdfb_probe,
- .remove = clcdfb_remove,
- .id_table = clcdfb_id_table,
+ .drv = {
+ .name = "clcd-pl11x",
+ },
+ .probe = clcdfb_probe,
+ .remove = clcdfb_remove,
+ .id_table = clcdfb_id_table,
};
static int __init amba_clcdfb_init(void)
--
1.9.3
-Parmeshwr
^ permalink raw reply related
* Re: [PATCH 1/2]Trivial patch: to solve indentation warnings in amba-clcd.c driver
From: Russell King - ARM Linux @ 2015-02-13 11:35 UTC (permalink / raw)
To: Parmeshwr Prasad
Cc: plagnioj, tomi.valkeinen, trivial, linux-fbdev, linux-kernel
In-Reply-To: <20150213112125.GA25078@linuxteamdev.amer.dell.com>
On Fri, Feb 13, 2015 at 06:21:33AM -0500, Parmeshwr Prasad wrote:
> @@ -288,7 +288,7 @@ static int clcdfb_set_par(struct fb_info *info)
> struct clcd_regs regs;
>
> fb->fb.fix.line_length = fb->fb.var.xres_virtual *
> - fb->fb.var.bits_per_pixel / 8;
> + fb->fb.var.bits_per_pixel / 8;
NAK on this one. The code as it stood before is much clearer since
we align the expression with the start of it on the preceding line.
> @@ -345,17 +345,17 @@ clcdfb_setcolreg(unsigned int regno, unsigned int red,
> unsigned int green,
>
> if (regno < 16)
> fb->cmap[regno] = convert_bitfield(transp, &fb->fb.var.transp) |
> - convert_bitfield(blue, &fb->fb.var.blue) |
> - convert_bitfield(green, &fb->fb.var.green) |
> - convert_bitfield(red, &fb->fb.var.red);
> + convert_bitfield(blue, &fb->fb.var.blue) |
> + convert_bitfield(green, &fb->fb.var.green) |
> + convert_bitfield(red, &fb->fb.var.red);
Ditto.
> @@ -612,15 +611,15 @@ static int clcdfb_of_init_tft_panel(struct clcd_fb *fb,
> u32 r0, u32 g0, u32 b0)
> u32 r0, g0, b0;
> u32 caps;
> } panels[] = {
> - { 0x110, 1, 7, 13, CLCD_CAP_5551 },
> - { 0x110, 0, 8, 16, CLCD_CAP_888 },
> - { 0x111, 4, 14, 20, CLCD_CAP_444 },
> - { 0x111, 3, 11, 19, CLCD_CAP_444 | CLCD_CAP_5551 },
> - { 0x111, 3, 10, 19, CLCD_CAP_444 | CLCD_CAP_5551 |
> - CLCD_CAP_565 },
> - { 0x111, 0, 8, 16, CLCD_CAP_444 | CLCD_CAP_5551 |
> - CLCD_CAP_565 | CLCD_CAP_888 },
> - };
> + {
> + 0x110, 1, 7, 13, CLCD_CAP_5551}, {
> + 0x110, 0, 8, 16, CLCD_CAP_888}, {
> + 0x111, 4, 14, 20, CLCD_CAP_444}, {
> + 0x111, 3, 11, 19, CLCD_CAP_444 | CLCD_CAP_5551}, {
> + 0x111, 3, 10, 19, CLCD_CAP_444 | CLCD_CAP_5551 |
> + CLCD_CAP_565}, {
> + 0x111, 0, 8, 16, CLCD_CAP_444 | CLCD_CAP_5551 |
> + CLCD_CAP_565 | CLCD_CAP_888},};
The original is better than what you're proposing.
> @@ -675,7 +674,7 @@ static int clcdfb_of_init_display(struct clcd_fb *fb)
> * result is a valid format.
> */
> bpp = max_bandwidth / (1000 / 8)
> - / PICOS2KHZ(fb->panel->mode.pixclock);
> + / PICOS2KHZ(fb->panel->mode.pixclock);
Not quite enough indentation.
> @@ -717,7 +716,9 @@ static int clcdfb_of_vram_setup(struct clcd_fb *fb)
> return -ENOMEM;
>
> fb->fb.fix.smem_start = of_translate_address(memory,
> - of_get_address(memory, 0, &size, NULL));
> + of_get_address(memory, 0,
> + &size,
> + NULL));
Thi sis the exception to the rule - where scrunching an expression so that
it takes multiple lines because of lack of right-hand space is not on.
The former version was a lot better.
> @@ -757,9 +757,9 @@ static int clcdfb_of_dma_setup(struct clcd_fb *fb)
> return err;
>
> framesize = fb->panel->mode.xres * fb->panel->mode.yres *
> - fb->panel->bpp / 8;
> + fb->panel->bpp / 8;
Similar comments as before.
> @@ -837,7 +838,8 @@ static int clcdfb_probe(struct amba_device *dev, const
> struct amba_id *id)
>
> fb = kzalloc(sizeof(struct clcd_fb), GFP_KERNEL);
> if (!fb) {
> - printk(KERN_INFO "CLCD: could not allocate new clcd_fb
> struct\n");
> + printk(KERN_INFO
> + "CLCD: could not allocate new clcd_fb struct\n");
Converting this to pr_info() would be better, or even dev_err().
> @@ -891,21 +893,21 @@ static int clcdfb_remove(struct amba_device *dev)
>
> static struct amba_id clcdfb_id_table[] = {
> {
> - .id = 0x00041110,
> - .mask = 0x000ffffe,
> - },
> - { 0, 0 },
> + .id = 0x00041110,
> + .mask = 0x000ffffe,
> + },
> + {0, 0},
The original is loads better.
> };
>
> MODULE_DEVICE_TABLE(amba, clcdfb_id_table);
>
> static struct amba_driver clcd_driver = {
> - .drv = {
> - .name = "clcd-pl11x",
> - },
> - .probe = clcdfb_probe,
> - .remove = clcdfb_remove,
> - .id_table = clcdfb_id_table,
> + .drv = {
> + .name = "clcd-pl11x",
> + },
Yuck. Again, the original is loads better.
And... your message seems to suffer total whitespace breakage - all
tabs seem to be converted to 8 spaces. Your patch will not apply
as long as that kind of breakage exists...
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply
* Re: [PATCH 1/2]Trivial patch: to solve indentation warnings in amba-clcd.c driver
From: Joe Perches @ 2015-02-13 11:59 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Parmeshwr Prasad, plagnioj, tomi.valkeinen, trivial, linux-fbdev,
linux-kernel
In-Reply-To: <20150213113515.GW8656@n2100.arm.linux.org.uk>
On Fri, 2015-02-13 at 11:35 +0000, Russell King - ARM Linux wrote:
> On Fri, Feb 13, 2015 at 06:21:33AM -0500, Parmeshwr Prasad wrote:
> > @@ -288,7 +288,7 @@ static int clcdfb_set_par(struct fb_info *info)
> > struct clcd_regs regs;
> >
> > fb->fb.fix.line_length = fb->fb.var.xres_virtual *
> > - fb->fb.var.bits_per_pixel / 8;
> > + fb->fb.var.bits_per_pixel / 8;
>
> NAK on this one. The code as it stood before is much clearer since
> we align the expression with the start of it on the preceding line.
I agree with all of what Russell wrote, but maybe this;
> > @@ -717,7 +716,9 @@ static int clcdfb_of_vram_setup(struct clcd_fb *fb)
> > return -ENOMEM;
> >
> > fb->fb.fix.smem_start = of_translate_address(memory,
> > - of_get_address(memory, 0, &size, NULL));
> > + of_get_address(memory, 0,
> > + &size,
> > + NULL));
>
> Thi sis the exception to the rule - where scrunching an expression so that
> it takes multiple lines because of lack of right-hand space is not on.
> The former version was a lot better.
Perhaps this could be better as:
fb->fb.fix.smem_start of_translate_address(memory,
of_get_address(memory, 0, &size, NULL));
But sometimes using multiple statements instead of
embedding function calls as arguments can be simpler
and more intelligible for the reader.
__be32 addr;
...
addr = of_get_address(memory, 0, &size, NULL);
fb->fb.fix.smem_start = of_translate_address(memory, addr);
^ permalink raw reply
* Re: [PATCH 1/2]Trivial patch: to solve indentation warnings in amba-clcd.c driver
From: Russell King - ARM Linux @ 2015-02-13 12:03 UTC (permalink / raw)
To: Joe Perches
Cc: Parmeshwr Prasad, plagnioj, tomi.valkeinen, trivial, linux-fbdev,
linux-kernel
In-Reply-To: <1423828796.2795.7.camel@perches.com>
On Fri, Feb 13, 2015 at 03:59:56AM -0800, Joe Perches wrote:
> Perhaps this could be better as:
>
> fb->fb.fix.smem_start > of_translate_address(memory,
> of_get_address(memory, 0, &size, NULL));
>
> But sometimes using multiple statements instead of
> embedding function calls as arguments can be simpler
> and more intelligible for the reader.
>
> __be32 addr;
>
> ...
>
> addr = of_get_address(memory, 0, &size, NULL);
> fb->fb.fix.smem_start = of_translate_address(memory, addr);
Yes, that would be a better solution, thanks.
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply
* Re: [PATCH 2/2]Trivial patch: to solve indentation warnings in amba-clcd.c driver
From: Parmeshwr Prasad @ 2015-02-13 13:28 UTC (permalink / raw)
To: Joe Perches
Cc: Russell King - ARM Linux, plagnioj@jcrosoft.com,
tomi.valkeinen@ti.com, trivial@kernel.org,
linux-fbdev@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <1423828796.2795.7.camel@perches.com>
This is second patch in series.
In driver in_atomic we should not use to check if code is unning in IRQ.
clcdfb_sleep() function is used to give some delay between operation.
I have used schedule_timeout() function for same amount of delay.
Please review the same.
Patch 1/2 I will fix comments and re-submitt.
From 06b275e44a3951ec0a57af59aea2a4c82595e456 Mon Sep 17 00:00:00 2001
From: Parmeshwr Prasad <parmeshwr_prasad@dell.com>
Date: Fri, 13 Feb 2015 06:10:34 -0500
Subject: [PATCH 2/2] Patch to replace in_atomic with proper function from
amba-clcd.c driver. In driver code in_atomic should not be used to check the
IRQ or kernel context. to remove that I have used schedule_timeout() with
respective delay. In this patch I have also removed printk() with respective
pr_* functions
Signed-off-by: Parmeshwr Prasad <parmeshwr_prasad@dell.com>
---
drivers/video/fbdev/amba-clcd.c | 48 +++++++++++++++++++----------------------
1 file changed, 22 insertions(+), 26 deletions(-)
diff --git a/drivers/video/fbdev/amba-clcd.c b/drivers/video/fbdev/amba-clcd.c
index 0ddc1f0..7e630f5c 100644
--- a/drivers/video/fbdev/amba-clcd.c
+++ b/drivers/video/fbdev/amba-clcd.c
@@ -33,8 +33,9 @@
#include <video/display_timing.h>
#include <video/of_display_timing.h>
#include <video/videomode.h>
-
-#include <asm/sizes.h>
+#include <linux/jiffies.h>
+#include <linux/sched.h>
+#include <linux/sizes.h>
#define to_clcd(info) container_of(info, struct clcd_fb, fb)
@@ -42,16 +43,15 @@
static const char *clcd_name = "CLCD FB";
/*
- * Unfortunately, the enable/disable functions may be called either from
- * process or IRQ context, and we _need_ to delay. This is _not_ good.
+ * Hardware need certain time for power setting
*/
-static inline void clcdfb_sleep(unsigned int ms)
+static inline void clcdfb_sleep(signed long ms)
{
- if (in_atomic()) {
- mdelay(ms);
- } else {
- msleep(ms);
- }
+ unsigned long delay;
+
+ delay = jiffies + (HZ / 1000) * ms;
+ set_current_state(TASK_INTERRUPTIBLE);
+ schedule_timeout(delay);
}
static inline void clcdfb_set_start(struct clcd_fb *fb)
@@ -313,14 +313,13 @@ static int clcdfb_set_par(struct fb_info *info)
clcdfb_enable(fb, regs.cntl);
#ifdef DEBUG
- printk(KERN_INFO
- "CLCD: Registers set to\n"
- " %08x %08x %08x %08x\n"
- " %08x %08x %08x %08x\n",
- readl(fb->regs + CLCD_TIM0), readl(fb->regs + CLCD_TIM1),
- readl(fb->regs + CLCD_TIM2), readl(fb->regs + CLCD_TIM3),
- readl(fb->regs + CLCD_UBAS), readl(fb->regs + CLCD_LBAS),
- readl(fb->regs + fb->off_ienb), readl(fb->regs + fb->off_cntl));
+ pr_info("CLCD: Registers set to\n"
+ " %08x %08x %08x %08x\n"
+ " %08x %08x %08x %08x\n",
+ readl(fb->regs + CLCD_TIM0), readl(fb->regs + CLCD_TIM1),
+ readl(fb->regs + CLCD_TIM2), readl(fb->regs + CLCD_TIM3),
+ readl(fb->regs + CLCD_UBAS), readl(fb->regs + CLCD_LBAS),
+ readl(fb->regs + fb->off_ienb), readl(fb->regs + fb->off_cntl));
#endif
return 0;
@@ -392,11 +391,10 @@ static int clcdfb_blank(int blank_mode, struct fb_info
*info)
{
struct clcd_fb *fb = to_clcd(info);
- if (blank_mode != 0) {
+ if (blank_mode != 0)
clcdfb_disable(fb);
- } else {
+ else
clcdfb_enable(fb, fb->clcd_cntl);
- }
return 0;
}
@@ -465,7 +463,7 @@ static int clcdfb_register(struct clcd_fb *fb)
fb->regs = ioremap(fb->fb.fix.mmio_start, fb->fb.fix.mmio_len);
if (!fb->regs) {
- printk(KERN_ERR "CLCD: unable to remap registers\n");
+ pr_err("CLCD: unable to remap registers\n");
ret = -ENOMEM;
goto clk_unprep;
}
@@ -536,7 +534,7 @@ static int clcdfb_register(struct clcd_fb *fb)
if (ret = 0)
goto out;
- printk(KERN_ERR "CLCD: cannot register framebuffer (%d)\n", ret);
+ pr_info("CLCD: cannot register framebuffer (%d)\n", ret);
fb_dealloc_cmap(&fb->fb.cmap);
unmap:
@@ -832,14 +830,12 @@ static int clcdfb_probe(struct amba_device *dev, const
struct amba_id *id)
ret = amba_request_regions(dev, NULL);
if (ret) {
- printk(KERN_ERR "CLCD: unable to reserve regs region\n");
+ pr_err("CLCD: unable to reserve regs region\n");
goto out;
}
fb = kzalloc(sizeof(struct clcd_fb), GFP_KERNEL);
if (!fb) {
- printk(KERN_INFO
- "CLCD: could not allocate new clcd_fb struct\n");
ret = -ENOMEM;
goto free_region;
}
--
1.9.3
-Parmeshwr
On Fri, Feb 13, 2015 at 05:59:56AM -0600, Joe Perches wrote:
> On Fri, 2015-02-13 at 11:35 +0000, Russell King - ARM Linux wrote:
> > On Fri, Feb 13, 2015 at 06:21:33AM -0500, Parmeshwr Prasad wrote:
> > > @@ -288,7 +288,7 @@ static int clcdfb_set_par(struct fb_info *info)
> > > struct clcd_regs regs;
> > >
> > > fb->fb.fix.line_length = fb->fb.var.xres_virtual *
> > > - fb->fb.var.bits_per_pixel / 8;
> > > + fb->fb.var.bits_per_pixel / 8;
> >
> > NAK on this one. The code as it stood before is much clearer since
> > we align the expression with the start of it on the preceding line.
>
> I agree with all of what Russell wrote, but maybe this;
>
> > > @@ -717,7 +716,9 @@ static int clcdfb_of_vram_setup(struct clcd_fb *fb)
> > > return -ENOMEM;
> > >
> > > fb->fb.fix.smem_start = of_translate_address(memory,
> > > - of_get_address(memory, 0, &size, NULL));
> > > + of_get_address(memory, 0,
> > > + &size,
> > > + NULL));
> >
> > Thi sis the exception to the rule - where scrunching an expression so that
> > it takes multiple lines because of lack of right-hand space is not on.
> > The former version was a lot better.
>
> Perhaps this could be better as:
>
> fb->fb.fix.smem_start > of_translate_address(memory,
> of_get_address(memory, 0, &size, NULL));
>
> But sometimes using multiple statements instead of
> embedding function calls as arguments can be simpler
> and more intelligible for the reader.
>
> __be32 addr;
>
> ...
>
> addr = of_get_address(memory, 0, &size, NULL);
> fb->fb.fix.smem_start = of_translate_address(memory, addr);
>
>
>
^ permalink raw reply related
* Re: [PATCH 2/2]Trivial patch: to solve indentation warnings in amba-clcd.c driver
From: Russell King - ARM Linux @ 2015-02-13 13:36 UTC (permalink / raw)
To: Parmeshwr Prasad
Cc: Joe Perches, plagnioj@jcrosoft.com, tomi.valkeinen@ti.com,
trivial@kernel.org, linux-fbdev@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20150213132805.GA1296@linuxteamdev.amer.dell.com>
On Fri, Feb 13, 2015 at 08:28:10AM -0500, Parmeshwr Prasad wrote:
>
> This is second patch in series.
> In driver in_atomic we should not use to check if code is unning in IRQ.
> clcdfb_sleep() function is used to give some delay between operation.
> I have used schedule_timeout() function for same amount of delay.
Frankly, this patch is a mess. It seems to contain unrelated changes.
Please always review your own patches before you send them - this will
allow you to catch such errors before you post them publically.
The change to clcdfb_sleep() is wrong in any case - you will end up
calling schedule_timeout() from illegal contexts (eg, when the
framebuffer gets blanked/unblanked.)
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply
* Re: [PATCH 2/2]Trivial patch: to solve indentation warnings in amba-clcd.c driver
From: Sudip Mukherjee @ 2015-02-14 10:20 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Parmeshwr Prasad, Joe Perches, plagnioj@jcrosoft.com,
tomi.valkeinen@ti.com, trivial@kernel.org,
linux-fbdev@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <20150213133618.GZ8656@n2100.arm.linux.org.uk>
On Fri, Feb 13, 2015 at 01:36:18PM +0000, Russell King - ARM Linux wrote:
> On Fri, Feb 13, 2015 at 08:28:10AM -0500, Parmeshwr Prasad wrote:
> >
> > This is second patch in series.
> > In driver in_atomic we should not use to check if code is unning in IRQ.
> > clcdfb_sleep() function is used to give some delay between operation.
> > I have used schedule_timeout() function for same amount of delay.
>
> Frankly, this patch is a mess. It seems to contain unrelated changes.
and this patch is corrupted. your commit message is a mess and the maintainer has to edit it by hand before applying.
if i remember correctly you have been told multiple times by many people to fix your commit message.
and try to use git send-email to send your patches.
regards
sudip
>
> Please always review your own patches before you send them - this will
> allow you to catch such errors before you post them publically.
>
> The change to clcdfb_sleep() is wrong in any case - you will end up
> calling schedule_timeout() from illegal contexts (eg, when the
> framebuffer gets blanked/unblanked.)
>
> --
^ permalink raw reply
* Re: [PATCH 4/8] fbdev: ssd1307fb: Use vmalloc to allocate video memory.
From: Thomas Niederprüm @ 2015-02-14 14:22 UTC (permalink / raw)
To: Maxime Ripard; +Cc: linux-fbdev, plagnioj, tomi.valkeinen, linux-kernel
In-Reply-To: <20150212151121.GJ2079@lukather>
Am Thu, 12 Feb 2015 16:11:21 +0100
schrieb Maxime Ripard <maxime.ripard@free-electrons.com>:
> On Sat, Feb 07, 2015 at 04:35:41PM +0100, Thomas Niederprüm wrote:
> > Am Sat, 7 Feb 2015 12:18:21 +0100
> > schrieb Maxime Ripard <maxime.ripard@free-electrons.com>:
> >
> > > Hi,
> > >
> > > On Fri, Feb 06, 2015 at 11:28:10PM +0100, niederp@physik.uni-kl.de
> > > wrote:
> > > > From: Thomas Niederprüm <niederp@physik.uni-kl.de>
> > > >
> > > > It makes sense to use vmalloc to allocate the video buffer
> > > > since it has to be page aligned memory for using it with mmap.
> > >
> > > Please wrap your commit log at 80 chars.
> >
> > I'll try to do so in future, sorry for that.
> >
> > >
> > > It looks like there's numerous fbdev drivers using this
> > > (especially since you copy pasted that code, without mentionning
> > > it).
> >
> > Yes, I should have mentioned that in the commit message. As
> > implicitly indicated in the cover letter the rvmalloc() and
> > rvfree() are copy pasted from the vfb driver. Honestly, I didn't
> > give this one too much thought. It seemed a viable solution to the
> > mmap problem. For a bit more history on that, see my comment below.
> >
> > >
> > > That should be turned into an allocator so that drivers all get
> > > this right.
> > >
> > > > Also deffered io seems buggy in combination with kmalloc'ed
> > > > memory (crash on unloading the module).
> > >
> > > And maybe that's the real issue to fix.
> >
> > The problem is solved by using vmalloc ;)
>
> Yep, but why do you need to mark the reserved pages?
>
> ...
As far as I understood mmaped memory is marked as userspace memory in
the page table and is therefore subject to swapping. The pages are
marked reserved to make clear that this memory can not be swapped and
thus lock the pages in memory. See discussions [0,1,2].
[0]http://stackoverflow.com/questions/10760479/mmap-kernel-buffer-to-user-space
[1]http://www.linuxquestions.org/questions/linux-kernel-70/why-why-setpagereserved-is-needed-when-map-a-kernel-space-to-user-space-885176/
[2]https://sites.google.com/site/skartikeyan/mmap.html
>
> > > > @@ -570,7 +608,7 @@ static int ssd1307fb_probe(struct i2c_client
> > > > *client, info->var.blue.offset = 0;
> > > >
> > > > info->screen_base = (u8 __force __iomem *)vmem;
> > > > - info->fix.smem_start = (unsigned long)vmem;
> > > > + info->fix.smem_start = __pa(vmem);
> > >
> > > Why are you changing from virtual to physical address here?
> >
> > Oh, good catch! This is still a residual of my attempts to get this
> > working with kmalloc'ed memory. In the current state the driver is
> > presenting a completely wrong memory address upon mmap. As reported
> > in [0] info->fix.smem_start has to hold the physical address of the
> > video memory if it was allocated using kmalloc. Correcting this let
> > me run into the problem that the kmalloc'ed memory was not page
> > aligned but, the memory address handed to userspace mmap was
> > aligned to the next full page, resulting in an inaccessable display
> > region. At that point I just copied the vmalloc approach from the
> > vfb driver.
> >
> > [0]
> > http://stackoverflow.com/questions/22285151/kernel-panic-using-deferred-io-on-kmalloced-buffer
>
> And the documentation of fb_fix_screeninfo indeed state that this
> should be the physical address:
> http://lxr.free-electrons.com/source/include/uapi/linux/fb.h#L158
>
> Could you make that a separate patch?
Will be a separate patch in v2.
>
> Thanks,
> Maxime
>
Thomas
^ permalink raw reply
* Re: [PATCH 4/8] fbdev: ssd1307fb: Use vmalloc to allocate video memory.
From: Maxime Ripard @ 2015-02-14 15:36 UTC (permalink / raw)
To: Thomas Niederprüm
Cc: linux-fbdev, plagnioj, tomi.valkeinen, linux-kernel
In-Reply-To: <20150214152212.1643da7d@maestro.intranet>
[-- Attachment #1: Type: text/plain, Size: 2579 bytes --]
On Sat, Feb 14, 2015 at 03:22:12PM +0100, Thomas Niederprüm wrote:
> Am Thu, 12 Feb 2015 16:11:21 +0100
> schrieb Maxime Ripard <maxime.ripard@free-electrons.com>:
>
> > On Sat, Feb 07, 2015 at 04:35:41PM +0100, Thomas Niederprüm wrote:
> > > Am Sat, 7 Feb 2015 12:18:21 +0100
> > > schrieb Maxime Ripard <maxime.ripard@free-electrons.com>:
> > >
> > > > Hi,
> > > >
> > > > On Fri, Feb 06, 2015 at 11:28:10PM +0100, niederp@physik.uni-kl.de
> > > > wrote:
> > > > > From: Thomas Niederprüm <niederp@physik.uni-kl.de>
> > > > >
> > > > > It makes sense to use vmalloc to allocate the video buffer
> > > > > since it has to be page aligned memory for using it with mmap.
> > > >
> > > > Please wrap your commit log at 80 chars.
> > >
> > > I'll try to do so in future, sorry for that.
> > >
> > > >
> > > > It looks like there's numerous fbdev drivers using this
> > > > (especially since you copy pasted that code, without mentionning
> > > > it).
> > >
> > > Yes, I should have mentioned that in the commit message. As
> > > implicitly indicated in the cover letter the rvmalloc() and
> > > rvfree() are copy pasted from the vfb driver. Honestly, I didn't
> > > give this one too much thought. It seemed a viable solution to the
> > > mmap problem. For a bit more history on that, see my comment below.
> > >
> > > >
> > > > That should be turned into an allocator so that drivers all get
> > > > this right.
> > > >
> > > > > Also deffered io seems buggy in combination with kmalloc'ed
> > > > > memory (crash on unloading the module).
> > > >
> > > > And maybe that's the real issue to fix.
> > >
> > > The problem is solved by using vmalloc ;)
> >
> > Yep, but why do you need to mark the reserved pages?
> >
> > ...
>
> As far as I understood mmaped memory is marked as userspace memory in
> the page table and is therefore subject to swapping. The pages are
> marked reserved to make clear that this memory can not be swapped and
> thus lock the pages in memory. See discussions [0,1,2].
>
> [0]http://stackoverflow.com/questions/10760479/mmap-kernel-buffer-to-user-space
> [1]http://www.linuxquestions.org/questions/linux-kernel-70/why-why-setpagereserved-is-needed-when-map-a-kernel-space-to-user-space-885176/
> [2]https://sites.google.com/site/skartikeyan/mmap.html
Hmmm, both https://lwn.net/Articles/28746/ and
http://linux-mm.org/DeviceDriverMmap tell otherwise :)
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox