Re: [PATCH v4 2/3] USB: PHY: JZ4770: Add support for new Ingenic SoCs.

[Zhou Yanjie]

Hi Paul,

å 2020/7/23 äå1:19, Paul Cercueil åé:
> Hi Zhou,
>
> Le mer. 22 juil. 2020 Ã 14:33, åçæ (Zhou Yanjie) 
> <zhouyanjie@xxxxxxxxxxxxxx> a Ãcrit :
> > Add support for probing the phy-jz4770 driver on the JZ4780 SoC,
> > the X1000 SoC and the X1830 SoC from Ingenic.
> >
> > Tested-by: åæ (Zhou Zheng) <sernia.zhou@xxxxxxxxxxx>
> > Co-developed-by: æéæ (Qi Pengzhen) <aric.pzqi@xxxxxxxxxxx>
> > Signed-off-by: æéæ (Qi Pengzhen) <aric.pzqi@xxxxxxxxxxx>
> > Signed-off-by: åçæ (Zhou Yanjie) <zhouyanjie@xxxxxxxxxxxxxx>
> > ---
> >
> > Notes:
> > ÂÂÂ v1->v2:
> > ÂÂÂ Add bindings for the JZ4780 SoC.
> >
> > ÂÂÂ v2->v3:
> > ÂÂÂ Use "of_device_get_match_data" instead "of_match_device"
> > ÂÂÂ to get version information.
> >
> > ÂÂÂ v3->v4:
> > ÂÂÂ Fix typos.
> >
> > Âdrivers/usb/phy/KconfigÂÂÂÂÂ |ÂÂ 4 +-
> > Âdrivers/usb/phy/phy-jz4770.c | 174 
> > ++++++++++++++++++++++++++++++++-----------
> > Â2 files changed, 133 insertions(+), 45 deletions(-)
> >
> > diff --git a/drivers/usb/phy/Kconfig b/drivers/usb/phy/Kconfig
> > index 4b3fa78995cf..775f0dd7b5f5 100644
> > --- a/drivers/usb/phy/Kconfig
> > +++ b/drivers/usb/phy/Kconfig
> > @@ -185,11 +185,11 @@ config USB_ULPI_VIEWPORT
> > ÂÂÂÂÂÂ controllers with a viewport register (e.g. Chipidea/ARC 
> > controllers).
> >
> > Âconfig JZ4770_PHY
> > -ÂÂÂ tristate "Ingenic JZ4770 Transceiver Driver"
> > +ÂÂÂ 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 JZ4770 SoC from Ingenic.
> > +ÂÂÂÂÂ on the JZ4770/JZ4780/X1000/X1830 SoC from Ingenic.
>
> 'JZ-series / X-series SoCs' would be more future-proof :)

Sure.


> > Âendmenu
> > diff --git a/drivers/usb/phy/phy-jz4770.c b/drivers/usb/phy/phy-jz4770.c
> > index 8f62dc2a90ff..cd49b32b4c13 100644
> > --- a/drivers/usb/phy/phy-jz4770.c
> > +++ b/drivers/usb/phy/phy-jz4770.c
> > @@ -1,27 +1,30 @@
> > Â// SPDX-License-Identifier: GPL-2.0
> > Â/*
> > - * Ingenic JZ4770 USB PHY driver
> > + * Ingenic SoCs USB PHY driver
> > Â * Copyright (c) Paul Cercueil <paul@xxxxxxxxxxxxxxx>
> > + * Copyright (c) æéæ (Qi Pengzhen) <aric.pzqi@xxxxxxxxxxx>
> > + * Copyright (c) åçæ (Zhou Yanjie) <zhouyanjie@xxxxxxxxxxxxxx>
> > Â */
> >
> > Â#include <linux/clk.h>
> > Â#include <linux/io.h>
> > Â#include <linux/module.h>
> > +#include <linux/of_device.h>
> > Â#include <linux/platform_device.h>
> > Â#include <linux/regulator/consumer.h>
> > Â#include <linux/usb/otg.h>
> > Â#include <linux/usb/phy.h>
> >
> > +/* OTGPHY register offsets */
> > Â#define REG_USBPCR_OFFSETÂÂÂ 0x00
> > Â#define REG_USBRDT_OFFSETÂÂÂ 0x04
> > Â#define REG_USBVBFIL_OFFSETÂÂÂ 0x08
> > Â#define REG_USBPCR1_OFFSETÂÂÂ 0x0c
> >
> > -/* USBPCR */
> > +/* bits within the USBPCR register */
> > Â#define USBPCR_USB_MODEÂÂÂÂÂÂÂ BIT(31)
> > Â#define USBPCR_AVLD_REGÂÂÂÂÂÂÂ BIT(30)
> > -#define USBPCR_INCRMÂÂÂÂÂÂÂ BIT(27)
> > -#define USBPCR_CLK12_ENÂÂÂÂÂÂÂ BIT(26)
> > +#define USBPCR_INCR_MASKÂÂÂ BIT(27)
>
> Please don't rename macros, it adds a lot of noise to the patch.

