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

[Takashi Iwai <tiwai@suse.de>]

At Thu, 01 Jun 2006 13:58:48 +0200,
Johannes Berg wrote:
> 
> --- /dev/null
> +++ b/sound/aoa/soundbus/i2sbus/i2sbus-pcm.c
> +static int clock_and_divisors(int mclk, int sclk, int rate, int *out)
> +{
> +	/* sclk must be derived from mclk! */
> +	if (mclk % sclk)
> +		return -1;
> +	/* derive sclk register value */
> +	if (i2s_sf_sclkdiv(mclk / sclk, out))
> +		return -1;
> +
> +	if (I2S_CLOCK_SPEED_18MHz % rate == 0) {
> +		if ((I2S_CLOCK_SPEED_18MHz / rate) % mclk == 0) {

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.

> --- /dev/null
> +++ b/sound/aoa/soundbus/i2sbus/i2sbus-core.c
> +static int alloc_dbdma_descriptor_ring(struct i2sbus_dev *i2sdev,
> +				       struct dbdma_command_mem *r,
> +				       int numcmds)
> +{
> +	/* one more for rounding */
> +	r->size = (numcmds+1) * sizeof(struct dbdma_cmd);
> +	/* We use the PCI APIs for now until the generic one gets fixed
> +	 * enough or until we get some macio-specific versions
> +	 */
> +	r->space = pci_alloc_consistent(macio_get_pci_dev(i2sdev->macio),
> +					r->size,
> +					&r->bus_addr);

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