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

[Ughreja, Rakesh A]

>-----Original Message-----
>From: Pierre-Louis Bossart [mailto:pierre-louis.bossart at linux.intel.com]
>Sent: Tuesday, February 27, 2018 12:41 AM
>To: Ughreja, Rakesh A <rakesh.a.ughreja at intel.com>; alsa-devel at alsa-
>project.org; broonie at kernel.org; tiwai at suse.de;
>liam.r.girdwood at linux.intel.com
>Cc: Koul, Vinod <vinod.koul at intel.com>; Patches Audio
><patches.audio at intel.com>
>Subject: Re: [alsa-devel] [PATCH v1 1/9] ASoC: Intel: Boards: Machine driver for
>Intel platforms
>
>
>>>> +	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.
>

So is it clear now ? Do I need to change anything ?

>
>>>> +/* 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.
>

Yes, I can make default as NULL.

>
>>>> +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?

Since its used only within the file, I marked it as 
static const char *

>
>>>> +		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?

platform_name variable is used in skl_hda_add_dai_link 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?

I would like to use different IDs for different combination of the codecs.
e.g. HDMI Only, HDA+HDMI to begin with.

>
>>
>>>
>>>> +};
>>>> +
>>>> +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.

This is mentioned in the commit message.