Re: [PATCH v5] video: add OpenCores VGA/LCD framebuffer driver
flat view
From: Jingoo Han <hidden>
Date: 2014-01-13 01:24:56
Also in:
lkml
On Saturday, January 11, 2014 5:13 AM, Stefan Kristiansson wrote:
This adds support for the VGA/LCD core available from OpenCores: http://opencores.org/project,vga_lcd The driver have been tested together with both OpenRISC and ARM (socfpga) processors. Signed-off-by: Stefan Kristiansson <stefan.kristiansson@saunalahti.fi> --- Changes in v2: - Add Microblaze as an example user and fix a typo in Xilinx Zynq Changes in v3: - Use devm_kzalloc instead of kzalloc - Remove superflous MODULE #ifdef Changes in v4: - Remove 'default n' in Kconfig - Simplify ioremap/request_mem_region by using devm_ioremap_resource - Remove release_mem_region Changes in v5: - Remove static structs to support multiple devices --- drivers/video/Kconfig | 16 ++ drivers/video/Makefile | 1 + drivers/video/ocfb.c | 440 +++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 457 insertions(+) create mode 100644 drivers/video/ocfb.c
It looks good. However, I added some minor comments. :-) Sorry for late response. [.....]
quoted hunk ↗ jump to hunk
+#include <linux/module.h> +#include <linux/kernel.h> +#include <linux/errno.h> +#include <linux/string.h> +#include <linux/slab.h> +#include <linux/delay.h> +#include <linux/mm.h> +#include <linux/dma-mapping.h> +#include <linux/fb.h> +#include <linux/init.h> +#include <linux/io.h> +#include <linux/platform_device.h> +#include <linux/of.h>
Would you re-order these headers alphabetically? It enhances the readability. [.....]
quoted hunk ↗ jump to hunk
+struct ocfb_dev { + struct fb_info info; + void __iomem *regs; + /* flag indicating whether the regs are little endian accessed */ + int little_endian; + /* Physical and virtual addresses of framebuffer */ + phys_addr_t fb_phys; + void __iomem *fb_virt; + u32 pseudo_palette[PALETTE_SIZE]; +};
Here, 'fb_virt' is already defined as 'void __iomem *'. [.....]
+ fbdev->info.fix.smem_start = fbdev->fb_phys; + fbdev->info.screen_base = (void __iomem *)fbdev->fb_virt;
Please remove unnecessary casting as below, because 'fb_virt' is already defined as 'void __iomem *'. + fbdev->info.screen_base = fbdev->fb_virt;
quoted hunk ↗ jump to hunk
+ fbdev->info.pseudo_palette = fbdev->pseudo_palette; + + /* Clear framebuffer */ + memset_io((void __iomem *)fbdev->fb_virt, 0, fbsize);
Same here. + memset_io(fbdev->fb_virt, 0, fbsize); Best regards, Jingoo Han