On Thu, Jan 6, 2011 at 4:20 PM, Mark Brown
<broonie@xxxxxxxxxxxxxxxxxxxxxxxxxxx> 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 19200000:
>> + 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 <dlambert@xxxxxx>");
>> +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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
