RE: [PATCH v9 06/14] ASoC: Intel: catpt: PCM operations

From: "Rojewski, Cezary" <cezary.rojewski@intel.com>
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: "pierre-louis.bossart@linux.intel.com"
	<pierre-louis.bossart@linux.intel.com>,
	"alsa-devel@alsa-project.org" <alsa-devel@alsa-project.org>,
	"Kaczmarski, Filip" <filip.kaczmarski@intel.com>,
	"N, Harshapriya" <harshapriya.n@intel.com>,
	"gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
	"Barlik, Marcin" <marcin.barlik@intel.com>,
	"zwisler@google.com" <zwisler@google.com>,
	"lgirdwood@gmail.com" <lgirdwood@gmail.com>,
	"tiwai@suse.com" <tiwai@suse.com>,
	"Proborszcz, Filip" <filip.proborszcz@intel.com>,
	"broonie@kernel.org" <broonie@kernel.org>,
	"amadeuszx.slawinski@linux.intel.com"
	<amadeuszx.slawinski@linux.intel.com>,
	"Wasko, Michal" <michal.wasko@intel.com>,
	"cujomalainey@chromium.org" <cujomalainey@chromium.org>,
	"Hejmowski, Krzysztof" <krzysztof.hejmowski@intel.com>,
	"ppapierkowski@habana.ai" <ppapierkowski@habana.ai>,
	"Gopal,  Vamshi Krishna" <vamshi.krishna.gopal@intel.com>
Subject: RE: [PATCH v9 06/14] ASoC: Intel: catpt: PCM operations
Date: Tue, 29 Sep 2020 13:26:51 +0000	[thread overview]
Message-ID: <80336977ccb643b49559492fa9ca5927@intel.com> (raw)
In-Reply-To: <20200929113340.GH3956970@smile.fi.intel.com>

On 2020-09-29 1:33 PM, Andy Shevchenko wrote:
> On Sat, Sep 26, 2020 at 10:59:02AM +0200, Cezary Rojewski wrote:
>> DSP designed for Lynxpoint and Wildcat Point offers no dynamic topology
>> i.e. all pipelines are already defined within firmware and host is
>> relegated to allocing stream for predefined pins. This is represented by
>> 'catpt_topology' member.
>>
>> Implementation covers all available pin types:
>> - system playback and capture
>> - two offload streams
>> - loopback (reference)
>> - bluetooth playback and capture
>>
>> PCM DAI operations differentiate between those pins as some (mainly
>> offload) are to be handled differently - DSP expects wp updates on each
>> notify_position notification.
>>
>> System playback has no volume control capability as it is routed to
>> mixer stream directly. Other primary streams - capture and two offloads
>> - offer individual volume controls.
>>
>> Compared to sound/soc/intel/haswell this configures SSP device format
>> automatically on pcm creation.
> 
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> Some thoughts below, but it shouldn't prevent v10 to appear.
> 
>> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>

...

>> +static u32 catpt_get_channel_map(enum catpt_channel_config config)
>> +{
>> +	switch (config) {
>> +	case CATPT_CHANNEL_CONFIG_MONO:
>> +		return GENMASK(31, 4) | CATPT_CHANNEL_CENTER;
> 
>> +	case CATPT_CHANNEL_CONFIG_DUAL_MONO:
>> +		return GENMASK(31, 8) | CATPT_CHANNEL_LEFT
>> +				      | (CATPT_CHANNEL_LEFT << 4);
> 
> Now we know why it used to be at the end of the switch-case. Up to you to leave
> here or move back.
> 

Reverting to previous relocation in v10.

...

>> +static int catpt_dai_apply_usettings(struct snd_soc_dai *dai,
>> +				     struct catpt_stream_runtime *stream)
>> +{
>> +	struct catpt_dev *cdev = dev_get_drvdata(dai->dev);
>> +	struct snd_soc_component *component = dai->component;
>> +	struct snd_kcontrol *pos, *kctl = NULL;
>> +	const char *name;
>> +	int ret;
>> +	u32 id = stream->info.stream_hw_id;
>> +
>> +	/* only selected streams have individual controls */
>> +	switch (id) {
>> +	case CATPT_PIN_ID_OFFLOAD1:
>> +		name = "Media0 Playback Volume";
>> +		break;
>> +	case CATPT_PIN_ID_OFFLOAD2:
>> +		name = "Media1 Playback Volume";
>> +		break;
>> +	case CATPT_PIN_ID_CAPTURE1:
>> +		name = "Mic Capture Volume";
>> +		break;
>> +	case CATPT_PIN_ID_REFERENCE:
>> +		name = "Loopback Mute";
>> +		break;
>> +	default:
>> +		return 0;
>> +	};
>> +
>> +	list_for_each_entry(pos, &component->card->snd_card->controls, list) {
>> +		if (pos->private_data == component &&
>> +		    !strncmp(name, pos->id.name, sizeof(pos->id.name))) {
>> +			kctl = pos;
>> +			break;
>> +		}
>> +	}
>> +	if (!kctl)
>> +		return -ENOENT;
> 
> Hmm... Seems we lack of something like
> 
> /*
>   * list_entry_is_head() - Test if the entry points to the head of the list
>   * ...
>   */
> #define list_entry_is_head(pos, head, member)	(&pos->member == head)
> 
> Up to you to consider. In above case we may drop pos from above loop and use kctl directly
> 
> 	struct snd_kcontrol *kctl;
> 	...
> 	list_for_each_entry(kctl, &component->card->snd_card->controls, list) {
> 		if (kctl->private_data == component &&
> 		    !strncmp(name, kctl->id.name, sizeof(kctl->id.name)))
> 			break;
> 	}
> 	if (list_entry_is_head(kctl, &component->card->snd_card->controls, list))
> 		return -ENOENT;
> 
> Note, you usually don't need any special Acks to modify list.h this way,
> although Andrew's name pops up WRT library stuff.
> 

Since you've already proposed the solution to LKML, once it's added to
the kernel, it will be re-used in catpt just like resource_union().

Good ideas all around, Andy. It's rather surprising to me how many good
ideas came during the upstream process of this small driver.

Czarek