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

[Cezary Rojewski <cezary.rojewski@intel.com>]

On 2022-04-27 12:12 AM, Pierre-Louis Bossart wrote:
> 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...


Yes, if something is being actively used even once skylake-driver is 
removed, it will stay there. No worries. I recommend moving SOF-specific 
boards into /sof/intel/boards/ though - it feels logical to have 
driver-specific boards within driver-specific folder as very limited 
number of boards, if any, are "common" here.

I've provided board-related patchset on the list simultaneously so 
people can see the full picture.

>> +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


Noooo :( Sad panda is sad.

'link_mask' is such a wonderful name as it matches naming used in our 
specs - which call BE side 'LINK'.

If it's a must then yes, we will resign from using 'link_mask'.

>> +		.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",
>> +	},
>> +	{},
>> +};
>> +

...

>> +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.


And that's what we chose to do! I'm sorry if the message brought any 
confusion here. sound/soc/intel/avs/boards is the subdirectory for 
avs-driver boards.

>> +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.


Keen eye. I'm not providing all the patches simultaneously so the 
patchsets are easier to review. Note that avs/core.c (available on 
upstream) still provides 'NULL' for its hda_ext_ops. Separate patches 
for the point you brought here (and the completing the avs 
initialization for that matter) will be sent later.

The default: be specific when choosing topology for specific board - 
allows for tailoring configuration-specific topology. However, if there 
is no such files, load "generic" topology instead.

In our repo, most of the hda-XXXX-tplg.bin files are actually symlinks 
to few, real HD-Audio codec topologies.

>> +	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;
>> +}
>> +