Re: [PATCH v2 1/4] ASoC: DMIC: Adding the OMAP DMIC driver
On Thu, Jan 6, 2011 at 4:20 PM, Mark Brown
broo...@opensource.wolfsonmicro.com wrote:
On Thu, Jan 06, 2011 at 08:00:36AM -0600, David Lambert wrote:
@@ -103,6 +106,7 @@ config SND_OMAP_SOC_SDP4430
depends on TWL4030_CORE SND_OMAP_SOC MACH_OMAP_4430SDP
select SND_OMAP_SOC_MCPDM
select SND_SOC_TWL6040
+ select SND_SOC_DMIC
help
Say Y if you want to add support for SoC audio on Texas Instruments
SDP4430.
Any tweaks to specific machines should be done separately to adding the
new drivers.
OK... I'll put that in to a separate patch.
+ struct omap_dmic *dmic = snd_soc_dai_get_drvdata(dai);
+ int ctrl, div_sel = -EINVAL;
+
+ if (div_id != OMAP_DMIC_CLKDIV)
+ return -ENODEV;
+
+ switch (dmic-clk_freq) {
+ case 1920:
+ if (div == 5)
+ div_sel = 0x1;
+ else if (div == 8)
+ div_sel = 0x0;
I suggested switch statements previously; you didn't comment on my
reply.
Sorry... my personal standard on when to go with a switch statement is
2 choices,
I'll change it over to a switch...
+static irqreturn_t omap_dmic_irq_handler(int irq, void *dev_id)
+{
+ struct omap_dmic *dmic = dev_id;
My comments on this function appear to have been mostly ignored also.
Being as with this hardware, the IRQ handler really does nothing, I
think it best
if I just take it out for now. It is useful in debugging cases to ensure you're
moving data. I'll just remove it.
+ switch (rate) {
+ case 192000:
+ div = 5;
+ break;
+ default:
+ div = 8;
Shouldn't the default case be a case 96000?
The default case IS 96000 (only options for rate here are 96000 and
192000), isn't it?
+ case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
+ break;
+ case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
+ break;
Remove the empty cases, they're handled by the default.
OK
+
+MODULE_AUTHOR(David Lambert dlamb...@ti.com);
+MODULE_DESCRIPTION(OMAP DMIC SoC Interface);
+MODULE_LICENSE(GPL);
As also previously noted you should have a MODULE_ALIAS.
The MODULE_ALIAS is in there... just following example of the other drivers
I looked at, it was placed above the platform_driver declaration.
+MODULE_ALIAS(platform:omap-dmic-dai);
+
+static struct platform_driver asoc_dmic_driver = {
+ .driver = {
+ .name = omap-dmic-dai,
+ .owner = THIS_MODULE,
+ },
+ .probe = asoc_dmic_probe,
+ .remove = __devexit_p(asoc_dmic_remove),
+};
--
David Lambert
OMAP™ Multimedia
214-567-5692
--
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
Re: [PATCH v2 1/4] ASoC: DMIC: Adding the OMAP DMIC driver
On Fri, Jan 07, 2011 at 10:34:53AM -0600, Lambert, David wrote:
On Thu, Jan 6, 2011 at 4:20 PM, Mark Brown
I suggested switch statements previously; you didn't comment on my
reply.
Sorry... my personal standard on when to go with a switch statement is
2 choices,
I'll change it over to a switch...
Think about the impression you're creating here: if someone had a review
comment on one version of a patch and you neither reply nor address it
in the patch they're very likely to have the same comment again.
+ switch (rate) {
+ case 192000:
+ div = 5;
+ break;
+ default:
+ div = 8;
Shouldn't the default case be a case 96000?
The default case IS 96000 (only options for rate here are 96000 and
192000), isn't it?
Think about this from a robustness, legibility and maintainability point
of view - the above code doesn't clearly do the right thing, and if any
other sample rates are added it'll be buggy.
--
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
Re: [PATCH v2 1/4] ASoC: DMIC: Adding the OMAP DMIC driver
On Thu, Jan 06, 2011 at 08:00:36AM -0600, David Lambert wrote:
@@ -103,6 +106,7 @@ config SND_OMAP_SOC_SDP4430
depends on TWL4030_CORE SND_OMAP_SOC MACH_OMAP_4430SDP
select SND_OMAP_SOC_MCPDM
select SND_SOC_TWL6040
+ select SND_SOC_DMIC
help
Say Y if you want to add support for SoC audio on Texas Instruments
SDP4430.
Any tweaks to specific machines should be done separately to adding the
new drivers.
+ struct omap_dmic *dmic = snd_soc_dai_get_drvdata(dai);
+ int ctrl, div_sel = -EINVAL;
+
+ if (div_id != OMAP_DMIC_CLKDIV)
+ return -ENODEV;
+
+ switch (dmic-clk_freq) {
+ case 1920:
+ if (div == 5)
+ div_sel = 0x1;
+ else if (div == 8)
+ div_sel = 0x0;
I suggested switch statements previously; you didn't comment on my
reply.
+static irqreturn_t omap_dmic_irq_handler(int irq, void *dev_id)
+{
+ struct omap_dmic *dmic = dev_id;
My comments on this function appear to have been mostly ignored also.
+ switch (rate) {
+ case 192000:
+ div = 5;
+ break;
+ default:
+ div = 8;
Shouldn't the default case be a case 96000?
+ case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
+ break;
+ case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
+ break;
Remove the empty cases, they're handled by the default.
+
+MODULE_AUTHOR(David Lambert dlamb...@ti.com);
+MODULE_DESCRIPTION(OMAP DMIC SoC Interface);
+MODULE_LICENSE(GPL);
As also previously noted you should have a MODULE_ALIAS.
--
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
