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;

+}

[...]