On Tue, Mar 6, 2012 at 8:52 PM, Mark Brown broonie@opensource.wolfsonmicro.com wrote:
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.
Current Intel's dma driver (pch_dma) doesn't work on this library. Because the library uses function which pch_dma doesn't support. e.g. device_prep_dma_cyclic So, we can't use this library.
+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.
I see. I'll use dynamic allocation.
- 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.
24bit data doesn't work on our system. So, we don't support S24_LE.
+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...
I saw mapping problem between machine driver and platform driver. e.g. cpu_dai_name "ml7213ioh"
- 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.
Do you mean request_irq should move to somewhere, like open() or hw_params() or ... ?
thanks.