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 <[email protected]>
---
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 <[email protected]>");
+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
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 <[email protected]>
> ---
> 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 <[email protected]>");
> +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
>
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).
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
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;
}
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