[PATCH v2 0/3] Bluetooth: hci_ll: Get BD address from NVMEM (was "bluetooth: hci_ll: Get MAC address from NVMEM")

[PATCH v2 0/3] Bluetooth: hci_ll: Get BD address from NVMEM (was "bluetooth: hci_ll: Get MAC address from NVMEM")

[David Lechner]

This series adds supporting getting the BD address from a NVMEM provider
for "LL" HCI controllers (Texas Instruments).

v2 changes:
* Fixed typos in dt-bindings
* Use "bd-address" instead of "mac-address"
* Updated dt-bindings to specify the byte order of "bd-address"
* New patch "Bluetooth: hci_ll: add support for setting public address"
* Dropped patch "Bluetooth: hci_ll: add constant for vendor-specific command"
  that is already in bluetooth-next
* Rework error handling
* Use bdaddr_t, bacmp and other bluetooth utils

David Lechner (3):
  Bluetooth: hci_ll: add support for setting public address
  dt-bindings: Add optional nvmem BD address bindings to ti,wlink-st
  Bluetooth: hci_ll: Add optional nvmem BD address source

 .../devicetree/bindings/net/ti,wilink-st.txt       |  5 ++
 drivers/bluetooth/hci_ll.c                         | 71 ++++++++++++++++++++++
 2 files changed, 76 insertions(+)

-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 1/3] Bluetooth: hci_ll: add support for setting public address

[David Lechner]

This adds support for setting the public address on Texas Instruments
Bluetooth chips using a vendor-specific command.

This has been tested on a CC2560A. The TI wiki also indicates that this
command should work on TI WL17xx/WL18xx Bluetooth chips.

Signed-off-by: David Lechner <david@lechnology.com>
---

v2 changes:
* This is a new patch in v2

 drivers/bluetooth/hci_ll.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/bluetooth/hci_ll.c b/drivers/bluetooth/hci_ll.c
index 974a788..b732004 100644
--- a/drivers/bluetooth/hci_ll.c
+++ b/drivers/bluetooth/hci_ll.c
@@ -57,6 +57,7 @@
 #include "hci_uart.h"
 
 /* Vendor-specific HCI commands */
+#define HCI_VS_WRITE_BD_ADDR			0xfc06
 #define HCI_VS_UPDATE_UART_HCI_BAUDRATE		0xff36
 
 /* HCILL commands */
@@ -662,6 +663,20 @@ static int download_firmware(struct ll_device *lldev)
 	return err;
 }
 
+static int ll_set_bdaddr(struct hci_dev *hdev, const bdaddr_t *bdaddr)
+{
+	bdaddr_t bdaddr_swapped;
+	struct sk_buff *skb;
+
+	baswap(&bdaddr_swapped, bdaddr);
+	skb = __hci_cmd_sync(hdev, HCI_VS_WRITE_BD_ADDR, sizeof(bdaddr_t),
+			     &bdaddr_swapped, HCI_INIT_TIMEOUT);
+	if (!IS_ERR(skb))
+		kfree_skb(skb);
+	
+	return PTR_ERR_OR_ZERO(skb);
+}
+
 static int ll_setup(struct hci_uart *hu)
 {
 	int err, retry = 3;
@@ -674,6 +689,8 @@ static int ll_setup(struct hci_uart *hu)
 
 	lldev = serdev_device_get_drvdata(serdev);
 
+	hu->hdev->set_bdaddr = ll_set_bdaddr;
+
 	serdev_device_set_flow_control(serdev, true);
 
 	do {
-- 
2.7.4

[PATCH v2 2/3] dt-bindings: Add optional nvmem BD address bindings to ti,wlink-st

[David Lechner]

This adds optional nvmem consumer properties to the ti,wlink-st device tree
bindings to allow specifying the BD address.

Signed-off-by: David Lechner <david@lechnology.com>
---

v2 changes:
* Renamed "mac-address" to "bd-address"
* Fixed typos in example
* Specify byte order of "bd-address"

 Documentation/devicetree/bindings/net/ti,wilink-st.txt | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/ti,wilink-st.txt b/Documentation/devicetree/bindings/net/ti,wilink-st.txt
index 1649c1f..a45a508 100644
--- a/Documentation/devicetree/bindings/net/ti,wilink-st.txt
+++ b/Documentation/devicetree/bindings/net/ti,wilink-st.txt
@@ -32,6 +32,9 @@ Optional properties:
    See ../clocks/clock-bindings.txt for details.
  - clock-names : Must include the following entry:
    "ext_clock" (External clock provided to the TI combo chip).
+ - nvmem-cells: phandle to nvmem data cell that contains a 6 byte BD address
+   with the most significant byte first (big-endian).
+ - nvmem-cell-names: "bd-address" (required when nvmem-cells is specified)
 
 Example:
 
@@ -43,5 +46,7 @@ Example:
 		enable-gpios = <&gpio1 7 GPIO_ACTIVE_HIGH>;
 		clocks = <&clk32k_wl18xx>;
 		clock-names = "ext_clock";
+		nvmem-cells = <&bd_address>;
+		nvmem-cell-names = "bd-address";
 	};
 };
