Message-ID: <ZQoILN6QCjzosCOs@google.com>
Date:   Tue, 19 Sep 2023 13:44:28 -0700
From:   Nick Desaulniers <ndesaulniers@...gle.com>
To:     Richard Fitzgerald <rf@...nsource.cirrus.com>
Cc:     tiwai@...e.com, alsa-devel@...a-project.org,
        patches@...nsource.cirrus.com, linux-kernel@...r.kernel.org,
        llvm@...ts.linux.dev
Subject: Re: [PATCH 2/2] ALSA: hda: cirrus_scodec: Add KUnit test

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@...nsource.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@...nsource.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
> 

Powered by blists - more mailing lists