Re: [PATCH v2] ALSA: hda - Add new driver for HDA controllers listed via ACPI

On Fri, May 16, 2025 at 09:56:13AM +0200, Takashi Iwai wrote:

On Thu, 15 May 2025 17:47:25 +0200, Daniel Dadap wrote:

...

Some systems expose HD-Audio controllers via objects in the ACPI tables which encapsulate the controller's interrupt and the base address for the HDA registers in an ACPI _CRS object, for example, as listed in this ACPI table dump excerpt:

    Device (HDA0)
    {
        Name (_HID, "NVDA2014")  // _HID: Hardware ID
        ...
        Name (_CRS, ResourceTemplate ()  // _CRS: Current Resource Settings
        {
            Memory32Fixed (ReadWrite,
                0x36078000,         // Address Base
                0x00008000,         // Address Length
                )
            Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive, ,, )
            {
                0x0000021E,
            }
        })
    }

Add support for HDA controllers discovered through ACPI, including support for some platforms which expose such HDA controllers on NVIDIA SoCs. This is done with a new driver which uses existing infrastructure for extracting resource information from _CRS objects and plumbs the parsed resource information through to the existing HDA infrastructure to enable HD-Audio functionality on such devices.

Although this driver is in the sound/pci/hda/ directory, it targets devices which are not actually enumerated on the PCI bus. This is because it depends upon the Intel "Azalia" infrastructure which has traditionally been used for PCI-based devices.

Signed-off-by: Daniel Dadap ddadap@nvidia.com

Thanks. Now I checked with checkpatch, and it complained a few things:

Thanks, Takashi. I forgot to ran checkpatch. I addressed the ones you called out, and a few more that you ignored (I think on purpose), except for this:

WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?

I can add myself as maintainer for this file if you like, but figured it could also just fall into the default maintainership of the directory (you). Let me know if you think it's worth changing it.

WARNING: Block comments use a trailing */ on a separate line #168: FILE: sound/pci/hda/hda_acpi.c:79:

• * devm_platform_get_and_ioremap_resource() */
  
  

ERROR: code indent should use tabs where possible #182: FILE: sound/pci/hda/hda_acpi.c:93: +^I IRQF_SHARED, KBUILD_MODNAME, azx);$

ERROR: code indent should use tabs where possible #308: FILE: sound/pci/hda/hda_acpi.c:219: +^I THIS_MODULE, 0, &hda->card);$

I disagree with the above two, but I changed it anyway because it's easier to do that than to argue with checkpatch. These are both continuations of long lines, and the single hard tab matches the actual indentation level of the code itself, with the subsequent spaces serving to aesthetically align the continuation. If someone is viewing the file with the tabstop set to anything other than 8, using hard tabs for the alignment portion will break the intended alignment, whereas using spaces will keep things aligned well regardless of the tabstop size, since the single initial tab will resize consistently with the tabstop.

Perhaps this style point has been discussed before, and the policy that is enforced by checkpatch is intentional for reasons I don't understand (I did not check), but if this behavior is unintentional, and using spaces for aligning continuations of long lines is supposed to be okay, I can look at updating checkpatch to allow it. But for now I'll go with the recommended indentation since that seems to be the style followed by other files here.

WARNING: Prefer "GPL" over "GPL v2" - see commit bf7fbeeae6db ("module: Cure th) #405: FILE: sound/pci/hda/hda_acpi.c:316: +MODULE_LICENSE("GPL v2");

Care to address those and resubmit?

Sure, I'll send a v4 shortly. I also took the opportunity to address an issue I noticed while cleaning up, by adding the following:

hda->data = acpi_device_get_match_data(&pdev->dev); + + /* Fall back to defaults if the table didn't have a *struct hda_data */ + if (!hda->data) + hda->data = devm_kzalloc(&pdev->dev, sizeof(*hda->data), + GFP_KERNEL); + if (!hda->data) + return -ENOMEM; + err = snd_card_new(&pdev->dev, SNDRV_DEFAULT_IDX1, SNDRV_DEFAULT_STR1,

My intention was to allow entries in the match table to omit supplying the pointer to a struct hda_data if they were fine with the defaults (hence use of the language "may be stored" in the comment describing the struct), but without the above the driver will dereference NULL if this is actually done.

thanks,

Takashi