Linux Sound subsystem development
 help / color / mirror / Atom feed
* [PATCH 00/15] Tidy up ASoC control get and put handlers
@ 2025-03-18 17:14 Charles Keepax
  2025-03-18 17:14 ` [PATCH 01/15] ASoC: ops-test: Add some basic kunit tests for soc-ops Charles Keepax
                   ` (15 more replies)
  0 siblings, 16 replies; 20+ messages in thread
From: Charles Keepax @ 2025-03-18 17:14 UTC (permalink / raw)
  To: broonie
  Cc: lgirdwood, shenghao-ding, kevin-lu, baojun.xu, linux-sound,
	linux-kernel, patches

There is a lot of duplicated and occasionally slightly incorrect code
around the ASoC control get and put handlers. This series add some kunit
tests and then refactors the code to get all the tests passing and
reduce some of the duplication. The focus here is on the volsw handlers,
future work could still be done on some of the others but these were the
ones that most required attention.

Hopefully the only slightly controversal change is the very last patch
which changes platform_max to be applied after the control type is
determined, more discussion in the commit message for that one.

Thanks,
Charles

Charles Keepax (15):
  ASoC: ops-test: Add some basic kunit tests for soc-ops
  ASoC: ops: Minor formatting fixups
  ASoC: ops: Update comments for xr_sx control helpers
  ASoC: ops: Update mask generation to use GENMASK
  ASoC: ops: Factor out helper to check valid control values
  ASoC: ops: Replace snd_soc_read_signed() with new helper
  ASoC: ops: Add control to register value helper
  ASoC: ops: Remove snd_soc_info_volsw_range()
  ASoC: ops: Remove snd_soc_get_volsw_range()
  ASoC: ops: Remove snd_soc_put_volsw_range()
  ASoC: ops: Factor out common code from info callbacks
  ASoC: ops: Factor out common code from put callbacks
  ASoC: ops: Factor out common code from get callbacks
  ASoC: ops: Remove some unnecessary local variables
  ASoC: ops: Apply platform_max after deciding control type

 include/sound/soc.h             |  24 +-
 sound/pci/hda/tas2781_hda_i2c.c |   2 +-
 sound/pci/hda/tas2781_hda_spi.c |   2 +-
 sound/soc/Kconfig               |   7 +
 sound/soc/Makefile              |   4 +
 sound/soc/codecs/wm5110.c       |   4 +-
 sound/soc/soc-ops-test.c        | 541 ++++++++++++++++++++++++
 sound/soc/soc-ops.c             | 712 +++++++++++---------------------
 sound/soc/soc-topology.c        |   4 +-
 9 files changed, 807 insertions(+), 493 deletions(-)
 create mode 100644 sound/soc/soc-ops-test.c

-- 
2.39.5


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH 01/15] ASoC: ops-test: Add some basic kunit tests for soc-ops
  2025-03-18 17:14 [PATCH 00/15] Tidy up ASoC control get and put handlers Charles Keepax
