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)
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_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.
> +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@xxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
