[v2,2/4] soundwire: core: add device tree support for slave devices
diff mbox series

Message ID 20190808144504.24823-3-srinivas.kandagatla@linaro.org
State New
Headers show
Series
  • ASoC: codecs: Add WSA881x Smart Speaker amplifier support
Related show

Commit Message

Srinivas Kandagatla Aug. 8, 2019, 2:45 p.m. UTC
This patch adds support to parsing device tree based
SoundWire slave devices.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/soundwire/bus.c   |  2 ++
 drivers/soundwire/bus.h   |  1 +
 drivers/soundwire/slave.c | 47 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 50 insertions(+)

Comments

Pierre-Louis Bossart Aug. 8, 2019, 3 p.m. UTC | #1
> @@ -35,6 +36,7 @@ static int sdw_slave_add(struct sdw_bus *bus,
>   
>   	slave->dev.release = sdw_slave_release;
>   	slave->dev.bus = &sdw_bus_type;
> +	slave->dev.of_node = of_node_get(to_of_node(fwnode));

shouldn't this protected by
#if IS_ENABLED(CONFIG_OF) ?

>   	slave->bus = bus;
>   	slave->status = SDW_SLAVE_UNATTACHED;
>   	slave->dev_num = 0;
> @@ -112,3 +114,48 @@ int sdw_acpi_find_slaves(struct sdw_bus *bus)
>   }
>   
>   #endif
> +
> +/*
> + * sdw_of_find_slaves() - Find Slave devices in master device tree node
> + * @bus: SDW bus instance
> + *
> + * Scans Master DT node for SDW child Slave devices and registers it.
> + */
> +int sdw_of_find_slaves(struct sdw_bus *bus)
> +{
> +	struct device *dev = bus->dev;
> +	struct device_node *node;
> +
> +	for_each_child_of_node(bus->dev->of_node, node) {
> +		struct sdw_slave_id id;
> +		const char *compat = NULL;
> +		int unique_id, ret;
> +		int ver, mfg_id, part_id, class_id;
> +
> +		compat = of_get_property(node, "compatible", NULL);
> +		if (!compat)
> +			continue;
> +
> +		ret = sscanf(compat, "sdw%x,%x,%x,%x",
> +			     &ver, &mfg_id, &part_id, &class_id);
> +		if (ret != 4) {
> +			dev_err(dev, "Manf ID & Product code not found %s\n",
> +				compat);
> +			continue;
> +		}
> +
> +		ret = of_property_read_u32(node, "sdw-instance-id", &unique_id);
> +		if (ret) {
> +			dev_err(dev, "Instance id not found:%d\n", ret);
> +			continue;

I am confused here.
If you have two identical devices on the same link, isn't this property 
required and that should be a real error instead of a continue?

> +		}
> +
> +		id.sdw_version = ver - 0xF;

maybe a comment in the code would help to make the encoding 
self-explanatory, as you did in the DT bindings

   Version number '0x10' represents SoundWire 1.0
   Version number '0x11' represents SoundWire 1.1

> +		id.unique_id = unique_id;
> +		id.mfg_id = mfg_id;
> +		id.part_id = part_id;
> +		id.class_id = class_id;
> +		sdw_slave_add(bus, &id, of_fwnode_handle(node));
> +	}
> +	return 0;
> +}
>
Srinivas Kandagatla Aug. 8, 2019, 3:17 p.m. UTC | #2
Thanks for taking time to review.

