Received-SPF: pass (google.com: domain of linux-kernel+bounces-79483-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <76922981-88ec-a376-ce61-ea1ff85f43d2@quicinc.com>
Date: Fri, 23 Feb 2024 22:22:50 -0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.13.1
Subject: Re: [PATCH v14 32/53] ALSA: usb-audio: Check for support for
 requested audio format
Content-Language: en-US
To: Takashi Iwai <tiwai@suse.de>
CC: <srinivas.kandagatla@linaro.org>, <mathias.nyman@intel.com>,
        <perex@perex.cz>, <conor+dt@kernel.org>, <corbet@lwn.net>,
        <lgirdwood@gmail.com>, <andersson@kernel.org>,
        <krzysztof.kozlowski+dt@linaro.org>, <gregkh@linuxfoundation.org>,
        <Thinh.Nguyen@synopsys.com>, <broonie@kernel.org>,
        <bgoswami@quicinc.com>, <tiwai@suse.com>, <robh+dt@kernel.org>,
        <konrad.dybcio@linaro.org>, <linux-kernel@vger.kernel.org>,
        <devicetree@vger.kernel.org>, <linux-sound@vger.kernel.org>,
        <linux-usb@vger.kernel.org>, <linux-arm-msm@vger.kernel.org>,
        <linux-doc@vger.kernel.org>, <alsa-devel@alsa-project.org>
References: <20240208231406.27397-1-quic_wcheng@quicinc.com>
 <20240208231406.27397-33-quic_wcheng@quicinc.com>
 <87v86x2a27.wl-tiwai@suse.de>
 <cb3b7857-dc6c-80db-4fa7-6772a856f328@quicinc.com>
 <7f0c4f85-5a63-4643-8553-e3f5d6af67ec@quicinc.com>
 <87y1bjpfn0.wl-tiwai@suse.de>
From: Wesley Cheng <quic_wcheng@quicinc.com>
In-Reply-To: <87y1bjpfn0.wl-tiwai@suse.de>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit

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@quicinc.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