Sure.


> > Â#define USBPCR_COMMONONNÂÂÂ BIT(25)
> > Â#define USBPCR_VBUSVLDEXTÂÂÂ BIT(24)
> > Â#define USBPCR_VBUSVLDEXTSELÂÂÂ BIT(23)
> > @@ -32,46 +35,80 @@
> >
> > Â#define USBPCR_IDPULLUP_LSBÂÂÂ 28
> > Â#define USBPCR_IDPULLUP_MASKÂÂÂ GENMASK(29, USBPCR_IDPULLUP_LSB)
> > -#define USBPCR_IDPULLUP_ALWAYSÂÂÂ (3 << USBPCR_IDPULLUP_LSB)
> > -#define USBPCR_IDPULLUP_SUSPENDÂÂÂ (1 << USBPCR_IDPULLUP_LSB)
> > -#define USBPCR_IDPULLUP_OTGÂÂÂ (0 << USBPCR_IDPULLUP_LSB)
> > +#define USBPCR_IDPULLUP_ALWAYSÂÂÂ (0x2 << USBPCR_IDPULLUP_LSB)
> > +#define USBPCR_IDPULLUP_SUSPENDÂÂÂ (0x1 << USBPCR_IDPULLUP_LSB)
> > +#define USBPCR_IDPULLUP_OTGÂÂÂ (0x0 << USBPCR_IDPULLUP_LSB)
>
> a '3' turned into a '2' here...

From the information in the programming manual, it can be 3 or 2.

I think using 2 with 1 and 0 below seems more in line with reading 
habits :)


> > Â#define USBPCR_COMPDISTUNE_LSBÂÂÂ 17
> > Â#define USBPCR_COMPDISTUNE_MASKÂÂÂ GENMASK(19, USBPCR_COMPDISTUNE_LSB)
> > -#define USBPCR_COMPDISTUNE_DFTÂÂÂ 4
> > +#define USBPCR_COMPDISTUNE_DFTÂÂÂ (0x4 << USBPCR_COMPDISTUNE_LSB)
>
> Why? Even if you have a valid reason to do that, it does not belong in 
> this patch.

USBPCR_COMPDISTUNE_DFT (and below) is using in jz4770_phy_init(), to do 
this can simplify jz4770_phy_init(), do I need separate it as a separate 
patch in the next version?


