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]
Message-Id: <UP05GQ.XQT4W4AH5E8W1@crapouillou.net>
Date:   Fri, 04 Sep 2020 16:10:42 +0200
From:   Paul Cercueil <paul@...pouillou.net>
To:     周琰杰 <zhouyanjie@...yeetech.com>
Cc:     kishon@...com, vkoul@...nel.org, balbi@...nel.org,
        gregkh@...uxfoundation.org, linux-usb@...r.kernel.org,
        linux-kernel@...r.kernel.org, christophe.jaillet@...adoo.fr,
        aric.pzqi@...enic.com, dongsheng.qiu@...enic.com,
        rick.tyliu@...enic.com, yanfei.li@...enic.com,
        sernia.zhou@...mail.com, zhenwenjin@...il.com
Subject: Re: [PATCH v2 1/1] USB: PHY: JZ4770: Use the generic PHY framework.

Hi Zhou,

Le lun. 31 août 2020 à 21:50, 周琰杰 (Zhou Yanjie) 
<zhouyanjie@...yeetech.com> a écrit :
> Used the generic PHY framework API to create the PHY,
> and move the driver to driver/phy/ingenic.
> 
> Tested-by: 周正 (Zhou Zheng) <sernia.zhou@...mail.com>
> Co-developed-by: 漆鹏振 (Qi Pengzhen) <aric.pzqi@...enic.com>
> Signed-off-by: 漆鹏振 (Qi Pengzhen) <aric.pzqi@...enic.com>
> Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@...yeetech.com>
> ---
> 
> Notes:
>     v1->v2:
>     Fix bug, ".of_match_table = 
> of_match_ptr(ingenic_usb_phy_of_matches)" is wrong
>     and should be replaced with ".of_match_table = 
> ingenic_usb_phy_of_matches".
> 
>  drivers/phy/Kconfig                                |   1 +
>  drivers/phy/Makefile                               |   1 +
>  drivers/phy/ingenic/Kconfig                        |  12 +
>  drivers/phy/ingenic/Makefile                       |   2 +
>  .../phy-jz4770.c => phy/ingenic/phy-ingenic-usb.c} | 256 
> ++++++++++++---------
>  drivers/usb/phy/Kconfig                            |   8 -
>  drivers/usb/phy/Makefile                           |   1 -
>  7 files changed, 165 insertions(+), 116 deletions(-)
>  create mode 100644 drivers/phy/ingenic/Kconfig
>  create mode 100644 drivers/phy/ingenic/Makefile
>  rename drivers/{usb/phy/phy-jz4770.c => 
> phy/ingenic/phy-ingenic-usb.c} (63%)
> 
> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> index de9362c25c07..0534b0fdd057 100644
> --- a/drivers/phy/Kconfig
> +++ b/drivers/phy/Kconfig
> @@ -55,6 +55,7 @@ source "drivers/phy/broadcom/Kconfig"
>  source "drivers/phy/cadence/Kconfig"
>  source "drivers/phy/freescale/Kconfig"
>  source "drivers/phy/hisilicon/Kconfig"
> +source "drivers/phy/ingenic/Kconfig"
>  source "drivers/phy/lantiq/Kconfig"
>  source "drivers/phy/marvell/Kconfig"
>  source "drivers/phy/mediatek/Kconfig"
> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
> index c27408e4daae..ab24f0d20763 100644
> --- a/drivers/phy/Makefile
> +++ b/drivers/phy/Makefile
> @@ -14,6 +14,7 @@ obj-y					+= allwinner/	\
>  					   cadence/	\
>  					   freescale/	\
>  					   hisilicon/	\
> +					   ingenic/	\
>  					   intel/	\
>  					   lantiq/	\
>  					   marvell/	\
> diff --git a/drivers/phy/ingenic/Kconfig b/drivers/phy/ingenic/Kconfig
> new file mode 100644
> index 000000000000..b9581eae89dd
> --- /dev/null
> +++ b/drivers/phy/ingenic/Kconfig
> @@ -0,0 +1,12 @@
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# Phy drivers for Ingenic platforms
> +#
> +config PHY_INGENIC_USB
> +	tristate "Ingenic SoCs USB PHY Driver"
> +	depends on (MACH_INGENIC && MIPS) || COMPILE_TEST

The original driver depends on MIPS || COMPILE_TEST, so you should do 
the same, otherwise you change more than what the patch description 
suggests.

> +	depends on USB_SUPPORT
> +	select GENERIC_PHY
> +	help
> +	  This driver provides USB PHY support for the USB controller found
> +	  on the JZ-series and X-series SoCs from Ingenic.
> diff --git a/drivers/phy/ingenic/Makefile 
> b/drivers/phy/ingenic/Makefile
> new file mode 100644
> index 000000000000..65d5ea00fc9d
> --- /dev/null
> +++ b/drivers/phy/ingenic/Makefile
> @@ -0,0 +1,2 @@
> +# SPDX-License-Identifier: GPL-2.0
> +obj-y		+= phy-ingenic-usb.o
> diff --git a/drivers/usb/phy/phy-jz4770.c 
> b/drivers/phy/ingenic/phy-ingenic-usb.c
> similarity index 63%
> rename from drivers/usb/phy/phy-jz4770.c
> rename to drivers/phy/ingenic/phy-ingenic-usb.c
> index f6d3731581eb..86a95b498785 100644
> --- a/drivers/usb/phy/phy-jz4770.c
> +++ b/drivers/phy/ingenic/phy-ingenic-usb.c
> @@ -7,12 +7,12 @@
>   */
> 
>  #include <linux/clk.h>
> +#include <linux/delay.h>
>  #include <linux/io.h>
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
>  #include <linux/regulator/consumer.h>
> -#include <linux/usb/otg.h>
> -#include <linux/usb/phy.h>
> +#include <linux/phy/phy.h>
> 
>  /* OTGPHY register offsets */
>  #define REG_USBPCR_OFFSET			0x00
> @@ -97,68 +97,49 @@ enum ingenic_usb_phy_version {
>  struct ingenic_soc_info {
>  	enum ingenic_usb_phy_version version;
> 
> -	void (*usb_phy_init)(struct usb_phy *phy);
> +	void (*usb_phy_init)(struct phy *phy);
>  };
> 
> -struct jz4770_phy {
> +struct ingenic_usb_phy {
>  	const struct ingenic_soc_info *soc_info;
> 
> -	struct usb_phy phy;
> -	struct usb_otg otg;
> +	struct phy *phy;
>  	struct device *dev;
>  	void __iomem *base;
>  	struct clk *clk;
>  	struct regulator *vcc_supply;
>  };
> 
> -static inline struct jz4770_phy *otg_to_jz4770_phy(struct usb_otg 
> *otg)
> +static int ingenic_usb_phy_init(struct phy *phy)
>  {
> -	return container_of(otg, struct jz4770_phy, otg);
> -}
> -
> -static inline struct jz4770_phy *phy_to_jz4770_phy(struct usb_phy 
> *phy)
> -{
> -	return container_of(phy, struct jz4770_phy, phy);
> -}
> -
> -static int ingenic_usb_phy_set_peripheral(struct usb_otg *otg,
> -				     struct usb_gadget *gadget)
> -{
> -	struct jz4770_phy *priv = otg_to_jz4770_phy(otg);
> -	u32 reg;
> +	struct ingenic_usb_phy *priv = phy_get_drvdata(phy);
> +	int err;
> 
> -	if (priv->soc_info->version >= ID_X1000) {
> -		reg = readl(priv->base + REG_USBPCR1_OFFSET);
> -		reg |= USBPCR1_BVLD_REG;
> -		writel(reg, priv->base + REG_USBPCR1_OFFSET);
> +	err = clk_prepare_enable(priv->clk);
> +	if (err) {
> +		dev_err(priv->dev, "Unable to start clock: %d\n", err);
> +		return err;
>  	}
> 
> -	reg = readl(priv->base + REG_USBPCR_OFFSET);
> -	reg &= ~USBPCR_USB_MODE;
> -	reg |= USBPCR_VBUSVLDEXT | USBPCR_VBUSVLDEXTSEL | 
> USBPCR_OTG_DISABLE;
> -	writel(reg, priv->base + REG_USBPCR_OFFSET);
> +	priv->soc_info->usb_phy_init(phy);
> 
>  	return 0;
>  }
> 
> -static int ingenic_usb_phy_set_host(struct usb_otg *otg, struct 
> usb_bus *host)
> +static int ingenic_usb_phy_exit(struct phy *phy)
>  {
> -	struct jz4770_phy *priv = otg_to_jz4770_phy(otg);
> -	u32 reg;
> +	struct ingenic_usb_phy *priv = phy_get_drvdata(phy);
> 
> -	reg = readl(priv->base + REG_USBPCR_OFFSET);
> -	reg &= ~(USBPCR_VBUSVLDEXT | USBPCR_VBUSVLDEXTSEL | 
> USBPCR_OTG_DISABLE);
> -	reg |= USBPCR_USB_MODE;
> -	writel(reg, priv->base + REG_USBPCR_OFFSET);
> +	clk_disable_unprepare(priv->clk);
> +	regulator_disable(priv->vcc_supply);
> 
>  	return 0;
>  }
> 
> -static int ingenic_usb_phy_init(struct usb_phy *phy)
> +static int ingenic_usb_phy_power_on(struct phy *phy)
>  {
> -	struct jz4770_phy *priv = phy_to_jz4770_phy(phy);
> +	struct ingenic_usb_phy *priv = phy_get_drvdata(phy);
>  	int err;
> -	u32 reg;
> 
>  	err = regulator_enable(priv->vcc_supply);
>  	if (err) {
> @@ -166,39 +147,71 @@ static int ingenic_usb_phy_init(struct usb_phy 
> *phy)
>  		return err;
>  	}
> 
> -	err = clk_prepare_enable(priv->clk);
> -	if (err) {
> -		dev_err(priv->dev, "Unable to start clock: %d\n", err);
> -		return err;
> -	}
> -
> -	priv->soc_info->usb_phy_init(phy);
> -
> -	/* Wait for PHY to reset */
> -	usleep_range(30, 300);
> -	reg = readl(priv->base + REG_USBPCR_OFFSET);
> -	writel(reg & ~USBPCR_POR, priv->base + REG_USBPCR_OFFSET);
> -	usleep_range(300, 1000);
> -
>  	return 0;
>  }
> 
> -static void ingenic_usb_phy_shutdown(struct usb_phy *phy)
> +static int ingenic_usb_phy_power_off(struct phy *phy)
>  {
> -	struct jz4770_phy *priv = phy_to_jz4770_phy(phy);
> +	struct ingenic_usb_phy *priv = phy_get_drvdata(phy);
> 
> -	clk_disable_unprepare(priv->clk);
>  	regulator_disable(priv->vcc_supply);
> +
> +	return 0;
>  }
> 
> -static void ingenic_usb_phy_remove(void *phy)
> +static int ingenic_usb_phy_set_mode(struct phy *phy,
> +				  enum phy_mode mode, int submode)
>  {
> -	usb_remove_phy(phy);
> +	struct ingenic_usb_phy *priv = phy_get_drvdata(phy);
> +	u32 reg;
> +
> +	switch (mode) {
> +	case PHY_MODE_USB_HOST:
> +		reg = readl(priv->base + REG_USBPCR_OFFSET);
> +		reg &= ~(USBPCR_VBUSVLDEXT | USBPCR_VBUSVLDEXTSEL | 
> USBPCR_OTG_DISABLE);
> +		reg |= USBPCR_USB_MODE;
> +		writel(reg, priv->base + REG_USBPCR_OFFSET);
> +
> +		break;
> +	case PHY_MODE_USB_DEVICE:
> +		if (priv->soc_info->version >= ID_X1000) {
> +			reg = readl(priv->base + REG_USBPCR1_OFFSET);
> +			reg |= USBPCR1_BVLD_REG;
> +			writel(reg, priv->base + REG_USBPCR1_OFFSET);
> +		}
> +
> +		reg = readl(priv->base + REG_USBPCR_OFFSET);
> +		reg &= ~USBPCR_USB_MODE;
> +		reg |= USBPCR_VBUSVLDEXT | USBPCR_VBUSVLDEXTSEL | 
> USBPCR_OTG_DISABLE;
> +		writel(reg, priv->base + REG_USBPCR_OFFSET);
> +
> +		break;
> +	case PHY_MODE_USB_OTG:
> +		reg = readl(priv->base + REG_USBPCR_OFFSET);
> +		reg &= ~USBPCR_OTG_DISABLE;
> +		reg |= USBPCR_VBUSVLDEXT | USBPCR_VBUSVLDEXTSEL | USBPCR_USB_MODE;
> +		writel(reg, priv->base + REG_USBPCR_OFFSET);
> +
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
>  }

I think the diff should be a bit smaller (and easier to review) if you 
move ingenic_usb_phy_init / ingenic_usb_phy_exit here, where they used 
to be.

> 
> -static void jz4770_usb_phy_init(struct usb_phy *phy)
> +static const struct phy_ops ingenic_usb_phy_ops = {
> +	.init		= ingenic_usb_phy_init,
> +	.exit		= ingenic_usb_phy_exit,
> +	.power_on	= ingenic_usb_phy_power_on,
> +	.power_off	= ingenic_usb_phy_power_off,
> +	.set_mode	= ingenic_usb_phy_set_mode,
> +	.owner		= THIS_MODULE,
> +};
> +
> +static void jz4770_usb_phy_init(struct phy *phy)
>  {
> -	struct jz4770_phy *priv = phy_to_jz4770_phy(phy);
> +	struct ingenic_usb_phy *priv = phy_get_drvdata(phy);
>  	u32 reg;
> 
>  	reg = USBPCR_AVLD_REG | USBPCR_COMMONONN | USBPCR_IDPULLUP_ALWAYS |
> @@ -206,11 +219,16 @@ static void jz4770_usb_phy_init(struct usb_phy 
> *phy)
>  		USBPCR_TXFSLSTUNE_DFT | USBPCR_TXRISETUNE_DFT | 
> USBPCR_TXVREFTUNE_DFT |
>  		USBPCR_POR;
>  	writel(reg, priv->base + REG_USBPCR_OFFSET);
> +
> +	/* Wait for PHY to reset */
> +	usleep_range(30, 300);
> +	writel(reg & ~USBPCR_POR, priv->base + REG_USBPCR_OFFSET);
> +	usleep_range(300, 1000);
>  }
> 
> -static void jz4780_usb_phy_init(struct usb_phy *phy)
> +static void jz4780_usb_phy_init(struct phy *phy)
>  {
> -	struct jz4770_phy *priv = phy_to_jz4770_phy(phy);
> +	struct ingenic_usb_phy *priv = phy_get_drvdata(phy);
>  	u32 reg;
> 
>  	reg = readl(priv->base + REG_USBPCR1_OFFSET) | USBPCR1_USB_SEL |
> @@ -219,11 +237,16 @@ static void jz4780_usb_phy_init(struct usb_phy 
> *phy)
> 
>  	reg = USBPCR_TXPREEMPHTUNE | USBPCR_COMMONONN | USBPCR_POR;
>  	writel(reg, priv->base + REG_USBPCR_OFFSET);
> +
> +	/* Wait for PHY to reset */
> +	usleep_range(30, 300);
> +	writel(reg & ~USBPCR_POR, priv->base + REG_USBPCR_OFFSET);
> +	usleep_range(300, 1000);
>  }
> 
> -static void x1000_usb_phy_init(struct usb_phy *phy)
> +static void x1000_usb_phy_init(struct phy *phy)
>  {
> -	struct jz4770_phy *priv = phy_to_jz4770_phy(phy);
> +	struct ingenic_usb_phy *priv = phy_get_drvdata(phy);
>  	u32 reg;
> 
>  	reg = readl(priv->base + REG_USBPCR1_OFFSET) | 
> USBPCR1_WORD_IF_16BIT;
> @@ -233,11 +256,16 @@ static void x1000_usb_phy_init(struct usb_phy 
> *phy)
>  		USBPCR_TXHSXVTUNE_DCR_15MV | USBPCR_TXVREFTUNE_INC_25PPT |
>  		USBPCR_COMMONONN | USBPCR_POR;
>  	writel(reg, priv->base + REG_USBPCR_OFFSET);
> +
> +	/* Wait for PHY to reset */
> +	usleep_range(30, 300);
> +	writel(reg & ~USBPCR_POR, priv->base + REG_USBPCR_OFFSET);
> +	usleep_range(300, 1000);
>  }
> 
> -static void x1830_usb_phy_init(struct usb_phy *phy)
> +static void x1830_usb_phy_init(struct phy *phy)
>  {
> -	struct jz4770_phy *priv = phy_to_jz4770_phy(phy);
> +	struct ingenic_usb_phy *priv = phy_get_drvdata(phy);
>  	u32 reg;
> 
>  	/* rdt */
> @@ -250,6 +278,11 @@ static void x1830_usb_phy_init(struct usb_phy 
> *phy)
>  	reg = USBPCR_IDPULLUP_OTG | USBPCR_VBUSVLDEXT 
> |	USBPCR_TXPREEMPHTUNE |
>  		USBPCR_COMMONONN | USBPCR_POR;
>  	writel(reg, priv->base + REG_USBPCR_OFFSET);
> +
> +	/* Wait for PHY to reset */
> +	usleep_range(30, 300);
> +	writel(reg & ~USBPCR_POR, priv->base + REG_USBPCR_OFFSET);
> +	usleep_range(300, 1000);

Why is that code repeated four times now? The old driver had that in 
ingenic_usb_phy_init().

>  }
> 
>  static const struct ingenic_soc_info jz4770_soc_info = {
> @@ -276,87 +309,96 @@ static const struct ingenic_soc_info 
> x1830_soc_info = {
>  	.usb_phy_init = x1830_usb_phy_init,
>  };
> 
> -static const struct of_device_id ingenic_usb_phy_of_matches[] = {
> -	{ .compatible = "ingenic,jz4770-phy", .data = &jz4770_soc_info },
> -	{ .compatible = "ingenic,jz4780-phy", .data = &jz4780_soc_info },
> -	{ .compatible = "ingenic,x1000-phy", .data = &x1000_soc_info },
> -	{ .compatible = "ingenic,x1830-phy", .data = &x1830_soc_info },
> -	{ /* sentinel */ }
> -};
> -MODULE_DEVICE_TABLE(of, ingenic_usb_phy_of_matches);
> -
> -static int jz4770_phy_probe(struct platform_device *pdev)
> +static int ingenic_usb_phy_probe(struct platform_device *pdev)
>  {
> -	struct device *dev = &pdev->dev;
> -	struct jz4770_phy *priv;
> +	struct ingenic_usb_phy *priv;
> +	struct phy_provider *provider;
>  	int err;
> 
> -	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);

I'd prefer that you keep a local 'dev' variable. Otherwise it only 
makes the diff bigger and it's harder to review.

>  	if (!priv)
>  		return -ENOMEM;
> 
> +	priv->dev = &pdev->dev;
> +
>  	priv->soc_info = device_get_match_data(&pdev->dev);
>  	if (!priv->soc_info) {
>  		dev_err(&pdev->dev, "Error: No device match found\n");
>  		return -ENODEV;
>  	}
> 
> -	platform_set_drvdata(pdev, priv);
> -	priv->dev = dev;
> -	priv->phy.dev = dev;
> -	priv->phy.otg = &priv->otg;
> -	priv->phy.label = "ingenic-usb-phy";
> -	priv->phy.init = ingenic_usb_phy_init;
> -	priv->phy.shutdown = ingenic_usb_phy_shutdown;
> -
> -	priv->otg.state = OTG_STATE_UNDEFINED;
> -	priv->otg.usb_phy = &priv->phy;
> -	priv->otg.set_host = ingenic_usb_phy_set_host;
> -	priv->otg.set_peripheral = ingenic_usb_phy_set_peripheral;
> -
>  	priv->base = devm_platform_ioremap_resource(pdev, 0);
>  	if (IS_ERR(priv->base)) {
> -		dev_err(dev, "Failed to map registers\n");
> +		dev_err(priv->dev, "Failed to map registers\n");
>  		return PTR_ERR(priv->base);
>  	}
> 
> -	priv->clk = devm_clk_get(dev, NULL);
> +	priv->clk = devm_clk_get(priv->dev, NULL);
>  	if (IS_ERR(priv->clk)) {
>  		err = PTR_ERR(priv->clk);
>  		if (err != -EPROBE_DEFER)
> -			dev_err(dev, "Failed to get clock\n");
> +			dev_err(priv->dev, "Failed to get clock\n");
>  		return err;
>  	}
> 
> -	priv->vcc_supply = devm_regulator_get(dev, "vcc");
> +	priv->vcc_supply = devm_regulator_get(priv->dev, "vcc");
>  	if (IS_ERR(priv->vcc_supply)) {
>  		err = PTR_ERR(priv->vcc_supply);
>  		if (err != -EPROBE_DEFER)
> -			dev_err(dev, "Failed to get regulator\n");
> +			dev_err(priv->dev, "Failed to get regulator\n");
>  		return err;
>  	}
> 
> -	err = usb_add_phy(&priv->phy, USB_PHY_TYPE_USB2);
> -	if (err) {
> -		if (err != -EPROBE_DEFER)
> -			dev_err(dev, "Unable to register PHY\n");
> -		return err;
> +	priv->phy = devm_phy_create(priv->dev, NULL, &ingenic_usb_phy_ops);
> +	if (IS_ERR(priv)) {
> +		dev_err(priv->dev, "Failed to create PHY: %ld\n",	PTR_ERR(priv));
> +		return PTR_ERR(priv);
> +	}

There's a stray tabulation character here.

Also, no need to print error codes in the probe function - they will be 
printed anyway since the driver will fail to probe.

> +
> +	provider = devm_of_phy_provider_register(priv->dev, 
> of_phy_simple_xlate);
> +	if (IS_ERR(provider)) {
> +		dev_err(priv->dev, "Failed to register PHY provider: %ld\n", 
> PTR_ERR(provider));
> +		return PTR_ERR(provider);
>  	}

Same here.

> 
> -	return devm_add_action_or_reset(dev, ingenic_usb_phy_remove, 
> &priv->phy);
> +	platform_set_drvdata(pdev, priv);
> +	phy_set_drvdata(priv->phy, priv);

These two do the same thing. Also, you must do it before registering 
the PHY, otherwise you have a race.

> +
> +	return 0;
> +}
> +
> +static int ingenic_usb_phy_remove(struct platform_device *pdev)
> +{
> +	struct ingenic_usb_phy *priv = platform_get_drvdata(pdev);
> +
> +	clk_disable_unprepare(priv->clk);
> +	regulator_disable(priv->vcc_supply);

I assume that ingenic_usb_phy_power_off() and ingenic_usb_phy_exit() 
are automatically called when the module is removed, did you test 
module removal?

> +
> +	return 0;
>  }
> 
> -static struct platform_driver ingenic_phy_driver = {
> -	.probe		= jz4770_phy_probe,
> +static const struct of_device_id ingenic_usb_phy_of_matches[] = {
> +	{ .compatible = "ingenic,jz4770-phy", .data = &jz4770_soc_info },
> +	{ .compatible = "ingenic,jz4780-phy", .data = &jz4780_soc_info },
> +	{ .compatible = "ingenic,x1000-phy", .data = &x1000_soc_info },
> +	{ .compatible = "ingenic,x1830-phy", .data = &x1830_soc_info },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, ingenic_usb_phy_of_matches);

You moved that code around, which only made the diff bigger and harder 
to review. Please keep it where it was.

> +
> +static struct platform_driver ingenic_usb_phy_driver = {
> +	.probe		= ingenic_usb_phy_probe,
> +	.remove		= ingenic_usb_phy_remove,
>  	.driver		= {
> -		.name	= "jz4770-phy",
> -		.of_match_table = of_match_ptr(ingenic_usb_phy_of_matches),
> +		.name	= "ingenic-usb-phy",
> +		.of_match_table = ingenic_usb_phy_of_matches,

You removed of_match_ptr(), which is a valid change (Ingenic SoCs all 
depend on Device Tree), but is unrelated to this patch.

>  	},
>  };
> -module_platform_driver(ingenic_phy_driver);
> +module_platform_driver(ingenic_usb_phy_driver);
> 
>  MODULE_AUTHOR("周琰杰 (Zhou Yanjie) <zhouyanjie@...yeetech.com>");
>  MODULE_AUTHOR("漆鹏振 (Qi Pengzhen) <aric.pzqi@...enic.com>");
>  MODULE_AUTHOR("Paul Cercueil <paul@...pouillou.net>");
>  MODULE_DESCRIPTION("Ingenic SoCs USB PHY driver");
> +MODULE_ALIAS("jz4770_phy");

Actually that would be "jz4770-phy".

Cheers,
-Paul

>  MODULE_LICENSE("GPL");
> diff --git a/drivers/usb/phy/Kconfig b/drivers/usb/phy/Kconfig
> index ef4787cd3d37..ff24fca0a2d9 100644
> --- a/drivers/usb/phy/Kconfig
> +++ b/drivers/usb/phy/Kconfig
> @@ -184,12 +184,4 @@ config USB_ULPI_VIEWPORT
>  	  Provides read/write operations to the ULPI phy register set for
>  	  controllers with a viewport register (e.g. Chipidea/ARC 
> controllers).
> 
> -config JZ4770_PHY
> -	tristate "Ingenic SoCs Transceiver Driver"
> -	depends on MIPS || COMPILE_TEST
> -	select USB_PHY
> -	help
> -	  This driver provides PHY support for the USB controller found
> -	  on the JZ-series and X-series SoCs from Ingenic.
> -
>  endmenu
> diff --git a/drivers/usb/phy/Makefile b/drivers/usb/phy/Makefile
> index b352bdbe8712..df1d99010079 100644
> --- a/drivers/usb/phy/Makefile
> +++ b/drivers/usb/phy/Makefile
> @@ -24,4 +24,3 @@ obj-$(CONFIG_USB_MXS_PHY)		+= phy-mxs-usb.o
>  obj-$(CONFIG_USB_ULPI)			+= phy-ulpi.o
>  obj-$(CONFIG_USB_ULPI_VIEWPORT)		+= phy-ulpi-viewport.o
>  obj-$(CONFIG_KEYSTONE_USB_PHY)		+= phy-keystone.o
> -obj-$(CONFIG_JZ4770_PHY)		+= phy-jz4770.o
> --
> 2.11.0
> 


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