[PATCH 0/8] soundwire: corrections to ACPI and DisCo properties

[PATCH 0/8] soundwire: corrections to ACPI and DisCo properties

[Pierre-Louis Bossart]

Now that we are done with cleanups, we can start fixing the code with
actual semantic or functional changes.

This patchset applies on top of everything Vinod and I contributed
this week.

The fist patch correct a simplifying assumption made earlier. With the
IceLake BIOS the existing code is fooled by the presence of a second
child device and the namespace walk needs to be filtered. This was not
needed on previous generations.

The second patch fixes a long-standing misalignment between code and
DisCo specification, preventing MIPI DisCo properties from being
parsed successfully.

The third and fourth patch remove restrictions preventing codec
drivers from reading DisCo properties.

The fifth patch adds definitions from the SoundWire spec that were
missed somehow, but will be very much needed for dynamic bandwidth
allocation.

The last 3 patches realign the code with the MIPI specification. The
existing code exposes properties that don't exist, or exposes them
with ambiguous wording. Sticking to the specification helps avoid
interpretation issues for integrators, and will make sure the
follow-up sysfs support is self-explanatory.

Parts of this code was initially written by my Intel colleagues Vinod
Koul, Sanyog Kale, Shreyas Nc and Hardik Shah, who are either no
longer with Intel or no longer involved in SoundWire development. When
relevant, I explictly added a note in commit messages to give them
credit for their hard work, but I removed their signed-off-by tags to
avoid email bounces and avoid spamming them forever with SoundWire
patches.

Pierre-Louis Bossart (8):
  soundwire: intel: filter SoundWire controller device search
  soundwire: mipi_disco: fix master/link error
  soundwire: mipi_disco: expose sdw_slave_read_dpn as symbol
  soundwire: mipi_disco: expose sdw_slave_read_dp0 as symbol
  soundwire: add port-related definitions
  soundwire: remove master data port properties
  soundwire: fix master properties
  soundwire: rename/clarify MIPI DisCo properties

 drivers/soundwire/bus.c        |  6 +--
 drivers/soundwire/intel.c      | 11 ++--
 drivers/soundwire/intel_init.c | 19 ++++++-
 drivers/soundwire/mipi_disco.c | 49 +++++++++---------
 drivers/soundwire/stream.c     |  8 +--
 include/linux/soundwire/sdw.h  | 94 +++++++++++++++++++++++++++-------
 6 files changed, 132 insertions(+), 55 deletions(-)

-- 
2.17.1

[PATCH 1/8] soundwire: intel: filter SoundWire controller device search

[Pierre-Louis Bossart]

The convention is that the SoundWire controller device is a child of
the HDAudio controller. However there can be more than one child
exposed in the DSDT table, and the current namespace walk returns the
last device.

Add a filter and terminate early when a valid _ADR is provided,
otherwise keep iterating to find the next child.

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

