All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] Add API to retrieve the Bluetooth Device Address (BD_ADDR)
@ 2018-09-20 22:34 Matthias Kaehlcke
  2018-09-20 22:34 ` [PATCH 1/2] device property: Add device_get_bd_address() and fwnode_get_bd_address() Matthias Kaehlcke
                   ` (2 more replies)
  0 siblings, 3 replies; 15+ messages in thread
From: Matthias Kaehlcke @ 2018-09-20 22:34 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Rafael J . Wysocki, Sakari Ailus,
	Marcin Wojtas, Andy Shevchenko, Sinan Kaya, Marcel Holtmann,
	Johan Hedberg
  Cc: linux-kernel, linux-bluetooth, Balakrishna Godavarthi,
	Loic Poulain, Brian Norris, 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       | 49 +++++++++++++++++++++++++++++++++++
 drivers/bluetooth/btqcomsmd.c |  5 ++--
 include/linux/property.h      |  4 +++
 3 files changed, 55 insertions(+), 3 deletions(-)

-- 
2.19.0.444.g18242da7ef-goog


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 1/2] device property: Add device_get_bd_address() and fwnode_get_bd_address()
  2018-09-20 22:34 [PATCH 0/2] Add API to retrieve the Bluetooth Device Address (BD_ADDR) Matthias Kaehlcke
@ 2018-09-20 22:34 ` Matthias Kaehlcke
  2018-09-21  4:26   ` Greg Kroah-Hartman
  2018-09-21  7:13   ` Sakari Ailus
  2018-09-20 22:34 ` [PATCH 2/2] Bluetooth: btqcomsmd: Get the BD address with device_get_bd_address() Matthias Kaehlcke
  2018-09-20 22:45 ` [PATCH 0/2] Add API to retrieve the Bluetooth Device Address (BD_ADDR) Sinan Kaya
  2 siblings, 2 replies; 15+ messages in thread
From: Matthias Kaehlcke @ 2018-09-20 22:34 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Rafael J . Wysocki, Sakari Ailus,
	Marcin Wojtas, Andy Shevchenko, Sinan Kaya, Marcel Holtmann,
	Johan Hedberg
  Cc: linux-kernel, linux-bluetooth, Balakrishna Godavarthi,
	Loic Poulain, Brian Norris, 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>
---
 drivers/base/property.c  | 49 ++++++++++++++++++++++++++++++++++++++++
 include/linux/property.h |  4 ++++
 2 files changed, 53 insertions(+)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index 240ab5230ff6..8fe546b9805a 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -18,6 +18,8 @@
 #include <linux/etherdevice.h>
 #include <linux/phy.h>
 
+#define BD_ADDR_LEN	6
+
 struct property_set {
 	struct device *dev;
 	struct fwnode_handle fwnode;
@@ -1315,6 +1317,53 @@ void *device_get_mac_address(struct device *dev, char *addr, int alen)
 }
 EXPORT_SYMBOL(device_get_mac_address);
 
+/**
+ * fwnode_get_bd_address - Get the Bluetooth Device Address (BD_ADDR) from the
+ *                         firmware node
+ * @fwnode:	Pointer to the firmware node
+ * @addr:	Address of buffer to store the BD address in
+ * @alen:	Length of the buffer pointed to by addr, should be BD_ADDR_LEN
+ *
+ * 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, u8 *addr, int alen)
+{
+	u8 buf[BD_ADDR_LEN];
+	int ret;
+
+	if (alen != BD_ADDR_LEN)
+		return -EINVAL;
+
+	ret = fwnode_property_read_u8_array(fwnode, "local-bd-address",
+					    buf, alen);
+	if (ret < 0)
+		return ret;
+	if (is_zero_ether_addr(buf))
+		return -ENODATA;
+
+	memcpy(addr, buf, BD_ADDR_LEN);
+
+	return 0;
+}
+EXPORT_SYMBOL(fwnode_get_bd_address);
+
+/**
+ * device_get_bd_address - Get the Bluetooth Device Address (BD_ADDR) for a
+ *                         given device
+ * @dev:	Pointer to the device
+ * @addr:	Address of buffer to store the BD address in
+ * @alen:	Length of the buffer pointed to by addr, should be BD_ADDR_LEN
+ */
+int device_get_bd_address(struct device *dev, u8 *addr, int alen)
+{
+	return fwnode_get_bd_address(dev_fwnode(dev), addr, alen);
+}
+EXPORT_SYMBOL(device_get_bd_address);
+
 /**
  * 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..4e2f1b276f4f 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -287,9 +287,13 @@ 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, u8 *addr, int alen);
+
 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,
+			  u8 *addr, int alen);
 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


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 2/2] Bluetooth: btqcomsmd: Get the BD address with device_get_bd_address()
  2018-09-20 22:34 [PATCH 0/2] Add API to retrieve the Bluetooth Device Address (BD_ADDR) Matthias Kaehlcke
  2018-09-20 22:34 ` [PATCH 1/2] device property: Add device_get_bd_address() and fwnode_get_bd_address() Matthias Kaehlcke
