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

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

[Matthias Kaehlcke]

(previous v3 post was messed up, resending as v3.1 with the correct stack)

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          | 18 ++++++++++++
 include/linux/types.h             |  5 ++++
 include/net/bluetooth/bluetooth.h |  5 ----
 5 files changed, 70 insertions(+), 8 deletions(-)

-- 
2.19.0.605.g01d371f741-goog

[PATCH v3.1 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.

The definition of bdaddr_t is moved to types.h to make it visible in
property.h without having to include (the mostly unrelated) bluetooth.h

Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
Changes in v3:
- move definition of bdaddr_t to types.h to avoid include of
  bluetooth.h from property.h
- add stubs for the new functions

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          | 18 ++++++++++++
 include/linux/types.h             |  5 ++++
 include/net/bluetooth/bluetooth.h |  5 ----
 4 files changed, 69 insertions(+), 5 deletions(-)

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..5df4e0bd8c83 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -286,10 +286,28 @@ 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);
+#if IS_ENABLED(CONFIG_BT)
+int device_get_bd_address(struct device *dev, bdaddr_t *bd_addr);
+#else
+static inline int device_get_bd_address(struct device *dev,
+					bdaddr_t *bd_addr)
+{
+	return -ENOTSUPP;
+}
+#endif
 
 int fwnode_get_phy_mode(struct fwnode_handle *fwnode);
 void *fwnode_get_mac_address(struct fwnode_handle *fwnode,
 			     char *addr, int alen);
+#if IS_ENABLED(CONFIG_BT)
+int fwnode_get_bd_address(struct fwnode_handle *fwnode, bdaddr_t *bd_addr);
+#else
+static inline int fwnode_get_bd_address(struct fwnode_handle *fwnode,
+					bdaddr_t *bd_addr)
+{
+	return -ENOTSUPP;
+}
+#endif
 struct fwnode_handle *fwnode_graph_get_next_endpoint(
 	const struct fwnode_handle *fwnode, struct fwnode_handle *prev);
 struct fwnode_handle *
diff --git a/include/linux/types.h b/include/linux/types.h
index 9834e90aa010..ff6984a00a3b 100644
--- a/include/linux/types.h
+++ b/include/linux/types.h
@@ -230,5 +230,10 @@ struct callback_head {
 typedef void (*rcu_callback_t)(struct rcu_head *head);
 typedef void (*call_rcu_func_t)(struct rcu_head *head, rcu_callback_t func);
 
+/* Bluetooth Device Address (BD_ADDR) */
+typedef struct {
+	__u8 b[6];
+} __packed bdaddr_t;
+
 #endif /*  __ASSEMBLY__ */
 #endif /* _LINUX_TYPES_H */
diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index ec9d6bc65855..9401c7431a8f 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -190,11 +190,6 @@ static inline const char *state_to_string(int state)
 	return "invalid state";
 }
 
-/* BD Address */
-typedef struct {
-	__u8 b[6];
-} __packed bdaddr_t;
-
 /* BD Address type */
 #define BDADDR_BREDR		0x00
 #define BDADDR_LE_PUBLIC	0x01
-- 
2.19.0.605.g01d371f741-goog

[PATCH v3.1 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>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
Changes in v3:
- added 'Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>' tag

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.605.g01d371f741-goog

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

[Sakari Ailus]

Hi Matthias,

On Tue, Sep 25, 2018 at 12:10:13PM -0700, Matthias Kaehlcke 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.
> 
> The definition of bdaddr_t is moved to types.h to make it visible in
> property.h without having to include (the mostly unrelated) bluetooth.h
> 
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> ---
> Changes in v3:
> - move definition of bdaddr_t to types.h to avoid include of
>   bluetooth.h from property.h
> - add stubs for the new functions
> 
> 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          | 18 ++++++++++++
>  include/linux/types.h             |  5 ++++
>  include/net/bluetooth/bluetooth.h |  5 ----
>  4 files changed, 69 insertions(+), 5 deletions(-)
> 
> 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));

sizeof(ba)

> +	if (ret < 0)
> +		return ret;
> +	if (is_zero_ether_addr((u8 *)&ba))
> +		return -ENODATA;
> +
> +	memcpy(bd_addr, &ba, sizeof(bdaddr_t));

How about simply:

	*bd_addr = ba;

Either way,

Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>