-- 
2.7.4

[PATCH v2 3/3] Bluetooth: hci_ll: Add optional nvmem BD address source

[David Lechner]

This adds an optional nvmem consumer to get a BD address from an external
source. The BD address is then set in the Bluetooth chip after the
firmware has been loaded.

This has been tested working with a TI CC2560A chip (in a LEGO MINDSTORMS
EV3).

Signed-off-by: David Lechner <david@lechnology.com>
---

v2 changes:
* Add support for HCI_QUIRK_INVALID_BDADDR when there is an error getting the
  BD address from nvmem
* Rework error handling
* rename "mac-address" to "bd-address"
* use bdaddr_t, bacmp and other bluetooth helper functions
* use ll_set_bdaddr() from new, separate patch

 drivers/bluetooth/hci_ll.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/drivers/bluetooth/hci_ll.c b/drivers/bluetooth/hci_ll.c
index b732004..f5fef2d 100644
--- a/drivers/bluetooth/hci_ll.c
+++ b/drivers/bluetooth/hci_ll.c
@@ -53,6 +53,7 @@
 #include <net/bluetooth/bluetooth.h>
 #include <net/bluetooth/hci_core.h>
 #include <linux/gpio/consumer.h>
+#include <linux/nvmem-consumer.h>
 
 #include "hci_uart.h"
 
