2013-10-31 22:08:29

by Bing Zhao

[permalink] [raw]
Subject: [PATCH 1/3] Bluetooth: btmrvl: use cal-data from device-tree instead of conf file

The cal-data is platform dependent. It's simpler and more feasible
to use device tree based cal-data instead of configuration file
based cal-data.

This patch remove configuration file based cal-data downloading
and replace it using cal-data from device tree.

Cc: Mike Frysinger <[email protected]>
Cc: Amitkumar Karwar <[email protected]>
Signed-off-by: Bing Zhao <[email protected]>
Signed-off-by: Hyuckjoo Lee <[email protected]>
---
drivers/bluetooth/btmrvl_drv.h | 4 --
drivers/bluetooth/btmrvl_main.c | 92 ++++++++---------------------------------
drivers/bluetooth/btmrvl_sdio.c | 9 +---
drivers/bluetooth/btmrvl_sdio.h | 2 -
4 files changed, 18 insertions(+), 89 deletions(-)

diff --git a/drivers/bluetooth/btmrvl_drv.h b/drivers/bluetooth/btmrvl_drv.h
index f9d1833..be76851 100644
--- a/drivers/bluetooth/btmrvl_drv.h
+++ b/drivers/bluetooth/btmrvl_drv.h
@@ -23,8 +23,6 @@
#include <linux/bitops.h>
#include <linux/slab.h>
#include <net/bluetooth/bluetooth.h>
-#include <linux/ctype.h>
-#include <linux/firmware.h>

