[RFC][PATCH] bcmai: introduce AI driver

[Rafał Miłecki]

2011/4/5 Joe Perches <joe at perches.com>:
> On Tue, 2011-04-05 at 21:57 +0200, Rafał Miłecki wrote:
>> Signed-off-by: Rafał Miłecki <zajec5 at gmail.com>
>
> Just some trivia.
>
>> diff --git a/drivers/bcmai/b43_pci_ai_bridge.c b/drivers/bcmai/b43_pci_ai_bridge.c
> []
>> +#include <linux/bcmai/bcmai.h>
>> +#include <linux/bcmai/bcmai_regs.h>
>> +#include <linux/pci.h>
>> +
>> +static const struct pci_device_id b43_pci_ai_bridge_tbl[] = {
>
> DEFINE_PCI_DEVICE_TABLE

Thanks, I'll convert.


>> diff --git a/drivers/bcmai/core.c b/drivers/bcmai/core.c
> []
>> +             bcmai_err("Power control not implemented!\n");
> []
>> diff --git a/include/linux/bcmai/bcmai.h b/include/linux/bcmai/bcmai.h
> []
>> +#define bcmai_info(fmt, args...)     printk(KERN_INFO "bcmai: " fmt, ##args)
>> +#ifdef CONFIG_BCMAI_DEBUG
>> +#define bcmai_dbg(fmt, args...)              printk(KERN_DEBUG "bcmai debug: " fmt, ##args)
>> +#else
>> +#define bcmai_dbg(fmt, args...)              do { } while (0)
>> +#endif
>> +#define bcmai_err(fmt, args...)              printk(KERN_ERR "bcmai error: " fmt, ##args)
>
> I think there's very little value in prefixing
> "error" and "debug" in front of equivalent
> KERN_<level> message levels.
>
> I think you might as well just use pr_<level>
> and pr_fmt.

Hm, I've taken this idea from ssb and b43:
[b43err] printk(KERN_ERR "b43-%s ERROR: %pV", ...);
[_pll_init] ssb_printk(KERN_ERR PFX "ERROR: PLL init unknown for...

Some more reviews, please? Should I drop that prefix, is Joe right? Or
is there some official coding-style for such a situations?



>> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> []
>> +/* AI core, see drivers/bcmai/ */
>> +struct bcmai_device_id {
>> +     __u16   manuf;
>> +     __u16   id;
>> +     __u8    rev;
>> +};
>
> Do some of these structs need __packed declarations?

I was reading about __packed long time ago and it was a little tricky
for me. However I don't see anything in mod_devicetable.h using that
__packed. Why should we?

-- 
Rafał