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

[Daniel Dadap <ddadap@nvidia.com>]

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