linux-kernel - Re: [PATCH v3] sound/soc/lapis: add platform driver for ML7213

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20120306115246.GD19635@opensource.wolfsonmicro.com>
Date:	Tue, 6 Mar 2012 11:52:47 +0000
From:	Mark Brown <broonie@...nsource.wolfsonmicro.com>
To:	Tomoya MORINAGA <tomoya.rohm@...il.com>
Cc:	Liam Girdwood <lrg@...com>, Jaroslav Kysela <perex@...ex.cz>,
	Takashi Iwai <tiwai@...e.de>, alsa-devel@...a-project.org,
	linux-kernel@...r.kernel.org, qi.wang@...el.com,
	yong.y.wang@...el.com, joel.clark@...el.com, kok.howg.ewe@...el.com
Subject: Re: [PATCH v3] sound/soc/lapis: add platform driver for ML7213

On Tue, Mar 06, 2012 at 02:48:05PM +0900, Tomoya MORINAGA wrote:
> 2012年3月3日1:39 Mark Brown <broonie@...nsource.wolfsonmicro.com>:

> > I just merged Lars-Peter's dmaengine library code which has been on the
> > list for a week or so - this should be updated to use that next time
> > it's posted.  That should save a lot of code from the driver and make
> > sure it's following best practices for dmaengine use.

> Sorry, we can't use this library.

This sort of response really isn't on.  Do you have some reason for
saying this?  Flat out refusing to do things with no reason is not
useful.

> >> +static struct ioh_i2s_data *i2s_data;
> >> +static struct ioh_i2s_dma dmadata[MAX_I2S_CH];

> > Why are these needed, aren't they dynamically allocated by the driver?

> You mean ASoC driver can't use global variable ?

In general no Linux driver should be using a global variable except for
things like this.

> >> +     case SNDRV_PCM_FORMAT_S32_LE:
> >> +             byte = 24;
> >> +             break;

> > That looks wrong...  are you sure you don't support S24_LE or something?

> This is correct.

> Because maximum transmit size of ML7213's I2S hw is 24bit.

Note we often lay out 24 bit audio in 32 bit blocks.

> >> +static struct platform_driver ioh_i2s_driver_plat = {
> >
> >> +static struct platform_driver ioh_dai_driver_plat = {
> >
> >> +static int ioh_i2s_pci_probe(struct pci_dev *pdev,
> >> +                          const struct pci_device_id *id)

> > Why are you creating these platform devices?  I don't understand the
> > function they serve.  The code handling them looks to have quite a few
> > problems but I'm not clear they should be there in the first place.

> if these platform devices aren't used, device detection doesn't work correctly.
> So, I added these.

You've not actually mentioned the problem you were seeing...

> >> +     rv = request_irq(pdev->irq, ioh_i2s_irq, IRQF_SHARED, "ml7213_ioh",
> >> +                      pdev);
> >> +     if (rv != 0) {
> >> +             printk(KERN_ERR "Failed to allocate irq\n");
> >> +             goto out_irq;
> >> +     }

> > Are you *sure* you're ready to handle interrupts at this point?

> This is just registering interrupt handler.
> As long as interrupt register is not enabled, the interrupt handler is
> not called.
> This is common request_irq description.
> What's is your concern ?

Apart from anything else you've got the interrupt requested as
IRQF_SHARED so the interrupt could get called at any time.  It's also
not clear that you've got the hardware in a known good state.

Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)