Re: [PATCH 10/14] ASoC: Intel: avs: Machine board registration

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

On 4/26/22 12:23, Cezary Rojewski wrote:
> AVS driver operates with granular audio card division in mind.
> Super-card approach (e.g.: I2S, DMIC and HDA DAIs combined) is
> deprecated in favour of individual cards - one per each device. This
> provides necessary dynamism, especially for configurations with number
> of codecs present and makes it easier to survive auxiliary devices
> failures - one card failing to probe does not prevent others from
> succeeding.
> 
> All boards spawned by AVS are unregistered on ->remove(). This includes
> dummy codecs such as DMIC.
> 
> As all machine boards found in sound/soc/intel/boards are irreversibly
> tied to 'super-card' approach, new boards are going to be introduced.
> This temporarily increases number of boards available under /intel
> directory until skylake-driver becomes deprecated and removed.

I thought you wanted your own directory for cards, what's the point of adding new machine drivers in intel/boards if they ONLY work with your AVS driver?

Also you can only remove the machine drivers that are NOT shared with SOF...


> +static struct snd_soc_acpi_mach *dmi_match_quirk(void *arg)
> +{
> +	struct snd_soc_acpi_mach *mach = arg;
> +	const struct dmi_system_id *dmi_id;
> +	struct dmi_system_id *dmi_table;
> +
> +	if (mach->quirk_data == NULL)
> +		return mach;
> +
> +	dmi_table = (struct dmi_system_id *)mach->quirk_data;
> +
> +	dmi_id = dmi_first_match(dmi_table);
> +	if (!dmi_id)
> +		return NULL;
> +
> +	return mach;
> +}
> +
> +#define AVS_SSP(x)		(BIT(x))
> +#define AVS_SSP_RANGE(a, b)	(GENMASK(b, a))
> +
> +/* supported I2S board codec configurations */
> +static struct snd_soc_acpi_mach avs_skl_i2s_machines[] = {
> +	{
> +		.id = "INT343A",
> +		.drv_name = "avs_rt286",
> +		.link_mask = AVS_SSP(0),

I've told this before, 'link_mask' was introduced for *SoundWire*. Please do not overload existing concepts and use this instead:

@i2s_link_mask: I2S/TDM links enabled on the board

> +		.tplg_filename = "skl-rt286-tplg.bin",
> +	},
> +	{
> +		.id = "10508825",
> +		.drv_name = "avs_nau8825",
> +		.link_mask = AVS_SSP(1),
> +		.tplg_filename = "skl-nau8825-tplg.bin",
> +	},
> +	{
> +		.id = "INT343B",
> +		.drv_name = "avs_ssm4567",
> +		.link_mask = AVS_SSP(0),
> +		.tplg_filename = "skl-ssm4567-tplg.bin",
> +	},
> +	{
> +		.id = "MX98357A",
> +		.drv_name = "avs_max98357a",
> +		.link_mask = AVS_SSP(0),
> +		.tplg_filename = "skl-max98357a-tplg.bin",
> +	},
> +	{},
> +};
> +
> +static struct snd_soc_acpi_mach avs_kbl_i2s_machines[] = {
> +	{
> +		.id = "INT343A",
> +		.drv_name = "avs_rt286",
> +		.link_mask = AVS_SSP(0),
> +		.quirk_data = &kbl_dmi_table,
> +		.machine_quirk = dmi_match_quirk,
> +		.tplg_filename = "kbl-rt286-tplg.bin",
> +	},
> +	{
> +		.id = "INT343A",
> +		.drv_name = "avs_rt298",
> +		.link_mask = AVS_SSP(0),
> +		.quirk_data = &kbl_r_dmi_table,
> +		.machine_quirk = dmi_match_quirk,
> +		.tplg_filename = "kblr-rt298-tplg.bin",
> +	},
> +	{
> +		.id = "MX98373",
> +		.drv_name = "avs_max98373",
> +		.link_mask = AVS_SSP(0),
> +		.tplg_filename = "kbl-max98373-tplg.bin",
> +	},
> +	{
> +		.id = "DLGS7219",
> +		.drv_name = "avs_da7219",
> +		.link_mask = AVS_SSP(1),
> +		.tplg_filename = "kbl-da7219-tplg.bin",
> +	},
> +	{},
> +};
> +
> +static struct snd_soc_acpi_mach avs_apl_i2s_machines[] = {
> +	{
> +		.id = "INT343A",
> +		.drv_name = "avs_rt298",
> +		.link_mask = AVS_SSP(5),
> +		.tplg_filename = "apl-rt298-tplg.bin",
> +	},
> +	{
> +		.id = "INT34C3",
> +		.drv_name = "avs_tdf8532",
> +		.link_mask = AVS_SSP_RANGE(0, 5),
> +		.pdata = (unsigned long[]){ 0, 0, 0x14, 0, 0, 0 }, /* SSP2 TDMs */
> +		.tplg_filename = "apl-tdf8532-tplg.bin",
> +	},
> +	{
> +		.id = "MX98357A",
> +		.drv_name = "avs_max98357a",
> +		.link_mask = AVS_SSP(5),
> +		.tplg_filename = "apl-max98357a-tplg.bin",
> +	},
> +	{
> +		.id = "DLGS7219",
> +		.drv_name = "avs_da7219",
> +		.link_mask = AVS_SSP(1),
> +		.tplg_filename = "apl-da7219-tplg.bin",
> +	},
> +	{},
> +};
> +
> +static struct snd_soc_acpi_mach avs_gml_i2s_machines[] = {
> +	{
> +		.id = "INT343A",
> +		.drv_name = "avs_rt298",
> +		.link_mask = AVS_SSP(2),
> +		.tplg_filename = "gml-rt298-tplg.bin",
> +	},
> +	{},
> +};
> +
> +static struct snd_soc_acpi_mach avs_test_i2s_machines[] = {
> +	{
> +		.drv_name = "avs_ssp_test",
> +		.link_mask = AVS_SSP(0),
> +		.tplg_filename = "avs_ssp_test.bin",
> +	},
> +	{
> +		.drv_name = "avs_ssp_test",
> +		.link_mask = AVS_SSP(1),
> +		.tplg_filename = "avs_ssp_test.bin",
> +	},
> +	{
> +		.drv_name = "avs_ssp_test",
> +		.link_mask = AVS_SSP(2),
> +		.tplg_filename = "avs_ssp_test.bin",
> +	},
> +	{
> +		.drv_name = "avs_ssp_test",
> +		.link_mask = AVS_SSP(3),
> +		.tplg_filename = "avs_ssp_test.bin",
> +	},
> +	{
> +		.drv_name = "avs_ssp_test",
> +		.link_mask = AVS_SSP(4),
> +		.tplg_filename = "avs_ssp_test.bin",
> +	},
> +	{
> +		.drv_name = "avs_ssp_test",
> +		.link_mask = AVS_SSP(5),
> +		.tplg_filename = "avs_ssp_test.bin",
> +	},
> +	/* no NULL terminator, as we depend on ARRAY SIZE due to .id == NULL */
> +};
> +
> +struct avs_acpi_boards {
> +	int id;
> +	struct snd_soc_acpi_mach *machs;
> +};
> +
> +#define AVS_MACH_ENTRY(_id, _mach) \
> +	{ .id = (_id), .machs = (_mach), }
> +
> +/* supported I2S boards per platform */
> +static const struct avs_acpi_boards i2s_boards[] = {
> +	AVS_MACH_ENTRY(0x9d70, avs_skl_i2s_machines), /* SKL */
> +	AVS_MACH_ENTRY(0x9d71, avs_kbl_i2s_machines), /* KBL */
> +	AVS_MACH_ENTRY(0x5a98, avs_apl_i2s_machines), /* APL */
> +	AVS_MACH_ENTRY(0x3198, avs_gml_i2s_machines), /* GML */
> +	{},

you are not using the intel/commmon matching and ACPI tables so I would recommend you deal with machine drivers in your private space.


> +static int avs_register_hda_board(struct avs_dev *adev, struct hda_codec *codec)
> +{
> +	struct snd_soc_acpi_mach mach = {{0}};
> +	struct platform_device *board;
> +	struct hdac_device *hdev = &codec->core;
> +	char *pname;
> +	int ret, id;
> +
> +	pname = devm_kasprintf(adev->dev, GFP_KERNEL, "%s-platform", dev_name(&hdev->dev));
> +	if (!pname)
> +		return -ENOMEM;
> +
> +	ret = avs_hda_platform_register(adev, pname);
> +	if (ret < 0)
> +		return ret;
> +
> +	mach.pdata = codec;
> +	mach.mach_params.platform = pname;
> +	mach.tplg_filename = devm_kasprintf(adev->dev, GFP_KERNEL, "hda-%08x-tplg.bin",
> +					    hdev->vendor_id);

this is surprising, how many topologies will you end-up supporting then? Topologies are typically NOT dependent on the HDaudio codec type or vendor and only deal with HDaudio link DMA configurations.

> +	if (!mach.tplg_filename)
> +		return -ENOMEM;
> +
> +	id = adev->base.core.idx * HDA_MAX_CODECS + hdev->addr;
> +	board = platform_device_register_data(NULL, "avs_hdaudio", id, (const void *)&mach,
> +					      sizeof(mach));
> +	if (IS_ERR(board)) {
> +		dev_err(adev->dev, "hda board register failed\n");
> +		return PTR_ERR(board);
> +	}
> +
> +	ret = devm_add_action(adev->dev, board_pdev_unregister, board);
> +	if (ret < 0) {
> +		platform_device_unregister(board);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +