Date: Fri, 23 Feb 2024 22:22:50 -0800
From: Wesley Cheng <quic_wcheng@...cinc.com>
To: Takashi Iwai <tiwai@...e.de>
CC: <srinivas.kandagatla@...aro.org>, <mathias.nyman@...el.com>,
        <perex@...ex.cz>, <conor+dt@...nel.org>, <corbet@....net>,
        <lgirdwood@...il.com>, <andersson@...nel.org>,
        <krzysztof.kozlowski+dt@...aro.org>, <gregkh@...uxfoundation.org>,
        <Thinh.Nguyen@...opsys.com>, <broonie@...nel.org>,
        <bgoswami@...cinc.com>, <tiwai@...e.com>, <robh+dt@...nel.org>,
        <konrad.dybcio@...aro.org>, <linux-kernel@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <linux-sound@...r.kernel.org>,
        <linux-usb@...r.kernel.org>, <linux-arm-msm@...r.kernel.org>,
        <linux-doc@...r.kernel.org>, <alsa-devel@...a-project.org>
Subject: Re: [PATCH v14 32/53] ALSA: usb-audio: Check for support for
 requested audio format

Hi Takashi,

On 2/17/2024 2:08 AM, Takashi Iwai wrote:
> On Sat, 17 Feb 2024 00:42:18 +0100,
> Wesley Cheng wrote:
>>
>> Hi Takashi,
>>
>> On 2/9/2024 1:34 PM, Wesley Cheng wrote:
>>> Hi Takashi,
>>>
>>> On 2/9/2024 2:42 AM, Takashi Iwai wrote:
>>>> On Fri, 09 Feb 2024 00:13:45 +0100,
>>>> Wesley Cheng wrote:
>>>>>
>>>>> Allow for checks on a specific USB audio device to see if a
>>>>> requested PCM
>>>>> format is supported.  This is needed for support when playback is
>>>>> initiated by the ASoC USB backend path.
>>>>>
>>>>> Signed-off-by: Wesley Cheng <quic_wcheng@...cinc.com>
>>>>> ---
>>>>>    sound/usb/card.c | 31 +++++++++++++++++++++++++++++++
>>>>>    sound/usb/card.h | 11 +++++++++++
>>>>>    2 files changed, 42 insertions(+)
>>>>>
>>>>> diff --git a/sound/usb/card.c b/sound/usb/card.c
>>>>> index 7dc8007ba839..1ad99a462038 100644
>>>>> --- a/sound/usb/card.c
>>>>> +++ b/sound/usb/card.c
>>>>> @@ -155,6 +155,37 @@ int snd_usb_unregister_platform_ops(void)
>>>>>    }
>>>>>    EXPORT_SYMBOL_GPL(snd_usb_unregister_platform_ops);
>>>>> +/*
>>>>> + * Checks to see if requested audio profile, i.e sample rate, # of
>>>>> + * channels, etc... is supported by the substream associated to the
>>>>> + * USB audio device.
>>>>> + */
>>>>> +struct snd_usb_stream *snd_usb_find_suppported_substream(int card_idx,
>>>>> +            struct snd_pcm_hw_params *params, int direction)
>>>>> +{
>>>>> +    struct snd_usb_audio *chip;
>>>>> +    struct snd_usb_substream *subs;
>>>>> +    struct snd_usb_stream *as;
>>>>> +
>>>>> +    /*
>>>>> +     * Register mutex is held when populating and clearing usb_chip
>>>>> +     * array.
>>>>> +     */
>>>>> +    guard(mutex)(&register_mutex);
>>>>> +    chip = usb_chip[card_idx];
>>>>> +
>>>>> +    if (chip && enable[card_idx]) {
>>>>> +        list_for_each_entry(as, &chip->pcm_list, list) {
>>>>> +            subs = &as->substream[direction];
>>>>> +            if (snd_usb_find_substream_format(subs, params))
>>>>> +                return as;
>>>>> +        }
>>>>> +    }
>>>>> +
>>>>> +    return NULL;
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(snd_usb_find_suppported_substream);
>>>>> +
>>>>>    /*
>>>>>     * disconnect streams
>>>>>     * called from usb_audio_disconnect()
>>>>> diff --git a/sound/usb/card.h b/sound/usb/card.h
>>>>> index 02e4ea898db5..ed4a664e24e5 100644
>>>>> --- a/sound/usb/card.h
>>>>> +++ b/sound/usb/card.h
>>>>> @@ -217,4 +217,15 @@ struct snd_usb_platform_ops {
>>>>>    int snd_usb_register_platform_ops(struct snd_usb_platform_ops *ops);
>>>>>    int snd_usb_unregister_platform_ops(void);
>>>>> +
>>>>> +#if IS_ENABLED(CONFIG_SND_USB_AUDIO)
>>>>> +struct snd_usb_stream *snd_usb_find_suppported_substream(int card_idx,
>>>>> +            struct snd_pcm_hw_params *params, int direction);
>>>>> +#else
>>>>> +static struct snd_usb_stream
>>>>> *snd_usb_find_suppported_substream(int card_idx,
>>>>> +            struct snd_pcm_hw_params *params, int direction)
>>>>> +{
>>>>> +    return NULL;
>>>>> +}
>>>>> +#endif /* IS_ENABLED(CONFIG_SND_USB_AUDIO) */
>>>>
>>>> The usefulness of ifdef guard here is doubtful, IMO.  This header is
>>>> only for USB-audio driver enablement, and not seen as generic
>>>> helpers.  So, just add the new function declarations without dummy
>>>> definitions.
>>>>
>>>
>>> Got it, will remove it.  We also have a dependency in place for the
>>> qc_audio_offload driver and SND USB AUDIO in the Kconfig.
>>>
>>
>> Looking at this again after trying some mixed Kconfig settings.  These
>> declarations aren't specific for USB-audio.  They are helpers that are
>> exposed to soc usb, so that it can do some basic verification with soc
>> usb before allowing the enable stream to continue.
> 
> Then rather the question is why snd-soc-usb calls those functions
> *unconditionally*.  No matter whether we have dependencies in Kconfig,
> calling the function means that the callee shall be drug when the
> corresponding code is running.
> 
> If it were generic core API stuff such as power-management or ACPI,
> it'd make sense to define dummy functions without the enablement, as
> many code may have optional calls.  If the API is enabled, it's anyway
> in the core.  If not, it's optional.  That'll be fine.
> 
> OTOH, the stuff you're calling certainly belongs to snd-usb-audio.
> Even if the call is really optional, it means that you'll have a hard
> dependency when snd-usb-audio is built, no matter whether you need or
> not.
> 
>> Since the ASoC
>> layer doesn't have insight on what audio profiles are supported by the
>> usb device, this API will ensure that the request profile is
>> supported.
>>
>> Issues are seen when we disable SND USB audio config and enable the
>> ASoC parts.
> 
> If snd-usb-audio is disabled, what snd-soc-usb would serve at all?
> Does it still make sense to keep it enabled?
> That said, the statement above (building snd-soc-usb without
> snd-usb-audio) looks already dubious; isn't it better to have a proper
> dependency in Kconfig, instead?
> 

Ok, took a look at it a bit more and should have gotten all the 
dependencies addressed through Kconfigs.  Thanks for the review comments 
and feedback.

Thanks
Wesley Cheng

Powered by blists - more mailing lists