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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
