Re: [PATCH 5/6] ASoC: add Component level struct snd_compr_ops

[Charles Keepax <ckeepax@opensource.wolfsonmicro.com>]

On Tue, Dec 20, 2016 at 01:59:43AM +0000, Kuninori Morimoto wrote:
> > > +	for_each_component(component, rtd->card) {
> > > +		const struct snd_compr_ops *compr_ops = component->compr_ops;
> > > +
> > > +		if (compr_ops && compr_ops->open) {
> > > +			int _ret = compr_ops->open(cstream);
> > > +
> > > +			if (_ret < 0) {
> > > +				pr_err("compress asoc: can't open component %s\n",
> > > +				       component->name);
> > > +				ret |= _ret;
> > > +			}
> > 
> > Apologies if I am missing something but this really doesn't look
> > like equivalent code? The old code calls the ops for the platform
> > associated with the stream, the new code looks like it will call
> > the ops for every single component in the system that has
> > compressed ops, which would definitely cause issue. Am I missing
> > something here?
> 
> Basically in *current* existing code, platform only has compr_ops.
> So, converting existing code to new code should be OK.
> 
> Current ALSA SoC clearly separating CPU/Codec/Platform, but in these days,
> new sound device has many features. Thus, it becaming difficult to
> say "it is XXX" today. For example, some Codec has CPU feature,
> some CPU has Platform feature etc.
> So, our (= Me/Lars) plan is that remove current CPU/Codec/Platform separation,
> and all will be Component connection.
> 
> Above code will call every component's compr_ops,
> currently, it should be platform now.
> But in the future device, Codec and/or CPU can have compr_ops.
> 
> If you want, currently, it can call 1st found compr_ops (= should be platform)
> only with FIXME comment at this point.
> 
> Does this clear answer for you ?
> 
> 	for_each_component(component, rtd->card) {
> 		const struct snd_compr_ops *compr_ops = component->compr_ops;
> 
> 		/* FIXME: 1st compr_ops only at this point */
> 		if (compr_ops && compr_ops->open) {

But how do you know this is the correct compressed open here? The
system could have multiple platforms providing compressed ops and
you have just picked the first one in the card list. I think you
are making the assumption that there is only one platform
providing compressed ops and that seems like a very dangerous
assumption to me. Our CODECs provide some compressed features as
do many applications processors, which would easily give you at
two platforms. But I could even imagine APs registering multiple
compressed platforms for different DSPs or some such.

Thanks,
Charles

> 			ret = compr_ops->open(cstream);
> 			if (ret < 0) {
> 				pr_err("compress asoc: can't open component %s\n",
> 				       component->name);
> 			}
> 			break;
> 		}
> 	}