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