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

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

On 2025-07-03 17:12, Takashi Iwai wrote:
> On Thu, 03 Jul 2025 16:52:10 +0200,
> Amadeusz Sławiński wrote:
>>
>>
>>
>> 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.
> 
> Interesting.  So the device_is_bound() returns true if it's called
> during the binding operation.
> 
>> I will run more tests on the working one tomorrow, unless you want to
>> try something else.
> 
> Thanks!

Apart from unrelated issue in snd_soc_avs driver for which I just sent a 
fix [1]. Everything looks good to me.

[1] 
https://lore.kernel.org/linux-sound/20250708105009.1883627-1-amadeuszx.slawinski@linux.intel.com/T/#u