Re: [PATCH v5 2/4] ASoC: Intel - add Skylake HDA audio driver

[Takashi Iwai <tiwai@suse.de>]

Just a few pin-points:

At Mon, 15 Jun 2015 16:56:33 +0100,
Mark Brown wrote:
> 
> On Thu, Jun 11, 2015 at 10:03:56PM +0530, Vinod Koul wrote:
> 
> > +/* initialize SD streams, use seprate streeam tag for PB and CP */
> > +static int skl_init_stream(struct hdac_ext_bus *ebus, int start_idx,
> > +				int num_stream, int dir)
> > +{
> > +	int stream_tag = 0;
> > +	int i, tag, idx = start_idx;
> > +
> > +	for (i = 0; i < num_stream; i++) {
> > +		struct hdac_ext_stream *stream =
> > +				kzalloc(sizeof(*stream), GFP_KERNEL);
> > +		if (!stream)
> > +			return -ENOMEM;
> > +		tag = ++stream_tag;
> > +		snd_hdac_ext_stream_init(ebus, stream, idx, dir, tag);
> > +		idx++;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static void skl_free_streams(struct hdac_ext_bus *ebus)
> > +{
> > +	struct hdac_stream *s;
> > +	struct hdac_ext_stream *stream;
> > +	struct hdac_bus *bus = ebus_to_hbus(ebus);
> > +
> > +	while (!list_empty(&bus->stream_list)) {
> > +		s = list_first_entry(&bus->stream_list, struct hdac_stream, list);
> > +		stream = stream_to_hdac_ext_stream(s);
> > +		list_del(&s->list);
> > +		kfree(stream);
> > +	}
> > +}
> 
> Still not sure why these are Sky Lake specific?

Currently the allocation and the free of each HDA(-ext) stream are
left to each controller driver.  (See the stream object is embedded.)

And, yes, the allocation (especially the assignment of the stream tag)
*is* SKL specific.  SKL has some twists in the interpretation of
HD-audio spec.


> > +static int skl_suspend(struct device *dev)
> > +{
> > +	struct pci_dev *pci = to_pci_dev(dev);
> > +	struct hdac_ext_bus *ebus = pci_get_drvdata(pci);
> > +	struct hdac_bus *bus = ebus_to_hbus(ebus);
> > +
> > +	snd_hdac_bus_stop_chip(bus);
> > +	snd_hdac_bus_enter_link_reset(bus);
> > +	if (bus->irq >= 0) {
> > +		free_irq(bus->irq, bus);
> 
> 
> Why are we freeing the interrupt over suspend?  That is very unusual
> behaviour.

Believe or not, a PCI controller may reassign to a different IRQ
number after hibernation.  This really happened in the past for some
old stuff, hence we have this in the legacy driver.

But SKL won't need this, I suppose.


> > +
> > +static const struct hdac_io_ops skl_io_ops = {
> > +	.reg_writel = skl_pci_writel,
> > +	.reg_readl = skl_pci_readl,
> > +	.reg_writew = skl_pci_writew,
> > +	.reg_readw = skl_pci_readw,
> > +	.reg_writeb = skl_pci_writeb,
> > +	.reg_readb = skl_pci_readb,
> > +	.dma_alloc_pages = skl_dma_alloc_pages,
> > +	.dma_free_pages = skl_dma_free_pages,
> > +};
> 
> Still not thrilled at open coding these wrappers.

This is not so sexy, right, but be patient until the next HDA core
code rewrite.


Takashi