Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <be6ef3e4-a3d6-3af8-0a47-506e2275b40b@quicinc.com>
Date:   Mon, 7 Aug 2023 17:50:22 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.1
Subject: Re: [PATCH v4 10/32] ASoC: qcom: Add USB backend ASoC driver for Q6
Content-Language: en-US
To:     Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>,
        <agross@kernel.org>, <andersson@kernel.org>, <robh+dt@kernel.org>,
        <krzysztof.kozlowski+dt@linaro.org>, <conor+dt@kernel.org>,
        <catalin.marinas@arm.com>, <will@kernel.org>,
        <mathias.nyman@intel.com>, <gregkh@linuxfoundation.org>,
        <lgirdwood@gmail.com>, <broonie@kernel.org>, <perex@perex.cz>,
        <tiwai@suse.com>, <srinivas.kandagatla@linaro.org>,
        <bgoswami@quicinc.com>, <Thinh.Nguyen@synopsys.com>
CC:     <linux-arm-msm@vger.kernel.org>, <devicetree@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>,
        <linux-arm-kernel@lists.infradead.org>,
        <linux-usb@vger.kernel.org>, <alsa-devel@alsa-project.org>,
        <quic_jackp@quicinc.com>, <oneukum@suse.com>,
        <albertccwang@google.com>, <o-takashi@sakamocchi.jp>
References: <20230725023416.11205-1-quic_wcheng@quicinc.com>
 <20230725023416.11205-11-quic_wcheng@quicinc.com>
 <37018459-ee43-d853-1d73-3c6234a265b2@linux.intel.com>
From:   Wesley Cheng <quic_wcheng@quicinc.com>
In-Reply-To: <37018459-ee43-d853-1d73-3c6234a265b2@linux.intel.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Pierre,

On 7/25/2023 1:45 AM, Pierre-Louis Bossart wrote:
> 
>> +struct q6usb_port_data {
>> +	struct q6afe_usb_cfg usb_cfg;
>> +	struct snd_soc_usb *usb;
>> +	struct q6usb_offload priv;
>> +	int active_idx;
> 
> what is an 'active_idx' ?
> 
> 

active_idx carries the USB sound card we're going to be offloading.

>> +static int q6usb_alsa_connection_cb(struct snd_soc_usb *usb, int card_idx,
>> +			int connected)
>> +{
>> +	struct snd_soc_dapm_context *dapm;
>> +	struct q6usb_port_data *data;
>> +
>> +	dapm = snd_soc_component_get_dapm(usb->component);
>> +	data = dev_get_drvdata(usb->component->dev);
> 
> shouldn't you test that 'dapm' and 'data' are not NULL ?
> 

q6usb_component_probe() would be the one that registers to SOC USB to 
add this callback.  At that time, the component's dev and dapm 
references should be populated, so that should ensure that those are 
valid.  However, we could see that usb->component to be NULL, as that 
assignment happens after adding the port.  Instead I will add a check 
for usb->component before attempting to access the dapm/data params.

Another thing I will modify is to add a component removal callback, 
which will remove the SOC USB port.  That will ensure that no 
connection_cb() events are issued, so we don't run into any NULL pointer 
issues during the remove path.

>> +
>> +	if (connected) {
> 
> this goes back to my earlier comment that you treat 'connected' as a
> boolean.
> 

Done, changed to boolean.

>> +		snd_soc_dapm_enable_pin(dapm, "USB_RX_BE");
>> +		/* We only track the latest USB headset plugged in */
>> +		data->active_idx = card_idx;
>> +	} else {
>> +		snd_soc_dapm_disable_pin(dapm, "USB_RX_BE");
>> +	}
>> +	snd_soc_dapm_sync(dapm);
>> +
>> +	return 0;
>> +}
>> +
>> +static int q6usb_component_probe(struct snd_soc_component *component)
>> +{
>> +	struct q6usb_port_data *data = dev_get_drvdata(component->dev);
>> +	struct snd_soc_dapm_context *dapm = snd_soc_component_get_dapm(component);
>> +
>> +	snd_soc_dapm_disable_pin(dapm, "USB_RX_BE");
>> +	snd_soc_dapm_sync(dapm);
>> +
>> +	data->usb = snd_soc_usb_add_port(component->dev, &data->priv, q6usb_alsa_connection_cb);
>> +	if (IS_ERR(data->usb)) {
>> +		dev_err(component->dev, "failed to add usb port\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	data->usb->component = component;
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct snd_soc_component_driver q6usb_dai_component = {
>> +	.probe = q6usb_component_probe,
> 
> erm, if you have a .probe that adds a port, don't you need a remove that
> removes the same port, and sets the pin state as well?
> 

Will add this as mentioned above.

>> +	.name = "q6usb-dai-component",
>> +	.dapm_widgets = q6usb_dai_widgets,
>> +	.num_dapm_widgets = ARRAY_SIZE(q6usb_dai_widgets),
>> +	.dapm_routes = q6usb_dapm_routes,
>> +	.num_dapm_routes = ARRAY_SIZE(q6usb_dapm_routes),
>> +	.of_xlate_dai_name = q6usb_audio_ports_of_xlate_dai_name,
>> +};
>> +
>> +static int q6usb_dai_dev_probe(struct platform_device *pdev)
>> +{
>> +	struct device_node *node = pdev->dev.of_node;
>> +	struct q6usb_port_data *data;
>> +	struct device *dev = &pdev->dev;
>> +	struct of_phandle_args args;
>> +	int ret;
>> +
>> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>> +	if (!data)
>> +		return -ENOMEM;
>> +
>> +	ret = of_property_read_u32(node, "qcom,usb-audio-intr-num",
>> +				&data->priv.intr_num);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "failed to read intr num.\n");
>> +		return ret;
>> +	}
>> +
>> +	ret = of_parse_phandle_with_fixed_args(node, "iommus", 1, 0, &args);
>> +	if (ret < 0)
>> +		data->priv.sid = -1;
>> +	else
>> +		data->priv.sid = args.args[0] & SID_MASK;
>> +
>> +	data->priv.domain = iommu_get_domain_for_dev(&pdev->dev);
>> +
>> +	data->priv.dev = dev;
>> +	dev_set_drvdata(dev, data);
>> +
>> +	ret = devm_snd_soc_register_component(dev, &q6usb_dai_component,
>> +					q6usb_be_dais, ARRAY_SIZE(q6usb_be_dais));
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	return 0;
> 
> return devm_snd_soc_register_component
> 
>> +}
>> +
>> +static int q6usb_dai_dev_remove(struct platform_device *pdev)
>> +{
>> +	snd_soc_usb_remove_port(&pdev->dev);
> 
> that seems wrong, the port is added in the component probe, not the
> platform device probe.
> 

Will fix this.

Thanks
Wesley Cheng