[RFC PATCH 0/6] soundwire: Add support to Qualcomm SoundWire master

[RFC PATCH 0/6] soundwire: Add support to Qualcomm SoundWire master

[Srinivas Kandagatla]

Hi All, 

This patchset is very first version of Qualcomm SoundWire Master Controller
found in most of Qualcomm SoCs and WCD audio codecs.

This driver along with WCD934x codec and WSA881x Class-D Smart Speaker Amplifier
drivers is on DragonBoard DB845c based of SDM845 SoC.
WCD934x and WSA881x patches will be posted soon.

SoundWire controller on SDM845 is integrated in WCD934x audio codec via
SlimBus interface.

Currently this driver is very minimal and only supports PDM.

Most of the code in this driver is rework of Qualcomm downstream drivers
used in Andriod. Credits to Banajit Goswami and Patrick Lai's Team.

TODO:
	Test and add PCM support.

Thanks,
srini

Srinivas Kandagatla (5):
  ASoC: core: add support to snd_soc_dai_get_sdw_stream()
  soundwire: core: define SDW_MAX_PORT
  soundwire: stream: make stream name a const pointer
  dt-bindings: soundwire: add bindings for Qcom controller
  soundwire: qcom: add support for SoundWire controller

Vinod Koul (1):
  soundwire: Add compute_params callback

 .../bindings/soundwire/qcom,swr.txt           |  62 ++
 drivers/soundwire/Kconfig                     |   9 +
 drivers/soundwire/Makefile                    |   4 +
 drivers/soundwire/qcom.c                      | 983 ++++++++++++++++++
 drivers/soundwire/stream.c                    |  11 +-
 include/linux/soundwire/sdw.h                 |   7 +-
 include/sound/soc-dai.h                       |  10 +
 7 files changed, 1083 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/soundwire/qcom,swr.txt
 create mode 100644 drivers/soundwire/qcom.c

-- 
2.21.0

[RFC PATCH 1/6] ASoC: core: add support to snd_soc_dai_get_sdw_stream()

[Srinivas Kandagatla]

On platforms which have smart speaker amplifiers connected via
soundwire and modeled as aux devices in ASoC, in such usecases machine
driver should be able to get sdw master stream from dai so that it can
use the runtime stream to setup slave streams.

