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

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1.1: Type: text/plain, Size: 2108 bytes --]

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.

> > > +	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?

> > > +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?

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]