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

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

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

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.

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

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