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

[Thierry Reding]

On Tue, Apr 08, 2014 at 12:15:33PM -0700, 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 thw module params, power_save and probe_mask.

s/thw/the/?

> diff --git a/Documentation/devicetree/bindings/sound/nvidia,tegra124-hda.txt b/Documentation/devicetree/bindings/sound/nvidia,tegra124-hda.txt
> new file mode 100644
> index 0000000..c9256d3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/nvidia,tegra124-hda.txt
> @@ -0,0 +1,20 @@
> +NVIDIA Tegra124 HDA controller
> +
> +Required properties:
> +- compatible : "nvidia,tegra124-hda"

From what I can tell this module hasn't changed significantly since
Tegra30 and therefore should be backwards compatible.

The convention so far has always been to name the document after the
first SoC generation that has the IP block. Similarily the driver should
match nvidia,tegra30-hda and use compatible strings for later SoC
generations.

Maybe an example will clarify what I mean:

	tegra30.dtsi:

		hda at 70030000 {
			compatible = "nvidia,tegra30-hda";
			...
		};

	tegra124.dtsi:

		hda at 70030000 {
			compatible = "nvidia,tegra124-hda", "nvidia,tegra30-hda";
			...
		};

I suspect that it will be fine if the driver only matched the compatible
string "nvidia,tegra30-hda" because the block hasn't changed since then.

> +- clocks : Must contain an entry for each required entry in clock-names.
> +- clock-names : Must include the following entries: hda, hdacodec_2x, hda2hdmi

I think in addition to the clocks this will also need resets and
reset-names properties.

> --- a/sound/pci/hda/Kconfig
> +++ b/sound/pci/hda/Kconfig
> @@ -20,6 +20,20 @@ config SND_HDA_INTEL
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called snd-hda-intel.
>  
> +config SND_HDA_TEGRA
> +	tristate "Tegra HD Audio"

Perhaps "NVIDIA Tegra HD Audio"?

> +	select SND_HDA
> +	help
> +	  Say Y here to support the HDA controller present in Nvidia

Nit: While it's not used consistently everywhere, I'd prefer if we could
move towards using only "NVIDIA" rather than the current mix (which does
include "Nvidia" and "nVidia").

> diff --git a/sound/pci/hda/hda_tegra.c b/sound/pci/hda/hda_tegra.c
[...]
> +#include <linux/clk.h>
> +#include <linux/clocksource.h>
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/moduleparam.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/mutex.h>
> +#include <linux/reboot.h>
> +#include <linux/io.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/of_device.h>
> +#include <linux/time.h>
> +#include <linux/completion.h>
> +
> +#include <sound/core.h>
> +#include <sound/initval.h>
> +#include <linux/firmware.h>

The ordering looks weird here. I'd prefer these to be alphabetically
sorted.

> +#define DRV_NAME "tegra-hda"

This is only used in one place (in the platform_driver initialization),
so I don't think the #define is needed here.

> +/* Defines for Nvidia Tegra HDA support */
> +#define NVIDIA_TEGRA_HDA_BAR0_OFFSET           0x8000
> +
> +#define NVIDIA_TEGRA_HDA_CFG_CMD_OFFSET        0x1004
> +#define NVIDIA_TEGRA_HDA_CFG_BAR0_OFFSET       0x1010

I think the _OFFSET suffix here isn't required. It just makes the name
needlessly long. And since all of these definitions aren't visible
outside of this file, I think the NVIDIA_TEGRA_ prefix can be dropped as
well.

> +struct hda_tegra_data {

Perhaps just "hda_tegra"?

> +	struct azx chip;
> +	struct platform_device *pdev;

Can't this be simply struct device?

> +	struct clk **platform_clks;
> +	int platform_clk_count;

Why the "platform_" prefix? Also I'm not a huge fan of accumulating
clocks into an array like this. I'm not sure it has much of an advantage
code-wise, since there's quite a bit of setup code required to allocate
the array and populate it with the proper clocks.

> +	void __iomem *remap_config_addr;

This is a really long name. Perhaps simply "base" or "regs" would work
just as well?

> +};
> +
> +static int probe_mask[SNDRV_CARDS] = {[0 ... (SNDRV_CARDS-1)] = -1};
> +module_param_array(probe_mask, int, NULL, 0444);
> +MODULE_PARM_DESC(probe_mask, "Bitmask to probe codecs (default = -1).");

Is this really necessary? Do we need this with Tegra? Can't we simply
assume that there's always only one codec? Or always use a probe_mask of
-1? Alternatively, wouldn't this be better off in DT?

> +static int power_save = CONFIG_SND_HDA_POWER_SAVE_DEFAULT;
> +static int *power_save_addr = &power_save;

Why keep this in a separate variable? It seems to me like you can simply
use &power_save directly at the one location where this is used?

> +module_param(power_save, bint, 0644);
> +MODULE_PARM_DESC(power_save,
> +		 "Automatic power-saving timeout (in seconds, 0 = disable).");

Thinking more about it, this seems like the thing you'd want for
something more generic such as PCI HDA where there may actually be more
variance. Is this still useful on Tegra?

> +/*
> + * DMA page allocation ops.
> + */
> +static int dma_alloc_pages(struct azx *chip,
> +			   int type,
> +			   size_t size,
> +			   struct snd_dma_buffer *buf)

This could be fewer lines:

	static int dma_alloc_pages(struct azx *chip, int type, size_t size,
				   struct snd_dma_buffer *buf)

> +{
> +	return snd_dma_alloc_pages(type,
> +				   chip->card->dev,
> +				   size, buf);

This fits perfectly on a single line.

> +/*
> + * Register access ops. Tegra HDA register access is DWORD only.
> + */
> +#define MASK_LONG_ALIGN		0x3UL
> +#define SHIFT_BYTE		3

I think these are obvious enough not to require symbolic names.

> +#define SHIFT_BITS(addr)	\
> +	(((unsigned int)(addr) & MASK_LONG_ALIGN) << SHIFT_BYTE)
> +#define ADDR_ALIGN_L(addr)	\
> +	(void *)((unsigned int)(addr) & ~MASK_LONG_ALIGN)

Can you use ALIGN() or PTR_ALIGN() here?

> +#define MASK(bits)		(BIT(bits) - 1)
> +#define MASK_REG(addr, bits)	(MASK(bits) << SHIFT_BITS(addr))
> +
> +static void tegra_hda_writel(u32 value, u32 *addr)
> +{
> +	writel(value, addr);
> +}
> +
> +static u32 tegra_hda_readl(u32 *addr)
> +{
> +	return readl(addr);
> +}
> +
> +static void tegra_hda_writew(u16 value, u16 *addr)
> +{
> +	unsigned int shift_bits = SHIFT_BITS(addr);
> +	writel((readl(ADDR_ALIGN_L(addr)) & ~MASK_REG(addr, 16)) |
> +	       ((value) << shift_bits), ADDR_ALIGN_L(addr));
> +}
> +
> +static u16 tegra_hda_readw(u16 *addr)
> +{
> +	return (readl(ADDR_ALIGN_L(addr)) >> SHIFT_BITS(addr)) & MASK(16);
> +}
> +
> +static void tegra_hda_writeb(u8 value, u8 *addr)
> +{
> +	writel((readl(ADDR_ALIGN_L(addr)) & ~MASK_REG(addr, 8)) |
> +	       ((value) << SHIFT_BITS(addr)), ADDR_ALIGN_L(addr));
> +}
> +
> +static u8 tegra_hda_readb(u8 *addr)
> +{
> +	return (readl(ADDR_ALIGN_L(addr)) >> SHIFT_BITS(addr)) & MASK(8);
> +}

It took me a really long time to review this and resolving all these
references. Can we not make this simpler somehow? Perhaps unwinding this
a bit may help, like this:

	static void tegra_hda_writew(u16 value, u16 *addr)
	{
		unsigned long shift = (addr & 0x3) << 3;
		unsigned long address = addr & ~0x3;
		u32 v;

		v = readl(address);
		v &= ~(0xffff << shift);
		v |= value << shift;
		writel(v, address);
	}

> +static const struct hda_controller_ops tegra_hda_reg_ops = {

Since these aren't only register operations, perhaps "tegra_hda_ops"?

> +static int tegra_hda_acquire_irq(struct azx *chip, int do_disconnect)

Shouldn't do_disconnect be bool?

> +{
> +	struct hda_tegra_data *tdata =

Nit: tdata is somewhat generic, perhaps simply call this "hda"?

> +		container_of(chip, struct hda_tegra_data, chip);

This pattern is repeated a few times, so perhaps add a helper such as:

	static inline struct hda_tegra *to_hda_tegra(struct azx *chip)
	{
		return container_of(chip, struct hda_tegra, chip);
	}

> +	int irq_id = platform_get_irq(tdata->pdev, 0);

I think this should be part of tegra_hda_probe().

> +	if (devm_request_irq(chip->card->dev, irq_id, azx_interrupt,
> +			     IRQF_SHARED, KBUILD_MODNAME, chip)) {

Perhaps store the error so that you can return (and print) the exact
error code?

> +		dev_err(chip->card->dev,
> +			"unable to grab IRQ %d, disabling device\n",

s/grab/request/?

> +static void tegra_hda_reg_update_bits(void __iomem *base, unsigned int reg,
> +				      unsigned int mask, unsigned int val)
> +{
> +	unsigned int data;
> +
> +	data = readl(base + reg);
> +	data &= ~mask;
> +	data |= (val & mask);
> +	writel(data, base + reg);
> +}
> +
> +static void hda_tegra_init(struct hda_tegra_data *tdata)
> +{
> +	/*Enable the PCI access */
> +	tegra_hda_reg_update_bits(tdata->remap_config_addr,
> +				  NVIDIA_TEGRA_HDA_IPFS_CONFIG,
> +				  NVIDIA_TEGRA_HDA_IPFS_EN_FPCI,
> +				  NVIDIA_TEGRA_HDA_IPFS_EN_FPCI);

I prefer explicit read-modify-write sequences...

> +	/* Enable MEM/IO space and bus master */
> +	tegra_hda_reg_update_bits(tdata->remap_config_addr,
> +				  NVIDIA_TEGRA_HDA_CFG_CMD_OFFSET, 0x507,

... because it makes it easier to see that 0x507 is actually a mask and
should get a symbolic name too, to make it obvious what fields are being
cleared.

> +				  NVIDIA_TEGRA_HDA_ENABLE_MEM_SPACE |
> +				  NVIDIA_TEGRA_HDA_ENABLE_IO_SPACE |
> +				  NVIDIA_TEGRA_HDA_ENABLE_BUS_MASTER |
> +				  NVIDIA_TEGRA_HDA_ENABLE_SERR);
> +	tegra_hda_reg_update_bits(tdata->remap_config_addr,
> +				  NVIDIA_TEGRA_HDA_CFG_BAR0_OFFSET, 0xFFFFFFFF,
> +				  NVIDIA_TEGRA_HDA_BAR0_INIT_PROGRAM);

Also these are just plain writes, so no need to actually clear the
register first.

> +
> +	return;

Unnecessary return statement.

> +static void hda_tegra_enable_clocks(struct hda_tegra_data *data)
> +{
> +	int i;

If you really insist on keeping the clock array, then this should be
unsigned int.

> +static int tegra_hda_runtime_resume(struct device *dev)
> +{
> +	struct snd_card *card = dev_get_drvdata(dev);
> +	struct azx *chip = card->private_data;
> +	struct hda_bus *bus;

I don't think this particular temporary gains much.

> +static const struct dev_pm_ops azx_pm = {

Perhaps tegra_hda_pm since these are Tegra-specific?

> +static int tegra_hda_free(struct azx *chip)
> +{
> +	int i;
> +
> +	if (chip->running)
> +		pm_runtime_get_noresume(chip->card->dev);
> +
> +	tegra_hda_notifier_unregister(chip);
> +
> +	if (chip->initialized) {
> +		for (i = 0; i < chip->num_streams; i++)
> +			azx_stream_stop(chip, &chip->azx_dev[i]);
> +		azx_stop_chip(chip);
> +	}
> +
> +	azx_free_stream_pages(chip);
> +
> +	return 0;
> +}
> +
> +static int tegra_hda_dev_free(struct snd_device *device)
> +{
> +	return tegra_hda_free(device->device_data);
> +}

These can be merged, since tegra_hda_free() isn't used elsewhere.

> +static int hda_tegra_init_chip(struct azx *chip)
> +{
> +	struct hda_tegra_data *tdata =
> +		container_of(chip, struct hda_tegra_data, chip);
> +	struct device *dev = &tdata->pdev->dev;
> +	struct resource *res, *region;
> +	int i;
> +
> +	tdata->platform_clk_count = ARRAY_SIZE(tegra_clk_names);
> +	tdata->platform_clks = devm_kzalloc(dev,
> +					    tdata->platform_clk_count *
> +						sizeof(*tdata->platform_clks),
> +					    GFP_KERNEL);

That's devm_kcalloc().

> +	res = platform_get_resource(tdata->pdev, IORESOURCE_MEM, 0);
> +	if (!res)
> +		return -EINVAL;
> +
> +	region = devm_request_mem_region(dev, res->start,
> +					 resource_size(res),
> +					 tdata->pdev->name);
> +	if (!region)
> +		return -ENOMEM;
> +
> +	chip->addr = res->start;
> +	chip->remap_addr = devm_ioremap(dev, res->start, resource_size(res));
> +	if (!chip->remap_addr)
> +		return -ENXIO;

All of the above can be rewritten as:

	res = platform_get_resource(tdata->pdev, IORESOURCE_MEM, 0);
	chip->remap_addr = devm_ioremap_resource(dev, res);
	if (IS_ERR(chip->remap_addr))
		return PTR_ERR(chip->remap_addr);

	chip->addr = res->start;

> +	tdata->remap_config_addr = chip->remap_addr;
> +	chip->remap_addr += NVIDIA_TEGRA_HDA_BAR0_OFFSET;
> +	chip->addr += NVIDIA_TEGRA_HDA_BAR0_OFFSET;

Perhaps a more intuitive way to write this would be (including my rename
suggestions):

	hda->regs = devm_ioremap_resource(...);
	...

	chip->remap_addr = hda->regs + NVIDIA_TEGRA_HDA_BAR0_OFFSET;
	chip->addr = res->start + NVIDIA_TEGRA_HDA_BAR0_OFFSET;

> +
> +	hda_tegra_enable_clocks(tdata);
> +
> +	hda_tegra_init(tdata);

Shouldn't both of these return an error which can be propagated on
failure?

> +static void power_down_all_codecs(struct azx *chip)
> +{
> +	/* The codecs were powered up in snd_hda_codec_new().
> +	 * Now all initialization done, so turn them down if possible
> +	 */
> +	struct hda_codec *codec;

Nit: Perhaps put the variable declaration before the comment. Or
alternatively put the comment above the function.

Also block comments are usually of this format:

	/*
	 * bla...
	 * ... bla.
	 */

> +static int tegra_hda_first_init(struct azx *chip)
> +{
> +	struct snd_card *card = chip->card;
> +	int err;
> +	unsigned short gcap;
> +
> +	err = hda_tegra_init_chip(chip);
> +	if (err)
> +		return err;
> +
> +	if (tegra_hda_acquire_irq(chip, 0) < 0)
> +		return -EBUSY;

Why not propagate whatever tegra_hda_acquire_irq() returned?

> +	synchronize_irq(chip->irq);
> +
> +	gcap = azx_readw(chip, GCAP);
> +	dev_dbg(card->dev, "chipset global capabilities = 0x%x\n", gcap);
> +
> +	/* 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;
> +	}
> +	chip->capture_index_offset = 0;
> +	chip->playback_index_offset = chip->capture_streams;
> +	chip->num_streams = chip->playback_streams + chip->capture_streams;
> +	chip->azx_dev = devm_kzalloc(card->dev,
> +				     chip->num_streams * sizeof(*chip->azx_dev),
> +				     GFP_KERNEL);

devm_kcalloc()

> +	strcpy(card->driver, "HDA-Tegra");

Perhaps this should match the driver name ("tegra-hda")?

> +/*
> + * constructor
> + */
> +static int tegra_hda_create(struct snd_card *card,
> +			    int dev,
> +			    struct platform_device *pdev,
> +			    unsigned int driver_caps,
> +			    const struct hda_controller_ops *hda_ops,
> +			    struct hda_tegra_data *tdata)

This could be fewer lines.

> +	err = snd_device_new(card, SNDRV_DEV_LOWLEVEL, chip, &ops);
> +	if (err < 0) {
> +		dev_err(card->dev, "Error creating device [card]!\n");

Perhaps drop "[card]"? Also I don't think the exclamation mark is
required. It's already an error message.

> +static unsigned int tegra_driver_flags = AZX_DCAPS_RIRB_DELAY |
> +					 AZX_DCAPS_PM_RUNTIME;

Perhaps a slightly more future-proof way of doing this would be by
adding a structure, like this:

	struct tegra_hda_soc_info {
		unsigned long flags;
	};

And then instantiate that per SoC:

	static const struct tegra_hda_soc_info tegra124_hda_soc_info = {
		.flags = ...;
	};

Although looking at the above AZX_* flags, they don't seem to be SoC-
but but rather driver-specific, so they don't really belong in the OF
device match table.

> +static const struct of_device_id tegra_platform_hda_match[] = {

Why not simply "tegra_hda_match"?

> +	{ .compatible = "nvidia,tegra124-hda", .data = &tegra_driver_flags },
> +	{},
> +};
> +
> +static int hda_tegra_probe(struct platform_device *pdev)

There seems to be a mix between hda_tegra_ and tegra_hda_ prefixes
throughout the driver. I like it when these are consistent because it is
easier to refer to the functions (you don't always have to remember
which function has which prefix).

> +{
> +	static int dev;
> +	struct snd_card *card;
> +	struct azx *chip;
> +	struct hda_tegra_data *tdata;
> +	const struct of_device_id *of_id;
> +	const unsigned int *driver_data;
> +	int err;
> +
> +	if (dev >= SNDRV_CARDS)
> +		return -ENODEV;

I'd really like to avoid this. But I also realize that this is something
inherited from the sound subsystem, so I'm unsure about how easy it is
to get rid of it.

> +	of_id = of_match_device(tegra_platform_hda_match, &pdev->dev);
> +	if (!of_id)
> +		return -EINVAL;

Perhaps -ENODEV?

> +	dev_set_drvdata(&pdev->dev, card);

platform_set_drvdata()?

> +static struct platform_driver tegra_platform_hda = {
> +	.driver = {
> +		.name = DRV_NAME,
> +		.owner = THIS_MODULE,

This is no longer required.

> +		.pm = &azx_pm,
> +		.of_match_table = tegra_platform_hda_match,
> +	},
> +	.probe = hda_tegra_probe,
> +	.remove = hda_tegra_remove,
> +};
> +module_platform_driver(tegra_platform_hda);
> +
> +MODULE_DESCRIPTION("Tegra HDA bus driver");
> +MODULE_LICENSE("GPL v2");

The file header says "GPL v2 or later", so either this or the header
needs to be updated.

> +MODULE_DEVICE_TABLE(of, tegra_platform_hda_match);

I think it's more idiomatic to put this directly below the OF match
table.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20140409/441c0946/attachment-0001.sig>