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 06:37:21AM -0500, Daniel Dadap wrote:
> 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:

Hi Takashi,

In the meantime I noticed the recent commits in the sound tree to convert
some string copies to use strscpy() with two arguments, so I sent a patch
v5 to also use the two-argument variant in the new driver. I also sent a
separate patch to update hda_tegra to use the new signature.

> 
>         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