On 08/08/2019 16:00, Pierre-Louis Bossart wrote:
> 
>> @@ -35,6 +36,7 @@ static int sdw_slave_add(struct sdw_bus *bus,
>>       slave->dev.release = sdw_slave_release;
>>       slave->dev.bus = &sdw_bus_type;
>> +    slave->dev.of_node = of_node_get(to_of_node(fwnode));
> 
> shouldn't this protected by
> #if IS_ENABLED(CONFIG_OF) ?
> 
These macros and functions have dummy entries, so it should not be an issue.
I did build soundwire with i386_defconfig with no issues.

>>       slave->bus = bus;
>>       slave->status = SDW_SLAVE_UNATTACHED;
>>       slave->dev_num = 0;
>> @@ -112,3 +114,48 @@ int sdw_acpi_find_slaves(struct sdw_bus *bus)
>>   }
>>   #endif
>> +
>> +/*
>> + * sdw_of_find_slaves() - Find Slave devices in master device tree node
>> + * @bus: SDW bus instance
>> + *
>> + * Scans Master DT node for SDW child Slave devices and registers it.
>> + */
>> +int sdw_of_find_slaves(struct sdw_bus *bus)
>> +{
>> +    struct device *dev = bus->dev;
>> +    struct device_node *node;
>> +
>> +    for_each_child_of_node(bus->dev->of_node, node) {
>> +        struct sdw_slave_id id;
>> +        const char *compat = NULL;
>> +        int unique_id, ret;
>> +        int ver, mfg_id, part_id, class_id;
>> +
>> +        compat = of_get_property(node, "compatible", NULL);
>> +        if (!compat)
>> +            continue;
>> +
>> +        ret = sscanf(compat, "sdw%x,%x,%x,%x",
>> +                 &ver, &mfg_id, &part_id, &class_id);
>> +        if (ret != 4) {
>> +            dev_err(dev, "Manf ID & Product code not found %s\n",
>> +                compat);
>> +            continue;
>> +        }
>> +
>> +        ret = of_property_read_u32(node, "sdw-instance-id", &unique_id);
>> +        if (ret) {
>> +            dev_err(dev, "Instance id not found:%d\n", ret);
>> +            continue;
> 
> I am confused here.
> If you have two identical devices on the same link, isn't this property 
> required and that should be a real error instead of a continue?

Yes, I agree it will be mandatory in such cases.

Am okay either way, I dont mind changing it to returning EINVAL in all 
the cases.

> 
>> +        }
>> +
>> +        id.sdw_version = ver - 0xF;
> 
> maybe a comment in the code would help to make the encoding 
> self-explanatory, as you did in the DT bindings
> 
>    Version number '0x10' represents SoundWire 1.0
>    Version number '0x11' represents SoundWire 1.1

Makes sense, will fix this in next version.
This info is also available in bindings.


--srini
> 
>> +        id.unique_id = unique_id;
>> +        id.mfg_id = mfg_id;
>> +        id.part_id = part_id;
>> +        id.class_id = class_id;
>> +        sdw_slave_add(bus, &id, of_fwnode_handle(node));
>> +    }
>> +    return 0;
>> +}
>>
Vinod Koul Aug. 9, 2019, 5:07 a.m. UTC | #3
On 08-08-19, 15:45, Srinivas Kandagatla wrote:
> This patch adds support to parsing device tree based
> SoundWire slave devices.
> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
>  drivers/soundwire/bus.c   |  2 ++
>  drivers/soundwire/bus.h   |  1 +
>  drivers/soundwire/slave.c | 47 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 50 insertions(+)
> 
> diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
> index fe745830a261..324c54dc52fb 100644
> --- a/drivers/soundwire/bus.c
> +++ b/drivers/soundwire/bus.c
> @@ -77,6 +77,8 @@ int sdw_add_bus_master(struct sdw_bus *bus)
>  	 */
>  	if (IS_ENABLED(CONFIG_ACPI) && ACPI_HANDLE(bus->dev))
>  		ret = sdw_acpi_find_slaves(bus);
> +	else if (IS_ENABLED(CONFIG_OF) && bus->dev->of_node)
> +		ret = sdw_of_find_slaves(bus);
>  	else
>  		ret = -ENOTSUPP; /* No ACPI/DT so error out */
>  
> diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h
> index 3048ca153f22..ee46befedbd1 100644
> --- a/drivers/soundwire/bus.h
> +++ b/drivers/soundwire/bus.h
> @@ -15,6 +15,7 @@ static inline int sdw_acpi_find_slaves(struct sdw_bus *bus)
>  }
>  #endif
>  
> +int sdw_of_find_slaves(struct sdw_bus *bus);
>  void sdw_extract_slave_id(struct sdw_bus *bus,
>  			  u64 addr, struct sdw_slave_id *id);
>  
> diff --git a/drivers/soundwire/slave.c b/drivers/soundwire/slave.c
> index f39a5815e25d..8ab76f5d5a56 100644
> --- a/drivers/soundwire/slave.c
> +++ b/drivers/soundwire/slave.c
> @@ -2,6 +2,7 @@
>  // Copyright(c) 2015-17 Intel Corporation.
>  
>  #include <linux/acpi.h>
> +#include <linux/of.h>
>  #include <linux/soundwire/sdw.h>
>  #include <linux/soundwire/sdw_type.h>
>  #include "bus.h"
> @@ -35,6 +36,7 @@ static int sdw_slave_add(struct sdw_bus *bus,
>  
>  	slave->dev.release = sdw_slave_release;
>  	slave->dev.bus = &sdw_bus_type;
> +	slave->dev.of_node = of_node_get(to_of_node(fwnode));
>  	slave->bus = bus;
>  	slave->status = SDW_SLAVE_UNATTACHED;
>  	slave->dev_num = 0;
> @@ -112,3 +114,48 @@ int sdw_acpi_find_slaves(struct sdw_bus *bus)
>  }
>  
>  #endif
> +
> +/*
> + * sdw_of_find_slaves() - Find Slave devices in master device tree node
> + * @bus: SDW bus instance
> + *
> + * Scans Master DT node for SDW child Slave devices and registers it.
> + */
> +int sdw_of_find_slaves(struct sdw_bus *bus)
> +{
> +	struct device *dev = bus->dev;
> +	struct device_node *node;
> +
> +	for_each_child_of_node(bus->dev->of_node, node) {
> +		struct sdw_slave_id id;
> +		const char *compat = NULL;
> +		int unique_id, ret;
> +		int ver, mfg_id, part_id, class_id;
> +
> +		compat = of_get_property(node, "compatible", NULL);
> +		if (!compat)
> +			continue;

Why not use of_find_compatible_node() that will return the node which is
sdw* and we dont need to checks on that..

> +
> +		ret = sscanf(compat, "sdw%x,%x,%x,%x",
> +			     &ver, &mfg_id, &part_id, &class_id);
> +		if (ret != 4) {
> +			dev_err(dev, "Manf ID & Product code not found %s\n",
> +				compat);
> +			continue;
> +		}
> +
> +		ret = of_property_read_u32(node, "sdw-instance-id", &unique_id);
> +		if (ret) {
> +			dev_err(dev, "Instance id not found:%d\n", ret);
> +			continue;
> +		}
> +
> +		id.sdw_version = ver - 0xF;
> +		id.unique_id = unique_id;
> +		id.mfg_id = mfg_id;
> +		id.part_id = part_id;
> +		id.class_id = class_id;

empty line here please

> +		sdw_slave_add(bus, &id, of_fwnode_handle(node));
> +	}

and here as well

> +	return 0;
> +}
> -- 
> 2.21.0
Vinod Koul Aug. 9, 2019, 5:46 a.m. UTC | #4
On 08-08-19, 16:17, Srinivas Kandagatla wrote:
> Thanks for taking time to review.
> 
> On 08/08/2019 16:00, Pierre-Louis Bossart wrote:
> > 
> > > @@ -35,6 +36,7 @@ static int sdw_slave_add(struct sdw_bus *bus,
> > >       slave->dev.release = sdw_slave_release;
> > >       slave->dev.bus = &sdw_bus_type;
> > > +    slave->dev.of_node = of_node_get(to_of_node(fwnode));
> > 
> > shouldn't this protected by
> > #if IS_ENABLED(CONFIG_OF) ?
> > 
> These macros and functions have dummy entries, so it should not be an issue.
> I did build soundwire with i386_defconfig with no issues.

That means this function was compiled without errors, that is not strange nowadays
given the ARM compiles ACPI and x86 OF, so check with OF being disable
just to be safe :) I think dummy entries are helping

> 
> > >       slave->bus = bus;
> > >       slave->status = SDW_SLAVE_UNATTACHED;
> > >       slave->dev_num = 0;
> > > @@ -112,3 +114,48 @@ int sdw_acpi_find_slaves(struct sdw_bus *bus)
> > >   }
> > >   #endif
> > > +
> > > +/*
> > > + * sdw_of_find_slaves() - Find Slave devices in master device tree node
> > > + * @bus: SDW bus instance
> > > + *
> > > + * Scans Master DT node for SDW child Slave devices and registers it.
> > > + */
> > > +int sdw_of_find_slaves(struct sdw_bus *bus)
> > > +{
> > > +    struct device *dev = bus->dev;
> > > +    struct device_node *node;
> > > +
> > > +    for_each_child_of_node(bus->dev->of_node, node) {
> > > +        struct sdw_slave_id id;
> > > +        const char *compat = NULL;
> > > +        int unique_id, ret;
> > > +        int ver, mfg_id, part_id, class_id;
> > > +
> > > +        compat = of_get_property(node, "compatible", NULL);
> > > +        if (!compat)
> > > +            continue;
> > > +
> > > +        ret = sscanf(compat, "sdw%x,%x,%x,%x",
> > > +                 &ver, &mfg_id, &part_id, &class_id);
> > > +        if (ret != 4) {
> > > +            dev_err(dev, "Manf ID & Product code not found %s\n",
> > > +                compat);
> > > +            continue;
> > > +        }
> > > +
> > > +        ret = of_property_read_u32(node, "sdw-instance-id", &unique_id);
> > > +        if (ret) {
> > > +            dev_err(dev, "Instance id not found:%d\n", ret);
> > > +            continue;
> > 
> > I am confused here.
> > If you have two identical devices on the same link, isn't this property
> > required and that should be a real error instead of a continue?
> 
> Yes, I agree it will be mandatory in such cases.
> 
> Am okay either way, I dont mind changing it to returning EINVAL in all the
> cases.

Do we want to abort? We are in loop scanning for devices so makes sense
if we do not do that and continue to check next one..
Srinivas Kandagatla Aug. 9, 2019, 8:24 a.m. UTC | #5
On 09/08/2019 06:46, Vinod Koul wrote:
>>>> +int sdw_of_find_slaves(struct sdw_bus *bus)
>>>> +{
>>>> +    struct device *dev = bus->dev;
>>>> +    struct device_node *node;
>>>> +
>>>> +    for_each_child_of_node(bus->dev->of_node, node) {
>>>> +        struct sdw_slave_id id;
>>>> +        const char *compat = NULL;
>>>> +        int unique_id, ret;
>>>> +        int ver, mfg_id, part_id, class_id;
>>>> +
>>>> +        compat = of_get_property(node, "compatible", NULL);
>>>> +        if (!compat)
>>>> +            continue;
>>>> +
>>>> +        ret = sscanf(compat, "sdw%x,%x,%x,%x",
>>>> +                 &ver, &mfg_id, &part_id, &class_id);
>>>> +        if (ret != 4) {
>>>> +            dev_err(dev, "Manf ID & Product code not found %s\n",
>>>> +                compat);
>>>> +            continue;
>>>> +        }
>>>> +
>>>> +        ret = of_property_read_u32(node, "sdw-instance-id", &unique_id);
>>>> +        if (ret) {
>>>> +            dev_err(dev, "Instance id not found:%d\n", ret);
>>>> +            continue;
>>> I am confused here.
>>> If you have two identical devices on the same link, isn't this property
>>> required and that should be a real error instead of a continue?
>> Yes, I agree it will be mandatory in such cases.
>>
>> Am okay either way, I dont mind changing it to returning EINVAL in all the
>> cases.
> Do we want to abort? We are in loop scanning for devices so makes sense
> if we do not do that and continue to check next one..

That was my inital plan.
Pierre suggested a better compatible to include instance ID and LinkID 
so this check would be part of the check one before this line.

--srini
Srinivas Kandagatla Aug. 9, 2019, 8:24 a.m. UTC | #6
On 09/08/2019 06:07, Vinod Koul wrote:
> On 08-08-19, 15:45, Srinivas Kandagatla wrote:
>> This patch adds support to parsing device tree based
>> SoundWire slave devices.
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
>>   drivers/soundwire/bus.c   |  2 ++
>>   drivers/soundwire/bus.h   |  1 +
>>   drivers/soundwire/slave.c | 47 +++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 50 insertions(+)
>>
>> diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
>> index fe745830a261..324c54dc52fb 100644
>> --- a/drivers/soundwire/bus.c
>> +++ b/drivers/soundwire/bus.c
>> @@ -77,6 +77,8 @@ int sdw_add_bus_master(struct sdw_bus *bus)
>>   	 */
>>   	if (IS_ENABLED(CONFIG_ACPI) && ACPI_HANDLE(bus->dev))
>>   		ret = sdw_acpi_find_slaves(bus);
>> +	else if (IS_ENABLED(CONFIG_OF) && bus->dev->of_node)
>> +		ret = sdw_of_find_slaves(bus);
>>   	else
>>   		ret = -ENOTSUPP; /* No ACPI/DT so error out */
>>   
>> diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h
>> index 3048ca153f22..ee46befedbd1 100644
>> --- a/drivers/soundwire/bus.h
>> +++ b/drivers/soundwire/bus.h
>> @@ -15,6 +15,7 @@ static inline int sdw_acpi_find_slaves(struct sdw_bus *bus)
>>   }
>>   #endif
>>   
>> +int sdw_of_find_slaves(struct sdw_bus *bus);
>>   void sdw_extract_slave_id(struct sdw_bus *bus,
>>   			  u64 addr, struct sdw_slave_id *id);
>>   
>> diff --git a/drivers/soundwire/slave.c b/drivers/soundwire/slave.c
>> index f39a5815e25d..8ab76f5d5a56 100644
>> --- a/drivers/soundwire/slave.c
>> +++ b/drivers/soundwire/slave.c
>> @@ -2,6 +2,7 @@
>>   // Copyright(c) 2015-17 Intel Corporation.
>>   
>>   #include <linux/acpi.h>
>> +#include <linux/of.h>
>>   #include <linux/soundwire/sdw.h>
>>   #include <linux/soundwire/sdw_type.h>
>>   #include "bus.h"
>> @@ -35,6 +36,7 @@ static int sdw_slave_add(struct sdw_bus *bus,
>>   
>>   	slave->dev.release = sdw_slave_release;
>>   	slave->dev.bus = &sdw_bus_type;
>> +	slave->dev.of_node = of_node_get(to_of_node(fwnode));
>>   	slave->bus = bus;
>>   	slave->status = SDW_SLAVE_UNATTACHED;
>>   	slave->dev_num = 0;
>> @@ -112,3 +114,48 @@ int sdw_acpi_find_slaves(struct sdw_bus *bus)
>>   }
>>   
>>   #endif
>> +
>> +/*
>> + * sdw_of_find_slaves() - Find Slave devices in master device tree node
>> + * @bus: SDW bus instance
>> + *
>> + * Scans Master DT node for SDW child Slave devices and registers it.
>> + */
>> +int sdw_of_find_slaves(struct sdw_bus *bus)
>> +{
>> +	struct device *dev = bus->dev;
>> +	struct device_node *node;
>> +
>> +	for_each_child_of_node(bus->dev->of_node, node) {
>> +		struct sdw_slave_id id;
>> +		const char *compat = NULL;
>> +		int unique_id, ret;
>> +		int ver, mfg_id, part_id, class_id;
>> +
>> +		compat = of_get_property(node, "compatible", NULL);
>> +		if (!compat)
>> +			continue;
> 
> Why not use of_find_compatible_node() that will return the node which is
> sdw* and we dont need to checks on that..

Am not sure if wild characters are really supported in this api.
Also AFIU, it would not very optimal to use this api and it would 
complicate the code without much gain.


> 
>> +
>> +		ret = sscanf(compat, "sdw%x,%x,%x,%x",
>> +			     &ver, &mfg_id, &part_id, &class_id);
>> +		if (ret != 4) {
>> +			dev_err(dev, "Manf ID & Product code not found %s\n",
>> +				compat);
>> +			continue;
>> +		}
>> +
>> +		ret = of_property_read_u32(node, "sdw-instance-id", &unique_id);
>> +		if (ret) {
>> +			dev_err(dev, "Instance id not found:%d\n", ret);
>> +			continue;
>> +		}
>> +
>> +		id.sdw_version = ver - 0xF;
>> +		id.unique_id = unique_id;
>> +		id.mfg_id = mfg_id;
>> +		id.part_id = part_id;
>> +		id.class_id = class_id;
> 
> empty line here please
yep, will add that.

thanks,
srini
> 
>> +		sdw_slave_add(bus, &id, of_fwnode_handle(node));
>> +	}
> 
> and here as well
> 
>> +	return 0;
>> +}
>> -- 
>> 2.21.0
>

Patch
diff mbox series

diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index fe745830a261..324c54dc52fb 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -77,6 +77,8 @@  int sdw_add_bus_master(struct sdw_bus *bus)
 	 */
 	if (IS_ENABLED(CONFIG_ACPI) && ACPI_HANDLE(bus->dev))
 		ret = sdw_acpi_find_slaves(bus);
+	else if (IS_ENABLED(CONFIG_OF) && bus->dev->of_node)
+		ret = sdw_of_find_slaves(bus);
 	else
 		ret = -ENOTSUPP; /* No ACPI/DT so error out */
 
diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h
index 3048ca153f22..ee46befedbd1 100644
--- a/drivers/soundwire/bus.h
+++ b/drivers/soundwire/bus.h
@@ -15,6 +15,7 @@  static inline int sdw_acpi_find_slaves(struct sdw_bus *bus)
 }
 #endif
 
+int sdw_of_find_slaves(struct sdw_bus *bus);
 void sdw_extract_slave_id(struct sdw_bus *bus,
 			  u64 addr, struct sdw_slave_id *id);
 
diff --git a/drivers/soundwire/slave.c b/drivers/soundwire/slave.c
index f39a5815e25d..8ab76f5d5a56 100644
--- a/drivers/soundwire/slave.c
+++ b/drivers/soundwire/slave.c
@@ -2,6 +2,7 @@ 
 // Copyright(c) 2015-17 Intel Corporation.
 
 #include <linux/acpi.h>
+#include <linux/of.h>
 #include <linux/soundwire/sdw.h>
 #include <linux/soundwire/sdw_type.h>
 #include "bus.h"
@@ -35,6 +36,7 @@  static int sdw_slave_add(struct sdw_bus *bus,
 
 	slave->dev.release = sdw_slave_release;
 	slave->dev.bus = &sdw_bus_type;
+	slave->dev.of_node = of_node_get(to_of_node(fwnode));
 	slave->bus = bus;
 	slave->status = SDW_SLAVE_UNATTACHED;
 	slave->dev_num = 0;
@@ -112,3 +114,48 @@  int sdw_acpi_find_slaves(struct sdw_bus *bus)
 }
 
 #endif
+
+/*
+ * sdw_of_find_slaves() - Find Slave devices in master device tree node
+ * @bus: SDW bus instance
+ *
+ * Scans Master DT node for SDW child Slave devices and registers it.
+ */
+int sdw_of_find_slaves(struct sdw_bus *bus)
+{
+	struct device *dev = bus->dev;
+	struct device_node *node;
+
+	for_each_child_of_node(bus->dev->of_node, node) {
+		struct sdw_slave_id id;
+		const char *compat = NULL;
+		int unique_id, ret;
+		int ver, mfg_id, part_id, class_id;
+
+		compat = of_get_property(node, "compatible", NULL);
+		if (!compat)
+			continue;
+
+		ret = sscanf(compat, "sdw%x,%x,%x,%x",
+			     &ver, &mfg_id, &part_id, &class_id);
+		if (ret != 4) {
+			dev_err(dev, "Manf ID & Product code not found %s\n",
+				compat);
+			continue;
+		}
+
+		ret = of_property_read_u32(node, "sdw-instance-id", &unique_id);
+		if (ret) {
+			dev_err(dev, "Instance id not found:%d\n", ret);
+			continue;
+		}
+
+		id.sdw_version = ver - 0xF;
+		id.unique_id = unique_id;
+		id.mfg_id = mfg_id;
+		id.part_id = part_id;
+		id.class_id = class_id;
+		sdw_slave_add(bus, &id, of_fwnode_handle(node));
+	}
+	return 0;
+}