Re: [PATCH v4 1/2] Bluetooth: Add device_get_bd_address()

From: Matthias Kaehlcke <mka@chromium.org>
To: Marcel Holtmann <marcel@holtmann.org>,
	"Rafael J . Wysocki" <rafael@kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Sinan Kaya <okaya@codeaurora.org>,
	Balakrishna Godavarthi <bgodavar@codeaurora.org>,
	Sakari Ailus <sakari.ailus@linux.intel.com>,
	Marcin Wojtas <mw@semihalf.com>,
	Andy Shevchenko Andy Shevchenko <andy.shevchenko@gmail.com>,
	Johan Hedberg <johan.hedberg@gmail.com>,
	LKML <linux-kernel@vger.kernel.org>,
	Bluez mailing list <linux-bluetooth@vger.kernel.org>,
	Loic Poulain <loic.poulain@linaro.org>,
	Brian Norris <briannorris@chromium.org>
Subject: Re: [PATCH v4 1/2] Bluetooth: Add device_get_bd_address()
Date: Mon, 15 Oct 2018 11:51:28 -0700	[thread overview]
Message-ID: <20181015185128.GT22824@google.com> (raw)
In-Reply-To: <9B742DB5-F584-4A47-A04B-4F72EB17519C@holtmann.org>

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