From mboxrd@z Thu Jan 1 00:00:00 1970 From: Bjorn Andersson Subject: Re: [PATCH v2 2/2] phy: qualcomm: usb: Add Super-Speed PHY driver Date: Tue, 29 Jan 2019 12:27:26 -0800 Message-ID: <20190129202726.GA3036@builder> References: <1548761715-4004-1-git-send-email-jorge.ramirez-ortiz@linaro.org> <1548761715-4004-3-git-send-email-jorge.ramirez-ortiz@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <1548761715-4004-3-git-send-email-jorge.ramirez-ortiz@linaro.org> Sender: linux-kernel-owner@vger.kernel.org To: Jorge Ramirez-Ortiz Cc: gregkh@linuxfoundation.org, mark.rutland@arm.com, kishon@ti.com, jackp@codeaurora.org, andy.gross@linaro.org, swboyd@chromium.org, shawn.guo@linaro.org, vkoul@kernel.org, khasim.mohammed@linaro.org, devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-arm-msm@vger.kernel.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org List-Id: linux-arm-msm@vger.kernel.org On Tue 29 Jan 03:35 PST 2019, Jorge Ramirez-Ortiz wrote: > diff --git a/drivers/phy/qualcomm/phy-qcom-usb-ss.c b/drivers/phy/qualcomm/phy-qcom-usb-ss.c > new file mode 100644 > index 0000000..e6ae96e > --- /dev/null > +++ b/drivers/phy/qualcomm/phy-qcom-usb-ss.c > @@ -0,0 +1,347 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2012-2014,2017 The Linux Foundation. All rights reserved. > + * Copyright (c) 2018, Linaro Limited > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define PHY_CTRL0 0x6C > +#define PHY_CTRL1 0x70 > +#define PHY_CTRL2 0x74 > +#define PHY_CTRL4 0x7C > + > +/* PHY_CTRL bits */ > +#define REF_PHY_EN BIT(0) > +#define LANE0_PWR_ON BIT(2) > +#define SWI_PCS_CLK_SEL BIT(4) > +#define TST_PWR_DOWN BIT(4) > +#define PHY_RESET BIT(7) > + > +enum phy_vdd_ctrl { ENABLE, DISABLE, }; Use bool to describe boolean values. > +enum phy_regulator { VDD, VDDA1P8, }; It doesn't look like you need either of these if you remove the set_voltage calls. > + > +struct ssphy_priv { > + void __iomem *base; > + struct device *dev; > + struct reset_control *reset_com; > + struct reset_control *reset_phy; > + struct regulator *vbus; > + struct regulator_bulk_data *regs; > + int num_regs; > + struct clk_bulk_data *clks; > + int num_clks; > + enum phy_mode mode; > +}; > + > +static inline void qcom_ssphy_updatel(void __iomem *addr, u32 mask, u32 val) > +{ > + writel((readl(addr) & ~mask) | val, addr); > +} > + > +static inline int qcom_ssphy_vbus_enable(struct regulator *vbus) > +{ > + return !regulator_is_enabled(vbus) ? regulator_enable(vbus) : 0; regulator_is_enabled() will check if the actual regulator is on, not if you already voted for it. So if something else is enabling the vbus regulator you will skip your enable and be in the mercy of them not releasing it. Presumably there's only one consumer of this particular regulator, but it's a bad habit. Please keep track of this drivers requested state in this driver, if qcom_ssphy_vbus_ctrl() is called not only for the state changes. > +} > + > +static inline int qcom_ssphy_vbus_disable(struct regulator *vbus) > +{ > + return regulator_is_enabled(vbus) ? regulator_disable(vbus) : 0; > +} > + > +static int qcom_ssphy_vdd_ctrl(struct ssphy_priv *priv, enum phy_vdd_ctrl ctrl) As discussed on IRC, I think you should just leave the voltage constraints to DeviceTree. > +{ > + const int vdd_min = ctrl == ENABLE ? 1050000 : 0; > + const int vdd_max = 1050000; > + int ret; > + > + ret = regulator_set_voltage(priv->regs[VDD].consumer, vdd_min, vdd_max); > + if (ret) > + dev_err(priv->dev, "Failed to set regulator vdd to %d\n", > + vdd_min); > + > + return ret; > +} [..] > +static int qcom_ssphy_power_on(struct phy *phy) > +{ > + struct ssphy_priv *priv = phy_get_drvdata(phy); > + int ret; > + > + ret = qcom_ssphy_vdd_ctrl(priv, ENABLE); > + if (ret) > + return ret; > + > + ret = regulator_bulk_enable(priv->num_regs, priv->regs); > + if (ret) > + goto err1; > + > + ret = clk_bulk_prepare_enable(priv->num_clks, priv->clks); > + if (ret) > + goto err2; > + > + ret = qcom_ssphy_vbus_ctrl(priv->vbus, priv->mode); > + if (ret) > + goto err3; > + > + ret = qcom_ssphy_do_reset(priv); > + if (ret) > + goto err4; > + > + writeb(SWI_PCS_CLK_SEL, priv->base + PHY_CTRL0); > + qcom_ssphy_updatel(priv->base + PHY_CTRL4, LANE0_PWR_ON, LANE0_PWR_ON); > + qcom_ssphy_updatel(priv->base + PHY_CTRL2, REF_PHY_EN, REF_PHY_EN); > + qcom_ssphy_updatel(priv->base + PHY_CTRL4, TST_PWR_DOWN, 0); > + > + return 0; > +err4: Name your labels based on what they do, e.g. err_disable_vbus. > + if (priv->vbus && priv->mode != PHY_MODE_INVALID) > + qcom_ssphy_vbus_disable(priv->vbus); qcom_ssphy_vbus_ctrl() above was either enabling or disabling vbus, but here you're directly calling disable to unroll that. It think the result is correct (in host this reverts to disabled and in gadget it's a no-op), but I'm not sure I like the design of sometimes calling straight to the vbus enable/disable and sometimes to the wrapper function. > +err3: err_clk_disable > + clk_bulk_disable_unprepare(priv->num_clks, priv->clks); > +err2: > + regulator_bulk_disable(priv->num_regs, priv->regs); > +err1: > + qcom_ssphy_vdd_ctrl(priv, DISABLE); > + > + return ret; > +} > + > +static int qcom_ssphy_power_off(struct phy *phy) > +{ > + struct ssphy_priv *priv = phy_get_drvdata(phy); > + > + qcom_ssphy_updatel(priv->base + PHY_CTRL4, LANE0_PWR_ON, 0); > + qcom_ssphy_updatel(priv->base + PHY_CTRL2, REF_PHY_EN, 0); > + qcom_ssphy_updatel(priv->base + PHY_CTRL4, TST_PWR_DOWN, TST_PWR_DOWN); > + > + clk_bulk_disable_unprepare(priv->num_clks, priv->clks); > + regulator_bulk_disable(priv->num_regs, priv->regs); > + > + if (priv->vbus && priv->mode != PHY_MODE_INVALID) > + qcom_ssphy_vbus_disable(priv->vbus); > + > + qcom_ssphy_vdd_ctrl(priv, DISABLE); > + > + return 0; > +} > + > +static int qcom_ssphy_init_clock(struct ssphy_priv *priv) > +{ > + const char * const clk_id[] = { "ref", "phy", "pipe", }; > + int i; > + > + priv->num_clks = ARRAY_SIZE(clk_id); > + priv->clks = devm_kcalloc(priv->dev, priv->num_clks, > + sizeof(*priv->clks), GFP_KERNEL); You know num_clks is 3, so I would suggest that you just change the sshphy_priv to clks[3] and skip the dynamic allocation. Also, as num_clks always is ARRAY_SIZE(priv->clks) I would suggest using the latter, to make that clear throughout the driver. > + if (!priv->clks) > + return -ENOMEM; > + > + for (i = 0; i < priv->num_clks; i++) > + priv->clks[i].id = clk_id[i]; There's no harm in just writing this on three lines: priv->clks[0].id = "ref"; priv->clks[1].id = "phy"; priv->clks[2].id = "pipe"; > + > + return devm_clk_bulk_get(priv->dev, priv->num_clks, priv->clks); > +} > + > +static int qcom_ssphy_init_regulator(struct ssphy_priv *priv) > +{ > + const char * const reg_supplies[] = { > + [VDD] = "vdd", > + [VDDA1P8] = "vdda1p8", > + }; > + int ret, i; > + > + priv->num_regs = ARRAY_SIZE(reg_supplies); > + priv->regs = devm_kcalloc(priv->dev, priv->num_regs, > + sizeof(*priv->regs), GFP_KERNEL); As with clocks, you know there will only be 2 of these, make it fixed size in ssphy_priv. And as with clocks, I would suggest using ARRAY_SIZE(priv->regs) throughout the driver to make it obvious that's it's a static number. > + if (!priv->regs) > + return -ENOMEM; > + > + for (i = 0; i < priv->num_regs; i++) > + priv->regs[i].supply = reg_supplies[i]; As with clocks, just unroll this and fill in the 2 supplies directly. > + > + ret = devm_regulator_bulk_get(priv->dev, priv->num_regs, priv->regs); > + if (ret) > + return ret; > + > + priv->vbus = devm_regulator_get_optional(priv->dev, "vbus"); get_optional means that if vbus-supply is not found, rather than returning a dummy regulator object this will fail with -ENODEV. Given the rest of the logic related to vbus you should set vbus to NULL if the returned PTR_ERR(value) is -ENODEV, and fail for other errors. Or just drop the _optional, and let your vbus controls operate on the dummy regulator you get back. (Right now vbus can't be NULL, so these checks are not very useful) > + if (IS_ERR(priv->vbus)) > + return PTR_ERR(priv->vbus); > + > + return 0; return PTR_ERR_OR_ZERO(priv->vbus) (Although that might change based on above comment) > +} Regards, Bjorn 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,2/2] phy: qualcomm: usb: Add Super-Speed PHY driver From: Bjorn Andersson Message-Id: <20190129202726.GA3036@builder> Date: Tue, 29 Jan 2019 12:27:26 -0800 To: Jorge Ramirez-Ortiz Cc: gregkh@linuxfoundation.org, mark.rutland@arm.com, kishon@ti.com, jackp@codeaurora.org, andy.gross@linaro.org, swboyd@chromium.org, shawn.guo@linaro.org, vkoul@kernel.org, khasim.mohammed@linaro.org, devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-arm-msm@vger.kernel.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org List-ID: T24gVHVlIDI5IEphbiAwMzozNSBQU1QgMjAxOSwgSm9yZ2UgUmFtaXJlei1PcnRpeiB3cm90ZToK PiBkaWZmIC0tZ2l0IGEvZHJpdmVycy9waHkvcXVhbGNvbW0vcGh5LXFjb20tdXNiLXNzLmMgYi9k cml2ZXJzL3BoeS9xdWFsY29tbS9waHktcWNvbS11c2Itc3MuYwo+IG5ldyBmaWxlIG1vZGUgMTAw NjQ0Cj4gaW5kZXggMDAwMDAwMC4uZTZhZTk2ZQo+IC0tLSAvZGV2L251bGwKPiArKysgYi9kcml2 ZXJzL3BoeS9xdWFsY29tbS9waHktcWNvbS11c2Itc3MuYwo+IEBAIC0wLDAgKzEsMzQ3IEBACj4g Ky8vIFNQRFgtTGljZW5zZS1JZGVudGlmaWVyOiBHUEwtMi4wCj4gKy8qCj4gKyAqIENvcHlyaWdo dCAoYykgMjAxMi0yMDE0LDIwMTcgVGhlIExpbnV4IEZvdW5kYXRpb24uIEFsbCByaWdodHMgcmVz ZXJ2ZWQuCj4gKyAqIENvcHlyaWdodCAoYykgMjAxOCwgTGluYXJvIExpbWl0ZWQKPiArICovCj4g Kwo+ICsjaW5jbHVkZSA8bGludXgvY2xrLmg+Cj4gKyNpbmNsdWRlIDxsaW51eC9kZWxheS5oPgo+ ICsjaW5jbHVkZSA8bGludXgvZXJyLmg+Cj4gKyNpbmNsdWRlIDxsaW51eC9pby5oPgo+ICsjaW5j bHVkZSA8bGludXgva2VybmVsLmg+Cj4gKyNpbmNsdWRlIDxsaW51eC9tb2R1bGUuaD4KPiArI2lu Y2x1ZGUgPGxpbnV4L29mLmg+Cj4gKyNpbmNsdWRlIDxsaW51eC9waHkvcGh5Lmg+Cj4gKyNpbmNs dWRlIDxsaW51eC9wbGF0Zm9ybV9kZXZpY2UuaD4KPiArI2luY2x1ZGUgPGxpbnV4L3JlZ3VsYXRv ci9jb25zdW1lci5oPgo+ICsjaW5jbHVkZSA8bGludXgvcmVzZXQuaD4KPiArI2luY2x1ZGUgPGxp bnV4L3NsYWIuaD4KPiArCj4gKyNkZWZpbmUgUEhZX0NUUkwwCQkJMHg2Qwo+ICsjZGVmaW5lIFBI WV9DVFJMMQkJCTB4NzAKPiArI2RlZmluZSBQSFlfQ1RSTDIJCQkweDc0Cj4gKyNkZWZpbmUgUEhZ X0NUUkw0CQkJMHg3Qwo+ICsKPiArLyogUEhZX0NUUkwgYml0cyAqLwo+ICsjZGVmaW5lIFJFRl9Q SFlfRU4JCQlCSVQoMCkKPiArI2RlZmluZSBMQU5FMF9QV1JfT04JCQlCSVQoMikKPiArI2RlZmlu ZSBTV0lfUENTX0NMS19TRUwJCQlCSVQoNCkKPiArI2RlZmluZSBUU1RfUFdSX0RPV04JCQlCSVQo NCkKPiArI2RlZmluZSBQSFlfUkVTRVQJCQlCSVQoNykKPiArCj4gK2VudW0gcGh5X3ZkZF9jdHJs IHsgRU5BQkxFLCBESVNBQkxFLCB9OwoKVXNlIGJvb2wgdG8gZGVzY3JpYmUgYm9vbGVhbiB2YWx1 ZXMuCgo+ICtlbnVtIHBoeV9yZWd1bGF0b3IgeyBWREQsIFZEREExUDgsIH07CgpJdCBkb2Vzbid0 IGxvb2sgbGlrZSB5b3UgbmVlZCBlaXRoZXIgb2YgdGhlc2UgaWYgeW91IHJlbW92ZSB0aGUKc2V0 X3ZvbHRhZ2UgY2FsbHMuCgo+ICsKPiArc3RydWN0IHNzcGh5X3ByaXYgewo+ICsJdm9pZCBfX2lv bWVtICpiYXNlOwo+ICsJc3RydWN0IGRldmljZSAqZGV2Owo+ICsJc3RydWN0IHJlc2V0X2NvbnRy b2wgKnJlc2V0X2NvbTsKPiArCXN0cnVjdCByZXNldF9jb250cm9sICpyZXNldF9waHk7Cj4gKwlz dHJ1Y3QgcmVndWxhdG9yICp2YnVzOwo+ICsJc3RydWN0IHJlZ3VsYXRvcl9idWxrX2RhdGEgKnJl Z3M7Cj4gKwlpbnQgbnVtX3JlZ3M7Cj4gKwlzdHJ1Y3QgY2xrX2J1bGtfZGF0YSAqY2xrczsKPiAr CWludCBudW1fY2xrczsKPiArCWVudW0gcGh5X21vZGUgbW9kZTsKPiArfTsKPiArCj4gK3N0YXRp YyBpbmxpbmUgdm9pZCBxY29tX3NzcGh5X3VwZGF0ZWwodm9pZCBfX2lvbWVtICphZGRyLCB1MzIg bWFzaywgdTMyIHZhbCkKPiArewo+ICsJd3JpdGVsKChyZWFkbChhZGRyKSAmIH5tYXNrKSB8IHZh bCwgYWRkcik7Cj4gK30KPiArCj4gK3N0YXRpYyBpbmxpbmUgaW50IHFjb21fc3NwaHlfdmJ1c19l bmFibGUoc3RydWN0IHJlZ3VsYXRvciAqdmJ1cykKPiArewo+ICsJcmV0dXJuICFyZWd1bGF0b3Jf aXNfZW5hYmxlZCh2YnVzKSA/IHJlZ3VsYXRvcl9lbmFibGUodmJ1cykgOiAwOwoKcmVndWxhdG9y X2lzX2VuYWJsZWQoKSB3aWxsIGNoZWNrIGlmIHRoZSBhY3R1YWwgcmVndWxhdG9yIGlzIG9uLCBu b3QgaWYKeW91IGFscmVhZHkgdm90ZWQgZm9yIGl0LgoKU28gaWYgc29tZXRoaW5nIGVsc2UgaXMg ZW5hYmxpbmcgdGhlIHZidXMgcmVndWxhdG9yIHlvdSB3aWxsIHNraXAgeW91cgplbmFibGUgYW5k IGJlIGluIHRoZSBtZXJjeSBvZiB0aGVtIG5vdCByZWxlYXNpbmcgaXQuIFByZXN1bWFibHkgdGhl cmUncwpvbmx5IG9uZSBjb25zdW1lciBvZiB0aGlzIHBhcnRpY3VsYXIgcmVndWxhdG9yLCBidXQg aXQncyBhIGJhZCBoYWJpdC4KClBsZWFzZSBrZWVwIHRyYWNrIG9mIHRoaXMgZHJpdmVycyByZXF1 ZXN0ZWQgc3RhdGUgaW4gdGhpcyBkcml2ZXIsIGlmCnFjb21fc3NwaHlfdmJ1c19jdHJsKCkgaXMg Y2FsbGVkIG5vdCBvbmx5IGZvciB0aGUgc3RhdGUgY2hhbmdlcy4KCj4gK30KPiArCj4gK3N0YXRp YyBpbmxpbmUgaW50IHFjb21fc3NwaHlfdmJ1c19kaXNhYmxlKHN0cnVjdCByZWd1bGF0b3IgKnZi dXMpCj4gK3sKPiArCXJldHVybiByZWd1bGF0b3JfaXNfZW5hYmxlZCh2YnVzKSA/IHJlZ3VsYXRv cl9kaXNhYmxlKHZidXMpIDogMDsKPiArfQo+ICsKPiArc3RhdGljIGludCBxY29tX3NzcGh5X3Zk ZF9jdHJsKHN0cnVjdCBzc3BoeV9wcml2ICpwcml2LCBlbnVtIHBoeV92ZGRfY3RybCBjdHJsKQoK QXMgZGlzY3Vzc2VkIG9uIElSQywgSSB0aGluayB5b3Ugc2hvdWxkIGp1c3QgbGVhdmUgdGhlIHZv bHRhZ2UKY29uc3RyYWludHMgdG8gRGV2aWNlVHJlZS4KCj4gK3sKPiArCWNvbnN0IGludCB2ZGRf bWluID0gY3RybCA9PSBFTkFCTEUgPyAxMDUwMDAwIDogMDsKPiArCWNvbnN0IGludCB2ZGRfbWF4 ID0gMTA1MDAwMDsKPiArCWludCByZXQ7Cj4gKwo+ICsJcmV0ID0gcmVndWxhdG9yX3NldF92b2x0 YWdlKHByaXYtPnJlZ3NbVkREXS5jb25zdW1lciwgdmRkX21pbiwgdmRkX21heCk7Cj4gKwlpZiAo cmV0KQo+ICsJCWRldl9lcnIocHJpdi0+ZGV2LCAiRmFpbGVkIHRvIHNldCByZWd1bGF0b3IgdmRk IHRvICVkXG4iLAo+ICsJCQl2ZGRfbWluKTsKPiArCj4gKwlyZXR1cm4gcmV0Owo+ICt9ClsuLl0K PiArc3RhdGljIGludCBxY29tX3NzcGh5X3Bvd2VyX29uKHN0cnVjdCBwaHkgKnBoeSkKPiArewo+ ICsJc3RydWN0IHNzcGh5X3ByaXYgKnByaXYgPSBwaHlfZ2V0X2RydmRhdGEocGh5KTsKPiArCWlu dCByZXQ7Cj4gKwo+ICsJcmV0ID0gcWNvbV9zc3BoeV92ZGRfY3RybChwcml2LCBFTkFCTEUpOwo+ ICsJaWYgKHJldCkKPiArCQlyZXR1cm4gcmV0Owo+ICsKPiArCXJldCA9IHJlZ3VsYXRvcl9idWxr X2VuYWJsZShwcml2LT5udW1fcmVncywgcHJpdi0+cmVncyk7Cj4gKwlpZiAocmV0KQo+ICsJCWdv dG8gZXJyMTsKPiArCj4gKwlyZXQgPSBjbGtfYnVsa19wcmVwYXJlX2VuYWJsZShwcml2LT5udW1f Y2xrcywgcHJpdi0+Y2xrcyk7Cj4gKwlpZiAocmV0KQo+ICsJCWdvdG8gZXJyMjsKPiArCj4gKwly ZXQgPSBxY29tX3NzcGh5X3ZidXNfY3RybChwcml2LT52YnVzLCBwcml2LT5tb2RlKTsKPiArCWlm IChyZXQpCj4gKwkJZ290byBlcnIzOwo+ICsKPiArCXJldCA9IHFjb21fc3NwaHlfZG9fcmVzZXQo cHJpdik7Cj4gKwlpZiAocmV0KQo+ICsJCWdvdG8gZXJyNDsKPiArCj4gKwl3cml0ZWIoU1dJX1BD U19DTEtfU0VMLCBwcml2LT5iYXNlICsgUEhZX0NUUkwwKTsKPiArCXFjb21fc3NwaHlfdXBkYXRl bChwcml2LT5iYXNlICsgUEhZX0NUUkw0LCBMQU5FMF9QV1JfT04sIExBTkUwX1BXUl9PTik7Cj4g KwlxY29tX3NzcGh5X3VwZGF0ZWwocHJpdi0+YmFzZSArIFBIWV9DVFJMMiwgUkVGX1BIWV9FTiwg UkVGX1BIWV9FTik7Cj4gKwlxY29tX3NzcGh5X3VwZGF0ZWwocHJpdi0+YmFzZSArIFBIWV9DVFJM NCwgVFNUX1BXUl9ET1dOLCAwKTsKPiArCj4gKwlyZXR1cm4gMDsKPiArZXJyNDoKCk5hbWUgeW91 ciBsYWJlbHMgYmFzZWQgb24gd2hhdCB0aGV5IGRvLCBlLmcuIGVycl9kaXNhYmxlX3ZidXMuCgo+ ICsJaWYgKHByaXYtPnZidXMgJiYgcHJpdi0+bW9kZSAhPSBQSFlfTU9ERV9JTlZBTElEKQo+ICsJ CXFjb21fc3NwaHlfdmJ1c19kaXNhYmxlKHByaXYtPnZidXMpOwoKcWNvbV9zc3BoeV92YnVzX2N0 cmwoKSBhYm92ZSB3YXMgZWl0aGVyIGVuYWJsaW5nIG9yIGRpc2FibGluZyB2YnVzLCBidXQKaGVy ZSB5b3UncmUgZGlyZWN0bHkgY2FsbGluZyBkaXNhYmxlIHRvIHVucm9sbCB0aGF0LiBJdCB0aGlu ayB0aGUgcmVzdWx0CmlzIGNvcnJlY3QgKGluIGhvc3QgdGhpcyByZXZlcnRzIHRvIGRpc2FibGVk IGFuZCBpbiBnYWRnZXQgaXQncyBhCm5vLW9wKSwgYnV0IEknbSBub3Qgc3VyZSBJIGxpa2UgdGhl IGRlc2lnbiBvZiBzb21ldGltZXMgY2FsbGluZyBzdHJhaWdodAp0byB0aGUgdmJ1cyBlbmFibGUv ZGlzYWJsZSBhbmQgc29tZXRpbWVzIHRvIHRoZSB3cmFwcGVyIGZ1bmN0aW9uLgoKPiArZXJyMzoK CmVycl9jbGtfZGlzYWJsZQoKPiArCWNsa19idWxrX2Rpc2FibGVfdW5wcmVwYXJlKHByaXYtPm51 bV9jbGtzLCBwcml2LT5jbGtzKTsKPiArZXJyMjoKPiArCXJlZ3VsYXRvcl9idWxrX2Rpc2FibGUo cHJpdi0+bnVtX3JlZ3MsIHByaXYtPnJlZ3MpOwo+ICtlcnIxOgo+ICsJcWNvbV9zc3BoeV92ZGRf Y3RybChwcml2LCBESVNBQkxFKTsKPiArCj4gKwlyZXR1cm4gcmV0Owo+ICt9Cj4gKwo+ICtzdGF0 aWMgaW50IHFjb21fc3NwaHlfcG93ZXJfb2ZmKHN0cnVjdCBwaHkgKnBoeSkKPiArewo+ICsJc3Ry dWN0IHNzcGh5X3ByaXYgKnByaXYgPSBwaHlfZ2V0X2RydmRhdGEocGh5KTsKPiArCj4gKwlxY29t X3NzcGh5X3VwZGF0ZWwocHJpdi0+YmFzZSArIFBIWV9DVFJMNCwgTEFORTBfUFdSX09OLCAwKTsK PiArCXFjb21fc3NwaHlfdXBkYXRlbChwcml2LT5iYXNlICsgUEhZX0NUUkwyLCBSRUZfUEhZX0VO LCAwKTsKPiArCXFjb21fc3NwaHlfdXBkYXRlbChwcml2LT5iYXNlICsgUEhZX0NUUkw0LCBUU1Rf UFdSX0RPV04sIFRTVF9QV1JfRE9XTik7Cj4gKwo+ICsJY2xrX2J1bGtfZGlzYWJsZV91bnByZXBh cmUocHJpdi0+bnVtX2Nsa3MsIHByaXYtPmNsa3MpOwo+ICsJcmVndWxhdG9yX2J1bGtfZGlzYWJs ZShwcml2LT5udW1fcmVncywgcHJpdi0+cmVncyk7Cj4gKwo+ICsJaWYgKHByaXYtPnZidXMgJiYg cHJpdi0+bW9kZSAhPSBQSFlfTU9ERV9JTlZBTElEKQo+ICsJCXFjb21fc3NwaHlfdmJ1c19kaXNh YmxlKHByaXYtPnZidXMpOwo+ICsKPiArCXFjb21fc3NwaHlfdmRkX2N0cmwocHJpdiwgRElTQUJM RSk7Cj4gKwo+ICsJcmV0dXJuIDA7Cj4gK30KPiArCj4gK3N0YXRpYyBpbnQgcWNvbV9zc3BoeV9p bml0X2Nsb2NrKHN0cnVjdCBzc3BoeV9wcml2ICpwcml2KQo+ICt7Cj4gKwljb25zdCBjaGFyICog Y29uc3QgY2xrX2lkW10gPSB7ICJyZWYiLCAicGh5IiwgInBpcGUiLCB9Owo+ICsJaW50IGk7Cj4g Kwo+ICsJcHJpdi0+bnVtX2Nsa3MgPSBBUlJBWV9TSVpFKGNsa19pZCk7Cj4gKwlwcml2LT5jbGtz ID0gZGV2bV9rY2FsbG9jKHByaXYtPmRldiwgcHJpdi0+bnVtX2Nsa3MsCj4gKwkJCQkgIHNpemVv ZigqcHJpdi0+Y2xrcyksIEdGUF9LRVJORUwpOwoKWW91IGtub3cgbnVtX2Nsa3MgaXMgMywgc28g SSB3b3VsZCBzdWdnZXN0IHRoYXQgeW91IGp1c3QgY2hhbmdlIHRoZQpzc2hwaHlfcHJpdiB0byBj bGtzWzNdIGFuZCBza2lwIHRoZSBkeW5hbWljIGFsbG9jYXRpb24uCgoKQWxzbywgYXMgbnVtX2Ns a3MgYWx3YXlzIGlzIEFSUkFZX1NJWkUocHJpdi0+Y2xrcykgSSB3b3VsZCBzdWdnZXN0IHVzaW5n CnRoZSBsYXR0ZXIsIHRvIG1ha2UgdGhhdCBjbGVhciB0aHJvdWdob3V0IHRoZSBkcml2ZXIuCgo+ ICsJaWYgKCFwcml2LT5jbGtzKQo+ICsJCXJldHVybiAtRU5PTUVNOwo+ICsKPiArCWZvciAoaSA9 IDA7IGkgPCBwcml2LT5udW1fY2xrczsgaSsrKQo+ICsJCXByaXYtPmNsa3NbaV0uaWQgPSBjbGtf aWRbaV07CgpUaGVyZSdzIG5vIGhhcm0gaW4ganVzdCB3cml0aW5nIHRoaXMgb24gdGhyZWUgbGlu ZXM6CgoJcHJpdi0+Y2xrc1swXS5pZCA9ICJyZWYiOwoJcHJpdi0+Y2xrc1sxXS5pZCA9ICJwaHki OwoJcHJpdi0+Y2xrc1syXS5pZCA9ICJwaXBlIjsKCj4gKwo+ICsJcmV0dXJuIGRldm1fY2xrX2J1 bGtfZ2V0KHByaXYtPmRldiwgcHJpdi0+bnVtX2Nsa3MsIHByaXYtPmNsa3MpOwo+ICt9Cj4gKwo+ ICtzdGF0aWMgaW50IHFjb21fc3NwaHlfaW5pdF9yZWd1bGF0b3Ioc3RydWN0IHNzcGh5X3ByaXYg KnByaXYpCj4gK3sKPiArCWNvbnN0IGNoYXIgKiBjb25zdCByZWdfc3VwcGxpZXNbXSA9IHsKPiAr CQlbVkREXSA9ICJ2ZGQiLAo+ICsJCVtWRERBMVA4XSA9ICJ2ZGRhMXA4IiwKPiArCX07Cj4gKwlp bnQgcmV0LCBpOwo+ICsKPiArCXByaXYtPm51bV9yZWdzID0gQVJSQVlfU0laRShyZWdfc3VwcGxp ZXMpOwo+ICsJcHJpdi0+cmVncyA9IGRldm1fa2NhbGxvYyhwcml2LT5kZXYsIHByaXYtPm51bV9y ZWdzLAo+ICsJCQkJICBzaXplb2YoKnByaXYtPnJlZ3MpLCBHRlBfS0VSTkVMKTsKCkFzIHdpdGgg Y2xvY2tzLCB5b3Uga25vdyB0aGVyZSB3aWxsIG9ubHkgYmUgMiBvZiB0aGVzZSwgbWFrZSBpdCBm aXhlZApzaXplIGluIHNzcGh5X3ByaXYuCgpBbmQgYXMgd2l0aCBjbG9ja3MsIEkgd291bGQgc3Vn Z2VzdCB1c2luZyBBUlJBWV9TSVpFKHByaXYtPnJlZ3MpCnRocm91Z2hvdXQgdGhlIGRyaXZlciB0 byBtYWtlIGl0IG9idmlvdXMgdGhhdCdzIGl0J3MgYSBzdGF0aWMgbnVtYmVyLiAKCj4gKwlpZiAo IXByaXYtPnJlZ3MpCj4gKwkJcmV0dXJuIC1FTk9NRU07Cj4gKwo+ICsJZm9yIChpID0gMDsgaSA8 IHByaXYtPm51bV9yZWdzOyBpKyspCj4gKwkJcHJpdi0+cmVnc1tpXS5zdXBwbHkgPSByZWdfc3Vw cGxpZXNbaV07CgpBcyB3aXRoIGNsb2NrcywganVzdCB1bnJvbGwgdGhpcyBhbmQgZmlsbCBpbiB0 aGUgMiBzdXBwbGllcyBkaXJlY3RseS4KCj4gKwo+ICsJcmV0ID0gZGV2bV9yZWd1bGF0b3JfYnVs a19nZXQocHJpdi0+ZGV2LCBwcml2LT5udW1fcmVncywgcHJpdi0+cmVncyk7Cj4gKwlpZiAocmV0 KQo+ICsJCXJldHVybiByZXQ7Cj4gKwo+ICsJcHJpdi0+dmJ1cyA9IGRldm1fcmVndWxhdG9yX2dl dF9vcHRpb25hbChwcml2LT5kZXYsICJ2YnVzIik7CgpnZXRfb3B0aW9uYWwgbWVhbnMgdGhhdCBp ZiB2YnVzLXN1cHBseSBpcyBub3QgZm91bmQsIHJhdGhlciB0aGFuCnJldHVybmluZyBhIGR1bW15 IHJlZ3VsYXRvciBvYmplY3QgdGhpcyB3aWxsIGZhaWwgd2l0aCAtRU5PREVWLgoKR2l2ZW4gdGhl IHJlc3Qgb2YgdGhlIGxvZ2ljIHJlbGF0ZWQgdG8gdmJ1cyB5b3Ugc2hvdWxkIHNldCB2YnVzIHRv IE5VTEwKaWYgdGhlIHJldHVybmVkIFBUUl9FUlIodmFsdWUpIGlzIC1FTk9ERVYsIGFuZCBmYWls IGZvciBvdGhlciBlcnJvcnMuCgoKT3IganVzdCBkcm9wIHRoZSBfb3B0aW9uYWwsIGFuZCBsZXQg eW91ciB2YnVzIGNvbnRyb2xzIG9wZXJhdGUgb24gdGhlCmR1bW15IHJlZ3VsYXRvciB5b3UgZ2V0 IGJhY2suCgooUmlnaHQgbm93IHZidXMgY2FuJ3QgYmUgTlVMTCwgc28gdGhlc2UgY2hlY2tzIGFy ZSBub3QgdmVyeSB1c2VmdWwpCgo+ICsJaWYgKElTX0VSUihwcml2LT52YnVzKSkKPiArCQlyZXR1 cm4gUFRSX0VSUihwcml2LT52YnVzKTsKPiArCj4gKwlyZXR1cm4gMDsKCnJldHVybiBQVFJfRVJS X09SX1pFUk8ocHJpdi0+dmJ1cykKCihBbHRob3VnaCB0aGF0IG1pZ2h0IGNoYW5nZSBiYXNlZCBv biBhYm92ZSBjb21tZW50KQoKPiArfQoKUmVnYXJkcywKQmpvcm4K 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=-5.5 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SPF_PASS,USER_AGENT_MUTT 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 62FD8C169C4 for ; Tue, 29 Jan 2019 20:27:51 +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 2F76D21473 for ; Tue, 29 Jan 2019 20:27:51 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="Cs48fZgP"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=linaro.org header.i=@linaro.org header.b="kReRXwnN" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 2F76D21473 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linaro.org 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:In-Reply-To:MIME-Version:References: Message-ID:Subject:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=r2yCtW4rp43l92KqVO99xBnYgA+USAKPn9avYvxBNFY=; b=Cs48fZgP0CrGHL zvJ6Me8ADwB8YvaW+RqrJCwD8DlN6k70jTH9iuJuSznBXK1BB8DKnho5S1BpAfC3dRXjKFFB8dUqE JYdmjGyKK5oQJDu6m0Pge8IbGsFl4W5MC8/Dalp9Op6YKpfckaPaVO+fap5jQ2bZIGhdQEyHIRiZe jeKDbCdrltTEZKopBNlLyU+L+L7fRZY7A1NvWRpu9U6VCL7/nu3I0M/XCL2Qhtukka3OfqjnBsLLS K6B6AlpPp1yxHfWcVrZYgR7oVSo0QCmJP8RBxeF/fIYNnE4zWE4xyeUlJvwEg6nLIfb/YbAv0hluK dLuj4zW+3+mLOiUj7ZDQ==; 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 1goZz3-0004tY-2O; Tue, 29 Jan 2019 20:27:49 +0000 Received: from mail-pg1-x543.google.com ([2607:f8b0:4864:20::543]) by bombadil.infradead.org with esmtps (Exim 4.90_1 #2 (Red Hat Linux)) id 1goZyz-0004t5-Ff for linux-arm-kernel@lists.infradead.org; Tue, 29 Jan 2019 20:27:47 +0000 Received: by mail-pg1-x543.google.com with SMTP id j10so9259194pga.1 for ; Tue, 29 Jan 2019 12:27:45 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=FcXX162GqhxYJvEtt0yO0BCjii24nkrvGMIFzvdDOIU=; b=kReRXwnNv9LdWNm9YCPt8kgqNiKnumGOhE3nk1ifJaU9YFoUPYUThUHCc/uGenOrZl Rsz6/DEbTIehqY2NJhGdHpyyE5h9HpTxUkyWD1iy3qfqym14NuD4JDGjYig5ONXLqcmK J20e2P7g+fAqwaeGD1nWEgpEdV2KNPls1dYg4= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=FcXX162GqhxYJvEtt0yO0BCjii24nkrvGMIFzvdDOIU=; b=SEftBmsbz4sXTjOVQtcy3/yIPJv/LKEYVf+94es4NbJW5XkO6vBnBVOcIcdL+PZlcV EpxuM+eBcGlF/2/iRUPtxbN+gUPG7e/FcJ+G2mRCs+gazfj2aV5Z69X2GrUFns/a4OBt usfGvlpyJBsBTobmAKLuDlTWfJiuVy9Z6itDK0T/aRTxjmO6s0jZaWa/DGLvP8ePmzM9 Ho8qY3mQpeogfIflhYZJwUIEu2dgcgKLjp8ZPzgaWn10dD9QwOOOcw2/iEm3+mh/ZQdK gtEGg9rzhR24ZAYrkZqXqcQPkQTLs58TnqNaoze/0hJRQln+BHkFCUDyAc2meDhxNzd3 9MCg== X-Gm-Message-State: AJcUukfdLU+fDmrZtkNe1KVtXIMGPiTvjMOCQ9vP0INkJa7QYS3esJRu kNwbbg/lSUwB3qPVJlKhXFZl1g== X-Google-Smtp-Source: ALg8bN4ZtZ2R5A1iBrYut3stE1Zu34zmPfEZwJwEBlHcx8zHXSHsl0XDs2uQHOyK2tofLtOYQ0O0+A== X-Received: by 2002:a62:4181:: with SMTP id g1mr27587030pfd.45.1548793664269; Tue, 29 Jan 2019 12:27:44 -0800 (PST) Received: from builder (104-188-17-28.lightspeed.sndgca.sbcglobal.net. [104.188.17.28]) by smtp.gmail.com with ESMTPSA id 6sm78912651pfv.30.2019.01.29.12.27.42 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 29 Jan 2019 12:27:43 -0800 (PST) Date: Tue, 29 Jan 2019 12:27:26 -0800 From: Bjorn Andersson To: Jorge Ramirez-Ortiz Subject: Re: [PATCH v2 2/2] phy: qualcomm: usb: Add Super-Speed PHY driver Message-ID: <20190129202726.GA3036@builder> References: <1548761715-4004-1-git-send-email-jorge.ramirez-ortiz@linaro.org> <1548761715-4004-3-git-send-email-jorge.ramirez-ortiz@linaro.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <1548761715-4004-3-git-send-email-jorge.ramirez-ortiz@linaro.org> User-Agent: Mutt/1.10.0 (2018-05-17) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20190129_122745_536167_54F73EA4 X-CRM114-Status: GOOD ( 22.43 ) 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: mark.rutland@arm.com, devicetree@vger.kernel.org, jackp@codeaurora.org, shawn.guo@linaro.org, gregkh@linuxfoundation.org, linux-usb@vger.kernel.org, khasim.mohammed@linaro.org, linux-kernel@vger.kernel.org, swboyd@chromium.org, vkoul@kernel.org, linux-arm-msm@vger.kernel.org, andy.gross@linaro.org, kishon@ti.com, 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 On Tue 29 Jan 03:35 PST 2019, Jorge Ramirez-Ortiz wrote: > diff --git a/drivers/phy/qualcomm/phy-qcom-usb-ss.c b/drivers/phy/qualcomm/phy-qcom-usb-ss.c > new file mode 100644 > index 0000000..e6ae96e > --- /dev/null > +++ b/drivers/phy/qualcomm/phy-qcom-usb-ss.c > @@ -0,0 +1,347 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2012-2014,2017 The Linux Foundation. All rights reserved. > + * Copyright (c) 2018, Linaro Limited > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define PHY_CTRL0 0x6C > +#define PHY_CTRL1 0x70 > +#define PHY_CTRL2 0x74 > +#define PHY_CTRL4 0x7C > + > +/* PHY_CTRL bits */ > +#define REF_PHY_EN BIT(0) > +#define LANE0_PWR_ON BIT(2) > +#define SWI_PCS_CLK_SEL BIT(4) > +#define TST_PWR_DOWN BIT(4) > +#define PHY_RESET BIT(7) > + > +enum phy_vdd_ctrl { ENABLE, DISABLE, }; Use bool to describe boolean values. > +enum phy_regulator { VDD, VDDA1P8, }; It doesn't look like you need either of these if you remove the set_voltage calls. > + > +struct ssphy_priv { > + void __iomem *base; > + struct device *dev; > + struct reset_control *reset_com; > + struct reset_control *reset_phy; > + struct regulator *vbus; > + struct regulator_bulk_data *regs; > + int num_regs; > + struct clk_bulk_data *clks; > + int num_clks; > + enum phy_mode mode; > +}; > + > +static inline void qcom_ssphy_updatel(void __iomem *addr, u32 mask, u32 val) > +{ > + writel((readl(addr) & ~mask) | val, addr); > +} > + > +static inline int qcom_ssphy_vbus_enable(struct regulator *vbus) > +{ > + return !regulator_is_enabled(vbus) ? regulator_enable(vbus) : 0; regulator_is_enabled() will check if the actual regulator is on, not if you already voted for it. So if something else is enabling the vbus regulator you will skip your enable and be in the mercy of them not releasing it. Presumably there's only one consumer of this particular regulator, but it's a bad habit. Please keep track of this drivers requested state in this driver, if qcom_ssphy_vbus_ctrl() is called not only for the state changes. > +} > + > +static inline int qcom_ssphy_vbus_disable(struct regulator *vbus) > +{ > + return regulator_is_enabled(vbus) ? regulator_disable(vbus) : 0; > +} > + > +static int qcom_ssphy_vdd_ctrl(struct ssphy_priv *priv, enum phy_vdd_ctrl ctrl) As discussed on IRC, I think you should just leave the voltage constraints to DeviceTree. > +{ > + const int vdd_min = ctrl == ENABLE ? 1050000 : 0; > + const int vdd_max = 1050000; > + int ret; > + > + ret = regulator_set_voltage(priv->regs[VDD].consumer, vdd_min, vdd_max); > + if (ret) > + dev_err(priv->dev, "Failed to set regulator vdd to %d\n", > + vdd_min); > + > + return ret; > +} [..] > +static int qcom_ssphy_power_on(struct phy *phy) > +{ > + struct ssphy_priv *priv = phy_get_drvdata(phy); > + int ret; > + > + ret = qcom_ssphy_vdd_ctrl(priv, ENABLE); > + if (ret) > + return ret; > + > + ret = regulator_bulk_enable(priv->num_regs, priv->regs); > + if (ret) > + goto err1; > + > + ret = clk_bulk_prepare_enable(priv->num_clks, priv->clks); > + if (ret) > + goto err2; > + > + ret = qcom_ssphy_vbus_ctrl(priv->vbus, priv->mode); > + if (ret) > + goto err3; > + > + ret = qcom_ssphy_do_reset(priv); > + if (ret) > + goto err4; > + > + writeb(SWI_PCS_CLK_SEL, priv->base + PHY_CTRL0); > + qcom_ssphy_updatel(priv->base + PHY_CTRL4, LANE0_PWR_ON, LANE0_PWR_ON); > + qcom_ssphy_updatel(priv->base + PHY_CTRL2, REF_PHY_EN, REF_PHY_EN); > + qcom_ssphy_updatel(priv->base + PHY_CTRL4, TST_PWR_DOWN, 0); > + > + return 0; > +err4: Name your labels based on what they do, e.g. err_disable_vbus. > + if (priv->vbus && priv->mode != PHY_MODE_INVALID) > + qcom_ssphy_vbus_disable(priv->vbus); qcom_ssphy_vbus_ctrl() above was either enabling or disabling vbus, but here you're directly calling disable to unroll that. It think the result is correct (in host this reverts to disabled and in gadget it's a no-op), but I'm not sure I like the design of sometimes calling straight to the vbus enable/disable and sometimes to the wrapper function. > +err3: err_clk_disable > + clk_bulk_disable_unprepare(priv->num_clks, priv->clks); > +err2: > + regulator_bulk_disable(priv->num_regs, priv->regs); > +err1: > + qcom_ssphy_vdd_ctrl(priv, DISABLE); > + > + return ret; > +} > + > +static int qcom_ssphy_power_off(struct phy *phy) > +{ > + struct ssphy_priv *priv = phy_get_drvdata(phy); > + > + qcom_ssphy_updatel(priv->base + PHY_CTRL4, LANE0_PWR_ON, 0); > + qcom_ssphy_updatel(priv->base + PHY_CTRL2, REF_PHY_EN, 0); > + qcom_ssphy_updatel(priv->base + PHY_CTRL4, TST_PWR_DOWN, TST_PWR_DOWN); > + > + clk_bulk_disable_unprepare(priv->num_clks, priv->clks); > + regulator_bulk_disable(priv->num_regs, priv->regs); > + > + if (priv->vbus && priv->mode != PHY_MODE_INVALID) > + qcom_ssphy_vbus_disable(priv->vbus); > + > + qcom_ssphy_vdd_ctrl(priv, DISABLE); > + > + return 0; > +} > + > +static int qcom_ssphy_init_clock(struct ssphy_priv *priv) > +{ > + const char * const clk_id[] = { "ref", "phy", "pipe", }; > + int i; > + > + priv->num_clks = ARRAY_SIZE(clk_id); > + priv->clks = devm_kcalloc(priv->dev, priv->num_clks, > + sizeof(*priv->clks), GFP_KERNEL); You know num_clks is 3, so I would suggest that you just change the sshphy_priv to clks[3] and skip the dynamic allocation. Also, as num_clks always is ARRAY_SIZE(priv->clks) I would suggest using the latter, to make that clear throughout the driver. > + if (!priv->clks) > + return -ENOMEM; > + > + for (i = 0; i < priv->num_clks; i++) > + priv->clks[i].id = clk_id[i]; There's no harm in just writing this on three lines: priv->clks[0].id = "ref"; priv->clks[1].id = "phy"; priv->clks[2].id = "pipe"; > + > + return devm_clk_bulk_get(priv->dev, priv->num_clks, priv->clks); > +} > + > +static int qcom_ssphy_init_regulator(struct ssphy_priv *priv) > +{ > + const char * const reg_supplies[] = { > + [VDD] = "vdd", > + [VDDA1P8] = "vdda1p8", > + }; > + int ret, i; > + > + priv->num_regs = ARRAY_SIZE(reg_supplies); > + priv->regs = devm_kcalloc(priv->dev, priv->num_regs, > + sizeof(*priv->regs), GFP_KERNEL); As with clocks, you know there will only be 2 of these, make it fixed size in ssphy_priv. And as with clocks, I would suggest using ARRAY_SIZE(priv->regs) throughout the driver to make it obvious that's it's a static number. > + if (!priv->regs) > + return -ENOMEM; > + > + for (i = 0; i < priv->num_regs; i++) > + priv->regs[i].supply = reg_supplies[i]; As with clocks, just unroll this and fill in the 2 supplies directly. > + > + ret = devm_regulator_bulk_get(priv->dev, priv->num_regs, priv->regs); > + if (ret) > + return ret; > + > + priv->vbus = devm_regulator_get_optional(priv->dev, "vbus"); get_optional means that if vbus-supply is not found, rather than returning a dummy regulator object this will fail with -ENODEV. Given the rest of the logic related to vbus you should set vbus to NULL if the returned PTR_ERR(value) is -ENODEV, and fail for other errors. Or just drop the _optional, and let your vbus controls operate on the dummy regulator you get back. (Right now vbus can't be NULL, so these checks are not very useful) > + if (IS_ERR(priv->vbus)) > + return PTR_ERR(priv->vbus); > + > + return 0; return PTR_ERR_OR_ZERO(priv->vbus) (Although that might change based on above comment) > +} Regards, Bjorn _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel