From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-0.9 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 1FEE9C43381 for ; Mon, 11 Mar 2019 21:56:45 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id D51692147C for ; Mon, 11 Mar 2019 21:56:44 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=googlemail.com header.i=@googlemail.com header.b="nmrXrFEC" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728364AbfCKV4n (ORCPT ); Mon, 11 Mar 2019 17:56:43 -0400 Received: from mail-oi1-f194.google.com ([209.85.167.194]:36600 "EHLO mail-oi1-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727459AbfCKV4n (ORCPT ); Mon, 11 Mar 2019 17:56:43 -0400 Received: by mail-oi1-f194.google.com with SMTP id t206so419166oib.3; Mon, 11 Mar 2019 14:56:42 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=googlemail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=J0SY9H+O5ksKh1g2euAqzMNvIQqAp2NPRH3/hUfSXSI=; b=nmrXrFECfAZa+kQV7ME+dHSTTCSJJHBIKRmpDB2RUkIv6fFcoyGyNkW2LwfiuFY5mf 3k77Jpi0cf124K1FqmNQ0WDRSArqoHTEPMk1bfKar+EnVMx63uk56JXq2IcFD16XGBX3 v6Ea0yqEW/WaUc6H4a2vfi8fZeHZXDfLIQM82+sHD7cTOOM1w5H/FNhaU8Uh0fnJGXBW Jn8VTNJRleKyUjAdm05Fy4as7Div2flRXyJsQHkVfGStWfMhsBpFfdCStB+o3lH2wu9J pGv9sMAp3W9NJxfFk8D+FqCX48s62SfACSAdWN1Ik7CK2xCSl8IxoZoc0i6QaQQ+ko/B U36w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=J0SY9H+O5ksKh1g2euAqzMNvIQqAp2NPRH3/hUfSXSI=; b=fAsuvDDdAwi0WwI1kmMOJM1GgvlprVYoCYQeLa93XgF4/2osvrlGngXJSS8L8+H4s9 CVDXoH4U5bEQ2YizI/wC5aWpiDtHWTqogeQm7ScfDyehXNMFurtru9mdLdelKdRHuamc 4Ug7XcPxjZpadxVvJkfR6N5hivtBSddpWjAvpmAHpMFMvlWexSpYCeiOVa8TwvKZNThh 0rnItIjFtlorrfNN0KOwVykH76ICdVIhrKERetwkEjvq7Hl/PqvpXUN+agTL+dPccNyn G6hCCLISRMhxxXOd5ypu7ZZ8GLbpS4sib97t2mUDTjqH/ekXd0AMeHKpec6GtBZDJmX2 OAQQ== X-Gm-Message-State: APjAAAWLRLcx0gV8N/s1wwOb453C3Ln+f9HCN3ylpXy5QVY8N8V1M3wq 9XgOEr6B9YJUIqQesGE97oeACNBzVZNnNzB7TmcXND89 X-Google-Smtp-Source: APXvYqyWTN1xngTAbvSsQbgPComY8HN376Jpmgcm2ToVxD21XHUQFW7eR3EKrIUTMaG734oMsVyd7kJ/LXEd1MAqeJA= X-Received: by 2002:aca:c286:: with SMTP id s128mr263743oif.39.1552341401702; Mon, 11 Mar 2019 14:56:41 -0700 (PDT) MIME-Version: 1.0 References: <20190304103846.2060-1-narmstrong@baylibre.com> <20190304103846.2060-9-narmstrong@baylibre.com> In-Reply-To: <20190304103846.2060-9-narmstrong@baylibre.com> From: Martin Blumenstingl Date: Mon, 11 Mar 2019 22:56:30 +0100 Message-ID: Subject: Re: [PATCH v2 8/8] usb: dwc3: Add Amlogic G12A DWC3 glue To: Neil Armstrong Cc: gregkh@linuxfoundation.org, hminas@synopsys.com, balbi@kernel.org, kishon@ti.com, linux-amlogic@lists.infradead.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Neil, On Mon, Mar 4, 2019 at 11:41 AM Neil Armstrong wrote: [...] > +static void dwc3_meson_g12a_usb_init_mode(struct dwc3_meson_g12a *priv) > +{ > + if (priv->otg_phy_mode == PHY_MODE_USB_DEVICE) { > + regmap_update_bits(priv->regmap, USB_R0, > + USB_R0_U2D_ACT, USB_R0_U2D_ACT); > + regmap_update_bits(priv->regmap, USB_R0, > + USB_R0_U2D_SS_SCALEDOWN_MODE_MASK, 0); > + regmap_update_bits(priv->regmap, USB_R4, > + USB_R4_P21_SLEEP_M0, USB_R4_P21_SLEEP_M0); > + } else { > + regmap_update_bits(priv->regmap, USB_R0, > + USB_R0_U2D_ACT, 0); > + regmap_update_bits(priv->regmap, USB_R4, > + USB_R4_P21_SLEEP_M0, 0); > + } > +} I was already confused by the name of this function in v1. do you think "dwc3_meson_g12a_usb_otg_apply_mode" is a suitable name? [...] > +static int dwc3_meson_g12a_otg_mode_set(struct dwc3_meson_g12a *priv, > + enum phy_mode mode) > +{ > + int ret; > + > + if (!priv->phys[USB2_OTG_PHY]) > + return -EINVAL; > + > + if (mode == PHY_MODE_USB_HOST) > + dev_info(priv->dev, "switching to Host Mode\n"); > + else > + dev_info(priv->dev, "switching to Device Mode\n"); > + > + if (priv->vbus) { > + if (mode == PHY_MODE_USB_DEVICE) > + ret = regulator_disable(priv->vbus); > + else > + ret = regulator_enable(priv->vbus); do we need to track the regulator status (whether it's enabled or not)? the regulator framework WARN()s if it detects "unbalanced disables for " (I haven't tested this on one of my boards yet, so maybe it works because the callers of dwc3_meson_g12a_otg_mode_set() protect against this by not calling this function if the mode doesn't change) > + if (ret) > + return ret; > + } > + > + priv->otg_phy_mode = mode; > + > + dwc3_meson_g12a_usb2_set_mode(priv, USB2_OTG_PHY, mode); > + > + dwc3_meson_g12a_usb_init_mode(priv); > + > + return phy_set_mode(priv->phys[USB2_OTG_PHY], mode); > + } this is the only place where phy_set_mode is called I'm fine with keeping it but then it should be consistent at least for all USB2 PHYs. I suggest to either move the phy_set_mode call to dwc3_meson_g12a_usb2_set_mode or to remove it. [...] > +static int dwc3_meson_g12a_role_set(struct device *dev, enum usb_role role) > +{ > + struct dwc3_meson_g12a *priv = dev_get_drvdata(dev); > + enum phy_mode mode; > + > + if (role == USB_ROLE_NONE) > + return 0; > + > + mode = role == USB_ROLE_HOST ? PHY_MODE_USB_HOST : PHY_MODE_USB_DEVICE; (without surrounding parens I find the "role == USB_ROLE_HOST" part hard to distinguish from the "mode =" operation. if more people think this way then please speak up - otherwise it's probably just my personal taste) [...] > +static struct device *dwc3_meson_g12_find_child(struct device *dev, > + const char *compatible) > +{ > + struct platform_device *pdev; > + struct device_node *np; > + > + np = of_find_compatible_node(dev->of_node, NULL, compatible); > + if (!np) > + return NULL; > + > + pdev = of_find_device_by_node(np); maybe switch to of_get_compatible_child() here? This was done for the MMC driver in c483a5cc9d09f4 ("mmc: meson-mx-sdio: fix OF child-node lookup"), but I'm not sure if the problem described there also applies to dwc3_meson_g12_find_child [...] > +static int dwc3_meson_g12a_probe(struct platform_device *pdev) > +{ > + struct dwc3_meson_g12a *priv; > + struct device *dev = &pdev->dev; > + struct device_node *np = dev->of_node; > + void __iomem *base; > + struct resource *res; > + enum phy_mode otg_id; > + int ret, i; > + > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + base = devm_ioremap_resource(dev, res); > + if (IS_ERR(base)) > + return PTR_ERR(base); > + > + priv->regmap = devm_regmap_init_mmio(dev, base, > + &phy_meson_g12a_usb3_regmap_conf); > + if (IS_ERR(priv->regmap)) > + return PTR_ERR(priv->regmap); > + > + priv->vbus = devm_regulator_get_optional(dev, "vbus"); > + if (IS_ERR(priv->vbus)) { > + if (PTR_ERR(priv->vbus) == -EPROBE_DEFER) > + return PTR_ERR(priv->vbus); > + priv->vbus = NULL; > + } > + > + priv->clk = devm_clk_get(dev, NULL); > + if (IS_ERR(priv->clk)) > + return PTR_ERR(priv->clk); > + > + ret = clk_prepare_enable(priv->clk); > + if (ret) > + return ret; > + > + platform_set_drvdata(pdev, priv); > + priv->dev = dev; > + > + priv->reset = devm_reset_control_get(dev, NULL); > + if (IS_ERR(priv->reset)) { > + ret = PTR_ERR(priv->reset); > + dev_err(dev, "failed to get device reset, err=%d\n", ret); > + return ret; > + } clk_prepare_enable is called a few lines above but this (and a few more) error-paths don't call clk_disable_unprepare. Jerome suggested in the dwmac-meson8b driver that I use something like, which will even allow you to drop the clk_disable_unprepare call from dwc3_meson_g12a_remove and catch all error cases in dwc3_meson_g12a_probe at the same time: devm_add_action_or_reset(dev, (void(*)(void *))clk_disable_unprepare, priv->clk); (if you go with this then you also need to remove the clk_disable_unprepare after of_platform_populate) [...] > +static int dwc3_meson_g12a_remove(struct platform_device *pdev) > +{ > + struct dwc3_meson_g12a *priv = platform_get_drvdata(pdev); > + struct device *dev = &pdev->dev; > + int i; > + > + usb_role_switch_unregister(priv->role_switch); > + > + of_platform_depopulate(dev); > + > + for (i = 0 ; i < PHY_COUNT ; ++i) { > + phy_power_off(priv->phys[i]); > + phy_exit(priv->phys[i]); > + phy_put(priv->phys[i]); > + } > + > + clk_disable_unprepare(priv->clk); > + clk_put(priv->clk); priv->clk is obtained with devm_clk_get so the common clock framework will call clk_put for us automatically [...] > +static int __maybe_unused dwc3_meson_g12a_suspend(struct device *dev) > +{ > + struct dwc3_meson_g12a *priv = dev_get_drvdata(dev); > + int i; > + > + for (i = 0 ; i < PHY_COUNT ; ++i) > + if (priv->phys[i]) > + phy_exit(priv->phys[i]); phy_init is NULL-safe, so you can drop the NULL-check above > + > + reset_control_assert(priv->reset); > + > + return 0; > +} > + > +static int __maybe_unused dwc3_meson_g12a_resume(struct device *dev) > +{ > + struct dwc3_meson_g12a *priv = dev_get_drvdata(dev); > + int i, ret; > + > + reset_control_deassert(priv->reset); > + > + dwc3_meson_g12a_usb_init(priv); > + > + /* Init PHYs */ > + for (i = 0 ; i < PHY_COUNT ; ++i) { > + if (priv->phys[i]) { > + ret = phy_init(priv->phys[i]); phy_init is NULL-safe, so you can drop the NULL-check above Regards Martin From mboxrd@z Thu Jan 1 00:00:00 1970 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Subject: [v2,8/8] usb: dwc3: Add Amlogic G12A DWC3 glue From: Martin Blumenstingl Message-Id: Date: Mon, 11 Mar 2019 22:56:30 +0100 To: Neil Armstrong Cc: gregkh@linuxfoundation.org, hminas@synopsys.com, balbi@kernel.org, kishon@ti.com, linux-amlogic@lists.infradead.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org List-ID: SGkgTmVpbCwKCk9uIE1vbiwgTWFyIDQsIDIwMTkgYXQgMTE6NDEgQU0gTmVpbCBBcm1zdHJvbmcg PG5hcm1zdHJvbmdAYmF5bGlicmUuY29tPiB3cm90ZToKWy4uLl0KPiArc3RhdGljIHZvaWQgZHdj M19tZXNvbl9nMTJhX3VzYl9pbml0X21vZGUoc3RydWN0IGR3YzNfbWVzb25fZzEyYSAqcHJpdikK PiArewo+ICsgICAgICAgaWYgKHByaXYtPm90Z19waHlfbW9kZSA9PSBQSFlfTU9ERV9VU0JfREVW SUNFKSB7Cj4gKyAgICAgICAgICAgICAgIHJlZ21hcF91cGRhdGVfYml0cyhwcml2LT5yZWdtYXAs IFVTQl9SMCwKPiArICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIFVTQl9SMF9VMkRfQUNU LCBVU0JfUjBfVTJEX0FDVCk7Cj4gKyAgICAgICAgICAgICAgIHJlZ21hcF91cGRhdGVfYml0cyhw cml2LT5yZWdtYXAsIFVTQl9SMCwKPiArICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIFVT Ql9SMF9VMkRfU1NfU0NBTEVET1dOX01PREVfTUFTSywgMCk7Cj4gKyAgICAgICAgICAgICAgIHJl Z21hcF91cGRhdGVfYml0cyhwcml2LT5yZWdtYXAsIFVTQl9SNCwKPiArICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgIFVTQl9SNF9QMjFfU0xFRVBfTTAsIFVTQl9SNF9QMjFfU0xFRVBfTTAp Owo+ICsgICAgICAgfSBlbHNlIHsKPiArICAgICAgICAgICAgICAgcmVnbWFwX3VwZGF0ZV9iaXRz KHByaXYtPnJlZ21hcCwgVVNCX1IwLAo+ICsgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg VVNCX1IwX1UyRF9BQ1QsIDApOwo+ICsgICAgICAgICAgICAgICByZWdtYXBfdXBkYXRlX2JpdHMo cHJpdi0+cmVnbWFwLCBVU0JfUjQsCj4gKyAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICBV U0JfUjRfUDIxX1NMRUVQX00wLCAwKTsKPiArICAgICAgIH0KPiArfQpJIHdhcyBhbHJlYWR5IGNv bmZ1c2VkIGJ5IHRoZSBuYW1lIG9mIHRoaXMgZnVuY3Rpb24gaW4gdjEuCmRvIHlvdSB0aGluayAi ZHdjM19tZXNvbl9nMTJhX3VzYl9vdGdfYXBwbHlfbW9kZSIgaXMgYSBzdWl0YWJsZSBuYW1lPwoK Wy4uLl0KPiArc3RhdGljIGludCBkd2MzX21lc29uX2cxMmFfb3RnX21vZGVfc2V0KHN0cnVjdCBk d2MzX21lc29uX2cxMmEgKnByaXYsCj4gKyAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg ICAgICAgIGVudW0gcGh5X21vZGUgbW9kZSkKPiArewo+ICsgICAgICAgaW50IHJldDsKPiArCj4g KyAgICAgICBpZiAoIXByaXYtPnBoeXNbVVNCMl9PVEdfUEhZXSkKPiArICAgICAgICAgICAgICAg cmV0dXJuIC1FSU5WQUw7Cj4gKwo+ICsgICAgICAgaWYgKG1vZGUgPT0gUEhZX01PREVfVVNCX0hP U1QpCj4gKyAgICAgICAgICAgICAgIGRldl9pbmZvKHByaXYtPmRldiwgInN3aXRjaGluZyB0byBI b3N0IE1vZGVcbiIpOwo+ICsgICAgICAgZWxzZQo+ICsgICAgICAgICAgICAgICBkZXZfaW5mbyhw cml2LT5kZXYsICJzd2l0Y2hpbmcgdG8gRGV2aWNlIE1vZGVcbiIpOwo+ICsKPiArICAgICAgIGlm IChwcml2LT52YnVzKSB7Cj4gKyAgICAgICAgICAgICAgIGlmIChtb2RlID09IFBIWV9NT0RFX1VT Ql9ERVZJQ0UpCj4gKyAgICAgICAgICAgICAgICAgICAgICAgcmV0ID0gcmVndWxhdG9yX2Rpc2Fi bGUocHJpdi0+dmJ1cyk7Cj4gKyAgICAgICAgICAgICAgIGVsc2UKPiArICAgICAgICAgICAgICAg ICAgICAgICByZXQgPSByZWd1bGF0b3JfZW5hYmxlKHByaXYtPnZidXMpOwpkbyB3ZSBuZWVkIHRv IHRyYWNrIHRoZSByZWd1bGF0b3Igc3RhdHVzICh3aGV0aGVyIGl0J3MgZW5hYmxlZCBvciBub3Qp Pwp0aGUgcmVndWxhdG9yIGZyYW1ld29yayBXQVJOKClzIGlmIGl0IGRldGVjdHMgInVuYmFsYW5j ZWQgZGlzYWJsZXMgZm9yCjxyZWd1bGF0b3I+IgooSSBoYXZlbid0IHRlc3RlZCB0aGlzIG9uIG9u ZSBvZiBteSBib2FyZHMgeWV0LCBzbyBtYXliZSBpdCB3b3JrcwpiZWNhdXNlIHRoZSBjYWxsZXJz IG9mIGR3YzNfbWVzb25fZzEyYV9vdGdfbW9kZV9zZXQoKSBwcm90ZWN0IGFnYWluc3QKdGhpcyBi eSBub3QgY2FsbGluZyB0aGlzIGZ1bmN0aW9uIGlmIHRoZSBtb2RlIGRvZXNuJ3QgY2hhbmdlKQoK PiArICAgICAgICAgICAgICAgaWYgKHJldCkKPiArICAgICAgICAgICAgICAgICAgICAgICByZXR1 cm4gcmV0Owo+ICsgICAgICAgICAgICAgICB9Cj4gKwo+ICsgICAgICAgICAgICAgICBwcml2LT5v dGdfcGh5X21vZGUgPSBtb2RlOwo+ICsKPiArICAgICAgICAgICAgICAgZHdjM19tZXNvbl9nMTJh X3VzYjJfc2V0X21vZGUocHJpdiwgVVNCMl9PVEdfUEhZLCBtb2RlKTsKPiArCj4gKyAgICAgICAg ICAgICAgIGR3YzNfbWVzb25fZzEyYV91c2JfaW5pdF9tb2RlKHByaXYpOwo+ICsKPiArICAgICAg ICAgICAgICAgcmV0dXJuIHBoeV9zZXRfbW9kZShwcml2LT5waHlzW1VTQjJfT1RHX1BIWV0sIG1v ZGUpOwo+ICsgfQp0aGlzIGlzIHRoZSBvbmx5IHBsYWNlIHdoZXJlIHBoeV9zZXRfbW9kZSBpcyBj YWxsZWQKSSdtIGZpbmUgd2l0aCBrZWVwaW5nIGl0IGJ1dCB0aGVuIGl0IHNob3VsZCBiZSBjb25z aXN0ZW50IGF0IGxlYXN0IGZvcgphbGwgVVNCMiBQSFlzLgpJIHN1Z2dlc3QgdG8gZWl0aGVyIG1v dmUgdGhlIHBoeV9zZXRfbW9kZSBjYWxsIHRvCmR3YzNfbWVzb25fZzEyYV91c2IyX3NldF9tb2Rl IG9yIHRvIHJlbW92ZSBpdC4KClsuLi5dCj4gK3N0YXRpYyBpbnQgZHdjM19tZXNvbl9nMTJhX3Jv bGVfc2V0KHN0cnVjdCBkZXZpY2UgKmRldiwgZW51bSB1c2Jfcm9sZSByb2xlKQo+ICt7Cj4gKyAg ICAgICBzdHJ1Y3QgZHdjM19tZXNvbl9nMTJhICpwcml2ID0gZGV2X2dldF9kcnZkYXRhKGRldik7 Cj4gKyAgICAgICBlbnVtIHBoeV9tb2RlIG1vZGU7Cj4gKwo+ICsgICAgICAgaWYgKHJvbGUgPT0g VVNCX1JPTEVfTk9ORSkKPiArICAgICAgICAgICAgICAgcmV0dXJuIDA7Cj4gKwo+ICsgICAgICAg bW9kZSA9IHJvbGUgPT0gVVNCX1JPTEVfSE9TVCA/IFBIWV9NT0RFX1VTQl9IT1NUIDogUEhZX01P REVfVVNCX0RFVklDRTsKKHdpdGhvdXQgc3Vycm91bmRpbmcgcGFyZW5zIEkgZmluZCB0aGUgInJv bGUgPT0gVVNCX1JPTEVfSE9TVCIgcGFydApoYXJkIHRvIGRpc3Rpbmd1aXNoIGZyb20gdGhlICJt b2RlID0iIG9wZXJhdGlvbi4gaWYgbW9yZSBwZW9wbGUgdGhpbmsKdGhpcyB3YXkgdGhlbiBwbGVh c2Ugc3BlYWsgdXAgLSBvdGhlcndpc2UgaXQncyBwcm9iYWJseSBqdXN0IG15CnBlcnNvbmFsIHRh c3RlKQoKWy4uLl0KPiArc3RhdGljIHN0cnVjdCBkZXZpY2UgKmR3YzNfbWVzb25fZzEyX2ZpbmRf Y2hpbGQoc3RydWN0IGRldmljZSAqZGV2LAo+ICsgICAgICAgICAgICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgICAgIGNvbnN0IGNoYXIgKmNvbXBhdGlibGUpCj4gK3sKPiArICAgICAg IHN0cnVjdCBwbGF0Zm9ybV9kZXZpY2UgKnBkZXY7Cj4gKyAgICAgICBzdHJ1Y3QgZGV2aWNlX25v ZGUgKm5wOwo+ICsKPiArICAgICAgIG5wID0gb2ZfZmluZF9jb21wYXRpYmxlX25vZGUoZGV2LT5v Zl9ub2RlLCBOVUxMLCBjb21wYXRpYmxlKTsKPiArICAgICAgIGlmICghbnApCj4gKyAgICAgICAg ICAgICAgIHJldHVybiBOVUxMOwo+ICsKPiArICAgICAgIHBkZXYgPSBvZl9maW5kX2RldmljZV9i eV9ub2RlKG5wKTsKbWF5YmUgc3dpdGNoIHRvIG9mX2dldF9jb21wYXRpYmxlX2NoaWxkKCkgaGVy ZT8gVGhpcyB3YXMgZG9uZSBmb3IgdGhlCk1NQyBkcml2ZXIgaW4gYzQ4M2E1Y2M5ZDA5ZjQgKCJt bWM6IG1lc29uLW14LXNkaW86IGZpeCBPRiBjaGlsZC1ub2RlCmxvb2t1cCIpLCBidXQgSSdtIG5v dCBzdXJlIGlmIHRoZSBwcm9ibGVtIGRlc2NyaWJlZCB0aGVyZSBhbHNvIGFwcGxpZXMKdG8gZHdj M19tZXNvbl9nMTJfZmluZF9jaGlsZAoKWy4uLl0KPiArc3RhdGljIGludCBkd2MzX21lc29uX2cx MmFfcHJvYmUoc3RydWN0IHBsYXRmb3JtX2RldmljZSAqcGRldikKPiArewo+ICsgICAgICAgc3Ry dWN0IGR3YzNfbWVzb25fZzEyYSAgKnByaXY7Cj4gKyAgICAgICBzdHJ1Y3QgZGV2aWNlICAgICAg ICAgICAqZGV2ID0gJnBkZXYtPmRldjsKPiArICAgICAgIHN0cnVjdCBkZXZpY2Vfbm9kZSAgICAg ICpucCA9IGRldi0+b2Zfbm9kZTsKPiArICAgICAgIHZvaWQgX19pb21lbSAqYmFzZTsKPiArICAg ICAgIHN0cnVjdCByZXNvdXJjZSAqcmVzOwo+ICsgICAgICAgZW51bSBwaHlfbW9kZSBvdGdfaWQ7 Cj4gKyAgICAgICBpbnQgcmV0LCBpOwo+ICsKPiArICAgICAgIHByaXYgPSBkZXZtX2t6YWxsb2Mo ZGV2LCBzaXplb2YoKnByaXYpLCBHRlBfS0VSTkVMKTsKPiArICAgICAgIGlmICghcHJpdikKPiAr ICAgICAgICAgICAgICAgcmV0dXJuIC1FTk9NRU07Cj4gKwo+ICsgICAgICAgcmVzID0gcGxhdGZv cm1fZ2V0X3Jlc291cmNlKHBkZXYsIElPUkVTT1VSQ0VfTUVNLCAwKTsKPiArICAgICAgIGJhc2Ug PSBkZXZtX2lvcmVtYXBfcmVzb3VyY2UoZGV2LCByZXMpOwo+ICsgICAgICAgaWYgKElTX0VSUihi YXNlKSkKPiArICAgICAgICAgICAgICAgcmV0dXJuIFBUUl9FUlIoYmFzZSk7Cj4gKwo+ICsgICAg ICAgcHJpdi0+cmVnbWFwID0gZGV2bV9yZWdtYXBfaW5pdF9tbWlvKGRldiwgYmFzZSwKPiArICAg ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAmcGh5X21lc29uX2cxMmFf dXNiM19yZWdtYXBfY29uZik7Cj4gKyAgICAgICBpZiAoSVNfRVJSKHByaXYtPnJlZ21hcCkpCj4g KyAgICAgICAgICAgICAgIHJldHVybiBQVFJfRVJSKHByaXYtPnJlZ21hcCk7Cj4gKwo+ICsgICAg ICAgcHJpdi0+dmJ1cyA9IGRldm1fcmVndWxhdG9yX2dldF9vcHRpb25hbChkZXYsICJ2YnVzIik7 Cj4gKyAgICAgICBpZiAoSVNfRVJSKHByaXYtPnZidXMpKSB7Cj4gKyAgICAgICAgICAgICAgIGlm IChQVFJfRVJSKHByaXYtPnZidXMpID09IC1FUFJPQkVfREVGRVIpCj4gKyAgICAgICAgICAgICAg ICAgICAgICAgcmV0dXJuIFBUUl9FUlIocHJpdi0+dmJ1cyk7Cj4gKyAgICAgICAgICAgICAgIHBy aXYtPnZidXMgPSBOVUxMOwo+ICsgICAgICAgfQo+ICsKPiArICAgICAgIHByaXYtPmNsayA9IGRl dm1fY2xrX2dldChkZXYsIE5VTEwpOwo+ICsgICAgICAgaWYgKElTX0VSUihwcml2LT5jbGspKQo+ ICsgICAgICAgICAgICAgICByZXR1cm4gUFRSX0VSUihwcml2LT5jbGspOwo+ICsKPiArICAgICAg IHJldCA9IGNsa19wcmVwYXJlX2VuYWJsZShwcml2LT5jbGspOwo+ICsgICAgICAgaWYgKHJldCkK PiArICAgICAgICAgICAgICAgcmV0dXJuIHJldDsKPiArCj4gKyAgICAgICBwbGF0Zm9ybV9zZXRf ZHJ2ZGF0YShwZGV2LCBwcml2KTsKPiArICAgICAgIHByaXYtPmRldiA9IGRldjsKPiArCj4gKyAg ICAgICBwcml2LT5yZXNldCA9IGRldm1fcmVzZXRfY29udHJvbF9nZXQoZGV2LCBOVUxMKTsKPiAr ICAgICAgIGlmIChJU19FUlIocHJpdi0+cmVzZXQpKSB7Cj4gKyAgICAgICAgICAgICAgIHJldCA9 IFBUUl9FUlIocHJpdi0+cmVzZXQpOwo+ICsgICAgICAgICAgICAgICBkZXZfZXJyKGRldiwgImZh aWxlZCB0byBnZXQgZGV2aWNlIHJlc2V0LCBlcnI9JWRcbiIsIHJldCk7Cj4gKyAgICAgICAgICAg ICAgIHJldHVybiByZXQ7Cj4gKyAgICAgICB9CmNsa19wcmVwYXJlX2VuYWJsZSBpcyBjYWxsZWQg YSBmZXcgbGluZXMgYWJvdmUgYnV0IHRoaXMgKGFuZCBhIGZldwptb3JlKSBlcnJvci1wYXRocyBk b24ndCBjYWxsIGNsa19kaXNhYmxlX3VucHJlcGFyZS4KSmVyb21lIHN1Z2dlc3RlZCBpbiB0aGUg ZHdtYWMtbWVzb244YiBkcml2ZXIgdGhhdCBJIHVzZSBzb21ldGhpbmcKbGlrZSwgd2hpY2ggd2ls bCBldmVuIGFsbG93IHlvdSB0byBkcm9wIHRoZSBjbGtfZGlzYWJsZV91bnByZXBhcmUgY2FsbApm cm9tIGR3YzNfbWVzb25fZzEyYV9yZW1vdmUgYW5kIGNhdGNoIGFsbCBlcnJvciBjYXNlcyBpbgpk d2MzX21lc29uX2cxMmFfcHJvYmUgYXQgdGhlIHNhbWUgdGltZToKICBkZXZtX2FkZF9hY3Rpb25f b3JfcmVzZXQoZGV2LCAodm9pZCgqKSh2b2lkCiopKWNsa19kaXNhYmxlX3VucHJlcGFyZSwgcHJp di0+Y2xrKTsKKGlmIHlvdSBnbyB3aXRoIHRoaXMgdGhlbiB5b3UgYWxzbyBuZWVkIHRvIHJlbW92 ZSB0aGUKY2xrX2Rpc2FibGVfdW5wcmVwYXJlIGFmdGVyIG9mX3BsYXRmb3JtX3BvcHVsYXRlKQoK Wy4uLl0KPiArc3RhdGljIGludCBkd2MzX21lc29uX2cxMmFfcmVtb3ZlKHN0cnVjdCBwbGF0Zm9y bV9kZXZpY2UgKnBkZXYpCj4gK3sKPiArICAgICAgIHN0cnVjdCBkd2MzX21lc29uX2cxMmEgKnBy aXYgPSBwbGF0Zm9ybV9nZXRfZHJ2ZGF0YShwZGV2KTsKPiArICAgICAgIHN0cnVjdCBkZXZpY2Ug KmRldiA9ICZwZGV2LT5kZXY7Cj4gKyAgICAgICBpbnQgaTsKPiArCj4gKyAgICAgICB1c2Jfcm9s ZV9zd2l0Y2hfdW5yZWdpc3Rlcihwcml2LT5yb2xlX3N3aXRjaCk7Cj4gKwo+ICsgICAgICAgb2Zf cGxhdGZvcm1fZGVwb3B1bGF0ZShkZXYpOwo+ICsKPiArICAgICAgIGZvciAoaSA9IDAgOyBpIDwg UEhZX0NPVU5UIDsgKytpKSB7Cj4gKyAgICAgICAgICAgICAgIHBoeV9wb3dlcl9vZmYocHJpdi0+ cGh5c1tpXSk7Cj4gKyAgICAgICAgICAgICAgIHBoeV9leGl0KHByaXYtPnBoeXNbaV0pOwo+ICsg ICAgICAgICAgICAgICBwaHlfcHV0KHByaXYtPnBoeXNbaV0pOwo+ICsgICAgICAgfQo+ICsKPiAr ICAgICAgIGNsa19kaXNhYmxlX3VucHJlcGFyZShwcml2LT5jbGspOwo+ICsgICAgICAgY2xrX3B1 dChwcml2LT5jbGspOwpwcml2LT5jbGsgaXMgb2J0YWluZWQgd2l0aCBkZXZtX2Nsa19nZXQgc28g dGhlIGNvbW1vbiBjbG9jayBmcmFtZXdvcmsKd2lsbCBjYWxsIGNsa19wdXQgZm9yIHVzIGF1dG9t YXRpY2FsbHkKClsuLi5dCj4gK3N0YXRpYyBpbnQgX19tYXliZV91bnVzZWQgZHdjM19tZXNvbl9n MTJhX3N1c3BlbmQoc3RydWN0IGRldmljZSAqZGV2KQo+ICt7Cj4gKyAgICAgICBzdHJ1Y3QgZHdj M19tZXNvbl9nMTJhICpwcml2ID0gZGV2X2dldF9kcnZkYXRhKGRldik7Cj4gKyAgICAgICBpbnQg aTsKPiArCj4gKyAgICAgICBmb3IgKGkgPSAwIDsgaSA8IFBIWV9DT1VOVCA7ICsraSkKPiArICAg ICAgICAgICAgICAgaWYgKHByaXYtPnBoeXNbaV0pCj4gKyAgICAgICAgICAgICAgICAgICAgICAg cGh5X2V4aXQocHJpdi0+cGh5c1tpXSk7CnBoeV9pbml0IGlzIE5VTEwtc2FmZSwgc28geW91IGNh biBkcm9wIHRoZSBOVUxMLWNoZWNrIGFib3ZlCgo+ICsKPiArICAgICAgIHJlc2V0X2NvbnRyb2xf YXNzZXJ0KHByaXYtPnJlc2V0KTsKPiArCj4gKyAgICAgICByZXR1cm4gMDsKPiArfQo+ICsKPiAr c3RhdGljIGludCBfX21heWJlX3VudXNlZCBkd2MzX21lc29uX2cxMmFfcmVzdW1lKHN0cnVjdCBk ZXZpY2UgKmRldikKPiArewo+ICsgICAgICAgc3RydWN0IGR3YzNfbWVzb25fZzEyYSAqcHJpdiA9 IGRldl9nZXRfZHJ2ZGF0YShkZXYpOwo+ICsgICAgICAgaW50IGksIHJldDsKPiArCj4gKyAgICAg ICByZXNldF9jb250cm9sX2RlYXNzZXJ0KHByaXYtPnJlc2V0KTsKPiArCj4gKyAgICAgICBkd2Mz X21lc29uX2cxMmFfdXNiX2luaXQocHJpdik7Cj4gKwo+ICsgICAgICAgLyogSW5pdCBQSFlzICov Cj4gKyAgICAgICBmb3IgKGkgPSAwIDsgaSA8IFBIWV9DT1VOVCA7ICsraSkgewo+ICsgICAgICAg ICAgICAgICBpZiAocHJpdi0+cGh5c1tpXSkgewo+ICsgICAgICAgICAgICAgICAgICAgICAgIHJl dCA9IHBoeV9pbml0KHByaXYtPnBoeXNbaV0pOwpwaHlfaW5pdCBpcyBOVUxMLXNhZmUsIHNvIHlv dSBjYW4gZHJvcCB0aGUgTlVMTC1jaGVjayBhYm92ZQoKClJlZ2FyZHMKTWFydGluCg== From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-0.8 required=3.0 tests=DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED,DKIM_VALID,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 0C4C4C43381 for ; Mon, 11 Mar 2019 21:57:00 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id CB8AA2147C for ; Mon, 11 Mar 2019 21:56:59 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="N2rybTvF"; dkim=fail reason="signature verification failed" (2048-bit key) header.d=googlemail.com header.i=@googlemail.com header.b="nmrXrFEC" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org CB8AA2147C Authentication-Results: mail.kernel.org; dmarc=fail (p=quarantine dis=none) header.from=googlemail.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:To:Subject:Message-ID:Date:From: In-Reply-To:References:MIME-Version:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=m2XT/d0/fXWhCXZFvck7amNnLz+z2t6D03BkWQYTNwY=; b=N2rybTvFP/N/TQ dsL6hffFtG+CTg0OqfkWG+qVm0xj+cWmalFczniyL7foL8fV6NEz8oGb40oYoWXGS0ubjBMj6CLwX vlPwrOwq7xODdBz3VXig/ukVD9jvBmSQ8EoUcQ+Dt2reGdrOx+hKjkh9yF7mPK+I3WETgvss5PNB1 6AWHBQ+BnFve9ZXG060F056pF7SxW1eFUUE9MGTlT+jkUpASbDh9P4nJLAWdIa6skYZBnH9vmA2cU RE44JKjp3JAM0BSjWkaCqti3xVybXVjQVhB2M3TxHlyGB1VRSdiJU+2kLf9PmMUTTPNWpLkDkERaj LmPborqY1/ZDoW892x1w==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.90_1 #2 (Red Hat Linux)) id 1h3Sud-0007QJ-Ki; Mon, 11 Mar 2019 21:56:47 +0000 Received: from mail-oi1-x242.google.com ([2607:f8b0:4864:20::242]) by bombadil.infradead.org with esmtps (Exim 4.90_1 #2 (Red Hat Linux)) id 1h3SuZ-0007PS-5Y; Mon, 11 Mar 2019 21:56:44 +0000 Received: by mail-oi1-x242.google.com with SMTP id u128so421923oie.2; Mon, 11 Mar 2019 14:56:42 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=googlemail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=J0SY9H+O5ksKh1g2euAqzMNvIQqAp2NPRH3/hUfSXSI=; b=nmrXrFECfAZa+kQV7ME+dHSTTCSJJHBIKRmpDB2RUkIv6fFcoyGyNkW2LwfiuFY5mf 3k77Jpi0cf124K1FqmNQ0WDRSArqoHTEPMk1bfKar+EnVMx63uk56JXq2IcFD16XGBX3 v6Ea0yqEW/WaUc6H4a2vfi8fZeHZXDfLIQM82+sHD7cTOOM1w5H/FNhaU8Uh0fnJGXBW Jn8VTNJRleKyUjAdm05Fy4as7Div2flRXyJsQHkVfGStWfMhsBpFfdCStB+o3lH2wu9J pGv9sMAp3W9NJxfFk8D+FqCX48s62SfACSAdWN1Ik7CK2xCSl8IxoZoc0i6QaQQ+ko/B U36w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=J0SY9H+O5ksKh1g2euAqzMNvIQqAp2NPRH3/hUfSXSI=; b=GsQSwJS67PZxS44xRzQFEc89cXzjaElw9oMoH+J2k0YUzF9Iph4lx8mIOtiifTjZbF ffAoMhsk+WSDii9abQQsRYNayoBmfq21Zte8UgBd39/JMJ1d6y8wKs/QlMW2dlHS8Qv5 zf8d+gm1+IBKK0vfgxoMp/gElOfe/fe8BTTLhzNB68Tkf9crDEcHTy8WDpoyjyFX259F ZCzowaQspokFxNBjEd92qUqTCiuBIVGZqrOYIxiS2/paynRH8Z5QP1aAjatk52YWEJ8I C4zU3VDwEPrH2V68Qe2/3m3h4Th3K95ZwhEZ9nZA7bNeznS5uj/h0SErsP2YoA8xYtnc eUuw== X-Gm-Message-State: APjAAAUtEVlizAeOYJjAe/x5ZsLHpO2MoXlIq0dMQ7UNAjMfGn/QkMeA a/Cu78VBE1x8vzEYgNM55F0KMauV0S/aCKJHyb0= X-Google-Smtp-Source: APXvYqyWTN1xngTAbvSsQbgPComY8HN376Jpmgcm2ToVxD21XHUQFW7eR3EKrIUTMaG734oMsVyd7kJ/LXEd1MAqeJA= X-Received: by 2002:aca:c286:: with SMTP id s128mr263743oif.39.1552341401702; Mon, 11 Mar 2019 14:56:41 -0700 (PDT) MIME-Version: 1.0 References: <20190304103846.2060-1-narmstrong@baylibre.com> <20190304103846.2060-9-narmstrong@baylibre.com> In-Reply-To: <20190304103846.2060-9-narmstrong@baylibre.com> From: Martin Blumenstingl Date: Mon, 11 Mar 2019 22:56:30 +0100 Message-ID: Subject: Re: [PATCH v2 8/8] usb: dwc3: Add Amlogic G12A DWC3 glue To: Neil Armstrong X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20190311_145643_235906_E0D47C1A X-CRM114-Status: GOOD ( 18.72 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: balbi@kernel.org, gregkh@linuxfoundation.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, kishon@ti.com, hminas@synopsys.com, linux-amlogic@lists.infradead.org, linux-arm-kernel@lists.infradead.org Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Neil, On Mon, Mar 4, 2019 at 11:41 AM Neil Armstrong wrote: [...] > +static void dwc3_meson_g12a_usb_init_mode(struct dwc3_meson_g12a *priv) > +{ > + if (priv->otg_phy_mode == PHY_MODE_USB_DEVICE) { > + regmap_update_bits(priv->regmap, USB_R0, > + USB_R0_U2D_ACT, USB_R0_U2D_ACT); > + regmap_update_bits(priv->regmap, USB_R0, > + USB_R0_U2D_SS_SCALEDOWN_MODE_MASK, 0); > + regmap_update_bits(priv->regmap, USB_R4, > + USB_R4_P21_SLEEP_M0, USB_R4_P21_SLEEP_M0); > + } else { > + regmap_update_bits(priv->regmap, USB_R0, > + USB_R0_U2D_ACT, 0); > + regmap_update_bits(priv->regmap, USB_R4, > + USB_R4_P21_SLEEP_M0, 0); > + } > +} I was already confused by the name of this function in v1. do you think "dwc3_meson_g12a_usb_otg_apply_mode" is a suitable name? [...] > +static int dwc3_meson_g12a_otg_mode_set(struct dwc3_meson_g12a *priv, > + enum phy_mode mode) > +{ > + int ret; > + > + if (!priv->phys[USB2_OTG_PHY]) > + return -EINVAL; > + > + if (mode == PHY_MODE_USB_HOST) > + dev_info(priv->dev, "switching to Host Mode\n"); > + else > + dev_info(priv->dev, "switching to Device Mode\n"); > + > + if (priv->vbus) { > + if (mode == PHY_MODE_USB_DEVICE) > + ret = regulator_disable(priv->vbus); > + else > + ret = regulator_enable(priv->vbus); do we need to track the regulator status (whether it's enabled or not)? the regulator framework WARN()s if it detects "unbalanced disables for " (I haven't tested this on one of my boards yet, so maybe it works because the callers of dwc3_meson_g12a_otg_mode_set() protect against this by not calling this function if the mode doesn't change) > + if (ret) > + return ret; > + } > + > + priv->otg_phy_mode = mode; > + > + dwc3_meson_g12a_usb2_set_mode(priv, USB2_OTG_PHY, mode); > + > + dwc3_meson_g12a_usb_init_mode(priv); > + > + return phy_set_mode(priv->phys[USB2_OTG_PHY], mode); > + } this is the only place where phy_set_mode is called I'm fine with keeping it but then it should be consistent at least for all USB2 PHYs. I suggest to either move the phy_set_mode call to dwc3_meson_g12a_usb2_set_mode or to remove it. [...] > +static int dwc3_meson_g12a_role_set(struct device *dev, enum usb_role role) > +{ > + struct dwc3_meson_g12a *priv = dev_get_drvdata(dev); > + enum phy_mode mode; > + > + if (role == USB_ROLE_NONE) > + return 0; > + > + mode = role == USB_ROLE_HOST ? PHY_MODE_USB_HOST : PHY_MODE_USB_DEVICE; (without surrounding parens I find the "role == USB_ROLE_HOST" part hard to distinguish from the "mode =" operation. if more people think this way then please speak up - otherwise it's probably just my personal taste) [...] > +static struct device *dwc3_meson_g12_find_child(struct device *dev, > + const char *compatible) > +{ > + struct platform_device *pdev; > + struct device_node *np; > + > + np = of_find_compatible_node(dev->of_node, NULL, compatible); > + if (!np) > + return NULL; > + > + pdev = of_find_device_by_node(np); maybe switch to of_get_compatible_child() here? This was done for the MMC driver in c483a5cc9d09f4 ("mmc: meson-mx-sdio: fix OF child-node lookup"), but I'm not sure if the problem described there also applies to dwc3_meson_g12_find_child [...] > +static int dwc3_meson_g12a_probe(struct platform_device *pdev) > +{ > + struct dwc3_meson_g12a *priv; > + struct device *dev = &pdev->dev; > + struct device_node *np = dev->of_node; > + void __iomem *base; > + struct resource *res; > + enum phy_mode otg_id; > + int ret, i; > + > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + base = devm_ioremap_resource(dev, res); > + if (IS_ERR(base)) > + return PTR_ERR(base); > + > + priv->regmap = devm_regmap_init_mmio(dev, base, > + &phy_meson_g12a_usb3_regmap_conf); > + if (IS_ERR(priv->regmap)) > + return PTR_ERR(priv->regmap); > + > + priv->vbus = devm_regulator_get_optional(dev, "vbus"); > + if (IS_ERR(priv->vbus)) { > + if (PTR_ERR(priv->vbus) == -EPROBE_DEFER) > + return PTR_ERR(priv->vbus); > + priv->vbus = NULL; > + } > + > + priv->clk = devm_clk_get(dev, NULL); > + if (IS_ERR(priv->clk)) > + return PTR_ERR(priv->clk); > + > + ret = clk_prepare_enable(priv->clk); > + if (ret) > + return ret; > + > + platform_set_drvdata(pdev, priv); > + priv->dev = dev; > + > + priv->reset = devm_reset_control_get(dev, NULL); > + if (IS_ERR(priv->reset)) { > + ret = PTR_ERR(priv->reset); > + dev_err(dev, "failed to get device reset, err=%d\n", ret); > + return ret; > + } clk_prepare_enable is called a few lines above but this (and a few more) error-paths don't call clk_disable_unprepare. Jerome suggested in the dwmac-meson8b driver that I use something like, which will even allow you to drop the clk_disable_unprepare call from dwc3_meson_g12a_remove and catch all error cases in dwc3_meson_g12a_probe at the same time: devm_add_action_or_reset(dev, (void(*)(void *))clk_disable_unprepare, priv->clk); (if you go with this then you also need to remove the clk_disable_unprepare after of_platform_populate) [...] > +static int dwc3_meson_g12a_remove(struct platform_device *pdev) > +{ > + struct dwc3_meson_g12a *priv = platform_get_drvdata(pdev); > + struct device *dev = &pdev->dev; > + int i; > + > + usb_role_switch_unregister(priv->role_switch); > + > + of_platform_depopulate(dev); > + > + for (i = 0 ; i < PHY_COUNT ; ++i) { > + phy_power_off(priv->phys[i]); > + phy_exit(priv->phys[i]); > + phy_put(priv->phys[i]); > + } > + > + clk_disable_unprepare(priv->clk); > + clk_put(priv->clk); priv->clk is obtained with devm_clk_get so the common clock framework will call clk_put for us automatically [...] > +static int __maybe_unused dwc3_meson_g12a_suspend(struct device *dev) > +{ > + struct dwc3_meson_g12a *priv = dev_get_drvdata(dev); > + int i; > + > + for (i = 0 ; i < PHY_COUNT ; ++i) > + if (priv->phys[i]) > + phy_exit(priv->phys[i]); phy_init is NULL-safe, so you can drop the NULL-check above > + > + reset_control_assert(priv->reset); > + > + return 0; > +} > + > +static int __maybe_unused dwc3_meson_g12a_resume(struct device *dev) > +{ > + struct dwc3_meson_g12a *priv = dev_get_drvdata(dev); > + int i, ret; > + > + reset_control_deassert(priv->reset); > + > + dwc3_meson_g12a_usb_init(priv); > + > + /* Init PHYs */ > + for (i = 0 ; i < PHY_COUNT ; ++i) { > + if (priv->phys[i]) { > + ret = phy_init(priv->phys[i]); phy_init is NULL-safe, so you can drop the NULL-check above Regards Martin _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-0.8 required=3.0 tests=DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED,DKIM_VALID,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 4448FC43381 for ; Mon, 11 Mar 2019 21:56:53 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 0F7D32147C for ; Mon, 11 Mar 2019 21:56:53 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="ATUJYUY6"; dkim=fail reason="signature verification failed" (2048-bit key) header.d=googlemail.com header.i=@googlemail.com header.b="nmrXrFEC" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 0F7D32147C Authentication-Results: mail.kernel.org; dmarc=fail (p=quarantine dis=none) header.from=googlemail.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-amlogic-bounces+linux-amlogic=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:To:Subject:Message-ID:Date:From: In-Reply-To:References:MIME-Version:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=7Ox0+1n9ouhlu6uW7g2NrLLOqQXZuL//rZ3aRBHWzhg=; b=ATUJYUY6JF4F5K NnVSNaht4x3q2CQo+L89R2oKNCxJKTwxtXOuDcoP67S/OiQaDAxmg+d1HXqVKP3MvLcO1NK1uqWdj LRflvMOw+vhh74ZBglKNZTLFikf8X1iUeFaqH0lXnMr+BXIBySYG63SNqkt9ZKbItCZqdm02KVatk UBmPRYcDCJQVn2CU/DLZNgqJI6OpnyBuuzrROhDSpDlDbq72kFH9d8WdWQLYFENVnoJlzNmF264T5 L5wJ/i4Pk483dsmu03x1JT9xJ6ZgoJBq9vnSw337r80f2V+bmldpqHhaz/I0h3ooWJlRK1WSpPVrV GPZdFj/PDlG/Hri0+13Q==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.90_1 #2 (Red Hat Linux)) id 1h3Suc-0007Pq-Dx; Mon, 11 Mar 2019 21:56:46 +0000 Received: from mail-oi1-x242.google.com ([2607:f8b0:4864:20::242]) by bombadil.infradead.org with esmtps (Exim 4.90_1 #2 (Red Hat Linux)) id 1h3SuZ-0007PS-5Y; Mon, 11 Mar 2019 21:56:44 +0000 Received: by mail-oi1-x242.google.com with SMTP id u128so421923oie.2; Mon, 11 Mar 2019 14:56:42 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=googlemail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=J0SY9H+O5ksKh1g2euAqzMNvIQqAp2NPRH3/hUfSXSI=; b=nmrXrFECfAZa+kQV7ME+dHSTTCSJJHBIKRmpDB2RUkIv6fFcoyGyNkW2LwfiuFY5mf 3k77Jpi0cf124K1FqmNQ0WDRSArqoHTEPMk1bfKar+EnVMx63uk56JXq2IcFD16XGBX3 v6Ea0yqEW/WaUc6H4a2vfi8fZeHZXDfLIQM82+sHD7cTOOM1w5H/FNhaU8Uh0fnJGXBW Jn8VTNJRleKyUjAdm05Fy4as7Div2flRXyJsQHkVfGStWfMhsBpFfdCStB+o3lH2wu9J pGv9sMAp3W9NJxfFk8D+FqCX48s62SfACSAdWN1Ik7CK2xCSl8IxoZoc0i6QaQQ+ko/B U36w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=J0SY9H+O5ksKh1g2euAqzMNvIQqAp2NPRH3/hUfSXSI=; b=GsQSwJS67PZxS44xRzQFEc89cXzjaElw9oMoH+J2k0YUzF9Iph4lx8mIOtiifTjZbF ffAoMhsk+WSDii9abQQsRYNayoBmfq21Zte8UgBd39/JMJ1d6y8wKs/QlMW2dlHS8Qv5 zf8d+gm1+IBKK0vfgxoMp/gElOfe/fe8BTTLhzNB68Tkf9crDEcHTy8WDpoyjyFX259F ZCzowaQspokFxNBjEd92qUqTCiuBIVGZqrOYIxiS2/paynRH8Z5QP1aAjatk52YWEJ8I C4zU3VDwEPrH2V68Qe2/3m3h4Th3K95ZwhEZ9nZA7bNeznS5uj/h0SErsP2YoA8xYtnc eUuw== X-Gm-Message-State: APjAAAUtEVlizAeOYJjAe/x5ZsLHpO2MoXlIq0dMQ7UNAjMfGn/QkMeA a/Cu78VBE1x8vzEYgNM55F0KMauV0S/aCKJHyb0= X-Google-Smtp-Source: APXvYqyWTN1xngTAbvSsQbgPComY8HN376Jpmgcm2ToVxD21XHUQFW7eR3EKrIUTMaG734oMsVyd7kJ/LXEd1MAqeJA= X-Received: by 2002:aca:c286:: with SMTP id s128mr263743oif.39.1552341401702; Mon, 11 Mar 2019 14:56:41 -0700 (PDT) MIME-Version: 1.0 References: <20190304103846.2060-1-narmstrong@baylibre.com> <20190304103846.2060-9-narmstrong@baylibre.com> In-Reply-To: <20190304103846.2060-9-narmstrong@baylibre.com> From: Martin Blumenstingl Date: Mon, 11 Mar 2019 22:56:30 +0100 Message-ID: Subject: Re: [PATCH v2 8/8] usb: dwc3: Add Amlogic G12A DWC3 glue To: Neil Armstrong X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20190311_145643_235906_E0D47C1A X-CRM114-Status: GOOD ( 18.72 ) X-BeenThere: linux-amlogic@lists.infradead.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: balbi@kernel.org, gregkh@linuxfoundation.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, kishon@ti.com, hminas@synopsys.com, linux-amlogic@lists.infradead.org, linux-arm-kernel@lists.infradead.org Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-amlogic" Errors-To: linux-amlogic-bounces+linux-amlogic=archiver.kernel.org@lists.infradead.org Hi Neil, On Mon, Mar 4, 2019 at 11:41 AM Neil Armstrong wrote: [...] > +static void dwc3_meson_g12a_usb_init_mode(struct dwc3_meson_g12a *priv) > +{ > + if (priv->otg_phy_mode == PHY_MODE_USB_DEVICE) { > + regmap_update_bits(priv->regmap, USB_R0, > + USB_R0_U2D_ACT, USB_R0_U2D_ACT); > + regmap_update_bits(priv->regmap, USB_R0, > + USB_R0_U2D_SS_SCALEDOWN_MODE_MASK, 0); > + regmap_update_bits(priv->regmap, USB_R4, > + USB_R4_P21_SLEEP_M0, USB_R4_P21_SLEEP_M0); > + } else { > + regmap_update_bits(priv->regmap, USB_R0, > + USB_R0_U2D_ACT, 0); > + regmap_update_bits(priv->regmap, USB_R4, > + USB_R4_P21_SLEEP_M0, 0); > + } > +} I was already confused by the name of this function in v1. do you think "dwc3_meson_g12a_usb_otg_apply_mode" is a suitable name? [...] > +static int dwc3_meson_g12a_otg_mode_set(struct dwc3_meson_g12a *priv, > + enum phy_mode mode) > +{ > + int ret; > + > + if (!priv->phys[USB2_OTG_PHY]) > + return -EINVAL; > + > + if (mode == PHY_MODE_USB_HOST) > + dev_info(priv->dev, "switching to Host Mode\n"); > + else > + dev_info(priv->dev, "switching to Device Mode\n"); > + > + if (priv->vbus) { > + if (mode == PHY_MODE_USB_DEVICE) > + ret = regulator_disable(priv->vbus); > + else > + ret = regulator_enable(priv->vbus); do we need to track the regulator status (whether it's enabled or not)? the regulator framework WARN()s if it detects "unbalanced disables for " (I haven't tested this on one of my boards yet, so maybe it works because the callers of dwc3_meson_g12a_otg_mode_set() protect against this by not calling this function if the mode doesn't change) > + if (ret) > + return ret; > + } > + > + priv->otg_phy_mode = mode; > + > + dwc3_meson_g12a_usb2_set_mode(priv, USB2_OTG_PHY, mode); > + > + dwc3_meson_g12a_usb_init_mode(priv); > + > + return phy_set_mode(priv->phys[USB2_OTG_PHY], mode); > + } this is the only place where phy_set_mode is called I'm fine with keeping it but then it should be consistent at least for all USB2 PHYs. I suggest to either move the phy_set_mode call to dwc3_meson_g12a_usb2_set_mode or to remove it. [...] > +static int dwc3_meson_g12a_role_set(struct device *dev, enum usb_role role) > +{ > + struct dwc3_meson_g12a *priv = dev_get_drvdata(dev); > + enum phy_mode mode; > + > + if (role == USB_ROLE_NONE) > + return 0; > + > + mode = role == USB_ROLE_HOST ? PHY_MODE_USB_HOST : PHY_MODE_USB_DEVICE; (without surrounding parens I find the "role == USB_ROLE_HOST" part hard to distinguish from the "mode =" operation. if more people think this way then please speak up - otherwise it's probably just my personal taste) [...] > +static struct device *dwc3_meson_g12_find_child(struct device *dev, > + const char *compatible) > +{ > + struct platform_device *pdev; > + struct device_node *np; > + > + np = of_find_compatible_node(dev->of_node, NULL, compatible); > + if (!np) > + return NULL; > + > + pdev = of_find_device_by_node(np); maybe switch to of_get_compatible_child() here? This was done for the MMC driver in c483a5cc9d09f4 ("mmc: meson-mx-sdio: fix OF child-node lookup"), but I'm not sure if the problem described there also applies to dwc3_meson_g12_find_child [...] > +static int dwc3_meson_g12a_probe(struct platform_device *pdev) > +{ > + struct dwc3_meson_g12a *priv; > + struct device *dev = &pdev->dev; > + struct device_node *np = dev->of_node; > + void __iomem *base; > + struct resource *res; > + enum phy_mode otg_id; > + int ret, i; > + > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + base = devm_ioremap_resource(dev, res); > + if (IS_ERR(base)) > + return PTR_ERR(base); > + > + priv->regmap = devm_regmap_init_mmio(dev, base, > + &phy_meson_g12a_usb3_regmap_conf); > + if (IS_ERR(priv->regmap)) > + return PTR_ERR(priv->regmap); > + > + priv->vbus = devm_regulator_get_optional(dev, "vbus"); > + if (IS_ERR(priv->vbus)) { > + if (PTR_ERR(priv->vbus) == -EPROBE_DEFER) > + return PTR_ERR(priv->vbus); > + priv->vbus = NULL; > + } > + > + priv->clk = devm_clk_get(dev, NULL); > + if (IS_ERR(priv->clk)) > + return PTR_ERR(priv->clk); > + > + ret = clk_prepare_enable(priv->clk); > + if (ret) > + return ret; > + > + platform_set_drvdata(pdev, priv); > + priv->dev = dev; > + > + priv->reset = devm_reset_control_get(dev, NULL); > + if (IS_ERR(priv->reset)) { > + ret = PTR_ERR(priv->reset); > + dev_err(dev, "failed to get device reset, err=%d\n", ret); > + return ret; > + } clk_prepare_enable is called a few lines above but this (and a few more) error-paths don't call clk_disable_unprepare. Jerome suggested in the dwmac-meson8b driver that I use something like, which will even allow you to drop the clk_disable_unprepare call from dwc3_meson_g12a_remove and catch all error cases in dwc3_meson_g12a_probe at the same time: devm_add_action_or_reset(dev, (void(*)(void *))clk_disable_unprepare, priv->clk); (if you go with this then you also need to remove the clk_disable_unprepare after of_platform_populate) [...] > +static int dwc3_meson_g12a_remove(struct platform_device *pdev) > +{ > + struct dwc3_meson_g12a *priv = platform_get_drvdata(pdev); > + struct device *dev = &pdev->dev; > + int i; > + > + usb_role_switch_unregister(priv->role_switch); > + > + of_platform_depopulate(dev); > + > + for (i = 0 ; i < PHY_COUNT ; ++i) { > + phy_power_off(priv->phys[i]); > + phy_exit(priv->phys[i]); > + phy_put(priv->phys[i]); > + } > + > + clk_disable_unprepare(priv->clk); > + clk_put(priv->clk); priv->clk is obtained with devm_clk_get so the common clock framework will call clk_put for us automatically [...] > +static int __maybe_unused dwc3_meson_g12a_suspend(struct device *dev) > +{ > + struct dwc3_meson_g12a *priv = dev_get_drvdata(dev); > + int i; > + > + for (i = 0 ; i < PHY_COUNT ; ++i) > + if (priv->phys[i]) > + phy_exit(priv->phys[i]); phy_init is NULL-safe, so you can drop the NULL-check above > + > + reset_control_assert(priv->reset); > + > + return 0; > +} > + > +static int __maybe_unused dwc3_meson_g12a_resume(struct device *dev) > +{ > + struct dwc3_meson_g12a *priv = dev_get_drvdata(dev); > + int i, ret; > + > + reset_control_deassert(priv->reset); > + > + dwc3_meson_g12a_usb_init(priv); > + > + /* Init PHYs */ > + for (i = 0 ; i < PHY_COUNT ; ++i) { > + if (priv->phys[i]) { > + ret = phy_init(priv->phys[i]); phy_init is NULL-safe, so you can drop the NULL-check above Regards Martin _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic