lkml.org 
[lkml]   [2014]   [Oct]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [alsa-devel] [PATCH] ASoC: add xtensa xtfpga I2S interface and platform
On 10/27/2014 08:07 PM, Max Filippov wrote:

A few minor things.

[...]
> +static irqreturn_t xtfpga_i2s_interrupt(int irq, void *dev_id)
> +{
> + struct xtfpga_i2s *i2s = dev_id;
> + struct snd_pcm_substream *tx_substream;
> + int tx_active;
> + unsigned int_status;
> +
> + regmap_read(i2s->regmap, XTFPGA_I2S_INT_STATUS, &int_status);
> + if (!(int_status & XTFPGA_I2S_INT_VALID))
> + return IRQ_NONE;
> +
> + if (int_status & XTFPGA_I2S_INT_UNDERRUN) {
> + i2s->tx_fifo_sz = 0;
> + regmap_update_bits(i2s->regmap, XTFPGA_I2S_CONFIG,
> + XTFPGA_I2S_CONFIG_INT_ENABLE |
> + XTFPGA_I2S_CONFIG_TX_ENABLE, 0);
> + } else {
> + i2s->tx_fifo_sz = i2s->tx_fifo_low;
> + regmap_update_bits(i2s->regmap, XTFPGA_I2S_CONFIG,
> + XTFPGA_I2S_CONFIG_INT_ENABLE, 0);
> + }
> +
> + rcu_read_lock();
> + tx_substream = rcu_dereference(i2s->tx_substream);
> + tx_active = tx_substream && snd_pcm_running(tx_substream);
> + if (tx_active) {
> + snd_pcm_period_elapsed(tx_substream);
> + if (int_status & XTFPGA_I2S_INT_UNDERRUN)
> + dev_dbg_ratelimited(i2s->dev, "%s: underrun\n",
> + __func__);
> + tx_substream = rcu_dereference(i2s->tx_substream);
> + tx_active = tx_substream && snd_pcm_running(tx_substream);
> + }
> + rcu_read_unlock();
> +
> + if (tx_active) {
> + if (i2s->tx_fifo_high < 256)
> + xtfpga_i2s_refill_fifo(i2s);
> + else
> + tasklet_hi_schedule(&i2s->refill_fifo);

Maybe use threaded IRQs instead of IRQ + tasklet.

> + } else {
> + xtfpga_i2s_irq_update_config(i2s, int_status);
> + }
> +
> + return IRQ_HANDLED;
> +}
[...]
> +
> +static int xtfpga_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
> +{
> + int ret;
> +
> + switch (cmd) {
> + case SNDRV_PCM_TRIGGER_START:
> + case SNDRV_PCM_TRIGGER_RESUME:
> + case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> + case SNDRV_PCM_TRIGGER_STOP:
> + case SNDRV_PCM_TRIGGER_SUSPEND:
> + case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
> + ret = 0;
> + break;
> +
> + default:
> + ret = -EINVAL;
> + break;
> + }
> + return ret;
> +}

If you don't do anything you don't need the handler. The core will handle
things if it is NULL.

> +static struct snd_pcm_ops xtfpga_pcm_ops = {

const

> + .open = xtfpga_pcm_open,
> + .close = xtfpga_pcm_close,
> + .ioctl = snd_pcm_lib_ioctl,
> + .hw_params = xtfpga_pcm_hw_params,
> + .trigger = xtfpga_pcm_trigger,
> + .pointer = xtfpga_pcm_pointer,
> +};
[...]
> +static int xtfpga_i2s_probe(struct platform_device *pdev)
> +{
> + struct xtfpga_i2s *i2s;
> + struct resource *mem, *irq;
> + int err;
> +
> + i2s = devm_kzalloc(&pdev->dev, sizeof(*i2s), GFP_KERNEL);
> + if (!i2s) {
> + err = -ENOMEM;
> + goto err;
> + }
> + dev_set_drvdata(&pdev->dev, i2s);

platform_set_drvdata(...)

> + i2s->dev = &pdev->dev;
> + dev_dbg(&pdev->dev, "dev: %p, i2s: %p\n", &pdev->dev, i2s);
> +
> + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!mem) {
> + dev_err(&pdev->dev, "No memory resource\n");
> + err = -ENODEV;
> + goto err;
> + }

devm_ioremap_resource will check if mem is NULL and print a error message,
so the check above can be removed.

> + i2s->regs = devm_ioremap_resource(&pdev->dev, mem);
> + if (IS_ERR(i2s->regs)) {
> + err = PTR_ERR(i2s->regs);
> + goto err;
> + }
> +
> + i2s->regmap = devm_regmap_init_mmio(&pdev->dev, i2s->regs,
> + &xtfpga_i2s_regmap_config);
> + if (IS_ERR(i2s->regmap)) {
> + dev_err(&pdev->dev, "regmap init failed\n");
> + err = PTR_ERR(i2s->regmap);
> + goto err;
> + }
> +
> + i2s->clk = devm_clk_get(&pdev->dev, NULL);
> + if (IS_ERR(i2s->clk)) {
> + dev_err(&pdev->dev, "couldn't get clock\n");
> + err = PTR_ERR(i2s->clk);
> + goto err;
> + }
> +
> + irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);

platform_get_irq(...)

> + if (!irq) {
> + dev_err(&pdev->dev, "No IRQ resource\n");
> + err = -ENODEV;
> + goto err;
> + }
> + err = devm_request_irq(&pdev->dev, irq->start, xtfpga_i2s_interrupt,
> + IRQF_SHARED, pdev->name, i2s);
> + if (err < 0) {
> + dev_err(&pdev->dev, "request_irq failed\n");
> + goto err;
> + }
> + tasklet_init(&i2s->refill_fifo, xtfpga_i2s_refill_fifo_tasklet,
> + (unsigned long)i2s);
> +

Since the tasklet is is used in the interrupt handler it should initialized
before the IRQ is requested.


> + regmap_write(i2s->regmap, XTFPGA_I2S_CONFIG,
> + (0x1 << XTFPGA_I2S_CONFIG_CHANNEL_BASE));
> + regmap_write(i2s->regmap, XTFPGA_I2S_INT_STATUS, XTFPGA_I2S_INT_VALID);
> + regmap_write(i2s->regmap, XTFPGA_I2S_INT_MASK, XTFPGA_I2S_INT_UNDERRUN);
> +
> + err = snd_soc_register_platform(&pdev->dev, &xtfpga_soc_platform);
> + if (err < 0) {
> + dev_err(&pdev->dev, "couldn't register platform\n");
> + goto err;
> + }
> + err = devm_snd_soc_register_component(&pdev->dev,
> + &xtfpga_i2s_component,
> + xtfpga_i2s_dai,
> + ARRAY_SIZE(xtfpga_i2s_dai));
> + if (err < 0)
> + dev_err(&pdev->dev, "couldn't register component\n");
> +err:
> + if (err)
> + dev_err(&pdev->dev, "%s: err = %d\n", __func__, err);
> + return err;
> +}
> +
> +static int xtfpga_i2s_remove(struct platform_device *pdev)
> +{
> + struct xtfpga_i2s *i2s = dev_get_drvdata(&pdev->dev);
> +
> + if (i2s) {

This will always be non NULL.

> + tasklet_kill(&i2s->refill_fifo);
> + if (i2s->regmap && !IS_ERR(i2s->regmap)) {
> + regmap_write(i2s->regmap, XTFPGA_I2S_CONFIG, 0);
> + regmap_write(i2s->regmap, XTFPGA_I2S_INT_MASK, 0);
> + regmap_write(i2s->regmap, XTFPGA_I2S_INT_STATUS,
> + XTFPGA_I2S_INT_VALID);
> + }
> + if (i2s->clk_enabled)
> + clk_disable_unprepare(i2s->clk);
> + }
> +
> + return 0;
> +}
[...]



\
 
 \ /
  Last update: 2014-10-28 17:41    [W:0.088 / U:2.164 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site