Mark Brown broonie@opensource.wolfsonmicro.com writes:
On Tue, May 11, 2010 at 06:23:47PM +0200, apatard@mandriva.com wrote:
This patch enables support for the i2s codec available on kirkwood platforms
It's not a CODEC, it's the I2S controller in the CPU. A CODEC is a device with DACs and ADCs in it.
- do {
/* we've enabled only bytes interrupts ... */You were checking for error interrupts further up the function?
if (status & ~(KIRKWOOD_INT_CAUSE_PLAY_BYTES | \KIRKWOOD_INT_CAUSE_REC_BYTES))return IRQ_NONE;Surely it'd make sense to log the unexpected interrupts for diagnostics? You should never get here if there are no interrupts you've enabled. Unless the interrupt is shared, but that'd be a bit surprising for a SoC.
it's not shared iirc but there are different possible interrupt cause. There used to be a message but I think it has been removed by error. Should I had it back or remove the check ? (I prefer adding it back but your point of view does matter).
It also looks like it'd be more sensible to either drop the do/while loop entirely (the IRQ core should handle the interrupt still being asserted and it doesn't look like any of the handling should take so long that interrupts reassert while it's running anyway) or move the handling of error interrupts and the first read into the loop so you don't have two slightly different paths.
The loop is there to handle playback and record interrupts at the same time. As regards the error interrupts, I preferred to put outside the loop to differentiate "normal" interrupts from "error" interrupts.
/* ack int */writel(status, priv->io + KIRKWOOD_INT_CAUSE);if (status & KIRKWOOD_INT_CAUSE_PLAY_BYTES)snd_pcm_period_elapsed(prdata->play_stream);if (status & KIRKWOOD_INT_CAUSE_REC_BYTES)snd_pcm_period_elapsed(prdata->rec_stream);mask = readl(priv->io + KIRKWOOD_INT_MASK);status = readl(priv->io + KIRKWOOD_INT_CAUSE) & mask;This masking didn't happen prior to the first entry into the loop.
Can you explain a little bit more ? at the beginning of the function, there are the 2 same lines.
if (soc_runtime->dai->cpu_dai->private_data == NULL) {It seems a bit icky to be managing the DAI private data here...
err = request_irq(priv->irq, kirkwood_dma_irq, IRQF_SHARED,"kirkwood-dma", prdata);I'd suggest "kirkwood-i2s" as a name here. Or take the name from the DAI.
ok
+static int kirkwood_dma_trigger(struct snd_pcm_substream *substream, int cmd) +{
- return 0;
+}
Remove this if it's not implemented.
+static int kirkwood_dma_mmap(struct snd_pcm_substream *substream,
struct vm_area_struct *vma)+{
- struct snd_pcm_runtime *runtime = substream->runtime;
- return dma_mmap_coherent(substream->pcm->card->dev, vma,
runtime->dma_area,runtime->dma_addr,runtime->dma_bytes);+}
Hrm, this looks like there should be a utility function to do it, it's not terribly driver specific.
hmm... I need to double check. I got some troubles with this kind of stuff on my ST LS2E/F mips platforms, so I wrote something known to be working.
+struct snd_soc_platform kirkwood_soc_platform = {
- .name = "kirkwood-dma",
I'd also add an audio in here or something.
+#define AUDIO_WIN_BASE_REG(win) (0xA00 + ((win)<<3)) +#define AUDIO_WIN_CTRL_REG(win) (0xA04 + ((win)<<3))
Namespacing. Or move into the driver, there seems little reason for anything else to be peering at these.
+#define KIRKWOOD_RATES \
- (SNDRV_PCM_RATE_44100 | \
SNDRV_PCM_RATE_48000 | SNDRV_PCM_RATE_96000)+#define KIRKWOOD_FORMATS \
- (SNDRV_PCM_FMTBIT_S16_LE | \
SNDRV_PCM_FMTBIT_S24_LE | \SNDRV_PCM_FMTBIT_S32_LE)Move these into the driver.
+static inline void kirkwood_set_dco(void __iomem *io, unsigned long rate) +{
- unsigned long value;
- value = KIRKWOOD_DCO_CTL_OFFSET_0;
- switch (rate) {
- default:
- case 44100:
value |= KIRKWOOD_DCO_CTL_FREQ_11;break;- case 48000:
value |= KIRKWOOD_DCO_CTL_FREQ_12;break;- case 96000:
value |= KIRKWOOD_DCO_CTL_FREQ_24;break;- }
- writel(value, io + KIRKWOOD_DCO_CTL);
- /* wait for dco locked */
- do {
cpu_relax();value = readl(io + KIRKWOOD_DCO_SPCR_STATUS);value &= KIRKWOOD_DCO_SPCR_STATUS;- } while (value == 0);
+}
Why is this an inline function in the header?
* Size settings in play/rec i2s control regs and play/rec control* regs must be the same.*/Ideally you'd be setting up constraints for this.
- /*
* specs says KIRKWOOD_PLAYCTL must be read 2 times before* changing it. So read 1 time here and 1 later.*/- value = readl(priv->io + KIRKWOOD_PLAYCTL);
Nice...
- switch (cmd) {
- case SNDRV_PCM_TRIGGER_START:
/* stop audio, enable interrupts */If audio is running when you're getting a start trigger something is wrong...
I'm too paranoid ? here
+static int kirkwood_i2s_set_sysclk(struct snd_soc_dai *cpu_dai,
int clk_id, unsigned int freq, int dir)+{
- return 0;
+}
Remove this.
+static void mv_audio_init(void __iomem *base) +{
- int timeout = 10000000;
- unsigned int reg_data;
- reg_data = readl(base + 0x1200);
- reg_data &= (~(0x333FF8));
- reg_data |= 0x111D18;
- writel(reg_data, base + 0x1200);
It's like magic!
yeah. I've not been able to find out any info in the specs about that and last time I tried with that it was not working :(
- do {
timeout--;- } while (timeout);
The driver is busy waiting here but not polling anything. Just use a sleep?
- /* FIXME : should not be hardcoded */
- priv->burst = 128;
Platform data?
+static int kirkwood_i2s_probe(struct platform_device *pdev,
struct snd_soc_dai *dai)This is the ASoC probe but...
mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);...it's pulling all sorts of resources out of the device which are specific to the I2S controller. You should have the I2S controller have a normal platform device which is probed on system startup and triggers DAI registration, just as you're doing in the CODEC driver. All the resources should be there. Apart from anything else this means you only need to set up each I2S controller once in the arch/arm code for the CPU.
ok. Will look at that stuff.
- mv_audio_init(priv->io);
Seems a bit odd ot have the separate init function in the middle of a bunch of other register writes.
I want to have interrupts disable before calling it. I don't want bad suprises.
Arnaud