| Message ID | 20231027110537.2103712-2-amadeuszx.slawinski@linux.intel.com |
|---|---|
| State | Accepted |
| Commit | 8b78fbf7bffac4f7b1b747fbce45ac32f530c96e |
| Headers | show |
| Series | ASoC: Intel: avs: Add support for rt5514 codec | expand |
On 27/10/2023 13:05, Amadeusz Sławiński wrote: > To support AVS-rt5514 configuration add machine board connecting AVS > platform component driver with rt5514 codec one. > > Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com> > --- > sound/soc/intel/avs/boards/Kconfig | 10 ++ > sound/soc/intel/avs/boards/Makefile | 2 + > sound/soc/intel/avs/boards/rt5514.c | 187 ++++++++++++++++++++++++++++ > 3 files changed, 199 insertions(+) > create mode 100644 sound/soc/intel/avs/boards/rt5514.c ... > + > +static struct platform_driver avs_rt5514_driver = { > + .probe = avs_rt5514_probe, > + .driver = { > + .name = "avs_rt5514", > + .pm = &snd_soc_pm_ops, > + }, > +}; > + > +module_platform_driver(avs_rt5514_driver); > + > +MODULE_LICENSE("GPL"); > +MODULE_ALIAS("platform:avs_rt5514"); You should not need MODULE_ALIAS() in normal cases. If you need it, usually it means your device ID table is wrong. Best regards, Krzysztof
On 10/28/2023 10:46 AM, Krzysztof Kozlowski wrote: > On 27/10/2023 13:05, Amadeusz Sławiński wrote: >> To support AVS-rt5514 configuration add machine board connecting AVS >> platform component driver with rt5514 codec one. >> >> Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com> >> --- >> sound/soc/intel/avs/boards/Kconfig | 10 ++ >> sound/soc/intel/avs/boards/Makefile | 2 + >> sound/soc/intel/avs/boards/rt5514.c | 187 ++++++++++++++++++++++++++++ >> 3 files changed, 199 insertions(+) >> create mode 100644 sound/soc/intel/avs/boards/rt5514.c > > ... > >> + >> +static struct platform_driver avs_rt5514_driver = { >> + .probe = avs_rt5514_probe, >> + .driver = { >> + .name = "avs_rt5514", >> + .pm = &snd_soc_pm_ops, >> + }, >> +}; >> + >> +module_platform_driver(avs_rt5514_driver); >> + >> +MODULE_LICENSE("GPL"); >> +MODULE_ALIAS("platform:avs_rt5514"); > > You should not need MODULE_ALIAS() in normal cases. If you need it, > usually it means your device ID table is wrong. > In theory yes, in practice it is a bit more complicated, as we use the driver alias in sound/soc/intel/avs/board_selection.c in snd_soc_acpi_mach and they should match. For example for rt286, there is: # modinfo /lib/modules/6.4.0-rc3+/kernel/sound/soc/intel/avs/boards/snd-soc-avs-rt286.ko | grep 286 filename: /lib/modules/6.4.0-rc3+/kernel/sound/soc/intel/avs/boards/snd-soc-avs-rt286.ko alias: platform:avs_rt286 name: snd_soc_avs_rt286 as you can see platform_driver::driver::name is not matching the driver name. I've did quick test with removing alias and changing snd_soc_acpi_mach definition for one board and it didn't load. Now that you pointed it out I also lean towards trying to remove MODULE_ALIAS() from board drivers, but it will probably require some more investigation if we really want to do it and implementing it properly.
On 30/10/2023 12:50, Amadeusz Sławiński wrote: > On 10/28/2023 10:46 AM, Krzysztof Kozlowski wrote: >> On 27/10/2023 13:05, Amadeusz Sławiński wrote: >>> To support AVS-rt5514 configuration add machine board connecting AVS >>> platform component driver with rt5514 codec one. >>> >>> Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com> >>> --- >>> sound/soc/intel/avs/boards/Kconfig | 10 ++ >>> sound/soc/intel/avs/boards/Makefile | 2 + >>> sound/soc/intel/avs/boards/rt5514.c | 187 ++++++++++++++++++++++++++++ >>> 3 files changed, 199 insertions(+) >>> create mode 100644 sound/soc/intel/avs/boards/rt5514.c >> >> ... >> >>> + >>> +static struct platform_driver avs_rt5514_driver = { >>> + .probe = avs_rt5514_probe, >>> + .driver = { >>> + .name = "avs_rt5514", >>> + .pm = &snd_soc_pm_ops, >>> + }, >>> +}; >>> + >>> +module_platform_driver(avs_rt5514_driver); >>> + >>> +MODULE_LICENSE("GPL"); >>> +MODULE_ALIAS("platform:avs_rt5514"); >> >> You should not need MODULE_ALIAS() in normal cases. If you need it, >> usually it means your device ID table is wrong. >> > > In theory yes, in practice it is a bit more complicated, as we use the > driver alias in sound/soc/intel/avs/board_selection.c in > snd_soc_acpi_mach and they should match. > > For example for rt286, there is: > # modinfo > /lib/modules/6.4.0-rc3+/kernel/sound/soc/intel/avs/boards/snd-soc-avs-rt286.ko > | grep 286 > filename: > /lib/modules/6.4.0-rc3+/kernel/sound/soc/intel/avs/boards/snd-soc-avs-rt286.ko > alias: platform:avs_rt286 > name: snd_soc_avs_rt286 > as you can see platform_driver::driver::name is not matching the driver > name. > > I've did quick test with removing alias and changing snd_soc_acpi_mach > definition for one board and it didn't load. Sorry, but why do you talk about platform name? We talk about ID table! > > Now that you pointed it out I also lean towards trying to remove > MODULE_ALIAS() from board drivers, but it will probably require some > more investigation if we really want to do it and implementing it properly. Ehm? We have been there. I've been dropping these useless aliases as well. You miss DEVICE_TABLE and proper ID entries, not adding aliases. Best regards, Krzysztof
On 10/30/2023 12:59 PM, Krzysztof Kozlowski wrote: > On 30/10/2023 12:50, Amadeusz Sławiński wrote: >> On 10/28/2023 10:46 AM, Krzysztof Kozlowski wrote: >>> On 27/10/2023 13:05, Amadeusz Sławiński wrote: >>>> To support AVS-rt5514 configuration add machine board connecting AVS >>>> platform component driver with rt5514 codec one. >>>> >>>> Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com> >>>> --- >>>> sound/soc/intel/avs/boards/Kconfig | 10 ++ >>>> sound/soc/intel/avs/boards/Makefile | 2 + >>>> sound/soc/intel/avs/boards/rt5514.c | 187 ++++++++++++++++++++++++++++ >>>> 3 files changed, 199 insertions(+) >>>> create mode 100644 sound/soc/intel/avs/boards/rt5514.c >>> >>> ... >>> >>>> + >>>> +static struct platform_driver avs_rt5514_driver = { >>>> + .probe = avs_rt5514_probe, >>>> + .driver = { >>>> + .name = "avs_rt5514", >>>> + .pm = &snd_soc_pm_ops, >>>> + }, >>>> +}; >>>> + >>>> +module_platform_driver(avs_rt5514_driver); >>>> + >>>> +MODULE_LICENSE("GPL"); >>>> +MODULE_ALIAS("platform:avs_rt5514"); >>> >>> You should not need MODULE_ALIAS() in normal cases. If you need it, >>> usually it means your device ID table is wrong. >>> >> >> In theory yes, in practice it is a bit more complicated, as we use the >> driver alias in sound/soc/intel/avs/board_selection.c in >> snd_soc_acpi_mach and they should match. >> >> For example for rt286, there is: >> # modinfo >> /lib/modules/6.4.0-rc3+/kernel/sound/soc/intel/avs/boards/snd-soc-avs-rt286.ko >> | grep 286 >> filename: >> /lib/modules/6.4.0-rc3+/kernel/sound/soc/intel/avs/boards/snd-soc-avs-rt286.ko >> alias: platform:avs_rt286 >> name: snd_soc_avs_rt286 >> as you can see platform_driver::driver::name is not matching the driver >> name. >> >> I've did quick test with removing alias and changing snd_soc_acpi_mach >> definition for one board and it didn't load. > > Sorry, but why do you talk about platform name? We talk about ID table! > >> >> Now that you pointed it out I also lean towards trying to remove >> MODULE_ALIAS() from board drivers, but it will probably require some >> more investigation if we really want to do it and implementing it properly. > > Ehm? We have been there. I've been dropping these useless aliases as > well. You miss DEVICE_TABLE and proper ID entries, not adding aliases. > Ah, DEVICE_TABLE, that's what I was missing, so I just to confirm, something like: diff --git a/sound/soc/intel/avs/boards/rt274.c b/sound/soc/intel/avs/boards/rt274.c index dd613aa15e80..659e10b1dcad 100644 --- a/sound/soc/intel/avs/boards/rt274.c +++ b/sound/soc/intel/avs/boards/rt274.c @@ -257,6 +257,14 @@ static int avs_rt274_probe(struct platform_device *pdev) return devm_snd_soc_register_card(dev, card); } +static const struct platform_device_id avs_rt274_driver_ids[] = { + { + .name = "avs_rt274", + }, + {}, +}; +MODULE_DEVICE_TABLE(platform, avs_rt274_driver_ids); + static struct platform_driver avs_rt274_driver = { .probe = avs_rt274_probe, .driver = { @@ -268,4 +276,3 @@ static struct platform_driver avs_rt274_driver = { module_platform_driver(avs_rt274_driver); MODULE_LICENSE("GPL"); -MODULE_ALIAS("platform:avs_rt274"); for all the boards?
On 30/10/2023 13:15, Amadeusz Sławiński wrote: >>>>> +static struct platform_driver avs_rt5514_driver = { >>>>> + .probe = avs_rt5514_probe, >>>>> + .driver = { >>>>> + .name = "avs_rt5514", >>>>> + .pm = &snd_soc_pm_ops, >>>>> + }, >>>>> +}; >>>>> + >>>>> +module_platform_driver(avs_rt5514_driver); >>>>> + >>>>> +MODULE_LICENSE("GPL"); >>>>> +MODULE_ALIAS("platform:avs_rt5514"); >>>> >>>> You should not need MODULE_ALIAS() in normal cases. If you need it, >>>> usually it means your device ID table is wrong. >>>> >>> >>> In theory yes, in practice it is a bit more complicated, as we use the >>> driver alias in sound/soc/intel/avs/board_selection.c in >>> snd_soc_acpi_mach and they should match. >>> >>> For example for rt286, there is: >>> # modinfo >>> /lib/modules/6.4.0-rc3+/kernel/sound/soc/intel/avs/boards/snd-soc-avs-rt286.ko >>> | grep 286 >>> filename: >>> /lib/modules/6.4.0-rc3+/kernel/sound/soc/intel/avs/boards/snd-soc-avs-rt286.ko >>> alias: platform:avs_rt286 >>> name: snd_soc_avs_rt286 >>> as you can see platform_driver::driver::name is not matching the driver >>> name. >>> >>> I've did quick test with removing alias and changing snd_soc_acpi_mach >>> definition for one board and it didn't load. >> >> Sorry, but why do you talk about platform name? We talk about ID table! >> >>> >>> Now that you pointed it out I also lean towards trying to remove >>> MODULE_ALIAS() from board drivers, but it will probably require some >>> more investigation if we really want to do it and implementing it properly. >> >> Ehm? We have been there. I've been dropping these useless aliases as >> well. You miss DEVICE_TABLE and proper ID entries, not adding aliases. >> > > Ah, DEVICE_TABLE, that's what I was missing, so I just to confirm, > something like: > > diff --git a/sound/soc/intel/avs/boards/rt274.c > b/sound/soc/intel/avs/boards/rt274.c > index dd613aa15e80..659e10b1dcad 100644 > --- a/sound/soc/intel/avs/boards/rt274.c > +++ b/sound/soc/intel/avs/boards/rt274.c > @@ -257,6 +257,14 @@ static int avs_rt274_probe(struct platform_device > *pdev) > return devm_snd_soc_register_card(dev, card); > } > > +static const struct platform_device_id avs_rt274_driver_ids[] = { > + { > + .name = "avs_rt274", > + }, > + {}, > +}; > +MODULE_DEVICE_TABLE(platform, avs_rt274_driver_ids); Yes, this is the basic method of probing all devices and auto-loading modules. I wrote at beginning that your ID table is wrong, but I was not aware that it is missing completely. Best regards, Krzysztof
diff --git a/sound/soc/intel/avs/boards/Kconfig b/sound/soc/intel/avs/boards/Kconfig index 07353d37ecae..00b0f6c176d6 100644 --- a/sound/soc/intel/avs/boards/Kconfig +++ b/sound/soc/intel/avs/boards/Kconfig @@ -125,6 +125,16 @@ config SND_SOC_INTEL_AVS_MACH_RT298 Say Y or m if you have such a device. This is a recommended option. If unsure select "N". +config SND_SOC_INTEL_AVS_MACH_RT5514 + tristate "rt5514 in I2S mode" + depends on I2C + depends on MFD_INTEL_LPSS || COMPILE_TEST + select SND_SOC_RT5514 + help + This adds support for ASoC machine driver with RT5514 I2S audio codec. + Say Y or m if you have such a device. This is a recommended option. + If unsure select "N". + config SND_SOC_INTEL_AVS_MACH_RT5663 tristate "rt5663 in I2S mode" depends on I2C diff --git a/sound/soc/intel/avs/boards/Makefile b/sound/soc/intel/avs/boards/Makefile index 34347bcd1e7d..0ff21d55be24 100644 --- a/sound/soc/intel/avs/boards/Makefile +++ b/sound/soc/intel/avs/boards/Makefile @@ -13,6 +13,7 @@ snd-soc-avs-probe-objs := probe.o snd-soc-avs-rt274-objs := rt274.o snd-soc-avs-rt286-objs := rt286.o snd-soc-avs-rt298-objs := rt298.o +snd-soc-avs-rt5514-objs := rt5514.o snd-soc-avs-rt5663-objs := rt5663.o snd-soc-avs-rt5682-objs := rt5682.o snd-soc-avs-ssm4567-objs := ssm4567.o @@ -30,6 +31,7 @@ obj-$(CONFIG_SND_SOC_INTEL_AVS_MACH_PROBE) += snd-soc-avs-probe.o obj-$(CONFIG_SND_SOC_INTEL_AVS_MACH_RT274) += snd-soc-avs-rt274.o obj-$(CONFIG_SND_SOC_INTEL_AVS_MACH_RT286) += snd-soc-avs-rt286.o obj-$(CONFIG_SND_SOC_INTEL_AVS_MACH_RT298) += snd-soc-avs-rt298.o +obj-$(CONFIG_SND_SOC_INTEL_AVS_MACH_RT5514) += snd-soc-avs-rt5514.o obj-$(CONFIG_SND_SOC_INTEL_AVS_MACH_RT5663) += snd-soc-avs-rt5663.o obj-$(CONFIG_SND_SOC_INTEL_AVS_MACH_RT5682) += snd-soc-avs-rt5682.o obj-$(CONFIG_SND_SOC_INTEL_AVS_MACH_SSM4567) += snd-soc-avs-ssm4567.o diff --git a/sound/soc/intel/avs/boards/rt5514.c b/sound/soc/intel/avs/boards/rt5514.c new file mode 100644 index 000000000000..ad486a52e5e3 --- /dev/null +++ b/sound/soc/intel/avs/boards/rt5514.c @@ -0,0 +1,187 @@ +// SPDX-License-Identifier: GPL-2.0-only +// +// Copyright(c) 2021-2023 Intel Corporation. All rights reserved. +// +// Authors: Cezary Rojewski <cezary.rojewski@intel.com> +// Amadeusz Slawinski <amadeuszx.slawinski@linux.intel.com> +// + +#include <linux/clk.h> +#include <linux/input.h> +#include <linux/module.h> +#include <sound/jack.h> +#include <sound/pcm.h> +#include <sound/pcm_params.h> +#include <sound/soc.h> +#include <sound/soc-acpi.h> +#include "../../../codecs/rt5514.h" +#include "../utils.h" + +#define RT5514_CODEC_DAI "rt5514-aif1" + +static const struct snd_soc_dapm_widget card_widgets[] = { + SND_SOC_DAPM_MIC("DMIC", NULL), +}; + +static const struct snd_soc_dapm_route card_base_routes[] = { + /* DMIC */ + { "DMIC1L", NULL, "DMIC" }, + { "DMIC1R", NULL, "DMIC" }, + { "DMIC2L", NULL, "DMIC" }, + { "DMIC2R", NULL, "DMIC" }, +}; + +static int avs_rt5514_codec_init(struct snd_soc_pcm_runtime *runtime) +{ + int ret = snd_soc_dapm_ignore_suspend(&runtime->card->dapm, "DMIC"); + + if (ret) + dev_err(runtime->dev, "DMIC - Ignore suspend failed = %d\n", ret); + + return ret; +} + +static int avs_rt5514_be_fixup(struct snd_soc_pcm_runtime *runtime, + struct snd_pcm_hw_params *params) +{ + struct snd_interval *rate, *channels; + struct snd_mask *fmt; + + rate = hw_param_interval(params, SNDRV_PCM_HW_PARAM_RATE); + channels = hw_param_interval(params, SNDRV_PCM_HW_PARAM_CHANNELS); + fmt = hw_param_mask(params, SNDRV_PCM_HW_PARAM_FORMAT); + + rate->min = rate->max = 48000; + channels->min = channels->max = 4; + + snd_mask_none(fmt); + snd_mask_set_format(fmt, SNDRV_PCM_FORMAT_S16_LE); + + return 0; +} + +static int avs_rt5514_hw_params(struct snd_pcm_substream *substream, + struct snd_pcm_hw_params *params) +{ + struct snd_soc_pcm_runtime *rtd = snd_soc_substream_to_rtd(substream); + struct snd_soc_dai *codec_dai = snd_soc_rtd_to_codec(rtd, 0); + int ret; + + ret = snd_soc_dai_set_tdm_slot(codec_dai, 0xF, 0, 8, 16); + if (ret < 0) { + dev_err(rtd->dev, "set TDM slot err:%d\n", ret); + return ret; + } + + ret = snd_soc_dai_set_sysclk(codec_dai, RT5514_SCLK_S_MCLK, 24576000, SND_SOC_CLOCK_IN); + if (ret < 0) + dev_err(rtd->dev, "set sysclk err: %d\n", ret); + + return ret; +} + +static const struct snd_soc_ops avs_rt5514_ops = { + .hw_params = avs_rt5514_hw_params, +}; + +static int avs_create_dai_link(struct device *dev, const char *platform_name, int ssp_port, + int tdm_slot, struct snd_soc_dai_link **dai_link) +{ + struct snd_soc_dai_link_component *platform; + struct snd_soc_dai_link *dl; + + dl = devm_kzalloc(dev, sizeof(*dl), GFP_KERNEL); + platform = devm_kzalloc(dev, sizeof(*platform), GFP_KERNEL); + if (!dl || !platform) + return -ENOMEM; + + platform->name = platform_name; + + dl->name = devm_kasprintf(dev, GFP_KERNEL, + AVS_STRING_FMT("SSP", "-Codec", ssp_port, tdm_slot)); + dl->cpus = devm_kzalloc(dev, sizeof(*dl->cpus), GFP_KERNEL); + dl->codecs = devm_kzalloc(dev, sizeof(*dl->codecs), GFP_KERNEL); + if (!dl->name || !dl->cpus || !dl->codecs) + return -ENOMEM; + + dl->cpus->dai_name = devm_kasprintf(dev, GFP_KERNEL, + AVS_STRING_FMT("SSP", " Pin", ssp_port, tdm_slot)); + dl->codecs->name = devm_kasprintf(dev, GFP_KERNEL, "i2c-10EC5514:00"); + dl->codecs->dai_name = devm_kasprintf(dev, GFP_KERNEL, RT5514_CODEC_DAI); + if (!dl->cpus->dai_name || !dl->codecs->name || !dl->codecs->dai_name) + return -ENOMEM; + + dl->num_cpus = 1; + dl->num_codecs = 1; + dl->platforms = platform; + dl->num_platforms = 1; + dl->id = 0; + dl->dai_fmt = SND_SOC_DAIFMT_DSP_B | SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_CBS_CFS; + dl->init = avs_rt5514_codec_init; + dl->be_hw_params_fixup = avs_rt5514_be_fixup; + dl->nonatomic = 1; + dl->no_pcm = 1; + dl->dpcm_capture = 1; + dl->ops = &avs_rt5514_ops; + + *dai_link = dl; + + return 0; +} + +static int avs_rt5514_probe(struct platform_device *pdev) +{ + struct snd_soc_dai_link *dai_link; + struct snd_soc_acpi_mach *mach; + struct snd_soc_card *card; + struct device *dev = &pdev->dev; + const char *pname; + int ssp_port, tdm_slot, ret; + + mach = dev_get_platdata(dev); + pname = mach->mach_params.platform; + + ret = avs_mach_get_ssp_tdm(dev, mach, &ssp_port, &tdm_slot); + if (ret) + return ret; + + ret = avs_create_dai_link(dev, pname, ssp_port, tdm_slot, &dai_link); + if (ret) { + dev_err(dev, "Failed to create dai link: %d", ret); + return ret; + } + + card = devm_kzalloc(dev, sizeof(*card), GFP_KERNEL); + if (!card) + return -ENOMEM; + + card->name = "avs_rt5514"; + card->dev = dev; + card->owner = THIS_MODULE; + card->dai_link = dai_link; + card->num_links = 1; + card->dapm_widgets = card_widgets; + card->num_dapm_widgets = ARRAY_SIZE(card_widgets); + card->dapm_routes = card_base_routes; + card->num_dapm_routes = ARRAY_SIZE(card_base_routes); + card->fully_routed = true; + + ret = snd_soc_fixup_dai_links_platform_name(card, pname); + if (ret) + return ret; + + return devm_snd_soc_register_card(dev, card); +} + +static struct platform_driver avs_rt5514_driver = { + .probe = avs_rt5514_probe, + .driver = { + .name = "avs_rt5514", + .pm = &snd_soc_pm_ops, + }, +}; + +module_platform_driver(avs_rt5514_driver); + +MODULE_LICENSE("GPL"); +MODULE_ALIAS("platform:avs_rt5514");
To support AVS-rt5514 configuration add machine board connecting AVS platform component driver with rt5514 codec one. Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com> --- sound/soc/intel/avs/boards/Kconfig | 10 ++ sound/soc/intel/avs/boards/Makefile | 2 + sound/soc/intel/avs/boards/rt5514.c | 187 ++++++++++++++++++++++++++++ 3 files changed, 199 insertions(+) create mode 100644 sound/soc/intel/avs/boards/rt5514.c