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

["Amadeusz Sławiński" <amadeuszx.slawinski@linux.intel.com>]

On 2025-07-03 16:43, Takashi Iwai wrote:
> On Thu, 03 Jul 2025 16:32:51 +0200,
> Takashi Iwai wrote:
>>
>> 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?
This one works.

> A possible alternative is like below.  This one might be safer.
> Let me know if either of them can work for you.

This one doesn't.

I will run more tests on the working one tomorrow, unless you want to 
try something else.