> +
> +	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..5df4e0bd8c83 100644
> --- a/include/linux/property.h
> +++ b/include/linux/property.h
> @@ -286,10 +286,28 @@ 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);
> +#if IS_ENABLED(CONFIG_BT)
> +int device_get_bd_address(struct device *dev, bdaddr_t *bd_addr);
> +#else
> +static inline int device_get_bd_address(struct device *dev,
> +					bdaddr_t *bd_addr)
> +{
> +	return -ENOTSUPP;
> +}
> +#endif
>  
>  int fwnode_get_phy_mode(struct fwnode_handle *fwnode);
>  void *fwnode_get_mac_address(struct fwnode_handle *fwnode,
>  			     char *addr, int alen);
> +#if IS_ENABLED(CONFIG_BT)
> +int fwnode_get_bd_address(struct fwnode_handle *fwnode, bdaddr_t *bd_addr);
> +#else
> +static inline int fwnode_get_bd_address(struct fwnode_handle *fwnode,
> +					bdaddr_t *bd_addr)
> +{
> +	return -ENOTSUPP;
> +}
> +#endif
>  struct fwnode_handle *fwnode_graph_get_next_endpoint(
>  	const struct fwnode_handle *fwnode, struct fwnode_handle *prev);
>  struct fwnode_handle *
> diff --git a/include/linux/types.h b/include/linux/types.h
> index 9834e90aa010..ff6984a00a3b 100644
> --- a/include/linux/types.h
> +++ b/include/linux/types.h
> @@ -230,5 +230,10 @@ struct callback_head {
>  typedef void (*rcu_callback_t)(struct rcu_head *head);
>  typedef void (*call_rcu_func_t)(struct rcu_head *head, rcu_callback_t func);
>  
> +/* Bluetooth Device Address (BD_ADDR) */
> +typedef struct {
> +	__u8 b[6];
> +} __packed bdaddr_t;
> +
>  #endif /*  __ASSEMBLY__ */
>  #endif /* _LINUX_TYPES_H */
> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> index ec9d6bc65855..9401c7431a8f 100644
> --- a/include/net/bluetooth/bluetooth.h
> +++ b/include/net/bluetooth/bluetooth.h
> @@ -190,11 +190,6 @@ static inline const char *state_to_string(int state)
>  	return "invalid state";
>  }
>  
> -/* BD Address */
> -typedef struct {
> -	__u8 b[6];
> -} __packed bdaddr_t;
> -
>  /* BD Address type */
>  #define BDADDR_BREDR		0x00
>  #define BDADDR_LE_PUBLIC	0x01

-- 
Kind regards,

Sakari Ailus
sakari.ailus@linux.intel.com

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

[Sakari Ailus]

On Tue, Sep 25, 2018 at 12:10:14PM -0700, Matthias Kaehlcke 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.
> 
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

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

[Matthias Kaehlcke]

Hi Sakari,

thanks for the reviews

On Wed, Sep 26, 2018 at 12:33:22AM +0300, Sakari Ailus wrote:
> Hi Matthias,
> 
> On Tue, Sep 25, 2018 at 12:10:13PM -0700, Matthias Kaehlcke 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.
> > 
> > The definition of bdaddr_t is moved to types.h to make it visible in
> > property.h without having to include (the mostly unrelated) bluetooth.h
> > 
> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> > ---
> > Changes in v3:
> > - move definition of bdaddr_t to types.h to avoid include of
> >   bluetooth.h from property.h
> > - add stubs for the new functions
> > 
> > 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          | 18 ++++++++++++
> >  include/linux/types.h             |  5 ++++
> >  include/net/bluetooth/bluetooth.h |  5 ----
> >  4 files changed, 69 insertions(+), 5 deletions(-)
> > 
> > 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));
> 
> sizeof(ba)
> 
> > +	if (ret < 0)
> > +		return ret;
> > +	if (is_zero_ether_addr((u8 *)&ba))
> > +		return -ENODATA;
> > +
> > +	memcpy(bd_addr, &ba, sizeof(bdaddr_t));
> 
> How about simply:
> 
> 	*bd_addr = ba;
> 
> Either way,
> 
> Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Thanks I'll address your comments in v4. I'll wait a day or two before
re-spinning for if others have additional feedback.

Cheers

Matthias

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

[Heikki Krogerus]

On Tue, Sep 25, 2018 at 12:10:13PM -0700, Matthias Kaehlcke wrote:
> +/**
> + * 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);

Let's not fill property.c with framework specific helper functions any
more!

Those functions are completely bluetooth specific, so they do not
belong here. The fact that some other framework already managed to
slip their helpers in does not justify others to do the same.


Thanks,

-- 
heikki

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

[Andy Shevchenko]

On Wed, Sep 26, 2018 at 2:37 PM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> On Tue, Sep 25, 2018 at 12:10:13PM -0700, Matthias Kaehlcke wrote:

> Let's not fill property.c with framework specific helper functions any
> more!
>
> Those functions are completely bluetooth specific, so they do not
> belong here. The fact that some other framework already managed to
> slip their helpers in does not justify others to do the same.

Actually good point.

-- 
With Best Regards,
Andy Shevchenko

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

[Matthias Kaehlcke]

On Wed, Sep 26, 2018 at 02:36:25PM +0300, Heikki Krogerus wrote:
> On Tue, Sep 25, 2018 at 12:10:13PM -0700, Matthias Kaehlcke wrote:
> > +/**
> > + * 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);
> 
> Let's not fill property.c with framework specific helper functions any
> more!
> 
> Those functions are completely bluetooth specific, so they do not
> belong here. The fact that some other framework already managed to
> slip their helpers in does not justify others to do the same.

You have a point, I'll see if I can find a better place.

Thanks

Matthias

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

[Marcel Holtmann]

Hi Heikki,

>> +/**
>> + * 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);
> 
> Let's not fill property.c with framework specific helper functions any
> more!
> 
> Those functions are completely bluetooth specific, so they do not
> belong here. The fact that some other framework already managed to
> slip their helpers in does not justify others to do the same.

so? The firmware guys decided to put MAC addresses and BD addresses into the firmware. So you have to deal with that.

Moving this into the Bluetooth subsystem is as pointless. I rather keep the accessor function to firmware specific data in one place and not spread around the whole tree. Especially once this is also provided via ACPI or some other means. I assumed that is what the whole device_get part was suppose to abstract.

Regards

Marcel

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

[Heikki Krogerus]

Hi Marcel,

On Thu, Sep 27, 2018 at 12:24:33PM +0200, Marcel Holtmann wrote:
> > Let's not fill property.c with framework specific helper functions any
> > more!
> > 
> > Those functions are completely bluetooth specific, so they do not
> > belong here. The fact that some other framework already managed to
> > slip their helpers in does not justify others to do the same.
> 
> so? The firmware guys decided to put MAC addresses and BD addresses into the
> firmware. So you have to deal with that.

I think you have misunderstood the point.

> Moving this into the Bluetooth subsystem is as pointless. I rather keep the
> accessor function to firmware specific data in one place and not spread around
> the whole tree. Especially once this is also provided via ACPI or some other
> means. I assumed that is what the whole device_get part was suppose to
> abstract.

Unified device property API defines a _generic_ API that can be used
by any type of device to access the device properties regardless of
the way the hardware is described.

Any device can use device_property_read_u8/u16/u32/u64/string()
functions, but only bluetooth devices can use device_get_bd_address().
Therefore that function does not belong to drivers/base/properties.c.


Thanks,

-- 
heikki

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

[Sakari Ailus]

Hi guys,

On Thu, Sep 27, 2018 at 02:38:29PM +0300, Heikki Krogerus wrote:
> Hi Marcel,
> 
> On Thu, Sep 27, 2018 at 12:24:33PM +0200, Marcel Holtmann wrote:
> > > Let's not fill property.c with framework specific helper functions any
> > > more!
> > > 
> > > Those functions are completely bluetooth specific, so they do not
> > > belong here. The fact that some other framework already managed to
> > > slip their helpers in does not justify others to do the same.
> > 
> > so? The firmware guys decided to put MAC addresses and BD addresses into the
> > firmware. So you have to deal with that.
> 
> I think you have misunderstood the point.
> 
> > Moving this into the Bluetooth subsystem is as pointless. I rather keep the
> > accessor function to firmware specific data in one place and not spread around
> > the whole tree. Especially once this is also provided via ACPI or some other
> > means. I assumed that is what the whole device_get part was suppose to
> > abstract.
> 
> Unified device property API defines a _generic_ API that can be used
> by any type of device to access the device properties regardless of
> the way the hardware is described.
> 
> Any device can use device_property_read_u8/u16/u32/u64/string()
> functions, but only bluetooth devices can use device_get_bd_address().
> Therefore that function does not belong to drivers/base/properties.c.

FWIW, what's been relevant for V4L2 devices has always been parsed within
the V4L2 framework. It's way more than needed by BT here though; see:

	drivers/media/v4l2-core/v4l2-fwnode.c

I wouldn't think of putting this under drivers/base.

-- 
Regards,

Sakari Ailus
sakari.ailus@linux.intel.com

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

[Matthias Kaehlcke]

On Thu, Sep 27, 2018 at 02:38:29PM +0300, Heikki Krogerus wrote:
> Hi Marcel,
> 
> On Thu, Sep 27, 2018 at 12:24:33PM +0200, Marcel Holtmann wrote:
> > > Let's not fill property.c with framework specific helper functions any
> > > more!
> > > 
> > > Those functions are completely bluetooth specific, so they do not
> > > belong here. The fact that some other framework already managed to
> > > slip their helpers in does not justify others to do the same.
> > 
> > so? The firmware guys decided to put MAC addresses and BD addresses into the
> > firmware. So you have to deal with that.
> 
> I think you have misunderstood the point.
> 
> > Moving this into the Bluetooth subsystem is as pointless. I rather keep the
> > accessor function to firmware specific data in one place and not spread around
> > the whole tree. Especially once this is also provided via ACPI or some other
> > means. I assumed that is what the whole device_get part was suppose to
> > abstract.
> 
> Unified device property API defines a _generic_ API that can be used
> by any type of device to access the device properties regardless of
> the way the hardware is described.
> 
> Any device can use device_property_read_u8/u16/u32/u64/string()
> functions, but only bluetooth devices can use device_get_bd_address().
> Therefore that function does not belong to drivers/base/properties.c.

Initially property.[ch] seemed the correct place to me since the
device_get_bd_address() is the equivalent to
device_get_mac_address(), which lives there. However I doubted to use
bdaddr_t in the interface when I noticed that the other functions only
use generic data types. Not a red flag but a first hint that it's
probably not the right place for Bluetooth specific functions. I
agree with Heikki that the Bluetooth subsystem seems a better home for
this API.

Cheers

Matthias