| Message ID | 20220426172346.3508411-11-cezary.rojewski@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ASoC: Intel: avs: Driver core and PCM operations | expand |
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; > +} > +
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; >> +} >> +
On 4/29/2022 4:01 PM, Cezary Rojewski wrote: >>> +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'. > I'll just note that header kernel doc for link_mask says: " * @link_mask: describes required board layout, e.g. for SoundWire." I would say there is no assumption that it is SDW only, so we could also argue that if it is it should be named sdw_link_mask and comment be fixed to remove "e.g." ;)
On 2022-05-04 11:41 AM, Amadeusz Sławiński wrote: > On 4/29/2022 4:01 PM, Cezary Rojewski wrote: > >>>> +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'. >> > > I'll just note that header kernel doc for link_mask says: > " * @link_mask: describes required board layout, e.g. for SoundWire." > I would say there is no assumption that it is SDW only, so we could also > argue that if it is it should be named sdw_link_mask and comment be > fixed to remove "e.g." ;) This is a marvelous suggestion! Adheres to fw spec, adheres to kernel doc. What more would one want! Regards, Czarek
On 04/05/2022 12:41, Amadeusz Sławiński wrote: > On 4/29/2022 4:01 PM, Cezary Rojewski wrote: >>>> +#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'. >> > > I'll just note that header kernel doc for link_mask says: > " * @link_mask: describes required board layout, e.g. for SoundWire." > I would say there is no assumption that it is SDW only, so we could also > argue that if it is it should be named sdw_link_mask and comment be > fixed to remove "e.g." ;) In mainline kernel it is (as of 5.18.0-rc5): /** * snd_soc_acpi_mach_params: interface for machine driver configuration * * @acpi_ipc_irq_index: used for BYT-CR detection * @platform: string used for HDAudio codec support * @codec_mask: used for HDAudio support * @dmic_num: number of SoC- or chipset-attached PDM digital microphones * @common_hdmi_codec_drv: use commom HDAudio HDMI codec driver * @link_mask: SoundWire links enabled on the board * @links: array of SoundWire link _ADR descriptors, null terminated * @i2s_link_mask: I2S/TDM links enabled on the board * @num_dai_drivers: number of elements in @dai_drivers * @dai_drivers: pointer to dai_drivers, used e.g. in nocodec mode */ link_mask is for SDW i2s_link_mask is for I2S It might be nicer to prefix the bare link_mask with sdw, it might happen, but the comment is pretty explicit. I would not dare to use link_mask for DMIC for example...
On 2022-05-04 1:26 PM, Péter Ujfalusi wrote: > On 04/05/2022 12:41, Amadeusz Sławiński wrote: >> On 4/29/2022 4:01 PM, Cezary Rojewski wrote: >>>>> +#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'. >>> >> >> I'll just note that header kernel doc for link_mask says: >> " * @link_mask: describes required board layout, e.g. for SoundWire." >> I would say there is no assumption that it is SDW only, so we could also >> argue that if it is it should be named sdw_link_mask and comment be >> fixed to remove "e.g." ;) > > In mainline kernel it is (as of 5.18.0-rc5): > /** > * snd_soc_acpi_mach_params: interface for machine driver configuration > * > * @acpi_ipc_irq_index: used for BYT-CR detection > * @platform: string used for HDAudio codec support > * @codec_mask: used for HDAudio support > * @dmic_num: number of SoC- or chipset-attached PDM digital microphones > * @common_hdmi_codec_drv: use commom HDAudio HDMI codec driver > * @link_mask: SoundWire links enabled on the board > * @links: array of SoundWire link _ADR descriptors, null terminated > * @i2s_link_mask: I2S/TDM links enabled on the board > * @num_dai_drivers: number of elements in @dai_drivers > * @dai_drivers: pointer to dai_drivers, used e.g. in nocodec mode > */ > > link_mask is for SDW > i2s_link_mask is for I2S > > It might be nicer to prefix the bare link_mask with sdw, it might > happen, but the comment is pretty explicit. I would not dare to use > link_mask for DMIC for example... > Ok, only now I realized that the 'i2s_link_mask' is already part of upstream kernel - commit: ASoC: soc-acpi: add information on I2S/TDM link mask from Mar 8th 2022. I agree on the "sdw_" prefix part. Right now, the name gives plenty of room for interpretation. Regards, Czarek
diff --git a/sound/soc/intel/avs/Makefile b/sound/soc/intel/avs/Makefile index 38285e73e75d..592d4dc02c56 100644 --- a/sound/soc/intel/avs/Makefile +++ b/sound/soc/intel/avs/Makefile @@ -1,7 +1,7 @@ # SPDX-License-Identifier: GPL-2.0-only snd-soc-avs-objs := dsp.o ipc.o messages.o utils.o core.o loader.o \ - topology.o path.o pcm.o + topology.o path.o pcm.o board_selection.o snd-soc-avs-objs += cldma.o snd-soc-avs-objs += trace.o diff --git a/sound/soc/intel/avs/avs.h b/sound/soc/intel/avs/avs.h index c3323f90b693..12846ad93efe 100644 --- a/sound/soc/intel/avs/avs.h +++ b/sound/soc/intel/avs/avs.h @@ -310,6 +310,9 @@ int avs_i2s_platform_register(struct avs_dev *adev, const char *name, unsigned l unsigned long *tdms); int avs_hda_platform_register(struct avs_dev *adev, const char *name); +int avs_register_all_boards(struct avs_dev *adev); +void avs_unregister_all_boards(struct avs_dev *adev); + /* Firmware tracing helpers */ unsigned int __kfifo_fromio_locked(struct kfifo *fifo, const void __iomem *src, unsigned int len, diff --git a/sound/soc/intel/avs/board_selection.c b/sound/soc/intel/avs/board_selection.c new file mode 100644 index 000000000000..711eef461a74 --- /dev/null +++ b/sound/soc/intel/avs/board_selection.c @@ -0,0 +1,463 @@ +// SPDX-License-Identifier: GPL-2.0-only +// +// Copyright(c) 2021-2022 Intel Corporation. All rights reserved. +// +// Authors: Cezary Rojewski <cezary.rojewski@intel.com> +// Amadeusz Slawinski <amadeuszx.slawinski@linux.intel.com> +// + +#include <linux/acpi.h> +#include <linux/module.h> +#include <linux/dmi.h> +#include <linux/pci.h> +#include <linux/platform_device.h> +#include <sound/hda_codec.h> +#include <sound/hda_register.h> +#include <sound/intel-nhlt.h> +#include <sound/soc-acpi.h> +#include <sound/soc-component.h> +#include "avs.h" + +static bool ssp_loopback_test; +module_param_named(ssp_loopback, ssp_loopback_test, bool, 0444); +MODULE_PARM_DESC(ssp_loopback, "SSP loopback test 0=disabled, 1=enabled"); + +static const struct dmi_system_id kbl_dmi_table[] = { + { + .matches = { + DMI_MATCH(DMI_SYS_VENDOR, "Intel Corporation"), + DMI_MATCH(DMI_BOARD_NAME, "Skylake Y LPDDR3 RVP3"), + }, + }, + {} +}; + +static const struct dmi_system_id kbl_r_dmi_table[] = { + { + .matches = { + DMI_MATCH(DMI_SYS_VENDOR, "Intel Corporation"), + DMI_MATCH(DMI_BOARD_NAME, "Kabylake R DDR4 RVP"), + }, + }, + {} +}; + +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), + .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 */ + {}, +}; + +static const struct avs_acpi_boards *avs_get_i2s_boards(struct avs_dev *adev) +{ + int id, i; + + id = adev->base.pci->device; + for (i = 0; i < ARRAY_SIZE(i2s_boards); i++) + if (i2s_boards[i].id == id) + return &i2s_boards[i]; + return NULL; +} + +/* platform devices owned by AVS audio are removed with this hook */ +static void board_pdev_unregister(void *data) +{ + platform_device_unregister(data); +} + +static int avs_register_dmic_board(struct avs_dev *adev) +{ + struct platform_device *codec, *board; + struct snd_soc_acpi_mach mach = {{0}}; + int ret; + + if (!adev->nhlt || + !intel_nhlt_has_endpoint_type(adev->nhlt, NHLT_LINK_DMIC)) { + dev_dbg(adev->dev, "no DMIC endpoints present\n"); + return 0; + } + + codec = platform_device_register_simple("dmic-codec", PLATFORM_DEVID_NONE, NULL, 0); + if (IS_ERR(codec)) { + dev_err(adev->dev, "dmic codec register failed\n"); + return PTR_ERR(codec); + } + + ret = devm_add_action(adev->dev, board_pdev_unregister, codec); + if (ret < 0) { + platform_device_unregister(codec); + return ret; + } + + ret = avs_dmic_platform_register(adev, "dmic-platform"); + if (ret < 0) + return ret; + + mach.tplg_filename = "dmic-tplg.bin"; + mach.mach_params.platform = "dmic-platform"; + + board = platform_device_register_data(NULL, "avs_dmic", PLATFORM_DEVID_NONE, + (const void *)&mach, sizeof(mach)); + if (IS_ERR(board)) { + dev_err(adev->dev, "dmic 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; +} + +static int avs_register_i2s_board(struct avs_dev *adev, struct snd_soc_acpi_mach *mach) +{ + struct platform_device *board; + int num_ssps; + char *name; + int ret; + + num_ssps = adev->hw_cfg.i2s_caps.ctrl_count; + if (fls(mach->link_mask) > num_ssps) { + dev_err(adev->dev, "Platform supports %d SSPs but board %s requires SSP%ld\n", + num_ssps, mach->drv_name, __fls(mach->link_mask)); + return -ENODEV; + } + + name = devm_kasprintf(adev->dev, GFP_KERNEL, "%s.%d-platform", mach->drv_name, + mach->link_mask); + if (!name) + return -ENOMEM; + + ret = avs_i2s_platform_register(adev, name, mach->link_mask, mach->pdata); + if (ret < 0) + return ret; + + mach->mach_params.platform = name; + + board = platform_device_register_data(NULL, mach->drv_name, mach->link_mask, + (const void *)mach, sizeof(*mach)); + if (IS_ERR(board)) { + dev_err(adev->dev, "ssp 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; +} + +static int avs_register_i2s_boards(struct avs_dev *adev) +{ + const struct avs_acpi_boards *boards; + struct snd_soc_acpi_mach *mach; + int ret; + + if (!adev->nhlt || !intel_nhlt_has_endpoint_type(adev->nhlt, NHLT_LINK_SSP)) { + dev_dbg(adev->dev, "no I2S endpoints present\n"); + return 0; + } + + if (ssp_loopback_test) { + int i, num_ssps; + + num_ssps = adev->hw_cfg.i2s_caps.ctrl_count; + /* constrain just in case FW says there can be more SSPs than possible */ + num_ssps = min_t(int, ARRAY_SIZE(avs_test_i2s_machines), num_ssps); + + mach = avs_test_i2s_machines; + + for (i = 0; i < num_ssps; i++) { + ret = avs_register_i2s_board(adev, &mach[i]); + if (ret < 0) + dev_warn(adev->dev, "register i2s %s failed: %d\n", mach->drv_name, + ret); + } + return 0; + } + + boards = avs_get_i2s_boards(adev); + if (!boards) { + dev_dbg(adev->dev, "no I2S endpoints supported\n"); + return 0; + } + + for (mach = boards->machs; mach->id[0]; mach++) { + if (!acpi_dev_present(mach->id, NULL, -1)) + continue; + + if (mach->machine_quirk) + if (!mach->machine_quirk(mach)) + continue; + + ret = avs_register_i2s_board(adev, mach); + if (ret < 0) + dev_warn(adev->dev, "register i2s %s failed: %d\n", mach->drv_name, ret); + } + + return 0; +} + +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); + 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; +} + +static int avs_register_hda_boards(struct avs_dev *adev) +{ + struct hdac_bus *bus = &adev->base.core; + struct hdac_device *hdev; + int ret; + + if (!bus->num_codecs) { + dev_dbg(adev->dev, "no HDA endpoints present\n"); + return 0; + } + + list_for_each_entry(hdev, &bus->codec_list, list) { + struct hda_codec *codec; + + codec = dev_to_hda_codec(&hdev->dev); + + ret = avs_register_hda_board(adev, codec); + if (ret < 0) + dev_warn(adev->dev, "register hda-%08x failed: %d\n", + codec->core.vendor_id, ret); + } + + return 0; +} + +int avs_register_all_boards(struct avs_dev *adev) +{ + int ret; + + ret = avs_register_dmic_board(adev); + if (ret < 0) + dev_warn(adev->dev, "enumerate DMIC endpoints failed: %d\n", + ret); + + ret = avs_register_i2s_boards(adev); + if (ret < 0) + dev_warn(adev->dev, "enumerate I2S endpoints failed: %d\n", + ret); + + ret = avs_register_hda_boards(adev); + if (ret < 0) + dev_warn(adev->dev, "enumerate HDA endpoints failed: %d\n", + ret); + + return 0; +} + +void avs_unregister_all_boards(struct avs_dev *adev) +{ + snd_soc_unregister_component(adev->dev); +}