On Tue, Aug 13, 2024 at 02:33:56PM -0400, Frank Li wrote: > On Sun, Aug 11, 2024 at 08:12:03PM -0700, Bjorn Andersson wrote: > > From: Bjorn Andersson <quic_bjorande@xxxxxxxxxxx> > > drivers/usb/dwc3/dwc3-qcom.c | 310 +++++++++++++++++++++++++++++++++++-------- [..] > > @@ -302,25 +306,16 @@ static void dwc3_qcom_interconnect_exit(struct dwc3_qcom *qcom) > > /* Only usable in contexts where the role can not change. */ > > static bool dwc3_qcom_is_host(struct dwc3_qcom *qcom) > > { > > - struct dwc3 *dwc; > > - > > - /* > > - * FIXME: Fix this layering violation. > > - */ > > - dwc = platform_get_drvdata(qcom->dwc3); > > - > > - /* Core driver may not have probed yet. */ > > - if (!dwc) > > - return false; > > + struct dwc3 *dwc = qcom->dwc; > > > > return dwc->xhci; > > dwc only use once. > > return qcom->dwc->xhci? > I like it, thanks for the suggestion. > > } > > [..] > > +/* Convert dev's DeviceTree representation from qcom,dwc3 to qcom,snps-dwc3 binding */ > > +static int dwc3_qcom_convert_legacy_dt(struct device *dev) > > +{ > > + struct device_node *qcom = dev->of_node; > > + struct device_node *dwc3; > > + struct property *prop; > > + int ret = 0; > > + > > + dwc3 = of_get_compatible_child(qcom, "snps,dwc3"); > > + if (!dwc3) > > + return 0; > > + > > + /* We have a child node, but no support for dynamic OF */ > > + if (!IS_ENABLED(CONFIG_OF_DYNAMIC)) > > + return -EINVAL; > > + > > + for_each_property_of_node(dwc3, prop) { > > + if (!strcmp(prop->name, "compatible")) > > + ; > > + else if (!strcmp(prop->name, "reg")) > > + ret = dwc3_qcom_legacy_update_reg(qcom, dwc3); > > + else if (!strcmp(prop->name, "interrupts")) > > + ret = dwc3_qcom_legacy_convert_interrupts(qcom, prop); > > + else > > + ret = dwc3_qcom_legacy_migrate_prop(qcom, prop); > > } > > > > -node_put: > > - of_node_put(dwc3_np); > > + if (ret < 0) > > + goto err_node_put; > > + > > + ret = dwc3_qcom_legacy_migrate_child(qcom, dwc3, "port"); > > + if (ret) > > + goto err_node_put; > > + > > + ret = dwc3_qcom_legacy_migrate_child(qcom, dwc3, "ports"); > > + if (ret) > > + goto err_node_put; > > + > > + of_detach_node(dwc3); > > + of_node_put(dwc3); > > > > + return 0; > > + > > +err_node_put: > > + of_node_put(dwc3); > > return ret; > > } > > Look like you copy children dwc3's property into current glue node. > Can you passdown dwc3's node into dwc3_probe(), let dwc3_probe to handle > these, Or move it into dwc3-core. otherwise, if imx want to do the same > thing, the the same code will be dupicated. > I tried that, as it would have saved me from having to do the dynamic rewrite. But the dwc3 core and host are full of device_property_read*(), phy_get(), platform_get_irq() etc which operates on the dwc->dev. I think it can be done, but this felt like a cleaner outcome, in particular once we transition the DeviceTree source. As you say, there should be a fair amount of room for duplication here, so perhaps we can move that to a "glue.c" and share it? [..] > > @@ -773,10 +937,14 @@ static int dwc3_qcom_probe(struct platform_device *pdev) > > goto reset_assert; > > } > > > > - res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + ret = of_address_to_resource(np, 0, &res); > > + if (ret < 0) > > + goto clk_disable; > > + res.end = res.start + SDM845_QSCRATCH_BASE_OFFSET; > > > > - qcom->qscratch_base = devm_ioremap_resource(dev, res); > > + qcom->qscratch_base = devm_ioremap(dev, res.end, SDM845_QSCRATCH_SIZE); > > if (IS_ERR(qcom->qscratch_base)) { > > + dev_err(dev, "failed to map qscratch region: %pe\n", qcom->qscratch_base); > > dev_err_probe()? > Sounds good. Thank you, Bjorn