[PATCH v2 0/2] Add API to retrieve the Bluetooth Device Address (BD_ADDR)

[PATCH v2 0/2] Add API to retrieve the Bluetooth Device Address (BD_ADDR)

[Matthias Kaehlcke]

On some systems the Bluetooth Device Address (BD_ADDR) isn't stored
on the Bluetooth chip itself. One way to configure the BD address is
through the device tree. The btqcomsmd driver is an example, it can
read the BD address from the DT property 'local-bd-address'. It is
also planned to extend the hci_qca driver to support setting the BD
address through the DT.

To avoid redundant open-coded reading of 'local-bd-address' and error
handling this series adds an API to retrieve the BD address of a device
and adapts the btqcomsmd driver to use this API.

Matthias Kaehlcke (2):
  device property: Add device_get_bd_address() and
    fwnode_get_bd_address()
  Bluetooth: btqcomsmd: Get the BD address with device_get_bd_address()

 drivers/base/property.c       | 46 +++++++++++++++++++++++++++++++++++
 drivers/bluetooth/btqcomsmd.c |  4 +--
 include/linux/property.h      |  3 +++
 3 files changed, 50 insertions(+), 3 deletions(-)

-- 
2.19.0.444.g18242da7ef-goog

[PATCH v2 1/2] device property: Add device_get_bd_address() and fwnode_get_bd_address()

[Matthias Kaehlcke]

Provide an API for Bluetooth drivers to retrieve the Bluetooth Device
address (BD_ADDR) for a device. If the device node has a property
'local-bd-address' the BD address is read from this property.

Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
Changes in v2:
- use bdaddr_t instead of byte pointer + len
- use EXPORT_SYMBOL_GPL for the new functions instead of EXPORT_SYMBOL
- put new functions inside #if IS_ENABLED(CONFIG_BT)
- some new line juggling in property.h
- added 'Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>' tag
---
 drivers/base/property.c  | 46 ++++++++++++++++++++++++++++++++++++++++
 include/linux/property.h |  3 +++
 2 files changed, 49 insertions(+)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index 240ab5230ff6..afe412133188 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -1315,6 +1315,52 @@ void *device_get_mac_address(struct device *dev, char *addr, int alen)
 }
 EXPORT_SYMBOL(device_get_mac_address);
 
