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

[Krzysztof Kozlowski <krzk@kernel.org>]

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