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@xxxxxxxxxxxxxxx>
>
> Some thoughts below, but it shouldn't prevent v10 to appear.
>
>> Signed-off-by: Cezary Rojewski <cezary.rojewski@xxxxxxxxx>
...
>> +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
