[alsa-devel] [PATCHv4 2/2] ALSA: hda - Add driver for Tegra SoC HDA

[Dylan Reid]

On Mon, Apr 21, 2014 at 1:02 PM, Stephen Warren <swarren at wwwdotorg.org> wrote:
> On 04/18/2014 03:46 PM, Dylan Reid wrote:
>> This adds a driver for the HDA block in Tegra SoCs.  The HDA bus is
>> used to communicate with the HDMI codec on Tegra124.
>>
>> Most of the code is re-used from the Intel/PCI HDA driver.  It brings
>> over only two of the module params, power_save and probe_mask.
>
> (I'm curious how this was tested using an upstream kernel considering we
> don't have HDMI support on Tegra124 enabled yet. If you added 1 or 2
> more CODEC IDs to patch 1/2, you could probably test on an earlier SoC
> generation)

Luckily this applies to our downstream tree with hdmi enabled.  The
only change needed is switching back from snd_card_new to
snd_card_create. So that's where I've been doing my testing.

I think we've got a tegra30 eval board.  I'll dig it up tomorrow and
give it a try on linux-next.

>
> Sorry for the slow review...
>
>> diff --git a/Documentation/devicetree/bindings/sound/nvidia,tegra30-hda.txt b/Documentation/devicetree/bindings/sound/nvidia,tegra30-hda.txt
>
>> +- compatible : "nvidia,tegra30-hda"
>> +- reg : Should contain the HDA registers location and length.
>> +- interrupts : The interrupt from the hda controller.
>
> hda should be capitalized.
>
>> +- clocks : Must contain an entry for each required entry in clock-names.
>
> Can you add the following line after that for consistency with other
> Tegra bindings:
>
> See ../clocks/clock-bindings.txt for details.
>
>> +- clock-names : Must include the following entries: hda, hdacodec_2x, hda2hdmi
>> +- resets : Must contain an entry for each entry in reset-names.
>> +  See ../reset/reset.txt for details.
>> +- reset-names : Must include the following entries: hda, hdacodec_2x, hda2hdmi
>> +
>> +Example:
>> +
>> +hda at 70030000 {
>
> that should be named hda at 0,70030000, since the reg property's value
> below assumes the parent has #address-cells=<2>.
>
>> +     compatible = "nvidia,tegra124-hda", "nvidia,tegra30-hda";
>> +     reg = <0x0 0x70030000 0x10000>;
>
> ... and here, since #address-cells=<2>, then #size-cells should be 2
> too, so that should be:
>
>         reg = <0x0 0x70030000 0 0x10000>;
>
>> diff --git a/sound/pci/hda/Kconfig b/sound/pci/hda/Kconfig
>
>> +config SND_HDA_TEGRA
>> +     tristate "NVIDIA Tegra HD Audio"
>> +     depends on OF && ARCH_TEGRA
>
> OF is selected by ARCH_TEGRA, so this only needs to depend on ARCH_TEGRA.
>
> (Of course, you could make this depend on ARCH_TEGRA || (COMPILE_TEST &&
> OF && ...)
>
>> diff --git a/sound/pci/hda/hda_tegra.c b/sound/pci/hda/hda_tegra.c
>> +/*
>> + *
>> + * Implementation of primary alsa driver code base for NVIDIA Tegra HDA.
>
> ALSA should be capitalized.
>
>> +static void hda_tegra_init(struct hda_tegra *hda)
>> +{
>> +     u32 v;
>> +
>> +     /*Enable the PCI access */
>
> There should be a space after /*.
>
>> +static int hda_tegra_init_chip(struct azx *chip, struct platform_device *pdev)
>
>> +     err = hda_tegra_enable_clocks(hda);
>> +     if (err)
>> +             return err;
>
> I'm not sure where the matching disable() occurs? Is the card assumed to
> be started in a powered state, so the next PM transition would be
> hda_tegra_suspend()? IIRC, other Tegra devices with PM start in a
> power-saved state, and hence would leave clocks stopped after probe().
> It's fine if that's the reason; it just looks different so I'm making
> sure that's what is going on.

Yes, this starts in a powered state.  The first chance to power down
is after the call to "power_down_all_codecs" that might arm a
power_save timeout and then power down the controller.  However the
HDMI codec here doesn't report that it supports D3 stop clock so the
controller will stay powered until it is suspended.

>
>> +static int hda_tegra_first_init(struct azx *chip, struct platform_device *pdev)
>
>> +     /* read number of streams from GCAP register instead of using
>> +      * hardcoded value
>> +      */
>> +     chip->capture_streams = (gcap >> 8) & 0x0f;
>> +     chip->playback_streams = (gcap >> 12) & 0x0f;
>> +     if (!chip->playback_streams && !chip->capture_streams) {
>> +             /* gcap didn't give any info, switching to old method */
>> +             chip->playback_streams = ICH6_NUM_PLAYBACK;
>> +             chip->capture_streams = ICH6_NUM_CAPTURE;
>
> Are ICH6_* defines appropriate for Tegra?
>
>> +static int hda_tegra_probe(struct platform_device *pdev)
>
>> +     of_id = of_match_device(hda_tegra_match, &pdev->dev);
>> +     if (!of_id)
>> +             return -ENODEV;
>
> Since of_id isn't used anywhere, there's no point calling
> of_match_device() to look it up. The driver core won't call
> hda_tegra_probe() unless there is a matching entry in the table.
>
>> +MODULE_DEVICE_TABLE(of, tegra_platform_hda_match);
>
> I think it's typical to put that line immediately after the table it
> applies to. Not a big deal though.
>
> With those minor issues fixed,
> Reviewed-by: Stephen Warren <swarren at nvidia.com>

Thanks for reviewing this Stephen, I'll run a test on the Tegra30
tomorrow and post a new version.

-Dylan