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

From: Matthias Kaehlcke <mka@chromium.org>
To: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Cc: Marcel Holtmann <marcel@holtmann.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	"Rafael J . Wysocki" <rafael@kernel.org>,
	Sakari Ailus <sakari.ailus@linux.intel.com>,
	Marcin Wojtas <mw@semihalf.com>,
	Andy Shevchenko Andy Shevchenko <andy.shevchenko@gmail.com>,
	Sinan Kaya <okaya@codeaurora.org>,
	Johan Hedberg <johan.hedberg@gmail.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Pekka Enberg <penberg@kernel.org>,
	Masahiro Yamada <yamada.masahiro@socionext.com>,
	Alexey Dobriyan <adobriyan@gmail.com>,
	linux-kernel@vger.kernel.org, linux-bluetooth@vger.kernel.org,
	Balakrishna Godavarthi <bgodavar@codeaurora.org>,
	Loic Poulain <loic.poulain@linaro.org>,
	Brian Norris <briannorris@chromium.org>
Subject: Re: [PATCH v3.1 1/2] device property: Add device_get_bd_address() and fwnode_get_bd_address()
Date: Thu, 27 Sep 2018 10:04:49 -0700	[thread overview]
Message-ID: <20180927170449.GF22824@google.com> (raw)
In-Reply-To: <20180927113829.GQ11965@kuha.fi.intel.com>

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