Re: [PATCH 1/5] ASoC: DMIC: Adding the OMAP DMIC driver

[Felipe Balbi <balbi@ti.com>]

Hi,

On Tue, Dec 28, 2010 at 07:13:58PM -0600, Lambert, David wrote:
>> one blank line only. BTW, are these used anywwhere outside the dmic.c
>> driver ? If not, it's better to move the definitions there.
>>
>
>They were originally in the omap-dmic.h header, but it was suggested
>that we move
>them to a platform header so that the driver could be more
>architecture independent
>and we could put the platform specific details here.  I'm OK putting
>them just about
>anywhere, as long as we have consensus.

The thing I don't like about putting register definitions under
<plat/*.h> is that it creates the need for making the driver "depends on
ARCH_OMAP" because it's the only architecture which has that file. If
you put under <linux/*> or directly on the .c source file, you can have
a much needed compile test with several architectures.

Even though the driver will never work with those other archs, compile
testing with several of them isn't bad at all.

>>> +#ifdef DEBUG
>>> +static void omap_dmic_reg_dump(struct omap_dmic *dmic)
>>> +{
>>> +       dev_dbg(dmic->dev, "***********************\n");
>>> +       dev_dbg(dmic->dev, "OMAP_DMIC_IRQSTATUS_RAW:  0x%04x\n",
>>> +               omap_dmic_read(dmic, OMAP_DMIC_IRQSTATUS_RAW));
>>> +       dev_dbg(dmic->dev, "OMAP_DMIC_IRQSTATUS:  0x%04x\n",
>>> +               omap_dmic_read(dmic, OMAP_DMIC_IRQSTATUS));
>>> +       dev_dbg(dmic->dev, "OMAP_DMIC_IRQENABLE_SET:  0x%04x\n",
>>> +               omap_dmic_read(dmic, OMAP_DMIC_IRQENABLE_SET));
>>> +       dev_dbg(dmic->dev, "OMAP_DMIC_IRQENABLE_CLR:  0x%04x\n",
>>> +               omap_dmic_read(dmic, OMAP_DMIC_IRQENABLE_CLR));
>>> +       dev_dbg(dmic->dev, "OMAP_DMIC_IRQWAKE_EN:  0x%04x\n",
>>> +               omap_dmic_read(dmic, OMAP_DMIC_IRQWAKE_EN));
>>> +       dev_dbg(dmic->dev, "OMAP_DMIC_DMAENABLE_SET:  0x%04x\n",
>>> +               omap_dmic_read(dmic, OMAP_DMIC_DMAENABLE_SET));
>>> +       dev_dbg(dmic->dev, "OMAP_DMIC_DMAENABLE_CLR:  0x%04x\n",
>>> +               omap_dmic_read(dmic, OMAP_DMIC_DMAENABLE_CLR));
>>> +       dev_dbg(dmic->dev, "OMAP_DMIC_DMAWAKEEN:  0x%04x\n",
>>> +               omap_dmic_read(dmic, OMAP_DMIC_DMAWAKEEN));
>>> +       dev_dbg(dmic->dev, "OMAP_DMIC_CTRL:  0x%04x\n",
>>> +               omap_dmic_read(dmic, OMAP_DMIC_CTRL));
>>> +       dev_dbg(dmic->dev, "OMAP_DMIC_DATA:  0x%04x\n",
>>> +               omap_dmic_read(dmic, OMAP_DMIC_DATA));
>>> +       dev_dbg(dmic->dev, "OMAP_DMIC_FIFO_CTRL:  0x%04x\n",
>>> +               omap_dmic_read(dmic, OMAP_DMIC_FIFO_CTRL));
>>> +       dev_dbg(dmic->dev, "OMAP_DMIC_FIFO_DMIC1R_DATA:  0x%08x\n",
>>> +               omap_dmic_read(dmic, OMAP_DMIC_FIFO_DMIC1R_DATA));
>>> +       dev_dbg(dmic->dev, "OMAP_DMIC_FIFO_DMIC1L_DATA:  0x%08x\n",
>>> +               omap_dmic_read(dmic, OMAP_DMIC_FIFO_DMIC1L_DATA));
>>> +       dev_dbg(dmic->dev, "OMAP_DMIC_FIFO_DMIC2R_DATA:  0x%08x\n",
>>> +               omap_dmic_read(dmic, OMAP_DMIC_FIFO_DMIC2R_DATA));
>>> +       dev_dbg(dmic->dev, "OMAP_DMIC_FIFO_DMIC2L_DATA:  0x%08x\n",
>>> +               omap_dmic_read(dmic, OMAP_DMIC_FIFO_DMIC2L_DATA));
>>> +       dev_dbg(dmic->dev, "OMAP_DMIC_FIFO_DMIC3R_DATA:  0x%08x\n",
>>> +               omap_dmic_read(dmic, OMAP_DMIC_FIFO_DMIC3R_DATA));
>>> +       dev_dbg(dmic->dev, "OMAP_DMIC_FIFO_DMIC3L_DATA:  0x%08x\n",
>>> +               omap_dmic_read(dmic, OMAP_DMIC_FIFO_DMIC3L_DATA));
>>> +       dev_dbg(dmic->dev, "***********************\n");
>>> +}
>>> +#else
>>> +static void omap_dmic_reg_dump(struct omap_dmic *dmic) {}
>>> +#endif
>>
>> Would be better to make a debugfs layer ??
>
>I'll look in to what it would take to do this.  Could this be a
>feature to add later?

It could, but this omap_dmic_reg_dump() really doesn't look nice. And
you even have a #undef DEBUG on the top of the file, which will require
code change to actually make this one work.

>>> +static irqreturn_t omap_dmic_irq_handler(int irq, void *dev_id)
>>> +{
>>> +       struct omap_dmic *dmic = dev_id;
>>> +       u32 irq_status;
>>> +
>>> +       irq_status = omap_dmic_read(dmic, OMAP_DMIC_IRQSTATUS);
>>> +
>>> +       /* Acknowledge irq event */
>>> +       omap_dmic_write(dmic, OMAP_DMIC_IRQSTATUS, irq_status);
>>> +       if (irq_status & OMAP_DMIC_IRQ_FULL)
>>> +               dev_dbg(dmic->dev, "DMIC FIFO error %x\n", irq_status);
>>> +
>>> +       if (irq_status & OMAP_DMIC_IRQ_EMPTY)
>>> +               dev_dbg(dmic->dev, "DMIC FIFO error %x\n", irq_status);
>>> +
>>> +       if (irq_status & OMAP_DMIC_IRQ)
>>> +               dev_dbg(dmic->dev, "DMIC write request\n");
>>
>> no locking needed ??
>>
>>> +static int omap_dmic_dai_startup(struct snd_pcm_substream *substream,
>>> +                                 struct snd_soc_dai *dai)
>>> +{
>>> +       struct omap_dmic *dmic = snd_soc_dai_get_drvdata(dai);
>>> +
>>> +       if (!dmic->active++)
>>> +               pm_runtime_get_sync(dmic->dev);
>>> +
>>> +       return 0;
>>> +}
>>> +
>>> +static void omap_dmic_dai_shutdown(struct snd_pcm_substream *substream,
>>> +                                   struct snd_soc_dai *dai)
>>> +{
>>> +       struct omap_dmic *dmic = snd_soc_dai_get_drvdata(dai);
>>> +
>>> +       if (!--dmic->active)
>>> +               pm_runtime_put_sync(dmic->dev);
>>
>> I could be wrong but I believe pm_runtime implementation on OMAP has
>> its own refcounting, so you could drop the need for dmic->active.
>
>This was a included for a future use case where the driver would be
>accessed by the
>Audio Back End.  In this case, there are multiple DAI's associated to
>this driver and
>we need to keep track of the number of active DAI's and only cal
>pm_runtime_put_sync()
>when there are no others running.

Isn't it the same thing ?

DAI 0 -> omap_dmic_dai_startup() -> pm_runtime_get_sync() ->
dev->power.usage_count = 1

DAI 1 -> omap_dmic_dai_startup() -> pm_runtime_get_sync() ->
dev->power.usage_count = 2

DAI 2 -> omap_dmic_dai_startup() -> pm_runtime_get_sync() ->
dev->power.usage_count = 3

DAI 0 -> omap_dmic_dai_shutdown() -> pm_runtime_put_sync() ->
dev->power.usage_count = 2

and so on. Device will only be put in suspend again, when
dev->power.usage_count reaches zero.

>>> +       ret = request_threaded_irq(dmic->irq, NULL, omap_dmic_irq_handler,
>>> +                                  IRQF_ONESHOT, "DMIC", (void *)dmic);
>>
>> Does this need to be threaded ? Doesn't look like. Also, you don't need
>> the (void *) cast.
>
>This was originally not a threaded IRQ handler.  But in an internal
>review, it was
>suggested that this was the current trend in drivers and I should
>follow that course.
>I'm open to having it non-threaded.

Ok, no problem. But in this case you should have a non-threaded part
which will read the IRQ status register to verify we *do* have an IRQ
from this device and only in such case you will wake up the thread by
returning IRQ_WAKE_THREAD.

-- 
balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html