Re: [Alsa-devel] [RFC 4/8] snd-aoa: add i2sbus

At Thu, 01 Jun 2006 13:58:48 +0200,
Johannes Berg wrote:

Equivalent with "I2S_CLOCK_SPEED_18MHZ % (rate * mclk) == 0" ?

+static int i2sbus_pcm_open(struct i2sbus_dev *i2sdev, int in)
+{

(snip)

+	list_for_each_entry(cii, &sdev->codec_list, list) {
+		if (cii->codec->open) {
+			err = cii->codec->open(cii, pi->substream);
+			if (err) {
+				result = err;
+				goto out_unlock;

What happens if the first code is opened but fail the secondary?
No need to close the first?

+static snd_pcm_uframes_t i2sbus_pcm_pointer(struct i2sbus_dev *i2sdev, int in)
+{
+	struct pcm_info *pi;
+	u32 fc;
+
+	get_pcm_info(i2sdev, in, &pi, NULL);
+
+	fc = in_le32(&i2sdev->intfregs->frame_count);
+	fc = fc - pi->frame_count;
+
+	return (bytes_to_frames(pi->substream->runtime,
+			       pi->current_period *
+			       snd_pcm_lib_period_bytes(pi->substream)) + fc) % pi->substream->runtime->buffer_size;
+}
+
+static inline void handle_interrupt(struct i2sbus_dev *i2sdev, int in)
+{
+	struct pcm_info *pi;
+	u32 fc;
+	u32 delta;
+
+	spin_lock(&i2sdev->low_lock);
+	get_pcm_info(i2sdev, in, &pi, NULL);
+	if (!pi->substream) {
+		printk(KERN_INFO "i2sbus: got %s irq while not active!\n",
+		       in ? "rx" : "tx");
+		goto out_unlock;
+	}
+
+	fc = in_le32(&i2sdev->intfregs->frame_count);
+	/* a counter overflow does not change the calculation. */
+	delta = fc - pi->frame_count;
+
+	/* update current_period */
+	while (delta >= pi->substream->runtime->period_size) {
+		pi->current_period++;
+		delta = delta - pi->substream->runtime->period_size;
+	}
+
+	if (unlikely(delta)) {
+		/* Some interrupt came late, so check the dbdma.
+		 * This special case exists to syncronize the frame_count with the
+		 * dbdma transfers, but is hit every once in a while. */
+		int period;
+
+		period = (in_le32(&pi->dbdma->cmdptr) - pi->dbdma_ring.bus_cmd_start) / sizeof(struct dbdma_cmd);
+		pi->current_period = pi->current_period % pi->substream->runtime->periods;
+
+		while (pi->current_period != period) {
+			pi->current_period = (pi->current_period + 1) % pi->substream->runtime->periods;
+			/* Set delta to zero, as the frame_count value is too high (otherwise the code path
+			 * will not be executed).
+			 * This is to correct the fact that the frame_count is too low at the beginning
+			 * due to the dbdma's buffer. */
+			delta = 0;

Too long lines...

+/* FIXME: this function needs an error handling strategy with labels */
+int
+i2sbus_attach_codec(struct soundbus_dev *dev, struct snd_card *card,
+		    struct codec_info *ci, void *data)
+{
+	int err, in = 0, out = 0;
+	struct transfer_info *tmp;
+	struct i2sbus_dev *i2sdev = soundbus_dev_to_i2sbus_dev(dev);
+	struct codec_info_item *cii;
+
+	if (!dev->pcmname || dev->pcmid == -1) {
+		printk(KERN_ERR "i2sbus: pcm name and id must be set!\n");

No error return?

+	/* well, we really should support scatter/gather DMA */
+	/* FIXME FIXME FIXME: If this fails, we BUG() when the alsa layer
+	 * later tries to allocate memory. Apparently we should be setting
+	 * some device pointer for that ...
+	 */
+	snd_pcm_lib_preallocate_pages_for_all(
+		dev->pcm, SNDRV_DMA_TYPE_DEV,
+		snd_dma_pci_data(macio_get_pci_dev(i2sdev->macio)),
+		64 * 1024, 64 * 1024);

Is the comment true?  Yes, you have to set the device pointer via
snd_pcm_lib_preallocate*().  But it must be OK even if preallocate
fails.

Better to use dma_alloc_coherent().  pci_alloc_consistent() implies
GFP_ATOMIC.

+static irqreturn_t i2sbus_bus_intr(int irq, void *devid, struct pt_regs *regs)
+{
+	struct i2sbus_dev *dev = devid;
+	u32 intreg;
+
+	spin_lock(&dev->low_lock);
+	intreg = in_le32(&dev->intfregs->intr_ctl);
+
+	printk(KERN_INFO "i2sbus: interrupt, intr reg is 0x%x!\n", intreg);

Should this be really always printed?


Takashi