@ 2018-09-20 22:34 ` Matthias Kaehlcke
  2018-09-20 22:45 ` [PATCH 0/2] Add API to retrieve the Bluetooth Device Address (BD_ADDR) Sinan Kaya
  2 siblings, 0 replies; 15+ messages in thread
From: Matthias Kaehlcke @ 2018-09-20 22:34 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Rafael J . Wysocki, Sakari Ailus,
	Marcin Wojtas, Andy Shevchenko, Sinan Kaya, Marcel Holtmann,
	Johan Hedberg
  Cc: linux-kernel, linux-bluetooth, Balakrishna Godavarthi,
	Loic Poulain, Brian Norris, 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>
---
 drivers/bluetooth/btqcomsmd.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/bluetooth/btqcomsmd.c b/drivers/bluetooth/btqcomsmd.c
index 7df3eed1ef5e..1f2de6aeb3dd 100644
--- a/drivers/bluetooth/btqcomsmd.c
+++ b/drivers/bluetooth/btqcomsmd.c
@@ -172,11 +172,10 @@ 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, (u8 *)&btq->bdaddr,
+				   sizeof(bdaddr_t)))
 		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


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH 0/2] Add API to retrieve the Bluetooth Device Address (BD_ADDR)
  2018-09-20 22:34 [PATCH 0/2] Add API to retrieve the Bluetooth Device Address (BD_ADDR) Matthias Kaehlcke
  2018-09-20 22:34 ` [PATCH 1/2] device property: Add device_get_bd_address() and fwnode_get_bd_address() Matthias Kaehlcke
  2018-09-20 22:34 ` [PATCH 2/2] Bluetooth: btqcomsmd: Get the BD address with device_get_bd_address() Matthias Kaehlcke
@ 2018-09-20 22:45 ` Sinan Kaya
  2018-09-20 23:01   ` Matthias Kaehlcke
  2 siblings, 1 reply; 15+ messages in thread
From: Sinan Kaya @ 2018-09-20 22:45 UTC (permalink / raw)
  To: Matthias Kaehlcke, Greg Kroah-Hartman, Rafael J . Wysocki,
	Sakari Ailus, Marcin Wojtas, Andy Shevchenko, Marcel Holtmann,
	Johan Hedberg
  Cc: linux-kernel, linux-bluetooth, Balakrishna Godavarthi,
	Loic Poulain, Brian Norris

On 9/20/2018 6:34 PM, Matthias Kaehlcke wrote:
> 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.

I don't think you have got enough number of line savings to make this
code to be folded into property.c

