Mark Brown <broonie@xxxxxxxxxxxxxxxxxxxxxxxxxxx> writes:
> On Tue, May 11, 2010 at 06:23:47PM +0200, apatard@xxxxxxxxxxxx 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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
