Hi, Jun Li writes: >> -----Original Message----- >> From: Felipe Balbi >> Sent: Friday, April 30, 2021 4:24 PM >> To: Jun Li ; gregkh@linuxfoundation.org >> Cc: shawnguo@kernel.org; dl-linux-imx ; >> thunder.leizhen@huawei.com; linux-usb@vger.kernel.org >> Subject: Re: [PATCH] usb: dwc3: imx8mp: detect dwc3 core node via compatible >> string >> >> Li Jun writes: >> >> > New schema of usb controller DT-node should be named with prefix >> > "^usb(@.*)?", dt changed the node name, but missed counter part change >> > in driver, fix it by switching to use compatible string as the dwc3 >> > core compatible string keeps "snps,dwc3" in all dt. >> > >> > Fixes: d1689cd3c0f4 ("arm64: dts: imx8mp: Use the correct name for >> > child node "snps, dwc3"") >> > Signed-off-by: Li Jun >> >> >> Nice fix :-) It may break down if we have two dwc3 nodes as child of a single >> parent, but I guess that's very unlikely anyway. >> >> Acked-by: Felipe Balbi >> >> That being said, why do need to keep a pointer to the child? I had a quick >> look at the driver and it doesn't seem like the pointer is necessary at all. > > I need keep the child pointer(dwc3 core platform device) to find the dwc3 core > instance struct(struct dwc3), the wakeup setting need check the dwc3 core's > current_dr_role and do runtime resume based on the child's dev. Right, you're talking about the code below. Some suggestions inline: > static void dwc3_imx8mp_wakeup_enable(struct dwc3_imx8mp *dwc3_imx) > { > struct dwc3 *dwc3 = platform_get_drvdata(dwc3_imx->dwc3); > u32 val; > > if (!dwc3) > return; I don't think you need to care if dwc3 has probed or not here. > val = readl(dwc3_imx->glue_base + USB_WAKEUP_CTRL); > > if ((dwc3->current_dr_role == DWC3_GCTL_PRTCAP_HOST) && dwc3->xhci) > val |= USB_WAKEUP_EN | USB_WAKEUP_SS_CONN | > USB_WAKEUP_U3_EN | USB_WAKEUP_DPDM_EN; > else if (dwc3->current_dr_role == DWC3_GCTL_PRTCAP_DEVICE) > val |= USB_WAKEUP_EN | USB_WAKEUP_VBUS_EN | > USB_WAKEUP_VBUS_SRC_SESS_VAL; for this, you could register a listener to the extcon notifier and update these bits accordingly. With that, you would already *know* that dwc3 is probed. > static irqreturn_t dwc3_imx8mp_interrupt(int irq, void *_dwc3_imx) > { > struct dwc3_imx8mp *dwc3_imx = _dwc3_imx; > struct dwc3 *dwc = platform_get_drvdata(dwc3_imx->dwc3); > > if (!dwc3_imx->pm_suspended) > return IRQ_HANDLED; > > disable_irq_nosync(dwc3_imx->irq); > dwc3_imx->wakeup_pending = true; > > if ((dwc->current_dr_role == DWC3_GCTL_PRTCAP_HOST) && dwc->xhci) > pm_runtime_resume(&dwc->xhci->dev); > else if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_DEVICE) > pm_runtime_get(dwc->dev); > > return IRQ_HANDLED; > } for this, maybe you need to teach dwc3 core about wakeup irqs instead. Have a look dev_pm_set_dedicated_wake_irq(). That would clean things up, I think ;) -- balbi