On Wednesday 12 March 2008, York Sun wrote: > +#include <linux/bootmem.h> > +#include <asm/rheap.h> > + > +#undef DEBUG > +#ifdef DEBUG > +#define DPRINTK(fmt, args...) printk(KERN_DEBUG "%s: " fmt, __func__, ## > args) > +#else > +#define DPRINTK(fmt, args...) > +#endif
Please don't define your own debug macros, but rather use pr_debug and related helpers from linux/kernel.h. > +static unsigned char *pixis_bdcfg0, *pixis_arch; > + These need to be __iomem, as far as I can see. Please run 'make C=1' to have this kind of problem checked by 'sparse' and clean up its findings. > @@ -161,12 +173,251 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_AL, 0x5229, > quirk_uli5229); > DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_AL, 0x5288, final_uli5288); > #endif /* CONFIG_PCI */ > > +static u32 get_busfreq(void) > +{ > + struct device_node *node; > + > + u32 fs_busfreq = 0; > + node = of_find_node_by_type(NULL, "cpu"); > + if (node) { > + unsigned int size; > + const unsigned int *prop = > + of_get_property(node, "bus-frequency", &size); > + if (prop) > + fs_busfreq = *prop; > + of_node_put(node); > + }; > + return fs_busfreq; > +} I guess this breaks for frequencies larger than 2^32 Ghz, right? IIRC, there is a method for encoding higher frequencies in the device tree, and you should probably support that, or even better, refer to some other function that is already interpreting it. > +#ifdef CONFIG_FB_FSL_DIU > + > +static rh_block_t diu_rh_block[16]; > +static rh_info_t diu_rh_info; > +static unsigned long diu_size = 1280 * 1024 * 4; /* One 1280x1024 buffer */ > +static void *diu_mem; diu_mem is probably __iomem as well, right? Also, it would be cleaner to have the variables in a data structure pointed to by your device->driver_data. It would only be strictly necessary if you expect to see a system with multiple DIU instances, which I think is unlikely, but still it is the way that people expect to see the code when they read it. > +unsigned int platform_get_pixel_format > + (unsigned int bits_per_pixel, int monitor_port) > +{ > + static const unsigned long pixelformat[][3] = { > + {0x88882317, 0x88083218, 0x65052119}, > + {0x88883316, 0x88082219, 0x65053118}, > + }; > + unsigned int pix_fmt, arch_monitor; > + > + arch_monitor = ((*pixis_arch == 0x01) && (monitor_port == 0))? 0 : 1; > + /* DVI port for board version 0x01 */ > + > + if (bits_per_pixel == 32) > + pix_fmt = pixelformat[arch_monitor][0]; > + else if (bits_per_pixel == 24) > + pix_fmt = pixelformat[arch_monitor][1]; > + else if (bits_per_pixel == 16) > + pix_fmt = pixelformat[arch_monitor][2]; > + else > + pix_fmt = pixelformat[1][0]; > + > + return pix_fmt; > +} > +EXPORT_SYMBOL(platform_get_pixel_format); Generally, when you create new functions that are going to be used just by your own code, they should be EXPORT_SYMBOL_GPL. It's your choice though, as you are the author. > +void platform_set_pixel_clock(unsigned int pixclock) > +{ > + u32 __iomem *clkdvdr; > + u32 temp; > + /* variables for pixel clock calcs */ > + ulong bestval, bestfreq, speed_ccb, minpixclock, maxpixclock; > + ulong pixval; > + long err; > + int i; > + > + clkdvdr = ioremap(get_immrbase() + 0xe0800, sizeof(u32)); Please don't use get_immrbase in new code. Instead, register an of_platform_driver for the device in the device tree, then use of_iomap to map its register from the driver probe() callback. > +void *fsl_diu_alloc(unsigned long size, unsigned long *phys) > +{ > + void *virt; > + > + DPRINTK("size=%lu\n", size); > + > + virt = dma_alloc_coherent(0, size, phys, GFP_DMA | GFP_KERNEL); > + > + if (virt) { > + DPRINTK("dma virt=%p phys=%lx\n", virt, *phys); > + return virt; > + } > + > + if (!diu_mem) { > + printk(KERN_INFO "%s: no diu_mem\n", __func__); > + return NULL; > + } > + > + virt = rh_alloc(&diu_rh_info, size, "DIU"); > + if (virt) > + *phys = virt_to_bus(virt); > + > + DPRINTK("rh virt=%p phys=%x\n", virt, *phys); > + > + return virt; > +} > +EXPORT_SYMBOL(fsl_diu_alloc); Don't use virt_to_bus in new code, it does not work with the DMA mapping API. Instead, use dma_map_single() to convert the kernel address into something that can be addressed by hardware. You probably don't need the dma_alloc_coherent path in that case, but always use dma_map_single on a newly allocated piece of kernel memory. Arnd <>< _______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev