Hi Tomas,
> > so I removed all the unneeded parties from this thread since there is no
> > point in spamming everybody with this.
> >
> >>vl_sdio.o
> >> btmrvl-y := btmrvl_main.o
> >> btmrvl-$(CONFIG_DEBUG_FS) += btmrvl_debugfs.o
> >>
> >> +obj-$(CONFIG_BT_IWMC3200) += iwmc3200bt.o
> >> +
> >
> > sort this one before the Marvell driver since it is a one file only
> > driver.
> >
> No problem
>
> >> + * Based on drivers/bluetooth/btsdio.c
> >> + * Copyright (C) 2007 Cambridge Silicon Radio Ltd.
> >> + * Copyright (C) 2007 Marcel Holtmann <marcel@xxxxxxxxxxxx>
> >
> > You mean "based on" or "90% copied" ;)
> >
>
> Right, we've tried actually to use btsdio but we differ in few key points,
> actually we coudn't locate and working HW with btsdio to try if our
> changes break it
> If someone know such device please let me know.
I have a Toshiba/Socket Bluetooth card that is Type-B that does work
according to the specs. You can still get them on eBay.
And once we have a clean driver, I will look into how we might be able
to drive the Intel device via the generic driver plus some quirks.
> >> +#ifdef CONFIG_BT_IWMC3200_DEBUG
> >> +#define IBT_DBG(func, fmt, arg...) \
> >> + dev_dbg(&((func)->dev), "%s: " fmt "\n" , __func__ , ## arg)
> >> +#define IBT_ERR(func, fmt, arg...) \
> >> + dev_err(&((func)->dev), "%s: " fmt "\n" , __func__ , ## arg)
> >> +#define IBT_DUMP(func, buf, size) do { \
> >> + char prefix_str[30 + 1]; \
> >> + scnprintf(prefix_str, 30, "%s %s", \
> >> + dev_driver_string(&((func)->dev)), \
> >> + dev_name(&((func)->dev))); \
> >> + print_hex_dump(KERN_DEBUG, prefix_str, DUMP_PREFIX_OFFSET, \
> >> + 16, 1, buf, size, false); \
> >> +} while (0)
> >> +
> >> +#define VD "-d"
> >> +#else
> >> +#define IBT_DBG(func, fmt, arg...)
> >> +#define IBT_ERR(func, fmt, arg...)
> >> +#define IBT_DUMP(func, buf, size)
> >> +#define VD
> >> +#endif /* CONFIG_BT_IWMC3200_DEBUG */
> >> +
> >> +#ifdef REPOSITORY_LABEL
> >> +#define RL REPOSITORY_LABEL
> >> +#else
> >> +#define RL local
> >> +#endif
> >
> > All of this debug stuff has to go away. It is really not needed and
> > BT_DBG with dynamic_printk support seems more than enough.
>
>
> Personally I prefer using dev_dbg in device driver then inventing it
> again with pr_debug.
I am open for suggestions, but these either happen in the Bluetooth core
or you use dev_dbg directly. Until then BT_DBG is your friend,
Also the ifdef DEBUG around IBT_ERR and IBT_DUMP is pretty bad since
dev_dbg can handle that by itself these days if not mistaken.
> >> +#define IWMCBT_VERSION "0.1.6"
> >> +
> >> +#define DRIVER_VERSION IWMCBT_VERSION "-" __stringify(RL) VD
> >
> > This one needs to be removed too. I don't care about the repository
> > label in upstream.
>
> See what I can do somehow I need to also maintain it internally
That is really not an upstream problem. Also please do like all other
Bluetooth drivers do.
#define VERSION "0.1.6"
Everything else is useless for us and too bloated.
> >> +#define IWMC_BT_SDIO_DEVID (0x1406)
> >> +
> >> +static const struct sdio_device_id btsdio_table[] = {
> >> + /* IWMC3200BT (0x1406) */
> >> + { SDIO_DEVICE(SDIO_VENDOR_ID_INTEL, 0x1406)},
> >> + { } /* Terminating entry */
> >> +};
> >
> > This is pointless. Just use the numbers directly. and put a proper
> > comment above SDIO_DEVICE with hardware this is. Also no need to repeat
> > the number in that comment.
>
> This is what we agreed up, VENDOR ID is defined, device id is inline
This is how I want you to do it:
static const struct sdio_device_id iwmc3200bt_table[] = {
/* Intel MultiCom 3200 Bluetooth device */
{ SDIO_DEVICE(SDIO_VENDOR_ID_INTEL, 0x1406)},
{ } /* Terminating entry */
};
No extra define that we don't use and no cryptic comment. The comment is
for the developers to understand what device this is for.
> >> +
> >> +MODULE_DEVICE_TABLE(sdio, btsdio_table);
> >> +
> >> +struct btsdio_data {
> >> + struct hci_dev *hdev;
> >> + struct sdio_func *func;
> >> +
> >> + struct work_struct work;
> >> +
> >> + struct sk_buff_head txq;
> >> + unsigned char tx_buf[2048];
> >> + unsigned char rx_buf[2048];
> >> +};
> >
> > I know that you guys copied btsdio driver, but if you change the name,
> > then also change the internal structure names. So this these should be
> > called iwmc3200_data and etc.
>
> Right, I just would like to postpone it for now for easier alignment with btsdio
It is one way or the other. Either we find a way to merge this into
btsdio.c or you name the structures properly. Also if I ever wanna undo
it then sed is my friend :)
Regards
Marcel
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
