Messages in this thread | | | Date | Tue, 28 Oct 2014 16:58:24 +0100 | From | Lars-Peter Clausen <> | Subject | Re: [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; > +} [...]
| |