Hi,

On 2018/12/3 16:12, Andy Shevchenko wrote:

On Mon, Dec 3, 2018 at 5:48 AM Yu Chen [off-list ref] wrote:

quoted

This driver handles the poweron and shutdown of dwc3 core
on Hisilicon Soc Platform.

quoted

+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

quoted

+       default USB_DWC3
+       help
+         Support USB2/3 functionality in HiSilicon Kirin platforms.
+         Say 'Y' or 'M' if you have one such device.

quoted

+++ 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.

quoted

+/**

Why /** ?

quoted

+ * dwc3-hisi.c - Support for dwc3 platform devices on HiSilicon platforms

Usually put filename in the file is not the best idea.

quoted

+ *
+ * Copyright (C) 2017-2018 Hilisicon Electronics Co., Ltd.
+ *             http://www.huawei.com
+ *
+ * Authors: Yu Chen [off-list ref]

quoted

+ *
+ * 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.

quoted

+ */
+
+#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.

quoted

+                       while (--i >= 0)

while (i--)

quoted

+                       while (--i >= 0)

while (i--)

quoted

+                       while (--i >= 0)

while (i--)

quoted

+                       while (--i >= 0)

while (i--)

quoted

+                       while (--i >= 0)

quoted

+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;

quoted

+

Redundant blank line

quoted

+       int                     ret;
+       int                     i;

quoted

+}

quoted

+#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.

quoted

+{
+       struct dwc3_hisi *dwc3_hisi = dev_get_drvdata(dev);
+       int ret;
+

quoted

+       dev_info(dev, "%s\n", __func__);

Noise

quoted

+}

quoted

+static int dwc3_hisi_resume(struct device *dev)

__maybe_unused ?

quoted

+{

quoted

+       dev_info(dev, "%s\n", __func__);

Noise.

quoted

+       /* Wait for clock stable */
+       msleep(100);

Don't you have any hardware notification that clocks are stable?
(Something like PLL locked?)

quoted

+
+       return 0;
+}
+#endif /* CONFIG_PM_SLEEP */

quoted

+static const struct of_device_id dwc3_hisi_match[] = {
+       { .compatible = "hisilicon,hi3660-dwc3" },
+       { /* sentinel */ },

No need comma for terminator line.

quoted

+};

Thanks a lot for your advice! I will check and fix the patch.

Chen Yu