| Message ID | 1512701860-8321-1-git-send-email-david@lechnology.com |
|---|---|
| Headers | show |
| Series | Bluetooth: hci_ll: Get BD address from NVMEM (was "bluetooth: hci_ll: Get MAC address from NVMEM") | expand |
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@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); > + 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@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. > + */ 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@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); >> + > > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@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); >>> + >> 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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html