raspberrypi/linux — Issue #4: framebuffer code suggestions
Author: Alan-Cox
Created: 2012-01-25 11:35:26+00:00
State: open

1. Your struct fbinfo_s uses int but in fact needs the bytes to precisely match the firmware interface. Also the values are in practice unsigned anyway, so use u32 to be clear

2. Volatile - this wrecks your code generation and gcc in some releases has been known to get it rather wrong. If the firmware updates the block only when you ask it then you just need to add wmb(); before it and rmb(); after it to cover compiler temporaries. They provide write and read barriers instead of the mess the C language volatile makes.

3. The code appears derived from the cirrusfb driver. That driver is copyrighted yet the copyright credits for the original have been stripped. We take such things very seriously, and stripping authorship credit is a GPL violation so this really wants a 'derived from ... etc' putting in by Broadcom. Nothing wrong with it being derived from and still GPL but the credit matters.

4. ioremap on the screen base can fail. It's not clear what you should do in that case but it probably should at least BUG(), otherwise you'll have a screen at NULL so anyone making it fail can now patch low physical memory at will

5. If you want the PI logo to get into the upstream you'll need to do it differently to hacking the existing logo

6. You set various HWACCEL bits you don't have HWACCEL for ?

7. General style - // comments, lots of printks that can go but appear to be still needed debug, pr_info etc not printk() and where possible dev_dbg() etc. lower case for the module arguments. Usually mundanities basically.