Re: [PATCH] DFU Driver and firmware for Atheros bluetooth chipset AR3011

[Marcel Holtmann <marcel@holtmann.org>]

Hi Vikram,

your email ended up in my spam folder because of your binary attachment.

> Signed-off-by: Vikram Kandukuri <Vikram.Kandukuri@atheors.com>
> 
> ---
>  drivers/bluetooth/Kconfig  |   12 +++
>  drivers/bluetooth/Makefile |    2 +
>  drivers/bluetooth/athbt.c  |  208 ++++++++++++++++++++++++++++++++++++++++++++
>  firmware/atherosbt.bin     |  Bin 0 -> 246804 bytes
>  4 files changed, 222 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/bluetooth/athbt.c
>  create mode 100644 firmware/atherosbt.bin

Following the naming convention from your WiFi drivers, this should
either by ath3k or ar3011 or ar30xx or something like that.

> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> index 652367a..5bc174e 100644
> --- a/drivers/bluetooth/Kconfig
> +++ b/drivers/bluetooth/Kconfig
> @@ -195,5 +195,17 @@ config BT_MRVL_SDIO
>  	  Say Y here to compile support for Marvell BT-over-SDIO driver
>  	  into the kernel or say M to compile it as module.
>  
> +config BT_ATHR
> +	tristate "Atheros firmware download driver"
> +	depends on BT_HCIBTUSB
> +	select FW_LOADER
> +	help
> +	  Bluetooth firmware download driver.
> +	  This driver loads the firmware into the Atheros bluetooth
> +          chipset.
> +
> +	  Say Y here to compile support for "Atheros firmware download driver"
> +          into the  kernel or say M to compile it as module (athbt).
> +
>  endmenu

To make this a little bit more consistent for the planned future
changes, I would propose that you use BT_ATH30XX or BT_ATH3K for this.

> diff --git a/drivers/bluetooth/Makefile b/drivers/bluetooth/Makefile
> index b3f57d2..f5b27e0 100644
> --- a/drivers/bluetooth/Makefile
> +++ b/drivers/bluetooth/Makefile
> @@ -18,6 +18,8 @@ obj-$(CONFIG_BT_HCIBTSDIO)	+= btsdio.o
>  obj-$(CONFIG_BT_MRVL)		+= btmrvl.o
>  obj-$(CONFIG_BT_MRVL_SDIO)	+= btmrvl_sdio.o
>  
> +obj-$(CONFIG_BT_ATHR)	        += athbt.o
> +
>  btmrvl-y			:= btmrvl_main.o
>  btmrvl-$(CONFIG_DEBUG_FS)	+= btmrvl_debugfs.o

Just sort this before the MRVL driver since this is a one file driver.

> diff --git a/drivers/bluetooth/athbt.c b/drivers/bluetooth/athbt.c
> new file mode 100644
> index 0000000..cb940c9
> --- /dev/null
> +++ b/drivers/bluetooth/athbt.c
> @@ -0,0 +1,208 @@
> +/*
> + * Copyright (c) 2008-2009 Atheros Communications Inc.
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation; either version 2 of the License, or
> + *  (at your option) any later version.
> + *
> + *  This program is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with this program; if not, write to the Free Software
> + *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> + *
> + */
> +
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
> +#include <linux/errno.h>
> +#include <linux/device.h>
> +#include <linux/firmware.h>
> +#include <linux/usb.h>
> +#include <net/bluetooth/bluetooth.h>
> +
> +#define DRV_VERSION "1.1"

We just use VERSION "1.1" in all the other drivers.

> +
> +MODULE_AUTHOR("Atheros Communications");
> +MODULE_DESCRIPTION("Atheros AR3011 firmware driver");
> +MODULE_VERSION(DRV_VERSION);
> +MODULE_SUPPORTED_DEVICE("Atheros AR3011 chipset");
> +MODULE_LICENSE("GPL");
> +MODULE_FIRMWARE("atherosbt.bin");

And to be consistent, please put this at the end of the driver. The
module author needs to have a contact information and not just the
company name. Read it as who maintains this driver.

For the firmware file name, I prefer ar3011.fw or similar like you do
for your other WiFi devices. Be at least a little bit consistent across
your own products here.

> +static struct usb_device_id athbt_table[] = {
> +	/* Atheros USB deviceID */
> +	{ USB_DEVICE(0x0CF3, 0x3000) },
> +	{ }	/* Terminating entry */
> +};

The word "deviceID" is pointless here. Use /* Atheros AR3011 without
firmware */

> +MODULE_DEVICE_TABLE(usb, athbt_table);
> +
> +#define USB_REQ_DFU_DNLOAD	1
> +#define FIRST_20BYTE            20
> +#define BLUK_SIZE               4096

BLUK? Typo?

> +#define ATHBT_IN_EP(data)       (0x81)
> +#define ATHBT_OUT_EP(data)	(0x02)
> +
> +struct athbt_data {
> +    struct usb_device *udev;
> +    u8 *fw_data;
> +    u32 fw_size;
> +    u32 fw_sent;
> +};

Tabs versus spaces.