diff --git a/drivers/soundwire/intel_init.c b/drivers/soundwire/intel_init.c
index d3d6b54c5791..f85db67d05f0 100644
--- a/drivers/soundwire/intel_init.c
+++ b/drivers/soundwire/intel_init.c
@@ -150,6 +150,12 @@ static acpi_status sdw_intel_acpi_cb(acpi_handle handle, u32 level,
 {
 	struct sdw_intel_res *res = cdata;
 	struct acpi_device *adev;
+	acpi_status status;
+	u64 adr;
+
+	status = acpi_evaluate_integer(handle, METHOD_NAME__ADR, NULL, &adr);
+	if (ACPI_FAILURE(status))
+		return AE_OK; /* keep going */
 
 	if (acpi_bus_get_device(handle, &adev)) {
 		pr_err("%s: Couldn't find ACPI handle\n", __func__);
@@ -157,7 +163,18 @@ static acpi_status sdw_intel_acpi_cb(acpi_handle handle, u32 level,
 	}
 
 	res->handle = handle;
-	return AE_OK;
+
+	/*
+	 * On some Intel platforms, multiple children of the HDAS
+	 * device can be found, but only one of them is the SoundWire
+	 * controller. The SNDW device is always exposed with
+	 * Name(_ADR, 0x40000000) so filter accordingly
+	 */
+	if (adr != 0x40000000)
+		return AE_OK; /* keep going */
+
+	/* device found, stop namespace walk */
+	return AE_CTRL_TERMINATE;
 }
 
 /**
-- 
2.17.1

[PATCH 2/8] soundwire: mipi_disco: fix master/link error

[Pierre-Louis Bossart]

The MIPI DisCo specification for SoundWire defines the
"mipi-sdw-link-N-subproperties", not the master-N properties. Fix to
parse firmware information.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/soundwire/mipi_disco.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/soundwire/mipi_disco.c b/drivers/soundwire/mipi_disco.c
index c1f51d6a23d2..6df68584c963 100644
--- a/drivers/soundwire/mipi_disco.c
+++ b/drivers/soundwire/mipi_disco.c
@@ -40,7 +40,7 @@ int sdw_master_read_prop(struct sdw_bus *bus)
 
 	/* Find master handle */
 	snprintf(name, sizeof(name),
-		 "mipi-sdw-master-%d-subproperties", bus->link_id);
+		 "mipi-sdw-link-%d-subproperties", bus->link_id);
 
 	link = device_get_named_child_node(bus->dev, name);
 	if (!link) {
-- 
2.17.1

[PATCH 3/8] soundwire: mipi_disco: expose sdw_slave_read_dpn as symbol

[Pierre-Louis Bossart]

sdw_slave_read_dpn was so far a static function, which prevented
codec drivers from using it. Expose as non-static function and add symbol

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

diff --git a/drivers/soundwire/mipi_disco.c b/drivers/soundwire/mipi_disco.c
index 6df68584c963..4995554bd6b6 100644
--- a/drivers/soundwire/mipi_disco.c
+++ b/drivers/soundwire/mipi_disco.c
@@ -161,9 +161,9 @@ static int sdw_slave_read_dp0(struct sdw_slave *slave,
 	return 0;
 }
 
-static int sdw_slave_read_dpn(struct sdw_slave *slave,
-			      struct sdw_dpn_prop *dpn, int count, int ports,
-			      char *type)
+int sdw_slave_read_dpn(struct sdw_slave *slave,
+		       struct sdw_dpn_prop *dpn, int count, int ports,
+		       char *type)
 {
 	struct fwnode_handle *node;
 	u32 bit, i = 0;
@@ -283,6 +283,7 @@ static int sdw_slave_read_dpn(struct sdw_slave *slave,
 
 	return 0;
 }
+EXPORT_SYMBOL(sdw_slave_read_dpn);
 
 /**
  * sdw_slave_read_prop() - Read Slave properties
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index 35662d9c2c62..04a45225e248 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -345,6 +345,9 @@ struct sdw_master_prop {
 
 int sdw_master_read_prop(struct sdw_bus *bus);
 int sdw_slave_read_prop(struct sdw_slave *slave);
+int sdw_slave_read_dpn(struct sdw_slave *slave,
+		       struct sdw_dpn_prop *dpn, int count, int ports,
+		       char *type);
 
 /*
  * SDW Slave Structures and APIs
-- 
2.17.1

[PATCH 4/8] soundwire: mipi_disco: expose sdw_slave_read_dp0 as symbol

[Pierre-Louis Bossart]

sdw_slave_read_dp0 was so far a static function, which prevented codec
drivers from using it. Expose as non-static function and add symbol

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

diff --git a/drivers/soundwire/mipi_disco.c b/drivers/soundwire/mipi_disco.c
index 4995554bd6b6..f6b1be920a19 100644
--- a/drivers/soundwire/mipi_disco.c
+++ b/drivers/soundwire/mipi_disco.c
@@ -121,9 +121,9 @@ int sdw_master_read_prop(struct sdw_bus *bus)
 }
 EXPORT_SYMBOL(sdw_master_read_prop);
 
-static int sdw_slave_read_dp0(struct sdw_slave *slave,
-			      struct fwnode_handle *port,
-			      struct sdw_dp0_prop *dp0)
+int sdw_slave_read_dp0(struct sdw_slave *slave,
+		       struct fwnode_handle *port,
+		       struct sdw_dp0_prop *dp0)
 {
 	int nval;
 
@@ -160,6 +160,7 @@ static int sdw_slave_read_dp0(struct sdw_slave *slave,
 
 	return 0;
 }
+EXPORT_SYMBOL(sdw_slave_read_dp0);
 
 int sdw_slave_read_dpn(struct sdw_slave *slave,
 		       struct sdw_dpn_prop *dpn, int count, int ports,
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index 04a45225e248..594c17ed8593 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -348,6 +348,9 @@ int sdw_slave_read_prop(struct sdw_slave *slave);
 int sdw_slave_read_dpn(struct sdw_slave *slave,
 		       struct sdw_dpn_prop *dpn, int count, int ports,
 		       char *type);
+int sdw_slave_read_dp0(struct sdw_slave *slave,
+		       struct fwnode_handle *port,
+		       struct sdw_dp0_prop *dp0);
 
 /*
  * SDW Slave Structures and APIs
-- 
2.17.1

[PATCH 5/8] soundwire: add port-related definitions

[Pierre-Louis Bossart]

Somehow previous header files did not include definition for
sink/source, flow and grouping.

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

diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index 594c17ed8593..bd05a85d345c 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -41,6 +41,31 @@ struct sdw_slave;
 #define SDW_DAI_ID_RANGE_START		100
 #define SDW_DAI_ID_RANGE_END		200
 
+enum {
+	SDW_PORT_DIRN_SINK = 0,
+	SDW_PORT_DIRN_SOURCE,
+	SDW_PORT_DIRN_MAX,
+};
+
+/*
+ * constants for flow control, ports and transport
+ *
+ * these are bit masks as devices can have multiple capabilities
+ */
+
+/*
+ * flow modes for SDW port. These can be isochronous, tx controlled,
+ * rx controlled or async
+ */
+#define SDW_PORT_FLOW_MODE_ISOCH	0
+#define SDW_PORT_FLOW_MODE_TX_CNTRL	BIT(0)
+#define SDW_PORT_FLOW_MODE_RX_CNTRL	BIT(1)
+#define SDW_PORT_FLOW_MODE_ASYNC	GENMASK(1, 0)
+
+/* sample packaging for block. It can be per port or per channel */
+#define SDW_BLOCK_PACKG_PER_PORT	BIT(0)
+#define SDW_BLOCK_PACKG_PER_CH		BIT(1)
+
 /**
  * enum sdw_slave_status - Slave status
  * @SDW_SLAVE_UNATTACHED: Slave is not attached with the bus.
@@ -76,6 +101,14 @@ enum sdw_command_response {
 	SDW_CMD_FAIL_OTHER = 4,
 };
 
+/* block group count enum */
+enum sdw_dpn_grouping {
+	SDW_BLK_GRP_CNT_1 = 0,
+	SDW_BLK_GRP_CNT_2 = 1,
+	SDW_BLK_GRP_CNT_3 = 2,
+	SDW_BLK_GRP_CNT_4 = 3,
+};
+
 /**
  * enum sdw_stream_type: data stream type
  *
@@ -100,6 +133,26 @@ enum sdw_data_direction {
 	SDW_DATA_DIR_TX = 1,
 };
 
+/**
+ * enum sdw_port_data_mode: Data Port mode
+ *
+ * @SDW_PORT_DATA_MODE_NORMAL: Normal data mode where audio data is received
+ * and transmitted.
+ * @SDW_PORT_DATA_MODE_STATIC_1: Simple test mode which uses static value of
+ * logic 1. The encoding will result in signal transitions at every bitslot
+ * owned by this Port
+ * @SDW_PORT_DATA_MODE_STATIC_0: Simple test mode which uses static value of
+ * logic 0. The encoding will result in no signal transitions
+ * @SDW_PORT_DATA_MODE_PRBS: Test mode which uses a PRBS generator to produce
+ * a pseudo random data pattern that is transferred
+ */
+enum sdw_port_data_mode {
+	SDW_PORT_DATA_MODE_NORMAL = 0,
+	SDW_PORT_DATA_MODE_STATIC_1 = 1,
+	SDW_PORT_DATA_MODE_STATIC_0 = 2,
+	SDW_PORT_DATA_MODE_PRBS = 3,
+};
+
 /*
  * SDW properties, defined in MIPI DisCo spec v1.0
  */
-- 
2.17.1

[PATCH 6/8] soundwire: remove master data port properties

[Pierre-Louis Bossart]

The SoundWire and DisCo specifications do not define Master data ports
or related properties. Data ports are only defined for Slave devices,
so remove the unused member in properties.

Credits: this patch is based on an earlier internal contribution by
Vinod Koul, Sanyog Kale, Shreyas Nc and Hardik Shah.

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

diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index bd05a85d345c..80584e9d5970 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -377,7 +377,6 @@ struct sdw_slave_prop {
  * @dynamic_frame: Dynamic frame supported
  * @err_threshold: Number of times that software may retry sending a single
  * command
- * @dpn_prop: Data Port N properties
  */
 struct sdw_master_prop {
 	u32 revision;
@@ -393,7 +392,6 @@ struct sdw_master_prop {
 	u32 default_col;
 	bool dynamic_frame;
 	u32 err_threshold;
-	struct sdw_dpn_prop *dpn_prop;
 };
 
 int sdw_master_read_prop(struct sdw_bus *bus);
-- 
2.17.1

[PATCH 7/8] soundwire: fix master properties

[Pierre-Louis Bossart]

The master-count is only defined for a controller or a Slave in the
MIPI DisCo for SoundWire document.

rename all fields with 'freq' as 'clk_freq' to follow the MIPI
specification and avoid confusion between bus clock and audio clocks.

fix support for clock_stop_mode0 and 1. The existing code uses a
bitmask between enums, one of which being zero. Or'ing with zero is
not very useful in general...Fix by or-ing with a BIT dependent on the
enum value.

Fix additional comments to align with MIPI spec

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/soundwire/bus.c        |  4 ++--
 drivers/soundwire/intel.c      | 11 ++++++-----
 drivers/soundwire/mipi_disco.c | 27 ++++++++++++++-------------
 drivers/soundwire/stream.c     |  2 +-
 include/linux/soundwire/sdw.h  | 20 +++++++++-----------
 5 files changed, 32 insertions(+), 32 deletions(-)

diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index aac35fc3cf22..96e42df8f458 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -87,7 +87,7 @@ int sdw_add_bus_master(struct sdw_bus *bus)
 
 	/*
 	 * Initialize clock values based on Master properties. The max
-	 * frequency is read from max_freq property. Current assumption
+	 * frequency is read from max_clk_freq property. Current assumption
 	 * is that the bus will start at highest clock frequency when
 	 * powered on.
 	 *
@@ -95,7 +95,7 @@ int sdw_add_bus_master(struct sdw_bus *bus)
 	 * to start with bank 0 (Table 40 of Spec)
 	 */
 	prop = &bus->prop;
-	bus->params.max_dr_freq = prop->max_freq * SDW_DOUBLE_RATE_FACTOR;
+	bus->params.max_dr_freq = prop->max_clk_freq * SDW_DOUBLE_RATE_FACTOR;
 	bus->params.curr_dr_freq = bus->params.max_dr_freq;
 	bus->params.curr_bank = SDW_BANK0;
 	bus->params.next_bank = SDW_BANK1;
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index 31336b0271b0..4ac141730b13 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -796,13 +796,14 @@ static int intel_prop_read(struct sdw_bus *bus)
 	sdw_master_read_prop(bus);
 
 	/* BIOS is not giving some values correctly. So, lets override them */
-	bus->prop.num_freq = 1;
-	bus->prop.freq = devm_kcalloc(bus->dev, bus->prop.num_freq,
-				      sizeof(*bus->prop.freq), GFP_KERNEL);
-	if (!bus->prop.freq)
+	bus->prop.num_clk_freq = 1;
+	bus->prop.clk_freq = devm_kcalloc(bus->dev, bus->prop.num_clk_freq,
+					  sizeof(*bus->prop.clk_freq),
+					  GFP_KERNEL);
+	if (!bus->prop.clk_freq)
 		return -ENOMEM;
 
-	bus->prop.freq[0] = bus->prop.max_freq;
+	bus->prop.clk_freq[0] = bus->prop.max_clk_freq;
 	bus->prop.err_threshold = 5;
 
 	return 0;
diff --git a/drivers/soundwire/mipi_disco.c b/drivers/soundwire/mipi_disco.c
index f6b1be920a19..7db816691393 100644
--- a/drivers/soundwire/mipi_disco.c
+++ b/drivers/soundwire/mipi_disco.c
@@ -50,39 +50,40 @@ int sdw_master_read_prop(struct sdw_bus *bus)
 
 	if (fwnode_property_read_bool(link,
 				      "mipi-sdw-clock-stop-mode0-supported"))
-		prop->clk_stop_mode = SDW_CLK_STOP_MODE0;
+		prop->clk_stop_modes |= BIT(SDW_CLK_STOP_MODE0);
 
 	if (fwnode_property_read_bool(link,
 				      "mipi-sdw-clock-stop-mode1-supported"))
-		prop->clk_stop_mode |= SDW_CLK_STOP_MODE1;
+		prop->clk_stop_modes |= BIT(SDW_CLK_STOP_MODE1);
 
 	fwnode_property_read_u32(link,
 				 "mipi-sdw-max-clock-frequency",
-				 &prop->max_freq);
+				 &prop->max_clk_freq);
 
 	nval = fwnode_property_read_u32_array(link,
 			"mipi-sdw-clock-frequencies-supported", NULL, 0);
 	if (nval > 0) {
-		prop->num_freq = nval;
-		prop->freq = devm_kcalloc(bus->dev, prop->num_freq,
-					  sizeof(*prop->freq), GFP_KERNEL);
-		if (!prop->freq)
+		prop->num_clk_freq = nval;
+		prop->clk_freq = devm_kcalloc(bus->dev, prop->num_clk_freq,
+					      sizeof(*prop->clk_freq),
+					      GFP_KERNEL);
+		if (!prop->clk_freq)
 			return -ENOMEM;
 
 		fwnode_property_read_u32_array(link,
 				"mipi-sdw-clock-frequencies-supported",
-				prop->freq, prop->num_freq);
+				prop->clk_freq, prop->num_clk_freq);
 	}
 
 	/*
 	 * Check the frequencies supported. If FW doesn't provide max
 	 * freq, then populate here by checking values.
 	 */
-	if (!prop->max_freq && prop->freq) {
-		prop->max_freq = prop->freq[0];
-		for (i = 1; i < prop->num_freq; i++) {
-			if (prop->freq[i] > prop->max_freq)
-				prop->max_freq = prop->freq[i];
+	if (!prop->max_clk_freq && prop->clk_freq) {
+		prop->max_clk_freq = prop->clk_freq[0];
+		for (i = 1; i < prop->num_clk_freq; i++) {
+			if (prop->clk_freq[i] > prop->max_clk_freq)
+				prop->max_clk_freq = prop->clk_freq[i];
 		}
 	}
 
diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
index d01060dbee96..89edc897b8eb 100644
--- a/drivers/soundwire/stream.c
+++ b/drivers/soundwire/stream.c
@@ -1474,7 +1474,7 @@ static int _sdw_prepare_stream(struct sdw_stream_runtime *stream)
 		memcpy(&params, &bus->params, sizeof(params));
 
 		/* TODO: Support Asynchronous mode */
-		if ((prop->max_freq % stream->params.rate) != 0) {
+		if ((prop->max_clk_freq % stream->params.rate) != 0) {
 			dev_err(bus->dev, "Async mode not supported\n");
 			return -EINVAL;
 		}
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index 80584e9d5970..89c51838b7ec 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -364,29 +364,27 @@ struct sdw_slave_prop {
 /**
  * struct sdw_master_prop - Master properties
  * @revision: MIPI spec version of the implementation
- * @master_count: Number of masters
- * @clk_stop_mode: Bitmap for Clock Stop modes supported
- * @max_freq: Maximum Bus clock frequency, in Hz
+ * @clk_stop_modes: Bitmap, bit N set when clock-stop-modeN supported
+ * @max_clk_freq: Maximum Bus clock frequency, in Hz
  * @num_clk_gears: Number of clock gears supported
  * @clk_gears: Clock gears supported
- * @num_freq: Number of clock frequencies supported, in Hz
- * @freq: Clock frequencies supported, in Hz
+ * @num_clk_freq: Number of clock frequencies supported, in Hz
+ * @clk_freq: Clock frequencies supported, in Hz
  * @default_frame_rate: Controller default Frame rate, in Hz
  * @default_row: Number of rows
  * @default_col: Number of columns
- * @dynamic_frame: Dynamic frame supported
+ * @dynamic_frame: Dynamic frame shape supported
  * @err_threshold: Number of times that software may retry sending a single
  * command
  */
 struct sdw_master_prop {
 	u32 revision;
-	u32 master_count;
-	enum sdw_clk_stop_mode clk_stop_mode;
-	u32 max_freq;
+	u32 clk_stop_modes;
+	u32 max_clk_freq;
 	u32 num_clk_gears;
 	u32 *clk_gears;
-	u32 num_freq;
-	u32 *freq;
+	u32 num_clk_freq;
+	u32 *clk_freq;
 	u32 default_frame_rate;
 	u32 default_row;
 	u32 default_col;
-- 
2.17.1

[PATCH 8/8] soundwire: rename/clarify MIPI DisCo properties

[Pierre-Louis Bossart]

The existing definitions are ambiguous and possibly misleading.

For DP0, 'flow-control' is only relevant for the BRA protocol and
should not be confused with async modes explicitly not supported for
DP0, add prefix to follow MIPI DisCo definition

The use of 'device_interrupts' is also questionable. The MIPI
SoundWire spec defines Slave-, DP0- and DPN-level
implementation-defined interrupts. Using the 'device' prefix in the
last two cases is misleading, not only is the term 'device' overloaded
but these properties are only valid at the DP0 and DPn levels. Rename
to follow the MIPI definitions, no need to be creative here.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/soundwire/bus.c        |  2 +-
 drivers/soundwire/mipi_disco.c |  6 +++---
 drivers/soundwire/stream.c     |  6 +++---
 include/linux/soundwire/sdw.h  | 13 +++++++------
 4 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index 96e42df8f458..fe745830a261 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -648,7 +648,7 @@ static int sdw_initialize_slave(struct sdw_slave *slave)
 		return 0;
 
 	/* Enable DP0 interrupts */
-	val = prop->dp0_prop->device_interrupts;
+	val = prop->dp0_prop->imp_def_interrupts;
 	val |= SDW_DP0_INT_PORT_READY | SDW_DP0_INT_BRA_FAILURE;
 
 	ret = sdw_update(slave, SDW_DP0_INTMASK, val, val);
diff --git a/drivers/soundwire/mipi_disco.c b/drivers/soundwire/mipi_disco.c
index 7db816691393..a065cba8c2c5 100644
--- a/drivers/soundwire/mipi_disco.c
+++ b/drivers/soundwire/mipi_disco.c
@@ -150,13 +150,13 @@ int sdw_slave_read_dp0(struct sdw_slave *slave,
 				dp0->words, dp0->num_words);
 	}
 
-	dp0->flow_controlled = fwnode_property_read_bool(port,
+	dp0->BRA_flow_controlled = fwnode_property_read_bool(port,
 				"mipi-sdw-bra-flow-controlled");
 
 	dp0->simple_ch_prep_sm = fwnode_property_read_bool(port,
 				"mipi-sdw-simplified-channel-prepare-sm");
 
-	dp0->device_interrupts = fwnode_property_read_bool(port,
+	dp0->imp_def_interrupts = fwnode_property_read_bool(port,
 				"mipi-sdw-imp-def-dp0-interrupts-supported");
 
 	return 0;
@@ -226,7 +226,7 @@ int sdw_slave_read_dpn(struct sdw_slave *slave,
 
 		fwnode_property_read_u32(node,
 				"mipi-sdw-imp-def-dpn-interrupts-supported",
-				&dpn[i].device_interrupts);
+				&dpn[i].imp_def_interrupts);
 
 		fwnode_property_read_u32(node, "mipi-sdw-min-channel-number",
 					 &dpn[i].min_ch);
diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
index 89edc897b8eb..ce9cb7fa4724 100644
--- a/drivers/soundwire/stream.c
+++ b/drivers/soundwire/stream.c
@@ -439,7 +439,7 @@ static int sdw_prep_deprep_slave_ports(struct sdw_bus *bus,
 
 	prep_ch.bank = bus->params.next_bank;
 
-	if (dpn_prop->device_interrupts || !dpn_prop->simple_ch_prep_sm)
+	if (dpn_prop->imp_def_interrupts || !dpn_prop->simple_ch_prep_sm)
 		intr = true;
 
 	/*
@@ -449,7 +449,7 @@ static int sdw_prep_deprep_slave_ports(struct sdw_bus *bus,
 	 */
 	if (prep && intr) {
 		ret = sdw_configure_dpn_intr(s_rt->slave, p_rt->num, prep,
-					     dpn_prop->device_interrupts);
+					     dpn_prop->imp_def_interrupts);
 		if (ret < 0)
 			return ret;
 	}
@@ -493,7 +493,7 @@ static int sdw_prep_deprep_slave_ports(struct sdw_bus *bus,
 	/* Disable interrupt after Port de-prepare */
 	if (!prep && intr)
 		ret = sdw_configure_dpn_intr(s_rt->slave, p_rt->num, prep,
-					     dpn_prop->device_interrupts);
+					     dpn_prop->imp_def_interrupts);
 
 	return ret;
 }
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index 89c51838b7ec..3b231472464a 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -206,10 +206,11 @@ enum sdw_clk_stop_mode {
  * (inclusive)
  * @num_words: number of wordlengths supported
  * @words: wordlengths supported
- * @flow_controlled: Slave implementation results in an OK_NotReady
+ * @BRA_flow_controlled: Slave implementation results in an OK_NotReady
  * response
  * @simple_ch_prep_sm: If channel prepare sequence is required
- * @device_interrupts: If implementation-defined interrupts are supported
+ * @imp_def_interrupts: If set, each bit corresponds to support for
+ * implementation-defined interrupts
  *
  * The wordlengths are specified by Spec as max, min AND number of
  * discrete values, implementation can define based on the wordlengths they
@@ -220,9 +221,9 @@ struct sdw_dp0_prop {
 	u32 min_word;
 	u32 num_words;
 	u32 *words;
-	bool flow_controlled;
+	bool BRA_flow_controlled;
 	bool simple_ch_prep_sm;
-	bool device_interrupts;
+	bool imp_def_interrupts;
 };
 
 /**
@@ -272,7 +273,7 @@ struct sdw_dpn_audio_mode {
  * @simple_ch_prep_sm: If the port supports simplified channel prepare state
  * machine
  * @ch_prep_timeout: Port-specific timeout value, in milliseconds
- * @device_interrupts: If set, each bit corresponds to support for
+ * @imp_def_interrupts: If set, each bit corresponds to support for
  * implementation-defined interrupts
  * @max_ch: Maximum channels supported
  * @min_ch: Minimum channels supported
@@ -297,7 +298,7 @@ struct sdw_dpn_prop {
 	u32 max_grouping;
 	bool simple_ch_prep_sm;
 	u32 ch_prep_timeout;
-	u32 device_interrupts;
+	u32 imp_def_interrupts;
 	u32 max_ch;
 	u32 min_ch;
 	u32 num_ch;
-- 
2.17.1

Re: [PATCH 2/8] soundwire: mipi_disco: fix master/link error

[Pierre-Louis Bossart]

On 5/3/19 7:29 PM, Pierre-Louis Bossart wrote:
> The MIPI DisCo specification for SoundWire defines the
> "mipi-sdw-link-N-subproperties", not the master-N properties. Fix to
> parse firmware information.

Please ignore this patch for now, there is a confusion in the spec 
itself that needs to be addressed by MIPI... Either there will be an 
errata issued, or we'll have to try both master- and link-N-properties 
to reconcile spec and actual implementations.

> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>   drivers/soundwire/mipi_disco.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/soundwire/mipi_disco.c b/drivers/soundwire/mipi_disco.c
> index c1f51d6a23d2..6df68584c963 100644
> --- a/drivers/soundwire/mipi_disco.c
> +++ b/drivers/soundwire/mipi_disco.c
> @@ -40,7 +40,7 @@ int sdw_master_read_prop(struct sdw_bus *bus)
>   
>   	/* Find master handle */
>   	snprintf(name, sizeof(name),
> -		 "mipi-sdw-master-%d-subproperties", bus->link_id);
> +		 "mipi-sdw-link-%d-subproperties", bus->link_id);
>   
>   	link = device_get_named_child_node(bus->dev, name);
>   	if (!link) {
> 

Re: [PATCH 1/8] soundwire: intel: filter SoundWire controller device search

[Vinod Koul]

On 03-05-19, 19:29, Pierre-Louis Bossart wrote:
> The convention is that the SoundWire controller device is a child of
> the HDAudio controller. However there can be more than one child
> exposed in the DSDT table, and the current namespace walk returns the
> last device.
> 
> Add a filter and terminate early when a valid _ADR is provided,
> otherwise keep iterating to find the next child.

So what are the other devices in DSDT here..

> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  drivers/soundwire/intel_init.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/soundwire/intel_init.c b/drivers/soundwire/intel_init.c
> index d3d6b54c5791..f85db67d05f0 100644
> --- a/drivers/soundwire/intel_init.c
> +++ b/drivers/soundwire/intel_init.c
> @@ -150,6 +150,12 @@ static acpi_status sdw_intel_acpi_cb(acpi_handle handle, u32 level,
>  {
>  	struct sdw_intel_res *res = cdata;
>  	struct acpi_device *adev;
> +	acpi_status status;
> +	u64 adr;
> +
> +	status = acpi_evaluate_integer(handle, METHOD_NAME__ADR, NULL, &adr);
> +	if (ACPI_FAILURE(status))
> +		return AE_OK; /* keep going */
>  
>  	if (acpi_bus_get_device(handle, &adev)) {
>  		pr_err("%s: Couldn't find ACPI handle\n", __func__);
> @@ -157,7 +163,18 @@ static acpi_status sdw_intel_acpi_cb(acpi_handle handle, u32 level,
>  	}
>  
>  	res->handle = handle;
> -	return AE_OK;
> +
> +	/*
> +	 * On some Intel platforms, multiple children of the HDAS
> +	 * device can be found, but only one of them is the SoundWire
> +	 * controller. The SNDW device is always exposed with
> +	 * Name(_ADR, 0x40000000) so filter accordingly
> +	 */
> +	if (adr != 0x40000000)

I do not recall if 4 corresponds to the links you have or soundwire
device type, is this number documented somewhere is HDA specs?

Also it might good to create a define for this
 
> +		return AE_OK; /* keep going */
> +
> +	/* device found, stop namespace walk */
> +	return AE_CTRL_TERMINATE;
>  }
>  
>  /**
> -- 
> 2.17.1

-- 
~Vinod

Re: [PATCH 3/8] soundwire: mipi_disco: expose sdw_slave_read_dpn as symbol

[Vinod Koul]

On 03-05-19, 19:29, Pierre-Louis Bossart wrote:
> sdw_slave_read_dpn was so far a static function, which prevented
> codec drivers from using it. Expose as non-static function and add symbol
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  drivers/soundwire/mipi_disco.c | 7 ++++---
>  include/linux/soundwire/sdw.h  | 3 +++
>  2 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/soundwire/mipi_disco.c b/drivers/soundwire/mipi_disco.c
> index 6df68584c963..4995554bd6b6 100644
> --- a/drivers/soundwire/mipi_disco.c
> +++ b/drivers/soundwire/mipi_disco.c
> @@ -161,9 +161,9 @@ static int sdw_slave_read_dp0(struct sdw_slave *slave,
>  	return 0;
>  }
>  
> -static int sdw_slave_read_dpn(struct sdw_slave *slave,
> -			      struct sdw_dpn_prop *dpn, int count, int ports,
> -			      char *type)
> +int sdw_slave_read_dpn(struct sdw_slave *slave,
> +		       struct sdw_dpn_prop *dpn, int count, int ports,
> +		       char *type)
>  {
>  	struct fwnode_handle *node;
>  	u32 bit, i = 0;
> @@ -283,6 +283,7 @@ static int sdw_slave_read_dpn(struct sdw_slave *slave,
>  
>  	return 0;
>  }
> +EXPORT_SYMBOL(sdw_slave_read_dpn);

Fine to export but would be great to accompany user with it. In
general do not add a API without user.

-- 
~Vinod

Re: [PATCH 1/8] soundwire: intel: filter SoundWire controller device search

[Pierre-Louis Bossart]

On 5/7/19 7:26 AM, Vinod Koul wrote:
> On 03-05-19, 19:29, Pierre-Louis Bossart wrote:
>> The convention is that the SoundWire controller device is a child of
>> the HDAudio controller. However there can be more than one child
>> exposed in the DSDT table, and the current namespace walk returns the
>> last device.
>>
>> Add a filter and terminate early when a valid _ADR is provided,
>> otherwise keep iterating to find the next child.
> 
> So what are the other devices in DSDT here..

this is what I see:

Scope (HDAS)
         {
             Device (IDA)
             {
                 Name (_ADR, 0x00020001)  // _ADR: Address
             }
         }

I thought this was nonsense but your question triggered me to look into 
the Intel SST ACPI specs (not public I am afraid but shared with the OS 
who shall not be named).
Using the same source of information as below, I *believe* this is 
HDaudio related, bits 31..16 mean HDaudio with codec SDI 2, and NodeId 1 
for the function group. This would make sense as I believe there are two 
codecs on the board that can be pin-strapped to boot either in HDaudio 
or SoundWire mode- but this is a conjecture only.

At any rate, we need a hardware rework and mutual exclusion between 
HDaudio and SoundWire, so we have to ignore this one when SoundWire is 
enabled.

>> +
>> +	/*
>> +	 * On some Intel platforms, multiple children of the HDAS
>> +	 * device can be found, but only one of them is the SoundWire
>> +	 * controller. The SNDW device is always exposed with
>> +	 * Name(_ADR, 0x40000000) so filter accordingly
>> +	 */
>> +	if (adr != 0x40000000)
> 
> I do not recall if 4 corresponds to the links you have or soundwire
> device type, is this number documented somewhere is HDA specs?

I thought it was a magic number, but I did check and for once it's 
documented and the values match the spec :-)
I see in the ACPI docs bits 31..28 set to 4 indicate a SoundWire Link 
Type and bits 3..0 indicate the SoundWire controller instance, the rest 
is reserved to zero.

> 
> Also it might good to create a define for this

I will respin this one to add the documentation above, and only filter 
on the 4 ms-bits. Thanks for forcing me to RTFM :-)

Re: [PATCH 7/8] soundwire: fix master properties

[Vinod Koul]

On 03-05-19, 19:29, Pierre-Louis Bossart wrote:
> The master-count is only defined for a controller or a Slave in the
> MIPI DisCo for SoundWire document.

... so remove it

> rename all fields with 'freq' as 'clk_freq' to follow the MIPI
> specification and avoid confusion between bus clock and audio clocks.

That sounds good to me.

> fix support for clock_stop_mode0 and 1. The existing code uses a
> bitmask between enums, one of which being zero. Or'ing with zero is
> not very useful in general...Fix by or-ing with a BIT dependent on the
> enum value.
> 
> Fix additional comments to align with MIPI spec

Ideally these should be different patches...


> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  drivers/soundwire/bus.c        |  4 ++--
>  drivers/soundwire/intel.c      | 11 ++++++-----
>  drivers/soundwire/mipi_disco.c | 27 ++++++++++++++-------------
>  drivers/soundwire/stream.c     |  2 +-
>  include/linux/soundwire/sdw.h  | 20 +++++++++-----------
>  5 files changed, 32 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
> index aac35fc3cf22..96e42df8f458 100644
> --- a/drivers/soundwire/bus.c
> +++ b/drivers/soundwire/bus.c
> @@ -87,7 +87,7 @@ int sdw_add_bus_master(struct sdw_bus *bus)
>  
>  	/*
>  	 * Initialize clock values based on Master properties. The max
> -	 * frequency is read from max_freq property. Current assumption
> +	 * frequency is read from max_clk_freq property. Current assumption
>  	 * is that the bus will start at highest clock frequency when
>  	 * powered on.
>  	 *
> @@ -95,7 +95,7 @@ int sdw_add_bus_master(struct sdw_bus *bus)
>  	 * to start with bank 0 (Table 40 of Spec)
>  	 */
>  	prop = &bus->prop;
> -	bus->params.max_dr_freq = prop->max_freq * SDW_DOUBLE_RATE_FACTOR;
> +	bus->params.max_dr_freq = prop->max_clk_freq * SDW_DOUBLE_RATE_FACTOR;
>  	bus->params.curr_dr_freq = bus->params.max_dr_freq;
>  	bus->params.curr_bank = SDW_BANK0;
>  	bus->params.next_bank = SDW_BANK1;
> diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
> index 31336b0271b0..4ac141730b13 100644
> --- a/drivers/soundwire/intel.c
> +++ b/drivers/soundwire/intel.c
> @@ -796,13 +796,14 @@ static int intel_prop_read(struct sdw_bus *bus)
>  	sdw_master_read_prop(bus);
>  
>  	/* BIOS is not giving some values correctly. So, lets override them */
> -	bus->prop.num_freq = 1;
> -	bus->prop.freq = devm_kcalloc(bus->dev, bus->prop.num_freq,
> -				      sizeof(*bus->prop.freq), GFP_KERNEL);
> -	if (!bus->prop.freq)
> +	bus->prop.num_clk_freq = 1;
> +	bus->prop.clk_freq = devm_kcalloc(bus->dev, bus->prop.num_clk_freq,
> +					  sizeof(*bus->prop.clk_freq),
> +					  GFP_KERNEL);
> +	if (!bus->prop.clk_freq)
>  		return -ENOMEM;
>  
> -	bus->prop.freq[0] = bus->prop.max_freq;
> +	bus->prop.clk_freq[0] = bus->prop.max_clk_freq;
>  	bus->prop.err_threshold = 5;
>  
>  	return 0;
> diff --git a/drivers/soundwire/mipi_disco.c b/drivers/soundwire/mipi_disco.c
> index f6b1be920a19..7db816691393 100644
> --- a/drivers/soundwire/mipi_disco.c
> +++ b/drivers/soundwire/mipi_disco.c
> @@ -50,39 +50,40 @@ int sdw_master_read_prop(struct sdw_bus *bus)
>  
>  	if (fwnode_property_read_bool(link,
>  				      "mipi-sdw-clock-stop-mode0-supported"))
> -		prop->clk_stop_mode = SDW_CLK_STOP_MODE0;
> +		prop->clk_stop_modes |= BIT(SDW_CLK_STOP_MODE0);
>  
>  	if (fwnode_property_read_bool(link,
>  				      "mipi-sdw-clock-stop-mode1-supported"))
> -		prop->clk_stop_mode |= SDW_CLK_STOP_MODE1;
> +		prop->clk_stop_modes |= BIT(SDW_CLK_STOP_MODE1);
>  
>  	fwnode_property_read_u32(link,
>  				 "mipi-sdw-max-clock-frequency",
> -				 &prop->max_freq);
> +				 &prop->max_clk_freq);
>  
>  	nval = fwnode_property_read_u32_array(link,
>  			"mipi-sdw-clock-frequencies-supported", NULL, 0);
>  	if (nval > 0) {
> -		prop->num_freq = nval;
> -		prop->freq = devm_kcalloc(bus->dev, prop->num_freq,
> -					  sizeof(*prop->freq), GFP_KERNEL);
> -		if (!prop->freq)
> +		prop->num_clk_freq = nval;
> +		prop->clk_freq = devm_kcalloc(bus->dev, prop->num_clk_freq,
> +					      sizeof(*prop->clk_freq),
> +					      GFP_KERNEL);
> +		if (!prop->clk_freq)
>  			return -ENOMEM;
>  
>  		fwnode_property_read_u32_array(link,
>  				"mipi-sdw-clock-frequencies-supported",
> -				prop->freq, prop->num_freq);
> +				prop->clk_freq, prop->num_clk_freq);
>  	}
>  
>  	/*
>  	 * Check the frequencies supported. If FW doesn't provide max
>  	 * freq, then populate here by checking values.
>  	 */
> -	if (!prop->max_freq && prop->freq) {
> -		prop->max_freq = prop->freq[0];
> -		for (i = 1; i < prop->num_freq; i++) {
> -			if (prop->freq[i] > prop->max_freq)
> -				prop->max_freq = prop->freq[i];
> +	if (!prop->max_clk_freq && prop->clk_freq) {
> +		prop->max_clk_freq = prop->clk_freq[0];
> +		for (i = 1; i < prop->num_clk_freq; i++) {
> +			if (prop->clk_freq[i] > prop->max_clk_freq)
> +				prop->max_clk_freq = prop->clk_freq[i];
>  		}
>  	}
>  
> diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
> index d01060dbee96..89edc897b8eb 100644
> --- a/drivers/soundwire/stream.c
> +++ b/drivers/soundwire/stream.c
> @@ -1474,7 +1474,7 @@ static int _sdw_prepare_stream(struct sdw_stream_runtime *stream)
>  		memcpy(&params, &bus->params, sizeof(params));
>  
>  		/* TODO: Support Asynchronous mode */
> -		if ((prop->max_freq % stream->params.rate) != 0) {
> +		if ((prop->max_clk_freq % stream->params.rate) != 0) {
>  			dev_err(bus->dev, "Async mode not supported\n");
>  			return -EINVAL;
>  		}
> diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
> index 80584e9d5970..89c51838b7ec 100644
> --- a/include/linux/soundwire/sdw.h
> +++ b/include/linux/soundwire/sdw.h
> @@ -364,29 +364,27 @@ struct sdw_slave_prop {
>  /**
>   * struct sdw_master_prop - Master properties
>   * @revision: MIPI spec version of the implementation
> - * @master_count: Number of masters
> - * @clk_stop_mode: Bitmap for Clock Stop modes supported
> - * @max_freq: Maximum Bus clock frequency, in Hz
> + * @clk_stop_modes: Bitmap, bit N set when clock-stop-modeN supported
> + * @max_clk_freq: Maximum Bus clock frequency, in Hz
>   * @num_clk_gears: Number of clock gears supported
>   * @clk_gears: Clock gears supported
> - * @num_freq: Number of clock frequencies supported, in Hz
> - * @freq: Clock frequencies supported, in Hz
> + * @num_clk_freq: Number of clock frequencies supported, in Hz
> + * @clk_freq: Clock frequencies supported, in Hz
>   * @default_frame_rate: Controller default Frame rate, in Hz
>   * @default_row: Number of rows
>   * @default_col: Number of columns
> - * @dynamic_frame: Dynamic frame supported
> + * @dynamic_frame: Dynamic frame shape supported
>   * @err_threshold: Number of times that software may retry sending a single
>   * command
>   */
>  struct sdw_master_prop {
>  	u32 revision;
> -	u32 master_count;
> -	enum sdw_clk_stop_mode clk_stop_mode;
> -	u32 max_freq;
> +	u32 clk_stop_modes;
> +	u32 max_clk_freq;
>  	u32 num_clk_gears;
>  	u32 *clk_gears;
> -	u32 num_freq;
> -	u32 *freq;
> +	u32 num_clk_freq;
> +	u32 *clk_freq;
>  	u32 default_frame_rate;
>  	u32 default_row;
>  	u32 default_col;
> -- 
> 2.17.1

-- 
~Vinod

Re: [PATCH 1/8] soundwire: intel: filter SoundWire controller device search

[Vinod Koul]

On 07-05-19, 09:43, Pierre-Louis Bossart wrote:
> 
> 
> On 5/7/19 7:26 AM, Vinod Koul wrote:
> > On 03-05-19, 19:29, Pierre-Louis Bossart wrote:
> > > The convention is that the SoundWire controller device is a child of
> > > the HDAudio controller. However there can be more than one child
> > > exposed in the DSDT table, and the current namespace walk returns the
> > > last device.
> > > 
> > > Add a filter and terminate early when a valid _ADR is provided,
> > > otherwise keep iterating to find the next child.
> > 
> > So what are the other devices in DSDT here..
> 
> this is what I see:
> 
> Scope (HDAS)
>         {
>             Device (IDA)
>             {
>                 Name (_ADR, 0x00020001)  // _ADR: Address
>             }
>         }
> 
> I thought this was nonsense but your question triggered me to look into the
> Intel SST ACPI specs (not public I am afraid but shared with the OS who
> shall not be named).
> Using the same source of information as below, I *believe* this is HDaudio
> related, bits 31..16 mean HDaudio with codec SDI 2, and NodeId 1 for the
> function group. This would make sense as I believe there are two codecs on
> the board that can be pin-strapped to boot either in HDaudio or SoundWire
> mode- but this is a conjecture only.
> 
> At any rate, we need a hardware rework and mutual exclusion between HDaudio
> and SoundWire, so we have to ignore this one when SoundWire is enabled.

That is how I was expecting it to be...

> > > +	/*
> > > +	 * On some Intel platforms, multiple children of the HDAS
> > > +	 * device can be found, but only one of them is the SoundWire
> > > +	 * controller. The SNDW device is always exposed with
> > > +	 * Name(_ADR, 0x40000000) so filter accordingly
> > > +	 */
> > > +	if (adr != 0x40000000)
> > 
> > I do not recall if 4 corresponds to the links you have or soundwire
> > device type, is this number documented somewhere is HDA specs?
> 
> I thought it was a magic number, but I did check and for once it's
> documented and the values match the spec :-)
> I see in the ACPI docs bits 31..28 set to 4 indicate a SoundWire Link Type
> and bits 3..0 indicate the SoundWire controller instance, the rest is
> reserved to zero.

So in that case we should mask with bits 31..28 and match, who knows you
may have multiple controller instances in future
I had a vague recollection that this was documented in the spec, glad
that in turned out to be the case.

Btw was the update to HDA spec made public?

> > Also it might good to create a define for this
> 
> I will respin this one to add the documentation above, and only filter on
> the 4 ms-bits. Thanks for forcing me to RTFM :-)

-- 
~Vinod

Re: [PATCH 1/8] soundwire: intel: filter SoundWire controller device search

[Pierre-Louis Bossart]

>>>> +	/*
>>>> +	 * On some Intel platforms, multiple children of the HDAS
>>>> +	 * device can be found, but only one of them is the SoundWire
>>>> +	 * controller. The SNDW device is always exposed with
>>>> +	 * Name(_ADR, 0x40000000) so filter accordingly
>>>> +	 */
>>>> +	if (adr != 0x40000000)
>>>
>>> I do not recall if 4 corresponds to the links you have or soundwire
>>> device type, is this number documented somewhere is HDA specs?
>>
>> I thought it was a magic number, but I did check and for once it's
>> documented and the values match the spec :-)
>> I see in the ACPI docs bits 31..28 set to 4 indicate a SoundWire Link Type
>> and bits 3..0 indicate the SoundWire controller instance, the rest is
>> reserved to zero.
> 
> So in that case we should mask with bits 31..28 and match, who knows you
> may have multiple controller instances in future

yes, I was planning on only using the link type.

> I had a vague recollection that this was documented in the spec, glad
> that in turned out to be the case.
> 
> Btw was the update to HDA spec made public?

Not that I know of. The previous NHLT public doc has actually 
disappeared from the Intel site and I can't find it any longer, so 
currently the amount of public documentation is trending to zero :-(

> 
>>> Also it might good to create a define for this
>>
>> I will respin this one to add the documentation above, and only filter on
>> the 4 ms-bits. Thanks for forcing me to RTFM :-)
> 

Re: [PATCH 1/8] soundwire: intel: filter SoundWire controller device search

[Pierre-Louis Bossart]

>>>> +	/*
>>>> +	 * On some Intel platforms, multiple children of the HDAS
>>>> +	 * device can be found, but only one of them is the SoundWire
>>>> +	 * controller. The SNDW device is always exposed with
>>>> +	 * Name(_ADR, 0x40000000) so filter accordingly
>>>> +	 */
>>>> +	if (adr != 0x40000000)
>>>
>>> I do not recall if 4 corresponds to the links you have or soundwire
>>> device type, is this number documented somewhere is HDA specs?
>>
>> I thought it was a magic number, but I did check and for once it's
>> documented and the values match the spec :-)
>> I see in the ACPI docs bits 31..28 set to 4 indicate a SoundWire Link Type
>> and bits 3..0 indicate the SoundWire controller instance, the rest is
>> reserved to zero.
> 
> So in that case we should mask with bits 31..28 and match, who knows you
> may have multiple controller instances in future

yes, I was planning on only using the link type.

> I had a vague recollection that this was documented in the spec, glad
> that in turned out to be the case.
> 
> Btw was the update to HDA spec made public?

Not that I know of. The previous NHLT public doc has actually 
disappeared from the Intel site and I can't find it any longer, so 
currently the amount of public documentation is trending to zero :-(

> 
>>> Also it might good to create a define for this
>>
>> I will respin this one to add the documentation above, and only filter on
>> the 4 ms-bits. Thanks for forcing me to RTFM :-)
> 

Re: [PATCH 1/8] soundwire: intel: filter SoundWire controller device search

[Vinod Koul]

On 08-05-19, 11:20, Pierre-Louis Bossart wrote:
> 
> > > > > +	/*
> > > > > +	 * On some Intel platforms, multiple children of the HDAS
> > > > > +	 * device can be found, but only one of them is the SoundWire
> > > > > +	 * controller. The SNDW device is always exposed with
> > > > > +	 * Name(_ADR, 0x40000000) so filter accordingly
> > > > > +	 */
> > > > > +	if (adr != 0x40000000)
> > > > 
> > > > I do not recall if 4 corresponds to the links you have or soundwire
> > > > device type, is this number documented somewhere is HDA specs?
> > > 
> > > I thought it was a magic number, but I did check and for once it's
> > > documented and the values match the spec :-)
> > > I see in the ACPI docs bits 31..28 set to 4 indicate a SoundWire Link Type
> > > and bits 3..0 indicate the SoundWire controller instance, the rest is
> > > reserved to zero.
> > 
> > So in that case we should mask with bits 31..28 and match, who knows you
> > may have multiple controller instances in future
> 
> yes, I was planning on only using the link type.
> 
> > I had a vague recollection that this was documented in the spec, glad
> > that in turned out to be the case.
> > 
> > Btw was the update to HDA spec made public?
> 
> Not that I know of. The previous NHLT public doc has actually disappeared
> from the Intel site and I can't find it any longer, so currently the amount
> of public documentation is trending to zero :-(
> 
> > 
> > > > Also it might good to create a define for this
> > > 
> > > I will respin this one to add the documentation above, and only filter on
> > > the 4 ms-bits. Thanks for forcing me to RTFM :-)

Yeah about that someone was indeed complaining about that on IRC, it is
shame that link is valid but doc is gone... check with Rakesh or someone
they might have a copy...

-- 
~Vinod