[PATCH 0/2] ALSA: hda: cs35l56: Handle speaker id GPIOs

[PATCH 0/2] ALSA: hda: cs35l56: Handle speaker id GPIOs

[Richard Fitzgerald]

Some manufacturers use multiple sources of speakers. Motherboard
GPIOs are set to indicate which type of speaker is fitted so that
the correct tunings can be loaded. Patch #1 adds support for this
and patch #2 adds a KUnit test for the new code.

Richard Fitzgerald (2):
  ALSA: hda: cs35l56: Add support for speaker id
  ALSA: hda: cirrus_scodec: Add KUnit test

 MAINTAINERS                        |   1 +
 sound/pci/hda/Kconfig              |  17 ++
 sound/pci/hda/Makefile             |   4 +
 sound/pci/hda/cirrus_scodec.c      |  73 ++++++
 sound/pci/hda/cirrus_scodec.h      |  13 +
 sound/pci/hda/cirrus_scodec_test.c | 370 +++++++++++++++++++++++++++++
 sound/pci/hda/cs35l56_hda.c        |  24 +-
 7 files changed, 501 insertions(+), 1 deletion(-)
 create mode 100644 sound/pci/hda/cirrus_scodec.c
 create mode 100644 sound/pci/hda/cirrus_scodec.h
 create mode 100644 sound/pci/hda/cirrus_scodec_test.c

-- 
2.30.2

[PATCH 1/2] ALSA: hda: cs35l56: Add support for speaker id

[Richard Fitzgerald]

Add handling of the "spk-id-gpios" _DSD property. If present, the
value indicated by the GPIOs is appended to the subsystem-id
part of the firmware name to load the appropriate tunings for that
speaker.

Some manufacturers use multiple sources of speakers, which need
different tunings for best performance. On these models the type
of speaker fitted is indicated by the values of one or more GPIOs.
The number formed by the GPIOs identifies the tuning required.

The speaker ID is only used in combination with a _SUB identifier
because the value is only meaningful if the exact model is known.

The code to get the speaker ID value has been implemented as a
new library so that the cs35l41_hda driver can be switched in
future to share common code. This library can be extended for
other common functionality shared by Cirrus Logic amp drivers.

Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
 MAINTAINERS                   |  1 +
 sound/pci/hda/Kconfig         |  5 +++
 sound/pci/hda/Makefile        |  2 +
 sound/pci/hda/cirrus_scodec.c | 73 +++++++++++++++++++++++++++++++++++
 sound/pci/hda/cirrus_scodec.h | 13 +++++++
 sound/pci/hda/cs35l56_hda.c   | 14 ++++++-
 6 files changed, 107 insertions(+), 1 deletion(-)
 create mode 100644 sound/pci/hda/cirrus_scodec.c
 create mode 100644 sound/pci/hda/cirrus_scodec.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 90f13281d297..23e73d19f347 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4912,6 +4912,7 @@ F:	drivers/spi/spi-cs42l43*
 F:	include/dt-bindings/sound/cs*
 F:	include/linux/mfd/cs42l43*
 F:	include/sound/cs*
+F:	sound/pci/hda/cirrus*
 F:	sound/pci/hda/cs*
 F:	sound/pci/hda/hda_cs_dsp_ctl.*
 F:	sound/soc/codecs/cs*
diff --git a/sound/pci/hda/Kconfig b/sound/pci/hda/Kconfig
index 0d7502d6e060..2980bfef0a4c 100644
--- a/sound/pci/hda/Kconfig
+++ b/sound/pci/hda/Kconfig
@@ -91,6 +91,9 @@ config SND_HDA_PATCH_LOADER
 	  start up.  The "patch" file can be specified via patch module
 	  option, such as patch=hda-init.
 
+config SND_HDA_CIRRUS_SCODEC
+	tristate
+
 config SND_HDA_SCODEC_CS35L41
 	tristate
 	select SND_HDA_GENERIC
@@ -144,6 +147,7 @@ config SND_HDA_SCODEC_CS35L56_I2C
 	select SND_HDA_GENERIC
 	select SND_SOC_CS35L56_SHARED
 	select SND_HDA_SCODEC_CS35L56
+	select SND_HDA_CIRRUS_SCODEC
 	select SND_HDA_CS_DSP_CONTROLS
 	help
 	  Say Y or M here to include CS35L56 amplifier support with
@@ -158,6 +162,7 @@ config SND_HDA_SCODEC_CS35L56_SPI
 	select SND_HDA_GENERIC
 	select SND_SOC_CS35L56_SHARED
 	select SND_HDA_SCODEC_CS35L56
+	select SND_HDA_CIRRUS_SCODEC
 	select SND_HDA_CS_DSP_CONTROLS
 	help
 	  Say Y or M here to include CS35L56 amplifier support with
diff --git a/sound/pci/hda/Makefile b/sound/pci/hda/Makefile
index f00fc9ed6096..aa445af0cf9a 100644
--- a/sound/pci/hda/Makefile
+++ b/sound/pci/hda/Makefile
@@ -28,6 +28,7 @@ snd-hda-codec-via-objs :=	patch_via.o
 snd-hda-codec-hdmi-objs :=	patch_hdmi.o hda_eld.o
 
 # side codecs
+snd-hda-cirrus-scodec-objs :=		cirrus_scodec.o
 snd-hda-scodec-cs35l41-objs :=		cs35l41_hda.o cs35l41_hda_property.o
 snd-hda-scodec-cs35l41-i2c-objs :=	cs35l41_hda_i2c.o
 snd-hda-scodec-cs35l41-spi-objs :=	cs35l41_hda_spi.o
@@ -56,6 +57,7 @@ obj-$(CONFIG_SND_HDA_CODEC_VIA) += snd-hda-codec-via.o
 obj-$(CONFIG_SND_HDA_CODEC_HDMI) += snd-hda-codec-hdmi.o
 
 # side codecs
+obj-$(CONFIG_SND_HDA_CIRRUS_SCODEC) += snd-hda-cirrus-scodec.o
 obj-$(CONFIG_SND_HDA_SCODEC_CS35L41) += snd-hda-scodec-cs35l41.o
 obj-$(CONFIG_SND_HDA_SCODEC_CS35L41_I2C) += snd-hda-scodec-cs35l41-i2c.o
 obj-$(CONFIG_SND_HDA_SCODEC_CS35L41_SPI) += snd-hda-scodec-cs35l41-spi.o
diff --git a/sound/pci/hda/cirrus_scodec.c b/sound/pci/hda/cirrus_scodec.c
new file mode 100644
index 000000000000..8de3bc7448fa
--- /dev/null
+++ b/sound/pci/hda/cirrus_scodec.c
@@ -0,0 +1,73 @@
+// SPDX-License-Identifier: GPL-2.0-only
+//
+// Common code for Cirrus side-codecs.
+//
+// Copyright (C) 2021, 2023 Cirrus Logic, Inc. and
+//               Cirrus Logic International Semiconductor Ltd.
+
+#include <linux/dev_printk.h>
+#include <linux/gpio/consumer.h>
+#include <linux/module.h>
+
+#include "cirrus_scodec.h"
+
+int cirrus_scodec_get_speaker_id(struct device *dev, int amp_index,
+				 int num_amps, int fixed_gpio_id)
+{
+	struct gpio_desc *speaker_id_desc;
+	int speaker_id = -ENOENT;
+
+	if (fixed_gpio_id >= 0) {
+		dev_dbg(dev, "Found Fixed Speaker ID GPIO (index = %d)\n", fixed_gpio_id);
+		speaker_id_desc = gpiod_get_index(dev, NULL, fixed_gpio_id, GPIOD_IN);
+		if (IS_ERR(speaker_id_desc)) {
+			speaker_id = PTR_ERR(speaker_id_desc);
+			return speaker_id;
+		}
+		speaker_id = gpiod_get_value_cansleep(speaker_id_desc);
+		gpiod_put(speaker_id_desc);
+	} else {
+		int base_index;
+		int gpios_per_amp;
+		int count;
+		int tmp;
+		int i;
+
+		count = gpiod_count(dev, "spk-id");
+		if (count > 0) {
+			speaker_id = 0;
+			gpios_per_amp = count / num_amps;
+			base_index = gpios_per_amp * amp_index;
+
+			if (count % num_amps)
+				return -EINVAL;
+
+			dev_dbg(dev, "Found %d Speaker ID GPIOs per Amp\n", gpios_per_amp);
+
+			for (i = 0; i < gpios_per_amp; i++) {
+				speaker_id_desc = gpiod_get_index(dev, "spk-id", i + base_index,
+								  GPIOD_IN);
+				if (IS_ERR(speaker_id_desc)) {
+					speaker_id = PTR_ERR(speaker_id_desc);
+					break;
+				}
+				tmp = gpiod_get_value_cansleep(speaker_id_desc);
+				gpiod_put(speaker_id_desc);
+				if (tmp < 0) {
+					speaker_id = tmp;
+					break;
+				}
+				speaker_id |= tmp << i;
+			}
+		}
+	}
+
+	dev_dbg(dev, "Speaker ID = %d\n", speaker_id);
+
+	return speaker_id;
+}
+EXPORT_SYMBOL_NS_GPL(cirrus_scodec_get_speaker_id, SND_HDA_CIRRUS_SCODEC);
+
+MODULE_DESCRIPTION("HDA Cirrus side-codec library");
+MODULE_AUTHOR("Richard Fitzgerald <rf@opensource.cirrus.com>");
+MODULE_LICENSE("GPL");
diff --git a/sound/pci/hda/cirrus_scodec.h b/sound/pci/hda/cirrus_scodec.h
new file mode 100644
index 000000000000..ba2041d8ef24
--- /dev/null
+++ b/sound/pci/hda/cirrus_scodec.h
@@ -0,0 +1,13 @@
+/* SPDX-License-Identifier: GPL-2.0
+ *
+ * Copyright (C) 2023 Cirrus Logic, Inc. and
+ *                    Cirrus Logic International Semiconductor Ltd.
+ */
+
+#ifndef CIRRUS_SCODEC_H
+#define CIRRUS_SCODEC_H
+
+int cirrus_scodec_get_speaker_id(struct device *dev, int amp_index,
+				 int num_amps, int fixed_gpio_id);
+
+#endif /* CIRRUS_SCODEC_H */
diff --git a/sound/pci/hda/cs35l56_hda.c b/sound/pci/hda/cs35l56_hda.c
index 87ffe8fbff99..44f5ca0e73e3 100644
--- a/sound/pci/hda/cs35l56_hda.c
+++ b/sound/pci/hda/cs35l56_hda.c
@@ -16,6 +16,7 @@
 #include <sound/core.h>
 #include <sound/hda_codec.h>
 #include <sound/tlv.h>