> +static int athbt_load_firmware(struct athbt_data *data, unsigned char *firmware
> +				, int count)
> +{
> +	u8 *send_buf;
> +	int err, pipe, len, size, sent = 0;
> +
> +	BT_INFO("athbt %p udev %p\n", data, data->udev);

This is BT_DBG for and not BT_INFO.

> +
> +	BT_INFO("ATHBT! USB loading firmware\n");

And this is pointless. Or at least include the firmware name and ATHBT
makes no sense to anybody.

> +
> +	pipe = usb_sndctrlpipe(data->udev, 0);
> +
> +	if ((usb_control_msg(data->udev, pipe,
> +				USB_REQ_DFU_DNLOAD,
> +				USB_TYPE_VENDOR, 0, 0,
> +				firmware, 20, USB_CTRL_SET_TIMEOUT)) < 0) {
> +		BT_INFO("Can't change to loading configuration err\n");

This is BT_ERR.

> +		return -EBUSY;
> +	}
> +	sent += 20;
> +	count -= 20;
> +
> +	send_buf = kmalloc(BLUK_SIZE, GFP_ATOMIC);
> +	if (!send_buf) {
> +		BT_INFO("Can't allocate memory chunk for firmware\n");

Same here.

> +		return -ENOMEM;
> +	}
> +
> +	while (count) {
> +		size = min_t(uint, count, BLUK_SIZE);
> +		pipe = usb_sndbulkpipe(data->udev, ATHBT_OUT_EP(data));
> +		memcpy(send_buf, firmware + sent, size);
> +
> +		err = usb_bulk_msg(data->udev, pipe, send_buf, size,
> +					&len, 3000);
> +
> +		if (err || (len != size)) {
> +			BT_INFO("Error in firmware loading err = %d,"
> +				"len = %d, size = %d\n", err, len, size);

And here.

> +			goto error;
> +		}
> +
> +		sent  += size;
> +		count -= size;
> +	}
> +
> +	kfree(send_buf);
> +	return 0;
> +
> +error:
> +	kfree(send_buf);
> +	return err;
> +}
> +
> +static int athbt_probe(struct usb_interface *intf,
> +			const struct usb_device_id *id)
> +{
> +	const struct firmware *firmware;
> +	struct usb_device *udev = interface_to_usbdev(intf);
> +	struct athbt_data *data;
> +	int size;
> +
> +	BT_INFO("athbt_probe intf %p id %p\n", intf, id);

BT_DBG.

> +
> +	/* Check number of endpoints */
> +	BT_INFO("intf->cur_altsetting->desc.bInterfaceNumber = %d\n",
> +		intf->cur_altsetting->desc.bInterfaceNumber);

Don't do that. Pointless information.

> +
> +	if (intf->cur_altsetting->desc.bInterfaceNumber != 0) {
> +		BT_INFO("athbt_probe: ENODEV\n");

This is not an error. Just return -ENODEV. Stop spamming dmesg like
crazy. This driver is suppose to load a firmware and not operate a
nuclear reactor.

> +		return -ENODEV;
> +	}
> +
> +	data = kzalloc(sizeof(*data), GFP_KERNEL);
> +	if (!data) {
> +		BT_INFO("Can't allocate memory for data structure");

BT_ERR.

> +		return -ENOMEM;
> +	}
> +
> +	data->udev  = udev;
> +
> +	if (request_firmware(&firmware, "atherosbt.bin", &udev->dev) < 0) {
> +		BT_INFO("atherosbt firmware request failed");
> +		kfree(data);
> +		return -EIO;
> +	}
> +
> +	size = max_t(uint, firmware->size, 4096);
> +	data->fw_data = kmalloc(size, GFP_KERNEL);
> +	if (!data->fw_data) {
> +		BT_INFO("Can't allocate memory for firmware\n");

BT_ERR.

> +		release_firmware(firmware);
> +		kfree(data);
> +		return -ENOMEM;
> +	}
> +
> +	memcpy(data->fw_data, firmware->data, firmware->size);
> +	data->fw_size = firmware->size;
> +	data->fw_sent = 0;
> +	release_firmware(firmware);
> +
> +	usb_set_intfdata(intf, data);
> +	if (athbt_load_firmware(data, data->fw_data, data->fw_size))
> +		BT_INFO("ath_load_firmware   failed\n");

If it fails loading the firmware you wanna keep it claiming the
interface. That is stupid. Return an error here and give some other
drivers a chance.

> +
> +	return 0;
> +}
> +
> +static void athbt_disconnect(struct usb_interface *intf)
> +{
> +	struct athbt_data *data = usb_get_intfdata(intf);
> +
> +	BT_INFO("athbt_disconnect intf %p\n", intf);

BT_DBG.

> +
> +	kfree(data->fw_data);
> +	kfree(data);
> +}
> +
> +static struct usb_driver athbt_driver = {
> +	.name		= "athbt",
> +	.probe		= athbt_probe,
> +	.disconnect	= athbt_disconnect,
> +	.id_table	= athbt_table,
> +};
> +
> +static int __init athbt_init(void)
> +{
> +	int err;
> +
> +	BT_INFO("Atheros firmware driver ver %s", DRV_VERSION);
> +
> +	err = usb_register(&athbt_driver);
> +	if (err < 0)
> +		BT_INFO("Failed to register USB driver");

Pointless. Just to return usb_register().

> +
> +	return err;
> +}
> +
> +static void __exit athbt_exit(void)
> +{
> +	BT_INFO("athbt_exit\n");

Don't even bother with that info here.

> +	usb_deregister(&athbt_driver);
> +}
> +
> +module_init(athbt_init);
> +module_exit(athbt_exit);
> diff --git a/firmware/atherosbt.bin b/firmware/atherosbt.bin
> new file mode 100644
> index 0000000000000000000000000000000000000000..4a5ffb8de898fab1595662742f287493689e2b3b
> GIT binary patch

What license is on this firmware. We can't include it into the Linux
kernel unless it us under GPL. If it is a binary license and has a
proper distribution license then it can go to the linux-firmware tree
once you fixed the name of it of course.

Regards

Marcel