* [PATCH v4 0/2] Add API to retrieve the Bluetooth Device Address (BD_ADDR) @ 2018-09-27 0:48 Matthias Kaehlcke 2018-09-27 0:48 ` [PATCH v4 1/2] Bluetooth: Add device_get_bd_address() Matthias Kaehlcke 2018-09-27 0:48 ` [PATCH v4 2/2] Bluetooth: btqcomsmd: Get the BD address with device_get_bd_address() Matthias Kaehlcke 0 siblings, 2 replies; 15+ messages in thread From: Matthias Kaehlcke @ 2018-09-27 0:48 UTC (permalink / raw) To: Greg Kroah-Hartman, Rafael J . Wysocki, Sakari Ailus, Marcin Wojtas, Andy Shevchenko 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 address is through the device tree. The btqcomsmd driver is an example, it can read the 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): Bluetooth: Add device_get_bd_address() Bluetooth: btqcomsmd: Get the BD address with device_get_bd_address() drivers/bluetooth/btqcomsmd.c | 4 +--- include/net/bluetooth/bluetooth.h | 2 ++ net/bluetooth/lib.c | 34 +++++++++++++++++++++++++++++++ 3 files changed, 37 insertions(+), 3 deletions(-) -- 2.19.0.605.g01d371f741-goog ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v4 1/2] Bluetooth: Add device_get_bd_address() 2018-09-27 0:48 [PATCH v4 0/2] Add API to retrieve the Bluetooth Device Address (BD_ADDR) Matthias Kaehlcke @ 2018-09-27 0:48 ` Matthias Kaehlcke 2018-09-27 6:50 ` Sakari Ailus 2018-09-27 16:41 ` Balakrishna Godavarthi 2018-09-27 0:48 ` [PATCH v4 2/2] Bluetooth: btqcomsmd: Get the BD address with device_get_bd_address() Matthias Kaehlcke 1 sibling, 2 replies; 15+ messages in thread From: Matthias Kaehlcke @ 2018-09-27 0:48 UTC (permalink / raw) To: Greg Kroah-Hartman, Rafael J . Wysocki, Sakari Ailus, Marcin Wojtas, Andy Shevchenko 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 firmware node of the device 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> Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- Changes in v4: - move code from driver/base/property.c to net/bluetooth/lib.c - undo move of bdaddr_t declaration - merge fwnode_get_bd_address() into device_get_bd_address(). as of now the function is not needed, it can be created later if necessary - minor improvements suggested by Sakari - updated commit message - added 'Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>' tag 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 --- include/net/bluetooth/bluetooth.h | 2 ++ net/bluetooth/lib.c | 34 +++++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+) diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h index ec9d6bc65855..6c4cecfda816 100644 --- a/include/net/bluetooth/bluetooth.h +++ b/include/net/bluetooth/bluetooth.h @@ -413,4 +413,6 @@ void mgmt_exit(void); void bt_sock_reclassify_lock(struct sock *sk, int proto); +int device_get_bd_address(struct device *dev, bdaddr_t *bd_addr); + #endif /* __BLUETOOTH_H */ diff --git a/net/bluetooth/lib.c b/net/bluetooth/lib.c index 63e65d9b4b24..78a58ea586c6 100644 --- a/net/bluetooth/lib.c +++ b/net/bluetooth/lib.c @@ -26,7 +26,10 @@ #define pr_fmt(fmt) "Bluetooth: " fmt +#include <linux/etherdevice.h> #include <linux/export.h> +#include <linux/fwnode.h> +#include <linux/property.h> #include <net/bluetooth/bluetooth.h> @@ -198,3 +201,34 @@ void bt_err_ratelimited(const char *format, ...) va_end(args); } EXPORT_SYMBOL(bt_err_ratelimited); + +/** + * 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 + * + * Search the firmware node of the device 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 device_get_bd_address(struct device *dev, bdaddr_t *bd_addr) +{ + struct fwnode_handle *fwnode = dev_fwnode(dev); + bdaddr_t ba; + int ret; + + ret = fwnode_property_read_u8_array(fwnode, "local-bd-address", + (u8 *)&ba, sizeof(ba)); + if (ret < 0) + return ret; + if (is_zero_ether_addr((u8 *)&ba)) + return -ENODATA; + + *bd_addr = ba; + + return 0; +} +EXPORT_SYMBOL_GPL(device_get_bd_address); -- 2.19.0.605.g01d371f741-goog ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v4 1/2] Bluetooth: Add device_get_bd_address() 2018-09-27 0:48 ` [PATCH v4 1/2] Bluetooth: Add device_get_bd_address() Matthias Kaehlcke @ 2018-09-27 6:50 ` Sakari Ailus 2018-09-27 16:41 ` Balakrishna Godavarthi 1 sibling, 0 replies; 15+ messages in thread From: Sakari Ailus @ 2018-09-27 6:50 UTC (permalink / raw) To: Matthias Kaehlcke Cc: Greg Kroah-Hartman, Rafael J . Wysocki, Marcin Wojtas, Andy Shevchenko Andy Shevchenko, Sinan Kaya, Marcel Holtmann, Johan Hedberg, linux-kernel, linux-bluetooth, Balakrishna Godavarthi, Loic Poulain, Brian Norris On Wed, Sep 26, 2018 at 05:48:09PM -0700, Matthias Kaehlcke wrote: > Provide an API for Bluetooth drivers to retrieve the Bluetooth Device > address (BD_ADDR) for a device. If the firmware node of the device > 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> > Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > Changes in v4: > - move code from driver/base/property.c to net/bluetooth/lib.c > - undo move of bdaddr_t declaration > - merge fwnode_get_bd_address() into device_get_bd_address(). as of now > the function is not needed, it can be created later if necessary > - minor improvements suggested by Sakari > - updated commit message > - added 'Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>' tag > > 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 > --- > include/net/bluetooth/bluetooth.h | 2 ++ > net/bluetooth/lib.c | 34 +++++++++++++++++++++++++++++++ Yeah, this seems like a better place for this stuff indeed. Thanks! -- Sakari Ailus sakari.ailus@linux.intel.com ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 1/2] Bluetooth: Add device_get_bd_address() 2018-09-27 0:48 ` [PATCH v4 1/2] Bluetooth: Add device_get_bd_address() Matthias Kaehlcke 2018-09-27 6:50 ` Sakari Ailus @ 2018-09-27 16:41 ` Balakrishna Godavarthi 2018-09-27 16:47 ` Sinan Kaya 1 sibling, 1 reply; 15+ messages in thread From: Balakrishna Godavarthi @ 2018-09-27 16:41 UTC (permalink / raw) To: Matthias Kaehlcke Cc: Greg Kroah-Hartman, Rafael J . Wysocki, Sakari Ailus, Marcin Wojtas, Andy Shevchenko Andy Shevchenko, Sinan Kaya, Marcel Holtmann, Johan Hedberg, linux-kernel, linux-bluetooth, Loic Poulain, Brian Norris Hi Matthias, On 2018-09-27 06:18, Matthias Kaehlcke wrote: > Provide an API for Bluetooth drivers to retrieve the Bluetooth Device > address (BD_ADDR) for a device. If the firmware node of the device > 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> > Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > Changes in v4: > - move code from driver/base/property.c to net/bluetooth/lib.c > - undo move of bdaddr_t declaration > - merge fwnode_get_bd_address() into device_get_bd_address(). as of now > the function is not needed, it can be created later if necessary > - minor improvements suggested by Sakari > - updated commit message > - added 'Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>' tag > > 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 > --- > include/net/bluetooth/bluetooth.h | 2 ++ > net/bluetooth/lib.c | 34 +++++++++++++++++++++++++++++++ > 2 files changed, 36 insertions(+) > > diff --git a/include/net/bluetooth/bluetooth.h > b/include/net/bluetooth/bluetooth.h > index ec9d6bc65855..6c4cecfda816 100644 > --- a/include/net/bluetooth/bluetooth.h > +++ b/include/net/bluetooth/bluetooth.h > @@ -413,4 +413,6 @@ void mgmt_exit(void); > > void bt_sock_reclassify_lock(struct sock *sk, int proto); > > +int device_get_bd_address(struct device *dev, bdaddr_t *bd_addr); > + > #endif /* __BLUETOOTH_H */ > diff --git a/net/bluetooth/lib.c b/net/bluetooth/lib.c > index 63e65d9b4b24..78a58ea586c6 100644 > --- a/net/bluetooth/lib.c > +++ b/net/bluetooth/lib.c > @@ -26,7 +26,10 @@ > > #define pr_fmt(fmt) "Bluetooth: " fmt > > +#include <linux/etherdevice.h> > #include <linux/export.h> > +#include <linux/fwnode.h> > +#include <linux/property.h> > > #include <net/bluetooth/bluetooth.h> > > @@ -198,3 +201,34 @@ void bt_err_ratelimited(const char *format, ...) > va_end(args); > } > EXPORT_SYMBOL(bt_err_ratelimited); > + > +/** > + * 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 > + * > + * Search the firmware node of the device 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 device_get_bd_address(struct device *dev, bdaddr_t *bd_addr) > +{ > + struct fwnode_handle *fwnode = dev_fwnode(dev); > + bdaddr_t ba; > + int ret; > + > + ret = fwnode_property_read_u8_array(fwnode, "local-bd-address", > + (u8 *)&ba, sizeof(ba)); > + if (ret < 0) > + return ret; > + if (is_zero_ether_addr((u8 *)&ba)) > + return -ENODATA; > + > + *bd_addr = ba; > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(device_get_bd_address); Reviewed-by: Balakrishna Godavarthi <bgodavar@codeaurora.org> -- Regards Balakrishna. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 1/2] Bluetooth: Add device_get_bd_address() 2018-09-27 16:41 ` Balakrishna Godavarthi @ 2018-09-27 16:47 ` Sinan Kaya 2018-09-27 17:13 ` Matthias Kaehlcke 0 siblings, 1 reply; 15+ messages in thread From: Sinan Kaya @ 2018-09-27 16:47 UTC (permalink / raw) To: Balakrishna Godavarthi, Matthias Kaehlcke Cc: Greg Kroah-Hartman, Rafael J . Wysocki, Sakari Ailus, Marcin Wojtas, Andy Shevchenko Andy Shevchenko, Marcel Holtmann, Johan Hedberg, linux-kernel, linux-bluetooth, Loic Poulain, Brian Norris On 9/27/2018 12:41 PM, Balakrishna Godavarthi wrote: > void bt_sock_reclassify_lock(struct sock *sk, int proto); > > +int device_get_bd_address(struct device *dev, bdaddr_t *bd_addr); Maybe change the API name to start with bt_ and get rid of device_? ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 1/2] Bluetooth: Add device_get_bd_address() 2018-09-27 16:47 ` Sinan Kaya @ 2018-09-27 17:13 ` Matthias Kaehlcke 2018-10-04 17:33 ` Matthias Kaehlcke 0 siblings, 1 reply; 15+ messages in thread From: Matthias Kaehlcke @ 2018-09-27 17:13 UTC (permalink / raw) To: Sinan Kaya Cc: Balakrishna Godavarthi, Greg Kroah-Hartman, Rafael J . Wysocki, Sakari Ailus, Marcin Wojtas, Andy Shevchenko Andy Shevchenko, Marcel Holtmann, Johan Hedberg, linux-kernel, linux-bluetooth, Loic Poulain, Brian Norris On Thu, Sep 27, 2018 at 12:47:06PM -0400, Sinan Kaya wrote: > On 9/27/2018 12:41 PM, Balakrishna Godavarthi wrote: > > void bt_sock_reclassify_lock(struct sock *sk, int proto); > > > > +int device_get_bd_address(struct device *dev, bdaddr_t *bd_addr); > > Maybe change the API name to start with bt_ and get rid of device_? device_ indicates that we get the BD_ADDR for a 'struct device' and not for e.g. a 'struct fwnode_handle'. Anyway with this version of the patch fwnode_get_bd_address() has been scrapped and it might never be introduced again, so I'm open to change the name to bt_ if there is a general preference for it. Cheers Matthias ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 1/2] Bluetooth: Add device_get_bd_address() 2018-09-27 17:13 ` Matthias Kaehlcke @ 2018-10-04 17:33 ` Matthias Kaehlcke 2018-10-15 18:03 ` Matthias Kaehlcke 2018-10-15 18:06 ` Marcel Holtmann 0 siblings, 2 replies; 15+ messages in thread From: Matthias Kaehlcke @ 2018-10-04 17:33 UTC (permalink / raw) To: Sinan Kaya Cc: Balakrishna Godavarthi, Greg Kroah-Hartman, Rafael J . Wysocki, Sakari Ailus, Marcin Wojtas, Andy Shevchenko Andy Shevchenko, Marcel Holtmann, Johan Hedberg, linux-kernel, linux-bluetooth, Loic Poulain, Brian Norris On Thu, Sep 27, 2018 at 10:13:05AM -0700, Matthias Kaehlcke wrote: > On Thu, Sep 27, 2018 at 12:47:06PM -0400, Sinan Kaya wrote: > > On 9/27/2018 12:41 PM, Balakrishna Godavarthi wrote: > > > void bt_sock_reclassify_lock(struct sock *sk, int proto); > > > > > > +int device_get_bd_address(struct device *dev, bdaddr_t *bd_addr); > > > > Maybe change the API name to start with bt_ and get rid of device_? > > device_ indicates that we get the BD_ADDR for a 'struct device' and > not for e.g. a 'struct fwnode_handle'. > > Anyway with this version of the patch fwnode_get_bd_address() has been > scrapped and it might never be introduced again, so I'm open to change > the name to bt_ if there is a general preference for it. Marcel, can you live with this being added to the Bluetooth code base instead of property? Also if you'd prefer the function to be named bt_get_bd_address() let me know. Cheers Matthias ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 1/2] Bluetooth: Add device_get_bd_address() 2018-10-04 17:33 ` Matthias Kaehlcke @ 2018-10-15 18:03 ` Matthias Kaehlcke 2018-10-15 18:06 ` Marcel Holtmann 1 sibling, 0 replies; 15+ messages in thread From: Matthias Kaehlcke @ 2018-10-15 18:03 UTC (permalink / raw) To: Marcel Holtmann Cc: Balakrishna Godavarthi, Greg Kroah-Hartman, Rafael J . Wysocki, Sakari Ailus, Marcin Wojtas, Andy Shevchenko Andy Shevchenko, Johan Hedberg, Sinan Kaya, linux-kernel, linux-bluetooth, Loic Poulain, Brian Norris, Douglas Anderson Hi Marcel, please let me know if any changes are needed to get this patch applied to bluetooth-next. Thanks Matthias On Thu, Oct 04, 2018 at 10:33:38AM -0700, Matthias Kaehlcke wrote: > On Thu, Sep 27, 2018 at 10:13:05AM -0700, Matthias Kaehlcke wrote: > > On Thu, Sep 27, 2018 at 12:47:06PM -0400, Sinan Kaya wrote: > > > On 9/27/2018 12:41 PM, Balakrishna Godavarthi wrote: > > > > void bt_sock_reclassify_lock(struct sock *sk, int proto); > > > > > > > > +int device_get_bd_address(struct device *dev, bdaddr_t *bd_addr); > > > > > > Maybe change the API name to start with bt_ and get rid of device_? > > > > device_ indicates that we get the BD_ADDR for a 'struct device' and > > not for e.g. a 'struct fwnode_handle'. > > > > Anyway with this version of the patch fwnode_get_bd_address() has been > > scrapped and it might never be introduced again, so I'm open to change > > the name to bt_ if there is a general preference for it. > > Marcel, can you live with this being added to the Bluetooth code base > instead of property? Also if you'd prefer the function to be named > bt_get_bd_address() let me know. > > Cheers > > Matthias ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 1/2] Bluetooth: Add device_get_bd_address() 2018-10-04 17:33 ` Matthias Kaehlcke 2018-10-15 18:03 ` Matthias Kaehlcke @ 2018-10-15 18:06 ` Marcel Holtmann 2018-10-15 18:51 ` Matthias Kaehlcke 1 sibling, 1 reply; 15+ messages in thread From: Marcel Holtmann @ 2018-10-15 18:06 UTC (permalink / raw) To: Matthias Kaehlcke Cc: Sinan Kaya, Balakrishna Godavarthi, Greg Kroah-Hartman, Rafael J . Wysocki, Sakari Ailus, Marcin Wojtas, Andy Shevchenko Andy Shevchenko, Johan Hedberg, LKML, Bluez mailing list, Loic Poulain, Brian Norris Hi Matthias, >>>> void bt_sock_reclassify_lock(struct sock *sk, int proto); >>>> >>>> +int device_get_bd_address(struct device *dev, bdaddr_t *bd_addr); >>> >>> Maybe change the API name to start with bt_ and get rid of device_? >> >> device_ indicates that we get the BD_ADDR for a 'struct device' and >> not for e.g. a 'struct fwnode_handle'. >> >> Anyway with this version of the patch fwnode_get_bd_address() has been >> scrapped and it might never be introduced again, so I'm open to change >> the name to bt_ if there is a general preference for it. > > Marcel, can you live with this being added to the Bluetooth code base > instead of property? Also if you'd prefer the function to be named > bt_get_bd_address() let me know. explain to me again why this is useful? I am failing to see the benefit if this is not part of the property.h API. Regards Marcel ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 1/2] Bluetooth: Add device_get_bd_address() 2018-10-15 18:06 ` Marcel Holtmann @ 2018-10-15 18:51 ` Matthias Kaehlcke 2018-10-16 6:52 ` Marcel Holtmann 0 siblings, 1 reply; 15+ messages in thread From: Matthias Kaehlcke @ 2018-10-15 18:51 UTC (permalink / raw) To: Marcel Holtmann, Rafael J . Wysocki, Greg Kroah-Hartman Cc: Sinan Kaya, Balakrishna Godavarthi, Sakari Ailus, Marcin Wojtas, Andy Shevchenko Andy Shevchenko, Johan Hedberg, LKML, Bluez mailing list, Loic Poulain, Brian Norris On Mon, Oct 15, 2018 at 08:06:02PM +0200, Marcel Holtmann wrote: > Hi Matthias, > > >>>> void bt_sock_reclassify_lock(struct sock *sk, int proto); > >>>> > >>>> +int device_get_bd_address(struct device *dev, bdaddr_t *bd_addr); > >>> > >>> Maybe change the API name to start with bt_ and get rid of device_? > >> > >> device_ indicates that we get the BD_ADDR for a 'struct device' and > >> not for e.g. a 'struct fwnode_handle'. > >> > >> Anyway with this version of the patch fwnode_get_bd_address() has been > >> scrapped and it might never be introduced again, so I'm open to change > >> the name to bt_ if there is a general preference for it. > > > > Marcel, can you live with this being added to the Bluetooth code base > > instead of property? Also if you'd prefer the function to be named > > bt_get_bd_address() let me know. > > explain to me again why this is useful? The official binding for providing the BD_ADDR through the device tree is the property 'local-bd-address'. device_get_bd_address() provides a common API to retrieve the BD_ADDR instead of requiring BT drivers to use the lower level fwnode_property_read_u8_array(). It also avoids repeating the check for an all zeroes BD_ADDR in multiple drivers. > I am failing to see the benefit if this is not part of the property.h API. My understanding is that the intention of property.h it to provide an API for common property types used by drivers from different subsystems, hence the implementation 'lives' in drivers/base. Obtaining the BD_ADDR is clearly limited to the Bluetooth subsystem, and drivers/base doesn't seem to be the right place for it. It's true, device_get_mac_address() lives in the common property code, but that doesn't necessarily mean it really should be there and we should do the same. I agree with Sakari that the the approach taken by V4L2 (drivers/media/v4l2-core/v4l2-fwnode.c) seems more appropriate. That said I wouldn't raise opposition if the maintainers of drivers/base agreed to add device_get_mac_address() there, however so far several recent authors of property.[ch] have raised objections. Thanks Matthias ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 1/2] Bluetooth: Add device_get_bd_address() 2018-10-15 18:51 ` Matthias Kaehlcke @ 2018-10-16 6:52 ` Marcel Holtmann 2018-10-16 21:02 ` Matthias Kaehlcke 2018-10-22 21:07 ` Pavel Machek 0 siblings, 2 replies; 15+ messages in thread From: Marcel Holtmann @ 2018-10-16 6:52 UTC (permalink / raw) To: Matthias Kaehlcke Cc: Rafael J . Wysocki, Greg Kroah-Hartman, Sinan Kaya, Balakrishna Godavarthi, Sakari Ailus, Marcin Wojtas, Andy Shevchenko Andy Shevchenko, Johan Hedberg, LKML, Bluez mailing list, Loic Poulain, Brian Norris Hi Matthias, >>>>>> void bt_sock_reclassify_lock(struct sock *sk, int proto); >>>>>> >>>>>> +int device_get_bd_address(struct device *dev, bdaddr_t *bd_addr); >>>>> >>>>> Maybe change the API name to start with bt_ and get rid of device_? >>>> >>>> device_ indicates that we get the BD_ADDR for a 'struct device' and >>>> not for e.g. a 'struct fwnode_handle'. >>>> >>>> Anyway with this version of the patch fwnode_get_bd_address() has been >>>> scrapped and it might never be introduced again, so I'm open to change >>>> the name to bt_ if there is a general preference for it. >>> >>> Marcel, can you live with this being added to the Bluetooth code base >>> instead of property? Also if you'd prefer the function to be named >>> bt_get_bd_address() let me know. >> >> explain to me again why this is useful? > > The official binding for providing the BD_ADDR through the device tree > is the property 'local-bd-address'. device_get_bd_address() provides a > common API to retrieve the BD_ADDR instead of requiring BT drivers to > use the lower level fwnode_property_read_u8_array(). It also avoids > repeating the check for an all zeroes BD_ADDR in multiple drivers. > >> I am failing to see the benefit if this is not part of the property.h API. > > My understanding is that the intention of property.h it to provide an > API for common property types used by drivers from different > subsystems, hence the implementation 'lives' in drivers/base. > Obtaining the BD_ADDR is clearly limited to the Bluetooth subsystem, > and drivers/base doesn't seem to be the right place for it. It's true, > device_get_mac_address() lives in the common property code, but that > doesn't necessarily mean it really should be there and we should do > the same. I agree with Sakari that the the approach taken by V4L2 > (drivers/media/v4l2-core/v4l2-fwnode.c) seems more appropriate. > > That said I wouldn't raise opposition if the maintainers of > drivers/base agreed to add device_get_mac_address() there, however so > far several recent authors of property.[ch] have raised objections. so if this is not in drivers/base/ then what is the point in making each driver do this? If it is a common property, then it can be well handled in the Bluetooth core when setting up the hardware. This whole BD_ADDR via DT is stupid anyway. Just so that is clear up-front. It has been a total hack and fully relies on boot loaders doing too much magic and then using DT to hide this magic. The BD_ADDR is required to be unique and that means no user will ever create a DT with that set. The boot loader always has to read some magic value and then convert it and merge it into the actual DT provided to the kernel. The clean part would be just to have proper APIs to read the memory of the persistent / programmed BD_ADDR and then access that. That all said, we have hdev->set_bdaddr address and the HCI_QUIRK_INVALID_BDADDR to mark the controller as not fully set up. And then actually user space can deal with getting the correct address and providing it. The code is already there that handles all of this if the BD_ADDR comes from user space. Actually hacking this into the driver and doing that in the hdev->setup callback is quirky to begin with. A user space provided address will just overwrite that. If you really want to make this generic, then introduce HCI_QUIRK_USE_BDADDR_PROPERTY that a driver can set and then do that all in hci_dev_do_open() so that if no user space provided BD_ADDR is available, it is read from local-bt-address property and if that is not available or empty, then mark the the device as unconfigured. I am intentionally saying unconfigured when you set HCI_QUIRK_USE_BDADDR_PROPERTY since I assume that the logic that we have behind HCI_QUIRK_INVALID_BDADDR is implied and whatever address comes back via Read_BD_Address is invalid. Otherwise this hardware should not set HCI_QUIRK_USE_BDADDR_PROPERTY at all. Regards Marcel ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 1/2] Bluetooth: Add device_get_bd_address() 2018-10-16 6:52 ` Marcel Holtmann @ 2018-10-16 21:02 ` Matthias Kaehlcke 2018-10-22 21:07 ` Pavel Machek 1 sibling, 0 replies; 15+ messages in thread From: Matthias Kaehlcke @ 2018-10-16 21:02 UTC (permalink / raw) To: Marcel Holtmann Cc: Rafael J . Wysocki, Greg Kroah-Hartman, Sinan Kaya, Balakrishna Godavarthi, Sakari Ailus, Marcin Wojtas, Andy Shevchenko Andy Shevchenko, Johan Hedberg, LKML, Bluez mailing list, Loic Poulain, Brian Norris Hi Marcel, On Tue, Oct 16, 2018 at 08:52:07AM +0200, Marcel Holtmann wrote: > Hi Matthias, > > >>>>>> void bt_sock_reclassify_lock(struct sock *sk, int proto); > >>>>>> > >>>>>> +int device_get_bd_address(struct device *dev, bdaddr_t *bd_addr); > >>>>> > >>>>> Maybe change the API name to start with bt_ and get rid of device_? > >>>> > >>>> device_ indicates that we get the BD_ADDR for a 'struct device' and > >>>> not for e.g. a 'struct fwnode_handle'. > >>>> > >>>> Anyway with this version of the patch fwnode_get_bd_address() has been > >>>> scrapped and it might never be introduced again, so I'm open to change > >>>> the name to bt_ if there is a general preference for it. > >>> > >>> Marcel, can you live with this being added to the Bluetooth code base > >>> instead of property? Also if you'd prefer the function to be named > >>> bt_get_bd_address() let me know. > >> > >> explain to me again why this is useful? > > > > The official binding for providing the BD_ADDR through the device tree > > is the property 'local-bd-address'. device_get_bd_address() provides a > > common API to retrieve the BD_ADDR instead of requiring BT drivers to > > use the lower level fwnode_property_read_u8_array(). It also avoids > > repeating the check for an all zeroes BD_ADDR in multiple drivers. > > > >> I am failing to see the benefit if this is not part of the property.h API. > > > > My understanding is that the intention of property.h it to provide an > > API for common property types used by drivers from different > > subsystems, hence the implementation 'lives' in drivers/base. > > Obtaining the BD_ADDR is clearly limited to the Bluetooth subsystem, > > and drivers/base doesn't seem to be the right place for it. It's true, > > device_get_mac_address() lives in the common property code, but that > > doesn't necessarily mean it really should be there and we should do > > the same. I agree with Sakari that the the approach taken by V4L2 > > (drivers/media/v4l2-core/v4l2-fwnode.c) seems more appropriate. > > > > That said I wouldn't raise opposition if the maintainers of > > drivers/base agreed to add device_get_mac_address() there, however so > > far several recent authors of property.[ch] have raised objections. > > so if this is not in drivers/base/ then what is the point in making > each driver do this? If it is a common property, then it can be well > handled in the Bluetooth core when setting up the hardware. Agreed, it would be better if this can be handled in the Bluetooth core. > This whole BD_ADDR via DT is stupid anyway. Just so that is clear > up-front. It has been a total hack and fully relies on boot loaders > doing too much magic and then using DT to hide this magic. The > BD_ADDR is required to be unique and that means no user will ever > create a DT with that set. The boot loader always has to read some > magic value and then convert it and merge it into the actual DT > provided to the kernel. The clean part would be just to have proper > APIs to read the memory of the persistent / programmed BD_ADDR and > then access that. Yes, the DT approach relies on the bootloader which isn't ideal. Part of the problem is that AFAIK there is currently no standard way for storing/retrieving persistent, board specific values like BD ADDR, so different custom mechanisms are used, which tend to be incompatible with each other (e.g. Chrome OS uses VPD: https://chromium.googlesource.com/chromiumos/platform/vpd/) Using the bootloader & DT is a pragmatic approach, since the DT is well established and the bootloader often already has DT awareness. That said I agree that a common solution would make our lives easier. > That all said, we have hdev->set_bdaddr address and the > HCI_QUIRK_INVALID_BDADDR to mark the controller as not fully set > up. And then actually user space can deal with getting the correct > address and providing it. The code is already there that handles all > of this if the BD_ADDR comes from user space. Actually hacking this > into the driver and doing that in the hdev->setup callback is quirky > to begin with. A user space provided address will just overwrite > that. > > If you really want to make this generic, then introduce > HCI_QUIRK_USE_BDADDR_PROPERTY that a driver can set and then do that > all in hci_dev_do_open() so that if no user space provided BD_ADDR > is available, it is read from local-bt-address property and if that > is not available or empty, then mark the the device as unconfigured. > > I am intentionally saying unconfigured when you set > HCI_QUIRK_USE_BDADDR_PROPERTY since I assume that the logic that we > have behind HCI_QUIRK_INVALID_BDADDR is implied and whatever address > comes back via Read_BD_Address is invalid. Otherwise this hardware > should not set HCI_QUIRK_USE_BDADDR_PROPERTY at all. Thanks for proposing this generic alternative solution and providing details! I'm not really experienced with hacking the Bluetooth core and don't understand your proposal entirely: I get the part of setting the quirk in the driver, checking for it in hci_dev_do_open(), reading the address from 'local-bd-address', setting it with hdev->bd_addr and marking the device as unconfigured if the address is empty/unavailable. I interpret that you suggest that 'local-bd-address' should only be used if user space doesn't provide a BD_ADDR. It is not evident to me where a user space provided address is set, in any case it doesn't seem to be in hci_dev_do_open(), my uneducated guess is that the address is set with the management command SET_PUBLIC_ADDRESS. Could you clarify on this? I also wonder how to identify the DT node corresponding to an HCI device, for hci_qca it's the node of hdev->dev.parent, but I'm not sure if that is universally true. If it isn't looking for the first parent with a DT node could be an option. Thanks Matthias ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 1/2] Bluetooth: Add device_get_bd_address() 2018-10-16 6:52 ` Marcel Holtmann 2018-10-16 21:02 ` Matthias Kaehlcke @ 2018-10-22 21:07 ` Pavel Machek 1 sibling, 0 replies; 15+ messages in thread From: Pavel Machek @ 2018-10-22 21:07 UTC (permalink / raw) To: Marcel Holtmann Cc: Matthias Kaehlcke, Rafael J . Wysocki, Greg Kroah-Hartman, Sinan Kaya, Balakrishna Godavarthi, Sakari Ailus, Marcin Wojtas, Andy Shevchenko Andy Shevchenko, Johan Hedberg, LKML, Bluez mailing list, Loic Poulain, Brian Norris [-- Attachment #1: Type: text/plain, Size: 2212 bytes --] Hi! > >> explain to me again why this is useful? > > > > The official binding for providing the BD_ADDR through the device tree > > is the property 'local-bd-address'. device_get_bd_address() provides a > > common API to retrieve the BD_ADDR instead of requiring BT drivers to > > use the lower level fwnode_property_read_u8_array(). It also avoids > > repeating the check for an all zeroes BD_ADDR in multiple drivers. > > > >> I am failing to see the benefit if this is not part of the property.h API. > > > > My understanding is that the intention of property.h it to provide an > > API for common property types used by drivers from different > > subsystems, hence the implementation 'lives' in drivers/base. > > Obtaining the BD_ADDR is clearly limited to the Bluetooth subsystem, > > and drivers/base doesn't seem to be the right place for it. It's true, > > device_get_mac_address() lives in the common property code, but that > > doesn't necessarily mean it really should be there and we should do > > the same. I agree with Sakari that the the approach taken by V4L2 > > (drivers/media/v4l2-core/v4l2-fwnode.c) seems more appropriate. > > > > That said I wouldn't raise opposition if the maintainers of > > drivers/base agreed to add device_get_mac_address() there, however so > > far several recent authors of property.[ch] have raised objections. > > so if this is not in drivers/base/ then what is the point in making each driver do this? If it is a common property, then it can be well handled in the Bluetooth core when setting up the hardware. > > This whole BD_ADDR via DT is stupid anyway. Just so that is clear > up-front. It has been a total hack and fully relies on boot > loaders doing too much magic and then using DT to hide this magic. Can you wrap lines at around 72 characters in the emails? We do ethernet addresses via DT. I don't know if doing that in bootloader is a hack or not, but if bootloader is already doing that for ethernet, bluettoth address in DT kind of makes sense. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 181 bytes --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v4 2/2] Bluetooth: btqcomsmd: Get the BD address with device_get_bd_address() 2018-09-27 0:48 [PATCH v4 0/2] Add API to retrieve the Bluetooth Device Address (BD_ADDR) Matthias Kaehlcke 2018-09-27 0:48 ` [PATCH v4 1/2] Bluetooth: Add device_get_bd_address() Matthias Kaehlcke @ 2018-09-27 0:48 ` Matthias Kaehlcke 2018-09-27 16:43 ` Balakrishna Godavarthi 1 sibling, 1 reply; 15+ messages in thread From: Matthias Kaehlcke @ 2018-09-27 0:48 UTC (permalink / raw) To: Greg Kroah-Hartman, Rafael J . Wysocki, Sakari Ailus, Marcin Wojtas, Andy Shevchenko 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> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- Changes in v4: - added 'Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>' tag 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v4 2/2] Bluetooth: btqcomsmd: Get the BD address with device_get_bd_address() 2018-09-27 0:48 ` [PATCH v4 2/2] Bluetooth: btqcomsmd: Get the BD address with device_get_bd_address() Matthias Kaehlcke @ 2018-09-27 16:43 ` Balakrishna Godavarthi 0 siblings, 0 replies; 15+ messages in thread From: Balakrishna Godavarthi @ 2018-09-27 16:43 UTC (permalink / raw) To: Matthias Kaehlcke Cc: Greg Kroah-Hartman, Rafael J . Wysocki, Sakari Ailus, Marcin Wojtas, Andy Shevchenko Andy Shevchenko, Sinan Kaya, Marcel Holtmann, Johan Hedberg, linux-kernel, linux-bluetooth, Loic Poulain, Brian Norris Hi Matthias, On 2018-09-27 06:18, 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> > --- > Changes in v4: > - added 'Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>' tag > > 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) Reviewed-by: Balakrishna Godavarthi <bgodavar@codeaurora.org> -- Regards Balakrishna. ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2018-10-22 21:07 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-09-27 0:48 [PATCH v4 0/2] Add API to retrieve the Bluetooth Device Address (BD_ADDR) Matthias Kaehlcke 2018-09-27 0:48 ` [PATCH v4 1/2] Bluetooth: Add device_get_bd_address() Matthias Kaehlcke 2018-09-27 6:50 ` Sakari Ailus 2018-09-27 16:41 ` Balakrishna Godavarthi 2018-09-27 16:47 ` Sinan Kaya 2018-09-27 17:13 ` Matthias Kaehlcke 2018-10-04 17:33 ` Matthias Kaehlcke 2018-10-15 18:03 ` Matthias Kaehlcke 2018-10-15 18:06 ` Marcel Holtmann 2018-10-15 18:51 ` Matthias Kaehlcke 2018-10-16 6:52 ` Marcel Holtmann 2018-10-16 21:02 ` Matthias Kaehlcke 2018-10-22 21:07 ` Pavel Machek 2018-09-27 0:48 ` [PATCH v4 2/2] Bluetooth: btqcomsmd: Get the BD address with device_get_bd_address() Matthias Kaehlcke 2018-09-27 16:43 ` Balakrishna Godavarthi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).