+#if IS_ENABLED(CONFIG_BT)
+
+/**
+ * fwnode_get_bd_address - Get the Bluetooth Device Address (BD_ADDR) from the
+ *                         firmware node
+ * @fwnode:	Pointer to the firmware node
+ * @bd_addr:	Pointer to struct to store the BD address in
+ *
+ * Search the firmware node for 'local-bd-address'.
+ *
+ * All-zero BD addresses are rejected, because those could be properties
+ * that exist in the firmware tables, but were not updated by the firmware. For
+ * example, the DTS could define 'local-bd-address', with zero BD addresses.
+ */
+int fwnode_get_bd_address(struct fwnode_handle *fwnode, bdaddr_t *bd_addr)
+{
+	bdaddr_t ba;
+	int ret;
+
+	ret = fwnode_property_read_u8_array(fwnode, "local-bd-address",
+					    (u8 *)&ba, sizeof(bdaddr_t));
+	if (ret < 0)
+		return ret;
+	if (is_zero_ether_addr((u8 *)&ba))
+		return -ENODATA;
+
+	memcpy(bd_addr, &ba, sizeof(bdaddr_t));
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(fwnode_get_bd_address);
+
+/**
+ * device_get_bd_address - Get the Bluetooth Device Address (BD_ADDR) for a
+ *                         given device
+ * @dev:	Pointer to the device
+ * @bd_addr:	Pointer to struct to store the BD address in
+ */
+int device_get_bd_address(struct device *dev, bdaddr_t *bd_addr)
+{
+	return fwnode_get_bd_address(dev_fwnode(dev), bd_addr);
+}
+EXPORT_SYMBOL_GPL(device_get_bd_address);
+
+#endif
+
 /**
  * fwnode_irq_get - Get IRQ directly from a fwnode
  * @fwnode:	Pointer to the firmware node
diff --git a/include/linux/property.h b/include/linux/property.h
index ac8a1ebc4c1b..8926cf95d27e 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -15,6 +15,7 @@
 
 #include <linux/fwnode.h>
 #include <linux/types.h>
+#include <net/bluetooth/bluetooth.h>
 
 struct device;
 
@@ -286,10 +287,12 @@ const void *device_get_match_data(struct device *dev);
 int device_get_phy_mode(struct device *dev);
 
 void *device_get_mac_address(struct device *dev, char *addr, int alen);
+int device_get_bd_address(struct device *dev, bdaddr_t *bd_addr);
 
 int fwnode_get_phy_mode(struct fwnode_handle *fwnode);
 void *fwnode_get_mac_address(struct fwnode_handle *fwnode,
 			     char *addr, int alen);
+int fwnode_get_bd_address(struct fwnode_handle *fwnode, bdaddr_t *bd_addr);
 struct fwnode_handle *fwnode_graph_get_next_endpoint(
 	const struct fwnode_handle *fwnode, struct fwnode_handle *prev);
 struct fwnode_handle *
-- 
2.19.0.444.g18242da7ef-goog

[PATCH v2 2/2] Bluetooth: btqcomsmd: Get the BD address with device_get_bd_address()

[Matthias Kaehlcke]

Use the new API to get the BD address instead of reading it directly
from the device tree.

Also remove an unncessary pair of braces in the same area of code.

Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
Changes in v2:
- pass bdaddr_t instead of byte pointer + len
---
 drivers/bluetooth/btqcomsmd.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/bluetooth/btqcomsmd.c b/drivers/bluetooth/btqcomsmd.c
index 7df3eed1ef5e..ff74d2c46991 100644
--- a/drivers/bluetooth/btqcomsmd.c
+++ b/drivers/bluetooth/btqcomsmd.c
@@ -172,11 +172,9 @@ static int btqcomsmd_probe(struct platform_device *pdev)
 	/* The local-bd-address property is usually injected by the
 	 * bootloader which has access to the allocated BD address.
 	 */
-	if (!of_property_read_u8_array(pdev->dev.of_node, "local-bd-address",
-				       (u8 *)&btq->bdaddr, sizeof(bdaddr_t))) {
+	if (!device_get_bd_address(&pdev->dev, &btq->bdaddr))
 		dev_info(&pdev->dev, "BD address %pMR retrieved from device-tree",
 			 &btq->bdaddr);
-	}
 
 	hdev = hci_alloc_dev();
 	if (!hdev)
-- 
2.19.0.444.g18242da7ef-goog

Re: [PATCH v2 1/2] device property: Add device_get_bd_address() and fwnode_get_bd_address()

[Andy Shevchenko]

On Mon, Sep 24, 2018 at 10:36 PM Matthias Kaehlcke <mka@chromium.org> wrote:
>
> Provide an API for Bluetooth drivers to retrieve the Bluetooth Device
> address (BD_ADDR) for a device. If the device node has a property
> 'local-bd-address' the BD address is read from this property.

> diff --git a/include/linux/property.h b/include/linux/property.h
> index ac8a1ebc4c1b..8926cf95d27e 100644
> --- a/include/linux/property.h
> +++ b/include/linux/property.h
> @@ -15,6 +15,7 @@
>
>  #include <linux/fwnode.h>
>  #include <linux/types.h>

> +#include <net/bluetooth/bluetooth.h>

Hmm... I don't understand why we need this complete header here.
If you are retrieving bdaddr_t type from it, so, better to move it to
types.h first.

Sorry I didn't notice this before.

>  struct device;

> +int device_get_bd_address(struct device *dev, bdaddr_t *bd_addr);

> +int fwnode_get_bd_address(struct fwnode_handle *fwnode, bdaddr_t *bd_addr);

I don't remember if we discussed below...
Since you put implementations under #ifdef, I'm not sure we can leave
header w/o stubs for !CONFIG_BT case.
I can imagine the case where some driver might use BT functionality
optionally in which case this enforces ugly #ifdef in the driver.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/2] Bluetooth: btqcomsmd: Get the BD address with device_get_bd_address()

[Andy Shevchenko]

On Mon, Sep 24, 2018 at 10:37 PM Matthias Kaehlcke <mka@chromium.org> wrote:
>
> Use the new API to get the BD address instead of reading it directly
> from the device tree.
>
> Also remove an unncessary pair of braces in the same area of code.
>

This looks nice!

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
> Changes in v2:
> - pass bdaddr_t instead of byte pointer + len
> ---
>  drivers/bluetooth/btqcomsmd.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/bluetooth/btqcomsmd.c b/drivers/bluetooth/btqcomsmd.c
> index 7df3eed1ef5e..ff74d2c46991 100644
> --- a/drivers/bluetooth/btqcomsmd.c
> +++ b/drivers/bluetooth/btqcomsmd.c
> @@ -172,11 +172,9 @@ static int btqcomsmd_probe(struct platform_device *pdev)
>         /* The local-bd-address property is usually injected by the
>          * bootloader which has access to the allocated BD address.
>          */
> -       if (!of_property_read_u8_array(pdev->dev.of_node, "local-bd-address",
> -                                      (u8 *)&btq->bdaddr, sizeof(bdaddr_t))) {
> +       if (!device_get_bd_address(&pdev->dev, &btq->bdaddr))
>                 dev_info(&pdev->dev, "BD address %pMR retrieved from device-tree",
>                          &btq->bdaddr);
> -       }
>
>         hdev = hci_alloc_dev();
>         if (!hdev)
> --
> 2.19.0.444.g18242da7ef-goog
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/2] device property: Add device_get_bd_address() and fwnode_get_bd_address()

