Re: [PATCH 1/2] ALSA: hda/hdmi: Add helper function to check if a device is HDMI codec

[Péter Ujfalusi <peter.ujfalusi@xxxxxxxxxxxxxxx>]

On 27/11/2023 17:20, Takashi Iwai wrote:
> On Mon, 27 Nov 2023 15:45:54 +0100,
> Péter Ujfalusi wrote:
>>
>>
>>
>> On 27/11/2023 16:31, Takashi Iwai wrote:
>>> On Mon, 27 Nov 2023 15:12:51 +0100,
>>> Péter Ujfalusi wrote:
>>>>
>>>>
>>>>
>>>> On 27/11/2023 15:18, Takashi Iwai wrote:
>>>>>> +bool snd_hda_device_is_hdmi(struct hdac_device *hdev)
>>>>>> +{
>>>>>> +	int i;
>>>>>> +
>>>>>> +	for (i = 0; i < ARRAY_SIZE(snd_hda_id_hdmi); i++) {
>>>>>> +		if (snd_hda_id_hdmi[i].vendor_id == hdev->vendor_id)
>>>>>> +			return true;
>>>>>> +	}
>>>>>> +
>>>>>> +	return false;
>>>>>> +}
>>>>>> +EXPORT_SYMBOL_GPL(snd_hda_device_is_hdmi);
>>>>>
>>>>> I'm afraid that this will bring unnecessary dependency on HDMI codec
>>>>> driver.
>>>>
>>>> For HDMI support we anyways need HDMI code?
>>>
>>> But the ASoC hdac-hda driver isn't specifically bound with HDMI, I
>>> thought?
>>>
>>> With your patch, now it becomes a hard-dependency.  It'll be even
>>> build failure when HDMI codec driver isn't enabled in Kconfig.
>>
>> The change in hdaudio.h handles the config dependency, if
>> CONFIG_SND_HDA_CODEC_HDMI is not enabled in Kconfig then
>> snd_hda_device_is_hdmi() will return false.
> 
> OK, that's at least good.
> But I still find it not ideal to bring the hard dependency there
> unnecessarily.
> 
> With the introduction of a flag in hdac_device, the necessary change
> would be even smaller like below.
> 
> 
> Takashi
> 
> --- a/include/sound/hdaudio.h
> +++ b/include/sound/hdaudio.h
> @@ -95,6 +95,7 @@ struct hdac_device {
>  	bool lazy_cache:1;	/* don't wake up for writes */
>  	bool caps_overwriting:1; /* caps overwrite being in process */
>  	bool cache_coef:1;	/* cache COEF read/write too */
> +	bool is_hdmi:1;		/* a HDMI/DP codec */
>  	unsigned int registered:1; /* codec was registered */
>  };
>  
> --- a/sound/pci/hda/patch_hdmi.c
> +++ b/sound/pci/hda/patch_hdmi.c
> @@ -2597,6 +2597,7 @@ static int patch_generic_hdmi(struct hda_codec *codec)
>  	}
>  
>  	generic_hdmi_init_per_pins(codec);
> +	codec->core.is_hdmi = true;
>  	return 0;
>  }
>  
> @@ -3472,6 +3473,7 @@ static int patch_simple_hdmi(struct hda_codec *codec,
>  	spec->pcm_playback = simple_pcm_playback;
>  
>  	codec->patch_ops = simple_hdmi_patch_ops;
> +	codec->core.is_hdmi = true;
>  
>  	return 0;
>  }

I see,  so this is what I was not sure how to do ;)
I will rework the series and resend tomorrow.

Thanks for the code snippet, I will make you as author of it, if it is
OK for you.

-- 
Péter