Hi, On 2018/12/3 16:12, Andy Shevchenko wrote: > 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. > >> +}; > Thanks a lot for your advice! I will check and fix the patch. Chen Yu