@@ -90,6 +91,7 @@ struct ll_device {
 	struct serdev_device *serdev;
 	struct gpio_desc *enable_gpio;
 	struct clk *ext_clk;
+	bdaddr_t bdaddr;
 };
 
 struct ll_struct {
@@ -715,6 +717,19 @@ static int ll_setup(struct hci_uart *hu)
 	if (err)
 		return err;
 
+	/* Set BD address if one was specified at probe */
+	if (!bacmp(&lldev->bdaddr, BDADDR_NONE)) {
+		/*
+		 * This means that there was an error getting the BD address
+		 * during probe, so mark the device as having a bad address.
+		 */
+		set_bit(HCI_QUIRK_INVALID_BDADDR, &hu->hdev->quirks);
+	} else if (bacmp(&lldev->bdaddr, BDADDR_ANY)) {
+		err = ll_set_bdaddr(hu->hdev, &lldev->bdaddr);
+		if (err)
+			set_bit(HCI_QUIRK_INVALID_BDADDR, &hu->hdev->quirks);
+	}
+
 	/* Operational speed if any */
 	if (hu->oper_speed)
 		speed = hu->oper_speed;
@@ -743,6 +758,7 @@ static int hci_ti_probe(struct serdev_device *serdev)
 {
 	struct hci_uart *hu;
 	struct ll_device *lldev;
+	struct nvmem_cell *bdaddr_cell;
 	u32 max_speed = 3000000;
 
 	lldev = devm_kzalloc(&serdev->dev, sizeof(struct ll_device), GFP_KERNEL);
@@ -764,6 +780,45 @@ static int hci_ti_probe(struct serdev_device *serdev)
 	of_property_read_u32(serdev->dev.of_node, "max-speed", &max_speed);
 	hci_uart_set_speeds(hu, 115200, max_speed);
 
+	/* optional BD address from nvram */
+	bdaddr_cell = nvmem_cell_get(&serdev->dev, "bd-address");
+	if (IS_ERR(bdaddr_cell)) {
+		int err = PTR_ERR(bdaddr_cell);
+
+		if (err == -EPROBE_DEFER)
+			return err;
+
+		/*
+		 * ENOENT means there is no matching nvmem cell and ENOSYS
+		 * means that nvmem is not enabled in the kernel configuration.
+		 */
+		if (err != -ENOENT && err != -ENOSYS) {
+			/*
+			 * If there was some other error, give userspace a
+			 * chance to fix the problem instead of failing to load
+			 * the driver. Using BDADDR_NONE as a flag that is
+			 * tested later in the setup function.
+			 */
+			dev_warn(&serdev->dev,
+				 "Failed to get \"bd-address\" nvmem cell (%d)\n",
+				 err);
+			bacpy(&lldev->bdaddr, BDADDR_NONE);
+		}
+	} else {
+		bdaddr_t *bdaddr;
+		int len;
+
+		bdaddr = nvmem_cell_read(bdaddr_cell, &len);
+		if (len != sizeof(bdaddr_t)) {
+			dev_err(&serdev->dev, "Invalid nvmem bd-address length\n");
+			nvmem_cell_put(bdaddr_cell);
+			return -EINVAL;
+		}
+
+		baswap(&lldev->bdaddr, bdaddr);
+		nvmem_cell_put(bdaddr_cell);
+	}
+
 	return hci_uart_register_device(hu, &llp);
 }
 
-- 
2.7.4

Re: [PATCH v2 1/3] Bluetooth: hci_ll: add support for setting public address

[Marcel Holtmann]

Hi David,

> This adds support for setting the public address on Texas Instruments
> Bluetooth chips using a vendor-specific command.
> 
> This has been tested on a CC2560A. The TI wiki also indicates that this
> command should work on TI WL17xx/WL18xx Bluetooth chips.
> 
> Signed-off-by: David Lechner <david-nq/r/kbU++upp/zk7JDF2g@public.gmane.org>
> ---
> 
> v2 changes:
> * This is a new patch in v2
> 
> drivers/bluetooth/hci_ll.c | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/bluetooth/hci_ll.c b/drivers/bluetooth/hci_ll.c
> index 974a788..b732004 100644
> --- a/drivers/bluetooth/hci_ll.c
> +++ b/drivers/bluetooth/hci_ll.c
> @@ -57,6 +57,7 @@
> #include "hci_uart.h"
> 
> /* Vendor-specific HCI commands */
> +#define HCI_VS_WRITE_BD_ADDR			0xfc06
> #define HCI_VS_UPDATE_UART_HCI_BAUDRATE		0xff36
> 
> /* HCILL commands */
> @@ -662,6 +663,20 @@ static int download_firmware(struct ll_device *lldev)
> 	return err;
> }
> 
> +static int ll_set_bdaddr(struct hci_dev *hdev, const bdaddr_t *bdaddr)
> +{
> +	bdaddr_t bdaddr_swapped;
> +	struct sk_buff *skb;
> +
> +	baswap(&bdaddr_swapped, bdaddr);
> +	skb = __hci_cmd_sync(hdev, HCI_VS_WRITE_BD_ADDR, sizeof(bdaddr_t),
> +			     &bdaddr_swapped, HCI_INIT_TIMEOUT);
> +	if (!IS_ERR(skb))
> +		kfree_skb(skb);
> +	

You have a trailing whitespace here.

Does the HCI command really expect the BD_ADDR in the swapped order. The caller of hdev->set_bdaddr while provide it in the same order as the HCI Read BD Address command and everything in HCI. So it seems odd that you have to swap it for the vendor command.

So have you actually tested this with btmgmt public-add <xx:xx..> and checked that the address comes out correctly. I think ll_set_bdaddr should function correctly for the mgmt interface. And if needed any other caller outside of mgmt has to do the swapping.

Regards

Marcel

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/3] dt-bindings: Add optional nvmem BD address bindings to ti,wlink-st

[Marcel Holtmann]

Hi David,


> This adds optional nvmem consumer properties to the ti,wlink-st device tree
> bindings to allow specifying the BD address.
> 
> Signed-off-by: David Lechner <david-nq/r/kbU++upp/zk7JDF2g@public.gmane.org>
> ---
> 
> v2 changes:
> * Renamed "mac-address" to "bd-address"
> * Fixed typos in example
> * Specify byte order of "bd-address"
> 
> Documentation/devicetree/bindings/net/ti,wilink-st.txt | 5 +++++
> 1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/ti,wilink-st.txt b/Documentation/devicetree/bindings/net/ti,wilink-st.txt
> index 1649c1f..a45a508 100644
> --- a/Documentation/devicetree/bindings/net/ti,wilink-st.txt
> +++ b/Documentation/devicetree/bindings/net/ti,wilink-st.txt
> @@ -32,6 +32,9 @@ Optional properties:
>    See ../clocks/clock-bindings.txt for details.
>  - clock-names : Must include the following entry:
>    "ext_clock" (External clock provided to the TI combo chip).
> + - nvmem-cells: phandle to nvmem data cell that contains a 6 byte BD address
> +   with the most significant byte first (big-endian).
> + - nvmem-cell-names: "bd-address" (required when nvmem-cells is specified)
> 
> Example:
> 
> @@ -43,5 +46,7 @@ Example:
> 		enable-gpios = <&gpio1 7 GPIO_ACTIVE_HIGH>;
> 		clocks = <&clk32k_wl18xx>;
> 		clock-names = "ext_clock";
> +		nvmem-cells = <&bd_address>;
> +		nvmem-cell-names = "bd-address”;

For me this looks good, but I like to get an extra ACK from Rob on this.

Regards

Marcel

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/3] Bluetooth: hci_ll: Add optional nvmem BD address source

[Marcel Holtmann]

Hi David,

> This adds an optional nvmem consumer to get a BD address from an external
> source. The BD address is then set in the Bluetooth chip after the
> firmware has been loaded.
> 
> This has been tested working with a TI CC2560A chip (in a LEGO MINDSTORMS
> EV3).
> 
> Signed-off-by: David Lechner <david-nq/r/kbU++upp/zk7JDF2g@public.gmane.org>
> ---
> 
> v2 changes:
> * Add support for HCI_QUIRK_INVALID_BDADDR when there is an error getting the
>  BD address from nvmem
> * Rework error handling
> * rename "mac-address" to "bd-address"
> * use bdaddr_t, bacmp and other bluetooth helper functions
> * use ll_set_bdaddr() from new, separate patch
> 
> drivers/bluetooth/hci_ll.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 55 insertions(+)
> 
> diff --git a/drivers/bluetooth/hci_ll.c b/drivers/bluetooth/hci_ll.c
> index b732004..f5fef2d 100644
> --- a/drivers/bluetooth/hci_ll.c
> +++ b/drivers/bluetooth/hci_ll.c
> @@ -53,6 +53,7 @@
> #include <net/bluetooth/bluetooth.h>
> #include <net/bluetooth/hci_core.h>
> #include <linux/gpio/consumer.h>
> +#include <linux/nvmem-consumer.h>
> 
> #include "hci_uart.h"
> 
> @@ -90,6 +91,7 @@ struct ll_device {
> 	struct serdev_device *serdev;
> 	struct gpio_desc *enable_gpio;
> 	struct clk *ext_clk;
> +	bdaddr_t bdaddr;
> };
> 
> struct ll_struct {
> @@ -715,6 +717,19 @@ static int ll_setup(struct hci_uart *hu)
> 	if (err)
> 		return err;
> 
> +	/* Set BD address if one was specified at probe */
> +	if (!bacmp(&lldev->bdaddr, BDADDR_NONE)) {
> +		/*
> +		 * This means that there was an error getting the BD address
> +		 * during probe, so mark the device as having a bad address.
> +		 */
> +		set_bit(HCI_QUIRK_INVALID_BDADDR, &hu->hdev->quirks);
> +	} else if (bacmp(&lldev->bdaddr, BDADDR_ANY)) {
> +		err = ll_set_bdaddr(hu->hdev, &lldev->bdaddr);
> +		if (err)
> +			set_bit(HCI_QUIRK_INVALID_BDADDR, &hu->hdev->quirks);
> +	}
> +
> 	/* Operational speed if any */
> 	if (hu->oper_speed)
> 		speed = hu->oper_speed;
> @@ -743,6 +758,7 @@ static int hci_ti_probe(struct serdev_device *serdev)
> {
> 	struct hci_uart *hu;
> 	struct ll_device *lldev;
> +	struct nvmem_cell *bdaddr_cell;
> 	u32 max_speed = 3000000;
> 
> 	lldev = devm_kzalloc(&serdev->dev, sizeof(struct ll_device), GFP_KERNEL);
> @@ -764,6 +780,45 @@ static int hci_ti_probe(struct serdev_device *serdev)
> 	of_property_read_u32(serdev->dev.of_node, "max-speed", &max_speed);
> 	hci_uart_set_speeds(hu, 115200, max_speed);
> 
> +	/* optional BD address from nvram */
> +	bdaddr_cell = nvmem_cell_get(&serdev->dev, "bd-address");
> +	if (IS_ERR(bdaddr_cell)) {
> +		int err = PTR_ERR(bdaddr_cell);
> +
> +		if (err == -EPROBE_DEFER)
> +			return err;
> +
> +		/*
> +		 * ENOENT means there is no matching nvmem cell and ENOSYS
> +		 * means that nvmem is not enabled in the kernel configuration.
> +		 */

Fix the comment style to this:

	/* foo
	 * bar
	 */

> +		if (err != -ENOENT && err != -ENOSYS) {
> +			/*
> +			 * If there was some other error, give userspace a
> +			 * chance to fix the problem instead of failing to load
> +			 * the driver. Using BDADDR_NONE as a flag that is
> +			 * tested later in the setup function.
> +			 */
> +			dev_warn(&serdev->dev,
> +				 "Failed to get \"bd-address\" nvmem cell (%d)\n",
> +				 err);
> +			bacpy(&lldev->bdaddr, BDADDR_NONE);
> +		}
> +	} else {
> +		bdaddr_t *bdaddr;
> +		int len;
> +
> +		bdaddr = nvmem_cell_read(bdaddr_cell, &len);
> +		if (len != sizeof(bdaddr_t)) {
> +			dev_err(&serdev->dev, "Invalid nvmem bd-address length\n");
> +			nvmem_cell_put(bdaddr_cell);
> +			return -EINVAL;
> +		}
> +
> +		baswap(&lldev->bdaddr, bdaddr);

This swapping needs a comment. Explain the format of the NVMEM storage and also which the HCI vendor command takes.

> +		nvmem_cell_put(bdaddr_cell);
> +	}
> +
> 	return hci_uart_register_device(hu, &llp);
> }

Regards

Marcel

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/3] Bluetooth: hci_ll: add support for setting public address

