Skip to content

framebuffer code suggestions #4

Closed
@Alan-Cox

Description

@Alan-Cox
  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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions