linux-kernel - Re: [PATCH v1 04/12] usb: dwc3: dwc3-hisi: Add code for dwc3 of Hisilicon Soc Platform

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 3 Dec 2018 10:12:01 +0200
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     chenyu56@...wei.com
Cc:     USB <linux-usb@...r.kernel.org>,
        devicetree <devicetree@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Suzhuangluan <suzhuangluan@...ilicon.com>,
        Kongfei <kongfei@...ilicon.com>, Felipe Balbi <balbi@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "David S. Miller" <davem@...emloft.net>,
        Mauro Carvalho Chehab <mchehab+samsung@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Arnd Bergmann <arnd@...db.de>,
        John Stultz <john.stultz@...aro.org>,
        Wangbinghui <wangbinghui@...ilicon.com>
Subject: Re: [PATCH v1 04/12] usb: dwc3: dwc3-hisi: Add code for dwc3 of
 Hisilicon Soc Platform

On Mon, Dec 3, 2018 at 5:48 AM Yu Chen <chenyu56@...wei.com> 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@...wei.com>

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