soundwire already as a set function, get function would provide more
flexibility to above configurations.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 include/sound/soc-dai.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/include/sound/soc-dai.h b/include/sound/soc-dai.h
index f5d70041108f..9f90b936fd9a 100644
--- a/include/sound/soc-dai.h
+++ b/include/sound/soc-dai.h
@@ -177,6 +177,7 @@ struct snd_soc_dai_ops {
 
 	int (*set_sdw_stream)(struct snd_soc_dai *dai,
 			void *stream, int direction);
+	void *(*get_sdw_stream)(struct snd_soc_dai *dai, int direction);
 	/*
 	 * DAI digital mute - optional.
 	 * Called by soc-core to minimise any pops.
@@ -385,4 +386,13 @@ static inline int snd_soc_dai_set_sdw_stream(struct snd_soc_dai *dai,
 		return -ENOTSUPP;
 }
 
+static inline void *snd_soc_dai_get_sdw_stream(struct snd_soc_dai *dai, int direction)
+{
+	if (dai->driver->ops->get_sdw_stream)
+		return dai->driver->ops->get_sdw_stream(dai, direction);
+	else
+		return NULL;
+
+}
+
 #endif
-- 
2.21.0

Re: [RFC PATCH 1/6] ASoC: core: add support to snd_soc_dai_get_sdw_stream()

[Cezary Rojewski]

On 2019-06-07 10:56, Srinivas Kandagatla wrote:
> On platforms which have smart speaker amplifiers connected via
> soundwire and modeled as aux devices in ASoC, in such usecases machine
> driver should be able to get sdw master stream from dai so that it can
> use the runtime stream to setup slave streams.
> 
> soundwire already as a set function, get function would provide more
> flexibility to above configurations.
> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
>   include/sound/soc-dai.h | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/include/sound/soc-dai.h b/include/sound/soc-dai.h
> index f5d70041108f..9f90b936fd9a 100644
> --- a/include/sound/soc-dai.h
> +++ b/include/sound/soc-dai.h
> @@ -177,6 +177,7 @@ struct snd_soc_dai_ops {
>   
>   	int (*set_sdw_stream)(struct snd_soc_dai *dai,
>   			void *stream, int direction);
> +	void *(*get_sdw_stream)(struct snd_soc_dai *dai, int direction);
>   	/*
>   	 * DAI digital mute - optional.
>   	 * Called by soc-core to minimise any pops.
> @@ -385,4 +386,13 @@ static inline int snd_soc_dai_set_sdw_stream(struct snd_soc_dai *dai,
>   		return -ENOTSUPP;
>   }
>   
> +static inline void *snd_soc_dai_get_sdw_stream(struct snd_soc_dai *dai, int direction)

Exceeds character limit?

> +{
> +	if (dai->driver->ops->get_sdw_stream)
> +		return dai->driver->ops->get_sdw_stream(dai, direction);
> +	else
> +		return NULL;

set_ equivalent returns -ENOTSUPP instead.
ERR_PTR seems to make more sense here.

> +

Unnecessary newline.

> +}
> +
>   #endif
> 

Re: [RFC PATCH 1/6] ASoC: core: add support to snd_soc_dai_get_sdw_stream()

[Srinivas Kandagatla]

Thanks for taking time to review,

On 08/06/2019 20:22, Cezary Rojewski wrote:
> On 2019-06-07 10:56, Srinivas Kandagatla wrote:
>> On platforms which have smart speaker amplifiers connected via
>> soundwire and modeled as aux devices in ASoC, in such usecases machine
>> driver should be able to get sdw master stream from dai so that it can
>> use the runtime stream to setup slave streams.
>>
>> soundwire already as a set function, get function would provide more
>> flexibility to above configurations.
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
>>   include/sound/soc-dai.h | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/include/sound/soc-dai.h b/include/sound/soc-dai.h
>> index f5d70041108f..9f90b936fd9a 100644
>> --- a/include/sound/soc-dai.h
>> +++ b/include/sound/soc-dai.h
>> @@ -177,6 +177,7 @@ struct snd_soc_dai_ops {
>>       int (*set_sdw_stream)(struct snd_soc_dai *dai,
>>               void *stream, int direction);
>> +    void *(*get_sdw_stream)(struct snd_soc_dai *dai, int direction);
>>       /*
>>        * DAI digital mute - optional.
>>        * Called by soc-core to minimise any pops.
>> @@ -385,4 +386,13 @@ static inline int 
>> snd_soc_dai_set_sdw_stream(struct snd_soc_dai *dai,
>>           return -ENOTSUPP;
>>   }
>> +static inline void *snd_soc_dai_get_sdw_stream(struct snd_soc_dai 
>> *dai, int direction)
> 
> Exceeds character limit?
> 
>> +{
>> +    if (dai->driver->ops->get_sdw_stream)
>> +        return dai->driver->ops->get_sdw_stream(dai, direction);
>> +    else
>> +        return NULL;
> 
> set_ equivalent returns -ENOTSUPP instead.
> ERR_PTR seems to make more sense here.
> 
>> +
> 
> Unnecessary newline.

I agree with all the comment, will fix this in next version.

thanks,
srini
> 
>> +}
>> +
>>   #endif
>>

Re: [RFC PATCH 1/6] ASoC: core: add support to snd_soc_dai_get_sdw_stream()

[Vinod Koul]

On 07-06-19, 09:56, Srinivas Kandagatla wrote:
> On platforms which have smart speaker amplifiers connected via
> soundwire and modeled as aux devices in ASoC, in such usecases machine
> driver should be able to get sdw master stream from dai so that it can
> use the runtime stream to setup slave streams.
> 
> soundwire already as a set function, get function would provide more
> flexibility to above configurations.
> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
>  include/sound/soc-dai.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/include/sound/soc-dai.h b/include/sound/soc-dai.h
> index f5d70041108f..9f90b936fd9a 100644
> --- a/include/sound/soc-dai.h
> +++ b/include/sound/soc-dai.h
> @@ -177,6 +177,7 @@ struct snd_soc_dai_ops {
>  
>  	int (*set_sdw_stream)(struct snd_soc_dai *dai,
>  			void *stream, int direction);
> +	void *(*get_sdw_stream)(struct snd_soc_dai *dai, int direction);

So who would be calling this API? Machine or someone else?

-- 
~Vinod

Re: [RFC PATCH 1/6] ASoC: core: add support to snd_soc_dai_get_sdw_stream()

[Srinivas Kandagatla]

On 10/06/2019 05:34, Vinod Koul wrote:
> On 07-06-19, 09:56, Srinivas Kandagatla wrote:
>> On platforms which have smart speaker amplifiers connected via
>> soundwire and modeled as aux devices in ASoC, in such usecases machine
>> driver should be able to get sdw master stream from dai so that it can
>> use the runtime stream to setup slave streams.
>>
>> soundwire already as a set function, get function would provide more
>> flexibility to above configurations.
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
>>   include/sound/soc-dai.h | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/include/sound/soc-dai.h b/include/sound/soc-dai.h
>> index f5d70041108f..9f90b936fd9a 100644
>> --- a/include/sound/soc-dai.h
>> +++ b/include/sound/soc-dai.h
>> @@ -177,6 +177,7 @@ struct snd_soc_dai_ops {
>>   
>>   	int (*set_sdw_stream)(struct snd_soc_dai *dai,
>>   			void *stream, int direction);
>> +	void *(*get_sdw_stream)(struct snd_soc_dai *dai, int direction);
> 
> So who would be calling this API? Machine or someone else?

 From Machine driver in my case where smart speaker amplifiers are 
modeled as aux device which is also dai less.

--srini
> 

[RFC PATCH 2/6] soundwire: Add compute_params callback

[Srinivas Kandagatla]

From: Vinod Koul <vkoul@kernel.org>

This callback allows masters to compute the bus parameters required.

Signed-off-by: Vinod Koul <vkoul@kernel.org>
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/soundwire/stream.c    | 9 +++++++++
 include/linux/soundwire/sdw.h | 2 ++
 2 files changed, 11 insertions(+)

diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
index 73c52cd4fec8..b86992145799 100644
--- a/drivers/soundwire/stream.c
+++ b/drivers/soundwire/stream.c
@@ -1485,6 +1485,15 @@ static int _sdw_prepare_stream(struct sdw_stream_runtime *stream)
 		bus->params.bandwidth += m_rt->stream->params.rate *
 			m_rt->ch_count * m_rt->stream->params.bps;
 
+		/* Compute params */
+		if (bus->compute_params) {
+			ret = bus->compute_params(bus);
+			if (ret < 0) {
+				dev_err(bus->dev, "Compute params failed: %d", ret);
+				return ret;
+			}
+		}
+
 		/* Program params */
 		ret = sdw_program_params(bus);
 		if (ret < 0) {
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index bea46bd8b6ce..aac68e879fae 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -718,6 +718,7 @@ struct sdw_master_ops {
  * Bit set implies used number, bit clear implies unused number.
  * @bus_lock: bus lock
  * @msg_lock: message lock
+ * @compute_params: points to Bus resource management implementation
  * @ops: Master callback ops
  * @port_ops: Master port callback ops
  * @params: Current bus parameters
@@ -739,6 +740,7 @@ struct sdw_bus {
 	DECLARE_BITMAP(assigned, SDW_MAX_DEVICES);
 	struct mutex bus_lock;
 	struct mutex msg_lock;
+	int (*compute_params)(struct sdw_bus *bus);
 	const struct sdw_master_ops *ops;
 	const struct sdw_master_port_ops *port_ops;
 	struct sdw_bus_params params;
-- 
2.21.0

[RFC PATCH 3/6] soundwire: core: define SDW_MAX_PORT

[Srinivas Kandagatla]

This patch adds SDW_MAX_PORT so that other driver can use it.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 include/linux/soundwire/sdw.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index aac68e879fae..80ca997e4e5d 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -36,6 +36,7 @@ struct sdw_slave;
 #define SDW_FRAME_CTRL_BITS		48
 #define SDW_MAX_DEVICES			11
 
+#define SDW_MAX_PORTS	14
 #define SDW_VALID_PORT_RANGE(n)		((n) <= 14 && (n) >= 1)
 
 #define SDW_DAI_ID_RANGE_START		100
-- 
2.21.0

Re: [alsa-devel] [RFC PATCH 3/6] soundwire: core: define SDW_MAX_PORT

[Pierre-Louis Bossart]

On 6/7/19 3:56 AM, Srinivas Kandagatla wrote:
> This patch adds SDW_MAX_PORT so that other driver can use it.
> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
>   include/linux/soundwire/sdw.h | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
> index aac68e879fae..80ca997e4e5d 100644
> --- a/include/linux/soundwire/sdw.h
> +++ b/include/linux/soundwire/sdw.h
> @@ -36,6 +36,7 @@ struct sdw_slave;
>   #define SDW_FRAME_CTRL_BITS		48
>   #define SDW_MAX_DEVICES			11
>   
> +#define SDW_MAX_PORTS	14

That's an ambiguous definition.
You can have 16 ports per the SoundWire spec, but DP0 is reserved for 
control and DP15 is an alias for all ports (same idea as device 15 for 
broadcast operations but limited to a single device), which leaves 14 
ports for audio usages.

In the MIPI specs, specifically the DisCo part, the difference is called 
about with the the DP0 and DPn notations, so this could be SDW_MAX_DPn. 
Alternatively you could use SDW_MAX_AUDIO_PORTS which is more 
self-explanatory and does not require in-depth familiarity with the spec.

>   #define SDW_VALID_PORT_RANGE(n)		((n) <= 14 && (n) >= 1)
>   
>   #define SDW_DAI_ID_RANGE_START		100
> 

Re: [alsa-devel] [RFC PATCH 3/6] soundwire: core: define SDW_MAX_PORT

[Cezary Rojewski]

On 2019-06-07 14:31, Pierre-Louis Bossart wrote:
> On 6/7/19 3:56 AM, Srinivas Kandagatla wrote:
>> This patch adds SDW_MAX_PORT so that other driver can use it.
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
>>   include/linux/soundwire/sdw.h | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/include/linux/soundwire/sdw.h 
>> b/include/linux/soundwire/sdw.h
>> index aac68e879fae..80ca997e4e5d 100644
>> --- a/include/linux/soundwire/sdw.h
>> +++ b/include/linux/soundwire/sdw.h
>> @@ -36,6 +36,7 @@ struct sdw_slave;
>>   #define SDW_FRAME_CTRL_BITS        48
>>   #define SDW_MAX_DEVICES            11
>> +#define SDW_MAX_PORTS    14
> 
> That's an ambiguous definition.
> You can have 16 ports per the SoundWire spec, but DP0 is reserved for 
> control and DP15 is an alias for all ports (same idea as device 15 for 
> broadcast operations but limited to a single device), which leaves 14 
> ports for audio usages.
> 
> In the MIPI specs, specifically the DisCo part, the difference is called 
> about with the the DP0 and DPn notations, so this could be SDW_MAX_DPn. 
> Alternatively you could use SDW_MAX_AUDIO_PORTS which is more 
> self-explanatory and does not require in-depth familiarity with the spec.
> 

This ambiguity spreads even further. Look at the name of #define below.
DP0 is by no means invalid. It's specific and has some optional 
registers, yes, but that's because of its engagement in BPT.

Given the fact SDW does not care about type of data being transported, 
even "AUDIO" seems misleading here. Though it's still better than no 
specifier at all.

>>   #define SDW_VALID_PORT_RANGE(n)        ((n) <= 14 && (n) >= 1)
>>   #define SDW_DAI_ID_RANGE_START        100
>>
> 

[RFC PATCH 4/6] soundwire: stream: make stream name a const pointer

[Srinivas Kandagatla]

Make stream name const pointer

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/soundwire/stream.c    | 2 +-
 include/linux/soundwire/sdw.h | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
index b86992145799..8da1a8d2dac1 100644
--- a/drivers/soundwire/stream.c
+++ b/drivers/soundwire/stream.c
@@ -863,7 +863,7 @@ EXPORT_SYMBOL(sdw_release_stream);
  * sdw_alloc_stream should be called only once per stream. Typically
  * invoked from ALSA/ASoC machine/platform driver.
  */
-struct sdw_stream_runtime *sdw_alloc_stream(char *stream_name)
+struct sdw_stream_runtime *sdw_alloc_stream(const char *stream_name)
 {
 	struct sdw_stream_runtime *stream;
 
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index 80ca997e4e5d..457be7d09a4a 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -831,7 +831,7 @@ struct sdw_stream_params {
  * @m_rt_count: Count of Master runtime(s) in this stream
  */
 struct sdw_stream_runtime {
-	char *name;
+	const char *name;
 	struct sdw_stream_params params;
 	enum sdw_stream_state state;
 	enum sdw_stream_type type;
@@ -839,7 +839,7 @@ struct sdw_stream_runtime {
 	int m_rt_count;
 };
 
-struct sdw_stream_runtime *sdw_alloc_stream(char *stream_name);
+struct sdw_stream_runtime *sdw_alloc_stream(const char *stream_name);
 void sdw_release_stream(struct sdw_stream_runtime *stream);
 int sdw_stream_add_master(struct sdw_bus *bus,
 		struct sdw_stream_config *stream_config,
-- 
2.21.0

[RFC PATCH 5/6] dt-bindings: soundwire: add bindings for Qcom controller

[Srinivas Kandagatla]

This patch adds bindings for Qualcomm soundwire controller.

Qualcomm SoundWire Master controller is present in most Qualcomm SoCs
either integrated as part of WCD audio codecs via slimbus or
as part of SOC I/O.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 .../bindings/soundwire/qcom,swr.txt           | 62 +++++++++++++++++++
 1 file changed, 62 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soundwire/qcom,swr.txt

diff --git a/Documentation/devicetree/bindings/soundwire/qcom,swr.txt b/Documentation/devicetree/bindings/soundwire/qcom,swr.txt
new file mode 100644
index 000000000000..eb84d0f4f36f
--- /dev/null
+++ b/Documentation/devicetree/bindings/soundwire/qcom,swr.txt
@@ -0,0 +1,62 @@
+Qualcomm SoundWire Controller
+
+This binding describes the Qualcomm SoundWire Controller Bindings.
+
+Required properties:
+
+- compatible:		Must be "qcom,soundwire-v<MAJOR>.<MINOR>.<STEP>",
+	 		example:
+			"qcom,soundwire-v1.3.0"
+			"qcom,soundwire-v1.5.0"
+			"qcom,soundwire-v1.6.0"
+- reg:			SoundWire controller address space.
+- interrupts:		SoundWire controller interrupt.
+- clock-names:		Must contain "iface".
+- clocks:		Interface clocks needed for controller.
+- #sound-dai-cells:	Must be 1 for digital audio interfaces on the controllers.
+- #address-cells:	Must be 1 for SoundWire devices;
+- #size-cells:		Must be <0> as SoundWire addresses have no size component.
+- qcom,dout-ports: 	Must be count of data out ports
+- qcom,din-ports: 	Must be count of data in ports
+- qcom,ports-offset1:	Must be frame offset1 of each data port.
+			Out followed by In. Used for Block size calculation.
+- qcom,ports-offset2: 	Must be frame offset2 of each data port.
+			Out followed by In. Used for Block size calculation.
+- qcom,ports-sinterval-low: Must be sample interval low of each data port.
+			Out followed by In. Used for Sample Interval calculation.
+
+= SoundWire devices
+Each subnode of the bus represents SoundWire device attached to it.
+The properties of these nodes are defined by the individual bindings.
+
+= EXAMPLE
+The following example represents a SoundWire controller on DB845c board
+which has controller integrated inside WCD934x codec on SDM845 SoC.
+
+soundwire: soundwire@c85 {
+	compatible = "qcom,soundwire-v1.3.0";
+	reg = <0xc85 0x20>;
+	interrupts = <20 IRQ_TYPE_EDGE_RISING>;
+	clocks = <&wcc>;
+	clock-names = "iface";
+	#sound-dai-cells = <1>;
+	#address-cells = <1>;
+	#size-cells = <0>;
+	qcom,dout-ports	= <6>;
+	qcom,din-ports	= <2>;
+	qcom,ports-sinterval-low =/bits/ 8  <0x07 0x1F 0x3F 0x7 0x1F 0x3F 0x0F 0x0F>;
+	qcom,ports-offset1 = /bits/ 8 <0x01 0x02 0x0C 0x6 0x12 0x0D 0x07 0x0A >;
+	qcom,ports-offset2 = /bits/ 8 <0x00 0x00 0x1F 0x00 0x00 0x1F 0x00 0x00>;
+
+	/* Left Speaker */
+	wsa8810@1{
+		....
+		reg = <1>;
+	};
+
+	/* Right Speaker */
+	wsa8810@2{
+		....
+		reg = <2>;
+	};
+};
-- 
2.21.0

Re: [alsa-devel] [RFC PATCH 5/6] dt-bindings: soundwire: add bindings for Qcom controller

[Pierre-Louis Bossart]

On 6/7/19 3:56 AM, Srinivas Kandagatla wrote:
> This patch adds bindings for Qualcomm soundwire controller.
> 
> Qualcomm SoundWire Master controller is present in most Qualcomm SoCs
> either integrated as part of WCD audio codecs via slimbus or
> as part of SOC I/O.
> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
>   .../bindings/soundwire/qcom,swr.txt           | 62 +++++++++++++++++++
>   1 file changed, 62 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/soundwire/qcom,swr.txt

you seem to use the 'swr' prefix in this patch. Most implementers use 
'sdw', and that's the default also used in the MIPI DisCo spec for 
properties. Can we align on the same naming conventions?

> 
> diff --git a/Documentation/devicetree/bindings/soundwire/qcom,swr.txt b/Documentation/devicetree/bindings/soundwire/qcom,swr.txt
> new file mode 100644
> index 000000000000..eb84d0f4f36f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soundwire/qcom,swr.txt
> @@ -0,0 +1,62 @@
> +Qualcomm SoundWire Controller
> +
> +This binding describes the Qualcomm SoundWire Controller Bindings.
> +
> +Required properties:
> +
> +- compatible:		Must be "qcom,soundwire-v<MAJOR>.<MINOR>.<STEP>",
> +	 		example:
> +			"qcom,soundwire-v1.3.0"
> +			"qcom,soundwire-v1.5.0"
> +			"qcom,soundwire-v1.6.0"
> +- reg:			SoundWire controller address space.
> +- interrupts:		SoundWire controller interrupt.
> +- clock-names:		Must contain "iface".
> +- clocks:		Interface clocks needed for controller.
> +- #sound-dai-cells:	Must be 1 for digital audio interfaces on the controllers.
> +- #address-cells:	Must be 1 for SoundWire devices;
> +- #size-cells:		Must be <0> as SoundWire addresses have no size component.
> +- qcom,dout-ports: 	Must be count of data out ports
> +- qcom,din-ports: 	Must be count of data in ports
> +- qcom,ports-offset1:	Must be frame offset1 of each data port.
> +			Out followed by In. Used for Block size calculation.
> +- qcom,ports-offset2: 	Must be frame offset2 of each data port.
> +			Out followed by In. Used for Block size calculation.
> +- qcom,ports-sinterval-low: Must be sample interval low of each data port.
> +			Out followed by In. Used for Sample Interval calculation.

These definitions are valid only for specific types of ports, I believe 
here it's a 'reduced' port since offset2 is not required for simpler 
ports and you don't have Hstart/Hstop.

so if you state that all of these properties are required, you are 
explicitly ruling out future implementations of simple ports or will 
have to redefine them later.

Also the definition 'frame offset1/2' is incorrect. the offset is 
defined within each Payload Transport Window - not each frame - and its 
definition depends on the packing mode used, which isn't defined or 
stated here.

And last it looks like you assume a fixed frame shape - likely 50 rows 
by 8 columns, it might be worth adding a note on the max values for 
offset1/2 implied by this frame shape.

> +
> += SoundWire devices
> +Each subnode of the bus represents SoundWire device attached to it.
> +The properties of these nodes are defined by the individual bindings.
> +
> += EXAMPLE
> +The following example represents a SoundWire controller on DB845c board
> +which has controller integrated inside WCD934x codec on SDM845 SoC.
> +
> +soundwire: soundwire@c85 {
> +	compatible = "qcom,soundwire-v1.3.0";
> +	reg = <0xc85 0x20>;
> +	interrupts = <20 IRQ_TYPE_EDGE_RISING>;
> +	clocks = <&wcc>;
> +	clock-names = "iface";
> +	#sound-dai-cells = <1>;
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +	qcom,dout-ports	= <6>;
> +	qcom,din-ports	= <2>;
> +	qcom,ports-sinterval-low =/bits/ 8  <0x07 0x1F 0x3F 0x7 0x1F 0x3F 0x0F 0x0F>;
> +	qcom,ports-offset1 = /bits/ 8 <0x01 0x02 0x0C 0x6 0x12 0x0D 0x07 0x0A >;
> +	qcom,ports-offset2 = /bits/ 8 <0x00 0x00 0x1F 0x00 0x00 0x1F 0x00 0x00>;
> +
> +	/* Left Speaker */
> +	wsa8810@1{
> +		....
> +		reg = <1>;
> +	};
> +
> +	/* Right Speaker */
> +	wsa8810@2{
> +		....
> +		reg = <2>;
> +	};
> +};
> 

Re: [alsa-devel] [RFC PATCH 5/6] dt-bindings: soundwire: add bindings for Qcom controller

[Srinivas Kandagatla]

Thanks for taking time to review,

On 07/06/2019 13:50, Pierre-Louis Bossart wrote:
> On 6/7/19 3:56 AM, Srinivas Kandagatla wrote:
>> This patch adds bindings for Qualcomm soundwire controller.
>>
>> Qualcomm SoundWire Master controller is present in most Qualcomm SoCs
>> either integrated as part of WCD audio codecs via slimbus or
>> as part of SOC I/O.
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
>>   .../bindings/soundwire/qcom,swr.txt           | 62 +++++++++++++++++++
>>   1 file changed, 62 insertions(+)
>>   create mode 100644 
>> Documentation/devicetree/bindings/soundwire/qcom,swr.txt
> 
> you seem to use the 'swr' prefix in this patch. Most implementers use 
> 'sdw', and that's the default also used in the MIPI DisCo spec for 
> properties. Can we align on the same naming conventions?
> 
>>
>> diff --git a/Documentation/devicetree/bindings/soundwire/qcom,swr.txt 
>> b/Documentation/devicetree/bindings/soundwire/qcom,swr.txt
>> new file mode 100644
>> index 000000000000..eb84d0f4f36f
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/soundwire/qcom,swr.txt
>> @@ -0,0 +1,62 @@
>> +Qualcomm SoundWire Controller
>> +
>> +This binding describes the Qualcomm SoundWire Controller Bindings.
>> +
>> +Required properties:
>> +
>> +- compatible:        Must be "qcom,soundwire-v<MAJOR>.<MINOR>.<STEP>",
>> +             example:
>> +            "qcom,soundwire-v1.3.0"
>> +            "qcom,soundwire-v1.5.0"
>> +            "qcom,soundwire-v1.6.0"
>> +- reg:            SoundWire controller address space.
>> +- interrupts:        SoundWire controller interrupt.
>> +- clock-names:        Must contain "iface".
>> +- clocks:        Interface clocks needed for controller.
>> +- #sound-dai-cells:    Must be 1 for digital audio interfaces on the 
>> controllers.
>> +- #address-cells:    Must be 1 for SoundWire devices;
>> +- #size-cells:        Must be <0> as SoundWire addresses have no size 
>> component.
>> +- qcom,dout-ports:     Must be count of data out ports
>> +- qcom,din-ports:     Must be count of data in ports
>> +- qcom,ports-offset1:    Must be frame offset1 of each data port.
>> +            Out followed by In. Used for Block size calculation.
>> +- qcom,ports-offset2:     Must be frame offset2 of each data port.
>> +            Out followed by In. Used for Block size calculation.
>> +- qcom,ports-sinterval-low: Must be sample interval low of each data 
>> port.
>> +            Out followed by In. Used for Sample Interval calculation.
> 
> These definitions are valid only for specific types of ports, I believe 
> here it's a 'reduced' port since offset2 is not required for simpler 
> ports and you don't have Hstart/Hstop.
> 
Yes, this version of the controller which am working on does not have 
DPN_SampleCtrl2 register for SampleIntervalHigh Field and has registers 
for programming offset2 does indeed indicate that these ports are 
reduced ports.

However looks like new versions of the this controller does support full 
data ports.

I can add more flexibility in bindings by adding qcom,dport-type field 
indicating this in next version.

> so if you state that all of these properties are required, you are 
> explicitly ruling out future implementations of simple ports or will 
> have to redefine them later.
> 
> Also the definition 'frame offset1/2' is incorrect. the offset is 
> defined within each Payload Transport Window - not each frame - and its 
> definition depends on the packing mode used, which isn't defined or 
> stated here.

Yep, BlockPackingMode is missing. 1.3 version of this controller only 
supports Block Per Port Block mode.

I guess this can be derived with in the driver by using compatible 
string, I can add some notes in the binding to make this more explicit.
I will also reword offset1/2 description to include transport window 
within frame

> 
> And last it looks like you assume a fixed frame shape - likely 50 rows 
> by 8 columns, it might be worth adding a note on the max values for 
> offset1/2 implied by this frame shape.

Its 48x16 in this case.


Thanks,
srini

Re: [RFC PATCH 5/6] dt-bindings: soundwire: add bindings for Qcom controller

[Vinod Koul]

On 07-06-19, 09:56, Srinivas Kandagatla wrote:
> This patch adds bindings for Qualcomm soundwire controller.
> 
> Qualcomm SoundWire Master controller is present in most Qualcomm SoCs
> either integrated as part of WCD audio codecs via slimbus or
> as part of SOC I/O.
> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
>  .../bindings/soundwire/qcom,swr.txt           | 62 +++++++++++++++++++

So I was expecting to see bus support patches for OF first. I think you
have missed those changes. Please do include those in v2 with bindings
and OF support for bus.

>  1 file changed, 62 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/soundwire/qcom,swr.txt
> 
> diff --git a/Documentation/devicetree/bindings/soundwire/qcom,swr.txt b/Documentation/devicetree/bindings/soundwire/qcom,swr.txt
> new file mode 100644
> index 000000000000..eb84d0f4f36f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soundwire/qcom,swr.txt
> @@ -0,0 +1,62 @@
> +Qualcomm SoundWire Controller
> +
> +This binding describes the Qualcomm SoundWire Controller Bindings.
> +
> +Required properties:
> +
> +- compatible:		Must be "qcom,soundwire-v<MAJOR>.<MINOR>.<STEP>",
> +	 		example:
> +			"qcom,soundwire-v1.3.0"
> +			"qcom,soundwire-v1.5.0"
> +			"qcom,soundwire-v1.6.0"
> +- reg:			SoundWire controller address space.
> +- interrupts:		SoundWire controller interrupt.
> +- clock-names:		Must contain "iface".
> +- clocks:		Interface clocks needed for controller.
> +- #sound-dai-cells:	Must be 1 for digital audio interfaces on the controllers.
> +- #address-cells:	Must be 1 for SoundWire devices;
> +- #size-cells:		Must be <0> as SoundWire addresses have no size component.
> +- qcom,dout-ports: 	Must be count of data out ports
> +- qcom,din-ports: 	Must be count of data in ports

On these I think we should have specified dpn properties as specified in
DisCo, but then looking at spec we do not define that for master, but
bus seems to have it.

Pierre do you why master does not have dpn properties in DisCo?

> +- qcom,ports-offset1:	Must be frame offset1 of each data port.
> +			Out followed by In. Used for Block size calculation.
> +- qcom,ports-offset2: 	Must be frame offset2 of each data port.
> +			Out followed by In. Used for Block size calculation.
> +- qcom,ports-sinterval-low: Must be sample interval low of each data port.
> +			Out followed by In. Used for Sample Interval calculation.

These are software so do not belong here
-- 
~Vinod

Re: [RFC PATCH 5/6] dt-bindings: soundwire: add bindings for Qcom controller

[Srinivas Kandagatla]

On 10/06/2019 05:51, Vinod Koul wrote:
> On 07-06-19, 09:56, Srinivas Kandagatla wrote:
>> This patch adds bindings for Qualcomm soundwire controller.
>>
>> Qualcomm SoundWire Master controller is present in most Qualcomm SoCs
>> either integrated as part of WCD audio codecs via slimbus or
>> as part of SOC I/O.
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
>>   .../bindings/soundwire/qcom,swr.txt           | 62 +++++++++++++++++++
> 
> So I was expecting to see bus support patches for OF first. I think you
> have missed those changes. Please do include those in v2 with bindings
> and OF support for bus.

I was planning to post bus bindings along with soundwire slave bindings 
of WSA881x driver!
I will be sending this soon!

> 
>>   1 file changed, 62 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/soundwire/qcom,swr.txt
>>
>> diff --git a/Documentation/devicetree/bindings/soundwire/qcom,swr.txt b/Documentation/devicetree/bindings/soundwire/qcom,swr.txt
>> new file mode 100644
>> index 000000000000..eb84d0f4f36f
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/soundwire/qcom,swr.txt
>> @@ -0,0 +1,62 @@
>> +Qualcomm SoundWire Controller
>> +
>> +This binding describes the Qualcomm SoundWire Controller Bindings.
>> +
>> +Required properties:
>> +
>> +- compatible:		Must be "qcom,soundwire-v<MAJOR>.<MINOR>.<STEP>",
>> +	 		example:
>> +			"qcom,soundwire-v1.3.0"
>> +			"qcom,soundwire-v1.5.0"
>> +			"qcom,soundwire-v1.6.0"
>> +- reg:			SoundWire controller address space.
>> +- interrupts:		SoundWire controller interrupt.
>> +- clock-names:		Must contain "iface".
>> +- clocks:		Interface clocks needed for controller.
>> +- #sound-dai-cells:	Must be 1 for digital audio interfaces on the controllers.
>> +- #address-cells:	Must be 1 for SoundWire devices;
>> +- #size-cells:		Must be <0> as SoundWire addresses have no size component.
>> +- qcom,dout-ports: 	Must be count of data out ports
>> +- qcom,din-ports: 	Must be count of data in ports
> 
> On these I think we should have specified dpn properties as specified in
> DisCo, but then looking at spec we do not define that for master, but
> bus seems to have it.
> 
> Pierre do you why master does not have dpn properties in DisCo?
> 
>> +- qcom,ports-offset1:	Must be frame offset1 of each data port.
>> +			Out followed by In. Used for Block size calculation.
>> +- qcom,ports-offset2: 	Must be frame offset2 of each data port.
>> +			Out followed by In. Used for Block size calculation.
>> +- qcom,ports-sinterval-low: Must be sample interval low of each data port.
>> +			Out followed by In. Used for Sample Interval calculation.
> 
> These are software so do not belong here
These are board-specfic properties w.r.t this controller ports 
configuration.
I don't see any other option to specify this?
Do you have any alternative suggestions?

--srini
> 

Re: [alsa-devel] [RFC PATCH 5/6] dt-bindings: soundwire: add bindings for Qcom controller

[Pierre-Louis Bossart]

>> diff --git a/Documentation/devicetree/bindings/soundwire/qcom,swr.txt b/Documentation/devicetree/bindings/soundwire/qcom,swr.txt
>> new file mode 100644
>> index 000000000000..eb84d0f4f36f
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/soundwire/qcom,swr.txt
>> @@ -0,0 +1,62 @@
>> +Qualcomm SoundWire Controller
>> +
>> +This binding describes the Qualcomm SoundWire Controller Bindings.
>> +
>> +Required properties:
>> +
>> +- compatible:		Must be "qcom,soundwire-v<MAJOR>.<MINOR>.<STEP>",
>> +	 		example:
>> +			"qcom,soundwire-v1.3.0"
>> +			"qcom,soundwire-v1.5.0"
>> +			"qcom,soundwire-v1.6.0"
>> +- reg:			SoundWire controller address space.
>> +- interrupts:		SoundWire controller interrupt.
>> +- clock-names:		Must contain "iface".
>> +- clocks:		Interface clocks needed for controller.
>> +- #sound-dai-cells:	Must be 1 for digital audio interfaces on the controllers.
>> +- #address-cells:	Must be 1 for SoundWire devices;
>> +- #size-cells:		Must be <0> as SoundWire addresses have no size component.
>> +- qcom,dout-ports: 	Must be count of data out ports
>> +- qcom,din-ports: 	Must be count of data in ports
> 
> On these I think we should have specified dpn properties as specified in
> DisCo, but then looking at spec we do not define that for master, but
> bus seems to have it.
> 
> Pierre do you why master does not have dpn properties in DisCo?

Because there are no DP0 or DPn registers defined for Masters in the 
SoundWire 1.x spec. DisCo is about specifying properties for standard 
registers, when they are not standard vendor extensions need to come 
into play.

> 
>> +- qcom,ports-offset1:	Must be frame offset1 of each data port.
>> +			Out followed by In. Used for Block size calculation.
>> +- qcom,ports-offset2: 	Must be frame offset2 of each data port.
>> +			Out followed by In. Used for Block size calculation.
>> +- qcom,ports-sinterval-low: Must be sample interval low of each data port.
>> +			Out followed by In. Used for Sample Interval calculation.
> 
> These are software so do not belong here

Not necessarily. They define the allocation expected on that link and I 
see no problem specifying those values here. It's the moral equivalent 
of specifying which TDM slots and the bit depth of one slot you'd use 
for DSP_A/B.
And if you push back, then what would be your alternate proposal on 
where those values might be stored? This is a very specific usage of the 
link and it makes sense to me to have the information in firmware - 
exposed with properties - rather than hard-coded in a pretend bus 
allocation routine in the kernel. Either the allocation is fully dynamic 
and it's handled by the kernel or it's static and it's better to store 
it in firmware to deal with platform-to-platform variations.

[RFC PATCH 6/6] soundwire: qcom: add support for SoundWire controller

[Srinivas Kandagatla]

Qualcomm SoundWire Master controller is present in most Qualcomm SoCs
either integrated as part of WCD audio codecs via slimbus or
as part of SOC I/O.

This patchset adds support to a very basic controller which has been
tested with WCD934x SoundWire controller connected to WSA881x smart
speaker amplifiers.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/soundwire/Kconfig  |   9 +
 drivers/soundwire/Makefile |   4 +
 drivers/soundwire/qcom.c   | 983 +++++++++++++++++++++++++++++++++++++
 3 files changed, 996 insertions(+)
 create mode 100644 drivers/soundwire/qcom.c

diff --git a/drivers/soundwire/Kconfig b/drivers/soundwire/Kconfig
index 53b55b79c4af..f44d4f36dbbb 100644
--- a/drivers/soundwire/Kconfig
+++ b/drivers/soundwire/Kconfig
@@ -34,4 +34,13 @@ config SOUNDWIRE_INTEL
 	  enable this config option to get the SoundWire support for that
 	  device.
 
+config SOUNDWIRE_QCOM
+	tristate "Qualcomm SoundWire Master driver"
+	select SOUNDWIRE_BUS
+	depends on SND_SOC
+	help
+	  SoundWire Qualcomm Master driver.
+	  If you have an Qualcomm platform which has a SoundWire Master then
+	  enable this config option to get the SoundWire support for that
+	  device
 endif
diff --git a/drivers/soundwire/Makefile b/drivers/soundwire/Makefile
index 5817beaca0e1..f4ebfde31372 100644
--- a/drivers/soundwire/Makefile
+++ b/drivers/soundwire/Makefile
@@ -16,3 +16,7 @@ obj-$(CONFIG_SOUNDWIRE_INTEL) += soundwire-intel.o
 
 soundwire-intel-init-objs := intel_init.o
 obj-$(CONFIG_SOUNDWIRE_INTEL) += soundwire-intel-init.o
+
+#Qualcomm driver
+soundwire-qcom-objs :=	qcom.o
+obj-$(CONFIG_SOUNDWIRE_QCOM) += soundwire-qcom.o
diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
new file mode 100644
index 000000000000..d1722d44d217
--- /dev/null
+++ b/drivers/soundwire/qcom.c
@@ -0,0 +1,983 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2019, Linaro Limited
+
+#include <linux/clk.h>
+#include <linux/completion.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+#include <linux/slimbus.h>
+#include <linux/soundwire/sdw.h>
+#include <linux/soundwire/sdw_registers.h>
+#include <sound/pcm_params.h>
+#include <sound/soc.h>
+#include "bus.h"
+
+#define SWRM_COMP_HW_VERSION					0x00
+#define SWRM_COMP_CFG_ADDR					0x04
+#define SWRM_COMP_CFG_IRQ_LEVEL_OR_PULSE_MSK			BIT(1)
+#define SWRM_COMP_CFG_ENABLE_MSK				BIT(0)
+#define SWRM_COMP_PARAMS					0x100
+#define SWRM_COMP_PARAMS_DOUT_PORTS_MASK			GENMASK(4, 0)
+#define SWRM_COMP_PARAMS_DIN_PORTS_MASK				GENMASK(9, 5)
+#define SWRM_COMP_PARAMS_WR_FIFO_DEPTH				GENMASK(14, 10)
+#define SWRM_COMP_PARAMS_RD_FIFO_DEPTH				GENMASK(19, 15)
+#define SWRM_COMP_PARAMS_AUTO_ENUM_SLAVES			GENMASK(32. 20)
+#define SWRM_INTERRUPT_STATUS					0x200
+#define SWRM_INTERRUPT_STATUS_RMSK				GENMASK(16, 0)
+#define SWRM_INTERRUPT_STATUS_SLAVE_PEND_IRQ			BIT(0)
+#define SWRM_INTERRUPT_STATUS_NEW_SLAVE_ATTACHED		BIT(1)
+#define SWRM_INTERRUPT_STATUS_CHANGE_ENUM_SLAVE_STATUS		BIT(2)
+#define SWRM_INTERRUPT_STATUS_MASTER_CLASH_DET			BIT(3)
+#define SWRM_INTERRUPT_STATUS_RD_FIFO_OVERFLOW			BIT(4)
+#define SWRM_INTERRUPT_STATUS_RD_FIFO_UNDERFLOW			BIT(5)
+#define SWRM_INTERRUPT_STATUS_WR_CMD_FIFO_OVERFLOW		BIT(6)
+#define SWRM_INTERRUPT_STATUS_CMD_ERROR				BIT(7)
+#define SWRM_INTERRUPT_STATUS_DOUT_PORT_COLLISION		BIT(8)
+#define SWRM_INTERRUPT_STATUS_READ_EN_RD_VALID_MISMATCH		BIT(9)
+#define SWRM_INTERRUPT_STATUS_SPECIAL_CMD_ID_FINISHED		BIT(10)
+#define SWRM_INTERRUPT_STATUS_NEW_SLAVE_AUTO_ENUM_FINISHED	BIT(11)
+#define SWRM_INTERRUPT_STATUS_AUTO_ENUM_FAILED			BIT(12)
+#define SWRM_INTERRUPT_STATUS_AUTO_ENUM_TABLE_IS_FULL		BIT(13)
+#define SWRM_INTERRUPT_STATUS_BUS_RESET_FINISHED		BIT(14)
+#define SWRM_INTERRUPT_STATUS_CLK_STOP_FINISHED			BIT(15)
+#define SWRM_INTERRUPT_STATUS_ERROR_PORT_TEST			BIT(16)
+#define SWRM_INTERRUPT_MASK_ADDR				0x204
+#define SWRM_INTERRUPT_CLEAR					0x208
+#define SWRM_CMD_FIFO_WR_CMD					0x300
+#define SWRM_CMD_FIFO_RD_CMD					0x304
+#define SWRM_CMD_FIFO_CMD					0x308
+#define SWRM_CMD_FIFO_STATUS					0x30C
+#define SWRM_CMD_FIFO_CFG_ADDR					0x314
+#define SWRM_CMD_FIFO_CFG_NUM_OF_CMD_RETRY_SHFT			0x0
+#define SWRM_CMD_FIFO_RD_FIFO_ADDR				0x318
+#define SWRM_ENUMERATOR_CFG_ADDR				0x500
+#define SWRM_MCP_FRAME_CTRL_BANK_ADDR(m)		(0x101C + 0x40 * (m))
+#define SWRM_MCP_FRAME_CTRL_BANK_SSP_PERIOD_SHFT		16
+#define SWRM_MCP_FRAME_CTRL_BANK_ROW_CTRL_SHFT			3
+#define SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_BMSK			GENMASK(2, 0)
+#define SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_SHFT			0
+#define SWRM_MCP_CFG_ADDR					0x1048
+#define SWRM_MCP_CFG_MAX_NUM_OF_CMD_NO_PINGS_BMSK		GENMASK(21, 17)
+#define SWRM_MCP_CFG_MAX_NUM_OF_CMD_NO_PINGS_SHFT		0x11
+#define SWRM_MCP_STATUS						0x104C
+#define SWRM_MCP_STATUS_BANK_NUM_MASK				BIT(0)
+#define SWRM_MCP_SLV_STATUS					0x1090
+#define SWRM_MCP_SLV_STATUS_MASK				GENMASK(1, 0)
+#define SWRM_DP_PORT_CTRL_BANK(n, m)	(0x1124 + 0x100 * (n - 1) + 0x40 * m)
+#define SWRM_DP_PORT_CTRL2_BANK(n, m)	(0x1126 + 0x100 * (n - 1) + 0x40 * m)
+#define SWRM_DP_PORT_CTRL_EN_CHAN_SHFT				0x18
+#define SWRM_DP_PORT_CTRL_OFFSET2_SHFT				0x10
+#define SWRM_DP_PORT_CTRL_OFFSET1_SHFT				0x08
+#define SWRM_AHB_BRIDGE_WR_DATA_0				0xc885
+#define SWRM_AHB_BRIDGE_WR_ADDR_0				0xc889
+#define SWRM_AHB_BRIDGE_RD_ADDR_0				0xc88d
+#define SWRM_AHB_BRIDGE_RD_DATA_0				0xc891
+
+#define SWRM_REG_VAL_PACK(data, dev, id, reg)	\
+			((reg) | ((id) << 16) | ((dev) << 20) | ((data) << 24))
+
+#define SWRM_MAX_ROW_VAL	0 /* Rows = 48 */
+#define SWRM_DEFAULT_ROWS	48
+#define SWRM_MIN_COL_VAL	0 /* Cols = 2 */
+#define SWRM_DEFAULT_COL	16
+#define SWRM_SPECIAL_CMD_ID	0xF
+#define MAX_FREQ_NUM		1
+#define TIMEOUT_MS		1000
+#define QCOM_SWRM_MAX_RD_LEN	0xf
+#define DEFAULT_CLK_FREQ	9600000
+#define SWRM_MAX_DAIS		0xF
+
+struct qcom_swrm_port_config {
+	u8 si;
+	u8 off1;
+	u8 off2;
+};
+
+struct qcom_swrm_ctrl {
+	struct sdw_bus bus;
+	struct device *dev;
+	struct regmap *regmap;
+	struct completion sp_cmd_comp;
+	struct work_struct slave_work;
+	/* read/write lock */
+	struct mutex lock;
+	/* Port alloc/free lock */
+	struct mutex port_lock;
+	struct clk *hclk;
+	int fifo_status;
+	void __iomem *base;
+	u8 wr_cmd_id;
+	u8 rd_cmd_id;
+	int irq;
+	unsigned int version;
+	int num_din_ports;
+	int num_dout_ports;
+	unsigned long dout_port_mask;
+	unsigned long din_port_mask;
+	struct qcom_swrm_port_config pconfig[SDW_MAX_PORTS];
+	struct sdw_stream_runtime *sruntime[SWRM_MAX_DAIS];
+	enum sdw_slave_status status[SDW_MAX_DEVICES];
+	u32 (*reg_read)(struct qcom_swrm_ctrl *ctrl, int reg);
+	int (*reg_write)(struct qcom_swrm_ctrl *ctrl, int reg, int val);
+};
+
+#define to_qcom_sdw(b)	container_of(b, struct qcom_swrm_ctrl, bus)
+
+struct usecase {
+	u8 num_port;
+	u8 num_ch;
+	u32 chrate;
+};
+
+static u32 qcom_swrm_slim_reg_read(struct qcom_swrm_ctrl *ctrl, int reg)
+{
+	struct regmap *wcd_regmap = ctrl->regmap;
+	u32 val = 0, ret;
+
+	/* pg register + offset */
+	regmap_bulk_write(wcd_regmap, SWRM_AHB_BRIDGE_RD_ADDR_0,
+			  (u8 *)&reg, 4);
+
+	ret = regmap_bulk_read(wcd_regmap, SWRM_AHB_BRIDGE_RD_DATA_0,
+			       (u8 *)&val, 4);
+	if (ret < 0)
+		dev_err(ctrl->dev, "Read Failure (%d)\n", ret);
+
+	return val;
+}
+
+static u32 qcom_swrm_mmio_reg_read(struct qcom_swrm_ctrl *ctrl, int reg)
+{
+	return readl_relaxed(ctrl->base + reg);
+}
+
+static int qcom_swrm_mmio_reg_write(struct qcom_swrm_ctrl *ctrl,
+				    int reg, int val)
+{
+	writel_relaxed(val, ctrl->base + reg);
+
+	return 0;
+}
+
+static int qcom_swrm_slim_reg_write(struct qcom_swrm_ctrl *ctrl,
+				    int reg, int val)
+{
+	struct regmap *wcd_regmap = ctrl->regmap;
+
+	/* pg register + offset */
+	regmap_bulk_write(wcd_regmap, SWRM_AHB_BRIDGE_WR_DATA_0,
+			  (u8 *)&val, 4);
+	/* write address register */
+	regmap_bulk_write(wcd_regmap, SWRM_AHB_BRIDGE_WR_ADDR_0,
+			  (u8 *)&reg, 4);
+
+	return 0;
+}
+
+static int qcom_swrm_cmd_fifo_wr_cmd(struct qcom_swrm_ctrl *ctrl, u8 cmd_data,
+				     u8 dev_addr, u16 reg_addr)
+{
+	int ret = 0;
+	u8 cmd_id;
+	u32 val;
+
+	mutex_lock(&ctrl->lock);
+	if (dev_addr == SDW_BROADCAST_DEV_NUM) {
+		cmd_id = SWRM_SPECIAL_CMD_ID;
+	} else {
+		if (++ctrl->wr_cmd_id == SWRM_SPECIAL_CMD_ID)
+			ctrl->wr_cmd_id = 0;
+
+		cmd_id = ctrl->wr_cmd_id;
+	}
+
+	val = SWRM_REG_VAL_PACK(cmd_data, dev_addr, cmd_id, reg_addr);
+	ret = ctrl->reg_write(ctrl, SWRM_CMD_FIFO_WR_CMD, val);
+	if (ret < 0) {
+		dev_err(ctrl->dev, "%s: reg 0x%x write failed, err:%d\n",
+			__func__, val, ret);
+		goto err;
+	}
+
+	if (dev_addr == SDW_BROADCAST_DEV_NUM) {
+		ctrl->fifo_status = 0;
+		ret = wait_for_completion_timeout(&ctrl->sp_cmd_comp,
+						  msecs_to_jiffies(TIMEOUT_MS));
+
+		if (!ret || ctrl->fifo_status) {
+			dev_err(ctrl->dev, "reg 0x%x write failed\n", val);
+			ret = -ENODATA;
+			goto err;
+		}
+	}
+err:
+	mutex_unlock(&ctrl->lock);
+	return ret;
+}
+
+static int qcom_swrm_cmd_fifo_rd_cmd(struct qcom_swrm_ctrl *ctrl,
+				     u8 dev_addr, u16 reg_addr,
+				     u32 len, u8 *rval)
+{
+	int i, ret = 0;
+	u8 cmd_id = 0;
+	u32 val;
+
+	mutex_lock(&ctrl->lock);
+	if (dev_addr == SDW_ENUM_DEV_NUM) {
+		cmd_id = SWRM_SPECIAL_CMD_ID;
+	} else {
+		if (++ctrl->rd_cmd_id == SWRM_SPECIAL_CMD_ID)
+			ctrl->rd_cmd_id = 0;
+
+		cmd_id = ctrl->rd_cmd_id;
+	}
+
+	val = SWRM_REG_VAL_PACK(len, dev_addr, cmd_id, reg_addr);
+	ret = ctrl->reg_write(ctrl, SWRM_CMD_FIFO_RD_CMD, val);
+	if (ret < 0) {
+		dev_err(ctrl->dev, "reg 0x%x write failed err:%d\n", val, ret);
+		goto err;
+	}
+
+	if (dev_addr == SDW_ENUM_DEV_NUM) {
+		ctrl->fifo_status = 0;
+		ret = wait_for_completion_timeout(&ctrl->sp_cmd_comp,
+						  msecs_to_jiffies(TIMEOUT_MS));
+
+		if (!ret || ctrl->fifo_status) {
+			dev_err(ctrl->dev, "reg 0x%x read failed\n", val);
+			ret = -ENODATA;
+			goto err;
+		}
+	}
+
+	for (i = 0; i < len; i++) {
+		rval[i] = ctrl->reg_read(ctrl, SWRM_CMD_FIFO_RD_FIFO_ADDR);
+		rval[i] &= 0xFF;
+	}
+
+err:
+	mutex_unlock(&ctrl->lock);
+	return ret;
+}
+
+static void qcom_swrm_get_device_status(struct qcom_swrm_ctrl *ctrl)
+{
+	u32 val = ctrl->reg_read(ctrl, SWRM_MCP_SLV_STATUS);
+	int i;
+
+	for (i = 1; i < SDW_MAX_DEVICES; i++) {
+		u32 s;
+
+		s = (val >> (i * 2));
+		s &= SWRM_MCP_SLV_STATUS_MASK;
+		ctrl->status[i] = s;
+	}
+}
+
+static irqreturn_t qcom_swrm_irq_handler(int irq, void *dev_id)
+{
+	struct qcom_swrm_ctrl *ctrl = dev_id;
+	u32 sts, value;
+
+	sts = ctrl->reg_read(ctrl, SWRM_INTERRUPT_STATUS);
+
+	if (sts & SWRM_INTERRUPT_STATUS_SPECIAL_CMD_ID_FINISHED)
+		complete(&ctrl->sp_cmd_comp);
+
+	if (sts & SWRM_INTERRUPT_STATUS_CMD_ERROR) {
+		value = ctrl->reg_read(ctrl, SWRM_CMD_FIFO_STATUS);
+		dev_err_ratelimited(ctrl->dev,
+				    "CMD error, fifo status 0x%x\n",
+				     value);
+		ctrl->reg_write(ctrl, SWRM_CMD_FIFO_CMD, 0x1);
+		if ((value & 0xF) == 0xF) {
+			ctrl->fifo_status = -ENODATA;
+			complete(&ctrl->sp_cmd_comp);
+		}
+	}
+
+	if ((sts & SWRM_INTERRUPT_STATUS_NEW_SLAVE_ATTACHED) ||
+	    sts & SWRM_INTERRUPT_STATUS_CHANGE_ENUM_SLAVE_STATUS) {
+		if (sts & SWRM_INTERRUPT_STATUS_NEW_SLAVE_ATTACHED)
+			ctrl->status[0] = SDW_SLAVE_ATTACHED;
+
+		schedule_work(&ctrl->slave_work);
+	}
+
+	if (sts & SWRM_INTERRUPT_STATUS_SLAVE_PEND_IRQ)
+		dev_dbg(ctrl->dev, "Slave pend irq\n");
+
+	if (sts & SWRM_INTERRUPT_STATUS_NEW_SLAVE_ATTACHED)
+		dev_dbg(ctrl->dev, "New slave attached\n");
+
+	if (sts & SWRM_INTERRUPT_STATUS_MASTER_CLASH_DET)
+		dev_err_ratelimited(ctrl->dev, "Bus clash detected\n");
+
+	if (sts & SWRM_INTERRUPT_STATUS_RD_FIFO_OVERFLOW)
+		dev_err(ctrl->dev, "Read FIFO overflow\n");
+
+	if (sts & SWRM_INTERRUPT_STATUS_RD_FIFO_UNDERFLOW)
+		dev_err(ctrl->dev, "Read FIFO underflow\n");
+
+	if (sts & SWRM_INTERRUPT_STATUS_WR_CMD_FIFO_OVERFLOW)
+		dev_err(ctrl->dev, "Write FIFO overflow\n");
+
+	if (sts & SWRM_INTERRUPT_STATUS_DOUT_PORT_COLLISION)
+		dev_err(ctrl->dev, "Port collision detected\n");
+
+	if (sts & SWRM_INTERRUPT_STATUS_READ_EN_RD_VALID_MISMATCH)
+		dev_err(ctrl->dev, "Read enable valid mismatch\n");
+
+	if (sts & SWRM_INTERRUPT_STATUS_SPECIAL_CMD_ID_FINISHED)
+		dev_err(ctrl->dev, "Cmd id finished\n");
+
+	if (sts & SWRM_INTERRUPT_STATUS_BUS_RESET_FINISHED)
+		dev_err(ctrl->dev, "Bus reset finished\n");
+
+	ctrl->reg_write(ctrl, SWRM_INTERRUPT_CLEAR, sts);
+
+	return IRQ_HANDLED;
+}
+
+static int qcom_swrm_init(struct qcom_swrm_ctrl *ctrl)
+{
+	u32 val;
+	u8 row_ctrl = SWRM_MAX_ROW_VAL;
+	u8 col_ctrl = SWRM_MIN_COL_VAL;
+	u8 ssp_period = 1;
+	u8 retry_cmd_num = 3;
+
+	/* Clear Rows and Cols */
+	val = ((row_ctrl << SWRM_MCP_FRAME_CTRL_BANK_ROW_CTRL_SHFT) |
+		(col_ctrl << SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_SHFT) |
+		(ssp_period << SWRM_MCP_FRAME_CTRL_BANK_SSP_PERIOD_SHFT));
+
+	ctrl->reg_write(ctrl, SWRM_MCP_FRAME_CTRL_BANK_ADDR(0), val);
+
+	/* Disable Auto enumeration */
+	ctrl->reg_write(ctrl, SWRM_ENUMERATOR_CFG_ADDR, 0);
+
+	/* Mask soundwire interrupts */
+	ctrl->reg_write(ctrl, SWRM_INTERRUPT_MASK_ADDR,
+					SWRM_INTERRUPT_STATUS_RMSK);
+
+	/* Configure No pings */
+	val = ctrl->reg_read(ctrl, SWRM_MCP_CFG_ADDR);
+
+	val &= ~SWRM_MCP_CFG_MAX_NUM_OF_CMD_NO_PINGS_BMSK;
+	val |= (0x1f << SWRM_MCP_CFG_MAX_NUM_OF_CMD_NO_PINGS_SHFT);
+	ctrl->reg_write(ctrl, SWRM_MCP_CFG_ADDR, val);
+
+	/* Configure number of retries of a read/write cmd */
+	val = (retry_cmd_num << SWRM_CMD_FIFO_CFG_NUM_OF_CMD_RETRY_SHFT);
+	ctrl->reg_write(ctrl, SWRM_CMD_FIFO_CFG_ADDR, val);
+
+	/* Set IRQ to PULSE */
+	ctrl->reg_write(ctrl, SWRM_COMP_CFG_ADDR,
+				SWRM_COMP_CFG_IRQ_LEVEL_OR_PULSE_MSK |
+				SWRM_COMP_CFG_ENABLE_MSK);
+	return 0;
+}
+
+static enum sdw_command_response qcom_swrm_xfer_msg(struct sdw_bus *bus,
+						    struct sdw_msg *msg)
+{
+	struct qcom_swrm_ctrl *ctrl = to_qcom_sdw(bus);
+	int ret, i, len;
+
+	if (msg->flags == SDW_MSG_FLAG_READ) {
+		for (i = 0; i < msg->len;) {
+			if ((msg->len - i) < QCOM_SWRM_MAX_RD_LEN)
+				len = msg->len - i;
+			else
+				len = QCOM_SWRM_MAX_RD_LEN;
+
+			ret = qcom_swrm_cmd_fifo_rd_cmd(ctrl, msg->dev_num,
+							msg->addr + i, len,
+						       &msg->buf[i]);
+			if (ret < 0) {
+				if (ret == -ENODATA)
+					return SDW_CMD_IGNORED;
+
+				return ret;
+			}
+
+			i = i + len;
+		}
+	} else if (msg->flags == SDW_MSG_FLAG_WRITE) {
+		for (i = 0; i < msg->len; i++) {
+			ret = qcom_swrm_cmd_fifo_wr_cmd(ctrl, msg->buf[i],
+							msg->dev_num,
+						       msg->addr + i);
+			if (ret < 0) {
+				if (ret == -ENODATA)
+					return SDW_CMD_IGNORED;
+
+				return ret;
+			}
+		}
+	}
+
+	return SDW_CMD_OK;
+}
+
+static int qcom_swrm_pre_bank_switch(struct sdw_bus *bus)
+{
+	u32 reg = SWRM_MCP_FRAME_CTRL_BANK_ADDR(bus->params.next_bank);
+	struct qcom_swrm_ctrl *ctrl = to_qcom_sdw(bus);
+	u32 val;
+
+	val = ctrl->reg_read(ctrl, reg);
+	val |= ((0 << SWRM_MCP_FRAME_CTRL_BANK_ROW_CTRL_SHFT) |
+		(7l << SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_SHFT));
+	ctrl->reg_write(ctrl, reg, val);
+
+	return 0;
+}
+
+static int qcom_swrm_port_params(struct sdw_bus *bus,
+				 struct sdw_port_params *p_params,
+				unsigned int bank)
+{
+	/* TBD */
+	return 0;
+}
+
+static int qcom_swrm_transport_params(struct sdw_bus *bus,
+				      struct sdw_transport_params *params,
+				     enum sdw_reg_bank bank)
+{
+	struct qcom_swrm_ctrl *ctrl = to_qcom_sdw(bus);
+	u32 value;
+
+	value = params->offset1 << SWRM_DP_PORT_CTRL_OFFSET1_SHFT;
+	value |= params->offset2 << SWRM_DP_PORT_CTRL_OFFSET2_SHFT;
+	value |= params->sample_interval - 1;
+
+	ctrl->reg_write(ctrl, SWRM_DP_PORT_CTRL_BANK((params->port_num), bank),
+			value);
+
+	return 0;
+}
+
+static int qcom_swrm_port_enable(struct sdw_bus *bus,
+				 struct sdw_enable_ch *enable_ch,
+				unsigned int bank)
+{
+	u32 reg = SWRM_DP_PORT_CTRL_BANK(enable_ch->port_num, bank);
+	struct qcom_swrm_ctrl *ctrl = to_qcom_sdw(bus);
+	u32 val;
+
+	val = ctrl->reg_read(ctrl, reg);
+	if (enable_ch->enable)
+		val |= (enable_ch->ch_mask << SWRM_DP_PORT_CTRL_EN_CHAN_SHFT);
+	else
+		val &= ~(enable_ch->ch_mask << SWRM_DP_PORT_CTRL_EN_CHAN_SHFT);
+
+	ctrl->reg_write(ctrl, reg, val);
+
+	return 0;
+}
+
+static struct sdw_master_port_ops qcom_swrm_port_ops = {
+	.dpn_set_port_params = qcom_swrm_port_params,
+	.dpn_set_port_transport_params = qcom_swrm_transport_params,
+	.dpn_port_enable_ch = qcom_swrm_port_enable,
+};
+
+static struct sdw_master_ops qcom_swrm_ops = {
+	.xfer_msg = qcom_swrm_xfer_msg,
+	.pre_bank_switch = qcom_swrm_pre_bank_switch,
+};
+
+static int qcom_swrm_compute_params(struct sdw_bus *bus)
+{
+	struct qcom_swrm_ctrl *ctrl = to_qcom_sdw(bus);
+	struct sdw_master_runtime *m_rt;
+	struct sdw_slave_runtime *s_rt;
+	struct sdw_port_runtime *p_rt;
+	int i = 0;
+
+	list_for_each_entry(m_rt, &bus->m_rt_list, bus_node) {
+		list_for_each_entry(p_rt, &m_rt->port_list, port_node) {
+			p_rt->transport_params.port_num = p_rt->num;
+			p_rt->transport_params.sample_interval =
+					ctrl->pconfig[p_rt->num - 1].si + 1;
+			p_rt->transport_params.offset1 =
+					ctrl->pconfig[p_rt->num - 1].off1;
+			p_rt->transport_params.offset2 =
+					ctrl->pconfig[p_rt->num - 1].off2;
+		}
+
+		list_for_each_entry(s_rt, &m_rt->slave_rt_list, m_rt_node) {
+			list_for_each_entry(p_rt, &s_rt->port_list, port_node) {
+				p_rt->transport_params.port_num = p_rt->num;
+				p_rt->transport_params.sample_interval =
+						ctrl->pconfig[i].si + 1;
+				p_rt->transport_params.offset1 =
+						ctrl->pconfig[i].off1;
+				p_rt->transport_params.offset2 =
+						ctrl->pconfig[i].off2;
+				i++;
+			}
+		}
+	}
+
+	return 0;
+}
+
+static u32 qcom_swrm_freq_tbl[MAX_FREQ_NUM] = {
+	DEFAULT_CLK_FREQ,
+};
+
+static void qcom_swrm_slave_wq(struct work_struct *work)
+{
+	struct qcom_swrm_ctrl *ctrl =
+			container_of(work, struct qcom_swrm_ctrl, slave_work);
+
+	qcom_swrm_get_device_status(ctrl);
+	sdw_handle_slave_status(&ctrl->bus, ctrl->status);
+}
+
+static int qcom_swrm_prepare(struct snd_pcm_substream *substream,
+			     struct snd_soc_dai *dai)
+{
+	struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(dai->dev);
+
+	if (!ctrl->sruntime[dai->id])
+		return -EINVAL;
+
+	return sdw_enable_stream(ctrl->sruntime[dai->id]);
+}
+
+static void qcom_swrm_stream_free_ports(struct qcom_swrm_ctrl *ctrl,
+					struct sdw_stream_runtime *stream)
+{
+	struct sdw_master_runtime *m_rt;
+	struct sdw_port_runtime *p_rt;
+	unsigned long *port_mask;
+
+	mutex_lock(&ctrl->port_lock);
+
+	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
+		if (m_rt->direction == SDW_DATA_DIR_RX)
+			port_mask = &ctrl->dout_port_mask;
+		else
+			port_mask = &ctrl->din_port_mask;
+
+		list_for_each_entry(p_rt, &m_rt->port_list, port_node) {
+			clear_bit(p_rt->num - 1, port_mask);
+		}
+	}
+
+	mutex_unlock(&ctrl->port_lock);
+}
+
+static int qcom_swrm_stream_alloc_ports(struct qcom_swrm_ctrl *ctrl,
+					struct sdw_stream_runtime *stream,
+				       struct snd_pcm_hw_params *params,
+				       int direction)
+{
+	struct sdw_port_config pconfig[SDW_MAX_PORTS];
+	struct sdw_stream_config sconfig;
+	struct sdw_master_runtime *m_rt;
+	struct sdw_slave_runtime *s_rt;
+	struct sdw_port_runtime *p_rt;
+	unsigned long *port_mask;
+	int i, maxport, pn, nports = 0, ret = 0;
+
+	mutex_lock(&ctrl->port_lock);
+	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
+		if (m_rt->direction == SDW_DATA_DIR_RX) {
+			maxport = ctrl->num_dout_ports;
+			port_mask = &ctrl->dout_port_mask;
+		} else {
+			maxport = ctrl->num_din_ports;
+			port_mask = &ctrl->din_port_mask;
+		}
+
+		list_for_each_entry(s_rt, &m_rt->slave_rt_list, m_rt_node) {
+			list_for_each_entry(p_rt, &s_rt->port_list, port_node) {
+				/* Port numbers start from 1 - 14*/
+				pn = find_first_zero_bit(port_mask, maxport);
+				if (pn > (maxport - 1)) {
+					dev_err(ctrl->dev, "All ports busy\n");
+					ret = -EBUSY;
+					goto err;
+				}
+				set_bit(pn, port_mask);
+				pconfig[nports].num = pn + 1;
+				pconfig[nports].ch_mask = p_rt->ch_mask;
+				nports++;
+			}
+		}
+	}
+
+	if (direction == SNDRV_PCM_STREAM_CAPTURE)
+		sconfig.direction = SDW_DATA_DIR_TX;
+	else
+		sconfig.direction = SDW_DATA_DIR_RX;
+
+	sconfig.ch_count = 1;
+	sconfig.frame_rate = params_rate(params);
+	sconfig.type = stream->type;
+	sconfig.bps = 1;
+	sdw_stream_add_master(&ctrl->bus, &sconfig, pconfig,
+			      nports, stream);
+err:
+	if (ret) {
+		for (i = 0; i < nports; i++)
+			clear_bit(pconfig[i].num - 1, port_mask);
+	}
+
+	mutex_unlock(&ctrl->port_lock);
+
+	return ret;
+}
+
+static int qcom_swrm_hw_params(struct snd_pcm_substream *substream,
+			       struct snd_pcm_hw_params *params,
+			      struct snd_soc_dai *dai)
+{
+	struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(dai->dev);
+	int ret;
+
+	ret = qcom_swrm_stream_alloc_ports(ctrl, ctrl->sruntime[dai->id],
+					   params, substream->stream);
+	if (ret)
+		return ret;
+
+	return sdw_prepare_stream(ctrl->sruntime[dai->id]);
+}
+
+static int qcom_swrm_hw_free(struct snd_pcm_substream *substream,
+			     struct snd_soc_dai *dai)
+{
+	struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(dai->dev);
+
+	qcom_swrm_stream_free_ports(ctrl, ctrl->sruntime[dai->id]);
+	sdw_stream_remove_master(&ctrl->bus, ctrl->sruntime[dai->id]);
+	sdw_deprepare_stream(ctrl->sruntime[dai->id]);
+	sdw_disable_stream(ctrl->sruntime[dai->id]);
+
+	return 0;
+}
+
+static void *qcom_pdm_get_sdw_stream(struct snd_soc_dai *dai,
+				     int direction)
+{
+	struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(dai->dev);
+
+	return ctrl->sruntime[dai->id];
+}
+
+static int qcom_pdm_set_sdw_stream(struct snd_soc_dai *dai,
+				   void *stream, int direction)
+{
+	return 0;
+}
+
+static int qcom_swrm_startup(struct snd_pcm_substream *stream,
+			     struct snd_soc_dai *dai)
+{
+	struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(dai->dev);
+	struct sdw_stream_runtime *sruntime;
+
+	sruntime = sdw_alloc_stream(dai->name);
+	if (!sruntime)
+		return -ENOMEM;
+
+	ctrl->sruntime[dai->id] = sruntime;
+
+	return 0;
+}
+
+static void qcom_swrm_shutdown(struct snd_pcm_substream *stream,
+			       struct snd_soc_dai *dai)
+{
+	struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(dai->dev);
+
+	sdw_release_stream(ctrl->sruntime[dai->id]);
+	ctrl->sruntime[dai->id] = NULL;
+}
+
+static const struct snd_soc_dai_ops qcom_swrm_pdm_dai_ops = {
+	.hw_params = qcom_swrm_hw_params,
+	.prepare = qcom_swrm_prepare,
+	.hw_free = qcom_swrm_hw_free,
+	.startup = qcom_swrm_startup,
+	.shutdown = qcom_swrm_shutdown,
+	.set_sdw_stream = qcom_pdm_set_sdw_stream,
+	.get_sdw_stream = qcom_pdm_get_sdw_stream,
+};
+
+static const struct snd_soc_component_driver qcom_swrm_dai_component = {
+	.name           = "soundwire",
+};
+
+static int qcom_swrm_register_dais(struct qcom_swrm_ctrl *ctrl)
+{
+	int num_dais = ctrl->num_dout_ports + ctrl->num_din_ports;
+	struct snd_soc_dai_driver *dais;
+	int i;
+
+	/* PDM dais are only tested for now */
+	dais = devm_kcalloc(ctrl->dev, num_dais, sizeof(*dais), GFP_KERNEL);
+	if (!dais)
+		return -ENOMEM;
+
+	for (i = 0; i < num_dais; i++) {
+		dais[i].name = kasprintf(GFP_KERNEL, "SDW Pin%d", i);
+		if (i < ctrl->num_dout_ports) {
+			dais[i].playback.stream_name = kasprintf(GFP_KERNEL,
+								 "SDW Tx%d", i);
+			if (!dais[i].playback.stream_name) {
+				kfree(dais[i].name);
+				return -ENOMEM;
+			}
+			dais[i].playback.channels_min = 1;
+			dais[i].playback.channels_max = 1;
+			dais[i].playback.rates = SNDRV_PCM_RATE_48000;
+			dais[i].playback.formats = SNDRV_PCM_FMTBIT_S16_LE;
+		} else {
+			dais[i].capture.stream_name = kasprintf(GFP_KERNEL,
+								"SDW Rx%d", i);
+			if (!dais[i].capture.stream_name) {
+				kfree(dais[i].name);
+				kfree(dais[i].playback.stream_name);
+				return -ENOMEM;
+			}
+
+			dais[i].capture.channels_min = 1;
+			dais[i].capture.channels_max = 1;
+			dais[i].capture.rates = SNDRV_PCM_RATE_48000;
+			dais[i].capture.formats = SNDRV_PCM_FMTBIT_S16_LE;
+		}
+		dais[i].ops = &qcom_swrm_pdm_dai_ops;
+		dais[i].id = i;
+	}
+
+	return devm_snd_soc_register_component(ctrl->dev,
+						&qcom_swrm_dai_component,
+						dais, num_dais);
+}
+
+static int qcom_swrm_get_port_config(struct qcom_swrm_ctrl *ctrl)
+{
+	struct device_node *np = ctrl->dev->of_node;
+	u8 off1[SDW_MAX_PORTS];
+	u8 off2[SDW_MAX_PORTS];
+	u8 si[SDW_MAX_PORTS];
+	int i, ret, nports, val;
+
+	val = ctrl->reg_read(ctrl, SWRM_COMP_PARAMS);
+	ctrl->num_dout_ports = val & SWRM_COMP_PARAMS_DOUT_PORTS_MASK;
+	ctrl->num_din_ports = (val & SWRM_COMP_PARAMS_DIN_PORTS_MASK) >> 5;
+
+	ret = of_property_read_u32(np, "qcom,din-ports", &val);
+	if (ret)
+		return ret;
+
+	if (val > ctrl->num_din_ports)
+		return -EINVAL;
+
+	ctrl->num_din_ports = val;
+
+	ret = of_property_read_u32(np, "qcom,dout-ports", &val);
+	if (ret)
+		return ret;
+
+	if (val > ctrl->num_dout_ports)
+		return -EINVAL;
+
+	ctrl->num_dout_ports = val;
+
+	nports = ctrl->num_dout_ports + ctrl->num_din_ports;
+
+	ret = of_property_read_u8_array(np, "qcom,ports-offset1",
+					off1, nports);
+	if (ret)
+		return ret;
+
+	ret = of_property_read_u8_array(np, "qcom,ports-offset2",
+					off2, nports);
+	if (ret)
+		return ret;
+
+	ret = of_property_read_u8_array(np, "qcom,ports-sinterval-low",
+					si, nports);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < nports; i++) {
+		ctrl->pconfig[i].si = si[i];
+		ctrl->pconfig[i].off1 = off1[i];
+		ctrl->pconfig[i].off2 = off2[i];
+	}
+
+	return 0;
+}
+
+static int qcom_swrm_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct sdw_master_prop *prop;
+	struct sdw_bus_params *params;
+	struct qcom_swrm_ctrl *ctrl;
+	struct resource *res;
+	int ret;
+
+	ctrl = devm_kzalloc(dev, sizeof(*ctrl), GFP_KERNEL);
+	if (!ctrl)
+		return -ENOMEM;
+
+	if (dev->parent->bus == &slimbus_bus) {
+		ctrl->reg_read = qcom_swrm_slim_reg_read;
+		ctrl->reg_write = qcom_swrm_slim_reg_write;
+		ctrl->regmap = dev_get_regmap(dev->parent, NULL);
+		if (!ctrl->regmap)
+			return -EINVAL;
+	} else {
+		ctrl->reg_read = qcom_swrm_mmio_reg_read;
+		ctrl->reg_write = qcom_swrm_mmio_reg_write;
+		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+		ctrl->base = devm_ioremap_resource(dev, res);
+		if (IS_ERR(ctrl->base))
+			return PTR_ERR(ctrl->base);
+	}
+
+	ctrl->irq = platform_get_irq(pdev, 0);
+	if (ctrl->irq < 0)
+		return ctrl->irq;
+
+	ctrl->hclk = devm_clk_get(dev, "iface");
+	if (IS_ERR(ctrl->hclk))
+		return PTR_ERR(ctrl->hclk);
+
+	clk_prepare_enable(ctrl->hclk);
+
+	ctrl->dev = dev;
+	dev_set_drvdata(&pdev->dev, ctrl);
+	init_completion(&ctrl->sp_cmd_comp);
+	mutex_init(&ctrl->lock);
+	mutex_init(&ctrl->port_lock);
+	INIT_WORK(&ctrl->slave_work, qcom_swrm_slave_wq);
+
+	ctrl->bus.dev = dev;
+	ctrl->bus.ops = &qcom_swrm_ops;
+	ctrl->bus.port_ops = &qcom_swrm_port_ops;
+	ctrl->bus.compute_params = &qcom_swrm_compute_params;
+
+	ret = qcom_swrm_get_port_config(ctrl);
+	if (ret)
+		return ret;
+
+	params = &ctrl->bus.params;
+	params->max_dr_freq = DEFAULT_CLK_FREQ;
+	params->curr_dr_freq = DEFAULT_CLK_FREQ;
+	params->col = SWRM_DEFAULT_COL;
+	params->row = SWRM_DEFAULT_ROWS;
+	params->curr_bank = ctrl->reg_read(ctrl, SWRM_MCP_STATUS) &
+				SWRM_MCP_STATUS_BANK_NUM_MASK;
+	params->next_bank = !params->curr_bank;
+
+	prop = &ctrl->bus.prop;
+	prop->max_clk_freq = DEFAULT_CLK_FREQ;
+	prop->num_clk_gears = 0;
+	prop->num_clk_freq = MAX_FREQ_NUM;
+	prop->clk_freq = &qcom_swrm_freq_tbl[0];
+	prop->default_col = SWRM_DEFAULT_COL;
+	prop->default_row = SWRM_DEFAULT_ROWS;
+
+	ctrl->version = ctrl->reg_read(ctrl, SWRM_COMP_HW_VERSION);
+
+	ret = devm_request_threaded_irq(dev, ctrl->irq, NULL,
+					qcom_swrm_irq_handler,
+					IRQF_TRIGGER_RISING,
+					"soundwire", ctrl);
+	if (ret) {
+		dev_err(dev, "Failed to request soundwire irq\n");
+		goto err;
+	}
+
+	ret = sdw_add_bus_master(&ctrl->bus);
+	if (ret) {
+		dev_err(dev, "Failed to register Soundwire controller (%d)\n",
+			ret);
+		goto err;
+	}
+
+	qcom_swrm_init(ctrl);
+	ret = qcom_swrm_register_dais(ctrl);
+	if (ret)
+		goto err;
+
+	dev_info(dev, "Qualcomm Soundwire controller v%x.%x.%x Registered\n",
+		 (ctrl->version >> 24) & 0xff, (ctrl->version >> 16) & 0xff,
+		 ctrl->version & 0xffff);
+
+	return 0;
+err:
+	clk_disable_unprepare(ctrl->hclk);
+	return ret;
+}
+
+static int qcom_swrm_remove(struct platform_device *pdev)
+{
+	struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(&pdev->dev);
+
+	sdw_delete_bus_master(&ctrl->bus);
+	clk_disable_unprepare(ctrl->hclk);
+
+	return 0;
+}
+
+static int qcom_swrm_runtime_suspend(struct device *device)
+{
+	/* TBD */
+	return 0;
+}
+
+static int qcom_swrm_runtime_resume(struct device *device)
+{
+	/* TBD */
+	return 0;
+}
+
+static const struct dev_pm_ops qcom_swrm_dev_pm_ops = {
+	SET_RUNTIME_PM_OPS(qcom_swrm_runtime_suspend,
+			   qcom_swrm_runtime_resume,
+			   NULL
+	)
+};
+
+static const struct of_device_id qcom_swrm_of_match[] = {
+	{ .compatible = "qcom,soundwire-v1.3.0",},
+	{ .compatible = "qcom,soundwire-v1.5.0",},
+	{ .compatible = "qcom,soundwire-v1.6.0",},
+	{/* sentinel */},
+};
+
+MODULE_DEVICE_TABLE(of, qcom_swrm_of_match);
+
+static struct platform_driver qcom_swrm_driver = {
+	.probe	= &qcom_swrm_probe,
+	.remove = &qcom_swrm_remove,
+	.driver = {
+		.name	= "qcom-soundwire",
+		.of_match_table = qcom_swrm_of_match,
+		.pm = &qcom_swrm_dev_pm_ops,
+	}
+};
+module_platform_driver(qcom_swrm_driver);
+
+MODULE_DESCRIPTION("Qualcomm soundwire driver");
+MODULE_LICENSE("GPL v2");
-- 
2.21.0

Re: [alsa-devel] [RFC PATCH 6/6] soundwire: qcom: add support for SoundWire controller

[Pierre-Louis Bossart]

> +config SOUNDWIRE_QCOM
> +	tristate "Qualcomm SoundWire Master driver"
> +	select SOUNDWIRE_BUS
> +	depends on SND_SOC

depends on SLIMBUS if you need the SlimBus link to talk to your 
SoundWire Master?

Also depends on device tree since you use of_ functions?

> +#define SWRM_COMP_HW_VERSION					0x00

Can we please use SDW_ or QCOM_SDW_ as prefix?

> +#define SWRM_INTERRUPT_STATUS_NEW_SLAVE_AUTO_ENUM_FINISHED	BIT(11)
> +#define SWRM_INTERRUPT_STATUS_AUTO_ENUM_FAILED			BIT(12)
> +#define SWRM_INTERRUPT_STATUS_AUTO_ENUM_TABLE_IS_FULL		BIT(13)

This hints at hardware support to assign Device Numbers automagically so 
will likely have impacts on the bus driver code, no?


> +#define SWRM_MAX_ROW_VAL	0 /* Rows = 48 */
> +#define SWRM_DEFAULT_ROWS	48
> +#define SWRM_MIN_COL_VAL	0 /* Cols = 2 */
> +#define SWRM_DEFAULT_COL	16
> +#define SWRM_SPECIAL_CMD_ID	0xF
> +#define MAX_FREQ_NUM		1
> +#define TIMEOUT_MS		1000
> +#define QCOM_SWRM_MAX_RD_LEN	0xf
> +#define DEFAULT_CLK_FREQ	9600000

The clocks and frame shape don't match usual expectations for PDM.
For a 9.6 MHz support, you would typically use 8 columns and 50 rows to 
transport PDM with a 50x oversampling. I've never seen anyone use 48x 
for PDM.

> +#define SWRM_MAX_DAIS		0xF
> +
> +struct qcom_swrm_port_config {
> +	u8 si;
> +	u8 off1;
> +	u8 off2;
> +};
> +
> +struct qcom_swrm_ctrl {
> +	struct sdw_bus bus;
> +	struct device *dev;
> +	struct regmap *regmap;
> +	struct completion sp_cmd_comp;
> +	struct work_struct slave_work;
> +	/* read/write lock */
> +	struct mutex lock;
> +	/* Port alloc/free lock */
> +	struct mutex port_lock;
> +	struct clk *hclk;
> +	int fifo_status;
> +	void __iomem *base;
> +	u8 wr_cmd_id;
> +	u8 rd_cmd_id;
> +	int irq;
> +	unsigned int version;
> +	int num_din_ports;
> +	int num_dout_ports;
> +	unsigned long dout_port_mask;
> +	unsigned long din_port_mask;
> +	struct qcom_swrm_port_config pconfig[SDW_MAX_PORTS];

this is not necessarily correct. the initial definitions for 
SDW_MAX_PORTS was for Slave devices. There is no definitions for Masters 
in the SoundWire spec, so you could use whatever constant you want for 
your hardware.

> +	struct sdw_stream_runtime *sruntime[SWRM_MAX_DAIS];
> +	enum sdw_slave_status status[SDW_MAX_DEVICES];
> +	u32 (*reg_read)(struct qcom_swrm_ctrl *ctrl, int reg);
> +	int (*reg_write)(struct qcom_swrm_ctrl *ctrl, int reg, int val);
> +};
> +
> +#define to_qcom_sdw(b)	container_of(b, struct qcom_swrm_ctrl, bus)
> +
> +struct usecase {
> +	u8 num_port;
> +	u8 num_ch;
> +	u32 chrate;
> +};

this structure doesn't seem to be used?

> +static int qcom_swrm_cmd_fifo_wr_cmd(struct qcom_swrm_ctrl *ctrl, u8 cmd_data,
> +				     u8 dev_addr, u16 reg_addr)
> +{
> +	int ret = 0;
> +	u8 cmd_id;
> +	u32 val;
> +
> +	mutex_lock(&ctrl->lock);
> +	if (dev_addr == SDW_BROADCAST_DEV_NUM) {
> +		cmd_id = SWRM_SPECIAL_CMD_ID;
> +	} else {
> +		if (++ctrl->wr_cmd_id == SWRM_SPECIAL_CMD_ID)
> +			ctrl->wr_cmd_id = 0;
> +
> +		cmd_id = ctrl->wr_cmd_id;
> +	}

might be worth having a helper/macro since you are doing the same thing 
below.

> +
> +	val = SWRM_REG_VAL_PACK(cmd_data, dev_addr, cmd_id, reg_addr);
> +	ret = ctrl->reg_write(ctrl, SWRM_CMD_FIFO_WR_CMD, val);
> +	if (ret < 0) {
> +		dev_err(ctrl->dev, "%s: reg 0x%x write failed, err:%d\n",
> +			__func__, val, ret);
> +		goto err;
> +	}
> +
> +	if (dev_addr == SDW_BROADCAST_DEV_NUM) {
> +		ctrl->fifo_status = 0;
> +		ret = wait_for_completion_timeout(&ctrl->sp_cmd_comp,
> +						  msecs_to_jiffies(TIMEOUT_MS));

This is odd. The SoundWire spec does not handle writes to a single 
device or broadcast writes differently. I don't see a clear reason why 
you would only timeout for a broadcast write.

> +
> +		if (!ret || ctrl->fifo_status) {
> +			dev_err(ctrl->dev, "reg 0x%x write failed\n", val);
> +			ret = -ENODATA;
> +			goto err;
> +		}
> +	}
> +err:
> +	mutex_unlock(&ctrl->lock);
> +	return ret;
> +}
> +
> +static int qcom_swrm_cmd_fifo_rd_cmd(struct qcom_swrm_ctrl *ctrl,
> +				     u8 dev_addr, u16 reg_addr,
> +				     u32 len, u8 *rval)
> +{
> +	int i, ret = 0;
> +	u8 cmd_id = 0;
> +	u32 val;
> +
> +	mutex_lock(&ctrl->lock);
> +	if (dev_addr == SDW_ENUM_DEV_NUM) {
> +		cmd_id = SWRM_SPECIAL_CMD_ID;
> +	} else {
> +		if (++ctrl->rd_cmd_id == SWRM_SPECIAL_CMD_ID)
> +			ctrl->rd_cmd_id = 0;
> +
> +		cmd_id = ctrl->rd_cmd_id;
> +	}

helper?

> +	val = SWRM_REG_VAL_PACK(len, dev_addr, cmd_id, reg_addr);
> +	ret = ctrl->reg_write(ctrl, SWRM_CMD_FIFO_RD_CMD, val);
> +	if (ret < 0) {
> +		dev_err(ctrl->dev, "reg 0x%x write failed err:%d\n", val, ret);
> +		goto err;
> +	}
> +
> +	if (dev_addr == SDW_ENUM_DEV_NUM) {
> +		ctrl->fifo_status = 0;
> +		ret = wait_for_completion_timeout(&ctrl->sp_cmd_comp,
> +						  msecs_to_jiffies(TIMEOUT_MS));
> 
same comment here, there isn't a clear reason to only timeout for a read 
from device0.

> +		if (!ret || ctrl->fifo_status) {
> +			dev_err(ctrl->dev, "reg 0x%x read failed\n", val);
> +			ret = -ENODATA;
> +			goto err;
> +		}
> +	}
> +
> +	for (i = 0; i < len; i++) {
> +		rval[i] = ctrl->reg_read(ctrl, SWRM_CMD_FIFO_RD_FIFO_ADDR);
> +		rval[i] &= 0xFF;
> +	}
> +
> +err:
> +	mutex_unlock(&ctrl->lock);
> +	return ret;
> +}
> +
> +static void qcom_swrm_get_device_status(struct qcom_swrm_ctrl *ctrl)
> +{
> +	u32 val = ctrl->reg_read(ctrl, SWRM_MCP_SLV_STATUS);
> +	int i;
> +
> +	for (i = 1; i < SDW_MAX_DEVICES; i++) {
> +		u32 s;
> +
> +		s = (val >> (i * 2));
> +		s &= SWRM_MCP_SLV_STATUS_MASK;
> +		ctrl->status[i] = s;
> +	}
> +}
> +
> +static irqreturn_t qcom_swrm_irq_handler(int irq, void *dev_id)
> +{
> +	struct qcom_swrm_ctrl *ctrl = dev_id;
> +	u32 sts, value;
> +
> +	sts = ctrl->reg_read(ctrl, SWRM_INTERRUPT_STATUS);
> +
> +	if (sts & SWRM_INTERRUPT_STATUS_SPECIAL_CMD_ID_FINISHED)
> +		complete(&ctrl->sp_cmd_comp);
> +
> +	if (sts & SWRM_INTERRUPT_STATUS_CMD_ERROR) {
> +		value = ctrl->reg_read(ctrl, SWRM_CMD_FIFO_STATUS);
> +		dev_err_ratelimited(ctrl->dev,
> +				    "CMD error, fifo status 0x%x\n",
> +				     value);
> +		ctrl->reg_write(ctrl, SWRM_CMD_FIFO_CMD, 0x1);
> +		if ((value & 0xF) == 0xF) {
> +			ctrl->fifo_status = -ENODATA;
> +			complete(&ctrl->sp_cmd_comp);
> +		}
> +	}
> +
> +	if ((sts & SWRM_INTERRUPT_STATUS_NEW_SLAVE_ATTACHED) ||
> +	    sts & SWRM_INTERRUPT_STATUS_CHANGE_ENUM_SLAVE_STATUS) {
> +		if (sts & SWRM_INTERRUPT_STATUS_NEW_SLAVE_ATTACHED)
> +			ctrl->status[0] = SDW_SLAVE_ATTACHED;
> +
> +		schedule_work(&ctrl->slave_work);
> +	}
> +
> +	if (sts & SWRM_INTERRUPT_STATUS_SLAVE_PEND_IRQ)
> +		dev_dbg(ctrl->dev, "Slave pend irq\n");
> +
> +	if (sts & SWRM_INTERRUPT_STATUS_NEW_SLAVE_ATTACHED)
> +		dev_dbg(ctrl->dev, "New slave attached\n");
> +
> +	if (sts & SWRM_INTERRUPT_STATUS_MASTER_CLASH_DET)
> +		dev_err_ratelimited(ctrl->dev, "Bus clash detected\n");
> +
> +	if (sts & SWRM_INTERRUPT_STATUS_RD_FIFO_OVERFLOW)
> +		dev_err(ctrl->dev, "Read FIFO overflow\n");
> +
> +	if (sts & SWRM_INTERRUPT_STATUS_RD_FIFO_UNDERFLOW)
> +		dev_err(ctrl->dev, "Read FIFO underflow\n");
> +
> +	if (sts & SWRM_INTERRUPT_STATUS_WR_CMD_FIFO_OVERFLOW)
> +		dev_err(ctrl->dev, "Write FIFO overflow\n");
> +
> +	if (sts & SWRM_INTERRUPT_STATUS_DOUT_PORT_COLLISION)
> +		dev_err(ctrl->dev, "Port collision detected\n");
> +
> +	if (sts & SWRM_INTERRUPT_STATUS_READ_EN_RD_VALID_MISMATCH)
> +		dev_err(ctrl->dev, "Read enable valid mismatch\n");
> +
> +	if (sts & SWRM_INTERRUPT_STATUS_SPECIAL_CMD_ID_FINISHED)
> +		dev_err(ctrl->dev, "Cmd id finished\n");
> +
> +	if (sts & SWRM_INTERRUPT_STATUS_BUS_RESET_FINISHED)
> +		dev_err(ctrl->dev, "Bus reset finished\n");

This list is odd as well. It makes sense to only log error cases if you 
don't really know how to handle them, but a 'NEW SLAVE ATTACHED' should 
lead to an action, no?

> +
> +	ctrl->reg_write(ctrl, SWRM_INTERRUPT_CLEAR, sts);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int qcom_swrm_init(struct qcom_swrm_ctrl *ctrl)
> +{
> +	u32 val;
> +	u8 row_ctrl = SWRM_MAX_ROW_VAL;
> +	u8 col_ctrl = SWRM_MIN_COL_VAL;
> +	u8 ssp_period = 1;
> +	u8 retry_cmd_num = 3;

probably want a define for those magic values, they are quite important.

> +
> +	/* Clear Rows and Cols */
> +	val = ((row_ctrl << SWRM_MCP_FRAME_CTRL_BANK_ROW_CTRL_SHFT) |
> +		(col_ctrl << SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_SHFT) |
> +		(ssp_period << SWRM_MCP_FRAME_CTRL_BANK_SSP_PERIOD_SHFT));
> +
> +	ctrl->reg_write(ctrl, SWRM_MCP_FRAME_CTRL_BANK_ADDR(0), val);
> +
> +	/* Disable Auto enumeration */
> +	ctrl->reg_write(ctrl, SWRM_ENUMERATOR_CFG_ADDR, 0);

This goes back to my earlier comment. Do you disable this 
auto-enumeration to avoid conflicts with the existing bus management? 
That's not necessarily smart, we may want to change that bus layer to 
reduce the enumeration time if hardware can do it.

> +
> +	/* Mask soundwire interrupts */
> +	ctrl->reg_write(ctrl, SWRM_INTERRUPT_MASK_ADDR,
> +					SWRM_INTERRUPT_STATUS_RMSK);
> +
> +	/* Configure No pings */
> +	val = ctrl->reg_read(ctrl, SWRM_MCP_CFG_ADDR);

If there is any sort of PREQ signaling for Slave-initiated interrupts, 
disabling PINGs is likely a non-conformant implementation since the 
master is required to issue a PING command within 32 frames. That's also 
the only way to know if a device is attached, so additional comments are 
likely required.

> +
> +	val &= ~SWRM_MCP_CFG_MAX_NUM_OF_CMD_NO_PINGS_BMSK;
> +	val |= (0x1f << SWRM_MCP_CFG_MAX_NUM_OF_CMD_NO_PINGS_SHFT);
> +	ctrl->reg_write(ctrl, SWRM_MCP_CFG_ADDR, val);
> +
> +	/* Configure number of retries of a read/write cmd */
> +	val = (retry_cmd_num << SWRM_CMD_FIFO_CFG_NUM_OF_CMD_RETRY_SHFT);
> +	ctrl->reg_write(ctrl, SWRM_CMD_FIFO_CFG_ADDR, val);
> +
> +	/* Set IRQ to PULSE */
> +	ctrl->reg_write(ctrl, SWRM_COMP_CFG_ADDR,
> +				SWRM_COMP_CFG_IRQ_LEVEL_OR_PULSE_MSK |
> +				SWRM_COMP_CFG_ENABLE_MSK);

indentation seems off in this code?

> +	return 0;
> +}
> +
> +static enum sdw_command_response qcom_swrm_xfer_msg(struct sdw_bus *bus,
> +						    struct sdw_msg *msg)
> +{
> +	struct qcom_swrm_ctrl *ctrl = to_qcom_sdw(bus);
> +	int ret, i, len;
> +
> +	if (msg->flags == SDW_MSG_FLAG_READ) {
> +		for (i = 0; i < msg->len;) {
> +			if ((msg->len - i) < QCOM_SWRM_MAX_RD_LEN)
> +				len = msg->len - i;
> +			else
> +				len = QCOM_SWRM_MAX_RD_LEN;
> +
> +			ret = qcom_swrm_cmd_fifo_rd_cmd(ctrl, msg->dev_num,
> +							msg->addr + i, len,
> +						       &msg->buf[i]);
> +			if (ret < 0) {
> +				if (ret == -ENODATA)
> +					return SDW_CMD_IGNORED;
> +
> +				return ret;
> +			}
> +
> +			i = i + len;
> +		}
> +	} else if (msg->flags == SDW_MSG_FLAG_WRITE) {
> +		for (i = 0; i < msg->len; i++) {
> +			ret = qcom_swrm_cmd_fifo_wr_cmd(ctrl, msg->buf[i],
> +							msg->dev_num,
> +						       msg->addr + i);
> +			if (ret < 0) {
> +				if (ret == -ENODATA)
> +					return SDW_CMD_IGNORED;
> +
> +				return ret;
> +			}
> +		}
> +	}
> +
> +	return SDW_CMD_OK;
> +}
> +
> +static int qcom_swrm_pre_bank_switch(struct sdw_bus *bus)
> +{
> +	u32 reg = SWRM_MCP_FRAME_CTRL_BANK_ADDR(bus->params.next_bank);
> +	struct qcom_swrm_ctrl *ctrl = to_qcom_sdw(bus);
> +	u32 val;
> +
> +	val = ctrl->reg_read(ctrl, reg);
> +	val |= ((0 << SWRM_MCP_FRAME_CTRL_BANK_ROW_CTRL_SHFT) |
> +		(7l << SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_SHFT));

magic values, probably need a macro here?

> +	ctrl->reg_write(ctrl, reg, val);
> +
> +	return 0;
> +}
> +

> +static int qcom_swrm_register_dais(struct qcom_swrm_ctrl *ctrl)
> +{
> +	int num_dais = ctrl->num_dout_ports + ctrl->num_din_ports;
> +	struct snd_soc_dai_driver *dais;
> +	int i;
> +
> +	/* PDM dais are only tested for now */
> +	dais = devm_kcalloc(ctrl->dev, num_dais, sizeof(*dais), GFP_KERNEL);
> +	if (!dais)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < num_dais; i++) {
> +		dais[i].name = kasprintf(GFP_KERNEL, "SDW Pin%d", i);

if (!dais[i].name)

> +		if (i < ctrl->num_dout_ports) {
> +			dais[i].playback.stream_name = kasprintf(GFP_KERNEL,
> +								 "SDW Tx%d", i);
> +			if (!dais[i].playback.stream_name) {
> +				kfree(dais[i].name);
> +				return -ENOMEM;
> +			}

also need to free previously allocated memory in earlier iterations, or 
use devm_

> +			dais[i].playback.channels_min = 1;
> +			dais[i].playback.channels_max = 1;
> +			dais[i].playback.rates = SNDRV_PCM_RATE_48000;
> +			dais[i].playback.formats = SNDRV_PCM_FMTBIT_S16_LE;
> +		} else {
> +			dais[i].capture.stream_name = kasprintf(GFP_KERNEL,
> +								"SDW Rx%d", i);
> +			if (!dais[i].capture.stream_name) {
> +				kfree(dais[i].name);
> +				kfree(dais[i].playback.stream_name);
> +				return -ENOMEM;
> +			}
> +
> +			dais[i].capture.channels_min = 1;
> +			dais[i].capture.channels_max = 1;
> +			dais[i].capture.rates = SNDRV_PCM_RATE_48000;
> +			dais[i].capture.formats = SNDRV_PCM_FMTBIT_S16_LE;
> +		}
> +		dais[i].ops = &qcom_swrm_pdm_dai_ops;
> +		dais[i].id = i;
> +	}
> +
> +	return devm_snd_soc_register_component(ctrl->dev,
> +						&qcom_swrm_dai_component,
> +						dais, num_dais);
> +}

Re: [alsa-devel] [RFC PATCH 6/6] soundwire: qcom: add support for SoundWire controller

[Srinivas Kandagatla]

Thanks for taking time to review,

On 07/06/2019 14:36, Pierre-Louis Bossart wrote:
> 
>> +config SOUNDWIRE_QCOM
>> +    tristate "Qualcomm SoundWire Master driver"
>> +    select SOUNDWIRE_BUS
>> +    depends on SND_SOC
> 
> depends on SLIMBUS if you need the SlimBus link to talk to your 
> SoundWire Master?
> 
> Also depends on device tree since you use of_ functions?
> 
Thats true, will fix this in next version.
>> +#define SWRM_COMP_HW_VERSION                    0x00
> 
> Can we please use SDW_ or QCOM_SDW_ as prefix?
> 

SWRM prefix is as per the data sheet register names, If it help am happy 
to add QCOM_ prefix it.

>> +#define SWRM_INTERRUPT_STATUS_NEW_SLAVE_AUTO_ENUM_FINISHED    BIT(11)
>> +#define SWRM_INTERRUPT_STATUS_AUTO_ENUM_FAILED            BIT(12)
>> +#define SWRM_INTERRUPT_STATUS_AUTO_ENUM_TABLE_IS_FULL        BIT(13)
> 
> This hints at hardware support to assign Device Numbers automagically so 
> will likely have impacts on the bus driver code, no?

yes, this controller has autoenumeration support, however I had disabled 
this code due to the fact that we need some more support in bus driver 
to accommodate this feature.

I will explore this side once again, may be before sending next version.


> 
> 
>> +#define SWRM_MAX_ROW_VAL    0 /* Rows = 48 */
>> +#define SWRM_DEFAULT_ROWS    48
>> +#define SWRM_MIN_COL_VAL    0 /* Cols = 2 */
>> +#define SWRM_DEFAULT_COL    16
>> +#define SWRM_SPECIAL_CMD_ID    0xF
>> +#define MAX_FREQ_NUM        1
>> +#define TIMEOUT_MS        1000
>> +#define QCOM_SWRM_MAX_RD_LEN    0xf
>> +#define DEFAULT_CLK_FREQ    9600000
> 
> The clocks and frame shape don't match usual expectations for PDM.
> For a 9.6 MHz support, you would typically use 8 columns and 50 rows to 
> transport PDM with a 50x oversampling. I've never seen anyone use 48x 
> for PDM.
> 
>> +#define SWRM_MAX_DAIS        0xF
>> +
>> +struct qcom_swrm_port_config {
>> +    u8 si;
>> +    u8 off1;
>> +    u8 off2;
>> +};
>> +
>> +struct qcom_swrm_ctrl {
>> +    struct sdw_bus bus;
>> +    struct device *dev;
>> +    struct regmap *regmap;
>> +    struct completion sp_cmd_comp;
>> +    struct work_struct slave_work;
>> +    /* read/write lock */
>> +    struct mutex lock;
>> +    /* Port alloc/free lock */
>> +    struct mutex port_lock;
>> +    struct clk *hclk;
>> +    int fifo_status;
>> +    void __iomem *base;
>> +    u8 wr_cmd_id;
>> +    u8 rd_cmd_id;
>> +    int irq;
>> +    unsigned int version;
>> +    int num_din_ports;
>> +    int num_dout_ports;
>> +    unsigned long dout_port_mask;
>> +    unsigned long din_port_mask;
>> +    struct qcom_swrm_port_config pconfig[SDW_MAX_PORTS];
> 
> this is not necessarily correct. the initial definitions for 
> SDW_MAX_PORTS was for Slave devices. There is no definitions for Masters 
> in the SoundWire spec, so you could use whatever constant you want for 
> your hardware.

Yes, I can move this define to this driver specific.

> 
>> +    struct sdw_stream_runtime *sruntime[SWRM_MAX_DAIS];
>> +    enum sdw_slave_status status[SDW_MAX_DEVICES];
>> +    u32 (*reg_read)(struct qcom_swrm_ctrl *ctrl, int reg);
>> +    int (*reg_write)(struct qcom_swrm_ctrl *ctrl, int reg, int val);
>> +};
>> +
>> +#define to_qcom_sdw(b)    container_of(b, struct qcom_swrm_ctrl, bus)
>> +
>> +struct usecase {
>> +    u8 num_port;
>> +    u8 num_ch;
>> +    u32 chrate;
>> +};
> 
> this structure doesn't seem to be used?
> 
looks like left over, I will remove this.

>> +static int qcom_swrm_cmd_fifo_wr_cmd(struct qcom_swrm_ctrl *ctrl, u8 
>> cmd_data,
>> +                     u8 dev_addr, u16 reg_addr)
>> +{
>> +    int ret = 0;
>> +    u8 cmd_id;
>> +    u32 val;
>> +
>> +    mutex_lock(&ctrl->lock);
>> +    if (dev_addr == SDW_BROADCAST_DEV_NUM) {
>> +        cmd_id = SWRM_SPECIAL_CMD_ID;
>> +    } else {
>> +        if (++ctrl->wr_cmd_id == SWRM_SPECIAL_CMD_ID)
>> +            ctrl->wr_cmd_id = 0;
>> +
>> +        cmd_id = ctrl->wr_cmd_id;
>> +    }
> 
> might be worth having a helper/macro since you are doing the same thing 
> below.
> 

I will do that!

>> +
>> +    val = SWRM_REG_VAL_PACK(cmd_data, dev_addr, cmd_id, reg_addr);
>> +    ret = ctrl->reg_write(ctrl, SWRM_CMD_FIFO_WR_CMD, val);
>> +    if (ret < 0) {
>> +        dev_err(ctrl->dev, "%s: reg 0x%x write failed, err:%d\n",
>> +            __func__, val, ret);
>> +        goto err;
>> +    }
>> +
>> +    if (dev_addr == SDW_BROADCAST_DEV_NUM) {
>> +        ctrl->fifo_status = 0;
>> +        ret = wait_for_completion_timeout(&ctrl->sp_cmd_comp,
>> +                          msecs_to_jiffies(TIMEOUT_MS));
> 
> This is odd. The SoundWire spec does not handle writes to a single 
> device or broadcast writes differently. I don't see a clear reason why 
> you would only timeout for a broadcast write.
> 

There is danger of blocking here without timeout.

>> +
>> +        if (!ret || ctrl->fifo_status) {
>> +            dev_err(ctrl->dev, "reg 0x%x write failed\n", val);
>> +            ret = -ENODATA;
>> +            goto err;
>> +        }
>> +    }
>> +err:
>> +    mutex_unlock(&ctrl->lock);
>> +    return ret;
>> +}
>> +
>> +static int qcom_swrm_cmd_fifo_rd_cmd(struct qcom_swrm_ctrl *ctrl,
>> +                     u8 dev_addr, u16 reg_addr,
>> +                     u32 len, u8 *rval)
>> +{
>> +    int i, ret = 0;
>> +    u8 cmd_id = 0;
>> +    u32 val;
>> +
>> +    mutex_lock(&ctrl->lock);
>> +    if (dev_addr == SDW_ENUM_DEV_NUM) {
>> +        cmd_id = SWRM_SPECIAL_CMD_ID;
>> +    } else {
>> +        if (++ctrl->rd_cmd_id == SWRM_SPECIAL_CMD_ID)
>> +            ctrl->rd_cmd_id = 0;
>> +
>> +        cmd_id = ctrl->rd_cmd_id;
>> +    }
> 
> helper?
yes will add that.

> 
>> +    val = SWRM_REG_VAL_PACK(len, dev_addr, cmd_id, reg_addr);
>> +    ret = ctrl->reg_write(ctrl, SWRM_CMD_FIFO_RD_CMD, val);
>> +    if (ret < 0) {
>> +        dev_err(ctrl->dev, "reg 0x%x write failed err:%d\n", val, ret);
>> +        goto err;
>> +    }
>> +
>> +    if (dev_addr == SDW_ENUM_DEV_NUM) {
>> +        ctrl->fifo_status = 0;
>> +        ret = wait_for_completion_timeout(&ctrl->sp_cmd_comp,
>> +                          msecs_to_jiffies(TIMEOUT_MS));
>>
> same comment here, there isn't a clear reason to only timeout for a read 
> from device0.
> 
>> +        if (!ret || ctrl->fifo_status) {
>> +            dev_err(ctrl->dev, "reg 0x%x read failed\n", val);
>> +            ret = -ENODATA;
>> +            goto err;
>> +        }
>> +    }
>> +
>> +    for (i = 0; i < len; i++) {
>> +        rval[i] = ctrl->reg_read(ctrl, SWRM_CMD_FIFO_RD_FIFO_ADDR);
>> +        rval[i] &= 0xFF;
>> +    }
>> +
>> +err:
>> +    mutex_unlock(&ctrl->lock);
>> +    return ret;
>> +}
>> +
>> +static void qcom_swrm_get_device_status(struct qcom_swrm_ctrl *ctrl)
>> +{
>> +    u32 val = ctrl->reg_read(ctrl, SWRM_MCP_SLV_STATUS);
>> +    int i;
>> +
>> +    for (i = 1; i < SDW_MAX_DEVICES; i++) {
>> +        u32 s;
>> +
>> +        s = (val >> (i * 2));
>> +        s &= SWRM_MCP_SLV_STATUS_MASK;
>> +        ctrl->status[i] = s;
>> +    }
>> +}
>> +
>> +static irqreturn_t qcom_swrm_irq_handler(int irq, void *dev_id)
>> +{
>> +    struct qcom_swrm_ctrl *ctrl = dev_id;
>> +    u32 sts, value;
>> +
>> +    sts = ctrl->reg_read(ctrl, SWRM_INTERRUPT_STATUS);
>> +
>> +    if (sts & SWRM_INTERRUPT_STATUS_SPECIAL_CMD_ID_FINISHED)
>> +        complete(&ctrl->sp_cmd_comp);
>> +
>> +    if (sts & SWRM_INTERRUPT_STATUS_CMD_ERROR) {
>> +        value = ctrl->reg_read(ctrl, SWRM_CMD_FIFO_STATUS);
>> +        dev_err_ratelimited(ctrl->dev,
>> +                    "CMD error, fifo status 0x%x\n",
>> +                     value);
>> +        ctrl->reg_write(ctrl, SWRM_CMD_FIFO_CMD, 0x1);
>> +        if ((value & 0xF) == 0xF) {
>> +            ctrl->fifo_status = -ENODATA;
>> +            complete(&ctrl->sp_cmd_comp);
>> +        }
>> +    }
>> +
>> +    if ((sts & SWRM_INTERRUPT_STATUS_NEW_SLAVE_ATTACHED) ||
>> +        sts & SWRM_INTERRUPT_STATUS_CHANGE_ENUM_SLAVE_STATUS) {
>> +        if (sts & SWRM_INTERRUPT_STATUS_NEW_SLAVE_ATTACHED)
>> +            ctrl->status[0] = SDW_SLAVE_ATTACHED;
>> +
>> +        schedule_work(&ctrl->slave_work);
>> +    }
>> +
>> +    if (sts & SWRM_INTERRUPT_STATUS_SLAVE_PEND_IRQ)
>> +        dev_dbg(ctrl->dev, "Slave pend irq\n");
>> +
>> +    if (sts & SWRM_INTERRUPT_STATUS_NEW_SLAVE_ATTACHED)
>> +        dev_dbg(ctrl->dev, "New slave attached\n");
>> +
>> +    if (sts & SWRM_INTERRUPT_STATUS_MASTER_CLASH_DET)
>> +        dev_err_ratelimited(ctrl->dev, "Bus clash detected\n");
>> +
>> +    if (sts & SWRM_INTERRUPT_STATUS_RD_FIFO_OVERFLOW)
>> +        dev_err(ctrl->dev, "Read FIFO overflow\n");
>> +
>> +    if (sts & SWRM_INTERRUPT_STATUS_RD_FIFO_UNDERFLOW)
>> +        dev_err(ctrl->dev, "Read FIFO underflow\n");
>> +
>> +    if (sts & SWRM_INTERRUPT_STATUS_WR_CMD_FIFO_OVERFLOW)
>> +        dev_err(ctrl->dev, "Write FIFO overflow\n");
>> +
>> +    if (sts & SWRM_INTERRUPT_STATUS_DOUT_PORT_COLLISION)
>> +        dev_err(ctrl->dev, "Port collision detected\n");
>> +
>> +    if (sts & SWRM_INTERRUPT_STATUS_READ_EN_RD_VALID_MISMATCH)
>> +        dev_err(ctrl->dev, "Read enable valid mismatch\n");
>> +
>> +    if (sts & SWRM_INTERRUPT_STATUS_SPECIAL_CMD_ID_FINISHED)
>> +        dev_err(ctrl->dev, "Cmd id finished\n");
>> +
>> +    if (sts & SWRM_INTERRUPT_STATUS_BUS_RESET_FINISHED)
>> +        dev_err(ctrl->dev, "Bus reset finished\n");
> 
> This list is odd as well. It makes sense to only log error cases if you 
> don't really know how to handle them, but a 'NEW SLAVE ATTACHED' should 
> lead to an action, no?

NEW SLAVE ATTACHED interrupt already schedules action by reporting this 
to soudwire bus layer.
Some of them are leftover as part of debug, i will try to clean them up 
and see if some of them can be handled properly.
> 
>> +
>> +    ctrl->reg_write(ctrl, SWRM_INTERRUPT_CLEAR, sts);
>> +
>> +    return IRQ_HANDLED;
>> +}
>> +
>> +static int qcom_swrm_init(struct qcom_swrm_ctrl *ctrl)
>> +{
>> +    u32 val;
>> +    u8 row_ctrl = SWRM_MAX_ROW_VAL;
>> +    u8 col_ctrl = SWRM_MIN_COL_VAL;
>> +    u8 ssp_period = 1;
>> +    u8 retry_cmd_num = 3;
> 
> probably want a define for those magic values, they are quite important.

Yep, will do that!

> 
>> +
>> +    /* Clear Rows and Cols */
>> +    val = ((row_ctrl << SWRM_MCP_FRAME_CTRL_BANK_ROW_CTRL_SHFT) |
>> +        (col_ctrl << SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_SHFT) |
>> +        (ssp_period << SWRM_MCP_FRAME_CTRL_BANK_SSP_PERIOD_SHFT));
>> +
>> +    ctrl->reg_write(ctrl, SWRM_MCP_FRAME_CTRL_BANK_ADDR(0), val);
>> +
>> +    /* Disable Auto enumeration */
>> +    ctrl->reg_write(ctrl, SWRM_ENUMERATOR_CFG_ADDR, 0);
> 
> This goes back to my earlier comment. Do you disable this 
> auto-enumeration to avoid conflicts with the existing bus management? 
> That's not necessarily smart, we may want to change that bus layer to 
> reduce the enumeration time if hardware can do it.

I will explore add this support in bus befor next version.

> 
>> +
>> +    /* Mask soundwire interrupts */
>> +    ctrl->reg_write(ctrl, SWRM_INTERRUPT_MASK_ADDR,
>> +                    SWRM_INTERRUPT_STATUS_RMSK);
>> +
>> +    /* Configure No pings */
>> +    val = ctrl->reg_read(ctrl, SWRM_MCP_CFG_ADDR);
> 
> If there is any sort of PREQ signaling for Slave-initiated interrupts, 
> disabling PINGs is likely a non-conformant implementation since the 
> master is required to issue a PING command within 32 frames. That's also 
> the only way to know if a device is attached, so additional comments are 
> likely required.
This is the value of Maximum number of consiecutive read/write commands 
without ping command in between. I will try to collect more details and 
add some comments here.


> 
>> +
>> +    val &= ~SWRM_MCP_CFG_MAX_NUM_OF_CMD_NO_PINGS_BMSK;
>> +    val |= (0x1f << SWRM_MCP_CFG_MAX_NUM_OF_CMD_NO_PINGS_SHFT);
>> +    ctrl->reg_write(ctrl, SWRM_MCP_CFG_ADDR, val);
>> +
>> +    /* Configure number of retries of a read/write cmd */
>> +    val = (retry_cmd_num << SWRM_CMD_FIFO_CFG_NUM_OF_CMD_RETRY_SHFT);
>> +    ctrl->reg_write(ctrl, SWRM_CMD_FIFO_CFG_ADDR, val);
>> +
>> +    /* Set IRQ to PULSE */
>> +    ctrl->reg_write(ctrl, SWRM_COMP_CFG_ADDR,
>> +                SWRM_COMP_CFG_IRQ_LEVEL_OR_PULSE_MSK |
>> +                SWRM_COMP_CFG_ENABLE_MSK);
> 
> indentation seems off in this code?
>
Yes! I will fix this.

>> +    return 0;
>> +}
>> +
>> +static enum sdw_command_response qcom_swrm_xfer_msg(struct sdw_bus *bus,
>> +                            struct sdw_msg *msg)
>> +{
>> +    struct qcom_swrm_ctrl *ctrl = to_qcom_sdw(bus);
>> +    int ret, i, len;
>> +
>> +    if (msg->flags == SDW_MSG_FLAG_READ) {
>> +        for (i = 0; i < msg->len;) {
>> +            if ((msg->len - i) < QCOM_SWRM_MAX_RD_LEN)
>> +                len = msg->len - i;
>> +            else
>> +                len = QCOM_SWRM_MAX_RD_LEN;
>> +
>> +            ret = qcom_swrm_cmd_fifo_rd_cmd(ctrl, msg->dev_num,
>> +                            msg->addr + i, len,
>> +                               &msg->buf[i]);
>> +            if (ret < 0) {
>> +                if (ret == -ENODATA)
>> +                    return SDW_CMD_IGNORED;
>> +
>> +                return ret;
>> +            }
>> +
>> +            i = i + len;
>> +        }
>> +    } else if (msg->flags == SDW_MSG_FLAG_WRITE) {
>> +        for (i = 0; i < msg->len; i++) {
>> +            ret = qcom_swrm_cmd_fifo_wr_cmd(ctrl, msg->buf[i],
>> +                            msg->dev_num,
>> +                               msg->addr + i);
>> +            if (ret < 0) {
>> +                if (ret == -ENODATA)
>> +                    return SDW_CMD_IGNORED;
>> +
>> +                return ret;
>> +            }
>> +        }
>> +    }
>> +
>> +    return SDW_CMD_OK;
>> +}
>> +
>> +static int qcom_swrm_pre_bank_switch(struct sdw_bus *bus)
>> +{
>> +    u32 reg = SWRM_MCP_FRAME_CTRL_BANK_ADDR(bus->params.next_bank);
>> +    struct qcom_swrm_ctrl *ctrl = to_qcom_sdw(bus);
>> +    u32 val;
>> +
>> +    val = ctrl->reg_read(ctrl, reg);
>> +    val |= ((0 << SWRM_MCP_FRAME_CTRL_BANK_ROW_CTRL_SHFT) |
>> +        (7l << SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_SHFT));
> 
> magic values, probably need a macro here?
> 
Okay, will add a proper define for this values.

>> +    ctrl->reg_write(ctrl, reg, val);
>> +
>> +    return 0;
>> +}
>> +
> 
>> +static int qcom_swrm_register_dais(struct qcom_swrm_ctrl *ctrl)
>> +{
>> +    int num_dais = ctrl->num_dout_ports + ctrl->num_din_ports;
>> +    struct snd_soc_dai_driver *dais;
>> +    int i;
>> +
>> +    /* PDM dais are only tested for now */
>> +    dais = devm_kcalloc(ctrl->dev, num_dais, sizeof(*dais), GFP_KERNEL);
>> +    if (!dais)
>> +        return -ENOMEM;
>> +
>> +    for (i = 0; i < num_dais; i++) {
>> +        dais[i].name = kasprintf(GFP_KERNEL, "SDW Pin%d", i);
> 
> if (!dais[i].name)
> 
>> +        if (i < ctrl->num_dout_ports) {
>> +            dais[i].playback.stream_name = kasprintf(GFP_KERNEL,
>> +                                 "SDW Tx%d", i);
>> +            if (!dais[i].playback.stream_name) {
>> +                kfree(dais[i].name);
>> +                return -ENOMEM;
>> +            }
> 
> also need to free previously allocated memory in earlier iterations, or 
> use devm_
> 
Yes, thats true, I will fix this in next version.


Thanks,
srini

Re: [alsa-devel] [RFC PATCH 6/6] soundwire: qcom: add support for SoundWire controller

[Pierre-Louis Bossart]

>>> +#define SWRM_COMP_HW_VERSION                    0x00
>>
>> Can we please use SDW_ or QCOM_SDW_ as prefix?
>>
> 
> SWRM prefix is as per the data sheet register names, If it help am happy 
> to add QCOM_ prefix it.

That'd be fine. As long as there is no duplication and two 
terms/prefixes used for the same thing I am happy.

>>> +
>>> +    val = SWRM_REG_VAL_PACK(cmd_data, dev_addr, cmd_id, reg_addr);
>>> +    ret = ctrl->reg_write(ctrl, SWRM_CMD_FIFO_WR_CMD, val);
>>> +    if (ret < 0) {
>>> +        dev_err(ctrl->dev, "%s: reg 0x%x write failed, err:%d\n",
>>> +            __func__, val, ret);
>>> +        goto err;
>>> +    }
>>> +
>>> +    if (dev_addr == SDW_BROADCAST_DEV_NUM) {
>>> +        ctrl->fifo_status = 0;
>>> +        ret = wait_for_completion_timeout(&ctrl->sp_cmd_comp,
>>> +                          msecs_to_jiffies(TIMEOUT_MS));
>>
>> This is odd. The SoundWire spec does not handle writes to a single 
>> device or broadcast writes differently. I don't see a clear reason why 
>> you would only timeout for a broadcast write.
>>
> 
> There is danger of blocking here without timeout.

Right, and it's fine to add a timeout. The question is why add a timeout 
*only* for a broadcast operation? It should be added for every 
transaction IMO, unless you have a reason not to do so.

>>
>>> +
>>> +    /* Mask soundwire interrupts */
>>> +    ctrl->reg_write(ctrl, SWRM_INTERRUPT_MASK_ADDR,
>>> +                    SWRM_INTERRUPT_STATUS_RMSK);
>>> +
>>> +    /* Configure No pings */
>>> +    val = ctrl->reg_read(ctrl, SWRM_MCP_CFG_ADDR);
>>
>> If there is any sort of PREQ signaling for Slave-initiated interrupts, 
>> disabling PINGs is likely a non-conformant implementation since the 
>> master is required to issue a PING command within 32 frames. That's 
>> also the only way to know if a device is attached, so additional 
>> comments are likely required.
> This is the value of Maximum number of consiecutive read/write commands 
> without ping command in between. I will try to collect more details and 
> add some comments here.

Right, so it's probably the comment that's too strict. It's not no pings 
it's no pings during a sequence of up to N read/writes.

Re: [alsa-devel] [RFC PATCH 6/6] soundwire: qcom: add support for SoundWire controller

[Srinivas Kandagatla]

On 10/06/2019 15:12, Pierre-Louis Bossart wrote:
>>>> +
>>>> +    if (dev_addr == SDW_BROADCAST_DEV_NUM) {
>>>> +        ctrl->fifo_status = 0;
>>>> +        ret = wait_for_completion_timeout(&ctrl->sp_cmd_comp,
>>>> +                          msecs_to_jiffies(TIMEOUT_MS));
>>>
>>> This is odd. The SoundWire spec does not handle writes to a single 
>>> device or broadcast writes differently. I don't see a clear reason 
>>> why you would only timeout for a broadcast write.
>>>
>>
>> There is danger of blocking here without timeout.
> 
> Right, and it's fine to add a timeout. The question is why add a timeout 
> *only* for a broadcast operation? It should be added for every 
> transaction IMO, unless you have a reason not to do so.
> 

I did try this before, the issue is when we read/write registers from 
interrupt handler, these can be deadlocked as we will be interrupt 
handler waiting for another completion interrupt, which will never 
happen unless we return from the first interrupt.

thanks,
srini

>>>

Re: [alsa-devel] [RFC PATCH 6/6] soundwire: qcom: add support for SoundWire controller

[Pierre-Louis Bossart]

On 6/11/19 5:29 AM, Srinivas Kandagatla wrote:
> 
> 
> On 10/06/2019 15:12, Pierre-Louis Bossart wrote:
>>>>> +
>>>>> +    if (dev_addr == SDW_BROADCAST_DEV_NUM) {
>>>>> +        ctrl->fifo_status = 0;
>>>>> +        ret = wait_for_completion_timeout(&ctrl->sp_cmd_comp,
>>>>> +                          msecs_to_jiffies(TIMEOUT_MS));
>>>>
>>>> This is odd. The SoundWire spec does not handle writes to a single 
>>>> device or broadcast writes differently. I don't see a clear reason 
>>>> why you would only timeout for a broadcast write.
>>>>
>>>
>>> There is danger of blocking here without timeout.
>>
>> Right, and it's fine to add a timeout. The question is why add a 
>> timeout *only* for a broadcast operation? It should be added for every 
>> transaction IMO, unless you have a reason not to do so.
>>
> 
> I did try this before, the issue is when we read/write registers from 
> interrupt handler, these can be deadlocked as we will be interrupt 
> handler waiting for another completion interrupt, which will never 
> happen unless we return from the first interrupt.

I don't quite get the issue. With the Intel hardware we only deal with 
Master registers (some of which mirror the bus state) in the handler and 
will only modify Slave registers in the thread. All changes to Slave 
registers will be subject to a timeout as well as a check for no 
response or NAK. Not sure what is specific about your solution that 
requires a different handling of commands depending on which device 
number is used. It could very well be that you've uncovered a flaw in 
the bus design but I still don't see how it would be Qualcomm-specific?

Re: [alsa-devel] [RFC PATCH 6/6] soundwire: qcom: add support for SoundWire controller

[Srinivas Kandagatla]

On 11/06/2019 13:21, Pierre-Louis Bossart wrote:
> 
> 
> On 6/11/19 5:29 AM, Srinivas Kandagatla wrote:
>>
>>
>> On 10/06/2019 15:12, Pierre-Louis Bossart wrote:
>>>>>> +
>>>>>> +    if (dev_addr == SDW_BROADCAST_DEV_NUM) {
>>>>>> +        ctrl->fifo_status = 0;
>>>>>> +        ret = wait_for_completion_timeout(&ctrl->sp_cmd_comp,
>>>>>> +                          msecs_to_jiffies(TIMEOUT_MS));
>>>>>
>>>>> This is odd. The SoundWire spec does not handle writes to a single 
>>>>> device or broadcast writes differently. I don't see a clear reason 
>>>>> why you would only timeout for a broadcast write.
>>>>>
>>>>
>>>> There is danger of blocking here without timeout.
>>>
>>> Right, and it's fine to add a timeout. The question is why add a 
>>> timeout *only* for a broadcast operation? It should be added for 
>>> every transaction IMO, unless you have a reason not to do so.
>>>
>>
>> I did try this before, the issue is when we read/write registers from 
>> interrupt handler, these can be deadlocked as we will be interrupt 
>> handler waiting for another completion interrupt, which will never 
>> happen unless we return from the first interrupt.
> 
> I don't quite get the issue. With the Intel hardware we only deal with 
> Master registers (some of which mirror the bus state) in the handler and 
> will only modify Slave registers in the thread. All changes to Slave 
> registers will be subject to a timeout as well as a check for no 
> response or NAK. Not sure what is specific about your solution that 
> requires a different handling of commands depending on which device 
> number is used. It could very well be that you've uncovered a flaw in 
> the bus design but I still don't see how it would be Qualcomm-specific?

Sorry It took bit more time for digging up the issue which I faced 
previously to answer this query. This is now fixed and v2 patchset has 
same handling for all the slave registers read/writes with no special 
casing.

Thanks,
srini

Re: [RFC PATCH 6/6] soundwire: qcom: add support for SoundWire controller

[Cezary Rojewski]

On 2019-06-07 10:56, Srinivas Kandagatla wrote:
> Qualcomm SoundWire Master controller is present in most Qualcomm SoCs
> either integrated as part of WCD audio codecs via slimbus or
> as part of SOC I/O.
> 
> This patchset adds support to a very basic controller which has been
> tested with WCD934x SoundWire controller connected to WSA881x smart
> speaker amplifiers.
> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
>   drivers/soundwire/Kconfig  |   9 +
>   drivers/soundwire/Makefile |   4 +
>   drivers/soundwire/qcom.c   | 983 +++++++++++++++++++++++++++++++++++++
>   3 files changed, 996 insertions(+)
>   create mode 100644 drivers/soundwire/qcom.c
> 
> diff --git a/drivers/soundwire/Kconfig b/drivers/soundwire/Kconfig
> index 53b55b79c4af..f44d4f36dbbb 100644
> --- a/drivers/soundwire/Kconfig
> +++ b/drivers/soundwire/Kconfig
> @@ -34,4 +34,13 @@ config SOUNDWIRE_INTEL
>   	  enable this config option to get the SoundWire support for that
>   	  device.
>   
> +config SOUNDWIRE_QCOM
> +	tristate "Qualcomm SoundWire Master driver"
> +	select SOUNDWIRE_BUS
> +	depends on SND_SOC
> +	help
> +	  SoundWire Qualcomm Master driver.
> +	  If you have an Qualcomm platform which has a SoundWire Master then
> +	  enable this config option to get the SoundWire support for that
> +	  device
>   endif
> diff --git a/drivers/soundwire/Makefile b/drivers/soundwire/Makefile
> index 5817beaca0e1..f4ebfde31372 100644
> --- a/drivers/soundwire/Makefile
> +++ b/drivers/soundwire/Makefile
> @@ -16,3 +16,7 @@ obj-$(CONFIG_SOUNDWIRE_INTEL) += soundwire-intel.o
>   
>   soundwire-intel-init-objs := intel_init.o
>   obj-$(CONFIG_SOUNDWIRE_INTEL) += soundwire-intel-init.o
> +
> +#Qualcomm driver
> +soundwire-qcom-objs :=	qcom.o
> +obj-$(CONFIG_SOUNDWIRE_QCOM) += soundwire-qcom.o
> diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
> new file mode 100644
> index 000000000000..d1722d44d217
> --- /dev/null
> +++ b/drivers/soundwire/qcom.c
> @@ -0,0 +1,983 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2019, Linaro Limited
> +
> +#include <linux/clk.h>
> +#include <linux/completion.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +#include <linux/slimbus.h>
> +#include <linux/soundwire/sdw.h>
> +#include <linux/soundwire/sdw_registers.h>
> +#include <sound/pcm_params.h>
> +#include <sound/soc.h>
> +#include "bus.h"
> +

Pierre already pointed this out - SWR looks odd. During my time with 
Soundwire I've met SDW and SNDW, but it's the first time I see SWR.

> +#define SWRM_COMP_HW_VERSION					0x00
> +#define SWRM_COMP_CFG_ADDR					0x04
> +#define SWRM_COMP_CFG_IRQ_LEVEL_OR_PULSE_MSK			BIT(1)
> +#define SWRM_COMP_CFG_ENABLE_MSK				BIT(0)
> +#define SWRM_COMP_PARAMS					0x100
> +#define SWRM_COMP_PARAMS_DOUT_PORTS_MASK			GENMASK(4, 0)
> +#define SWRM_COMP_PARAMS_DIN_PORTS_MASK				GENMASK(9, 5)
> +#define SWRM_COMP_PARAMS_WR_FIFO_DEPTH				GENMASK(14, 10)
> +#define SWRM_COMP_PARAMS_RD_FIFO_DEPTH				GENMASK(19, 15)
> +#define SWRM_COMP_PARAMS_AUTO_ENUM_SLAVES			GENMASK(32. 20)
> +#define SWRM_INTERRUPT_STATUS					0x200
> +#define SWRM_INTERRUPT_STATUS_RMSK				GENMASK(16, 0)
> +#define SWRM_INTERRUPT_STATUS_SLAVE_PEND_IRQ			BIT(0)
> +#define SWRM_INTERRUPT_STATUS_NEW_SLAVE_ATTACHED		BIT(1)
> +#define SWRM_INTERRUPT_STATUS_CHANGE_ENUM_SLAVE_STATUS		BIT(2)
> +#define SWRM_INTERRUPT_STATUS_MASTER_CLASH_DET			BIT(3)
> +#define SWRM_INTERRUPT_STATUS_RD_FIFO_OVERFLOW			BIT(4)
> +#define SWRM_INTERRUPT_STATUS_RD_FIFO_UNDERFLOW			BIT(5)
> +#define SWRM_INTERRUPT_STATUS_WR_CMD_FIFO_OVERFLOW		BIT(6)
> +#define SWRM_INTERRUPT_STATUS_CMD_ERROR				BIT(7)
> +#define SWRM_INTERRUPT_STATUS_DOUT_PORT_COLLISION		BIT(8)
> +#define SWRM_INTERRUPT_STATUS_READ_EN_RD_VALID_MISMATCH		BIT(9)
> +#define SWRM_INTERRUPT_STATUS_SPECIAL_CMD_ID_FINISHED		BIT(10)
> +#define SWRM_INTERRUPT_STATUS_NEW_SLAVE_AUTO_ENUM_FINISHED	BIT(11)
> +#define SWRM_INTERRUPT_STATUS_AUTO_ENUM_FAILED			BIT(12)
> +#define SWRM_INTERRUPT_STATUS_AUTO_ENUM_TABLE_IS_FULL		BIT(13)
> +#define SWRM_INTERRUPT_STATUS_BUS_RESET_FINISHED		BIT(14)
> +#define SWRM_INTERRUPT_STATUS_CLK_STOP_FINISHED			BIT(15)
> +#define SWRM_INTERRUPT_STATUS_ERROR_PORT_TEST			BIT(16)
> +#define SWRM_INTERRUPT_MASK_ADDR				0x204
> +#define SWRM_INTERRUPT_CLEAR					0x208

You seem to shortcut every reg here similarly to how it's done in SDW 
spec. INTERRUPT is represented by INT there, and by doing so, this 
define block would look more like a real family.

> +#define SWRM_CMD_FIFO_WR_CMD					0x300
> +#define SWRM_CMD_FIFO_RD_CMD					0x304
> +#define SWRM_CMD_FIFO_CMD					0x308
> +#define SWRM_CMD_FIFO_STATUS					0x30C
> +#define SWRM_CMD_FIFO_CFG_ADDR					0x314
> +#define SWRM_CMD_FIFO_CFG_NUM_OF_CMD_RETRY_SHFT			0x0
> +#define SWRM_CMD_FIFO_RD_FIFO_ADDR				0x318
> +#define SWRM_ENUMERATOR_CFG_ADDR				0x500
> +#define SWRM_MCP_FRAME_CTRL_BANK_ADDR(m)		(0x101C + 0x40 * (m))
> +#define SWRM_MCP_FRAME_CTRL_BANK_SSP_PERIOD_SHFT		16
> +#define SWRM_MCP_FRAME_CTRL_BANK_ROW_CTRL_SHFT			3
> +#define SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_BMSK			GENMASK(2, 0)
> +#define SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_SHFT			0
> +#define SWRM_MCP_CFG_ADDR					0x1048
> +#define SWRM_MCP_CFG_MAX_NUM_OF_CMD_NO_PINGS_BMSK		GENMASK(21, 17)
> +#define SWRM_MCP_CFG_MAX_NUM_OF_CMD_NO_PINGS_SHFT		0x11
> +#define SWRM_MCP_STATUS						0x104C
> +#define SWRM_MCP_STATUS_BANK_NUM_MASK				BIT(0)
> +#define SWRM_MCP_SLV_STATUS					0x1090
> +#define SWRM_MCP_SLV_STATUS_MASK				GENMASK(1, 0)
> +#define SWRM_DP_PORT_CTRL_BANK(n, m)	(0x1124 + 0x100 * (n - 1) + 0x40 * m)

Some of these you align, others leave with the equal amount of tabs 
despite different widths.

> +#define SWRM_DP_PORT_CTRL2_BANK(n, m)	(0x1126 + 0x100 * (n - 1) + 0x40 * m)

Consider reusing _CTRL_ and simply adding offset for 2_.

> +#define SWRM_DP_PORT_CTRL_EN_CHAN_SHFT				0x18
> +#define SWRM_DP_PORT_CTRL_OFFSET2_SHFT				0x10
> +#define SWRM_DP_PORT_CTRL_OFFSET1_SHFT				0x08
> +#define SWRM_AHB_BRIDGE_WR_DATA_0				0xc885
> +#define SWRM_AHB_BRIDGE_WR_ADDR_0				0xc889
> +#define SWRM_AHB_BRIDGE_RD_ADDR_0				0xc88d
> +#define SWRM_AHB_BRIDGE_RD_DATA_0				0xc891
> +
> +#define SWRM_REG_VAL_PACK(data, dev, id, reg)	\
> +			((reg) | ((id) << 16) | ((dev) << 20) | ((data) << 24))
> +
> +#define SWRM_MAX_ROW_VAL	0 /* Rows = 48 */
> +#define SWRM_DEFAULT_ROWS	48
> +#define SWRM_MIN_COL_VAL	0 /* Cols = 2 */
> +#define SWRM_DEFAULT_COL	16
> +#define SWRM_SPECIAL_CMD_ID	0xF
> +#define MAX_FREQ_NUM		1
> +#define TIMEOUT_MS		1000
> +#define QCOM_SWRM_MAX_RD_LEN	0xf
> +#define DEFAULT_CLK_FREQ	9600000
> +#define SWRM_MAX_DAIS		0xF

Given the scale of this block, it might be good to reiterate all defines 
and see if indeed all are QCom specific. Maybe some could be replaced by 
equivalents from common code.

> +
> +struct qcom_swrm_port_config {
> +	u8 si;
> +	u8 off1;
> +	u8 off2;
> +};
> +
> +struct qcom_swrm_ctrl {
> +	struct sdw_bus bus;
> +	struct device *dev;
> +	struct regmap *regmap;
> +	struct completion sp_cmd_comp;
> +	struct work_struct slave_work;
> +	/* read/write lock */
> +	struct mutex lock;
> +	/* Port alloc/free lock */
> +	struct mutex port_lock;
> +	struct clk *hclk;
> +	int fifo_status;
> +	void __iomem *base;
> +	u8 wr_cmd_id;
> +	u8 rd_cmd_id;
> +	int irq;
> +	unsigned int version;

Given the fact you don't use version field directly and always shift it, 
I'd consider making use of union here to listing version bits 
explicitly. Overall size won't change.

> +	int num_din_ports;
> +	int num_dout_ports;
> +	unsigned long dout_port_mask;
> +	unsigned long din_port_mask;
> +	struct qcom_swrm_port_config pconfig[SDW_MAX_PORTS];
> +	struct sdw_stream_runtime *sruntime[SWRM_MAX_DAIS];
> +	enum sdw_slave_status status[SDW_MAX_DEVICES];
> +	u32 (*reg_read)(struct qcom_swrm_ctrl *ctrl, int reg);
> +	int (*reg_write)(struct qcom_swrm_ctrl *ctrl, int reg, int val);
> +};
> +
> +#define to_qcom_sdw(b)	container_of(b, struct qcom_swrm_ctrl, bus)
> +
> +struct usecase {
> +	u8 num_port;
> +	u8 num_ch;
> +	u32 chrate;
> +};
> +

"usecase" looks ambiguous at best.

> +static u32 qcom_swrm_slim_reg_read(struct qcom_swrm_ctrl *ctrl, int reg)
> +{
> +	struct regmap *wcd_regmap = ctrl->regmap;
> +	u32 val = 0, ret;
> +
> +	/* pg register + offset */
> +	regmap_bulk_write(wcd_regmap, SWRM_AHB_BRIDGE_RD_ADDR_0,
> +			  (u8 *)&reg, 4);
> +
> +	ret = regmap_bulk_read(wcd_regmap, SWRM_AHB_BRIDGE_RD_DATA_0,
> +			       (u8 *)&val, 4);
> +	if (ret < 0)
> +		dev_err(ctrl->dev, "Read Failure (%d)\n", ret);
> +
> +	return val;
> +}
> +
> +static u32 qcom_swrm_mmio_reg_read(struct qcom_swrm_ctrl *ctrl, int reg)
> +{
> +	return readl_relaxed(ctrl->base + reg);
> +}
> +
> +static int qcom_swrm_mmio_reg_write(struct qcom_swrm_ctrl *ctrl,
> +				    int reg, int val)
> +{
> +	writel_relaxed(val, ctrl->base + reg);
> +
> +	return 0;
> +}
> +
> +static int qcom_swrm_slim_reg_write(struct qcom_swrm_ctrl *ctrl,
> +				    int reg, int val)
> +{
> +	struct regmap *wcd_regmap = ctrl->regmap;
> +
> +	/* pg register + offset */
> +	regmap_bulk_write(wcd_regmap, SWRM_AHB_BRIDGE_WR_DATA_0,
> +			  (u8 *)&val, 4);
> +	/* write address register */
> +	regmap_bulk_write(wcd_regmap, SWRM_AHB_BRIDGE_WR_ADDR_0,
> +			  (u8 *)&reg, 4);
> +
> +	return 0;
> +}

Ok, so you choose to declare write op as returning "int" yet either it 
cannot do so (void writel_relaxed) or ret is completely ignored 
(regmap_bulk_write does return an int value). Either switch to void or 
check against returned value whenever possible.

> +
> +static int qcom_swrm_cmd_fifo_wr_cmd(struct qcom_swrm_ctrl *ctrl, u8 cmd_data,
> +				     u8 dev_addr, u16 reg_addr)
> +{
> +	int ret = 0;
> +	u8 cmd_id;
> +	u32 val;
> +
> +	mutex_lock(&ctrl->lock);
> +	if (dev_addr == SDW_BROADCAST_DEV_NUM) {
> +		cmd_id = SWRM_SPECIAL_CMD_ID;
> +	} else {
> +		if (++ctrl->wr_cmd_id == SWRM_SPECIAL_CMD_ID)
> +			ctrl->wr_cmd_id = 0;
> +
> +		cmd_id = ctrl->wr_cmd_id;
> +	}
> +
> +	val = SWRM_REG_VAL_PACK(cmd_data, dev_addr, cmd_id, reg_addr);
> +	ret = ctrl->reg_write(ctrl, SWRM_CMD_FIFO_WR_CMD, val);
> +	if (ret < 0) {
> +		dev_err(ctrl->dev, "%s: reg 0x%x write failed, err:%d\n",
> +			__func__, val, ret);
> +		goto err;
> +	}
> +
> +	if (dev_addr == SDW_BROADCAST_DEV_NUM) {
> +		ctrl->fifo_status = 0;
> +		ret = wait_for_completion_timeout(&ctrl->sp_cmd_comp,
> +						  msecs_to_jiffies(TIMEOUT_MS));
> +
> +		if (!ret || ctrl->fifo_status) {
> +			dev_err(ctrl->dev, "reg 0x%x write failed\n", val);

Both, this and err msg above are generic enough to be put into goto to 
save some space.

> +			ret = -ENODATA;
> +			goto err;
> +		}
> +	}
> +err:
> +	mutex_unlock(&ctrl->lock);
> +	return ret;
> +}
> +
> +static int qcom_swrm_cmd_fifo_rd_cmd(struct qcom_swrm_ctrl *ctrl,
> +				     u8 dev_addr, u16 reg_addr,
> +				     u32 len, u8 *rval)
> +{
> +	int i, ret = 0;
> +	u8 cmd_id = 0;
> +	u32 val;
> +
> +	mutex_lock(&ctrl->lock);
> +	if (dev_addr == SDW_ENUM_DEV_NUM) {
> +		cmd_id = SWRM_SPECIAL_CMD_ID;
> +	} else {
> +		if (++ctrl->rd_cmd_id == SWRM_SPECIAL_CMD_ID)
> +			ctrl->rd_cmd_id = 0;
> +
> +		cmd_id = ctrl->rd_cmd_id;
> +	}
> +
> +	val = SWRM_REG_VAL_PACK(len, dev_addr, cmd_id, reg_addr);
> +	ret = ctrl->reg_write(ctrl, SWRM_CMD_FIFO_RD_CMD, val);
> +	if (ret < 0) {
> +		dev_err(ctrl->dev, "reg 0x%x write failed err:%d\n", val, ret);

Same for _rt_.

> +		goto err;
> +	}
> +
> +	if (dev_addr == SDW_ENUM_DEV_NUM) {
> +		ctrl->fifo_status = 0;
> +		ret = wait_for_completion_timeout(&ctrl->sp_cmd_comp,
> +						  msecs_to_jiffies(TIMEOUT_MS));
> +
> +		if (!ret || ctrl->fifo_status) {
> +			dev_err(ctrl->dev, "reg 0x%x read failed\n", val);

Just to be sure. It is really "read" that is failing here?

> +			ret = -ENODATA;
> +			goto err;
> +		}
> +	}
> +
> +	for (i = 0; i < len; i++) {
> +		rval[i] = ctrl->reg_read(ctrl, SWRM_CMD_FIFO_RD_FIFO_ADDR);
> +		rval[i] &= 0xFF;
> +	}
> +
> +err:
> +	mutex_unlock(&ctrl->lock);
> +	return ret;
> +}
> +
> +static void qcom_swrm_get_device_status(struct qcom_swrm_ctrl *ctrl)
> +{
> +	u32 val = ctrl->reg_read(ctrl, SWRM_MCP_SLV_STATUS);
> +	int i;
> +
> +	for (i = 1; i < SDW_MAX_DEVICES; i++) {
> +		u32 s;
> +
> +		s = (val >> (i * 2));
> +		s &= SWRM_MCP_SLV_STATUS_MASK;
> +		ctrl->status[i] = s;
> +	}
> +}
> +
> +static irqreturn_t qcom_swrm_irq_handler(int irq, void *dev_id)
> +{
> +	struct qcom_swrm_ctrl *ctrl = dev_id;
> +	u32 sts, value;
> +
> +	sts = ctrl->reg_read(ctrl, SWRM_INTERRUPT_STATUS);
> +
> +	if (sts & SWRM_INTERRUPT_STATUS_SPECIAL_CMD_ID_FINISHED)
> +		complete(&ctrl->sp_cmd_comp);
> +
> +	if (sts & SWRM_INTERRUPT_STATUS_CMD_ERROR) {
> +		value = ctrl->reg_read(ctrl, SWRM_CMD_FIFO_STATUS);
> +		dev_err_ratelimited(ctrl->dev,
> +				    "CMD error, fifo status 0x%x\n",
> +				     value);
> +		ctrl->reg_write(ctrl, SWRM_CMD_FIFO_CMD, 0x1);
> +		if ((value & 0xF) == 0xF) {
> +			ctrl->fifo_status = -ENODATA;
> +			complete(&ctrl->sp_cmd_comp);
> +		}
> +	}
> +
> +	if ((sts & SWRM_INTERRUPT_STATUS_NEW_SLAVE_ATTACHED) ||
> +	    sts & SWRM_INTERRUPT_STATUS_CHANGE_ENUM_SLAVE_STATUS) {
> +		if (sts & SWRM_INTERRUPT_STATUS_NEW_SLAVE_ATTACHED)
> +			ctrl->status[0] = SDW_SLAVE_ATTACHED;
> +
> +		schedule_work(&ctrl->slave_work);
> +	}
> +
> +	if (sts & SWRM_INTERRUPT_STATUS_SLAVE_PEND_IRQ)
> +		dev_dbg(ctrl->dev, "Slave pend irq\n");
> +
> +	if (sts & SWRM_INTERRUPT_STATUS_NEW_SLAVE_ATTACHED)
> +		dev_dbg(ctrl->dev, "New slave attached\n");
> +
> +	if (sts & SWRM_INTERRUPT_STATUS_MASTER_CLASH_DET)
> +		dev_err_ratelimited(ctrl->dev, "Bus clash detected\n");
> +
> +	if (sts & SWRM_INTERRUPT_STATUS_RD_FIFO_OVERFLOW)
> +		dev_err(ctrl->dev, "Read FIFO overflow\n");
> +
> +	if (sts & SWRM_INTERRUPT_STATUS_RD_FIFO_UNDERFLOW)
> +		dev_err(ctrl->dev, "Read FIFO underflow\n");
> +
> +	if (sts & SWRM_INTERRUPT_STATUS_WR_CMD_FIFO_OVERFLOW)
> +		dev_err(ctrl->dev, "Write FIFO overflow\n");
> +
> +	if (sts & SWRM_INTERRUPT_STATUS_DOUT_PORT_COLLISION)
> +		dev_err(ctrl->dev, "Port collision detected\n");
> +
> +	if (sts & SWRM_INTERRUPT_STATUS_READ_EN_RD_VALID_MISMATCH)
> +		dev_err(ctrl->dev, "Read enable valid mismatch\n");
> +
> +	if (sts & SWRM_INTERRUPT_STATUS_SPECIAL_CMD_ID_FINISHED)
> +		dev_err(ctrl->dev, "Cmd id finished\n");
> +
> +	if (sts & SWRM_INTERRUPT_STATUS_BUS_RESET_FINISHED)
> +		dev_err(ctrl->dev, "Bus reset finished\n");

If you do not handle these errors at all, consider declaring 
ERROR-message table. I believe leaving erroneus status as is may lead to 
fatal consequences. If there is no intention for handling even the most 
critical cases, please add TODO/ comment here.

> +
> +	ctrl->reg_write(ctrl, SWRM_INTERRUPT_CLEAR, sts);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int qcom_swrm_init(struct qcom_swrm_ctrl *ctrl)
> +{
> +	u32 val;
> +	u8 row_ctrl = SWRM_MAX_ROW_VAL;
> +	u8 col_ctrl = SWRM_MIN_COL_VAL;
> +	u8 ssp_period = 1;
> +	u8 retry_cmd_num = 3;
> +
> +	/* Clear Rows and Cols */
> +	val = ((row_ctrl << SWRM_MCP_FRAME_CTRL_BANK_ROW_CTRL_SHFT) |
> +		(col_ctrl << SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_SHFT) |
> +		(ssp_period << SWRM_MCP_FRAME_CTRL_BANK_SSP_PERIOD_SHFT));
> +
> +	ctrl->reg_write(ctrl, SWRM_MCP_FRAME_CTRL_BANK_ADDR(0), val);
> +
> +	/* Disable Auto enumeration */
> +	ctrl->reg_write(ctrl, SWRM_ENUMERATOR_CFG_ADDR, 0);
> +
> +	/* Mask soundwire interrupts */
> +	ctrl->reg_write(ctrl, SWRM_INTERRUPT_MASK_ADDR,
> +					SWRM_INTERRUPT_STATUS_RMSK);
> +
> +	/* Configure No pings */
> +	val = ctrl->reg_read(ctrl, SWRM_MCP_CFG_ADDR);
> +
> +	val &= ~SWRM_MCP_CFG_MAX_NUM_OF_CMD_NO_PINGS_BMSK;
> +	val |= (0x1f << SWRM_MCP_CFG_MAX_NUM_OF_CMD_NO_PINGS_SHFT);
> +	ctrl->reg_write(ctrl, SWRM_MCP_CFG_ADDR, val);
> +
> +	/* Configure number of retries of a read/write cmd */
> +	val = (retry_cmd_num << SWRM_CMD_FIFO_CFG_NUM_OF_CMD_RETRY_SHFT);
> +	ctrl->reg_write(ctrl, SWRM_CMD_FIFO_CFG_ADDR, val);
> +
> +	/* Set IRQ to PULSE */
> +	ctrl->reg_write(ctrl, SWRM_COMP_CFG_ADDR,
> +				SWRM_COMP_CFG_IRQ_LEVEL_OR_PULSE_MSK |
> +				SWRM_COMP_CFG_ENABLE_MSK);
> +	return 0;

As in my previous comment, you should check against ret from reg_write 
if void approach is not chosen.

> +}
> +
> +static enum sdw_command_response qcom_swrm_xfer_msg(struct sdw_bus *bus,
> +						    struct sdw_msg *msg)
> +{
> +	struct qcom_swrm_ctrl *ctrl = to_qcom_sdw(bus);
> +	int ret, i, len;
> +
> +	if (msg->flags == SDW_MSG_FLAG_READ) {
> +		for (i = 0; i < msg->len;) {
> +			if ((msg->len - i) < QCOM_SWRM_MAX_RD_LEN)
> +				len = msg->len - i;
> +			else
> +				len = QCOM_SWRM_MAX_RD_LEN;
> +
> +			ret = qcom_swrm_cmd_fifo_rd_cmd(ctrl, msg->dev_num,
> +							msg->addr + i, len,
> +						       &msg->buf[i]);
> +			if (ret < 0) {
> +				if (ret == -ENODATA)
> +					return SDW_CMD_IGNORED;
> +
> +				return ret;
> +			}
> +
> +			i = i + len;

Any reason for inlining this incrementation? If _rd_ fails, we leave the 
loop anyway.

> +		}
> +	} else if (msg->flags == SDW_MSG_FLAG_WRITE) {
> +		for (i = 0; i < msg->len; i++) {
> +			ret = qcom_swrm_cmd_fifo_wr_cmd(ctrl, msg->buf[i],
> +							msg->dev_num,
> +						       msg->addr + i);
> +			if (ret < 0) {
> +				if (ret == -ENODATA)
> +					return SDW_CMD_IGNORED;
> +
> +				return ret;
> +			}
> +		}
> +	}
> +
> +	return SDW_CMD_OK;
> +}
> +
> +static int qcom_swrm_pre_bank_switch(struct sdw_bus *bus)
> +{
> +	u32 reg = SWRM_MCP_FRAME_CTRL_BANK_ADDR(bus->params.next_bank);
> +	struct qcom_swrm_ctrl *ctrl = to_qcom_sdw(bus);
> +	u32 val;
> +
> +	val = ctrl->reg_read(ctrl, reg);
> +	val |= ((0 << SWRM_MCP_FRAME_CTRL_BANK_ROW_CTRL_SHFT) |
> +		(7l << SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_SHFT));
> +	ctrl->reg_write(ctrl, reg, val);
> +
> +	return 0;

s/return 0/return ctrl->reg_write(ctrl, reg, val)/

> +}
> +
> +static int qcom_swrm_port_params(struct sdw_bus *bus,
> +				 struct sdw_port_params *p_params,
> +				unsigned int bank)
> +{
> +	/* TBD */
> +	return 0;
> +}
> +
> +static int qcom_swrm_transport_params(struct sdw_bus *bus,
> +				      struct sdw_transport_params *params,
> +				     enum sdw_reg_bank bank)
> +{
> +	struct qcom_swrm_ctrl *ctrl = to_qcom_sdw(bus);
> +	u32 value;
> +
> +	value = params->offset1 << SWRM_DP_PORT_CTRL_OFFSET1_SHFT;
> +	value |= params->offset2 << SWRM_DP_PORT_CTRL_OFFSET2_SHFT;
> +	value |= params->sample_interval - 1;
> +
> +	ctrl->reg_write(ctrl, SWRM_DP_PORT_CTRL_BANK((params->port_num), bank),
> +			value);
> +
> +	return 0;

Another "return issue" here.

> +}
> +
> +static int qcom_swrm_port_enable(struct sdw_bus *bus,
> +				 struct sdw_enable_ch *enable_ch,
> +				unsigned int bank)
> +{
> +	u32 reg = SWRM_DP_PORT_CTRL_BANK(enable_ch->port_num, bank);
> +	struct qcom_swrm_ctrl *ctrl = to_qcom_sdw(bus);
> +	u32 val;
> +
> +	val = ctrl->reg_read(ctrl, reg);
> +	if (enable_ch->enable)
> +		val |= (enable_ch->ch_mask << SWRM_DP_PORT_CTRL_EN_CHAN_SHFT);
> +	else
> +		val &= ~(enable_ch->ch_mask << SWRM_DP_PORT_CTRL_EN_CHAN_SHFT);
> +
> +	ctrl->reg_write(ctrl, reg, val);
> +
> +	return 0;
> +}
> +
> +static struct sdw_master_port_ops qcom_swrm_port_ops = {
> +	.dpn_set_port_params = qcom_swrm_port_params,
> +	.dpn_set_port_transport_params = qcom_swrm_transport_params,
> +	.dpn_port_enable_ch = qcom_swrm_port_enable,
> +};
> +
> +static struct sdw_master_ops qcom_swrm_ops = {
> +	.xfer_msg = qcom_swrm_xfer_msg,
> +	.pre_bank_switch = qcom_swrm_pre_bank_switch,
> +};
> +
> +static int qcom_swrm_compute_params(struct sdw_bus *bus)
> +{
> +	struct qcom_swrm_ctrl *ctrl = to_qcom_sdw(bus);
> +	struct sdw_master_runtime *m_rt;
> +	struct sdw_slave_runtime *s_rt;
> +	struct sdw_port_runtime *p_rt;
> +	int i = 0;
> +
> +	list_for_each_entry(m_rt, &bus->m_rt_list, bus_node) {
> +		list_for_each_entry(p_rt, &m_rt->port_list, port_node) {
> +			p_rt->transport_params.port_num = p_rt->num;
> +			p_rt->transport_params.sample_interval =
> +					ctrl->pconfig[p_rt->num - 1].si + 1;
> +			p_rt->transport_params.offset1 =
> +					ctrl->pconfig[p_rt->num - 1].off1;
> +			p_rt->transport_params.offset2 =
> +					ctrl->pconfig[p_rt->num - 1].off2;

ctrl->pconfig[ <idx> ] colleagues bellow make use of local index 
variable which clearly makes it more readable.

> +		}
> +
> +		list_for_each_entry(s_rt, &m_rt->slave_rt_list, m_rt_node) {
> +			list_for_each_entry(p_rt, &s_rt->port_list, port_node) {
> +				p_rt->transport_params.port_num = p_rt->num;
> +				p_rt->transport_params.sample_interval =
> +						ctrl->pconfig[i].si + 1;
> +				p_rt->transport_params.offset1 =
> +						ctrl->pconfig[i].off1;
> +				p_rt->transport_params.offset2 =
> +						ctrl->pconfig[i].off2;
> +				i++;
> +			}
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static u32 qcom_swrm_freq_tbl[MAX_FREQ_NUM] = {
> +	DEFAULT_CLK_FREQ,
> +};
> +
> +static void qcom_swrm_slave_wq(struct work_struct *work)
> +{
> +	struct qcom_swrm_ctrl *ctrl =
> +			container_of(work, struct qcom_swrm_ctrl, slave_work);
> +
> +	qcom_swrm_get_device_status(ctrl);
> +	sdw_handle_slave_status(&ctrl->bus, ctrl->status);
> +}
> +
> +static int qcom_swrm_prepare(struct snd_pcm_substream *substream,
> +			     struct snd_soc_dai *dai)
> +{
> +	struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(dai->dev);
> +
> +	if (!ctrl->sruntime[dai->id])
> +		return -EINVAL;
> +
> +	return sdw_enable_stream(ctrl->sruntime[dai->id]);
> +}
> +
> +static void qcom_swrm_stream_free_ports(struct qcom_swrm_ctrl *ctrl,
> +					struct sdw_stream_runtime *stream)
> +{
> +	struct sdw_master_runtime *m_rt;
> +	struct sdw_port_runtime *p_rt;
> +	unsigned long *port_mask;
> +
> +	mutex_lock(&ctrl->port_lock);
> +
> +	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
> +		if (m_rt->direction == SDW_DATA_DIR_RX)
> +			port_mask = &ctrl->dout_port_mask;
> +		else
> +			port_mask = &ctrl->din_port_mask;
> +
> +		list_for_each_entry(p_rt, &m_rt->port_list, port_node) {
> +			clear_bit(p_rt->num - 1, port_mask);
> +		}

Unnecessary brackets.

> +	}
> +
> +	mutex_unlock(&ctrl->port_lock);
> +}
> +
> +static int qcom_swrm_stream_alloc_ports(struct qcom_swrm_ctrl *ctrl,
> +					struct sdw_stream_runtime *stream,
> +				       struct snd_pcm_hw_params *params,
> +				       int direction)
> +{
> +	struct sdw_port_config pconfig[SDW_MAX_PORTS];
> +	struct sdw_stream_config sconfig;
> +	struct sdw_master_runtime *m_rt;
> +	struct sdw_slave_runtime *s_rt;
> +	struct sdw_port_runtime *p_rt;
> +	unsigned long *port_mask;
> +	int i, maxport, pn, nports = 0, ret = 0;
> +
> +	mutex_lock(&ctrl->port_lock);
> +	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
> +		if (m_rt->direction == SDW_DATA_DIR_RX) {
> +			maxport = ctrl->num_dout_ports;
> +			port_mask = &ctrl->dout_port_mask;
> +		} else {
> +			maxport = ctrl->num_din_ports;
> +			port_mask = &ctrl->din_port_mask;
> +		}
> +
> +		list_for_each_entry(s_rt, &m_rt->slave_rt_list, m_rt_node) {
> +			list_for_each_entry(p_rt, &s_rt->port_list, port_node) {
> +				/* Port numbers start from 1 - 14*/
> +				pn = find_first_zero_bit(port_mask, maxport);
> +				if (pn > (maxport - 1)) {
> +					dev_err(ctrl->dev, "All ports busy\n");
> +					ret = -EBUSY;
> +					goto err;
> +				}
> +				set_bit(pn, port_mask);
> +				pconfig[nports].num = pn + 1;
> +				pconfig[nports].ch_mask = p_rt->ch_mask;
> +				nports++;
> +			}
> +		}
> +	}
> +
> +	if (direction == SNDRV_PCM_STREAM_CAPTURE)
> +		sconfig.direction = SDW_DATA_DIR_TX;
> +	else
> +		sconfig.direction = SDW_DATA_DIR_RX;
> +
> +	sconfig.ch_count = 1;
> +	sconfig.frame_rate = params_rate(params);
> +	sconfig.type = stream->type;
> +	sconfig.bps = 1;

Hmm. frame_rate and type gets assigned based on "input" data yet the 
rest is hardcoded. Is this intended?

> +	sdw_stream_add_master(&ctrl->bus, &sconfig, pconfig,
> +			      nports, stream);
> +err:
> +	if (ret) {
> +		for (i = 0; i < nports; i++)
> +			clear_bit(pconfig[i].num - 1, port_mask);
> +	}
> +
> +	mutex_unlock(&ctrl->port_lock);
> +
> +	return ret;
> +}
> +
> +static int qcom_swrm_hw_params(struct snd_pcm_substream *substream,
> +			       struct snd_pcm_hw_params *params,
> +			      struct snd_soc_dai *dai)
> +{
> +	struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(dai->dev);
> +	int ret;
> +
> +	ret = qcom_swrm_stream_alloc_ports(ctrl, ctrl->sruntime[dai->id],
> +					   params, substream->stream);
> +	if (ret)
> +		return ret;
> +
> +	return sdw_prepare_stream(ctrl->sruntime[dai->id]);
> +}
> +
> +static int qcom_swrm_hw_free(struct snd_pcm_substream *substream,
> +			     struct snd_soc_dai *dai)
> +{
> +	struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(dai->dev);
> +
> +	qcom_swrm_stream_free_ports(ctrl, ctrl->sruntime[dai->id]);
> +	sdw_stream_remove_master(&ctrl->bus, ctrl->sruntime[dai->id]);
> +	sdw_deprepare_stream(ctrl->sruntime[dai->id]);
> +	sdw_disable_stream(ctrl->sruntime[dai->id]);

Declaring local variable initialized with ctrl->sruntime[dai->id] should 
prove more readable.

> +
> +	return 0;
> +}
> +
> +static void *qcom_pdm_get_sdw_stream(struct snd_soc_dai *dai,
> +				     int direction)
> +{
> +	struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(dai->dev);
> +
> +	return ctrl->sruntime[dai->id];
> +}
> +
> +static int qcom_pdm_set_sdw_stream(struct snd_soc_dai *dai,
> +				   void *stream, int direction)
> +{
> +	return 0;
> +}
> +
> +static int qcom_swrm_startup(struct snd_pcm_substream *stream,
> +			     struct snd_soc_dai *dai)
> +{
> +	struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(dai->dev);
> +	struct sdw_stream_runtime *sruntime;
> +
> +	sruntime = sdw_alloc_stream(dai->name);
> +	if (!sruntime)
> +		return -ENOMEM;
> +
> +	ctrl->sruntime[dai->id] = sruntime;
> +
> +	return 0;
> +}
> +
> +static void qcom_swrm_shutdown(struct snd_pcm_substream *stream,
> +			       struct snd_soc_dai *dai)
> +{
> +	struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(dai->dev);
> +
> +	sdw_release_stream(ctrl->sruntime[dai->id]);
> +	ctrl->sruntime[dai->id] = NULL;
> +}
> +
> +static const struct snd_soc_dai_ops qcom_swrm_pdm_dai_ops = {
> +	.hw_params = qcom_swrm_hw_params,
> +	.prepare = qcom_swrm_prepare,
> +	.hw_free = qcom_swrm_hw_free,
> +	.startup = qcom_swrm_startup,
> +	.shutdown = qcom_swrm_shutdown,
> +	.set_sdw_stream = qcom_pdm_set_sdw_stream,
> +	.get_sdw_stream = qcom_pdm_get_sdw_stream,
> +};
> +
> +static const struct snd_soc_component_driver qcom_swrm_dai_component = {
> +	.name           = "soundwire",
> +};
> +
> +static int qcom_swrm_register_dais(struct qcom_swrm_ctrl *ctrl)
> +{
> +	int num_dais = ctrl->num_dout_ports + ctrl->num_din_ports;
> +	struct snd_soc_dai_driver *dais;
> +	int i;
> +
> +	/* PDM dais are only tested for now */
> +	dais = devm_kcalloc(ctrl->dev, num_dais, sizeof(*dais), GFP_KERNEL);
> +	if (!dais)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < num_dais; i++) {
> +		dais[i].name = kasprintf(GFP_KERNEL, "SDW Pin%d", i);
> +		if (i < ctrl->num_dout_ports) {
> +			dais[i].playback.stream_name = kasprintf(GFP_KERNEL,
> +								 "SDW Tx%d", i);
> +			if (!dais[i].playback.stream_name) {
> +				kfree(dais[i].name);
> +				return -ENOMEM;

Now this got me worried. What about memory allocated in iterations 
before the failure? It must be freed in error handling path. goto should 
be of help here.

> +			}
> +			dais[i].playback.channels_min = 1;
> +			dais[i].playback.channels_max = 1;
> +			dais[i].playback.rates = SNDRV_PCM_RATE_48000;
> +			dais[i].playback.formats = SNDRV_PCM_FMTBIT_S16_LE;

All of these formats are hardcoded. Consider declaring a "template" 
format above and simply initialize each dai with it.

> +		} else {
> +			dais[i].capture.stream_name = kasprintf(GFP_KERNEL,
> +								"SDW Rx%d", i);
> +			if (!dais[i].capture.stream_name) {
> +				kfree(dais[i].name);
> +				kfree(dais[i].playback.stream_name);
> +				return -ENOMEM;

Same memory deallocation issue here.

> +			}
> +
> +			dais[i].capture.channels_min = 1;
> +			dais[i].capture.channels_max = 1;
> +			dais[i].capture.rates = SNDRV_PCM_RATE_48000;
> +			dais[i].capture.formats = SNDRV_PCM_FMTBIT_S16_LE;

Comment regarding playback dai initialization applies here too.

> +		}
> +		dais[i].ops = &qcom_swrm_pdm_dai_ops;
> +		dais[i].id = i;
> +	}
> +
> +	return devm_snd_soc_register_component(ctrl->dev,
> +						&qcom_swrm_dai_component,
> +						dais, num_dais);
> +}
> +
> +static int qcom_swrm_get_port_config(struct qcom_swrm_ctrl *ctrl)
> +{
> +	struct device_node *np = ctrl->dev->of_node;
> +	u8 off1[SDW_MAX_PORTS];
> +	u8 off2[SDW_MAX_PORTS];
> +	u8 si[SDW_MAX_PORTS];

Array of struct qcom_swrm_port_config instead of this trio?

> +	int i, ret, nports, val;
> +
> +	val = ctrl->reg_read(ctrl, SWRM_COMP_PARAMS);
> +	ctrl->num_dout_ports = val & SWRM_COMP_PARAMS_DOUT_PORTS_MASK;
> +	ctrl->num_din_ports = (val & SWRM_COMP_PARAMS_DIN_PORTS_MASK) >> 5;
> +
> +	ret = of_property_read_u32(np, "qcom,din-ports", &val);
> +	if (ret)
> +		return ret;
> +
> +	if (val > ctrl->num_din_ports)
> +		return -EINVAL;
> +
> +	ctrl->num_din_ports = val;
> +
> +	ret = of_property_read_u32(np, "qcom,dout-ports", &val);
> +	if (ret)
> +		return ret;
> +
> +	if (val > ctrl->num_dout_ports)
> +		return -EINVAL;
> +
> +	ctrl->num_dout_ports = val;
> +
> +	nports = ctrl->num_dout_ports + ctrl->num_din_ports;
> +
> +	ret = of_property_read_u8_array(np, "qcom,ports-offset1",
> +					off1, nports);
> +	if (ret)
> +		return ret;
> +
> +	ret = of_property_read_u8_array(np, "qcom,ports-offset2",
> +					off2, nports);
> +	if (ret)
> +		return ret;
> +
> +	ret = of_property_read_u8_array(np, "qcom,ports-sinterval-low",
> +					si, nports);
> +	if (ret)
> +		return ret;
> +
> +	for (i = 0; i < nports; i++) {
> +		ctrl->pconfig[i].si = si[i];
> +		ctrl->pconfig[i].off1 = off1[i];
> +		ctrl->pconfig[i].off2 = off2[i];
> +	}

Using array I spoke of earlier leads to brackets being redundant here.

> +
> +	return 0;
> +}
> +
> +static int qcom_swrm_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct sdw_master_prop *prop;
> +	struct sdw_bus_params *params;
> +	struct qcom_swrm_ctrl *ctrl;
> +	struct resource *res;
> +	int ret;
> +
> +	ctrl = devm_kzalloc(dev, sizeof(*ctrl), GFP_KERNEL);
> +	if (!ctrl)
> +		return -ENOMEM;
> +
> +	if (dev->parent->bus == &slimbus_bus) {
> +		ctrl->reg_read = qcom_swrm_slim_reg_read;
> +		ctrl->reg_write = qcom_swrm_slim_reg_write;
> +		ctrl->regmap = dev_get_regmap(dev->parent, NULL);
> +		if (!ctrl->regmap)
> +			return -EINVAL;
> +	} else {
> +		ctrl->reg_read = qcom_swrm_mmio_reg_read;
> +		ctrl->reg_write = qcom_swrm_mmio_reg_write;
> +		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +		ctrl->base = devm_ioremap_resource(dev, res);
> +		if (IS_ERR(ctrl->base))
> +			return PTR_ERR(ctrl->base);
> +	}
> +
> +	ctrl->irq = platform_get_irq(pdev, 0);
> +	if (ctrl->irq < 0)
> +		return ctrl->irq;
> +
> +	ctrl->hclk = devm_clk_get(dev, "iface");
> +	if (IS_ERR(ctrl->hclk))
> +		return PTR_ERR(ctrl->hclk);
> +
> +	clk_prepare_enable(ctrl->hclk);
> +
> +	ctrl->dev = dev;
> +	dev_set_drvdata(&pdev->dev, ctrl);
> +	init_completion(&ctrl->sp_cmd_comp);
> +	mutex_init(&ctrl->lock);
> +	mutex_init(&ctrl->port_lock);
> +	INIT_WORK(&ctrl->slave_work, qcom_swrm_slave_wq);
> +
> +	ctrl->bus.dev = dev;
> +	ctrl->bus.ops = &qcom_swrm_ops;
> +	ctrl->bus.port_ops = &qcom_swrm_port_ops;
> +	ctrl->bus.compute_params = &qcom_swrm_compute_params;
> +
> +	ret = qcom_swrm_get_port_config(ctrl);
> +	if (ret)
> +		return ret;
> +
> +	params = &ctrl->bus.params;
> +	params->max_dr_freq = DEFAULT_CLK_FREQ;
> +	params->curr_dr_freq = DEFAULT_CLK_FREQ;
> +	params->col = SWRM_DEFAULT_COL;
> +	params->row = SWRM_DEFAULT_ROWS;
> +	params->curr_bank = ctrl->reg_read(ctrl, SWRM_MCP_STATUS) &
> +				SWRM_MCP_STATUS_BANK_NUM_MASK;
> +	params->next_bank = !params->curr_bank;
> +
> +	prop = &ctrl->bus.prop;
> +	prop->max_clk_freq = DEFAULT_CLK_FREQ;
> +	prop->num_clk_gears = 0;
> +	prop->num_clk_freq = MAX_FREQ_NUM;
> +	prop->clk_freq = &qcom_swrm_freq_tbl[0];
> +	prop->default_col = SWRM_DEFAULT_COL;
> +	prop->default_row = SWRM_DEFAULT_ROWS;
> +
> +	ctrl->version = ctrl->reg_read(ctrl, SWRM_COMP_HW_VERSION);
> +
> +	ret = devm_request_threaded_irq(dev, ctrl->irq, NULL,
> +					qcom_swrm_irq_handler,
> +					IRQF_TRIGGER_RISING,
> +					"soundwire", ctrl);
> +	if (ret) {
> +		dev_err(dev, "Failed to request soundwire irq\n");
> +		goto err;
> +	}
> +
> +	ret = sdw_add_bus_master(&ctrl->bus);
> +	if (ret) {
> +		dev_err(dev, "Failed to register Soundwire controller (%d)\n",
> +			ret);
> +		goto err;
> +	}
> +
> +	qcom_swrm_init(ctrl);
> +	ret = qcom_swrm_register_dais(ctrl);
> +	if (ret)
> +		goto err;
> +
> +	dev_info(dev, "Qualcomm Soundwire controller v%x.%x.%x Registered\n",
> +		 (ctrl->version >> 24) & 0xff, (ctrl->version >> 16) & 0xff,
> +		 ctrl->version & 0xffff);
> +
> +	return 0;
> +err:
> +	clk_disable_unprepare(ctrl->hclk);
> +	return ret;
> +}
> +
> +static int qcom_swrm_remove(struct platform_device *pdev)
> +{
> +	struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(&pdev->dev);
> +
> +	sdw_delete_bus_master(&ctrl->bus);
> +	clk_disable_unprepare(ctrl->hclk);
> +
> +	return 0;
> +}
> +
> +static int qcom_swrm_runtime_suspend(struct device *device)
> +{
> +	/* TBD */
> +	return 0;
> +}
> +
> +static int qcom_swrm_runtime_resume(struct device *device)
> +{
> +	/* TBD */
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops qcom_swrm_dev_pm_ops = {
> +	SET_RUNTIME_PM_OPS(qcom_swrm_runtime_suspend,
> +			   qcom_swrm_runtime_resume,
> +			   NULL
> +	)
> +};
> +
> +static const struct of_device_id qcom_swrm_of_match[] = {
> +	{ .compatible = "qcom,soundwire-v1.3.0",},
> +	{ .compatible = "qcom,soundwire-v1.5.0",},
> +	{ .compatible = "qcom,soundwire-v1.6.0",},
> +	{/* sentinel */},
> +};
> +
> +MODULE_DEVICE_TABLE(of, qcom_swrm_of_match);
> +
> +static struct platform_driver qcom_swrm_driver = {
> +	.probe	= &qcom_swrm_probe,
> +	.remove = &qcom_swrm_remove,
> +	.driver = {
> +		.name	= "qcom-soundwire",
> +		.of_match_table = qcom_swrm_of_match,
> +		.pm = &qcom_swrm_dev_pm_ops,
> +	}
> +};
> +module_platform_driver(qcom_swrm_driver);
> +
> +MODULE_DESCRIPTION("Qualcomm soundwire driver");
> +MODULE_LICENSE("GPL v2");
> 

Re: [RFC PATCH 6/6] soundwire: qcom: add support for SoundWire controller

[Cezary Rojewski]

On 2019-06-07 10:56, Srinivas Kandagatla wrote:
> Qualcomm SoundWire Master controller is present in most Qualcomm SoCs
> either integrated as part of WCD audio codecs via slimbus or
> as part of SOC I/O.
> 
> This patchset adds support to a very basic controller which has been
> tested with WCD934x SoundWire controller connected to WSA881x smart
> speaker amplifiers.
> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
>   drivers/soundwire/Kconfig  |   9 +
>   drivers/soundwire/Makefile |   4 +
>   drivers/soundwire/qcom.c   | 983 +++++++++++++++++++++++++++++++++++++
>   3 files changed, 996 insertions(+)
>   create mode 100644 drivers/soundwire/qcom.c
> 
> diff --git a/drivers/soundwire/Kconfig b/drivers/soundwire/Kconfig
> index 53b55b79c4af..f44d4f36dbbb 100644
> --- a/drivers/soundwire/Kconfig
> +++ b/drivers/soundwire/Kconfig
> @@ -34,4 +34,13 @@ config SOUNDWIRE_INTEL
>   	  enable this config option to get the SoundWire support for that
>   	  device.
>   
> +config SOUNDWIRE_QCOM
> +	tristate "Qualcomm SoundWire Master driver"
> +	select SOUNDWIRE_BUS
> +	depends on SND_SOC
> +	help
> +	  SoundWire Qualcomm Master driver.
> +	  If you have an Qualcomm platform which has a SoundWire Master then
> +	  enable this config option to get the SoundWire support for that
> +	  device
>   endif
> diff --git a/drivers/soundwire/Makefile b/drivers/soundwire/Makefile
> index 5817beaca0e1..f4ebfde31372 100644
> --- a/drivers/soundwire/Makefile
> +++ b/drivers/soundwire/Makefile
> @@ -16,3 +16,7 @@ obj-$(CONFIG_SOUNDWIRE_INTEL) += soundwire-intel.o
>   
>   soundwire-intel-init-objs := intel_init.o
>   obj-$(CONFIG_SOUNDWIRE_INTEL) += soundwire-intel-init.o
> +
> +#Qualcomm driver
> +soundwire-qcom-objs :=	qcom.o
> +obj-$(CONFIG_SOUNDWIRE_QCOM) += soundwire-qcom.o
> diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
> new file mode 100644
> index 000000000000..d1722d44d217
> --- /dev/null
> +++ b/drivers/soundwire/qcom.c
> @@ -0,0 +1,983 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2019, Linaro Limited
> +
> +#include <linux/clk.h>
> +#include <linux/completion.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +#include <linux/slimbus.h>
> +#include <linux/soundwire/sdw.h>
> +#include <linux/soundwire/sdw_registers.h>
> +#include <sound/pcm_params.h>
> +#include <sound/soc.h>
> +#include "bus.h"
> +

Pierre already pointed this out - SWR looks odd. During my time with 
Soundwire I've met SDW and SNDW, but it's the first time I see SWR.

> +#define SWRM_COMP_HW_VERSION					0x00
> +#define SWRM_COMP_CFG_ADDR					0x04
> +#define SWRM_COMP_CFG_IRQ_LEVEL_OR_PULSE_MSK			BIT(1)
> +#define SWRM_COMP_CFG_ENABLE_MSK				BIT(0)
> +#define SWRM_COMP_PARAMS					0x100
> +#define SWRM_COMP_PARAMS_DOUT_PORTS_MASK			GENMASK(4, 0)
> +#define SWRM_COMP_PARAMS_DIN_PORTS_MASK				GENMASK(9, 5)
> +#define SWRM_COMP_PARAMS_WR_FIFO_DEPTH				GENMASK(14, 10)
> +#define SWRM_COMP_PARAMS_RD_FIFO_DEPTH				GENMASK(19, 15)
> +#define SWRM_COMP_PARAMS_AUTO_ENUM_SLAVES			GENMASK(32. 20)
> +#define SWRM_INTERRUPT_STATUS					0x200
> +#define SWRM_INTERRUPT_STATUS_RMSK				GENMASK(16, 0)
> +#define SWRM_INTERRUPT_STATUS_SLAVE_PEND_IRQ			BIT(0)
> +#define SWRM_INTERRUPT_STATUS_NEW_SLAVE_ATTACHED		BIT(1)
> +#define SWRM_INTERRUPT_STATUS_CHANGE_ENUM_SLAVE_STATUS		BIT(2)
> +#define SWRM_INTERRUPT_STATUS_MASTER_CLASH_DET			BIT(3)
> +#define SWRM_INTERRUPT_STATUS_RD_FIFO_OVERFLOW			BIT(4)
> +#define SWRM_INTERRUPT_STATUS_RD_FIFO_UNDERFLOW			BIT(5)
> +#define SWRM_INTERRUPT_STATUS_WR_CMD_FIFO_OVERFLOW		BIT(6)
> +#define SWRM_INTERRUPT_STATUS_CMD_ERROR				BIT(7)
> +#define SWRM_INTERRUPT_STATUS_DOUT_PORT_COLLISION		BIT(8)
> +#define SWRM_INTERRUPT_STATUS_READ_EN_RD_VALID_MISMATCH		BIT(9)
> +#define SWRM_INTERRUPT_STATUS_SPECIAL_CMD_ID_FINISHED		BIT(10)
> +#define SWRM_INTERRUPT_STATUS_NEW_SLAVE_AUTO_ENUM_FINISHED	BIT(11)
> +#define SWRM_INTERRUPT_STATUS_AUTO_ENUM_FAILED			BIT(12)
> +#define SWRM_INTERRUPT_STATUS_AUTO_ENUM_TABLE_IS_FULL		BIT(13)
> +#define SWRM_INTERRUPT_STATUS_BUS_RESET_FINISHED		BIT(14)
> +#define SWRM_INTERRUPT_STATUS_CLK_STOP_FINISHED			BIT(15)
> +#define SWRM_INTERRUPT_STATUS_ERROR_PORT_TEST			BIT(16)
> +#define SWRM_INTERRUPT_MASK_ADDR				0x204
> +#define SWRM_INTERRUPT_CLEAR					0x208

You seem to shortcut every reg here similarly to how it's done in SDW 
spec. INTERRUPT is represented by INT there, and by doing so, this 
define block would look more like a real family.

> +#define SWRM_CMD_FIFO_WR_CMD					0x300
> +#define SWRM_CMD_FIFO_RD_CMD					0x304
> +#define SWRM_CMD_FIFO_CMD					0x308
> +#define SWRM_CMD_FIFO_STATUS					0x30C
> +#define SWRM_CMD_FIFO_CFG_ADDR					0x314
> +#define SWRM_CMD_FIFO_CFG_NUM_OF_CMD_RETRY_SHFT			0x0
> +#define SWRM_CMD_FIFO_RD_FIFO_ADDR				0x318
> +#define SWRM_ENUMERATOR_CFG_ADDR				0x500
> +#define SWRM_MCP_FRAME_CTRL_BANK_ADDR(m)		(0x101C + 0x40 * (m))
> +#define SWRM_MCP_FRAME_CTRL_BANK_SSP_PERIOD_SHFT		16
> +#define SWRM_MCP_FRAME_CTRL_BANK_ROW_CTRL_SHFT			3
> +#define SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_BMSK			GENMASK(2, 0)
> +#define SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_SHFT			0
> +#define SWRM_MCP_CFG_ADDR					0x1048
> +#define SWRM_MCP_CFG_MAX_NUM_OF_CMD_NO_PINGS_BMSK		GENMASK(21, 17)
> +#define SWRM_MCP_CFG_MAX_NUM_OF_CMD_NO_PINGS_SHFT		0x11
> +#define SWRM_MCP_STATUS						0x104C
> +#define SWRM_MCP_STATUS_BANK_NUM_MASK				BIT(0)
> +#define SWRM_MCP_SLV_STATUS					0x1090
> +#define SWRM_MCP_SLV_STATUS_MASK				GENMASK(1, 0)
> +#define SWRM_DP_PORT_CTRL_BANK(n, m)	(0x1124 + 0x100 * (n - 1) + 0x40 * m)

Some of these you align, others leave with the equal amount of tabs 
despite different widths.

> +#define SWRM_DP_PORT_CTRL2_BANK(n, m)	(0x1126 + 0x100 * (n - 1) + 0x40 * m)

Consider reusing _CTRL_ and simply adding offset for 2_.

> +#define SWRM_DP_PORT_CTRL_EN_CHAN_SHFT				0x18
> +#define SWRM_DP_PORT_CTRL_OFFSET2_SHFT				0x10
> +#define SWRM_DP_PORT_CTRL_OFFSET1_SHFT				0x08
> +#define SWRM_AHB_BRIDGE_WR_DATA_0				0xc885
> +#define SWRM_AHB_BRIDGE_WR_ADDR_0				0xc889
> +#define SWRM_AHB_BRIDGE_RD_ADDR_0				0xc88d
> +#define SWRM_AHB_BRIDGE_RD_DATA_0				0xc891
> +
> +#define SWRM_REG_VAL_PACK(data, dev, id, reg)	\
> +			((reg) | ((id) << 16) | ((dev) << 20) | ((data) << 24))
> +
> +#define SWRM_MAX_ROW_VAL	0 /* Rows = 48 */
> +#define SWRM_DEFAULT_ROWS	48
> +#define SWRM_MIN_COL_VAL	0 /* Cols = 2 */
> +#define SWRM_DEFAULT_COL	16
> +#define SWRM_SPECIAL_CMD_ID	0xF
> +#define MAX_FREQ_NUM		1
> +#define TIMEOUT_MS		1000
> +#define QCOM_SWRM_MAX_RD_LEN	0xf
> +#define DEFAULT_CLK_FREQ	9600000
> +#define SWRM_MAX_DAIS		0xF

Given the scale of this block, it might be good to reiterate all defines 
and see if indeed all are QCom specific. Maybe some could be replaced by 
equivalents from common code.

> +
> +struct qcom_swrm_port_config {
> +	u8 si;
> +	u8 off1;
> +	u8 off2;
> +};
> +
> +struct qcom_swrm_ctrl {
> +	struct sdw_bus bus;
> +	struct device *dev;
> +	struct regmap *regmap;
> +	struct completion sp_cmd_comp;
> +	struct work_struct slave_work;
> +	/* read/write lock */
> +	struct mutex lock;
> +	/* Port alloc/free lock */
> +	struct mutex port_lock;
> +	struct clk *hclk;
> +	int fifo_status;
> +	void __iomem *base;
> +	u8 wr_cmd_id;
> +	u8 rd_cmd_id;
> +	int irq;
> +	unsigned int version;

Given the fact you don't use version field directly and always shift it, 
I'd consider making use of union here to listing version bits 
explicitly. Overall size won't change.

> +	int num_din_ports;
> +	int num_dout_ports;
> +	unsigned long dout_port_mask;
> +	unsigned long din_port_mask;
> +	struct qcom_swrm_port_config pconfig[SDW_MAX_PORTS];
> +	struct sdw_stream_runtime *sruntime[SWRM_MAX_DAIS];
> +	enum sdw_slave_status status[SDW_MAX_DEVICES];
> +	u32 (*reg_read)(struct qcom_swrm_ctrl *ctrl, int reg);
> +	int (*reg_write)(struct qcom_swrm_ctrl *ctrl, int reg, int val);
> +};
> +
> +#define to_qcom_sdw(b)	container_of(b, struct qcom_swrm_ctrl, bus)
> +
> +struct usecase {
> +	u8 num_port;
> +	u8 num_ch;
> +	u32 chrate;
> +};
> +

"usecase" looks ambiguous at best.

> +static u32 qcom_swrm_slim_reg_read(struct qcom_swrm_ctrl *ctrl, int reg)
> +{
> +	struct regmap *wcd_regmap = ctrl->regmap;
> +	u32 val = 0, ret;
> +
> +	/* pg register + offset */
> +	regmap_bulk_write(wcd_regmap, SWRM_AHB_BRIDGE_RD_ADDR_0,
> +			  (u8 *)&reg, 4);
> +
> +	ret = regmap_bulk_read(wcd_regmap, SWRM_AHB_BRIDGE_RD_DATA_0,
> +			       (u8 *)&val, 4);
> +	if (ret < 0)
> +		dev_err(ctrl->dev, "Read Failure (%d)\n", ret);
> +
> +	return val;
> +}
> +
> +static u32 qcom_swrm_mmio_reg_read(struct qcom_swrm_ctrl *ctrl, int reg)
> +{
> +	return readl_relaxed(ctrl->base + reg);
> +}
> +
> +static int qcom_swrm_mmio_reg_write(struct qcom_swrm_ctrl *ctrl,
> +				    int reg, int val)
> +{
> +	writel_relaxed(val, ctrl->base + reg);
> +
> +	return 0;
> +}
> +
> +static int qcom_swrm_slim_reg_write(struct qcom_swrm_ctrl *ctrl,
> +				    int reg, int val)
> +{
> +	struct regmap *wcd_regmap = ctrl->regmap;
> +
> +	/* pg register + offset */
> +	regmap_bulk_write(wcd_regmap, SWRM_AHB_BRIDGE_WR_DATA_0,
> +			  (u8 *)&val, 4);
> +	/* write address register */
> +	regmap_bulk_write(wcd_regmap, SWRM_AHB_BRIDGE_WR_ADDR_0,
> +			  (u8 *)&reg, 4);
> +
> +	return 0;
> +}

Ok, so you choose to declare write op as returning "int" yet either it 
cannot do so (void writel_relaxed) or ret is completely ignored 
(regmap_bulk_write does return an int value). Either switch to void or 
check against returned value whenever possible.

> +
> +static int qcom_swrm_cmd_fifo_wr_cmd(struct qcom_swrm_ctrl *ctrl, u8 cmd_data,
> +				     u8 dev_addr, u16 reg_addr)
> +{
> +	int ret = 0;
> +	u8 cmd_id;
> +	u32 val;
> +
> +	mutex_lock(&ctrl->lock);
> +	if (dev_addr == SDW_BROADCAST_DEV_NUM) {
> +		cmd_id = SWRM_SPECIAL_CMD_ID;
> +	} else {
> +		if (++ctrl->wr_cmd_id == SWRM_SPECIAL_CMD_ID)
> +			ctrl->wr_cmd_id = 0;
> +
> +		cmd_id = ctrl->wr_cmd_id;
> +	}
> +
> +	val = SWRM_REG_VAL_PACK(cmd_data, dev_addr, cmd_id, reg_addr);
> +	ret = ctrl->reg_write(ctrl, SWRM_CMD_FIFO_WR_CMD, val);
> +	if (ret < 0) {
> +		dev_err(ctrl->dev, "%s: reg 0x%x write failed, err:%d\n",
> +			__func__, val, ret);
> +		goto err;
> +	}
> +
> +	if (dev_addr == SDW_BROADCAST_DEV_NUM) {
> +		ctrl->fifo_status = 0;
> +		ret = wait_for_completion_timeout(&ctrl->sp_cmd_comp,
> +						  msecs_to_jiffies(TIMEOUT_MS));
> +
> +		if (!ret || ctrl->fifo_status) {
> +			dev_err(ctrl->dev, "reg 0x%x write failed\n", val);

Both, this and err msg above are generic enough to be put into goto to 
save some space.

> +			ret = -ENODATA;
> +			goto err;
> +		}
> +	}
> +err:
> +	mutex_unlock(&ctrl->lock);
> +	return ret;
> +}
> +
> +static int qcom_swrm_cmd_fifo_rd_cmd(struct qcom_swrm_ctrl *ctrl,
> +				     u8 dev_addr, u16 reg_addr,
> +				     u32 len, u8 *rval)
> +{
> +	int i, ret = 0;
> +	u8 cmd_id = 0;
> +	u32 val;
> +
> +	mutex_lock(&ctrl->lock);
> +	if (dev_addr == SDW_ENUM_DEV_NUM) {
> +		cmd_id = SWRM_SPECIAL_CMD_ID;
> +	} else {
> +		if (++ctrl->rd_cmd_id == SWRM_SPECIAL_CMD_ID)
> +			ctrl->rd_cmd_id = 0;
> +
> +		cmd_id = ctrl->rd_cmd_id;
> +	}
> +
> +	val = SWRM_REG_VAL_PACK(len, dev_addr, cmd_id, reg_addr);
> +	ret = ctrl->reg_write(ctrl, SWRM_CMD_FIFO_RD_CMD, val);
> +	if (ret < 0) {
> +		dev_err(ctrl->dev, "reg 0x%x write failed err:%d\n", val, ret);

Same for _rt_.

> +		goto err;
> +	}
> +
> +	if (dev_addr == SDW_ENUM_DEV_NUM) {
> +		ctrl->fifo_status = 0;
> +		ret = wait_for_completion_timeout(&ctrl->sp_cmd_comp,
> +						  msecs_to_jiffies(TIMEOUT_MS));
> +
> +		if (!ret || ctrl->fifo_status) {
> +			dev_err(ctrl->dev, "reg 0x%x read failed\n", val);

Just to be sure. It is really "read" that is failing here?

> +			ret = -ENODATA;
> +			goto err;
> +		}
> +	}
> +
> +	for (i = 0; i < len; i++) {
> +		rval[i] = ctrl->reg_read(ctrl, SWRM_CMD_FIFO_RD_FIFO_ADDR);
> +		rval[i] &= 0xFF;
> +	}
> +
> +err:
> +	mutex_unlock(&ctrl->lock);
> +	return ret;
> +}
> +
> +static void qcom_swrm_get_device_status(struct qcom_swrm_ctrl *ctrl)
> +{
> +	u32 val = ctrl->reg_read(ctrl, SWRM_MCP_SLV_STATUS);
> +	int i;
> +
> +	for (i = 1; i < SDW_MAX_DEVICES; i++) {
> +		u32 s;
> +
> +		s = (val >> (i * 2));
> +		s &= SWRM_MCP_SLV_STATUS_MASK;
> +		ctrl->status[i] = s;
> +	}
> +}
> +
> +static irqreturn_t qcom_swrm_irq_handler(int irq, void *dev_id)
> +{
> +	struct qcom_swrm_ctrl *ctrl = dev_id;
> +	u32 sts, value;
> +
> +	sts = ctrl->reg_read(ctrl, SWRM_INTERRUPT_STATUS);
> +
> +	if (sts & SWRM_INTERRUPT_STATUS_SPECIAL_CMD_ID_FINISHED)
> +		complete(&ctrl->sp_cmd_comp);
> +
> +	if (sts & SWRM_INTERRUPT_STATUS_CMD_ERROR) {
> +		value = ctrl->reg_read(ctrl, SWRM_CMD_FIFO_STATUS);
> +		dev_err_ratelimited(ctrl->dev,
> +				    "CMD error, fifo status 0x%x\n",
> +				     value);
> +		ctrl->reg_write(ctrl, SWRM_CMD_FIFO_CMD, 0x1);
> +		if ((value & 0xF) == 0xF) {
> +			ctrl->fifo_status = -ENODATA;
> +			complete(&ctrl->sp_cmd_comp);
> +		}
> +	}
> +
> +	if ((sts & SWRM_INTERRUPT_STATUS_NEW_SLAVE_ATTACHED) ||
> +	    sts & SWRM_INTERRUPT_STATUS_CHANGE_ENUM_SLAVE_STATUS) {
> +		if (sts & SWRM_INTERRUPT_STATUS_NEW_SLAVE_ATTACHED)
> +			ctrl->status[0] = SDW_SLAVE_ATTACHED;
> +
> +		schedule_work(&ctrl->slave_work);
> +	}
> +
> +	if (sts & SWRM_INTERRUPT_STATUS_SLAVE_PEND_IRQ)
> +		dev_dbg(ctrl->dev, "Slave pend irq\n");
> +
> +	if (sts & SWRM_INTERRUPT_STATUS_NEW_SLAVE_ATTACHED)
> +		dev_dbg(ctrl->dev, "New slave attached\n");
> +
> +	if (sts & SWRM_INTERRUPT_STATUS_MASTER_CLASH_DET)
> +		dev_err_ratelimited(ctrl->dev, "Bus clash detected\n");
> +
> +	if (sts & SWRM_INTERRUPT_STATUS_RD_FIFO_OVERFLOW)
> +		dev_err(ctrl->dev, "Read FIFO overflow\n");
> +
> +	if (sts & SWRM_INTERRUPT_STATUS_RD_FIFO_UNDERFLOW)
> +		dev_err(ctrl->dev, "Read FIFO underflow\n");
> +
> +	if (sts & SWRM_INTERRUPT_STATUS_WR_CMD_FIFO_OVERFLOW)
> +		dev_err(ctrl->dev, "Write FIFO overflow\n");
> +
> +	if (sts & SWRM_INTERRUPT_STATUS_DOUT_PORT_COLLISION)
> +		dev_err(ctrl->dev, "Port collision detected\n");
> +
> +	if (sts & SWRM_INTERRUPT_STATUS_READ_EN_RD_VALID_MISMATCH)
> +		dev_err(ctrl->dev, "Read enable valid mismatch\n");
> +
> +	if (sts & SWRM_INTERRUPT_STATUS_SPECIAL_CMD_ID_FINISHED)
> +		dev_err(ctrl->dev, "Cmd id finished\n");
> +
> +	if (sts & SWRM_INTERRUPT_STATUS_BUS_RESET_FINISHED)
> +		dev_err(ctrl->dev, "Bus reset finished\n");

If you do not handle these errors at all, consider declaring 
ERROR-message table. I believe leaving erroneus status as is may lead to 
fatal consequences. If there is no intention for handling even the most 
critical cases, please add TODO/ comment here.

> +
> +	ctrl->reg_write(ctrl, SWRM_INTERRUPT_CLEAR, sts);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int qcom_swrm_init(struct qcom_swrm_ctrl *ctrl)
> +{
> +	u32 val;
> +	u8 row_ctrl = SWRM_MAX_ROW_VAL;
> +	u8 col_ctrl = SWRM_MIN_COL_VAL;
> +	u8 ssp_period = 1;
> +	u8 retry_cmd_num = 3;
> +
> +	/* Clear Rows and Cols */
> +	val = ((row_ctrl << SWRM_MCP_FRAME_CTRL_BANK_ROW_CTRL_SHFT) |
> +		(col_ctrl << SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_SHFT) |
> +		(ssp_period << SWRM_MCP_FRAME_CTRL_BANK_SSP_PERIOD_SHFT));
> +
> +	ctrl->reg_write(ctrl, SWRM_MCP_FRAME_CTRL_BANK_ADDR(0), val);
> +
> +	/* Disable Auto enumeration */
> +	ctrl->reg_write(ctrl, SWRM_ENUMERATOR_CFG_ADDR, 0);
> +
> +	/* Mask soundwire interrupts */
> +	ctrl->reg_write(ctrl, SWRM_INTERRUPT_MASK_ADDR,
> +					SWRM_INTERRUPT_STATUS_RMSK);
> +
> +	/* Configure No pings */
> +	val = ctrl->reg_read(ctrl, SWRM_MCP_CFG_ADDR);
> +
> +	val &= ~SWRM_MCP_CFG_MAX_NUM_OF_CMD_NO_PINGS_BMSK;
> +	val |= (0x1f << SWRM_MCP_CFG_MAX_NUM_OF_CMD_NO_PINGS_SHFT);
> +	ctrl->reg_write(ctrl, SWRM_MCP_CFG_ADDR, val);
> +
> +	/* Configure number of retries of a read/write cmd */
> +	val = (retry_cmd_num << SWRM_CMD_FIFO_CFG_NUM_OF_CMD_RETRY_SHFT);
> +	ctrl->reg_write(ctrl, SWRM_CMD_FIFO_CFG_ADDR, val);
> +
> +	/* Set IRQ to PULSE */
> +	ctrl->reg_write(ctrl, SWRM_COMP_CFG_ADDR,
> +				SWRM_COMP_CFG_IRQ_LEVEL_OR_PULSE_MSK |
> +				SWRM_COMP_CFG_ENABLE_MSK);
> +	return 0;

As in my previous comment, you should check against ret from reg_write 
if void approach is not chosen.

> +}
> +
> +static enum sdw_command_response qcom_swrm_xfer_msg(struct sdw_bus *bus,
> +						    struct sdw_msg *msg)
> +{
> +	struct qcom_swrm_ctrl *ctrl = to_qcom_sdw(bus);
> +	int ret, i, len;
> +
> +	if (msg->flags == SDW_MSG_FLAG_READ) {
> +		for (i = 0; i < msg->len;) {
> +			if ((msg->len - i) < QCOM_SWRM_MAX_RD_LEN)
> +				len = msg->len - i;
> +			else
> +				len = QCOM_SWRM_MAX_RD_LEN;
> +
> +			ret = qcom_swrm_cmd_fifo_rd_cmd(ctrl, msg->dev_num,
> +							msg->addr + i, len,
> +						       &msg->buf[i]);
> +			if (ret < 0) {
> +				if (ret == -ENODATA)
> +					return SDW_CMD_IGNORED;
> +
> +				return ret;
> +			}
> +
> +			i = i + len;

Any reason for inlining this incrementation? If _rd_ fails, we leave the 
loop anyway.

> +		}
> +	} else if (msg->flags == SDW_MSG_FLAG_WRITE) {
> +		for (i = 0; i < msg->len; i++) {
> +			ret = qcom_swrm_cmd_fifo_wr_cmd(ctrl, msg->buf[i],
> +							msg->dev_num,
> +						       msg->addr + i);
> +			if (ret < 0) {
> +				if (ret == -ENODATA)
> +					return SDW_CMD_IGNORED;
> +
> +				return ret;
> +			}
> +		}
> +	}
> +
> +	return SDW_CMD_OK;
> +}
> +
> +static int qcom_swrm_pre_bank_switch(struct sdw_bus *bus)
> +{
> +	u32 reg = SWRM_MCP_FRAME_CTRL_BANK_ADDR(bus->params.next_bank);
> +	struct qcom_swrm_ctrl *ctrl = to_qcom_sdw(bus);
> +	u32 val;
> +
> +	val = ctrl->reg_read(ctrl, reg);
> +	val |= ((0 << SWRM_MCP_FRAME_CTRL_BANK_ROW_CTRL_SHFT) |
> +		(7l << SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_SHFT));
> +	ctrl->reg_write(ctrl, reg, val);
> +
> +	return 0;

s/return 0/return ctrl->reg_write(ctrl, reg, val)/

> +}
> +
> +static int qcom_swrm_port_params(struct sdw_bus *bus,
> +				 struct sdw_port_params *p_params,
> +				unsigned int bank)
> +{
> +	/* TBD */
> +	return 0;
> +}
> +
> +static int qcom_swrm_transport_params(struct sdw_bus *bus,
> +				      struct sdw_transport_params *params,
> +				     enum sdw_reg_bank bank)
> +{
> +	struct qcom_swrm_ctrl *ctrl = to_qcom_sdw(bus);
> +	u32 value;
> +
> +	value = params->offset1 << SWRM_DP_PORT_CTRL_OFFSET1_SHFT;
> +	value |= params->offset2 << SWRM_DP_PORT_CTRL_OFFSET2_SHFT;
> +	value |= params->sample_interval - 1;
> +
> +	ctrl->reg_write(ctrl, SWRM_DP_PORT_CTRL_BANK((params->port_num), bank),
> +			value);
> +
> +	return 0;

Another "return issue" here.

> +}
> +
> +static int qcom_swrm_port_enable(struct sdw_bus *bus,
> +				 struct sdw_enable_ch *enable_ch,
> +				unsigned int bank)
> +{
> +	u32 reg = SWRM_DP_PORT_CTRL_BANK(enable_ch->port_num, bank);
> +	struct qcom_swrm_ctrl *ctrl = to_qcom_sdw(bus);
> +	u32 val;
> +
> +	val = ctrl->reg_read(ctrl, reg);
> +	if (enable_ch->enable)
> +		val |= (enable_ch->ch_mask << SWRM_DP_PORT_CTRL_EN_CHAN_SHFT);
> +	else
> +		val &= ~(enable_ch->ch_mask << SWRM_DP_PORT_CTRL_EN_CHAN_SHFT);
> +
> +	ctrl->reg_write(ctrl, reg, val);
> +
> +	return 0;
> +}
> +
> +static struct sdw_master_port_ops qcom_swrm_port_ops = {
> +	.dpn_set_port_params = qcom_swrm_port_params,
> +	.dpn_set_port_transport_params = qcom_swrm_transport_params,
> +	.dpn_port_enable_ch = qcom_swrm_port_enable,
> +};
> +
> +static struct sdw_master_ops qcom_swrm_ops = {
> +	.xfer_msg = qcom_swrm_xfer_msg,
> +	.pre_bank_switch = qcom_swrm_pre_bank_switch,
> +};
> +
> +static int qcom_swrm_compute_params(struct sdw_bus *bus)
> +{
> +	struct qcom_swrm_ctrl *ctrl = to_qcom_sdw(bus);
> +	struct sdw_master_runtime *m_rt;
> +	struct sdw_slave_runtime *s_rt;
> +	struct sdw_port_runtime *p_rt;
> +	int i = 0;
> +
> +	list_for_each_entry(m_rt, &bus->m_rt_list, bus_node) {
> +		list_for_each_entry(p_rt, &m_rt->port_list, port_node) {
> +			p_rt->transport_params.port_num = p_rt->num;
> +			p_rt->transport_params.sample_interval =
> +					ctrl->pconfig[p_rt->num - 1].si + 1;
> +			p_rt->transport_params.offset1 =
> +					ctrl->pconfig[p_rt->num - 1].off1;
> +			p_rt->transport_params.offset2 =
> +					ctrl->pconfig[p_rt->num - 1].off2;

ctrl->pconfig[ <idx> ] colleagues bellow make use of local index 
variable which clearly makes it more readable.

> +		}
> +
> +		list_for_each_entry(s_rt, &m_rt->slave_rt_list, m_rt_node) {
> +			list_for_each_entry(p_rt, &s_rt->port_list, port_node) {
> +				p_rt->transport_params.port_num = p_rt->num;
> +				p_rt->transport_params.sample_interval =
> +						ctrl->pconfig[i].si + 1;
> +				p_rt->transport_params.offset1 =
> +						ctrl->pconfig[i].off1;
> +				p_rt->transport_params.offset2 =
> +						ctrl->pconfig[i].off2;
> +				i++;
> +			}
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static u32 qcom_swrm_freq_tbl[MAX_FREQ_NUM] = {
> +	DEFAULT_CLK_FREQ,
> +};
> +
> +static void qcom_swrm_slave_wq(struct work_struct *work)
> +{
> +	struct qcom_swrm_ctrl *ctrl =
> +			container_of(work, struct qcom_swrm_ctrl, slave_work);
> +
> +	qcom_swrm_get_device_status(ctrl);
> +	sdw_handle_slave_status(&ctrl->bus, ctrl->status);
> +}
> +
> +static int qcom_swrm_prepare(struct snd_pcm_substream *substream,
> +			     struct snd_soc_dai *dai)
> +{
> +	struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(dai->dev);
> +
> +	if (!ctrl->sruntime[dai->id])
> +		return -EINVAL;
> +
> +	return sdw_enable_stream(ctrl->sruntime[dai->id]);
> +}
> +
> +static void qcom_swrm_stream_free_ports(struct qcom_swrm_ctrl *ctrl,
> +					struct sdw_stream_runtime *stream)
> +{
> +	struct sdw_master_runtime *m_rt;
> +	struct sdw_port_runtime *p_rt;
> +	unsigned long *port_mask;
> +
> +	mutex_lock(&ctrl->port_lock);
> +
> +	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
> +		if (m_rt->direction == SDW_DATA_DIR_RX)
> +			port_mask = &ctrl->dout_port_mask;
> +		else
> +			port_mask = &ctrl->din_port_mask;
> +
> +		list_for_each_entry(p_rt, &m_rt->port_list, port_node) {
> +			clear_bit(p_rt->num - 1, port_mask);
> +		}

Unnecessary brackets.

> +	}
> +
> +	mutex_unlock(&ctrl->port_lock);
> +}
> +
> +static int qcom_swrm_stream_alloc_ports(struct qcom_swrm_ctrl *ctrl,
> +					struct sdw_stream_runtime *stream,
> +				       struct snd_pcm_hw_params *params,
> +				       int direction)
> +{
> +	struct sdw_port_config pconfig[SDW_MAX_PORTS];
> +	struct sdw_stream_config sconfig;
> +	struct sdw_master_runtime *m_rt;
> +	struct sdw_slave_runtime *s_rt;
> +	struct sdw_port_runtime *p_rt;
> +	unsigned long *port_mask;
> +	int i, maxport, pn, nports = 0, ret = 0;
> +
> +	mutex_lock(&ctrl->port_lock);
> +	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
> +		if (m_rt->direction == SDW_DATA_DIR_RX) {
> +			maxport = ctrl->num_dout_ports;
> +			port_mask = &ctrl->dout_port_mask;
> +		} else {
> +			maxport = ctrl->num_din_ports;
> +			port_mask = &ctrl->din_port_mask;
> +		}
> +
> +		list_for_each_entry(s_rt, &m_rt->slave_rt_list, m_rt_node) {
> +			list_for_each_entry(p_rt, &s_rt->port_list, port_node) {
> +				/* Port numbers start from 1 - 14*/
> +				pn = find_first_zero_bit(port_mask, maxport);
> +				if (pn > (maxport - 1)) {
> +					dev_err(ctrl->dev, "All ports busy\n");
> +					ret = -EBUSY;
> +					goto err;
> +				}
> +				set_bit(pn, port_mask);
> +				pconfig[nports].num = pn + 1;
> +				pconfig[nports].ch_mask = p_rt->ch_mask;
> +				nports++;
> +			}
> +		}
> +	}
> +
> +	if (direction == SNDRV_PCM_STREAM_CAPTURE)
> +		sconfig.direction = SDW_DATA_DIR_TX;
> +	else
> +		sconfig.direction = SDW_DATA_DIR_RX;
> +
> +	sconfig.ch_count = 1;
> +	sconfig.frame_rate = params_rate(params);
> +	sconfig.type = stream->type;
> +	sconfig.bps = 1;

Hmm. frame_rate and type gets assigned based on "input" data yet the 
rest is hardcoded. Is this intended?

> +	sdw_stream_add_master(&ctrl->bus, &sconfig, pconfig,
> +			      nports, stream);
> +err:
> +	if (ret) {
> +		for (i = 0; i < nports; i++)
> +			clear_bit(pconfig[i].num - 1, port_mask);
> +	}
> +
> +	mutex_unlock(&ctrl->port_lock);
> +
> +	return ret;
> +}
> +
> +static int qcom_swrm_hw_params(struct snd_pcm_substream *substream,
> +			       struct snd_pcm_hw_params *params,
> +			      struct snd_soc_dai *dai)
> +{
> +	struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(dai->dev);
> +	int ret;
> +
> +	ret = qcom_swrm_stream_alloc_ports(ctrl, ctrl->sruntime[dai->id],
> +					   params, substream->stream);
> +	if (ret)
> +		return ret;
> +
> +	return sdw_prepare_stream(ctrl->sruntime[dai->id]);
> +}
> +
> +static int qcom_swrm_hw_free(struct snd_pcm_substream *substream,
> +			     struct snd_soc_dai *dai)
> +{
> +	struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(dai->dev);
> +
> +	qcom_swrm_stream_free_ports(ctrl, ctrl->sruntime[dai->id]);
> +	sdw_stream_remove_master(&ctrl->bus, ctrl->sruntime[dai->id]);
> +	sdw_deprepare_stream(ctrl->sruntime[dai->id]);
> +	sdw_disable_stream(ctrl->sruntime[dai->id]);

Declaring local variable initialized with ctrl->sruntime[dai->id] should 
prove more readable.

> +
> +	return 0;
> +}
> +
> +static void *qcom_pdm_get_sdw_stream(struct snd_soc_dai *dai,
> +				     int direction)
> +{
> +	struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(dai->dev);
> +
> +	return ctrl->sruntime[dai->id];
> +}
> +
> +static int qcom_pdm_set_sdw_stream(struct snd_soc_dai *dai,
> +				   void *stream, int direction)
> +{
> +	return 0;
> +}
> +
> +static int qcom_swrm_startup(struct snd_pcm_substream *stream,
> +			     struct snd_soc_dai *dai)
> +{
> +	struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(dai->dev);
> +	struct sdw_stream_runtime *sruntime;
> +
> +	sruntime = sdw_alloc_stream(dai->name);
> +	if (!sruntime)
> +		return -ENOMEM;
> +
> +	ctrl->sruntime[dai->id] = sruntime;
> +
> +	return 0;
> +}
> +
> +static void qcom_swrm_shutdown(struct snd_pcm_substream *stream,
> +			       struct snd_soc_dai *dai)
> +{
> +	struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(dai->dev);
> +
> +	sdw_release_stream(ctrl->sruntime[dai->id]);
> +	ctrl->sruntime[dai->id] = NULL;
> +}
> +
> +static const struct snd_soc_dai_ops qcom_swrm_pdm_dai_ops = {
> +	.hw_params = qcom_swrm_hw_params,
> +	.prepare = qcom_swrm_prepare,
> +	.hw_free = qcom_swrm_hw_free,
> +	.startup = qcom_swrm_startup,
> +	.shutdown = qcom_swrm_shutdown,
> +	.set_sdw_stream = qcom_pdm_set_sdw_stream,
> +	.get_sdw_stream = qcom_pdm_get_sdw_stream,
> +};
> +
> +static const struct snd_soc_component_driver qcom_swrm_dai_component = {
> +	.name           = "soundwire",
> +};
> +
> +static int qcom_swrm_register_dais(struct qcom_swrm_ctrl *ctrl)
> +{
> +	int num_dais = ctrl->num_dout_ports + ctrl->num_din_ports;
> +	struct snd_soc_dai_driver *dais;
> +	int i;
> +
> +	/* PDM dais are only tested for now */
> +	dais = devm_kcalloc(ctrl->dev, num_dais, sizeof(*dais), GFP_KERNEL);
> +	if (!dais)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < num_dais; i++) {
> +		dais[i].name = kasprintf(GFP_KERNEL, "SDW Pin%d", i);
> +		if (i < ctrl->num_dout_ports) {
> +			dais[i].playback.stream_name = kasprintf(GFP_KERNEL,
> +								 "SDW Tx%d", i);
> +			if (!dais[i].playback.stream_name) {
> +				kfree(dais[i].name);
> +				return -ENOMEM;

Now this got me worried. What about memory allocated in iterations 
before the failure? It must be freed in error handling path. goto should 
be of help here.

> +			}
> +			dais[i].playback.channels_min = 1;
> +			dais[i].playback.channels_max = 1;
> +			dais[i].playback.rates = SNDRV_PCM_RATE_48000;
> +			dais[i].playback.formats = SNDRV_PCM_FMTBIT_S16_LE;

All of these formats are hardcoded. Consider declaring a "template" 
format above and simply initialize each dai with it.

> +		} else {
> +			dais[i].capture.stream_name = kasprintf(GFP_KERNEL,
> +								"SDW Rx%d", i);
> +			if (!dais[i].capture.stream_name) {
> +				kfree(dais[i].name);
> +				kfree(dais[i].playback.stream_name);
> +				return -ENOMEM;

Same memory deallocation issue here.

> +			}
> +
> +			dais[i].capture.channels_min = 1;
> +			dais[i].capture.channels_max = 1;
> +			dais[i].capture.rates = SNDRV_PCM_RATE_48000;
> +			dais[i].capture.formats = SNDRV_PCM_FMTBIT_S16_LE;

Comment regarding playback dai initialization applies here too.

> +		}
> +		dais[i].ops = &qcom_swrm_pdm_dai_ops;
> +		dais[i].id = i;
> +	}
> +
> +	return devm_snd_soc_register_component(ctrl->dev,
> +						&qcom_swrm_dai_component,
> +						dais, num_dais);
> +}
> +
> +static int qcom_swrm_get_port_config(struct qcom_swrm_ctrl *ctrl)
> +{
> +	struct device_node *np = ctrl->dev->of_node;
> +	u8 off1[SDW_MAX_PORTS];
> +	u8 off2[SDW_MAX_PORTS];
> +	u8 si[SDW_MAX_PORTS];

Array of struct qcom_swrm_port_config instead of this trio?

> +	int i, ret, nports, val;
> +
> +	val = ctrl->reg_read(ctrl, SWRM_COMP_PARAMS);
> +	ctrl->num_dout_ports = val & SWRM_COMP_PARAMS_DOUT_PORTS_MASK;
> +	ctrl->num_din_ports = (val & SWRM_COMP_PARAMS_DIN_PORTS_MASK) >> 5;
> +
> +	ret = of_property_read_u32(np, "qcom,din-ports", &val);
> +	if (ret)
> +		return ret;
> +
> +	if (val > ctrl->num_din_ports)
> +		return -EINVAL;
> +
> +	ctrl->num_din_ports = val;
> +
> +	ret = of_property_read_u32(np, "qcom,dout-ports", &val);
> +	if (ret)
> +		return ret;
> +
> +	if (val > ctrl->num_dout_ports)
> +		return -EINVAL;
> +
> +	ctrl->num_dout_ports = val;
> +
> +	nports = ctrl->num_dout_ports + ctrl->num_din_ports;
> +
> +	ret = of_property_read_u8_array(np, "qcom,ports-offset1",
> +					off1, nports);
> +	if (ret)
> +		return ret;
> +
> +	ret = of_property_read_u8_array(np, "qcom,ports-offset2",
> +					off2, nports);
> +	if (ret)
> +		return ret;
> +
> +	ret = of_property_read_u8_array(np, "qcom,ports-sinterval-low",
> +					si, nports);
> +	if (ret)
> +		return ret;
> +
> +	for (i = 0; i < nports; i++) {
> +		ctrl->pconfig[i].si = si[i];
> +		ctrl->pconfig[i].off1 = off1[i];
> +		ctrl->pconfig[i].off2 = off2[i];
> +	}

Using array I spoke of earlier leads to brackets being redundant here.

> +
> +	return 0;
> +}
> +
> +static int qcom_swrm_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct sdw_master_prop *prop;
> +	struct sdw_bus_params *params;
> +	struct qcom_swrm_ctrl *ctrl;
> +	struct resource *res;
> +	int ret;
> +
> +	ctrl = devm_kzalloc(dev, sizeof(*ctrl), GFP_KERNEL);
> +	if (!ctrl)
> +		return -ENOMEM;
> +
> +	if (dev->parent->bus == &slimbus_bus) {
> +		ctrl->reg_read = qcom_swrm_slim_reg_read;
> +		ctrl->reg_write = qcom_swrm_slim_reg_write;
> +		ctrl->regmap = dev_get_regmap(dev->parent, NULL);
> +		if (!ctrl->regmap)
> +			return -EINVAL;
> +	} else {
> +		ctrl->reg_read = qcom_swrm_mmio_reg_read;
> +		ctrl->reg_write = qcom_swrm_mmio_reg_write;
> +		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +		ctrl->base = devm_ioremap_resource(dev, res);
> +		if (IS_ERR(ctrl->base))
> +			return PTR_ERR(ctrl->base);
> +	}
> +
> +	ctrl->irq = platform_get_irq(pdev, 0);
> +	if (ctrl->irq < 0)
> +		return ctrl->irq;
> +
> +	ctrl->hclk = devm_clk_get(dev, "iface");
> +	if (IS_ERR(ctrl->hclk))
> +		return PTR_ERR(ctrl->hclk);
> +
> +	clk_prepare_enable(ctrl->hclk);
> +
> +	ctrl->dev = dev;
> +	dev_set_drvdata(&pdev->dev, ctrl);
> +	init_completion(&ctrl->sp_cmd_comp);
> +	mutex_init(&ctrl->lock);
> +	mutex_init(&ctrl->port_lock);
> +	INIT_WORK(&ctrl->slave_work, qcom_swrm_slave_wq);
> +
> +	ctrl->bus.dev = dev;
> +	ctrl->bus.ops = &qcom_swrm_ops;
> +	ctrl->bus.port_ops = &qcom_swrm_port_ops;
> +	ctrl->bus.compute_params = &qcom_swrm_compute_params;
> +
> +	ret = qcom_swrm_get_port_config(ctrl);
> +	if (ret)
> +		return ret;
> +
> +	params = &ctrl->bus.params;
> +	params->max_dr_freq = DEFAULT_CLK_FREQ;
> +	params->curr_dr_freq = DEFAULT_CLK_FREQ;
> +	params->col = SWRM_DEFAULT_COL;
> +	params->row = SWRM_DEFAULT_ROWS;
> +	params->curr_bank = ctrl->reg_read(ctrl, SWRM_MCP_STATUS) &
> +				SWRM_MCP_STATUS_BANK_NUM_MASK;
> +	params->next_bank = !params->curr_bank;
> +
> +	prop = &ctrl->bus.prop;
> +	prop->max_clk_freq = DEFAULT_CLK_FREQ;
> +	prop->num_clk_gears = 0;
> +	prop->num_clk_freq = MAX_FREQ_NUM;
> +	prop->clk_freq = &qcom_swrm_freq_tbl[0];
> +	prop->default_col = SWRM_DEFAULT_COL;
> +	prop->default_row = SWRM_DEFAULT_ROWS;
> +
> +	ctrl->version = ctrl->reg_read(ctrl, SWRM_COMP_HW_VERSION);
> +
> +	ret = devm_request_threaded_irq(dev, ctrl->irq, NULL,
> +					qcom_swrm_irq_handler,
> +					IRQF_TRIGGER_RISING,
> +					"soundwire", ctrl);
> +	if (ret) {
> +		dev_err(dev, "Failed to request soundwire irq\n");
> +		goto err;
> +	}
> +
> +	ret = sdw_add_bus_master(&ctrl->bus);
> +	if (ret) {
> +		dev_err(dev, "Failed to register Soundwire controller (%d)\n",
> +			ret);
> +		goto err;
> +	}
> +
> +	qcom_swrm_init(ctrl);
> +	ret = qcom_swrm_register_dais(ctrl);
> +	if (ret)
> +		goto err;
> +
> +	dev_info(dev, "Qualcomm Soundwire controller v%x.%x.%x Registered\n",
> +		 (ctrl->version >> 24) & 0xff, (ctrl->version >> 16) & 0xff,
> +		 ctrl->version & 0xffff);
> +
> +	return 0;
> +err:
> +	clk_disable_unprepare(ctrl->hclk);
> +	return ret;
> +}
> +
> +static int qcom_swrm_remove(struct platform_device *pdev)
> +{
> +	struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(&pdev->dev);
> +
> +	sdw_delete_bus_master(&ctrl->bus);
> +	clk_disable_unprepare(ctrl->hclk);
> +
> +	return 0;
> +}
> +
> +static int qcom_swrm_runtime_suspend(struct device *device)
> +{
> +	/* TBD */
> +	return 0;
> +}
> +
> +static int qcom_swrm_runtime_resume(struct device *device)
> +{
> +	/* TBD */
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops qcom_swrm_dev_pm_ops = {
> +	SET_RUNTIME_PM_OPS(qcom_swrm_runtime_suspend,
> +			   qcom_swrm_runtime_resume,
> +			   NULL
> +	)
> +};
> +
> +static const struct of_device_id qcom_swrm_of_match[] = {
> +	{ .compatible = "qcom,soundwire-v1.3.0",},
> +	{ .compatible = "qcom,soundwire-v1.5.0",},
> +	{ .compatible = "qcom,soundwire-v1.6.0",},
> +	{/* sentinel */},
> +};
> +
> +MODULE_DEVICE_TABLE(of, qcom_swrm_of_match);
> +
> +static struct platform_driver qcom_swrm_driver = {
> +	.probe	= &qcom_swrm_probe,
> +	.remove = &qcom_swrm_remove,
> +	.driver = {
> +		.name	= "qcom-soundwire",
> +		.of_match_table = qcom_swrm_of_match,
> +		.pm = &qcom_swrm_dev_pm_ops,
> +	}
> +};
> +module_platform_driver(qcom_swrm_driver);
> +
> +MODULE_DESCRIPTION("Qualcomm soundwire driver");
> +MODULE_LICENSE("GPL v2");
> 

Re: [RFC PATCH 6/6] soundwire: qcom: add support for SoundWire controller

[Srinivas Kandagatla]

Thanks for taking time to review,
I agre with most of the comments specially handling returns and making 
code more readable.
Will fix them in next version.

On 08/06/2019 22:53, Cezary Rojewski wrote:
> On 2019-06-07 10:56, Srinivas Kandagatla wrote:
>> Qualcomm SoundWire Master controller is present in most Qualcomm SoCs
>> either integrated as part of WCD audio codecs via slimbus or
>> as part of SOC I/O.
>>
>> This patchset adds support to a very basic controller which has been
>> tested with WCD934x SoundWire controller connected to WSA881x smart
>> speaker amplifiers.
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
>>   drivers/soundwire/Kconfig  |   9 +
>>   drivers/soundwire/Makefile |   4 +
>>   drivers/soundwire/qcom.c   | 983 +++++++++++++++++++++++++++++++++++++
>>   3 files changed, 996 insertions(+)
>>   create mode 100644 drivers/soundwire/qcom.c
>>
>> diff --git a/drivers/soundwire/Kconfig b/drivers/soundwire/Kconfig
>> index 53b55b79c4af..f44d4f36dbbb 100644
>> --- a/drivers/soundwire/Kconfig
>> +++ b/drivers/soundwire/Kconfig
>> @@ -34,4 +34,13 @@ config SOUNDWIRE_INTEL
>>         enable this config option to get the SoundWire support for that
>>         device.
>> +config SOUNDWIRE_QCOM
>> +    tristate "Qualcomm SoundWire Master driver"
>> +    select SOUNDWIRE_BUS
>> +    depends on SND_SOC
>> +    help
>> +      SoundWire Qualcomm Master driver.
>> +      If you have an Qualcomm platform which has a SoundWire Master then
>> +      enable this config option to get the SoundWire support for that
>> +      device
>>   endif
>> diff --git a/drivers/soundwire/Makefile b/drivers/soundwire/Makefile
>> index 5817beaca0e1..f4ebfde31372 100644
>> --- a/drivers/soundwire/Makefile
>> +++ b/drivers/soundwire/Makefile
>> @@ -16,3 +16,7 @@ obj-$(CONFIG_SOUNDWIRE_INTEL) += soundwire-intel.o
>>   soundwire-intel-init-objs := intel_init.o
>>   obj-$(CONFIG_SOUNDWIRE_INTEL) += soundwire-intel-init.o
>> +
>> +#Qualcomm driver
>> +soundwire-qcom-objs :=    qcom.o
>> +obj-$(CONFIG_SOUNDWIRE_QCOM) += soundwire-qcom.o
>> diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
>> new file mode 100644
>> index 000000000000..d1722d44d217
>> --- /dev/null
>> +++ b/drivers/soundwire/qcom.c
>> @@ -0,0 +1,983 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// Copyright (c) 2019, Linaro Limited
>> +
>> +#include <linux/clk.h>
>> +#include <linux/completion.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/regmap.h>
>> +#include <linux/slab.h>
>> +#include <linux/slimbus.h>
>> +#include <linux/soundwire/sdw.h>
>> +#include <linux/soundwire/sdw_registers.h>
>> +#include <sound/pcm_params.h>
>> +#include <sound/soc.h>
>> +#include "bus.h"
>> +
> 
> Pierre already pointed this out - SWR looks odd. During my time with 
> Soundwire I've met SDW and SNDW, but it's the first time I see SWR.

These names are derived from register names from hw datasheet.
So I have not much choice here other than using them as it. May be 
adding QCOM_ prefix make it bit more non-confusing.
> 
> You seem to shortcut every reg here similarly to how it's done in SDW 
> spec. INTERRUPT is represented by INT there, and by doing so, this 
> define block would look more like a real family.
> 
Will do that in next version.!

>> +#define SWRM_CMD_FIFO_WR_CMD                    0x300
>> +#define SWRM_CMD_FIFO_RD_CMD                    0x304
>> +#define SWRM_CMD_FIFO_CMD                    0x308
>> +#define SWRM_CMD_FIFO_STATUS                    0x30C
>> +#define SWRM_CMD_FIFO_CFG_ADDR                    0x314
>> +#define SWRM_CMD_FIFO_CFG_NUM_OF_CMD_RETRY_SHFT            0x0
>> +#define SWRM_CMD_FIFO_RD_FIFO_ADDR                0x318
>> +#define SWRM_ENUMERATOR_CFG_ADDR                0x500
>> +#define SWRM_MCP_FRAME_CTRL_BANK_ADDR(m)        (0x101C + 0x40 * (m))
>> +#define SWRM_MCP_FRAME_CTRL_BANK_SSP_PERIOD_SHFT        16
>> +#define SWRM_MCP_FRAME_CTRL_BANK_ROW_CTRL_SHFT            3
>> +#define SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_BMSK            GENMASK(2, 0)
>> +#define SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_SHFT            0
>> +#define SWRM_MCP_CFG_ADDR                    0x1048
>> +#define SWRM_MCP_CFG_MAX_NUM_OF_CMD_NO_PINGS_BMSK        GENMASK(21, 17)
>> +#define SWRM_MCP_CFG_MAX_NUM_OF_CMD_NO_PINGS_SHFT        0x11
>> +#define SWRM_MCP_STATUS                        0x104C
>> +#define SWRM_MCP_STATUS_BANK_NUM_MASK                BIT(0)
>> +#define SWRM_MCP_SLV_STATUS                    0x1090
>> +#define SWRM_MCP_SLV_STATUS_MASK                GENMASK(1, 0)
>> +#define SWRM_DP_PORT_CTRL_BANK(n, m)    (0x1124 + 0x100 * (n - 1) + 
>> 0x40 * m)
> 
> Some of these you align, others leave with the equal amount of tabs 
> despite different widths.
>
I will take a closer look at such instance before sending next version.

>> +#define SWRM_DP_PORT_CTRL2_BANK(n, m)    (0x1126 + 0x100 * (n - 1) + 
>> 0x40 * m)
> 
> Consider reusing _CTRL_ and simply adding offset for 2_.
Okay.

> 
>> +#define SWRM_DP_PORT_CTRL_EN_CHAN_SHFT                0x18
>> +#define SWRM_DP_PORT_CTRL_OFFSET2_SHFT                0x10
>> +#define SWRM_DP_PORT_CTRL_OFFSET1_SHFT                0x08
>> +#define SWRM_AHB_BRIDGE_WR_DATA_0                0xc885
>> +#define SWRM_AHB_BRIDGE_WR_ADDR_0                0xc889
>> +#define SWRM_AHB_BRIDGE_RD_ADDR_0                0xc88d
>> +#define SWRM_AHB_BRIDGE_RD_DATA_0                0xc891
>> +
>> +#define SWRM_REG_VAL_PACK(data, dev, id, reg)    \
>> +            ((reg) | ((id) << 16) | ((dev) << 20) | ((data) << 24))
>> +
>> +#define SWRM_MAX_ROW_VAL    0 /* Rows = 48 */
>> +#define SWRM_DEFAULT_ROWS    48
>> +#define SWRM_MIN_COL_VAL    0 /* Cols = 2 */
>> +#define SWRM_DEFAULT_COL    16
>> +#define SWRM_SPECIAL_CMD_ID    0xF
>> +#define MAX_FREQ_NUM        1
>> +#define TIMEOUT_MS        1000
>> +#define QCOM_SWRM_MAX_RD_LEN    0xf
>> +#define DEFAULT_CLK_FREQ    9600000
>> +#define SWRM_MAX_DAIS        0xF
> 
> Given the scale of this block, it might be good to reiterate all defines 
> and see if indeed all are QCom specific. Maybe some could be replaced by 
> equivalents from common code.
> 
I did take a look at common defines, however these values are pretty 
much specific to this controller configuration.>> +
>> +struct qcom_swrm_port_config {
>> +    u8 si;
>> +    u8 off1;
>> +    u8 off2;
>> +};
>> +
>> +struct qcom_swrm_ctrl {
>> +    struct sdw_bus bus;
>> +    struct device *dev;
>> +    struct regmap *regmap;
>> +    struct completion sp_cmd_comp;
>> +    struct work_struct slave_work;
>> +    /* read/write lock */
>> +    struct mutex lock;
>> +    /* Port alloc/free lock */
>> +    struct mutex port_lock;
>> +    struct clk *hclk;
>> +    int fifo_status;
>> +    void __iomem *base;
>> +    u8 wr_cmd_id;
>> +    u8 rd_cmd_id;
>> +    int irq;
>> +    unsigned int version;
> 
> Given the fact you don't use version field directly and always shift it, 
> I'd consider making use of union here to listing version bits 
> explicitly. Overall size won't change.
Okay, I tried to keep most of the driver simple as I could as this is 
the first version, I will explore adding union as you suggested.

> 
>> +    int num_din_ports;
>> +    int num_dout_ports;
>> +    unsigned long dout_port_mask;
>> +    unsigned long din_port_mask;
>> +    struct qcom_swrm_port_config pconfig[SDW_MAX_PORTS];
>> +    struct sdw_stream_runtime *sruntime[SWRM_MAX_DAIS];
>> +    enum sdw_slave_status status[SDW_MAX_DEVICES];
>> +    u32 (*reg_read)(struct qcom_swrm_ctrl *ctrl, int reg);
>> +    int (*reg_write)(struct qcom_swrm_ctrl *ctrl, int reg, int val);
>> +};
>> +
>> +#define to_qcom_sdw(b)    container_of(b, struct qcom_swrm_ctrl, bus)
>> +
>> +struct usecase {
>> +    u8 num_port;
>> +    u8 num_ch;
>> +    u32 chrate;
>> +};
>> +
> 
> "usecase" looks ambiguous at best.

Its leftover

> 
>> +static u32 qcom_swrm_slim_reg_read(struct qcom_swrm_ctrl *ctrl, int reg)
>> +{
>> +    struct regmap *wcd_regmap = ctrl->regmap;
>> +    u32 val = 0, ret;
>> +
>> +    /* pg register + offset */
>> +    regmap_bulk_write(wcd_regmap, SWRM_AHB_BRIDGE_RD_ADDR_0,
>> +              (u8 *)&reg, 4);
>> +
>> +    ret = regmap_bulk_read(wcd_regmap, SWRM_AHB_BRIDGE_RD_DATA_0,
>> +                   (u8 *)&val, 4);
>> +    if (ret < 0)
>> +        dev_err(ctrl->dev, "Read Failure (%d)\n", ret);
>> +
>> +    return val;
>> +}
>> +
>> +static u32 qcom_swrm_mmio_reg_read(struct qcom_swrm_ctrl *ctrl, int reg)
>> +{
>> +    return readl_relaxed(ctrl->base + reg);
>> +}
>> +
>> +static int qcom_swrm_mmio_reg_write(struct qcom_swrm_ctrl *ctrl,
>> +                    int reg, int val)
>> +{
>> +    writel_relaxed(val, ctrl->base + reg);
>> +
>> +    return 0;
>> +}
>> +
>> +static int qcom_swrm_slim_reg_write(struct qcom_swrm_ctrl *ctrl,
>> +                    int reg, int val)
>> +{
>> +    struct regmap *wcd_regmap = ctrl->regmap;
>> +
>> +    /* pg register + offset */
>> +    regmap_bulk_write(wcd_regmap, SWRM_AHB_BRIDGE_WR_DATA_0,
>> +              (u8 *)&val, 4);
>> +    /* write address register */
>> +    regmap_bulk_write(wcd_regmap, SWRM_AHB_BRIDGE_WR_ADDR_0,
>> +              (u8 *)&reg, 4);
>> +
>> +    return 0;
>> +}
> 
> Ok, so you choose to declare write op as returning "int" yet either it 
> cannot do so (void writel_relaxed) or ret is completely ignored 
> (regmap_bulk_write does return an int value). Either switch to void or 
> check against returned value whenever possible.
> 

Its right to check the return values, I will change it this accordingly 
and fix all such occurances.

>> +
>> +static int qcom_swrm_cmd_fifo_wr_cmd(struct qcom_swrm_ctrl *ctrl, u8 
>> cmd_data,
>> +                     u8 dev_addr, u16 reg_addr)
>> +{
>> +    int ret = 0;
>> +    u8 cmd_id;
>> +    u32 val;
>> +
>> +    mutex_lock(&ctrl->lock);
>> +    if (dev_addr == SDW_BROADCAST_DEV_NUM) {
>> +        cmd_id = SWRM_SPECIAL_CMD_ID;
>> +    } else {
>> +        if (++ctrl->wr_cmd_id == SWRM_SPECIAL_CMD_ID)
>> +            ctrl->wr_cmd_id = 0;
>> +
>> +        cmd_id = ctrl->wr_cmd_id;
>> +    }
>> +
>> +    val = SWRM_REG_VAL_PACK(cmd_data, dev_addr, cmd_id, reg_addr);
>> +    ret = ctrl->reg_write(ctrl, SWRM_CMD_FIFO_WR_CMD, val);
>> +    if (ret < 0) {
>> +        dev_err(ctrl->dev, "%s: reg 0x%x write failed, err:%d\n",
>> +            __func__, val, ret);
>> +        goto err;
>> +    }
>> +
>> +    if (dev_addr == SDW_BROADCAST_DEV_NUM) {
>> +        ctrl->fifo_status = 0;
>> +        ret = wait_for_completion_timeout(&ctrl->sp_cmd_comp,
>> +                          msecs_to_jiffies(TIMEOUT_MS));
>> +
>> +        if (!ret || ctrl->fifo_status) {
>> +            dev_err(ctrl->dev, "reg 0x%x write failed\n", val);
> 
> Both, this and err msg above are generic enough to be put into goto to 
> save some space.

I agree!

> 
>> +            ret = -ENODATA;
>> +            goto err;
>> +        }
>> +    }
>> +err:
>> +    mutex_unlock(&ctrl->lock);
>> +    return ret;
>> +}
>> +
>> +static int qcom_swrm_cmd_fifo_rd_cmd(struct qcom_swrm_ctrl *ctrl,
>> +                     u8 dev_addr, u16 reg_addr,
>> +                     u32 len, u8 *rval)
>> +{
>> +    int i, ret = 0;
>> +    u8 cmd_id = 0;
>> +    u32 val;
>> +
>> +    mutex_lock(&ctrl->lock);
>> +    if (dev_addr == SDW_ENUM_DEV_NUM) {
>> +        cmd_id = SWRM_SPECIAL_CMD_ID;
>> +    } else {
>> +        if (++ctrl->rd_cmd_id == SWRM_SPECIAL_CMD_ID)
>> +            ctrl->rd_cmd_id = 0;
>> +
>> +        cmd_id = ctrl->rd_cmd_id;
>> +    }
>> +
>> +    val = SWRM_REG_VAL_PACK(len, dev_addr, cmd_id, reg_addr);
>> +    ret = ctrl->reg_write(ctrl, SWRM_CMD_FIFO_RD_CMD, val);
>> +    if (ret < 0) {
>> +        dev_err(ctrl->dev, "reg 0x%x write failed err:%d\n", val, ret);
> 
> Same for _rt_.
> 
>> +        goto err;
>> +    }
>> +
>> +    if (dev_addr == SDW_ENUM_DEV_NUM) {
>> +        ctrl->fifo_status = 0;
>> +        ret = wait_for_completion_timeout(&ctrl->sp_cmd_comp,
>> +                          msecs_to_jiffies(TIMEOUT_MS));
>> +
>> +        if (!ret || ctrl->fifo_status) {
>> +            dev_err(ctrl->dev, "reg 0x%x read failed\n", val);
> 
> Just to be sure. It is really "read" that is failing here?
>
The final result is that read has not been sucessfull with a complete 
interrupt. So yes read is failing here.


>> +            ret = -ENODATA;
>> +            goto err;
>> +        }
>> +    }
>> +
>> +    for (i = 0; i < len; i++) {
>> +        rval[i] = ctrl->reg_read(ctrl, SWRM_CMD_FIFO_RD_FIFO_ADDR);
>> +        rval[i] &= 0xFF;
>> +    }
>> +
>> +err:
>> +    mutex_unlock(&ctrl->lock);
>> +    return ret;
>> +}
>> +
>> +static void qcom_swrm_get_device_status(struct qcom_swrm_ctrl *ctrl)
>> +{
>> +    u32 val = ctrl->reg_read(ctrl, SWRM_MCP_SLV_STATUS);
>> +    int i;
>> +
>> +    for (i = 1; i < SDW_MAX_DEVICES; i++) {
>> +        u32 s;
>> +
>> +        s = (val >> (i * 2));
>> +        s &= SWRM_MCP_SLV_STATUS_MASK;
>> +        ctrl->status[i] = s;
>> +    }
>> +}
>> +
>> +static irqreturn_t qcom_swrm_irq_handler(int irq, void *dev_id)
>> +{
>> +    struct qcom_swrm_ctrl *ctrl = dev_id;
>> +    u32 sts, value;
>> +
>> +    sts = ctrl->reg_read(ctrl, SWRM_INTERRUPT_STATUS);
>> +
>> +    if (sts & SWRM_INTERRUPT_STATUS_SPECIAL_CMD_ID_FINISHED)
>> +        complete(&ctrl->sp_cmd_comp);
>> +
>> +    if (sts & SWRM_INTERRUPT_STATUS_CMD_ERROR) {
>> +        value = ctrl->reg_read(ctrl, SWRM_CMD_FIFO_STATUS);
>> +        dev_err_ratelimited(ctrl->dev,
>> +                    "CMD error, fifo status 0x%x\n",
>> +                     value);
>> +        ctrl->reg_write(ctrl, SWRM_CMD_FIFO_CMD, 0x1);
>> +        if ((value & 0xF) == 0xF) {
>> +            ctrl->fifo_status = -ENODATA;
>> +            complete(&ctrl->sp_cmd_comp);
>> +        }
>> +    }
>> +
>> +    if ((sts & SWRM_INTERRUPT_STATUS_NEW_SLAVE_ATTACHED) ||
>> +        sts & SWRM_INTERRUPT_STATUS_CHANGE_ENUM_SLAVE_STATUS) {
>> +        if (sts & SWRM_INTERRUPT_STATUS_NEW_SLAVE_ATTACHED)
>> +            ctrl->status[0] = SDW_SLAVE_ATTACHED;
>> +
>> +        schedule_work(&ctrl->slave_work);
>> +    }
>> +
>> +    if (sts & SWRM_INTERRUPT_STATUS_SLAVE_PEND_IRQ)
>> +        dev_dbg(ctrl->dev, "Slave pend irq\n");
>> +
>> +    if (sts & SWRM_INTERRUPT_STATUS_NEW_SLAVE_ATTACHED)
>> +        dev_dbg(ctrl->dev, "New slave attached\n");
>> +
>> +    if (sts & SWRM_INTERRUPT_STATUS_MASTER_CLASH_DET)
>> +        dev_err_ratelimited(ctrl->dev, "Bus clash detected\n");
>> +
>> +    if (sts & SWRM_INTERRUPT_STATUS_RD_FIFO_OVERFLOW)
>> +        dev_err(ctrl->dev, "Read FIFO overflow\n");
>> +
>> +    if (sts & SWRM_INTERRUPT_STATUS_RD_FIFO_UNDERFLOW)
>> +        dev_err(ctrl->dev, "Read FIFO underflow\n");
>> +
>> +    if (sts & SWRM_INTERRUPT_STATUS_WR_CMD_FIFO_OVERFLOW)
>> +        dev_err(ctrl->dev, "Write FIFO overflow\n");
>> +
>> +    if (sts & SWRM_INTERRUPT_STATUS_DOUT_PORT_COLLISION)
>> +        dev_err(ctrl->dev, "Port collision detected\n");
>> +
>> +    if (sts & SWRM_INTERRUPT_STATUS_READ_EN_RD_VALID_MISMATCH)
>> +        dev_err(ctrl->dev, "Read enable valid mismatch\n");
>> +
>> +    if (sts & SWRM_INTERRUPT_STATUS_SPECIAL_CMD_ID_FINISHED)
>> +        dev_err(ctrl->dev, "Cmd id finished\n");
>> +
>> +    if (sts & SWRM_INTERRUPT_STATUS_BUS_RESET_FINISHED)
>> +        dev_err(ctrl->dev, "Bus reset finished\n");
> 
> If you do not handle these errors at all, consider declaring 
> ERROR-message table. I believe leaving erroneus status as is may lead to 
> fatal consequences. If there is no intention for handling even the most 
> critical cases, please add TODO/ comment here.

Yep, I agree will clean this up!

> 
>> +
>> +    ctrl->reg_write(ctrl, SWRM_INTERRUPT_CLEAR, sts);
>> +
>> +    return IRQ_HANDLED;
>> +}
>> +
>> +static int qcom_swrm_init(struct qcom_swrm_ctrl *ctrl)
>> +{
>> +    u32 val;
>> +    u8 row_ctrl = SWRM_MAX_ROW_VAL;
>> +    u8 col_ctrl = SWRM_MIN_COL_VAL;
>> +    u8 ssp_period = 1;
>> +    u8 retry_cmd_num = 3;
>> +
>> +    /* Clear Rows and Cols */
>> +    val = ((row_ctrl << SWRM_MCP_FRAME_CTRL_BANK_ROW_CTRL_SHFT) |
>> +        (col_ctrl << SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_SHFT) |
>> +        (ssp_period << SWRM_MCP_FRAME_CTRL_BANK_SSP_PERIOD_SHFT));
>> +
>> +    ctrl->reg_write(ctrl, SWRM_MCP_FRAME_CTRL_BANK_ADDR(0), val);
>> +
>> +    /* Disable Auto enumeration */
>> +    ctrl->reg_write(ctrl, SWRM_ENUMERATOR_CFG_ADDR, 0);
>> +
>> +    /* Mask soundwire interrupts */
>> +    ctrl->reg_write(ctrl, SWRM_INTERRUPT_MASK_ADDR,
>> +                    SWRM_INTERRUPT_STATUS_RMSK);
>> +
>> +    /* Configure No pings */
>> +    val = ctrl->reg_read(ctrl, SWRM_MCP_CFG_ADDR);
>> +
>> +    val &= ~SWRM_MCP_CFG_MAX_NUM_OF_CMD_NO_PINGS_BMSK;
>> +    val |= (0x1f << SWRM_MCP_CFG_MAX_NUM_OF_CMD_NO_PINGS_SHFT);
>> +    ctrl->reg_write(ctrl, SWRM_MCP_CFG_ADDR, val);
>> +
>> +    /* Configure number of retries of a read/write cmd */
>> +    val = (retry_cmd_num << SWRM_CMD_FIFO_CFG_NUM_OF_CMD_RETRY_SHFT);
>> +    ctrl->reg_write(ctrl, SWRM_CMD_FIFO_CFG_ADDR, val);
>> +
>> +    /* Set IRQ to PULSE */
>> +    ctrl->reg_write(ctrl, SWRM_COMP_CFG_ADDR,
>> +                SWRM_COMP_CFG_IRQ_LEVEL_OR_PULSE_MSK |
>> +                SWRM_COMP_CFG_ENABLE_MSK);
>> +    return 0;
> 
> As in my previous comment, you should check against ret from reg_write 
> if void approach is not chosen.

I agree!

> 
>> +}
>> +
>> +static enum sdw_command_response qcom_swrm_xfer_msg(struct sdw_bus *bus,
>> +                            struct sdw_msg *msg)
>> +{
>> +    struct qcom_swrm_ctrl *ctrl = to_qcom_sdw(bus);
>> +    int ret, i, len;
>> +
>> +    if (msg->flags == SDW_MSG_FLAG_READ) {
>> +        for (i = 0; i < msg->len;) {
>> +            if ((msg->len - i) < QCOM_SWRM_MAX_RD_LEN)
>> +                len = msg->len - i;
>> +            else
>> +                len = QCOM_SWRM_MAX_RD_LEN;
>> +
>> +            ret = qcom_swrm_cmd_fifo_rd_cmd(ctrl, msg->dev_num,
>> +                            msg->addr + i, len,
>> +                               &msg->buf[i]);
>> +            if (ret < 0) {
>> +                if (ret == -ENODATA)
>> +                    return SDW_CMD_IGNORED;
>> +
>> +                return ret;
>> +            }
>> +
>> +            i = i + len;
> 
> Any reason for inlining this incrementation? If _rd_ fails, we leave the 
> loop anyway.
> 
>> +        }
>> +    } else if (msg->flags == SDW_MSG_FLAG_WRITE) {
>> +        for (i = 0; i < msg->len; i++) {
>> +            ret = qcom_swrm_cmd_fifo_wr_cmd(ctrl, msg->buf[i],
>> +                            msg->dev_num,
>> +                               msg->addr + i);
>> +            if (ret < 0) {
>> +                if (ret == -ENODATA)
>> +                    return SDW_CMD_IGNORED;
>> +
>> +                return ret;
>> +            }
>> +        }
>> +    }
>> +
>> +    return SDW_CMD_OK;
>> +}
>> +
>> +static int qcom_swrm_pre_bank_switch(struct sdw_bus *bus)
>> +{
>> +    u32 reg = SWRM_MCP_FRAME_CTRL_BANK_ADDR(bus->params.next_bank);
>> +    struct qcom_swrm_ctrl *ctrl = to_qcom_sdw(bus);
>> +    u32 val;
>> +
>> +    val = ctrl->reg_read(ctrl, reg);
>> +    val |= ((0 << SWRM_MCP_FRAME_CTRL_BANK_ROW_CTRL_SHFT) |
>> +        (7l << SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_SHFT));
>> +    ctrl->reg_write(ctrl, reg, val);
>> +
>> +    return 0;
> 
> s/return 0/return ctrl->reg_write(ctrl, reg, val)/
> 
>> +}
>> +
>> +static int qcom_swrm_port_params(struct sdw_bus *bus,
>> +                 struct sdw_port_params *p_params,
>> +                unsigned int bank)
>> +{
>> +    /* TBD */
>> +    return 0;
>> +}
>> +
>> +static int qcom_swrm_transport_params(struct sdw_bus *bus,
>> +                      struct sdw_transport_params *params,
>> +                     enum sdw_reg_bank bank)
>> +{
>> +    struct qcom_swrm_ctrl *ctrl = to_qcom_sdw(bus);
>> +    u32 value;
>> +
>> +    value = params->offset1 << SWRM_DP_PORT_CTRL_OFFSET1_SHFT;
>> +    value |= params->offset2 << SWRM_DP_PORT_CTRL_OFFSET2_SHFT;
>> +    value |= params->sample_interval - 1;
>> +
>> +    ctrl->reg_write(ctrl, SWRM_DP_PORT_CTRL_BANK((params->port_num), 
>> bank),
>> +            value);
>> +
>> +    return 0;
> 
> Another "return issue" here.
> 
>> +}
>> +
>> +static int qcom_swrm_port_enable(struct sdw_bus *bus,
>> +                 struct sdw_enable_ch *enable_ch,
>> +                unsigned int bank)
>> +{
>> +    u32 reg = SWRM_DP_PORT_CTRL_BANK(enable_ch->port_num, bank);
>> +    struct qcom_swrm_ctrl *ctrl = to_qcom_sdw(bus);
>> +    u32 val;
>> +
>> +    val = ctrl->reg_read(ctrl, reg);
>> +    if (enable_ch->enable)
>> +        val |= (enable_ch->ch_mask << SWRM_DP_PORT_CTRL_EN_CHAN_SHFT);
>> +    else
>> +        val &= ~(enable_ch->ch_mask << SWRM_DP_PORT_CTRL_EN_CHAN_SHFT);
>> +
>> +    ctrl->reg_write(ctrl, reg, val);
>> +
>> +    return 0;
>> +}
>> +
>> +static struct sdw_master_port_ops qcom_swrm_port_ops = {
>> +    .dpn_set_port_params = qcom_swrm_port_params,
>> +    .dpn_set_port_transport_params = qcom_swrm_transport_params,
>> +    .dpn_port_enable_ch = qcom_swrm_port_enable,
>> +};
>> +
>> +static struct sdw_master_ops qcom_swrm_ops = {
>> +    .xfer_msg = qcom_swrm_xfer_msg,
>> +    .pre_bank_switch = qcom_swrm_pre_bank_switch,
>> +};
>> +
>> +static int qcom_swrm_compute_params(struct sdw_bus *bus)
>> +{
>> +    struct qcom_swrm_ctrl *ctrl = to_qcom_sdw(bus);
>> +    struct sdw_master_runtime *m_rt;
>> +    struct sdw_slave_runtime *s_rt;
>> +    struct sdw_port_runtime *p_rt;
>> +    int i = 0;
>> +
>> +    list_for_each_entry(m_rt, &bus->m_rt_list, bus_node) {
>> +        list_for_each_entry(p_rt, &m_rt->port_list, port_node) {
>> +            p_rt->transport_params.port_num = p_rt->num;
>> +            p_rt->transport_params.sample_interval =
>> +                    ctrl->pconfig[p_rt->num - 1].si + 1;
>> +            p_rt->transport_params.offset1 =
>> +                    ctrl->pconfig[p_rt->num - 1].off1;
>> +            p_rt->transport_params.offset2 =
>> +                    ctrl->pconfig[p_rt->num - 1].off2;
> 
> ctrl->pconfig[ <idx> ] colleagues bellow make use of local index 
> variable which clearly makes it more readable.
> 
>> +        }
>> +
>> +        list_for_each_entry(s_rt, &m_rt->slave_rt_list, m_rt_node) {
>> +            list_for_each_entry(p_rt, &s_rt->port_list, port_node) {
>> +                p_rt->transport_params.port_num = p_rt->num;
>> +                p_rt->transport_params.sample_interval =
>> +                        ctrl->pconfig[i].si + 1;
>> +                p_rt->transport_params.offset1 =
>> +                        ctrl->pconfig[i].off1;
>> +                p_rt->transport_params.offset2 =
>> +                        ctrl->pconfig[i].off2;
>> +                i++;
>> +            }
>> +        }
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static u32 qcom_swrm_freq_tbl[MAX_FREQ_NUM] = {
>> +    DEFAULT_CLK_FREQ,
>> +};
>> +
>> +static void qcom_swrm_slave_wq(struct work_struct *work)
>> +{
>> +    struct qcom_swrm_ctrl *ctrl =
>> +            container_of(work, struct qcom_swrm_ctrl, slave_work);
>> +
>> +    qcom_swrm_get_device_status(ctrl);
>> +    sdw_handle_slave_status(&ctrl->bus, ctrl->status);
>> +}
>> +
>> +static int qcom_swrm_prepare(struct snd_pcm_substream *substream,
>> +                 struct snd_soc_dai *dai)
>> +{
>> +    struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(dai->dev);
>> +
>> +    if (!ctrl->sruntime[dai->id])
>> +        return -EINVAL;
>> +
>> +    return sdw_enable_stream(ctrl->sruntime[dai->id]);
>> +}
>> +
>> +static void qcom_swrm_stream_free_ports(struct qcom_swrm_ctrl *ctrl,
>> +                    struct sdw_stream_runtime *stream)
>> +{
>> +    struct sdw_master_runtime *m_rt;
>> +    struct sdw_port_runtime *p_rt;
>> +    unsigned long *port_mask;
>> +
>> +    mutex_lock(&ctrl->port_lock);
>> +
>> +    list_for_each_entry(m_rt, &stream->master_list, stream_node) {
>> +        if (m_rt->direction == SDW_DATA_DIR_RX)
>> +            port_mask = &ctrl->dout_port_mask;
>> +        else
>> +            port_mask = &ctrl->din_port_mask;
>> +
>> +        list_for_each_entry(p_rt, &m_rt->port_list, port_node) {
>> +            clear_bit(p_rt->num - 1, port_mask);
>> +        }
> 
> Unnecessary brackets.
> 
>> +    }
>> +
>> +    mutex_unlock(&ctrl->port_lock);
>> +}
>> +
>> +static int qcom_swrm_stream_alloc_ports(struct qcom_swrm_ctrl *ctrl,
>> +                    struct sdw_stream_runtime *stream,
>> +                       struct snd_pcm_hw_params *params,
>> +                       int direction)
>> +{
>> +    struct sdw_port_config pconfig[SDW_MAX_PORTS];
>> +    struct sdw_stream_config sconfig;
>> +    struct sdw_master_runtime *m_rt;
>> +    struct sdw_slave_runtime *s_rt;
>> +    struct sdw_port_runtime *p_rt;
>> +    unsigned long *port_mask;
>> +    int i, maxport, pn, nports = 0, ret = 0;
>> +
>> +    mutex_lock(&ctrl->port_lock);
>> +    list_for_each_entry(m_rt, &stream->master_list, stream_node) {
>> +        if (m_rt->direction == SDW_DATA_DIR_RX) {
>> +            maxport = ctrl->num_dout_ports;
>> +            port_mask = &ctrl->dout_port_mask;
>> +        } else {
>> +            maxport = ctrl->num_din_ports;
>> +            port_mask = &ctrl->din_port_mask;
>> +        }
>> +
>> +        list_for_each_entry(s_rt, &m_rt->slave_rt_list, m_rt_node) {
>> +            list_for_each_entry(p_rt, &s_rt->port_list, port_node) {
>> +                /* Port numbers start from 1 - 14*/
>> +                pn = find_first_zero_bit(port_mask, maxport);
>> +                if (pn > (maxport - 1)) {
>> +                    dev_err(ctrl->dev, "All ports busy\n");
>> +                    ret = -EBUSY;
>> +                    goto err;
>> +                }
>> +                set_bit(pn, port_mask);
>> +                pconfig[nports].num = pn + 1;
>> +                pconfig[nports].ch_mask = p_rt->ch_mask;
>> +                nports++;
>> +            }
>> +        }
>> +    }
>> +
>> +    if (direction == SNDRV_PCM_STREAM_CAPTURE)
>> +        sconfig.direction = SDW_DATA_DIR_TX;
>> +    else
>> +        sconfig.direction = SDW_DATA_DIR_RX;
>> +
>> +    sconfig.ch_count = 1;
>> +    sconfig.frame_rate = params_rate(params);
>> +    sconfig.type = stream->type;
>> +    sconfig.bps = 1;
> 
> Hmm. frame_rate and type gets assigned based on "input" data yet the 
> rest is hardcoded. Is this intended?

For now I only managed to test PDM on this controller, and I agree these 
need to be more generic..
>> +    sdw_stream_add_master(&ctrl->bus, &sconfig, pconfig,
>> +                  nports, stream);
>> +err:
>> +    if (ret) {
>> +        for (i = 0; i < nports; i++)
>> +            clear_bit(pconfig[i].num - 1, port_mask);
>> +    }
>> +
>> +    mutex_unlock(&ctrl->port_lock);
>> +
>> +    return ret;
>> +}
>> +
>> +static int qcom_swrm_hw_params(struct snd_pcm_substream *substream,
>> +                   struct snd_pcm_hw_params *params,
>> +                  struct snd_soc_dai *dai)
>> +{
>> +    struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(dai->dev);
>> +    int ret;
>> +
>> +    ret = qcom_swrm_stream_alloc_ports(ctrl, ctrl->sruntime[dai->id],
>> +                       params, substream->stream);
>> +    if (ret)
>> +        return ret;
>> +
>> +    return sdw_prepare_stream(ctrl->sruntime[dai->id]);
>> +}
>> +
>> +static int qcom_swrm_hw_free(struct snd_pcm_substream *substream,
>> +                 struct snd_soc_dai *dai)
>> +{
>> +    struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(dai->dev);
>> +
>> +    qcom_swrm_stream_free_ports(ctrl, ctrl->sruntime[dai->id]);
>> +    sdw_stream_remove_master(&ctrl->bus, ctrl->sruntime[dai->id]);
>> +    sdw_deprepare_stream(ctrl->sruntime[dai->id]);
>> +    sdw_disable_stream(ctrl->sruntime[dai->id]);
> 
> Declaring local variable initialized with ctrl->sruntime[dai->id] should 
> prove more readable.
> 
I agree!

>> +
>> +    return 0;
>> +}
>> +
>> +static int qcom_swrm_register_dais(struct qcom_swrm_ctrl *ctrl)
>> +{
>> +    int num_dais = ctrl->num_dout_ports + ctrl->num_din_ports;
>> +    struct snd_soc_dai_driver *dais;
>> +    int i;
>> +
>> +    /* PDM dais are only tested for now */
>> +    dais = devm_kcalloc(ctrl->dev, num_dais, sizeof(*dais), GFP_KERNEL);
>> +    if (!dais)
>> +        return -ENOMEM;
>> +
>> +    for (i = 0; i < num_dais; i++) {
>> +        dais[i].name = kasprintf(GFP_KERNEL, "SDW Pin%d", i);
>> +        if (i < ctrl->num_dout_ports) {
>> +            dais[i].playback.stream_name = kasprintf(GFP_KERNEL,
>> +                                 "SDW Tx%d", i);
>> +            if (!dais[i].playback.stream_name) {
>> +                kfree(dais[i].name);
>> +                return -ENOMEM;
> 
> Now this got me worried. What about memory allocated in iterations 
> before the failure? It must be freed in error handling path. goto should 
> be of help here.
> 
I agree.

>> +            }
>> +            dais[i].playback.channels_min = 1;
>> +            dais[i].playback.channels_max = 1;
>> +            dais[i].playback.rates = SNDRV_PCM_RATE_48000;
>> +            dais[i].playback.formats = SNDRV_PCM_FMTBIT_S16_LE;
> 
> All of these formats are hardcoded. Consider declaring a "template" 
> format above and simply initialize each dai with it.

Okay!

> 
>> +        } else {
>> +            dais[i].capture.stream_name = kasprintf(GFP_KERNEL,
>> +                                "SDW Rx%d", i);
>> +            if (!dais[i].capture.stream_name) {
>> +                kfree(dais[i].name);
>> +                kfree(dais[i].playback.stream_name);
>> +                return -ENOMEM;
> 
> Same memory deallocation issue here.
I see that, I will take care of this in next version.
> 
>> +            }
>> +
>> +            dais[i].capture.channels_min = 1;
>> +            dais[i].capture.channels_max = 1;
>> +            dais[i].capture.rates = SNDRV_PCM_RATE_48000;
>> +            dais[i].capture.formats = SNDRV_PCM_FMTBIT_S16_LE;
> 
> Comment regarding playback dai initialization applies here too.
> 
>> +        }
>> +        dais[i].ops = &qcom_swrm_pdm_dai_ops;
>> +        dais[i].id = i;
>> +    }
>> +
>> +    return devm_snd_soc_register_component(ctrl->dev,
>> +                        &qcom_swrm_dai_component,
>> +                        dais, num_dais);
>> +}
>> +
>> +static int qcom_swrm_get_port_config(struct qcom_swrm_ctrl *ctrl)
>> +{
>> +    struct device_node *np = ctrl->dev->of_node;
>> +    u8 off1[SDW_MAX_PORTS];
>> +    u8 off2[SDW_MAX_PORTS];
>> +    u8 si[SDW_MAX_PORTS];
> 
> Array of struct qcom_swrm_port_config instead of this trio?
> 
Makes sense! Will do that in next version.
>> +    int i, ret, nports, val;
>> +
>> +    val = ctrl->reg_read(ctrl, SWRM_COMP_PARAMS);
>> +    ctrl->num_dout_ports = val & SWRM_COMP_PARAMS_DOUT_PORTS_MASK;
>> +    ctrl->num_din_ports = (val & SWRM_COMP_PARAMS_DIN_PORTS_MASK) >> 5;
>> +
>> +    ret = of_property_read_u32(np, "qcom,din-ports", &val);
>> +    if (ret)
>> +        return ret;
>> +
>> +    if (val > ctrl->num_din_ports)
>> +        return -EINVAL;
>> +
>> +    ctrl->num_din_ports = val;
>> +
>> +    ret = of_property_read_u32(np, "qcom,dout-ports", &val);
>> +    if (ret)
>> +        return ret;
>> +
>> +    if (val > ctrl->num_dout_ports)
>> +        return -EINVAL;
>> +
>> +    ctrl->num_dout_ports = val;
>> +
>> +    nports = ctrl->num_dout_ports + ctrl->num_din_ports;
>> +
>> +    ret = of_property_read_u8_array(np, "qcom,ports-offset1",
>> +                    off1, nports);
>> +    if (ret)
>> +        return ret;
>> +
>> +    ret = of_property_read_u8_array(np, "qcom,ports-offset2",
>> +                    off2, nports);
>> +    if (ret)
>> +        return ret;
>> +
>> +    ret = of_property_read_u8_array(np, "qcom,ports-sinterval-low",
>> +                    si, nports);
>> +    if (ret)
>> +        return ret;
>> +
>> +    for (i = 0; i < nports; i++) {
>> +        ctrl->pconfig[i].si = si[i];
>> +        ctrl->pconfig[i].off1 = off1[i];
>> +        ctrl->pconfig[i].off2 = off2[i];
>> +    }
> 
> Using array I spoke of earlier leads to brackets being redundant here.
> 
>> +
>> +    return 0;
>> +}
>> +

Re: [RFC PATCH 6/6] soundwire: qcom: add support for SoundWire controller

[Vinod Koul]

On 07-06-19, 09:56, Srinivas Kandagatla wrote:
> Qualcomm SoundWire Master controller is present in most Qualcomm SoCs
> either integrated as part of WCD audio codecs via slimbus or
> as part of SOC I/O.
> 
> This patchset adds support to a very basic controller which has been
> tested with WCD934x SoundWire controller connected to WSA881x smart
> speaker amplifiers.
> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
>  drivers/soundwire/Kconfig  |   9 +
>  drivers/soundwire/Makefile |   4 +
>  drivers/soundwire/qcom.c   | 983 +++++++++++++++++++++++++++++++++++++

Can you split this to two patches (at least), master driver followed by
DAI driver

> +
> +#define SWRM_COMP_HW_VERSION					0x00

What does COMP stand for?

> +#define SWRM_COMP_CFG_ADDR					0x04
> +#define SWRM_COMP_CFG_IRQ_LEVEL_OR_PULSE_MSK			BIT(1)
> +#define SWRM_COMP_CFG_ENABLE_MSK				BIT(0)
> +#define SWRM_COMP_PARAMS					0x100
> +#define SWRM_COMP_PARAMS_DOUT_PORTS_MASK			GENMASK(4, 0)
> +#define SWRM_COMP_PARAMS_DIN_PORTS_MASK				GENMASK(9, 5)
> +#define SWRM_COMP_PARAMS_WR_FIFO_DEPTH				GENMASK(14, 10)
> +#define SWRM_COMP_PARAMS_RD_FIFO_DEPTH				GENMASK(19, 15)
> +#define SWRM_COMP_PARAMS_AUTO_ENUM_SLAVES			GENMASK(32. 20)

Thats a lot of text, So as others have said we need to rethink the
naming conventions, perhaps QC_SDW_PARAM_AUTO_ENUM (feel free to drop
SDW as well from here as it QC specific!

Also move common ones to core..

> +#define SWRM_INTERRUPT_STATUS					0x200
> +#define SWRM_INTERRUPT_STATUS_RMSK				GENMASK(16, 0)
> +#define SWRM_INTERRUPT_STATUS_SLAVE_PEND_IRQ			BIT(0)
> +#define SWRM_INTERRUPT_STATUS_NEW_SLAVE_ATTACHED		BIT(1)
> +#define SWRM_INTERRUPT_STATUS_CHANGE_ENUM_SLAVE_STATUS		BIT(2)
> +#define SWRM_INTERRUPT_STATUS_MASTER_CLASH_DET			BIT(3)
> +#define SWRM_INTERRUPT_STATUS_RD_FIFO_OVERFLOW			BIT(4)
> +#define SWRM_INTERRUPT_STATUS_RD_FIFO_UNDERFLOW			BIT(5)
> +#define SWRM_INTERRUPT_STATUS_WR_CMD_FIFO_OVERFLOW		BIT(6)
> +#define SWRM_INTERRUPT_STATUS_CMD_ERROR				BIT(7)
> +#define SWRM_INTERRUPT_STATUS_DOUT_PORT_COLLISION		BIT(8)
> +#define SWRM_INTERRUPT_STATUS_READ_EN_RD_VALID_MISMATCH		BIT(9)
> +#define SWRM_INTERRUPT_STATUS_SPECIAL_CMD_ID_FINISHED		BIT(10)
> +#define SWRM_INTERRUPT_STATUS_NEW_SLAVE_AUTO_ENUM_FINISHED	BIT(11)
> +#define SWRM_INTERRUPT_STATUS_AUTO_ENUM_FAILED			BIT(12)
> +#define SWRM_INTERRUPT_STATUS_AUTO_ENUM_TABLE_IS_FULL		BIT(13)
> +#define SWRM_INTERRUPT_STATUS_BUS_RESET_FINISHED		BIT(14)
> +#define SWRM_INTERRUPT_STATUS_CLK_STOP_FINISHED			BIT(15)
> +#define SWRM_INTERRUPT_STATUS_ERROR_PORT_TEST			BIT(16)
> +#define SWRM_INTERRUPT_MASK_ADDR				0x204
> +#define SWRM_INTERRUPT_CLEAR					0x208
> +#define SWRM_CMD_FIFO_WR_CMD					0x300
> +#define SWRM_CMD_FIFO_RD_CMD					0x304
> +#define SWRM_CMD_FIFO_CMD					0x308
> +#define SWRM_CMD_FIFO_STATUS					0x30C
> +#define SWRM_CMD_FIFO_CFG_ADDR					0x314
> +#define SWRM_CMD_FIFO_CFG_NUM_OF_CMD_RETRY_SHFT			0x0
> +#define SWRM_CMD_FIFO_RD_FIFO_ADDR				0x318
> +#define SWRM_ENUMERATOR_CFG_ADDR				0x500
> +#define SWRM_MCP_FRAME_CTRL_BANK_ADDR(m)		(0x101C + 0x40 * (m))
> +#define SWRM_MCP_FRAME_CTRL_BANK_SSP_PERIOD_SHFT		16
> +#define SWRM_MCP_FRAME_CTRL_BANK_ROW_CTRL_SHFT			3
> +#define SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_BMSK			GENMASK(2, 0)
> +#define SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_SHFT			0
> +#define SWRM_MCP_CFG_ADDR					0x1048
> +#define SWRM_MCP_CFG_MAX_NUM_OF_CMD_NO_PINGS_BMSK		GENMASK(21, 17)
> +#define SWRM_MCP_CFG_MAX_NUM_OF_CMD_NO_PINGS_SHFT		0x11
> +#define SWRM_MCP_STATUS						0x104C
> +#define SWRM_MCP_STATUS_BANK_NUM_MASK				BIT(0)
> +#define SWRM_MCP_SLV_STATUS					0x1090
> +#define SWRM_MCP_SLV_STATUS_MASK				GENMASK(1, 0)
> +#define SWRM_DP_PORT_CTRL_BANK(n, m)	(0x1124 + 0x100 * (n - 1) + 0x40 * m)
> +#define SWRM_DP_PORT_CTRL2_BANK(n, m)	(0x1126 + 0x100 * (n - 1) + 0x40 * m)
> +#define SWRM_DP_PORT_CTRL_EN_CHAN_SHFT				0x18
> +#define SWRM_DP_PORT_CTRL_OFFSET2_SHFT				0x10
> +#define SWRM_DP_PORT_CTRL_OFFSET1_SHFT				0x08
> +#define SWRM_AHB_BRIDGE_WR_DATA_0				0xc885
> +#define SWRM_AHB_BRIDGE_WR_ADDR_0				0xc889
> +#define SWRM_AHB_BRIDGE_RD_ADDR_0				0xc88d
> +#define SWRM_AHB_BRIDGE_RD_DATA_0				0xc891
> +
> +#define SWRM_REG_VAL_PACK(data, dev, id, reg)	\
> +			((reg) | ((id) << 16) | ((dev) << 20) | ((data) << 24))
> +
> +#define SWRM_MAX_ROW_VAL	0 /* Rows = 48 */
> +#define SWRM_DEFAULT_ROWS	48
> +#define SWRM_MIN_COL_VAL	0 /* Cols = 2 */
> +#define SWRM_DEFAULT_COL	16
> +#define SWRM_SPECIAL_CMD_ID	0xF
> +#define MAX_FREQ_NUM		1
> +#define TIMEOUT_MS		1000
> +#define QCOM_SWRM_MAX_RD_LEN	0xf
> +#define DEFAULT_CLK_FREQ	9600000
> +#define SWRM_MAX_DAIS		0xF

I was thinking it would make sense to move this to DT, DAI is after all
a hw property!

> +static int qcom_swrm_cmd_fifo_wr_cmd(struct qcom_swrm_ctrl *ctrl, u8 cmd_data,
> +				     u8 dev_addr, u16 reg_addr)
> +{
> +	int ret = 0;
> +	u8 cmd_id;
> +	u32 val;
> +
> +	mutex_lock(&ctrl->lock);
> +	if (dev_addr == SDW_BROADCAST_DEV_NUM) {
> +		cmd_id = SWRM_SPECIAL_CMD_ID;
> +	} else {
> +		if (++ctrl->wr_cmd_id == SWRM_SPECIAL_CMD_ID)
> +			ctrl->wr_cmd_id = 0;
> +
> +		cmd_id = ctrl->wr_cmd_id;
> +	}
> +
> +	val = SWRM_REG_VAL_PACK(cmd_data, dev_addr, cmd_id, reg_addr);
> +	ret = ctrl->reg_write(ctrl, SWRM_CMD_FIFO_WR_CMD, val);
> +	if (ret < 0) {
> +		dev_err(ctrl->dev, "%s: reg 0x%x write failed, err:%d\n",
> +			__func__, val, ret);
> +		goto err;
> +	}
> +
> +	if (dev_addr == SDW_BROADCAST_DEV_NUM) {
> +		ctrl->fifo_status = 0;
> +		ret = wait_for_completion_timeout(&ctrl->sp_cmd_comp,
> +						  msecs_to_jiffies(TIMEOUT_MS));

why not wait for completion on non broadcast?

> +static int qcom_swrm_cmd_fifo_rd_cmd(struct qcom_swrm_ctrl *ctrl,
> +				     u8 dev_addr, u16 reg_addr,
> +				     u32 len, u8 *rval)
> +{
> +	int i, ret = 0;

Superfluous initialization of ret

> +static irqreturn_t qcom_swrm_irq_handler(int irq, void *dev_id)
> +{
> +	struct qcom_swrm_ctrl *ctrl = dev_id;
> +	u32 sts, value;
> +
> +	sts = ctrl->reg_read(ctrl, SWRM_INTERRUPT_STATUS);
> +
> +	if (sts & SWRM_INTERRUPT_STATUS_SPECIAL_CMD_ID_FINISHED)
> +		complete(&ctrl->sp_cmd_comp);
> +
> +	if (sts & SWRM_INTERRUPT_STATUS_CMD_ERROR) {
> +		value = ctrl->reg_read(ctrl, SWRM_CMD_FIFO_STATUS);
> +		dev_err_ratelimited(ctrl->dev,
> +				    "CMD error, fifo status 0x%x\n",
> +				     value);
> +		ctrl->reg_write(ctrl, SWRM_CMD_FIFO_CMD, 0x1);
> +		if ((value & 0xF) == 0xF) {
> +			ctrl->fifo_status = -ENODATA;
> +			complete(&ctrl->sp_cmd_comp);
> +		}
> +	}
> +
> +	if ((sts & SWRM_INTERRUPT_STATUS_NEW_SLAVE_ATTACHED) ||
> +	    sts & SWRM_INTERRUPT_STATUS_CHANGE_ENUM_SLAVE_STATUS) {
> +		if (sts & SWRM_INTERRUPT_STATUS_NEW_SLAVE_ATTACHED)
> +			ctrl->status[0] = SDW_SLAVE_ATTACHED;
> +
> +		schedule_work(&ctrl->slave_work);

So why are we scheduling work, you are the thread handler so I think it
should be okay and better to invoke bus for status update.

> +	}
> +
> +	if (sts & SWRM_INTERRUPT_STATUS_SLAVE_PEND_IRQ)
> +		dev_dbg(ctrl->dev, "Slave pend irq\n");
> +
> +	if (sts & SWRM_INTERRUPT_STATUS_NEW_SLAVE_ATTACHED)
> +		dev_dbg(ctrl->dev, "New slave attached\n");

No updating bus on the status?

> +static enum sdw_command_response qcom_swrm_xfer_msg(struct sdw_bus *bus,
> +						    struct sdw_msg *msg)
> +{
> +	struct qcom_swrm_ctrl *ctrl = to_qcom_sdw(bus);
> +	int ret, i, len;
> +
> +	if (msg->flags == SDW_MSG_FLAG_READ) {
> +		for (i = 0; i < msg->len;) {
> +			if ((msg->len - i) < QCOM_SWRM_MAX_RD_LEN)
> +				len = msg->len - i;
> +			else
> +				len = QCOM_SWRM_MAX_RD_LEN;
> +
> +			ret = qcom_swrm_cmd_fifo_rd_cmd(ctrl, msg->dev_num,
> +							msg->addr + i, len,
> +						       &msg->buf[i]);
> +			if (ret < 0) {
> +				if (ret == -ENODATA)
> +					return SDW_CMD_IGNORED;
> +
> +				return ret;
> +			}
> +
> +			i = i + len;
> +		}
> +	} else if (msg->flags == SDW_MSG_FLAG_WRITE) {
> +		for (i = 0; i < msg->len; i++) {
> +			ret = qcom_swrm_cmd_fifo_wr_cmd(ctrl, msg->buf[i],
> +							msg->dev_num,
> +						       msg->addr + i);
> +			if (ret < 0) {
> +				if (ret == -ENODATA)
> +					return SDW_CMD_IGNORED;
> +
> +				return ret;

So we need to convert this to sdw_command_response before returning.

> +static int qcom_swrm_prepare(struct snd_pcm_substream *substream,
> +			     struct snd_soc_dai *dai)
> +{
> +	struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(dai->dev);
> +
> +	if (!ctrl->sruntime[dai->id])
> +		return -EINVAL;
> +
> +	return sdw_enable_stream(ctrl->sruntime[dai->id]);

Hmm you need to handle dai prepare being called for multiple times.

> +static int qcom_pdm_set_sdw_stream(struct snd_soc_dai *dai,
> +				   void *stream, int direction)
> +{
> +	return 0;
> +}

lets remove if we dont intend to do anything!

> +static int qcom_swrm_runtime_suspend(struct device *device)
> +{
> +	/* TBD */
> +	return 0;
> +}
> +
> +static int qcom_swrm_runtime_resume(struct device *device)
> +{
> +	/* TBD */
> +	return 0;
> +}

Again, lets remove these, add when we have the functionality
-- 
~Vinod

Re: [RFC PATCH 6/6] soundwire: qcom: add support for SoundWire controller

[Srinivas Kandagatla]

Thanks for taking time to review!


On 10/06/2019 07:40, Vinod Koul wrote:
> On 07-06-19, 09:56, Srinivas Kandagatla wrote:
>> Qualcomm SoundWire Master controller is present in most Qualcomm SoCs
>> either integrated as part of WCD audio codecs via slimbus or
>> as part of SOC I/O.
>>
>> This patchset adds support to a very basic controller which has been
>> tested with WCD934x SoundWire controller connected to WSA881x smart
>> speaker amplifiers.
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
>>   drivers/soundwire/Kconfig  |   9 +
>>   drivers/soundwire/Makefile |   4 +
>>   drivers/soundwire/qcom.c   | 983 +++++++++++++++++++++++++++++++++++++
> 
> Can you split this to two patches (at least), master driver followed by
> DAI driver

Sure.

> 
>> +
>> +#define SWRM_COMP_HW_VERSION					0x00
> 
> What does COMP stand for?

Controller splits registers into "Component(core configuration 
registers)", "Interrupt" "Command Fifo" and so on...

> 
>> +#define SWRM_COMP_CFG_ADDR					0x04
>> +#define SWRM_COMP_CFG_IRQ_LEVEL_OR_PULSE_MSK			BIT(1)
>> +#define SWRM_COMP_CFG_ENABLE_MSK				BIT(0)
>> +#define SWRM_COMP_PARAMS					0x100
>> +#define SWRM_COMP_PARAMS_DOUT_PORTS_MASK			GENMASK(4, 0)
>> +#define SWRM_COMP_PARAMS_DIN_PORTS_MASK				GENMASK(9, 5)
>> +#define SWRM_COMP_PARAMS_WR_FIFO_DEPTH				GENMASK(14, 10)
>> +#define SWRM_COMP_PARAMS_RD_FIFO_DEPTH				GENMASK(19, 15)
>> +#define SWRM_COMP_PARAMS_AUTO_ENUM_SLAVES			GENMASK(32. 20)
> 
> Thats a lot of text, So as others have said we need to rethink the
> naming conventions, perhaps QC_SDW_PARAM_AUTO_ENUM (feel free to drop
> SDW as well from here as it QC specific!
> 
> Also move common ones to core..
> 
>> +#define SWRM_INTERRUPT_STATUS					0x200
>> +#define SWRM_INTERRUPT_STATUS_RMSK				GENMASK(16, 0)
>> +#define SWRM_INTERRUPT_STATUS_SLAVE_PEND_IRQ			BIT(0)
>> +#define SWRM_INTERRUPT_STATUS_NEW_SLAVE_ATTACHED		BIT(1)
>> +#define SWRM_INTERRUPT_STATUS_CHANGE_ENUM_SLAVE_STATUS		BIT(2)
>> +#define SWRM_INTERRUPT_STATUS_MASTER_CLASH_DET			BIT(3)
>> +#define SWRM_INTERRUPT_STATUS_RD_FIFO_OVERFLOW			BIT(4)
>> +#define SWRM_INTERRUPT_STATUS_RD_FIFO_UNDERFLOW			BIT(5)
>> +#define SWRM_INTERRUPT_STATUS_WR_CMD_FIFO_OVERFLOW		BIT(6)
>> +#define SWRM_INTERRUPT_STATUS_CMD_ERROR				BIT(7)
>> +#define SWRM_INTERRUPT_STATUS_DOUT_PORT_COLLISION		BIT(8)
>> +#define SWRM_INTERRUPT_STATUS_READ_EN_RD_VALID_MISMATCH		BIT(9)
>> +#define SWRM_INTERRUPT_STATUS_SPECIAL_CMD_ID_FINISHED		BIT(10)
>> +#define SWRM_INTERRUPT_STATUS_NEW_SLAVE_AUTO_ENUM_FINISHED	BIT(11)
>> +#define SWRM_INTERRUPT_STATUS_AUTO_ENUM_FAILED			BIT(12)
>> +#define SWRM_INTERRUPT_STATUS_AUTO_ENUM_TABLE_IS_FULL		BIT(13)
>> +#define SWRM_INTERRUPT_STATUS_BUS_RESET_FINISHED		BIT(14)
>> +#define SWRM_INTERRUPT_STATUS_CLK_STOP_FINISHED			BIT(15)
>> +#define SWRM_INTERRUPT_STATUS_ERROR_PORT_TEST			BIT(16)
>> +#define SWRM_INTERRUPT_MASK_ADDR				0x204
>> +#define SWRM_INTERRUPT_CLEAR					0x208
>> +#define SWRM_CMD_FIFO_WR_CMD					0x300
>> +#define SWRM_CMD_FIFO_RD_CMD					0x304
>> +#define SWRM_CMD_FIFO_CMD					0x308
>> +#define SWRM_CMD_FIFO_STATUS					0x30C
>> +#define SWRM_CMD_FIFO_CFG_ADDR					0x314
>> +#define SWRM_CMD_FIFO_CFG_NUM_OF_CMD_RETRY_SHFT			0x0
>> +#define SWRM_CMD_FIFO_RD_FIFO_ADDR				0x318
>> +#define SWRM_ENUMERATOR_CFG_ADDR				0x500
>> +#define SWRM_MCP_FRAME_CTRL_BANK_ADDR(m)		(0x101C + 0x40 * (m))
>> +#define SWRM_MCP_FRAME_CTRL_BANK_SSP_PERIOD_SHFT		16
>> +#define SWRM_MCP_FRAME_CTRL_BANK_ROW_CTRL_SHFT			3
>> +#define SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_BMSK			GENMASK(2, 0)
>> +#define SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_SHFT			0
>> +#define SWRM_MCP_CFG_ADDR					0x1048
>> +#define SWRM_MCP_CFG_MAX_NUM_OF_CMD_NO_PINGS_BMSK		GENMASK(21, 17)
>> +#define SWRM_MCP_CFG_MAX_NUM_OF_CMD_NO_PINGS_SHFT		0x11
>> +#define SWRM_MCP_STATUS						0x104C
>> +#define SWRM_MCP_STATUS_BANK_NUM_MASK				BIT(0)
>> +#define SWRM_MCP_SLV_STATUS					0x1090
>> +#define SWRM_MCP_SLV_STATUS_MASK				GENMASK(1, 0)
>> +#define SWRM_DP_PORT_CTRL_BANK(n, m)	(0x1124 + 0x100 * (n - 1) + 0x40 * m)
>> +#define SWRM_DP_PORT_CTRL2_BANK(n, m)	(0x1126 + 0x100 * (n - 1) + 0x40 * m)
>> +#define SWRM_DP_PORT_CTRL_EN_CHAN_SHFT				0x18
>> +#define SWRM_DP_PORT_CTRL_OFFSET2_SHFT				0x10
>> +#define SWRM_DP_PORT_CTRL_OFFSET1_SHFT				0x08
>> +#define SWRM_AHB_BRIDGE_WR_DATA_0				0xc885
>> +#define SWRM_AHB_BRIDGE_WR_ADDR_0				0xc889
>> +#define SWRM_AHB_BRIDGE_RD_ADDR_0				0xc88d
>> +#define SWRM_AHB_BRIDGE_RD_DATA_0				0xc891
>> +
>> +#define SWRM_REG_VAL_PACK(data, dev, id, reg)	\
>> +			((reg) | ((id) << 16) | ((dev) << 20) | ((data) << 24))
>> +
>> +#define SWRM_MAX_ROW_VAL	0 /* Rows = 48 */
>> +#define SWRM_DEFAULT_ROWS	48
>> +#define SWRM_MIN_COL_VAL	0 /* Cols = 2 */
>> +#define SWRM_DEFAULT_COL	16
>> +#define SWRM_SPECIAL_CMD_ID	0xF
>> +#define MAX_FREQ_NUM		1
>> +#define TIMEOUT_MS		1000
>> +#define QCOM_SWRM_MAX_RD_LEN	0xf
>> +#define DEFAULT_CLK_FREQ	9600000
>> +#define SWRM_MAX_DAIS		0xF
> 
> I was thinking it would make sense to move this to DT, DAI is after all
> a hw property!

I will give that a try before sending out next version.
> 
>> +static int qcom_swrm_cmd_fifo_wr_cmd(struct qcom_swrm_ctrl *ctrl, u8 cmd_data,
>> +				     u8 dev_addr, u16 reg_addr)
>> +{
>> +	int ret = 0;
>> +	u8 cmd_id;
>> +	u32 val;
>> +
>> +	mutex_lock(&ctrl->lock);
>> +	if (dev_addr == SDW_BROADCAST_DEV_NUM) {
>> +		cmd_id = SWRM_SPECIAL_CMD_ID;
>> +	} else {
>> +		if (++ctrl->wr_cmd_id == SWRM_SPECIAL_CMD_ID)
>> +			ctrl->wr_cmd_id = 0;
>> +
>> +		cmd_id = ctrl->wr_cmd_id;
>> +	}
>> +
>> +	val = SWRM_REG_VAL_PACK(cmd_data, dev_addr, cmd_id, reg_addr);
>> +	ret = ctrl->reg_write(ctrl, SWRM_CMD_FIFO_WR_CMD, val);
>> +	if (ret < 0) {
>> +		dev_err(ctrl->dev, "%s: reg 0x%x write failed, err:%d\n",
>> +			__func__, val, ret);
>> +		goto err;
>> +	}
>> +
>> +	if (dev_addr == SDW_BROADCAST_DEV_NUM) {
>> +		ctrl->fifo_status = 0;
>> +		ret = wait_for_completion_timeout(&ctrl->sp_cmd_comp,
>> +						  msecs_to_jiffies(TIMEOUT_MS));
> 
> why not wait for completion on non broadcast?
> 

This could lead to dead-lock if we endup reading registers from 
interrupt handler.

>> +static int qcom_swrm_cmd_fifo_rd_cmd(struct qcom_swrm_ctrl *ctrl,
>> +				     u8 dev_addr, u16 reg_addr,
>> +				     u32 len, u8 *rval)
>> +{
>> +	int i, ret = 0;
> 
> Superfluous initialization of ret
> 
I agree.
>> +static irqreturn_t qcom_swrm_irq_handler(int irq, void *dev_id)
>> +{
>> +	struct qcom_swrm_ctrl *ctrl = dev_id;
>> +	u32 sts, value;
>> +
>> +	sts = ctrl->reg_read(ctrl, SWRM_INTERRUPT_STATUS);
>> +
>> +	if (sts & SWRM_INTERRUPT_STATUS_SPECIAL_CMD_ID_FINISHED)
>> +		complete(&ctrl->sp_cmd_comp);
>> +
>> +	if (sts & SWRM_INTERRUPT_STATUS_CMD_ERROR) {
>> +		value = ctrl->reg_read(ctrl, SWRM_CMD_FIFO_STATUS);
>> +		dev_err_ratelimited(ctrl->dev,
>> +				    "CMD error, fifo status 0x%x\n",
>> +				     value);
>> +		ctrl->reg_write(ctrl, SWRM_CMD_FIFO_CMD, 0x1);
>> +		if ((value & 0xF) == 0xF) {
>> +			ctrl->fifo_status = -ENODATA;
>> +			complete(&ctrl->sp_cmd_comp);
>> +		}
>> +	}
>> +
>> +	if ((sts & SWRM_INTERRUPT_STATUS_NEW_SLAVE_ATTACHED) ||
>> +	    sts & SWRM_INTERRUPT_STATUS_CHANGE_ENUM_SLAVE_STATUS) {
>> +		if (sts & SWRM_INTERRUPT_STATUS_NEW_SLAVE_ATTACHED)
>> +			ctrl->status[0] = SDW_SLAVE_ATTACHED;
>> +
>> +		schedule_work(&ctrl->slave_work);
> 
> So why are we scheduling work, you are the thread handler so I think it
> should be okay and better to invoke bus for status update.

I had seen lockup issues as this would trigger broadcast messages which 
would wait on completion interrupt!

> 
>> +	}
>> +
>> +	if (sts & SWRM_INTERRUPT_STATUS_SLAVE_PEND_IRQ)
>> +		dev_dbg(ctrl->dev, "Slave pend irq\n");
>> +
>> +	if (sts & SWRM_INTERRUPT_STATUS_NEW_SLAVE_ATTACHED)
>> +		dev_dbg(ctrl->dev, "New slave attached\n");
> 
> No updating bus on the status?
> 
Its done down below this function! These are debug messages only!
looks redundant, will remove it.

>> +static enum sdw_command_response qcom_swrm_xfer_msg(struct sdw_bus *bus,
>> +						    struct sdw_msg *msg)
>> +{
>> +	struct qcom_swrm_ctrl *ctrl = to_qcom_sdw(bus);
>> +	int ret, i, len;
>> +
>> +	if (msg->flags == SDW_MSG_FLAG_READ) {
>> +		for (i = 0; i < msg->len;) {
>> +			if ((msg->len - i) < QCOM_SWRM_MAX_RD_LEN)
>> +				len = msg->len - i;
>> +			else
>> +				len = QCOM_SWRM_MAX_RD_LEN;
>> +
>> +			ret = qcom_swrm_cmd_fifo_rd_cmd(ctrl, msg->dev_num,
>> +							msg->addr + i, len,
>> +						       &msg->buf[i]);
>> +			if (ret < 0) {
>> +				if (ret == -ENODATA)
>> +					return SDW_CMD_IGNORED;
>> +
>> +				return ret;
>> +			}
>> +
>> +			i = i + len;
>> +		}
>> +	} else if (msg->flags == SDW_MSG_FLAG_WRITE) {
>> +		for (i = 0; i < msg->len; i++) {
>> +			ret = qcom_swrm_cmd_fifo_wr_cmd(ctrl, msg->buf[i],
>> +							msg->dev_num,
>> +						       msg->addr + i);
>> +			if (ret < 0) {
>> +				if (ret == -ENODATA)
>> +					return SDW_CMD_IGNORED;
>> +
>> +				return ret;
> 
> So we need to convert this to sdw_command_response before returning.
> 
Sure!

>> +static int qcom_swrm_prepare(struct snd_pcm_substream *substream,
>> +			     struct snd_soc_dai *dai)
>> +{
>> +	struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(dai->dev);
>> +
>> +	if (!ctrl->sruntime[dai->id])
>> +		return -EINVAL;
>> +
>> +	return sdw_enable_stream(ctrl->sruntime[dai->id]);
> 
> Hmm you need to handle dai prepare being called for multiple times.
> Thanks for pointing this out, Will fix this.

>> +static int qcom_pdm_set_sdw_stream(struct snd_soc_dai *dai,
>> +				   void *stream, int direction)
>> +{
>> +	return 0;
>> +}
> 
> lets remove if we dont intend to do anything!
> 
Hm, not sure how I missed this one!

>> +static int qcom_swrm_runtime_suspend(struct device *device)
>> +{
>> +	/* TBD */
>> +	return 0;
>> +}
>> +
>> +static int qcom_swrm_runtime_resume(struct device *device)
>> +{
>> +	/* TBD */
>> +	return 0;
>> +}
> 
> Again, lets remove these, add when we have the functionality
We have issues without this, as soundwire bus would return error on 
runtime pm get/set. For RFC, I had to make this dummy, I will be able to 
add and test some code in next 1/2 spins.

Thanks,
srini
>