On Wed, Apr 9, 2014 at 2:22 AM, Thierry Reding thierry.reding@gmail.com wrote:
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/?
Thanks a lot for the thorough and detailed review, Theirry. It is very much appreciated, this looks much better after addressing your comments, v2 on the way.
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@70030000 { compatible = "nvidia,tegra30-hda"; ... }; tegra124.dtsi: hda@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.
Thanks, hadn't seen that one yet, I'll add it.
--- 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_HDAhelpSay Y here to support the HDA controller present in NvidiaNit: 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"?
Good call on shortening these, saved a few wrapped lines.
struct azx chip;struct platform_device *pdev;Can't this be simply struct device?
Yes, the pdev was only needed at probe time.
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.
10 lines less code without the array =)
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?
Removed per following discussion, thanks.
+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?
There are userspace tools that take advantage of this. For example laptop mode tools changes this setting based on whether the system is attached to A/C or not.
+/*
- 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?
Both of those end up using __ALIGN_KERNEL and that rounds up. 0x01 -> 0x04, here we want 0x01 -> 0x04. Would have been nice, I didn't see another pre-defined macro that did this.
+#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); }
Good suggestion, went with almost exactly that. Thanks!
+static const struct hda_controller_ops tegra_hda_reg_ops = {
Since these aren't only register operations, perhaps "tegra_hda_ops"?
Agreed.
+static int tegra_hda_acquire_irq(struct azx *chip, int do_disconnect)
Shouldn't do_disconnect be bool?
Argument removed as it was left over from the PCI driver and not needed here.
+{
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().
I moved this with the rest of this function to be inline with the first_init call made from 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.
breaking these into explicit read/modify/write made this cleaner and allowed several steps to be skipped.
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.
Array gone.
+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?
hda_tegra_init can't create and error. enable_clocks on the other hand should handle clk_prepare_enable failing, I'll add that.
+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?
Done, code inlined here and returns any error code returned from request_irq.
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.
I think you're right, I'll move these out of the table and make the flags local to the probe function.
+static const struct of_device_id tegra_platform_hda_match[] = {
Why not simply "tegra_hda_match"?
Changed.
{ .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).
Good point, the split was almost 50/50, change to always put hda before tegra.
+{
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.
In this case we're only going to have one card, I think we can safely drop this, and set the dev_index in the chip structure to zero.
of_id = of_match_device(tegra_platform_hda_match, &pdev->dev);if (!of_id)return -EINVAL;Perhaps -ENODEV?
That's better.
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.
Removed.
.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.
Header updated.
+MODULE_DEVICE_TABLE(of, tegra_platform_hda_match);
I think it's more idiomatic to put this directly below the OF match table.
Moved.
Thierry