Re: [PATCH v2 6/7] usb: dwc3: qcom: Transition to flattened model

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux