Re: [alsa-devel] [PATCH 3/3 v2] sound/soc/lapis: add platform driver for ML7213 IOH I2S
On Tue, Dec 20, 2011 at 11:45:44AM +0900, Tomoya MORINAGA wrote:
+static int ignore_overrun = 1; +module_param(ignore_overrun, int, 0444); +MODULE_PARM_DESC(ignore_overrun, "ignore RX overruns (default=0)");
This shouldn't be a driver specific thing, and if we were to have such a feature a module parameter doesn't seem like a great way of doing it.
+/*****************************************************************************
- I2S HAL (Hardware Abstruction Layer)
- *****************************************************************************/
Please follow the kernel coding style in terms of comments.
+static void ioh_i2s_enable_interrupts(int ch, enum dma_data_direction dir) +{
- unsigned int intr_lines;
- if (dir)
intr_lines = 1 << (I2S_IMASK_RX_BIT_START + ch);- else
intr_lines = 1 << (I2S_IMASK_TX_BIT_START + ch);
I'd expect a switch statement corresponding to the enum, not an if statement.
+static void ioh_i2s_disable_interrupts(int ch, enum dma_data_direction dir) +{
- unsigned int intr_lines;
- /*intr_lines&=I2S_ALL_INTERRUPT_BITS; */
- intr_lines = ioread32(i2s_data->iobase + I2SIMASK_OFFSET);
What's this commented out code for? Also as a coding style thing you should have a space between /* and the text in the comment (this applies throughout the driver).
- /*disable interrupts for specified channel */
- if (dir)
intr_lines |= 1 << (I2S_IMASK_RX_BIT_START + ch);- else
intr_lines |= 1 << (I2S_IMASK_TX_BIT_START + ch);- /*Mask the specific interrupt bits */
- iowrite32(intr_lines, i2s_data->iobase + I2SIMASK_OFFSET);
What ensures that this read/modify/write cycle can't race with another caller?
+/* Clear interrupt status */ +static void ioh_i2s_clear_tx_sts_ir(int ch) +{
- int offset = ch * 0x800;
- iowrite32(I2S_TX_FINT | I2S_TX_AFINT | I2S_TX_EINT | I2S_TX_AEINT,
i2s_data->iobase + I2SISTTX_OFFSET + offset);+}
This appears to unconditionally acknowledge all interrupts, generally you should only acknowledge interrupts that have been handled. Otherwise it's possible that interrupts may be dropped if they're flagged between the status read and acknowledgement write.
+/*****************************************************************************
- I2S Middle ware
- *****************************************************************************/
I'm really not convinced of the value of these layers, reading the code it really feels incredibly verbose in comparison with what I'd expect from such a driver and I can't help that think that a lot of this verbosity is down to muddling through these abstraction layers.
+static bool filter(struct dma_chan *chan, void *slave)
This needs a better name. It's not namespaced and it's not clear what it's filtering.
+static struct dma_chan *ioh_request_dma_channel(
int ch, struct ioh_i2s_dma *dma, enum dma_data_direction dir)+{
- dma_cap_mask_t mask;
- struct dma_chan *chan;
- struct pci_dev *dma_dev;
- dma_cap_zero(mask);
- dma_cap_set(DMA_SLAVE, mask);
- dma_dev = pci_get_bus_and_slot(2, PCI_DEVFN(0, 1)); /* Get DMA's dev
information */- if (dir == DMA_FROM_DEVICE) { /* Rx */
switch statement to select between multiple options in the enum. This applies throughout the code.
+static void +ioh_i2s_write(struct snd_pcm_substream *substream, const void *data, int len) +{
- int rem1;
- int rem2;
What is this supposed to do? There's an awful lot of it, and it looks like it's supposed to be rewriting the data format for some reason which isn't something that a driver ought to be doing (apart from anything else it does rather defeat the point of DMA).
+static void ioh_i2s_configure_i2s_regs(int ch, enum ioh_direction dir) +{
- int offset = ch * 0x800;
You should have a function to map the channel into a number.
- if (dir) {
/* Rx register */iowrite32(I2S_AFULL_THRESH / 2,i2s_data->iobase + I2SAFRX_OFFSET + offset);iowrite32(0x1F, i2s_data->iobase + I2SAERX_OFFSET + offset);iowrite32(0x1F, i2s_data->iobase + I2SMSKRX_OFFSET + offset);iowrite32(0xC, i2s_data->iobase + I2SISTRX_OFFSET + offset);
Lots of magic numbers here and given that the function is called just "configure" it's not clear what the intended purpose is. This needs to be clarified.
/* Rx configuration */if (atomic_read(&dma->rx_busy)) {dev_err(i2s_data->dev, "rx i2s%dalready opened\n", ch);
...
- } else {
/* Tx configuration */if (atomic_read(&dma->tx_busy)) {
There's a *lot* of duplicate code in the driver for TX and RX, it should be possible to abstract out much more common code.
+static void i2s_tx_almost_empty_ir(int ch) +{
- struct dma_async_tx_descriptor *desc;
- int num;
- int tx_comp_index;
- struct ioh_i2s_dma *dma = &dmadata[ch];
- struct scatterlist *sg = dma->sg_tx_p;
- void *cb_ch;
- dev_dbg(i2s_data->dev, "%s: data_head=%p data_complete=%p\n", __func__,
dma->tx_data_head, dma->tx_complete);- num = ((int)dma->tx_avail) / (I2S_AEMPTY_THRESH * 4);
That case looks *very* suspect.
- tx_comp_index = (((int)(dma->tx_complete - dma->tx_head))) /\
(I2S_AEMPTY_THRESH * 4);
There is very rarely any need for line continuations outside of macros.
+static inline void ioh_i2s_interrupt_sub_tx(int ch) +{
- unsigned int status;
- int offset = ch * 0x800;
- status = ioread32(i2s_data->iobase + I2SISTTX_OFFSET + offset);
- if (status & I2S_TX_EINT)
i2s_tx_empty_ir(ch);- if (status & I2S_TX_AEINT)
i2s_tx_almost_empty_ir(ch);
Given that all you're doing is logging just open code the log message.
- default:
return -1;
Return error codes, you EPERM almost certainly isn't the error you meant to report.
+static int ioh_i2s_pci_suspend(struct pci_dev *pdev, pm_message_t state) +{
- int ret;
- ioh_i2s_save_reg_conf(pdev);
You should be doing this in ASoC suspend/resume callbacks, not in PCI ones.
2011/12/20 Mark Brown broonie@opensource.wolfsonmicro.com:
On Tue, Dec 20, 2011 at 11:45:44AM +0900, Tomoya MORINAGA wrote:
+static int ignore_overrun = 1; +module_param(ignore_overrun, int, 0444); +MODULE_PARM_DESC(ignore_overrun, "ignore RX overruns (default=0)");
This shouldn't be a driver specific thing, and if we were to have such a feature a module parameter doesn't seem like a great way of doing it.
I'll delete this parameter.
+/*****************************************************************************
- I2S HAL (Hardware Abstruction Layer)
- *****************************************************************************/
Please follow the kernel coding style in terms of comments.
I' ll modify this.
+static void ioh_i2s_enable_interrupts(int ch, enum dma_data_direction dir) +{
- unsigned int intr_lines;
- if (dir)
- intr_lines = 1 << (I2S_IMASK_RX_BIT_START + ch);
- else
- intr_lines = 1 << (I2S_IMASK_TX_BIT_START + ch);
I'd expect a switch statement corresponding to the enum, not an if statement.
"if" will replace "switch".
+static void ioh_i2s_disable_interrupts(int ch, enum dma_data_direction dir) +{
- unsigned int intr_lines;
- /*intr_lines&=I2S_ALL_INTERRUPT_BITS; */
- intr_lines = ioread32(i2s_data->iobase + I2SIMASK_OFFSET);
What's this commented out code for? Also as a coding style thing you should have a space between /* and the text in the comment (this applies throughout the driver).
I'll delete.
- /*disable interrupts for specified channel */
- if (dir)
- intr_lines |= 1 << (I2S_IMASK_RX_BIT_START + ch);
- else
- intr_lines |= 1 << (I2S_IMASK_TX_BIT_START + ch);
- /*Mask the specific interrupt bits */
- iowrite32(intr_lines, i2s_data->iobase + I2SIMASK_OFFSET);
What ensures that this read/modify/write cycle can't race with another caller?
I'll add exclusive processing like spin-lock.
+/* Clear interrupt status */ +static void ioh_i2s_clear_tx_sts_ir(int ch) +{
- int offset = ch * 0x800;
- iowrite32(I2S_TX_FINT | I2S_TX_AFINT | I2S_TX_EINT | I2S_TX_AEINT,
- i2s_data->iobase + I2SISTTX_OFFSET + offset);
+}
This appears to unconditionally acknowledge all interrupts, generally you should only acknowledge interrupts that have been handled. Otherwise it's possible that interrupts may be dropped if they're flagged between the status read and acknowledgement write.
I can understand your saying. If following your saying, only called by i2s_dma_tx_complete is enough.
However I think adding clearing interrupt status at both before and after i2s communicating start/finish is better.
+/*****************************************************************************
- I2S Middle ware
- *****************************************************************************/
I'm really not convinced of the value of these layers, reading the code it really feels incredibly verbose in comparison with what I'd expect from such a driver and I can't help that think that a lot of this verbosity is down to muddling through these abstraction layers.
+static bool filter(struct dma_chan *chan, void *slave)
This needs a better name. It's not namespaced and it's not clear what it's filtering.
In case of using Linux standard DMA API, function "filter" is the most popular name. Almost drivers which use standard use "filter".
+static struct dma_chan *ioh_request_dma_channel(
- int ch, struct ioh_i2s_dma *dma, enum dma_data_direction dir)
+{
- dma_cap_mask_t mask;
- struct dma_chan *chan;
- struct pci_dev *dma_dev;
- dma_cap_zero(mask);
- dma_cap_set(DMA_SLAVE, mask);
- dma_dev = pci_get_bus_and_slot(2, PCI_DEVFN(0, 1)); /* Get DMA's dev
- information */
- if (dir == DMA_FROM_DEVICE) { /* Rx */
switch statement to select between multiple options in the enum. This applies throughout the code.
"if" will replace "switch".
+static void +ioh_i2s_write(struct snd_pcm_substream *substream, const void *data, int len) +{
- int rem1;
- int rem2;
What is this supposed to do? There's an awful lot of it, and it looks like it's supposed to be rewriting the data format for some reason which isn't something that a driver ought to be doing (apart from anything else it does rather defeat the point of DMA).
This driver has buffer which is for preventing noisy sound by jitter So, this buffer is variable.
+static void ioh_i2s_configure_i2s_regs(int ch, enum ioh_direction dir) +{
- int offset = ch * 0x800;
You should have a function to map the channel into a number.
understand.
- if (dir) {
- /* Rx register */
- iowrite32(I2S_AFULL_THRESH / 2,
- i2s_data->iobase + I2SAFRX_OFFSET + offset);
- iowrite32(0x1F, i2s_data->iobase + I2SAERX_OFFSET + offset);
- iowrite32(0x1F, i2s_data->iobase + I2SMSKRX_OFFSET + offset);
- iowrite32(0xC, i2s_data->iobase + I2SISTRX_OFFSET + offset);
Lots of magic numbers here and given that the function is called just "configure" it's not clear what the intended purpose is. This needs to be clarified.
understand.
- /* Rx configuration */
- if (atomic_read(&dma->rx_busy)) {
- dev_err(i2s_data->dev, "rx i2s%dalready opened\n", ch);
...
- } else {
- /* Tx configuration */
- if (atomic_read(&dma->tx_busy)) {
There's a *lot* of duplicate code in the driver for TX and RX, it should be possible to abstract out much more common code.
I'll study.
+static void i2s_tx_almost_empty_ir(int ch) +{
- struct dma_async_tx_descriptor *desc;
- int num;
- int tx_comp_index;
- struct ioh_i2s_dma *dma = &dmadata[ch];
- struct scatterlist *sg = dma->sg_tx_p;
- void *cb_ch;
- dev_dbg(i2s_data->dev, "%s: data_head=%p data_complete=%p\n", __func__,
- dma->tx_data_head, dma->tx_complete);
- num = ((int)dma->tx_avail) / (I2S_AEMPTY_THRESH * 4);
That case looks *very* suspect.
This is correct. Why do you think so ?
- tx_comp_index = (((int)(dma->tx_complete - dma->tx_head))) /\
- (I2S_AEMPTY_THRESH * 4);
There is very rarely any need for line continuations outside of macros.
+static inline void ioh_i2s_interrupt_sub_tx(int ch) +{
- unsigned int status;
- int offset = ch * 0x800;
- status = ioread32(i2s_data->iobase + I2SISTTX_OFFSET + offset);
- if (status & I2S_TX_EINT)
- i2s_tx_empty_ir(ch);
- if (status & I2S_TX_AEINT)
- i2s_tx_almost_empty_ir(ch);
Given that all you're doing is logging just open code the log message.
Understand.
- default:
- return -1;
Return error codes, you EPERM almost certainly isn't the error you meant to report.
I'll modify.
+static int ioh_i2s_pci_suspend(struct pci_dev *pdev, pm_message_t state) +{
- int ret;
- ioh_i2s_save_reg_conf(pdev);
You should be doing this in ASoC suspend/resume callbacks, not in PCI ones.
Understand.
thanks, tomoya
On Wed, Dec 21, 2011 at 08:22:56PM +0900, Tomoya MORINAGA wrote:
2011/12/20 Mark Brown broonie@opensource.wolfsonmicro.com:
On Tue, Dec 20, 2011 at 11:45:44AM +0900, Tomoya MORINAGA wrote:
- iowrite32(I2S_TX_FINT | I2S_TX_AFINT | I2S_TX_EINT | I2S_TX_AEINT,
- i2s_data->iobase + I2SISTTX_OFFSET + offset);
This appears to unconditionally acknowledge all interrupts, generally you should only acknowledge interrupts that have been handled. Otherwise it's possible that interrupts may be dropped if they're flagged between the status read and acknowledgement write.
I can understand your saying. If following your saying, only called by i2s_dma_tx_complete is enough.
However I think adding clearing interrupt status at both before and after i2s communicating start/finish is better.
No, the issue is that you're acknowleding a hard coded set of interrupts, not the interrupts that were actually handled. The ordering isn't really the issue, the issue is that you could acknowledge things that weren't handled. For example:
1. ISR runs, reads status A 2. Condition B happens 3. ISR handles condition A. 4. ISR acknowledges both A and B.
would mean that B would just get ignored.
+static bool filter(struct dma_chan *chan, void *slave)
This needs a better name. It's not namespaced and it's not clear what it's filtering.
In case of using Linux standard DMA API, function "filter" is the most popular name. Almost drivers which use standard use "filter".
I suspect they may have the filter immediately next to the code that uses it, which certainly isn't the case here.
+ioh_i2s_write(struct snd_pcm_substream *substream, const void *data, int len) +{
- int rem1;
- int rem2;
What is this supposed to do? There's an awful lot of it, and it looks like it's supposed to be rewriting the data format for some reason which isn't something that a driver ought to be doing (apart from anything else it does rather defeat the point of DMA).
This driver has buffer which is for preventing noisy sound by jitter So, this buffer is variable.
You're implementing some sort of custom buffering in your driver? That sounds terribly unidiomatic - pretty much all DMA drivers are very thin and manage to work well, I'd expect this is masking some problems in the code rather than anything else. Can you provide more detail on what this is working around?
- num = ((int)dma->tx_avail) / (I2S_AEMPTY_THRESH * 4);
That case looks *very* suspect.
This is correct. Why do you think so ?
I can't tell what it's supposed to do, and casting that isn't super clear especially casting to integer types tends to be a big red flag.
2011/12/22 Mark Brown broonie@opensource.wolfsonmicro.com:
On Wed, Dec 21, 2011 at 08:22:56PM +0900, Tomoya MORINAGA wrote:
2011/12/20 Mark Brown broonie@opensource.wolfsonmicro.com:
On Tue, Dec 20, 2011 at 11:45:44AM +0900, Tomoya MORINAGA wrote:
- iowrite32(I2S_TX_FINT | I2S_TX_AFINT | I2S_TX_EINT | I2S_TX_AEINT,
- i2s_data->iobase + I2SISTTX_OFFSET + offset);
This appears to unconditionally acknowledge all interrupts, generally you should only acknowledge interrupts that have been handled. Otherwise it's possible that interrupts may be dropped if they're flagged between the status read and acknowledgement write.
I can understand your saying. If following your saying, only called by i2s_dma_tx_complete is enough.
However I think adding clearing interrupt status at both before and after i2s communicating start/finish is better.
No, the issue is that you're acknowleding a hard coded set of interrupts, not the interrupts that were actually handled. The ordering isn't really the issue, the issue is that you could acknowledge things that weren't handled. For example:
1. ISR runs, reads status A 2. Condition B happens 3. ISR handles condition A. 4. ISR acknowledges both A and B.
would mean that B would just get ignored.
I can understand your saying. However, in case of Tx interrupt, only ALMOST-EMPTY occurs. in case of Rx, only Rx ALMOST-FULL occurs. So as long as a user uses ML7213 I2S, your mentioned behavior is never happened.
+static bool filter(struct dma_chan *chan, void *slave)
This needs a better name. It's not namespaced and it's not clear what it's filtering.
In case of using Linux standard DMA API, function "filter" is the most popular name. Almost drivers which use standard use "filter".
I suspect they may have the filter immediately next to the code that uses it, which certainly isn't the case here.
They have the "filter" function immediately next to the code uses "dma_request_channel". Anyway, if you want me to change the name, I can change the name. Please decide it.
+ioh_i2s_write(struct snd_pcm_substream *substream, const void *data, int len) +{
- int rem1;
- int rem2;
What is this supposed to do? There's an awful lot of it, and it looks like it's supposed to be rewriting the data format for some reason which isn't something that a driver ought to be doing (apart from anything else it does rather defeat the point of DMA).
This driver has buffer which is for preventing noisy sound by jitter So, this buffer is variable.
You're implementing some sort of custom buffering in your driver? That sounds terribly unidiomatic - pretty much all DMA drivers are very thin and manage to work well, I'd expect this is masking some problems in the code rather than anything else. Can you provide more detail on what this is working around?
Not sorted but queuing only. In sound/voice control system, queuing is not rare, I think. If necessary, though this method is very common, I can send the method of the queue.
- num = ((int)dma->tx_avail) / (I2S_AEMPTY_THRESH * 4);
That case looks *very* suspect.
This is correct. Why do you think so ?
I can't tell what it's supposed to do, and casting that isn't super clear especially casting to integer types tends to be a big red flag.
Currently, "tx_avail" is "int" type. So, "(int)" is redundant. Maybe, once type of "tx_avail" is not "int" but something pointer type. I'll modify like below. num = dma->tx_avail / (I2S_AEMPTY_THRESH * 4);
thanks, tomoya
On Thu, Dec 22, 2011 at 05:10:24PM +0900, Tomoya MORINAGA wrote:
I can understand your saying. However, in case of Tx interrupt, only ALMOST-EMPTY occurs. in case of Rx, only Rx ALMOST-FULL occurs. So as long as a user uses ML7213 I2S, your mentioned behavior is never happened.
That's not what your code says... it may be that the other interrupts are very rare but that's not the same thing.
They have the "filter" function immediately next to the code uses "dma_request_channel".
That's a rather large function...
Anyway, if you want me to change the name, I can change the name. Please decide it.
Rename.
You're implementing some sort of custom buffering in your driver? That sounds terribly unidiomatic - pretty much all DMA drivers are very thin and manage to work well, I'd expect this is masking some problems in the code rather than anything else. Can you provide more detail on what this is working around?
Not sorted but queuing only. In sound/voice control system, queuing is not rare, I think. If necessary, though this method is very common, I can send the method of the queue.
No, please describe the problem you're trying to fix. If nothing else think about what you're saying here - if this is a common need then it's something that's going to be handled in generic code, not open coded in individual drivers. Like I say I would expect that you have problems elsewhere in your code which you are masking with this.
2011/12/22 Mark Brown broonie@opensource.wolfsonmicro.com:
On Thu, Dec 22, 2011 at 05:10:24PM +0900, Tomoya MORINAGA wrote:
I can understand your saying. However, in case of Tx interrupt, only ALMOST-EMPTY occurs. in case of Rx, only Rx ALMOST-FULL occurs. So as long as a user uses ML7213 I2S, your mentioned behavior is never happened.
That's not what your code says... it may be that the other interrupts are very rare but that's not the same thing.
I'll delete these except initialize() and transfer_complete().
They have the "filter" function immediately next to the code uses "dma_request_channel".
That's a rather large function...
Anyway, if you want me to change the name, I can change the name. Please decide it.
Rename.
I'll obey your opinion.
You're implementing some sort of custom buffering in your driver? That sounds terribly unidiomatic - pretty much all DMA drivers are very thin and manage to work well, I'd expect this is masking some problems in the code rather than anything else. Can you provide more detail on what this is working around?
Not sorted but queuing only. In sound/voice control system, queuing is not rare, I think. If necessary, though this method is very common, I can send the method of the queue.
No, please describe the problem you're trying to fix.
When CPU is heavy load, this buffer is useful. The heavy load causes delaying receiving processing. If there is no buffer, stream sound/voice can be broken. If there is the buffer, it can prevent the broken sound.
thanks, tomoya
On Mon, Dec 26, 2011 at 03:33:29PM +0900, Tomoya MORINAGA wrote:
2011/12/22 Mark Brown broonie@opensource.wolfsonmicro.com:
Not sorted but queuing only. In sound/voice control system, queuing is not rare, I think. If necessary, though this method is very common, I can send the method of the queue.
No, please describe the problem you're trying to fix.
When CPU is heavy load, this buffer is useful. The heavy load causes delaying receiving processing. If there is no buffer, stream sound/voice can be broken. If there is the buffer, it can prevent the broken sound.
So you're just talking about standard underflows if the application can't keep up? There's *no* reason for your driver to do anything about this, it's a really basic thing that affects all audio hardware. Just write a driver for the hardware.
2011/12/26 Mark Brown broonie@opensource.wolfsonmicro.com:
On Mon, Dec 26, 2011 at 03:33:29PM +0900, Tomoya MORINAGA wrote:
2011/12/22 Mark Brown broonie@opensource.wolfsonmicro.com:
Not sorted but queuing only. In sound/voice control system, queuing is not rare, I think. If necessary, though this method is very common, I can send the method of the queue.
No, please describe the problem you're trying to fix.
When CPU is heavy load, this buffer is useful. The heavy load causes delaying receiving processing. If there is no buffer, stream sound/voice can be broken. If there is the buffer, it can prevent the broken sound.
So you're just talking about standard underflows if the application can't keep up?
No. not only underflow but overflow.
There's *no* reason for your driver to do anything about this, it's a really basic thing that affects all audio hardware. Just write a driver for the hardware.
In case driver layer doesn't have the function, which part works for the under/overflow ? Application ? or nothing ?
tomoya
On Tue, Dec 27, 2011 at 10:25:25AM +0900, Tomoya MORINAGA wrote:
2011/12/26 Mark Brown broonie@opensource.wolfsonmicro.com:
So you're just talking about standard underflows if the application can't keep up?
No. not only underflow but overflow.
Same difference.
There's *no* reason for your driver to do anything about this, it's a really basic thing that affects all audio hardware. Just write a driver for the hardware.
In case driver layer doesn't have the function, which part works for the under/overflow ? Application ? or nothing ?
Your driver has to accurately describe what the hardware supports and then the application layer is responsible for making sure it keeps up with things.
participants (2)
-
Mark Brown -
Tomoya MORINAGA