Re: [PATCH 2/2] Add DIU platform code for MPC8610HPCD

[Arnd Bergmann <arnd@arndb.de>]

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 <><