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@xxxxxxxxxxxxx> > --- > 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. [.....] > +#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. [.....] > +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; > + 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 -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
