[alsa-devel] [RFC 00/19] Enable platform HDA drivers

[Takashi Iwai]

At Thu, 27 Feb 2014 22:35:43 -0800,
Dylan Reid wrote:
> 
> This series rearranges some code in the HDA driver to break
> dependencies on PCI and allow for most of the HDA code to be reused
> for non-PCI based HDA controllers.
> 
> I tried to break it up to make it less scary and to keep changes away
> from copies of code.  It is mostly copies with a few modifications to
> make more of the code usable by both HDA drivers.  The main changes
> were to avoid using pci to get a device pointer, and to make the bus
> reads/writes into callbacks so that the Tegra driver could ensure
> 32-bit access to the HDA registers.
> 
> The final patch is a work in progress included here to provide context
> for the preceding patches.  The Tegra HDA driver still needs some
> cleaning up, but nothing that will affect the other changes.  The last
> infrastructure issue is that the Tegra driver uses DT, causing it to be
> probed before the codec patches.  I haven't determined how to fix that
> yet.
> 
> I didn't include the patch to move the hda directory from under pci
> because of its size and how easy it will be to regenerate later.
> 
> Thanks for looking.  Let me know if you think any of these should be
> squashed or re-ordered and if the changes and breakup of code makes
> sense.

The changes look almost good as I supposed.
Through a quick glance, some random comments:

- I prefer hda_controller.c (or something like that) to hda_shared.c.
  It's a helper module for the controller drivers, after all.

- mark_pages_wc() and co can be moved back to hda_intel.c.
  Instead, provide the page allocator in ops, something like:

	int (*alloc_pages)(struct azx *chip, int size,
				struct snd_dma_buffer *buf);
	void (*free_pages)(struct azx *chip, struct snd_dma_buffer *buf);

  then X86-specific hacks can be done there.

- The rest x86-specific hack is mmap.  Maybe this can be also handled
  via an ops.  For non-x86, just call snd_pcm_lib_default_mmap()
  (which is equivalent with pcm_ops.mmap = NULL).

- There are lots of workarounds in the code, and which are really
  needed for Tegra?  For example, bdl_pos_adj can be optional, and you
  can put a NULL check for chip->bdl_pos_adj and handle as 0 in
  azx_setup_periods().

  The buffer position check can be back to hda_intel.c, too.  Instead,
  we may give another ops, e.g.
	bool (*position_check)(struct azx *, struct azx_dev *dev);
  and call it (optionally) in the irq handler.  This checks the
  validity of the position, return true if it's OK, else queue the
  work, etc.

  irqreturn_t azx_interrupt(int irq, void *dev_id)
  {
	.....
		if (status & azx_dev->sd_int_sta_mask) {
			....
			/* check whether this IRQ is really acceptable */
			if (!chip->ops->position_check)
				ok = true;
			else
				ok = chip->ops->position_check(chip, azx_dev);
			if (ok) {
				spin_unlock(&chip->reg_lock);
				snd_pcm_period_elapsed(azx_dev->substream);
				spin_lock(&chip->reg_lock);
			}

  Then we can move azx_irq_pending_work() to hda_intel.c, too.

  I'm not sure whether position_fix stuff is better in the shared code
  or not.  Many hardware have problems regarding this, so it'd be
  still helpful to provide workarounds in the common code for future
  hardware, too.


thanks,

Takashi