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

[Takashi Iwai <tiwai@xxxxxxx>]

On Mon, 27 Nov 2023 16:40:57 +0100,
Péter Ujfalusi wrote:
> 
> 
> 
> 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.

Sure, no problem.


Takashi