From: Arnd Bergmann <arnd@arndb•de>
To: linuxppc-dev@ozlabs•org
Cc: York Sun <yorksun@freescale•com>, linux-kernel@vger•kernel.org
Subject: Re: [PATCH 2/2] Add DIU platform code for MPC8610HPCD
Date: Mon, 14 Apr 2008 16:18:34 +0200 [thread overview]
Message-ID: <200804141618.34641.arnd@arndb.de> (raw)
In-Reply-To: <12053582232536-git-send-email-yorksun@freescale.com>
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 <><
next prev parent reply other threads:[~2008-04-14 14:18 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2008-03-12 21:43 Driver for Freescale Display Interface Unit (A LCD controller) York Sun
2008-03-12 21:43 ` [PATCH 1/2] Driver for Freescale 8610 and 5121 DIU York Sun
2008-03-12 21:43 ` [PATCH 2/2] Add DIU platform code for MPC8610HPCD York Sun
2008-04-14 14:18 ` Arnd Bergmann [this message]
2008-03-12 22:20 ` [PATCH 1/2] Driver for Freescale 8610 and 5121 DIU Jiri Slaby
2008-04-11 21:45 ` Jiri Slaby
2008-04-12 5:18 ` Andrew Morton
2008-04-14 13:39 ` Timur Tabi
2008-04-14 13:43 ` Jiri Slaby
2008-04-14 13:45 ` Timur Tabi
2008-04-14 13:54 ` Jiri Slaby
2008-04-14 14:12 ` Timur Tabi
2008-04-14 14:24 ` Jiri Slaby
2008-04-14 14:49 ` Timur Tabi
2008-04-14 15:43 ` Jiri Slaby
2008-04-14 15:46 ` Timur Tabi
2008-04-14 19:03 ` Andrew Morton
2008-03-13 0:10 ` Stephen Rothwell
2008-03-13 10:17 ` Driver for Freescale Display Interface Unit (A LCD controller) Geert Uytterhoeven
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=200804141618.34641.arnd@arndb.de \
--to=arnd@arndb$(echo .)de \
--cc=linux-kernel@vger$(echo .)kernel.org \
--cc=linuxppc-dev@ozlabs$(echo .)org \
--cc=yorksun@freescale$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox