[PATCH 0/3] soundwire: use UniqueID only when relevant

[PATCH 0/3] soundwire: use UniqueID only when relevant

[Pierre-Louis Bossart]

The hardware UniqueID, typically enabled with pin-strapping, is
required during enumeration to avoid conflicts between devices of the
same type.

When there are no devices of the same type, using the UniqueID is
overkill and results in a lot of probe errors due to mismatches
between ACPI tables and hardware capabilities. For example it's not
uncommon for BIOS vendors to copy/paste the same settings between
platforms but the hardware pin-strapping is different. This is
perfectly legit and permitted by MIPI specs.

With this patchset, the UniqueID is only used when multiple devices of
the same type are detected. The loop to detect multiple identical
devices is not super efficient but with typically fewer than 4 devices
per link there's no real incentive to be smarter.

This change is only implemented for ACPI platforms, for DeviceTree
there is no change.

Pierre-Louis Bossart (3):
  soundwire: remove bitfield for unique_id, use u8
  soundwire: slave: add helper to extract slave ID
  soundwire: ignore uniqueID when irrelevant

 drivers/soundwire/bus.c       |  7 +--
 drivers/soundwire/slave.c     | 98 +++++++++++++++++++++++++++--------
 include/linux/soundwire/sdw.h |  4 +-
 3 files changed, 84 insertions(+), 25 deletions(-)

-- 
2.20.1

[PATCH 1/3] soundwire: remove bitfield for unique_id, use u8

[Pierre-Louis Bossart]

There is no good reason why the unique_id needs to be stored as 4
bits. The code will work without changes with a u8 since all values
are already filtered while parsing the ACPI tables and Slave devID
registers.

Use u8 representation. This will allow us to encode a
"IGNORE_UNIQUE_ID" value to account for firmware/BIOS creativity.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 include/linux/soundwire/sdw.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index 688b40e65c89..28745b9ba279 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -403,6 +403,8 @@ int sdw_slave_read_prop(struct sdw_slave *slave);
  * SDW Slave Structures and APIs
  */
 
+#define SDW_IGNORED_UNIQUE_ID 0xFF
+
 /**
  * struct sdw_slave_id - Slave ID
  * @mfg_id: MIPI Manufacturer ID
@@ -418,7 +420,7 @@ struct sdw_slave_id {
 	__u16 mfg_id;
 	__u16 part_id;
 	__u8 class_id;
-	__u8 unique_id:4;
+	__u8 unique_id;
 	__u8 sdw_version:4;
 };
 
-- 
2.20.1

[PATCH 2/3] soundwire: slave: add helper to extract slave ID

[Pierre-Louis Bossart]

Simplify the loop with a helper. The only functionality change is that
we continue the loop even with an ACPI error.

Follow-up patches will build on this change.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/soundwire/slave.c | 50 ++++++++++++++++++++++++---------------
 1 file changed, 31 insertions(+), 19 deletions(-)

diff --git a/drivers/soundwire/slave.c b/drivers/soundwire/slave.c
index 6473fa602f82..5dbc76772d21 100644
--- a/drivers/soundwire/slave.c
+++ b/drivers/soundwire/slave.c
@@ -64,6 +64,36 @@ static int sdw_slave_add(struct sdw_bus *bus,
 }
 
 #if IS_ENABLED(CONFIG_ACPI)
+
+static bool find_slave(struct sdw_bus *bus,
+		       struct acpi_device *adev,
+		       struct sdw_slave_id *id)
+{
+	unsigned long long addr;
+	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 false;
+	}
+
+	/* Extract link id from ADR, Bit 51 to 48 (included) */
+	link_id = (addr >> 48) & GENMASK(3, 0);
+
+	/* Check for link_id match */
+	if (link_id != bus->link_id)
+		return false;
+
+	sdw_extract_slave_id(bus, addr, id);
+
+	return true;
+}
+
 /*
  * sdw_acpi_find_slaves() - Find Slave devices in Master ACPI node
  * @bus: SDW bus instance
@@ -81,29 +111,11 @@ int sdw_acpi_find_slaves(struct sdw_bus *bus)
 	}
 
 	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, Bit 51 to 48 (included) */
-		link_id = (addr >> 48) & GENMASK(3, 0);
-
-		/* Check for link_id match */
-		if (link_id != bus->link_id)
+		if (!find_slave(bus, adev, &id))
 			continue;
 
-		sdw_extract_slave_id(bus, addr, &id);
-
 		/*
 		 * don't error check for sdw_slave_add as we want to continue
 		 * adding Slaves
-- 
2.20.1

[PATCH 3/3] soundwire: ignore uniqueID when irrelevant

[Pierre-Louis Bossart]

The uniqueID is useful when there are two or more devices of the same
type (identical manufacturer ID, part ID) on the same link.

When there is a single device of a given type on a link, its uniqueID
is irrelevant. It's not uncommon on actual platforms to see variations
of the uniqueID, or differences between devID registers and ACPI _ADR
fields.

This patch suggests a filter on startup to identify 'single' devices
and tag them accordingly. The uniqueID is then not used for the probe,
and the device name omits the uniqueID as well.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/soundwire/bus.c   |  7 +++---
 drivers/soundwire/slave.c | 52 ++++++++++++++++++++++++++++++++++++---
 2 files changed, 52 insertions(+), 7 deletions(-)

diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index fc53dbe57f85..be5d437058ed 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -422,10 +422,11 @@ static struct sdw_slave *sdw_get_slave(struct sdw_bus *bus, int i)
 
 static int sdw_compare_devid(struct sdw_slave *slave, struct sdw_slave_id id)
 {
-	if (slave->id.unique_id != id.unique_id ||
-	    slave->id.mfg_id != id.mfg_id ||
+	if (slave->id.mfg_id != id.mfg_id ||
 	    slave->id.part_id != id.part_id ||
-	    slave->id.class_id != id.class_id)
+	    slave->id.class_id != id.class_id ||
+	    (slave->id.unique_id != SDW_IGNORED_UNIQUE_ID &&
+	     slave->id.unique_id != id.unique_id))
 		return -ENODEV;
 
 	return 0;
diff --git a/drivers/soundwire/slave.c b/drivers/soundwire/slave.c
index 5dbc76772d21..19919975bb6d 100644
--- a/drivers/soundwire/slave.c
+++ b/drivers/soundwire/slave.c
@@ -29,10 +29,17 @@ static int sdw_slave_add(struct sdw_bus *bus,
 	slave->dev.parent = bus->dev;
 	slave->dev.fwnode = fwnode;
 
-	/* name shall be sdw:link:mfg:part:class:unique */
-	dev_set_name(&slave->dev, "sdw:%x:%x:%x:%x:%x",
-		     bus->link_id, id->mfg_id, id->part_id,
-		     id->class_id, id->unique_id);
+	if (id->unique_id == SDW_IGNORED_UNIQUE_ID) {
+		/* name shall be sdw:link:mfg:part:class */
+		dev_set_name(&slave->dev, "sdw:%x:%x:%x:%x",
+			     bus->link_id, id->mfg_id, id->part_id,
+			     id->class_id);
+	} else {
+		/* name shall be sdw:link:mfg:part:class:unique */
+		dev_set_name(&slave->dev, "sdw:%x:%x:%x:%x:%x",
+			     bus->link_id, id->mfg_id, id->part_id,
+			     id->class_id, id->unique_id);
+	}
 
 	slave->dev.release = sdw_slave_release;
 	slave->dev.bus = &sdw_bus_type;
@@ -103,6 +110,7 @@ static bool find_slave(struct sdw_bus *bus,
 int sdw_acpi_find_slaves(struct sdw_bus *bus)
 {
 	struct acpi_device *adev, *parent;
+	struct acpi_device *adev2, *parent2;
 
 	parent = ACPI_COMPANION(bus->dev);
 	if (!parent) {
@@ -112,10 +120,46 @@ int sdw_acpi_find_slaves(struct sdw_bus *bus)
 
 	list_for_each_entry(adev, &parent->children, node) {
 		struct sdw_slave_id id;
+		struct sdw_slave_id id2;
+		bool ignore_unique_id = true;
 
 		if (!find_slave(bus, adev, &id))
 			continue;
 
+		/* brute-force O(N^2) search for duplicates */
+		parent2 = parent;
+		list_for_each_entry(adev2, &parent2->children, node) {
+
+			if (adev == adev2)
+				continue;
+
+			if (!find_slave(bus, adev2, &id2))
+				continue;
+
+			if (id.sdw_version != id2.sdw_version ||
+			    id.mfg_id != id2.mfg_id ||
+			    id.part_id != id2.part_id ||
+			    id.class_id != id2.class_id)
+				continue;
+
+			if (id.unique_id != id2.unique_id) {
+				dev_dbg(bus->dev,
+					"Valid unique IDs %x %x for Slave mfg %x part %d\n",
+					id.unique_id, id2.unique_id,
+					id.mfg_id, id.part_id);
+				ignore_unique_id = false;
+			} else {
+				dev_err(bus->dev,
+					"Invalid unique IDs %x %x for Slave mfg %x part %d\n",
+					id.unique_id, id2.unique_id,
+					id.mfg_id, id.part_id);
+				return -ENODEV;
+			}
+		}
+
+		if (ignore_unique_id)
+			id.unique_id = SDW_IGNORED_UNIQUE_ID;
+
 		/*
 		 * don't error check for sdw_slave_add as we want to continue
 		 * adding Slaves
-- 
2.20.1

Re: [PATCH 1/3] soundwire: remove bitfield for unique_id, use u8

[Vinod Koul]

On 22-10-19, 18:48, Pierre-Louis Bossart wrote:
> There is no good reason why the unique_id needs to be stored as 4
> bits. The code will work without changes with a u8 since all values

Well this was due to the fact the slave id defined by MIPI has unique id
as 4 bits. In fact if you look closely there are other fields in
sdw_slave_id doing this

> are already filtered while parsing the ACPI tables and Slave devID
> registers.
> 
> Use u8 representation. This will allow us to encode a
> "IGNORE_UNIQUE_ID" value to account for firmware/BIOS creativity.

Why are we shoving firmware/BIOS issues into the core?

> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  include/linux/soundwire/sdw.h | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
> index 688b40e65c89..28745b9ba279 100644
> --- a/include/linux/soundwire/sdw.h
> +++ b/include/linux/soundwire/sdw.h
> @@ -403,6 +403,8 @@ int sdw_slave_read_prop(struct sdw_slave *slave);
>   * SDW Slave Structures and APIs
>   */
>  
> +#define SDW_IGNORED_UNIQUE_ID 0xFF
> +
>  /**
>   * struct sdw_slave_id - Slave ID
>   * @mfg_id: MIPI Manufacturer ID
> @@ -418,7 +420,7 @@ struct sdw_slave_id {
>  	__u16 mfg_id;
>  	__u16 part_id;
>  	__u8 class_id;
> -	__u8 unique_id:4;
> +	__u8 unique_id;
>  	__u8 sdw_version:4;
>  };
>  
> -- 
> 2.20.1

-- 
~Vinod

Re: [PATCH 3/3] soundwire: ignore uniqueID when irrelevant

[Vinod Koul]

On 22-10-19, 18:48, Pierre-Louis Bossart wrote:
> The uniqueID is useful when there are two or more devices of the same
> type (identical manufacturer ID, part ID) on the same link.

Right!

> When there is a single device of a given type on a link, its uniqueID
> is irrelevant. It's not uncommon on actual platforms to see variations
> of the uniqueID, or differences between devID registers and ACPI _ADR
> fields.

Ideally this should be fixed in firmware, I do not like the fact the we are
poking in core for firmware issues!

> This patch suggests a filter on startup to identify 'single' devices
> and tag them accordingly. 

So you try to see if the board has a single device and mark them so that
you can skip the unique id, did I get that right?

What about the boards which have multiple devices? How doing solve
these?

> The uniqueID is then not used for the probe,
> and the device name omits the uniqueID as well.
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  drivers/soundwire/bus.c   |  7 +++---
>  drivers/soundwire/slave.c | 52 ++++++++++++++++++++++++++++++++++++---
>  2 files changed, 52 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
> index fc53dbe57f85..be5d437058ed 100644
> --- a/drivers/soundwire/bus.c
> +++ b/drivers/soundwire/bus.c
> @@ -422,10 +422,11 @@ static struct sdw_slave *sdw_get_slave(struct sdw_bus *bus, int i)
>  
>  static int sdw_compare_devid(struct sdw_slave *slave, struct sdw_slave_id id)
>  {
> -	if (slave->id.unique_id != id.unique_id ||
> -	    slave->id.mfg_id != id.mfg_id ||
> +	if (slave->id.mfg_id != id.mfg_id ||
>  	    slave->id.part_id != id.part_id ||
> -	    slave->id.class_id != id.class_id)
> +	    slave->id.class_id != id.class_id ||
> +	    (slave->id.unique_id != SDW_IGNORED_UNIQUE_ID &&
> +	     slave->id.unique_id != id.unique_id))
>  		return -ENODEV;
>  
>  	return 0;
> diff --git a/drivers/soundwire/slave.c b/drivers/soundwire/slave.c
> index 5dbc76772d21..19919975bb6d 100644
> --- a/drivers/soundwire/slave.c
> +++ b/drivers/soundwire/slave.c
> @@ -29,10 +29,17 @@ static int sdw_slave_add(struct sdw_bus *bus,
>  	slave->dev.parent = bus->dev;
>  	slave->dev.fwnode = fwnode;
>  
> -	/* name shall be sdw:link:mfg:part:class:unique */
> -	dev_set_name(&slave->dev, "sdw:%x:%x:%x:%x:%x",
> -		     bus->link_id, id->mfg_id, id->part_id,
> -		     id->class_id, id->unique_id);
> +	if (id->unique_id == SDW_IGNORED_UNIQUE_ID) {
> +		/* name shall be sdw:link:mfg:part:class */
> +		dev_set_name(&slave->dev, "sdw:%x:%x:%x:%x",
> +			     bus->link_id, id->mfg_id, id->part_id,
> +			     id->class_id);
> +	} else {
> +		/* name shall be sdw:link:mfg:part:class:unique */
> +		dev_set_name(&slave->dev, "sdw:%x:%x:%x:%x:%x",
> +			     bus->link_id, id->mfg_id, id->part_id,
> +			     id->class_id, id->unique_id);
> +	}
>  
>  	slave->dev.release = sdw_slave_release;
>  	slave->dev.bus = &sdw_bus_type;
> @@ -103,6 +110,7 @@ static bool find_slave(struct sdw_bus *bus,
>  int sdw_acpi_find_slaves(struct sdw_bus *bus)
>  {
>  	struct acpi_device *adev, *parent;
> +	struct acpi_device *adev2, *parent2;
>  
>  	parent = ACPI_COMPANION(bus->dev);
>  	if (!parent) {
> @@ -112,10 +120,46 @@ int sdw_acpi_find_slaves(struct sdw_bus *bus)
>  
>  	list_for_each_entry(adev, &parent->children, node) {
>  		struct sdw_slave_id id;
> +		struct sdw_slave_id id2;
> +		bool ignore_unique_id = true;
>  
>  		if (!find_slave(bus, adev, &id))
>  			continue;
>  
> +		/* brute-force O(N^2) search for duplicates */
> +		parent2 = parent;
> +		list_for_each_entry(adev2, &parent2->children, node) {
> +
> +			if (adev == adev2)
> +				continue;
> +
> +			if (!find_slave(bus, adev2, &id2))
> +				continue;
> +
> +			if (id.sdw_version != id2.sdw_version ||
> +			    id.mfg_id != id2.mfg_id ||
> +			    id.part_id != id2.part_id ||
> +			    id.class_id != id2.class_id)
> +				continue;
> +
> +			if (id.unique_id != id2.unique_id) {
> +				dev_dbg(bus->dev,
> +					"Valid unique IDs %x %x for Slave mfg %x part %d\n",
> +					id.unique_id, id2.unique_id,
> +					id.mfg_id, id.part_id);
> +				ignore_unique_id = false;
> +			} else {
> +				dev_err(bus->dev,
> +					"Invalid unique IDs %x %x for Slave mfg %x part %d\n",
> +					id.unique_id, id2.unique_id,
> +					id.mfg_id, id.part_id);
> +				return -ENODEV;
> +			}
> +		}
> +
> +		if (ignore_unique_id)
> +			id.unique_id = SDW_IGNORED_UNIQUE_ID;
> +
>  		/*
>  		 * don't error check for sdw_slave_add as we want to continue
>  		 * adding Slaves
> -- 
> 2.20.1

-- 
~Vinod

Re: [alsa-devel] [PATCH 1/3] soundwire: remove bitfield for unique_id, use u8

[Pierre-Louis Bossart]

On 10/24/19 6:29 AM, Vinod Koul wrote:
> On 22-10-19, 18:48, Pierre-Louis Bossart wrote:
>> There is no good reason why the unique_id needs to be stored as 4
>> bits. The code will work without changes with a u8 since all values
> 
> Well this was due to the fact the slave id defined by MIPI has unique id
> as 4 bits. In fact if you look closely there are other fields in
> sdw_slave_id doing this

it's not because we extract 4 bits that we need to store the information 
in 4 bits.

> 
>> are already filtered while parsing the ACPI tables and Slave devID
>> registers.
>>
>> Use u8 representation. This will allow us to encode a
>> "IGNORE_UNIQUE_ID" value to account for firmware/BIOS creativity.
> 
> Why are we shoving firmware/BIOS issues into the core?

The core uses a matching formula which is too strict and does not work 
on multiple platforms.

You can argue that the BIOS should be fixed, but the counter argument is 
that the practice of ignoring the unique ID is allowed by the MIPI standard.

> 
>>
>> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>> ---
>>   include/linux/soundwire/sdw.h | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
>> index 688b40e65c89..28745b9ba279 100644
>> --- a/include/linux/soundwire/sdw.h
>> +++ b/include/linux/soundwire/sdw.h
>> @@ -403,6 +403,8 @@ int sdw_slave_read_prop(struct sdw_slave *slave);
>>    * SDW Slave Structures and APIs
>>    */
>>   
>> +#define SDW_IGNORED_UNIQUE_ID 0xFF
>> +
>>   /**
>>    * struct sdw_slave_id - Slave ID
>>    * @mfg_id: MIPI Manufacturer ID
>> @@ -418,7 +420,7 @@ struct sdw_slave_id {
>>   	__u16 mfg_id;
>>   	__u16 part_id;
>>   	__u8 class_id;
>> -	__u8 unique_id:4;
>> +	__u8 unique_id;
>>   	__u8 sdw_version:4;
>>   };
>>   
>> -- 
>> 2.20.1
> 

Re: [alsa-devel] [PATCH 3/3] soundwire: ignore uniqueID when irrelevant

[Pierre-Louis Bossart]

On 10/24/19 6:39 AM, Vinod Koul wrote:
> On 22-10-19, 18:48, Pierre-Louis Bossart wrote:
>> The uniqueID is useful when there are two or more devices of the same
>> type (identical manufacturer ID, part ID) on the same link.
> 
> Right!

the key part is "two or more". When it's "one" then the uniqueID has no 
defined meaning.

> 
>> When there is a single device of a given type on a link, its uniqueID
>> is irrelevant. It's not uncommon on actual platforms to see variations
>> of the uniqueID, or differences between devID registers and ACPI _ADR
>> fields.
> 
> Ideally this should be fixed in firmware, I do not like the fact the we are
> poking in core for firmware issues!

I will be the first to complain about BIOS issues, and the need for 
workarounds in the kernel, but here the BIOS vendors rely on permissions 
defined in the standard, see lines 3320..31 in the MIPI SoundWire 1.2 
document. You will see that there is no requirement to use the full set 
of devID registers to identify a Slave device. The only requirement is 
to read devID0 first to force a state machine transition and enable 
arbitration.

In other words, it's a nice case of BIOS folks telling the kernel folks 
we are too strict in our interpretation of the standard, and what they 
do is a feature and not a bug.

> 
>> This patch suggests a filter on startup to identify 'single' devices
>> and tag them accordingly.
> 
> So you try to see if the board has a single device and mark them so that
> you can skip the unique id, did I get that right?
> 
> What about the boards which have multiple devices? How doing solve
> these?

The uniqueID is used when multiple devices of the same type are detected 
in ACPI tables. No change, see [1] below

> 
>> The uniqueID is then not used for the probe,
>> and the device name omits the uniqueID as well.
>>
>> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>> ---
>>   drivers/soundwire/bus.c   |  7 +++---
>>   drivers/soundwire/slave.c | 52 ++++++++++++++++++++++++++++++++++++---
>>   2 files changed, 52 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
>> index fc53dbe57f85..be5d437058ed 100644
>> --- a/drivers/soundwire/bus.c
>> +++ b/drivers/soundwire/bus.c
>> @@ -422,10 +422,11 @@ static struct sdw_slave *sdw_get_slave(struct sdw_bus *bus, int i)
>>   
>>   static int sdw_compare_devid(struct sdw_slave *slave, struct sdw_slave_id id)
>>   {
>> -	if (slave->id.unique_id != id.unique_id ||
>> -	    slave->id.mfg_id != id.mfg_id ||
>> +	if (slave->id.mfg_id != id.mfg_id ||
>>   	    slave->id.part_id != id.part_id ||
>> -	    slave->id.class_id != id.class_id)
>> +	    slave->id.class_id != id.class_id ||
>> +	    (slave->id.unique_id != SDW_IGNORED_UNIQUE_ID &&
>> +	     slave->id.unique_id != id.unique_id))

[1] this is where the unique_id is ignored if it was tagged as 
irrelevant while scanning ACPI tables. If it is not ignored, then the 
same comparison applied

>>   		return -ENODEV;
>>   
>>   	return 0;
>> diff --git a/drivers/soundwire/slave.c b/drivers/soundwire/slave.c
>> index 5dbc76772d21..19919975bb6d 100644
>> --- a/drivers/soundwire/slave.c
>> +++ b/drivers/soundwire/slave.c
>> @@ -29,10 +29,17 @@ static int sdw_slave_add(struct sdw_bus *bus,
>>   	slave->dev.parent = bus->dev;
>>   	slave->dev.fwnode = fwnode;
>>   
>> -	/* name shall be sdw:link:mfg:part:class:unique */
>> -	dev_set_name(&slave->dev, "sdw:%x:%x:%x:%x:%x",
>> -		     bus->link_id, id->mfg_id, id->part_id,
>> -		     id->class_id, id->unique_id);
>> +	if (id->unique_id == SDW_IGNORED_UNIQUE_ID) {
>> +		/* name shall be sdw:link:mfg:part:class */
>> +		dev_set_name(&slave->dev, "sdw:%x:%x:%x:%x",
>> +			     bus->link_id, id->mfg_id, id->part_id,
>> +			     id->class_id);
>> +	} else {
>> +		/* name shall be sdw:link:mfg:part:class:unique */
>> +		dev_set_name(&slave->dev, "sdw:%x:%x:%x:%x:%x",
>> +			     bus->link_id, id->mfg_id, id->part_id,
>> +			     id->class_id, id->unique_id);
>> +	}
>>   
>>   	slave->dev.release = sdw_slave_release;
>>   	slave->dev.bus = &sdw_bus_type;
>> @@ -103,6 +110,7 @@ static bool find_slave(struct sdw_bus *bus,
>>   int sdw_acpi_find_slaves(struct sdw_bus *bus)
>>   {
>>   	struct acpi_device *adev, *parent;
>> +	struct acpi_device *adev2, *parent2;
>>   
>>   	parent = ACPI_COMPANION(bus->dev);
>>   	if (!parent) {
>> @@ -112,10 +120,46 @@ int sdw_acpi_find_slaves(struct sdw_bus *bus)
>>   
>>   	list_for_each_entry(adev, &parent->children, node) {
>>   		struct sdw_slave_id id;
>> +		struct sdw_slave_id id2;
>> +		bool ignore_unique_id = true;
>>   
>>   		if (!find_slave(bus, adev, &id))
>>   			continue;
>>   
>> +		/* brute-force O(N^2) search for duplicates */
>> +		parent2 = parent;
>> +		list_for_each_entry(adev2, &parent2->children, node) {
>> +
>> +			if (adev == adev2)
>> +				continue;
>> +
>> +			if (!find_slave(bus, adev2, &id2))
>> +				continue;
>> +
>> +			if (id.sdw_version != id2.sdw_version ||
>> +			    id.mfg_id != id2.mfg_id ||
>> +			    id.part_id != id2.part_id ||
>> +			    id.class_id != id2.class_id)
>> +				continue;
>> +
>> +			if (id.unique_id != id2.unique_id) {
>> +				dev_dbg(bus->dev,
>> +					"Valid unique IDs %x %x for Slave mfg %x part %d\n",
>> +					id.unique_id, id2.unique_id,
>> +					id.mfg_id, id.part_id);
>> +				ignore_unique_id = false;
>> +			} else {
>> +				dev_err(bus->dev,
>> +					"Invalid unique IDs %x %x for Slave mfg %x part %d\n",
>> +					id.unique_id, id2.unique_id,
>> +					id.mfg_id, id.part_id);
>> +				return -ENODEV;
>> +			}
>> +		}
>> +
>> +		if (ignore_unique_id)
>> +			id.unique_id = SDW_IGNORED_UNIQUE_ID;
>> +
>>   		/*
>>   		 * don't error check for sdw_slave_add as we want to continue
>>   		 * adding Slaves
>> -- 
>> 2.20.1
> 

Re: [alsa-devel] [PATCH 0/3] soundwire: use UniqueID only when relevant

[Pierre-Louis Bossart]

On 10/22/19 6:48 PM, Pierre-Louis Bossart wrote:
> The hardware UniqueID, typically enabled with pin-strapping, is
> required during enumeration to avoid conflicts between devices of the
> same type.
> 
> When there are no devices of the same type, using the UniqueID is
> overkill and results in a lot of probe errors due to mismatches
> between ACPI tables and hardware capabilities. For example it's not
> uncommon for BIOS vendors to copy/paste the same settings between
> platforms but the hardware pin-strapping is different. This is
> perfectly legit and permitted by MIPI specs.
> 
> With this patchset, the UniqueID is only used when multiple devices of
> the same type are detected. The loop to detect multiple identical
> devices is not super efficient but with typically fewer than 4 devices
> per link there's no real incentive to be smarter.
> 
> This change is only implemented for ACPI platforms, for DeviceTree
> there is no change.

Vinod, this series has been submitted for review on October 22 and I 
answered to your questions. There's been no feedback since October 24, 
so is there any sustained objection here?

ACPI platforms are completely unmanageable without this patchset.

> 
> Pierre-Louis Bossart (3):
>    soundwire: remove bitfield for unique_id, use u8
>    soundwire: slave: add helper to extract slave ID
>    soundwire: ignore uniqueID when irrelevant
> 
>   drivers/soundwire/bus.c       |  7 +--
>   drivers/soundwire/slave.c     | 98 +++++++++++++++++++++++++++--------
>   include/linux/soundwire/sdw.h |  4 +-
>   3 files changed, 84 insertions(+), 25 deletions(-)
> 

Re: [PATCH 0/3] soundwire: use UniqueID only when relevant

[Vinod Koul]

On 22-10-19, 18:48, Pierre-Louis Bossart wrote:
> The hardware UniqueID, typically enabled with pin-strapping, is
> required during enumeration to avoid conflicts between devices of the
> same type.
> 
> When there are no devices of the same type, using the UniqueID is
> overkill and results in a lot of probe errors due to mismatches
> between ACPI tables and hardware capabilities. For example it's not
> uncommon for BIOS vendors to copy/paste the same settings between
> platforms but the hardware pin-strapping is different. This is
> perfectly legit and permitted by MIPI specs.
> 
> With this patchset, the UniqueID is only used when multiple devices of
> the same type are detected. The loop to detect multiple identical
> devices is not super efficient but with typically fewer than 4 devices
> per link there's no real incentive to be smarter.
> 
> This change is only implemented for ACPI platforms, for DeviceTree
> there is no change.

I do not fully agree with the approach here but I do understand why this
was done and do not have a better alternative, so applied now

-- 
~Vinod