On Mon, Dec 3, 2018 at 5:48 AM Yu Chen <chenyu56@xxxxxxxxxx> wrote: > > This driver handles the poweron and shutdown of dwc3 core > on Hisilicon Soc Platform. > +config USB_DWC3_HISI > + tristate "HiSilicon Kirin Platforms" > + depends on ((ARCH_HISI && ARM64) || COMPILE_TEST) && OF It would be easy to read if depends on OF would be on a separate line > + default USB_DWC3 > + help > + Support USB2/3 functionality in HiSilicon Kirin platforms. > + Say 'Y' or 'M' if you have one such device. > +++ b/drivers/usb/dwc3/dwc3-hisi.c > @@ -0,0 +1,335 @@ > +// SPDX-License-Identifier: GPL-2.0+ You need to talk to your manager and fix License correspondingly. Currently there is an ambigous reading. > +/** Why /** ? > + * dwc3-hisi.c - Support for dwc3 platform devices on HiSilicon platforms Usually put filename in the file is not the best idea. > + * > + * Copyright (C) 2017-2018 Hilisicon Electronics Co., Ltd. > + * http://www.huawei.com > + * > + * Authors: Yu Chen <chenyu56@xxxxxxxxxx> > + * > + * This program is free software: you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 of > + * the License as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. The idea of SPDX is exactly to remove the above boilerplate text. > + */ > + > +#include <linux/module.h> > +#include <linux/kernel.h> > +#include <linux/slab.h> > +#include <linux/of.h> > +#include <linux/of_platform.h> > +#include <linux/pm_runtime.h> > +#include <linux/clk.h> > +#include <linux/clk-provider.h> > +#include <linux/regmap.h> > +#include <linux/reset.h> > +#include <linux/extcon.h> > +#include <linux/regulator/consumer.h> > +#include <linux/pinctrl/consumer.h> It would be easy to maintain ordered list in the future. > + while (--i >= 0) while (i--) > + while (--i >= 0) while (i--) > + while (--i >= 0) while (i--) > + while (--i >= 0) while (i--) > + while (--i >= 0) > +static int dwc3_hisi_probe(struct platform_device *pdev) > +{ > + struct dwc3_hisi *dwc3_hisi; > + struct device *dev = &pdev->dev; > + struct device_node *np = dev->of_node; > + Redundant blank line > + int ret; > + int i; > +} > +#ifdef CONFIG_PM_SLEEP > +static int dwc3_hisi_suspend(struct device *dev) Don't know the usual practice here, but you can just use __maybe_unused and remove this #ifdef. > +{ > + struct dwc3_hisi *dwc3_hisi = dev_get_drvdata(dev); > + int ret; > + > + dev_info(dev, "%s\n", __func__); Noise > +} > +static int dwc3_hisi_resume(struct device *dev) __maybe_unused ? > +{ > + dev_info(dev, "%s\n", __func__); Noise. > + /* Wait for clock stable */ > + msleep(100); Don't you have any hardware notification that clocks are stable? (Something like PLL locked?) > + > + return 0; > +} > +#endif /* CONFIG_PM_SLEEP */ > +static const struct of_device_id dwc3_hisi_match[] = { > + { .compatible = "hisilicon,hi3660-dwc3" }, > + { /* sentinel */ }, No need comma for terminator line. > +}; -- With Best Regards, Andy Shevchenko
