On Mon, 2007-07-16 at 17:08 +0200, Takashi Iwai wrote:
> At Mon, 16 Jul 2007 16:14:59 +0200,
> Hans-Christian Egtvedt wrote:
> >
> > --- /dev/null
> > +++ b/include/linux/spi/at73c213.h
> > @@ -0,0 +1,25 @@
> > +/*
> > + * Board-specific data used to set up AT73c213 audio DAC driver.
> > + */
> > +
> > +#ifndef __LINUX_SPI_AT73C213_H
> > +#define __LINUX_SPI_AT73C213_H
> > +
> > +/**
> > + * at73c213_board_info - how the external DAC is wired to the device.
> > + *
> > + * @ssc_id: SSC platform_driver id the DAC shall use to stream the audio.
> > + * @dac_clk: the external clock used to provide master clock to the DAC.
> > + * @shortname: a short discription for the DAC, seen by userspace tools.
> > + *
> > + * This struct contains the configuration of the hardware connection to the
> > + * external DAC. The DAC needs a master clock and a I2S audio stream. It also
> > + * provides a name which is used to identify it in userspace tools.
> > + */
> > +struct at73c213_board_info {
> > + int ssc_id;
> > + struct clk *dac_clk;
> > + char shortname[32];
> > +};
> > +
> > +#endif /* __LINUX_SPI_AT73C213_H */
>
> Any reason to put this into include/linux? Are (or will be) there
> users of this except for ALSA driver?
I think Haavards answer describes the usage well.
> > diff --git a/sound/spi/at73c213.c b/sound/spi/at73c213.c
> > new file mode 100644
> > index 0000000..ea6126b
> > --- /dev/null
> > +++ b/sound/spi/at73c213.c
> (snip)
> > +#include <linux/kmod.h>
>
> Unneeded.
Removed
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/io.h>
> > +
> > +#include <sound/initval.h>
> > +#include <sound/driver.h>
>
> Please put sound/driver.h at first. This will make it for us much
> easier to maintain the out-of-kernel tree.
Ok, applied.
> > +/*
> > + * Calculate and set bitrate and divisions.
> > + */
> > +static int snd_at73c213_set_bitrate(struct snd_at73c213 *chip)
> > +{
> > + unsigned long ssc_rate = clk_get_rate(chip->ssc->clk);
> > + unsigned long dac_rate_new, ssc_div, status;
> > + unsigned long ssc_div_max, ssc_div_min;
> > + int max_tries;
> > +
> > + /*
> > + * We connect two clocks here, picking divisors so the I2S clocks
> > + * out data at the same rate the DAC clocks it in ... and as close
> > + * as practical to the desired target rate.
> > + *
> > + * The DAC master clock (MCLK) is programmable, and is either 256
> > + * or (not here) 384 times the I2S output clock (BCLK).
> > + */
> > +
> > + /* SSC clock / (bitrate * stereo * 16-bit). */
> > + ssc_div = ssc_rate / (BITRATE_TARGET * 2 * 16);
> > + ssc_div_min = ssc_rate / (BITRATE_MAX * 2 * 16);
> > + ssc_div_max = ssc_rate / (BITRATE_MIN * 2 * 16);
> > + max_tries = (ssc_div_max - ssc_div_min) / 2;
> > +
> > + /* ssc_div must be a power of 2. */
> > + ssc_div = (ssc_div + 1) & ~1UL;
> > +
> > + if ((ssc_rate / (ssc_div * 2 * 16)) < BITRATE_MIN) {
> > + ssc_div -= 2;
> > + if ((ssc_rate / (ssc_div * 2 * 16)) > BITRATE_MAX) {
> > + return -ENXIO;
> > + }
>
> Remove braces for a single-if block.
Applied.
> > + }
> > +
> > + /* Search for a possible bitrate. */
> > + do {
> > + /* SSC clock / (ssc divider * 16-bit * stereo). */
> > + if ((ssc_rate / (ssc_div * 2 * 16)) < BITRATE_MIN)
> > + return -ENXIO;
> > +
> > + /* 256 / (2 * 16) = 8 */
> > + dac_rate_new = 8 * (ssc_rate / ssc_div);
> > +
> > + status = clk_round_rate(chip->board->dac_clk, dac_rate_new);
> > + if (status < 0)
> > + return status;
> > +
> > + /* Ignore difference smaller than 256 Hz. */
> > + if ((status/256) == (dac_rate_new/256))
> > + goto set_rate;
> > +
> > + ssc_div += 2;
> > + } while (--max_tries);
>
> Can't be max_tries = 0 in theory?
Added a check earlier in the code which sets max_tries to 1 if it is
smaller than 1. Could have been ugly for really low rates on the module,
although it should not happen(tm).
if (max_tries < 1)
max_tries = 1;
> > +static int snd_at73c213_pcm_trigger(struct snd_pcm_substream *substream,
> > + int cmd)
> > +{
> > + struct snd_at73c213 *chip = snd_pcm_substream_chip(substream);
> > + int retval = 0;
> > + unsigned long flags = 0;
> > +
> > + spin_lock_irqsave(&chip->lock, flags);
>
> The trigger callback is already spinlock+irqsaved.
> So, spin_lock() / spin_unlock() should be enough.
Ok, applied.
> > + snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_PLAYBACK, &at73c213_playback_ops);
> > +
> > + retval = snd_pcm_lib_preallocate_pages_for_all(chip->pcm,
> > + SNDRV_DMA_TYPE_DEV,
> > + snd_dma_continuous_data(GFP_KERNEL),
> > + 64 * 1024, 64 * 1024);
>
> For SNDRV_DMA_TYPE_DEV, pass the struct device pointer of the device
> to the third argument. Then the pre-allocator will allocate the
> memory via dma_alloc_coherent() with the givn device. If it's not
> appropriate, SNDRV_DMA_TYPE_CONTINUOUS type. Anyway,
> snd_dma_continuous_data() is only for SNDRV_DMA_TYPE_CONTINUOUS.
Ok, applied. Using &chip->spi->dev.
> > +/*
> > + * Mixer functions.
> > + */
> > +static int snd_at73c213_mono_get(struct snd_kcontrol *kcontrol,
> > + struct snd_ctl_elem_value *ucontrol)
> > +{
> > + struct snd_at73c213 *chip = snd_kcontrol_chip(kcontrol);
> > + unsigned long flags;
> > + int reg = kcontrol->private_value & 0xff;
> > + int shift = (kcontrol->private_value >> 8) & 0xff;
> > + int mask = (kcontrol->private_value >> 16) & 0xff;
> > + int invert = (kcontrol->private_value >> 24) & 0xff;
> > +
> > + spin_lock_irqsave(&chip->lock, flags);
>
> The mixer (control) callbacks are all schedulable, so you need no
> irqsave but spin_lock_irq() should be fine.
Ok, applied.
> > +static int __devinit snd_at73c213_mixer(struct snd_at73c213 *chip)
> > +{
> > + struct snd_card *card;
> > + int errval, idx;
> > +
> > + if (chip == NULL || chip->pcm == NULL)
> > + return -EINVAL;
> > +
> > + card = chip->card;
> > +
> > + strcpy(card->mixername, chip->pcm->name);
> > +
> > + for (idx = 0; idx < ARRAY_SIZE(snd_at73c213_controls); idx++) {
> > + if ((errval = snd_ctl_add(card,
> > + snd_ctl_new1(&snd_at73c213_controls[idx],
> > + chip))) < 0) {
> > + goto cleanup;
> > + }
>
> Split errval = ; and if (errval < 0) ...
> Also drop braces for a single if block.
Ok, applied.
I have attached an updated patch with the changes you noted above.
Thanks for the review.
--
With kind regards,
Hans-Christian Egtvedt, siv.ing. (M.Sc.)
Applications Engineer - AVR32 System Solutions - Atmel Norway
Attachment:
0001-ALSA-sound-driver-for-the-AT73C213-DAC-using-Atmel-S.patch
Description: application/mbox
_______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
