On Mon, Apr 21, 2014 at 1:02 PM, Stephen Warren swarren@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@70030000 {
that should be named hda@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_TEGRAOF 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@nvidia.com
Thanks for reviewing this Stephen, I'll run a test on the Tegra30 tomorrow and post a new version.
-Dylan