Re: [RFC] Reorganizing HD-audio driver code?

[Takashi Iwai <tiwai@suse.de>]

On Thu, 03 Jul 2025 15:57:48 +0200,
Amadeusz Sławiński wrote:
> 
> 
> 
> On 2025-07-03 15:38, Takashi Iwai wrote:
> > On Thu, 03 Jul 2025 15:29:56 +0200,
> > Amadeusz Sławiński wrote:
> >> 
> >> 
> >> 
> >> On 2025-07-02 16:53, Takashi Iwai wrote:
> >>> On Sun, 29 Jun 2025 11:22:25 +0200,
> >>> Takashi Iwai wrote:
> >>>> 
> >>>> On Fri, 27 Jun 2025 15:22:20 +0200,
> >>>> Richard Fitzgerald wrote:
> >>>>> 
> >>>>> On 27/06/2025 1:04 pm, Takashi Iwai wrote:
> >>>>>> Hi,
> >>>>>> 
> >>>>>> HD-audio driver is known to be quite messy in both file structures and
> >>>>>> its design, but until now I haven't touched its files paths so much
> >>>>>> because I set a higher priority for the easiness of backport to stable
> >>>>>> kernels.  But, you can't leave garbages forever, it's been already
> >>>>>> high time for a large clean up.
> >>>>>> 
> >>>>>> So I tried a quick code reorganization, and put the result in
> >>>>>> test/hda-reorg branch of sound.git tree.
> >>>>>>      https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git/log/?h=test/hda-reorg
> >>>>>> 
> >>>>>> The basic idea is to move the code from sound/pci/hda/* into different
> >>>>>> subdirectories in sound/hda/ per functionality, as most of the stuff
> >>>>>> are independent from PCI, but rather HD-audio bus specific.
> >>>>>> 
> >>>>> 
> >>>>> This all seems reasonable to me. I always thought it strange that there
> >>>>> is a sound/hda directory but most of the HDA support isn't in that
> >>>>> directory.
> >>>> 
> >>>> Thanks for your review!
> >>>> 
> >>>> It's a long story: at the time of ASoC Intel HD-audio support, we
> >>>> thought to implement more ASoC-friendly way of HD-audio controller and
> >>>> codec drivers, e.g. adapting DAPM and others.  So we began with the
> >>>> factoring out the HD-audio basic core stuff to the common directory
> >>>> sound/hda/*, while keeping the rest legacy stuff almost as is.  But
> >>>> ASoC implementation didn't fly in the end from various reasons, and
> >>>> the legacy HD-audio stuff was good enough for the actual use cases;
> >>>> it's too bit to fail, after all.
> >>>> 
> >>>>>> The Realtek codec is split further to smaller pieces (which was really
> >>>>>> huge).
> >>>>> 
> >>>>> Yes, that file was very confusing having support and quirks for so many
> >>>>> Realtek parts all in one file.
> >>>>> 
> >>>>>> The Cirrus and TI sub-codec drivers are moved to codecs/side-codecs
> >>>>>> subdirectory:
> >>>>> 
> >>>>> That's ok
> >>>>> 
> >>>>>> They can be put to each own directory and drop the file name prefix,
> >>>>>> if we want, too.  Let me know if Cirrus and TI people would like to
> >>>>>> split to more subdirectories.
> >>>>> 
> >>>>> I don't mind either way.
> >>>>> The hda_component* files are common to the amps and the realtek driver
> >>>>> so I wonder whether they belong in helpers. They are only utility
> >>>>> wrappers around the kernel component-binding APIs.
> >>>> 
> >>>> Right.  So far, just because it's basically only binding with
> >>>> side-codecs, I put into side-codecs subdirectory.
> >>>> 
> >>>>>> *HOWEVER* the biggest question is: whether it's worth?
> >>>>>> 
> >>>>>> Essentially, this makes almost impossible to make a patch for stable
> >>>>>> trees from the original commit as is; one has to translate the file
> >>>>>> paths and adjust manually in each patch.
> >>>>> 
> >>>>> Depends which file you are patching and how much it has changed.
> >>>>> Git can figure out file renames (and changing directory is a rename).
> >>>> 
> >>>> I'm afraid that the Realtek codec changes might be hard to track
> >>>> automatically.  Maybe the Cirrus side-codecs stuff would work.
> >>>> 
> >>>>>> Also, of course, if anyone is working on HD-audio stuff right now, the
> >>>>>> work had to be adjusted to the new file path.  It'd be one-off action,
> >>>>>> though.
> >>>>> 
> >>>>> Speaking only for Cirrus, I don't think this makes much extra effort
> >>>>> for us. Our amp drivers have only changed location, so that should be
> >>>>> trivial. The other file we change is patch_realtek.c but that typically
> >>>>> is only 1-line quirk entries.
> >>>> 
> >>>> OK, thanks for confirmation!
> >>> 
> >>> ... and now I worked further on this, resulting in larger set of
> >>> changes.  It's not only the file location changes but also the
> >>> HD-audio codec driver binding changes.  So I put more people to Cc for
> >>> catch their cautions.
> >>> 
> >>> To recap:
> >>> 
> >>> - The all HD-audio driver code are moved under sound/hda.
> >>> 
> >>>     % ls sound/hda
> >>>     codecs/  common/  controllers/  core/  Kconfig  Makefile
> >>> 
> >>>     * The former hda core code is found in sound/hda/core.
> >>>     * The former snd-hda-codec code is found in sound/hda/common.
> >>>     * The former snd-hda-intel, tegra and acpi are put in
> >>>       sound/hda/controllers.
> >>>     * The former patch_* and co are put to sound/hda/codecs.
> >>>     * Realtek codec driver is split to several modules as
> >>>       sound/hda/codecs/realtek/alc*.
> >>>     * Cirrus codec driver is split to cs420x and cs421x, put under
> >>>       sound/hda/codecs/cirrus together with cs8409.
> >>>     * HDMI codec driver is split to several modules under
> >>>       sound/hda/codecs/hdmi
> >>>     * Cirrus and TI sub-codecs are put under
> >>>       sound/hda/codecs/side-codecs
> >>> 
> >>> - The HD-audio codec driver binding is changed from the embedded
> >>>     hda_codec.patch_ops to hda_codec_driver.ops.
> >>>     (As of now, hda_codec_driver.ops is a pointer, but it can be
> >>>      embedded later, too.)
> >>> 
> >>>     This change required some code to be modified without the dynamic
> >>>     override of callbacks.
> >>> 
> >>> - In future, we may convert the runtime PM handling to use the
> >>>     standard pm_ops, too.  This was raised some time ago during the
> >>>     discussion with Realtek devs.
> >>> 
> >>> The current patches are found in test/hda-reorg branch of sound git
> >>> tree:
> >>>     https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git/log/?h=test/hda-reorg
> >>> 
> >>> It's based on master branch for taking all changes of for-linus and
> >>> for-next, hence the branch may be still frequently rebased.
> >>> 
> >>> So, please let me know if you see any issues or suggestions for this
> >>> conversion.  I have no concrete plan for merging, but if everything
> >>> looks fine, it can be even on the next 6.17, too.
> >>> 
> >> 
> >> I've built it and put it on one of our machines and hit KASAN during
> >> HDMI initialization, when I go back to master branch it works, so it
> >> is something on test/hda-reorg.
> > 
> > Yes, there was bug in my branch that I fixed in this morning.
> > Could you try the latest test/hda-reorg branch, commit
> > 6491d5b2e256a678b3a138500bf601c916d3913e
> > ?
> > 
> 
> Still broken :(

OK, thanks for confirmation.

I guess it's a power up code path that happens just before the codec
driver initialization.

Could you check whether the patch below covers it?


Takashi

--- a/sound/hda/codecs/hdmi/intelhdmi.c
+++ b/sound/hda/codecs/hdmi/intelhdmi.c
@@ -77,7 +77,7 @@ static void intel_haswell_fixup_enable_dp12(struct hda_codec *codec)
 static void haswell_set_power_state(struct hda_codec *codec, hda_nid_t fg,
 				unsigned int power_state)
 {
-	if (power_state == AC_PWRST_D0) {
+	if (codec->spec && power_state == AC_PWRST_D0) {
 		intel_haswell_enable_all_pins(codec, false);
 		intel_haswell_fixup_enable_dp12(codec);
 	}