On Wed, 10 Jun 2009 22:03:00 -0400
Bob Copeland <me@xxxxxxxxxxxxxxx> wrote:
> This adds the wl12xx_sdio module, enabling the SDIO interface for
> wl12xx, as used by the Google G1 phone and others.
>
> Signed-off-by: Bob Copeland <me@xxxxxxxxxxxxxxx>
> ---
I think the drivers looks quite ok. There are really just a few things
I'm concerned about:
- Please put the device id:s in sdio_ids.h. There is no pci-ids
equivalent for SDIO so I'd like to fill up the kernel header with
device ids as much as possible.
- Why do you have a platform device with the sole purpose of enabling
power to the SDIO card? Shouldn't this be handled in the arch code?
> + sdio_set_block_size(func, 512);
Since you do not actually rely on this, why not leave it at the default
value?
> +static void __devexit wl12xx_sdio_remove(struct sdio_func *func)
> +{
> + struct wl12xx *wl = sdio_get_drvdata(func);
> +
> + sdio_claim_host(func);
> + sdio_release_irq(func);
> + sdio_disable_func(func);
> + sdio_release_host(func);
> +
> + wl12xx_free_hw(wl);
> +}
This seems wrong. Shouldn't you unregister the device with the upper
layers first?
Rgds
--
-- Pierre Ossman
WARNING: This correspondence is being monitored by the
Swedish government. Make sure your server uses encryption
for SMTP traffic and consider using PGP for end-to-end
encryption.
Attachment:
signature.asc
Description: PGP signature