@ 2025-03-18 17:14 ` Charles Keepax
  2025-03-18 17:14 ` [PATCH 02/15] ASoC: ops: Minor formatting fixups Charles Keepax
                   ` (14 subsequent siblings)
  15 siblings, 0 replies; 20+ messages in thread
From: Charles Keepax @ 2025-03-18 17:14 UTC (permalink / raw)
  To: broonie
  Cc: lgirdwood, shenghao-ding, kevin-lu, baojun.xu, linux-sound,
	linux-kernel, patches

Add some basic kunit tests for some of the ASoC control put and get
helpers. This will assist in doing some refactoring. Note that presently
some tests fail, but the rest of the series will fix these up.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 sound/soc/Kconfig        |   7 +
 sound/soc/Makefile       |   4 +
 sound/soc/soc-ops-test.c | 541 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 552 insertions(+)
 create mode 100644 sound/soc/soc-ops-test.c

diff --git a/sound/soc/Kconfig b/sound/soc/Kconfig
index 5efba76abb31a..8b7d51266f814 100644
--- a/sound/soc/Kconfig
+++ b/sound/soc/Kconfig
@@ -81,6 +81,13 @@ config SND_SOC_UTILS_KUNIT_TEST
 	help
 	  If you want to perform tests on ALSA SoC utils library say Y here.
 
+config SND_SOC_OPS_KUNIT_TEST
+	tristate "KUnit tests for SoC ops"
+	depends on KUNIT
+	default KUNIT_ALL_TESTS
+	help
+	  If you want to perform tests on ALSA SoC ops library say Y here.
+
 config SND_SOC_ACPI
 	tristate
 
diff --git a/sound/soc/Makefile b/sound/soc/Makefile
index 08baaa11d8139..358e227c5ab61 100644
--- a/sound/soc/Makefile
+++ b/sound/soc/Makefile
@@ -21,6 +21,10 @@ ifneq ($(CONFIG_SND_SOC_UTILS_KUNIT_TEST),)
 obj-$(CONFIG_SND_SOC_UTILS_KUNIT_TEST) += soc-utils-test.o
 endif
 
+ifneq ($(CONFIG_SND_SOC_OPS_KUNIT_TEST),)
+obj-$(CONFIG_SND_SOC_OPS_KUNIT_TEST) += soc-ops-test.o
+endif
+
 ifneq ($(CONFIG_SND_SOC_GENERIC_DMAENGINE_PCM),)
 snd-soc-core-y += soc-generic-dmaengine-pcm.o
 endif
diff --git a/sound/soc/soc-ops-test.c b/sound/soc/soc-ops-test.c
new file mode 100644
index 0000000000000..dc1e482bba6a9
--- /dev/null
+++ b/sound/soc/soc-ops-test.c
@@ -0,0 +1,541 @@
+// SPDX-License-Identifier: GPL-2.0-only
+// Copyright (C) 2025 Cirrus Logic, Inc. and
+//                    Cirrus Logic International Semiconductor Ltd.
+
+#include <kunit/device.h>
+#include <kunit/test.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/string.h>
+#include <sound/asound.h>
+#include <sound/control.h>
+#include <sound/soc.h>
+#include <sound/soc-component.h>
+
+enum soc_ops_test_control_layout {
+	SOC_OPS_TEST_SINGLE,
+	SOC_OPS_TEST_DOUBLE,
+	SOC_OPS_TEST_DOUBLE_R,
+};
+
+#define TEST_MC(clayout, xmin, xmax, xpmax, xsign, xinvert) \
+	.mc = { \
+		.min = xmin, .max = xmax, .platform_max = xpmax, \
+		.reg = 0, .shift = 0, .sign_bit = xsign, .invert = xinvert, \
+		.rreg = SOC_OPS_TEST_##clayout == SOC_OPS_TEST_DOUBLE_R ? 1 : 0, \
+		.rshift = SOC_OPS_TEST_##clayout == SOC_OPS_TEST_DOUBLE ? 16 : 0, \
+	}
+
+#define TEST_UINFO(clayout, ctype, cmin, cmax) \
+	.uinfo = { \
+		.type = SNDRV_CTL_ELEM_TYPE_##ctype, \
+		.count = SOC_OPS_TEST_##clayout == SOC_OPS_TEST_SINGLE ? 1 : 2, \
+		.value.integer.min = cmin, \
+		.value.integer.max = cmax, \
+	}
+
+#define ITEST(cname, clayout, ctype, cfunc, cmin, cmax, \
+	      xmin, xmax, xpmax, xsign, xinvert) \
+	{ \
+		.name = cname, \
+		.func_name = #cfunc, \
+		.layout = SOC_OPS_TEST_##clayout, \
+		.info = snd_soc_info_##cfunc, \
+		TEST_MC(clayout, xmin, xmax, xpmax, xsign, xinvert), \
+		TEST_UINFO(clayout, ctype, cmin, cmax), \
+	}
+
+#define ATEST(clayout, cfunc, cctl, cret, cinit, \
+	      xmask, xreg, xmin, xmax, xpmax, xsign, xinvert) \
+	{ \
+		.func_name = #cfunc, \
+		.layout = SOC_OPS_TEST_##clayout, \
+		.put = snd_soc_put_##cfunc, \
+		.get = snd_soc_get_##cfunc, \
+		TEST_MC(clayout, xmin, xmax, xpmax, xsign, xinvert), \
+		.lctl = cctl, .rctl = cctl, \
+		.lmask = SOC_OPS_TEST_##clayout == SOC_OPS_TEST_DOUBLE ? \
+				(xmask) | (xmask) << 16 : (xmask), \
+		.rmask = SOC_OPS_TEST_##clayout == SOC_OPS_TEST_DOUBLE_R ? (xmask) : 0, \
+		.init = cinit ? 0xFFFFFFFF : 0x00000000, \
+		.lreg = SOC_OPS_TEST_##clayout == SOC_OPS_TEST_DOUBLE ? \
+				(xreg) | (xreg) << 16 : (xreg), \
+		.rreg = SOC_OPS_TEST_##clayout == SOC_OPS_TEST_DOUBLE_R ? (xreg) : 0, \
+		.ret = cret, \
+	}
+
+struct soc_ops_test_priv {
+	struct kunit *test;
+
+	struct snd_soc_component component;
+};
+
+struct info_test_param {
+	const char * const name;
+	const char * const func_name;
+	enum soc_ops_test_control_layout layout;
+	struct soc_mixer_control mc;
+	int (*info)(struct snd_kcontrol *kctl, struct snd_ctl_elem_info *info);
+
+	struct snd_ctl_elem_info uinfo;
+};
+
+struct access_test_param {
+	const char * const func_name;
+	enum soc_ops_test_control_layout layout;
+	struct soc_mixer_control mc;
+	int (*put)(struct snd_kcontrol *kctl, struct snd_ctl_elem_value *value);
+	int (*get)(struct snd_kcontrol *kctl, struct snd_ctl_elem_value *value);
+
+	unsigned int init;
+	unsigned int lmask;
+	unsigned int rmask;
+	unsigned int lreg;
+	unsigned int rreg;
+	long lctl;
+	long rctl;
+	int ret;
+};
+
+static const struct info_test_param all_info_test_params[] = {
+	// Handling of volume control name for types
+	ITEST("Test Control",        SINGLE,   BOOLEAN, volsw,    0,  1,   0,  1,  0, 0, 0),
+	ITEST("Test Volume",         SINGLE,   INTEGER, volsw,    0,  1,   0,  1,  0, 0, 0),
+	ITEST("Test Volume Control", SINGLE,   BOOLEAN, volsw,    0,  1,   0,  1,  0, 0, 0),
+	ITEST("Test Control",        DOUBLE_R, BOOLEAN, volsw,    0,  1,   0,  1,  0, 0, 0),
+	ITEST("Test Volume",         DOUBLE_R, INTEGER, volsw,    0,  1,   0,  1,  0, 0, 0),
+	ITEST("Test Volume Control", DOUBLE_R, BOOLEAN, volsw,    0,  1,   0,  1,  0, 0, 0),
+	ITEST("Test Control",        DOUBLE,   BOOLEAN, volsw,    0,  1,   0,  1,  0, 0, 0),
+	ITEST("Test Volume",         DOUBLE,   INTEGER, volsw,    0,  1,   0,  1,  0, 0, 0),
+	ITEST("Test Volume Control", DOUBLE,   BOOLEAN, volsw,    0,  1,   0,  1,  0, 0, 0),
+	ITEST("Test Control",        SINGLE,   BOOLEAN, volsw,    0,  1,   0,  1,  0, 0, 1),
+	ITEST("Test Volume",         SINGLE,   INTEGER, volsw,    0,  1,   0,  1,  0, 0, 1),
+	ITEST("Test Volume Control", SINGLE,   BOOLEAN, volsw,    0,  1,   0,  1,  0, 0, 1),
+	ITEST("Test Control",        DOUBLE,   BOOLEAN, volsw,    0,  1,   0,  1,  0, 0, 1),
+	ITEST("Test Volume",         DOUBLE,   INTEGER, volsw,    0,  1,   0,  1,  0, 0, 1),
+	ITEST("Test Volume Control", DOUBLE,   BOOLEAN, volsw,    0,  1,   0,  1,  0, 0, 1),
+	ITEST("Test Control",        SINGLE,   INTEGER, volsw,    0,  2,   0,  2,  0, 0, 0),
+	ITEST("Test Volume",         SINGLE,   INTEGER, volsw,    0,  2,   0,  2,  0, 0, 0),
+	ITEST("Test Volume Control", SINGLE,   INTEGER, volsw,    0,  2,   0,  2,  0, 0, 0),
+	ITEST("Test Control",        SINGLE,   INTEGER, volsw,    0,  1,   0,  2,  1, 0, 0),
+	ITEST("Test Volume",         SINGLE,   INTEGER, volsw,    0,  1,   0,  2,  1, 0, 0),
+	ITEST("Test Volume Control", SINGLE,   INTEGER, volsw,    0,  1,   0,  2,  1, 0, 0),
+	// Negative minimums
+	ITEST("Test Control",        SINGLE,   INTEGER, volsw,    0, 20, -10, 10,  0, 4, 0),
+	ITEST("Test Control",        SINGLE,   INTEGER, volsw,    0, 15, -10, 10, 15, 4, 0),
+	ITEST("Test Control",        SINGLE,   INTEGER, volsw,    0, 20, -10, 10,  0, 4, 1),
+	ITEST("Test Control",        SINGLE,   INTEGER, volsw,    0, 15, -10, 10, 15, 4, 1),
+	// SX control volume control naming
+	ITEST("Test Control",        SINGLE,   BOOLEAN, volsw_sx, 0,  1, 0xF,  1,  0, 0, 0),
+	ITEST("Test Volume",         SINGLE,   INTEGER, volsw_sx, 0,  1, 0xF,  1,  0, 0, 0),
+	ITEST("Test Volume Control", SINGLE,   BOOLEAN, volsw_sx, 0,  1, 0xF,  1,  0, 0, 0),
+	ITEST("Test Control",        SINGLE,   INTEGER, volsw_sx, 0,  4, 0xE,  4,  0, 0, 0),
+	ITEST("Test Volume",         SINGLE,   INTEGER, volsw_sx, 0,  4, 0xE,  4,  0, 0, 0),
+	ITEST("Test Volume Control", SINGLE,   INTEGER, volsw_sx, 0,  4, 0xE,  4,  0, 0, 0),
+	ITEST("Test Control",        SINGLE,   INTEGER, volsw_sx, 0,  3, 0xE,  4,  3, 0, 0),
+	ITEST("Test Volume",         SINGLE,   INTEGER, volsw_sx, 0,  3, 0xE,  4,  3, 0, 0),
+	ITEST("Test Volume Control", SINGLE,   INTEGER, volsw_sx, 0,  3, 0xE,  4,  3, 0, 0),
+};
+
+static const struct access_test_param all_access_test_params[] = {
+	// Single positive value controls
+	ATEST(SINGLE,   volsw,     10,   1, false, 0x1F, 0x0A,    0,  20,  0, 0, 0),
+	ATEST(SINGLE,   volsw,      0,   0, false, 0x1F, 0x00,    0,  20,  0, 0, 0),
+	ATEST(SINGLE,   volsw,     20,   1, false, 0x1F, 0x14,    0,  20,  0, 0, 0),
+	ATEST(SINGLE,   volsw,     10,   1, false, 0x1F, 0x0A,    0,  20, 15, 0, 0),
+	ATEST(SINGLE,   volsw,     25, -22, false, 0x1F, 0x00,    0,  20, 15, 0, 0),
+	ATEST(SINGLE,   volsw,     15,   1, false, 0x1F, 0x0F,    0,  20, 15, 0, 0),
+	// Inverted single positive value controls
+	ATEST(SINGLE,   volsw,     10,   1, false, 0x1F, 0x0A,    0,  20,  0, 0, 1),
+	ATEST(SINGLE,   volsw,      0,   1, false, 0x1F, 0x14,    0,  20,  0, 0, 1),
+	ATEST(SINGLE,   volsw,     20,   0, false, 0x1F, 0x00,    0,  20,  0, 0, 1),
+	ATEST(SINGLE,   volsw,     10,   1, false, 0x1F, 0x0A,    0,  20, 15, 0, 1),
+	ATEST(SINGLE,   volsw,     25, -22, false, 0x1F, 0x00,    0,  20, 15, 0, 1),
+	ATEST(SINGLE,   volsw,     15,   1, false, 0x1F, 0x05,    0,  20, 15, 0, 1),
+	ATEST(SINGLE,   volsw,     10,   1, true,  0x1F, 0x0A,    0,  20,  0, 0, 0),
+	ATEST(SINGLE,   volsw,      0,   1, true,  0x1F, 0x00,    0,  20,  0, 0, 0),
+	ATEST(SINGLE,   volsw,     20,   1, true,  0x1F, 0x14,    0,  20,  0, 0, 0),
+	ATEST(SINGLE,   volsw,     10,   1, true,  0x1F, 0x0A,    0,  20, 15, 0, 0),
+	ATEST(SINGLE,   volsw,     25, -22, true,  0x1F, 0x00,    0,  20, 15, 0, 0),
+	ATEST(SINGLE,   volsw,     15,   1, true,  0x1F, 0x0F,    0,  20, 15, 0, 0),
+	// Single negative value controls
+	ATEST(SINGLE,   volsw,     10,   0, false, 0x1F, 0x00,  -10,  10,  0, 4, 0),
+	ATEST(SINGLE,   volsw,      0,   1, false, 0x1F, 0x16,  -10,  10,  0, 4, 0),
+	ATEST(SINGLE,   volsw,     20,   1, false, 0x1F, 0x0A,  -10,  10,  0, 4, 0),
+	ATEST(SINGLE,   volsw,     10,   0, false, 0x1F, 0x00,  -10,  10, 15, 4, 0),
+	ATEST(SINGLE,   volsw,     25, -22, false, 0x1F, 0x00,  -10,  10, 15, 4, 0),
+	ATEST(SINGLE,   volsw,     15,   1, false, 0x1F, 0x05,  -10,  10, 15, 4, 0),
+	// Single non-zero minimum positive value controls
+	ATEST(SINGLE,   volsw,     10,   1, false, 0x1F, 0x14,   10,  30,  0, 0, 0),
+	ATEST(SINGLE,   volsw,      0,   1, false, 0x1F, 0x0A,   10,  30,  0, 0, 0),
+	ATEST(SINGLE,   volsw,     20,   1, false, 0x1F, 0x1E,   10,  30,  0, 0, 0),
+	ATEST(SINGLE,   volsw,     10,   1, false, 0x1F, 0x14,   10,  30, 15, 0, 0),
+	ATEST(SINGLE,   volsw,     25, -22, false, 0x1F, 0x00,   10,  30, 15, 0, 0),
+	ATEST(SINGLE,   volsw,     15,   1, false, 0x1F, 0x19,   10,  30, 15, 0, 0),
+	// Inverted single non-zero minimum positive value controls
+	ATEST(SINGLE,   volsw,     10,   1, false, 0x1F, 0x14,   10,  30,  0, 0, 1),
+	ATEST(SINGLE,   volsw,      0,   1, false, 0x1F, 0x1E,   10,  30,  0, 0, 1),
+	ATEST(SINGLE,   volsw,     20,   1, false, 0x1F, 0x0A,   10,  30,  0, 0, 1),
+	ATEST(SINGLE,   volsw,     10,   1, false, 0x1F, 0x14,   10,  30, 15, 0, 1),
+	ATEST(SINGLE,   volsw,     25, -22, false, 0x1F, 0x00,   10,  30, 15, 0, 1),
+	ATEST(SINGLE,   volsw,     15,   1, false, 0x1F, 0x0F,   10,  30, 15, 0, 1),
+	// Double register positive value controls
+	ATEST(DOUBLE_R, volsw,     10,   1, false, 0x1F, 0x0A,    0,  20,  0, 0, 0),
+	ATEST(DOUBLE_R, volsw,      0,   0, false, 0x1F, 0x00,    0,  20,  0, 0, 0),
+	ATEST(DOUBLE_R, volsw,     20,   1, false, 0x1F, 0x14,    0,  20,  0, 0, 0),
+	ATEST(DOUBLE_R, volsw,     10,   1, false, 0x1F, 0x0A,    0,  20, 15, 0, 0),
+	ATEST(DOUBLE_R, volsw,     25, -22, false, 0x1F, 0x00,    0,  20, 15, 0, 0),
+	ATEST(DOUBLE_R, volsw,     15,   1, false, 0x1F, 0x0F,    0,  20, 15, 0, 0),
+	// Double register negative value controls
+	ATEST(DOUBLE_R, volsw,     10,   0, false, 0x1F, 0x00,  -10,  10,  0, 4, 0),
+	ATEST(DOUBLE_R, volsw,      0,   1, false, 0x1F, 0x16,  -10,  10,  0, 4, 0),
+	ATEST(DOUBLE_R, volsw,     20,   1, false, 0x1F, 0x0A,  -10,  10,  0, 4, 0),
+	ATEST(DOUBLE_R, volsw,     10,   0, false, 0x1F, 0x00,  -10,  10, 15, 4, 0),
+	ATEST(DOUBLE_R, volsw,     25, -22, false, 0x1F, 0x00,  -10,  10, 15, 4, 0),
+	ATEST(DOUBLE_R, volsw,     15,   1, false, 0x1F, 0x05,  -10,  10, 15, 4, 0),
+	ATEST(DOUBLE_R, volsw,     10,   1, true,  0x1F, 0x00,  -10,  10,  0, 4, 0),
+	ATEST(DOUBLE_R, volsw,      0,   1, true,  0x1F, 0x16,  -10,  10,  0, 4, 0),
+	ATEST(DOUBLE_R, volsw,     20,   1, true,  0x1F, 0x0A,  -10,  10,  0, 4, 0),
+	ATEST(DOUBLE_R, volsw,     10,   1, true,  0x1F, 0x00,  -10,  10, 15, 4, 0),
+	ATEST(DOUBLE_R, volsw,     25, -22, true,  0x1F, 0x00,  -10,  10, 15, 4, 0),
+	ATEST(DOUBLE_R, volsw,     15,   1, true,  0x1F, 0x05,  -10,  10, 15, 4, 0),
+	// Inverted double register negative value controls
+	ATEST(DOUBLE_R, volsw,     10,   1, true,  0x1F, 0x00,  -10,  10,  0, 4, 1),
+	ATEST(DOUBLE_R, volsw,      0,   1, true,  0x1F, 0x0A,  -10,  10,  0, 4, 1),
+	ATEST(DOUBLE_R, volsw,     20,   1, true,  0x1F, 0x16,  -10,  10,  0, 4, 1),
+	ATEST(DOUBLE_R, volsw,     10,   1, true,  0x1F, 0x00,  -10,  10, 15, 4, 1),
+	ATEST(DOUBLE_R, volsw,     25, -22, true,  0x1F, 0x00,  -10,  10, 15, 4, 1),
+	ATEST(DOUBLE_R, volsw,     15,   1, true,  0x1F, 0x1B,  -10,  10, 15, 4, 1),
+	// Double register non-zero minimum positive value controls
+	ATEST(DOUBLE_R, volsw,     10,   1, false, 0x1F, 0x14,   10,  30,  0, 0, 0),
+	ATEST(DOUBLE_R, volsw,      0,   1, false, 0x1F, 0x0A,   10,  30,  0, 0, 0),
+	ATEST(DOUBLE_R, volsw,     20,   1, false, 0x1F, 0x1E,   10,  30,  0, 0, 0),
+	ATEST(DOUBLE_R, volsw,     10,   1, false, 0x1F, 0x14,   10,  30, 15, 0, 0),
+	ATEST(DOUBLE_R, volsw,     25, -22, false, 0x1F, 0x00,   10,  30, 15, 0, 0),
+	ATEST(DOUBLE_R, volsw,     15,   1, false, 0x1F, 0x19,   10,  30, 15, 0, 0),
+	// Double shift positive value controls
+	ATEST(DOUBLE,   volsw,     10,   1, false, 0x1F, 0x0A,    0,  20,  0, 0, 0),
+	ATEST(DOUBLE,   volsw,      0,   0, false, 0x1F, 0x00,    0,  20,  0, 0, 0),
+	ATEST(DOUBLE,   volsw,     20,   1, false, 0x1F, 0x14,    0,  20,  0, 0, 0),
+	ATEST(DOUBLE,   volsw,     10,   1, false, 0x1F, 0x0A,    0,  20, 15, 0, 0),
+	ATEST(DOUBLE,   volsw,     25, -22, false, 0x1F, 0x00,    0,  20, 15, 0, 0),
+	ATEST(DOUBLE,   volsw,     15,   1, false, 0x1F, 0x0F,    0,  20, 15, 0, 0),
+	// Double shift negative value controls
+	ATEST(DOUBLE,   volsw,     10,   0, false, 0x1F, 0x00,  -10,  10,  0, 4, 0),
+	ATEST(DOUBLE,   volsw,      0,   1, false, 0x1F, 0x16,  -10,  10,  0, 4, 0),
+	ATEST(DOUBLE,   volsw,     20,   1, false, 0x1F, 0x0A,  -10,  10,  0, 4, 0),
+	ATEST(DOUBLE,   volsw,     10,   0, false, 0x1F, 0x00,  -10,  10, 15, 4, 0),
+	ATEST(DOUBLE,   volsw,     25, -22, false, 0x1F, 0x00,  -10,  10, 15, 4, 0),
+	ATEST(DOUBLE,   volsw,     15,   1, false, 0x1F, 0x05,  -10,  10, 15, 4, 0),
+	// Inverted double shift negative value controls
+	ATEST(DOUBLE,   volsw,     10,   0, false, 0x1F, 0x00,  -10,  10,  0, 4, 1),
+	ATEST(DOUBLE,   volsw,      0,   1, false, 0x1F, 0x0A,  -10,  10,  0, 4, 1),
+	ATEST(DOUBLE,   volsw,     20,   1, false, 0x1F, 0x16,  -10,  10,  0, 4, 1),
+	ATEST(DOUBLE,   volsw,     10,   0, false, 0x1F, 0x00,  -10,  10, 15, 4, 1),
+	ATEST(DOUBLE,   volsw,     25, -22, false, 0x1F, 0x00,  -10,  10, 15, 4, 1),
+	ATEST(DOUBLE,   volsw,     15,   1, false, 0x1F, 0x1B,  -10,  10, 15, 4, 1),
+	// Double shift non-zero minimum positive value controls
+	ATEST(DOUBLE,   volsw,     10,   1, false, 0x1F, 0x14,   10,  30,  0, 0, 0),
+	ATEST(DOUBLE,   volsw,      0,   1, false, 0x1F, 0x0A,   10,  30,  0, 0, 0),
+	ATEST(DOUBLE,   volsw,     20,   1, false, 0x1F, 0x1E,   10,  30,  0, 0, 0),
+	ATEST(DOUBLE,   volsw,     10,   1, false, 0x1F, 0x14,   10,  30, 15, 0, 0),
+	ATEST(DOUBLE,   volsw,     25, -22, false, 0x1F, 0x00,   10,  30, 15, 0, 0),
+	ATEST(DOUBLE,   volsw,     15,   1, false, 0x1F, 0x19,   10,  30, 15, 0, 0),
+	ATEST(DOUBLE,   volsw,     10,   1, true,  0x1F, 0x14,   10,  30,  0, 0, 0),
+	ATEST(DOUBLE,   volsw,      0,   1, true,  0x1F, 0x0A,   10,  30,  0, 0, 0),
+	ATEST(DOUBLE,   volsw,     20,   1, true,  0x1F, 0x1E,   10,  30,  0, 0, 0),
+	ATEST(DOUBLE,   volsw,     10,   1, true,  0x1F, 0x14,   10,  30, 15, 0, 0),
+	ATEST(DOUBLE,   volsw,     25, -22, true,  0x1F, 0x00,   10,  30, 15, 0, 0),
+	ATEST(DOUBLE,   volsw,     15,   1, true,  0x1F, 0x19,   10,  30, 15, 0, 0),
+	// Single SX all values
+	ATEST(SINGLE,   volsw_sx,   0,   1, false,  0xF, 0x0F, 0x0F,   4,  0, 0, 0),
+	ATEST(SINGLE,   volsw_sx,   1,   0, false,  0xF, 0x00, 0x0F,   4,  0, 0, 0),
+	ATEST(SINGLE,   volsw_sx,   2,   1, false,  0xF, 0x01, 0x0F,   4,  0, 0, 0),
+	ATEST(SINGLE,   volsw_sx,   3,   1, false,  0xF, 0x02, 0x0F,   4,  0, 0, 0),
+	ATEST(SINGLE,   volsw_sx,   4,   1, false,  0xF, 0x03, 0x0F,   4,  0, 0, 0),
+	ATEST(SINGLE,   volsw_sx,   5, -22, false,  0xF, 0x00, 0x0F,   4,  0, 0, 0),
+	ATEST(SINGLE,   volsw_sx,   0,   0, true,   0xF, 0x0F, 0x0F,   4,  0, 0, 0),
+	ATEST(SINGLE,   volsw_sx,   1,   1, true,   0xF, 0x00, 0x0F,   4,  0, 0, 0),
+	ATEST(SINGLE,   volsw_sx,   2,   1, true,   0xF, 0x01, 0x0F,   4,  0, 0, 0),
+	ATEST(SINGLE,   volsw_sx,   3,   1, true,   0xF, 0x02, 0x0F,   4,  0, 0, 0),
+	ATEST(SINGLE,   volsw_sx,   4,   1, true,   0xF, 0x03, 0x0F,   4,  0, 0, 0),
+	ATEST(SINGLE,   volsw_sx,   5, -22, true,   0xF, 0x00, 0x0F,   4,  0, 0, 0),
+	// Inverted single SX all values
+	ATEST(SINGLE,   volsw_sx,   0,   1, false, 0x1F, 0x03, 0x0F,   4,  0, 0, 1),
+	ATEST(SINGLE,   volsw_sx,   1,   1, false, 0x1F, 0x02, 0x0F,   4,  0, 0, 1),
+	ATEST(SINGLE,   volsw_sx,   2,   1, false, 0x1F, 0x01, 0x0F,   4,  0, 0, 1),
+	ATEST(SINGLE,   volsw_sx,   3,   0, false, 0x1F, 0x00, 0x0F,   4,  0, 0, 1),
+	ATEST(SINGLE,   volsw_sx,   4,   1, false, 0x1F, 0x0F, 0x0F,   4,  0, 0, 1),
+	ATEST(SINGLE,   volsw_sx,   5, -22, false, 0x1F, 0x00, 0x0F,   4,  0, 0, 1),
+	// Single SX select values
+	ATEST(SINGLE,   volsw_sx,   0,   1, false, 0xFF, 0x88, 0x88, 144,  0, 0, 0),
+	ATEST(SINGLE,   volsw_sx,   1,   1, false, 0xFF, 0x89, 0x88, 144,  0, 0, 0),
+	ATEST(SINGLE,   volsw_sx, 119,   1, false, 0xFF, 0xFF, 0x88, 144,  0, 0, 0),
+	ATEST(SINGLE,   volsw_sx, 120,   0, false, 0xFF, 0x00, 0x88, 144,  0, 0, 0),
+	ATEST(SINGLE,   volsw_sx, 121,   1, false, 0xFF, 0x01, 0x88, 144,  0, 0, 0),
+	ATEST(SINGLE,   volsw_sx, 143,   1, false, 0xFF, 0x17, 0x88, 144,  0, 0, 0),
+	ATEST(SINGLE,   volsw_sx, 144,   1, false, 0xFF, 0x18, 0x88, 144,  0, 0, 0),
+	ATEST(SINGLE,   volsw_sx, 145, -22, false, 0xFF, 0x00, 0x88, 144,  0, 0, 0),
+	ATEST(SINGLE,   volsw_sx,   0,   1, true,  0xFF, 0x88, 0x88, 144,  0, 0, 0),
+	ATEST(SINGLE,   volsw_sx,   1,   1, true,  0xFF, 0x89, 0x88, 144,  0, 0, 0),
+	ATEST(SINGLE,   volsw_sx, 119,   0, true,  0xFF, 0xFF, 0x88, 144,  0, 0, 0),
+	ATEST(SINGLE,   volsw_sx, 120,   1, true,  0xFF, 0x00, 0x88, 144,  0, 0, 0),
+	ATEST(SINGLE,   volsw_sx, 121,   1, true,  0xFF, 0x01, 0x88, 144,  0, 0, 0),
+	ATEST(SINGLE,   volsw_sx, 143,   1, true,  0xFF, 0x17, 0x88, 144,  0, 0, 0),
+	ATEST(SINGLE,   volsw_sx, 144,   1, true,  0xFF, 0x18, 0x88, 144,  0, 0, 0),
+	ATEST(SINGLE,   volsw_sx, 145, -22, true,  0xFF, 0x00, 0x88, 144,  0, 0, 0),
+	// Double shift SX select values
+	ATEST(DOUBLE,   volsw_sx,   0,   1, true,  0xFF, 0x88, 0x88, 144,  0, 0, 0),
+	ATEST(DOUBLE,   volsw_sx,   1,   1, true,  0xFF, 0x89, 0x88, 144,  0, 0, 0),
+	ATEST(DOUBLE,   volsw_sx, 119,   0, true,  0xFF, 0xFF, 0x88, 144,  0, 0, 0),
+	ATEST(DOUBLE,   volsw_sx, 120,   1, true,  0xFF, 0x00, 0x88, 144,  0, 0, 0),
+	ATEST(DOUBLE,   volsw_sx, 121,   1, true,  0xFF, 0x01, 0x88, 144,  0, 0, 0),
+	ATEST(DOUBLE,   volsw_sx, 143,   1, true,  0xFF, 0x17, 0x88, 144,  0, 0, 0),
+	ATEST(DOUBLE,   volsw_sx, 144,   1, true,  0xFF, 0x18, 0x88, 144,  0, 0, 0),
+	ATEST(DOUBLE,   volsw_sx, 145, -22, true,  0xFF, 0x00, 0x88, 144,  0, 0, 0),
+	// Double register SX select values
+	ATEST(DOUBLE_R, volsw_sx,   0,   1, true,  0xFF, 0x88, 0x88, 144,  0, 0, 0),
+	ATEST(DOUBLE_R, volsw_sx,   1,   1, true,  0xFF, 0x89, 0x88, 144,  0, 0, 0),
+	ATEST(DOUBLE_R, volsw_sx, 119,   0, true,  0xFF, 0xFF, 0x88, 144,  0, 0, 0),
+	ATEST(DOUBLE_R, volsw_sx, 120,   1, true,  0xFF, 0x00, 0x88, 144,  0, 0, 0),
+	ATEST(DOUBLE_R, volsw_sx, 121,   1, true,  0xFF, 0x01, 0x88, 144,  0, 0, 0),
+	ATEST(DOUBLE_R, volsw_sx, 143,   1, true,  0xFF, 0x17, 0x88, 144,  0, 0, 0),
+	ATEST(DOUBLE_R, volsw_sx, 144,   1, true,  0xFF, 0x18, 0x88, 144,  0, 0, 0),
+	ATEST(DOUBLE_R, volsw_sx, 145, -22, true,  0xFF, 0x00, 0x88, 144,  0, 0, 0),
+};
+
+static const char *control_type_str(const snd_ctl_elem_type_t type)
+{
+	switch (type) {
+	case SNDRV_CTL_ELEM_TYPE_BOOLEAN:
+		return "bool";
+	case SNDRV_CTL_ELEM_TYPE_INTEGER:
+		return "int";
+	default:
+		return "unknown";
+	}
+}
+
+static const char *control_layout_str(const enum soc_ops_test_control_layout layout)
+{
+	switch (layout) {
+	case SOC_OPS_TEST_SINGLE:
+		return "single";
+	case SOC_OPS_TEST_DOUBLE:
+		return "double";
+	case SOC_OPS_TEST_DOUBLE_R:
+		return "double_r";
+	default:
+		return "unknown";
+	}
+};
+
+static int mock_regmap_read(void *context, const void *reg_buf,
+			    const size_t reg_size, void *val_buf,
+			    size_t val_size)
+{
+	struct soc_ops_test_priv *priv = context;
+
+	KUNIT_FAIL(priv->test, "Unexpected bus read");
+
+	return -EIO;
+}
+
+static int mock_regmap_gather_write(void *context,
+				    const void *reg_buf, size_t reg_size,
+				    const void *val_buf, size_t val_size)
+{
+	struct soc_ops_test_priv *priv = context;
+
+	KUNIT_FAIL(priv->test, "Unexpected bus gather_write");
+
+	return -EIO;
+}
+
+static int mock_regmap_write(void *context, const void *val_buf,
+			     size_t val_size)
+{
+	struct soc_ops_test_priv *priv = context;
+
+	KUNIT_FAIL(priv->test, "Unexpected bus write");
+
+	return -EIO;
+}
+
+static const struct regmap_bus mock_regmap_bus = {
+	.read = mock_regmap_read,
+	.write = mock_regmap_write,
+	.gather_write = mock_regmap_gather_write,
+	.reg_format_endian_default = REGMAP_ENDIAN_NATIVE,
+	.val_format_endian_default = REGMAP_ENDIAN_NATIVE,
+};
+
+static const struct regmap_config mock_regmap_config = {
+	.reg_bits = 32,
+	.val_bits = 32,
+	.reg_format_endian = REGMAP_ENDIAN_NATIVE,
+	.val_format_endian = REGMAP_ENDIAN_NATIVE,
+	.max_register = 0x1,
+	.cache_type = REGCACHE_FLAT,
+};
+
+static int soc_ops_test_init(struct kunit *test)
+{
+	struct soc_ops_test_priv *priv;
+	struct regmap *regmap;
+	struct device *dev;
+
+	priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->test = test;
+
+	dev = kunit_device_register(test, "soc_ops_test_drv");
+	if (IS_ERR(dev))
+		return PTR_ERR(dev);
+
+	regmap = devm_regmap_init(dev, &mock_regmap_bus, priv, &mock_regmap_config);
+	if (IS_ERR(regmap))
+		return PTR_ERR(regmap);
+
+	/* No actual hardware, we just use the cache */
+	regcache_cache_only(regmap, true);
+
+	priv->component.dev = dev;
+	priv->component.regmap = regmap;
+	mutex_init(&priv->component.io_mutex);
+
+	test->priv = priv;
+
+	return 0;
+}
+
+static void soc_ops_test_exit(struct kunit *test)
+{
+	struct soc_ops_test_priv *priv = test->priv;
+
+	kunit_device_unregister(test, priv->component.dev);
+}
+
+static void info_test_desc(const struct info_test_param *param, char *desc)
+{
+	snprintf(desc, KUNIT_PARAM_DESC_SIZE,
+		 "%s %s %s: ctl range: %ld->%ld, reg range: %d->%d(%d), sign: %d, inv: %d",
+		 control_layout_str(param->layout), param->func_name,
+		 control_type_str(param->uinfo.type),
+		 param->uinfo.value.integer.min, param->uinfo.value.integer.max,
+		 param->mc.min, param->mc.max, param->mc.platform_max,
+		 param->mc.sign_bit, param->mc.invert);
+}
+
+static void soc_ops_test_info(struct kunit *test)
+{
+	struct soc_ops_test_priv *priv = test->priv;
+	const struct info_test_param *param = test->param_value;
+	const struct snd_ctl_elem_info *target = &param->uinfo;
+	struct snd_ctl_elem_info result;
+	struct snd_kcontrol kctl = {
+		.private_data = &priv->component,
+		.private_value = (unsigned long)&param->mc,
+	};
+	int ret;
+
+	strscpy(kctl.id.name, param->name, sizeof(kctl.id.name));
+
+	ret = param->info(&kctl, &result);
+	KUNIT_ASSERT_FALSE(test, ret);
+
+	KUNIT_EXPECT_EQ(test, result.count, target->count);
+	KUNIT_EXPECT_EQ(test, result.type, target->type);
+	KUNIT_EXPECT_EQ(test, result.value.integer.min, target->value.integer.min);
+	KUNIT_EXPECT_EQ(test, result.value.integer.max, target->value.integer.max);
+}
+
+static void access_test_desc(const struct access_test_param *param, char *desc)
+{
+	if (param->ret < 0) {
+		snprintf(desc, KUNIT_PARAM_DESC_SIZE,
+			 "%s %s: %ld,%ld -> range: %d->%d(%d), sign: %d, inv: %d -> err: %d",
+			 control_layout_str(param->layout), param->func_name,
+			 param->lctl, param->rctl,
+			 param->mc.min, param->mc.max, param->mc.platform_max,
+			 param->mc.sign_bit, param->mc.invert,
+			 param->ret);
+	} else {
+		snprintf(desc, KUNIT_PARAM_DESC_SIZE,
+			 "%s %s: %ld,%ld -> range: %d->%d(%d), sign: %d, inv: %d -> %#x,%#x",
+			 control_layout_str(param->layout), param->func_name,
+			 param->lctl, param->rctl,
+			 param->mc.min, param->mc.max, param->mc.platform_max,
+			 param->mc.sign_bit, param->mc.invert,
+			 param->lreg, param->rreg);
+	}
+}
+
+static void soc_ops_test_access(struct kunit *test)
+{
+	struct soc_ops_test_priv *priv = test->priv;
+	const struct access_test_param *param = test->param_value;
+	struct snd_kcontrol kctl = {
+		.private_data = &priv->component,
+		.private_value = (unsigned long)&param->mc,
+	};
+	struct snd_ctl_elem_value result;
+	unsigned int val;
+	int ret;
+
+	ret = regmap_write(priv->component.regmap, 0x0, param->init);
+	KUNIT_ASSERT_FALSE(test, ret);
+	ret = regmap_write(priv->component.regmap, 0x1, param->init);
+	KUNIT_ASSERT_FALSE(test, ret);
+
+	result.value.integer.value[0] = param->lctl;
+	result.value.integer.value[1] = param->rctl;
+
+	ret = param->put(&kctl, &result);
+	KUNIT_ASSERT_EQ(test, ret, param->ret);
+	if (ret < 0)
+		return;
+
+	ret = regmap_read(priv->component.regmap, 0x0, &val);
+	KUNIT_ASSERT_FALSE(test, ret);
+	KUNIT_EXPECT_EQ(test, val, (param->init & ~param->lmask) | param->lreg);
+
+	ret = regmap_read(priv->component.regmap, 0x1, &val);
+	KUNIT_ASSERT_FALSE(test, ret);
+	KUNIT_EXPECT_EQ(test, val, (param->init & ~param->rmask) | param->rreg);
+
+	result.value.integer.value[0] = 0;
+	result.value.integer.value[1] = 0;
+
+	ret = param->get(&kctl, &result);
+	KUNIT_ASSERT_GE(test, ret, 0);
+
+	KUNIT_EXPECT_EQ(test, result.value.integer.value[0], param->lctl);
+	if (param->layout != SOC_OPS_TEST_SINGLE)
+		KUNIT_EXPECT_EQ(test, result.value.integer.value[1], param->rctl);
+	else
+		KUNIT_EXPECT_EQ(test, result.value.integer.value[1], 0);
+}
+
+KUNIT_ARRAY_PARAM(all_info_tests, all_info_test_params, info_test_desc);
+KUNIT_ARRAY_PARAM(all_access_tests, all_access_test_params, access_test_desc);
+
+static struct kunit_case soc_ops_test_cases[] = {
+	KUNIT_CASE_PARAM(soc_ops_test_info, all_info_tests_gen_params),
+	KUNIT_CASE_PARAM(soc_ops_test_access, all_access_tests_gen_params),
+	{}
+};
+
+static struct kunit_suite soc_ops_test_suite = {
+	.name = "soc-ops",
+	.init = soc_ops_test_init,
+	.exit = soc_ops_test_exit,
+	.test_cases = soc_ops_test_cases,
+};
+
+kunit_test_suites(&soc_ops_test_suite);
+
+MODULE_DESCRIPTION("ASoC soc-ops kunit test");
+MODULE_LICENSE("GPL");
-- 
2.39.5


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH 02/15] ASoC: ops: Minor formatting fixups
  2025-03-18 17:14 [PATCH 00/15] Tidy up ASoC control get and put handlers Charles Keepax
  2025-03-18 17:14 ` [PATCH 01/15] ASoC: ops-test: Add some basic kunit tests for soc-ops Charles Keepax
