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