Not my call but seems redundant IMO.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 0/2] Add API to retrieve the Bluetooth Device Address (BD_ADDR)
  2018-09-20 22:45 ` [PATCH 0/2] Add API to retrieve the Bluetooth Device Address (BD_ADDR) Sinan Kaya
@ 2018-09-20 23:01   ` Matthias Kaehlcke
  2018-09-20 23:03     ` Sinan Kaya
  0 siblings, 1 reply; 15+ messages in thread
From: Matthias Kaehlcke @ 2018-09-20 23:01 UTC (permalink / raw)
  To: Sinan Kaya
  Cc: Greg Kroah-Hartman, Rafael J . Wysocki, Sakari Ailus,
	Marcin Wojtas, Andy Shevchenko, Marcel Holtmann, Johan Hedberg,
	linux-kernel, linux-bluetooth, Balakrishna Godavarthi,
	Loic Poulain, Brian Norris

On Thu, Sep 20, 2018 at 06:45:58PM -0400, Sinan Kaya wrote:
> On 9/20/2018 6:34 PM, Matthias Kaehlcke wrote:
> > 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.
> 
> I don't think you have got enough number of line savings to make this
> code to be folded into property.c

Most of the new lines are documentation ;-)

> Not my call but seems redundant IMO.

Certainly true if this was the only driver. However another one will
follow soon and 'local-bd-address' is the official binding, so it's
not unlikely there will be more.

Also fwnode_get_bd_address() handles the case where the property
exists but is all zeros which adds a few LOC, which isn't handled by
the btqcomsmd driver.

Cheers

Matthias

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 0/2] Add API to retrieve the Bluetooth Device Address (BD_ADDR)
  2018-09-20 23:01   ` Matthias Kaehlcke
@ 2018-09-20 23:03     ` Sinan Kaya
  2018-09-20 23:19       ` Matthias Kaehlcke
  0 siblings, 1 reply; 15+ messages in thread
From: Sinan Kaya @ 2018-09-20 23:03 UTC (permalink / raw)
  To: Matthias Kaehlcke
  Cc: Greg Kroah-Hartman, Rafael J . Wysocki, Sakari Ailus,
	Marcin Wojtas, Andy Shevchenko, Marcel Holtmann, Johan Hedberg,
	linux-kernel, linux-bluetooth, Balakrishna Godavarthi,
	Loic Poulain, Brian Norris

On 9/20/2018 7:01 PM, Matthias Kaehlcke wrote:
> Certainly true if this was the only driver. However another one will
> follow soon and 'local-bd-address' is the official binding, so it's
> not unlikely there will be more.

I was missing this information.
Is there a way to convert existing users to this if any as well?

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 0/2] Add API to retrieve the Bluetooth Device Address (BD_ADDR)
  2018-09-20 23:03     ` Sinan Kaya
@ 2018-09-20 23:19       ` Matthias Kaehlcke
  2018-09-21  5:25         ` Sinan Kaya
  0 siblings, 1 reply; 15+ messages in thread
From: Matthias Kaehlcke @ 2018-09-20 23:19 UTC (permalink / raw)
  To: Sinan Kaya
  Cc: Greg Kroah-Hartman, Rafael J . Wysocki, Sakari Ailus,
	Marcin Wojtas, Andy Shevchenko, Marcel Holtmann, Johan Hedberg,
	linux-kernel, linux-bluetooth, Balakrishna Godavarthi,
	Loic Poulain, Brian Norris

On Thu, Sep 20, 2018 at 07:03:15PM -0400, Sinan Kaya wrote:
> On 9/20/2018 7:01 PM, Matthias Kaehlcke wrote:
> > Certainly true if this was the only driver. However another one will
> > follow soon and 'local-bd-address' is the official binding, so it's
> > not unlikely there will be more.
> 
> I was missing this information.

https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/net/bluetooth.txt

> Is there a way to convert existing users to this if any as well?

It depends.

If the existing users use a *really* reasonable name for the property
and use the same format it could be an option to add it to the lookup
besides 'local-bd-address', similar to what fwnode_get_mac_address()
does.

If the existing users populate a custom property with the BD address
in the bootloader you could roll out a bootloader change. You'd have
to make sure that bootloader and kernel match. The bootloader could
still populate the custom property to be compatible with 'old'
kernels.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] device property: Add device_get_bd_address() and fwnode_get_bd_address()
  2018-09-20 22:34 ` [PATCH 1/2] device property: Add device_get_bd_address() and fwnode_get_bd_address() Matthias Kaehlcke