@ 2025-03-18 17:14 ` Charles Keepax
  2025-03-18 17:14 ` [PATCH 03/15] ASoC: ops: Update comments for xr_sx control helpers Charles Keepax
                   ` (13 subsequent siblings)
  15 siblings, 0 replies; 20+ messages in thread
From: Charles Keepax @ 2025-03-18 17:14 UTC (permalink / raw)
  To: broonie
  Cc: lgirdwood, shenghao-ding, kevin-lu, baojun.xu, linux-sound,
	linux-kernel, patches

No functional changes just tidying up some tabbing etc.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 sound/soc/soc-ops.c | 106 ++++++++++++++++++++++----------------------
 1 file changed, 54 insertions(+), 52 deletions(-)

diff --git a/sound/soc/soc-ops.c b/sound/soc/soc-ops.c
index cd5f927bcd4eb..9039bf3b6fb48 100644
--- a/sound/soc/soc-ops.c
+++ b/sound/soc/soc-ops.c
@@ -37,7 +37,7 @@
  * Returns 0 for success.
  */
 int snd_soc_info_enum_double(struct snd_kcontrol *kcontrol,
-	struct snd_ctl_elem_info *uinfo)
+			     struct snd_ctl_elem_info *uinfo)
 {
 	struct soc_enum *e = (struct soc_enum *)kcontrol->private_value;
 
@@ -56,7 +56,7 @@ EXPORT_SYMBOL_GPL(snd_soc_info_enum_double);
  * Returns 0 for success.
  */
 int snd_soc_get_enum_double(struct snd_kcontrol *kcontrol,
-	struct snd_ctl_elem_value *ucontrol)
+			    struct snd_ctl_elem_value *ucontrol)
 {
 	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
 	struct soc_enum *e = (struct soc_enum *)kcontrol->private_value;
@@ -87,7 +87,7 @@ EXPORT_SYMBOL_GPL(snd_soc_get_enum_double);
  * Returns 0 for success.
  */
 int snd_soc_put_enum_double(struct snd_kcontrol *kcontrol,
-	struct snd_ctl_elem_value *ucontrol)
+			    struct snd_ctl_elem_value *ucontrol)
 {
 	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
 	struct soc_enum *e = (struct soc_enum *)kcontrol->private_value;
@@ -124,8 +124,9 @@ EXPORT_SYMBOL_GPL(snd_soc_put_enum_double);
  * the given registervalue into a signed integer if sign_bit is non-zero.
  */
 static void snd_soc_read_signed(struct snd_soc_component *component,
-	unsigned int reg, unsigned int mask, unsigned int shift,
-	unsigned int sign_bit, int *signed_val)
+				unsigned int reg, unsigned int mask,
+				unsigned int shift, unsigned int sign_bit,
+				int *signed_val)
 {
 	int ret;
 	unsigned int val;
@@ -168,7 +169,7 @@ static void snd_soc_read_signed(struct snd_soc_component *component,
  * Returns 0 for success.
  */
 int snd_soc_info_volsw(struct snd_kcontrol *kcontrol,
-	struct snd_ctl_elem_info *uinfo)
+		       struct snd_ctl_elem_info *uinfo)
 {
 	struct soc_mixer_control *mc =
 		(struct soc_mixer_control *)kcontrol->private_value;
@@ -247,7 +248,7 @@ EXPORT_SYMBOL_GPL(snd_soc_info_volsw_sx);
  * Returns 0 for success.
  */
 int snd_soc_get_volsw(struct snd_kcontrol *kcontrol,
-	struct snd_ctl_elem_value *ucontrol)
+		      struct snd_ctl_elem_value *ucontrol)
 {
 	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
 	struct soc_mixer_control *mc =
@@ -300,7 +301,7 @@ EXPORT_SYMBOL_GPL(snd_soc_get_volsw);
  * Returns 0 for success.
  */
 int snd_soc_put_volsw(struct snd_kcontrol *kcontrol,
-	struct snd_ctl_elem_value *ucontrol)
+		      struct snd_ctl_elem_value *ucontrol)
 {
 	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
 	struct soc_mixer_control *mc =
@@ -362,9 +363,8 @@ int snd_soc_put_volsw(struct snd_kcontrol *kcontrol,
 		err = snd_soc_component_update_bits(component, reg2, val_mask,
 						    val2);
 		/* Don't discard any error code or drop change flag */
-		if (ret == 0 || err < 0) {
+		if (ret == 0 || err < 0)
 			ret = err;
-		}
 	}
 
 	return ret;
@@ -382,11 +382,11 @@ EXPORT_SYMBOL_GPL(snd_soc_put_volsw);
  * Returns 0 for success.
  */
 int snd_soc_get_volsw_sx(struct snd_kcontrol *kcontrol,
-		      struct snd_ctl_elem_value *ucontrol)
+			 struct snd_ctl_elem_value *ucontrol)
 {
 	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
 	struct soc_mixer_control *mc =
-	    (struct soc_mixer_control *)kcontrol->private_value;
+		(struct soc_mixer_control *)kcontrol->private_value;
 	unsigned int reg = mc->reg;
 	unsigned int reg2 = mc->rreg;
 	unsigned int shift = mc->shift;
@@ -423,18 +423,17 @@ int snd_soc_put_volsw_sx(struct snd_kcontrol *kcontrol,
 {
 	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
 	struct soc_mixer_control *mc =
-	    (struct soc_mixer_control *)kcontrol->private_value;
-
+		(struct soc_mixer_control *)kcontrol->private_value;
 	unsigned int reg = mc->reg;
 	unsigned int reg2 = mc->rreg;
 	unsigned int shift = mc->shift;
 	unsigned int rshift = mc->rshift;
+	unsigned int val, val_mask;
 	int max = mc->max;
 	int min = mc->min;
 	unsigned int mask = (1U << (fls(min + max) - 1)) - 1;
 	int err = 0;
 	int ret;
-	unsigned int val, val_mask;
 
 	if (ucontrol->value.integer.value[0] < 0)
 		return -EINVAL;
@@ -465,13 +464,13 @@ int snd_soc_put_volsw_sx(struct snd_kcontrol *kcontrol,
 		val2 = val2 << rshift;
 
 		err = snd_soc_component_update_bits(component, reg2, val_mask,
-			val2);
+						    val2);
 
 		/* Don't discard any error code or drop change flag */
-		if (ret == 0 || err < 0) {
+		if (ret == 0 || err < 0)
 			ret = err;
-		}
 	}
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(snd_soc_put_volsw_sx);
@@ -487,7 +486,7 @@ EXPORT_SYMBOL_GPL(snd_soc_put_volsw_sx);
  * returns 0 for success.
  */
 int snd_soc_info_volsw_range(struct snd_kcontrol *kcontrol,
-	struct snd_ctl_elem_info *uinfo)
+			     struct snd_ctl_elem_info *uinfo)
 {
 	struct soc_mixer_control *mc =
 		(struct soc_mixer_control *)kcontrol->private_value;
@@ -516,7 +515,7 @@ EXPORT_SYMBOL_GPL(snd_soc_info_volsw_range);
  * Returns 0 for success.
  */
 int snd_soc_put_volsw_range(struct snd_kcontrol *kcontrol,
-	struct snd_ctl_elem_value *ucontrol)
+			    struct snd_ctl_elem_value *ucontrol)
 {
 	struct soc_mixer_control *mc =
 		(struct soc_mixer_control *)kcontrol->private_value;
@@ -568,11 +567,10 @@ int snd_soc_put_volsw_range(struct snd_kcontrol *kcontrol,
 		val = val << shift;
 
 		err = snd_soc_component_update_bits(component, rreg, val_mask,
-			val);
+						    val);
 		/* Don't discard any error code or drop change flag */
-		if (ret == 0 || err < 0) {
+		if (ret == 0 || err < 0)
 			ret = err;
-		}
 	}
 
 	return ret;
@@ -589,7 +587,7 @@ EXPORT_SYMBOL_GPL(snd_soc_put_volsw_range);
  * Returns 0 for success.
  */
 int snd_soc_get_volsw_range(struct snd_kcontrol *kcontrol,
-	struct snd_ctl_elem_value *ucontrol)
+			    struct snd_ctl_elem_value *ucontrol)
 {
 	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
 	struct soc_mixer_control *mc =
@@ -663,8 +661,7 @@ static int snd_soc_clip_to_platform_max(struct snd_kcontrol *kctl)
  *
  * Return 0 for success, else error.
  */
-int snd_soc_limit_volume(struct snd_soc_card *card,
-	const char *name, int max)
+int snd_soc_limit_volume(struct snd_soc_card *card, const char *name, int max)
 {
 	struct snd_kcontrol *kctl;
 	int ret = -EINVAL;
@@ -675,12 +672,15 @@ int snd_soc_limit_volume(struct snd_soc_card *card,
 
 	kctl = snd_soc_card_get_kcontrol(card, name);
 	if (kctl) {
-		struct soc_mixer_control *mc = (struct soc_mixer_control *)kctl->private_value;
+		struct soc_mixer_control *mc =
+			(struct soc_mixer_control *)kctl->private_value;
+
 		if (max <= mc->max - mc->min) {
 			mc->platform_max = max;
 			ret = snd_soc_clip_to_platform_max(kctl);
 		}
 	}
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(snd_soc_limit_volume);
@@ -740,8 +740,8 @@ int snd_soc_bytes_put(struct snd_kcontrol *kcontrol,
 {
 	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
 	struct soc_bytes *params = (void *)kcontrol->private_value;
-	int ret, len;
 	unsigned int val, mask;
+	int ret, len;
 
 	if (!component->regmap || !params->num_regs)
 		return -EINVAL;
@@ -772,15 +772,13 @@ int snd_soc_bytes_put(struct snd_kcontrol *kcontrol,
 			break;
 		case 2:
 			mask = ~params->mask;
-			ret = regmap_parse_val(component->regmap,
-							&mask, &mask);
+			ret = regmap_parse_val(component->regmap, &mask, &mask);
 			if (ret != 0)
 				return ret;
 
 			((u16 *)data)[0] &= mask;
 
-			ret = regmap_parse_val(component->regmap,
-							&val, &val);
+			ret = regmap_parse_val(component->regmap, &val, &val);
 			if (ret != 0)
 				return ret;
 
@@ -788,15 +786,13 @@ int snd_soc_bytes_put(struct snd_kcontrol *kcontrol,
 			break;
 		case 4:
 			mask = ~params->mask;
-			ret = regmap_parse_val(component->regmap,
-							&mask, &mask);
+			ret = regmap_parse_val(component->regmap, &mask, &mask);
 			if (ret != 0)
 				return ret;
 
 			((u32 *)data)[0] &= mask;
 
-			ret = regmap_parse_val(component->regmap,
-							&val, &val);
+			ret = regmap_parse_val(component->regmap, &val, &val);
 			if (ret != 0)
 				return ret;
 
@@ -812,7 +808,7 @@ int snd_soc_bytes_put(struct snd_kcontrol *kcontrol,
 EXPORT_SYMBOL_GPL(snd_soc_bytes_put);
 
 int snd_soc_bytes_info_ext(struct snd_kcontrol *kcontrol,
-			struct snd_ctl_elem_info *ucontrol)
+			   struct snd_ctl_elem_info *ucontrol)
 {
 	struct soc_bytes_ext *params = (void *)kcontrol->private_value;
 
@@ -824,7 +820,7 @@ int snd_soc_bytes_info_ext(struct snd_kcontrol *kcontrol,
 EXPORT_SYMBOL_GPL(snd_soc_bytes_info_ext);
 
 int snd_soc_bytes_tlv_callback(struct snd_kcontrol *kcontrol, int op_flag,
-				unsigned int size, unsigned int __user *tlv)
+			       unsigned int size, unsigned int __user *tlv)
 {
 	struct soc_bytes_ext *params = (void *)kcontrol->private_value;
 	unsigned int count = size < params->max ? size : params->max;
@@ -840,6 +836,7 @@ int snd_soc_bytes_tlv_callback(struct snd_kcontrol *kcontrol, int op_flag,
 			ret = params->put(kcontrol, tlv, count);
 		break;
 	}
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(snd_soc_bytes_tlv_callback);
@@ -856,10 +853,11 @@ EXPORT_SYMBOL_GPL(snd_soc_bytes_tlv_callback);
  * Returns 0 for success.
  */
 int snd_soc_info_xr_sx(struct snd_kcontrol *kcontrol,
-	struct snd_ctl_elem_info *uinfo)
+		       struct snd_ctl_elem_info *uinfo)
 {
 	struct soc_mreg_control *mc =
 		(struct soc_mreg_control *)kcontrol->private_value;
+
 	uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER;
 	uinfo->count = 1;
 	uinfo->value.integer.min = mc->min;
@@ -883,7 +881,7 @@ EXPORT_SYMBOL_GPL(snd_soc_info_xr_sx);
  * Returns 0 for success.
  */
 int snd_soc_get_xr_sx(struct snd_kcontrol *kcontrol,
-	struct snd_ctl_elem_value *ucontrol)
+		      struct snd_ctl_elem_value *ucontrol)
 {
 	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
 	struct soc_mreg_control *mc =
@@ -891,17 +889,18 @@ int snd_soc_get_xr_sx(struct snd_kcontrol *kcontrol,
 	unsigned int regbase = mc->regbase;
 	unsigned int regcount = mc->regcount;
 	unsigned int regwshift = component->val_bytes * BITS_PER_BYTE;
-	unsigned int regwmask = (1UL<<regwshift)-1;
+	unsigned int regwmask = (1UL << regwshift) - 1;
 	unsigned int invert = mc->invert;
-	unsigned long mask = (1UL<<mc->nbits)-1;
+	unsigned long mask = (1UL << mc->nbits) - 1;
 	long min = mc->min;
 	long max = mc->max;
 	long val = 0;
 	unsigned int i;
 
 	for (i = 0; i < regcount; i++) {
-		unsigned int regval = snd_soc_component_read(component, regbase+i);
-		val |= (regval & regwmask) << (regwshift*(regcount-i-1));
+		unsigned int regval = snd_soc_component_read(component, regbase + i);
+
+		val |= (regval & regwmask) << (regwshift * (regcount - i - 1));
 	}
 	val &= mask;
 	if (min < 0 && val > max)
@@ -928,7 +927,7 @@ EXPORT_SYMBOL_GPL(snd_soc_get_xr_sx);
  * Returns 0 for success.
  */
 int snd_soc_put_xr_sx(struct snd_kcontrol *kcontrol,
-	struct snd_ctl_elem_value *ucontrol)
+		      struct snd_ctl_elem_value *ucontrol)
 {
 	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
 	struct soc_mreg_control *mc =
@@ -936,9 +935,9 @@ int snd_soc_put_xr_sx(struct snd_kcontrol *kcontrol,
 	unsigned int regbase = mc->regbase;
 	unsigned int regcount = mc->regcount;
 	unsigned int regwshift = component->val_bytes * BITS_PER_BYTE;
-	unsigned int regwmask = (1UL<<regwshift)-1;
+	unsigned int regwmask = (1UL << regwshift) - 1;
 	unsigned int invert = mc->invert;
-	unsigned long mask = (1UL<<mc->nbits)-1;
+	unsigned long mask = (1UL << mc->nbits) - 1;
 	long max = mc->max;
 	long val = ucontrol->value.integer.value[0];
 	int ret = 0;
@@ -950,10 +949,13 @@ int snd_soc_put_xr_sx(struct snd_kcontrol *kcontrol,
 		val = max - val;
 	val &= mask;
 	for (i = 0; i < regcount; i++) {
-		unsigned int regval = (val >> (regwshift*(regcount-i-1))) & regwmask;
-		unsigned int regmask = (mask >> (regwshift*(regcount-i-1))) & regwmask;
-		int err = snd_soc_component_update_bits(component, regbase+i,
+		unsigned int regval = (val >> (regwshift * (regcount - i - 1))) &
+				      regwmask;
+		unsigned int regmask = (mask >> (regwshift * (regcount - i - 1))) &
+				       regwmask;
+		int err = snd_soc_component_update_bits(component, regbase + i,
 							regmask, regval);
+
 		if (err < 0)
 			return err;
 		if (err > 0)
@@ -974,7 +976,7 @@ EXPORT_SYMBOL_GPL(snd_soc_put_xr_sx);
  * Returns 0 for success.
  */
 int snd_soc_get_strobe(struct snd_kcontrol *kcontrol,
-	struct snd_ctl_elem_value *ucontrol)
+		       struct snd_ctl_elem_value *ucontrol)
 {
 	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
 	struct soc_mixer_control *mc =
@@ -1007,7 +1009,7 @@ EXPORT_SYMBOL_GPL(snd_soc_get_strobe);
  * Returns 1 for success.
  */
 int snd_soc_put_strobe(struct snd_kcontrol *kcontrol,
-	struct snd_ctl_elem_value *ucontrol)
+		       struct snd_ctl_elem_value *ucontrol)
 {
 	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
 	struct soc_mixer_control *mc =
-- 
2.39.5


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH 03/15] ASoC: ops: Update comments for xr_sx control helpers
  2025-03-18 17:14 [PATCH 00/15] Tidy up ASoC control get and put handlers Charles Keepax
  2025-03-18 17:14 ` [PATCH 01/15] ASoC: ops-test: Add some basic kunit tests for soc-ops Charles Keepax
  2025-03-18 17:14 ` [PATCH 02/15] ASoC: ops: Minor formatting fixups Charles Keepax
@ 2025-03-18 17:14 ` Charles Keepax
  2025-03-18 17:14 ` [PATCH 04/15] ASoC: ops: Update mask generation to use GENMASK Charles Keepax
                   ` (12 subsequent siblings)
  15 siblings, 0 replies; 20+ messages in thread
From: Charles Keepax @ 2025-03-18 17:14 UTC (permalink / raw)
  To: broonie
  Cc: lgirdwood, shenghao-ding, kevin-lu, baojun.xu, linux-sound,
	linux-kernel, patches

Update the comments for the xr_sx control helper functions to better
clarify the difference between these and the normal sx helpers.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 sound/soc/soc-ops.c | 29 ++++++++++++++++-------------
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/sound/soc/soc-ops.c b/sound/soc/soc-ops.c
index 9039bf3b6fb48..dac55138210d5 100644
--- a/sound/soc/soc-ops.c
+++ b/sound/soc/soc-ops.c
@@ -846,9 +846,10 @@ EXPORT_SYMBOL_GPL(snd_soc_bytes_tlv_callback);
  * @kcontrol: mreg control
  * @uinfo: control element information
  *
- * Callback to provide information of a control that can
- * span multiple codec registers which together
- * forms a single signed value in a MSB/LSB manner.
+ * Callback to provide information of a control that can span multiple
+ * codec registers which together forms a single signed value. Note
+ * that unlike the non-xr variant of sx controls these may or may not
+ * include the sign bit, depending on nbits, and there is no shift.
  *
  * Returns 0 for success.
  */
@@ -872,11 +873,12 @@ EXPORT_SYMBOL_GPL(snd_soc_info_xr_sx);
  * @kcontrol: mreg control
  * @ucontrol: control element information
  *
- * Callback to get the value of a control that can span
- * multiple codec registers which together forms a single
- * signed value in a MSB/LSB manner. The control supports
- * specifying total no of bits used to allow for bitfields
- * across the multiple codec registers.
+ * Callback to get the value of a control that can span multiple codec
+ * registers which together forms a single signed value. The control
+ * supports specifying total no of bits used to allow for bitfields
+ * across the multiple codec registers. Note that unlike the non-xr
+ * variant of sx controls these may or may not include the sign bit,
+ * depending on nbits, and there is no shift.
  *
  * Returns 0 for success.
  */
@@ -918,11 +920,12 @@ EXPORT_SYMBOL_GPL(snd_soc_get_xr_sx);
  * @kcontrol: mreg control
  * @ucontrol: control element information
  *
- * Callback to set the value of a control that can span
- * multiple codec registers which together forms a single
- * signed value in a MSB/LSB manner. The control supports
- * specifying total no of bits used to allow for bitfields
- * across the multiple codec registers.
+ * Callback to set the value of a control that can span multiple codec
+ * registers which together forms a single signed value. The control
+ * supports specifying total no of bits used to allow for bitfields
+ * across the multiple codec registers. Note that unlike the non-xr
+ * variant of sx controls these may or may not include the sign bit,
+ * depending on nbits, and there is no shift.
  *
  * Returns 0 for success.
  */
-- 
2.39.5


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH 04/15] ASoC: ops: Update mask generation to use GENMASK
  2025-03-18 17:14 [PATCH 00/15] Tidy up ASoC control get and put handlers Charles Keepax
                   ` (2 preceding siblings ...)
  2025-03-18 17:14 ` [PATCH 03/15] ASoC: ops: Update comments for xr_sx control helpers Charles Keepax
@ 2025-03-18 17:14 ` Charles Keepax
  2025-03-18 17:14 ` [PATCH 05/15] ASoC: ops: Factor out helper to check valid control values Charles Keepax
                   ` (11 subsequent siblings)
  15 siblings, 0 replies; 20+ messages in thread
From: Charles Keepax @ 2025-03-18 17:14 UTC (permalink / raw)
  To: broonie
  Cc: lgirdwood, shenghao-ding, kevin-lu, baojun.xu, linux-sound,
	linux-kernel, patches

Use GENMASK to make the masks for the various control helper functions.
Also factor out a shared helper function for the volsw and volsw_range
controls since the same code is appropriate for each. Note this does add
support for sign_bit into the volsw_range callbacks.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 sound/soc/soc-ops.c | 46 +++++++++++++++++++++++++--------------------
 1 file changed, 26 insertions(+), 20 deletions(-)

diff --git a/sound/soc/soc-ops.c b/sound/soc/soc-ops.c
index dac55138210d5..54945017e1f1e 100644
--- a/sound/soc/soc-ops.c
+++ b/sound/soc/soc-ops.c
@@ -158,6 +158,20 @@ static void snd_soc_read_signed(struct snd_soc_component *component,
 	*signed_val = ret;
 }
 
+static int soc_mixer_mask(struct soc_mixer_control *mc)
+{
+	if (mc->sign_bit)
+		return GENMASK(mc->sign_bit, 0);
+	else
+		return GENMASK(fls(mc->max) - 1, 0);
+}
+
+static int soc_mixer_sx_mask(struct soc_mixer_control *mc)
+{
+	// min + max will take us 1-bit over the size of the mask
+	return GENMASK(fls(mc->min + mc->max) - 2, 0);
+}
+
 /**
  * snd_soc_info_volsw - single mixer info callback
  * @kcontrol: mixer control
@@ -260,13 +274,10 @@ int snd_soc_get_volsw(struct snd_kcontrol *kcontrol,
 	int max = mc->max;
 	int min = mc->min;
 	int sign_bit = mc->sign_bit;
-	unsigned int mask = (1ULL << fls(max)) - 1;
+	unsigned int mask = soc_mixer_mask(mc);
 	unsigned int invert = mc->invert;
 	int val;
 
-	if (sign_bit)
-		mask = BIT(sign_bit + 1) - 1;
-
 	snd_soc_read_signed(component, reg, mask, shift, sign_bit, &val);
 
 	ucontrol->value.integer.value[0] = val - min;
@@ -312,17 +323,13 @@ int snd_soc_put_volsw(struct snd_kcontrol *kcontrol,
 	unsigned int rshift = mc->rshift;
 	int max = mc->max;
 	int min = mc->min;
-	unsigned int sign_bit = mc->sign_bit;
-	unsigned int mask = (1 << fls(max)) - 1;
+	unsigned int mask = soc_mixer_mask(mc);
 	unsigned int invert = mc->invert;
 	int err, ret;
 	bool type_2r = false;
 	unsigned int val2 = 0;
 	unsigned int val, val_mask;
 
-	if (sign_bit)
-		mask = BIT(sign_bit + 1) - 1;
-
 	if (ucontrol->value.integer.value[0] < 0)
 		return -EINVAL;
 	val = ucontrol->value.integer.value[0];
@@ -391,9 +398,8 @@ int snd_soc_get_volsw_sx(struct snd_kcontrol *kcontrol,
 	unsigned int reg2 = mc->rreg;
 	unsigned int shift = mc->shift;
 	unsigned int rshift = mc->rshift;
-	int max = mc->max;
 	int min = mc->min;
-	unsigned int mask = (1U << (fls(min + max) - 1)) - 1;
+	unsigned int mask = soc_mixer_sx_mask(mc);
 	unsigned int val;
 
 	val = snd_soc_component_read(component, reg);
@@ -431,7 +437,7 @@ int snd_soc_put_volsw_sx(struct snd_kcontrol *kcontrol,
 	unsigned int val, val_mask;
 	int max = mc->max;
 	int min = mc->min;
-	unsigned int mask = (1U << (fls(min + max) - 1)) - 1;
+	unsigned int mask = soc_mixer_sx_mask(mc);
 	int err = 0;
 	int ret;
 
@@ -525,7 +531,7 @@ int snd_soc_put_volsw_range(struct snd_kcontrol *kcontrol,
 	unsigned int shift = mc->shift;
 	int min = mc->min;
 	int max = mc->max;
-	unsigned int mask = (1 << fls(max)) - 1;
+	unsigned int mask = soc_mixer_mask(mc);
 	unsigned int invert = mc->invert;
 	unsigned int val, val_mask;
 	int err, ret, tmp;
@@ -597,7 +603,7 @@ int snd_soc_get_volsw_range(struct snd_kcontrol *kcontrol,
 	unsigned int shift = mc->shift;
 	int min = mc->min;
 	int max = mc->max;
-	unsigned int mask = (1 << fls(max)) - 1;
+	unsigned int mask = soc_mixer_mask(mc);
 	unsigned int invert = mc->invert;
 	unsigned int val;
 
@@ -891,9 +897,9 @@ int snd_soc_get_xr_sx(struct snd_kcontrol *kcontrol,
 	unsigned int regbase = mc->regbase;
 	unsigned int regcount = mc->regcount;
 	unsigned int regwshift = component->val_bytes * BITS_PER_BYTE;
-	unsigned int regwmask = (1UL << regwshift) - 1;
+	unsigned int regwmask = GENMASK(regwshift - 1, 0);
+	unsigned long mask = GENMASK(mc->nbits - 1, 0);
 	unsigned int invert = mc->invert;
-	unsigned long mask = (1UL << mc->nbits) - 1;
 	long min = mc->min;
 	long max = mc->max;
 	long val = 0;
@@ -938,9 +944,9 @@ int snd_soc_put_xr_sx(struct snd_kcontrol *kcontrol,
 	unsigned int regbase = mc->regbase;
 	unsigned int regcount = mc->regcount;
 	unsigned int regwshift = component->val_bytes * BITS_PER_BYTE;
-	unsigned int regwmask = (1UL << regwshift) - 1;
+	unsigned int regwmask = GENMASK(regwshift - 1, 0);
+	unsigned long mask = GENMASK(mc->nbits - 1, 0);
 	unsigned int invert = mc->invert;
-	unsigned long mask = (1UL << mc->nbits) - 1;
 	long max = mc->max;
 	long val = ucontrol->value.integer.value[0];
 	int ret = 0;
@@ -986,7 +992,7 @@ int snd_soc_get_strobe(struct snd_kcontrol *kcontrol,
 		(struct soc_mixer_control *)kcontrol->private_value;
 	unsigned int reg = mc->reg;
 	unsigned int shift = mc->shift;
-	unsigned int mask = 1 << shift;
+	unsigned int mask = BIT(shift);
 	unsigned int invert = mc->invert != 0;
 	unsigned int val;
 
@@ -1019,7 +1025,7 @@ int snd_soc_put_strobe(struct snd_kcontrol *kcontrol,
 		(struct soc_mixer_control *)kcontrol->private_value;
 	unsigned int reg = mc->reg;
 	unsigned int shift = mc->shift;
-	unsigned int mask = 1 << shift;
+	unsigned int mask = BIT(shift);
 	unsigned int invert = mc->invert != 0;
 	unsigned int strobe = ucontrol->value.enumerated.item[0] != 0;
 	unsigned int val1 = (strobe ^ invert) ? mask : 0;
-- 
2.39.5


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH 05/15] ASoC: ops: Factor out helper to check valid control values
  2025-03-18 17:14 [PATCH 00/15] Tidy up ASoC control get and put handlers Charles Keepax
                   ` (3 preceding siblings ...)
  2025-03-18 17:14 ` [PATCH 04/15] ASoC: ops: Update mask generation to use GENMASK Charles Keepax
@ 2025-03-18 17:14 ` Charles Keepax
  2025-03-18 17:14 ` [PATCH 06/15] ASoC: ops: Replace snd_soc_read_signed() with new helper Charles Keepax
                   ` (10 subsequent siblings)
  15 siblings, 0 replies; 20+ messages in thread
From: Charles Keepax @ 2025-03-18 17:14 UTC (permalink / raw)
  To: broonie
  Cc: lgirdwood, shenghao-ding, kevin-lu, baojun.xu, linux-sound,
	linux-kernel, patches

Most of the put handlers have identical code to verify the value passed
in from user-space. Factor this out into a single helper function.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 sound/soc/soc-ops.c | 82 ++++++++++++++++++++++++---------------------
 1 file changed, 43 insertions(+), 39 deletions(-)

diff --git a/sound/soc/soc-ops.c b/sound/soc/soc-ops.c
index 54945017e1f1e..53a141426a967 100644
--- a/sound/soc/soc-ops.c
+++ b/sound/soc/soc-ops.c
@@ -158,6 +158,20 @@ static void snd_soc_read_signed(struct snd_soc_component *component,
 	*signed_val = ret;
 }
 