+#include "cirrus_scodec.h"
 #include "cs35l56_hda.h"
 #include "hda_component.h"
 #include "hda_cs_dsp_ctl.h"
@@ -869,7 +870,17 @@ static int cs35l56_hda_read_acpi(struct cs35l56_hda *cs35l56, int id)
 			 "Read ACPI _SUB failed(%ld): fallback to generic firmware\n",
 			 PTR_ERR(sub));
 	} else {
-		cs35l56->system_name = sub;
+		ret = cirrus_scodec_get_speaker_id(cs35l56->base.dev, cs35l56->index, nval, -1);
+		if (ret == -ENOENT) {
+			cs35l56->system_name = sub;
+		} else if (ret >= 0) {
+			cs35l56->system_name = kasprintf(GFP_KERNEL, "%s-spkid%d", sub, ret);
+			kfree(sub);
+			if (!cs35l56->system_name)
+				return -ENOMEM;
+		} else {
+			return ret;
+		}
 	}
 
 	cs35l56->base.reset_gpio = devm_gpiod_get_index_optional(cs35l56->base.dev,
@@ -1025,6 +1036,7 @@ const struct dev_pm_ops cs35l56_hda_pm_ops = {
 EXPORT_SYMBOL_NS_GPL(cs35l56_hda_pm_ops, SND_HDA_SCODEC_CS35L56);
 
 MODULE_DESCRIPTION("CS35L56 HDA Driver");
+MODULE_IMPORT_NS(SND_HDA_CIRRUS_SCODEC);
 MODULE_IMPORT_NS(SND_HDA_CS_DSP_CONTROLS);
 MODULE_IMPORT_NS(SND_SOC_CS35L56_SHARED);
 MODULE_AUTHOR("Richard Fitzgerald <rf@opensource.cirrus.com>");
-- 
2.30.2

[PATCH 2/2] ALSA: hda: cirrus_scodec: Add KUnit test

[Richard Fitzgerald]

Add a KUnit test for cirrus_scodec_get_speaker_id(). It is impractical
to have enough hardware with every possible permutation of speaker id.
So use a test harness to test all theoretically supported options.

The test harness consists of:
- a mock GPIO controller.
- a mock struct device to represent the scodec driver
- software nodes to provide the fwnode info that would normally come
  from ACPI.

Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
 sound/pci/hda/Kconfig              |  12 +
 sound/pci/hda/Makefile             |   2 +
 sound/pci/hda/cirrus_scodec_test.c | 370 +++++++++++++++++++++++++++++
 sound/pci/hda/cs35l56_hda.c        |  10 +
 4 files changed, 394 insertions(+)
 create mode 100644 sound/pci/hda/cirrus_scodec_test.c

diff --git a/sound/pci/hda/Kconfig b/sound/pci/hda/Kconfig
index 2980bfef0a4c..706cdc589e6f 100644
--- a/sound/pci/hda/Kconfig
+++ b/sound/pci/hda/Kconfig
@@ -94,6 +94,18 @@ config SND_HDA_PATCH_LOADER
 config SND_HDA_CIRRUS_SCODEC
 	tristate
 
+config SND_HDA_CIRRUS_SCODEC_KUNIT_TEST
+	tristate "KUnit test for Cirrus side-codec library" if !KUNIT_ALL_TESTS
+	select SND_HDA_CIRRUS_SCODEC
+	depends on KUNIT
+	default KUNIT_ALL_TESTS
+	help
+	  This builds KUnit tests for the cirrus side-codec library.
+	  For more information on KUnit and unit tests in general,
+	  please refer to the KUnit documentation in
+	  Documentation/dev-tools/kunit/.
+	  If in doubt, say "N".
+
 config SND_HDA_SCODEC_CS35L41
 	tristate
 	select SND_HDA_GENERIC
diff --git a/sound/pci/hda/Makefile b/sound/pci/hda/Makefile
index aa445af0cf9a..793e296c3f64 100644
--- a/sound/pci/hda/Makefile
+++ b/sound/pci/hda/Makefile
@@ -29,6 +29,7 @@ snd-hda-codec-hdmi-objs :=	patch_hdmi.o hda_eld.o
 
 # side codecs
 snd-hda-cirrus-scodec-objs :=		cirrus_scodec.o
+snd-hda-cirrus-scodec-test-objs :=	cirrus_scodec_test.o
 snd-hda-scodec-cs35l41-objs :=		cs35l41_hda.o cs35l41_hda_property.o
 snd-hda-scodec-cs35l41-i2c-objs :=	cs35l41_hda_i2c.o
 snd-hda-scodec-cs35l41-spi-objs :=	cs35l41_hda_spi.o
@@ -58,6 +59,7 @@ obj-$(CONFIG_SND_HDA_CODEC_HDMI) += snd-hda-codec-hdmi.o
 
 # side codecs
 obj-$(CONFIG_SND_HDA_CIRRUS_SCODEC) += snd-hda-cirrus-scodec.o
+obj-$(CONFIG_SND_HDA_CIRRUS_SCODEC_KUNIT_TEST) += snd-hda-cirrus-scodec-test.o
 obj-$(CONFIG_SND_HDA_SCODEC_CS35L41) += snd-hda-scodec-cs35l41.o
 obj-$(CONFIG_SND_HDA_SCODEC_CS35L41_I2C) += snd-hda-scodec-cs35l41-i2c.o
 obj-$(CONFIG_SND_HDA_SCODEC_CS35L41_SPI) += snd-hda-scodec-cs35l41-spi.o
diff --git a/sound/pci/hda/cirrus_scodec_test.c b/sound/pci/hda/cirrus_scodec_test.c
new file mode 100644
index 000000000000..5eb590cd4fe2
--- /dev/null
+++ b/sound/pci/hda/cirrus_scodec_test.c
@@ -0,0 +1,370 @@
+// SPDX-License-Identifier: GPL-2.0-only
+//
+// KUnit test for the Cirrus side-codec library.
+//
+// Copyright (C) 2023 Cirrus Logic, Inc. and
+//                    Cirrus Logic International Semiconductor Ltd.
+
+#include <kunit/test.h>
+#include <linux/gpio/driver.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+#include "cirrus_scodec.h"
+
+struct cirrus_scodec_test_gpio {
+	unsigned int pin_state;
+	struct gpio_chip chip;
+};
+
+struct cirrus_scodec_test_priv {
+	struct platform_device amp_pdev;
+	struct platform_device *gpio_pdev;
+	struct cirrus_scodec_test_gpio *gpio_priv;
+};
+
+static int cirrus_scodec_test_gpio_get_direction(struct gpio_chip *chip,
+						 unsigned int offset)
+{
+	return GPIO_LINE_DIRECTION_IN;
+}
+
+static int cirrus_scodec_test_gpio_direction_in(struct gpio_chip *chip,
+						unsigned int offset)
+{
+	return 0;
+}
+
+static int cirrus_scodec_test_gpio_get(struct gpio_chip *chip, unsigned int offset)
+{
+	struct cirrus_scodec_test_gpio *gpio_priv = gpiochip_get_data(chip);
+
+	return !!(gpio_priv->pin_state & BIT(offset));
+}
+
+static int cirrus_scodec_test_gpio_direction_out(struct gpio_chip *chip,
+						 unsigned int offset, int value)
+{
+	return -EOPNOTSUPP;
+}
+
+static void cirrus_scodec_test_gpio_set(struct gpio_chip *chip, unsigned int offset,
+					int value)
+{
+}
+
+static int cirrus_scodec_test_gpio_set_config(struct gpio_chip *gc,
+					      unsigned int offset,
+					      unsigned long config)
+{
+	switch (pinconf_to_config_param(config)) {
+	case PIN_CONFIG_OUTPUT:
+	case PIN_CONFIG_OUTPUT_ENABLE:
+		return -EOPNOTSUPP;
+	default:
+		return 0;
+	}
+}
+
+static const struct gpio_chip cirrus_scodec_test_gpio_chip = {
+	.label			= "cirrus_scodec_test_gpio",
+	.owner			= THIS_MODULE,
+	.request		= gpiochip_generic_request,
+	.free			= gpiochip_generic_free,
+	.get_direction		= cirrus_scodec_test_gpio_get_direction,
+	.direction_input	= cirrus_scodec_test_gpio_direction_in,
+	.get			= cirrus_scodec_test_gpio_get,
+	.direction_output	= cirrus_scodec_test_gpio_direction_out,
+	.set			= cirrus_scodec_test_gpio_set,
+	.set_config		= cirrus_scodec_test_gpio_set_config,
+	.base			= -1,
+	.ngpio			= 32,
+};
+
+static int cirrus_scodec_test_gpio_probe(struct platform_device *pdev)
+{
+	struct cirrus_scodec_test_gpio *gpio_priv;
+	int ret;
+
+	gpio_priv = devm_kzalloc(&pdev->dev, sizeof(*gpio_priv), GFP_KERNEL);
+	if (!gpio_priv)
+		return -ENOMEM;
+
+	/* GPIO core modifies our struct gpio_chip so use a copy */
+	gpio_priv->chip = cirrus_scodec_test_gpio_chip;
+	ret = devm_gpiochip_add_data(&pdev->dev, &gpio_priv->chip, gpio_priv);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret, "Failed to add gpiochip\n");
+
+	dev_set_drvdata(&pdev->dev, gpio_priv);
+
+	return 0;
+}
+
+static struct platform_driver cirrus_scodec_test_gpio_driver = {
+	.driver.name	= "cirrus_scodec_test_gpio_drv",
+	.probe		= cirrus_scodec_test_gpio_probe,
+};
+
+/* software_node referencing the gpio driver */
+static const struct software_node cirrus_scodec_test_gpio_swnode = {
+	.name = "cirrus_scodec_test_gpio",
+};
+
+static int cirrus_scodec_test_create_gpio(struct kunit *test)
+{
+	struct cirrus_scodec_test_priv *priv = test->priv;
+	int ret;
+
+	priv->gpio_pdev = platform_device_alloc(cirrus_scodec_test_gpio_driver.driver.name, -1);
+	if (!priv->gpio_pdev)
+		return -ENOMEM;
+
+	ret = device_add_software_node(&priv->gpio_pdev->dev, &cirrus_scodec_test_gpio_swnode);
+	if (ret) {
+		platform_device_put(priv->gpio_pdev);
+		KUNIT_FAIL(test, "Failed to add swnode to gpio: %d\n", ret);
+		return ret;
+	}
+
+	ret = platform_device_add(priv->gpio_pdev);
+	if (ret) {
+		platform_device_put(priv->gpio_pdev);
+		KUNIT_FAIL(test, "Failed to add gpio platform device: %d\n", ret);
+		return ret;
+	}
+
+	priv->gpio_priv = dev_get_drvdata(&priv->gpio_pdev->dev);
+	if (!priv->gpio_priv) {
+		platform_device_put(priv->gpio_pdev);
+		KUNIT_FAIL(test, "Failed to get gpio private data: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void cirrus_scodec_test_set_gpio_ref_arg(struct software_node_ref_args *arg,
+						int gpio_num)
+{
+	struct software_node_ref_args template =
+		SOFTWARE_NODE_REFERENCE(&cirrus_scodec_test_gpio_swnode, gpio_num, 0);
+
+	*arg = template;
+}
+
+static int cirrus_scodec_test_set_spkid_swnode(struct kunit *test,
+					       struct device *dev,
+					       struct software_node_ref_args *args,
+					       int num_args)
+{
+	const struct property_entry props_template[] = {
+		PROPERTY_ENTRY_REF_ARRAY_LEN("spk-id-gpios", args, num_args),
+		{ }
+	};
+	struct property_entry *props;
+	struct software_node *node;
+
+	node = kunit_kzalloc(test, sizeof(*node), GFP_KERNEL);
+	if (!node)
+		return -ENOMEM;
+
+	props = kunit_kzalloc(test, sizeof(props_template), GFP_KERNEL);
+	if (!props)
+		return -ENOMEM;
+
+	memcpy(props, props_template, sizeof(props_template));
+	node->properties = props;
+
+	return device_add_software_node(dev, node);
+}
+
+struct cirrus_scodec_test_spkid_param {
+	int num_amps;
+	int gpios_per_amp;
+	int num_amps_sharing;
+};
+
+static void cirrus_scodec_test_spkid_parse(struct kunit *test)
+{
+	struct cirrus_scodec_test_priv *priv = test->priv;
+	const struct cirrus_scodec_test_spkid_param *param = test->param_value;
+	int num_spk_id_refs = param->num_amps * param->gpios_per_amp;
+	struct software_node_ref_args *refs;
+	struct device *dev = &priv->amp_pdev.dev;
+	unsigned int v;
+	int i, ret;
+
+	refs = kunit_kcalloc(test, num_spk_id_refs, sizeof(*refs), GFP_KERNEL);
+	KUNIT_ASSERT_NOT_NULL(test, refs);
+
+	for (i = 0, v = 0; i < num_spk_id_refs; ) {
+		cirrus_scodec_test_set_gpio_ref_arg(&refs[i++], v++);
+
+		/*
+		 * If amps are sharing GPIOs repeat the last set of
+		 * GPIOs until we've done that number of amps.
+		 * We have done all GPIOs for an amp when i is a multiple
+		 * of gpios_per_amp.
+		 * We have done all amps sharing the same GPIOs when i is
+		 * a multiple of (gpios_per_amp * num_amps_sharing).
+		 */
+		if (!(i % param->gpios_per_amp) &&
+		    (i % (param->gpios_per_amp * param->num_amps_sharing)))
+			v -= param->gpios_per_amp;
+	}
+
+	ret = cirrus_scodec_test_set_spkid_swnode(test, dev, refs, num_spk_id_refs);
+	KUNIT_EXPECT_EQ_MSG(test, ret, 0, "Failed to add swnode\n");
+
+	for (i = 0; i < param->num_amps; ++i) {
+		for (v = 0; v < (1 << param->gpios_per_amp); ++v) {
+			/* Set only the GPIO bits used by this amp */
+			priv->gpio_priv->pin_state =
+				v << (param->gpios_per_amp * (i / param->num_amps_sharing));
+
+			ret = cirrus_scodec_get_speaker_id(dev, i, param->num_amps, -1);
+			KUNIT_EXPECT_EQ_MSG(test, ret, v,
+					    "get_speaker_id failed amp:%d pin_state:%#x\n",
+					    i, priv->gpio_priv->pin_state);
+		}
+	}
+}
+
+static void cirrus_scodec_test_no_spkid(struct kunit *test)
+{
+	struct cirrus_scodec_test_priv *priv = test->priv;
+	struct device *dev = &priv->amp_pdev.dev;
+	int ret;
+
+	ret = cirrus_scodec_get_speaker_id(dev, 0, 4, -1);
+	KUNIT_EXPECT_EQ(test, ret, -ENOENT);
+}
+
+static void cirrus_scodec_test_dev_release(struct device *dev)
+{
+}
+
+static int cirrus_scodec_test_case_init(struct kunit *test)
+{
+	struct cirrus_scodec_test_priv *priv;
+	int ret;
+
+	priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	test->priv = priv;
+
+	/* Create dummy GPIO */
+	ret = cirrus_scodec_test_create_gpio(test);
+	if (ret < 0)
+		return ret;
+
+	/* Create dummy amp driver dev */
+	priv->amp_pdev.name = "cirrus_scodec_test_amp_drv";
+	priv->amp_pdev.id = -1;
+	priv->amp_pdev.dev.release = cirrus_scodec_test_dev_release;
+	ret = platform_device_register(&priv->amp_pdev);
+	KUNIT_ASSERT_GE_MSG(test, ret, 0, "Failed to register amp platform device\n");
+
+	return 0;
+}
+
+static void cirrus_scodec_test_case_exit(struct kunit *test)
+{
+	struct cirrus_scodec_test_priv *priv = test->priv;
+
+	if (priv->amp_pdev.name)
+		platform_device_unregister(&priv->amp_pdev);
+
+	if (priv->gpio_pdev) {
+		device_remove_software_node(&priv->gpio_pdev->dev);
+		platform_device_unregister(priv->gpio_pdev);
+	}
+}
+
+static int cirrus_scodec_test_suite_init(struct kunit_suite *suite)
+{
+	int ret;
+
+	/* Register mock GPIO driver */
+	ret = platform_driver_register(&cirrus_scodec_test_gpio_driver);
+	if (ret < 0) {
+		kunit_err(suite, "Failed to register gpio platform driver, %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void cirrus_scodec_test_suite_exit(struct kunit_suite *suite)
+{
+	platform_driver_unregister(&cirrus_scodec_test_gpio_driver);
+}
+
+static const struct cirrus_scodec_test_spkid_param cirrus_scodec_test_spkid_param_cases[] = {
+	{ .num_amps = 2, .gpios_per_amp = 1, .num_amps_sharing = 1 },
+	{ .num_amps = 2, .gpios_per_amp = 2, .num_amps_sharing = 1 },
+	{ .num_amps = 2, .gpios_per_amp = 3, .num_amps_sharing = 1 },
+	{ .num_amps = 2, .gpios_per_amp = 4, .num_amps_sharing = 1 },
+	{ .num_amps = 3, .gpios_per_amp = 1, .num_amps_sharing = 1 },
+	{ .num_amps = 3, .gpios_per_amp = 2, .num_amps_sharing = 1 },
+	{ .num_amps = 3, .gpios_per_amp = 3, .num_amps_sharing = 1 },
+	{ .num_amps = 3, .gpios_per_amp = 4, .num_amps_sharing = 1 },
+	{ .num_amps = 4, .gpios_per_amp = 1, .num_amps_sharing = 1 },
+	{ .num_amps = 4, .gpios_per_amp = 2, .num_amps_sharing = 1 },
+	{ .num_amps = 4, .gpios_per_amp = 3, .num_amps_sharing = 1 },
+	{ .num_amps = 4, .gpios_per_amp = 4, .num_amps_sharing = 1 },
+
+	/* Same GPIO shared by all amps */
+	{ .num_amps = 2, .gpios_per_amp = 1, .num_amps_sharing = 2 },
+	{ .num_amps = 2, .gpios_per_amp = 2, .num_amps_sharing = 2 },
+	{ .num_amps = 2, .gpios_per_amp = 3, .num_amps_sharing = 2 },
+	{ .num_amps = 2, .gpios_per_amp = 4, .num_amps_sharing = 2 },
+	{ .num_amps = 3, .gpios_per_amp = 1, .num_amps_sharing = 3 },
+	{ .num_amps = 3, .gpios_per_amp = 2, .num_amps_sharing = 3 },
+	{ .num_amps = 3, .gpios_per_amp = 3, .num_amps_sharing = 3 },
+	{ .num_amps = 3, .gpios_per_amp = 4, .num_amps_sharing = 3 },
+	{ .num_amps = 4, .gpios_per_amp = 1, .num_amps_sharing = 4 },
+	{ .num_amps = 4, .gpios_per_amp = 2, .num_amps_sharing = 4 },
+	{ .num_amps = 4, .gpios_per_amp = 3, .num_amps_sharing = 4 },
+	{ .num_amps = 4, .gpios_per_amp = 4, .num_amps_sharing = 4 },
+
+	/* Two sets of shared GPIOs */
+	{ .num_amps = 4, .gpios_per_amp = 1, .num_amps_sharing = 2 },
+	{ .num_amps = 4, .gpios_per_amp = 2, .num_amps_sharing = 2 },
+	{ .num_amps = 4, .gpios_per_amp = 3, .num_amps_sharing = 2 },
+	{ .num_amps = 4, .gpios_per_amp = 4, .num_amps_sharing = 2 },
+};
+
+static void cirrus_scodec_test_spkid_param_desc(const struct cirrus_scodec_test_spkid_param *param,
+						char *desc)
+{
+	snprintf(desc, KUNIT_PARAM_DESC_SIZE, "amps:%d gpios_per_amp:%d num_amps_sharing:%d",
+		 param->num_amps, param->gpios_per_amp, param->num_amps_sharing);
+}
+
+KUNIT_ARRAY_PARAM(cirrus_scodec_test_spkid, cirrus_scodec_test_spkid_param_cases,
+		  cirrus_scodec_test_spkid_param_desc);
+
+static struct kunit_case cirrus_scodec_test_cases[] = {
+	KUNIT_CASE_PARAM(cirrus_scodec_test_spkid_parse, cirrus_scodec_test_spkid_gen_params),
+	KUNIT_CASE(cirrus_scodec_test_no_spkid),
+	{ } /* terminator */
+};
+
+static struct kunit_suite cirrus_scodec_test_suite = {
+	.name = "snd-hda-scodec-cs35l56-test",
+	.suite_init = cirrus_scodec_test_suite_init,
+	.suite_exit = cirrus_scodec_test_suite_exit,
+	.init = cirrus_scodec_test_case_init,
+	.exit = cirrus_scodec_test_case_exit,
+	.test_cases = cirrus_scodec_test_cases,
+};
+
+kunit_test_suite(cirrus_scodec_test_suite);
+
+MODULE_IMPORT_NS(SND_HDA_CIRRUS_SCODEC);
+MODULE_AUTHOR("Richard Fitzgerald <rf@opensource.cirrus.com>");
+MODULE_LICENSE("GPL");
diff --git a/sound/pci/hda/cs35l56_hda.c b/sound/pci/hda/cs35l56_hda.c
index 44f5ca0e73e3..d3cfdad7dd76 100644
--- a/sound/pci/hda/cs35l56_hda.c
+++ b/sound/pci/hda/cs35l56_hda.c
@@ -1035,6 +1035,16 @@ const struct dev_pm_ops cs35l56_hda_pm_ops = {
 };
 EXPORT_SYMBOL_NS_GPL(cs35l56_hda_pm_ops, SND_HDA_SCODEC_CS35L56);
 
+#if IS_ENABLED(CONFIG_SND_HDA_SCODEC_CS35L56_KUNIT_TEST)
+/* Hooks to export static function to KUnit test */
+
+int cs35l56_hda_test_hook_get_speaker_id(struct device *dev, int amp_index, int num_amps)
+{
+	return cs35l56_hda_get_speaker_id(dev, amp_index, num_amps);
+}
+EXPORT_SYMBOL_NS_GPL(cs35l56_hda_test_hook_get_speaker_id, SND_HDA_SCODEC_CS35L56);
+#endif
+
 MODULE_DESCRIPTION("CS35L56 HDA Driver");
 MODULE_IMPORT_NS(SND_HDA_CIRRUS_SCODEC);
 MODULE_IMPORT_NS(SND_HDA_CS_DSP_CONTROLS);
-- 
2.30.2

Re: [PATCH 0/2] ALSA: hda: cs35l56: Handle speaker id GPIOs

[Takashi Iwai]

On Mon, 18 Sep 2023 11:51:27 +0200,
Richard Fitzgerald wrote:
> 
> Some manufacturers use multiple sources of speakers. Motherboard
> GPIOs are set to indicate which type of speaker is fitted so that
> the correct tunings can be loaded. Patch #1 adds support for this
> and patch #2 adds a KUnit test for the new code.
> 
> Richard Fitzgerald (2):
>   ALSA: hda: cs35l56: Add support for speaker id
>   ALSA: hda: cirrus_scodec: Add KUnit test

Applied both patches to for-next branch now.


thanks,

Takashi

Re: [PATCH 2/2] ALSA: hda: cirrus_scodec: Add KUnit test

[Nick Desaulniers]

On Mon, Sep 18, 2023 at 10:51:29AM +0100, Richard Fitzgerald wrote:
> Add a KUnit test for cirrus_scodec_get_speaker_id(). It is impractical
> to have enough hardware with every possible permutation of speaker id.
> So use a test harness to test all theoretically supported options.
> 
> The test harness consists of:
> - a mock GPIO controller.
> - a mock struct device to represent the scodec driver
> - software nodes to provide the fwnode info that would normally come
>   from ACPI.
> 
> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
> ---
>  sound/pci/hda/Kconfig              |  12 +
>  sound/pci/hda/Makefile             |   2 +
>  sound/pci/hda/cirrus_scodec_test.c | 370 +++++++++++++++++++++++++++++
>  sound/pci/hda/cs35l56_hda.c        |  10 +
>  4 files changed, 394 insertions(+)
>  create mode 100644 sound/pci/hda/cirrus_scodec_test.c
> 
> diff --git a/sound/pci/hda/Kconfig b/sound/pci/hda/Kconfig
> index 2980bfef0a4c..706cdc589e6f 100644
> --- a/sound/pci/hda/Kconfig
> +++ b/sound/pci/hda/Kconfig
> @@ -94,6 +94,18 @@ config SND_HDA_PATCH_LOADER
>  config SND_HDA_CIRRUS_SCODEC
>  	tristate
>  
> +config SND_HDA_CIRRUS_SCODEC_KUNIT_TEST
> +	tristate "KUnit test for Cirrus side-codec library" if !KUNIT_ALL_TESTS
> +	select SND_HDA_CIRRUS_SCODEC
> +	depends on KUNIT
> +	default KUNIT_ALL_TESTS
> +	help
> +	  This builds KUnit tests for the cirrus side-codec library.
> +	  For more information on KUnit and unit tests in general,
> +	  please refer to the KUnit documentation in
> +	  Documentation/dev-tools/kunit/.
> +	  If in doubt, say "N".
> +
>  config SND_HDA_SCODEC_CS35L41
>  	tristate
>  	select SND_HDA_GENERIC
> diff --git a/sound/pci/hda/Makefile b/sound/pci/hda/Makefile
> index aa445af0cf9a..793e296c3f64 100644
> --- a/sound/pci/hda/Makefile
> +++ b/sound/pci/hda/Makefile
> @@ -29,6 +29,7 @@ snd-hda-codec-hdmi-objs :=	patch_hdmi.o hda_eld.o
>  
>  # side codecs
>  snd-hda-cirrus-scodec-objs :=		cirrus_scodec.o
> +snd-hda-cirrus-scodec-test-objs :=	cirrus_scodec_test.o
>  snd-hda-scodec-cs35l41-objs :=		cs35l41_hda.o cs35l41_hda_property.o
>  snd-hda-scodec-cs35l41-i2c-objs :=	cs35l41_hda_i2c.o
>  snd-hda-scodec-cs35l41-spi-objs :=	cs35l41_hda_spi.o
> @@ -58,6 +59,7 @@ obj-$(CONFIG_SND_HDA_CODEC_HDMI) += snd-hda-codec-hdmi.o
>  
>  # side codecs
>  obj-$(CONFIG_SND_HDA_CIRRUS_SCODEC) += snd-hda-cirrus-scodec.o
> +obj-$(CONFIG_SND_HDA_CIRRUS_SCODEC_KUNIT_TEST) += snd-hda-cirrus-scodec-test.o
>  obj-$(CONFIG_SND_HDA_SCODEC_CS35L41) += snd-hda-scodec-cs35l41.o
>  obj-$(CONFIG_SND_HDA_SCODEC_CS35L41_I2C) += snd-hda-scodec-cs35l41-i2c.o
>  obj-$(CONFIG_SND_HDA_SCODEC_CS35L41_SPI) += snd-hda-scodec-cs35l41-spi.o
> diff --git a/sound/pci/hda/cirrus_scodec_test.c b/sound/pci/hda/cirrus_scodec_test.c
> new file mode 100644
> index 000000000000..5eb590cd4fe2
> --- /dev/null
> +++ b/sound/pci/hda/cirrus_scodec_test.c
> @@ -0,0 +1,370 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +//
> +// KUnit test for the Cirrus side-codec library.
> +//
> +// Copyright (C) 2023 Cirrus Logic, Inc. and
> +//                    Cirrus Logic International Semiconductor Ltd.
> +
> +#include <kunit/test.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +
> +#include "cirrus_scodec.h"
> +
> +struct cirrus_scodec_test_gpio {
> +	unsigned int pin_state;
> +	struct gpio_chip chip;
> +};
> +
> +struct cirrus_scodec_test_priv {
> +	struct platform_device amp_pdev;
> +	struct platform_device *gpio_pdev;
> +	struct cirrus_scodec_test_gpio *gpio_priv;
> +};
> +
> +static int cirrus_scodec_test_gpio_get_direction(struct gpio_chip *chip,
> +						 unsigned int offset)
> +{
> +	return GPIO_LINE_DIRECTION_IN;
> +}
> +
> +static int cirrus_scodec_test_gpio_direction_in(struct gpio_chip *chip,
> +						unsigned int offset)
> +{
> +	return 0;
> +}
> +
> +static int cirrus_scodec_test_gpio_get(struct gpio_chip *chip, unsigned int offset)
> +{
> +	struct cirrus_scodec_test_gpio *gpio_priv = gpiochip_get_data(chip);
> +
> +	return !!(gpio_priv->pin_state & BIT(offset));
> +}
> +
> +static int cirrus_scodec_test_gpio_direction_out(struct gpio_chip *chip,
> +						 unsigned int offset, int value)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static void cirrus_scodec_test_gpio_set(struct gpio_chip *chip, unsigned int offset,
> +					int value)
> +{
> +}
> +
> +static int cirrus_scodec_test_gpio_set_config(struct gpio_chip *gc,
> +					      unsigned int offset,
> +					      unsigned long config)
> +{
> +	switch (pinconf_to_config_param(config)) {
> +	case PIN_CONFIG_OUTPUT:
> +	case PIN_CONFIG_OUTPUT_ENABLE:
> +		return -EOPNOTSUPP;
> +	default:
> +		return 0;
> +	}
> +}
> +
> +static const struct gpio_chip cirrus_scodec_test_gpio_chip = {
> +	.label			= "cirrus_scodec_test_gpio",
> +	.owner			= THIS_MODULE,
> +	.request		= gpiochip_generic_request,
> +	.free			= gpiochip_generic_free,
> +	.get_direction		= cirrus_scodec_test_gpio_get_direction,
> +	.direction_input	= cirrus_scodec_test_gpio_direction_in,
> +	.get			= cirrus_scodec_test_gpio_get,
> +	.direction_output	= cirrus_scodec_test_gpio_direction_out,
> +	.set			= cirrus_scodec_test_gpio_set,
> +	.set_config		= cirrus_scodec_test_gpio_set_config,
> +	.base			= -1,
> +	.ngpio			= 32,
> +};
> +
> +static int cirrus_scodec_test_gpio_probe(struct platform_device *pdev)
> +{
> +	struct cirrus_scodec_test_gpio *gpio_priv;
> +	int ret;
> +
> +	gpio_priv = devm_kzalloc(&pdev->dev, sizeof(*gpio_priv), GFP_KERNEL);
> +	if (!gpio_priv)
> +		return -ENOMEM;
> +
> +	/* GPIO core modifies our struct gpio_chip so use a copy */
> +	gpio_priv->chip = cirrus_scodec_test_gpio_chip;
> +	ret = devm_gpiochip_add_data(&pdev->dev, &gpio_priv->chip, gpio_priv);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "Failed to add gpiochip\n");
> +
> +	dev_set_drvdata(&pdev->dev, gpio_priv);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver cirrus_scodec_test_gpio_driver = {
> +	.driver.name	= "cirrus_scodec_test_gpio_drv",
> +	.probe		= cirrus_scodec_test_gpio_probe,
> +};
> +
> +/* software_node referencing the gpio driver */
> +static const struct software_node cirrus_scodec_test_gpio_swnode = {
> +	.name = "cirrus_scodec_test_gpio",
> +};
> +
> +static int cirrus_scodec_test_create_gpio(struct kunit *test)
> +{
> +	struct cirrus_scodec_test_priv *priv = test->priv;
> +	int ret;
> +
> +	priv->gpio_pdev = platform_device_alloc(cirrus_scodec_test_gpio_driver.driver.name, -1);
> +	if (!priv->gpio_pdev)
> +		return -ENOMEM;
> +
> +	ret = device_add_software_node(&priv->gpio_pdev->dev, &cirrus_scodec_test_gpio_swnode);
> +	if (ret) {
> +		platform_device_put(priv->gpio_pdev);
> +		KUNIT_FAIL(test, "Failed to add swnode to gpio: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = platform_device_add(priv->gpio_pdev);
> +	if (ret) {
> +		platform_device_put(priv->gpio_pdev);
> +		KUNIT_FAIL(test, "Failed to add gpio platform device: %d\n", ret);
> +		return ret;
> +	}
> +
> +	priv->gpio_priv = dev_get_drvdata(&priv->gpio_pdev->dev);
> +	if (!priv->gpio_priv) {
> +		platform_device_put(priv->gpio_pdev);
> +		KUNIT_FAIL(test, "Failed to get gpio private data: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void cirrus_scodec_test_set_gpio_ref_arg(struct software_node_ref_args *arg,
> +						int gpio_num)
> +{
> +	struct software_node_ref_args template =
> +		SOFTWARE_NODE_REFERENCE(&cirrus_scodec_test_gpio_swnode, gpio_num, 0);

I'm observing the following error when building with:

$ make LLVM=1 -j128 allmodconfig sound/pci/hda/cirrus_scodec_test.o

sound/pci/hda/cirrus_scodec_test.c:151:60: error: initializer element is not a compile-time constant
  151 |                 SOFTWARE_NODE_REFERENCE(&cirrus_scodec_test_gpio_swnode, gpio_num, 0);
      |                                                                          ^~~~~~~~
/builds/linux/include/linux/property.h:291:37: note: expanded from macro 'SOFTWARE_NODE_REFERENCE'
  291 |         .nargs = ARRAY_SIZE(((u64[]){ 0, ##__VA_ARGS__ })) - 1, \
      |                                            ^~~~~~~~~~~
/builds/linux/include/linux/kernel.h:57:75: note: expanded from macro 'ARRAY_SIZE'
   57 | #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
      |                                                                           ^~~
/builds/linux/include/linux/compiler.h:228:59: note: expanded from macro '__must_be_array'
  228 | #define __must_be_array(a)      BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0]))
      |                                                                ^
/builds/linux/include/linux/compiler_types.h:366:63: note: expanded from macro '__same_type'
  366 | #define __same_type(a, b) __builtin_types_compatible_p(typeof(a), typeof(b))
      |                                                               ^
/builds/linux/include/linux/build_bug.h:16:62: note: expanded from macro 'BUILD_BUG_ON_ZERO'
   16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
      |                                                              ^

This needs to be fixed before being sent to mainline else it will break
our builds; our CI is already red in -next over this.

> +
> +	*arg = template;
> +}
> +
> +static int cirrus_scodec_test_set_spkid_swnode(struct kunit *test,
> +					       struct device *dev,
> +					       struct software_node_ref_args *args,
> +					       int num_args)
> +{
> +	const struct property_entry props_template[] = {
> +		PROPERTY_ENTRY_REF_ARRAY_LEN("spk-id-gpios", args, num_args),
> +		{ }
> +	};
> +	struct property_entry *props;
> +	struct software_node *node;
> +
> +	node = kunit_kzalloc(test, sizeof(*node), GFP_KERNEL);
> +	if (!node)
> +		return -ENOMEM;
> +
> +	props = kunit_kzalloc(test, sizeof(props_template), GFP_KERNEL);
> +	if (!props)
> +		return -ENOMEM;
> +
> +	memcpy(props, props_template, sizeof(props_template));
> +	node->properties = props;
> +
> +	return device_add_software_node(dev, node);
> +}
> +
> +struct cirrus_scodec_test_spkid_param {
> +	int num_amps;
> +	int gpios_per_amp;
> +	int num_amps_sharing;
> +};
> +
> +static void cirrus_scodec_test_spkid_parse(struct kunit *test)
> +{
> +	struct cirrus_scodec_test_priv *priv = test->priv;
> +	const struct cirrus_scodec_test_spkid_param *param = test->param_value;
> +	int num_spk_id_refs = param->num_amps * param->gpios_per_amp;
> +	struct software_node_ref_args *refs;
> +	struct device *dev = &priv->amp_pdev.dev;
> +	unsigned int v;
> +	int i, ret;
> +
> +	refs = kunit_kcalloc(test, num_spk_id_refs, sizeof(*refs), GFP_KERNEL);
> +	KUNIT_ASSERT_NOT_NULL(test, refs);
> +
> +	for (i = 0, v = 0; i < num_spk_id_refs; ) {
> +		cirrus_scodec_test_set_gpio_ref_arg(&refs[i++], v++);
> +
> +		/*
> +		 * If amps are sharing GPIOs repeat the last set of
> +		 * GPIOs until we've done that number of amps.
> +		 * We have done all GPIOs for an amp when i is a multiple
> +		 * of gpios_per_amp.
> +		 * We have done all amps sharing the same GPIOs when i is
> +		 * a multiple of (gpios_per_amp * num_amps_sharing).
> +		 */
> +		if (!(i % param->gpios_per_amp) &&
> +		    (i % (param->gpios_per_amp * param->num_amps_sharing)))
> +			v -= param->gpios_per_amp;
> +	}
> +
> +	ret = cirrus_scodec_test_set_spkid_swnode(test, dev, refs, num_spk_id_refs);
> +	KUNIT_EXPECT_EQ_MSG(test, ret, 0, "Failed to add swnode\n");
> +
> +	for (i = 0; i < param->num_amps; ++i) {
> +		for (v = 0; v < (1 << param->gpios_per_amp); ++v) {
> +			/* Set only the GPIO bits used by this amp */
> +			priv->gpio_priv->pin_state =
> +				v << (param->gpios_per_amp * (i / param->num_amps_sharing));
> +
> +			ret = cirrus_scodec_get_speaker_id(dev, i, param->num_amps, -1);
> +			KUNIT_EXPECT_EQ_MSG(test, ret, v,
> +					    "get_speaker_id failed amp:%d pin_state:%#x\n",
> +					    i, priv->gpio_priv->pin_state);
> +		}
> +	}
> +}
> +
> +static void cirrus_scodec_test_no_spkid(struct kunit *test)
> +{
> +	struct cirrus_scodec_test_priv *priv = test->priv;
> +	struct device *dev = &priv->amp_pdev.dev;
> +	int ret;
> +
> +	ret = cirrus_scodec_get_speaker_id(dev, 0, 4, -1);
> +	KUNIT_EXPECT_EQ(test, ret, -ENOENT);
> +}
> +
> +static void cirrus_scodec_test_dev_release(struct device *dev)
> +{
> +}
> +
> +static int cirrus_scodec_test_case_init(struct kunit *test)
> +{
> +	struct cirrus_scodec_test_priv *priv;
> +	int ret;
> +
> +	priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	test->priv = priv;
> +
> +	/* Create dummy GPIO */
> +	ret = cirrus_scodec_test_create_gpio(test);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Create dummy amp driver dev */
> +	priv->amp_pdev.name = "cirrus_scodec_test_amp_drv";
> +	priv->amp_pdev.id = -1;
> +	priv->amp_pdev.dev.release = cirrus_scodec_test_dev_release;
> +	ret = platform_device_register(&priv->amp_pdev);
> +	KUNIT_ASSERT_GE_MSG(test, ret, 0, "Failed to register amp platform device\n");
> +
> +	return 0;
> +}
> +
> +static void cirrus_scodec_test_case_exit(struct kunit *test)
> +{
> +	struct cirrus_scodec_test_priv *priv = test->priv;
> +
> +	if (priv->amp_pdev.name)
> +		platform_device_unregister(&priv->amp_pdev);
> +
> +	if (priv->gpio_pdev) {
> +		device_remove_software_node(&priv->gpio_pdev->dev);
> +		platform_device_unregister(priv->gpio_pdev);
> +	}
> +}
> +
> +static int cirrus_scodec_test_suite_init(struct kunit_suite *suite)
> +{
> +	int ret;
> +
> +	/* Register mock GPIO driver */
> +	ret = platform_driver_register(&cirrus_scodec_test_gpio_driver);
> +	if (ret < 0) {
> +		kunit_err(suite, "Failed to register gpio platform driver, %d\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void cirrus_scodec_test_suite_exit(struct kunit_suite *suite)
> +{
> +	platform_driver_unregister(&cirrus_scodec_test_gpio_driver);
> +}
> +
> +static const struct cirrus_scodec_test_spkid_param cirrus_scodec_test_spkid_param_cases[] = {
> +	{ .num_amps = 2, .gpios_per_amp = 1, .num_amps_sharing = 1 },
> +	{ .num_amps = 2, .gpios_per_amp = 2, .num_amps_sharing = 1 },
> +	{ .num_amps = 2, .gpios_per_amp = 3, .num_amps_sharing = 1 },
> +	{ .num_amps = 2, .gpios_per_amp = 4, .num_amps_sharing = 1 },
> +	{ .num_amps = 3, .gpios_per_amp = 1, .num_amps_sharing = 1 },
> +	{ .num_amps = 3, .gpios_per_amp = 2, .num_amps_sharing = 1 },
> +	{ .num_amps = 3, .gpios_per_amp = 3, .num_amps_sharing = 1 },
> +	{ .num_amps = 3, .gpios_per_amp = 4, .num_amps_sharing = 1 },
> +	{ .num_amps = 4, .gpios_per_amp = 1, .num_amps_sharing = 1 },
> +	{ .num_amps = 4, .gpios_per_amp = 2, .num_amps_sharing = 1 },
> +	{ .num_amps = 4, .gpios_per_amp = 3, .num_amps_sharing = 1 },
> +	{ .num_amps = 4, .gpios_per_amp = 4, .num_amps_sharing = 1 },
> +
> +	/* Same GPIO shared by all amps */
> +	{ .num_amps = 2, .gpios_per_amp = 1, .num_amps_sharing = 2 },
> +	{ .num_amps = 2, .gpios_per_amp = 2, .num_amps_sharing = 2 },
> +	{ .num_amps = 2, .gpios_per_amp = 3, .num_amps_sharing = 2 },
> +	{ .num_amps = 2, .gpios_per_amp = 4, .num_amps_sharing = 2 },
> +	{ .num_amps = 3, .gpios_per_amp = 1, .num_amps_sharing = 3 },
> +	{ .num_amps = 3, .gpios_per_amp = 2, .num_amps_sharing = 3 },
> +	{ .num_amps = 3, .gpios_per_amp = 3, .num_amps_sharing = 3 },
> +	{ .num_amps = 3, .gpios_per_amp = 4, .num_amps_sharing = 3 },
> +	{ .num_amps = 4, .gpios_per_amp = 1, .num_amps_sharing = 4 },
> +	{ .num_amps = 4, .gpios_per_amp = 2, .num_amps_sharing = 4 },
> +	{ .num_amps = 4, .gpios_per_amp = 3, .num_amps_sharing = 4 },
> +	{ .num_amps = 4, .gpios_per_amp = 4, .num_amps_sharing = 4 },
> +
> +	/* Two sets of shared GPIOs */
> +	{ .num_amps = 4, .gpios_per_amp = 1, .num_amps_sharing = 2 },
> +	{ .num_amps = 4, .gpios_per_amp = 2, .num_amps_sharing = 2 },
> +	{ .num_amps = 4, .gpios_per_amp = 3, .num_amps_sharing = 2 },
> +	{ .num_amps = 4, .gpios_per_amp = 4, .num_amps_sharing = 2 },
> +};
> +
> +static void cirrus_scodec_test_spkid_param_desc(const struct cirrus_scodec_test_spkid_param *param,
> +						char *desc)
> +{
> +	snprintf(desc, KUNIT_PARAM_DESC_SIZE, "amps:%d gpios_per_amp:%d num_amps_sharing:%d",
> +		 param->num_amps, param->gpios_per_amp, param->num_amps_sharing);
> +}
> +
> +KUNIT_ARRAY_PARAM(cirrus_scodec_test_spkid, cirrus_scodec_test_spkid_param_cases,
> +		  cirrus_scodec_test_spkid_param_desc);
> +
> +static struct kunit_case cirrus_scodec_test_cases[] = {
> +	KUNIT_CASE_PARAM(cirrus_scodec_test_spkid_parse, cirrus_scodec_test_spkid_gen_params),
> +	KUNIT_CASE(cirrus_scodec_test_no_spkid),
> +	{ } /* terminator */
> +};
> +
> +static struct kunit_suite cirrus_scodec_test_suite = {
> +	.name = "snd-hda-scodec-cs35l56-test",
> +	.suite_init = cirrus_scodec_test_suite_init,
> +	.suite_exit = cirrus_scodec_test_suite_exit,
> +	.init = cirrus_scodec_test_case_init,
> +	.exit = cirrus_scodec_test_case_exit,
> +	.test_cases = cirrus_scodec_test_cases,
> +};
> +
> +kunit_test_suite(cirrus_scodec_test_suite);
> +
> +MODULE_IMPORT_NS(SND_HDA_CIRRUS_SCODEC);
> +MODULE_AUTHOR("Richard Fitzgerald <rf@opensource.cirrus.com>");
> +MODULE_LICENSE("GPL");
> diff --git a/sound/pci/hda/cs35l56_hda.c b/sound/pci/hda/cs35l56_hda.c
> index 44f5ca0e73e3..d3cfdad7dd76 100644
> --- a/sound/pci/hda/cs35l56_hda.c
> +++ b/sound/pci/hda/cs35l56_hda.c
> @@ -1035,6 +1035,16 @@ const struct dev_pm_ops cs35l56_hda_pm_ops = {
>  };
>  EXPORT_SYMBOL_NS_GPL(cs35l56_hda_pm_ops, SND_HDA_SCODEC_CS35L56);
>  
> +#if IS_ENABLED(CONFIG_SND_HDA_SCODEC_CS35L56_KUNIT_TEST)
> +/* Hooks to export static function to KUnit test */
> +
> +int cs35l56_hda_test_hook_get_speaker_id(struct device *dev, int amp_index, int num_amps)
> +{
> +	return cs35l56_hda_get_speaker_id(dev, amp_index, num_amps);
> +}
> +EXPORT_SYMBOL_NS_GPL(cs35l56_hda_test_hook_get_speaker_id, SND_HDA_SCODEC_CS35L56);
> +#endif
> +
>  MODULE_DESCRIPTION("CS35L56 HDA Driver");
>  MODULE_IMPORT_NS(SND_HDA_CIRRUS_SCODEC);
>  MODULE_IMPORT_NS(SND_HDA_CS_DSP_CONTROLS);
> -- 
> 2.30.2
> 

Re: [PATCH 2/2] ALSA: hda: cirrus_scodec: Add KUnit test

[Takashi Iwai]

On Tue, 19 Sep 2023 22:44:28 +0200,
Nick Desaulniers wrote:
> 
> On Mon, Sep 18, 2023 at 10:51:29AM +0100, Richard Fitzgerald wrote:
(snip)
> > +static void cirrus_scodec_test_set_gpio_ref_arg(struct software_node_ref_args *arg,
> > +						int gpio_num)
> > +{
> > +	struct software_node_ref_args template =
> > +		SOFTWARE_NODE_REFERENCE(&cirrus_scodec_test_gpio_swnode, gpio_num, 0);
> 
> I'm observing the following error when building with:
> 
> $ make LLVM=1 -j128 allmodconfig sound/pci/hda/cirrus_scodec_test.o
> 
> sound/pci/hda/cirrus_scodec_test.c:151:60: error: initializer element is not a compile-time constant
>   151 |                 SOFTWARE_NODE_REFERENCE(&cirrus_scodec_test_gpio_swnode, gpio_num, 0);
>       |                                                                          ^~~~~~~~
> /builds/linux/include/linux/property.h:291:37: note: expanded from macro 'SOFTWARE_NODE_REFERENCE'
>   291 |         .nargs = ARRAY_SIZE(((u64[]){ 0, ##__VA_ARGS__ })) - 1, \
>       |                                            ^~~~~~~~~~~
> /builds/linux/include/linux/kernel.h:57:75: note: expanded from macro 'ARRAY_SIZE'
>    57 | #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
>       |                                                                           ^~~
> /builds/linux/include/linux/compiler.h:228:59: note: expanded from macro '__must_be_array'
>   228 | #define __must_be_array(a)      BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0]))
>       |                                                                ^
> /builds/linux/include/linux/compiler_types.h:366:63: note: expanded from macro '__same_type'
>   366 | #define __same_type(a, b) __builtin_types_compatible_p(typeof(a), typeof(b))
>       |                                                               ^
> /builds/linux/include/linux/build_bug.h:16:62: note: expanded from macro 'BUILD_BUG_ON_ZERO'
>    16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
>       |                                                              ^

Hm, this looks like some inconsistent handling of the temporary array
passed to ARRAY_SIZE() in the SOFTWARE_NODE_REFERENCE macro.  LLVM
can't treat it if it contains a variable in the given array, while GCC
doesn't care.

A hackish workaround would be the patch like below, but it's really
ugly.  Ideally speaking, it should be fixed in linux/properties.h, but
I have no idea how to fix there for LLVM.

Adding more relevant people to Cc.


thanks,

Takashi

--- a/sound/pci/hda/cirrus_scodec_test.c
+++ b/sound/pci/hda/cirrus_scodec_test.c
@@ -148,8 +148,9 @@ static void cirrus_scodec_test_set_gpio_ref_arg(struct software_node_ref_args *a
 						int gpio_num)
 {
 	struct software_node_ref_args template =
-		SOFTWARE_NODE_REFERENCE(&cirrus_scodec_test_gpio_swnode, gpio_num, 0);
+		SOFTWARE_NODE_REFERENCE(&cirrus_scodec_test_gpio_swnode, 0, 0);
 
+	template.args[0] = gpio_num;
 	*arg = template;
 }
 

Re: [PATCH 2/2] ALSA: hda: cirrus_scodec: Add KUnit test

[Richard Fitzgerald]

On 20/9/23 07:51, Takashi Iwai wrote:
> On Tue, 19 Sep 2023 22:44:28 +0200,
> Nick Desaulniers wrote:
>>
>> On Mon, Sep 18, 2023 at 10:51:29AM +0100, Richard Fitzgerald wrote:
> (snip)
>>> +static void cirrus_scodec_test_set_gpio_ref_arg(struct software_node_ref_args *arg,
>>> +						int gpio_num)
>>> +{
>>> +	struct software_node_ref_args template =
>>> +		SOFTWARE_NODE_REFERENCE(&cirrus_scodec_test_gpio_swnode, gpio_num, 0);
>>
>> I'm observing the following error when building with:
>>
>> $ make LLVM=1 -j128 allmodconfig sound/pci/hda/cirrus_scodec_test.o
>>
>> sound/pci/hda/cirrus_scodec_test.c:151:60: error: initializer element is not a compile-time constant
>>    151 |                 SOFTWARE_NODE_REFERENCE(&cirrus_scodec_test_gpio_swnode, gpio_num, 0);
>>        |                                                                          ^~~~~~~~
>> /builds/linux/include/linux/property.h:291:37: note: expanded from macro 'SOFTWARE_NODE_REFERENCE'
>>    291 |         .nargs = ARRAY_SIZE(((u64[]){ 0, ##__VA_ARGS__ })) - 1, \
>>        |                                            ^~~~~~~~~~~
>> /builds/linux/include/linux/kernel.h:57:75: note: expanded from macro 'ARRAY_SIZE'
>>     57 | #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
>>        |                                                                           ^~~
>> /builds/linux/include/linux/compiler.h:228:59: note: expanded from macro '__must_be_array'
>>    228 | #define __must_be_array(a)      BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0]))
>>        |                                                                ^
>> /builds/linux/include/linux/compiler_types.h:366:63: note: expanded from macro '__same_type'
>>    366 | #define __same_type(a, b) __builtin_types_compatible_p(typeof(a), typeof(b))
>>        |                                                               ^
>> /builds/linux/include/linux/build_bug.h:16:62: note: expanded from macro 'BUILD_BUG_ON_ZERO'
>>     16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
>>        |                                                              ^
> 
> Hm, this looks like some inconsistent handling of the temporary array
> passed to ARRAY_SIZE() in the SOFTWARE_NODE_REFERENCE macro.  LLVM
> can't treat it if it contains a variable in the given array, while GCC
> doesn't care.
> 
> A hackish workaround would be the patch like below, but it's really
> ugly.  Ideally speaking, it should be fixed in linux/properties.h, but
> I have no idea how to fix there for LLVM.
> 
> Adding more relevant people to Cc.
> 
> 
> thanks,
> 
> Takashi
> 
> --- a/sound/pci/hda/cirrus_scodec_test.c
> +++ b/sound/pci/hda/cirrus_scodec_test.c
> @@ -148,8 +148,9 @@ static void cirrus_scodec_test_set_gpio_ref_arg(struct software_node_ref_args *a
>   						int gpio_num)
>   {
>   	struct software_node_ref_args template =
> -		SOFTWARE_NODE_REFERENCE(&cirrus_scodec_test_gpio_swnode, gpio_num, 0);
> +		SOFTWARE_NODE_REFERENCE(&cirrus_scodec_test_gpio_swnode, 0, 0);
>   
> +	template.args[0] = gpio_num;
>   	*arg = template;
>   }
>   

The tests must generate sw nodes dynamically, not a fixed const struct.
I wanted to avoid directly accessing the internals of the SW node
structures. Use only the macros.

I can rewrite this code to open-code the construction of the
software_node_ref_args. Or I can wait a while in case anyone has a
suggested fix for the macros.

Its seems reasonable that you should be able to create software nodes
dynamically. A "real" use of these might need to construct the data from
some other data that is not known at runtime (for example, where the
ACPI provides some information but not the necessary info).

Re: [PATCH 2/2] ALSA: hda: cirrus_scodec: Add KUnit test

[Takashi Iwai]

On Wed, 20 Sep 2023 10:27:58 +0200,
Richard Fitzgerald wrote:
> 
> On 20/9/23 07:51, Takashi Iwai wrote:
> > On Tue, 19 Sep 2023 22:44:28 +0200,
> > Nick Desaulniers wrote:
> >> 
> >> On Mon, Sep 18, 2023 at 10:51:29AM +0100, Richard Fitzgerald wrote:
> > (snip)
> >>> +static void cirrus_scodec_test_set_gpio_ref_arg(struct software_node_ref_args *arg,
> >>> +						int gpio_num)
> >>> +{
> >>> +	struct software_node_ref_args template =
> >>> +		SOFTWARE_NODE_REFERENCE(&cirrus_scodec_test_gpio_swnode, gpio_num, 0);
> >> 
> >> I'm observing the following error when building with:
> >> 
> >> $ make LLVM=1 -j128 allmodconfig sound/pci/hda/cirrus_scodec_test.o
> >> 
> >> sound/pci/hda/cirrus_scodec_test.c:151:60: error: initializer element is not a compile-time constant
> >>    151 |                 SOFTWARE_NODE_REFERENCE(&cirrus_scodec_test_gpio_swnode, gpio_num, 0);
> >>        |                                                                          ^~~~~~~~
> >> /builds/linux/include/linux/property.h:291:37: note: expanded from macro 'SOFTWARE_NODE_REFERENCE'
> >>    291 |         .nargs = ARRAY_SIZE(((u64[]){ 0, ##__VA_ARGS__ })) - 1, \
> >>        |                                            ^~~~~~~~~~~
> >> /builds/linux/include/linux/kernel.h:57:75: note: expanded from macro 'ARRAY_SIZE'
> >>     57 | #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
> >>        |                                                                           ^~~
> >> /builds/linux/include/linux/compiler.h:228:59: note: expanded from macro '__must_be_array'
> >>    228 | #define __must_be_array(a)      BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0]))
> >>        |                                                                ^
> >> /builds/linux/include/linux/compiler_types.h:366:63: note: expanded from macro '__same_type'
> >>    366 | #define __same_type(a, b) __builtin_types_compatible_p(typeof(a), typeof(b))
> >>        |                                                               ^
> >> /builds/linux/include/linux/build_bug.h:16:62: note: expanded from macro 'BUILD_BUG_ON_ZERO'
> >>     16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
> >>        |                                                              ^
> > 
> > Hm, this looks like some inconsistent handling of the temporary array
> > passed to ARRAY_SIZE() in the SOFTWARE_NODE_REFERENCE macro.  LLVM
> > can't treat it if it contains a variable in the given array, while GCC
> > doesn't care.
> > 
> > A hackish workaround would be the patch like below, but it's really
> > ugly.  Ideally speaking, it should be fixed in linux/properties.h, but
> > I have no idea how to fix there for LLVM.
> > 
> > Adding more relevant people to Cc.
> > 
> > 
> > thanks,
> > 
> > Takashi
> > 
> > --- a/sound/pci/hda/cirrus_scodec_test.c
> > +++ b/sound/pci/hda/cirrus_scodec_test.c
> > @@ -148,8 +148,9 @@ static void cirrus_scodec_test_set_gpio_ref_arg(struct software_node_ref_args *a
> >   						int gpio_num)
> >   {
> >   	struct software_node_ref_args template =
> > -		SOFTWARE_NODE_REFERENCE(&cirrus_scodec_test_gpio_swnode, gpio_num, 0);
> > +		SOFTWARE_NODE_REFERENCE(&cirrus_scodec_test_gpio_swnode, 0, 0);
> >   +	template.args[0] = gpio_num;
> >   	*arg = template;
> >   }
> >   
> 
> The tests must generate sw nodes dynamically, not a fixed const struct.
> I wanted to avoid directly accessing the internals of the SW node
> structures. Use only the macros.
> 
> I can rewrite this code to open-code the construction of the
> software_node_ref_args. Or I can wait a while in case anyone has a
> suggested fix for the macros.
> 
> Its seems reasonable that you should be able to create software nodes
> dynamically. A "real" use of these might need to construct the data from
> some other data that is not known at runtime (for example, where the
> ACPI provides some information but not the necessary info).

Yeah, fixing the macro would be ideal.

An easy and compromise solution would be to factor out the
ARRAY_SIZE() call and allow giving nargs explicitly.

e.g.

--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -285,13 +285,18 @@ struct software_node_ref_args {
 	u64 args[NR_FWNODE_REFERENCE_ARGS];
 };
 
-#define SOFTWARE_NODE_REFERENCE(_ref_, ...)			\
+#define __SOFTWARE_NODE_REFERENCE(_ref_, _nargs_, ...)		\
 (const struct software_node_ref_args) {				\
 	.node = _ref_,						\
-	.nargs = ARRAY_SIZE(((u64[]){ 0, ##__VA_ARGS__ })) - 1,	\
+	.nargs = _nargs_,					\
 	.args = { __VA_ARGS__ },				\
 }
 
+#define SOFTWARE_NODE_REFERENCE(_ref_, ...) \
+	__SOFTWARE_NODE_REFERENCE(_ref_,\
+		ARRAY_SIZE(((u64[]){ 0, ##__VA_ARGS__ })) - 1,\
+		##__VA_ARGS__)
+
 /**
  * struct property_entry - "Built-in" device property representation.
  * @name: Name of the property.
--- a/sound/pci/hda/cirrus_scodec_test.c
+++ b/sound/pci/hda/cirrus_scodec_test.c
@@ -148,7 +148,7 @@ static void cirrus_scodec_test_set_gpio_ref_arg(struct software_node_ref_args *a
 						int gpio_num)
 {
 	struct software_node_ref_args template =
-		SOFTWARE_NODE_REFERENCE(&cirrus_scodec_test_gpio_swnode, gpio_num, 0);
+		__SOFTWARE_NODE_REFERENCE(&cirrus_scodec_test_gpio_swnode, 2, gpio_num, 0);
 
 	*arg = template;
 }


... though I'm not convinced by this change, either.


Takashi

Re: [PATCH 2/2] ALSA: hda: cirrus_scodec: Add KUnit test

[Andy Shevchenko]

On Wed, Sep 20, 2023 at 08:51:57AM +0200, Takashi Iwai wrote:
> On Tue, 19 Sep 2023 22:44:28 +0200,
> Nick Desaulniers wrote:
> > 
> > On Mon, Sep 18, 2023 at 10:51:29AM +0100, Richard Fitzgerald wrote:
> (snip)
> > > +static void cirrus_scodec_test_set_gpio_ref_arg(struct software_node_ref_args *arg,
> > > +						int gpio_num)
> > > +{
> > > +	struct software_node_ref_args template =
> > > +		SOFTWARE_NODE_REFERENCE(&cirrus_scodec_test_gpio_swnode, gpio_num, 0);
> > 
> > I'm observing the following error when building with:
> > 
> > $ make LLVM=1 -j128 allmodconfig sound/pci/hda/cirrus_scodec_test.o
> > 
> > sound/pci/hda/cirrus_scodec_test.c:151:60: error: initializer element is not a compile-time constant
> >   151 |                 SOFTWARE_NODE_REFERENCE(&cirrus_scodec_test_gpio_swnode, gpio_num, 0);
> >       |                                                                          ^~~~~~~~
> > /builds/linux/include/linux/property.h:291:37: note: expanded from macro 'SOFTWARE_NODE_REFERENCE'
> >   291 |         .nargs = ARRAY_SIZE(((u64[]){ 0, ##__VA_ARGS__ })) - 1, \
> >       |                                            ^~~~~~~~~~~
> > /builds/linux/include/linux/kernel.h:57:75: note: expanded from macro 'ARRAY_SIZE'
> >    57 | #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
> >       |                                                                           ^~~
> > /builds/linux/include/linux/compiler.h:228:59: note: expanded from macro '__must_be_array'
> >   228 | #define __must_be_array(a)      BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0]))
> >       |                                                                ^
> > /builds/linux/include/linux/compiler_types.h:366:63: note: expanded from macro '__same_type'
> >   366 | #define __same_type(a, b) __builtin_types_compatible_p(typeof(a), typeof(b))
> >       |                                                               ^
> > /builds/linux/include/linux/build_bug.h:16:62: note: expanded from macro 'BUILD_BUG_ON_ZERO'
> >    16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
> >       |                                                              ^
> 
> Hm, this looks like some inconsistent handling of the temporary array
> passed to ARRAY_SIZE() in the SOFTWARE_NODE_REFERENCE macro.  LLVM
> can't treat it if it contains a variable in the given array, while GCC
> doesn't care.
> 
> A hackish workaround would be the patch like below, but it's really
> ugly.  Ideally speaking, it should be fixed in linux/properties.h, but
> I have no idea how to fix there for LLVM.
> 
> Adding more relevant people to Cc.

Thank you, I think it's quite easy to fix. Lemme cook the patch...

-- 
With Best Regards,
Andy Shevchenko