lkml.org 
[lkml]   [2018]   [Dec]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [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@huawei.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@huawei.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

    \
     
     \ /
      Last update: 2018-12-03 09:13    [W:2.187 / U:0.184 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site