RE: [alsa-devel] [PATCHv2 6/6] ASoC: OMAP4: Add McPDM platform driver

[Liam Girdwood <lrg@slimlogic.co.uk>]

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