Messages in this thread | | | Date | Wed, 21 Dec 2011 20:22:56 +0900 | Subject | Re: [PATCH 3/3 v2] sound/soc/lapis: add platform driver for ML7213 IOH I2S | From | Tomoya MORINAGA <> |
| |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |