| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 28 Oct 2014 16:58:24 +0100
From: Lars-Peter Clausen <lars@...afoo.de>
To: Max Filippov <jcmvbkbc@...il.com>, alsa-devel@...a-project.org
CC: Mark Rutland <mark.rutland@....com>, devicetree@...r.kernel.org,
Pawel Moll <pawel.moll@....com>, linux-xtensa@...ux-xtensa.org,
Takashi Iwai <tiwai@...e.de>,
Ian Campbell <ijc+devicetree@...lion.org.uk>,
Liam Girdwood <lgirdwood@...il.com>,
Rob Herring <robh+dt@...nel.org>, linux-kernel@...r.kernel.org,
Mark Brown <broonie@...nel.org>,
Kumar Gala <galak@...eaurora.org>,
Grant Likely <grant.likely@...aro.org>
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;
> +}
[...]
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists