[PATCH 1/2] ASoC: wm_adsp: Only use __be32 for big-endian data

[PATCH 1/2] ASoC: wm_adsp: Only use __be32 for big-endian data

[Richard Fitzgerald]

This fixes some minor cases where u32 or unsigned int types were used
to store big-endian data, and __be32 types used to store both big-endian
and cpu-endian data. This was producing sparse warnings.

Most cases resulted from using the same variable to hold the big-endian
value and its converted cpu-endian value. These can be simply fixed by
introducing another local variable, or avoiding storing the intermediate
value back into the original variable.

One special case is the raw_buf used in the compressed streams to transfer
data from DSP to user-side. The endian conversion happens in-place (as
there's no point introducing another buffer) so a cast to (__be32 *) is
added when passing it to wm_adsp_read_raw_data_block().

Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
 sound/soc/codecs/wm_adsp.c | 33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/sound/soc/codecs/wm_adsp.c b/sound/soc/codecs/wm_adsp.c
index 5b7d81a91df3..8cfa8ac1b8c4 100644
--- a/sound/soc/codecs/wm_adsp.c
+++ b/sound/soc/codecs/wm_adsp.c
@@ -980,7 +980,7 @@ static int wm_coeff_write_acked_control(struct wm_coeff_ctl *ctl,
 					unsigned int event_id)
 {
 	struct wm_adsp *dsp = ctl->dsp;
-	u32 val = cpu_to_be32(event_id);
+	__be32 val = cpu_to_be32(event_id);
 	unsigned int reg;
 	int i, ret;
 
@@ -3704,6 +3704,7 @@ static int wm_adsp_write_data_word(struct wm_adsp *dsp, int mem_type,
 				   unsigned int mem_addr, u32 data)
 {
 	struct wm_adsp_region const *mem = wm_adsp_find_region(dsp, mem_type);
+	__be32 val = cpu_to_be32(data & 0x00ffffffu);
 	unsigned int reg;
 
 	if (!mem)
@@ -3711,9 +3712,7 @@ static int wm_adsp_write_data_word(struct wm_adsp *dsp, int mem_type,
 
 	reg = dsp->ops->region_to_reg(mem, mem_addr);
 
-	data = cpu_to_be32(data & 0x00ffffffu);
-
-	return regmap_raw_write(dsp->regmap, reg, &data, sizeof(data));
+	return regmap_raw_write(dsp->regmap, reg, &val, sizeof(val));
 }
 
 static inline int wm_adsp_buffer_read(struct wm_adsp_compr_buf *buf,
@@ -3870,7 +3869,8 @@ static int wm_adsp_buffer_parse_coeff(struct wm_coeff_ctl *ctl)
 {
 	struct wm_adsp_host_buf_coeff_v1 coeff_v1;
 	struct wm_adsp_compr_buf *buf;
-	unsigned int val, reg;
+	unsigned int reg, version;
+	__be32 bufp;
 	int ret, i;
 
 	ret = wm_coeff_base_reg(ctl, &reg);
@@ -3878,17 +3878,17 @@ static int wm_adsp_buffer_parse_coeff(struct wm_coeff_ctl *ctl)
 		return ret;
 
 	for (i = 0; i < 5; ++i) {
-		ret = regmap_raw_read(ctl->dsp->regmap, reg, &val, sizeof(val));
+		ret = regmap_raw_read(ctl->dsp->regmap, reg, &bufp, sizeof(bufp));
 		if (ret < 0)
 			return ret;
 
-		if (val)
+		if (bufp)
 			break;
 
 		usleep_range(1000, 2000);
 	}
 
-	if (!val) {
+	if (!bufp) {
 		adsp_err(ctl->dsp, "Failed to acquire host buffer\n");
 		return -EIO;
 	}
@@ -3898,7 +3898,7 @@ static int wm_adsp_buffer_parse_coeff(struct wm_coeff_ctl *ctl)
 		return -ENOMEM;
 
 	buf->host_buf_mem_type = ctl->alg_region.type;
-	buf->host_buf_ptr = be32_to_cpu(val);
+	buf->host_buf_ptr = be32_to_cpu(bufp);
 
 	ret = wm_adsp_buffer_populate(buf);
 	if (ret < 0)
@@ -3918,14 +3918,13 @@ static int wm_adsp_buffer_parse_coeff(struct wm_coeff_ctl *ctl)
 	if (ret < 0)
 		return ret;
 
-	coeff_v1.versions = be32_to_cpu(coeff_v1.versions);
-	val = coeff_v1.versions & HOST_BUF_COEFF_COMPAT_VER_MASK;
-	val >>= HOST_BUF_COEFF_COMPAT_VER_SHIFT;
+	version = be32_to_cpu(coeff_v1.versions) & HOST_BUF_COEFF_COMPAT_VER_MASK;
+	version >>= HOST_BUF_COEFF_COMPAT_VER_SHIFT;
 
-	if (val > HOST_BUF_COEFF_SUPPORTED_COMPAT_VER) {
+	if (version > HOST_BUF_COEFF_SUPPORTED_COMPAT_VER) {
 		adsp_err(ctl->dsp,
 			 "Host buffer coeff ver %u > supported version %u\n",
-			 val, HOST_BUF_COEFF_SUPPORTED_COMPAT_VER);
+			 version, HOST_BUF_COEFF_SUPPORTED_COMPAT_VER);
 		return -EINVAL;
 	}
 
@@ -3935,9 +3934,9 @@ static int wm_adsp_buffer_parse_coeff(struct wm_coeff_ctl *ctl)
 			      (char *)&coeff_v1.name);
 
 	compr_dbg(buf, "host_buf_ptr=%x coeff version %u\n",
-		  buf->host_buf_ptr, val);
+		  buf->host_buf_ptr, version);
 
-	return val;
+	return version;
 }
 
 static int wm_adsp_buffer_init(struct wm_adsp *dsp)
@@ -4269,7 +4268,7 @@ static int wm_adsp_buffer_capture_block(struct wm_adsp_compr *compr, int target)
 
 	/* Read data from DSP */
 	ret = wm_adsp_read_raw_data_block(buf->dsp, mem_type, adsp_addr,
-					  nwords, compr->raw_buf);
+					  nwords, (__be32 *)compr->raw_buf);
 	if (ret < 0)
 		return ret;
 
-- 
2.20.1

[PATCH 2/2] ASoC: wm_adsp: Use snd_ctl_elem_type_t for control types

[Richard Fitzgerald]

Sparse will complain about trying to convert between values declared
as snd_ctl_elem_type_t and other types. This patch converts to
consistently use snd_ctl_elem_type_t for control type values. A __force
cast is needed in a couple of cases where the control type value is
parsed out of a DSP data block.

Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
 sound/soc/codecs/wm_adsp.c | 12 +++++++-----
 sound/soc/codecs/wmfw.h    |  6 +++---
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/sound/soc/codecs/wm_adsp.c b/sound/soc/codecs/wm_adsp.c
index 8cfa8ac1b8c4..f8ad768364c2 100644
--- a/sound/soc/codecs/wm_adsp.c
+++ b/sound/soc/codecs/wm_adsp.c
@@ -619,7 +619,7 @@ struct wm_coeff_ctl {
 	unsigned int set:1;
 	struct soc_bytes_ext bytes_ext;
 	unsigned int flags;
-	unsigned int type;
+	snd_ctl_elem_type_t type;
 };
 
 static const char *wm_adsp_mem_region_name(unsigned int type)
@@ -1420,7 +1420,7 @@ static int wm_adsp_create_control(struct wm_adsp *dsp,
 				  const struct wm_adsp_alg_region *alg_region,
 				  unsigned int offset, unsigned int len,
 				  const char *subname, unsigned int subname_len,
-				  unsigned int flags, unsigned int type)
+				  unsigned int flags, snd_ctl_elem_type_t type)
 {
 	struct wm_coeff_ctl *ctl;
 	struct wmfw_ctl_work *ctl_work;
@@ -1554,7 +1554,7 @@ struct wm_coeff_parsed_coeff {
 	int mem_type;
 	const u8 *name;
 	int name_len;
-	int ctl_type;
+	snd_ctl_elem_type_t ctl_type;
 	int flags;
 	int len;
 };
@@ -1649,7 +1649,7 @@ static inline void wm_coeff_parse_coeff(struct wm_adsp *dsp, const u8 **data,
 		blk->mem_type = le16_to_cpu(raw->hdr.type);
 		blk->name = raw->name;
 		blk->name_len = strlen(raw->name);
-		blk->ctl_type = le16_to_cpu(raw->ctl_type);
+		blk->ctl_type = (__force snd_ctl_elem_type_t)le16_to_cpu(raw->ctl_type);
 		blk->flags = le16_to_cpu(raw->flags);
 		blk->len = le32_to_cpu(raw->len);
 		break;
@@ -1662,7 +1662,9 @@ static inline void wm_coeff_parse_coeff(struct wm_adsp *dsp, const u8 **data,
 						      &blk->name);
 		wm_coeff_parse_string(sizeof(u8), &tmp, NULL);
 		wm_coeff_parse_string(sizeof(u16), &tmp, NULL);
-		blk->ctl_type = wm_coeff_parse_int(sizeof(raw->ctl_type), &tmp);
+		blk->ctl_type =
+			(__force snd_ctl_elem_type_t)wm_coeff_parse_int(sizeof(raw->ctl_type),
+									&tmp);
 		blk->flags = wm_coeff_parse_int(sizeof(raw->flags), &tmp);
 		blk->len = wm_coeff_parse_int(sizeof(raw->len), &tmp);
 
diff --git a/sound/soc/codecs/wmfw.h b/sound/soc/codecs/wmfw.h
index 7423272c30e9..f3d51602f85c 100644
--- a/sound/soc/codecs/wmfw.h
+++ b/sound/soc/codecs/wmfw.h
@@ -24,9 +24,9 @@
 #define WMFW_CTL_FLAG_READABLE    0x0001
 
 /* Non-ALSA coefficient types start at 0x1000 */
-#define WMFW_CTL_TYPE_ACKED       0x1000 /* acked control */
-#define WMFW_CTL_TYPE_HOSTEVENT   0x1001 /* event control */
-#define WMFW_CTL_TYPE_HOST_BUFFER 0x1002 /* host buffer pointer */
+#define WMFW_CTL_TYPE_ACKED       ((__force snd_ctl_elem_type_t)0x1000) /* acked control */
+#define WMFW_CTL_TYPE_HOSTEVENT   ((__force snd_ctl_elem_type_t)0x1001) /* event control */
+#define WMFW_CTL_TYPE_HOST_BUFFER ((__force snd_ctl_elem_type_t)0x1002) /* host buffer pointer */
 
 struct wmfw_header {
 	char magic[4];
-- 
2.20.1

Re: [PATCH 1/2] ASoC: wm_adsp: Only use __be32 for big-endian data

[Mark Brown]

On Wed, 30 Dec 2020 17:24:26 +0000, Richard Fitzgerald wrote:
> This fixes some minor cases where u32 or unsigned int types were used
> to store big-endian data, and __be32 types used to store both big-endian
> and cpu-endian data. This was producing sparse warnings.
> 
> Most cases resulted from using the same variable to hold the big-endian
> value and its converted cpu-endian value. These can be simply fixed by
> introducing another local variable, or avoiding storing the intermediate
> value back into the original variable.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/2] ASoC: wm_adsp: Only use __be32 for big-endian data
      commit: a0b653e89a3afd2a833f23589a83488fac842ddb
[2/2] ASoC: wm_adsp: Use snd_ctl_elem_type_t for control types
      commit: f6212e0ab3ff28d4e2e53a520496a052c241df39

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark