lkml.org 
[lkml]   [2012]   [Mar]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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@opensource.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.
[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2012-03-06 12:55    [W:0.137 / U:0.124 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site