On Wednesday 14 October 2009 14:14:10 ext Mark Brown wrote:
On Tue, Oct 13, 2009 at 03:03:11PM +0300, Peter Ujfalusi wrote:
b) The nSample mode implementation uses one interrupt line from DAC33 to
Just a thought, but could a timer based approach work here?
Yes, there is a mode, when instead of interrupt, host side timer can be used. This mode needs slightly different way of configuration than the interrupt mode, and it has not been tested. So this is the reason, why it is not implemented in the driver.
- case SND_SOC_BIAS_PREPARE:
break;- case SND_SOC_BIAS_STANDBY:
dac33_soft_power(codec, 0);break;- case SND_SOC_BIAS_OFF:
break;- }
It'd be nice to do the full power down in BIAS_OFF for future use.
The full power off is taken when the dac33_soc_suspend is called (the hard_power is restored in the dac33_soc_resume function). I can move it here, than add dac33_hard_power(codec, 1) to _PREPARE, if the previous state was _OFF. Than I can remove the dac33_soc_suspend and dac33_soc_resume?
+static irqreturn_t dac33_interrupt_handler(int irq, void *dev) +{
- struct snd_soc_codec *codec = dev;
- struct tlv320dac33_priv *dac33 = codec->private_data;
- if (dac33->state == DAC33_PLAYBACK)
queue_work(dac33->dac33_wq, &dac33->work);- return IRQ_HANDLED;
+}
Should that return IRQ_HANDLED if it's not expecting an interrupt?
Good point. Interrupts are only coming, if the codec is running in nSample mode. The work is scheduled only, if the state is PLAYBACK, the other case would be the FLUSH state when there is a possibility to receive interrupt, but if the state already FLUSH, than we have had a work scheduled to write the command to the dac33.
The interrupt is actually handled, but there is nothing need to be done, so I think it is valid to return with IRQ_HANDLED.
I'd also expect to see something that either acknowledges the interrupt to deassert it or
The interrupt is self cleaning on dac33 side (in the mode, which we are using it, there is a mode, where additional write is needed to clear the interrupt status).
keeps it masked until the work has had a chance to do something.
Hmm, yes, but with the current implementation there is no need for that: The interrupt is triggered on rising edge. We only use the alarm threshold interrupt from dac33. 1. At startup I configure the dac33 to have the alarm threshold to be 10ms of audio in the buffer. Initially dac33 will read at least 20ms of audio (it can be more) 2. dac33 will asserts the interrupt line, when the buffer usage reaches the alarm threshold, and keeps it high until the load level is under the alarm threshold value 3. We instruct the dac33 to read nSample amount (at least 20ms of audio). This is done in the dac33_work:DAC33_PLAYBACK 4. The interrupt line will be de-asserted, when the fill rate is above the alarm threshold (10ms) 5. dac33 recives the nSample amount of audio data, and only plays audio from the buffer. 6. goto 2.
Than on trigger STOP, I set the state to FLUSH and schedule the work, if an interrupt comes, it is ignored, since the work has been scheduled. In dac33_work, the FIFO flush is enabled, which means the the dac33 will play out the remaining samples and will not ask for a new one (interrupt line is not going to be asserted).
Now this part needs further tuning, since it is possible that at shutdown we actually leave some samples unplayed in the buffer and stop the dac33.
- pdata = (struct tlv320dac33_platform_data *)client->dev.platform_data;
Unneeded cast.
Oh, I've missed that.
- dac33->dac33_wq = create_rt_workqueue("tlv320dac33");
- if (dac33->dac33_wq == NULL) {
ret = -ENOMEM;goto error_wq;- }
Might be nice to skip the workqueue stuff if we don't have an IRQ.
Yes.
Conditional registration of the nSample controls based on having the IRQ may also be nice.
Will do that.
ret = snd_soc_register_codec(codec);
if (ret != 0) {
dev_err(codec->dev, "Failed to register codec: %d\n", ret);goto error_codec;}
ret = snd_soc_register_dai(&dac33_dai);
if (ret != 0) {
dev_err(codec->dev, "Failed to register DAI: %d\n", ret);snd_soc_unregister_codec(codec);goto error_codec;}
I'd do these last so that we don't have ASoC level init trying to use the device only to have it yanked from underneath it by a failure in the following code.
I'll move these
Thanks, Péter