[David Lechner]

On 12/08/2017 02:07 AM, Marcel Holtmann wrote:
> Hi David,
> 
>> This adds support for setting the public address on Texas Instruments
>> Bluetooth chips using a vendor-specific command.
>>
>> This has been tested on a CC2560A. The TI wiki also indicates that this
>> command should work on TI WL17xx/WL18xx Bluetooth chips.
>>
>> Signed-off-by: David Lechner <david-nq/r/kbU++upp/zk7JDF2g@public.gmane.org>
>> ---
>>
>> v2 changes:
>> * This is a new patch in v2
>>
>> drivers/bluetooth/hci_ll.c | 17 +++++++++++++++++
>> 1 file changed, 17 insertions(+)
>>
>> diff --git a/drivers/bluetooth/hci_ll.c b/drivers/bluetooth/hci_ll.c
>> index 974a788..b732004 100644
>> --- a/drivers/bluetooth/hci_ll.c
>> +++ b/drivers/bluetooth/hci_ll.c
>> @@ -57,6 +57,7 @@
>> #include "hci_uart.h"
>>
>> /* Vendor-specific HCI commands */
>> +#define HCI_VS_WRITE_BD_ADDR			0xfc06
>> #define HCI_VS_UPDATE_UART_HCI_BAUDRATE		0xff36
>>
>> /* HCILL commands */
>> @@ -662,6 +663,20 @@ static int download_firmware(struct ll_device *lldev)
>> 	return err;
>> }
>>
>> +static int ll_set_bdaddr(struct hci_dev *hdev, const bdaddr_t *bdaddr)
>> +{
>> +	bdaddr_t bdaddr_swapped;
>> +	struct sk_buff *skb;
>> +
>> +	baswap(&bdaddr_swapped, bdaddr);
>> +	skb = __hci_cmd_sync(hdev, HCI_VS_WRITE_BD_ADDR, sizeof(bdaddr_t),
>> +			     &bdaddr_swapped, HCI_INIT_TIMEOUT);
>> +	if (!IS_ERR(skb))
>> +		kfree_skb(skb);
>> +	
> 
> You have a trailing whitespace here.
> 
> Does the HCI command really expect the BD_ADDR in the swapped order. The caller of hdev->set_bdaddr while provide it in the same order as the HCI Read BD Address command and everything in HCI. So it seems odd that you have to swap it for the vendor command.
> 
> So have you actually tested this with btmgmt public-add <xx:xx..> and checked that the address comes out correctly. I think ll_set_bdaddr should function correctly for the mgmt interface. And if needed any other caller outside of mgmt has to do the swapping.
> 

I did test using btmgmt public-address 00:11:22:33:44:55, which is how I 
found out that the order needed to be swapped. Like you, I was 
surprised. I couldn't find any documentation from TI saying what the 
order is supposed to be, so I can only assume that because this works, 
it is indeed correct as-is.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/3] Bluetooth: hci_ll: add support for setting public address

