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