[Matthias Kaehlcke]

Hi Andy,

thanks for your feedback!

On Mon, Sep 24, 2018 at 11:46:37PM +0300, Andy Shevchenko wrote:
> On Mon, Sep 24, 2018 at 10:36 PM Matthias Kaehlcke <mka@chromium.org> wrote:
> >
> > Provide an API for Bluetooth drivers to retrieve the Bluetooth Device
> > address (BD_ADDR) for a device. If the device node has a property
> > 'local-bd-address' the BD address is read from this property.
> 
> > diff --git a/include/linux/property.h b/include/linux/property.h
> > index ac8a1ebc4c1b..8926cf95d27e 100644
> > --- a/include/linux/property.h
> > +++ b/include/linux/property.h
> > @@ -15,6 +15,7 @@
> >
> >  #include <linux/fwnode.h>
> >  #include <linux/types.h>
> 
> > +#include <net/bluetooth/bluetooth.h>
> 
> Hmm... I don't understand why we need this complete header here.

Yes, this is what deterred me from passing bdaddr_t in the first place.

> If you are retrieving bdaddr_t type from it, so, better to move it to
> types.h first.

Sounds good to me if it is an acceptable solution to put something
bluetoothy in types.h.

> Sorry I didn't notice this before.
> 
> >  struct device;
> 
> > +int device_get_bd_address(struct device *dev, bdaddr_t *bd_addr);
> 
> > +int fwnode_get_bd_address(struct fwnode_handle *fwnode, bdaddr_t *bd_addr);
> 
> I don't remember if we discussed below...
> Since you put implementations under #ifdef, I'm not sure we can leave
> header w/o stubs for !CONFIG_BT case.
> I can imagine the case where some driver might use BT functionality
> optionally in which case this enforces ugly #ifdef in the driver.

To avoid another possible re-spin: do you prefer a single #ifdef, for
both device_get_bd_address() and fwnode_get_bd_address(), or keep the
strict grouping of device_*() and fwnode_*() functions?

Cheers

Matthias