+static int soc_mixer_valid_ctl(struct soc_mixer_control *mc, long val, int max)
+{
+	if (val < 0)
+		return -EINVAL;
+
+	if (mc->platform_max && val > mc->platform_max)
+		return -EINVAL;
+
+	if (val > max)
+		return -EINVAL;
+
+	return 0;
+}
+
 static int soc_mixer_mask(struct soc_mixer_control *mc)
 {
 	if (mc->sign_bit)
@@ -330,26 +344,24 @@ int snd_soc_put_volsw(struct snd_kcontrol *kcontrol,
 	unsigned int val2 = 0;
 	unsigned int val, val_mask;
 
-	if (ucontrol->value.integer.value[0] < 0)
-		return -EINVAL;
+	ret = soc_mixer_valid_ctl(mc, ucontrol->value.integer.value[0],
+				  mc->max - mc->min);
+	if (ret)
+		return ret;
+
 	val = ucontrol->value.integer.value[0];
-	if (mc->platform_max && val > mc->platform_max)
-		return -EINVAL;
-	if (val > max - min)
-		return -EINVAL;
 	val = (val + min) & mask;
 	if (invert)
 		val = max - val;
 	val_mask = mask << shift;
 	val = val << shift;
 	if (snd_soc_volsw_is_stereo(mc)) {
-		if (ucontrol->value.integer.value[1] < 0)
-			return -EINVAL;
+		ret = soc_mixer_valid_ctl(mc, ucontrol->value.integer.value[1],
+					  mc->max - mc->min);
+		if (ret)
+			return ret;
+
 		val2 = ucontrol->value.integer.value[1];
-		if (mc->platform_max && val2 > mc->platform_max)
-			return -EINVAL;
-		if (val2 > max - min)
-			return -EINVAL;
 		val2 = (val2 + min) & mask;
 		if (invert)
 			val2 = max - val2;
@@ -435,19 +447,16 @@ int snd_soc_put_volsw_sx(struct snd_kcontrol *kcontrol,
 	unsigned int shift = mc->shift;
 	unsigned int rshift = mc->rshift;
 	unsigned int val, val_mask;
-	int max = mc->max;
 	int min = mc->min;
 	unsigned int mask = soc_mixer_sx_mask(mc);
 	int err = 0;
 	int ret;
 
-	if (ucontrol->value.integer.value[0] < 0)
-		return -EINVAL;
+	ret = soc_mixer_valid_ctl(mc, ucontrol->value.integer.value[0], mc->max);
+	if (ret)
+		return ret;
+
 	val = ucontrol->value.integer.value[0];
-	if (mc->platform_max && val > mc->platform_max)
-		return -EINVAL;
-	if (val > max)
-		return -EINVAL;
 	val_mask = mask << shift;
 	val = (val + min) & mask;
 	val = val << shift;
@@ -458,13 +467,14 @@ int snd_soc_put_volsw_sx(struct snd_kcontrol *kcontrol,
 	ret = err;
 
 	if (snd_soc_volsw_is_stereo(mc)) {
-		unsigned int val2 = ucontrol->value.integer.value[1];
+		unsigned int val2;
 
-		if (mc->platform_max && val2 > mc->platform_max)
-			return -EINVAL;
-		if (val2 > max)
-			return -EINVAL;
+		ret = soc_mixer_valid_ctl(mc, ucontrol->value.integer.value[1],
+					  mc->max);
+		if (ret)
+			return ret;
 
+		val2 = ucontrol->value.integer.value[1];
 		val_mask = mask << rshift;
 		val2 = (val2 + min) & mask;
 		val2 = val2 << rshift;
@@ -534,15 +544,12 @@ int snd_soc_put_volsw_range(struct snd_kcontrol *kcontrol,
 	unsigned int mask = soc_mixer_mask(mc);
 	unsigned int invert = mc->invert;
 	unsigned int val, val_mask;
-	int err, ret, tmp;
+	int err, ret;
 
-	tmp = ucontrol->value.integer.value[0];
-	if (tmp < 0)
-		return -EINVAL;
-	if (mc->platform_max && tmp > mc->platform_max)
-		return -EINVAL;
-	if (tmp > mc->max - mc->min)
-		return -EINVAL;
+	ret = soc_mixer_valid_ctl(mc, ucontrol->value.integer.value[0],
+				  mc->max - mc->min);
+	if (ret)
+		return ret;
 
 	if (invert)
 		val = (max - ucontrol->value.integer.value[0]) & mask;
@@ -557,13 +564,10 @@ int snd_soc_put_volsw_range(struct snd_kcontrol *kcontrol,
 	ret = err;
 
 	if (snd_soc_volsw_is_stereo(mc)) {
-		tmp = ucontrol->value.integer.value[1];
-		if (tmp < 0)
-			return -EINVAL;
-		if (mc->platform_max && tmp > mc->platform_max)
-			return -EINVAL;
-		if (tmp > mc->max - mc->min)
-			return -EINVAL;
+		ret = soc_mixer_valid_ctl(mc, ucontrol->value.integer.value[1],
+					  mc->max - mc->min);
+		if (ret)
+			return ret;
 
 		if (invert)
 			val = (max - ucontrol->value.integer.value[1]) & mask;
-- 
2.39.5


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH 06/15] ASoC: ops: Replace snd_soc_read_signed() with new helper
  2025-03-18 17:14 [PATCH 00/15] Tidy up ASoC control get and put handlers Charles Keepax
                   ` (4 preceding siblings ...)
  2025-03-18 17:14 ` [PATCH 05/15] ASoC: ops: Factor out helper to check valid control values Charles Keepax
@ 2025-03-18 17:14 ` Charles Keepax
  2025-03-18 17:14 ` [PATCH 07/15] ASoC: ops: Add control to register value helper Charles Keepax
                   ` (9 subsequent siblings)
  15 siblings, 0 replies; 20+ messages in thread
From: Charles Keepax @ 2025-03-18 17:14 UTC (permalink / raw)
  To: broonie
  Cc: lgirdwood, shenghao-ding, kevin-lu, baojun.xu, linux-sound,
	linux-kernel, patches

The current snd_soc_read_signed() helper is only used from
snd_soc_get_volsw() and can be implemented more simply with
sign_extend. Remove snd_soc_read_signed() and add a new
soc_mixer_reg_to_ctl() helper. This new helper does not
include the reading of the register, but does include min and
max handling. This allows the helper to replace more of the
duplicated code and makes it easier to process the differences
between single, double register and double shift style controls.

It is worth noting this adds support for sign_bit into the _range
and sx callbacks and the invert option to sx callbacks.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 sound/soc/soc-ops.c | 134 ++++++++++++++------------------------------
 1 file changed, 41 insertions(+), 93 deletions(-)

diff --git a/sound/soc/soc-ops.c b/sound/soc/soc-ops.c
index 53a141426a967..af4e678173172 100644
--- a/sound/soc/soc-ops.c
+++ b/sound/soc/soc-ops.c
@@ -110,52 +110,20 @@ int snd_soc_put_enum_double(struct snd_kcontrol *kcontrol,
 }
 EXPORT_SYMBOL_GPL(snd_soc_put_enum_double);
 
-/**
- * snd_soc_read_signed - Read a codec register and interpret as signed value
- * @component: component
- * @reg: Register to read
- * @mask: Mask to use after shifting the register value
- * @shift: Right shift of register value
- * @sign_bit: Bit that describes if a number is negative or not.
- * @signed_val: Pointer to where the read value should be stored
- *
- * This functions reads a codec register. The register value is shifted right
- * by 'shift' bits and masked with the given 'mask'. Afterwards it translates
- * the given registervalue into a signed integer if sign_bit is non-zero.
- */
-static void snd_soc_read_signed(struct snd_soc_component *component,
-				unsigned int reg, unsigned int mask,
-				unsigned int shift, unsigned int sign_bit,
-				int *signed_val)
+static int soc_mixer_reg_to_ctl(struct soc_mixer_control *mc, unsigned int reg_val,
+				unsigned int mask, unsigned int shift, int max)
 {
-	int ret;
-	unsigned int val;
-
-	val = snd_soc_component_read(component, reg);
-	val = (val >> shift) & mask;
+	int val = (reg_val >> shift) & mask;
 
-	if (!sign_bit) {
-		*signed_val = val;
-		return;
-	}
-
-	/* non-negative number */
-	if (!(val & BIT(sign_bit))) {
-		*signed_val = val;
-		return;
-	}
+	if (mc->sign_bit)
+		val = sign_extend32(val, mc->sign_bit);
 
-	ret = val;
+	val -= mc->min;
 
-	/*
-	 * The register most probably does not contain a full-sized int.
-	 * Instead we have an arbitrary number of bits in a signed
-	 * representation which has to be translated into a full-sized int.
-	 * This is done by filling up all bits above the sign-bit.
-	 */
-	ret |= ~((int)(BIT(sign_bit) - 1));
+	if (mc->invert)
+		val = max - val;
 
-	*signed_val = ret;
+	return val & mask;
 }
 
 static int soc_mixer_valid_ctl(struct soc_mixer_control *mc, long val, int max)
@@ -281,34 +249,25 @@ int snd_soc_get_volsw(struct snd_kcontrol *kcontrol,
 	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
 	struct soc_mixer_control *mc =
 		(struct soc_mixer_control *)kcontrol->private_value;
-	unsigned int reg = mc->reg;
-	unsigned int reg2 = mc->rreg;
-	unsigned int shift = mc->shift;
-	unsigned int rshift = mc->rshift;
-	int max = mc->max;
-	int min = mc->min;
-	int sign_bit = mc->sign_bit;
+	int max = mc->max - mc->min;
 	unsigned int mask = soc_mixer_mask(mc);
-	unsigned int invert = mc->invert;
+	unsigned int reg_val;
 	int val;
 
-	snd_soc_read_signed(component, reg, mask, shift, sign_bit, &val);
+	reg_val = snd_soc_component_read(component, mc->reg);
+	val = soc_mixer_reg_to_ctl(mc, reg_val, mask, mc->shift, max);
 
-	ucontrol->value.integer.value[0] = val - min;
-	if (invert)
-		ucontrol->value.integer.value[0] =
-			max - ucontrol->value.integer.value[0];
+	ucontrol->value.integer.value[0] = val;
 
 	if (snd_soc_volsw_is_stereo(mc)) {
-		if (reg == reg2)
-			snd_soc_read_signed(component, reg, mask, rshift, sign_bit, &val);
-		else
-			snd_soc_read_signed(component, reg2, mask, shift, sign_bit, &val);
+		if (mc->reg == mc->rreg) {
+			val = soc_mixer_reg_to_ctl(mc, reg_val, mask, mc->rshift, max);
+		} else {
+			reg_val = snd_soc_component_read(component, mc->rreg);
+			val = soc_mixer_reg_to_ctl(mc, reg_val, mask, mc->shift, max);
+		}
 
-		ucontrol->value.integer.value[1] = val - min;
-		if (invert)
-			ucontrol->value.integer.value[1] =
-				max - ucontrol->value.integer.value[1];
+		ucontrol->value.integer.value[1] = val;
 	}
 
 	return 0;
@@ -408,18 +367,19 @@ int snd_soc_get_volsw_sx(struct snd_kcontrol *kcontrol,
 		(struct soc_mixer_control *)kcontrol->private_value;
 	unsigned int reg = mc->reg;
 	unsigned int reg2 = mc->rreg;
-	unsigned int shift = mc->shift;
-	unsigned int rshift = mc->rshift;
-	int min = mc->min;
 	unsigned int mask = soc_mixer_sx_mask(mc);
-	unsigned int val;
+	unsigned int reg_val;
+	int val;
 
-	val = snd_soc_component_read(component, reg);
-	ucontrol->value.integer.value[0] = ((val >> shift) - min) & mask;
+	reg_val = snd_soc_component_read(component, reg);
+	val = soc_mixer_reg_to_ctl(mc, reg_val, mask, mc->shift, mc->max);
+
+	ucontrol->value.integer.value[0] = val;
 
 	if (snd_soc_volsw_is_stereo(mc)) {
-		val = snd_soc_component_read(component, reg2);
-		val = ((val >> rshift) - min) & mask;
+		reg_val = snd_soc_component_read(component, reg2);
+		val = soc_mixer_reg_to_ctl(mc, reg_val, mask, mc->rshift, mc->max);
+
 		ucontrol->value.integer.value[1] = val;
 	}
 
@@ -602,33 +562,21 @@ int snd_soc_get_volsw_range(struct snd_kcontrol *kcontrol,
 	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
 	struct soc_mixer_control *mc =
 		(struct soc_mixer_control *)kcontrol->private_value;
-	unsigned int reg = mc->reg;
-	unsigned int rreg = mc->rreg;
-	unsigned int shift = mc->shift;
-	int min = mc->min;
-	int max = mc->max;
+	int max = mc->max - mc->min;
 	unsigned int mask = soc_mixer_mask(mc);
-	unsigned int invert = mc->invert;
-	unsigned int val;
+	unsigned int reg_val;
+	int val;
 
-	val = snd_soc_component_read(component, reg);
-	ucontrol->value.integer.value[0] = (val >> shift) & mask;
-	if (invert)
-		ucontrol->value.integer.value[0] =
-			max - ucontrol->value.integer.value[0];
-	else
-		ucontrol->value.integer.value[0] =
-			ucontrol->value.integer.value[0] - min;
+	reg_val = snd_soc_component_read(component, mc->reg);
+	val = soc_mixer_reg_to_ctl(mc, reg_val, mask, mc->shift, max);
+
+	ucontrol->value.integer.value[0] = val;
 
 	if (snd_soc_volsw_is_stereo(mc)) {
-		val = snd_soc_component_read(component, rreg);
-		ucontrol->value.integer.value[1] = (val >> shift) & mask;
-		if (invert)
-			ucontrol->value.integer.value[1] =
-				max - ucontrol->value.integer.value[1];
-		else
-			ucontrol->value.integer.value[1] =
-				ucontrol->value.integer.value[1] - min;
+		reg_val = snd_soc_component_read(component, mc->rreg);
+		val = soc_mixer_reg_to_ctl(mc, reg_val, mask, mc->shift, max);
+
+		ucontrol->value.integer.value[1] = val;
 	}
 
 	return 0;
-- 
2.39.5


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH 07/15] ASoC: ops: Add control to register value helper
  2025-03-18 17:14 [PATCH 00/15] Tidy up ASoC control get and put handlers Charles Keepax
                   ` (5 preceding siblings ...)
  2025-03-18 17:14 ` [PATCH 06/15] ASoC: ops: Replace snd_soc_read_signed() with new helper Charles Keepax
@ 2025-03-18 17:14 ` Charles Keepax
  2025-03-18 17:14 ` [PATCH 08/15] ASoC: ops: Remove snd_soc_info_volsw_range() Charles Keepax
                   ` (8 subsequent siblings)
  15 siblings, 0 replies; 20+ messages in thread
From: Charles Keepax @ 2025-03-18 17:14 UTC (permalink / raw)
  To: broonie
  Cc: lgirdwood, shenghao-ding, kevin-lu, baojun.xu, linux-sound,
	linux-kernel, patches

Add a helper function to convert from control values to register values
that can replace a lot of the duplicated code in the various put
handlers.

This also fixes a small issue in snd_soc_put_volsw where the value is
converted to a control value before doing the invert, but the invert
is done against the register max which will result in incorrect values
for inverted controls with a non-zero minimum.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 sound/soc/soc-ops.c | 97 ++++++++++++++++++++-------------------------
 1 file changed, 43 insertions(+), 54 deletions(-)

diff --git a/sound/soc/soc-ops.c b/sound/soc/soc-ops.c
index af4e678173172..888afdd23f84e 100644
--- a/sound/soc/soc-ops.c
+++ b/sound/soc/soc-ops.c
@@ -126,6 +126,20 @@ static int soc_mixer_reg_to_ctl(struct soc_mixer_control *mc, unsigned int reg_v
 	return val & mask;
 }
 
+static unsigned int soc_mixer_ctl_to_reg(struct soc_mixer_control *mc, int val,
+					 unsigned int mask, unsigned int shift,
+					 int max)
+{
+	unsigned int reg_val;
+
+	if (mc->invert)
+		val = max - val;
+
+	reg_val = val + mc->min;
+
+	return (reg_val & mask) << shift;
+}
+
 static int soc_mixer_valid_ctl(struct soc_mixer_control *mc, long val, int max)
 {
 	if (val < 0)
@@ -292,43 +306,35 @@ int snd_soc_put_volsw(struct snd_kcontrol *kcontrol,
 		(struct soc_mixer_control *)kcontrol->private_value;
 	unsigned int reg = mc->reg;
 	unsigned int reg2 = mc->rreg;
-	unsigned int shift = mc->shift;
-	unsigned int rshift = mc->rshift;
-	int max = mc->max;
-	int min = mc->min;
+	int max = mc->max - mc->min;
 	unsigned int mask = soc_mixer_mask(mc);
-	unsigned int invert = mc->invert;
 	int err, ret;
 	bool type_2r = false;
 	unsigned int val2 = 0;
 	unsigned int val, val_mask;
 
-	ret = soc_mixer_valid_ctl(mc, ucontrol->value.integer.value[0],
-				  mc->max - mc->min);
+	ret = soc_mixer_valid_ctl(mc, ucontrol->value.integer.value[0], max);
 	if (ret)
 		return ret;
 
-	val = ucontrol->value.integer.value[0];
-	val = (val + min) & mask;
-	if (invert)
-		val = max - val;
-	val_mask = mask << shift;
-	val = val << shift;
+	val = soc_mixer_ctl_to_reg(mc, ucontrol->value.integer.value[0],
+				   mask, mc->shift, max);
+	val_mask = mask << mc->shift;
+
 	if (snd_soc_volsw_is_stereo(mc)) {
-		ret = soc_mixer_valid_ctl(mc, ucontrol->value.integer.value[1],
-					  mc->max - mc->min);
+		ret = soc_mixer_valid_ctl(mc, ucontrol->value.integer.value[1], max);
 		if (ret)
 			return ret;
 
-		val2 = ucontrol->value.integer.value[1];
-		val2 = (val2 + min) & mask;
-		if (invert)
-			val2 = max - val2;
 		if (reg == reg2) {
-			val_mask |= mask << rshift;
-			val |= val2 << rshift;
+			val |= soc_mixer_ctl_to_reg(mc,
+						    ucontrol->value.integer.value[1],
+						    mask, mc->rshift, max);
+			val_mask |= mask << mc->rshift;
 		} else {
-			val2 = val2 << shift;
+			val2 = soc_mixer_ctl_to_reg(mc,
+						    ucontrol->value.integer.value[1],
+						    mask, mc->shift, max);
 			type_2r = true;
 		}
 	}
@@ -404,10 +410,7 @@ int snd_soc_put_volsw_sx(struct snd_kcontrol *kcontrol,
 		(struct soc_mixer_control *)kcontrol->private_value;
 	unsigned int reg = mc->reg;
 	unsigned int reg2 = mc->rreg;
-	unsigned int shift = mc->shift;
-	unsigned int rshift = mc->rshift;
 	unsigned int val, val_mask;
-	int min = mc->min;
 	unsigned int mask = soc_mixer_sx_mask(mc);
 	int err = 0;
 	int ret;
@@ -416,10 +419,9 @@ int snd_soc_put_volsw_sx(struct snd_kcontrol *kcontrol,
 	if (ret)
 		return ret;
 
-	val = ucontrol->value.integer.value[0];
-	val_mask = mask << shift;
-	val = (val + min) & mask;
-	val = val << shift;
+	val = soc_mixer_ctl_to_reg(mc, ucontrol->value.integer.value[0],
+				   mask, mc->shift, mc->max);
+	val_mask = mask << mc->shift;
 
 	err = snd_soc_component_update_bits(component, reg, val_mask, val);
 	if (err < 0)
@@ -427,20 +429,17 @@ int snd_soc_put_volsw_sx(struct snd_kcontrol *kcontrol,
 	ret = err;
 
 	if (snd_soc_volsw_is_stereo(mc)) {
-		unsigned int val2;
-
 		ret = soc_mixer_valid_ctl(mc, ucontrol->value.integer.value[1],
 					  mc->max);
 		if (ret)
 			return ret;
 
-		val2 = ucontrol->value.integer.value[1];
-		val_mask = mask << rshift;
-		val2 = (val2 + min) & mask;
-		val2 = val2 << rshift;
+		val = soc_mixer_ctl_to_reg(mc, ucontrol->value.integer.value[1],
+					   mask, mc->rshift, mc->max);
+		val_mask = mask << mc->rshift;
 
 		err = snd_soc_component_update_bits(component, reg2, val_mask,
-						    val2);
+						    val);
 
 		/* Don't discard any error code or drop change flag */
 		if (ret == 0 || err < 0)
@@ -498,12 +497,10 @@ int snd_soc_put_volsw_range(struct snd_kcontrol *kcontrol,
 	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
 	unsigned int reg = mc->reg;
 	unsigned int rreg = mc->rreg;
-	unsigned int shift = mc->shift;
-	int min = mc->min;
-	int max = mc->max;
+	int max = mc->max - mc->min;
 	unsigned int mask = soc_mixer_mask(mc);
-	unsigned int invert = mc->invert;
-	unsigned int val, val_mask;
+	unsigned int val_mask = mask << mc->shift;
+	unsigned int val;
 	int err, ret;
 
 	ret = soc_mixer_valid_ctl(mc, ucontrol->value.integer.value[0],
@@ -511,12 +508,8 @@ int snd_soc_put_volsw_range(struct snd_kcontrol *kcontrol,
 	if (ret)
 		return ret;
 
-	if (invert)
-		val = (max - ucontrol->value.integer.value[0]) & mask;
-	else
-		val = ((ucontrol->value.integer.value[0] + min) & mask);
-	val_mask = mask << shift;
-	val = val << shift;
+	val = soc_mixer_ctl_to_reg(mc, ucontrol->value.integer.value[0],
+				   mask, mc->shift, max);
 
 	err = snd_soc_component_update_bits(component, reg, val_mask, val);
 	if (err < 0)
@@ -525,16 +518,12 @@ int snd_soc_put_volsw_range(struct snd_kcontrol *kcontrol,
 
 	if (snd_soc_volsw_is_stereo(mc)) {
 		ret = soc_mixer_valid_ctl(mc, ucontrol->value.integer.value[1],
-					  mc->max - mc->min);
+					  max);
 		if (ret)
 			return ret;
 
-		if (invert)
-			val = (max - ucontrol->value.integer.value[1]) & mask;
-		else
-			val = ((ucontrol->value.integer.value[1] + min) & mask);
-		val_mask = mask << shift;
-		val = val << shift;
+		val = soc_mixer_ctl_to_reg(mc, ucontrol->value.integer.value[1],
+					   mask, mc->shift, max);
 
 		err = snd_soc_component_update_bits(component, rreg, val_mask,
 						    val);
-- 
2.39.5


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH 08/15] ASoC: ops: Remove snd_soc_info_volsw_range()
  2025-03-18 17:14 [PATCH 00/15] Tidy up ASoC control get and put handlers Charles Keepax
                   ` (6 preceding siblings ...)
  2025-03-18 17:14 ` [PATCH 07/15] ASoC: ops: Add control to register value helper Charles Keepax
@ 2025-03-18 17:14 ` Charles Keepax
  2025-03-18 17:14 ` [PATCH 09/15] ASoC: ops: Remove snd_soc_get_volsw_range() Charles Keepax
                   ` (7 subsequent siblings)
  15 siblings, 0 replies; 20+ messages in thread
From: Charles Keepax @ 2025-03-18 17:14 UTC (permalink / raw)
  To: broonie
  Cc: lgirdwood, shenghao-ding, kevin-lu, baojun.xu, linux-sound,
	linux-kernel, patches

The only difference between snd_soc_info_volsw() and
snd_soc_info_volsw_range() is that the later will not force a 2
value control to be of type integer if the name ends in "Volume".

The kernel currently contains no users of snd_soc_info_volsw_range()
that would return a boolean control with this code, so the risk is
quite low and it seems appropriate that it should contain volume
control detection. So remove snd_soc_info_volsw_range() and point its
users at snd_soc_info_volsw().

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 include/sound/soc.h             | 12 +++++------
 sound/pci/hda/tas2781_hda_i2c.c |  2 +-
 sound/pci/hda/tas2781_hda_spi.c |  2 +-
 sound/soc/soc-ops.c             | 36 +++------------------------------
 sound/soc/soc-topology.c        |  2 +-
 5 files changed, 11 insertions(+), 43 deletions(-)

diff --git a/include/sound/soc.h b/include/sound/soc.h
index d73fe26de1669..b310f2c599f87 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -65,7 +65,7 @@ struct platform_device;
 	.private_value = SOC_SINGLE_VALUE(reg, shift, 0, max, invert, 0) }
 #define SOC_SINGLE_RANGE(xname, xreg, xshift, xmin, xmax, xinvert) \
 {	.iface = SNDRV_CTL_ELEM_IFACE_MIXER, .name = (xname),\
-	.info = snd_soc_info_volsw_range, .get = snd_soc_get_volsw_range, \
+	.info = snd_soc_info_volsw, .get = snd_soc_get_volsw_range, \
 	.put = snd_soc_put_volsw_range, \
 	.private_value = SOC_SINGLE_VALUE(xreg, xshift, xmin, xmax, xinvert, 0) }
 #define SOC_SINGLE_TLV(xname, reg, shift, max, invert, tlv_array) \
@@ -90,7 +90,7 @@ struct platform_device;
 	.access = SNDRV_CTL_ELEM_ACCESS_TLV_READ |\
 		 SNDRV_CTL_ELEM_ACCESS_READWRITE,\
 	.tlv.p = (tlv_array), \
-	.info = snd_soc_info_volsw_range, \
+	.info = snd_soc_info_volsw, \
 	.get = snd_soc_get_volsw_range, .put = snd_soc_put_volsw_range, \
 	.private_value = SOC_SINGLE_VALUE(xreg, xshift, xmin, xmax, xinvert, 0) }
 #define SOC_DOUBLE(xname, reg, shift_left, shift_right, max, invert) \
@@ -116,7 +116,7 @@ struct platform_device;
 #define SOC_DOUBLE_R_RANGE(xname, reg_left, reg_right, xshift, xmin, \
 			   xmax, xinvert)		\
 {	.iface = SNDRV_CTL_ELEM_IFACE_MIXER, .name = (xname),\
-	.info = snd_soc_info_volsw_range, \
+	.info = snd_soc_info_volsw, \
 	.get = snd_soc_get_volsw_range, .put = snd_soc_put_volsw_range, \
 	.private_value = SOC_DOUBLE_R_VALUE(reg_left, reg_right, \
 					    xshift, xmin, xmax, xinvert) }
@@ -164,7 +164,7 @@ struct platform_device;
 	.access = SNDRV_CTL_ELEM_ACCESS_TLV_READ |\
 		 SNDRV_CTL_ELEM_ACCESS_READWRITE,\
 	.tlv.p = (tlv_array), \
-	.info = snd_soc_info_volsw_range, \
+	.info = snd_soc_info_volsw, \
 	.get = snd_soc_get_volsw_range, .put = snd_soc_put_volsw_range, \
 	.private_value = SOC_DOUBLE_R_VALUE(reg_left, reg_right, \
 					    xshift, xmin, xmax, xinvert) }
@@ -266,7 +266,7 @@ struct platform_device;
 	.access = SNDRV_CTL_ELEM_ACCESS_TLV_READ |\
 		 SNDRV_CTL_ELEM_ACCESS_READWRITE,\
 	.tlv.p = (tlv_array), \
-	.info = snd_soc_info_volsw_range, \
+	.info = snd_soc_info_volsw, \
 	.get = xhandler_get, .put = xhandler_put, \
 	.private_value = SOC_SINGLE_VALUE(xreg, xshift, xmin, xmax, xinvert, 0) }
 #define SOC_DOUBLE_EXT_TLV(xname, xreg, shift_left, shift_right, xmax, xinvert,\
@@ -569,8 +569,6 @@ int snd_soc_get_volsw_sx(struct snd_kcontrol *kcontrol,
 	struct snd_ctl_elem_value *ucontrol);
 int snd_soc_put_volsw_sx(struct snd_kcontrol *kcontrol,
 	struct snd_ctl_elem_value *ucontrol);
-int snd_soc_info_volsw_range(struct snd_kcontrol *kcontrol,
-	struct snd_ctl_elem_info *uinfo);
 int snd_soc_put_volsw_range(struct snd_kcontrol *kcontrol,
 	struct snd_ctl_elem_value *ucontrol);
 int snd_soc_get_volsw_range(struct snd_kcontrol *kcontrol,
diff --git a/sound/pci/hda/tas2781_hda_i2c.c b/sound/pci/hda/tas2781_hda_i2c.c
index be9a90f643ebb..50c5e5f265895 100644
--- a/sound/pci/hda/tas2781_hda_i2c.c
+++ b/sound/pci/hda/tas2781_hda_i2c.c
@@ -45,7 +45,7 @@
 	.access = SNDRV_CTL_ELEM_ACCESS_TLV_READ |\
 		 SNDRV_CTL_ELEM_ACCESS_READWRITE,\
 	.tlv.p = (tlv_array), \
-	.info = snd_soc_info_volsw_range, \
+	.info = snd_soc_info_volsw, \
 	.get = xhandler_get, .put = xhandler_put, \
 	.private_value = (unsigned long)&(struct soc_mixer_control) \
 		{.reg = xreg, .rreg = xreg, .shift = xshift, \
diff --git a/sound/pci/hda/tas2781_hda_spi.c b/sound/pci/hda/tas2781_hda_spi.c
index 00676cbb2c8e4..399f2e4c3b62b 100644
--- a/sound/pci/hda/tas2781_hda_spi.c
+++ b/sound/pci/hda/tas2781_hda_spi.c
@@ -52,7 +52,7 @@
 	.access = SNDRV_CTL_ELEM_ACCESS_TLV_READ | \
 		SNDRV_CTL_ELEM_ACCESS_READWRITE, \
 	.tlv.p = (tlv_array), \
-	.info = snd_soc_info_volsw_range, \
+	.info = snd_soc_info_volsw, \
 	.get = xhandler_get, .put = xhandler_put, \
 	.private_value = (unsigned long)&(struct soc_mixer_control) { \
 		.reg = xreg, .rreg = xreg, \
diff --git a/sound/soc/soc-ops.c b/sound/soc/soc-ops.c
index 888afdd23f84e..1b52ba12df8df 100644
--- a/sound/soc/soc-ops.c
+++ b/sound/soc/soc-ops.c
@@ -169,12 +169,12 @@ static int soc_mixer_sx_mask(struct soc_mixer_control *mc)
 }
 
 /**
- * snd_soc_info_volsw - single mixer info callback
+ * snd_soc_info_volsw - single mixer info callback with range.
  * @kcontrol: mixer control
  * @uinfo: control element information
  *
- * Callback to provide information about a single mixer control, or a double
- * mixer control that spans 2 registers.
+ * Callback to provide information, with a range, about a single mixer control,
+ * or a double mixer control that spans 2 registers.
  *
  * Returns 0 for success.
  */
@@ -450,36 +450,6 @@ int snd_soc_put_volsw_sx(struct snd_kcontrol *kcontrol,
 }
 EXPORT_SYMBOL_GPL(snd_soc_put_volsw_sx);
 
-/**
- * snd_soc_info_volsw_range - single mixer info callback with range.
- * @kcontrol: mixer control
- * @uinfo: control element information
- *
- * Callback to provide information, within a range, about a single
- * mixer control.
- *
- * returns 0 for success.
- */
-int snd_soc_info_volsw_range(struct snd_kcontrol *kcontrol,
-			     struct snd_ctl_elem_info *uinfo)
-{
-	struct soc_mixer_control *mc =
-		(struct soc_mixer_control *)kcontrol->private_value;
-	int max;
-
-	max = mc->max - mc->min;
-	if (mc->platform_max && mc->platform_max < max)
-		max = mc->platform_max;
-
-	uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER;
-	uinfo->count = snd_soc_volsw_is_stereo(mc) ? 2 : 1;
-	uinfo->value.integer.min = 0;
-	uinfo->value.integer.max = max;
-
-	return 0;
-}
-EXPORT_SYMBOL_GPL(snd_soc_info_volsw_range);
-
 /**
  * snd_soc_put_volsw_range - single mixer put value callback with range.
  * @kcontrol: mixer control
diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
index 2b86cc3311f76..9cbddfbbc556b 100644
--- a/sound/soc/soc-topology.c
+++ b/sound/soc/soc-topology.c
@@ -132,7 +132,7 @@ static const struct snd_soc_tplg_kcontrol_ops io_ops[] = {
 	{SND_SOC_TPLG_CTL_BYTES, snd_soc_bytes_get,
 		snd_soc_bytes_put, snd_soc_bytes_info},
 	{SND_SOC_TPLG_CTL_RANGE, snd_soc_get_volsw_range,
-		snd_soc_put_volsw_range, snd_soc_info_volsw_range},
+		snd_soc_put_volsw_range, snd_soc_info_volsw},
 	{SND_SOC_TPLG_CTL_VOLSW_XR_SX, snd_soc_get_xr_sx,
 		snd_soc_put_xr_sx, snd_soc_info_xr_sx},
 	{SND_SOC_TPLG_CTL_STROBE, snd_soc_get_strobe,
-- 
2.39.5


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH 09/15] ASoC: ops: Remove snd_soc_get_volsw_range()
  2025-03-18 17:14 [PATCH 00/15] Tidy up ASoC control get and put handlers Charles Keepax
                   ` (7 preceding siblings ...)
  2025-03-18 17:14 ` [PATCH 08/15] ASoC: ops: Remove snd_soc_info_volsw_range() Charles Keepax
@ 2025-03-18 17:14 ` Charles Keepax
  2025-03-18 17:14 ` [PATCH 10/15] ASoC: ops: Remove snd_soc_put_volsw_range() Charles Keepax
                   ` (6 subsequent siblings)
  15 siblings, 0 replies; 20+ messages in thread
From: Charles Keepax @ 2025-03-18 17:14 UTC (permalink / raw)
  To: broonie
  Cc: lgirdwood, shenghao-ding, kevin-lu, baojun.xu, linux-sound,
	linux-kernel, patches

With the addition of the soc_mixer_reg_to_ctl() helper it is now very
clear that the only difference between snd_soc_get_volsw() and
snd_soc_get_volsw_range() is that the former supports double controls
with both values in the same register. As such we can combine both
functions.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 include/sound/soc.h       | 10 ++++------
 sound/soc/codecs/wm5110.c |  2 +-
 sound/soc/soc-ops.c       | 42 +++------------------------------------
 sound/soc/soc-topology.c  |  2 +-
 4 files changed, 9 insertions(+), 47 deletions(-)

diff --git a/include/sound/soc.h b/include/sound/soc.h
index b310f2c599f87..b11c6cdb0201c 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -65,7 +65,7 @@ struct platform_device;
 	.private_value = SOC_SINGLE_VALUE(reg, shift, 0, max, invert, 0) }
 #define SOC_SINGLE_RANGE(xname, xreg, xshift, xmin, xmax, xinvert) \
 {	.iface = SNDRV_CTL_ELEM_IFACE_MIXER, .name = (xname),\
-	.info = snd_soc_info_volsw, .get = snd_soc_get_volsw_range, \
+	.info = snd_soc_info_volsw, .get = snd_soc_get_volsw, \
 	.put = snd_soc_put_volsw_range, \
 	.private_value = SOC_SINGLE_VALUE(xreg, xshift, xmin, xmax, xinvert, 0) }
 #define SOC_SINGLE_TLV(xname, reg, shift, max, invert, tlv_array) \
@@ -91,7 +91,7 @@ struct platform_device;
 		 SNDRV_CTL_ELEM_ACCESS_READWRITE,\
 	.tlv.p = (tlv_array), \
 	.info = snd_soc_info_volsw, \
-	.get = snd_soc_get_volsw_range, .put = snd_soc_put_volsw_range, \
+	.get = snd_soc_get_volsw, .put = snd_soc_put_volsw_range, \
 	.private_value = SOC_SINGLE_VALUE(xreg, xshift, xmin, xmax, xinvert, 0) }
 #define SOC_DOUBLE(xname, reg, shift_left, shift_right, max, invert) \
 {	.iface = SNDRV_CTL_ELEM_IFACE_MIXER, .name = (xname),\
@@ -117,7 +117,7 @@ struct platform_device;
 			   xmax, xinvert)		\
 {	.iface = SNDRV_CTL_ELEM_IFACE_MIXER, .name = (xname),\
 	.info = snd_soc_info_volsw, \
-	.get = snd_soc_get_volsw_range, .put = snd_soc_put_volsw_range, \
+	.get = snd_soc_get_volsw, .put = snd_soc_put_volsw_range, \
 	.private_value = SOC_DOUBLE_R_VALUE(reg_left, reg_right, \
 					    xshift, xmin, xmax, xinvert) }
 #define SOC_DOUBLE_TLV(xname, reg, shift_left, shift_right, max, invert, tlv_array) \
@@ -165,7 +165,7 @@ struct platform_device;
 		 SNDRV_CTL_ELEM_ACCESS_READWRITE,\
 	.tlv.p = (tlv_array), \
 	.info = snd_soc_info_volsw, \
-	.get = snd_soc_get_volsw_range, .put = snd_soc_put_volsw_range, \
+	.get = snd_soc_get_volsw, .put = snd_soc_put_volsw_range, \
 	.private_value = SOC_DOUBLE_R_VALUE(reg_left, reg_right, \
 					    xshift, xmin, xmax, xinvert) }
 #define SOC_DOUBLE_R_SX_TLV(xname, xreg, xrreg, xshift, xmin, xmax, tlv_array) \
@@ -571,8 +571,6 @@ int snd_soc_put_volsw_sx(struct snd_kcontrol *kcontrol,
 	struct snd_ctl_elem_value *ucontrol);
 int snd_soc_put_volsw_range(struct snd_kcontrol *kcontrol,
 	struct snd_ctl_elem_value *ucontrol);
-int snd_soc_get_volsw_range(struct snd_kcontrol *kcontrol,
-	struct snd_ctl_elem_value *ucontrol);
 int snd_soc_limit_volume(struct snd_soc_card *card,
 	const char *name, int max);
 int snd_soc_bytes_info(struct snd_kcontrol *kcontrol,
diff --git a/sound/soc/codecs/wm5110.c b/sound/soc/codecs/wm5110.c
index 64eee0d2347da..c24b42c375785 100644
--- a/sound/soc/codecs/wm5110.c
+++ b/sound/soc/codecs/wm5110.c
@@ -477,7 +477,7 @@ static int wm5110_in_pga_get(struct snd_kcontrol *kcontrol,
 	 */
 	snd_soc_dapm_mutex_lock(dapm);
 
-	ret = snd_soc_get_volsw_range(kcontrol, ucontrol);
+	ret = snd_soc_get_volsw(kcontrol, ucontrol);
 
 	snd_soc_dapm_mutex_unlock(dapm);
 
diff --git a/sound/soc/soc-ops.c b/sound/soc/soc-ops.c
index 1b52ba12df8df..fbda8e21c5a60 100644
--- a/sound/soc/soc-ops.c
+++ b/sound/soc/soc-ops.c
@@ -248,12 +248,12 @@ int snd_soc_info_volsw_sx(struct snd_kcontrol *kcontrol,
 EXPORT_SYMBOL_GPL(snd_soc_info_volsw_sx);
 
 /**
- * snd_soc_get_volsw - single mixer get callback
+ * snd_soc_get_volsw - single mixer get callback with range
  * @kcontrol: mixer control
  * @ucontrol: control element information
  *
- * Callback to get the value of a single mixer control, or a double mixer
- * control that spans 2 registers.
+ * Callback to get the value, within a range, of a single mixer control, or a
+ * double mixer control that spans 2 registers.
  *
  * Returns 0 for success.
  */
@@ -506,42 +506,6 @@ int snd_soc_put_volsw_range(struct snd_kcontrol *kcontrol,
 }
 EXPORT_SYMBOL_GPL(snd_soc_put_volsw_range);
 
-/**
- * snd_soc_get_volsw_range - single mixer get callback with range
- * @kcontrol: mixer control
- * @ucontrol: control element information
- *
- * Callback to get the value, within a range, of a single mixer control.
- *
- * Returns 0 for success.
- */
-int snd_soc_get_volsw_range(struct snd_kcontrol *kcontrol,
-			    struct snd_ctl_elem_value *ucontrol)
-{
-	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
-	struct soc_mixer_control *mc =
-		(struct soc_mixer_control *)kcontrol->private_value;
-	int max = mc->max - mc->min;
-	unsigned int mask = soc_mixer_mask(mc);
-	unsigned int reg_val;
-	int val;
-
-	reg_val = snd_soc_component_read(component, mc->reg);
-	val = soc_mixer_reg_to_ctl(mc, reg_val, mask, mc->shift, max);
-
-	ucontrol->value.integer.value[0] = val;
-
-	if (snd_soc_volsw_is_stereo(mc)) {
-		reg_val = snd_soc_component_read(component, mc->rreg);
-		val = soc_mixer_reg_to_ctl(mc, reg_val, mask, mc->shift, max);
-
-		ucontrol->value.integer.value[1] = val;
-	}
-
-	return 0;
-}
-EXPORT_SYMBOL_GPL(snd_soc_get_volsw_range);
-
 static int snd_soc_clip_to_platform_max(struct snd_kcontrol *kctl)
 {
 	struct soc_mixer_control *mc = (struct soc_mixer_control *)kctl->private_value;
diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
index 9cbddfbbc556b..3c988925c512f 100644
--- a/sound/soc/soc-topology.c
+++ b/sound/soc/soc-topology.c
@@ -131,7 +131,7 @@ static const struct snd_soc_tplg_kcontrol_ops io_ops[] = {
 		snd_soc_put_enum_double, NULL},
 	{SND_SOC_TPLG_CTL_BYTES, snd_soc_bytes_get,
 		snd_soc_bytes_put, snd_soc_bytes_info},
-	{SND_SOC_TPLG_CTL_RANGE, snd_soc_get_volsw_range,
+	{SND_SOC_TPLG_CTL_RANGE, snd_soc_get_volsw,
 		snd_soc_put_volsw_range, snd_soc_info_volsw},
 	{SND_SOC_TPLG_CTL_VOLSW_XR_SX, snd_soc_get_xr_sx,
 		snd_soc_put_xr_sx, snd_soc_info_xr_sx},
-- 
2.39.5


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH 10/15] ASoC: ops: Remove snd_soc_put_volsw_range()
  2025-03-18 17:14 [PATCH 00/15] Tidy up ASoC control get and put handlers Charles Keepax
                   ` (8 preceding siblings ...)
  2025-03-18 17:14 ` [PATCH 09/15] ASoC: ops: Remove snd_soc_get_volsw_range() Charles Keepax
@ 2025-03-18 17:14 ` Charles Keepax
  2025-03-18 17:14 ` [PATCH 11/15] ASoC: ops: Factor out common code from info callbacks Charles Keepax
                   ` (5 subsequent siblings)
  15 siblings, 0 replies; 20+ messages in thread
From: Charles Keepax @ 2025-03-18 17:14 UTC (permalink / raw)
  To: broonie
  Cc: lgirdwood, shenghao-ding, kevin-lu, baojun.xu, linux-sound,
	linux-kernel, patches

With the addition of the soc_mixer_ctl_to_reg() helper it is now very
clear that the only difference between snd_soc_put_volsw() and
snd_soc_put_volsw_range() is that the former supports double controls
with both values in the same register. As such we can combine both
functions.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 include/sound/soc.h       | 10 +++----
 sound/soc/codecs/wm5110.c |  2 +-
 sound/soc/soc-ops.c       | 62 ++-------------------------------------
 sound/soc/soc-topology.c  |  2 +-
 4 files changed, 9 insertions(+), 67 deletions(-)

diff --git a/include/sound/soc.h b/include/sound/soc.h
index b11c6cdb0201c..952ed77b8c87f 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -66,7 +66,7 @@ struct platform_device;
 #define SOC_SINGLE_RANGE(xname, xreg, xshift, xmin, xmax, xinvert) \
 {	.iface = SNDRV_CTL_ELEM_IFACE_MIXER, .name = (xname),\
 	.info = snd_soc_info_volsw, .get = snd_soc_get_volsw, \
-	.put = snd_soc_put_volsw_range, \
+	.put = snd_soc_put_volsw, \
 	.private_value = SOC_SINGLE_VALUE(xreg, xshift, xmin, xmax, xinvert, 0) }
 #define SOC_SINGLE_TLV(xname, reg, shift, max, invert, tlv_array) \
 {	.iface = SNDRV_CTL_ELEM_IFACE_MIXER, .name = xname, \
@@ -91,7 +91,7 @@ struct platform_device;
 		 SNDRV_CTL_ELEM_ACCESS_READWRITE,\
 	.tlv.p = (tlv_array), \
 	.info = snd_soc_info_volsw, \
-	.get = snd_soc_get_volsw, .put = snd_soc_put_volsw_range, \
+	.get = snd_soc_get_volsw, .put = snd_soc_put_volsw, \
 	.private_value = SOC_SINGLE_VALUE(xreg, xshift, xmin, xmax, xinvert, 0) }
 #define SOC_DOUBLE(xname, reg, shift_left, shift_right, max, invert) \
 {	.iface = SNDRV_CTL_ELEM_IFACE_MIXER, .name = (xname),\
@@ -117,7 +117,7 @@ struct platform_device;
 			   xmax, xinvert)		\
 {	.iface = SNDRV_CTL_ELEM_IFACE_MIXER, .name = (xname),\
 	.info = snd_soc_info_volsw, \
-	.get = snd_soc_get_volsw, .put = snd_soc_put_volsw_range, \
+	.get = snd_soc_get_volsw, .put = snd_soc_put_volsw, \
 	.private_value = SOC_DOUBLE_R_VALUE(reg_left, reg_right, \
 					    xshift, xmin, xmax, xinvert) }
 #define SOC_DOUBLE_TLV(xname, reg, shift_left, shift_right, max, invert, tlv_array) \
@@ -165,7 +165,7 @@ struct platform_device;
 		 SNDRV_CTL_ELEM_ACCESS_READWRITE,\
 	.tlv.p = (tlv_array), \
 	.info = snd_soc_info_volsw, \
-	.get = snd_soc_get_volsw, .put = snd_soc_put_volsw_range, \
+	.get = snd_soc_get_volsw, .put = snd_soc_put_volsw, \
 	.private_value = SOC_DOUBLE_R_VALUE(reg_left, reg_right, \
 					    xshift, xmin, xmax, xinvert) }
 #define SOC_DOUBLE_R_SX_TLV(xname, xreg, xrreg, xshift, xmin, xmax, tlv_array) \
@@ -569,8 +569,6 @@ int snd_soc_get_volsw_sx(struct snd_kcontrol *kcontrol,
 	struct snd_ctl_elem_value *ucontrol);
 int snd_soc_put_volsw_sx(struct snd_kcontrol *kcontrol,
 	struct snd_ctl_elem_value *ucontrol);
-int snd_soc_put_volsw_range(struct snd_kcontrol *kcontrol,
-	struct snd_ctl_elem_value *ucontrol);
 int snd_soc_limit_volume(struct snd_soc_card *card,
 	const char *name, int max);
 int snd_soc_bytes_info(struct snd_kcontrol *kcontrol,
diff --git a/sound/soc/codecs/wm5110.c b/sound/soc/codecs/wm5110.c
index c24b42c375785..212eca675f27e 100644
--- a/sound/soc/codecs/wm5110.c
+++ b/sound/soc/codecs/wm5110.c
@@ -497,7 +497,7 @@ static int wm5110_in_pga_put(struct snd_kcontrol *kcontrol,
 	 */
 	snd_soc_dapm_mutex_lock(dapm);
 
-	ret = snd_soc_put_volsw_range(kcontrol, ucontrol);
+	ret = snd_soc_put_volsw(kcontrol, ucontrol);
 
 	snd_soc_dapm_mutex_unlock(dapm);
 
diff --git a/sound/soc/soc-ops.c b/sound/soc/soc-ops.c
index fbda8e21c5a60..d26d9e050af12 100644
--- a/sound/soc/soc-ops.c
+++ b/sound/soc/soc-ops.c
@@ -289,12 +289,12 @@ int snd_soc_get_volsw(struct snd_kcontrol *kcontrol,
 EXPORT_SYMBOL_GPL(snd_soc_get_volsw);
 
 /**
- * snd_soc_put_volsw - single mixer put callback
+ * snd_soc_put_volsw - single mixer put callback with range
  * @kcontrol: mixer control
  * @ucontrol: control element information
  *
- * Callback to set the value of a single mixer control, or a double mixer
- * control that spans 2 registers.
+ * Callback to set the value , within a range, of a single mixer control, or
+ * a double mixer control that spans 2 registers.
  *
  * Returns 0 for success.
  */
@@ -450,62 +450,6 @@ int snd_soc_put_volsw_sx(struct snd_kcontrol *kcontrol,
 }
 EXPORT_SYMBOL_GPL(snd_soc_put_volsw_sx);
 
-/**
- * snd_soc_put_volsw_range - single mixer put value callback with range.
- * @kcontrol: mixer control
- * @ucontrol: control element information
- *
- * Callback to set the value, within a range, for a single mixer control.
- *
- * Returns 0 for success.
- */
-int snd_soc_put_volsw_range(struct snd_kcontrol *kcontrol,
-			    struct snd_ctl_elem_value *ucontrol)
-{
-	struct soc_mixer_control *mc =
-		(struct soc_mixer_control *)kcontrol->private_value;
-	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
-	unsigned int reg = mc->reg;
-	unsigned int rreg = mc->rreg;
-	int max = mc->max - mc->min;
-	unsigned int mask = soc_mixer_mask(mc);
-	unsigned int val_mask = mask << mc->shift;
-	unsigned int val;
-	int err, ret;
-
-	ret = soc_mixer_valid_ctl(mc, ucontrol->value.integer.value[0],
-				  mc->max - mc->min);
-	if (ret)
-		return ret;
-
-	val = soc_mixer_ctl_to_reg(mc, ucontrol->value.integer.value[0],
-				   mask, mc->shift, max);
-
-	err = snd_soc_component_update_bits(component, reg, val_mask, val);
-	if (err < 0)
-		return err;
-	ret = err;
-
-	if (snd_soc_volsw_is_stereo(mc)) {
-		ret = soc_mixer_valid_ctl(mc, ucontrol->value.integer.value[1],
-					  max);
-		if (ret)
-			return ret;
-
-		val = soc_mixer_ctl_to_reg(mc, ucontrol->value.integer.value[1],
-					   mask, mc->shift, max);
-
-		err = snd_soc_component_update_bits(component, rreg, val_mask,
-						    val);
-		/* Don't discard any error code or drop change flag */
-		if (ret == 0 || err < 0)
-			ret = err;
-	}
-
-	return ret;
-}
-EXPORT_SYMBOL_GPL(snd_soc_put_volsw_range);
-
 static int snd_soc_clip_to_platform_max(struct snd_kcontrol *kctl)
 {
 	struct soc_mixer_control *mc = (struct soc_mixer_control *)kctl->private_value;
diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
index 3c988925c512f..7b0b8531bb32f 100644
--- a/sound/soc/soc-topology.c
+++ b/sound/soc/soc-topology.c
@@ -132,7 +132,7 @@ static const struct snd_soc_tplg_kcontrol_ops io_ops[] = {
 	{SND_SOC_TPLG_CTL_BYTES, snd_soc_bytes_get,
 		snd_soc_bytes_put, snd_soc_bytes_info},
 	{SND_SOC_TPLG_CTL_RANGE, snd_soc_get_volsw,
-		snd_soc_put_volsw_range, snd_soc_info_volsw},
+		snd_soc_put_volsw, snd_soc_info_volsw},
 	{SND_SOC_TPLG_CTL_VOLSW_XR_SX, snd_soc_get_xr_sx,
 		snd_soc_put_xr_sx, snd_soc_info_xr_sx},
 	{SND_SOC_TPLG_CTL_STROBE, snd_soc_get_strobe,
-- 
2.39.5


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH 11/15] ASoC: ops: Factor out common code from info callbacks
  2025-03-18 17:14 [PATCH 00/15] Tidy up ASoC control get and put handlers Charles Keepax
                   ` (9 preceding siblings ...)
  2025-03-18 17:14 ` [PATCH 10/15] ASoC: ops: Remove snd_soc_put_volsw_range() Charles Keepax
@ 2025-03-18 17:14 ` Charles Keepax
  2025-03-18 17:14 ` [PATCH 12/15] ASoC: ops: Factor out common code from put callbacks Charles Keepax
                   ` (4 subsequent siblings)
  15 siblings, 0 replies; 20+ messages in thread
From: Charles Keepax @ 2025-03-18 17:14 UTC (permalink / raw)
  To: broonie
  Cc: lgirdwood, shenghao-ding, kevin-lu, baojun.xu, linux-sound,
	linux-kernel, patches

snd_soc_info_volsw() and snd_soc_info_volsw_sx() do very similar
things, and have a lot of code in common. Already this is causing
some issues as the detection of volume controls has been fixed
in the normal callback but not the sx callback. Factor out a new
helper containing the common code and leave the function specific
bits behind in each callback.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 sound/soc/soc-ops.c | 64 ++++++++++++++++++---------------------------
 1 file changed, 26 insertions(+), 38 deletions(-)

diff --git a/sound/soc/soc-ops.c b/sound/soc/soc-ops.c
index d26d9e050af12..29537dd3a0633 100644
--- a/sound/soc/soc-ops.c
+++ b/sound/soc/soc-ops.c
@@ -168,6 +168,30 @@ static int soc_mixer_sx_mask(struct soc_mixer_control *mc)
 	return GENMASK(fls(mc->min + mc->max) - 2, 0);
 }
 
+static int soc_info_volsw(struct snd_kcontrol *kcontrol,
+			  struct snd_ctl_elem_info *uinfo,
+			  struct soc_mixer_control *mc, int max)
+{
+	if (mc->platform_max && mc->platform_max < max)
+		max = mc->platform_max;
+
+	uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER;
+
+	if (max == 1) {
+		/* Even two value controls ending in Volume should be integer */
+		const char *vol_string = strstr(kcontrol->id.name, " Volume");
+
+		if (!vol_string || strcmp(vol_string, " Volume"))
+			uinfo->type = SNDRV_CTL_ELEM_TYPE_BOOLEAN;
+	}
+
+	uinfo->count = snd_soc_volsw_is_stereo(mc) ? 2 : 1;
+	uinfo->value.integer.min = 0;
+	uinfo->value.integer.max = max;
+
+	return 0;
+}
+
 /**
  * snd_soc_info_volsw - single mixer info callback with range.
  * @kcontrol: mixer control
@@ -183,29 +207,8 @@ int snd_soc_info_volsw(struct snd_kcontrol *kcontrol,
 {
 	struct soc_mixer_control *mc =
 		(struct soc_mixer_control *)kcontrol->private_value;
-	const char *vol_string = NULL;
-	int max;
-
-	max = uinfo->value.integer.max = mc->max - mc->min;
-	if (mc->platform_max && mc->platform_max < max)
-		max = mc->platform_max;
-
-	if (max == 1) {
-		/* Even two value controls ending in Volume should always be integer */
-		vol_string = strstr(kcontrol->id.name, " Volume");
-		if (vol_string && !strcmp(vol_string, " Volume"))
-			uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER;
-		else
-			uinfo->type = SNDRV_CTL_ELEM_TYPE_BOOLEAN;
-	} else {
-		uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER;
-	}
 
-	uinfo->count = snd_soc_volsw_is_stereo(mc) ? 2 : 1;
-	uinfo->value.integer.min = 0;
-	uinfo->value.integer.max = max;
-
-	return 0;
+	return soc_info_volsw(kcontrol, uinfo, mc, mc->max - mc->min);
 }
 EXPORT_SYMBOL_GPL(snd_soc_info_volsw);
 
@@ -227,23 +230,8 @@ int snd_soc_info_volsw_sx(struct snd_kcontrol *kcontrol,
 {
 	struct soc_mixer_control *mc =
 		(struct soc_mixer_control *)kcontrol->private_value;
-	int max;
 
-	if (mc->platform_max)
-		max = mc->platform_max;
-	else
-		max = mc->max;
-
-	if (max == 1 && !strstr(kcontrol->id.name, " Volume"))
-		uinfo->type = SNDRV_CTL_ELEM_TYPE_BOOLEAN;
-	else
-		uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER;
-
-	uinfo->count = snd_soc_volsw_is_stereo(mc) ? 2 : 1;
-	uinfo->value.integer.min = 0;
-	uinfo->value.integer.max = max;
-
-	return 0;
+	return soc_info_volsw(kcontrol, uinfo, mc, mc->max);
 }
 EXPORT_SYMBOL_GPL(snd_soc_info_volsw_sx);
 
-- 
2.39.5


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH 12/15] ASoC: ops: Factor out common code from put callbacks
  2025-03-18 17:14 [PATCH 00/15] Tidy up ASoC control get and put handlers Charles Keepax
                   ` (10 preceding siblings ...)
  2025-03-18 17:14 ` [PATCH 11/15] ASoC: ops: Factor out common code from info callbacks Charles Keepax
@ 2025-03-18 17:14 ` Charles Keepax
  2025-03-18 17:14 ` [PATCH 13/15] ASoC: ops: Factor out common code from get callbacks Charles Keepax
                   ` (3 subsequent siblings)
  15 siblings, 0 replies; 20+ messages in thread
From: Charles Keepax @ 2025-03-18 17:14 UTC (permalink / raw)
  To: broonie
  Cc: lgirdwood, shenghao-ding, kevin-lu, baojun.xu, linux-sound,
	linux-kernel, patches

There are only two differences between snd_soc_put_volsw() and
snd_soc_put_volsw_sx(). The maximum field is handled differently, and
snd_soc_put_volsw() supports double controls with both values in the
same register.

Factor out the common code into a new helper and pass in the
appropriate max value such that it is handled correctly for each
control.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 sound/soc/soc-ops.c | 138 +++++++++++++++++---------------------------
 1 file changed, 53 insertions(+), 85 deletions(-)

diff --git a/sound/soc/soc-ops.c b/sound/soc/soc-ops.c
index 29537dd3a0633..0b62ffb2e222f 100644
--- a/sound/soc/soc-ops.c
+++ b/sound/soc/soc-ops.c
@@ -192,6 +192,57 @@ static int soc_info_volsw(struct snd_kcontrol *kcontrol,
 	return 0;
 }
 
+static int soc_put_volsw(struct snd_kcontrol *kcontrol,
+			 struct snd_ctl_elem_value *ucontrol,
+			 struct soc_mixer_control *mc, int mask, int max)
+{
+	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
+	unsigned int val1, val_mask;
+	unsigned int val2 = 0;
+	bool double_r = false;
+	int ret;
+
+	ret = soc_mixer_valid_ctl(mc, ucontrol->value.integer.value[0], max);
+	if (ret)
+		return ret;
+
+	val1 = soc_mixer_ctl_to_reg(mc, ucontrol->value.integer.value[0],
+				    mask, mc->shift, max);
+	val_mask = mask << mc->shift;
+
+	if (snd_soc_volsw_is_stereo(mc)) {
+		ret = soc_mixer_valid_ctl(mc, ucontrol->value.integer.value[1], max);
+		if (ret)
+			return ret;
+
+		if (mc->reg == mc->rreg) {
+			val1 |= soc_mixer_ctl_to_reg(mc,
+						     ucontrol->value.integer.value[1],
+						     mask, mc->rshift, max);
+			val_mask |= mask << mc->rshift;
+		} else {
+			val2 = soc_mixer_ctl_to_reg(mc,
+						    ucontrol->value.integer.value[1],
+						    mask, mc->shift, max);
+			double_r = true;
+		}
+	}
+
+	ret = snd_soc_component_update_bits(component, mc->reg, val_mask, val1);
+	if (ret < 0)
+		return ret;
+
+	if (double_r) {
+		int err = snd_soc_component_update_bits(component, mc->rreg,
+							val_mask, val2);
+		/* Don't drop change flag */
+		if (err)
+			return err;
+	}
+
+	return ret;
+}
+
 /**
  * snd_soc_info_volsw - single mixer info callback with range.
  * @kcontrol: mixer control
@@ -289,57 +340,11 @@ EXPORT_SYMBOL_GPL(snd_soc_get_volsw);
 int snd_soc_put_volsw(struct snd_kcontrol *kcontrol,
 		      struct snd_ctl_elem_value *ucontrol)
 {
-	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
 	struct soc_mixer_control *mc =
 		(struct soc_mixer_control *)kcontrol->private_value;
-	unsigned int reg = mc->reg;
-	unsigned int reg2 = mc->rreg;
-	int max = mc->max - mc->min;
 	unsigned int mask = soc_mixer_mask(mc);
-	int err, ret;
-	bool type_2r = false;
-	unsigned int val2 = 0;
-	unsigned int val, val_mask;
 
-	ret = soc_mixer_valid_ctl(mc, ucontrol->value.integer.value[0], max);
-	if (ret)
-		return ret;
-
-	val = soc_mixer_ctl_to_reg(mc, ucontrol->value.integer.value[0],
-				   mask, mc->shift, max);
-	val_mask = mask << mc->shift;
-
-	if (snd_soc_volsw_is_stereo(mc)) {
-		ret = soc_mixer_valid_ctl(mc, ucontrol->value.integer.value[1], max);
-		if (ret)
-			return ret;
-
-		if (reg == reg2) {
-			val |= soc_mixer_ctl_to_reg(mc,
-						    ucontrol->value.integer.value[1],
-						    mask, mc->rshift, max);
-			val_mask |= mask << mc->rshift;
-		} else {
-			val2 = soc_mixer_ctl_to_reg(mc,
-						    ucontrol->value.integer.value[1],
-						    mask, mc->shift, max);
-			type_2r = true;
-		}
-	}
-	err = snd_soc_component_update_bits(component, reg, val_mask, val);
-	if (err < 0)
-		return err;
-	ret = err;
-
-	if (type_2r) {
-		err = snd_soc_component_update_bits(component, reg2, val_mask,
-						    val2);
-		/* Don't discard any error code or drop change flag */
-		if (ret == 0 || err < 0)
-			ret = err;
-	}
-
-	return ret;
+	return soc_put_volsw(kcontrol, ucontrol, mc, mask, mc->max - mc->min);
 }
 EXPORT_SYMBOL_GPL(snd_soc_put_volsw);
 
@@ -393,48 +398,11 @@ EXPORT_SYMBOL_GPL(snd_soc_get_volsw_sx);
 int snd_soc_put_volsw_sx(struct snd_kcontrol *kcontrol,
 			 struct snd_ctl_elem_value *ucontrol)
 {
-	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
 	struct soc_mixer_control *mc =
 		(struct soc_mixer_control *)kcontrol->private_value;
-	unsigned int reg = mc->reg;
-	unsigned int reg2 = mc->rreg;
-	unsigned int val, val_mask;
 	unsigned int mask = soc_mixer_sx_mask(mc);
-	int err = 0;
-	int ret;
-
-	ret = soc_mixer_valid_ctl(mc, ucontrol->value.integer.value[0], mc->max);
-	if (ret)
-		return ret;
-
-	val = soc_mixer_ctl_to_reg(mc, ucontrol->value.integer.value[0],
-				   mask, mc->shift, mc->max);
-	val_mask = mask << mc->shift;
-
-	err = snd_soc_component_update_bits(component, reg, val_mask, val);
-	if (err < 0)
-		return err;
-	ret = err;
 
-	if (snd_soc_volsw_is_stereo(mc)) {
-		ret = soc_mixer_valid_ctl(mc, ucontrol->value.integer.value[1],
-					  mc->max);
-		if (ret)
-			return ret;
-
-		val = soc_mixer_ctl_to_reg(mc, ucontrol->value.integer.value[1],
-					   mask, mc->rshift, mc->max);
-		val_mask = mask << mc->rshift;
-
-		err = snd_soc_component_update_bits(component, reg2, val_mask,
-						    val);
-
-		/* Don't discard any error code or drop change flag */
-		if (ret == 0 || err < 0)
-			ret = err;
-	}
-
-	return ret;
+	return soc_put_volsw(kcontrol, ucontrol, mc, mask, mc->max);
 }
 EXPORT_SYMBOL_GPL(snd_soc_put_volsw_sx);
 
-- 
2.39.5


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH 13/15] ASoC: ops: Factor out common code from get callbacks
  2025-03-18 17:14 [PATCH 00/15] Tidy up ASoC control get and put handlers Charles Keepax
                   ` (11 preceding siblings ...)
  2025-03-18 17:14 ` [PATCH 12/15] ASoC: ops: Factor out common code from put callbacks Charles Keepax
@ 2025-03-18 17:14 ` Charles Keepax
  2025-03-19 15:12   ` Mark Brown
  2025-03-18 17:14 ` [PATCH 14/15] ASoC: ops: Remove some unnecessary local variables Charles Keepax
                   ` (2 subsequent siblings)
  15 siblings, 1 reply; 20+ messages in thread
From: Charles Keepax @ 2025-03-18 17:14 UTC (permalink / raw)
  To: broonie
  Cc: lgirdwood, shenghao-ding, kevin-lu, baojun.xu, linux-sound,
	linux-kernel, patches

There are only two differences between snd_soc_get_volsw() and
snd_soc_get_volsw_sx(). The maximum field is handled differently, and
snd_soc_get_volsw() supports double controls with both values in the
same register.

Factor out the common code into a new helper and pass in the
appropriate max value such that it is handled correctly for each
control.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 sound/soc/soc-ops.c | 66 ++++++++++++++++++++-------------------------
 1 file changed, 29 insertions(+), 37 deletions(-)

diff --git a/sound/soc/soc-ops.c b/sound/soc/soc-ops.c
index 0b62ffb2e222f..3ec3242a2b114 100644
--- a/sound/soc/soc-ops.c
+++ b/sound/soc/soc-ops.c
@@ -243,6 +243,33 @@ static int soc_put_volsw(struct snd_kcontrol *kcontrol,
 	return ret;
 }
 
+static int soc_get_volsw(struct snd_kcontrol *kcontrol,
+			 struct snd_ctl_elem_value *ucontrol,
+			 struct soc_mixer_control *mc, int mask, int max)
+{
+	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
+	unsigned int reg_val;
+	int val;
+
+	reg_val = snd_soc_component_read(component, mc->reg);
+	val = soc_mixer_reg_to_ctl(mc, reg_val, mask, mc->shift, max);
+
+	ucontrol->value.integer.value[0] = val;
+
+	if (snd_soc_volsw_is_stereo(mc)) {
+		if (mc->reg == mc->rreg) {
+			val = soc_mixer_reg_to_ctl(mc, reg_val, mask, mc->rshift, max);
+		} else {
+			reg_val = snd_soc_component_read(component, mc->rreg);
+			val = soc_mixer_reg_to_ctl(mc, reg_val, mask, mc->shift, max);
+		}
+
+		ucontrol->value.integer.value[1] = val;
+	}
+
+	return 0;
+}
+
 /**
  * snd_soc_info_volsw - single mixer info callback with range.
  * @kcontrol: mixer control
@@ -299,31 +326,11 @@ EXPORT_SYMBOL_GPL(snd_soc_info_volsw_sx);
 int snd_soc_get_volsw(struct snd_kcontrol *kcontrol,
 		      struct snd_ctl_elem_value *ucontrol)
 {
-	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
 	struct soc_mixer_control *mc =
 		(struct soc_mixer_control *)kcontrol->private_value;
-	int max = mc->max - mc->min;
 	unsigned int mask = soc_mixer_mask(mc);
-	unsigned int reg_val;
-	int val;
 
-	reg_val = snd_soc_component_read(component, mc->reg);
-	val = soc_mixer_reg_to_ctl(mc, reg_val, mask, mc->shift, max);
-
-	ucontrol->value.integer.value[0] = val;
-
-	if (snd_soc_volsw_is_stereo(mc)) {
-		if (mc->reg == mc->rreg) {
-			val = soc_mixer_reg_to_ctl(mc, reg_val, mask, mc->rshift, max);
-		} else {
-			reg_val = snd_soc_component_read(component, mc->rreg);
-			val = soc_mixer_reg_to_ctl(mc, reg_val, mask, mc->shift, max);
-		}
-
-		ucontrol->value.integer.value[1] = val;
-	}
-
-	return 0;
+	return soc_get_volsw(kcontrol, ucontrol, mc, mask, mc->max - mc->min);
 }
 EXPORT_SYMBOL_GPL(snd_soc_get_volsw);
 
@@ -361,28 +368,13 @@ EXPORT_SYMBOL_GPL(snd_soc_put_volsw);
 int snd_soc_get_volsw_sx(struct snd_kcontrol *kcontrol,
 			 struct snd_ctl_elem_value *ucontrol)
 {
-	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
 	struct soc_mixer_control *mc =
 		(struct soc_mixer_control *)kcontrol->private_value;
 	unsigned int reg = mc->reg;
 	unsigned int reg2 = mc->rreg;
 	unsigned int mask = soc_mixer_sx_mask(mc);
-	unsigned int reg_val;
-	int val;
-
-	reg_val = snd_soc_component_read(component, reg);
-	val = soc_mixer_reg_to_ctl(mc, reg_val, mask, mc->shift, mc->max);
-
-	ucontrol->value.integer.value[0] = val;
-
-	if (snd_soc_volsw_is_stereo(mc)) {
-		reg_val = snd_soc_component_read(component, reg2);
-		val = soc_mixer_reg_to_ctl(mc, reg_val, mask, mc->rshift, mc->max);
 
-		ucontrol->value.integer.value[1] = val;
-	}
-
-	return 0;
+	return soc_get_volsw(kcontrol, ucontrol, mc, mask, mc->max);
 }
 EXPORT_SYMBOL_GPL(snd_soc_get_volsw_sx);
 
-- 
2.39.5


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH 14/15] ASoC: ops: Remove some unnecessary local variables
  2025-03-18 17:14 [PATCH 00/15] Tidy up ASoC control get and put handlers Charles Keepax
                   ` (12 preceding siblings ...)
  2025-03-18 17:14 ` [PATCH 13/15] ASoC: ops: Factor out common code from get callbacks Charles Keepax
@ 2025-03-18 17:14 ` Charles Keepax
  2025-03-18 17:14 ` [PATCH 15/15] ASoC: ops: Apply platform_max after deciding control type Charles Keepax
  2025-03-20 18:45 ` [PATCH 00/15] Tidy up ASoC control get and put handlers Mark Brown
  15 siblings, 0 replies; 20+ messages in thread
From: Charles Keepax @ 2025-03-18 17:14 UTC (permalink / raw)
  To: broonie
  Cc: lgirdwood, shenghao-ding, kevin-lu, baojun.xu, linux-sound,
	linux-kernel, patches

Remove some local variables that aren't adding much in terms of clarity
or space saving.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 sound/soc/soc-ops.c | 44 +++++++++++++++++---------------------------
 1 file changed, 17 insertions(+), 27 deletions(-)

diff --git a/sound/soc/soc-ops.c b/sound/soc/soc-ops.c
index 3ec3242a2b114..3ac5b3a62c812 100644
--- a/sound/soc/soc-ops.c
+++ b/sound/soc/soc-ops.c
@@ -370,8 +370,6 @@ int snd_soc_get_volsw_sx(struct snd_kcontrol *kcontrol,
 {
 	struct soc_mixer_control *mc =
 		(struct soc_mixer_control *)kcontrol->private_value;
-	unsigned int reg = mc->reg;
-	unsigned int reg2 = mc->rreg;
 	unsigned int mask = soc_mixer_sx_mask(mc);
 
 	return soc_get_volsw(kcontrol, ucontrol, mc, mask, mc->max);
@@ -666,9 +664,6 @@ int snd_soc_get_xr_sx(struct snd_kcontrol *kcontrol,
 	unsigned int regwshift = component->val_bytes * BITS_PER_BYTE;
 	unsigned int regwmask = GENMASK(regwshift - 1, 0);
 	unsigned long mask = GENMASK(mc->nbits - 1, 0);
-	unsigned int invert = mc->invert;
-	long min = mc->min;
-	long max = mc->max;
 	long val = 0;
 	unsigned int i;
 
@@ -678,10 +673,10 @@ int snd_soc_get_xr_sx(struct snd_kcontrol *kcontrol,
 		val |= (regval & regwmask) << (regwshift * (regcount - i - 1));
 	}
 	val &= mask;
-	if (min < 0 && val > max)
+	if (mc->min < 0 && val > mc->max)
 		val |= ~mask;
-	if (invert)
-		val = max - val;
+	if (mc->invert)
+		val = mc->max - val;
 	ucontrol->value.integer.value[0] = val;
 
 	return 0;
@@ -713,16 +708,14 @@ int snd_soc_put_xr_sx(struct snd_kcontrol *kcontrol,
 	unsigned int regwshift = component->val_bytes * BITS_PER_BYTE;
 	unsigned int regwmask = GENMASK(regwshift - 1, 0);
 	unsigned long mask = GENMASK(mc->nbits - 1, 0);
-	unsigned int invert = mc->invert;
-	long max = mc->max;
 	long val = ucontrol->value.integer.value[0];
 	int ret = 0;
 	unsigned int i;
 
 	if (val < mc->min || val > mc->max)
 		return -EINVAL;
-	if (invert)
-		val = max - val;
+	if (mc->invert)
+		val = mc->max - val;
 	val &= mask;
 	for (i = 0; i < regcount; i++) {
 		unsigned int regval = (val >> (regwshift * (regcount - i - 1))) &
@@ -757,17 +750,16 @@ int snd_soc_get_strobe(struct snd_kcontrol *kcontrol,
 	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
 	struct soc_mixer_control *mc =
 		(struct soc_mixer_control *)kcontrol->private_value;
-	unsigned int reg = mc->reg;
-	unsigned int shift = mc->shift;
-	unsigned int mask = BIT(shift);
 	unsigned int invert = mc->invert != 0;
+	unsigned int mask = BIT(mc->shift);
 	unsigned int val;
 
-	val = snd_soc_component_read(component, reg);
+	val = snd_soc_component_read(component, mc->reg);
 	val &= mask;
 
-	if (shift != 0 && val != 0)
-		val = val >> shift;
+	if (mc->shift != 0 && val != 0)
+		val = val >> mc->shift;
+
 	ucontrol->value.enumerated.item[0] = val ^ invert;
 
 	return 0;
@@ -790,19 +782,17 @@ int snd_soc_put_strobe(struct snd_kcontrol *kcontrol,
 	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
 	struct soc_mixer_control *mc =
 		(struct soc_mixer_control *)kcontrol->private_value;
-	unsigned int reg = mc->reg;
-	unsigned int shift = mc->shift;
-	unsigned int mask = BIT(shift);
-	unsigned int invert = mc->invert != 0;
 	unsigned int strobe = ucontrol->value.enumerated.item[0] != 0;
+	unsigned int invert = mc->invert != 0;
+	unsigned int mask = BIT(mc->shift);
 	unsigned int val1 = (strobe ^ invert) ? mask : 0;
 	unsigned int val2 = (strobe ^ invert) ? 0 : mask;
-	int err;
+	int ret;
 
-	err = snd_soc_component_update_bits(component, reg, mask, val1);
-	if (err < 0)
-		return err;
+	ret = snd_soc_component_update_bits(component, mc->reg, mask, val1);
+	if (ret < 0)
+		return ret;
 
-	return snd_soc_component_update_bits(component, reg, mask, val2);
+	return snd_soc_component_update_bits(component, mc->reg, mask, val2);
 }
 EXPORT_SYMBOL_GPL(snd_soc_put_strobe);
-- 
2.39.5


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH 15/15] ASoC: ops: Apply platform_max after deciding control type
  2025-03-18 17:14 [PATCH 00/15] Tidy up ASoC control get and put handlers Charles Keepax
                   ` (13 preceding siblings ...)
  2025-03-18 17:14 ` [PATCH 14/15] ASoC: ops: Remove some unnecessary local variables Charles Keepax
@ 2025-03-18 17:14 ` Charles Keepax
  2025-03-20 18:45 ` [PATCH 00/15] Tidy up ASoC control get and put handlers Mark Brown
  15 siblings, 0 replies; 20+ messages in thread
From: Charles Keepax @ 2025-03-18 17:14 UTC (permalink / raw)
  To: broonie
  Cc: lgirdwood, shenghao-ding, kevin-lu, baojun.xu, linux-sound,
	linux-kernel, patches

It doesn't really make sense for the type of a control to change based
on the platform_max field. platform_max allows a specific system to
limit values of a control for safety but it seems reasonable the
control type should remain the same between different systems, even
if it is restricted down to just two values. Move the application of
platform_max to after control type determination in soc_info_volsw().

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 sound/soc/soc-ops.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/sound/soc/soc-ops.c b/sound/soc/soc-ops.c
index 3ac5b3a62c812..8d4dd11c9aef1 100644
--- a/sound/soc/soc-ops.c
+++ b/sound/soc/soc-ops.c
@@ -172,9 +172,6 @@ static int soc_info_volsw(struct snd_kcontrol *kcontrol,
 			  struct snd_ctl_elem_info *uinfo,
 			  struct soc_mixer_control *mc, int max)
 {
-	if (mc->platform_max && mc->platform_max < max)
-		max = mc->platform_max;
-
 	uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER;
 
 	if (max == 1) {
@@ -185,6 +182,9 @@ static int soc_info_volsw(struct snd_kcontrol *kcontrol,
 			uinfo->type = SNDRV_CTL_ELEM_TYPE_BOOLEAN;
 	}
 
+	if (mc->platform_max && mc->platform_max < max)
+		max = mc->platform_max;
+
 	uinfo->count = snd_soc_volsw_is_stereo(mc) ? 2 : 1;
 	uinfo->value.integer.min = 0;
 	uinfo->value.integer.max = max;
-- 
2.39.5


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH 13/15] ASoC: ops: Factor out common code from get callbacks
  2025-03-18 17:14 ` [PATCH 13/15] ASoC: ops: Factor out common code from get callbacks Charles Keepax
@ 2025-03-19 15:12   ` Mark Brown
  2025-03-19 16:04     ` Charles Keepax
  0 siblings, 1 reply; 20+ messages in thread
From: Mark Brown @ 2025-03-19 15:12 UTC (permalink / raw)
  To: Charles Keepax
  Cc: lgirdwood, shenghao-ding, kevin-lu, baojun.xu, linux-sound,
	linux-kernel, patches

[-- Attachment #1: Type: text/plain, Size: 889 bytes --]

On Tue, Mar 18, 2025 at 05:14:57PM +0000, Charles Keepax wrote:
> There are only two differences between snd_soc_get_volsw() and
> snd_soc_get_volsw_sx(). The maximum field is handled differently, and
> snd_soc_get_volsw() supports double controls with both values in the
> same register.

This breaks an x86 allmodconfig build:

/build/stage/linux/sound/soc/soc-ops.c: In function ‘snd_soc_get_volsw_sx’:
/build/stage/linux/sound/soc/soc-ops.c:374:22: error: unused variable ‘reg2’ [-W
error=unused-variable]
  374 |         unsigned int reg2 = mc->rreg;
      |                      ^~~~
/build/stage/linux/sound/soc/soc-ops.c:373:22: error: unused variable ‘reg’ [-We
rror=unused-variable]
  373 |         unsigned int reg = mc->reg;
      |                      ^~~
cc1: all warnings being treated as errors

Please ensure your serieses are bisectable.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 13/15] ASoC: ops: Factor out common code from get callbacks
  2025-03-19 15:12   ` Mark Brown
@ 2025-03-19 16:04     ` Charles Keepax
  2025-03-19 16:08       ` Mark Brown
  0 siblings, 1 reply; 20+ messages in thread
From: Charles Keepax @ 2025-03-19 16:04 UTC (permalink / raw)
  To: Mark Brown
  Cc: lgirdwood, shenghao-ding, kevin-lu, baojun.xu, linux-sound,
	linux-kernel, patches

On Wed, Mar 19, 2025 at 03:12:56PM +0000, Mark Brown wrote:
> On Tue, Mar 18, 2025 at 05:14:57PM +0000, Charles Keepax wrote:
> > There are only two differences between snd_soc_get_volsw() and
> > snd_soc_get_volsw_sx(). The maximum field is handled differently, and
> > snd_soc_get_volsw() supports double controls with both values in the
> > same register.
> 
> This breaks an x86 allmodconfig build:
> 
> /build/stage/linux/sound/soc/soc-ops.c: In function ‘snd_soc_get_volsw_sx’:
> /build/stage/linux/sound/soc/soc-ops.c:374:22: error: unused variable ‘reg2’ [-W
> error=unused-variable]
>   374 |         unsigned int reg2 = mc->rreg;
>       |                      ^~~~
> /build/stage/linux/sound/soc/soc-ops.c:373:22: error: unused variable ‘reg’ [-We
> rror=unused-variable]
>   373 |         unsigned int reg = mc->reg;
>       |                      ^~~
> cc1: all warnings being treated as errors
> 
> Please ensure your serieses are bisectable.

Sorry those do get cleaned up in the next patch but will respin
to move them into this patch, must have got muddled in the rebase
at some point.

Thanks,
Charles

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 13/15] ASoC: ops: Factor out common code from get callbacks
  2025-03-19 16:04     ` Charles Keepax
@ 2025-03-19 16:08       ` Mark Brown
  0 siblings, 0 replies; 20+ messages in thread
From: Mark Brown @ 2025-03-19 16:08 UTC (permalink / raw)
  To: Charles Keepax
  Cc: lgirdwood, shenghao-ding, kevin-lu, baojun.xu, linux-sound,
	linux-kernel, patches

[-- Attachment #1: Type: text/plain, Size: 353 bytes --]

On Wed, Mar 19, 2025 at 04:04:57PM +0000, Charles Keepax wrote:

> Sorry those do get cleaned up in the next patch but will respin
> to move them into this patch, must have got muddled in the rebase
> at some point.

I've still got all the prior patches queued so only this and following
patches need a resend unless runtime testing turns something up.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 00/15] Tidy up ASoC control get and put handlers
  2025-03-18 17:14 [PATCH 00/15] Tidy up ASoC control get and put handlers Charles Keepax
                   ` (14 preceding siblings ...)
  2025-03-18 17:14 ` [PATCH 15/15] ASoC: ops: Apply platform_max after deciding control type Charles Keepax
@ 2025-03-20 18:45 ` Mark Brown
  15 siblings, 0 replies; 20+ messages in thread
From: Mark Brown @ 2025-03-20 18:45 UTC (permalink / raw)
  To: Charles Keepax
  Cc: lgirdwood, shenghao-ding, kevin-lu, baojun.xu, linux-sound,
	linux-kernel, patches

On Tue, 18 Mar 2025 17:14:44 +0000, Charles Keepax wrote:
> There is a lot of duplicated and occasionally slightly incorrect code
> around the ASoC control get and put handlers. This series add some kunit
> tests and then refactors the code to get all the tests passing and
> reduce some of the duplication. The focus here is on the volsw handlers,
> future work could still be done on some of the others but these were the
> ones that most required attention.
> 
> [...]

Applied to

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

Thanks!

[01/15] ASoC: ops-test: Add some basic kunit tests for soc-ops
        commit: 7a24b876ad8cdd56457e881d384a038922b508c3
[02/15] ASoC: ops: Minor formatting fixups
        commit: 534bfb330b2612199b2afaafc769ccb42bebb204
[03/15] ASoC: ops: Update comments for xr_sx control helpers
        commit: 7f978180673b4f3b68fcc66fc1f1d74a1fc5a93a
[04/15] ASoC: ops: Update mask generation to use GENMASK
        commit: c6002c1177cafb4462b6c188d2a62eb67f15165f
[05/15] ASoC: ops: Factor out helper to check valid control values
        commit: eeb76cb1fa0dcccf33091b04ba150076dfbeb6fd
[06/15] ASoC: ops: Replace snd_soc_read_signed() with new helper
        commit: 1522aacd0114069b7f01f047b9e5b159399af295
[07/15] ASoC: ops: Add control to register value helper
        commit: ed336066202c02f0b0e47d0cf08fd8f40a42351f
[08/15] ASoC: ops: Remove snd_soc_info_volsw_range()
        commit: 894a37c9de4b8a447ffa609d552e91ccb447c0a9
[09/15] ASoC: ops: Remove snd_soc_get_volsw_range()
        commit: fd7442561cfe9516b37cdb1d229dc1f811dc86cc
[10/15] ASoC: ops: Remove snd_soc_put_volsw_range()
        commit: 7d5df968f95cee274740d5fa42e0798ffb59bd38
[11/15] ASoC: ops: Factor out common code from info callbacks
        commit: 9dfcafe2037acc14265cead8d8a937a8bc4e01d8
[12/15] ASoC: ops: Factor out common code from put callbacks
        commit: 318e8794e05ca1879441a602e78c74f9d7e18309
[13/15] ASoC: ops: Factor out common code from get callbacks
        commit: 1e3cd64a29baa874b89180ac0744178ecb00f3cd
[14/15] ASoC: ops: Remove some unnecessary local variables
        commit: 94dfe71f0a4eb0d7df542560c22961dedf45141d
[15/15] ASoC: ops: Apply platform_max after deciding control type
        commit: 502a668fad12b6ca10bcbb615d62e61d3b669c99

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


^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2025-03-20 18:45 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-03-18 17:14 [PATCH 00/15] Tidy up ASoC control get and put handlers Charles Keepax
2025-03-18 17:14 ` [PATCH 01/15] ASoC: ops-test: Add some basic kunit tests for soc-ops Charles Keepax
2025-03-18 17:14 ` [PATCH 02/15] ASoC: ops: Minor formatting fixups Charles Keepax
2025-03-18 17:14 ` [PATCH 03/15] ASoC: ops: Update comments for xr_sx control helpers Charles Keepax
2025-03-18 17:14 ` [PATCH 04/15] ASoC: ops: Update mask generation to use GENMASK Charles Keepax
2025-03-18 17:14 ` [PATCH 05/15] ASoC: ops: Factor out helper to check valid control values Charles Keepax
2025-03-18 17:14 ` [PATCH 06/15] ASoC: ops: Replace snd_soc_read_signed() with new helper Charles Keepax
2025-03-18 17:14 ` [PATCH 07/15] ASoC: ops: Add control to register value helper Charles Keepax
2025-03-18 17:14 ` [PATCH 08/15] ASoC: ops: Remove snd_soc_info_volsw_range() Charles Keepax
2025-03-18 17:14 ` [PATCH 09/15] ASoC: ops: Remove snd_soc_get_volsw_range() Charles Keepax
2025-03-18 17:14 ` [PATCH 10/15] ASoC: ops: Remove snd_soc_put_volsw_range() Charles Keepax
2025-03-18 17:14 ` [PATCH 11/15] ASoC: ops: Factor out common code from info callbacks Charles Keepax
2025-03-18 17:14 ` [PATCH 12/15] ASoC: ops: Factor out common code from put callbacks Charles Keepax
2025-03-18 17:14 ` [PATCH 13/15] ASoC: ops: Factor out common code from get callbacks Charles Keepax
2025-03-19 15:12   ` Mark Brown
2025-03-19 16:04     ` Charles Keepax
2025-03-19 16:08       ` Mark Brown
2025-03-18 17:14 ` [PATCH 14/15] ASoC: ops: Remove some unnecessary local variables Charles Keepax
2025-03-18 17:14 ` [PATCH 15/15] ASoC: ops: Apply platform_max after deciding control type Charles Keepax
2025-03-20 18:45 ` [PATCH 00/15] Tidy up ASoC control get and put handlers Mark Brown

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox