Re: [PATCH v1 04/12] usb: dwc3: dwc3-hisi: Add code for dwc3 of Hisilicon Soc Platform
From: Andy Shevchenko
Date: Mon Dec 03 2018 - 03:12:15 EST
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