[Marcel Holtmann]

Hi David,

>>> This adds support for setting the public address on Texas Instruments
>>> Bluetooth chips using a vendor-specific command.
>>> 
>>> This has been tested on a CC2560A. The TI wiki also indicates that this
>>> command should work on TI WL17xx/WL18xx Bluetooth chips.
>>> 
>>> Signed-off-by: David Lechner <david-nq/r/kbU++upp/zk7JDF2g@public.gmane.org>
>>> ---
>>> 
>>> v2 changes:
>>> * This is a new patch in v2
>>> 
>>> drivers/bluetooth/hci_ll.c | 17 +++++++++++++++++
>>> 1 file changed, 17 insertions(+)
>>> 
>>> diff --git a/drivers/bluetooth/hci_ll.c b/drivers/bluetooth/hci_ll.c
>>> index 974a788..b732004 100644
>>> --- a/drivers/bluetooth/hci_ll.c
>>> +++ b/drivers/bluetooth/hci_ll.c
>>> @@ -57,6 +57,7 @@
>>> #include "hci_uart.h"
>>> 
>>> /* Vendor-specific HCI commands */
>>> +#define HCI_VS_WRITE_BD_ADDR			0xfc06
>>> #define HCI_VS_UPDATE_UART_HCI_BAUDRATE		0xff36
>>> 
>>> /* HCILL commands */
>>> @@ -662,6 +663,20 @@ static int download_firmware(struct ll_device *lldev)
>>> 	return err;
>>> }
>>> 
>>> +static int ll_set_bdaddr(struct hci_dev *hdev, const bdaddr_t *bdaddr)
>>> +{
>>> +	bdaddr_t bdaddr_swapped;
>>> +	struct sk_buff *skb;
>>> +
>>> +	baswap(&bdaddr_swapped, bdaddr);
>>> +	skb = __hci_cmd_sync(hdev, HCI_VS_WRITE_BD_ADDR, sizeof(bdaddr_t),
>>> +			     &bdaddr_swapped, HCI_INIT_TIMEOUT);
>>> +	if (!IS_ERR(skb))
>>> +		kfree_skb(skb);
>>> +	
>> You have a trailing whitespace here.
>> Does the HCI command really expect the BD_ADDR in the swapped order. The caller of hdev->set_bdaddr while provide it in the same order as the HCI Read BD Address command and everything in HCI. So it seems odd that you have to swap it for the vendor command.
>> So have you actually tested this with btmgmt public-add <xx:xx..> and checked that the address comes out correctly. I think ll_set_bdaddr should function correctly for the mgmt interface. And if needed any other caller outside of mgmt has to do the swapping.
> 
> I did test using btmgmt public-address 00:11:22:33:44:55, which is how I found out that the order needed to be swapped. Like you, I was surprised. I couldn't find any documentation from TI saying what the order is supposed to be, so I can only assume that because this works, it is indeed correct as-is.

then please add a comment for that and I would appreciate to have the parts from btmon showing the public-addr command and the following HCI Read BD Address command as part of the commit message. Just for being able to dig this out at some later point if needed.

Regards

Marcel

Re: [PATCH v2 2/3] dt-bindings: Add optional nvmem BD address bindings to ti,wlink-st

[Rob Herring]

On Thu, Dec 07, 2017 at 08:57:39PM -0600, David Lechner wrote:
> This adds optional nvmem consumer properties to the ti,wlink-st device tree
> bindings to allow specifying the BD address.
> 
> Signed-off-by: David Lechner <david@lechnology.com>
> ---
> 
> v2 changes:
> * Renamed "mac-address" to "bd-address"
> * Fixed typos in example
> * Specify byte order of "bd-address"
> 
>  Documentation/devicetree/bindings/net/ti,wilink-st.txt | 5 +++++
>  1 file changed, 5 insertions(+)

Reviewed-by: Rob Herring <robh@kernel.org>