On 11/08/2013 09:56 PM, Florian Meier wrote:
Is the I2S clock part of a generic clocking module? If yes it might be
better to implement this as a clock chip driver.
[...]
> +static void bcm2835_i2s_stop(struct bcm2835_i2s_dev *dev,
> + struct snd_pcm_substream *substream)
> +{
> + unsigned int still_running;
> +
> + if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) {
> + regmap_update_bits(dev->i2s_regmap, BCM2835_I2S_CS_A_REG,
> + BCM2835_I2S_RXON, 0);
> + } else {
> + regmap_update_bits(dev->i2s_regmap, BCM2835_I2S_CS_A_REG,
> + BCM2835_I2S_TXON, 0);
> + }
> +
> + regmap_read(dev->i2s_regmap, BCM2835_I2S_CS_A_REG, &still_running);
> + still_running &= BCM2835_I2S_TXON | BCM2835_I2S_RXON;
> +
still_running can be replaced with dai->active, I think
> + /* Stop also the clock when not SND_SOC_DAIFMT_CONT */
> + if (!still_running && !(dev->fmt & SND_SOC_DAIFMT_CONT))
> + bcm2835_i2s_stop_clock(dev);
> +
> +}
> +
> +static int bcm2835_i2s_trigger(struct snd_pcm_substream *substream, int cmd,
> + struct snd_soc_dai *dai)
> +{
> + struct bcm2835_i2s_dev *dev = snd_soc_dai_get_drvdata(dai);
> +
> + switch (cmd) {
> + case SNDRV_PCM_TRIGGER_START:
> + case SNDRV_PCM_TRIGGER_RESUME:
> + case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> + bcm2835_i2s_start_clock(dev);
> +
> + if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) {
> + regmap_update_bits(dev->i2s_regmap,
> + BCM2835_I2S_CS_A_REG,
> + BCM2835_I2S_RXON, BCM2835_I2S_RXON);
> + } else {
> + regmap_update_bits(dev->i2s_regmap,
> + BCM2835_I2S_CS_A_REG,
> + BCM2835_I2S_TXON, BCM2835_I2S_TXON);
> + }
Doing something like
if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
mask = BCM2835_I2S_RXON;
else
mask = BCM2835_I2S_TXON;
regmap_update_bits(dev->i2s_regmap,
BCM2835_I2S_CS_A_REG, mask, mask);
makes the code a bit shorter. Same for bcm2835_i2s_stop().
> + break;
> +
> + case SNDRV_PCM_TRIGGER_STOP:
> + case SNDRV_PCM_TRIGGER_SUSPEND:
> + case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
> + bcm2835_i2s_stop(dev, substream);
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int bcm2835_i2s_startup(struct snd_pcm_substream *substream,
> + struct snd_soc_dai *dai)
> +{
> + struct bcm2835_i2s_dev *dev = snd_soc_dai_get_drvdata(dai);
> +
> + if (!dev->first_stream) {
> + dev->first_stream = substream;
> +
Since you use first_stream and second_stream for anything else I think you
can just use dai->active here instead.
> + /* should this still be running stop it */
> + bcm2835_i2s_stop_clock(dev);
> +
> + /* enable PCM block */
> + regmap_update_bits(dev->i2s_regmap, BCM2835_I2S_CS_A_REG,
> + BCM2835_I2S_EN, -1);
> +
> + /*
> + * Disable STBY
> + * Requires at least 4 PCM clock cycles to take effect
> + */
> + regmap_update_bits(dev->i2s_regmap, BCM2835_I2S_CS_A_REG,
> + BCM2835_I2S_STBY, -1);
> +
> + } else {
> + dev->second_stream = substream;
> + }
> +
> + return 0;
> +}
> +
[...]
> +static int bcm2835_i2s_dai_probe(struct snd_soc_dai *dai)
> +{
> + struct bcm2835_i2s_dev *dev = snd_soc_dai_get_drvdata(dai);
> +
> + /* Set the appropriate DMA parameters */
> + dev->dma_data[SNDRV_PCM_STREAM_PLAYBACK].addr =
> + (dma_addr_t)BCM2835_I2S_FIFO_PHYSICAL_ADDR;
> + dev->dma_data[SNDRV_PCM_STREAM_CAPTURE].addr =
> + (dma_addr_t)BCM2835_I2S_FIFO_PHYSICAL_ADDR;
The dma address for the FIFO should not be hardcoded but rather use an
offset to the resource that get's pass to the driver.
dma_data->addr = mem->start + BCM2835_I2S_FIFO_A_REG;
I think it should be fine to initialize dma_data already in the driver
probe() function.
> +
> + dev->dma_data[SNDRV_PCM_STREAM_PLAYBACK].slave_id =
> + BCM2835_DMA_DREQ_PCM_TX;
> + dev->dma_data[SNDRV_PCM_STREAM_CAPTURE].slave_id =
> + BCM2835_DMA_DREQ_PCM_RX;
> +
> + dev->dma_data[SNDRV_PCM_STREAM_PLAYBACK].addr_width =
> + DMA_SLAVE_BUSWIDTH_4_BYTES;
> + dev->dma_data[SNDRV_PCM_STREAM_CAPTURE].addr_width =
> + DMA_SLAVE_BUSWIDTH_4_BYTES;
> +
> + /* TODO other burst parameters possible? */
> + dev->dma_data[SNDRV_PCM_STREAM_PLAYBACK].maxburst = 2;
> + dev->dma_data[SNDRV_PCM_STREAM_CAPTURE].maxburst = 2;
> +
> + dai->playback_dma_data = &dev->dma_data[SNDRV_PCM_STREAM_PLAYBACK];
> + dai->capture_dma_data = &dev->dma_data[SNDRV_PCM_STREAM_CAPTURE];
> +
> + return 0;
> +}
> +
[...]
> +
> +static bool bcm2835_i2s_wr_rd_reg(struct device *dev, unsigned int reg)
> +{
> + switch (reg) {
> + case BCM2835_I2S_CS_A_REG:
> + case BCM2835_I2S_FIFO_A_REG:
> + case BCM2835_I2S_MODE_A_REG:
> + case BCM2835_I2S_RXC_A_REG:
> + case BCM2835_I2S_TXC_A_REG:
> + case BCM2835_I2S_DREQ_A_REG:
> + case BCM2835_I2S_INTEN_A_REG:
> + case BCM2835_I2S_INTSTC_A_REG:
> + case BCM2835_I2S_GRAY_REG:
> + return true;
> + default:
> + return false;
> + };
> +}
I think you use the default implementation of the here, which is basically
(reg >= 0 && reg <= config->max_register). Just leave the writeable_reg and
readable_reg fields of your config NULL.
[...]
> +
> +static const struct regmap_config bcm2835_i2s_regmap_config = {
> +};
This one seems to be unused.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
