[alsa-devel] [PATCH v1 1/9] ASoC: Intel: Boards: Machine driver for Intel platforms

[Pierre-Louis Bossart]

>>> +	select SND_SOC_HDAC_HDMI
>>> +	help
>>> +	  This adds support for ASoC Onboard Codec HDA machine driver. This
>> will
>>> +	  create an alsa sound card for HDA Codecs.
>>
>> does this handle idisp as well? If yes, does this option conflict with
>> the enablement of the other machine drivers which use the hdac_hdmi codec?
> 
> Yes, the machine driver does handle iDisp codec. I didn't understand your
> comment about conflict and other machine driver using the hdac_hdmi
> codec. There is only one iDisp codec and so what is the reason for having
> two machine drivers accessing the same codec ?

thinking a bit more, all the existing machine drivers which provide 
support for HDMI/iDisp also cater to I2S codecs. Since you deal with 
HDaudio only if you haven't found a known I2S codec there is no real 
source of conflict.


>>> +/* skl_hda_digital audio interface glue - connects codec <--> CPU */
>>> +struct snd_soc_dai_link skl_hda_be_dai_links[HDA_DSP_MAX_BE_DAI_LINKS]
>> = {
>>> +
>>> +	/* Back End DAI links */
>>> +	{
>>> +		.name = "iDisp1",
>>> +		.id = 1,
>>> +		.cpu_dai_name = "iDisp1 Pin",
>>> +		.codec_name = "ehdaudio0D2",
>>> +		.codec_dai_name = "intel-hdmi-hifi1",
>>> +		.platform_name = "0000:00:1f.3",
>>
>> what happens if you run on a device with a different ID, e.g. APL or GLK?
> 
> This is just a default ID. The same machine driver should work.
> The platform name (ID) is passed from platform driver using platform_data.
> You can see that in the later part of the code. So this name gets overwritten.

i don't see the point of having a default then? It could just as well be 
"deadbeef" or not initialized to avoid storing useless strings.


>>> +static const char *platform_name = "0000:00:1f.3";
>>
>> same comment as above, this is not constant across all devices
> 
> Yes and its overwritten when it is received from platform driver.
> You can see in the later part of the code.
> 
> Copy/pasting the code here
> 
>>> +	pdata = dev_get_drvdata(&pdev->dev);
>>> +	if (pdata && pdata->platform) {
>>> +		platform_name = pdata->platform;

can be be a const char * then?

>>> +		for (i = 0; i < ctx->pcm_count; i++)
>>> +			skl_hda_be_dai_links[i].platform_name =
>> platform_name;

and why do you need a static variable in the first place, wouldn't

skl_hda_be_dai_links[i].platform_name = pdata->platform work just as well?


>>> +static const struct platform_device_id skl_hda_board_ids[] = {
>>> +	{ .name = "skl_hda_generic" },
>>> +	{ }
>>> +};
>>> +
>>> +static struct platform_driver skl_hda_audio = {
>>> +	.probe = skl_hda_audio_probe,
>>> +	.driver = {
>>> +		.name = "skl_hda_generic",
>>> +		.pm = &snd_soc_pm_ops,
>>> +	},
>>> +	.id_table = skl_hda_board_ids,
>>
>> is this needed if you have a single id?
> 
> I did the way other machine drivers are doing.
> But I think it's not needed for single ID. But I think it's this way so that
> more IDs can be added ? So I will keep it as it is.

what other IDS would you use?

> 
>>
>>> +};
>>> +
>>> +module_platform_driver(skl_hda_audio)
>>> +
>>> +/* Module information */
>>> +MODULE_DESCRIPTION("SKL/KBL HDA Generic Machine driver");
>>
>> I don't see how this handles HDAudio codecs in general, is this limited
>> to iDISP/HDMI support?
> 
> In this patch only the iDisp/HDMI support is added. In the later patches
> [Patch 9/9] support for HDA codec is added.

yes, but I have to go read 8 patches before finding it out... I comment 
patches one by one.