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

["Lambert, David" <dlambert@ti.com>]

On Thu, Jan 6, 2011 at 4:20 PM, Mark Brown
<broonie@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 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@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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html