#define BTM_HEADER_LEN 4
#define BTM_UPLD_SIZE 2312
@@ -43,8 +41,6 @@ struct btmrvl_thread {
struct btmrvl_device {
void *card;
struct hci_dev *hcidev;
- struct device *dev;
- const char *cal_data;

u8 dev_type;

diff --git a/drivers/bluetooth/btmrvl_main.c b/drivers/bluetooth/btmrvl_main.c
index 5cf31c4..a17812d 100644
--- a/drivers/bluetooth/btmrvl_main.c
+++ b/drivers/bluetooth/btmrvl_main.c
@@ -19,7 +19,7 @@
**/

#include <linux/module.h>
-
+#include <linux/of.h>
#include <net/bluetooth/bluetooth.h>
#include <net/bluetooth/hci_core.h>

@@ -417,52 +417,8 @@ static int btmrvl_open(struct hci_dev *hdev)
return 0;
}

-/*
- * This function parses provided calibration data input. It should contain
- * hex bytes separated by space or new line character. Here is an example.
- * 00 1C 01 37 FF FF FF FF 02 04 7F 01
- * CE BA 00 00 00 2D C6 C0 00 00 00 00
- * 00 F0 00 00
- */
-static int btmrvl_parse_cal_cfg(const u8 *src, u32 len, u8 *dst, u32 dst_size)
-{
- const u8 *s = src;
- u8 *d = dst;
- int ret;
- u8 tmp[3];
-
- tmp[2] = '\0';
- while ((s - src) <= len - 2) {
- if (isspace(*s)) {
- s++;
- continue;
- }
-
- if (isxdigit(*s)) {
- if ((d - dst) >= dst_size) {
- BT_ERR("calibration data file too big!!!");
- return -EINVAL;
- }
-
- memcpy(tmp, s, 2);
-
- ret = kstrtou8(tmp, 16, d++);
- if (ret < 0)
- return ret;
-
- s += 2;
- } else {
- return -EINVAL;
- }
- }
- if (d == dst)
- return -EINVAL;
-
- return 0;
-}
-
-static int btmrvl_load_cal_data(struct btmrvl_private *priv,
- u8 *config_data)
+static int btmrvl_download_cal_data(struct btmrvl_private *priv,
+ u8 *config_data)
{
int i, ret;
u8 data[BT_CMD_DATA_SIZE];
@@ -490,54 +446,40 @@ static int btmrvl_load_cal_data(struct btmrvl_private *priv,
return 0;
}

-static int
-btmrvl_process_cal_cfg(struct btmrvl_private *priv, u8 *data, u32 size)
+static int btmrvl_cal_data_dt(struct btmrvl_private *priv)
{
+ struct device_node *dt_node;
u8 cal_data[BT_CAL_DATA_SIZE];
+ const char name[] = "btmrvl_caldata";
+ const char property[] = "btmrvl,caldata";
int ret;

- ret = btmrvl_parse_cal_cfg(data, size, cal_data, sizeof(cal_data));
+ dt_node = of_find_node_by_name(NULL, name);
+ if (!dt_node)
+ return -ENODEV;
+
+ ret = of_property_read_u8_array(dt_node, property, cal_data,
+ sizeof(cal_data));
if (ret)
return ret;

- ret = btmrvl_load_cal_data(priv, cal_data);
+ BT_DBG("Use cal data from device tree");
+ ret = btmrvl_download_cal_data(priv, cal_data);
if (ret) {
- BT_ERR("Fail to load calibrate data");
+ BT_ERR("Fail to download calibrate data");
return ret;
}

return 0;
}

-static int btmrvl_cal_data_config(struct btmrvl_private *priv)
-{
- const struct firmware *cfg;
- int ret;
- const char *cal_data = priv->btmrvl_dev.cal_data;
-
- if (!cal_data)
- return 0;
-
- ret = request_firmware(&cfg, cal_data, priv->btmrvl_dev.dev);
- if (ret < 0) {
- BT_DBG("Failed to get %s file, skipping cal data download",
- cal_data);
- return 0;
- }
-
- ret = btmrvl_process_cal_cfg(priv, (u8 *)cfg->data, cfg->size);
- release_firmware(cfg);
- return ret;
-}
-
static int btmrvl_setup(struct hci_dev *hdev)
{
struct btmrvl_private *priv = hci_get_drvdata(hdev);

btmrvl_send_module_cfg_cmd(priv, MODULE_BRINGUP_REQ);

- if (btmrvl_cal_data_config(priv))
- BT_ERR("Set cal data failed");
+ btmrvl_cal_data_dt(priv);

priv->btmrvl_dev.psmode = 1;
btmrvl_enable_ps(priv);
diff --git a/drivers/bluetooth/btmrvl_sdio.c b/drivers/bluetooth/btmrvl_sdio.c
index fabcf5b..1b52c9f 100644
--- a/drivers/bluetooth/btmrvl_sdio.c
+++ b/drivers/bluetooth/btmrvl_sdio.c
@@ -18,6 +18,7 @@
* this warranty disclaimer.
**/

+#include <linux/firmware.h>
#include <linux/slab.h>

#include <linux/mmc/sdio_ids.h>
@@ -101,7 +102,6 @@ static const struct btmrvl_sdio_card_reg btmrvl_reg_88xx = {
static const struct btmrvl_sdio_device btmrvl_sdio_sd8688 = {
.helper = "mrvl/sd8688_helper.bin",
.firmware = "mrvl/sd8688.bin",
- .cal_data = NULL,
.reg = &btmrvl_reg_8688,
.sd_blksz_fw_dl = 64,
};
@@ -109,7 +109,6 @@ static const struct btmrvl_sdio_device btmrvl_sdio_sd8688 = {
static const struct btmrvl_sdio_device btmrvl_sdio_sd8787 = {
.helper = NULL,
.firmware = "mrvl/sd8787_uapsta.bin",
- .cal_data = NULL,
.reg = &btmrvl_reg_87xx,
.sd_blksz_fw_dl = 256,
};
@@ -117,7 +116,6 @@ static const struct btmrvl_sdio_device btmrvl_sdio_sd8787 = {
static const struct btmrvl_sdio_device btmrvl_sdio_sd8797 = {
.helper = NULL,
.firmware = "mrvl/sd8797_uapsta.bin",
- .cal_data = "mrvl/sd8797_caldata.conf",
.reg = &btmrvl_reg_87xx,
.sd_blksz_fw_dl = 256,
};
@@ -125,7 +123,6 @@ static const struct btmrvl_sdio_device btmrvl_sdio_sd8797 = {
static const struct btmrvl_sdio_device btmrvl_sdio_sd8897 = {
.helper = NULL,
.firmware = "mrvl/sd8897_uapsta.bin",
- .cal_data = NULL,
.reg = &btmrvl_reg_88xx,
.sd_blksz_fw_dl = 256,
};
@@ -1007,7 +1004,6 @@ static int btmrvl_sdio_probe(struct sdio_func *func,
struct btmrvl_sdio_device *data = (void *) id->driver_data;
card->helper = data->helper;
card->firmware = data->firmware;
- card->cal_data = data->cal_data;
card->reg = data->reg;
card->sd_blksz_fw_dl = data->sd_blksz_fw_dl;
}
@@ -1036,8 +1032,6 @@ static int btmrvl_sdio_probe(struct sdio_func *func,
}

card->priv = priv;
- priv->btmrvl_dev.dev = &card->func->dev;
- priv->btmrvl_dev.cal_data = card->cal_data;

/* Initialize the interface specific function pointers */
priv->hw_host_to_card = btmrvl_sdio_host_to_card;
@@ -1220,5 +1214,4 @@ MODULE_FIRMWARE("mrvl/sd8688_helper.bin");
MODULE_FIRMWARE("mrvl/sd8688.bin");
MODULE_FIRMWARE("mrvl/sd8787_uapsta.bin");
MODULE_FIRMWARE("mrvl/sd8797_uapsta.bin");
-MODULE_FIRMWARE("mrvl/sd8797_caldata.conf");
MODULE_FIRMWARE("mrvl/sd8897_uapsta.bin");
diff --git a/drivers/bluetooth/btmrvl_sdio.h b/drivers/bluetooth/btmrvl_sdio.h
index 6872d9e..43d35a6 100644
--- a/drivers/bluetooth/btmrvl_sdio.h
+++ b/drivers/bluetooth/btmrvl_sdio.h
@@ -85,7 +85,6 @@ struct btmrvl_sdio_card {
u32 ioport;
const char *helper;
const char *firmware;
- const char *cal_data;
const struct btmrvl_sdio_card_reg *reg;
u16 sd_blksz_fw_dl;
u8 rx_unit;
@@ -95,7 +94,6 @@ struct btmrvl_sdio_card {
struct btmrvl_sdio_device {
const char *helper;
const char *firmware;
- const char *cal_data;
const struct btmrvl_sdio_card_reg *reg;
u16 sd_blksz_fw_dl;
};
--
1.8.0


2013-10-31 22:08:31

by Bing Zhao

[permalink] [raw]
Subject: [PATCH 3/3] Bluetooth: btmrvl: operate on 16-bit opcodes instead of ogf/ocf

Replace ogf/ocf and its packing with 16-bit opcodes.

Signed-off-by: Bing Zhao <[email protected]>
Signed-off-by: Amitkumar Karwar <[email protected]>
---
drivers/bluetooth/btmrvl_drv.h | 19 +++++++++++--------
drivers/bluetooth/btmrvl_main.c | 21 +++++++++------------
2 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/drivers/bluetooth/btmrvl_drv.h b/drivers/bluetooth/btmrvl_drv.h
index ffac0e4..7399303 100644
--- a/drivers/bluetooth/btmrvl_drv.h
+++ b/drivers/bluetooth/btmrvl_drv.h
@@ -86,12 +86,12 @@ struct btmrvl_private {

#define MRVL_VENDOR_PKT 0xFE

-/* Bluetooth commands */
-#define BT_CMD_AUTO_SLEEP_MODE 0x23
-#define BT_CMD_HOST_SLEEP_CONFIG 0x59
-#define BT_CMD_HOST_SLEEP_ENABLE 0x5A
-#define BT_CMD_MODULE_CFG_REQ 0x5B
-#define BT_CMD_LOAD_CONFIG_DATA 0x61
+/* Vendor specific Bluetooth commands */
+#define BT_CMD_AUTO_SLEEP_MODE 0xFC23
+#define BT_CMD_HOST_SLEEP_CONFIG 0xFC59
+#define BT_CMD_HOST_SLEEP_ENABLE 0xFC5A
+#define BT_CMD_MODULE_CFG_REQ 0xFC5B
+#define BT_CMD_LOAD_CONFIG_DATA 0xFC61

/* Sub-commands: Module Bringup/Shutdown Request/Response */
#define MODULE_BRINGUP_REQ 0xF1
@@ -100,6 +100,11 @@ struct btmrvl_private {

#define MODULE_SHUTDOWN_REQ 0xF2

+/* Vendor specific Bluetooth events */
+#define BT_EVENT_AUTO_SLEEP_MODE 0x23
+#define BT_EVENT_HOST_SLEEP_CONFIG 0x59
+#define BT_EVENT_HOST_SLEEP_ENABLE 0x5A
+#define BT_EVENT_MODULE_CFG_REQ 0x5B
#define BT_EVENT_POWER_STATE 0x20

/* Bluetooth Power States */
@@ -107,8 +112,6 @@ struct btmrvl_private {
#define BT_PS_DISABLE 0x03
#define BT_PS_SLEEP 0x01

-#define OGF 0x3F
-
/* Host Sleep states */
#define HS_ACTIVATED 0x01
#define HS_DEACTIVATED 0x00
diff --git a/drivers/bluetooth/btmrvl_main.c b/drivers/bluetooth/btmrvl_main.c
index f13bef8..1e0320a 100644
--- a/drivers/bluetooth/btmrvl_main.c
+++ b/drivers/bluetooth/btmrvl_main.c
@@ -50,12 +50,10 @@ bool btmrvl_check_evtpkt(struct btmrvl_private *priv, struct sk_buff *skb)

if (hdr->evt == HCI_EV_CMD_COMPLETE) {
struct hci_ev_cmd_complete *ec;
- u16 opcode, ocf, ogf;
+ u16 opcode;

ec = (void *) (skb->data + HCI_EVENT_HDR_SIZE);
opcode = __le16_to_cpu(ec->opcode);
- ocf = hci_opcode_ocf(opcode);
- ogf = hci_opcode_ogf(opcode);

if (priv->btmrvl_dev.sendcmdflag) {
priv->btmrvl_dev.sendcmdflag = false;
@@ -63,9 +61,8 @@ bool btmrvl_check_evtpkt(struct btmrvl_private *priv, struct sk_buff *skb)
wake_up_interruptible(&priv->adapter->cmd_wait_q);
}

- if (ogf == OGF) {
- BT_DBG("vendor event skipped: ogf 0x%4.4x ocf 0x%4.4x",
- ogf, ocf);
+ if (hci_opcode_ogf(opcode) == 0x3F) {
+ BT_DBG("vendor event skipped: opcode=%#4.4x", opcode);
kfree_skb(skb);
return false;
}
@@ -89,7 +86,7 @@ int btmrvl_process_event(struct btmrvl_private *priv, struct sk_buff *skb)
}

switch (event->data[0]) {
- case BT_CMD_AUTO_SLEEP_MODE:
+ case BT_EVENT_AUTO_SLEEP_MODE:
if (!event->data[2]) {
if (event->data[1] == BT_PS_ENABLE)
adapter->psmode = 1;
@@ -102,7 +99,7 @@ int btmrvl_process_event(struct btmrvl_private *priv, struct sk_buff *skb)
}
break;

- case BT_CMD_HOST_SLEEP_CONFIG:
+ case BT_EVENT_HOST_SLEEP_CONFIG:
if (!event->data[3])
BT_DBG("gpio=%x, gap=%x", event->data[1],
event->data[2]);
@@ -110,7 +107,7 @@ int btmrvl_process_event(struct btmrvl_private *priv, struct sk_buff *skb)
BT_DBG("HSCFG command failed");
break;

- case BT_CMD_HOST_SLEEP_ENABLE:
+ case BT_EVENT_HOST_SLEEP_ENABLE:
if (!event->data[1]) {
adapter->hs_state = HS_ACTIVATED;
if (adapter->psmode)
@@ -121,7 +118,7 @@ int btmrvl_process_event(struct btmrvl_private *priv, struct sk_buff *skb)
}
break;

- case BT_CMD_MODULE_CFG_REQ:
+ case BT_EVENT_MODULE_CFG_REQ:
if (priv->btmrvl_dev.sendcmdflag &&
event->data[1] == MODULE_BRINGUP_REQ) {
BT_DBG("EVENT:%s",
@@ -166,7 +163,7 @@ exit:
}
EXPORT_SYMBOL_GPL(btmrvl_process_event);

-static int btmrvl_send_sync_cmd(struct btmrvl_private *priv, u16 cmd_no,
+static int btmrvl_send_sync_cmd(struct btmrvl_private *priv, u16 opcode,
const void *param, u8 len)
{
struct sk_buff *skb;
@@ -179,7 +176,7 @@ static int btmrvl_send_sync_cmd(struct btmrvl_private *priv, u16 cmd_no,
}

hdr = (struct hci_command_hdr *)skb_put(skb, HCI_COMMAND_HDR_SIZE);
- hdr->opcode = cpu_to_le16(hci_opcode_pack(OGF, cmd_no));
+ hdr->opcode = cpu_to_le16(opcode);
hdr->plen = len;

if (len)
--
1.8.0

2013-10-31 22:08:30

by Bing Zhao

[permalink] [raw]
Subject: [PATCH 2/3] Bluetooth: btmrvl: remove cal-data byte swapping and redundant mem copy

The device tree property can define the cal-data in proper order.
There is no need to swap the bytes in driver.
Also remove the redundant cal-data memory copy after removing the
byte swapping.

Cc: Mike Frysinger <[email protected]>
Cc: Amitkumar Karwar <[email protected]>
Signed-off-by: Bing Zhao <[email protected]>
Signed-off-by: Hyuckjoo Lee <[email protected]>
---
drivers/bluetooth/btmrvl_drv.h | 2 +-
drivers/bluetooth/btmrvl_main.c | 27 ++++++++++-----------------
2 files changed, 11 insertions(+), 18 deletions(-)

diff --git a/drivers/bluetooth/btmrvl_drv.h b/drivers/bluetooth/btmrvl_drv.h
index be76851..ffac0e4 100644
--- a/drivers/bluetooth/btmrvl_drv.h
+++ b/drivers/bluetooth/btmrvl_drv.h
@@ -117,7 +117,7 @@ struct btmrvl_private {
#define PS_SLEEP 0x01
#define PS_AWAKE 0x00

-#define BT_CMD_DATA_SIZE 32
+#define BT_CAL_HDR_LEN 4
#define BT_CAL_DATA_SIZE 28

struct btmrvl_event {
diff --git a/drivers/bluetooth/btmrvl_main.c b/drivers/bluetooth/btmrvl_main.c
index a17812d..f13bef8 100644
--- a/drivers/bluetooth/btmrvl_main.c
+++ b/drivers/bluetooth/btmrvl_main.c
@@ -418,28 +418,20 @@ static int btmrvl_open(struct hci_dev *hdev)
}

static int btmrvl_download_cal_data(struct btmrvl_private *priv,
- u8 *config_data)
+ u8 *data, int len)
{
- int i, ret;
- u8 data[BT_CMD_DATA_SIZE];
+ int ret;

data[0] = 0x00;
data[1] = 0x00;
data[2] = 0x00;
- data[3] = BT_CMD_DATA_SIZE - 4;
-
- /* Swap cal-data bytes. Each four bytes are swapped. Considering 4
- * byte SDIO header offset, mapping of input and output bytes will be
- * {3, 2, 1, 0} -> {0+4, 1+4, 2+4, 3+4},
- * {7, 6, 5, 4} -> {4+4, 5+4, 6+4, 7+4} */
- for (i = 4; i < BT_CMD_DATA_SIZE; i++)
- data[i] = config_data[(i / 4) * 8 - 1 - i];
+ data[3] = len;

print_hex_dump_bytes("Calibration data: ",
- DUMP_PREFIX_OFFSET, data, BT_CMD_DATA_SIZE);
+ DUMP_PREFIX_OFFSET, data, BT_CAL_HDR_LEN + len);

ret = btmrvl_send_sync_cmd(priv, BT_CMD_LOAD_CONFIG_DATA, data,
- BT_CMD_DATA_SIZE);
+ BT_CAL_HDR_LEN + len);
if (ret)
BT_ERR("Failed to download caibration data\n");

@@ -449,7 +441,7 @@ static int btmrvl_download_cal_data(struct btmrvl_private *priv,
static int btmrvl_cal_data_dt(struct btmrvl_private *priv)
{
struct device_node *dt_node;
- u8 cal_data[BT_CAL_DATA_SIZE];
+ u8 cal_data[BT_CAL_HDR_LEN + BT_CAL_DATA_SIZE];
const char name[] = "btmrvl_caldata";
const char property[] = "btmrvl,caldata";
int ret;
@@ -458,13 +450,14 @@ static int btmrvl_cal_data_dt(struct btmrvl_private *priv)
if (!dt_node)
return -ENODEV;

- ret = of_property_read_u8_array(dt_node, property, cal_data,
- sizeof(cal_data));
+ ret = of_property_read_u8_array(dt_node, property,
+ cal_data + BT_CAL_HDR_LEN,
+ BT_CAL_DATA_SIZE);
if (ret)
return ret;

BT_DBG("Use cal data from device tree");
- ret = btmrvl_download_cal_data(priv, cal_data);
+ ret = btmrvl_download_cal_data(priv, cal_data, BT_CAL_DATA_SIZE);
if (ret) {
BT_ERR("Fail to download calibrate data");
return ret;
--
1.8.0

2013-11-01 22:33:54

by Marcel Holtmann

[permalink] [raw]
Subject: Re: [PATCH 3/3] Bluetooth: btmrvl: operate on 16-bit opcodes instead of ogf/ocf

Hi Bing,

>>> Replace ogf/ocf and its packing with 16-bit opcodes.
>>>
>>> Signed-off-by: Bing Zhao <[email protected]>
>>> Signed-off-by: Amitkumar Karwar <[email protected]>
>>> ---
>>> drivers/bluetooth/btmrvl_drv.h | 19 +++++++++++--------
>>> drivers/bluetooth/btmrvl_main.c | 21 +++++++++------------
>>> 2 files changed, 20 insertions(+), 20 deletions(-)
>>
>> patch looks fine to me. Since you send it as 3/3, I don?t know if you want me to apply it right away
>> or just when the whole series is ready to be applied.
>
> Please apply 3/3 first if that's convenient for you.
> I will resend 1/3 and 2/3 in v2 series.

patch has been applied to bluetooth-next tree.

Regards

Marcel


2013-11-01 22:21:37

by Marcel Holtmann

[permalink] [raw]
Subject: Re: [PATCH 1/3] Bluetooth: btmrvl: use cal-data from device-tree instead of conf file

Hi Bing,

>>> +#include <linux/of.h>
>>> #include <net/bluetooth/bluetooth.h>
>>> #include <net/bluetooth/hci_core.h>
>>
>> here now it becomes tricky. Does this now depend on CONFIG_OF as well?
>
> No. It doesn't depend on CONFIG_OF.
> The of.h takes good care of the two OF functions (of_find_node_by_name and of_property_read_u8_array) used in this patch. If CONFIG_OF is not defined, the 1st function returns NULL and the 2nd function returns -ENOSYS.
>
>>
>> What happens to x86 systems that generally do not use open firmware or device tree. Do they still
>> work?
>
> On x86 systems without OF or device tree, the entire calibration data downloading will be skipped.
>
>> Which kind of devices need the calibration data anyway? All of them? Just the ones without
>> EEPROM?
>
> Some ARM versions of Chromebook need the calibration data.
> They do have EEPROM but still need a piece of new calibration data in test mode.
>
>>
>> You need to pretty much verbose in your commit message on what is going on. And I get the feeling
>> that you have to handle the case where CONFIG_OF is not selected.
>
> When CONFIG_OF is not selected, or the specific property is not present in the device tree, the calibration downloading will not happen.
>
> I can add above messages to the commit log in v2.

that would be a good idea. Thanks.

Regards

Marcel


2013-11-01 22:11:05

by Bing Zhao

[permalink] [raw]
Subject: RE: [PATCH 3/3] Bluetooth: btmrvl: operate on 16-bit opcodes instead of ogf/ocf

Hi Marcel,

> Hi Bing,
>=20
> > Replace ogf/ocf and its packing with 16-bit opcodes.
> >
> > Signed-off-by: Bing Zhao <[email protected]>
> > Signed-off-by: Amitkumar Karwar <[email protected]>
> > ---
> > drivers/bluetooth/btmrvl_drv.h | 19 +++++++++++--------
> > drivers/bluetooth/btmrvl_main.c | 21 +++++++++------------
> > 2 files changed, 20 insertions(+), 20 deletions(-)
>=20
> patch looks fine to me. Since you send it as 3/3, I don=92t know if you w=
ant me to apply it right away
> or just when the whole series is ready to be applied.

Please apply 3/3 first if that's convenient for you.
I will resend 1/3 and 2/3 in v2 series.

Thanks,
Bing

2013-11-01 22:08:46

by Bing Zhao

[permalink] [raw]
Subject: RE: [PATCH 1/3] Bluetooth: btmrvl: use cal-data from device-tree instead of conf file

Hi Marcel,

> > +#include <linux/of.h>
> > #include <net/bluetooth/bluetooth.h>
> > #include <net/bluetooth/hci_core.h>
>=20
> here now it becomes tricky. Does this now depend on CONFIG_OF as well?

No. It doesn't depend on CONFIG_OF.
The of.h takes good care of the two OF functions (of_find_node_by_name and =
of_property_read_u8_array) used in this patch. If CONFIG_OF is not defined,=
the 1st function returns NULL and the 2nd function returns -ENOSYS.

>=20
> What happens to x86 systems that generally do not use open firmware or de=
vice tree. Do they still
> work?=20

On x86 systems without OF or device tree, the entire calibration data downl=
oading will be skipped.

> Which kind of devices need the calibration data anyway? All of them? Just=
the ones without
> EEPROM?

Some ARM versions of Chromebook need the calibration data.
They do have EEPROM but still need a piece of new calibration data in test =
mode.

>=20
> You need to pretty much verbose in your commit message on what is going o=
n. And I get the feeling
> that you have to handle the case where CONFIG_OF is not selected.

When CONFIG_OF is not selected, or the specific property is not present in =
the device tree, the calibration downloading will not happen.

I can add above messages to the commit log in v2.

Thanks,
Bing

2013-11-01 21:36:23

by Marcel Holtmann

[permalink] [raw]
Subject: Re: [PATCH 3/3] Bluetooth: btmrvl: operate on 16-bit opcodes instead of ogf/ocf

Hi Bing,

> Replace ogf/ocf and its packing with 16-bit opcodes.
>
> Signed-off-by: Bing Zhao <[email protected]>
> Signed-off-by: Amitkumar Karwar <[email protected]>
> ---
> drivers/bluetooth/btmrvl_drv.h | 19 +++++++++++--------
> drivers/bluetooth/btmrvl_main.c | 21 +++++++++------------
> 2 files changed, 20 insertions(+), 20 deletions(-)

patch looks fine to me. Since you send it as 3/3, I don?t know if you want me to apply it right away or just when the whole series is ready to be applied.

Regards

Marcel


2013-11-01 21:33:43

by Marcel Holtmann

[permalink] [raw]
Subject: Re: [PATCH 1/3] Bluetooth: btmrvl: use cal-data from device-tree instead of conf file

Hi Bing,

> The cal-data is platform dependent. It's simpler and more feasible
> to use device tree based cal-data instead of configuration file
> based cal-data.
>
> This patch remove configuration file based cal-data downloading
> and replace it using cal-data from device tree.
>
> Cc: Mike Frysinger <[email protected]>
> Cc: Amitkumar Karwar <[email protected]>
> Signed-off-by: Bing Zhao <[email protected]>
> Signed-off-by: Hyuckjoo Lee <[email protected]>
> ---
> drivers/bluetooth/btmrvl_drv.h | 4 --
> drivers/bluetooth/btmrvl_main.c | 92 ++++++++---------------------------------
> drivers/bluetooth/btmrvl_sdio.c | 9 +---
> drivers/bluetooth/btmrvl_sdio.h | 2 -
> 4 files changed, 18 insertions(+), 89 deletions(-)
>
> diff --git a/drivers/bluetooth/btmrvl_drv.h b/drivers/bluetooth/btmrvl_drv.h
> index f9d1833..be76851 100644
> --- a/drivers/bluetooth/btmrvl_drv.h
> +++ b/drivers/bluetooth/btmrvl_drv.h
> @@ -23,8 +23,6 @@
> #include <linux/bitops.h>
> #include <linux/slab.h>
> #include <net/bluetooth/bluetooth.h>
> -#include <linux/ctype.h>
> -#include <linux/firmware.h>
>
> #define BTM_HEADER_LEN 4
> #define BTM_UPLD_SIZE 2312
> @@ -43,8 +41,6 @@ struct btmrvl_thread {
> struct btmrvl_device {
> void *card;
> struct hci_dev *hcidev;
> - struct device *dev;
> - const char *cal_data;
>
> u8 dev_type;
>
> diff --git a/drivers/bluetooth/btmrvl_main.c b/drivers/bluetooth/btmrvl_main.c
> index 5cf31c4..a17812d 100644
> --- a/drivers/bluetooth/btmrvl_main.c
> +++ b/drivers/bluetooth/btmrvl_main.c
> @@ -19,7 +19,7 @@
> **/
>
> #include <linux/module.h>
> -
> +#include <linux/of.h>
> #include <net/bluetooth/bluetooth.h>
> #include <net/bluetooth/hci_core.h>

here now it becomes tricky. Does this now depend on CONFIG_OF as well?

What happens to x86 systems that generally do not use open firmware or device tree. Do they still work? Which kind of devices need the calibration data anyway? All of them? Just the ones without EEPROM?

You need to pretty much verbose in your commit message on what is going on. And I get the feeling that you have to handle the case where CONFIG_OF is not selected.

Regards

Marcel