[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <233c62be-c0fa-499e-9f8f-e90cf0b23aa3@oss.qualcomm.com>
Date: Thu, 7 Aug 2025 10:47:52 +0530
From: Krishna Kurapati <krishna.kurapati@....qualcomm.com>
To: Konrad Dybcio <konrad.dybcio@....qualcomm.com>,
Thinh Nguyen <Thinh.Nguyen@...opsys.com>
Cc: linux-arm-msm@...r.kernel.org, linux-usb@...r.kernel.org,
linux-kernel@...r.kernel.org,
Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>,
Bjorn Andersson <bjorn.andersson@....qualcomm.com>,
Mathias Nyman <mathias.nyman@...el.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [PATCH v3 2/3] usb: dwc3: qcom: Implement glue callbacks to
facilitate runtime suspend
On 8/6/2025 4:02 PM, Konrad Dybcio wrote:
> On 8/6/25 11:58 AM, Krishna Kurapati wrote:
[...]
>> +static void dwc3_qcom_set_role_notifier(struct dwc3 *dwc, enum usb_role next_role)
>> +{
>> + struct dwc3_qcom *qcom = to_dwc3_qcom(dwc);
>> +
>> + if (qcom->current_role == next_role)
>> + return;
>> +
>> + if (pm_runtime_resume_and_get(qcom->dev) < 0) {
>
> no need for the "< 0":
>
> """
> Return 0 if the runtime PM usage counter of @dev has been
> incremented or a negative error code otherwise.
> """
>
ACK. Will remove the "<" condition here.
>> + dev_dbg(qcom->dev, "Failed to resume device\n");
>
> This probably belongs in the suspend/resume calls themselves
>
I think today, resume fails only if "clk_bulk_prepare_enable" fails.
Would like to keep this log here for now.
>> + return;
>> + }
>> +
>> + if (qcom->current_role == USB_ROLE_DEVICE &&
>> + next_role != USB_ROLE_DEVICE)
>
> The second part is unnecessary because the first if-condition in this
> function ensures it
ACK.
>> + dwc3_qcom_vbus_override_enable(qcom, false);
>> + else if ((qcom->current_role != USB_ROLE_DEVICE) &&
>> + (next_role == USB_ROLE_DEVICE))
>
> similarly here
>
> meaning this can become
>
> dwc3_qcom_vbus_override_enable(qcom, next_role == USB_ROLE_DEVICE)
>
> (I'm not sure if it's easier to read, up to you)
>
Will keep the if-else check as is for now.
>> + dwc3_qcom_vbus_override_enable(qcom, true);
>> +
>> + pm_runtime_mark_last_busy(qcom->dev);
>> + pm_runtime_put_sync(qcom->dev);
>> +
>> + /*
>> + * Current role changes via usb_role_switch_set_role callback protected
>> + * internally by mutex lock.
>> + */
>> + qcom->current_role = next_role;
>> +}
>> +
>> +static void dwc3_qcom_run_stop_notifier(struct dwc3 *dwc, bool is_on)
>> +{
>> + struct dwc3_qcom *qcom = to_dwc3_qcom(dwc);
>> +
>> + /*
>> + * When autosuspend is enabled and controller goes to suspend
>> + * after removing UDC from userspace, the next UDC write needs
>> + * setting of QSCRATCH VBUS_VALID to "1" to generate a connect
>> + * done event.
>> + */
>> + if (!is_on)
>> + return;
>> +
>> + dwc3_qcom_vbus_override_enable(qcom, is_on);
>
> this argument logically becomes true, always
ACK. Will just pass true here in v4.
>
>> + pm_runtime_mark_last_busy(qcom->dev);
>> +}
>> +
>> +struct dwc3_glue_ops dwc3_qcom_glue_ops = {
>> + .pre_set_role = dwc3_qcom_set_role_notifier,
>> + .pre_run_stop = dwc3_qcom_run_stop_notifier,
>> +};
>> +
>> static int dwc3_qcom_probe(struct platform_device *pdev)
>> {
>> struct dwc3_probe_data probe_data = {};
>> @@ -636,6 +685,23 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
>> if (ignore_pipe_clk)
>> dwc3_qcom_select_utmi_clk(qcom);
>>
>> + qcom->mode = usb_get_dr_mode(dev);
>> +
>> + if (qcom->mode == USB_DR_MODE_HOST) {
>> + qcom->current_role = USB_ROLE_HOST;
>> + } else if (qcom->mode == USB_DR_MODE_PERIPHERAL) {
>> + qcom->current_role = USB_ROLE_DEVICE;
>> + dwc3_qcom_vbus_override_enable(qcom, true);
>> + } else {
>> + if ((device_property_read_bool(dev, "usb-role-switch")) &&
>> + (usb_get_role_switch_default_mode(dev) == USB_DR_MODE_HOST))
>
> currently this will never be true on any qcom dt ("role-switch-default-mode" is
> not present anywhere)
Agree. But I wrote for the sake of covering all cases and just in case
anyone uses this property tomorrow.
Regards,
Krishna,
Powered by blists - more mailing lists