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

[Takashi Iwai <tiwai@suse.de>]

At Mon, 15 Jun 2015 17:42:02 +0100,
Mark Brown wrote:
> 
> On Mon, Jun 15, 2015 at 06:35:16PM +0200, Takashi Iwai wrote:
> > Mark Brown wrote:
> > > On Thu, Jun 11, 2015 at 10:03:56PM +0530, Vinod Koul wrote:
> 
> > > > +	for (i = 0; i < num_stream; i++) {
> > > > +		struct hdac_ext_stream *stream =
> > > > +				kzalloc(sizeof(*stream), GFP_KERNEL);
> 
> > > 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.)
> 
> It looks awfully like it's dynamically allocated here...
> 
> > 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.
> 
> I can see the thing calling these functions being driver specific but as
> far as I can see all these are doing is allocating structs, initialising
> them with passed in parameters, putting them on a list and calling a
> core function on them.  It's not the bit taking the decisions, it's just
> doing mechanical things.

Actually, this can be done a bit more cleanly -- if there will be no
more additions to the stream object.  It wasn't clear, so the
allocation and free are left to the driver, so far.

> > > > +	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.
> 
> Do you mean PCI controller or BIOS here?

Weird combination of both.

> > > > +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.
> 
> Or we could add the ops to the core now and convert the drivers later?

This should be feasible.


Takashi