diff mbox

[03/14] soundwire: Add Master registration

Message ID 1508382211-3154-4-git-send-email-vinod.koul@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Vinod Koul Oct. 19, 2017, 3:03 a.m. UTC
A Master registers with SoundWire bus and scans the firmware provided
for device description. In this patch we scan the ACPI namespaces and
create the SoundWire Slave devices based on the ACPI description

Signed-off-by: Sanyog Kale <sanyog.r.kale@intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
---
 drivers/soundwire/Makefile    |   2 +-
 drivers/soundwire/bus.c       | 150 ++++++++++++++++++++++++++++++++++++
 drivers/soundwire/bus.h       |  20 +++++
 drivers/soundwire/slave.c     | 172 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/soundwire/sdw.h |   3 +
 5 files changed, 346 insertions(+), 1 deletion(-)
 create mode 100644 drivers/soundwire/bus.c
 create mode 100644 drivers/soundwire/slave.c

Comments

Takashi Iwai Oct. 19, 2017, 8:54 a.m. UTC | #1
On Thu, 19 Oct 2017 05:03:19 +0200,
Vinod Koul wrote:
> 
> +/**
> + * sdw_add_bus_master: add a bus Master instance
> + *
> + * @bus: bus instance
> + *
> + * Initializes the bus instance, read properties and create child
> + * devices.
> + */
> +int sdw_add_bus_master(struct sdw_bus *bus)
> +{
> +	int ret;
> +
> +	if (!bus->dev) {
> +		pr_err("SoundWire bus has no device");
> +		return -ENODEV;
> +	}
> +
> +	mutex_init(&bus->bus_lock);
> +	INIT_LIST_HEAD(&bus->slaves);
> +
> +	/*
> +	 * SDW is an enumerable bus, but devices can be powered off. So,
> +	 * they won't be able to report as present.
> +	 *
> +	 * Create Slave devices based on Slaves described in
> +	 * the respective firmware (ACPI/DT)
> +	 */
> +
> +	if (IS_ENABLED(CONFIG_ACPI) && bus->dev && ACPI_HANDLE(bus->dev))
> +		ret = sdw_acpi_find_slaves(bus);
> +	else if (IS_ENABLED(CONFIG_OF) && bus->dev && bus->dev->of_node)

The bus->dev NULL check is already done at the beginning of the
function, so here are superfluous.


> +static int sdw_delete_slave(struct device *dev, void *data)
> +{
> +	struct sdw_slave *slave = dev_to_sdw_dev(dev);
> +	struct sdw_bus *bus = slave->bus;
> +
> +	mutex_lock(&bus->bus_lock);
> +	if (!list_empty(&bus->slaves))
> +		list_del(&slave->node);

You can perform list_del_init() without empty check.

> +void sdw_extract_slave_id(struct sdw_bus *bus,
> +			unsigned long long addr, struct sdw_slave_id *id)

Use u64 instead.

> +{
> +	dev_dbg(bus->dev, "SDW Slave Addr: %llx", addr);
> +
> +	/*
> +	 * Spec definition
> +	 *   Register		Bit	Contents
> +	 *   DevId_0 [7:4]	47:44	sdw_version
> +	 *   DevId_0 [3:0]	43:40	unique_id
> +	 *   DevId_1		39:32	mfg_id [15:8]
> +	 *   DevId_2		31:24	mfg_id [7:0]
> +	 *   DevId_3		23:16	part_id [15:8]
> +	 *   DevId_4		15:08	part_id [7:0]
> +	 *   DevId_5		07:00	class_id
> +	 */
> +	id->sdw_version = (addr >> 44) & GENMASK(3, 0);
> +	id->unique_id = (addr >> 40) & GENMASK(3, 0);
> +	id->mfg_id = (addr >> 24) & GENMASK(15, 0);
> +	id->part_id = (addr >> 8) & GENMASK(15, 0);
> +	id->class_id = addr & GENMASK(7, 0);
> +
> +	dev_info(bus->dev,
> +		"SDW Slave class_id %x, part_id %x, mfg_id %x, unique_id %x, version %x",
> +				id->class_id, id->part_id, id->mfg_id,
> +				id->unique_id, id->sdw_version);
> +

Do we want to print a message always at each invocation?

> +static int sdw_slave_add(struct sdw_bus *bus,
> +		struct sdw_slave_id *id, struct fwnode_handle *fwnode)
> +{
> +	struct sdw_slave *slave;
> +	char name[32];
> +	int ret;
> +
> +	slave = kzalloc(sizeof(*slave), GFP_KERNEL);
> +	if (!slave)
> +		return -ENOMEM;
> +
> +	/* Initialize data structure */
> +	memcpy(&slave->id, id, sizeof(*id));
> +
> +	/* name shall be sdw:link:mfg:part:class:unique */
> +	snprintf(name, sizeof(name), "sdw:%x:%x:%x:%x:%x",
> +			bus->link_id, id->mfg_id, id->part_id,
> +			id->class_id, id->unique_id);

You can set the name directly via dev_set_name().  It's printf format,
after all.

> +	slave->dev.parent = bus->dev;
> +	slave->dev.fwnode = fwnode;
> +	dev_set_name(&slave->dev, "%s", name);
> +	slave->dev.release = sdw_slave_release;
> +	slave->dev.bus = &sdw_bus_type;
> +	slave->bus = bus;
> +	slave->status = SDW_SLAVE_UNATTACHED;
> +	slave->dev_num = 0;
> +
> +	mutex_lock(&bus->bus_lock);
> +	list_add_tail(&slave->node, &bus->slaves);
> +	mutex_unlock(&bus->bus_lock);
> +
> +	ret = device_register(&slave->dev);
> +	if (ret) {
> +		dev_err(bus->dev, "Failed to add slave: ret %d\n", ret);
> +
> +		/*
> +		 * On err, don't free but drop ref as this will be freed
> +		 * when release method is invoked.
> +		 */
> +		put_device(&slave->dev);

Wouldn't it leave a stale link to bus?


thanks,

Takashi
Vinod Koul Oct. 20, 2017, 5:19 a.m. UTC | #2
On Thu, Oct 19, 2017 at 10:54:50AM +0200, Takashi Iwai wrote:
> On Thu, 19 Oct 2017 05:03:19 +0200,
> Vinod Koul wrote:

> > +int sdw_add_bus_master(struct sdw_bus *bus)
> > +{
> > +	int ret;
> > +
> > +	if (!bus->dev) {
> > +		pr_err("SoundWire bus has no device");
> > +		return -ENODEV;
> > +	}
> > +
> > +	mutex_init(&bus->bus_lock);
> > +	INIT_LIST_HEAD(&bus->slaves);
> > +
> > +	/*
> > +	 * SDW is an enumerable bus, but devices can be powered off. So,
> > +	 * they won't be able to report as present.
> > +	 *
> > +	 * Create Slave devices based on Slaves described in
> > +	 * the respective firmware (ACPI/DT)
> > +	 */
> > +
> > +	if (IS_ENABLED(CONFIG_ACPI) && bus->dev && ACPI_HANDLE(bus->dev))
> > +		ret = sdw_acpi_find_slaves(bus);
> > +	else if (IS_ENABLED(CONFIG_OF) && bus->dev && bus->dev->of_node)
> 
> The bus->dev NULL check is already done at the beginning of the
> function, so here are superfluous.

right

> > +static int sdw_delete_slave(struct device *dev, void *data)
> > +{
> > +	struct sdw_slave *slave = dev_to_sdw_dev(dev);
> > +	struct sdw_bus *bus = slave->bus;
> > +
> > +	mutex_lock(&bus->bus_lock);
> > +	if (!list_empty(&bus->slaves))
> > +		list_del(&slave->node);
> 
> You can perform list_del_init() without empty check.

Better :)

> 
> > +void sdw_extract_slave_id(struct sdw_bus *bus,
> > +			unsigned long long addr, struct sdw_slave_id *id)
> 
> Use u64 instead.

okay

> > +{
> > +	dev_dbg(bus->dev, "SDW Slave Addr: %llx", addr);
> > +
> > +	/*
> > +	 * Spec definition
> > +	 *   Register		Bit	Contents
> > +	 *   DevId_0 [7:4]	47:44	sdw_version
> > +	 *   DevId_0 [3:0]	43:40	unique_id
> > +	 *   DevId_1		39:32	mfg_id [15:8]
> > +	 *   DevId_2		31:24	mfg_id [7:0]
> > +	 *   DevId_3		23:16	part_id [15:8]
> > +	 *   DevId_4		15:08	part_id [7:0]
> > +	 *   DevId_5		07:00	class_id
> > +	 */
> > +	id->sdw_version = (addr >> 44) & GENMASK(3, 0);
> > +	id->unique_id = (addr >> 40) & GENMASK(3, 0);
> > +	id->mfg_id = (addr >> 24) & GENMASK(15, 0);
> > +	id->part_id = (addr >> 8) & GENMASK(15, 0);
> > +	id->class_id = addr & GENMASK(7, 0);
> > +
> > +	dev_info(bus->dev,
> > +		"SDW Slave class_id %x, part_id %x, mfg_id %x, unique_id %x, version %x",
> > +				id->class_id, id->part_id, id->mfg_id,
> > +				id->unique_id, id->sdw_version);
> > +
> 
> Do we want to print a message always at each invocation?

Not really, lets make it debug

> > +static int sdw_slave_add(struct sdw_bus *bus,
> > +		struct sdw_slave_id *id, struct fwnode_handle *fwnode)
> > +{
> > +	struct sdw_slave *slave;
> > +	char name[32];
> > +	int ret;
> > +
> > +	slave = kzalloc(sizeof(*slave), GFP_KERNEL);
> > +	if (!slave)
> > +		return -ENOMEM;
> > +
> > +	/* Initialize data structure */
> > +	memcpy(&slave->id, id, sizeof(*id));
> > +
> > +	/* name shall be sdw:link:mfg:part:class:unique */
> > +	snprintf(name, sizeof(name), "sdw:%x:%x:%x:%x:%x",
> > +			bus->link_id, id->mfg_id, id->part_id,
> > +			id->class_id, id->unique_id);
> 
> You can set the name directly via dev_set_name().  It's printf format,
> after all.

right, am using it but with this string :D

> > +	slave->dev.parent = bus->dev;
> > +	slave->dev.fwnode = fwnode;
> > +	dev_set_name(&slave->dev, "%s", name);
> > +	slave->dev.release = sdw_slave_release;
> > +	slave->dev.bus = &sdw_bus_type;
> > +	slave->bus = bus;
> > +	slave->status = SDW_SLAVE_UNATTACHED;
> > +	slave->dev_num = 0;
> > +
> > +	mutex_lock(&bus->bus_lock);
> > +	list_add_tail(&slave->node, &bus->slaves);
> > +	mutex_unlock(&bus->bus_lock);
> > +
> > +	ret = device_register(&slave->dev);
> > +	if (ret) {
> > +		dev_err(bus->dev, "Failed to add slave: ret %d\n", ret);
> > +
> > +		/*
> > +		 * On err, don't free but drop ref as this will be freed
> > +		 * when release method is invoked.
> > +		 */
> > +		put_device(&slave->dev);
> 
> Wouldn't it leave a stale link to bus?

yes that needs to be removed too, thanks for pointing
Greg Kroah-Hartman Oct. 20, 2017, 10:47 a.m. UTC | #3
On Thu, Oct 19, 2017 at 08:33:19AM +0530, Vinod Koul wrote:
> +void sdw_extract_slave_id(struct sdw_bus *bus,
> +			unsigned long long addr, struct sdw_slave_id *id)
> +{
> +	dev_dbg(bus->dev, "SDW Slave Addr: %llx", addr);
> +
> +	/*
> +	 * Spec definition
> +	 *   Register		Bit	Contents
> +	 *   DevId_0 [7:4]	47:44	sdw_version
> +	 *   DevId_0 [3:0]	43:40	unique_id
> +	 *   DevId_1		39:32	mfg_id [15:8]
> +	 *   DevId_2		31:24	mfg_id [7:0]
> +	 *   DevId_3		23:16	part_id [15:8]
> +	 *   DevId_4		15:08	part_id [7:0]
> +	 *   DevId_5		07:00	class_id
> +	 */
> +	id->sdw_version = (addr >> 44) & GENMASK(3, 0);
> +	id->unique_id = (addr >> 40) & GENMASK(3, 0);
> +	id->mfg_id = (addr >> 24) & GENMASK(15, 0);
> +	id->part_id = (addr >> 8) & GENMASK(15, 0);
> +	id->class_id = addr & GENMASK(7, 0);
> +
> +	dev_info(bus->dev,
> +		"SDW Slave class_id %x, part_id %x, mfg_id %x, unique_id %x, version %x",
> +				id->class_id, id->part_id, id->mfg_id,
> +				id->unique_id, id->sdw_version);
> +

Why are you so noisy?  Shouldn't this be dev_dbg()?


> +static void sdw_slave_release(struct device *dev)
> +{
> +	struct sdw_slave *slave = dev_to_sdw_dev(dev);
> +
> +	kfree(slave);
> +}

Ah, finally a release function, nice.  But that's all you need to do for
it?

thanks,

greg k-h
Vinod Koul Oct. 20, 2017, 4:05 p.m. UTC | #4
On Fri, Oct 20, 2017 at 12:47:26PM +0200, Greg Kroah-Hartman wrote:
> On Thu, Oct 19, 2017 at 08:33:19AM +0530, Vinod Koul wrote:
> > +void sdw_extract_slave_id(struct sdw_bus *bus,
> > +			unsigned long long addr, struct sdw_slave_id *id)
> > +{
> > +	dev_dbg(bus->dev, "SDW Slave Addr: %llx", addr);
> > +
> > +	/*
> > +	 * Spec definition
> > +	 *   Register		Bit	Contents
> > +	 *   DevId_0 [7:4]	47:44	sdw_version
> > +	 *   DevId_0 [3:0]	43:40	unique_id
> > +	 *   DevId_1		39:32	mfg_id [15:8]
> > +	 *   DevId_2		31:24	mfg_id [7:0]
> > +	 *   DevId_3		23:16	part_id [15:8]
> > +	 *   DevId_4		15:08	part_id [7:0]
> > +	 *   DevId_5		07:00	class_id
> > +	 */
> > +	id->sdw_version = (addr >> 44) & GENMASK(3, 0);
> > +	id->unique_id = (addr >> 40) & GENMASK(3, 0);
> > +	id->mfg_id = (addr >> 24) & GENMASK(15, 0);
> > +	id->part_id = (addr >> 8) & GENMASK(15, 0);
> > +	id->class_id = addr & GENMASK(7, 0);
> > +
> > +	dev_info(bus->dev,
> > +		"SDW Slave class_id %x, part_id %x, mfg_id %x, unique_id %x, version %x",
> > +				id->class_id, id->part_id, id->mfg_id,
> > +				id->unique_id, id->sdw_version);
> > +
> 
> Why are you so noisy?  Shouldn't this be dev_dbg()?

Sure, Takashi had same comment so this is fixed.

> > +static void sdw_slave_release(struct device *dev)
> > +{
> > +	struct sdw_slave *slave = dev_to_sdw_dev(dev);
> > +
> > +	kfree(slave);
> > +}
> 
> Ah, finally a release function, nice.  But that's all you need to do for
> it?

The only pending thing is to release the memory. The bus reference and
related cleanup is done when device is unregistered.
Mark Brown Oct. 21, 2017, 9:12 a.m. UTC | #5
On Thu, Oct 19, 2017 at 08:33:19AM +0530, Vinod Koul wrote:

> +	/*
> +	 * SDW is an enumerable bus, but devices can be powered off. So,
> +	 * they won't be able to report as present.
> +	 *
> +	 * Create Slave devices based on Slaves described in
> +	 * the respective firmware (ACPI/DT)
> +	 */
> +
> +	if (IS_ENABLED(CONFIG_ACPI) && bus->dev && ACPI_HANDLE(bus->dev))
> +		ret = sdw_acpi_find_slaves(bus);
> +	else if (IS_ENABLED(CONFIG_OF) && bus->dev && bus->dev->of_node)
> +		ret = sdw_of_find_slaves(bus);
> +	else
> +		ret = -ENOTSUPP; /* No ACPI/DT so error out */

Devices *can* be powered off but is there any reason that a system
couldn't be designed with them always powered?  Also given that we don't
actually have any DT support the stubs for it are at best misleading,
it's not going to be hard for someone to add them later.

> +	mutex_lock(&bus->bus_lock);
> +	if (!list_empty(&bus->slaves))
> +		list_del(&slave->node);

Shouldn't that be a while?  Or at least warn if there's anything extra
there.  The code just looks very wrong as is.
Vinod Koul Oct. 21, 2017, 11:35 a.m. UTC | #6
On Sat, Oct 21, 2017 at 10:12:30AM +0100, Mark Brown wrote:
> On Thu, Oct 19, 2017 at 08:33:19AM +0530, Vinod Koul wrote:
> 
> > +	/*
> > +	 * SDW is an enumerable bus, but devices can be powered off. So,
> > +	 * they won't be able to report as present.
> > +	 *
> > +	 * Create Slave devices based on Slaves described in
> > +	 * the respective firmware (ACPI/DT)
> > +	 */
> > +
> > +	if (IS_ENABLED(CONFIG_ACPI) && bus->dev && ACPI_HANDLE(bus->dev))
> > +		ret = sdw_acpi_find_slaves(bus);
> > +	else if (IS_ENABLED(CONFIG_OF) && bus->dev && bus->dev->of_node)
> > +		ret = sdw_of_find_slaves(bus);
> > +	else
> > +		ret = -ENOTSUPP; /* No ACPI/DT so error out */
> 
> Devices *can* be powered off but is there any reason that a system
> couldn't be designed with them always powered?  Also given that we don't
> actually have any DT support the stubs for it are at best misleading,
> it's not going to be hard for someone to add them later.

Some people asked me to add it so it is clear how it would be supported on
DT based platforms. I would be too glad to remove if we have consensus on
that. Agree it is not too hard to add :)

> 
> > +	mutex_lock(&bus->bus_lock);
> > +	if (!list_empty(&bus->slaves))
> > +		list_del(&slave->node);
> 
> Shouldn't that be a while?  Or at least warn if there's anything extra
> there.  The code just looks very wrong as is.

I think you missed that it is called from sdw_delete_bus_master() which does
the loop by invoking device_for_each_child(), so this ones is supposed to
ensure one Slave is removed cleaned.

Let me know if i misread your comment.


Thanks
Mark Brown Oct. 23, 2017, 8:24 a.m. UTC | #7
On Sat, Oct 21, 2017 at 05:05:13PM +0530, Vinod Koul wrote:
> On Sat, Oct 21, 2017 at 10:12:30AM +0100, Mark Brown wrote:
> > On Thu, Oct 19, 2017 at 08:33:19AM +0530, Vinod Koul wrote:

> > > +	mutex_lock(&bus->bus_lock);
> > > +	if (!list_empty(&bus->slaves))
> > > +		list_del(&slave->node);

> > Shouldn't that be a while?  Or at least warn if there's anything extra
> > there.  The code just looks very wrong as is.

> I think you missed that it is called from sdw_delete_bus_master() which does
> the loop by invoking device_for_each_child(), so this ones is supposed to
> ensure one Slave is removed cleaned.

> Let me know if i misread your comment.

My point is that this code just looks so obviously wrong it doesn't
matter if it actually works, we should refactor so people don't look at
the code and immediately think they've spotted a bug.
Vinod Koul Oct. 23, 2017, 11:19 a.m. UTC | #8
On Mon, Oct 23, 2017 at 09:24:26AM +0100, Mark Brown wrote:
> On Sat, Oct 21, 2017 at 05:05:13PM +0530, Vinod Koul wrote:
> > On Sat, Oct 21, 2017 at 10:12:30AM +0100, Mark Brown wrote:
> > > On Thu, Oct 19, 2017 at 08:33:19AM +0530, Vinod Koul wrote:
> 
> > > > +	mutex_lock(&bus->bus_lock);
> > > > +	if (!list_empty(&bus->slaves))
> > > > +		list_del(&slave->node);
> 
> > > Shouldn't that be a while?  Or at least warn if there's anything extra
> > > there.  The code just looks very wrong as is.
> 
> > I think you missed that it is called from sdw_delete_bus_master() which does
> > the loop by invoking device_for_each_child(), so this ones is supposed to
> > ensure one Slave is removed cleaned.
> 
> > Let me know if i misread your comment.
> 
> My point is that this code just looks so obviously wrong it doesn't
> matter if it actually works, we should refactor so people don't look at
> the code and immediately think they've spotted a bug.

Okay will check and try to refactor this part.
Srinivas Kandagatla Nov. 9, 2017, 9:14 p.m. UTC | #9
On 19/10/17 04:03, Vinod Koul wrote:

> +/**
> + * sdw_add_bus_master: add a bus Master instance
> + *
> + * @bus: bus instance
> + *
> + * Initializes the bus instance, read properties and create child
> + * devices.
> + */

Some of the exported functions are missing kerneldocs.
Is it something you plan to add in next version of the patcheset?

> +int sdw_add_bus_master(struct sdw_bus *bus)
> +{
> +	int ret;
> +
> +	if (!bus->dev) {
> +		pr_err("SoundWire bus has no device");
> +		return -ENODEV;
> +	}
> +
> +	mutex_init(&bus->bus_lock);
> +	INIT_LIST_HEAD(&bus->slaves);
> +
> +	/*
> +	 * SDW is an enumerable bus, but devices can be powered off. So,
> +	 * they won't be able to report as present.
> +	 *
> +	 * Create Slave devices based on Slaves described in
> +	 * the respective firmware (ACPI/DT)
> +	 */
> +
> +	if (IS_ENABLED(CONFIG_ACPI) && bus->dev && ACPI_HANDLE(bus->dev))
> +		ret = sdw_acpi_find_slaves(bus);
> +	else if (IS_ENABLED(CONFIG_OF) && bus->dev && bus->dev->of_node)
> +		ret = sdw_of_find_slaves(bus);
> +	else
bus->dev is already checked in the start of the function, do we need to 
check once again ?

> +		ret = -ENOTSUPP; /* No ACPI/DT so error out */
> +
> +	if (ret) {
> +		dev_err(bus->dev, "Finding slaves failed:%d\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(sdw_add_bus_master);
Vinod Koul Nov. 10, 2017, 5:02 a.m. UTC | #10
On Thu, Nov 09, 2017 at 09:14:16PM +0000, Srinivas Kandagatla wrote:
> 
> 
> On 19/10/17 04:03, Vinod Koul wrote:
> 
> >+/**
> >+ * sdw_add_bus_master: add a bus Master instance
> >+ *
> >+ * @bus: bus instance
> >+ *
> >+ * Initializes the bus instance, read properties and create child
> >+ * devices.
> >+ */
> 
> Some of the exported functions are missing kerneldocs.
> Is it something you plan to add in next version of the patcheset?

I though most were, will double check to be sure.

> 
> >+int sdw_add_bus_master(struct sdw_bus *bus)
> >+{
> >+	int ret;
> >+
> >+	if (!bus->dev) {
> >+		pr_err("SoundWire bus has no device");
> >+		return -ENODEV;
> >+	}
> >+
> >+	mutex_init(&bus->bus_lock);
> >+	INIT_LIST_HEAD(&bus->slaves);
> >+
> >+	/*
> >+	 * SDW is an enumerable bus, but devices can be powered off. So,
> >+	 * they won't be able to report as present.
> >+	 *
> >+	 * Create Slave devices based on Slaves described in
> >+	 * the respective firmware (ACPI/DT)
> >+	 */
> >+
> >+	if (IS_ENABLED(CONFIG_ACPI) && bus->dev && ACPI_HANDLE(bus->dev))
> >+		ret = sdw_acpi_find_slaves(bus);
> >+	else if (IS_ENABLED(CONFIG_OF) && bus->dev && bus->dev->of_node)
> >+		ret = sdw_of_find_slaves(bus);
> >+	else
> bus->dev is already checked in the start of the function, do we need to
> check once again ?

yes already fixed, thanks
diff mbox

Patch

diff --git a/drivers/soundwire/Makefile b/drivers/soundwire/Makefile
index d1281def7662..c875e434f8b3 100644
--- a/drivers/soundwire/Makefile
+++ b/drivers/soundwire/Makefile
@@ -3,5 +3,5 @@ 
 #
 
 #Bus Objs
-soundwire-bus-objs := bus_type.o
+soundwire-bus-objs := bus_type.o bus.o slave.o
 obj-$(CONFIG_SOUNDWIRE_BUS) += soundwire-bus.o
diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
new file mode 100644
index 000000000000..57250f38a8d4
--- /dev/null
+++ b/drivers/soundwire/bus.c
@@ -0,0 +1,150 @@ 
+/*
+ *  This file is provided under a dual BSD/GPLv2 license.  When using or
+ *  redistributing this file, you may do so under either license.
+ *
+ *  GPL LICENSE SUMMARY
+ *
+ *  Copyright(c) 2015-17 Intel Corporation.
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of version 2 of the GNU General Public License as
+ *  published by the Free Software Foundation.
+ *
+ *  This program is distributed in the hope that it will be useful, but
+ *  WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  General Public License for more details.
+ *
+ *  BSD LICENSE
+ *
+ *  Copyright(c) 2015-17 Intel Corporation.
+ *
+ *  Redistribution and use in source and binary forms, with or without
+ *  modification, are permitted provided that the following conditions
+ *  are met:
+ *
+ *    * Redistributions of source code must retain the above copyright
+ *      notice, this list of conditions and the following disclaimer.
+ *    * Redistributions in binary form must reproduce the above copyright
+ *      notice, this list of conditions and the following disclaimer in
+ *      the documentation and/or other materials provided with the
+ *      distribution.
+ *    * Neither the name of Intel Corporation nor the names of its
+ *      contributors may be used to endorse or promote products derived
+ *      from this software without specific prior written permission.
+ *
+ *  THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ *  "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ *  LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ *  A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ *  OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ *  SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ *  LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ *  DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ *  THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ *  (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ *  OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ *
+ */
+
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/pm_runtime.h>
+#include <linux/soundwire/sdw.h>
+#include "bus.h"
+
+/**
+ * sdw_add_bus_master: add a bus Master instance
+ *
+ * @bus: bus instance
+ *
+ * Initializes the bus instance, read properties and create child
+ * devices.
+ */
+int sdw_add_bus_master(struct sdw_bus *bus)
+{
+	int ret;
+
+	if (!bus->dev) {
+		pr_err("SoundWire bus has no device");
+		return -ENODEV;
+	}
+
+	mutex_init(&bus->bus_lock);
+	INIT_LIST_HEAD(&bus->slaves);
+
+	/*
+	 * SDW is an enumerable bus, but devices can be powered off. So,
+	 * they won't be able to report as present.
+	 *
+	 * Create Slave devices based on Slaves described in
+	 * the respective firmware (ACPI/DT)
+	 */
+
+	if (IS_ENABLED(CONFIG_ACPI) && bus->dev && ACPI_HANDLE(bus->dev))
+		ret = sdw_acpi_find_slaves(bus);
+	else if (IS_ENABLED(CONFIG_OF) && bus->dev && bus->dev->of_node)
+		ret = sdw_of_find_slaves(bus);
+	else
+		ret = -ENOTSUPP; /* No ACPI/DT so error out */
+
+	if (ret) {
+		dev_err(bus->dev, "Finding slaves failed:%d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(sdw_add_bus_master);
+
+static int sdw_delete_slave(struct device *dev, void *data)
+{
+	struct sdw_slave *slave = dev_to_sdw_dev(dev);
+	struct sdw_bus *bus = slave->bus;
+
+	mutex_lock(&bus->bus_lock);
+	if (!list_empty(&bus->slaves))
+		list_del(&slave->node);
+	mutex_unlock(&bus->bus_lock);
+
+	device_unregister(dev);
+	return 0;
+}
+
+void sdw_delete_bus_master(struct sdw_bus *bus)
+{
+	device_for_each_child(bus->dev, NULL, sdw_delete_slave);
+}
+EXPORT_SYMBOL(sdw_delete_bus_master);
+
+void sdw_extract_slave_id(struct sdw_bus *bus,
+			unsigned long long addr, struct sdw_slave_id *id)
+{
+	dev_dbg(bus->dev, "SDW Slave Addr: %llx", addr);
+
+	/*
+	 * Spec definition
+	 *   Register		Bit	Contents
+	 *   DevId_0 [7:4]	47:44	sdw_version
+	 *   DevId_0 [3:0]	43:40	unique_id
+	 *   DevId_1		39:32	mfg_id [15:8]
+	 *   DevId_2		31:24	mfg_id [7:0]
+	 *   DevId_3		23:16	part_id [15:8]
+	 *   DevId_4		15:08	part_id [7:0]
+	 *   DevId_5		07:00	class_id
+	 */
+	id->sdw_version = (addr >> 44) & GENMASK(3, 0);
+	id->unique_id = (addr >> 40) & GENMASK(3, 0);
+	id->mfg_id = (addr >> 24) & GENMASK(15, 0);
+	id->part_id = (addr >> 8) & GENMASK(15, 0);
+	id->class_id = addr & GENMASK(7, 0);
+
+	dev_info(bus->dev,
+		"SDW Slave class_id %x, part_id %x, mfg_id %x, unique_id %x, version %x",
+				id->class_id, id->part_id, id->mfg_id,
+				id->unique_id, id->sdw_version);
+
+}
diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h
index 2683c6798b95..f61bc9f59445 100644
--- a/drivers/soundwire/bus.h
+++ b/drivers/soundwire/bus.h
@@ -57,6 +57,26 @@ 
 #include <linux/acpi.h>
 #include <linux/soundwire/sdw.h>
 
+#if IS_ENABLED(CONFIG_ACPI)
+int sdw_acpi_find_slaves(struct sdw_bus *bus);
+#else
+static inline int sdw_acpi_find_slaves(struct sdw_bus *bus)
+{
+	return -ENOTSUPP;
+}
+#endif
+
+#if IS_ENABLED(CONFIG_OF)
+int sdw_of_find_slaves(struct sdw_bus *bus);
+#else
+static inline int sdw_of_find_slaves(struct sdw_bus *bus)
+{
+	return -ENOTSUPP;
+}
+#endif
+
 int sdw_slave_modalias(struct sdw_slave *slave, char *buf, size_t size);
+void sdw_extract_slave_id(struct sdw_bus *bus,
+			unsigned long long addr, struct sdw_slave_id *id);
 
 #endif /* __SDW_BUS_H */
diff --git a/drivers/soundwire/slave.c b/drivers/soundwire/slave.c
new file mode 100644
index 000000000000..ee8e0e848f64
--- /dev/null
+++ b/drivers/soundwire/slave.c
@@ -0,0 +1,172 @@ 
+/*
+ *  This file is provided under a dual BSD/GPLv2 license.  When using or
+ *  redistributing this file, you may do so under either license.
+ *
+ *  GPL LICENSE SUMMARY
+ *
+ *  Copyright(c) 2015-17 Intel Corporation.
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of version 2 of the GNU General Public License as
+ *  published by the Free Software Foundation.
+ *
+ *  This program is distributed in the hope that it will be useful, but
+ *  WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  General Public License for more details.
+ *
+ *  BSD LICENSE
+ *
+ *  Copyright(c) 2015-17 Intel Corporation.
+ *
+ *  Redistribution and use in source and binary forms, with or without
+ *  modification, are permitted provided that the following conditions
+ *  are met:
+ *
+ *    * Redistributions of source code must retain the above copyright
+ *      notice, this list of conditions and the following disclaimer.
+ *    * Redistributions in binary form must reproduce the above copyright
+ *      notice, this list of conditions and the following disclaimer in
+ *      the documentation and/or other materials provided with the
+ *      distribution.
+ *    * Neither the name of Intel Corporation nor the names of its
+ *      contributors may be used to endorse or promote products derived
+ *      from this software without specific prior written permission.
+ *
+ *  THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ *  "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ *  LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ *  A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ *  OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ *  SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ *  LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ *  DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ *  THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ *  (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ *  OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ *
+ */
+
+#include <linux/acpi.h>
+#include <linux/device.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/soundwire/sdw.h>
+#include "bus.h"
+
+static void sdw_slave_release(struct device *dev)
+{
+	struct sdw_slave *slave = dev_to_sdw_dev(dev);
+
+	kfree(slave);
+}
+
+static int sdw_slave_add(struct sdw_bus *bus,
+		struct sdw_slave_id *id, struct fwnode_handle *fwnode)
+{
+	struct sdw_slave *slave;
+	char name[32];
+	int ret;
+
+	slave = kzalloc(sizeof(*slave), GFP_KERNEL);
+	if (!slave)
+		return -ENOMEM;
+
+	/* Initialize data structure */
+	memcpy(&slave->id, id, sizeof(*id));
+
+	/* name shall be sdw:link:mfg:part:class:unique */
+	snprintf(name, sizeof(name), "sdw:%x:%x:%x:%x:%x",
+			bus->link_id, id->mfg_id, id->part_id,
+			id->class_id, id->unique_id);
+
+	slave->dev.parent = bus->dev;
+	slave->dev.fwnode = fwnode;
+	dev_set_name(&slave->dev, "%s", name);
+	slave->dev.release = sdw_slave_release;
+	slave->dev.bus = &sdw_bus_type;
+	slave->bus = bus;
+	slave->status = SDW_SLAVE_UNATTACHED;
+	slave->dev_num = 0;
+
+	mutex_lock(&bus->bus_lock);
+	list_add_tail(&slave->node, &bus->slaves);
+	mutex_unlock(&bus->bus_lock);
+
+	ret = device_register(&slave->dev);
+	if (ret) {
+		dev_err(bus->dev, "Failed to add slave: ret %d\n", ret);
+
+		/*
+		 * On err, don't free but drop ref as this will be freed
+		 * when release method is invoked.
+		 */
+		put_device(&slave->dev);
+		return ret;
+	}
+
+	return 0;
+}
+
+#if IS_ENABLED(CONFIG_ACPI)
+/*
+ * sdw_acpi_find_slaves: Find Slave devices in Master ACPI node
+ *
+ * @bus: SDW bus instance
+ *
+ * Scans Master ACPI node for SDW child Slave devices and registers it.
+ */
+int sdw_acpi_find_slaves(struct sdw_bus *bus)
+{
+	struct acpi_device *adev, *parent;
+
+	parent = ACPI_COMPANION(bus->dev);
+	if (!parent) {
+		dev_err(bus->dev, "Can't find parent for acpi bind\n");
+		return -ENODEV;
+	}
+
+	list_for_each_entry(adev, &parent->children, node) {
+		unsigned long long addr;
+		struct sdw_slave_id id;
+		unsigned int link_id;
+		acpi_status status;
+
+		status = acpi_evaluate_integer(adev->handle,
+					METHOD_NAME__ADR, NULL, &addr);
+
+		if (ACPI_FAILURE(status)) {
+			dev_err(bus->dev, "_ADR resolution failed: %x\n",
+							status);
+			return status;
+		}
+
+		/* Extract link id from ADR, it is from 48 to 51 bits */
+		link_id = (addr >> 48) & GENMASK(3, 0);
+
+		/* Check for link_id match */
+		if (link_id != bus->link_id)
+			continue;
+
+		sdw_extract_slave_id(bus, addr, &id);
+
+		/*
+		 * don't error check for sdw_slave_add as we want to continue
+		 * adding Slaves
+		 */
+		sdw_slave_add(bus, &id, acpi_fwnode_handle(adev));
+	}
+
+	return 0;
+}
+
+#endif
+
+#if IS_ENABLED(CONFIG_OF)
+int sdw_of_find_slaves(struct sdw_bus *bus)
+{
+	/* placeholder now, fill on OF support */
+	return -ENOTSUPP;
+}
+#endif
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index 2b089463104d..deb5ba6c63c2 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -167,4 +167,7 @@  struct sdw_bus {
 	struct mutex bus_lock;
 };
 
+int sdw_add_bus_master(struct sdw_bus *bus);
+void sdw_delete_bus_master(struct sdw_bus *bus);
+
 #endif /* __SOUNDWIRE_H */