[alsa-devel] [PATCH 08/12] ASoC: SOF: Generic probe compress operations

[Pierre-Louis Bossart]

On 1/27/20 6:28 AM, Cezary Rojewski wrote:
> On 2020-01-24 21:00, Pierre-Louis Bossart wrote:
>>
>>> +int sof_probe_compr_open(struct snd_compr_stream *cstream,
>>> +        struct snd_soc_dai *dai)
>>> +{
>>> +    struct snd_sof_dev *sdev =
>>> +                snd_soc_component_get_drvdata(dai->component);
>>> +    int ret;
>>> +
>>> +    ret = snd_sof_probe_compr_assign(sdev, cstream, dai);
>>> +    if (ret < 0) {
>>> +        dev_err(dai->dev, "Failed to assign probe stream: %d\n", ret);
>>> +        return ret;
>>> +    }
>>> +
>>> +    sdev->extractor = ret;
>>
>> could you either rename the 'extractor' field to something meaningful 
>> or add a comment on what is stored in this field? A stream tag? a 
>> device number? It's only used once for the init.
> 
> 'extractor' is _very_ meaningful and explicit so the naming should stay. 
> Rewording to stream_tag / dev num or anything of that sort would achieve 
> the exact opposite: ambiguity. Code around 'extractor' usage clearly 
> states what it is for. I'd rather prefer such descriptions to stay 
> within Documentation - which will be released on a later date as SOF's 
> probes are very, very fresh subject.

'extractor' is an integer, and there's nothing that tells me what values 
it can take, and what it corresponds to.

You also store the result of 'probe_compr_assign' but don't use it in a 
free. The only place where I see it used is

+    ret = sof_ipc_probe_init(sdev, sdev->extractor, rtd->dma_bytes);

And then if look back at Patch 7, I see this:

+int sof_ipc_probe_init(struct snd_sof_dev *sdev,
+		u32 stream_tag, size_t buffer_size)

so your 'extractor' is really an 'extractor_stream_tag'.

BTW please try and compile with W=1, you'll see kernel-doc errors with 
missing parameters, thanks!