> > Â#define USBPCR_OTGTUNE_LSBÂÂÂ 14
> > Â#define USBPCR_OTGTUNE_MASKÂÂÂ GENMASK(16, USBPCR_OTGTUNE_LSB)
> > -#define USBPCR_OTGTUNE_DFTÂÂÂ 4
> > +#define USBPCR_OTGTUNE_DFTÂÂÂ (0x4 << USBPCR_OTGTUNE_LSB)
> >
> > Â#define USBPCR_SQRXTUNE_LSBÂÂÂ 11
> > Â#define USBPCR_SQRXTUNE_MASKÂÂÂ GENMASK(13, USBPCR_SQRXTUNE_LSB)
> > -#define USBPCR_SQRXTUNE_DFTÂÂÂ 3
> > +#define USBPCR_SQRXTUNE_DCR_20PCTÂÂÂ (0x7 << USBPCR_SQRXTUNE_LSB)
> > +#define USBPCR_SQRXTUNE_DFTÂÂÂ (0x3 << USBPCR_SQRXTUNE_LSB)
> >
> > Â#define USBPCR_TXFSLSTUNE_LSBÂÂÂ 7
> > Â#define USBPCR_TXFSLSTUNE_MASKÂÂÂ GENMASK(10, USBPCR_TXFSLSTUNE_LSB)
> > -#define USBPCR_TXFSLSTUNE_DFTÂÂÂ 3
> > +#define USBPCR_TXFSLSTUNE_DCR_50PPTÂÂÂ (0xf << USBPCR_TXFSLSTUNE_LSB)
> > +#define USBPCR_TXFSLSTUNE_DCR_25PPTÂÂÂ (0x7 << USBPCR_TXFSLSTUNE_LSB)
> > +#define USBPCR_TXFSLSTUNE_DFTÂÂÂ (0x3 << USBPCR_TXFSLSTUNE_LSB)
> > +#define USBPCR_TXFSLSTUNE_INC_25PPTÂÂÂ (0x1 << USBPCR_TXFSLSTUNE_LSB)
> > +#define USBPCR_TXFSLSTUNE_INC_50PPTÂÂÂ (0x0 << USBPCR_TXFSLSTUNE_LSB)
> > +
> > +#define USBPCR_TXHSXVTUNE_LSBÂÂÂ 4
> > +#define USBPCR_TXHSXVTUNE_MASKÂÂÂ GENMASK(5, USBPCR_TXHSXVTUNE_LSB)
> > +#define USBPCR_TXHSXVTUNE_DFTÂÂÂ (0x3 << USBPCR_TXHSXVTUNE_LSB)
> > +#define USBPCR_TXHSXVTUNE_DCR_15MVÂÂÂ (0x1 << USBPCR_TXHSXVTUNE_LSB)
> >
> > Â#define USBPCR_TXRISETUNE_LSBÂÂÂ 4
> > Â#define USBPCR_TXRISETUNE_MASKÂÂÂ GENMASK(5, USBPCR_TXRISETUNE_LSB)
> > -#define USBPCR_TXRISETUNE_DFTÂÂÂ 3
> > +#define USBPCR_TXRISETUNE_DFTÂÂÂ (0x3 << USBPCR_TXRISETUNE_LSB)
> >
> > Â#define USBPCR_TXVREFTUNE_LSBÂÂÂ 0
> > Â#define USBPCR_TXVREFTUNE_MASKÂÂÂ GENMASK(3, USBPCR_TXVREFTUNE_LSB)
> > -#define USBPCR_TXVREFTUNE_DFTÂÂÂ 5
> > +#define USBPCR_TXVREFTUNE_INC_25PPTÂÂÂ (0x7 << USBPCR_TXVREFTUNE_LSB)
> > +#define USBPCR_TXVREFTUNE_DFTÂÂÂ (0x5 << USBPCR_TXVREFTUNE_LSB)
> >
> > -/* USBRDT */
> > +/* bits within the USBRDTR register */
> > +#define USBRDT_UTMI_RSTÂÂÂÂÂÂÂ BIT(27)
> > +#define USBRDT_HB_MASKÂÂÂÂÂÂÂ BIT(26)
> > Â#define USBRDT_VBFIL_LD_ENÂÂÂ BIT(25)
> > Â#define USBRDT_IDDIG_ENÂÂÂÂÂÂÂ BIT(24)
> > Â#define USBRDT_IDDIG_REGÂÂÂ BIT(23)
> > -
> > -#define USBRDT_USBRDT_LSBÂÂÂ 0
> > -#define USBRDT_USBRDT_MASKÂÂÂ GENMASK(22, USBRDT_USBRDT_LSB)
> > -
> > -/* USBPCR1 */
> > -#define USBPCR1_UHC_POWONÂÂÂ BIT(5)
> > +#define USBRDT_VBFIL_ENÂÂÂÂÂÂÂ BIT(2)
> > +
> > +/* bits within the USBPCR1 register */
> > +#define USBPCR1_BVLD_REGÂÂÂÂÂÂÂÂÂÂÂ BIT(31)
> > +#define USBPCR1_DPPDÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ BIT(29)
> > +#define USBPCR1_DMPDÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ BIT(28)
> > +#define USBPCR1_USB_SELÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ BIT(28)
> > +#define USBPCR1_WORD_IF_16BITÂÂÂÂÂÂÂ BIT(19)
> > +
> > +#define USBPCR1_REFCLKSEL_LSBÂÂÂÂÂÂÂ 26
> > +#define USBPCR1_REFCLKSEL_MASKÂÂÂÂÂÂÂ GENMASK(27, 
> > USBPCR1_REFCLKDIV_LSB)
> > +#define USBPCR1_REFCLKSEL_CLKCOREÂÂÂ (0x3 << USBPCR1_REFCLKSEL_LSB)
> > +
> > +#define USBPCR1_REFCLKDIV_LSBÂÂÂÂÂÂÂ 24
> > +#define USBPCR1_REFCLKDIV_MASKÂÂÂÂÂÂÂ GENMASK(25, 
> > USBPCR1_REFCLKDIV_LSB)
> > +#define USBPCR1_REFCLKDIV_48MÂÂÂÂÂÂÂ (0x2 << USBPCR1_REFCLKDIV_LSB)
> > +#define USBPCR1_REFCLKDIV_24MÂÂÂÂÂÂÂ (0x1 << USBPCR1_REFCLKDIV_LSB)
> > +#define USBPCR1_REFCLKDIV_12MÂÂÂÂÂÂÂ (0x0 << USBPCR1_REFCLKDIV_LSB)
> > +
> > +enum ingenic_usb_phy_version {
> > +ÂÂÂ ID_JZ4770,
> > +ÂÂÂ ID_JZ4780,
> > +ÂÂÂ ID_X1000,
> > +ÂÂÂ ID_X1830,
> > +};
> >
> > Âstruct jz4770_phy {
> > +ÂÂÂ enum ingenic_usb_phy_version version;
> > +
> > ÂÂÂÂ struct usb_phy phy;
> > ÂÂÂÂ struct usb_otg otg;
> > ÂÂÂÂ struct device *dev;
> > @@ -96,6 +133,12 @@ static int jz4770_phy_set_peripheral(struct 
> > usb_otg *otg,
> > ÂÂÂÂ struct jz4770_phy *priv = otg_to_jz4770_phy(otg);
> > ÂÂÂÂ u32 reg;
> >
> > +ÂÂÂ if (priv->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;
> > @@ -135,17 +178,59 @@ static int jz4770_phy_init(struct usb_phy *phy)
> > ÂÂÂÂÂÂÂÂ return err;
> > ÂÂÂÂ }
> >
> > -ÂÂÂ reg = USBPCR_AVLD_REG | USBPCR_COMMONONN | USBPCR_IDPULLUP_ALWAYS |
> > -ÂÂÂÂÂÂÂ (USBPCR_COMPDISTUNE_DFT << USBPCR_COMPDISTUNE_LSB) |
> > -ÂÂÂÂÂÂÂ (USBPCR_OTGTUNE_DFT << USBPCR_OTGTUNE_LSB) |
> > -ÂÂÂÂÂÂÂ (USBPCR_SQRXTUNE_DFT << USBPCR_SQRXTUNE_LSB) |
> > -ÂÂÂÂÂÂÂ (USBPCR_TXFSLSTUNE_DFT << USBPCR_TXFSLSTUNE_LSB) |
> > -ÂÂÂÂÂÂÂ (USBPCR_TXRISETUNE_DFT << USBPCR_TXRISETUNE_LSB) |
> > -ÂÂÂÂÂÂÂ (USBPCR_TXVREFTUNE_DFT << USBPCR_TXVREFTUNE_LSB) |
> > -ÂÂÂÂÂÂÂ USBPCR_POR;
> > +ÂÂÂ if (priv->version >= ID_X1830) {
> > +ÂÂÂÂÂÂÂ /* rdt */
> > +ÂÂÂÂÂÂÂ writel(USBRDT_VBFIL_EN | USBRDT_UTMI_RST, priv->base + 
> > REG_USBRDT_OFFSET);
> > +
> > +ÂÂÂÂÂÂÂ reg = readl(priv->base + REG_USBPCR1_OFFSET) | 
> > USBPCR1_WORD_IF_16BIT |
> > +ÂÂÂÂÂÂÂÂÂÂÂ USBPCR1_DMPD | USBPCR1_DPPD;
> > +ÂÂÂÂÂÂÂ writel(reg, priv->base + REG_USBPCR1_OFFSET);
> > +
> > +ÂÂÂÂÂÂÂ reg = USBPCR_IDPULLUP_OTG | USBPCR_VBUSVLDEXT | 
> > USBPCR_TXPREEMPHTUNE;
> > +ÂÂÂ } else if (priv->version >= ID_X1000) {
> > +ÂÂÂÂÂÂÂ reg = readl(priv->base + REG_USBPCR1_OFFSET) | 
> > USBPCR1_WORD_IF_16BIT;
> > +ÂÂÂÂÂÂÂ writel(reg, priv->base + REG_USBPCR1_OFFSET);
> > +
> > +ÂÂÂÂÂÂÂ reg = USBPCR_SQRXTUNE_DCR_20PCT | USBPCR_TXPREEMPHTUNE |
> > +ÂÂÂÂÂÂÂÂÂÂÂ USBPCR_TXHSXVTUNE_DCR_15MV | USBPCR_TXVREFTUNE_INC_25PPT;
> > +ÂÂÂ } else if (priv->version >= ID_JZ4780) {
> > +ÂÂÂÂÂÂÂ reg = readl(priv->base + REG_USBPCR1_OFFSET) | 
> > USBPCR1_USB_SEL |
> > +ÂÂÂÂÂÂÂÂÂÂÂ USBPCR1_WORD_IF_16BIT;
> > +ÂÂÂÂÂÂÂ writel(reg, priv->base + REG_USBPCR1_OFFSET);
> > +
> > +ÂÂÂÂÂÂÂ reg = USBPCR_TXPREEMPHTUNE;
> > +ÂÂÂ } else {
> > +ÂÂÂÂÂÂÂ reg = USBPCR_AVLD_REG | USBPCR_IDPULLUP_ALWAYS |
> > +ÂÂÂÂÂÂÂÂÂÂÂ USBPCR_COMPDISTUNE_DFT | USBPCR_OTGTUNE_DFT |
> > +ÂÂÂÂÂÂÂÂÂÂÂ USBPCR_SQRXTUNE_DFT | USBPCR_TXFSLSTUNE_DFT |
> > +ÂÂÂÂÂÂÂÂÂÂÂ USBPCR_TXRISETUNE_DFT | USBPCR_TXVREFTUNE_DFT;
> > +ÂÂÂ }
> > +
> > +ÂÂÂ reg |= USBPCR_COMMONONN | USBPCR_POR;
> > ÂÂÂÂ writel(reg, priv->base + REG_USBPCR_OFFSET);
> >
> > -ÂÂÂ /* Wait for PHY to reset */
> > +ÂÂÂ /*
> > +ÂÂÂÂ * Power-On Reset(POR)
> > +ÂÂÂÂ * Function:This customer-specific signal resets all test registers
> > +ÂÂÂÂ * and state machines in the USB 2.0 nanoPHY.
> > +ÂÂÂÂ * The POR signal must be asserted for a minimum of 10 Îs.
> > +ÂÂÂÂ * For POR timing information:
> > +ÂÂÂÂ *
> > +ÂÂÂÂ * T0: Power-on reset (POR) is initiated. 0 (reference).
> > +ÂÂÂÂ * T1: T1 indicates when POR can be set to 1âb0. (To provide 
> > examples,
> > +ÂÂÂÂ * values for T2 and T3 are also shown where T1 = T0 + 30 Îs.);
> > +ÂÂÂÂ * In general, T1 must be â T0 + 10 Îs. T0 + 10 Îs â T1.
> > +ÂÂÂÂ * T2: T2 indicates when PHYCLOCK, CLK48MOHCI, and CLK12MOHCI are
> > +ÂÂÂÂ * available at the macro output, based on the USB 2.0 nanoPHY
> > +ÂÂÂÂ * reference clock source.
> > +ÂÂÂÂ * Crystal:
> > +ÂÂÂÂ *ÂÂÂ â When T1 = T0 + 10 Îs:
> > +ÂÂÂÂ *ÂÂÂÂÂ T2 < T1 + 805 Îs = T0 + 815 Îs
> > +ÂÂÂÂ *ÂÂÂ â When T1 = T0 + 30 Îs:
> > +ÂÂÂÂ *ÂÂÂÂÂ T2 < T1 + 805 Îs = T0 + 835 Îs
> > +ÂÂÂÂ * see "Reset and Power-Saving Signals" on page 60 an âPowering Up
> > +ÂÂÂÂ * and Powering Down the USB 2.0 nanoPHYâ on page 73.
> > +ÂÂÂÂ */
>
> I don't think this comment is very interesting. The code already says 
> that the hardware needs at least 30 Âs to reset, all the low-level 
> details are in the programming manual for those who want to learn 
> more. Besides, since you don't actually modify the reset code, why 
> update the comment in this patch?

Okay, I will drop it in the next version.


> > ÂÂÂÂ usleep_range(30, 300);
> > ÂÂÂÂ writel(reg & ~USBPCR_POR, priv->base + REG_USBPCR_OFFSET);
> > ÂÂÂÂ usleep_range(300, 1000);
> > @@ -166,6 +251,15 @@ static void jz4770_phy_remove(void *phy)
> > ÂÂÂÂ usb_remove_phy(phy);
> > Â}
> >
> > +static const struct of_device_id ingenic_usb_phy_of_matches[] = {
> > +ÂÂÂ { .compatible = "ingenic,jz4770-phy", .data = (void *) ID_JZ4770 },
> > +ÂÂÂ { .compatible = "ingenic,jz4780-phy", .data = (void *) ID_JZ4780 },
> > +ÂÂÂ { .compatible = "ingenic,x1000-phy", .data = (void *) ID_X1000 },
> > +ÂÂÂ { .compatible = "ingenic,x1830-phy", .data = (void *) ID_X1830 },
>
> I'm not a fan of having ID_* as platform data. Create 'soc_info' 
> structures, one per supported SoC, and pass them here. Then you can 
> add a 'version' field of type 'enum ingenic_usb_phy_version'. And add 
> a '.phy_init' field as a function pointer, that would be called within 
> jz4770_phy_init(). That would make the jz4770_phy_init() function much 
> cleaner (no if/else/else/else/...).


Sure, I will do it in the next version.


> > +ÂÂÂ { }
> > +};
> > +MODULE_DEVICE_TABLE(of, ingenic_usb_phy_of_matches);
> > +
> > Âstatic int jz4770_phy_probe(struct platform_device *pdev)
> > Â{
> > ÂÂÂÂ struct device *dev = &pdev->dev;
> > @@ -176,11 +270,13 @@ static int jz4770_phy_probe(struct 
> > platform_device *pdev)
> > ÂÂÂÂ if (!priv)
> > ÂÂÂÂÂÂÂÂ return -ENOMEM;
> >
> > +ÂÂÂ priv->version = (enum 
> > ingenic_usb_phy_version)of_device_get_match_data(dev);
> > +
> > ÂÂÂÂ platform_set_drvdata(pdev, priv);
> > ÂÂÂÂ priv->dev = dev;
> > ÂÂÂÂ priv->phy.dev = dev;
> > ÂÂÂÂ priv->phy.otg = &priv->otg;
> > -ÂÂÂ priv->phy.label = "jz4770-phy";
> > +ÂÂÂ priv->phy.label = "ingenic-usb-phy";
> > ÂÂÂÂ priv->phy.init = jz4770_phy_init;
> > ÂÂÂÂ priv->phy.shutdown = jz4770_phy_shutdown;
> >
> > @@ -221,23 +317,15 @@ static int jz4770_phy_probe(struct 
> > platform_device *pdev)
> > ÂÂÂÂ return devm_add_action_or_reset(dev, jz4770_phy_remove, 
> > &priv->phy);
> > Â}
> >
> > -#ifdef CONFIG_OF
> > -static const struct of_device_id jz4770_phy_of_matches[] = {
> > -ÂÂÂ { .compatible = "ingenic,jz4770-phy" },
> > -ÂÂÂ { }
> > -};
> > -MODULE_DEVICE_TABLE(of, jz4770_phy_of_matches);
> > -#endif
> > -
> > -static struct platform_driver jz4770_phy_driver = {
> > +static struct platform_driver ingenic_usb_phy_driver = {
> > ÂÂÂÂ .probeÂÂÂÂÂÂÂ = jz4770_phy_probe,
> > ÂÂÂÂ .driverÂÂÂÂÂÂÂ = {
> > -ÂÂÂÂÂÂÂ .nameÂÂÂ = "jz4770-phy",
> > -ÂÂÂÂÂÂÂ .of_match_table = of_match_ptr(jz4770_phy_of_matches),
> > +ÂÂÂÂÂÂÂ .nameÂÂÂ = "ingenic-usb-phy",
> > +ÂÂÂÂÂÂÂ .of_match_table = of_match_ptr(ingenic_usb_phy_of_matches),
>
> It's preferred not to rename the functions unless you have a good 
> reason, and you definitely shouldn't rename the module. It's OK to 
> have a driver called 'jz4770-phy' even if it supports more SoCs. 
> Renaming it breaks userspace ABI.


Sure.


Thanks and best regards!


> Cheers,
> -Paul
>
> > ÂÂÂÂ },
> > Â};
> > -module_platform_driver(jz4770_phy_driver);
> > +module_platform_driver(ingenic_usb_phy_driver);
> >
> > ÂMODULE_AUTHOR("Paul Cercueil <paul@xxxxxxxxxxxxxxx>");
> > -MODULE_DESCRIPTION("Ingenic JZ4770 USB PHY driver");
> > +MODULE_DESCRIPTION("Ingenic SoCs USB PHY driver");
> > ÂMODULE_LICENSE("GPL");
> > --
> > 2.11.0