[alsa-devel] [PATCH V2 1/2] ALSA: Alchemy AC97C/I2SC audio support

[Manuel Lauss]

On Fri, Jul 22, 2011 at 9:40 AM, Lars-Peter Clausen <lars at metafoo.de> wrote:
> On 07/22/2011 08:54 AM, Manuel Lauss wrote:
>> On Fri, Jul 22, 2011 at 1:54 AM, Lars-Peter Clausen <lars at metafoo.de> wrote:
>>>> diff --git a/sound/soc/au1x/db1000.c b/sound/soc/au1x/db1000.c
>>>> +     ret = -ENOMEM;
>>>> +     db1000_asoc97_dev = platform_device_alloc("soc-audio", 0);
>>>
>>> New drivers shouldn't user soc-audio anymore, just register a normal platform
>>> device driver.
>>
>> Can you point to an example of the new way?
>>
>
> sound/soc/samsung/speyside.c

Thanks,



>>>> diff --git a/sound/soc/au1x/dma.c b/sound/soc/au1x/dma.c
>>>> new file mode 100644
>>>> index 0000000..0f7d90a
>>>> --- /dev/null
>>>> +++ b/sound/soc/au1x/dma.c
>>>> @@ -0,0 +1,470 @@
>>>> [...]
>>>> +
>>>> +static struct platform_driver alchemy_ac97pcm_driver = {
>>>> +     .driver = {
>>>> +             .name   = AC97C_DMANAME,
>>>> +             .owner  = THIS_MODULE,
>>>> +     },
>>>> +     .probe          = alchemy_pcm_drvprobe,
>>>> +     .remove         = __devexit_p(alchemy_pcm_drvremove),
>>>> +};
>>>> +
>>>> +static struct platform_driver alchemy_i2spcm_driver = {
>>>> +     .driver = {
>>>> +             .name   = I2SC_DMANAME,
>>>> +             .owner  = THIS_MODULE,
>>>> +     },
>>>> +     .probe          = alchemy_pcm_drvprobe,
>>>> +     .remove         = __devexit_p(alchemy_pcm_drvremove),
>>>> +};
>>>
>>> You shouldn't really have to register two identical drivers for this. If you
>>> really want to be able to instantiate the driver with two different names use
>>> platform_device_id. But in my opinion it should be enough to just have one
>>> generic name, since there is nothing AC97 or I2S specific in this driver.
>>
>> I need a unique name for the DMA device in soc_dai_link.  This was the
>> easiest way. Especially since both ac97 and i2s can be active at
>> runtime.
>
> If you want to instantiate two pcm drivers you can just give the devices
> different ids. As there is nothing I2C or AC97 specific in the pcm driver it
> should not matter which one is used for what, if two devices are active at the
> same time.
> Right now you need to know which one is which, because you instantiate the
> driver with either the I2C or AC97 DMA addresses, but if you use
> snd_soc_dai_get_dma_data as described below and pass the DMA address at runtime
> this issue will go away.

so instead of "alchemy-pcm-{ac97|i2s}" i'd have "alchemy-pcm.[01]". Not really
much clearer in my book.  And I still need to know the correct suffix for
dai link.

Or am I missing something fundamentally?



>>>> [...]
>>>> +
>>>> +struct platform_device *alchemy_pcm_add(struct platform_device *pdev, int type)
>>>> +{
>>> +       struct resource *res, *r;
>>> +       struct platform_device *pd;
>>> +       char *pdevname;
>>> +       int id[2];
>>> +       int ret;
>>> +
>>> +       r = platform_get_resource(pdev, IORESOURCE_DMA, 0);
>>> +       if (!r)
>>> +               return NULL;
>>> +       id[0] = r->start;
>>> +
>>> +       r = platform_get_resource(pdev, IORESOURCE_DMA, 1);
>>> +       if (!r)
>>> +               return NULL;
>>> +       id[1] = r->start;
>>> +
>>> +       res = kzalloc(sizeof(struct resource) * 2, GFP_KERNEL);
>>> +       if (!res)
>>> +               return NULL;
>>> +
>>> +       res[0].start = res[0].end = id[0];
>>> +       res[1].start = res[1].end = id[1];
>>> +       res[0].flags = res[1].flags = IORESOURCE_DMA;
>>> +
>>> +       /* "alchemy-pcm-ac97" or "alchemy-pcm-i2s" */
>>> +       pdevname = (type == 0) ? AC97C_DMANAME : I2SC_DMANAME;
>>> +       pd = platform_device_alloc(pdevname, -1);
>>> +       if (!pd)
>>> +               goto out;
>>> +
>>> +       pd->resource = res;
>>> +       pd->num_resources = 2;
>>> +
>>> +       ret = platform_device_add(pd);
>>> +       if (!ret)
>>> +               return pd;
>>> +
>>> +       platform_device_put(pd);
>>> +out:
>>> +       kfree(res);
>>> +       return NULL;
>>>> +}
>>>
>>> This function looks a bit fishy. The pcm driver should be registered by the
>>> platform code file as well. If you need different DMA regions for I2C and AC97
>>> use snd_soc_dai_set_dma_data and snd_soc_dai_get_dma_data to pass them to the
>>> PCM driver from the I2S or AC97 driver.
>>
>> I like to pass the DMA id's along with the ac97/i2s resource
>> information (since they
>> belong together anyway). As an added benefit I get a sensibly named dma device
>> with the correct DMA information, all by simply registering the ac97
>> platform device.
>
> There is nothing wrong with passing the DMA ids along with the other AC97/I2C
> resources. At least for the AC97 and I2C driver. But the PCM driver should use
> snd_soc_dai_get_dma_data to get the DMA addresses at runtime rather then during
> device instantiation. Take a look at how other platforms handle this.


The ac97/i2s units know they need dma and therefore register the
device themselves.
Having the board code register an "empty" dma device along with the
ac97/i2s devices
seems like a waste of code and a source of potential bugs.  I'll change my code.

Thank you!
        Manuel Lauss
--
To unsubscribe from this list: send the line "unsubscribe alsa-devel" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html