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

[Dylan Reid]

On Wed, Apr 9, 2014 at 2:22 AM, Thierry Reding <thierry.reding at 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 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.

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_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"?

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