@ 2018-09-21  4:26   ` Greg Kroah-Hartman
  2018-09-21 16:30     ` Matthias Kaehlcke
  2018-09-21  7:13   ` Sakari Ailus
  1 sibling, 1 reply; 15+ messages in thread
From: Greg Kroah-Hartman @ 2018-09-21  4:26 UTC (permalink / raw)
  To: Matthias Kaehlcke
  Cc: Rafael J . Wysocki, Sakari Ailus, Marcin Wojtas, Andy Shevchenko,
	Sinan Kaya, Marcel Holtmann, Johan Hedberg, linux-kernel,
	linux-bluetooth, Balakrishna Godavarthi, Loic Poulain,
	Brian Norris

On Thu, Sep 20, 2018 at 03:34:35PM -0700, Matthias Kaehlcke wrote:
> +EXPORT_SYMBOL(fwnode_get_bd_address);

EXPORT_SYMBOL_GPL()?  That matches the majority in this file.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 0/2] Add API to retrieve the Bluetooth Device Address (BD_ADDR)
  2018-09-20 23:19       ` Matthias Kaehlcke
@ 2018-09-21  5:25         ` Sinan Kaya
  2018-09-21 16:45           ` Matthias Kaehlcke
  0 siblings, 1 reply; 15+ messages in thread
From: Sinan Kaya @ 2018-09-21  5:25 UTC (permalink / raw)
  To: Matthias Kaehlcke
  Cc: Greg Kroah-Hartman, Rafael J . Wysocki, Sakari Ailus,
	Marcin Wojtas, Andy Shevchenko, Marcel Holtmann, Johan Hedberg,
	linux-kernel, linux-bluetooth, Balakrishna Godavarthi,
	Loic Poulain, Brian Norris

On 9/20/2018 7:19 PM, Matthias Kaehlcke wrote:
> If the existing users populate a custom property with the BD address
> in the bootloader you could roll out a bootloader change. You'd have
> to make sure that bootloader and kernel match. The bootloader could
> still populate the custom property to be compatible with 'old'
> kernels.

If there is no standard about the name of the property, this probably
doesn't belong to property.c but should rather be hosted in bluetooth
directory for consumption among the bluetooth drivers.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] device property: Add device_get_bd_address() and fwnode_get_bd_address()
  2018-09-20 22:34 ` [PATCH 1/2] device property: Add device_get_bd_address() and fwnode_get_bd_address() Matthias Kaehlcke
  2018-09-21  4:26   ` Greg Kroah-Hartman
