RE: [alsa-devel] [PATCHv2 6/6] ASoC: OMAP4: Add McPDM platform driver
Jarkko Nikula wrote:
> Hi
>
> On Mon, 25 Jan 2010 15:06:44 -0600
> "Candelaria Villareal, Jorge" wrote:
>
> > > > +static int omap_mcpdm_dai_startup(struct snd_pcm_substream
> > > *substream,
> > > > + struct snd_soc_dai *dai)
> > > > +{
> > > > + struct snd_soc_pcm_runtime *rtd =
> substream->private_data;
> > > > + struct snd_soc_dai *cpu_dai = rtd->dai->cpu_dai;
> > > > + int err = 0;
> > > > +
> > > > + if (!cpu_dai->active)
> > > > + err = omap_mcpdm_request();
> > >
> > > Will anything else use this hw interface other than ALSA audio ?
> > > If not, the request is probably better in the machine
> driver probe().
> >
> > omap_mcpdm_request will enable the functional clock. Isn't it better
> > for the clock to be enabled only when is about to get used?
> >
> Definitely yes if there is no any need to keep block active after the
> request. That would help the power-management if there are no active
> clocks when the streams are suspended with omap_mcpdm_stop() but the
> block remains reserved (i.e. omap_mcpdm_free is not called).
>
> The current McBSP implementation is not the best example here but
> hopefully that will get fixed at some point.
>
> > > stream is either PLAYBACK or CAPTURE here.
> >
> > Downlink and uplink were chosen instead to avoid confusion
> > because that is how it is referred in McPDM technical
> > documentation. But I do understand that we are talking
> > about ALSA stream, not McPDM...
> ...
> > Here, however, the methods set_uplink & set_downlink are
> > Named after McPDM data paths. In this case is it preferred
> > to name everything using ALSA convention? Or would it be ok
> > to use downlink/uplink on certain cases?
> >
> I think something like below is clear enough. It is easy to
> see that we
> are dealing with ALSA playback stream which corresponds to
> downlink path
> in HW.
>
> if (stream == SNDRV_PCM_STREAM_PLAYBACK)
> omap_mcpdm_downlink_opxxx();
>
> Also for the user(-space) standard playback and capture naming is more
> clear than downlink/uplink terminology.
So the idea would be to show that ALSA playback/capture correspond
to McPDM downlink/uplink paths. Sounds good, I can try this for all
downlink/uplink operations.
>
>
> --
> Jarkko
> --
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: [alsa-devel] [PATCHv2 6/6] ASoC: OMAP4: Add McPDM platform driver
On Mon, 2010-01-25 at 15:06 -0600, Candelaria Villareal, Jorge wrote:
> From: Liam Girdwood wrote:
> > On Fri, 2010-01-22 at 17:15 -0600, Candelaria Villareal, Jorge wrote:
snip
> > > +static int omap_mcpdm_dai_hw_params(struct
> > snd_pcm_substream *substream,
> > > + struct snd_pcm_hw_params *params,
> > > + struct snd_soc_dai *dai)
> > > +{
> > > + struct snd_soc_pcm_runtime *rtd = substream->private_data;
> > > + struct snd_soc_dai *cpu_dai = rtd->dai->cpu_dai;
> > > + struct omap_mcpdm_data *mcpdm_priv = cpu_dai->private_data;
> > > + struct omap_mcpdm_link *mcpdm_links = mcpdm_priv->links;
> > > + int stream = substream->stream;
> > > + int channels, err, link_mask = 0;
> > > +
> > > + cpu_dai->dma_data = &omap_mcpdm_dai_dma_params[stream];
> > > +
> > > + channels = params_channels(params);
> > > + switch (channels) {
> > > + case 4:
> > > + if (stream)
> >
> > stream is either PLAYBACK or CAPTURE here.
>
> Downlink and uplink were chosen instead to avoid confusion
> because that is how it is referred in McPDM technical
> documentation. But I do understand that we are talking
> about ALSA stream, not McPDM...
>
> >
> > > + /* up to 2 channels for uplink */
>
> So I should refer to CAPTURE instead of UPLINK in here.
I think it's best to use both CAPTURE and UPLINK in the comments.
>
> > > + return -EINVAL;
> > > + link_mask |= 1 << 3;
> > > + case 3:
> > > + if (stream)
> > > + /* up to 2 channels for uplink */
> > > + return -EINVAL;
> > > + link_mask |= 1 << 2;
> > > + case 2:
> > > + link_mask |= 1 << 1;
> > > + case 1:
> > > + link_mask |= 1 << 0;
> > > + break;
> > > + default:
> > > + /* unsupported number of channels */
> > > + return -EINVAL;
> > > + }
> > > +
> > > + if (stream) {
> >
> > ditto
>
> Here, however, the methods set_uplink & set_downlink are
> Named after McPDM data paths. In this case is it preferred
> to name everything using ALSA convention? Or would it be ok
> to use downlink/uplink on certain cases?
I would name after ALSA but also have comments to state mcpdm link.
Thanks
Liam
--
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: [alsa-devel] [PATCHv2 6/6] ASoC: OMAP4: Add McPDM platform driver
On Tue, 2010-01-26 at 10:27 +0200, Jarkko Nikula wrote:
> Hi
>
> On Mon, 25 Jan 2010 15:06:44 -0600
> "Candelaria Villareal, Jorge" wrote:
>
> > > > +static int omap_mcpdm_dai_startup(struct snd_pcm_substream
> > > *substream,
> > > > + struct snd_soc_dai *dai)
> > > > +{
> > > > + struct snd_soc_pcm_runtime *rtd = substream->private_data;
> > > > + struct snd_soc_dai *cpu_dai = rtd->dai->cpu_dai;
> > > > + int err = 0;
> > > > +
> > > > + if (!cpu_dai->active)
> > > > + err = omap_mcpdm_request();
> > >
> > > Will anything else use this hw interface other than ALSA audio ?
> > > If not, the request is probably better in the machine driver probe().
> >
> > omap_mcpdm_request will enable the functional clock. Isn't it better
> > for the clock to be enabled only when is about to get used?
> >
> Definitely yes if there is no any need to keep block active after the
> request. That would help the power-management if there are no active
> clocks when the streams are suspended with omap_mcpdm_stop() but the
> block remains reserved (i.e. omap_mcpdm_free is not called).
Ah ok, it seems that some other platforms have similar get_resource_x()
functions that do not additionally call their clk_enable_resource_x().
(last time I looked).
This is fine.
Liam
--
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: [alsa-devel] [PATCHv2 6/6] ASoC: OMAP4: Add McPDM platform driver
Hi
On Mon, 25 Jan 2010 15:06:44 -0600
"Candelaria Villareal, Jorge" wrote:
> > > +static int omap_mcpdm_dai_startup(struct snd_pcm_substream
> > *substream,
> > > + struct snd_soc_dai *dai)
> > > +{
> > > + struct snd_soc_pcm_runtime *rtd = substream->private_data;
> > > + struct snd_soc_dai *cpu_dai = rtd->dai->cpu_dai;
> > > + int err = 0;
> > > +
> > > + if (!cpu_dai->active)
> > > + err = omap_mcpdm_request();
> >
> > Will anything else use this hw interface other than ALSA audio ?
> > If not, the request is probably better in the machine driver probe().
>
> omap_mcpdm_request will enable the functional clock. Isn't it better
> for the clock to be enabled only when is about to get used?
>
Definitely yes if there is no any need to keep block active after the
request. That would help the power-management if there are no active
clocks when the streams are suspended with omap_mcpdm_stop() but the
block remains reserved (i.e. omap_mcpdm_free is not called).
The current McBSP implementation is not the best example here but
hopefully that will get fixed at some point.
> > stream is either PLAYBACK or CAPTURE here.
>
> Downlink and uplink were chosen instead to avoid confusion
> because that is how it is referred in McPDM technical
> documentation. But I do understand that we are talking
> about ALSA stream, not McPDM...
...
> Here, however, the methods set_uplink & set_downlink are
> Named after McPDM data paths. In this case is it preferred
> to name everything using ALSA convention? Or would it be ok
> to use downlink/uplink on certain cases?
>
I think something like below is clear enough. It is easy to see that we
are dealing with ALSA playback stream which corresponds to downlink path
in HW.
if (stream == SNDRV_PCM_STREAM_PLAYBACK)
omap_mcpdm_downlink_opxxx();
Also for the user(-space) standard playback and capture naming is more
clear than downlink/uplink terminology.
--
Jarkko
--
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: [alsa-devel] [PATCHv2 6/6] ASoC: OMAP4: Add McPDM platform driver
From: Liam Girdwood wrote:
> On Fri, 2010-01-22 at 17:15 -0600, Candelaria Villareal, Jorge wrote:
> > From: Misael Lopez Cruz
> >
> > McPDM platform driver is configured to use sDMA in order to transfer
> > to/from memory. Support for interfacing with ABE will be
> added later.
> >
> > McPDM dai currently supports up to 4 downlink channels and 2 uplink
> > channels simultaneously, as well as 88.2 and 96 KHz, and a sample
> > size of 32 bits.
> >
> > Signed-off-by: Misael Lopez Cruz
> > Signed-off-by: Margarita Olaya
> > Signed-off-by: Jorge Eduardo Candelaria
> > ---
> > sound/soc/omap/Makefile |2 +-
> > sound/soc/omap/omap-mcpdm.c | 250
> +++
> > sound/soc/omap/omap-mcpdm.h | 29 +
> > 3 files changed, 280 insertions(+), 1 deletions(-)
> > create mode 100644 sound/soc/omap/omap-mcpdm.c
> > create mode 100644 sound/soc/omap/omap-mcpdm.h
> >
> > diff --git a/sound/soc/omap/Makefile b/sound/soc/omap/Makefile
> > index bf8b388..d6382fa 100644
> > --- a/sound/soc/omap/Makefile
> > +++ b/sound/soc/omap/Makefile
> > @@ -1,7 +1,7 @@
> > # OMAP Platform Support
> > snd-soc-omap-objs := omap-pcm.o
> > snd-soc-omap-mcbsp-objs := omap-mcbsp.o
> > -snd-soc-omap-mcpdm-objs := mcpdm.o
> > +snd-soc-omap-mcpdm-objs := mcpdm.o omap-mcpdm.o
> >
> > obj-$(CONFIG_SND_OMAP_SOC) += snd-soc-omap.o
> > obj-$(CONFIG_SND_OMAP_SOC_MCBSP) += snd-soc-omap-mcbsp.o
> > diff --git a/sound/soc/omap/omap-mcpdm.c
> b/sound/soc/omap/omap-mcpdm.c
> > new file mode 100644
> > index 000..180cca6
> > --- /dev/null
> > +++ b/sound/soc/omap/omap-mcpdm.c
> > @@ -0,0 +1,250 @@
> > +/*
> > + * omap-mcpdm.c -- OMAP ALSA SoC DAI driver using McPDM port
> > + *
> > + * Copyright (C) 2009 Texas Instruments
> > + *
> > + * Author: Misael Lopez Cruz
> > + * Contact: Jorge Eduardo Candelaria
> > + * Margarita Olaya
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * version 2 as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be
> useful, but
> > + * WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
> See the GNU
> > + * General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General
> Public License
> > + * along with this program; if not, write to the Free Software
> > + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
> > + * 02110-1301 USA
> > + *
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#include
> > +#include
> > +#include
> > +#include "mcpdm.h"
> > +#include "omap-mcpdm.h"
> > +#include "omap-pcm.h"
> > +
> > +struct omap_mcpdm_data {
> > + struct omap_mcpdm_link *links;
> > + int active;
> > +};
> > +
> > +static struct omap_mcpdm_link omap_mcpdm_links[] = {
> > + /* downlink */
> > + {
> > + .irq_mask = MCPDM_DN_IRQ_EMPTY | MCPDM_DN_IRQ_FULL,
> > + .threshold = 1,
> > + .format = PDMOUTFORMAT_LJUST,
> > + },
> > + /* uplink */
> > + {
> > + .irq_mask = MCPDM_UP_IRQ_EMPTY | MCPDM_UP_IRQ_FULL,
> > + .threshold = 1,
> > + .format = PDMOUTFORMAT_LJUST,
> > + },
> > +};
> > +
> > +static struct omap_mcpdm_data mcpdm_data = {
> > + .links = omap_mcpdm_links,
> > + .active = 0,
> > +};
> > +
> > +/*
> > + * Stream DMA parameters
> > + */
> > +static struct omap_pcm_dma_data omap_mcpdm_dai_dma_params[] = {
> > + {
> > + .name = "Audio downlink",
> > + .dma_req = OMAP44XX_DMA_MCPDM_DL,
> > + .data_type = OMAP_DMA_DATA_TYPE_S32,
> > + .sync_mode = OMAP_DMA_SYNC_PACKET,
> > + .packet_size = 16,
> > + .port_addr = OMAP44XX_MCPDM_L3_BASE + MCPDM_DN_DATA,
> > + },
> > + {
> > + .name = "Audio uplink",
> > + .dma_req = OMAP44XX_DMA_MCPDM_UP,
> > + .data_type = OMAP_DMA_DATA_TYPE_S32,
> > + .sync_mode = OMAP_DMA_SYNC_PACKET,
> > + .packet_size = 16,
> > + .port_addr = OMAP44XX_MCPDM_L3_BASE + MCPDM_UP_DATA,
> > + },
> > +};
> > +
> > +static int omap_mcpdm_dai_startup(struct snd_pcm_substream
> *substream,
> > + struct snd_soc_dai *dai)
> > +{
> > + struct snd_soc_pcm_runtime *rtd = substream->private_data;
> > + struct snd_soc_dai *cpu_dai = rtd->dai->cpu_dai;
> > + int err = 0;
> > +
> > + if (!cpu_dai->active)
> > + err = omap_mcpdm_request();
>
> Will anything else use this hw interface other than ALSA audio ?
> If not, the request is probably better in the machine driver probe().
omap_mcpdm_request will enable the functional clock. Isn't it better
for the clock to be enabled only when is about to get used?
>
> > +
Re: [alsa-devel] [PATCHv2 6/6] ASoC: OMAP4: Add McPDM platform driver
On Fri, 2010-01-22 at 17:15 -0600, Candelaria Villareal, Jorge wrote:
> From: Misael Lopez Cruz
>
> McPDM platform driver is configured to use sDMA in order to transfer
> to/from memory. Support for interfacing with ABE will be added later.
>
> McPDM dai currently supports up to 4 downlink channels and 2 uplink
> channels simultaneously, as well as 88.2 and 96 KHz, and a sample
> size of 32 bits.
>
> Signed-off-by: Misael Lopez Cruz
> Signed-off-by: Margarita Olaya
> Signed-off-by: Jorge Eduardo Candelaria
> ---
> sound/soc/omap/Makefile |2 +-
> sound/soc/omap/omap-mcpdm.c | 250
> +++
> sound/soc/omap/omap-mcpdm.h | 29 +
> 3 files changed, 280 insertions(+), 1 deletions(-)
> create mode 100644 sound/soc/omap/omap-mcpdm.c
> create mode 100644 sound/soc/omap/omap-mcpdm.h
>
> diff --git a/sound/soc/omap/Makefile b/sound/soc/omap/Makefile
> index bf8b388..d6382fa 100644
> --- a/sound/soc/omap/Makefile
> +++ b/sound/soc/omap/Makefile
> @@ -1,7 +1,7 @@
> # OMAP Platform Support
> snd-soc-omap-objs := omap-pcm.o
> snd-soc-omap-mcbsp-objs := omap-mcbsp.o
> -snd-soc-omap-mcpdm-objs := mcpdm.o
> +snd-soc-omap-mcpdm-objs := mcpdm.o omap-mcpdm.o
>
> obj-$(CONFIG_SND_OMAP_SOC) += snd-soc-omap.o
> obj-$(CONFIG_SND_OMAP_SOC_MCBSP) += snd-soc-omap-mcbsp.o
> diff --git a/sound/soc/omap/omap-mcpdm.c b/sound/soc/omap/omap-mcpdm.c
> new file mode 100644
> index 000..180cca6
> --- /dev/null
> +++ b/sound/soc/omap/omap-mcpdm.c
> @@ -0,0 +1,250 @@
> +/*
> + * omap-mcpdm.c -- OMAP ALSA SoC DAI driver using McPDM port
> + *
> + * Copyright (C) 2009 Texas Instruments
> + *
> + * Author: Misael Lopez Cruz
> + * Contact: Jorge Eduardo Candelaria
> + * Margarita Olaya
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
> + * 02110-1301 USA
> + *
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +#include
> +#include "mcpdm.h"
> +#include "omap-mcpdm.h"
> +#include "omap-pcm.h"
> +
> +struct omap_mcpdm_data {
> + struct omap_mcpdm_link *links;
> + int active;
> +};
> +
> +static struct omap_mcpdm_link omap_mcpdm_links[] = {
> + /* downlink */
> + {
> + .irq_mask = MCPDM_DN_IRQ_EMPTY | MCPDM_DN_IRQ_FULL,
> + .threshold = 1,
> + .format = PDMOUTFORMAT_LJUST,
> + },
> + /* uplink */
> + {
> + .irq_mask = MCPDM_UP_IRQ_EMPTY | MCPDM_UP_IRQ_FULL,
> + .threshold = 1,
> + .format = PDMOUTFORMAT_LJUST,
> + },
> +};
> +
> +static struct omap_mcpdm_data mcpdm_data = {
> + .links = omap_mcpdm_links,
> + .active = 0,
> +};
> +
> +/*
> + * Stream DMA parameters
> + */
> +static struct omap_pcm_dma_data omap_mcpdm_dai_dma_params[] = {
> + {
> + .name = "Audio downlink",
> + .dma_req = OMAP44XX_DMA_MCPDM_DL,
> + .data_type = OMAP_DMA_DATA_TYPE_S32,
> + .sync_mode = OMAP_DMA_SYNC_PACKET,
> + .packet_size = 16,
> + .port_addr = OMAP44XX_MCPDM_L3_BASE + MCPDM_DN_DATA,
> + },
> + {
> + .name = "Audio uplink",
> + .dma_req = OMAP44XX_DMA_MCPDM_UP,
> + .data_type = OMAP_DMA_DATA_TYPE_S32,
> + .sync_mode = OMAP_DMA_SYNC_PACKET,
> + .packet_size = 16,
> + .port_addr = OMAP44XX_MCPDM_L3_BASE + MCPDM_UP_DATA,
> + },
> +};
> +
> +static int omap_mcpdm_dai_startup(struct snd_pcm_substream *substream,
> + struct snd_soc_dai *dai)
> +{
> + struct snd_soc_pcm_runtime *rtd = substream->private_data;
> + struct snd_soc_dai *cpu_dai = rtd->dai->cpu_dai;
> + int err = 0;
> +
> + if (!cpu_dai->active)
> + err = omap_mcpdm_request();
Will anything else use this hw interface other than ALSA audio ?
If not, the request is probably better in the machine driver probe().
> +
> + return err;
> +}
> +
> +static void omap_mcpdm_dai_shutdown(struct snd_pcm_substream *substream,
> + struct snd_soc_dai *dai)
> +{
> + struct snd_soc_pcm_runtime *rtd = substream->private_data;
> + struct snd_soc_dai *cpu_dai = rtd->dai->cpu_dai;
> +
> + if (!cpu_dai->active)
> + omap_mcpdm_free();
ditto.
> +}
> +
>
