Re: [PATCH 1/2] ASoC: Intel: avs: Add rt5514 machine board

[Amadeusz Sławiński <amadeuszx.slawinski@xxxxxxxxxxxxxxx>]

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@xxxxxxxxxxxxxxx>
> > > > ---
> > > >    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?