@ 2018-09-21  7:13   ` Sakari Ailus
  2018-09-21 12:47     ` Andy Shevchenko
  2018-09-21 16:40     ` Matthias Kaehlcke
  1 sibling, 2 replies; 15+ messages in thread
From: Sakari Ailus @ 2018-09-21  7:13 UTC (permalink / raw)
  To: Matthias Kaehlcke
  Cc: Greg Kroah-Hartman, Rafael J . Wysocki, Marcin Wojtas,
	Andy Shevchenko, Sinan Kaya, Marcel Holtmann, Johan Hedberg,
	linux-kernel, linux-bluetooth, Balakrishna Godavarthi,
	Loic Poulain, Brian Norris

Hi Matthias,

On Thu, Sep 20, 2018 at 03:34:35PM -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.
> 
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
>  drivers/base/property.c  | 49 ++++++++++++++++++++++++++++++++++++++++
>  include/linux/property.h |  4 ++++
>  2 files changed, 53 insertions(+)
> 
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index 240ab5230ff6..8fe546b9805a 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -18,6 +18,8 @@
>  #include <linux/etherdevice.h>
>  #include <linux/phy.h>
>  
> +#define BD_ADDR_LEN	6
> +
>  struct property_set {
>  	struct device *dev;
>  	struct fwnode_handle fwnode;
> @@ -1315,6 +1317,53 @@ void *device_get_mac_address(struct device *dev, char *addr, int alen)
>  }
>  EXPORT_SYMBOL(device_get_mac_address);
>  
> +/**
> + * fwnode_get_bd_address - Get the Bluetooth Device Address (BD_ADDR) from the
> + *                         firmware node
> + * @fwnode:	Pointer to the firmware node
> + * @addr:	Address of buffer to store the BD address in
> + * @alen:	Length of the buffer pointed to by addr, should be BD_ADDR_LEN
> + *
> + * 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, u8 *addr, int alen)

How about using bdaddr_t instead? The original caller was casting it to u8
probably just because there was no function specifically for this purpose.

I wonder if this also should be made depend on CONFIG_BT. The fwnode
framework is unconditionally a part of the kernel and it's not very big at
the moment but adding stuff little by little will add up eventually.

> +{
> +	u8 buf[BD_ADDR_LEN];
> +	int ret;
> +
> +	if (alen != BD_ADDR_LEN)
> +		return -EINVAL;
> +
> +	ret = fwnode_property_read_u8_array(fwnode, "local-bd-address",
> +					    buf, alen);
> +	if (ret < 0)
> +		return ret;
> +	if (is_zero_ether_addr(buf))
> +		return -ENODATA;
> +
> +	memcpy(addr, buf, BD_ADDR_LEN);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(fwnode_get_bd_address);
> +
> +/**
> + * device_get_bd_address - Get the Bluetooth Device Address (BD_ADDR) for a
> + *                         given device
> + * @dev:	Pointer to the device
> + * @addr:	Address of buffer to store the BD address in
> + * @alen:	Length of the buffer pointed to by addr, should be BD_ADDR_LEN
> + */
> +int device_get_bd_address(struct device *dev, u8 *addr, int alen)
> +{
> +	return fwnode_get_bd_address(dev_fwnode(dev), addr, alen);
> +}
> +EXPORT_SYMBOL(device_get_bd_address);
> +
>  /**
>   * 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..4e2f1b276f4f 100644
> --- a/include/linux/property.h
> +++ b/include/linux/property.h
> @@ -287,9 +287,13 @@ 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, u8 *addr, int alen);
> +
>  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,
> +			  u8 *addr, int alen);

This would be a good opportunity to add a newline here, and avoid adding
one after device_get_mac_address().

>  struct fwnode_handle *fwnode_graph_get_next_endpoint(
>  	const struct fwnode_handle *fwnode, struct fwnode_handle *prev);
>  struct fwnode_handle *

-- 
Regards,

Sakari Ailus
sakari.ailus@linux.intel.com

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] device property: Add device_get_bd_address() and fwnode_get_bd_address()
  2018-09-21  7:13   ` Sakari Ailus
@ 2018-09-21 12:47     ` Andy Shevchenko
  2018-09-21 16:40     ` Matthias Kaehlcke
  1 sibling, 0 replies; 15+ messages in thread
From: Andy Shevchenko @ 2018-09-21 12:47 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: Matthias Kaehlcke, Greg Kroah-Hartman, Rafael J. Wysocki, mw,
	Andy Shevchenko, Sinan Kaya, Marcel Holtmann, Johan Hedberg,
	Linux Kernel Mailing List, linux-bluetooth, bgodavar,
	Loic Poulain, Brian Norris

On Fri, Sep 21, 2018 at 10:16 AM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Matthias,
>
> On Thu, Sep 20, 2018 at 03:34:35PM -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.
> >

I have nothing to object for the idea and agree fully on Sakari's comments.

After addressing them, fill free to add my

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

> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > ---
> >  drivers/base/property.c  | 49 ++++++++++++++++++++++++++++++++++++++++
> >  include/linux/property.h |  4 ++++
> >  2 files changed, 53 insertions(+)
> >
> > diff --git a/drivers/base/property.c b/drivers/base/property.c
> > index 240ab5230ff6..8fe546b9805a 100644
> > --- a/drivers/base/property.c
> > +++ b/drivers/base/property.c
> > @@ -18,6 +18,8 @@
> >  #include <linux/etherdevice.h>
> >  #include <linux/phy.h>
> >
> > +#define BD_ADDR_LEN  6
> > +
> >  struct property_set {
> >       struct device *dev;
> >       struct fwnode_handle fwnode;
> > @@ -1315,6 +1317,53 @@ void *device_get_mac_address(struct device *dev, char *addr, int alen)
> >  }
> >  EXPORT_SYMBOL(device_get_mac_address);
> >
> > +/**
> > + * fwnode_get_bd_address - Get the Bluetooth Device Address (BD_ADDR) from the
> > + *                         firmware node
> > + * @fwnode:  Pointer to the firmware node
> > + * @addr:    Address of buffer to store the BD address in
> > + * @alen:    Length of the buffer pointed to by addr, should be BD_ADDR_LEN
> > + *
> > + * 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, u8 *addr, int alen)
>
> How about using bdaddr_t instead? The original caller was casting it to u8
> probably just because there was no function specifically for this purpose.
>
> I wonder if this also should be made depend on CONFIG_BT. The fwnode
> framework is unconditionally a part of the kernel and it's not very big at
> the moment but adding stuff little by little will add up eventually.
>
> > +{
> > +     u8 buf[BD_ADDR_LEN];
> > +     int ret;
> > +
> > +     if (alen != BD_ADDR_LEN)
> > +             return -EINVAL;
> > +
> > +     ret = fwnode_property_read_u8_array(fwnode, "local-bd-address",
> > +                                         buf, alen);
> > +     if (ret < 0)
> > +             return ret;
> > +     if (is_zero_ether_addr(buf))
> > +             return -ENODATA;
> > +
> > +     memcpy(addr, buf, BD_ADDR_LEN);
> > +
> > +     return 0;
> > +}
> > +EXPORT_SYMBOL(fwnode_get_bd_address);
> > +
> > +/**
> > + * device_get_bd_address - Get the Bluetooth Device Address (BD_ADDR) for a
> > + *                         given device
> > + * @dev:     Pointer to the device
> > + * @addr:    Address of buffer to store the BD address in
> > + * @alen:    Length of the buffer pointed to by addr, should be BD_ADDR_LEN
> > + */
> > +int device_get_bd_address(struct device *dev, u8 *addr, int alen)
> > +{
> > +     return fwnode_get_bd_address(dev_fwnode(dev), addr, alen);
> > +}
> > +EXPORT_SYMBOL(device_get_bd_address);
> > +
> >  /**
> >   * 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..4e2f1b276f4f 100644
> > --- a/include/linux/property.h
> > +++ b/include/linux/property.h
> > @@ -287,9 +287,13 @@ 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, u8 *addr, int alen);
> > +
> >  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,
> > +                       u8 *addr, int alen);
>
> This would be a good opportunity to add a newline here, and avoid adding
> one after device_get_mac_address().
>
> >  struct fwnode_handle *fwnode_graph_get_next_endpoint(
> >       const struct fwnode_handle *fwnode, struct fwnode_handle *prev);
> >  struct fwnode_handle *
>
> --
> Regards,
>
> Sakari Ailus
> sakari.ailus@linux.intel.com



-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] device property: Add device_get_bd_address() and fwnode_get_bd_address()
  2018-09-21  4:26   ` Greg Kroah-Hartman
@ 2018-09-21 16:30     ` Matthias Kaehlcke
  0 siblings, 0 replies; 15+ messages in thread
From: Matthias Kaehlcke @ 2018-09-21 16:30 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Rafael J . Wysocki, Sakari Ailus, Marcin Wojtas, Andy Shevchenko,
	Sinan Kaya, Marcel Holtmann, Johan Hedberg, linux-kernel,
	linux-bluetooth, Balakrishna Godavarthi, Loic Poulain,
	Brian Norris

On Fri, Sep 21, 2018 at 06:26:40AM +0200, Greg Kroah-Hartman wrote:
> On Thu, Sep 20, 2018 at 03:34:35PM -0700, Matthias Kaehlcke wrote:
> > +EXPORT_SYMBOL(fwnode_get_bd_address);
> 
> EXPORT_SYMBOL_GPL()?  That matches the majority in this file.

Sounds good

Thanks for the review!

m.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] device property: Add device_get_bd_address() and fwnode_get_bd_address()
  2018-09-21  7:13   ` Sakari Ailus
  2018-09-21 12:47     ` Andy Shevchenko
@ 2018-09-21 16:40     ` Matthias Kaehlcke
  1 sibling, 0 replies; 15+ messages in thread
From: Matthias Kaehlcke @ 2018-09-21 16:40 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: Greg Kroah-Hartman, Rafael J . Wysocki, Marcin Wojtas,
	Andy Shevchenko, Sinan Kaya, Marcel Holtmann, Johan Hedberg,
	linux-kernel, linux-bluetooth, Balakrishna Godavarthi,
	Loic Poulain, Brian Norris

Hi Sakari,

On Fri, Sep 21, 2018 at 10:13:12AM +0300, Sakari Ailus wrote:
> Hi Matthias,
> 
> On Thu, Sep 20, 2018 at 03:34:35PM -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.
> > 
> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > ---
> >  drivers/base/property.c  | 49 ++++++++++++++++++++++++++++++++++++++++
> >  include/linux/property.h |  4 ++++
> >  2 files changed, 53 insertions(+)
> > 
> > diff --git a/drivers/base/property.c b/drivers/base/property.c
> > index 240ab5230ff6..8fe546b9805a 100644
> > --- a/drivers/base/property.c
> > +++ b/drivers/base/property.c
> > @@ -18,6 +18,8 @@
> >  #include <linux/etherdevice.h>
> >  #include <linux/phy.h>
> >  
> > +#define BD_ADDR_LEN	6
> > +
> >  struct property_set {
> >  	struct device *dev;
> >  	struct fwnode_handle fwnode;
> > @@ -1315,6 +1317,53 @@ void *device_get_mac_address(struct device *dev, char *addr, int alen)
> >  }
> >  EXPORT_SYMBOL(device_get_mac_address);
> >  
> > +/**
> > + * fwnode_get_bd_address - Get the Bluetooth Device Address (BD_ADDR) from the
> > + *                         firmware node
> > + * @fwnode:	Pointer to the firmware node
> > + * @addr:	Address of buffer to store the BD address in
> > + * @alen:	Length of the buffer pointed to by addr, should be BD_ADDR_LEN
> > + *
> > + * 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, u8 *addr, int alen)
> 
> How about using bdaddr_t instead? The original caller was casting it to u8
> probably just because there was no function specifically for this purpose.

I was considering that, but wasn't sure if it would be 'correct' to
include net/bluetooth/bluetooth.h (with the typdef of bdaddr_t) from
property.h since it is very frugal on headers and complex types.

That said I'd also prefer to use bdaddr_t if the API isn't limited to
basic data types on purpose.

> I wonder if this also should be made depend on CONFIG_BT. The fwnode
> framework is unconditionally a part of the kernel and it's not very big at
> the moment but adding stuff little by little will add up eventually.

Sounds good!

> > +{
> > +	u8 buf[BD_ADDR_LEN];
> > +	int ret;
> > +
> > +	if (alen != BD_ADDR_LEN)
> > +		return -EINVAL;
> > +
> > +	ret = fwnode_property_read_u8_array(fwnode, "local-bd-address",
> > +					    buf, alen);
> > +	if (ret < 0)
> > +		return ret;
> > +	if (is_zero_ether_addr(buf))
> > +		return -ENODATA;
> > +
> > +	memcpy(addr, buf, BD_ADDR_LEN);
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(fwnode_get_bd_address);
> > +
> > +/**
> > + * device_get_bd_address - Get the Bluetooth Device Address (BD_ADDR) for a
> > + *                         given device
> > + * @dev:	Pointer to the device
> > + * @addr:	Address of buffer to store the BD address in
> > + * @alen:	Length of the buffer pointed to by addr, should be BD_ADDR_LEN
> > + */
> > +int device_get_bd_address(struct device *dev, u8 *addr, int alen)
> > +{
> > +	return fwnode_get_bd_address(dev_fwnode(dev), addr, alen);
> > +}
> > +EXPORT_SYMBOL(device_get_bd_address);
> > +
> >  /**
> >   * 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..4e2f1b276f4f 100644
> > --- a/include/linux/property.h
> > +++ b/include/linux/property.h
> > @@ -287,9 +287,13 @@ 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, u8 *addr, int alen);
> > +
> >  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,
> > +			  u8 *addr, int alen);
> 
> This would be a good opportunity to add a newline here, and avoid adding
> one after device_get_mac_address().

Will do

Thanks for the review!

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 0/2] Add API to retrieve the Bluetooth Device Address (BD_ADDR)
  2018-09-21  5:25         ` Sinan Kaya
@ 2018-09-21 16:45           ` Matthias Kaehlcke
  2018-09-21 16:49             ` Sinan Kaya
  0 siblings, 1 reply; 15+ messages in thread
From: Matthias Kaehlcke @ 2018-09-21 16:45 UTC (permalink / raw)
  To: Sinan Kaya
  Cc: Greg Kroah-Hartman, Rafael J . Wysocki, Sakari Ailus,
	Marcin Wojtas, Andy Shevchenko, Marcel Holtmann, Johan Hedberg,
	linux-kernel, linux-bluetooth, Balakrishna Godavarthi,
	Loic Poulain, Brian Norris

On Fri, Sep 21, 2018 at 01:25:45AM -0400, Sinan Kaya wrote:
> On 9/20/2018 7:19 PM, Matthias Kaehlcke wrote:
> > If the existing users populate a custom property with the BD address
> > in the bootloader you could roll out a bootloader change. You'd have
> > to make sure that bootloader and kernel match. The bootloader could
> > still populate the custom property to be compatible with 'old'
> > kernels.
> 
> If there is no standard about the name of the property, this probably
> doesn't belong to property.c but should rather be hosted in bluetooth
> directory for consumption among the bluetooth drivers.

Well, there is a generic binding
(https://elixir.bootlin.com/linux/v4.18/source/Documentation/devicetree/bindings/net/bluetooth.txt),
the fact that there are drivers that aren't adhering to it doesn't
change that.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 0/2] Add API to retrieve the Bluetooth Device Address (BD_ADDR)
  2018-09-21 16:45           ` Matthias Kaehlcke
@ 2018-09-21 16:49             ` Sinan Kaya
  0 siblings, 0 replies; 15+ messages in thread
From: Sinan Kaya @ 2018-09-21 16:49 UTC (permalink / raw)
  To: Matthias Kaehlcke
  Cc: Greg Kroah-Hartman, Rafael J . Wysocki, Sakari Ailus,
	Marcin Wojtas, Andy Shevchenko, Marcel Holtmann, Johan Hedberg,
	linux-kernel, linux-bluetooth, Balakrishna Godavarthi,
	Loic Poulain, Brian Norris

On 9/21/2018 12:45 PM, Matthias Kaehlcke wrote:
> Well, there is a generic binding
> (https://elixir.bootlin.com/linux/v4.18/source/Documentation/devicetree/bindings/net/bluetooth.txt),
> the fact that there are drivers that aren't adhering to it doesn't
> change that.

OK. If there is some soft of a "standard" naming convention, we should be good.
Otherwise, as Sakari also raised the concern; common code becomes a dumping
ground for drivers.

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2018-09-21 16:49 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-20 22:34 [PATCH 0/2] Add API to retrieve the Bluetooth Device Address (BD_ADDR) Matthias Kaehlcke
2018-09-20 22:34 ` [PATCH 1/2] device property: Add device_get_bd_address() and fwnode_get_bd_address() Matthias Kaehlcke
2018-09-21  4:26   ` Greg Kroah-Hartman
2018-09-21 16:30     ` Matthias Kaehlcke
2018-09-21  7:13   ` Sakari Ailus
2018-09-21 12:47     ` Andy Shevchenko
2018-09-21 16:40     ` Matthias Kaehlcke
2018-09-20 22:34 ` [PATCH 2/2] Bluetooth: btqcomsmd: Get the BD address with device_get_bd_address() Matthias Kaehlcke
2018-09-20 22:45 ` [PATCH 0/2] Add API to retrieve the Bluetooth Device Address (BD_ADDR) Sinan Kaya
2018-09-20 23:01   ` Matthias Kaehlcke
2018-09-20 23:03     ` Sinan Kaya
2018-09-20 23:19       ` Matthias Kaehlcke
2018-09-21  5:25         ` Sinan Kaya
2018-09-21 16:45           ` Matthias Kaehlcke
2018-09-21 16:49             ` Sinan Kaya

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.